[PATCH v2 21/30] patman: Require tags to be before sign-off
Sean Anderson
seanga2 at gmail.com
Mon Oct 26 02:23:03 CET 2020
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 at 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 at 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 at 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 at chromium.org>
> Series-to: u-boot
> Series-prefix: RFC
> Series-cc: Stefan Brüns <stefan.bruens at rwth-aachen.de>
> @@ -40,6 +39,7 @@ This is a test of how the cover
> letter
> works
> END
> +Signed-off-by: Simon Glass <sjg at 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 at 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 at 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 at chromium.org>
> Series-to: u-boot
> Series-prefix: RFC
> Series-cc: Stefan Brüns <stefan.bruens at 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 at chromium.org>
>
More information about the U-Boot
mailing list