[webkit-reviews] review granted: [Bug 21457] resolve-ChangeLogs should be able to operate on a git revision range : [Attachment 24181] Patch v1

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted David Kilzer (ddkilzer)
<ddkilzer at webkit.org>'s request 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 24181: Patch v1
https://bugs.webkit.org/attachment.cgi?id=24181&action=edit

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
 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.

 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?

 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;

r=me on this version of the patch, but let's keep talking about the issues you
raised.