On 6/10/21 4:07 PM, Tobias Burnus wrote:
(Moved to gcc-patches, missed this when I replied to the initial email)
Regarding patch at: https://gcc.gnu.org/pipermail/gcc/2021-June/236357.html
On 10.06.21 14:45, Jonathan Wakely wrote:
As well as the "contrig" typo that Florian noticed, the subject says
"in in" which should be "is in". And it should be CC'd to gcc-patches.
I like this more than my attempt, however ...
--- a/contrib/gcc-changelog/git_repository.py
+++ b/contrib/gcc-changelog/git_repository.py
@@ -59,8 +59,9 @@ def parse_git_revisions(repo_path, revisions, ref_name=None):
date = datetime.utcfromtimestamp(c.committed_date)
author = '%s <%s>' % (c.author.name, c.author.email)
- git_info = GitInfo(c.hexsha, date, author,
- c.message.split('\n'), modified_files)
+ message = c.message.split('\n')
+ git_info = GitInfo(c.hexsha, date, author, message[0],
+ message[1:], modified_files)
Doesn't using message[1:] here mean that other checks which currently
look at all of self.info.lines will no longer check the subject line?
...
For example, we have:
# Skip Update copyright years commits
if self.info.lines and self.info.lines[0] == 'Update copyright years.':
return
This will never match now, because you've extracted that into the
'subject' instead.
Well, it never matched before for git_email.py, it only did match for
git_repository.py. I think the difference between your work and mine was
that I started with git_email.py – and you started with git_repository.py.
I now pass again the whole message to git_commit.py – also for emails. I
think that's more consistent when checking for an empty line as second line.
I see this approach better.
And for the copyright case, I added a testcase :-)
Aside: We should also have a check that the second line is blank, i.e.
the commit message is a single line subject, followed by blank,
followed by the body. And if we enforced that, then message[2:] would
be better than message[1:].
Added as check – but I pass now all (also subject + '\n' + body) for the
email, which I think it easier to grasp. But as three is always an empty
line for emails (to separate header and body), I cannot add a testcase
for it, unfortunately.
Updated patch enclosed.
I'm sending a small update that handles some flake8 issues and as defined in
setup.cfg,
line limit for the file is 120 characters.
I have one question: Do we really need the revert regex? What about using
GitCommit::revert_commit?
Cheers,
Martin
Tobias
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstrasse 201, 80634 München
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Frank
Thürauf
diff --git a/contrib/gcc-changelog/git_commit.py b/contrib/gcc-changelog/git_commit.py
index 6752c1111a8..cdafb17968c 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -157,8 +157,7 @@ author_line_regex = \
additional_author_regex = re.compile(r'^\t(?P<spaces>\ *)?(?P<name>.* <.*>)')
changelog_regex = re.compile(r'^(?:[fF]or +)?([a-z0-9+-/]*)ChangeLog:?')
subject_pr_skip_regex = re.compile(r'[Rr]evert')
-subject_pr_regex = \
- re.compile(r'(^|\W)PR\s+(?P<component>[a-zA-Z+-]+)/(?P<pr>\d{4,7})')
+subject_pr_regex = re.compile(r'(^|\W)PR\s+(?P<component>[a-zA-Z+-]+)/(?P<pr>\d{4,7})')
subject_pr2_regex = re.compile(r'[(\[]PR\s*(?P<pr>\d{4,7})[)\]]')
pr_regex = re.compile(r'\tPR (?P<component>[a-z+-]+\/)?(?P<pr>[0-9]+)$')
dr_regex = re.compile(r'\tDR ([0-9]+)$')
@@ -312,20 +311,16 @@ class GitCommit:
if self.info.lines and self.info.lines[0] == 'Update copyright years.':
return
- if len(self.info.lines) > 1 and self.info.lines[1] != '':
- self.errors.append(Error('Expected empty second line in commit message',
- info.lines[0]))
+ if len(self.info.lines) > 1 and self.info.lines[1]:
+ self.errors.append(Error('Expected empty second line in commit message', info.lines[0]))
# Extract PR numbers form the subject line
# Match either [PRnnnn] / (PRnnnn) or PR component/nnnn
if self.info.lines and not subject_pr_skip_regex.search(info.lines[0]):
- self.subject_prs = \
- set([m.group('pr')
- for m in subject_pr2_regex.finditer(info.lines[0])])
+ self.subject_prs = {m.group('pr') for m in subject_pr2_regex.finditer(info.lines[0])}
for m in subject_pr_regex.finditer(info.lines[0]):
if not m.group('component') in bug_components:
- self.errors.append(Error('invalid PR component in subject',
- info.lines[0]))
+ self.errors.append(Error('invalid PR component in subject', info.lines[0]))
self.subject_prs.add(m.group('pr'))
# Identify first if the commit is a Revert commit
@@ -368,8 +363,8 @@ class GitCommit:
self.check_mentioned_files()
self.check_for_correct_changelog()
if self.subject_prs:
- self.errors.append(Error("PR %s in subject but not in changelog" %
- ", ".join(self.subject_prs),
+ self.errors.append(Error('PR %s in subject but not in changelog' %
+ ', '.join(self.subject_prs),
self.info.lines[0]))
@property
@@ -487,6 +482,7 @@ class GitCommit:
elif pr_regex.match(line):
m = pr_regex.match(line)
component = m.group('component')
+ pr = m.group('pr')
if not component:
self.errors.append(Error('missing PR component', line))
continue
@@ -495,8 +491,8 @@ class GitCommit:
continue
else:
pr_line = line.lstrip()
- if m.group('pr') in self.subject_prs:
- self.subject_prs.remove(m.group('pr'))
+ if pr in self.subject_prs:
+ self.subject_prs.remove(pr)
elif dr_regex.match(line):
pr_line = line.lstrip()
diff --git a/contrib/gcc-changelog/git_email.py b/contrib/gcc-changelog/git_email.py
index 5fa8947e630..87b419cae5d 100755
--- a/contrib/gcc-changelog/git_email.py
+++ b/contrib/gcc-changelog/git_email.py
@@ -40,7 +40,7 @@ class GitEmail(GitCommit):
diff = PatchSet.from_filename(filename)
date = None
author = None
- subject = ""
+ subject = ''
subject_last = False
with open(self.filename, 'r') as f:
