| Message ID | 20201026010442.1606893-22-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | patman: Collect review tags and comments from Patchwork | expand |
On 10/25/20 9:04 PM, Simon Glass wrote: > At present it is possible to put sign-off tags before the change log. This > works but then it is hard for patman to add its own tags to a commit. Also > if the commit has a Change-Id (e.g. for Gerrit) the commit becomes invalid > if there is anything after it. NAK. I put SOBs before changelogs in all my series. Doing it like this makes the pre-patman commit more-closely match what is sent to the mailing list, and what will eventually be committed. What is most important is the summary, the description, who has signed off, and lastly any changelogs which will be removed by git am. I also like to put SOBs on top because it groups all the commit-specific information before any patman-specific tags. The last commit (the HEAD) usually has several tags (Series-to/cc, Series-process-log, Cover-letter, etc.) which are unrelated to the commit itself. The tool should do what is best for us humans, not what is convenient for the tool. --Sean > > Report a warning if patman tags are in the wrong place. > > One test patch already has the sign-off in the wrong place. Update the > second one to avoid warnings. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > (no changes since v1) > > tools/patman/README | 3 ++- > tools/patman/func_test.py | 23 ++++++++++++++++++- > tools/patman/patchstream.py | 22 ++++++++++++++++++ > .../0001-pci-Correct-cast-for-sandbox.patch | 2 +- > ...-for-sandbox-in-fdtdec_setup_mem_siz.patch | 2 +- > tools/patman/test/test01.txt | 4 ++-- > 6 files changed, 50 insertions(+), 6 deletions(-) > > diff --git a/tools/patman/README b/tools/patman/README > index 6664027ed7d..54036e1de4a 100644 > --- a/tools/patman/README > +++ b/tools/patman/README > @@ -161,7 +161,8 @@ How to add tags > =============== > > To make this script useful you must add tags like the following into any > -commit. Most can only appear once in the whole series. > +commit. Most can only appear once in the whole series. Tags should be placed > +before the sign-off line / Change-Id. > > Series-to: email / alias > Email address / alias to send patch series to (you can add this > diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py > index b39e3f671dc..5732c674817 100644 > --- a/tools/patman/func_test.py > +++ b/tools/patman/func_test.py > @@ -194,6 +194,9 @@ class TestFunctional(unittest.TestCase): > 'fred': [self.fred], > } > > + # NOTE: If you change this file you must also change the patch files in > + # the same directory, since we assume that the metadata file matches the > + # patched. If it doesn't, this test will fail. > text = self._get_text('test01.txt') > series = patchstream.get_metadata_for_test(text) > cover_fname, args = self._create_patches_for_test(series) > @@ -213,6 +216,10 @@ class TestFunctional(unittest.TestCase): > os.remove(cc_file) > > lines = iter(out[0].getvalue().splitlines()) > + self.assertIn('1 warnings for', next(lines)) > + self.assertEqual( > + "\t Tag 'Commit-notes' should be before sign-off / Change-Id", > + next(lines)) > self.assertEqual('Cleaned %s patches' % len(series.commits), > next(lines)) > self.assertEqual('Change log missing for v2', next(lines)) > @@ -223,7 +230,7 @@ class TestFunctional(unittest.TestCase): > self.assertEqual('', next(lines)) > self.assertIn('Send a total of %d patches' % count, next(lines)) > prev = next(lines) > - for i, commit in enumerate(series.commits): > + for i in range(len(series.commits)): > self.assertEqual(' %s' % args[i], prev) > while True: > prev = next(lines) > @@ -588,3 +595,17 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c > self.assertEqual( > ["Found possible blank line(s) at end of file 'lib/fdtdec.c'"], > pstrm.commit.warn) > + > + def testTagsAfterSignoff(self): > + """Test detection of tags after the signoff""" > + text = '''This is a patch > + > +Signed-off-by: Terminator 2 > +Series-changes: 2 > +- A change > + > +''' > + pstrm = PatchStream.process_text(text) > + self.assertEqual( > + ["Tag 'Series-changes' should be before sign-off / Change-Id"], > + pstrm.commit.warn) > diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py > index 1cb4d6ed0dd..75b074a0185 100644 > --- a/tools/patman/patchstream.py > +++ b/tools/patman/patchstream.py > @@ -81,6 +81,8 @@ class PatchStream: > self.blank_count = 0 # Number of blank lines stored up > self.state = STATE_MSG_HEADER # What state are we in? > self.commit = None # Current commit > + self.saw_signoff = False # Found sign-off line for this commit > + self.saw_change_id = False # Found Change-Id for this commit > > @staticmethod > def process_text(text, is_comment=False): > @@ -176,6 +178,9 @@ class PatchStream: > self.skip_blank = True > self.section = [] > > + self.saw_signoff = False > + self.saw_change_id = False > + > def _parse_version(self, value, line): > """Parse a version from a *-changes tag > > @@ -343,6 +348,10 @@ class PatchStream: > elif cover_match: > name = cover_match.group(1) > value = cover_match.group(2) > + if self.saw_signoff or self.saw_change_id: > + self._add_warn( > + "Tag 'Cover-%s' should be before sign-off / Change-Id" % > + name) > if name == 'letter': > self.in_section = 'cover' > self.skip_blank = False > @@ -374,6 +383,10 @@ class PatchStream: > elif series_tag_match: > name = series_tag_match.group(1) > value = series_tag_match.group(2) > + if self.saw_signoff or self.saw_change_id: > + self._add_warn( > + "Tag 'Series-%s' should be before sign-off / Change-Id" % > + name) > if name == 'changes': > # value is the version number: e.g. 1, or 2 > self.in_change = 'Series' > @@ -385,6 +398,7 @@ class PatchStream: > # Detect Change-Id tags > elif change_id_match: > value = change_id_match.group(1) > + self.saw_change_id = True > if self.is_log: > if self.commit.change_id: > raise ValueError( > @@ -397,6 +411,10 @@ class PatchStream: > elif commit_tag_match: > name = commit_tag_match.group(1) > value = commit_tag_match.group(2) > + if self.saw_signoff or self.saw_change_id: > + self._add_warn( > + "Tag 'Commit-%s' should be before sign-off / Change-Id" % > + name) > if name == 'notes': > self._add_to_commit(name) > self.skip_blank = True > @@ -427,6 +445,7 @@ class PatchStream: > > # Suppress duplicate signoffs > elif signoff_match: > + self.saw_signoff = True > if (self.is_log or not self.commit or > self.commit.CheckDuplicateSignoff(signoff_match.group(1))): > out = [line] > @@ -527,6 +546,8 @@ class PatchStream: > re_fname = re.compile('diff --git a/(.*) b/.*') > > self._write_message_id(outfd) > + self.saw_signoff = False > + self.saw_change_id = False > > while True: > line = infd.readline() > @@ -637,6 +658,7 @@ def fix_patch(backup_dir, fname, series, cmt): > infd = open(fname, 'r', encoding='utf-8') > pst = PatchStream(series) > pst.commit = cmt > + cmt.warn = [] > pst.process_stream(infd, outfd) > infd.close() > outfd.close() > diff --git a/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch b/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch > index 038943c2c9b..1efe0593fd3 100644 > --- a/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch > +++ b/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch > @@ -14,7 +14,6 @@ cmd/pci.c:152:11: warning: format ‘%llx’ expects argument of type > > Fix it with a cast. > > -Signed-off-by: Simon Glass <sjg@chromium.org> > Commit-changes: 2 > - Changes only for this commit > > @@ -24,6 +23,7 @@ about some things > from the first commit > END > > +Signed-off-by: Simon Glass <sjg@chromium.org> > Commit-notes: > Some notes about > the first commit > diff --git a/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch b/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch > index 56278a6ce9b..616ed4abd86 100644 > --- a/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch > +++ b/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch > @@ -14,7 +14,6 @@ lib/fdtdec.c:1203:8: warning: format ‘%llx’ expects argument of type > > Fix it with a cast. > > -Signed-off-by: Simon Glass <sjg@chromium.org> > Series-to: u-boot > Series-prefix: RFC > Series-cc: Stefan Brüns <stefan.bruens@rwth-aachen.de> > @@ -40,6 +39,7 @@ This is a test of how the cover > letter > works > END > +Signed-off-by: Simon Glass <sjg@chromium.org> > --- > fs/fat/fat.c | 1 + > lib/efi_loader/efi_memory.c | 1 + > diff --git a/tools/patman/test/test01.txt b/tools/patman/test/test01.txt > index b238a8b4bab..6ef8faf66b8 100644 > --- a/tools/patman/test/test01.txt > +++ b/tools/patman/test/test01.txt > @@ -12,7 +12,6 @@ Date: Sat Apr 15 15:39:08 2017 -0600 > > Fix it with a cast. > > - Signed-off-by: Simon Glass <sjg@chromium.org> > Commit-changes: 2 > - second revision change > > @@ -22,6 +21,7 @@ Date: Sat Apr 15 15:39:08 2017 -0600 > from the first commit > END > > + Signed-off-by: Simon Glass <sjg@chromium.org> > Commit-notes: > Some notes about > the first commit > @@ -41,7 +41,6 @@ Date: Sat Apr 15 15:39:08 2017 -0600 > > Fix it with a cast. > > - Signed-off-by: Simon Glass <sjg@chromium.org> > Series-to: u-boot > Series-prefix: RFC > Series-cc: Stefan Brüns <stefan.bruens@rwth-aachen.de> > @@ -67,3 +66,4 @@ Date: Sat Apr 15 15:39:08 2017 -0600 > letter > works > END > + Signed-off-by: Simon Glass <sjg@chromium.org> >
Hi Sean, On Sun, 25 Oct 2020 at 19:23, Sean Anderson <seanga2@gmail.com> wrote: > > On 10/25/20 9:04 PM, Simon Glass wrote: > > At present it is possible to put sign-off tags before the change log. This > > works but then it is hard for patman to add its own tags to a commit. Also > > if the commit has a Change-Id (e.g. for Gerrit) the commit becomes invalid > > if there is anything after it. > > NAK. I put SOBs before changelogs in all my series. Doing it like this > makes the pre-patman commit more-closely match what is sent to the > mailing list, and what will eventually be committed. What is most > important is the summary, the description, who has signed off, and > lastly any changelogs which will be removed by git am. > > I also like to put SOBs on top because it groups all the commit-specific > information before any patman-specific tags. The last commit (the HEAD) > usually has several tags (Series-to/cc, Series-process-log, > Cover-letter, etc.) which are unrelated to the commit itself. > > The tool should do what is best for us humans, not what is convenient > for the tool. I suppose it can be done. But we have the same problem with Change-Id. Do you use gerit? Regards, SImon
Simon Glass wrote: > On Sun, 25 Oct 2020 at 19:23, Sean Anderson <seanga2@gmail.com> wrote: >> I also like to put SOBs on top because it groups all the commit-specific >> information before any patman-specific tags. The last commit (the HEAD) >> usually has several tags (Series-to/cc, Series-process-log, >> Cover-letter, etc.) which are unrelated to the commit itself. >> >> The tool should do what is best for us humans, not what is convenient >> for the tool. > > I suppose it can be done. But we have the same problem with Change-Id. > Do you use gerit? I assume this is mostly about Commit-Notes, when used without following the rfc-style format that has subsequent lines indented. If so, then this issue isn't specific to Gerrit: it applies when using Git directly (the "git interpret-trailers" command, "git commit --amend --signoff", and so on). As long as you're following rfc822 syntax, all is fine, so in some sense the issue here is the Commit-Notes tag. Should it go in the commit message body instead of the trailer paragraph? Thanks, Jonathan
On 10/27/20 2:05 AM, Jonathan Nieder wrote: > Simon Glass wrote: >> On Sun, 25 Oct 2020 at 19:23, Sean Anderson <seanga2@gmail.com> wrote: > >>> I also like to put SOBs on top because it groups all the commit-specific >>> information before any patman-specific tags. The last commit (the HEAD) >>> usually has several tags (Series-to/cc, Series-process-log, >>> Cover-letter, etc.) which are unrelated to the commit itself. >>> >>> The tool should do what is best for us humans, not what is convenient >>> for the tool. >> >> I suppose it can be done. But we have the same problem with Change-Id. >> Do you use gerit? I have occasionally used gerrit, but not on any projects where I also would like to use patman. I don't care about the Change-Id requirement. Using the following commit as an example, > doc: Update logging documentation > > This updates logging documentation with some examples of the new commands > added in the previous commits. It also removes some items from the to-do > list which have been implemented. > to confirm, you would like to force the next two lines to be placed last? > Signed-off-by: Sean Anderson <seanga2@gmail.com> > Reviewed-by: Simon Glass <sjg@chromium.org> I like them in their current location, because everything above this line will be present in the final commit as it is applied. > Series-changes: 2 > - Clarify wording of filter documentation > - Reorganize log documentation; related sections should now be more proximate > - Include enum definitions instead of re-documenting them > - Add a few informational commands > > Series-changes: 3 > - Fix heading level of Filters section > - Remove a few more already-implemented features from the TODO list > Everything above this line will be present in the patch as it is emailed. > Series-version: 3 > Series-process-log: sort > Series-to: u-boot > Series-cc: sjg > Series-cc: trini > Series-cc: xypron > Cover-letter: > log: Add commands for manipulating filters > This series adds several commands for adding, listing, and removing log filters. > It also adds getopt, since the filter-add command needs to have several > optional arguments to be complete, and positional specification of those > arguments would have been difficult. > END > I assume this is mostly about Commit-Notes, when used without > following the rfc-style format that has subsequent lines indented. Can you elaborate a bit on what you're referring to? > > If so, then this issue isn't specific to Gerrit: it applies when using > Git directly (the "git interpret-trailers" command, "git commit > --amend --signoff", and so on). > > As long as you're following rfc822 syntax, all is fine, so in some > sense the issue here is the Commit-Notes tag. Should it go in the > commit message body instead of the trailer paragraph? > > Thanks, > Jonathan --Sean
diff --git a/tools/patman/README b/tools/patman/README index 6664027ed7d..54036e1de4a 100644 --- a/tools/patman/README +++ b/tools/patman/README @@ -161,7 +161,8 @@ How to add tags =============== To make this script useful you must add tags like the following into any -commit. Most can only appear once in the whole series. +commit. Most can only appear once in the whole series. Tags should be placed +before the sign-off line / Change-Id. Series-to: email / alias Email address / alias to send patch series to (you can add this diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py index b39e3f671dc..5732c674817 100644 --- a/tools/patman/func_test.py +++ b/tools/patman/func_test.py @@ -194,6 +194,9 @@ class TestFunctional(unittest.TestCase): 'fred': [self.fred], } + # NOTE: If you change this file you must also change the patch files in + # the same directory, since we assume that the metadata file matches the + # patched. If it doesn't, this test will fail. text = self._get_text('test01.txt') series = patchstream.get_metadata_for_test(text) cover_fname, args = self._create_patches_for_test(series) @@ -213,6 +216,10 @@ class TestFunctional(unittest.TestCase): os.remove(cc_file) lines = iter(out[0].getvalue().splitlines()) + self.assertIn('1 warnings for', next(lines)) + self.assertEqual( + "\t Tag 'Commit-notes' should be before sign-off / Change-Id", + next(lines)) self.assertEqual('Cleaned %s patches' % len(series.commits), next(lines)) self.assertEqual('Change log missing for v2', next(lines)) @@ -223,7 +230,7 @@ class TestFunctional(unittest.TestCase): self.assertEqual('', next(lines)) self.assertIn('Send a total of %d patches' % count, next(lines)) prev = next(lines) - for i, commit in enumerate(series.commits): + for i in range(len(series.commits)): self.assertEqual(' %s' % args[i], prev) while True: prev = next(lines) @@ -588,3 +595,17 @@ diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c self.assertEqual( ["Found possible blank line(s) at end of file 'lib/fdtdec.c'"], pstrm.commit.warn) + + def testTagsAfterSignoff(self): + """Test detection of tags after the signoff""" + text = '''This is a patch + +Signed-off-by: Terminator 2 +Series-changes: 2 +- A change + +''' + pstrm = PatchStream.process_text(text) + self.assertEqual( + ["Tag 'Series-changes' should be before sign-off / Change-Id"], + pstrm.commit.warn) diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py index 1cb4d6ed0dd..75b074a0185 100644 --- a/tools/patman/patchstream.py +++ b/tools/patman/patchstream.py @@ -81,6 +81,8 @@ class PatchStream: self.blank_count = 0 # Number of blank lines stored up self.state = STATE_MSG_HEADER # What state are we in? self.commit = None # Current commit + self.saw_signoff = False # Found sign-off line for this commit + self.saw_change_id = False # Found Change-Id for this commit @staticmethod def process_text(text, is_comment=False): @@ -176,6 +178,9 @@ class PatchStream: self.skip_blank = True self.section = [] + self.saw_signoff = False + self.saw_change_id = False + def _parse_version(self, value, line): """Parse a version from a *-changes tag @@ -343,6 +348,10 @@ class PatchStream: elif cover_match: name = cover_match.group(1) value = cover_match.group(2) + if self.saw_signoff or self.saw_change_id: + self._add_warn( + "Tag 'Cover-%s' should be before sign-off / Change-Id" % + name) if name == 'letter': self.in_section = 'cover' self.skip_blank = False @@ -374,6 +383,10 @@ class PatchStream: elif series_tag_match: name = series_tag_match.group(1) value = series_tag_match.group(2) + if self.saw_signoff or self.saw_change_id: + self._add_warn( + "Tag 'Series-%s' should be before sign-off / Change-Id" % + name) if name == 'changes': # value is the version number: e.g. 1, or 2 self.in_change = 'Series' @@ -385,6 +398,7 @@ class PatchStream: # Detect Change-Id tags elif change_id_match: value = change_id_match.group(1) + self.saw_change_id = True if self.is_log: if self.commit.change_id: raise ValueError( @@ -397,6 +411,10 @@ class PatchStream: elif commit_tag_match: name = commit_tag_match.group(1) value = commit_tag_match.group(2) + if self.saw_signoff or self.saw_change_id: + self._add_warn( + "Tag 'Commit-%s' should be before sign-off / Change-Id" % + name) if name == 'notes': self._add_to_commit(name) self.skip_blank = True @@ -427,6 +445,7 @@ class PatchStream: # Suppress duplicate signoffs elif signoff_match: + self.saw_signoff = True if (self.is_log or not self.commit or self.commit.CheckDuplicateSignoff(signoff_match.group(1))): out = [line] @@ -527,6 +546,8 @@ class PatchStream: re_fname = re.compile('diff --git a/(.*) b/.*') self._write_message_id(outfd) + self.saw_signoff = False + self.saw_change_id = False while True: line = infd.readline() @@ -637,6 +658,7 @@ def fix_patch(backup_dir, fname, series, cmt): infd = open(fname, 'r', encoding='utf-8') pst = PatchStream(series) pst.commit = cmt + cmt.warn = [] pst.process_stream(infd, outfd) infd.close() outfd.close() diff --git a/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch b/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch index 038943c2c9b..1efe0593fd3 100644 --- a/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch +++ b/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch @@ -14,7 +14,6 @@ cmd/pci.c:152:11: warning: format ‘%llx’ expects argument of type Fix it with a cast. -Signed-off-by: Simon Glass <sjg@chromium.org> Commit-changes: 2 - Changes only for this commit @@ -24,6 +23,7 @@ about some things from the first commit END +Signed-off-by: Simon Glass <sjg@chromium.org> Commit-notes: Some notes about the first commit diff --git a/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch b/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch index 56278a6ce9b..616ed4abd86 100644 --- a/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch +++ b/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch @@ -14,7 +14,6 @@ lib/fdtdec.c:1203:8: warning: format ‘%llx’ expects argument of type Fix it with a cast. -Signed-off-by: Simon Glass <sjg@chromium.org> Series-to: u-boot Series-prefix: RFC Series-cc: Stefan Brüns <stefan.bruens@rwth-aachen.de> @@ -40,6 +39,7 @@ This is a test of how the cover letter works END +Signed-off-by: Simon Glass <sjg@chromium.org> --- fs/fat/fat.c | 1 + lib/efi_loader/efi_memory.c | 1 + diff --git a/tools/patman/test/test01.txt b/tools/patman/test/test01.txt index b238a8b4bab..6ef8faf66b8 100644 --- a/tools/patman/test/test01.txt +++ b/tools/patman/test/test01.txt @@ -12,7 +12,6 @@ Date: Sat Apr 15 15:39:08 2017 -0600 Fix it with a cast. - Signed-off-by: Simon Glass <sjg@chromium.org> Commit-changes: 2 - second revision change @@ -22,6 +21,7 @@ Date: Sat Apr 15 15:39:08 2017 -0600 from the first commit END + Signed-off-by: Simon Glass <sjg@chromium.org> Commit-notes: Some notes about the first commit @@ -41,7 +41,6 @@ Date: Sat Apr 15 15:39:08 2017 -0600 Fix it with a cast. - Signed-off-by: Simon Glass <sjg@chromium.org> Series-to: u-boot Series-prefix: RFC Series-cc: Stefan Brüns <stefan.bruens@rwth-aachen.de> @@ -67,3 +66,4 @@ Date: Sat Apr 15 15:39:08 2017 -0600 letter works END + Signed-off-by: Simon Glass <sjg@chromium.org>
At present it is possible to put sign-off tags before the change log. This works but then it is hard for patman to add its own tags to a commit. Also if the commit has a Change-Id (e.g. for Gerrit) the commit becomes invalid if there is anything after it. Report a warning if patman tags are in the wrong place. One test patch already has the sign-off in the wrong place. Update the second one to avoid warnings. Signed-off-by: Simon Glass <sjg@chromium.org> --- (no changes since v1) tools/patman/README | 3 ++- tools/patman/func_test.py | 23 ++++++++++++++++++- tools/patman/patchstream.py | 22 ++++++++++++++++++ .../0001-pci-Correct-cast-for-sandbox.patch | 2 +- ...-for-sandbox-in-fdtdec_setup_mem_siz.patch | 2 +- tools/patman/test/test01.txt | 4 ++-- 6 files changed, 50 insertions(+), 6 deletions(-)