From: Sean Anderson <seanga2@gmail.com>
To: u-boot@lists.denx.de
Subject: [PATCH v2 21/30] patman: Require tags to be before sign-off
Date: Sun, 25 Oct 2020 21:23:03 -0400 [thread overview]
Message-ID: <4efd0586-e31d-6dbe-b8b7-3c17a7c3d888@gmail.com> (raw)
In-Reply-To: <20201026010442.1606893-22-sjg@chromium.org>
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>
>
next prev parent reply other threads:[~2020-10-26 1:23 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-26 1:04 [PATCH v2 00/30] patman: Collect review tags and comments from Patchwork Simon Glass
2020-10-26 1:04 ` [PATCH v2 01/30] patman: Correct operation of -n Simon Glass
2020-10-26 1:04 ` [PATCH v2 02/30] azure/gitLab/travis: Add pygit2 as a dependency for tests Simon Glass
2020-10-26 1:04 ` [PATCH v2 03/30] patman: Update how tests are run Simon Glass
2020-10-26 1:04 ` [PATCH v2 04/30] patman: Fix whitespace errors in func_test Simon Glass
2020-10-26 1:04 ` [PATCH v2 05/30] patman: Use capture_sys_output() consistently Simon Glass
2020-10-26 1:04 ` [PATCH v2 06/30] patman: Fix remaining pylint3 warnings in func_test Simon Glass
2020-10-26 1:04 ` [PATCH v2 07/30] patman: Allow linking a series with patchwork Simon Glass
2020-10-26 1:04 ` [PATCH v2 08/30] patman: Fix indenting in patchstream Simon Glass
2020-10-26 1:04 ` [PATCH v2 09/30] patman: Fix constant style " Simon Glass
2020-10-26 1:04 ` [PATCH v2 10/30] patman: Rename functions " Simon Glass
2020-10-26 1:04 ` [PATCH v2 11/30] patman: Rename variables " Simon Glass
2020-10-26 1:04 ` [PATCH v2 12/30] patman: Drop unused args " Simon Glass
2020-10-26 1:04 ` [PATCH v2 13/30] patman: Fix up argument/return docs " Simon Glass
2020-10-26 1:04 ` [PATCH v2 14/30] patman: Move warning collection to a function Simon Glass
2020-10-26 1:04 ` [PATCH v2 15/30] patman: Attach warnings to individual patches Simon Glass
2020-10-26 1:04 ` [PATCH v2 16/30] patman: Convert 'Series-xxx' tag errors into warnings Simon Glass
2020-10-26 1:04 ` [PATCH v2 17/30] patman: Drop unused signoff member Simon Glass
2020-10-26 1:04 ` [PATCH v2 18/30] patman: Add a test for PatchStream tags Simon Glass
2020-10-26 1:04 ` [PATCH v2 19/30] patman: Add some tests for warnings Simon Glass
2020-10-26 1:04 ` [PATCH v2 20/30] patman: Convert testBasic() to use an interator Simon Glass
2020-10-26 1:04 ` [PATCH v2 21/30] patman: Require tags to be before sign-off Simon Glass
2020-10-26 1:23 ` Sean Anderson [this message]
2020-10-27 4:53 ` Simon Glass
2020-10-27 6:05 ` Jonathan Nieder
2020-10-27 20:33 ` Sean Anderson
2020-10-26 1:04 ` [PATCH v2 22/30] patman: Fix spelling of plural for warning Simon Glass
2020-10-26 1:04 ` [PATCH v2 23/30] patman: Don't ignore lines starting with hash Simon Glass
2020-10-26 1:04 ` [PATCH v2 24/30] patman: Allow showing a Commit as a string Simon Glass
2020-10-26 1:04 ` [PATCH v2 25/30] patman: Improve handling of files Simon Glass
2020-10-26 1:04 ` [PATCH v2 26/30] patman: Detect missing upstream in CountCommitsToBranch Simon Glass
2020-10-26 1:04 ` [PATCH v2 27/30] patman: Support checking for review tags in patchwork Simon Glass
2020-10-26 1:04 ` [PATCH v2 28/30] patman: Support updating a branch with review tags Simon Glass
2020-10-26 1:04 ` [PATCH v2 29/30] patman: Support parsing of review snippets Simon Glass
2020-10-26 1:04 ` [PATCH v2 30/30] patman: Support listing comments from patchwork Simon Glass
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4efd0586-e31d-6dbe-b8b7-3c17a7c3d888@gmail.com \
--to=seanga2@gmail.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox