RE: [IMPORTANT] ChangeLog related changes
Thanks both! Cheers, Tamar > -Original Message- > From: Martin Liška > Sent: Wednesday, June 10, 2020 2:41 PM > To: Tamar Christina ; Jonathan Wakely > > Cc: Jakub Jelinek ; g...@gcc.gnu.org; gcc-patches patc...@gcc.gnu.org> > Subject: Re: [IMPORTANT] ChangeLog related changes > > On 6/10/20 3:34 PM, Tamar Christina wrote: > > Hi All, > > > > Hello. > > > We've been wondering since we no longer list authors in the changelog > > (at least mklog doesn't generate it), > > You are right, it's preferred solution and it's documented here: > https://gcc.gnu.org/codingconventions.html#ChangeLogs > > ''' > a commit author and committer date stamp can be automatically deduced > from a git commit - we recommend to use it ''' > > but we miss to document that additional authors are automatically taken > from: > Co-Authored-By: > > I'll document that. > > Martin > > > How do we handle multi author patches nowadays? > > > > Tried searching for it on the website but couldn’t find anything. > > > > Thanks, > > Tamar > > > >> -Original Message- > >> From: Gcc-patches On Behalf Of > >> Martin Liška > >> Sent: Wednesday, June 10, 2020 8:38 AM > >> To: Jonathan Wakely > >> Cc: Jakub Jelinek ; g...@gcc.gnu.org; gcc-patches > >> > >> Subject: Re: [IMPORTANT] ChangeLog related changes > >> > >> On 6/9/20 10:29 PM, Jonathan Wakely wrote: > >>> OK, here's a proper patch for the changes you liked, dropping the > >>> changes to the Error exception type. > >>> > >>> pytest contrib/gcc-changelog/test_email.py passes. > >>> > >>> OK for master? > >> > >> I like it and I've just pushed the patch to master. > >> > >> Martin
Re: [IMPORTANT] ChangeLog related changes
On 6/10/20 3:34 PM, Tamar Christina wrote: Hi All, Hello. We've been wondering since we no longer list authors in the changelog (at least mklog doesn't generate it), You are right, it's preferred solution and it's documented here: https://gcc.gnu.org/codingconventions.html#ChangeLogs ''' a commit author and committer date stamp can be automatically deduced from a git commit - we recommend to use it ''' but we miss to document that additional authors are automatically taken from: Co-Authored-By: I'll document that. Martin How do we handle multi author patches nowadays? Tried searching for it on the website but couldn’t find anything. Thanks, Tamar -Original Message- From: Gcc-patches On Behalf Of Martin Liška Sent: Wednesday, June 10, 2020 8:38 AM To: Jonathan Wakely Cc: Jakub Jelinek ; g...@gcc.gnu.org; gcc-patches Subject: Re: [IMPORTANT] ChangeLog related changes On 6/9/20 10:29 PM, Jonathan Wakely wrote: OK, here's a proper patch for the changes you liked, dropping the changes to the Error exception type. pytest contrib/gcc-changelog/test_email.py passes. OK for master? I like it and I've just pushed the patch to master. Martin
Re: [IMPORTANT] ChangeLog related changes
On Wed, Jun 10, 2020 at 01:34:54PM +, Tamar Christina wrote: > Hi All, > > We've been wondering since we no longer list authors in the changelog (at > least mklog doesn't generate it), > How do we handle multi author patches nowadays? > > Tried searching for it on the website but couldn’t find anything. You can add Co-authored-by: name to your commit. If we don't already document it, we should. Marek
RE: [IMPORTANT] ChangeLog related changes
Hi All, We've been wondering since we no longer list authors in the changelog (at least mklog doesn't generate it), How do we handle multi author patches nowadays? Tried searching for it on the website but couldn’t find anything. Thanks, Tamar > -Original Message- > From: Gcc-patches On Behalf Of Martin > Liška > Sent: Wednesday, June 10, 2020 8:38 AM > To: Jonathan Wakely > Cc: Jakub Jelinek ; g...@gcc.gnu.org; gcc-patches patc...@gcc.gnu.org> > Subject: Re: [IMPORTANT] ChangeLog related changes > > On 6/9/20 10:29 PM, Jonathan Wakely wrote: > > OK, here's a proper patch for the changes you liked, dropping the > > changes to the Error exception type. > > > > pytest contrib/gcc-changelog/test_email.py passes. > > > > OK for master? > > I like it and I've just pushed the patch to master. > > Martin
Re: [IMPORTANT] ChangeLog related changes
On 6/9/20 10:29 PM, Jonathan Wakely wrote: OK, here's a proper patch for the changes you liked, dropping the changes to the Error exception type. pytest contrib/gcc-changelog/test_email.py passes. OK for master? I like it and I've just pushed the patch to master. Martin
Re: [IMPORTANT] ChangeLog related changes
On Tue, 2 Jun 2020 at 15:25, Martin Liška wrote:
>
> On 6/2/20 4:14 PM, Jonathan Wakely wrote:
> > On Tue, 2 Jun 2020 at 14:56, Jonathan Wakely wrote:
> >>
> >> On Tue, 2 Jun 2020 at 14:16, Martin Liška wrote:
> >>>
> >>> On 6/2/20 1:48 PM, Martin Liška wrote:
> I tend to this approach. Let me prepare a patch candidate for it.
> >>>
> >>> There's a patch for it. Can you please Jonathan take a look?
> >>
> >> Looks great, thanks!
> >>
> >> +if name not in wildcard_prefixes:
> >> +msg = 'unsupported wilcard prefix'
> >>
> >> Typo "wilcard"
> >>
> >> +if pattern not in used_patterns:
> >> +self.errors.append(Error('a file pattern not used in a
> >> patch',
> >> + pattern))
> >>
> >> If the script printed this error I don't think I'd know what it was
> >> complaining about. How about "pattern doesn't match any changed files"
> >> ?
> >>
> >> I find the existing error 'file not changed in a patch' to be
> >> suboptimal too. We're checking revisions (or commits), not patches.
> >
> > Along those lines, here's an incomplete patch (tests aren't updated
> > yet, no commit log, your latest change isn't merged ) to revise the
> > error messages. I've tried to make them more consistent (e.g change
> > 'file not changed in a patch' to 'unchanged file mentioned in a
> > ChangeLog' which mirrors the error for the opposite condition,
> > 'changed file not mentioned in a ChangeLog').
>
> I welcome this.
>
> >
> > I've also moved line numbers to the start of the line (like GCC's own
> > diagnostics) and not used the "line" argument of the Error constructor
> > for things that aren't line numbers. I've aimed to be consistent, so
> > that line numbers come at the start, pathnames and patterns come at
> > the end (and there's a space before them).
>
> Well, the Error ctor argument 'line' is bit misleading. It's a string and
> not an integer:
>
>File "/home/marxin/Programming/gcc/contrib/gcc-changelog/git_commit.py",
> line 177, in __repr__
> return '%d: %s' % (self.line, self.message)
> TypeError: %d format: a number is required, not str
>
> and it represents a line from a git commit message. I use it in order to
> provide
> line which is affected by an error.
>
> >
> > I also suggest changing 'additional author must prepend with tab and 4
> > spaces' to 'additional author must be indented with one tab and four
> > spaces'.>
> > The intent is to improve the user experience, since for many people
> > who are new to git *any* error can cause confusion, so extra,
> > gcc-specific errors that we issue should aim to be easy to understand.
>
> I like your wordings.
OK, here's a proper patch for the changes you liked, dropping the
changes to the Error exception type.
pytest contrib/gcc-changelog/test_email.py passes.
OK for master?
commit 49652b7f5b57b88c1e0e27cf8ac488cbc85f1c7d
Author: Jonathan Wakely
Date: Tue Jun 9 21:25:50 2020 +0100
gcc-changelog: Improve git_commit.py diagnostics
This changes some error messages to be more self-consistent and to fix
some grammar.
contrib/ChangeLog:
* gcc-changelog/git_commit.py (GitCommit.parse_changelog):
Improve error strings.
* gcc-changelog/test_email.py: Update expected errors.
diff --git a/contrib/gcc-changelog/git_commit.py
b/contrib/gcc-changelog/git_commit.py
index f85d4c83c63..0b350ba7fda 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -377,8 +377,8 @@ class GitCommit:
elif additional_author_regex.match(line):
m = additional_author_regex.match(line)
if len(m.group('spaces')) != 4:
-msg = 'additional author must prepend with tab ' \
- 'and 4 spaces'
+msg = 'additional author must be indented with '\
+ 'one tab and four spaces'
self.errors.append(Error(msg, line))
else:
author_tuple = (m.group('name'), None)
@@ -438,15 +438,14 @@ class GitCommit:
m = star_prefix_regex.match(line)
if m:
if len(m.group('spaces')) != 1:
-err = Error('one space should follow asterisk',
-line)
-self.errors.append(err)
+msg = 'one space should follow asterisk'
+self.errors.append(Error(msg, line))
else:
last_entry.lines.append(line)
else:
if last_entry.is_empty:
msg = 'first line should start with a tab, ' \
- 'asterisk and space'
+
Re: [IMPORTANT] ChangeLog related changes
On 6/2/20 4:14 PM, Jonathan Wakely wrote:
On Tue, 2 Jun 2020 at 14:56, Jonathan Wakely wrote:
On Tue, 2 Jun 2020 at 14:16, Martin Liška wrote:
On 6/2/20 1:48 PM, Martin Liška wrote:
I tend to this approach. Let me prepare a patch candidate for it.
There's a patch for it. Can you please Jonathan take a look?
Looks great, thanks!
+if name not in wildcard_prefixes:
+msg = 'unsupported wilcard prefix'
Typo "wilcard"
+if pattern not in used_patterns:
+self.errors.append(Error('a file pattern not used in a patch',
+ pattern))
If the script printed this error I don't think I'd know what it was
complaining about. How about "pattern doesn't match any changed files"
?
I find the existing error 'file not changed in a patch' to be
suboptimal too. We're checking revisions (or commits), not patches.
Along those lines, here's an incomplete patch (tests aren't updated
yet, no commit log, your latest change isn't merged ) to revise the
error messages. I've tried to make them more consistent (e.g change
'file not changed in a patch' to 'unchanged file mentioned in a
ChangeLog' which mirrors the error for the opposite condition,
'changed file not mentioned in a ChangeLog').
I welcome this.
I've also moved line numbers to the start of the line (like GCC's own
diagnostics) and not used the "line" argument of the Error constructor
for things that aren't line numbers. I've aimed to be consistent, so
that line numbers come at the start, pathnames and patterns come at
the end (and there's a space before them).
Well, the Error ctor argument 'line' is bit misleading. It's a string and
not an integer:
File "/home/marxin/Programming/gcc/contrib/gcc-changelog/git_commit.py", line
177, in __repr__
return '%d: %s' % (self.line, self.message)
TypeError: %d format: a number is required, not str
and it represents a line from a git commit message. I use it in order to provide
line which is affected by an error.
I also suggest changing 'additional author must prepend with tab and 4
spaces' to 'additional author must be indented with one tab and four
spaces'.>
The intent is to improve the user experience, since for many people
who are new to git *any* error can cause confusion, so extra,
gcc-specific errors that we issue should aim to be easy to understand.
I like your wordings.
Martin
Do you like the direction?
Re: [IMPORTANT] ChangeLog related changes
On Tue, 2 Jun 2020 at 14:56, Jonathan Wakely wrote:
>
> On Tue, 2 Jun 2020 at 14:16, Martin Liška wrote:
> >
> > On 6/2/20 1:48 PM, Martin Liška wrote:
> > > I tend to this approach. Let me prepare a patch candidate for it.
> >
> > There's a patch for it. Can you please Jonathan take a look?
>
> Looks great, thanks!
>
> +if name not in wildcard_prefixes:
> +msg = 'unsupported wilcard prefix'
>
> Typo "wilcard"
>
> +if pattern not in used_patterns:
> +self.errors.append(Error('a file pattern not used in a
> patch',
> + pattern))
>
> If the script printed this error I don't think I'd know what it was
> complaining about. How about "pattern doesn't match any changed files"
> ?
>
> I find the existing error 'file not changed in a patch' to be
> suboptimal too. We're checking revisions (or commits), not patches.
Along those lines, here's an incomplete patch (tests aren't updated
yet, no commit log, your latest change isn't merged ) to revise the
error messages. I've tried to make them more consistent (e.g change
'file not changed in a patch' to 'unchanged file mentioned in a
ChangeLog' which mirrors the error for the opposite condition,
'changed file not mentioned in a ChangeLog').
I've also moved line numbers to the start of the line (like GCC's own
diagnostics) and not used the "line" argument of the Error constructor
for things that aren't line numbers. I've aimed to be consistent, so
that line numbers come at the start, pathnames and patterns come at
the end (and there's a space before them).
I also suggest changing 'additional author must prepend with tab and 4
spaces' to 'additional author must be indented with one tab and four
spaces'.
The intent is to improve the user experience, since for many people
who are new to git *any* error can cause confusion, so extra,
gcc-specific errors that we issue should aim to be easy to understand.
Do you like the direction?
diff --git a/contrib/gcc-changelog/git_commit.py
b/contrib/gcc-changelog/git_commit.py
index 069900d7cbf..453f0f61803 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -173,10 +173,9 @@ class Error:
self.line = line
def __repr__(self):
-s = self.message
if self.line:
-s += ':"%s"' % self.line
-return s
+return '%d: %s' % (self.line, self.message)
+return self.message
class ChangeLogEntry:
@@ -376,8 +375,8 @@ class GitCommit:
elif additional_author_regex.match(line):
m = additional_author_regex.match(line)
if len(m.group('spaces')) != 4:
-msg = 'additional author must prepend with tab ' \
- 'and 4 spaces'
+msg = 'additional author must be indented with '\
+ ' one tab and four spaces'
self.errors.append(Error(msg, line))
else:
author_tuple = (m.group('name'), None)
@@ -437,15 +436,14 @@ class GitCommit:
m = star_prefix_regex.match(line)
if m:
if len(m.group('spaces')) != 1:
-err = Error('one space should follow asterisk',
-line)
-self.errors.append(err)
+msg = 'one space should follow asterisk'
+self.errors.append(Error(msg, line))
else:
last_entry.lines.append(line)
else:
if last_entry.is_empty:
msg = 'first line should start with a tab, ' \
- 'asterisk and space'
+ 'an asterisk and a space'
self.errors.append(Error(msg, line))
else:
last_entry.lines.append(line)
@@ -526,8 +524,8 @@ class GitCommit:
used_patterns = set()
for entry in self.changelog_entries:
if not entry.files:
-msg = 'ChangeLog must contain at least one file entry'
-self.errors.append(Error(msg, entry.folder))
+msg = 'no files mentioned for ChangeLog in directory: "%s"'
+self.errors.append(Error(msg % entry.folder))
assert not entry.folder.endswith('/')
for file in entry.files:
if not self.is_changelog_filename(file):
@@ -539,7 +537,8 @@ class GitCommit:
if not self.is_changelog_filename(x[0])]
changed_files = set(cand)
for file in sorted(mentioned_files - changed_files):
-self.errors.append(Error('file not changed in a patch', file))
+msg
Re: [IMPORTANT] ChangeLog related changes
On 6/2/20 3:56 PM, Jonathan Wakely wrote:
On Tue, 2 Jun 2020 at 14:16, Martin Liška wrote:
On 6/2/20 1:48 PM, Martin Liška wrote:
I tend to this approach. Let me prepare a patch candidate for it.
There's a patch for it. Can you please Jonathan take a look?
Looks great, thanks!
+if name not in wildcard_prefixes:
+msg = 'unsupported wilcard prefix'
Typo "wilcard"
+if pattern not in used_patterns:
+self.errors.append(Error('a file pattern not used in a patch',
+ pattern))
If the script printed this error I don't think I'd know what it was
complaining about. How about "pattern doesn't match any changed files"
?
Yes, thank you for the corrections. I've just pushed that to master.
Martin
I find the existing error 'file not changed in a patch' to be
suboptimal too. We're checking revisions (or commits), not patches.
Re: [IMPORTANT] ChangeLog related changes
On Tue, 2 Jun 2020 at 14:16, Martin Liška wrote:
>
> On 6/2/20 1:48 PM, Martin Liška wrote:
> > I tend to this approach. Let me prepare a patch candidate for it.
>
> There's a patch for it. Can you please Jonathan take a look?
Looks great, thanks!
+if name not in wildcard_prefixes:
+msg = 'unsupported wilcard prefix'
Typo "wilcard"
+if pattern not in used_patterns:
+self.errors.append(Error('a file pattern not used in a patch',
+ pattern))
If the script printed this error I don't think I'd know what it was
complaining about. How about "pattern doesn't match any changed files"
?
I find the existing error 'file not changed in a patch' to be
suboptimal too. We're checking revisions (or commits), not patches.
Re: [IMPORTANT] ChangeLog related changes
On 6/2/20 1:48 PM, Martin Liška wrote:
I tend to this approach. Let me prepare a patch candidate for it.
There's a patch for it. Can you please Jonathan take a look?
Thanks,
Martin
>From 4d2cf31b6deb03c9ddc8062b9a45d2511e4a58bb Mon Sep 17 00:00:00 2001
From: Martin Liska
Date: Tue, 2 Jun 2020 15:13:22 +0200
Subject: [PATCH] gcc-changelog: support patterns
contrib/ChangeLog:
* gcc-changelog/git_commit.py: Support foo/bar/*: patterns in
wildcard_prefixes locations.
* gcc-changelog/test_email.py: Test it.
* gcc-changelog/test_patches.txt: Add 3 new patches.
---
contrib/gcc-changelog/git_commit.py| 52 +---
contrib/gcc-changelog/test_email.py| 12
contrib/gcc-changelog/test_patches.txt | 86 ++
3 files changed, 142 insertions(+), 8 deletions(-)
diff --git a/contrib/gcc-changelog/git_commit.py b/contrib/gcc-changelog/git_commit.py
index e2ef6c61ed0..37a3ef3fa92 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -136,6 +136,11 @@ ignored_prefixes = [
'libsanitizer/',
]
+wildcard_prefixes = [
+'gcc/testsuite/',
+'libstdc++-v3/doc/html/'
+]
+
misc_files = [
'gcc/DATESTAMP',
'gcc/BASE-VER',
@@ -182,11 +187,10 @@ class ChangeLogEntry:
self.initial_prs = list(prs)
self.prs = list(prs)
self.lines = []
+self.files = []
+self.file_patterns = []
-@property
-def files(self):
-files = []
-
+def parse_file_names(self):
# Whether the content currently processed is between a star prefix the
# end of the file list: a colon or an open paren.
in_location = False
@@ -215,8 +219,10 @@ class ChangeLogEntry:
for file in line.split(','):
file = file.strip()
if file:
-files.append(file)
-return files
+if file.endswith('*'):
+self.file_patterns.append(file[:-1])
+else:
+self.files.append(file)
@property
def datetime(self):
@@ -275,8 +281,10 @@ class GitCommit:
self.parse_lines(all_are_ignored)
if self.changes:
self.parse_changelog()
+self.parse_file_names()
self.check_for_empty_description()
self.deduce_changelog_locations()
+self.check_file_patterns()
if not self.errors:
self.check_mentioned_files()
self.check_for_correct_changelog()
@@ -442,6 +450,18 @@ class GitCommit:
else:
last_entry.lines.append(line)
+def parse_file_names(self):
+for entry in self.changelog_entries:
+entry.parse_file_names()
+
+def check_file_patterns(self):
+for entry in self.changelog_entries:
+for pattern in entry.file_patterns:
+name = os.path.join(entry.folder, pattern)
+if name not in wildcard_prefixes:
+msg = 'unsupported wilcard prefix'
+self.errors.append(Error(msg, name))
+
def check_for_empty_description(self):
for entry in self.changelog_entries:
for i, line in enumerate(entry.lines):
@@ -502,6 +522,8 @@ class GitCommit:
assert folder_count == len(self.changelog_entries)
mentioned_files = set()
+mentioned_patterns = []
+used_patterns = set()
for entry in self.changelog_entries:
if not entry.files:
msg = 'ChangeLog must contain at least one file entry'
@@ -510,6 +532,8 @@ class GitCommit:
for file in entry.files:
if not self.is_changelog_filename(file):
mentioned_files.add(os.path.join(entry.folder, file))
+for pattern in entry.file_patterns:
+mentioned_patterns.append(os.path.join(entry.folder, pattern))
cand = [x[0] for x in self.modified_files
if not self.is_changelog_filename(x[0])]
@@ -543,9 +567,21 @@ class GitCommit:
assert file.startswith(entry.folder)
file = file[len(entry.folder):].lstrip('/')
entry.lines.append('\t* %s: New file.' % file)
+entry.files.append(file)
else:
-msg = 'changed file not mentioned in a ChangeLog'
-self.errors.append(Error(msg, file))
+used_pattern = [p for p in mentioned_patterns
+if file.startswith(p)]
+used_pattern = used_pattern[0] if used_pattern else None
+if used_pattern:
+used_patterns.add(used_pattern)
+else:
+msg = 'changed file not mentioned in a ChangeLog'
+
Re: [IMPORTANT] ChangeLog related changes
On 6/2/20 1:22 PM, Jonathan Wakely via Gcc-patches wrote: On Tue, 2 Jun 2020 at 12:09, Jonathan Wakely wrote: On Tue, 2 Jun 2020 at 12:05, Jonathan Wakely wrote: On Tue, 2 Jun 2020 at 11:56, Gerald Pfeifer wrote: On Mon, 1 Jun 2020, Jonathan Wakely via Gcc-patches wrote: The libstdc++ manual is written in Docbook XML, but we commit both the XML and generated HTML pages to Git. Sometimes a small XML file can result in dozens of mechanical changes to the generated HTML files, which we record in the ChangeLog as: * doc/html/*: Regenerated. With the new checks we need to name every generated file individually. If we add that directory to the ignored_prefixes list, we won't need to name them. But then the doc/html/* entry will give an error, and changes to the HTML files can be committed without any ChangeLog entry. Should we just stop mentioning the HTML in the ChangeLog? We could do something like the attached patch, but it seems overkill for this one special case. The change makes sense, but indeed it feels like a very specialized case in a general script. Yes, that was my thought too. On the other hand, the script is just meant to enforce our policies, not dictate them. But on the gripping hand, if the policy can't be checked simply, maybe it's a bad policy. Similar to "doc/html/*" I've sometimes used "testsuite/*" for changes that affect huge numbers of files in the libstdc++ testsuite, e.g. commit r7-2817-52066eae5d3dd6b7c0a1b843469582dbdbb941eb did: 2911 files changed, 3072 insertions(+), 4512 deletions(-) Nice example. I don't want to list thousands of files at once. So maybe a general approach for allowing wildcards in specific directories makes sense. I tend to this approach. Let me prepare a patch candidate for it. What will we do on January 1 2021 when Jakub updates the copyright years in every file in the tree, turn off the hook temporarily? We should probably add early bail out for git commits with 'Bump copyright year'. I'll help Jakub with that. Martin
Re: [IMPORTANT] ChangeLog related changes
On Tue, 2 Jun 2020 at 12:09, Jonathan Wakely wrote: > > On Tue, 2 Jun 2020 at 12:05, Jonathan Wakely wrote: > > > > On Tue, 2 Jun 2020 at 11:56, Gerald Pfeifer wrote: > > > > > > On Mon, 1 Jun 2020, Jonathan Wakely via Gcc-patches wrote: > > > > The libstdc++ manual is written in Docbook XML, but we commit both the > > > > XML and generated HTML pages to Git. Sometimes a small XML file can > > > > result in dozens of mechanical changes to the generated HTML files, > > > > which we record in the ChangeLog as: > > > > > > > > * doc/html/*: Regenerated. > > > > > > > > With the new checks we need to name every generated file individually. > > > > > > > > If we add that directory to the ignored_prefixes list, we won't need > > > > to name them. But then the doc/html/* entry will give an error, and > > > > changes to the HTML files can be committed without any ChangeLog > > > > entry. Should we just stop mentioning the HTML in the ChangeLog? > > > > > > > > We could do something like the attached patch, but it seems overkill > > > > for this one special case. > > > > > > The change makes sense, but indeed it feels like a very specialized > > > case in a general script. > > > > Yes, that was my thought too. > > On the other hand, the script is just meant to enforce our policies, > not dictate them. But on the gripping hand, if the policy can't be > checked simply, maybe it's a bad policy. Similar to "doc/html/*" I've sometimes used "testsuite/*" for changes that affect huge numbers of files in the libstdc++ testsuite, e.g. commit r7-2817-52066eae5d3dd6b7c0a1b843469582dbdbb941eb did: 2911 files changed, 3072 insertions(+), 4512 deletions(-) I don't want to list thousands of files at once. So maybe a general approach for allowing wildcards in specific directories makes sense. What will we do on January 1 2021 when Jakub updates the copyright years in every file in the tree, turn off the hook temporarily?
Re: [IMPORTANT] ChangeLog related changes
On Tue, 2 Jun 2020 at 12:05, Jonathan Wakely wrote: > > On Tue, 2 Jun 2020 at 11:56, Gerald Pfeifer wrote: > > > > On Mon, 1 Jun 2020, Jonathan Wakely via Gcc-patches wrote: > > > The libstdc++ manual is written in Docbook XML, but we commit both the > > > XML and generated HTML pages to Git. Sometimes a small XML file can > > > result in dozens of mechanical changes to the generated HTML files, > > > which we record in the ChangeLog as: > > > > > > * doc/html/*: Regenerated. > > > > > > With the new checks we need to name every generated file individually. > > > > > > If we add that directory to the ignored_prefixes list, we won't need > > > to name them. But then the doc/html/* entry will give an error, and > > > changes to the HTML files can be committed without any ChangeLog > > > entry. Should we just stop mentioning the HTML in the ChangeLog? > > > > > > We could do something like the attached patch, but it seems overkill > > > for this one special case. > > > > The change makes sense, but indeed it feels like a very specialized > > case in a general script. > > Yes, that was my thought too. On the other hand, the script is just meant to enforce our policies, not dictate them. But on the gripping hand, if the policy can't be checked simply, maybe it's a bad policy.
Re: [IMPORTANT] ChangeLog related changes
On Tue, 2 Jun 2020 at 07:44, Martin Liška wrote:
>
> On 6/1/20 7:24 PM, Jonathan Wakely wrote:
> > On Mon, 25 May 2020 at 23:50, Jakub Jelinek via Gcc
> > wrote:
> >>
> >> Hi!
> >>
> >> I've turned the strict mode of Martin Liška's hook changes,
> >> which means that from now on no commits to the trunk or release branches
> >> should be changing any ChangeLog files together with the other files,
> >> ChangeLog entry should be solely in the commit message.
> >> The DATESTAMP bumping script will be updating the ChangeLog files for you.
> >> If somebody makes a mistake in that, please wait 24 hours (at least until
> >> after 00:16 UTC after your commit) so that the script will create the
> >> ChangeLog entries, and afterwards it can be fixed by adjusting the
> >> ChangeLog
> >> files. But you can only touch the ChangeLog files in that case (and
> >> shouldn't write a ChangeLog entry for that in the commit message).
> >>
> >> If anything goes wrong, please let me, other RMs and Martin Liška know.
> >
> > The libstdc++ manual is written in Docbook XML, but we commit both the
> > XML and generated HTML pages to Git. Sometimes a small XML file can
> > result in dozens of mechanical changes to the generated HTML files,
> > which we record in the ChangeLog as:
> >
> > * doc/html/*: Regenerated.
> >
> > With the new checks we need to name every generated file individually.
> >
> > If we add that directory to the ignored_prefixes list, we won't need
> > to name them. But then the doc/html/* entry will give an error, and
> > changes to the HTML files can be committed without any ChangeLog
> > entry. Should we just stop mentioning the HTML in the ChangeLog?
> >
> > We could do something like the attached patch, but it seems overkill
> > for this one special case.
> >
>
> The patch is fine to me.
> Can you please a pytest for the situation:
> contrib/gcc-changelog/test_email.py ?
Tests now added (and passing) for the positive case and both negative cases.
commit 21f171e1f8af7e222a87523d7957174f985a1dd5
Author: Jonathan Wakely
Date: Mon Jun 1 18:33:48 2020 +0100
gcc-changelog: Allow "doc/html/*" in libstdc++ changelog
contrib/ChangeLog:
* gcc-changelog/git_commit.py (check_mentioned_files): Allow
wildcard path for generated files in libstdc++-v3/doc/html.
* gcc-changelog/test_email.py (test_libstdcxx_html_regenerated):
New test.
* gcc-changelog/test_patches.txt: Add patches.
diff --git a/contrib/gcc-changelog/git_commit.py
b/contrib/gcc-changelog/git_commit.py
index b8c7f718c50..d723d890f31 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -501,6 +501,7 @@ class GitCommit:
assert folder_count == len(self.changelog_entries)
mentioned_files = set()
+libstdcxx_html_regenerated = False
for entry in self.changelog_entries:
if not entry.files:
msg = 'ChangeLog must contain at least one file entry'
@@ -508,16 +509,33 @@ class GitCommit:
assert not entry.folder.endswith('/')
for file in entry.files:
if not self.is_changelog_filename(file):
-mentioned_files.add(os.path.join(entry.folder, file))
+file = os.path.join(entry.folder, file)
+if file == 'libstdc++-v3/doc/html/*':
+libstdcxx_html_regenerated = True
+else:
+mentioned_files.add(file)
cand = [x[0] for x in self.modified_files
if not self.is_changelog_filename(x[0])]
changed_files = set(cand)
+if libstdcxx_html_regenerated:
+libstdcxx_html_regenerated = False
+for c in changed_files:
+if c.startswith('libstdc++-v3/doc/html/'):
+libstdcxx_html_regenerated = True
+break
+if not libstdcxx_html_regenerated:
+self.errors.append(Error('No libstdc++ HTML changes found'))
+
for file in sorted(mentioned_files - changed_files):
self.errors.append(Error('file not changed in a patch', file))
for file in sorted(changed_files - mentioned_files):
if not self.in_ignored_location(file):
-if file in self.new_files:
+if file.startswith('libstdc++-v3/doc/html/'):
+if not libstdcxx_html_regenerated:
+msg = 'libstdc++ HTML changes not in ChangeLog'
+self.errors.append(Error(msg, file))
+elif file in self.new_files:
changelog_location = self.get_changelog_by_path(file)
# Python2: we cannot use next(filter(...))
entries = filter(lambda x: x.folder == changelog_location,
diff --git a/contrib/gcc-changelog/test_email.py
b/contrib/gcc-changelog/test_email.py
Re: [IMPORTANT] ChangeLog related changes
On Tue, 2 Jun 2020 at 11:56, Gerald Pfeifer wrote: > > On Mon, 1 Jun 2020, Jonathan Wakely via Gcc-patches wrote: > > The libstdc++ manual is written in Docbook XML, but we commit both the > > XML and generated HTML pages to Git. Sometimes a small XML file can > > result in dozens of mechanical changes to the generated HTML files, > > which we record in the ChangeLog as: > > > > * doc/html/*: Regenerated. > > > > With the new checks we need to name every generated file individually. > > > > If we add that directory to the ignored_prefixes list, we won't need > > to name them. But then the doc/html/* entry will give an error, and > > changes to the HTML files can be committed without any ChangeLog > > entry. Should we just stop mentioning the HTML in the ChangeLog? > > > > We could do something like the attached patch, but it seems overkill > > for this one special case. > > The change makes sense, but indeed it feels like a very specialized > case in a general script. Yes, that was my thought too. > Thinking out of the box (and admittedly with a dose of igorance, which > means I am likely missing something): Is not keeping the libstdc++/doc > HTML in Git a viable option? Only creating that HTML as part of releases > and maybe snapshots? It gets sync'd to https://gcc.gnu.org/onlinedocs/libstdc++ nightly. We could generate it nightly, but we'd need all the docbook stylesheets etc. on sourceware. Or we could just generate it for snapshots (which would still need the docbook stuff on the server) and only sync the onlinedocs weekly from the snapshot.
Re: [IMPORTANT] ChangeLog related changes
On Mon, 1 Jun 2020, Jonathan Wakely via Gcc-patches wrote: > The libstdc++ manual is written in Docbook XML, but we commit both the > XML and generated HTML pages to Git. Sometimes a small XML file can > result in dozens of mechanical changes to the generated HTML files, > which we record in the ChangeLog as: > > * doc/html/*: Regenerated. > > With the new checks we need to name every generated file individually. > > If we add that directory to the ignored_prefixes list, we won't need > to name them. But then the doc/html/* entry will give an error, and > changes to the HTML files can be committed without any ChangeLog > entry. Should we just stop mentioning the HTML in the ChangeLog? > > We could do something like the attached patch, but it seems overkill > for this one special case. The change makes sense, but indeed it feels like a very specialized case in a general script. Thinking out of the box (and admittedly with a dose of igorance, which means I am likely missing something): Is not keeping the libstdc++/doc HTML in Git a viable option? Only creating that HTML as part of releases and maybe snapshots? Gerald
Re: [IMPORTANT] ChangeLog related changes
On 6/1/20 7:24 PM, Jonathan Wakely wrote: On Mon, 25 May 2020 at 23:50, Jakub Jelinek via Gcc wrote: Hi! I've turned the strict mode of Martin Liška's hook changes, which means that from now on no commits to the trunk or release branches should be changing any ChangeLog files together with the other files, ChangeLog entry should be solely in the commit message. The DATESTAMP bumping script will be updating the ChangeLog files for you. If somebody makes a mistake in that, please wait 24 hours (at least until after 00:16 UTC after your commit) so that the script will create the ChangeLog entries, and afterwards it can be fixed by adjusting the ChangeLog files. But you can only touch the ChangeLog files in that case (and shouldn't write a ChangeLog entry for that in the commit message). If anything goes wrong, please let me, other RMs and Martin Liška know. The libstdc++ manual is written in Docbook XML, but we commit both the XML and generated HTML pages to Git. Sometimes a small XML file can result in dozens of mechanical changes to the generated HTML files, which we record in the ChangeLog as: * doc/html/*: Regenerated. With the new checks we need to name every generated file individually. If we add that directory to the ignored_prefixes list, we won't need to name them. But then the doc/html/* entry will give an error, and changes to the HTML files can be committed without any ChangeLog entry. Should we just stop mentioning the HTML in the ChangeLog? We could do something like the attached patch, but it seems overkill for this one special case. The patch is fine to me. Can you please a pytest for the situation: contrib/gcc-changelog/test_email.py ? Martin
Re: [IMPORTANT] ChangeLog related changes
On Mon, 25 May 2020 at 23:50, Jakub Jelinek via Gcc wrote:
>
> Hi!
>
> I've turned the strict mode of Martin Liška's hook changes,
> which means that from now on no commits to the trunk or release branches
> should be changing any ChangeLog files together with the other files,
> ChangeLog entry should be solely in the commit message.
> The DATESTAMP bumping script will be updating the ChangeLog files for you.
> If somebody makes a mistake in that, please wait 24 hours (at least until
> after 00:16 UTC after your commit) so that the script will create the
> ChangeLog entries, and afterwards it can be fixed by adjusting the ChangeLog
> files. But you can only touch the ChangeLog files in that case (and
> shouldn't write a ChangeLog entry for that in the commit message).
>
> If anything goes wrong, please let me, other RMs and Martin Liška know.
The libstdc++ manual is written in Docbook XML, but we commit both the
XML and generated HTML pages to Git. Sometimes a small XML file can
result in dozens of mechanical changes to the generated HTML files,
which we record in the ChangeLog as:
* doc/html/*: Regenerated.
With the new checks we need to name every generated file individually.
If we add that directory to the ignored_prefixes list, we won't need
to name them. But then the doc/html/* entry will give an error, and
changes to the HTML files can be committed without any ChangeLog
entry. Should we just stop mentioning the HTML in the ChangeLog?
We could do something like the attached patch, but it seems overkill
for this one special case.
diff --git a/contrib/gcc-changelog/git_commit.py
b/contrib/gcc-changelog/git_commit.py
index 4f82b58f64b..add0defaeed 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -501,6 +501,7 @@ class GitCommit:
assert folder_count == len(self.changelog_entries)
mentioned_files = set()
+libstdcxx_html_regenerated = False
for entry in self.changelog_entries:
if not entry.files:
msg = 'ChangeLog must contain a file entry'
@@ -508,16 +509,33 @@ class GitCommit:
assert not entry.folder.endswith('/')
for file in entry.files:
if not self.is_changelog_filename(file):
-mentioned_files.add(os.path.join(entry.folder, file))
+file = os.path.join(entry.folder, file)
+if file == 'libstdc++-v3/doc/html/*':
+libstdcxx_html_regenerated = True
+else:
+mentioned_files.add(file)
cand = [x[0] for x in self.modified_files
if not self.is_changelog_filename(x[0])]
changed_files = set(cand)
+if libstdcxx_html_regenerated:
+libstdcxx_html_regenerated = False
+for c in changed_files:
+if c.startswith('libstdc++-v3/doc/html/'):
+libstdcxx_html_regenerated = True
+break
+if not libstdcxx_html_regenerated:
+self.errors.append(Error('No libstdc++ HTML changes found'))
+
for file in sorted(mentioned_files - changed_files):
self.errors.append(Error('file not changed in a patch', file))
for file in sorted(changed_files - mentioned_files):
if not self.in_ignored_location(file):
-if file in self.new_files:
+if file.startswith('libstdc++-v3/doc/html/'):
+if not libstdcxx_html_regenerated:
+msg = 'libstdc++ HTML changes not in ChangeLog'
+self.errors.append(Error(msg, file))
+elif file in self.new_files:
changelog_location = self.get_changelog_by_path(file)
# Python2: we cannot use next(filter(...))
entries = filter(lambda x: x.folder == changelog_location,
Re: [IMPORTANT] ChangeLog related changes
Great, thanks! On Tue, May 26, 2020 at 2:08 PM Martin Liška wrote: > > On 5/26/20 7:22 AM, Hongtao Liu via Gcc wrote: > > i commit a separate patch alone only for ChangeLog files, should i revert > > it? > > Hello. > > I've just done it. > > Martin -- BR, Hongtao
Re: [IMPORTANT] ChangeLog related changes
On 5/26/20 7:22 AM, Hongtao Liu via Gcc wrote: i commit a separate patch alone only for ChangeLog files, should i revert it? Hello. I've just done it. Martin
Re: [IMPORTANT] ChangeLog related changes
On Tue, May 26, 2020 at 6:49 AM Jakub Jelinek via Gcc-patches wrote: > > Hi! > > I've turned the strict mode of Martin Liška's hook changes, > which means that from now on no commits to the trunk or release branches > should be changing any ChangeLog files together with the other files, > ChangeLog entry should be solely in the commit message. > The DATESTAMP bumping script will be updating the ChangeLog files for you. Oh, no wonder my patch was rejected by git hook with error message --- ChangeLog files, DATESTAMP, BASE-VER and DEV-PHASE can be modified only separately from other files --- > If somebody makes a mistake in that, please wait 24 hours (at least until i commit a separate patch alone only for ChangeLog files, should i revert it? > after 00:16 UTC after your commit) so that the script will create the > ChangeLog entries, and afterwards it can be fixed by adjusting the ChangeLog > files. But you can only touch the ChangeLog files in that case (and > shouldn't write a ChangeLog entry for that in the commit message). > > If anything goes wrong, please let me, other RMs and Martin Liška know. > > Jakub > -- BR, Hongtao
[IMPORTANT] ChangeLog related changes
Hi! I've turned the strict mode of Martin Liška's hook changes, which means that from now on no commits to the trunk or release branches should be changing any ChangeLog files together with the other files, ChangeLog entry should be solely in the commit message. The DATESTAMP bumping script will be updating the ChangeLog files for you. If somebody makes a mistake in that, please wait 24 hours (at least until after 00:16 UTC after your commit) so that the script will create the ChangeLog entries, and afterwards it can be fixed by adjusting the ChangeLog files. But you can only touch the ChangeLog files in that case (and shouldn't write a ChangeLog entry for that in the commit message). If anything goes wrong, please let me, other RMs and Martin Liška know. Jakub
