Bug 13884

Created attachment 14739 [details] patch for prepare-ChangeLog

*** This bug has been marked as a duplicate of 13732 ***

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 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.

(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.

Created attachment 14760 [details] second version of prepare-ChangeLog patch

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.

Created attachment 14848 [details] third version of prepare-ChangeLog patch

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 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; }

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 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)

Created attachment 15182 [details] fourth version of patch

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 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?

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?

(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.

(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")

Created attachment 15186 [details] fifth version

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 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.

(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).

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).