| Message ID | 20200320053636.745307-2-seanga2@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | patman: Add changelog customization options | expand |
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
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
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
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
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)
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(-)