diff mbox series

[v2,21/30] patman: Require tags to be before sign-off

Message ID 20201026010442.1606893-22-sjg@chromium.org
State Not Applicable
Headers show
Series [v2,01/30] patman: Correct operation of -n | expand

Commit Message

Simon Glass Oct. 26, 2020, 1:04 a.m. UTC
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(-)

Comments

Simon Glass Oct. 27, 2020, 4:53 a.m. UTC | #1
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
Jonathan Nieder Oct. 27, 2020, 6:05 a.m. UTC | #2
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
diff mbox series

Patch

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>