Bug 13884 - patch for prepare-ChangeLog to populate ChangeLog files from a git commit
Summary: patch for prepare-ChangeLog to populate ChangeLog files from a git commit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 523.x (Safari 3)
Hardware: Other OS X 10.4
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-05-26 12:17 PDT by Simon Hausmann
Modified: 2007-06-23 01:55 PDT (History)
2 users (show)

See Also:


Attachments
patch for prepare-ChangeLog (6.66 KB, patch)
2007-05-26 12:18 PDT, Simon Hausmann
ddkilzer: review-
Details | Formatted Diff | Diff
second version of prepare-ChangeLog patch (7.87 KB, patch)
2007-05-28 13:07 PDT, Simon Hausmann
no flags Details | Formatted Diff | Diff
third version of prepare-ChangeLog patch (7.88 KB, patch)
2007-06-03 06:55 PDT, Simon Hausmann
timothy: review-
Details | Formatted Diff | Diff
fourth version of patch (8.46 KB, patch)
2007-06-22 06:49 PDT, Simon Hausmann
no flags Details | Formatted Diff | Diff
fifth version (8.34 KB, patch)
2007-06-22 12:18 PDT, Simon Hausmann
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Hausmann 2007-05-26 12:17:17 PDT
The attached patch extends prepare-ChangeLog to fill out the ChangeLog files a given git commit affects. The changelog message is taken from the git commit log and the reviewer can either be specified on the commandline using --git-reviewer=<name> or it can be automatically extracted from the git commit using the git typical Signed-Off-By: lines (as git commit -s conveniently creates them).

This simplifies the use of git(-svn) with the WebKit svn repository by delaying the changelog population. Rebasing git commits that have also the changelog files filled out is a very painful operation because it constantly produces merging conflicts.
Comment 1 Simon Hausmann 2007-05-26 12:18:27 PDT
Created attachment 14739 [details]
patch for prepare-ChangeLog
Comment 2 David Kilzer (:ddkilzer) 2007-05-27 19:19:13 PDT

*** This bug has been marked as a duplicate of 13732 ***
Comment 3 David Kilzer (:ddkilzer) 2007-05-28 03:52:00 PDT
Sorry, still thinking in terms of an svn repository (in which using prepare-ChangeLog on a remote repo does no good since everything would already be committed).

Comment 4 David Kilzer (:ddkilzer) 2007-05-28 04:53:52 PDT
Comment on attachment 14739 [details]
patch for prepare-ChangeLog

Now that I understand what this is for, the patch makes more sense!

> my $showHelp = 0;
> my $updateChangeLogs = 1;
> my $spewDiff = $ENV{"PREPARE_CHANGELOG_DIFF"};
>+my $gitCommit = 0;
>+my $gitReviewer = "";

Please keep these variables in alphabetical order (and feel free to move $spewDiff).

> my $parseOptionsResult =
>     GetOptions("diff|d!" => \$spewDiff,
>                "help|h!" => \$showHelp,
>                "open|o!" => \$openChangeLogs,
>-               "update!" => \$updateChangeLogs);
>+               "update!" => \$updateChangeLogs,
>+               "git-commit:s" => \$gitCommit,
>+               "git-reviewer:s" => \$gitReviewer);

Alphabetical order, please!

Also, please update the $showHelp section with documentation about these new switches.

>+            if ($gitCommit) {
>+                if (/^([A-Z])\t(.+)$/) {
>+                    $status = $1;
>+                    $file = $2;
>+                } else {
>+                    print;  # error output from svn stat
>+                }

Instead of using "[A-Z]" I would prefer it if you only listed "supported" statuses.  (I'm working on a patch to do something similar for svn statuses.)

This code should also be able to determine if a file was copied from another file (either due to renaming or copying).

If it gets too long, please break it out into a separate subroutine.

>+    $name = `git log --max-count=1 --pretty=\"format:%an\" \"$gitCommit\"`;
>+    $email_address = `git log --max-count=1 --pretty=\"format:%ae\" \"$gitCommit\"`;

Please use $GIT instead of "git" here.

>+    if ($gitCommit) {
>+        my $gitLog = `git cat-file commit \"$gitCommit\"`;

And here.

>+        my @lines = split(/\n/, $gitLog);
>+
>+        my $reviewer = "";
>+
>+        $gitLog = "";
>+        my $inHeader = 1;
>+        foreach my $line (@lines) {
>+            if ($inHeader) {
>+                if (!$line) {
>+                    $inHeader = 0;
>+                }
>+                next;
>+            }
>+            if ($line =~ /Signed-off-by: (.+)/) {
>+                $reviewer = $1;
>+                $reviewer =~ s/(.+)\s<.*>/$1/;
>+            } elsif (length $line == 0) {
>+                $gitLog = $gitLog . "\n";
>+            } else {
>+                $gitLog = $gitLog . "        " . $line . "\n";
>+            }
>+        }
>+
>+        if (!$reviewer) {
>+            $reviewer = $gitReviewer;
>+        }
>+
>+        if (!$reviewer) {
>+            print "WARNING!!! Change was not reviewed!\n";
>+            $reviewer = "NOBODY (OO" . "PS!)";
>+        }
>+
>+        print CHANGE_LOG "        Reviewed by $reviewer.\n\n";
>+        print CHANGE_LOG $gitLog . "\n";

Can more than one person sign-off on a patch?  If so, this code should handle that case as well.

Overall it looks good!  I'd like Adam or Timothy to test the git changes as well when the final patch is approved.
Comment 5 David Kilzer (:ddkilzer) 2007-05-28 05:50:59 PDT
(In reply to comment #4)
> Instead of using "[A-Z]" I would prefer it if you only listed "supported"
> statuses.  (I'm working on a patch to do something similar for svn statuses.)

See Attachment 14756 [details] on Bug 10342.
Comment 6 Simon Hausmann 2007-05-28 13:07:20 PDT
Created attachment 14760 [details]
second version of prepare-ChangeLog patch
Comment 7 Simon Hausmann 2007-05-28 13:08:40 PDT
I hope all comments are addressed in the latest version of the patch. File renames are now handled as well. The patch itself applies on top of the one in bug 10342.
Comment 8 Simon Hausmann 2007-06-03 06:55:20 PDT
Created attachment 14848 [details]
third version of prepare-ChangeLog patch
Comment 9 Simon Hausmann 2007-06-03 06:56:59 PDT
I have attached a third version of the patch that makes the script more tolerant against different cases for the signed-off line (changed the Signed-Off-By: .+ regex basically to [Ss]igned-[Oo]ff-[Bb]y: .+), since that seems to happen sometimes.
Comment 10 Timothy Hatcher 2007-06-15 07:14:39 PDT
Comment on attachment 14848 [details]
third version of prepare-ChangeLog patch

These two changes seem wrong. Why are you changing the line that has isSVN() and testing for $gitCommit there? Shouldn't that be on the line with isGit()?

@@ -999,7 +1074,7 @@ sub isModifiedOrAddedStatus($)
         "renamed" => 1,
     );
 
-    return $svn{$status} if isSVN();
+    return $svn{$status} if (isSVN() || $gitCommit);
     return $git{$status} if isGit();
 }
 
@@ -1037,7 +1112,7 @@ sub statusDescription($$)
         "renamed" => " Renamed from \%s.",
     );
 
-    return sprintf($svn{$status}, $original) if isSVN() && exists $svn{$status};
+    return sprintf($svn{$status}, $original) if (isSVN() || $gitCommit) && exists $svn{$status};
     return sprintf($git{$status}, $original) if isGit() && exists $git{$status};
     return undef;
 }
Comment 11 Simon Hausmann 2007-06-17 12:58:57 PDT
Those two places deliberately use the svn codes for figure out what changed with the given file because the output of git diff on a tree with --name-status is compatible with the svn status codes. When using prepare-ChangeLog on a change that is in the working directory and/or the git index a different command (git status) is used to determine the status of the changed files.
Comment 12 Adam Roben (:aroben) 2007-06-21 16:25:27 PDT
Comment on attachment 14848 [details]
third version of prepare-ChangeLog patch

+            return "$GIT diff \"$gitCommit\" \"$gitCommit^\"";

The order of these arguments seems reversed to me -- won't this give you a reversed diff?

+            $command = "$GIT diff -r --name-status -M \"$gitCommit\" \"$gitCommit^\"";

Ditto.

+                } elsif (/^(R)[0-9]{1,3}\t([^\t]+)\t([^\t\n]+)$/) {

It would be nice to have an example of a line this would match (I know I didn't do this for some other gnarly regexps in this script, but we need to start somewhere)
Comment 13 Simon Hausmann 2007-06-22 06:49:00 PDT
Created attachment 15182 [details]
fourth version of patch
Comment 14 Simon Hausmann 2007-06-22 06:50:47 PDT
You're right, the ^ was misplaced (which caused the bug that added files were recorded as removed in the ChangeLog).

I've updated the patch to fix this problem and I noticed that there was one difference in the svn status and git --name-status output.
Comment 15 Adam Roben (:aroben) 2007-06-22 10:15:46 PDT
Comment on attachment 15182 [details]
fourth version of patch

+    my %gitCommitStatus = (
+        "A" => 1,
+        "M" => 1,
+        "D" => 1,
+    );

I don't think "D" is an "added or modified" status, while "R" is, so why do we need to use something different from %svnStatus?
Comment 16 Simon Hausmann 2007-06-22 11:51:09 PDT
AFAIK svn uses R for replaced, which git doesn't use. The reason why I've added "D" is to handle the case when a commit consists only of file deletions. That is of course strictly speaking not a modified or added state, but if it's not handled here then prepare-ChangeLog thinks nothing changed at all, which isn't correct either. I've just tried with a svn checkout and the same happens there, too (svn rm WebCore/Makefile; WebKitTools/Scripts/prepare-ChangeLog -> No changes found).

If you prefer I could merge the two arrays again (ignoring that git doesn't use 'R') and add 'D' as state. Is that ok?
Comment 17 Adam Roben (:aroben) 2007-06-22 12:03:31 PDT
(In reply to comment #16)
> AFAIK svn uses R for replaced, which git doesn't use.

   Actually, git confusingly uses R to mean "renamed" rather than "replaced". This is probably a bug that should be fixed in git, since --name-status is striving to be svn-compatible. But it does seem that "R" should be in this map.

> The reason why I've added
> "D" is to handle the case when a commit consists only of file deletions. That
> is of course strictly speaking not a modified or added state, but if it's not
> handled here then prepare-ChangeLog thinks nothing changed at all, which isn't
> correct either. I've just tried with a svn checkout and the same happens there,
> too (svn rm WebCore/Makefile; WebKitTools/Scripts/prepare-ChangeLog -> No
> changes found).

   Ah, that's a problem I've noticed before as well. However, I think just adding "D" to this map will cause the undesired effect of showing _every_ deleted file beneath a deleted directory, so we need to be a bit smarter about this case. I think for now I'd prefer leaving just %svnStatus as is and using it for the git-commit case. Then we can file and fix the "doesn't show deleted files" bug separately.
Comment 18 Adam Roben (:aroben) 2007-06-22 12:13:46 PDT
(In reply to comment #17)
>    Actually, git confusingly uses R to mean "renamed" rather than "replaced".
> This is probably a bug that should be fixed in git, since --name-status is
> striving to be svn-compatible.

Seems my assumption may have been wrong and this may be intended behavior. Here are the codes git uses (from Documentation/diff-format.txt):

M: in-place edit
C: copy-edit
R: rename-edit
A: create
D: delete
U: unmerged

   So, isAddedOrModifiedStatus should return true for M, C, R, A, which should be the same as for svn (though it looks like currently the svn case is missing "C")

Comment 19 Simon Hausmann 2007-06-22 12:18:30 PDT
Created attachment 15186 [details]
fifth version
Comment 20 Simon Hausmann 2007-06-22 12:21:28 PDT
Agreed and changed accordingly.

If you want I can try to port the existing support for populating the changelog from the output of git status to git diff -r --name-status HEAD. That should remove some code again in total. (I'd like to do it in a second independent patch though)
Comment 21 Adam Roben (:aroben) 2007-06-22 12:45:54 PDT
Comment on attachment 15186 [details]
fifth version

+            $command = "$GIT diff -r --name-status -M \"$gitCommit^\" \"$gitCommit\"";

   We should probably pass -C here as well.

     return $git{$status} if isGit();
+    return 0 if $gitCommit; # an existing commit cannot have conflicts

   Shouldn't these two lines be reversed?

+    # deliberately share the same description between svn status output and git diff --name-status
+    return sprintf($svn{$status}, $original) if (isSVN() || $gitCommit) && exists $svn{$status};

   I guess we now know that the description for "R" is different for the $gitCommit case.

   These are all pretty minor, so r=me either way, though I'd like to see them addressed at some point.
Comment 22 Adam Roben (:aroben) 2007-06-22 12:46:34 PDT
(In reply to comment #20)
> If you want I can try to port the existing support for populating the changelog
> from the output of git status to git diff -r --name-status HEAD. That should
> remove some code again in total. (I'd like to do it in a second independent
> patch though)

   Yes, I think that would be great (I probably would have gone that route had I known of --name-status initially).
Comment 23 Simon Hausmann 2007-06-23 01:55:36 PDT
Committed in r23748.

I'll try to address the -C in a second patch because with -C the status handling also needs to be adjusted (svn status would show 'C' for a conflict but with git diff -C --name-status 'C' means copied).