[PATCH 0/2] patman: Add changelog customization options

[PATCH 0/2] patman: Add changelog customization options

[Sean Anderson]

This series adds a few changes I have been using locally as options for
patman.


Sean Anderson (2):
  patman: Add option to suppress empty changelog entries
  patman: Add option to disable combined changelogs

 tools/patman/func_test.py   |  4 ++--
 tools/patman/patchstream.py | 22 ++++++++++++----------
 tools/patman/patman.py      | 13 ++++++++++---
 tools/patman/series.py      | 12 +++++++-----
 tools/patman/test.py        |  2 +-
 5 files changed, 32 insertions(+), 21 deletions(-)

-- 
2.25.1

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

[Sean Anderson]

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@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(-)

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@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)
-- 
2.25.1

[PATCH 2/2] patman: Add option to disable combined changelogs

[Sean Anderson]

By default patman generates a combined changelog for the cover letter. This
may not always be desireable.

Many patches may have the same changes. These can be coalesced with
"Series-process-log: uniq", but this is imperfect. First, this cannot be
used when there are multi-line changes. In addition, similar changes like
"Move foo to patch 7" will not be merged with the similar "Move foo to this
patch from patch 6".

Changes may not make sens outside of the patch they are written for. For
example, a change line of "Add check for bar" does not make sense outside
of the context in which bar might be checked for. Some changes like "New"
or "Lint" may be repeated many times throughout different change logs, but
carry no useful information in a summary.

Lastly, I like to summarize the broad strokes of the changes I have made in
the cover letter, while documenting all the details in the appropriate
patches. I think this make it easier to get a good feel for what has
changed, without making it difficult to wade through every change in the
whole series.

For these reasons, this patch adds an option to disable the automatic
changelog.

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

 tools/patman/func_test.py   | 2 +-
 tools/patman/patchstream.py | 7 ++++---
 tools/patman/patman.py      | 6 +++++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py
index 0a8dc9b661..65eccceb74 100644
--- a/tools/patman/func_test.py
+++ b/tools/patman/func_test.py
@@ -151,7 +151,7 @@ class TestFunctional(unittest.TestCase):
         with capture() as out:
             patchstream.FixPatches(series, args, False)
             if cover_fname and series.get('cover'):
-                patchstream.InsertCoverLetter(cover_fname, series, count)
+                patchstream.InsertCoverLetter(cover_fname, series, count, True)
             series.DoChecks()
             cc_file = series.MakeCcFile(process_tags, cover_fname,
                                         not ignore_bad_tags, add_maintainers,
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
index 3d83ed6adb..1cc9bce00c 100644
--- a/tools/patman/patchstream.py
+++ b/tools/patman/patchstream.py
@@ -551,7 +551,7 @@ def FixPatches(series, fnames, empty_changes):
         count += 1
     print('Cleaned %d patches' % count)
 
-def InsertCoverLetter(fname, series, count):
+def InsertCoverLetter(fname, series, count, changelog):
     """Inserts a cover letter with the required info into patch 0
 
     Args:
@@ -581,7 +581,8 @@ def InsertCoverLetter(fname, series, count):
                 line += '\n'.join(series.notes) + '\n'
 
             # Now the change list
-            out = series.MakeChangeLog(None)
-            line += '\n' + '\n'.join(out)
+            if changelog:
+                out = series.MakeChangeLog(None, False)
+                line += '\n' + '\n'.join(out)
         fd.write(line)
     fd.close()
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index 6f92c5b7f3..aa123c18c2 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -62,6 +62,9 @@ parser.add_option('--no-check', action='store_false', dest='check_patch',
                   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 aliases")
+parser.add_option('--no-changelog', action = 'store_false', dest='changelog',
+                  default=True,
+		  help="Don't create a changelog in the cover letter")
 parser.add_option('--no-empty-changes', action = 'store_false',
                   dest='empty_changes', default=True,
 		  help="Suppress empty change entries in patch changelogs")
@@ -151,7 +154,8 @@ else:
     # Fix up the patch files to our liking, and insert the cover letter
     patchstream.FixPatches(series, args, options.empty_changes)
     if cover_fname and series.get('cover'):
-        patchstream.InsertCoverLetter(cover_fname, series, options.count)
+        patchstream.InsertCoverLetter(cover_fname, series, options.count,
+	        options.changelog)
 
     # Do a few checks on the series
     series.DoChecks()
-- 
2.25.1

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

[Simon Glass]

Hi Sean,

On Thu, 19 Mar 2020 at 23:37, Sean Anderson <seanga2@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@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

[PATCH 2/2] patman: Add option to disable combined changelogs

[Simon Glass]

Hi Sean,

On Thu, 19 Mar 2020 at 23:37, Sean Anderson <seanga2@gmail.com> wrote:
>
> By default patman generates a combined changelog for the cover letter. This
> may not always be desireable.
>
> Many patches may have the same changes. These can be coalesced with
> "Series-process-log: uniq", but this is imperfect. First, this cannot be
> used when there are multi-line changes. In addition, similar changes like

We could perhaps support this if we used indentation to indicate multiple lines

- line 1 of a multi-line message
  line 2 here
- this is line 1 of a single-line message

> "Move foo to patch 7" will not be merged with the similar "Move foo to this
> patch from patch 6".
>
> Changes may not make sens outside of the patch they are written for. For

sense

> example, a change line of "Add check for bar" does not make sense outside
> of the context in which bar might be checked for. Some changes like "New"
> or "Lint" may be repeated many times throughout different change logs, but
> carry no useful information in a summary.
>
> Lastly, I like to summarize the broad strokes of the changes I have made in
> the cover letter, while documenting all the details in the appropriate
> patches. I think this make it easier to get a good feel for what has
> changed, without making it difficult to wade through every change in the
> whole series.
>
> For these reasons, this patch adds an option to disable the automatic
> changelog.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
>  tools/patman/func_test.py   | 2 +-
>  tools/patman/patchstream.py | 7 ++++---
>  tools/patman/patman.py      | 6 +++++-
>  3 files changed, 10 insertions(+), 5 deletions(-)

Thanks for the great explanations on your patches.

In the case where there is no automatic change log on the cover
letter, do you use 'Series-notes' instead? I'd like to enforce some
sort of change log in the cover letter.

I also think this would be better as a tag in a commit, like
'Series-no-change-log: yes'. That way you set it up when you create
the patches, and it persists without needing to add the options each
time.

Regards,
Simon

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

[Sean Anderson]

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

[PATCH 2/2] patman: Add option to disable combined changelogs

[Sean Anderson]

On 3/21/20 10:43 AM, Simon Glass wrote:
> Hi Sean,
> 
> On Thu, 19 Mar 2020 at 23:37, Sean Anderson <seanga2@gmail.com> wrote:
>>
>> By default patman generates a combined changelog for the cover letter. This
>> may not always be desireable.
>>
>> Many patches may have the same changes. These can be coalesced with
>> "Series-process-log: uniq", but this is imperfect. First, this cannot be
>> used when there are multi-line changes. In addition, similar changes like
> 
> We could perhaps support this if we used indentation to indicate multiple lines
> 
> - line 1 of a multi-line message
>   line 2 here
> - this is line 1 of a single-line message

That is probably the best solution in general.

>> "Move foo to patch 7" will not be merged with the similar "Move foo to this
>> patch from patch 6".
>>
>> Changes may not make sens outside of the patch they are written for. For
> 
> sense
> 
>> example, a change line of "Add check for bar" does not make sense outside
>> of the context in which bar might be checked for. Some changes like "New"
>> or "Lint" may be repeated many times throughout different change logs, but
>> carry no useful information in a summary.
>>
>> Lastly, I like to summarize the broad strokes of the changes I have made in
>> the cover letter, while documenting all the details in the appropriate
>> patches. I think this make it easier to get a good feel for what has
>> changed, without making it difficult to wade through every change in the
>> whole series.
>>
>> For these reasons, this patch adds an option to disable the automatic
>> changelog.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>>
>>  tools/patman/func_test.py   | 2 +-
>>  tools/patman/patchstream.py | 7 ++++---
>>  tools/patman/patman.py      | 6 +++++-
>>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> Thanks for the great explanations on your patches.
> 
> In the case where there is no automatic change log on the cover
> letter, do you use 'Series-notes' instead? I'd like to enforce some
> sort of change log in the cover letter.

That could work, but it doesn't really support combining changes from
multiple patches. I've been doing it manually, but some automation would
definitely be better. One idea would be to do something like

Series-changes 12:
* Summary of the below changes
- Small change
- Move foo to bar.c
- Reformat code to do baz
* Fix bug where device caught fire

Where only lines beginning with '*' would be included in the combined
changelog. The character could be configurable. Another option could be
to do something like

Series-changes 12:
- Summary of the below changes
  - Small change
  - Move foo to bar.c
  - Reformat code to do baz
- Fix bug where device caught fire

This is nicer aesthetically, but would make any future multi-line
treatment of series-changes more difficult.

One last option could be to do something like

Summary-changes 12:
- Summary of below changes
- Fix bug where device caught fire

Where those changes would be included in the cover letter, but not
Series-changes. This is probably the easiest to implement, but risks
adding duplication and making commits more verbose.

> I also think this would be better as a tag in a commit, like
> 'Series-no-change-log: yes'. That way you set it up when you create
> the patches, and it persists without needing to add the options each
> time.

That's probably the best approach.

Should I rebase this series on top of the patch you cc-d me on ("patman:
Update to use absolute imports")?

--Sean

[PATCH 2/2] patman: Add option to disable combined changelogs

[Simon Glass]

Hi Sean,

On Sat, 21 Mar 2020 at 12:57, Sean Anderson <seanga2@gmail.com> wrote:
>
> On 3/21/20 10:43 AM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Thu, 19 Mar 2020 at 23:37, Sean Anderson <seanga2@gmail.com> wrote:
> >>
> >> By default patman generates a combined changelog for the cover letter. This
> >> may not always be desireable.
> >>
> >> Many patches may have the same changes. These can be coalesced with
> >> "Series-process-log: uniq", but this is imperfect. First, this cannot be
> >> used when there are multi-line changes. In addition, similar changes like
> >
> > We could perhaps support this if we used indentation to indicate multiple lines
> >
> > - line 1 of a multi-line message
> >   line 2 here
> > - this is line 1 of a single-line message
>
> That is probably the best solution in general.

OK then let's do that.

>
> >> "Move foo to patch 7" will not be merged with the similar "Move foo to this
> >> patch from patch 6".
> >>
> >> Changes may not make sens outside of the patch they are written for. For
> >
> > sense
> >
> >> example, a change line of "Add check for bar" does not make sense outside
> >> of the context in which bar might be checked for. Some changes like "New"
> >> or "Lint" may be repeated many times throughout different change logs, but
> >> carry no useful information in a summary.
> >>
> >> Lastly, I like to summarize the broad strokes of the changes I have made in
> >> the cover letter, while documenting all the details in the appropriate
> >> patches. I think this make it easier to get a good feel for what has
> >> changed, without making it difficult to wade through every change in the
> >> whole series.
> >>
> >> For these reasons, this patch adds an option to disable the automatic
> >> changelog.
> >>
> >> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> >> ---
> >>
> >>  tools/patman/func_test.py   | 2 +-
> >>  tools/patman/patchstream.py | 7 ++++---
> >>  tools/patman/patman.py      | 6 +++++-
> >>  3 files changed, 10 insertions(+), 5 deletions(-)
> >
> > Thanks for the great explanations on your patches.
> >
> > In the case where there is no automatic change log on the cover
> > letter, do you use 'Series-notes' instead? I'd like to enforce some
> > sort of change log in the cover letter.
>
> That could work, but it doesn't really support combining changes from
> multiple patches. I've been doing it manually, but some automation would
> definitely be better. One idea would be to do something like

It does collate the notes and put it in the cover letter, or at least it should.

>
> Series-changes 12:
> * Summary of the below changes
> - Small change
> - Move foo to bar.c
> - Reformat code to do baz
> * Fix bug where device caught fire
>
> Where only lines beginning with '*' would be included in the combined
> changelog. The character could be configurable. Another option could be
> to do something like
>
> Series-changes 12:
> - Summary of the below changes
>   - Small change
>   - Move foo to bar.c
>   - Reformat code to do baz
> - Fix bug where device caught fire
>
> This is nicer aesthetically, but would make any future multi-line
> treatment of series-changes more difficult.

Perhaps the clearest thing is to have:

Series-changes: (behaviour as now)
Commit-changes: (change log just in the commit/patch, not repeated in
the cover letter)
Cover-changes: (change log just in the cover letter)

This should minimise duplication and makes the scheme optional.

>
> One last option could be to do something like
>
> Summary-changes 12:
> - Summary of below changes
> - Fix bug where device caught fire
>
> Where those changes would be included in the cover letter, but not
> Series-changes. This is probably the easiest to implement, but risks
> adding duplication and making commits more verbose.
>
> > I also think this would be better as a tag in a commit, like
> > 'Series-no-change-log: yes'. That way you set it up when you create
> > the patches, and it persists without needing to add the options each
> > time.
>
> That's probably the best approach.
>
> Should I rebase this series on top of the patch you cc-d me on ("patman:
> Update to use absolute imports")?

Can we hold off until we figure out what we definitely want?

Regards,
Simon

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

[Simon Glass]

Hi Sean,

On Sat, 21 Mar 2020 at 12:44, Sean Anderson <seanga2@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

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

[Sean Anderson]

On 3/21/20 3:17 PM, Simon Glass wrote:
> Hi Sean,
> 
> On Sat, 21 Mar 2020 at 12:44, Sean Anderson <seanga2@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