| Message ID | 20200320053636.745307-3-seanga2@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | patman: Add changelog customization options | expand |
Hi Sean, On Thu, 19 Mar 2020 at 23:37, Sean Anderson <seanga2 at 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 at 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 at 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 at 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 at 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 at 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 at 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
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()
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 at 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(-)