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

[Sean Anderson]

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