[webkit-reviews] review denied: [Bug 13884] patch for prepare-ChangeLog to populate ChangeLog files from a git commit : [Attachment 14739] patch for prepare-ChangeLog

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Simon Hausmann
<hausmann at kde.org>'s request for review:
Bug 13884: patch for prepare-ChangeLog to populate ChangeLog files from a git
commit
http://bugs.webkit.org/show_bug.cgi?id=13884

Attachment 14739: patch for prepare-ChangeLog
http://bugs.webkit.org/attachment.cgi?id=14739&action=edit

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
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.