Page MenuHomePhabricator

--range support for git
ClosedPublic

Authored by talshiri on Jun 3 2014, 11:13 PM.
Tags
None
Referenced Files
F12837133: D9369.id22592.diff
Thu, Mar 28, 4:37 PM
Unknown Object (File)
Sun, Mar 24, 11:25 AM
Unknown Object (File)
Tue, Feb 27, 11:14 PM
Unknown Object (File)
Feb 15 2024, 1:01 AM
Unknown Object (File)
Feb 4 2024, 3:55 AM
Unknown Object (File)
Jan 4 2024, 9:41 AM
Unknown Object (File)
Dec 24 2023, 11:44 PM
Unknown Object (File)
Dec 22 2023, 8:41 PM
Subscribers

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
Branch
range_support
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 838
Build 838: [Placeholder Plan] Wait for 30 Seconds

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
172–175

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.

323–326

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

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

339

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
398–402

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.

400

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

435–438

(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
172–175

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.

339

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
398–402

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.

435–438

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
79

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.

84

Can this be private?

87

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

112

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.

113–114

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

305–307

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

src/workflow/ArcanistDiffWorkflow.php
436

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

440

"ranged" -> "Ranges"

src/workflow/ArcanistWhichWorkflow.php
98 ↗(On Diff #22575)

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

130 ↗(On Diff #22575)

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
339

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
339

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