Re: [PATCH 1/2] gitlog-to-changelog: support multi-author commits.

[Gary V. Vaughan]

Hi Jim,

Sorry for the long wait.  With the flooding in Bangkok affecting everything,
I've had very spotted access to the Internet, otherwise I would have responded
sooner.

On 1 Nov 2011, at 20:09, Jim Meyering wrote:

> Gary V. Vaughan wrote:
>> More generally useful gnulib-local goodness from my Libtool `next' branch:
>> 
>> I'm sure this is far from idiomatic Perl, but I'd very much like for this
>> patch or something similar to be pushed so that FSF projects have a means
>> to correctly track multiple patch authors with a generated ChangeLog file.
> 
> I like the idea.
> 
>> The idea is to add bare 'Co-authored-by: ' lines to the git log message,
> 
> You mentioned consensus.
> What other projects already use that syntax?

I found references to it in debbugs and bzr.  It's all on the first few pages
of google search results for 'git commit multiple authors'.

Thanks for the review. I'm afraid my Perl skills amount to googling for cut'n'
paste snippets, because I try to avoid writing in Perl whenever there's any
alternative.  If you'd like to reformat these patches to your preferred style
and commit them yourself rather than help me to write them idiomatically, that
would be splendid!

>> @@ -124,6 +124,7 @@ sub quoted_cmd(@)
>>             . "(Is your Git too old?  Version 1.5.1 or later is 
>> required.)\n");
>> 
>>   my $prev_date_line = '';
>> +  my @prev_coauthors = ();
> 
> Please drop the initializer.

I've left it in: Without it, I get the following errors:

Possible unintended interpolation of @prev_coauthors in string at 
./build-aux/gitlog-to-changelog line 272.
Global symbol "@prev_coauthors" requires explicit package name at 
./build-aux/gitlog-to-changelog line 272.
Global symbol "@prev_coauthors" requires explicit package name at 
./build-aux/gitlog-to-changelog line 281.
Execution of ./build-aux/gitlog-to-changelog aborted due to compilation errors. 
                     

>>   while (1)
>>     {
>>       defined (my $in = <PIPE>)
>> @@ -146,18 +147,28 @@ sub quoted_cmd(@)
>>           . "(expected date/author/email):\n$author_line\n";
>> 
>>       my $date_line = sprintf "%s  $2\n", strftime ("%F", localtime ($1));
>> -      # If this line would be the same as the previous date/name/email
>> -      # line, then arrange not to print it.
>> -      if ($date_line ne $prev_date_line)
>> +
>> +      # Format 'Co-authored-by: A U Thor <address@hidden>' lines in
>> +      # standard multi-author ChangeLog format.
>> +      my @coauthors = grep /^Co-authored-by:.*>$/, @line;
> 
> Here you require only the trailing ">", yet below, the
> transformation happens only if there is also a "<".
> This is different from Signed-off-by: in that if we ignore a
> line because it lacks an email address, then that probably
> deserves a diagnostic.

Done.  I've also added a warning for missing email addresses.

> Any annotation that we standardize like this should be
> syntax-checked via a git log hook like the one I recently
> added to coreutils (see scripts/git-hooks).

Sure.  The coreutils commit-msg hook has some portability issues on Mac OS X,
so while I modelled this commit-msg on the coreutils one, I tried to rewrite
the parts that didn't work OOTB on my Mac to be portable.  Feel free to crib
those portable parts of this one into coreutils, or reformat this one to
coreutils style as you prefer.

> An alternative is to accept anything after the ":" and then, to use
> "s/\s*</  </" to ensure that the number of spaces before the "<" is two.
> 
>> +      s/^Co-authored-by:\s*([^<]+)\s+</\t    \1  </
> 
> Please use $1, not \1.

I thought $1 was a positional parameter?  I'm not sure what the advantage of
choosing a different back-reference syntax to other unix regexp using tools
is, but no matter... after taking your other suggestions, this line doesn't
exist any more :)

>> +        for (@coauthors);
> 
>> +
>> +      # If this header would be the same as the previous date/name/email/
>> +      # coauthors header, then arrange not to print it.
>> +      if ($date_line ne $prev_date_line or "@coauthors" ne 
>> "@prev_coauthors")
> 
> s/or/||/

Done.

>>         {
>>           $prev_date_line eq ''
>>             or print "\n";
>>           print $date_line;
>> +          print join ("\n", @coauthors), "\n"
>> +            unless (@coauthors == 0);
> 
> Please write that so that the conditional part is indented:
> 
>             @coauthors
>               and print join ("\n", @coauthors), "\n"

Done, I also added a semi-colon to the end of that line, and rebased against
current master.

Okay to push the following?

From 73131d956d8da994bae5f59e321548ba26ddd566 Mon Sep 17 00:00:00 2001
From: "Gary V. Vaughan" <address@hidden>
Date: Tue, 1 Nov 2011 17:58:37 +0700
Subject: [PATCH 1/2] gitlog-to-changelog: support multi-author commits.

The FSF cares about keeping track of all authors of patches to its
projects, but Git doesn't provide obvious support for multi-author
changesets. Consensus seems to be forming around the use of extra
Signed-off-by inspired lines in the log message formatted as
`Co-authored-by: A U Thor <address@hidden>' for round-tripping
multi-author commits between version control systems.
* gitlog-to-changelog: Extract `Co-authored-by:' lines from the git
log message and output in standard ChangeLog multi-author format.
Reported by Peter Rosin <address@hidden>
---
 ChangeLog                     |   13 ++++++
 build-aux/gitlog-to-changelog |   27 +++++++++++--
 scripts/git-hooks/commit-msg  |   85 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 121 insertions(+), 4 deletions(-)
 create mode 100755 scripts/git-hooks/commit-msg

diff --git a/ChangeLog b/ChangeLog
index 962b2c4..49d4f72 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,16 @@
+2011-11-01  Gary V. Vaughan  <address@hidden>
+
+       gitlog-to-changelog: support multi-author commits.
+       The FSF cares about keeping track of all authors of patches to its
+       projects, but Git doesn't provide obvious support for multi-author
+       changesets. Consensus seems to be forming around the use of extra
+       Signed-off-by inspired lines in the log message formatted as
+       `Co-authored-by: A U Thor <address@hidden>' for round-tripping
+       multi-author commits between version control systems.
+       * gitlog-to-changelog: Extract `Co-authored-by:' lines from the git
+       log message and output in standard ChangeLog multi-author format.
+       Reported by Peter Rosin <address@hidden>
+
 2011-11-13  Bruno Haible  <address@hidden>
            Jim Meyering  <address@hidden>
 
diff --git a/build-aux/gitlog-to-changelog b/build-aux/gitlog-to-changelog
index 56c7e4e..9d98b82 100755
--- a/build-aux/gitlog-to-changelog
+++ b/build-aux/gitlog-to-changelog
@@ -200,6 +200,7 @@ sub parse_amend_file($)
             . "(Is your Git too old?  Version 1.5.1 or later is required.)\n");
 
   my $prev_date_line = '';
+  my @prev_coauthors = ();
   while (1)
     {
       defined (my $in = <PIPE>)
@@ -249,18 +250,36 @@ sub parse_amend_file($)
           . "(expected date/author/email):\n$author_line\n";
 
       my $date_line = sprintf "%s  $2\n", strftime ("%F", localtime ($1));
-      # If this line would be the same as the previous date/name/email
-      # line, then arrange not to print it.
-      if ($date_line ne $prev_date_line)
+
+      # Format 'Co-authored-by: A U Thor <address@hidden>' lines in
+      # standard multi-author ChangeLog format.
+      my @coauthors = grep /^Co-authored-by:.*$/, @line;
+      for (@coauthors)
+        {
+          s/^Co-authored-by:\s*/\t    /;
+          s/\s*</  </;
+
+          $_ =~ /<address@hidden>/
+            or warn "$ME: warning: Missing email address for "
+              . substr ($_, 5) . ".\n";
+        }
+
+      # If this header would be the same as the previous date/name/email/
+      # coauthors header, then arrange not to print it.
+      if ($date_line ne $prev_date_line or "@coauthors" ne "@prev_coauthors")
         {
           $prev_date_line eq ''
             or print "\n";
           print $date_line;
+          @coauthors
+            and print join ("\n", @coauthors), "\n";
         }
       $prev_date_line = $date_line;
+      @prev_coauthors = @coauthors;
 
-      # Omit "Signed-off-by..." lines.
+      # Omit "Co-authored-by..." and "Signed-off-by..." lines.
       @line = grep !/^Signed-off-by: .*>$/, @line;
+      @line = grep !/^Co-authored-by: /, @line;
 
       # Remove leading and trailing blank lines.
       if (@line)
diff --git a/scripts/git-hooks/commit-msg b/scripts/git-hooks/commit-msg
new file mode 100755
index 0000000..5efdf1f
--- /dev/null
+++ b/scripts/git-hooks/commit-msg
@@ -0,0 +1,85 @@
+#!/bin/sh
+# An example hook script for catching duplicate or malformed
+# Co-authored-by lines in the commit message.
+
+: ${SED="sed"}
+test "${ECHO+set}" = set || ECHO='printf %s\n'
+
+basename="$SED -e "'s|^.*/||'
+
+nl='
+'
+
+progpath="$0"
+progname=`$ECHO "$progpath" |$basename`
+
+log_file=$1
+export log_file
+
+fn_error ()
+{
+    prefix="$progname: error: "
+
+    save_IFS=$IFS
+    IFS=$nl
+    for line in $*; do
+      IFS=$save_IFS
+      $ECHO "$prefix$line" 1>&2
+      prefix="$progname:        "
+    done
+    IFS=$save_IFS
+}
+
+fn_re_edit ()
+{
+    $ECHO 'Press return to edit. Ctrl-C to abort...' >&2
+    read v
+    ${EDITOR-vi} "$log_file"
+}
+
+fn_rewrite ()
+{
+    # Output once to stderr
+    fn_error "$*"
+
+    # And again as a comment in the log_file ready for re-editing
+    $ECHO "$*" |$SED 's,^,# ,'
+    echo
+    cat "$log_file"
+}
+
+fn_check_msg ()
+{
+    return_status=0
+
+    CAB_re='^Co-authored-by: '
+
+    # Flag duplicated Co-authored-by lines.
+    dups=`grep "$CAB_re" "$log_file" 2>/dev/null \
+        |sort |uniq -c |sed -e '/^[     ]*1[    ]/d'`
+    
+    test -n "$dups" && {
+       $ECHO 'Duplicate Co-authored-by lines:
+'"$dups"
+       return_status=1
+    }
+
+    # Make sure each Co-authored-by line contains a valid email.
+    email_re='<address@hidden>'
+
+    grep "$CAB_re" "$log_file" 2>/dev/null \
+        |while read CAB; do
+            test 0 -eq `expr "$CAB" : ".*$email_re"` && {
+                echo "Malformed or missing email in \`$CAB'"
+               return_status=1
+            }
+         done
+
+    return $return_status
+}
+
+while :; do
+    err=`fn_check_msg` && break
+    fn_rewrite "$err" > "${log_file}T" && mv "${log_file}T" "$log_file"
+    fn_re_edit
+done
-- 
1.7.7.3



Cheers,
-- 
Gary V. Vaughan (gary AT gnu DOT org)