Make "arc diff" sort of detect dependent revisions
ClosedPublic
Actions

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

Details

Reviewers

amckinley

Maniphest Tasks

T11343: Generate default "Depends on" line in commit message when multiple diffs are stacked

Commits

rARC5ddc5732239a: Make "arc diff" sort of detect dependent revisions

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

Branch

dep1

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/repository/api/ArcanistGitAPI.php:1060	XHP16	TODO Comment

Unit

Tests Passed

Build Status

Buildable 18558
Build 25001: Run Core Tests
Build 25000: arc lint + arc unit

Event Timeline

epriestley created this revision.Sep 27 2017, 5:05 PM

Harbormaster completed remote builds in B18556: Diff 44778.Sep 27 2017, 5:06 PM

Also fix a flipped condition in arc lint around all/changed paths.

Harbormaster completed remote builds in B18558: Diff 44780.Sep 27 2017, 5:09 PM

epriestley mentioned this in D18644: Remove "arc lint --only-changed".Sep 27 2017, 5:09 PM

epriestley mentioned this in T11343: Generate default "Depends on" line in commit message when multiple diffs are stacked.

epriestley added a child revision: D18654: Support automatic "Depends On" detection in Mercurial.Sep 27 2017, 5:23 PM

amckinley accepted this revision.Sep 27 2017, 5:25 PM

amckinley added inline comments.

src/workflow/ArcanistDiffWorkflow.php
2930–2933	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.

Closed by commit rARC5ddc5732239a: Make "arc diff" sort of detect dependent revisions (authored by epriestley). · Explain WhySep 27 2017, 5:46 PM

This revision was automatically updated to reflect the committed changes.

jmeador added a subscriber: jmeador.Nov 7 2017, 7:15 AM

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?

epriestley mentioned this in D18766: Properly exclude others' revisions from automatic "Depends On".Nov 7 2017, 1:19 PM

epriestley added inline comments.Nov 7 2017, 1:19 PM

src/workflow/ArcanistDiffWorkflow.php
2932	Thanks, D18766.

epriestley mentioned this in rARC057bdb1c712a: Properly exclude others' revisions from automatic "Depends On".Nov 7 2017, 11:36 PM

In D18651#227868, @epriestley wrote:

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.