diff mbox series

[7/7] lib/oe/patch: Use git notes to store the filenames for the patches

Message ID 20240216185956.2606978-7-pkj@axis.com
State Accepted, archived
Commit f5e6183b9557477bef74024a587de0bfcc2b7c0d
Headers show
Series [1/7] devtool: standard: Add some missing whitespace | expand

Commit Message

Peter Kjellerstedt Feb. 16, 2024, 6:59 p.m. UTC 
The old way of keeping track of the filenames for the patches that
correspond to the commits was to add a special comment line to the end
of the commit message, e.g., "%% original patch: <filename>", using a
temporary git hook. This method had some drawbacks, e.g.:

* It caused problems if one wanted to push the commits upstream as the
  comment line had to be manually removed.
* The comment line would end up in patches if someone used git
  format-path rather than devtool finish to generate the patches.
* The comment line could interfere with global Git hooks used to
  validate the format of the Git commit message.
* When regenerating patches with `devtool finish --force-patch-refresh`,
  the process typically resulted in adding empty lines to the end of the
  commit messages in the updated patches.

A better way of keeping track of the patch filenames is to use Git
notes. This way the commit messages remain unaffected, but the
information is still shown when, e.g., doing `git log`. A special Git
notes space, refs/notes/devtool, is used to not intefere with the
default Git notes. It is configured to be shown in, e.g., `git log` and
to survive rewrites (i.e., `git commit --amend` and `git rebase`).

Since there is no longer any need for a temporary Git hook, the code
that manipulated the .git/hooks directory has also been removed. To
avoid potential problems due to global Git hooks, --no-verify was added
to the `git commit` command.

To not cause troubles for those who have done `devtool modify` for a
recipe with the old solution and then do `devtool finish` with the new
solution, the code will fall back to look for the old strings in the
commit message if no Git note can be found.

While not technically motivated like above, the way to keep track of
ignored commits is also changed to use Git notes to avoid having
different methods to store similar information.

Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
 meta/lib/oe/patch.py                    | 105 +++++++++++++++---------
 meta/lib/oeqa/selftest/cases/devtool.py |   9 +-
 scripts/lib/devtool/standard.py         |  15 ++--
 3 files changed, 78 insertions(+), 51 deletions(-)

Comments

Richard Purdie Feb. 17, 2024, 12:34 p.m. UTC | #1 
On Fri, 2024-02-16 at 19:59 +0100, Peter Kjellerstedt wrote:
> The old way of keeping track of the filenames for the patches that
> correspond to the commits was to add a special comment line to the end
> of the commit message, e.g., "%% original patch: <filename>", using a
> temporary git hook. This method had some drawbacks, e.g.:
> 
> * It caused problems if one wanted to push the commits upstream as the
>   comment line had to be manually removed.
> * The comment line would end up in patches if someone used git
>   format-path rather than devtool finish to generate the patches.
> * The comment line could interfere with global Git hooks used to
>   validate the format of the Git commit message.
> * When regenerating patches with `devtool finish --force-patch-refresh`,
>   the process typically resulted in adding empty lines to the end of the
>   commit messages in the updated patches.
> 
> A better way of keeping track of the patch filenames is to use Git
> notes. This way the commit messages remain unaffected, but the
> information is still shown when, e.g., doing `git log`. A special Git
> notes space, refs/notes/devtool, is used to not intefere with the
> default Git notes. It is configured to be shown in, e.g., `git log` and
> to survive rewrites (i.e., `git commit --amend` and `git rebase`).
> 
> Since there is no longer any need for a temporary Git hook, the code
> that manipulated the .git/hooks directory has also been removed. To
> avoid potential problems due to global Git hooks, --no-verify was added
> to the `git commit` command.
> 
> To not cause troubles for those who have done `devtool modify` for a
> recipe with the old solution and then do `devtool finish` with the new
> solution, the code will fall back to look for the old strings in the
> commit message if no Git note can be found.
> 
> While not technically motivated like above, the way to keep track of
> ignored commits is also changed to use Git notes to avoid having
> different methods to store similar information.
> 
> Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> ---
>  meta/lib/oe/patch.py                    | 105 +++++++++++++++---------
>  meta/lib/oeqa/selftest/cases/devtool.py |   9 +-
>  scripts/lib/devtool/standard.py         |  15 ++--
>  3 files changed, 78 insertions(+), 51 deletions(-)

I've tweaked the EXCLUDE_FROM_WORLD issue and I can squash that in but
with that we still saw:

https://autobuilder.yoctoproject.org/typhoon/#/builders/87/builds/6433/steps/14/logs/stdio

which I think is from this patch.

Cheers,

Richard
Peter Kjellerstedt Feb. 18, 2024, 10:10 p.m. UTC | #2 
> -----Original Message-----
> From: Richard Purdie <richard.purdie@linuxfoundation.org>
> Sent: den 17 februari 2024 13:34
> To: Peter Kjellerstedt <peter.kjellerstedt@axis.com>; openembedded-
> core@lists.openembedded.org
> Subject: Re: [OE-core] [PATCH 7/7] lib/oe/patch: Use git notes to store
> the filenames for the patches
> 
> On Fri, 2024-02-16 at 19:59 +0100, Peter Kjellerstedt wrote:
> > The old way of keeping track of the filenames for the patches that
> > correspond to the commits was to add a special comment line to the end
> > of the commit message, e.g., "%% original patch: <filename>", using a
> > temporary git hook. This method had some drawbacks, e.g.:
> >
> > * It caused problems if one wanted to push the commits upstream as the
> >   comment line had to be manually removed.
> > * The comment line would end up in patches if someone used git
> >   format-path rather than devtool finish to generate the patches.
> > * The comment line could interfere with global Git hooks used to
> >   validate the format of the Git commit message.
> > * When regenerating patches with `devtool finish --force-patch-refresh`,
> >   the process typically resulted in adding empty lines to the end of the
> >   commit messages in the updated patches.
> >
> > A better way of keeping track of the patch filenames is to use Git
> > notes. This way the commit messages remain unaffected, but the
> > information is still shown when, e.g., doing `git log`. A special Git
> > notes space, refs/notes/devtool, is used to not intefere with the
> > default Git notes. It is configured to be shown in, e.g., `git log` and
> > to survive rewrites (i.e., `git commit --amend` and `git rebase`).
> >
> > Since there is no longer any need for a temporary Git hook, the code
> > that manipulated the .git/hooks directory has also been removed. To
> > avoid potential problems due to global Git hooks, --no-verify was added
> > to the `git commit` command.
> >
> > To not cause troubles for those who have done `devtool modify` for a
> > recipe with the old solution and then do `devtool finish` with the new
> > solution, the code will fall back to look for the old strings in the
> > commit message if no Git note can be found.
> >
> > While not technically motivated like above, the way to keep track of
> > ignored commits is also changed to use Git notes to avoid having
> > different methods to store similar information.
> >
> > Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
> > ---
> >  meta/lib/oe/patch.py                    | 105 +++++++++++++++---------
> >  meta/lib/oeqa/selftest/cases/devtool.py |   9 +-
> >  scripts/lib/devtool/standard.py         |  15 ++--
> >  3 files changed, 78 insertions(+), 51 deletions(-)
> 
> I've tweaked the EXCLUDE_FROM_WORLD issue and I can squash that in but
> with that we still saw:
> 
> https://autobuilder.yoctoproject.org/typhoon/#/builders/87/builds/6433/steps/14/logs/stdio
> 
> which I think is from this patch.
> 
> Cheers,
> 
> Richard

While I had checked when `git notes` was introduced (1.7.1 for the 
functionality I use), I had not realized that the --fixed-value option 
to `git config` was only introduced in 2.30 (way after the current OE 
minimum requirement of Git 1.8.3.1). Thankfully, the --fixed-value 
option is not really needed in this case and can just be removed.

I have an updated patch series that fixes this, the missing 
EXCLUDE_FROM_WORLD, and a problem related to how git rebase handles 
notes when a commit is fully absorbed during a rebase. The latter 
could trigger when devtool upgrade is used.

I will send the patches once oe-selftest -r devtool is done...

//Peter
diff mbox series

Patch

diff --git a/meta/lib/oe/patch.py b/meta/lib/oe/patch.py
index 3ded5f3601..4e971f8b30 100644
--- a/meta/lib/oe/patch.py
+++ b/meta/lib/oe/patch.py
@@ -294,8 +294,9 @@  class PatchTree(PatchSet):
         self.Pop(all=True)
 
 class GitApplyTree(PatchTree):
-    patch_line_prefix = '%% original patch'
-    ignore_commit_prefix = '%% ignore'
+    notes_ref = "refs/notes/devtool"
+    original_patch = 'original patch'
+    ignore_commit = 'ignore'
 
     def __init__(self, dir, d):
         PatchTree.__init__(self, dir, d)
@@ -452,7 +453,7 @@  class GitApplyTree(PatchTree):
         # Prepare git command
         cmd = ["git"]
         GitApplyTree.gitCommandUserOptions(cmd, commituser, commitemail)
-        cmd += ["commit", "-F", tmpfile]
+        cmd += ["commit", "-F", tmpfile, "--no-verify"]
         # git doesn't like plain email addresses as authors
         if author and '<' in author:
             cmd.append('--author="%s"' % author)
@@ -460,15 +461,49 @@  class GitApplyTree(PatchTree):
             cmd.append('--date="%s"' % date)
         return (tmpfile, cmd)
 
+    @staticmethod
+    def addNote(repo, ref, key, value=None):
+        note = key + (": %s" % value if value else "")
+        notes_ref = GitApplyTree.notes_ref
+        cmd = ["git", "config", "--fixed-value", "notes.displayRef", notes_ref, notes_ref]
+        runcmd(cmd, repo)
+        cmd = ["git", "config", "--fixed-value", "notes.rewriteRef", notes_ref, notes_ref]
+        runcmd(cmd, repo)
+        cmd = ["git", "notes", "--ref", notes_ref, "append", "-m", note, ref]
+        runcmd(cmd, repo)
+
+    @staticmethod
+    def getNotes(repo, ref):
+        import re
+
+        note = None
+        cmd = ["git", "notes", "--ref", GitApplyTree.notes_ref, "show", ref]
+        try:
+            note = runcmd(cmd, repo)
+            prefix = ""
+        except CmdError:
+            cmd = ['git', 'show', '-s', '--format=%B', ref]
+            note = runcmd(cmd, repo)
+            prefix = "%% "
+
+        note_re = re.compile(r'^%s(.*?)(?::\s*(.*))?$' % prefix)
+        notes = dict()
+        for line in note.splitlines():
+            m = note_re.match(line)
+            if m:
+                notes[m.group(1)] = m.group(2)
+
+        return notes
+
     @staticmethod
     def commitIgnored(subject, dir=None, files=None, d=None):
         if files:
             runcmd(['git', 'add'] + files, dir)
-        message = "%s\n\n%s" % (subject, GitApplyTree.ignore_commit_prefix)
         cmd = ["git"]
         GitApplyTree.gitCommandUserOptions(cmd, d=d)
-        cmd += ["commit", "-m", message, "--no-verify"]
+        cmd += ["commit", "-m", subject, "--no-verify"]
         runcmd(cmd, dir)
+        GitApplyTree.addNote(dir, "HEAD", GitApplyTree.ignore_commit)
 
     @staticmethod
     def extractPatches(tree, startcommits, outdir, paths=None):
@@ -484,18 +519,20 @@  class GitApplyTree(PatchTree):
                 out = runcmd(["sh", "-c", " ".join(shellcmd)], os.path.join(tree, name))
                 if out:
                     for srcfile in out.split():
-                        outfile = os.path.basename(srcfile)
+                        # This loop, which is used to remove any line that
+                        # starts with "%% original patch", is kept for backwards
+                        # compatibility. If/when that compatibility is dropped,
+                        # it can be replaced with code to just read the first
+                        # line of the patch file to get the SHA-1, and the code
+                        # below that writes the modified patch file can be
+                        # replaced with a simple file move.
                         for encoding in ['utf-8', 'latin-1']:
                             patchlines = []
                             try:
                                 with open(srcfile, 'r', encoding=encoding, newline='') as f:
                                     for line in f:
-                                        if line.startswith(GitApplyTree.patch_line_prefix):
-                                            outfile = line.split()[-1].strip()
+                                        if line.startswith("%% " + GitApplyTree.original_patch):
                                             continue
-                                        if line.startswith(GitApplyTree.ignore_commit_prefix):
-                                            outfile = None
-                                            break
                                         patchlines.append(line)
                             except UnicodeDecodeError:
                                 continue
@@ -503,11 +540,16 @@  class GitApplyTree(PatchTree):
                         else:
                             raise PatchError('Unable to find a character encoding to decode %s' % srcfile)
 
-                        if outfile:
-                            bb.utils.mkdirhier(os.path.join(outdir, name))
-                            with open(os.path.join(outdir, name, outfile), 'w') as of:
-                                for line in patchlines:
-                                    of.write(line)
+                        sha1 = patchlines[0].split()[1]
+                        notes = GitApplyTree.getNotes(os.path.join(tree, name), sha1)
+                        if GitApplyTree.ignore_commit in notes:
+                            continue
+                        outfile = notes.get(GitApplyTree.original_patch, os.path.basename(srcfile))
+
+                        bb.utils.mkdirhier(os.path.join(outdir, name))
+                        with open(os.path.join(outdir, name, outfile), 'w') as of:
+                            for line in patchlines:
+                                of.write(line)
         finally:
             shutil.rmtree(tempdir)
 
@@ -555,28 +597,11 @@  class GitApplyTree(PatchTree):
 
             return runcmd(["sh", "-c", " ".join(shellcmd)], self.dir)
 
-        # Add hooks which add a pointer to the original patch file name in the commit message
         reporoot = (runcmd("git rev-parse --show-toplevel".split(), self.dir) or '').strip()
         if not reporoot:
             raise Exception("Cannot get repository root for directory %s" % self.dir)
-        gitdir = (runcmd("git rev-parse --absolute-git-dir".split(), self.dir) or '').strip()
-        if not gitdir:
-            raise Exception("Cannot get gitdir for directory %s" % self.dir)
-        hooks_dir = os.path.join(gitdir, 'hooks')
-        hooks_dir_backup = hooks_dir + '.devtool-orig'
-        if os.path.lexists(hooks_dir_backup):
-            raise Exception("Git hooks backup directory already exists: %s" % hooks_dir_backup)
-        if os.path.lexists(hooks_dir):
-            shutil.move(hooks_dir, hooks_dir_backup)
-        os.mkdir(hooks_dir)
-        commithook = os.path.join(hooks_dir, 'commit-msg')
-        applyhook = os.path.join(hooks_dir, 'applypatch-msg')
-        with open(commithook, 'w') as f:
-            # NOTE: the formatting here is significant; if you change it you'll also need to
-            # change other places which read it back
-            f.write('echo "\n%s: $PATCHFILE" >> $1' % GitApplyTree.patch_line_prefix)
-        os.chmod(commithook, 0o755)
-        shutil.copy2(commithook, applyhook)
+
+        patch_applied = True
         try:
             patchfilevar = 'PATCHFILE="%s"' % os.path.basename(patch['file'])
             if self._need_dirty_check():
@@ -587,7 +612,7 @@  class GitApplyTree(PatchTree):
                     pass
                 else:
                     if output:
-                        # The tree is dirty, not need to try to apply patches with git anymore
+                        # The tree is dirty, no need to try to apply patches with git anymore
                         # since they fail, fallback directly to patch
                         output = PatchTree._applypatch(self, patch, force, reverse, run)
                         output += self._commitpatch(patch, patchfilevar)
@@ -620,10 +645,12 @@  class GitApplyTree(PatchTree):
                     output = PatchTree._applypatch(self, patch, force, reverse, run)
                 output += self._commitpatch(patch, patchfilevar)
                 return output
+        except:
+            patch_applied = False
+            raise
         finally:
-            shutil.rmtree(hooks_dir)
-            if os.path.lexists(hooks_dir_backup):
-                shutil.move(hooks_dir_backup, hooks_dir)
+            if patch_applied:
+                GitApplyTree.addNote(self.dir, "HEAD", GitApplyTree.original_patch, os.path.basename(patch['file']))
 
 
 class QuiltTree(PatchSet):
diff --git a/meta/lib/oeqa/selftest/cases/devtool.py b/meta/lib/oeqa/selftest/cases/devtool.py
index abc85c90a7..a94f4a7148 100644
--- a/meta/lib/oeqa/selftest/cases/devtool.py
+++ b/meta/lib/oeqa/selftest/cases/devtool.py
@@ -988,9 +988,10 @@  class DevtoolModifyTests(DevtoolBase):
         self.assertIn(tempdir, result.output)
         # Check git repo
         self._check_src_repo(tempdir)
-        # Check that the patch is correctly applied
-        # last commit message in the tree must contain
-        # %% original patch: <patchname>
+        # Check that the patch is correctly applied.
+        # The last commit message in the tree must contain the following note:
+        # Notes (devtool):
+        #     original patch: <patchname>
         # ..
         patchname = None
         for uri in src_uri:
@@ -998,7 +999,7 @@  class DevtoolModifyTests(DevtoolBase):
                 patchname = uri.replace("file://", "").partition('.patch')[0] + '.patch'
         self.assertIsNotNone(patchname)
         result = runCmd('git -C %s log -1' % tempdir)
-        self.assertIn("%%%% original patch: %s" % patchname, result.output)
+        self.assertIn("Notes (devtool):\n    original patch: %s" % patchname, result.output)
 
         # Configure the recipe to check that the git dependencies are correctly patched in cargo config
         bitbake('-c configure %s' % testrecipe)
diff --git a/scripts/lib/devtool/standard.py b/scripts/lib/devtool/standard.py
index 6d7fd17fbd..7972b4f822 100644
--- a/scripts/lib/devtool/standard.py
+++ b/scripts/lib/devtool/standard.py
@@ -937,14 +937,13 @@  def modify(args, config, basepath, workspace):
             seen_patches = []
             for branch in branches:
                 branch_patches[branch] = []
-                (stdout, _) = bb.process.run('git log devtool-base..%s' % branch, cwd=srctree)
-                for line in stdout.splitlines():
-                    line = line.strip()
-                    if line.startswith(oe.patch.GitApplyTree.patch_line_prefix):
-                        origpatch = line[len(oe.patch.GitApplyTree.patch_line_prefix):].split(':', 1)[-1].strip()
-                        if not origpatch in seen_patches:
-                            seen_patches.append(origpatch)
-                            branch_patches[branch].append(origpatch)
+                (stdout, _) = bb.process.run('git rev-list devtool-base..%s' % branch, cwd=srctree)
+                for sha1 in stdout.splitlines():
+                    notes = oe.patch.GitApplyTree.getNotes(srctree, sha1.strip())
+                    origpatch = notes.get(oe.patch.GitApplyTree.original_patch)
+                    if origpatch and origpatch not in seen_patches:
+                        seen_patches.append(origpatch)
+                        branch_patches[branch].append(origpatch)
 
         # Need to grab this here in case the source is within a subdirectory
         srctreebase = srctree