Re: [IMPORTANT] ChangeLog related changes

[Jonathan Wakely <jwakely.gcc@gmail.com>]

[-- 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')