[Webkit-unassigned] [Bug 13884] patch for prepare-ChangeLog to populate ChangeLog files from a git commit

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=13884


ddkilzer at webkit.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #14739|review?                     |review-
               Flag|                            |




------- Comment #4 from ddkilzer at webkit.org  2007-05-28 04:53 PDT -------
(From update of attachment 14739)
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.


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.