Page MenuHomePhabricator

--range support for git
ClosedPublic

Authored by talshiri on Jun 3 2014, 11:13 PM.

Details

Summary

This adds support for passing range of commits for arc diff. This is useful when you want to submit code reviews for past commits without mucking around with the working copy.

This will probably require changes :)

Test Plan

Tested locally, but totally need to add tests for this

Diff Detail

Repository
rARC Arcanist
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

talshiri retitled this revision from to --range support for git.
talshiri updated this object.
talshiri edited the test plan for this revision. (Show Details)
talshiri added a reviewer: epriestley.
talshiri edited edge metadata.

Fixed diff being reversed :P

epriestley edited edge metadata.

Some inlines, and a couple of other thoughts:

  • If "head" and "base" are the same, we should probably raise an explicit error. For example, --range HEAD..HEAD.
  • If "head" is not a descendant of "base", we should raise an explicit error. For example, --range HEAD^..HEAD^^^. The diff commands will work (producing reverse diffs) but I think the log command will not. If users actually want this feature we could maybe build it eventually, but I'd guess that 99% of invocations like this are mistakes and the other 1% are misguided. Users who want to do this can use --raw already, which seems fine since the local commit info isn't really that meaningful.
  • arc which should support this flag too.

Generally, this looks pretty reasonable overall.

src/repository/api/ArcanistGitAPI.php
200–203

This is a little misleading because the user may not have specified the symbolic head, but the message will say they did.

I think it would be better to default the symbolic head to null, have getHeadCommit() deal with resolution, and then specialize the error messages to be explicit about what was specified.

350–353

Minor nits:

  • Don't put text on the /** line; start on the next line.
  • For consistency, use full sentences with periods and such, both in the main documentation and in the parameter/return documentation.
  • The use of "HEAD^" is a bit confusing to me, consider something like: (like "HEAD^")
  • Don't specify variable names, just types (@param string The symbolic commit to resolve.). This is unique to this codebase, but the parameter names are completely redundant with the method signature and I think they're not very useful so we don't use them.
355

$commit_hash or similar might be a better name for this variable than $merge_base?

412

I don't think we let you get here with a dirty working copy, but this will change the behavior if we do.

src/workflow/ArcanistDiffWorkflow.php
397–401

Instead of --range, maybe this should be something like --head, --tip, --to, or similar? Particularly:

  • Calls like arc diff X --range Y..Z are confusing/ambiguous.
  • Calls like arc diff --base X --range Y..Z are confusing/ambiguous.
  • a..b is a valid branch name in Mercurial.
  • Generally, there's no way to say "use my base rule normally, just stop at HEAD^" without a lot of extra work. This seems somewhat useful to support.

On the other side of things, it seems easy to run arc diff X --head Y instead of arc diff --range X..Y, so we aren't losing anything by doing this.

399

This should use full sentences and explicitly state that features will be disabled.

448–461

(Probably mooted by the above, but this could use some polish if it sticks around. For example, --range quack will explode right now, I think.)

This revision now requires changes to proceed.Jun 5 2014, 6:03 PM
src/repository/api/ArcanistGitAPI.php
200–203

But the original message explicitly mentioned HEAD, which is the default here.

getHeadCommit() is problematic, since it resolves it to the commit hash, which is more confusing.

412

Oh, that's true. Should I alter this so it will diff against the passed commit if --range isn't being used? Is it ever desirable to diff against working copy?

src/workflow/ArcanistDiffWorkflow.php
397–401

I mostly wanted to keep it inline with git syntax, since it's well known (if a little confusing) at this point, and it's nice being able to do:

$ git log a..b
(verify output)
$ arc diff --range a..b
and know that you'll send those exact commits in.

I like --head, though, and all of your points are valid. I'll switch it over to --head.
Would it be reasonable to add git-specific syntax when you're in git repos? something like --git-range? It creates ambiguity (--head X --range a..b), but I like the ability to use the same range specifier as with normal git commands.

448–461

Yeah it will explode :)

talshiri edited edge metadata.
  • changed "--range" to "--head"

Are there any outstanding issues here that I need to fix before it can land?

epriestley edited edge metadata.

Some inlines, but pretty much all nitpicks.

src/repository/api/ArcanistGitAPI.php
81

This documentation should probably be explicit about what happens when $parent and $child are the same. Notably, in Mercurial, a commit is a descendant of itself.

86

Can this be private?

89

You can just trim(), there won't be any \2 in the output in this case.

129

When an error is a user's fault, throw ArcanistUsageException instead of Exception.

Generally, strings should be full sentences: use uppercase initial letters and terminal punctuation.

130–131

This message is flipped -- "base commit {$head_commit}" should be "base commit {$base_commit}".

332–334

Since this is in a final/leaf class and Git supports commit ranges, you can omit this check.

src/workflow/ArcanistDiffWorkflow.php
448

(null should be the default-by-default, and can be omitted?)

452

"ranged" -> "Ranges"

src/workflow/ArcanistWhichWorkflow.php
97

For consistency, prefer {$x} over ${x}.

130–131

This should use the specified HEAD.

This revision now requires changes to proceed.Jun 10 2014, 7:56 PM
talshiri edited edge metadata.
  • cleaned up patch
talshiri edited edge metadata.
  • cleaned up patch
epriestley edited edge metadata.

An issue came to me in a dream. I think this is good to go otherwise.

src/repository/api/ArcanistGitAPI.php
412

Sleeping on it, I realize we do actually diff against the dirty working copy in one case: if you run arc lint, we'll call ArcanistBaseWorkflow::getChangedLines() and end up here, expecting to use the full diff (including dirty changes in the working copy) to figure out which lines have been changed by the user, so we can filter/adjust messages (e.g., not show warnings on unchanged lines).

This will alter the behavior, causing us to ignore dirty changes in the working copy.

I think ArcanistGitAPI::getAllLocalChanges() can be tweaked to pass a flag to getFullGitDiff(), which can then do a one-argument diff (i.e., no end of the range).

This is a little messy, but as far as I know there's no Y we can write in X..Y to mean "changes since X, including dirty changes in the working copy", so I think this ultimately needs to have two flavors of invocation anyway. This is also easy to clean up later if it turns out to not have made much sense.

This revision now requires changes to proceed.Jun 11 2014, 1:07 PM

Haha!

src/repository/api/ArcanistGitAPI.php
412

Ah yes. Ok, let me take care of it.

talshiri edited edge metadata.
  • getFullGitDiff() now explicitly gets base and head revisions. This provides a slightly cleaner API, and allows fetching a diff against the working copy.

(I think this is just the last two commits on the branch? arc which should be able to figure out what's up if it's unclear.)

talshiri edited edge metadata.

send all commits

epriestley edited edge metadata.
This revision is now accepted and ready to land.Jun 11 2014, 9:36 PM
epriestley updated this revision to Diff 22714.

Closed by commit rARC6b192f3178f9 (authored by @talshiri, committed by @epriestley).