Page MenuHomePhabricator

Try switching to the branch of the same name, instead of detached head
ClosedPublic

Authored by alexmv on Nov 5 2015, 11:43 PM.
Tags
None
Referenced Files
F13249846: D14420.id34836.diff
Fri, May 24, 11:19 AM
F13249845: D14420.id34835.diff
Fri, May 24, 11:19 AM
F13249844: D14420.diff
Fri, May 24, 11:19 AM
F13249472: D14420.id34834.diff
Fri, May 24, 9:18 AM
F13249471: D14420.id.diff
Fri, May 24, 9:18 AM
F13246020: D14420.diff
Thu, May 23, 7:09 AM
F13241208: D14420.diff
Wed, May 22, 7:21 PM
F13224761: D14420.diff
Sun, May 19, 10:43 AM
Subscribers

Details

Summary

Landing from a branch that directly tracks origin/master places one in
a detached HEAD state. Instead, examine if there is a local branch of
the name that we landed onto, that also tracks the upstream; if so,
switch to that.

Test Plan

Made a branch via git checkout -b testing origin/master
and tried to arc land

Diff Detail

Repository
rARC Arcanist
Branch
to-upstream/tracking-upstream
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 8645
Build 10011: arc lint + arc unit

Event Timeline

alexmv retitled this revision from to Try switching to the branch of the same name, instead of detached head.
alexmv updated this object.
alexmv edited the test plan for this revision. (Show Details)
alexmv added a reviewer: epriestley.
epriestley edited edge metadata.

Your test plan should also probably cover the old behavior (when no named branch exists) but I think the overall behavior is correct.

You should have commit access now. You can use "Land Revision" in the UI, or realign your origin to this install (if it's pointed at GitHub) and arc land.

src/land/ArcanistGitLandEngine.php
276

This $path->isConnectedToRemote() call seems a little fragile: it may be testing the original $path, which was connected to the remote before we adjusted it. But I can't really come up with a way to rewrite it that is unambiguously more clear, and I believe the behavior and semantics are ultimately correct.

This revision is now accepted and ready to land.Nov 6 2015, 12:00 AM
src/land/ArcanistGitLandEngine.php
276

Yeah, I waffled on this a bit. If we didn't set it, then $path->getLength() is still 0. I can add that as an explicit check, though ->isConnectedToRemote() handles that case already.

Yeah, I think it's fine as is. I can't imagine anyone's going to sneak a poorly-behaved cache into isConnectedToRemote() or anything.

I think we can reasonably get test coverage here too before any more major changes occur, we just don't have much support infrastructure for covering high-level workflows yet and I think it will take most of a day to build what I have in mind.

Oh, change didn't get pushed to the staging area:

https://secure.phabricator.com/drydock/lease/578/

I'd guess because you don't have a public key on this install yet.

Just uploaded. Should I try again?

Oh, you'd have to arc diff again first to get it pushed.

alexmv edited edge metadata.

No-op diff

Hmm, still no dice. Is the output of arc diff showing that the push to the staging area succeeded?

(You can likely just land from the CLI whenever you're tired of fiddling with it, but also fine to keep clicking buttons if you want to get it to work.)

Nope, no staging area:

$ arc diff HEAD~
Linting...
 LINT OKAY  No lint problems.
Running unit tests...
   PASS   16ms★  ArcanistLibraryTestCase::testLibraryMap
   PASS    9ms★  ArcanistLibraryTestCase::testEverythingImplemented
   PASS   36ms★  ArcanistLibraryTestCase::testMethodVisibility
 UNIT OKAY  No unit test failures.
 SKIP STAGING  Unable to determine repository for this change.
Updated an existing Differential revision:
        Revision URI: https://secure.phabricator.com/D14420

Included changes:
  M       src/land/ArcanistGitLandEngine.php

I'm going to go ahead and land this from the command line, unless this is interesting to debug from your point of view.

Oh, we don't actually list the repository explicitly in .arcconfig so it won't be able to figure it out until you point your origin here so the origins match. Then you should get this from arc which:

$ arc which
REPOSITORY
To identify the repository associated with this working copy, arc followed this process:

    Configuration value "repository.callsign" is empty.

    This repository has no VCS UUID (this is normal for git/hg).

    The remote URI for this working copy is
    "ssh://dweller@secure.phabricator.com/diffusion/ARC/".

    Found a unique matching repository.

This working copy is associated with the ARC repository.
...

...and it should push to staging.

The problem was even though the upstream remote of the branch I was working on was in this arcanist, the remote named origin still pointed to our internal fork. Temporarily swapping the names of the upstreams around made it work.

Would you take a patch to use the new upstream path logic instead of hard-coding looking at "origin"?

Would you take a patch to use the new upstream path logic instead of hard-coding looking at "origin"?

Sure -- or maybe we should look at the upstream first, then origin if that fails to identify a repository? Offhand, that seems always better and less likely to break anyone with a weird workflow.