Page MenuHomePhabricator

Make "arc diff" sort of detect dependent revisions
ClosedPublic

Authored by epriestley on Sep 27 2017, 5:05 PM.

Details

Summary

Depends on D18645. Fixes T11343. Currently, when you "arc diff", we don't detect dependencies even if your working copy is on top of another open revision you authored.

Use the new Ref/Hardpoint infrastructure from the experimental branch to look up the revision associated with the base commit when you run arc diff. If it exists and there's exactly one open revision you authored associated with it, add "Depends on ..." to the summary. This is a bit rough but the whole experimental branch is a bit of a wilderness for now.

Also remove --cache (passthru flag for arc lint --cache, which I removed in D18643) and getUnderlyingWorkingCopyRevision() (no callsites).

(This won't yet work in Mercurial, and does not make sense in SVN.)

Test Plan

Created this revision, got an auto "depends on" up top there.

Diff Detail

Repository
rARC Arcanist
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

  • Also fix a flipped condition in arc lint around all/changed paths.
amckinley added inline comments.
src/workflow/ArcanistDiffWorkflow.php
2929–2932

Is there some tricky reason why this would be a bad thing? Seems like depending on someone else's revision is a reasonable workflow.

This revision is now accepted and ready to land.Sep 27 2017, 5:25 PM

There's no technical reason you can't depend on someone else's revision.

I think actual cases might be mistakes more often than intentional, though. For example, you ran arc patch and forgot that your working copy was dirty with someone else's junk, or someone else has landed a change and you've pulled it, but the server side is misconfigured and the revision hasn't closed yet. So this version of the rule seems a little less likely to misfire, at least to start with. If users are actually building changes that depend on one anothers' unlanded changes, we could revisit this rule once we're more confident the simpler cases work.

This revision was automatically updated to reflect the committed changes.
jmeador added inline comments.
src/workflow/ArcanistDiffWorkflow.php
2932

Am I missing something here or should this condition also have the unset($revision_refs[$key] bit in it?

src/workflow/ArcanistDiffWorkflow.php
2932

Thanks, D18766.

If users are actually building changes that depend on one anothers' unlanded changes, we could revisit this rule once we're more confident the simpler cases work.

Just as one data point, this is relatively common in my case and I carry a patch to remove the check.