[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
bugzilla-daemon at webkit.org
Mon May 28 04:53:53 PDT 2007
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.
More information about the webkit-reviews
mailing list