Page MenuHomePhabricator

Make "arc diff" sort of detect dependent revisions
ClosedPublic

Authored by epriestley on Sep 27 2017, 5:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 3:40 AM
Unknown Object (File)
Tue, Mar 12, 4:54 PM
Unknown Object (File)
Fri, Mar 1, 3:23 AM
Unknown Object (File)
Wed, Feb 28, 3:09 PM
Unknown Object (File)
Feb 24 2024, 4:53 PM
Unknown Object (File)
Feb 22 2024, 6:26 PM
Unknown Object (File)
Feb 21 2024, 11:00 AM
Unknown Object (File)
Feb 3 2024, 5:00 PM
Subscribers

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
Branch
dep1
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/repository/api/ArcanistGitAPI.php:1060XHP16TODO Comment
Unit
Tests Passed
Build Status
Buildable 18558
Build 25001: Run Core Tests
Build 25000: arc lint + arc unit

Event Timeline

  • Also fix a flipped condition in arc lint around all/changed paths.
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.

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.