[PATCH 0/2] patman: Add changelog customization options
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(-)
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)
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
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()
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
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
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
participants (2)
-
Sean Anderson -
Simon Glass