[PATCH v3 2/4] patman: Suppress empty changelog entries

[Simon Glass]

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