[webkit-reviews] review requested: [Bug 21457] resolve-ChangeLogs should be able to operate on a git revision range : [Attachment 24300] Patch v2

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has asked Adam Roben (aroben)
<aroben at apple.com> for review:
Bug 21457: resolve-ChangeLogs should be able to operate on a git revision range
https://bugs.webkit.org/show_bug.cgi?id=21457

Attachment 24300: Patch v2
https://bugs.webkit.org/attachment.cgi?id=24300&action=edit

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
Patch v2

(In reply to comment #2)
> (From update of attachment 24181 [edit])
>  87 Usage: @{[ basename($0) ]} -f|--fix-merge [revision-range]
>  88	     @{[ basename($0) ]} [options] path/to/ChangeLog
> [path/to/another/ChangeLog ...]
> 
> I think it makes sense to reverse the order of these two examples, since the
> second one is the more common usage for non-git users.

Updated.  The -f|--fix-merged switch actually takes an optional argument now
(the range) as well as a list of ChangeLog files, so having separate lines
doesn't make sense anymore.

>  264	   exec("$GIT filter-branch --tree-filter 'PREVIOUS_COMMIT=\`$GIT
> rev-parse \$GIT_COMMIT^\` && MAPPED_PREVIOUS_COMMIT=\`map \$PREVIOUS_COMMIT\`

> $0 -f \"" . join('" "', @changeLogs) . "\"' $revisionRange");
> 
> Does exec have a multi-argument form? Would using that make this easier to
> read?

I tried using the multi-argument form, but I think it breaks due to the way git
re-invokes the command (since git-filter-branch doesn't exist anymore).  The
exec() changed to system() anyway to support removal of .git/refs/original.

>  384	   if (!$fileMine || !$fileOlder || !$fileNewer) {
>  385	       next;
>  386	   }
> 
> I know this code just got moved, but I think this would be a little easier to

> read as:
> 
> next unless $fileMine && $fileOlder && $fileNewer;

Using "next" here was actually a bug--I extracted this code from a for() loop
into a method, but forgot to change the "next" into an early return statement. 
Thanks!

(In reply to comment #5)
> It sounds like removing .git/refs/original when the command completes
> successfully is the right way to go. If the command fails, ideally we would
> reset the repository back to the way it was before the command was run (which

> of course includes deleting .git/refs/original). If we can't do that we
should
> at least print out an informative error message describing how the user can
> recover (and of course mention that .git/refs/original needs to be deleted).

The .git/refs/original directory is removed on a successful run of git
filter-branch.	The refs aren't changed if the filter-branch operation errors
out, so there is nothing for the script to do in that case.

Also note that this patch fixes all known issues with the example you provided
offline.