* [PATCH v3 0/4] patman: Add changelog customization options
@ 2020-05-03 21:55 Sean Anderson
2020-05-03 21:55 ` [PATCH v3 1/4] patman: Modify functional tests for new behavior Sean Anderson
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Sean Anderson @ 2020-05-03 21:55 UTC (permalink / raw)
To: u-boot
This series adds a few changes I have been using locally as new tags for
patman. This series has itself been developed using these patches, and the
unprocessed commits can be viewed at [1].
[1] https://github.com/Forty-Bot/u-boot/tree/patman
Changes in v3:
- Document empty changelog suppression in README
- Fix KeyError when running tests
- Fix some corner cases for no changes messages
- Modify tests for new behavior
- Update commit subjects
Changes in v2:
- Add a note when there are no changes in the current revision
- Add documentation for new tags
- Add patch for multi-line changes in changelogs
- Switch to using commit tags for changelog control, instead of
command-line options
Sean Anderson (4):
patman: Modify functional tests for new behavior
patman: Suppress empty changelog entries
patman: Add new tags for finer-grained changelog control
patman: Support multi-line changes in changelogs
tools/patman/README | 48 +++++++++-
tools/patman/func_test.py | 58 +++++++++--
tools/patman/patchstream.py | 96 +++++++++++++------
tools/patman/patman.py | 2 +-
tools/patman/series.py | 53 +++++++---
.../0001-pci-Correct-cast-for-sandbox.patch | 3 +
...-for-sandbox-in-fdtdec_setup_mem_siz.patch | 12 ++-
tools/patman/test/test01.txt | 15 ++-
8 files changed, 233 insertions(+), 54 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/4] patman: Modify functional tests for new behavior
2020-05-03 21:55 [PATCH v3 0/4] patman: Add changelog customization options Sean Anderson
@ 2020-05-03 21:55 ` Sean Anderson
2020-05-04 14:17 ` Simon Glass
2020-05-03 21:55 ` [PATCH v3 2/4] patman: Suppress empty changelog entries Sean Anderson
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Sean Anderson @ 2020-05-03 21:55 UTC (permalink / raw)
To: u-boot
This patch adds or modifies functional tests for the Cover-changes,
Commit-changes, and Series-process-log tags in order to account for new
behavior added in the next several patches. The '(no changes since v1)'
case is not tested for, since that would need an additional commit to test
in addition to testing the existing code paths.
Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
Changes in v3:
- New
tools/patman/func_test.py | 58 ++++++++++++++++---
.../0001-pci-Correct-cast-for-sandbox.patch | 3 +
...-for-sandbox-in-fdtdec_setup_mem_siz.patch | 12 +++-
tools/patman/test/test01.txt | 15 ++++-
4 files changed, 79 insertions(+), 9 deletions(-)
diff --git a/tools/patman/func_test.py b/tools/patman/func_test.py
index 76319fff37..eedc7f5e18 100644
--- a/tools/patman/func_test.py
+++ b/tools/patman/func_test.py
@@ -85,19 +85,33 @@ class TestFunctional(unittest.TestCase):
Series-prefix: RFC
Series-cc: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
Cover-letter-cc: Lord M?lchett <clergy@palace.gov>
- Series-version: 2
+ Series-version: 3
+ Patch-cc: fred
+ Series-process-log: sort, uniq
Series-changes: 4
- Some changes
+ - Multi
+ line
+ change
+
+ Commit-changes: 2
+ - Changes only for this commit
+
+ Cover-changes: 4
+ - Some notes for the cover letter
Cover-letter:
test: A test patch series
This is a test of how the cover
- leter
+ letter
works
END
and this in the first commit:
+ Commit-changes: 2
+ - second revision change
+
Series-notes:
some notes
about some things
@@ -205,7 +219,7 @@ class TestFunctional(unittest.TestCase):
expected = '''
This is a test of how the cover
-leter
+letter
works
some notes
@@ -213,7 +227,11 @@ about some things
from the first commit
Changes in v4:
+- Multi
+ line
+ change
- Some changes
+- Some notes for the cover letter
Simon Glass (2):
pci: Correct cast for sandbox
@@ -240,8 +258,34 @@ Simon Glass (2):
subject = [line for line in lines if line.startswith('Subject')]
self.assertEqual('Subject: [RFC %d/%d]' % (i + 1, count),
subject[0][:18])
+
+ # Check that we got our commit notes
+ start = 0
+ expected = ''
+
if i == 0:
- # Check that we got our commit notes
- self.assertEqual('---', lines[17])
- self.assertEqual('Some notes about', lines[18])
- self.assertEqual('the first commit', lines[19])
+ start = 17
+ expected = '''---
+Some notes about
+the first commit
+
+(no changes since v2)
+
+Changes in v2:
+- second revision change'''
+ elif i == 1:
+ start = 17
+ expected = '''---
+
+Changes in v4:
+- Multi
+ line
+ change
+- Some changes
+
+Changes in v2:
+- Changes only for this commit'''
+
+ if expected:
+ expected = expected.splitlines()
+ self.assertEqual(expected, lines[start:(start+len(expected))])
diff --git a/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch b/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch
index 7191176f75..038943c2c9 100644
--- a/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch
+++ b/tools/patman/test/0001-pci-Correct-cast-for-sandbox.patch
@@ -15,6 +15,9 @@ cmd/pci.c:152:11: warning: format ?%llx? expects argument of type
Fix it with a cast.
Signed-off-by: Simon Glass <sjg@chromium.org>
+Commit-changes: 2
+- Changes only for this commit
+
Series-notes:
some notes
about some things
diff --git a/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch b/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch
index 702c0306ff..56278a6ce9 100644
--- a/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch
+++ b/tools/patman/test/0002-fdt-Correct-cast-for-sandbox-in-fdtdec_setup_mem_siz.patch
@@ -21,13 +21,23 @@ Series-cc: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
Cover-letter-cc: Lord M?lchett <clergy@palace.gov>
Series-version: 3
Patch-cc: fred
+Series-process-log: sort, uniq
Series-changes: 4
- Some changes
+- Multi
+ line
+ change
+
+Commit-changes: 2
+- Changes only for this commit
+
+Cover-changes: 4
+- Some notes for the cover letter
Cover-letter:
test: A test patch series
This is a test of how the cover
-leter
+letter
works
END
---
diff --git a/tools/patman/test/test01.txt b/tools/patman/test/test01.txt
index 478ea93674..b238a8b4ba 100644
--- a/tools/patman/test/test01.txt
+++ b/tools/patman/test/test01.txt
@@ -13,6 +13,9 @@ Date: Sat Apr 15 15:39:08 2017 -0600
Fix it with a cast.
Signed-off-by: Simon Glass <sjg@chromium.org>
+ Commit-changes: 2
+ - second revision change
+
Series-notes:
some notes
about some things
@@ -45,12 +48,22 @@ Date: Sat Apr 15 15:39:08 2017 -0600
Cover-letter-cc: Lord M?lchett <clergy@palace.gov>
Series-version: 3
Patch-cc: fred
+ Series-process-log: sort, uniq
Series-changes: 4
- Some changes
+ - Multi
+ line
+ change
+
+ Commit-changes: 2
+ - Changes only for this commit
+
+ Cover-changes: 4
+ - Some notes for the cover letter
Cover-letter:
test: A test patch series
This is a test of how the cover
- leter
+ letter
works
END
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 2/4] patman: Suppress empty changelog entries
2020-05-03 21:55 [PATCH v3 0/4] patman: Add changelog customization options Sean Anderson
2020-05-03 21:55 ` [PATCH v3 1/4] patman: Modify functional tests for new behavior Sean Anderson
@ 2020-05-03 21:55 ` Sean Anderson
2020-05-04 14:39 ` Simon Glass
2020-05-03 21:55 ` [PATCH v3 3/4] patman: Add new tags for finer-grained changelog control Sean Anderson
2020-05-03 21:55 ` [PATCH v3 4/4] patman: Support multi-line changes in changelogs Sean Anderson
3 siblings, 1 reply; 11+ messages in thread
From: Sean Anderson @ 2020-05-03 21:55 UTC (permalink / raw)
To: u-boot
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@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(-)
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):
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 3/4] patman: Add new tags for finer-grained changelog control
2020-05-03 21:55 [PATCH v3 0/4] patman: Add changelog customization options Sean Anderson
2020-05-03 21:55 ` [PATCH v3 1/4] patman: Modify functional tests for new behavior Sean Anderson
2020-05-03 21:55 ` [PATCH v3 2/4] patman: Suppress empty changelog entries Sean Anderson
@ 2020-05-03 21:55 ` Sean Anderson
2020-05-04 14:39 ` Simon Glass
2020-05-03 21:55 ` [PATCH v3 4/4] patman: Support multi-line changes in changelogs Sean Anderson
3 siblings, 1 reply; 11+ messages in thread
From: Sean Anderson @ 2020-05-03 21:55 UTC (permalink / raw)
To: u-boot
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. 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 sense 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.
This patch adds two new tags to add changelog entries which only appear in
the cover letter, or only appear in the commit. Changes documented with
"Commit-changes" will only appear in the commit, and will not appear in the
cover letter. Changes documented with "Cover-changes" will not appear in
any commit, and will only appear in the cover letter.
Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
(no changes since v2)
Changes in v2:
- Add documentation for new tags
- Switch to using commit tags for changelog control, instead of
command-line options
tools/patman/README | 17 +++++++++
tools/patman/patchstream.py | 73 ++++++++++++++++++++++---------------
tools/patman/patman.py | 2 +-
tools/patman/series.py | 13 ++++++-
4 files changed, 73 insertions(+), 32 deletions(-)
diff --git a/tools/patman/README b/tools/patman/README
index d1d9891c4c..5a67a49e88 100644
--- a/tools/patman/README
+++ b/tools/patman/README
@@ -247,6 +247,23 @@ Series-changes: n
to update the log there and then, knowing that the script will
do the rest.
+Commit-changes: n
+- This line will not appear in the cover-letter changelog
+<blank line>
+ This tag is like Series-changes, except changes in this changelog will
+ only appear in the changelog of the commit this tag is in. This is
+ useful when you want to add notes which may not make sense in the cover
+ letter. For example, you can have short changes such as "New" or
+ "Lint".
+
+Cover-changes: n
+- This line will only appear in the cover letter
+<blank line>
+ This tag is like Series-changes, except changes in this changelog will
+ only appear in the cover-letter changelog. This is useful to summarize
+ changes made with Commit-changes, or to add additional context to
+ changes.
+
Patch-cc: Their Name <email>
This copies a single patch to another email address. Note that the
Cc: used by git send-email is ignored by patman, but will be
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
index df3eb7483b..f29ad87e70 100644
--- a/tools/patman/patchstream.py
+++ b/tools/patman/patchstream.py
@@ -24,11 +24,8 @@ re_allowed_after_test = re.compile('^Signed-off-by:')
# Signoffs
re_signoff = re.compile('^Signed-off-by: *(.*)')
-# The start of the cover letter
-re_cover = re.compile('^Cover-letter:')
-
-# A cover letter Cc
-re_cover_cc = re.compile('^Cover-letter-cc: *(.*)')
+# Cover letter tag
+re_cover = re.compile('^Cover-([a-z-]*): *(.*)')
# Patch series tag
re_series_tag = re.compile('^Series-([a-z-]*): *(.*)')
@@ -65,7 +62,7 @@ class PatchStream:
def __init__(self, series, 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=
+ self.lines_after_test = 0 # Number of lines found after TEST=
self.warn = [] # List of warnings we have collected
self.linenum = 1 # Output line number we are up to
self.in_section = None # Name of start...END section we are in
@@ -73,7 +70,8 @@ class PatchStream:
self.section = [] # The current section...END section
self.series = series # Info about the patch series
self.is_log = is_log # True if indent like git log
- self.in_change = 0 # Non-zero if we are in a change list
+ self.in_change = None # Name of the change list we are in
+ self.change_version = 0 # Non-zero if we are in a change list
self.blank_count = 0 # Number of blank lines stored up
self.state = STATE_MSG_HEADER # What state are we in?
self.signoff = [] # Contents of signoff line
@@ -124,6 +122,14 @@ class PatchStream:
self.skip_blank = True
self.section = []
+ def ParseVersion(self, value, line):
+ """Parse a version from a *-changes tag"""
+ try:
+ return int(value)
+ except ValueError as str:
+ raise ValueError("%s: Cannot decode version info '%s'" %
+ (self.commit.hash, line))
+
def ProcessLine(self, line):
"""Process a single line of a patch file or commit log
@@ -163,7 +169,6 @@ class PatchStream:
change_id_match = re_change_id.match(line)
commit_tag_match = re_commit_tag.match(line)
cover_match = re_cover.match(line)
- cover_cc_match = re_cover_cc.match(line)
signoff_match = re_signoff.match(line)
tag_match = None
if self.state == STATE_PATCH_HEADER:
@@ -183,8 +188,7 @@ class PatchStream:
# If a tag is detected, or a new commit starts
if series_tag_match or commit_tag_match or change_id_match or \
- cover_match or cover_cc_match or signoff_match or \
- self.state == STATE_MSG_HEADER:
+ cover_match or signoff_match or self.state == STATE_MSG_HEADER:
# but we are already in a section, this means 'END' is missing
# for that section, fix it up.
if self.in_section:
@@ -205,8 +209,9 @@ class PatchStream:
# but we are already in a change list, that means a blank line
# is missing, fix it up.
if self.in_change:
- self.warn.append("Missing 'blank line' in section 'Series-changes'")
- self.in_change = 0
+ self.warn.append("Missing 'blank line' in section '%s-changes'" % self.in_change)
+ self.in_change = None
+ self.change_version = 0
# If we are in a section, keep collecting lines until we see END
if self.in_section:
@@ -242,26 +247,37 @@ class PatchStream:
elif self.skip_blank and is_blank:
self.skip_blank = False
- # Detect the start of a cover letter section
+ # Detect Cover-xxx tags
elif cover_match:
- self.in_section = 'cover'
- self.skip_blank = False
-
- elif cover_cc_match:
- value = cover_cc_match.group(1)
- self.AddToSeries(line, 'cover-cc', value)
+ name = cover_match.group(1)
+ value = cover_match.group(2)
+ if name == 'letter':
+ self.in_section = 'cover'
+ self.skip_blank = False
+ elif name == 'letter-cc':
+ self.AddToSeries(line, 'cover-cc', value)
+ elif name == 'changes':
+ self.in_change = 'Cover'
+ self.change_version = self.ParseVersion(value, line)
# If we are in a change list, key collected lines until a blank one
elif self.in_change:
if is_blank:
# Blank line ends this change list
- self.in_change = 0
+ self.in_change = None
+ self.change_version = 0
elif line == '---':
- self.in_change = 0
+ self.in_change = None
+ self.change_version = 0
out = self.ProcessLine(line)
else:
if self.is_log:
- self.series.AddChange(self.in_change, self.commit, line)
+ if self.in_change == 'Series':
+ self.series.AddChange(self.change_version, self.commit, line)
+ elif self.in_change == 'Cover':
+ self.series.AddChange(self.change_version, None, line)
+ elif self.in_change == 'Commit':
+ self.commit.AddChange(self.change_version, line)
self.skip_blank = False
# Detect Series-xxx tags
@@ -270,12 +286,8 @@ class PatchStream:
value = series_tag_match.group(2)
if name == 'changes':
# value is the version number: e.g. 1, or 2
- try:
- value = int(value)
- except ValueError as str:
- raise ValueError("%s: Cannot decode version info '%s'" %
- (self.commit.hash, line))
- self.in_change = int(value)
+ self.in_change = 'Series'
+ self.change_version = self.ParseVersion(value, line)
else:
self.AddToSeries(line, name, value)
self.skip_blank = True
@@ -297,6 +309,9 @@ class PatchStream:
if name == 'notes':
self.AddToCommit(line, name, value)
self.skip_blank = True
+ elif name == 'changes':
+ self.in_change = 'Commit'
+ self.change_version = self.ParseVersion(value, line)
# Detect the start of a new commit
elif commit_match:
@@ -340,7 +355,7 @@ 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)
out += [line]
diff --git a/tools/patman/patman.py b/tools/patman/patman.py
index cf53e532dd..6ee0597ff4 100755
--- a/tools/patman/patman.py
+++ b/tools/patman/patman.py
@@ -61,7 +61,7 @@ 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('--smtp-server', type='str',
help="Specify the SMTP server to 'git send-email'")
parser.add_option('-T', '--thread', action='store_true', dest='thread',
diff --git a/tools/patman/series.py b/tools/patman/series.py
index 4359442174..cc74284f54 100644
--- a/tools/patman/series.py
+++ b/tools/patman/series.py
@@ -4,6 +4,7 @@
from __future__ import print_function
+import collections
import itertools
import os
@@ -158,7 +159,15 @@ class Series(dict):
- Fix the widget
- Jog the dial
"""
- versions = sorted(self.changes, reverse=True)
+ # Collect changes from the series and this commit
+ changes = collections.defaultdict(list)
+ for version, changelist in self.changes.items():
+ changes[version] += changelist
+ if commit:
+ for version, changelist in commit.changes.items():
+ changes[version] += [[commit, text] for text in changelist]
+
+ versions = sorted(changes, reverse=True)
newest_version = 1
try:
newest_version = max(newest_version, int(self.version))
@@ -175,7 +184,7 @@ class Series(dict):
need_blank = False
for version in versions:
out = []
- for this_commit, text in self.changes[version]:
+ for this_commit, text in changes[version]:
if commit and this_commit != commit:
continue
if 'uniq' not in process_it or text not in out:
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 4/4] patman: Support multi-line changes in changelogs
2020-05-03 21:55 [PATCH v3 0/4] patman: Add changelog customization options Sean Anderson
` (2 preceding siblings ...)
2020-05-03 21:55 ` [PATCH v3 3/4] patman: Add new tags for finer-grained changelog control Sean Anderson
@ 2020-05-03 21:55 ` Sean Anderson
2020-05-04 14:39 ` Simon Glass
3 siblings, 1 reply; 11+ messages in thread
From: Sean Anderson @ 2020-05-03 21:55 UTC (permalink / raw)
To: u-boot
This patch adds support to multi-line changes. That is, if one has a line
in a changelog like
- Do a thing but
it spans multiple lines
Using Series-process-log sort would sort as if those lines were unrelated.
With this patch, any change line starting with whitespace will be
considered part of the change before it.
Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
(no changes since v2)
Changes in v2:
- New
tools/patman/README | 10 ++++++++--
tools/patman/patchstream.py | 35 +++++++++++++++++++++++++++--------
2 files changed, 35 insertions(+), 10 deletions(-)
diff --git a/tools/patman/README b/tools/patman/README
index 5a67a49e88..155736568c 100644
--- a/tools/patman/README
+++ b/tools/patman/README
@@ -270,8 +270,14 @@ Patch-cc: Their Name <email>
interpreted by git send-email if you use it.
Series-process-log: sort, uniq
- This tells patman to sort and/or uniq the change logs. It is
- assumed that each change log entry is only a single line long.
+ This tells patman to sort and/or uniq the change logs. Changes may be
+ multiple lines long, as long as each subsequent line of a change begins
+ with a whitespace character. For example,
+
+- This change
+ continues onto the next line
+- But this change is separate
+
Use 'sort' to sort the entries, and 'uniq' to include only
unique entries. If omitted, no change log processing is done.
Separate each tag with a comma.
diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
index f29ad87e70..eeac5d268e 100644
--- a/tools/patman/patchstream.py
+++ b/tools/patman/patchstream.py
@@ -45,6 +45,9 @@ re_commit = re.compile('^commit ([0-9a-f]*)$')
# We detect these since checkpatch doesn't always do it
re_space_before_tab = re.compile('^[+].* \t')
+# Match indented lines for changes
+re_leading_whitespace = re.compile('^\s')
+
# States we can be in - can we use range() and still have comments?
STATE_MSG_HEADER = 0 # Still in the message header
STATE_PATCH_SUBJECT = 1 # In patch subject (first line of log for a commit)
@@ -72,6 +75,7 @@ class PatchStream:
self.is_log = is_log # True if indent like git log
self.in_change = None # Name of the change list we are in
self.change_version = 0 # Non-zero if we are in a change list
+ self.change_lines = [] # Lines of the current change
self.blank_count = 0 # Number of blank lines stored up
self.state = STATE_MSG_HEADER # What state are we in?
self.signoff = [] # Contents of signoff line
@@ -130,6 +134,20 @@ class PatchStream:
raise ValueError("%s: Cannot decode version info '%s'" %
(self.commit.hash, line))
+ def FinalizeChange(self):
+ """Finalize a (multi-line) change and add it to the series or commit"""
+ if not self.change_lines:
+ return
+ change = '\n'.join(self.change_lines)
+
+ if self.in_change == 'Series':
+ self.series.AddChange(self.change_version, self.commit, change)
+ elif self.in_change == 'Cover':
+ self.series.AddChange(self.change_version, None, change)
+ elif self.in_change == 'Commit':
+ self.commit.AddChange(self.change_version, change)
+ self.change_lines = []
+
def ProcessLine(self, line):
"""Process a single line of a patch file or commit log
@@ -170,6 +188,7 @@ class PatchStream:
commit_tag_match = re_commit_tag.match(line)
cover_match = re_cover.match(line)
signoff_match = re_signoff.match(line)
+ leading_whitespace_match = re_leading_whitespace.match(line)
tag_match = None
if self.state == STATE_PATCH_HEADER:
tag_match = re_tag.match(line)
@@ -210,6 +229,7 @@ class PatchStream:
# is missing, fix it up.
if self.in_change:
self.warn.append("Missing 'blank line' in section '%s-changes'" % self.in_change)
+ self.FinalizeChange()
self.in_change = None
self.change_version = 0
@@ -264,20 +284,18 @@ class PatchStream:
elif self.in_change:
if is_blank:
# Blank line ends this change list
+ self.FinalizeChange()
self.in_change = None
self.change_version = 0
elif line == '---':
+ self.FinalizeChange()
self.in_change = None
self.change_version = 0
out = self.ProcessLine(line)
- else:
- if self.is_log:
- if self.in_change == 'Series':
- self.series.AddChange(self.change_version, self.commit, line)
- elif self.in_change == 'Cover':
- self.series.AddChange(self.change_version, None, line)
- elif self.in_change == 'Commit':
- self.commit.AddChange(self.change_version, line)
+ elif self.is_log:
+ if not leading_whitespace_match:
+ self.FinalizeChange()
+ self.change_lines.append(line)
self.skip_blank = False
# Detect Series-xxx tags
@@ -370,6 +388,7 @@ class PatchStream:
def Finalize(self):
"""Close out processing of this patch stream"""
+ self.FinalizeChange()
self.CloseCommit()
if self.lines_after_test:
self.warn.append('Found %d lines after TEST=' %
--
2.26.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 1/4] patman: Modify functional tests for new behavior
2020-05-03 21:55 ` [PATCH v3 1/4] patman: Modify functional tests for new behavior Sean Anderson
@ 2020-05-04 14:17 ` Simon Glass
0 siblings, 0 replies; 11+ messages in thread
From: Simon Glass @ 2020-05-04 14:17 UTC (permalink / raw)
To: u-boot
Hi Sean,
On Sun, 3 May 2020 at 15:55, Sean Anderson <seanga2@gmail.com> wrote:
>
> This patch adds or modifies functional tests for the Cover-changes,
> Commit-changes, and Series-process-log tags in order to account for new
> behavior added in the next several patches. The '(no changes since v1)'
> case is not tested for, since that would need an additional commit to test
> in addition to testing the existing code paths.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> Changes in v3:
> - New
>
> tools/patman/func_test.py | 58 ++++++++++++++++---
> .../0001-pci-Correct-cast-for-sandbox.patch | 3 +
> ...-for-sandbox-in-fdtdec_setup_mem_siz.patch | 12 +++-
> tools/patman/test/test01.txt | 15 ++++-
> 4 files changed, 79 insertions(+), 9 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
But this patch should come last since otherwise it break bisectability.
I should be able to do that when applying.
Regards,
Simon
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 4/4] patman: Support multi-line changes in changelogs
2020-05-03 21:55 ` [PATCH v3 4/4] patman: Support multi-line changes in changelogs Sean Anderson
@ 2020-05-04 14:39 ` Simon Glass
0 siblings, 0 replies; 11+ messages in thread
From: Simon Glass @ 2020-05-04 14:39 UTC (permalink / raw)
To: u-boot
On Sun, 3 May 2020 at 15:55, Sean Anderson <seanga2@gmail.com> wrote:
>
> This patch adds support to multi-line changes. That is, if one has a line
> in a changelog like
> - Do a thing but
> it spans multiple lines
> Using Series-process-log sort would sort as if those lines were unrelated.
> With this patch, any change line starting with whitespace will be
> considered part of the change before it.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - New
>
> tools/patman/README | 10 ++++++++--
> tools/patman/patchstream.py | 35 +++++++++++++++++++++++++++--------
> 2 files changed, 35 insertions(+), 10 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 2/4] patman: Suppress empty changelog entries
2020-05-03 21:55 ` [PATCH v3 2/4] patman: Suppress empty changelog entries Sean Anderson
@ 2020-05-04 14:39 ` Simon Glass
2020-05-04 16:59 ` Sean Anderson
0 siblings, 1 reply; 11+ messages in thread
From: Simon Glass @ 2020-05-04 14:39 UTC (permalink / raw)
To: u-boot
Hi Sean,
On Sun, 3 May 2020 at 15:55, Sean Anderson <seanga2@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@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@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 3/4] patman: Add new tags for finer-grained changelog control
2020-05-03 21:55 ` [PATCH v3 3/4] patman: Add new tags for finer-grained changelog control Sean Anderson
@ 2020-05-04 14:39 ` Simon Glass
0 siblings, 0 replies; 11+ messages in thread
From: Simon Glass @ 2020-05-04 14:39 UTC (permalink / raw)
To: u-boot
HI Sean,
On Sun, 3 May 2020 at 15:55, Sean Anderson <seanga2@gmail.com> wrote:
>
> By default patman generates a combined changelog for the cover letter. This
> may not always be desireable.
desirable
>
> Many patches may have the same changes. These can be coalesced with
> "Series-process-log: uniq", but this is imperfect. 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 sense 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
makes
> changed, without making it difficult to wade through every change in the
> whole series.
>
> This patch adds two new tags to add changelog entries which only appear in
> the cover letter, or only appear in the commit. Changes documented with
> "Commit-changes" will only appear in the commit, and will not appear in the
> cover letter. Changes documented with "Cover-changes" will not appear in
> any commit, and will only appear in the cover letter.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
>
> (no changes since v2)
>
> Changes in v2:
> - Add documentation for new tags
> - Switch to using commit tags for changelog control, instead of
> command-line options
>
> tools/patman/README | 17 +++++++++
> tools/patman/patchstream.py | 73 ++++++++++++++++++++++---------------
> tools/patman/patman.py | 2 +-
> tools/patman/series.py | 13 ++++++-
> 4 files changed, 73 insertions(+), 32 deletions(-)
Reviewed-by: Simon Glass <sjg@chromium.org>
(with fixes added)
Also please can you rebase on mainline as there is a minor conflict in series.py
>
> diff --git a/tools/patman/README b/tools/patman/README
> index d1d9891c4c..5a67a49e88 100644
> --- a/tools/patman/README
> +++ b/tools/patman/README
> @@ -247,6 +247,23 @@ Series-changes: n
> to update the log there and then, knowing that the script will
> do the rest.
>
> +Commit-changes: n
> +- This line will not appear in the cover-letter changelog
> +<blank line>
> + This tag is like Series-changes, except changes in this changelog will
> + only appear in the changelog of the commit this tag is in. This is
> + useful when you want to add notes which may not make sense in the cover
> + letter. For example, you can have short changes such as "New" or
> + "Lint".
> +
> +Cover-changes: n
> +- This line will only appear in the cover letter
> +<blank line>
> + This tag is like Series-changes, except changes in this changelog will
> + only appear in the cover-letter changelog. This is useful to summarize
> + changes made with Commit-changes, or to add additional context to
> + changes.
> +
> Patch-cc: Their Name <email>
> This copies a single patch to another email address. Note that the
> Cc: used by git send-email is ignored by patman, but will be
> diff --git a/tools/patman/patchstream.py b/tools/patman/patchstream.py
> index df3eb7483b..f29ad87e70 100644
> --- a/tools/patman/patchstream.py
> +++ b/tools/patman/patchstream.py
> @@ -24,11 +24,8 @@ re_allowed_after_test = re.compile('^Signed-off-by:')
> # Signoffs
> re_signoff = re.compile('^Signed-off-by: *(.*)')
>
> -# The start of the cover letter
> -re_cover = re.compile('^Cover-letter:')
> -
> -# A cover letter Cc
> -re_cover_cc = re.compile('^Cover-letter-cc: *(.*)')
> +# Cover letter tag
> +re_cover = re.compile('^Cover-([a-z-]*): *(.*)')
>
> # Patch series tag
> re_series_tag = re.compile('^Series-([a-z-]*): *(.*)')
> @@ -65,7 +62,7 @@ class PatchStream:
> def __init__(self, series, 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=
> + self.lines_after_test = 0 # Number of lines found after TEST=
> self.warn = [] # List of warnings we have collected
> self.linenum = 1 # Output line number we are up to
> self.in_section = None # Name of start...END section we are in
> @@ -73,7 +70,8 @@ class PatchStream:
> self.section = [] # The current section...END section
> self.series = series # Info about the patch series
> self.is_log = is_log # True if indent like git log
> - self.in_change = 0 # Non-zero if we are in a change list
> + self.in_change = None # Name of the change list we are in
> + self.change_version = 0 # Non-zero if we are in a change list
> self.blank_count = 0 # Number of blank lines stored up
> self.state = STATE_MSG_HEADER # What state are we in?
> self.signoff = [] # Contents of signoff line
> @@ -124,6 +122,14 @@ class PatchStream:
> self.skip_blank = True
> self.section = []
>
> + def ParseVersion(self, value, line):
> + """Parse a version from a *-changes tag"""
Args:
Returns:
> + try:
> + return int(value)
> + except ValueError as str:
> + raise ValueError("%s: Cannot decode version info '%s'" %
> + (self.commit.hash, line))
> +
[..]
Regards,
Simon
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 2/4] patman: Suppress empty changelog entries
2020-05-04 14:39 ` Simon Glass
@ 2020-05-04 16:59 ` Sean Anderson
2020-05-04 19:26 ` Simon Glass
0 siblings, 1 reply; 11+ messages in thread
From: Sean Anderson @ 2020-05-04 16:59 UTC (permalink / raw)
To: u-boot
On 5/4/20 10:39 AM, Simon Glass wrote:
> Hi Sean,
>
> On Sun, 3 May 2020 at 15:55, Sean Anderson <seanga2@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@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@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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 2/4] patman: Suppress empty changelog entries
2020-05-04 16:59 ` Sean Anderson
@ 2020-05-04 19:26 ` Simon Glass
0 siblings, 0 replies; 11+ messages in thread
From: Simon Glass @ 2020-05-04 19:26 UTC (permalink / raw)
To: u-boot
Hi Sean,
On Mon, 4 May 2020 at 10:59, Sean Anderson <seanga2@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@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@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@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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-05-04 19:26 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-03 21:55 [PATCH v3 0/4] patman: Add changelog customization options Sean Anderson
2020-05-03 21:55 ` [PATCH v3 1/4] patman: Modify functional tests for new behavior Sean Anderson
2020-05-04 14:17 ` Simon Glass
2020-05-03 21:55 ` [PATCH v3 2/4] patman: Suppress empty changelog entries Sean Anderson
2020-05-04 14:39 ` Simon Glass
2020-05-04 16:59 ` Sean Anderson
2020-05-04 19:26 ` Simon Glass
2020-05-03 21:55 ` [PATCH v3 3/4] patman: Add new tags for finer-grained changelog control Sean Anderson
2020-05-04 14:39 ` Simon Glass
2020-05-03 21:55 ` [PATCH v3 4/4] patman: Support multi-line changes in changelogs Sean Anderson
2020-05-04 14:39 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.