Paths

Table of Contentst

Differential D17949

If the base commit for `arc patch` does not exist locally, try to fetch it
ClosedPublic
Actions

Authored by alexmv on May 18 2017, 4:33 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Commits

rARC21fe07925b07: (stable) Promote 2017 Week 20
rARC129d51fa0936: If the base commit for `arc patch` does not exist locally, try to fetch it

Summary

If the commit does not exist locally, aborting still leaves
the user checked out on the branch. In nearly all cases, all that is
necessary is a fetch -- but the branch must also be cleaned up. This
leads to the pattern of:

arc patch D12345
[...base commit does not exist...]
^C
git checkout master
git branch -D arcpatch-D12345
git fetch
arc patch D12345

Solve this common problem by simply trying to fetch once if the commit does not
exist locally.

Test Plan

Ran arc patch on a recent diff.

Diff Detail

Repository

rARC Arcanist

Branch

up-fetch-first (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 17081
Build 22843: Run Core Tests
Build 22842: arc lint + arc unit

Event Timeline

alexmv created this revision.May 18 2017, 4:33 PM

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptMay 18 2017, 4:33 PM

Herald added a subscriber: epriestley. · View Herald Transcript

Harbormaster completed remote builds in B17081: Diff 43171.May 18 2017, 4:33 PM

I can't really come up with any good reason not to bring this upstream. Two weak reasons are:

Users may not want to fetch --all for some reason.

You might want to use execPassthru() or similar (without --quiet) to show users status and errors.

This revision is now accepted and ready to land.May 18 2017, 4:52 PM

Er, my other reason was:

Weird --tags / --no-tags expectations?

But we'll hit both of these issues, if they do exist, on the pathway through T11091, and it may help to know about them earlier.

My theory for not execPassthru was if it fails, those failures are likely irrelevant to the matter at hand (maybe they have an old, invalid, remote still hanging out), and spewing them to the console is just going to cause confusion.

Oh, I didn't catch that you're completely ignoring the error. I suspect we don't want to do that in the long run (it leads to us getting bug reports like "arc patch said it fetched, but the commit is definitely in the remote! your software is BAD and has made my life AWFUL"), but this seems reasonable for now since I think T11091 is somewhere on the horizon.

See, e.g., T12721 for a somewhat similar case where silent local misconfiguration led to a support request.

alexmv closed this revision.May 18 2017, 5:13 PM

Users may not want to fetch --all for some reason.

I think I would have some reasons not to want to fetch --all, but I'm probably not a typical user.
None-the-less, is there maybe a way to fetch the commits without changing the refs in .git/refs/remotes/?

None-the-less, is there maybe a way to fetch the commits without changing the refs in .git/refs/remotes/?

We could do something like git fetch <remote> '+refs/*:refs/murky.chasm/*' -- I'm not sure how that interacts with --all. I'm not overjoyed about the prospect of doing this.

Also I think it probably becomes infinitely bad very quickly if you have the working copy configured as a remote.

Revision Contents
Changeset List

Path

Size

src/

workflow/

ArcanistPatchWorkflow.php

12 lines

Diff 43171

View Options

src/workflow/ArcanistPatchWorkflow.php

If the base commit for `arc patch` does not exist locally, try to fetch itClosedPublicActions