diff mbox series

[U-Boot,RFC] patman: Use the Change-Id, version, and prefix in the Message-Id

Message ID 20190828132723.0.RFC.Ie6289f437ae533d7fcaddfcee9202f0e92c6b2b9@changeid
State RFC
Delegated to: Simon Glass
Headers show
Series [U-Boot,RFC] patman: Use the Change-Id, version, and prefix in the Message-Id | expand

Commit Message

Doug Anderson Aug. 28, 2019, 8:27 p.m. UTC 
As per the centithread on ksummit-discuss [1], there are folks who
feel that if a Change-Id is present in a developer's local commit that
said Change-Id could be interesting to include in upstream posts.
Specifically if two commits are posted with the same Change-Id there's
a reasonable chance that they are either the same commit or a newer
version of the same commit.  Specifically this is because that's how
gerrit has trained people to work.

There is much angst about Change-Id in upstream Linux, but one thing
that seems safe and non-controversial is to include the Change-Id as
part of the string of crud that makes up a Message-Id.

Let's give that a try.

In theory (if there is enough adoption) this could help a tool more
reliably find various versions of a commit.  This actually might work
pretty well for U-Boot where (I believe) quite a number of developers
use patman, so there could be critical mass (assuming that enough of
these people also use a git hook that adds Change-Id to their
commits).

In theory one could imagine something like this could be integrated
into other tools, possibly even git-send-email.  Getting it into
patman seems like a sane first step, though.

NOTE: this patch is being posted using a patman containing this patch,
so you should be able to see the Message-Id of this patch and see that
it contains my local Change-Id, which ends in 2b9 if you want to
check.

[1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2019-August/006739.html

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 tools/patman/README         |  8 +++++-
 tools/patman/commit.py      |  3 ++
 tools/patman/patchstream.py | 56 +++++++++++++++++++++++++++++++++++--
 3 files changed, 64 insertions(+), 3 deletions(-)

Comments

Johannes Berg Aug. 28, 2019, 9:05 p.m. UTC | #1 
Hi Doug,

Some comments on the actual mechanics here (vs. on the kernel list I
picked this up from).

It seems like perhaps you could increase the "count" variable. You got:

20190828132723.0.RFC.Ie6289f437ae533d7fcaddfcee9202f0e92c6b2b9@changeid

but usually patches are numbered starting from 1, not 0. Especially for
series, it might be easier to understand if that was the same here.

I'd probably also put it after the tag, so you'd have "RFC.1" instead of
"0.RFC", I'm saying it mostly because I originally thought you somehow
generated a timestamp with a decimal point :-)

Obviously none of that really matters, but it'd be easier to understand
(and if necessary reverse-engineer) IMHO.

johannes
Doug Anderson Aug. 28, 2019, 9:14 p.m. UTC | #2 
Hi,

On Wed, Aug 28, 2019 at 2:05 PM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Hi Doug,
>
> Some comments on the actual mechanics here (vs. on the kernel list I
> picked this up from).
>
> It seems like perhaps you could increase the "count" variable. You got:
>
> 20190828132723.0.RFC.Ie6289f437ae533d7fcaddfcee9202f0e92c6b2b9@changeid
>
> but usually patches are numbered starting from 1, not 0. Especially for
> series, it might be easier to understand if that was the same here.
>
> I'd probably also put it after the tag, so you'd have "RFC.1" instead of
> "0.RFC", I'm saying it mostly because I originally thought you somehow
> generated a timestamp with a decimal point :-)
>
> Obviously none of that really matters, but it'd be easier to understand
> (and if necessary reverse-engineer) IMHO.

I'm happy to make these changes--they seem sane to me.  NOTE that most
patches don't actually have a PREFIX but I purposely added one to this
patch to test it.  :-)  The next version will likely have a "version"
but no prefix, so with your suggestion I'll have:

datetimestring.2.1.Ie6289f437ae533d7fcaddfcee9202f0e92c6b2b9

I may add a "v" before the 2 in the next version also.  I was going to
do that but then I forgot to before I posted.  :-P  Thus:

datetimestring.v2.1.Ie6289f437ae533d7fcaddfcee9202f0e92c6b2b9

If I post v2 as an RFC also, it would be:

datetimestring.RFC.v2.1.Ie6289f437ae533d7fcaddfcee9202f0e92c6b2b9


For now I'll wait a few days before posting a v2 in case anyone wants
to provide additional feedback (or tell me that I'm a terrible person
for liking Change-Ids).


-Doug
diff mbox series

Patch

diff --git a/tools/patman/README b/tools/patman/README
index 7917fc8bdc33..02d582974495 100644
--- a/tools/patman/README
+++ b/tools/patman/README
@@ -259,12 +259,18 @@  Series-process-log: sort, uniq
 	unique entries. If omitted, no change log processing is done.
 	Separate each tag with a comma.
 
+Change-Id:
+	This tag is stripped out but is used to generate the Message-Id
+	of the emails that will be sent. When you keep the Change-Id the
+	same you are asserting that this is a slightly different version
+	(but logically the same patch) as other patches that have been
+	sent out with the same Change-Id.
+
 Various other tags are silently removed, like these Chrome OS and
 Gerrit tags:
 
 BUG=...
 TEST=...
-Change-Id:
 Review URL:
 Reviewed-on:
 Commit-xxxx: (except Commit-notes)
diff --git a/tools/patman/commit.py b/tools/patman/commit.py
index 2bf3a0ba5b92..48d0529c5365 100644
--- a/tools/patman/commit.py
+++ b/tools/patman/commit.py
@@ -21,6 +21,8 @@  class Commit:
             The dict is indexed by change version (an integer)
         cc_list: List of people to aliases/emails to cc on this commit
         notes: List of lines in the commit (not series) notes
+        change_id: the Change-Id: tag that was stripped from this commit
+            and can be used to generate the Message-Id.
     """
     def __init__(self, hash):
         self.hash = hash
@@ -30,6 +32,7 @@  class Commit:
         self.cc_list = []
         self.signoff_set = set()
         self.notes = []
+        self.change_id = None
 
     def AddChange(self, version, info):
         """Add a new change line to the change list for a version.
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
index b6455b0fa383..e3f8f0de16d0 100644
--- a/tools/patman/patchstream.py
+++ b/tools/patman/patchstream.py
@@ -2,6 +2,7 @@ 
 # Copyright (c) 2011 The Chromium OS Authors.
 #
 
+import datetime
 import math
 import os
 import re
@@ -14,7 +15,7 @@  import gitutil
 from series import Series
 
 # Tags that we detect and remove
-re_remove = re.compile('^BUG=|^TEST=|^BRANCH=|^Change-Id:|^Review URL:'
+re_remove = re.compile('^BUG=|^TEST=|^BRANCH=|^Review URL:'
     '|Reviewed-on:|Commit-\w*:')
 
 # Lines which are allowed after a TEST= line
@@ -32,6 +33,9 @@  re_cover_cc = re.compile('^Cover-letter-cc: *(.*)')
 # Patch series tag
 re_series_tag = re.compile('^Series-([a-z-]*): *(.*)')
 
+# Change-Id will be used to generate the Message-Id and then be stripped
+re_change_id = re.compile('^Change-Id: *(.*)')
+
 # Commit series tag
 re_commit_tag = re.compile('^Commit-([a-z-]*): *(.*)')
 
@@ -156,6 +160,7 @@  class PatchStream:
 
         # Handle state transition and skipping blank lines
         series_tag_match = re_series_tag.match(line)
+        change_id_match = re_change_id.match(line)
         commit_tag_match = re_commit_tag.match(line)
         cover_match = re_cover.match(line)
         cover_cc_match = re_cover_cc.match(line)
@@ -177,7 +182,7 @@  class PatchStream:
             self.state = STATE_MSG_HEADER
 
         # If a tag is detected, or a new commit starts
-        if series_tag_match or commit_tag_match or \
+        if series_tag_match or commit_tag_match or change_id_match or \
            cover_match or cover_cc_match or signoff_match or \
            self.state == STATE_MSG_HEADER:
             # but we are already in a section, this means 'END' is missing
@@ -275,6 +280,15 @@  class PatchStream:
                 self.AddToSeries(line, name, value)
                 self.skip_blank = True
 
+        # Detect Change-Id tags
+        elif change_id_match:
+            value = change_id_match.group(1)
+            if self.is_log:
+                if self.commit.change_id:
+                    raise ValueError("%s: Two Change-Ids: '%s' vs. '%s'" %
+                        (self.commit.hash, self.commit.change_id, value))
+                self.commit.change_id = value
+
         # Detect Commit-xxx tags
         elif commit_tag_match:
             name = commit_tag_match.group(1)
@@ -345,6 +359,40 @@  class PatchStream:
             self.warn.append('Found %d lines after TEST=' %
                     self.lines_after_test)
 
+    def WriteMessageId(self, outfd):
+        """Write the Message-Id into the output.
+
+        This is based on the Change-Id in the original patch, the version,
+        and the prefix.
+
+        Args:
+            outfd: Output stream file object
+        """
+        if not self.commit.change_id:
+            return
+
+        # In theory there is email.utils.make_msgid() which would be nice
+        # to use, but it already produces something way too long and thus
+        # will produce ugly commit lines if someone throws this into
+        # a "Link:" tag in the final commit.  So (sigh) roll our own.
+
+        # These parts are just there to guarantee some uniqueness
+        timepart = datetime.datetime.now().strftime("%Y%m%d%H%M%S")
+        seqpart = str(self.commit.count)
+        parts = [timepart, seqpart]
+
+        # These seem like they would be nice to include.
+        if 'prefix' in self.series:
+            parts.append(self.series['prefix'])
+        if 'version' in self.series:
+            parts.append(self.series['version'])
+
+        # The Change-Id must be last, right before the @
+        parts.append(self.commit.change_id)
+
+        # Join parts together with "." and write it out.
+        outfd.write('Message-Id: <%s@changeid>\n' % '.'.join(parts))
+
     def ProcessStream(self, infd, outfd):
         """Copy a stream from infd to outfd, filtering out unwanting things.
 
@@ -358,6 +406,9 @@  class PatchStream:
         fname = None
         last_fname = None
         re_fname = re.compile('diff --git a/(.*) b/.*')
+
+        self.WriteMessageId(outfd)
+
         while True:
             line = infd.readline()
             if not line:
@@ -481,6 +532,7 @@  def FixPatches(series, fnames):
     for fname in fnames:
         commit = series.commits[count]
         commit.patch = fname
+        commit.count = count
         result = FixPatch(backup_dir, fname, series, commit)
         if result:
             print('%d warnings for %s:' % (len(result), fname))