| Message ID | 20200503215533.360196-3-seanga2@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | patman: Add changelog customization options | expand |
Hi Sean, On Sun, 3 May 2020 at 15:55, Sean Anderson <seanga2 at gmail.com> wrote: > > 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 existing behaviour could result > in a changelog of > > Changes in v7: None > Changes in v6: None > Changes in v5: > - Make a change > > Changes in v4: None > > Changes in v3: > - New > > Changes in v2: None > > With this patch applied and with --no-empty-changes, the same patch would > look like > > (no changes since v5) > > Changes in v5: > - Make a change > > Changes in v3: > - 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> > --- > > Changes in v3: > - Document empty changelog suppression in README > - Fix KeyError when running tests > - Fix no changes message being output for revision 1 > - Fix no changes message sometimes being output before every > non-newest-revision change > - Make the newest_version logic more robust (and ugly) > - Update commit subject > > Changes in v2: > - Add a note when there are no changes in the current revision > - Make this the default behaviour, and remove the option > > tools/patman/README | 21 ++++++++++++++++++++ > tools/patman/series.py | 44 +++++++++++++++++++++++++++++++----------- > 2 files changed, 54 insertions(+), 11 deletions(-) Reviewed-by: Simon Glass <sjg at chromium.org> Please see comment below though. [..] > diff --git a/tools/patman/series.py b/tools/patman/series.py > index 6d9d48b123..4359442174 100644 > --- a/tools/patman/series.py > +++ b/tools/patman/series.py > @@ -146,38 +146,60 @@ class Series(dict): > Changes in v4: > - Jog the dial back closer to the widget > > - Changes in v3: None > Changes in v2: > - Fix the widget > - Jog the dial > > - etc. > + If there are no new changes in a patch, a note will be added > + > + (no changes since v2) > + > + Changes in v2: > + - Fix the widget > + - Jog the dial > """ > + versions = sorted(self.changes, reverse=True) > + newest_version = 1 > + try: > + newest_version = max(newest_version, int(self.version)) > + except (ValueError, KeyError): > + pass > + try: > + newest_version = max(newest_version, versions[0]) > + except IndexError: > + pass Can we do this without exceptions so it is more deterministic? E.g. if 'version' in self: newest_version = max(newest_version, int(self.version)) if versions: newest_version = max(newest_version, versions[0]) > + > final = [] > process_it = self.get('process_log', '').split(',') > process_it = [item.strip() for item in process_it] > need_blank = False > - for change in sorted(self.changes, reverse=True): > + for version in versions: > out = [] > - for this_commit, text in self.changes[change]: > + for this_commit, text in self.changes[version]: > if commit and this_commit != commit: > continue > if 'uniq' not in process_it or text not in out: > out.append(text) > - line = 'Changes in v%d:' % change > - have_changes = len(out) > 0 > if 'sort' in process_it: > out = sorted(out) > + have_changes = len(out) > 0 > + line = 'Changes in v%d:' % version > if have_changes: > out.insert(0, line) > - else: > - out = [line + ' None'] > - if need_blank: > - out.insert(0, '') > + if version < newest_version and len(final) == 0: > + out.insert(0, '') > + out.insert(0, '(no changes since v%d)' % version) > + newest_version = 0 > + # Only add a new line if we output something > + if need_blank: > + out.insert(0, '') > final += out > need_blank = have_changes > - if self.changes: > + > + if len(final) > 0: > final.append('') > + elif newest_version != 1: > + final = ['(no changes since v1)', ''] > return final > > def DoChecks(self): > -- > 2.26.2 > Regards, Simon
On 5/4/20 10:39 AM, Simon Glass wrote: > Hi Sean, > > On Sun, 3 May 2020 at 15:55, Sean Anderson <seanga2 at gmail.com> wrote: >> >> 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 existing behaviour could result >> in a changelog of >> >> Changes in v7: None >> Changes in v6: None >> Changes in v5: >> - Make a change >> >> Changes in v4: None >> >> Changes in v3: >> - New >> >> Changes in v2: None >> >> With this patch applied and with --no-empty-changes, the same patch would >> look like >> >> (no changes since v5) >> >> Changes in v5: >> - Make a change >> >> Changes in v3: >> - 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> >> --- >> >> Changes in v3: >> - Document empty changelog suppression in README >> - Fix KeyError when running tests >> - Fix no changes message being output for revision 1 >> - Fix no changes message sometimes being output before every >> non-newest-revision change >> - Make the newest_version logic more robust (and ugly) >> - Update commit subject >> >> Changes in v2: >> - Add a note when there are no changes in the current revision >> - Make this the default behaviour, and remove the option >> >> tools/patman/README | 21 ++++++++++++++++++++ >> tools/patman/series.py | 44 +++++++++++++++++++++++++++++++----------- >> 2 files changed, 54 insertions(+), 11 deletions(-) > > Reviewed-by: Simon Glass <sjg at chromium.org> > > Please see comment below though. > > [..] > >> diff --git a/tools/patman/series.py b/tools/patman/series.py >> index 6d9d48b123..4359442174 100644 >> --- a/tools/patman/series.py >> +++ b/tools/patman/series.py >> @@ -146,38 +146,60 @@ class Series(dict): >> Changes in v4: >> - Jog the dial back closer to the widget >> >> - Changes in v3: None >> Changes in v2: >> - Fix the widget >> - Jog the dial >> >> - etc. >> + If there are no new changes in a patch, a note will be added >> + >> + (no changes since v2) >> + >> + Changes in v2: >> + - Fix the widget >> + - Jog the dial >> """ >> + versions = sorted(self.changes, reverse=True) >> + newest_version = 1 >> + try: >> + newest_version = max(newest_version, int(self.version)) >> + except (ValueError, KeyError): >> + pass >> + try: >> + newest_version = max(newest_version, versions[0]) >> + except IndexError: >> + pass > > Can we do this without exceptions so it is more deterministic? > > E.g. > > if 'version' in self: > newest_version = max(newest_version, int(self.version)) > if versions: > newest_version = max(newest_version, versions[0]) Is it fine to not check for ValueError in this instance? I noticed that the other area where it's used also doesn't check, however that is in DoChecks (and also doesn't check if the key exists either). --Sean
Hi Sean, On Mon, 4 May 2020 at 10:59, Sean Anderson <seanga2 at gmail.com> wrote: > > On 5/4/20 10:39 AM, Simon Glass wrote: > > Hi Sean, > > > > On Sun, 3 May 2020 at 15:55, Sean Anderson <seanga2 at gmail.com> wrote: > >> > >> 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 existing behaviour could result > >> in a changelog of > >> > >> Changes in v7: None > >> Changes in v6: None > >> Changes in v5: > >> - Make a change > >> > >> Changes in v4: None > >> > >> Changes in v3: > >> - New > >> > >> Changes in v2: None > >> > >> With this patch applied and with --no-empty-changes, the same patch would > >> look like > >> > >> (no changes since v5) > >> > >> Changes in v5: > >> - Make a change > >> > >> Changes in v3: > >> - 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> > >> --- > >> > >> Changes in v3: > >> - Document empty changelog suppression in README > >> - Fix KeyError when running tests > >> - Fix no changes message being output for revision 1 > >> - Fix no changes message sometimes being output before every > >> non-newest-revision change > >> - Make the newest_version logic more robust (and ugly) > >> - Update commit subject > >> > >> Changes in v2: > >> - Add a note when there are no changes in the current revision > >> - Make this the default behaviour, and remove the option > >> > >> tools/patman/README | 21 ++++++++++++++++++++ > >> tools/patman/series.py | 44 +++++++++++++++++++++++++++++++----------- > >> 2 files changed, 54 insertions(+), 11 deletions(-) > > > > Reviewed-by: Simon Glass <sjg at chromium.org> > > > > Please see comment below though. > > > > [..] > > > >> diff --git a/tools/patman/series.py b/tools/patman/series.py > >> index 6d9d48b123..4359442174 100644 > >> --- a/tools/patman/series.py > >> +++ b/tools/patman/series.py > >> @@ -146,38 +146,60 @@ class Series(dict): > >> Changes in v4: > >> - Jog the dial back closer to the widget > >> > >> - Changes in v3: None > >> Changes in v2: > >> - Fix the widget > >> - Jog the dial > >> > >> - etc. > >> + If there are no new changes in a patch, a note will be added > >> + > >> + (no changes since v2) > >> + > >> + Changes in v2: > >> + - Fix the widget > >> + - Jog the dial > >> """ > >> + versions = sorted(self.changes, reverse=True) > >> + newest_version = 1 > >> + try: > >> + newest_version = max(newest_version, int(self.version)) > >> + except (ValueError, KeyError): > >> + pass > >> + try: > >> + newest_version = max(newest_version, versions[0]) > >> + except IndexError: > >> + pass > > > > Can we do this without exceptions so it is more deterministic? > > > > E.g. > > > > if 'version' in self: > > newest_version = max(newest_version, int(self.version)) > > if versions: > > newest_version = max(newest_version, versions[0]) > > Is it fine to not check for ValueError in this instance? I noticed that > the other area where it's used also doesn't check, however that is in > DoChecks (and also doesn't check if the key exists either). If the tests pass then you probably don't need checks. Having said that the test coverage is not 100%, so do a bit of manual testing too. I'm fine with raising an error if something is wrong. I just don't like using exceptions to figure out what to do. Regards, Simon
diff --git a/tools/patman/README b/tools/patman/README index 02d5829744..d1d9891c4c 100644 --- a/tools/patman/README +++ b/tools/patman/README @@ -474,6 +474,27 @@ print out the command line patman would have used. not later when you can't remember which patch you changed. You can always go back and change or remove logs from commits. +7. Patches will have no changelog entries for revisions where they did not +change. For clarity, if there are no changes for this patch in the most +recent revision of the series, a note will be added. For example, a patch +with the following tags in the commit + + Series-version: 5 + Series-changes: 2 + - Some change + + Series-changes: 4 + - Another change + +would have a changelog of + + (no changes since v4) + + Changes in v4: + - Another change + + Changes in v2: + - Some change Other thoughts ============== diff --git a/tools/patman/series.py b/tools/patman/series.py index 6d9d48b123..4359442174 100644 --- a/tools/patman/series.py +++ b/tools/patman/series.py @@ -146,38 +146,60 @@ class Series(dict): Changes in v4: - Jog the dial back closer to the widget - Changes in v3: None Changes in v2: - Fix the widget - Jog the dial - etc. + If there are no new changes in a patch, a note will be added + + (no changes since v2) + + Changes in v2: + - Fix the widget + - Jog the dial """ + versions = sorted(self.changes, reverse=True) + newest_version = 1 + try: + newest_version = max(newest_version, int(self.version)) + except (ValueError, KeyError): + pass + try: + newest_version = max(newest_version, versions[0]) + except IndexError: + pass + final = [] process_it = self.get('process_log', '').split(',') process_it = [item.strip() for item in process_it] need_blank = False - for change in sorted(self.changes, reverse=True): + for version in versions: out = [] - for this_commit, text in self.changes[change]: + for this_commit, text in self.changes[version]: if commit and this_commit != commit: continue if 'uniq' not in process_it or text not in out: out.append(text) - line = 'Changes in v%d:' % change - have_changes = len(out) > 0 if 'sort' in process_it: out = sorted(out) + have_changes = len(out) > 0 + line = 'Changes in v%d:' % version if have_changes: out.insert(0, line) - else: - out = [line + ' None'] - if need_blank: - out.insert(0, '') + if version < newest_version and len(final) == 0: + out.insert(0, '') + out.insert(0, '(no changes since v%d)' % version) + newest_version = 0 + # Only add a new line if we output something + if need_blank: + out.insert(0, '') final += out need_blank = have_changes - if self.changes: + + if len(final) > 0: final.append('') + elif newest_version != 1: + final = ['(no changes since v1)', ''] return final def DoChecks(self):
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 existing behaviour could result in a changelog of Changes in v7: None Changes in v6: None Changes in v5: - Make a change Changes in v4: None Changes in v3: - New Changes in v2: None With this patch applied and with --no-empty-changes, the same patch would look like (no changes since v5) Changes in v5: - Make a change Changes in v3: - 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> --- Changes in v3: - Document empty changelog suppression in README - Fix KeyError when running tests - Fix no changes message being output for revision 1 - Fix no changes message sometimes being output before every non-newest-revision change - Make the newest_version logic more robust (and ugly) - Update commit subject Changes in v2: - Add a note when there are no changes in the current revision - Make this the default behaviour, and remove the option tools/patman/README | 21 ++++++++++++++++++++ tools/patman/series.py | 44 +++++++++++++++++++++++++++++++----------- 2 files changed, 54 insertions(+), 11 deletions(-)