Paths

Table of Contentst

Differential D13989

Minimize reliance on 'git branch' output format
ClosedPublic
Actions

Authored by jbeta on Aug 24 2015, 1:53 PM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T5554: arc fails with svn/git/hg non english locales

Commits

rARC46009145f75d: Minimize reliance on 'git branch' output format

Summary

Ref T5554. Both the current branch name (if on a branch), as well as the
list of all local branches, can be retrieved without having to parse the
output from "git branch".

Unfortunately, there seems to be no git plumbing for "get list of
branches containing this commit" yet.
(see http://marc.info/?l=git&m=141408477614635&w=2)

For that case, this commit whitelists the output from "git branch" using
the known valid branch names from "git for-each-ref".

Test Plan

Set up a test repo with this structure:

|   *  Commit B1, on branch "subfeature"
|  /
| *    Commit A1, on branch "feature"
|/
*      Commit M1, on branch "master"
|

In subfeature, I tried:

arc which --base 'git:branch-unique(master)'
arc feature

After that, I detached my HEAD (don't worry, I got better) and tried again.

Nothing looked broken.

(Tested with git 1.7.2.5 and 2.5.0.)

Diff Detail

Repository

rARC Arcanist

Branch

avoid-parsing-git-branch

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 7808
Build 8648: [Placeholder Plan] Wait for 30 Seconds
Build 8647: arc lint + arc unit

Event Timeline

jbeta updated this revision to Diff 33773.Aug 24 2015, 1:53 PM

jbeta retitled this revision from to Minimize reliance on 'git branch' output format.

jbeta updated this object.

jbeta edited the test plan for this revision. (Show Details)

jbeta added a reviewer: epriestley.

jbeta added a task: T5554: arc fails with svn/git/hg non english locales.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptAug 24 2015, 1:53 PM

Herald added a subscriber: Korvin. · View Herald Transcript

Rebased against master

Turned out, (test) is a valid branch name in git.

Macro wat:

This diff fixes problems when parsing those from git branch, too.

I think I found a case where we can improve behavior, see inline. Definitely could be missing something.

src/repository/api/ArcanistGitAPI.php
485–489	It looks like this fails if there is a branch named `HEAD`, but plain `git status` also emits warnings in this case for me, so it's probably reasonable not to support this. When you are on a branch `x`, and there is also a tag `x`, this appears to output `heads/x`. However, `heads/x` is also a valid branch name, so if the command outputs `heads/x`, I think we can't distinguish between the user being on branch `heads/x` and the user being on branch `x`, with a tag named `x` also existing in the working copy. It looks like `git symbolic-ref HEAD` might have cleaner behavior in these cases -- I can't immediately come up with a case where it gets the wrong behavior, although we'll have to do a little more work to parse it.

This revision now requires changes to proceed.Aug 24 2015, 4:05 PM

Turned out, (test) is a valid branch name in git.

Fun Fact: Until fairly recently, newlines were valid in Mercurial branches.

jbeta added inline comments.Aug 24 2015, 8:30 PM

src/repository/api/ArcanistGitAPI.php
485–489	A branch named `HEAD` doesn't really seem supported by git itself, no (and it's quite far along the path of evil and perversion anyway): beta\|master:~/code/arcanist$ git branch HEAD fatal: it does not make sense to create 'HEAD' manually You're right, this logic fails with a tag and branch of the same name and I can't find appropriate flags to make `rev-parse` ignore tags. I'll update this diff with a `symbolic-ref` approach, which does seem cleaner.
905	The "branch and tag with the same name" case also breaks this: these shortnames include unneeded `heads/` prefixes. Will update.

Haha, nice. It doesn't warn you if you use git checkout -b, at least on my machine:

$ git --version
git version 2.3.2 (Apple Git-55)

$ git checkout -b HEAD
Switched to a new branch 'HEAD'
Your branch is based on 'origin/master', but the upstream is gone.
  (use "git branch --unset-upstream" to fixup)

...but you do run into trouble pretty fast:

$ git status
warning: refname 'HEAD' is ambiguous.
warning: refname 'HEAD' is ambiguous.
On branch HEAD
nothing to commit, working directory clean

You can also create a tag called HEAD but I couldn't coax any spooky behavior out of it above and beyond having a HEAD branch.

Switched strategy to parsing branch refs instead of relying on git "short" formats

Should be more robust now.

(Worldwide, owners of branches named heads/HEADS and (no branch) rejoice.)

This looks about as reasonable as we're likely to get to me, thanks!

This revision is now accepted and ready to land.Aug 25 2015, 12:57 AM

Closed by commit rARC46009145f75d: Minimize reliance on 'git branch' output format (authored by jbeta, committed by epriestley). · Explain WhyAug 25 2015, 12:58 AM

This revision was automatically updated to reflect the committed changes.

• cyn0228.ch added a task: T5375: Tokenizers fail in Firefox on Android.Sep 7 2015, 10:42 PM

chad removed a task: T5375: Tokenizers fail in Firefox on Android.Sep 7 2015, 10:45 PM

Revision Contents
Changeset List

Path

Size

src/

repository/

api/

ArcanistGitAPI.php

92 lines

Diff 33806

View Options

src/repository/api/ArcanistGitAPI.php

Minimize reliance on 'git branch' output formatClosedPublicActions