diff mbox series

[1/2] patman: Add option to suppress empty changelog entries

Message ID 20200320053636.745307-2-seanga2@gmail.com
State New
Headers show
Series patman: Add changelog customization options | expand

Commit Message

Sean Anderson March 20, 2020, 5:36 a.m. UTC
By default, patman outputs a line for every edition of the series in every
patch, regardless of whether any changes were made. This can result in many
redundant lines in patch changelogs, especially when a patch did not exist
before a certain revision. For example, the default behaviour could result
in a changelog of

Changes in v6:
- Make a change

Changes in v5: None

Changes in v4:
- New

Changes in v3: None
Changes in v2: None
Changes in v1: None

With this patch applied and with --no-empty-changes, the same patch would
look like

Changes in v6:
- Make a change

Changes in v4:
- New

This is entirely aesthetic, but I think it reduces clutter, especially for
patches added later on in a series.

Signed-off-by: Sean Anderson <seanga2 at gmail.com>
---

 tools/patman/func_test.py   |  2 +-
 tools/patman/patchstream.py | 15 ++++++++-------
 tools/patman/patman.py      |  7 +++++--
 tools/patman/series.py      | 12 +++++++-----
 tools/patman/test.py        |  2 +-
 5 files changed, 22 insertions(+), 16 deletions(-)

Comments

Simon Glass March 21, 2020, 2:42 p.m. UTC | #1
Hi Sean,

On Thu, 19 Mar 2020 at 23:37, Sean Anderson <seanga2 at gmail.com> wrote:
>
> By default, patman outputs a line for every edition of the series in every
> patch, regardless of whether any changes were made. This can result in many
> redundant lines in patch changelogs, especially when a patch did not exist
> before a certain revision. For example, the default behaviour could result
> in a changelog of
>
> Changes in v6:
> - Make a change
>
> Changes in v5: None
>
> Changes in v4:
> - New
>
> Changes in v3: None
> Changes in v2: None
> Changes in v1: None
>
> With this patch applied and with --no-empty-changes, the same patch would
> look like
>
> Changes in v6:
> - Make a change
>
> Changes in v4:
> - New
>
> This is entirely aesthetic, but I think it reduces clutter, especially for
> patches added later on in a series.
>
> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
> ---
>
>  tools/patman/func_test.py   |  2 +-
>  tools/patman/patchstream.py | 15 ++++++++-------
>  tools/patman/patman.py      |  7 +++++--
>  tools/patman/series.py      | 12 +++++++-----
>  tools/patman/test.py        |  2 +-
>  5 files changed, 22 insertions(+), 16 deletions(-)

I can see the value here, particularly for the 'new' case. But I
actually appreciate the positive confirmation that nothing changed.
Sometimes people send patches and fail to add a change log.

What happens if a patch has no changes at all since v1? Do you think
we should always report 'None' for the last version?

Regards,
Simon
Sean Anderson March 21, 2020, 6:44 p.m. UTC | #2
On 3/21/20 10:42 AM, Simon Glass wrote:
> Hi Sean,
> 
> I can see the value here, particularly for the 'new' case. But I
> actually appreciate the positive confirmation that nothing changed.
> Sometimes people send patches and fail to add a change log.

Hm, I don't know if this patch would affect that. If there are no
"Series-changes" tags, we just get nothing (vs. a bunch of "None"s).

> What happens if a patch has no changes at all since v1? Do you think
> we should always report 'None' for the last version?

In my opinion, I think we should report nothing. Of course, this patch
is entirely for aesthetics. It's perfectly valid to do one thing or
another. In my patches, I like to emulate what I would write if I was
doing it by hand.

--Sean
Simon Glass March 21, 2020, 7:17 p.m. UTC | #3
Hi Sean,

On Sat, 21 Mar 2020 at 12:44, Sean Anderson <seanga2 at gmail.com> wrote:
>
> On 3/21/20 10:42 AM, Simon Glass wrote:
> > Hi Sean,
> >
> > I can see the value here, particularly for the 'new' case. But I
> > actually appreciate the positive confirmation that nothing changed.
> > Sometimes people send patches and fail to add a change log.
>
> Hm, I don't know if this patch would affect that. If there are no
> "Series-changes" tags, we just get nothing (vs. a bunch of "None"s).
>
> > What happens if a patch has no changes at all since v1? Do you think
> > we should always report 'None' for the last version?
>
> In my opinion, I think we should report nothing. Of course, this patch
> is entirely for aesthetics. It's perfectly valid to do one thing or
> another. In my patches, I like to emulate what I would write if I was
> doing it by hand.

But as a reviewer, for a v2...n patchset I really do want to see a
change log. If nothing has changed I want to know that, and the
absence of a change log is not enough to convince me that there are no
changes.

While you have structured your patch as an option, it would be better
to make it the default, so long as we can avoid confusion.

Perhaps we should have something like '(no changes since v1)' added in
this case?

Regards,
Simon
Sean Anderson March 21, 2020, 7:24 p.m. UTC | #4
On 3/21/20 3:17 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Sat, 21 Mar 2020 at 12:44, Sean Anderson <seanga2 at gmail.com> wrote:
>>
>> On 3/21/20 10:42 AM, Simon Glass wrote:
>>> Hi Sean,
>>>
>>> I can see the value here, particularly for the 'new' case. But I
>>> actually appreciate the positive confirmation that nothing changed.
>>> Sometimes people send patches and fail to add a change log.
>>
>> Hm, I don't know if this patch would affect that. If there are no
>> "Series-changes" tags, we just get nothing (vs. a bunch of "None"s).
>>
>>> What happens if a patch has no changes at all since v1? Do you think
>>> we should always report 'None' for the last version?
>>
>> In my opinion, I think we should report nothing. Of course, this patch
>> is entirely for aesthetics. It's perfectly valid to do one thing or
>> another. In my patches, I like to emulate what I would write if I was
>> doing it by hand.
> 
> But as a reviewer, for a v2...n patchset I really do want to see a
> change log. If nothing has changed I want to know that, and the
> absence of a change log is not enough to convince me that there are no
> changes.
> 
> While you have structured your patch as an option, it would be better
> to make it the default, so long as we can avoid confusion.
> 
> Perhaps we should have something like '(no changes since v1)' added in
> this case?
> 
> Regards,
> Simon
> 

That sounds good.

--Sean
diff mbox series

Patch

diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py
index 76319fff37..0a8dc9b661 100644
--- a/tools/patman/func_test.py
+++ b/tools/patman/func_test.py
@@ -149,7 +149,7 @@  class TestFunctional(unittest.TestCase):
         series = patchstream.GetMetaDataForTest(text)
         cover_fname, args = self.CreatePatchesForTest(series)
         with capture() as out:
-            patchstream.FixPatches(series, args)
+            patchstream.FixPatches(series, args, False)
             if cover_fname and series.get('cover'):
                 patchstream.InsertCoverLetter(cover_fname, series, count)
             series.DoChecks()
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
index df3eb7483b..3d83ed6adb 100644
--- a/tools/patman/patchstream.py
+++ b/tools/patman/patchstream.py
@@ -62,7 +62,7 @@  class PatchStream:
     unwanted tags or inject additional ones. These correspond to the two
     phases of processing.
     """
-    def __init__(self, series, name=None, is_log=False):
+    def __init__(self, series, empty_changes=True, name=None, is_log=False):
         self.skip_blank = False          # True to skip a single blank line
         self.found_test = False          # Found a TEST= line
         self.lines_after_test = 0        # MNumber of lines found after TEST=
@@ -78,6 +78,7 @@  class PatchStream:
         self.state = STATE_MSG_HEADER    # What state are we in?
         self.signoff = []                # Contents of signoff line
         self.commit = None               # Current commit
+        self.empty_changes = empty_changes    # Whether to output empty changes
 
     def AddToSeries(self, line, name, value):
         """Add a new Series-xxx tag.
@@ -340,9 +341,9 @@  class PatchStream:
             elif line == '---':
                 self.state = STATE_DIFFS
 
-                # Output the tags (signeoff first), then change list
+                # Output the tags (signoff first), then change list
                 out = []
-                log = self.series.MakeChangeLog(self.commit)
+                log = self.series.MakeChangeLog(self.commit, self.empty_changes)
                 out += [line]
                 if self.commit:
                     out += self.commit.notes
@@ -495,7 +496,7 @@  def GetMetaDataForTest(text):
     ps.Finalize()
     return series
 
-def FixPatch(backup_dir, fname, series, commit):
+def FixPatch(backup_dir, fname, series, commit, empty_changes):
     """Fix up a patch file, by adding/removing as required.
 
     We remove our tags from the patch file, insert changes lists, etc.
@@ -513,7 +514,7 @@  def FixPatch(backup_dir, fname, series, commit):
     handle, tmpname = tempfile.mkstemp()
     outfd = os.fdopen(handle, 'w', encoding='utf-8')
     infd = open(fname, 'r', encoding='utf-8')
-    ps = PatchStream(series)
+    ps = PatchStream(series, empty_changes=empty_changes)
     ps.commit = commit
     ps.ProcessStream(infd, outfd)
     infd.close()
@@ -525,7 +526,7 @@  def FixPatch(backup_dir, fname, series, commit):
     shutil.move(tmpname, fname)
     return ps.warn
 
-def FixPatches(series, fnames):
+def FixPatches(series, fnames, empty_changes):
     """Fix up a list of patches identified by filenames
 
     The patch files are processed in place, and overwritten.
@@ -541,7 +542,7 @@  def FixPatches(series, fnames):
         commit = series.commits[count]
         commit.patch = fname
         commit.count = count
-        result = FixPatch(backup_dir, fname, series, commit)
+        result = FixPatch(backup_dir, fname, series, commit, empty_changes)
         if result:
             print('%d warnings for %s:' % (len(result), fname))
             for warn in result:
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index cf53e532dd..6f92c5b7f3 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -61,7 +61,10 @@  parser.add_option('--no-check', action='store_false', dest='check_patch',
                   default=True,
                   help="Don't check for patch compliance")
 parser.add_option('--no-tags', action='store_false', dest='process_tags',
-                  default=True, help="Don't process subject tags as aliaes")
+                  default=True, help="Don't process subject tags as aliases")
+parser.add_option('--no-empty-changes', action = 'store_false',
+                  dest='empty_changes', default=True,
+		  help="Suppress empty change entries in patch changelogs")
 parser.add_option('--smtp-server', type='str',
                   help="Specify the SMTP server to 'git send-email'")
 parser.add_option('-T', '--thread', action='store_true', dest='thread',
@@ -146,7 +149,7 @@  else:
                 series)
 
     # Fix up the patch files to our liking, and insert the cover letter
-    patchstream.FixPatches(series, args)
+    patchstream.FixPatches(series, args, options.empty_changes)
     if cover_fname and series.get('cover'):
         patchstream.InsertCoverLetter(cover_fname, series, options.count)
 
diff --git a/tools/patman/series.py b/tools/patman/series.py
index a15f7625ed..24538e8895 100644
--- a/tools/patman/series.py
+++ b/tools/patman/series.py
@@ -137,7 +137,7 @@  class Series(dict):
         if cmd:
             print('Git command: %s' % cmd)
 
-    def MakeChangeLog(self, commit):
+    def MakeChangeLog(self, commit, empty_changes):
         """Create a list of changes for each version.
 
         Return:
@@ -151,7 +151,8 @@  class Series(dict):
             - Fix the widget
             - Jog the dial
 
-            etc.
+            etc. If empty_changes is False, suppress output of versions without
+            any changes.
         """
         final = []
         process_it = self.get('process_log', '').split(',')
@@ -170,9 +171,10 @@  class Series(dict):
                 out = sorted(out)
             if have_changes:
                 out.insert(0, line)
-            else:
-                out = [line + ' None']
-            if need_blank:
+            elif empty_changes:
+                out.insert(0, ' None')
+            # Only add a new line if we output something
+            if need_blank and (empty_changes or have_changes):
                 out.insert(0, '')
             final += out
             need_blank = have_changes
diff --git a/tools/patman/test.py b/tools/patman/test.py
index 889e186606..610ffaede6 100644
--- a/tools/patman/test.py
+++ b/tools/patman/test.py
@@ -89,7 +89,7 @@  Signed-off-by: Simon Glass <sjg at chromium.org>
         com.change_id = 'I80fe1d0c0b7dd10aa58ce5bb1d9290b6664d5413'
         com.count = -1
 
-        patchstream.FixPatch(None, inname, series.Series(), com)
+        patchstream.FixPatch(None, inname, series.Series(), com, False)
 
         rc = os.system('diff -u %s %s' % (inname, expname))
         self.assertEqual(rc, 0)