[IMPORTANT] ChangeLog related changes

[IMPORTANT] ChangeLog related changes

[Jakub Jelinek]

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

Re: [IMPORTANT] ChangeLog related changes

[Hongtao Liu]

On Tue, May 26, 2020 at 6:49 AM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> 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

Re: [IMPORTANT] ChangeLog related changes

[Martin Liška]

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

[Hongtao Liu]

Great, thanks!

On Tue, May 26, 2020 at 2:08 PM Martin Liška <mliska@suse.cz> 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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 1623 bytes --]

On Mon, 25 May 2020 at 23:50, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> 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.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2399 bytes --]

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

[Martin Liška]

On 6/1/20 7:24 PM, Jonathan Wakely wrote:
> On Mon, 25 May 2020 at 23:50, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> 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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

On Tue, 2 Jun 2020 at 07:44, Martin Liška <mliska@suse.cz> wrote:
>
> On 6/1/20 7:24 PM, Jonathan Wakely wrote:
> > On Mon, 25 May 2020 at 23:50, Jakub Jelinek via Gcc <gcc@gcc.gnu.org> 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.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 10292 bytes --]

commit 21f171e1f8af7e222a87523d7957174f985a1dd5
Author: Jonathan Wakely <jwakely@redhat.com>
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
index 2465669786e..3f4bc9dffd0 100755
--- a/contrib/gcc-changelog/test_email.py
+++ b/contrib/gcc-changelog/test_email.py
@@ -318,3 +318,14 @@ class TestGccChangelog(unittest.TestCase):
         assert len(email.errors) == 2
         assert email.errors[0].message == 'missing description of a change'
         assert email.errors[1].message == 'missing description of a change'
+
+    def test_libstdcxx_html_regenerated(self):
+        email = self.from_patch_glob('0001-Fix-text-of-hyperlink-in-manual')
+        assert not email.errors
+        email = self.from_patch_glob('0002-libstdc-Fake-test-change-1.patch')
+        assert len(email.errors) == 1
+        assert email.errors[0].message == 'No libstdc++ HTML changes found'
+        email = self.from_patch_glob('0003-libstdc-Fake-test-change-2.patch')
+        assert len(email.errors) == 1
+        msg = 'libstdc++ HTML changes not in ChangeLog'
+        assert email.errors[0].message == msg
diff --git a/contrib/gcc-changelog/test_patches.txt b/contrib/gcc-changelog/test_patches.txt
index 25311fbf300..2c308f6477c 100644
--- a/contrib/gcc-changelog/test_patches.txt
+++ b/contrib/gcc-changelog/test_patches.txt
@@ -2973,3 +2973,127 @@ index 1cd5872c03d..6f95aedb3d3 100644
 +
 -- 
 2.26.2
+
+=== 0001-Fix-text-of-hyperlink-in-manual.patch ===
+From c7904d9e08a0ca3f733be3c2e8a3b912fa851fc5 Mon Sep 17 00:00:00 2001
+From: Jonathan Wakely <jwakely@redhat.com>
+Date: Fri, 8 Mar 2019 13:56:53 +0000
+Subject: [PATCH] Fix text of hyperlink in manual
+
+	* doc/xml/manual/using.xml: Use link element instead of xref.
+	* doc/html/*: Regenerate.
+
+---
+ libstdc++-v3/ChangeLog                         | 3 +++
+ libstdc++-v3/doc/html/manual/using_macros.html | 3 ++-
+ libstdc++-v3/doc/xml/manual/using.xml          | 4 ++--
+ 3 files changed, 7 insertions(+), 3 deletions(-)
+
+diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog
+index e752285bdb2..7da25945a8c 100644
+--- a/libstdc++-v3/ChangeLog
++++ b/libstdc++-v3/ChangeLog
+@@ -1,5 +1,8 @@
+ 2019-03-08  Jonathan Wakely  <jwakely@redhat.com>
+ 
++	* doc/xml/manual/using.xml: Use link element instead of xref.
++	* doc/html/*: Regenerate.
++
+ 	* include/bits/fs_path.h (path::format): Add fixed underlying type.
+ 
+ 2019-03-08  François Dumont  <fdumont@gcc.gnu.org>
+diff --git a/libstdc++-v3/doc/html/manual/using_macros.html b/libstdc++-v3/doc/html/manual/using_macros.html
+index 7030bd2d0fd..dad6564a97d 100644
+--- a/libstdc++-v3/doc/html/manual/using_macros.html
++++ b/libstdc++-v3/doc/html/manual/using_macros.html
+@@ -116,7 +116,8 @@
+         enabled for
+         <code class="classname">std::vector&lt;T, std::allocator&lt;T&gt;&gt;</code>
+         and only when <code class="classname">std::allocator</code> is derived from
+-        <a class="xref" href="memory.html#allocator.impl" title="Implementation">the section called “Implementation”</a>. The annotations
++        <a class="link" href="memory.html#allocator.impl" title="Implementation"><code class="classname">new_allocator</code>
++        or <code class="classname">malloc_allocator</code></a>. The annotations
+         must be present on all vector operations or none, so this macro must
+         be defined to the same value for all translation units that create,
+         destroy or modify vectors.
+diff --git a/libstdc++-v3/doc/xml/manual/using.xml b/libstdc++-v3/doc/xml/manual/using.xml
+index 2d44a739406..7647e9b8dad 100644
+--- a/libstdc++-v3/doc/xml/manual/using.xml
++++ b/libstdc++-v3/doc/xml/manual/using.xml
+@@ -1192,8 +1192,8 @@ g++ -Winvalid-pch -I. -include stdc++.h -H -g -O2 hello.cc -o test.exe
+         enabled for
+         <classname>std::vector&lt;T, std::allocator&lt;T&gt;&gt;</classname>
+         and only when <classname>std::allocator</classname> is derived from
+-        <xref linkend="allocator.impl"><classname>new_allocator</classname>
+-        or <classname>malloc_allocator</classname></xref>. The annotations
++        <link linkend="allocator.impl"><classname>new_allocator</classname>
++        or <classname>malloc_allocator</classname></link>. The annotations
+         must be present on all vector operations or none, so this macro must
+         be defined to the same value for all translation units that create,
+         destroy or modify vectors.
+-- 
+2.25.4
+
+=== 0002-libstdc-Fake-test-change-1.patch ===
+From fe4ade6778d1d97214db12bf2c40d0f40e7f953a Mon Sep 17 00:00:00 2001
+From: Jonathan Wakely <jwakely@redhat.com>
+Date: Tue, 2 Jun 2020 11:52:34 +0100
+Subject: [PATCH] libstdc++: Fake change for testing git_commit.py
+
+libstdc++-v3/ChangeLog:
+
+	* doc/xml/faq.xml: Fake change.
+	* doc/html/*: Regenerated.
+---
+ libstdc++-v3/doc/xml/faq.xml | 1 +
+ 1 file changed, 1 insertion(+)
+
+diff --git a/libstdc++-v3/doc/xml/faq.xml b/libstdc++-v3/doc/xml/faq.xml
+index e419d3c22a0..bcc14dd6d90 100644
+--- a/libstdc++-v3/doc/xml/faq.xml
++++ b/libstdc++-v3/doc/xml/faq.xml
+@@ -1315,3 +1315,4 @@
+ </article>
+ 
+ </book>
++fake change
+-- 
+2.25.4
+
+=== 0003-libstdc-Fake-test-change-2.patch ===
+From e460effb3a42c1c046b682fe266da418f2693ef3 Mon Sep 17 00:00:00 2001
+From: Jonathan Wakely <jwakely@redhat.com>
+Date: Tue, 2 Jun 2020 11:52:34 +0100
+Subject: [PATCH] libstdc++: Fake change for testing 2
+
+libstdc++-v3/ChangeLog:
+
+	* doc/xml/faq.xml: Fake change.
+---
+ libstdc++-v3/doc/html/faq.html | 2 +-
+ libstdc++-v3/doc/xml/faq.xml   | 1 +
+ 2 files changed, 2 insertions(+), 1 deletion(-)
+
+diff --git a/libstdc++-v3/doc/html/faq.html b/libstdc++-v3/doc/html/faq.html
+index 967e5f5f348..95d21b5bf9f 100644
+--- a/libstdc++-v3/doc/html/faq.html
++++ b/libstdc++-v3/doc/html/faq.html
+@@ -910,4 +910,4 @@
+     </p><p>
+     See <a class="link" href="manual/strings.html#strings.string.shrink" title="Shrink to Fit">Shrink-to-fit
+     strings</a> for a similar solution for strings.
+-    </p></td></tr></tbody></table></div></div><div class="navfooter"><hr /><table width="100%" summary="Navigation footer"><tr><td width="40%" align="left"><a accesskey="p" href="bk03.html">Prev</a> </td><td width="20%" align="center"><a accesskey="u" href="bk03.html">Up</a></td><td width="40%" align="right"> </td></tr><tr><td width="40%" align="left" valign="top"> </td><td width="20%" align="center"><a accesskey="h" href="index.html">Home</a></td><td width="40%" align="right" valign="top"> </td></tr></table></div></body></html>
+\ No newline at end of file
++    </p></td></tr></tbody></table></div></div><div class="navfooter"><hr /><table width="100%" summary="Navigation footer"><tr><td width="40%" align="left"><a accesskey="p" href="bk03.html">Prev</a> </td><td width="20%" align="center"><a accesskey="u" href="bk03.html">Up</a></td><td width="40%" align="right"> </td></tr><tr><td width="40%" align="left" valign="top"> </td><td width="20%" align="center"><a accesskey="h" href="index.html">Home</a></td><td width="40%" align="right" valign="top"> </td></tr></table></div></body></html>fake change
+diff --git a/libstdc++-v3/doc/xml/faq.xml b/libstdc++-v3/doc/xml/faq.xml
+index e419d3c22a0..bcc14dd6d90 100644
+--- a/libstdc++-v3/doc/xml/faq.xml
++++ b/libstdc++-v3/doc/xml/faq.xml
+@@ -1315,3 +1315,4 @@
+ </article>
+ 
+ </book>
++fake change
+-- 
+2.25.4
+

Re: [IMPORTANT] ChangeLog related changes

[Gerald Pfeifer]

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

[Jonathan Wakely]

On Tue, 2 Jun 2020 at 11:56, Gerald Pfeifer <gerald@pfeifer.com> 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

[Jonathan Wakely]

On Tue, 2 Jun 2020 at 12:05, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Tue, 2 Jun 2020 at 11:56, Gerald Pfeifer <gerald@pfeifer.com> 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

[Jonathan Wakely]

On Tue, 2 Jun 2020 at 12:09, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Tue, 2 Jun 2020 at 12:05, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> >
> > On Tue, 2 Jun 2020 at 11:56, Gerald Pfeifer <gerald@pfeifer.com> 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

[Martin Liška]

On 6/2/20 1:22 PM, Jonathan Wakely via Gcc-patches wrote:
> On Tue, 2 Jun 2020 at 12:09, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>>
>> On Tue, 2 Jun 2020 at 12:05, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>>>
>>> On Tue, 2 Jun 2020 at 11:56, Gerald Pfeifer <gerald@pfeifer.com> 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

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 198 bytes --]

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

[-- Attachment #2: 0001-gcc-changelog-support-patterns.patch --]
[-- Type: text/x-patch, Size: 9157 bytes --]

From 4d2cf31b6deb03c9ddc8062b9a45d2511e4a58bb Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
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'
+                        self.errors.append(Error(msg, file))
+
+        for pattern in mentioned_patterns:
+            if pattern not in used_patterns:
+                self.errors.append(Error('a file pattern not used in a patch',
+                                         pattern))
 
     def check_for_correct_changelog(self):
         for entry in self.changelog_entries:
diff --git a/contrib/gcc-changelog/test_email.py b/contrib/gcc-changelog/test_email.py
index 2465669786e..308513011cb 100755
--- a/contrib/gcc-changelog/test_email.py
+++ b/contrib/gcc-changelog/test_email.py
@@ -318,3 +318,15 @@ class TestGccChangelog(unittest.TestCase):
         assert len(email.errors) == 2
         assert email.errors[0].message == 'missing description of a change'
         assert email.errors[1].message == 'missing description of a change'
+
+    def test_libstdcxx_html_regenerated(self):
+        email = self.from_patch_glob('0001-Fix-text-of-hyperlink')
+        assert not email.errors
+        email = self.from_patch_glob('0002-libstdc-Fake-test-change-1.patch')
+        assert len(email.errors) == 1
+        assert email.errors[0].message == 'a file pattern not used in a patch'
+        assert email.errors[0].line == 'libstdc++-v3/doc/html/'
+        email = self.from_patch_glob('0003-libstdc-Fake-test-change-2.patch')
+        assert len(email.errors) == 1
+        msg = 'changed file not mentioned in a ChangeLog'
+        assert email.errors[0].message == msg
diff --git a/contrib/gcc-changelog/test_patches.txt b/contrib/gcc-changelog/test_patches.txt
index 25311fbf300..5d9b62d2637 100644
--- a/contrib/gcc-changelog/test_patches.txt
+++ b/contrib/gcc-changelog/test_patches.txt
@@ -2973,3 +2973,89 @@ index 1cd5872c03d..6f95aedb3d3 100644
 +
 -- 
 2.26.2
+
+=== 0001-Fix-text-of-hyperlink-in-manual.patch ===
+From c7904d9e08a0ca3f733be3c2e8a3b912fa851fc5 Mon Sep 17 00:00:00 2001
+From: Jonathan Wakely <jwakely@redhat.com>
+Date: Fri, 8 Mar 2019 13:56:53 +0000
+Subject: [PATCH] Fix text of hyperlink in manual
+
+	* doc/xml/manual/using.xml: Use link element instead of xref.
+	* doc/html/*: Regenerate.
+
+---
+ libstdc++-v3/ChangeLog                         | 3 +++
+ libstdc++-v3/doc/html/manual/using_macros.html | 3 ++-
+ libstdc++-v3/doc/xml/manual/using.xml          | 4 ++--
+ 3 files changed, 7 insertions(+), 3 deletions(-)
+
+diff --git a/libstdc++-v3/doc/html/manual/using_macros.html b/libstdc++-v3/doc/html/manual/using_macros.html
+index 7030bd2d0fd..dad6564a97d 100644
+--- a/libstdc++-v3/doc/html/manual/using_macros.html
++++ b/libstdc++-v3/doc/html/manual/using_macros.html
+@@ -1 +1,2 @@
+ 
++
+diff --git a/libstdc++-v3/doc/xml/manual/using.xml b/libstdc++-v3/doc/xml/manual/using.xml
+index 2d44a739406..7647e9b8dad 100644
+--- a/libstdc++-v3/doc/xml/manual/using.xml
++++ b/libstdc++-v3/doc/xml/manual/using.xml
+@@ -1 +1,2 @@
+ 
++
+-- 
+2.25.4
+
+=== 0002-libstdc-Fake-test-change-1.patch ===
+From fe4ade6778d1d97214db12bf2c40d0f40e7f953a Mon Sep 17 00:00:00 2001
+From: Jonathan Wakely <jwakely@redhat.com>
+Date: Tue, 2 Jun 2020 11:52:34 +0100
+Subject: [PATCH] libstdc++: Fake change for testing git_commit.py
+
+libstdc++-v3/ChangeLog:
+
+	* doc/xml/faq.xml: Fake change.
+	* doc/html/*: Regenerated.
+---
+ libstdc++-v3/doc/xml/faq.xml | 1 +
+ 1 file changed, 1 insertion(+)
+
+diff --git a/libstdc++-v3/doc/xml/faq.xml b/libstdc++-v3/doc/xml/faq.xml
+index e419d3c22a0..bcc14dd6d90 100644
+--- a/libstdc++-v3/doc/xml/faq.xml
++++ b/libstdc++-v3/doc/xml/faq.xml
+@@ -1 +1,2 @@
+ 
++
+-- 
+2.25.4
+
+=== 0003-libstdc-Fake-test-change-2.patch ===
+From e460effb3a42c1c046b682fe266da418f2693ef3 Mon Sep 17 00:00:00 2001
+From: Jonathan Wakely <jwakely@redhat.com>
+Date: Tue, 2 Jun 2020 11:52:34 +0100
+Subject: [PATCH] libstdc++: Fake change for testing 2
+
+libstdc++-v3/ChangeLog:
+
+	* doc/xml/faq.xml: Fake change.
+---
+ libstdc++-v3/doc/html/faq.html | 2 +-
+ libstdc++-v3/doc/xml/faq.xml   | 1 +
+ 2 files changed, 2 insertions(+), 1 deletion(-)
+
+diff --git a/libstdc++-v3/doc/html/faq.html b/libstdc++-v3/doc/html/faq.html
+index 967e5f5f348..95d21b5bf9f 100644
+--- a/libstdc++-v3/doc/html/faq.html
++++ b/libstdc++-v3/doc/html/faq.html
+@@ -1 +1,2 @@
+ 
++
+--- a/libstdc++-v3/doc/xml/faq.xml
++++ b/libstdc++-v3/doc/xml/faq.xml
+@@ -1 +1,2 @@
+ 
++
+-- 
+2.25.4
+
-- 
2.26.2

Re: [IMPORTANT] ChangeLog related changes

[Jonathan Wakely]

On Tue, 2 Jun 2020 at 14:16, Martin Liška <mliska@suse.cz> 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

[Martin Liška]

On 6/2/20 3:56 PM, Jonathan Wakely wrote:
> On Tue, 2 Jun 2020 at 14:16, Martin Liška <mliska@suse.cz> 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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

On Tue, 2 Jun 2020 at 14:56, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>
> On Tue, 2 Jun 2020 at 14:16, Martin Liška <mliska@suse.cz> 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?

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 4362 bytes --]

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 = 'unchanged file mentioned in a ChangeLog: "%s"'
+            self.errors.append(Error(msg % file))
         for file in sorted(changed_files - mentioned_files):
             if not self.in_ignored_location(file):
                 if file in self.new_files:
@@ -575,7 +574,7 @@ class GitCommit:
                     if used_pattern:
                         used_patterns.add(used_pattern)
                     else:
-                        msg = 'changed file not mentioned in a ChangeLog'
+                        msg = 'changed file not mentioned in a ChangeLog: "%s"'
                         self.errors.append(Error(msg, file))
 
         for pattern in mentioned_patterns:
@@ -589,8 +588,9 @@ class GitCommit:
                 full_path = os.path.join(entry.folder, file)
                 changelog_location = self.get_changelog_by_path(full_path)
                 if changelog_location != entry.folder:
-                    msg = 'wrong ChangeLog location "%s", should be "%s"'
-                    err = Error(msg % (entry.folder, changelog_location), file)
+                    msg = 'wrong ChangeLog location for "%s": "%s"'
+                    path = os.path.join(changelog_location, file)
+                    err = Error(msg % (path, entry.folder))
                     self.errors.append(err)
 
     def to_changelog_entries(self, use_commit_ts=False):

Re: [IMPORTANT] ChangeLog related changes

[Martin Liška]

On 6/2/20 4:14 PM, Jonathan Wakely wrote:
> On Tue, 2 Jun 2020 at 14:56, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
>>
>> On Tue, 2 Jun 2020 at 14:16, Martin Liška <mliska@suse.cz> 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

[Jonathan Wakely]

[-- Attachment #1: Type: text/plain, Size: 2999 bytes --]

On Tue, 2 Jun 2020 at 15:25, Martin Liška <mliska@suse.cz> wrote:
>
> On 6/2/20 4:14 PM, Jonathan Wakely wrote:
> > On Tue, 2 Jun 2020 at 14:56, Jonathan Wakely <jwakely.gcc@gmail.com> wrote:
> >>
> >> On Tue, 2 Jun 2020 at 14:16, Martin Liška <mliska@suse.cz> 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?

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 5029 bytes --]

commit 49652b7f5b57b88c1e0e27cf8ac488cbc85f1c7d
Author: Jonathan Wakely <jwakely@redhat.com>
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'
+                                  'an asterisk and a space'
                             self.errors.append(Error(msg, line))
                         else:
                             last_entry.lines.append(line)
@@ -527,7 +526,7 @@ class GitCommit:
         used_patterns = set()
         for entry in self.changelog_entries:
             if not entry.files:
-                msg = 'ChangeLog must contain at least one file entry'
+                msg = 'no files mentioned for ChangeLog in directory'
                 self.errors.append(Error(msg, entry.folder))
             assert not entry.folder.endswith('/')
             for file in entry.files:
@@ -540,7 +539,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 = 'unchanged file mentioned in a ChangeLog'
+            self.errors.append(Error(msg, file))
         for file in sorted(changed_files - mentioned_files):
             if not self.in_ignored_location(file):
                 if file in self.new_files:
diff --git a/contrib/gcc-changelog/test_email.py b/contrib/gcc-changelog/test_email.py
index 04ddad3f100..df57bb5c94a 100755
--- a/contrib/gcc-changelog/test_email.py
+++ b/contrib/gcc-changelog/test_email.py
@@ -105,7 +105,7 @@ class TestGccChangelog(unittest.TestCase):
         email = self.from_patch_glob('0096')
         assert email.errors
         err = email.errors[0]
-        assert err.message == 'file not changed in a patch'
+        assert err.message == 'unchanged file mentioned in a ChangeLog'
         assert err.line == 'gcc/testsuite/gcc.target/aarch64/' \
                            'advsimd-intrinsics/vdot-compile-3-1.c'
 
@@ -161,8 +161,8 @@ class TestGccChangelog(unittest.TestCase):
 
     def test_additional_author_list(self):
         email = self.from_patch_glob('0342')
-        assert (email.errors[1].message == 'additional author must prepend '
-                                           'with tab and 4 spaces')
+        assert (email.errors[1].message == 'additional author must be indented '
+                                           'with one tab and four spaces')
 
     def test_trailing_whitespaces(self):
         email = self.get_git_email('trailing-whitespaces.patch')
@@ -260,8 +260,8 @@ class TestGccChangelog(unittest.TestCase):
 
     def test_wrong_changelog_entry(self):
         email = self.from_patch_glob('0020-IPA-Avoid')
-        assert (email.errors[0].message
-                == 'first line should start with a tab, asterisk and space')
+        msg = 'first line should start with a tab, an asterisk and a space'
+        assert (email.errors[0].message == msg)
 
     def test_cherry_pick_format(self):
         email = self.from_patch_glob('0001-c-Alias.patch')

Re: [IMPORTANT] ChangeLog related changes

[Martin Liška]

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

[Tamar Christina]

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 <gcc-patches-bounces@gcc.gnu.org> On Behalf Of Martin
> Liška
> Sent: Wednesday, June 10, 2020 8:38 AM
> To: Jonathan Wakely <jwakely.gcc@gmail.com>
> Cc: Jakub Jelinek <jakub@redhat.com>; gcc@gcc.gnu.org; gcc-patches <gcc-
> patches@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

[Marek Polacek]

On Wed, Jun 10, 2020 at 01:34:54PM +0000, 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 <mail> to your commit.

If we don't already document it, we should.

Marek

Re: [IMPORTANT] ChangeLog related changes

[Martin Liška]

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 <gcc-patches-bounces@gcc.gnu.org> On Behalf Of Martin
>> Liška
>> Sent: Wednesday, June 10, 2020 8:38 AM
>> To: Jonathan Wakely <jwakely.gcc@gmail.com>
>> Cc: Jakub Jelinek <jakub@redhat.com>; gcc@gcc.gnu.org; gcc-patches <gcc-
>> patches@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

[Tamar Christina]

Thanks both!

Cheers,
Tamar

> -----Original Message-----
> From: Martin Liška <mliska@suse.cz>
> Sent: Wednesday, June 10, 2020 2:41 PM
> To: Tamar Christina <Tamar.Christina@arm.com>; Jonathan Wakely
> <jwakely.gcc@gmail.com>
> Cc: Jakub Jelinek <jakub@redhat.com>; gcc@gcc.gnu.org; gcc-patches <gcc-
> patches@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 <gcc-patches-bounces@gcc.gnu.org> On Behalf Of
> >> Martin Liška
> >> Sent: Wednesday, June 10, 2020 8:38 AM
> >> To: Jonathan Wakely <jwakely.gcc@gmail.com>
> >> Cc: Jakub Jelinek <jakub@redhat.com>; gcc@gcc.gnu.org; gcc-patches
> >> <gcc- patches@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