[BUG] branch renamed to 'HEAD'

[BUG] branch renamed to 'HEAD'

[Luc Van Oostenryck]

Hi,

I just discover something which very much seems a bug to me
while making an error in renaming a branch.
The scenario is the following:
- I have a branch named 'orig'
- I want to make some experimental changes on it:
	$ git checkout -b temp orig
	$ ... edit some files ...
	$ ... make some tests & commits ...
- I'm happy with my changes, so I want to have my original
  branch to now points to the head of this temp branch
  but did it wrongly:
	$ git branch -m -f orig @
- Now I discover that I don't have anymore a branch named 'orig'
  That's fine, I made an error.
- I'm searching what had happened and discover the name my branch 
  have been renamed to: 'HEAD'
  In others words I have now an entry .git/refs/heads/HEAD
  which points to where my original branch pointed.

In my opinion, it's a bug that '@' have been expanded/resolved
into a branch named 'HEAD'.


Luc Van Oostenryck

Re: [BUG] branch renamed to 'HEAD'

[Karthik Nayak]

Hello,

Thanks for reporting, but I don't think it is a bug.

On Mon, Feb 27, 2017 at 10:22 AM, Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
> Hi,
>
> I just discover something which very much seems a bug to me
> while making an error in renaming a branch.
> The scenario is the following:
> - I have a branch named 'orig'
> - I want to make some experimental changes on it:
>         $ git checkout -b temp orig
>         $ ... edit some files ...
>         $ ... make some tests & commits ...
> - I'm happy with my changes, so I want to have my original
>   branch to now points to the head of this temp branch
>   but did it wrongly:
>         $ git branch -m -f orig @

Here you are using the '-m' flag, which is to rename a branch. So what
you're essentially
doing is:
    $ git branch -m -f orig HEAD
Do note that this won't reset 'orig' to point to 'HEAD', rather this
renames 'orig' to 'HEAD'.

What you actually want to do (to reset 'orig' to 'HEAD') is:
    $ git branch -f orig @
This would make orig point to the current HEAD.

-- 
Regards,
Karthik Nayak

Re: [BUG] branch renamed to 'HEAD'

[Luc Van Oostenryck]

On Mon, Feb 27, 2017 at 11:43:46AM +0530, Karthik Nayak wrote:
> Hello,
> 
> Thanks for reporting, but I don't think it is a bug.
> 
> On Mon, Feb 27, 2017 at 10:22 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > Hi,
> >
> > I just discover something which very much seems a bug to me
> > while making an error in renaming a branch.
> > The scenario is the following:
> > - I have a branch named 'orig'
> > - I want to make some experimental changes on it:
> >         $ git checkout -b temp orig
> >         $ ... edit some files ...
> >         $ ... make some tests & commits ...
> > - I'm happy with my changes, so I want to have my original
> >   branch to now points to the head of this temp branch
> >   but did it wrongly:
> >         $ git branch -m -f orig @
> 
> Here you are using the '-m' flag, which is to rename a branch. So what
> you're essentially
> doing is:
>     $ git branch -m -f orig HEAD
> Do note that this won't reset 'orig' to point to 'HEAD', rather this
> renames 'orig' to 'HEAD'.
> 
> What you actually want to do (to reset 'orig' to 'HEAD') is:
>     $ git branch -f orig @
> This would make orig point to the current HEAD.

Sure. I said it the description that I made an error in the renaming.

What I consider as a bug is that '@', which stands for refs/HEAD,
have been interpreted as a branch named 'HEAD' and thus created a
reference refs/heads/HEAD.

Luc

Re: [BUG] branch renamed to 'HEAD'

[Jeff King]

On Mon, Feb 27, 2017 at 11:43:46AM +0530, Karthik Nayak wrote:

> On Mon, Feb 27, 2017 at 10:22 AM, Luc Van Oostenryck
> <luc.vanoostenryck@gmail.com> wrote:
> > Hi,
> >
> > I just discover something which very much seems a bug to me
> > while making an error in renaming a branch.
> > The scenario is the following:
> > - I have a branch named 'orig'
> > - I want to make some experimental changes on it:
> >         $ git checkout -b temp orig
> >         $ ... edit some files ...
> >         $ ... make some tests & commits ...
> > - I'm happy with my changes, so I want to have my original
> >   branch to now points to the head of this temp branch
> >   but did it wrongly:
> >         $ git branch -m -f orig @
> 
> Here you are using the '-m' flag, which is to rename a branch. So what
> you're essentially
> doing is:
>     $ git branch -m -f orig HEAD
> Do note that this won't reset 'orig' to point to 'HEAD', rather this
> renames 'orig' to 'HEAD'.
> 
> What you actually want to do (to reset 'orig' to 'HEAD') is:
>     $ git branch -f orig @
> This would make orig point to the current HEAD.

Regardless of the original intent, I think it is wrong to convert "@" to
a branch named "HEAD". I think the bug is in strbuf_check_branch_ref(),
which blindly sticks "refs/heads/" in front of any value we get from
interpret_branch_name(), which clearly does not make sense for HEAD.

-Peff

Re: [BUG] branch renamed to 'HEAD'

[Jeff King]

On Mon, Feb 27, 2017 at 02:49:15AM -0500, Jeff King wrote:

> > >         $ git branch -m -f orig @
> [...]
> 
> Regardless of the original intent, I think it is wrong to convert "@" to
> a branch named "HEAD". I think the bug is in strbuf_check_branch_ref(),
> which blindly sticks "refs/heads/" in front of any value we get from
> interpret_branch_name(), which clearly does not make sense for HEAD.

I do think the bug is in strbuf_check_branch_ref(), but it's hard for it
to do a better job. It needs to feed arbitrary expressions into
interpret_branch_name() to resolve things like "@{upstream}", "@{-1}",
"foo@{upstream}", etc.

The problem is that it expects a branch name to come out of
interpret_branch_name(), which _mostly_ happens. The exception is HEAD,
which is a "special" name. But the returned value doesn't indicate
whether it is special or not.

My first thought was that we might be handling "@" in the wrong place.
But it needs to happen here to make things like "@@{upstream}" work
(which turns "@" into HEAD, and then finds its upstream).

So I think the options are:

  1. Before calling interpret_branch_name(), strbuf_check_branch_ref()
     checks for "@". I don't like this because it's making assumptions
     about how the result will be parsed and interpreted.

  2. interpret_branch_name() returns a flag that says "this isn't
     _really_ a branch name".

  3. After interpret_branch_name() returns, check whether the result is
     "HEAD".

Doing (2) is the "right" thing in the sense that the "is it a branch"
logic remains with the matching parsing code. But we have to surface
that value (maybe across recursion via reinterpret?). Since we're
unlikely to ever grow a return value that matches this case except
"HEAD", it might be simplest to just do (3).

-Peff

Re: [BUG] branch renamed to 'HEAD'

[Jeff King]

On Mon, Feb 27, 2017 at 03:01:58AM -0500, Jeff King wrote:

> I do think the bug is in strbuf_check_branch_ref(), but it's hard for it
> to do a better job. It needs to feed arbitrary expressions into
> interpret_branch_name() to resolve things like "@{upstream}", "@{-1}",
> "foo@{upstream}", etc.
> 
> The problem is that it expects a branch name to come out of
> interpret_branch_name(), which _mostly_ happens. The exception is HEAD,
> which is a "special" name. But the returned value doesn't indicate
> whether it is special or not.
> 
> My first thought was that we might be handling "@" in the wrong place.
> But it needs to happen here to make things like "@@{upstream}" work
> (which turns "@" into HEAD, and then finds its upstream).
> 
> So I think the options are:
> 
>   1. Before calling interpret_branch_name(), strbuf_check_branch_ref()
>      checks for "@". I don't like this because it's making assumptions
>      about how the result will be parsed and interpreted.
> 
>   2. interpret_branch_name() returns a flag that says "this isn't
>      _really_ a branch name".
> 
>   3. After interpret_branch_name() returns, check whether the result is
>      "HEAD".
> 
> Doing (2) is the "right" thing in the sense that the "is it a branch"
> logic remains with the matching parsing code. But we have to surface
> that value (maybe across recursion via reinterpret?). Since we're
> unlikely to ever grow a return value that matches this case except
> "HEAD", it might be simplest to just do (3).

Ugh. Actually, there are a few complications I found:

  1. Checking "HEAD" afterwards means you can't actually have a branch
     named "HEAD". Doing so is probably insane, but we probably really
     _do_ want to just disallow the @-conversion here.

  2. This isn't limited to just HEAD and @-conversion. For instance:

     $ git clone /some/repo tmp
     $ cd tmp
     $ git branch -m master @{upstream}
     $ git for-each-ref --format='%(refname)'
     refs/heads/origin/master
     refs/remotes/origin/HEAD
     refs/remotes/origin/master

     Er, what? Now we have a branch called origin/master.

So I think it probably is fundamentally wrong to be calling
interpret_branch_name() here at all, if we're just going to tack
"refs/heads/" in front of it. We don't know that we're getting out a
real branchname.

But we do still need to handle @{-1}. And I suppose it's even possible
that you could want to use foo@{upstream} as long as that upstream
points to a _local_ branch.

So perhaps the fundamental issue is that interpret_branch_name() does
not give us fully qualified refs in the return value. We don't have any
clue if the return value is in "refs/heads" or "refs/remotes", or what.
We can fix that, but we're still stuck comparing the result to see if it
starts with "refs/" or is just "HEAD".

Which is wrong. If the user fed us a branch name of "refs/remotes/foo",
then the correct branch name is "refs/heads/refs/remotes/foo" (as stupid
as that probably is). It's only the branch-reinterpretation that we need
to be careful of. So we really do need to somehow have
interpret_branch_name() tell us whether or not it found an actual
branch. Actually, it passes through unknown names, so it can only tell
us when it definitely found something _outside_ of refs/heads/.

I guess something like the patch below works, but I wonder if there is a
less-horrible way to accomplish the same thing.

diff --git a/cache.h b/cache.h
index 61fc86e6d..d52e24f4f 100644
--- a/cache.h
+++ b/cache.h
@@ -1319,7 +1319,8 @@ extern char *oid_to_hex_r(char *out, const struct object_id *oid);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern char *oid_to_hex(const struct object_id *oid);	/* same static buffer as sha1_to_hex */
 
-extern int interpret_branch_name(const char *str, int len, struct strbuf *);
+extern int interpret_branch_name(const char *str, int len, struct strbuf *,
+				 int *not_in_refs_heads);
 extern int get_oid_mb(const char *str, struct object_id *oid);
 
 extern int validate_headref(const char *ref);
diff --git a/refs.c b/refs.c
index cd36b64ed..78b32dd22 100644
--- a/refs.c
+++ b/refs.c
@@ -404,7 +404,7 @@ int refname_match(const char *abbrev_name, const char *full_name)
 static char *substitute_branch_name(const char **string, int *len)
 {
 	struct strbuf buf = STRBUF_INIT;
-	int ret = interpret_branch_name(*string, *len, &buf);
+	int ret = interpret_branch_name(*string, *len, &buf, NULL);
 
 	if (ret == *len) {
 		size_t size;
diff --git a/revision.c b/revision.c
index b37dbec37..233764802 100644
--- a/revision.c
+++ b/revision.c
@@ -147,7 +147,7 @@ static void add_pending_object_with_path(struct rev_info *revs,
 		revs->no_walk = 0;
 	if (revs->reflog_info && obj->type == OBJ_COMMIT) {
 		struct strbuf buf = STRBUF_INIT;
-		int len = interpret_branch_name(name, 0, &buf);
+		int len = interpret_branch_name(name, 0, &buf, NULL);
 		int st;
 
 		if (0 < len && name[len] && buf.len)
diff --git a/sha1_name.c b/sha1_name.c
index 73a915ff1..2ef77afb0 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1155,7 +1155,8 @@ int get_oid_mb(const char *name, struct object_id *oid)
 }
 
 /* parse @something syntax, when 'something' is not {.*} */
-static int interpret_empty_at(const char *name, int namelen, int len, struct strbuf *buf)
+static int interpret_empty_at(const char *name, int namelen, int len,
+			      struct strbuf *buf, int *not_in_refs_heads)
 {
 	const char *next;
 
@@ -1173,10 +1174,13 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str
 
 	strbuf_reset(buf);
 	strbuf_add(buf, "HEAD", 4);
+	*not_in_refs_heads = 1;
 	return 1;
 }
 
-static int reinterpret(const char *name, int namelen, int len, struct strbuf *buf)
+static int reinterpret(const char *name, int namelen, int len,
+		       struct strbuf *buf, int *not_in_refs_heads)
+
 {
 	/* we have extra data, which might need further processing */
 	struct strbuf tmp = STRBUF_INIT;
@@ -1184,7 +1188,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu
 	int ret;
 
 	strbuf_add(buf, name + len, namelen - len);
-	ret = interpret_branch_name(buf->buf, buf->len, &tmp);
+	ret = interpret_branch_name(buf->buf, buf->len, &tmp, not_in_refs_heads);
 	/* that data was not interpreted, remove our cruft */
 	if (ret < 0) {
 		strbuf_setlen(buf, used);
@@ -1209,7 +1213,8 @@ static int interpret_branch_mark(const char *name, int namelen,
 				 int at, struct strbuf *buf,
 				 int (*get_mark)(const char *, int),
 				 const char *(*get_data)(struct branch *,
-							 struct strbuf *))
+							 struct strbuf *),
+				 int *not_in_refs_heads)
 {
 	int len;
 	struct branch *branch;
@@ -1234,6 +1239,9 @@ static int interpret_branch_mark(const char *name, int namelen,
 	if (!value)
 		die("%s", err.buf);
 
+	if (!starts_with(value, "refs/heads/"))
+		*not_in_refs_heads = 1;
+
 	set_shortened_ref(buf, value);
 	return len + at;
 }
@@ -1259,12 +1267,17 @@ static int interpret_branch_mark(const char *name, int namelen,
  * If the input was ok but there are not N branch switches in the
  * reflog, it returns 0.
  */
-int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
+int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,
+			  int *not_in_refs_heads)
 {
 	char *at;
 	const char *start;
+	int dummy;
 	int len = interpret_nth_prior_checkout(name, namelen, buf);
 
+	if (!not_in_refs_heads)
+		not_in_refs_heads = &dummy;
+
 	if (!namelen)
 		namelen = strlen(name);
 
@@ -1274,24 +1287,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 		if (len == namelen)
 			return len; /* consumed all */
 		else
-			return reinterpret(name, namelen, len, buf);
+			return reinterpret(name, namelen, len, buf,
+					   not_in_refs_heads);
 	}
 
 	for (start = name;
 	     (at = memchr(start, '@', namelen - (start - name)));
 	     start = at + 1) {
 
-		len = interpret_empty_at(name, namelen, at - name, buf);
+		len = interpret_empty_at(name, namelen, at - name, buf,
+					 not_in_refs_heads);
 		if (len > 0)
-			return reinterpret(name, namelen, len, buf);
+			return reinterpret(name, namelen, len, buf,
+					   not_in_refs_heads);
 
 		len = interpret_branch_mark(name, namelen, at - name, buf,
-					    upstream_mark, branch_get_upstream);
+					    upstream_mark, branch_get_upstream,
+					    not_in_refs_heads);
 		if (len > 0)
 			return len;
 
 		len = interpret_branch_mark(name, namelen, at - name, buf,
-					    push_mark, branch_get_push);
+					    push_mark, branch_get_push,
+					    not_in_refs_heads);
 		if (len > 0)
 			return len;
 	}
@@ -1299,10 +1317,11 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 	return -1;
 }
 
-int strbuf_branchname(struct strbuf *sb, const char *name)
+static int strbuf_branchname_1(struct strbuf *sb, const char *name,
+			       int *not_in_refs_heads)
 {
 	int len = strlen(name);
-	int used = interpret_branch_name(name, len, sb);
+	int used = interpret_branch_name(name, len, sb, not_in_refs_heads);
 
 	if (used == len)
 		return 0;
@@ -1312,9 +1331,17 @@ int strbuf_branchname(struct strbuf *sb, const char *name)
 	return len;
 }
 
+int strbuf_branchname(struct strbuf *sb, const char *name)
+{
+	return strbuf_branchname_1(sb, name, NULL);
+}
+
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 {
-	strbuf_branchname(sb, name);
+	int not_in_refs_heads = 0;
+	strbuf_branchname_1(sb, name, &not_in_refs_heads);
+	if (not_in_refs_heads)
+		return -1;
 	if (name[0] == '-')
 		return -1;
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);

Re: [BUG] branch renamed to 'HEAD'

[Luc Van Oostenryck]

On Mon, Feb 27, 2017 at 04:02:33AM -0500, Jeff King wrote:
> Ugh. Actually, there are a few complications I found:
> 
>   1. Checking "HEAD" afterwards means you can't actually have a branch
>      named "HEAD". Doing so is probably insane, but we probably really
>      _do_ want to just disallow the @-conversion here.
> 
>   2. This isn't limited to just HEAD and @-conversion. For instance:

It's a bit what I was afraid of.

Luc

Re: [BUG] branch renamed to 'HEAD'

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I guess something like the patch below works, but I wonder if there is a
> less-horrible way to accomplish the same thing.

I suspect that a less-horrible would be a lot more intrusive.  It
would go like "interpret-branch-name only gives local branch name,
and when it does not show it, the callers that know they do not
necessarily need local branch name would call other at-mark things".
As you pointed out with the @{upstream} that potentially point at a
local branch, it will quickly get more involved, I would think, and
I tend to think that this patch of yours is probably the least evil
one among possible solutions.  

Perhaps with s/not_in_refs_heads/not_a_branch_name/ (or swapping
polarity, "is_a_branch_name"), the resulting code may not be too
hard to read?

Thanks.

Re: [BUG] branch renamed to 'HEAD'

[Jacob Keller]

On Mon, Feb 27, 2017 at 2:28 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> I guess something like the patch below works, but I wonder if there is a
>> less-horrible way to accomplish the same thing.
>
> I suspect that a less-horrible would be a lot more intrusive.  It
> would go like "interpret-branch-name only gives local branch name,
> and when it does not show it, the callers that know they do not
> necessarily need local branch name would call other at-mark things".
> As you pointed out with the @{upstream} that potentially point at a
> local branch, it will quickly get more involved, I would think, and
> I tend to think that this patch of yours is probably the least evil
> one among possible solutions.
>
> Perhaps with s/not_in_refs_heads/not_a_branch_name/ (or swapping
> polarity, "is_a_branch_name"), the resulting code may not be too
> hard to read?
>
> Thanks.

What about changing interpret-branch-name gains a flag to return a
fully qualified ref rather than returning just the name? That seems
like it would be more reasonable behavior.

Thanks,
Jake

Re: [BUG] branch renamed to 'HEAD'

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> What about changing interpret-branch-name gains a flag to return a
> fully qualified ref rather than returning just the name? That seems
> like it would be more reasonable behavior.

There are two kinds of callers to i-b-n.  The ones that want a local
branch name because they are parsing special places on the command
line that using a local branch name makes difference (as opposed to
using any extended SHA-1 expression), like "git checkout master"
(which means different thing from "git checkout master^0").  And the
ones that can use any object name.

It depends on how your flag works, but if it means "add refs/heads/
when you got a local branch name", then that would not work well for
the former callers, as end-user inputs @{-1} and refs/heads/master
would become indistinguishable.  The former is expanded to 'master'
(if you were on that branch) and ends up being refs/heads/master.
"git checkout refs/heads/master" would be (unless you have a branch
with that name, i.e. refs/heads/refs/heads/master) a request to
detach HEAD at the commit, but the user wanted to be on the previous
branch.  And the latter iclass of callers are probably already happy
with or without the flag, so they won't be helped, either.

A flag to affect the behaviour (as opposed to &flag as a secondary
return value, like Peff's patch does) can be made to work.  Perhaps
a flag that says "keep the input as is if the result is not a local
branch name" would pass an input "@" intact and that may be
sufficient to allow "git branch -m @" to rename the current branch
to "@" (I do not think it is a sensible rename, though ;-).  But
probably some callers need to keep the original input and compare
with the result to see if we expanded anything if we go that route.
At that point, I am not sure if there are much differences in the
ease of use between the two approaches.

Re: [BUG] branch renamed to 'HEAD'

[Jeff King]

On Mon, Feb 27, 2017 at 02:28:09PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I guess something like the patch below works, but I wonder if there is a
> > less-horrible way to accomplish the same thing.
> 
> I suspect that a less-horrible would be a lot more intrusive.  It
> would go like "interpret-branch-name only gives local branch name,
> and when it does not show it, the callers that know they do not
> necessarily need local branch name would call other at-mark things".
> As you pointed out with the @{upstream} that potentially point at a
> local branch, it will quickly get more involved, I would think, and
> I tend to think that this patch of yours is probably the least evil
> one among possible solutions.  
> 
> Perhaps with s/not_in_refs_heads/not_a_branch_name/ (or swapping
> polarity, "is_a_branch_name"), the resulting code may not be too
> hard to read?

I actually started with not_a_branch_name, but I wanted specifically to
talk about refs_heads, because we sometimes refer to remote-tracking
branches as "branches" (and the function is called interpret_branch_name(),
after all).

I agree it would be easier to read if the logic were flipped, but I'm
not sure that would be correct. The function knows when it has done a
replacement that takes us outside of a normal branch name. But when it
hasn't, it doesn't really know how the result should be interpreted.

For our purposes in this caller it is enough to say that "foo" should be
treated as "refs/heads/foo", but I don't think that is generally true of
interpret_branch_name(), which might be called as part of get_sha1().

So one alternative is to leave the logic the same way but try to give it
a better name. E.g., call it something like "replaced_with_non_branch"
or something. But there, another negation snuck in. The correct
non-negated thing is really "replaced_with_HEAD_or_refs_remotes" but
that is rather awkward.

-Peff

Re: [BUG] branch renamed to 'HEAD'

[Jeff King]

On Mon, Feb 27, 2017 at 03:05:37PM -0800, Jacob Keller wrote:

> > Perhaps with s/not_in_refs_heads/not_a_branch_name/ (or swapping
> > polarity, "is_a_branch_name"), the resulting code may not be too
> > hard to read?
> 
> What about changing interpret-branch-name gains a flag to return a
> fully qualified ref rather than returning just the name? That seems
> like it would be more reasonable behavior.

That's not sufficient. If I feed "refs/remotes/origin/master" as a
branch name to git-branch, then as silly as that is, it is the branch
whose ref is "refs/heads/refs/remotes/origin/master".

Since interpret_branch_name() is not fully qualifying everything, but
rather just _sometimes_ replace @-marks with refnames, we cannot tell
from just the string the difference between "the user fed us
refs/remotes/foo" and "the @-mark expanded to a non-branch
refs/remotes/foo". We need one extra bit of information to know whether
an expansion occurred.

You could give a flag that says "do not expand to anything outside of
refs/heads/" that would suppress the @->HEAD mark, as well as
@{upstream} when upstream is outside of refs/heads. That would certainly
be less nasty than the out-parameter, but I wasn't sure that the error
handling was what we wanted.

In strbuf_branchname(), we quietly turn that error into a "0", which
causes us to retain the original text. We then feed that into
check_refname_format() in strbuf_check_branch_ref(). Which I think would
complain, and you'd get "not a valid refname: @{upstream}". If we have
the out-bit, we can say "I understand what @{upstream} means, but it
does not expand to a local branch". That's a more specific error, but
maybe it is not worth the hassle to produce it.

-Peff

Re: [BUG] branch renamed to 'HEAD'

[Jeff King]

On Mon, Feb 27, 2017 at 04:33:36PM -0800, Junio C Hamano wrote:

> A flag to affect the behaviour (as opposed to &flag as a secondary
> return value, like Peff's patch does) can be made to work.  Perhaps
> a flag that says "keep the input as is if the result is not a local
> branch name" would pass an input "@" intact and that may be
> sufficient to allow "git branch -m @" to rename the current branch
> to "@" (I do not think it is a sensible rename, though ;-).  But
> probably some callers need to keep the original input and compare
> with the result to see if we expanded anything if we go that route.
> At that point, I am not sure if there are much differences in the
> ease of use between the two approaches.

I just went into more detail in my reply to Jacob, but I do think this
is a workable approach (and fortunately we seem to have banned bare "@"
as a name, along with anything containing "@{}", so I think we would end
up rejecting these nonsense names).

I'll see if I can work up a patch. We'll still need to pass the flag
around through the various functions, but at least it will be a flag and
not a confusing negated out-parameter.

-Peff

Re: [BUG] branch renamed to 'HEAD'

[Jacob Keller]

On Mon, Feb 27, 2017 at 4:53 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 27, 2017 at 04:33:36PM -0800, Junio C Hamano wrote:
>
>> A flag to affect the behaviour (as opposed to &flag as a secondary
>> return value, like Peff's patch does) can be made to work.  Perhaps
>> a flag that says "keep the input as is if the result is not a local
>> branch name" would pass an input "@" intact and that may be
>> sufficient to allow "git branch -m @" to rename the current branch
>> to "@" (I do not think it is a sensible rename, though ;-).  But
>> probably some callers need to keep the original input and compare
>> with the result to see if we expanded anything if we go that route.
>> At that point, I am not sure if there are much differences in the
>> ease of use between the two approaches.
>
> I just went into more detail in my reply to Jacob, but I do think this
> is a workable approach (and fortunately we seem to have banned bare "@"
> as a name, along with anything containing "@{}", so I think we would end
> up rejecting these nonsense names).
>
> I'll see if I can work up a patch. We'll still need to pass the flag
> around through the various functions, but at least it will be a flag and
> not a confusing negated out-parameter.
>
> -Peff

Yes, this is pretty much what I had imagined. I look forward to seeing
the patch.

Thanks,
Jake

Re: [BUG] branch renamed to 'HEAD'

[Jeff King]

On Mon, Feb 27, 2017 at 07:53:02PM -0500, Jeff King wrote:

> On Mon, Feb 27, 2017 at 04:33:36PM -0800, Junio C Hamano wrote:
> 
> > A flag to affect the behaviour (as opposed to &flag as a secondary
> > return value, like Peff's patch does) can be made to work.  Perhaps
> > a flag that says "keep the input as is if the result is not a local
> > branch name" would pass an input "@" intact and that may be
> > sufficient to allow "git branch -m @" to rename the current branch
> > to "@" (I do not think it is a sensible rename, though ;-).  But
> > probably some callers need to keep the original input and compare
> > with the result to see if we expanded anything if we go that route.
> > At that point, I am not sure if there are much differences in the
> > ease of use between the two approaches.
> 
> I just went into more detail in my reply to Jacob, but I do think this
> is a workable approach (and fortunately we seem to have banned bare "@"
> as a name, along with anything containing "@{}", so I think we would end
> up rejecting these nonsense names).
> 
> I'll see if I can work up a patch. We'll still need to pass the flag
> around through the various functions, but at least it will be a flag and
> not a confusing negated out-parameter.

OK, I have a series which fixes this (diffstat below). When I audited
the other callers of interpret_branch_name() and strbuf_branchname(), it
turned out to be even more complicated. The callers basically fall into
a few buckets:

  1. Callers like get_sha1() and merge_name() pass the result to
     dwim_ref(), and are prepared to handle anything.

  2. Some callers stick "refs/heads/" in front of the result, and
     obviously only want local names. Most of git-branch and
     git-checkout fall into this boat.

  3. "git branch -d" can delete local _or_ remote branches, depending on
     the "-r" flag. So the expansion it wants varies, and we need to
     handle "just local" or "just remote".

So I converted the "only_branch" flag to an "allowed" bit-field. No
callers actually ask for more than a single type at once, but it was
easy to do it that way. It serves all of the callers, and will easily
adapt for the future (e.g., if "git branch -a -d" were ever allowed).

  [1/8]: interpret_branch_name: move docstring to header file
  [2/8]: strbuf_branchname: drop return value
  [3/8]: strbuf_branchname: add docstring
  [4/8]: interpret_branch_name: allow callers to restrict expansions
  [5/8]: t3204: test git-branch @-expansion corner cases
  [6/8]: branch: restrict @-expansions when deleting
  [7/8]: strbuf_check_ref_format(): expand only local branches
  [8/8]: checkout: restrict @-expansions when finding branch

 builtin/branch.c                      |   5 +-
 builtin/checkout.c                    |   2 +-
 builtin/merge.c                       |   2 +-
 cache.h                               |  32 ++++++++-
 refs.c                                |   2 +-
 revision.c                            |   2 +-
 sha1_name.c                           |  76 ++++++++++-----------
 strbuf.h                              |  21 +++++-
 t/t3204-branch-name-interpretation.sh | 122 ++++++++++++++++++++++++++++++++++
 9 files changed, 220 insertions(+), 44 deletions(-)
 create mode 100755 t/t3204-branch-name-interpretation.sh

-Peff

[PATCH 1/8] interpret_branch_name: move docstring to header file

[Jeff King]

We generally put docstrings with function declarations,
because it's the callers who need to know how the function
works. Let's do so for interpret_branch_name().

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h     | 21 +++++++++++++++++++++
 sha1_name.c | 21 ---------------------
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/cache.h b/cache.h
index 80b6372cf..c67995caa 100644
--- a/cache.h
+++ b/cache.h
@@ -1363,6 +1363,27 @@ extern char *oid_to_hex_r(char *out, const struct object_id *oid);
 extern char *sha1_to_hex(const unsigned char *sha1);	/* static buffer result! */
 extern char *oid_to_hex(const struct object_id *oid);	/* same static buffer as sha1_to_hex */
 
+/*
+ * This reads short-hand syntax that not only evaluates to a commit
+ * object name, but also can act as if the end user spelled the name
+ * of the branch from the command line.
+ *
+ * - "@{-N}" finds the name of the Nth previous branch we were on, and
+ *   places the name of the branch in the given buf and returns the
+ *   number of characters parsed if successful.
+ *
+ * - "<branch>@{upstream}" finds the name of the other ref that
+ *   <branch> is configured to merge with (missing <branch> defaults
+ *   to the current branch), and places the name of the branch in the
+ *   given buf and returns the number of characters parsed if
+ *   successful.
+ *
+ * If the input is not of the accepted format, it returns a negative
+ * number to signal an error.
+ *
+ * If the input was ok but there are not N branch switches in the
+ * reflog, it returns 0.
+ */
 extern int interpret_branch_name(const char *str, int len, struct strbuf *);
 extern int get_oid_mb(const char *str, struct object_id *oid);
 
diff --git a/sha1_name.c b/sha1_name.c
index 9b5d14b4b..28865b3a1 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1238,27 +1238,6 @@ static int interpret_branch_mark(const char *name, int namelen,
 	return len + at;
 }
 
-/*
- * This reads short-hand syntax that not only evaluates to a commit
- * object name, but also can act as if the end user spelled the name
- * of the branch from the command line.
- *
- * - "@{-N}" finds the name of the Nth previous branch we were on, and
- *   places the name of the branch in the given buf and returns the
- *   number of characters parsed if successful.
- *
- * - "<branch>@{upstream}" finds the name of the other ref that
- *   <branch> is configured to merge with (missing <branch> defaults
- *   to the current branch), and places the name of the branch in the
- *   given buf and returns the number of characters parsed if
- *   successful.
- *
- * If the input is not of the accepted format, it returns a negative
- * number to signal an error.
- *
- * If the input was ok but there are not N branch switches in the
- * reflog, it returns 0.
- */
 int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 {
 	char *at;
-- 
2.12.0.359.gd4c8c42e9

[PATCH 2/8] strbuf_branchname: drop return value

[Jeff King]

The return value from strbuf_branchname() is confusing and
useless: it's 0 if the whole name was consumed by an @-mark,
but otherwise is the length of the original name we fed.

No callers actually look at the return value, so let's just
get rid of it.

Signed-off-by: Jeff King <peff@peff.net>
---
 sha1_name.c | 5 +----
 strbuf.h    | 2 +-
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 28865b3a1..4c1e91184 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1279,17 +1279,14 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 	return -1;
 }
 
-int strbuf_branchname(struct strbuf *sb, const char *name)
+void strbuf_branchname(struct strbuf *sb, const char *name)
 {
 	int len = strlen(name);
 	int used = interpret_branch_name(name, len, sb);
 
-	if (used == len)
-		return 0;
 	if (used < 0)
 		used = 0;
 	strbuf_add(sb, name + used, len - used);
-	return len;
 }
 
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
diff --git a/strbuf.h b/strbuf.h
index cf1b5409e..47df0500d 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -560,7 +560,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 	strbuf_complete(sb, '\n');
 }
 
-extern int strbuf_branchname(struct strbuf *sb, const char *name);
+extern void strbuf_branchname(struct strbuf *sb, const char *name);
 extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
 
 extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
-- 
2.12.0.359.gd4c8c42e9

[PATCH 3/8] strbuf_branchname: add docstring

[Jeff King]

This function and its companion, strbuf_check_branch_ref(),
did not have their purpose or semantics explained. Let's do
so.

Signed-off-by: Jeff King <peff@peff.net>
---
 strbuf.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/strbuf.h b/strbuf.h
index 47df0500d..6b51b2604 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -560,7 +560,22 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 	strbuf_complete(sb, '\n');
 }
 
+/*
+ * Copy "name" to "sb", expanding any special @-marks as handled by
+ * interpret_branch_name(). The result is a non-qualified branch name
+ * (so "foo" or "origin/master" instead of "refs/heads/foo" or
+ * "refs/remotes/origin/master").
+ *
+ * Note that the resulting name may not be a syntactically valid refname.
+ */
 extern void strbuf_branchname(struct strbuf *sb, const char *name);
+
+/*
+ * Like strbuf_branchname() above, but confirm that the result is
+ * syntactically valid to be used as a local branch name in refs/heads/.
+ *
+ * The return value is "0" if the result is valid, and "-1" otherwise.
+ */
 extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
 
 extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
-- 
2.12.0.359.gd4c8c42e9

[PATCH 4/8] interpret_branch_name: allow callers to restrict expansions

[Jeff King]

The original purpose of interpret_branch_name() was to be
used by get_sha1() in resolving refs. As such, some of its
expansions may point to refs outside of the local
"refs/heads" namespace.

Over time, the function has been picked up by other callers
who want to use the ref-expansion to give the user access to
the same shortcuts (e.g., allowing "git branch" to delete
via "@{-1}" or "@{upstream}").  These uses have confusing
corner cases when the expansion isn't in refs/heads/ (for
instance, deleting "@" tries to delete refs/heads/HEAD,
which is nonsense).

Callers can't know from the returned string how the
expansion happened (e.g., did the user really ask for a
branch named "HEAD", or did we do a bogus expansion?). One
fix would be to return some out-parameters describing the
types of expansion that occurred. This has the benefit that
the caller can generate precise error messages ("I
understood @{upstream} to mean origin/master, but that is a
remote tracking branch, so you cannot create it as a local
name").

However, out-parameters make calling interface somewhat
cumbersome. Instead, let's do the opposite: let the caller
tell us which elements to expand. That's easier to pass in,
and none of the callers give more precise error messages
than "@{upstream} isn't a valid branch name" anyway (which
should be sufficient).

The strbuf_branchname() function needs a similar parameter,
as most of the callers access interpret_branch_name()
through it. For now, we'll pass "0" for "no restrictions" in
each caller, and update them individually in subsequent
patches.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c   |  2 +-
 builtin/checkout.c |  2 +-
 builtin/merge.c    |  2 +-
 cache.h            | 13 +++++++++++--
 refs.c             |  2 +-
 revision.c         |  2 +-
 sha1_name.c        | 52 +++++++++++++++++++++++++++++++++++++++-------------
 strbuf.h           |  6 +++++-
 8 files changed, 60 insertions(+), 21 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 94f7de7fa..cf0ece55d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -216,7 +216,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		char *target = NULL;
 		int flags = 0;
 
-		strbuf_branchname(&bname, argv[i]);
+		strbuf_branchname(&bname, argv[i], 0);
 		free(name);
 		name = mkpathdup(fmt, bname.buf);
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f174f5030..05eefd994 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -452,7 +452,7 @@ static void setup_branch_path(struct branch_info *branch)
 {
 	struct strbuf buf = STRBUF_INIT;
 
-	strbuf_branchname(&buf, branch->name);
+	strbuf_branchname(&buf, branch->name, 0);
 	if (strcmp(buf.buf, branch->name))
 		branch->name = xstrdup(buf.buf);
 	strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
diff --git a/builtin/merge.c b/builtin/merge.c
index a96d4fb50..848a29855 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -438,7 +438,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
 	char *found_ref;
 	int len, early;
 
-	strbuf_branchname(&bname, remote);
+	strbuf_branchname(&bname, remote, 0);
 	remote = bname.buf;
 
 	memset(branch_head, 0, sizeof(branch_head));
diff --git a/cache.h b/cache.h
index c67995caa..a8816c914 100644
--- a/cache.h
+++ b/cache.h
@@ -1383,8 +1383,17 @@ extern char *oid_to_hex(const struct object_id *oid);	/* same static buffer as s
  *
  * If the input was ok but there are not N branch switches in the
  * reflog, it returns 0.
- */
-extern int interpret_branch_name(const char *str, int len, struct strbuf *);
+ *
+ * If "allowed" is non-zero, it is a treated as a bitfield of allowable
+ * expansions: local branches ("refs/heads/"), remote branches
+ * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
+ * allowed, even ones to refs outside of those namespaces.
+ */
+#define INTERPRET_BRANCH_LOCAL (1<<0)
+#define INTERPRET_BRANCH_REMOTE (1<<1)
+#define INTERPRET_BRANCH_HEAD (1<<2)
+extern int interpret_branch_name(const char *str, int len, struct strbuf *,
+				 unsigned allowed);
 extern int get_oid_mb(const char *str, struct object_id *oid);
 
 extern int validate_headref(const char *ref);
diff --git a/refs.c b/refs.c
index 6d0961921..da62119c2 100644
--- a/refs.c
+++ b/refs.c
@@ -405,7 +405,7 @@ int refname_match(const char *abbrev_name, const char *full_name)
 static char *substitute_branch_name(const char **string, int *len)
 {
 	struct strbuf buf = STRBUF_INIT;
-	int ret = interpret_branch_name(*string, *len, &buf);
+	int ret = interpret_branch_name(*string, *len, &buf, 0);
 
 	if (ret == *len) {
 		size_t size;
diff --git a/revision.c b/revision.c
index b37dbec37..771d079f6 100644
--- a/revision.c
+++ b/revision.c
@@ -147,7 +147,7 @@ static void add_pending_object_with_path(struct rev_info *revs,
 		revs->no_walk = 0;
 	if (revs->reflog_info && obj->type == OBJ_COMMIT) {
 		struct strbuf buf = STRBUF_INIT;
-		int len = interpret_branch_name(name, 0, &buf);
+		int len = interpret_branch_name(name, 0, &buf, 0);
 		int st;
 
 		if (0 < len && name[len] && buf.len)
diff --git a/sha1_name.c b/sha1_name.c
index 4c1e91184..b21997c29 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1176,7 +1176,8 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str
 	return 1;
 }
 
-static int reinterpret(const char *name, int namelen, int len, struct strbuf *buf)
+static int reinterpret(const char *name, int namelen, int len,
+		       struct strbuf *buf, unsigned allowed)
 {
 	/* we have extra data, which might need further processing */
 	struct strbuf tmp = STRBUF_INIT;
@@ -1184,7 +1185,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu
 	int ret;
 
 	strbuf_add(buf, name + len, namelen - len);
-	ret = interpret_branch_name(buf->buf, buf->len, &tmp);
+	ret = interpret_branch_name(buf->buf, buf->len, &tmp, allowed);
 	/* that data was not interpreted, remove our cruft */
 	if (ret < 0) {
 		strbuf_setlen(buf, used);
@@ -1205,11 +1206,27 @@ static void set_shortened_ref(struct strbuf *buf, const char *ref)
 	free(s);
 }
 
+static int branch_interpret_allowed(const char *refname, unsigned allowed)
+{
+	if (!allowed)
+		return 1;
+
+	if ((allowed & INTERPRET_BRANCH_LOCAL) &&
+	    starts_with(refname, "refs/heads/"))
+		return 1;
+	if ((allowed & INTERPRET_BRANCH_REMOTE) &&
+	    starts_with(refname, "refs/remotes/"))
+		return 1;
+
+	return 0;
+}
+
 static int interpret_branch_mark(const char *name, int namelen,
 				 int at, struct strbuf *buf,
 				 int (*get_mark)(const char *, int),
 				 const char *(*get_data)(struct branch *,
-							 struct strbuf *))
+							 struct strbuf *),
+				 unsigned allowed)
 {
 	int len;
 	struct branch *branch;
@@ -1234,11 +1251,15 @@ static int interpret_branch_mark(const char *name, int namelen,
 	if (!value)
 		die("%s", err.buf);
 
+	if (!branch_interpret_allowed(value, allowed))
+		return -1;
+
 	set_shortened_ref(buf, value);
 	return len + at;
 }
 
-int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
+int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,
+			  unsigned allowed)
 {
 	char *at;
 	const char *start;
@@ -1254,24 +1275,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 		if (len == namelen)
 			return len; /* consumed all */
 		else
-			return reinterpret(name, namelen, len, buf);
+			return reinterpret(name, namelen, len, buf, allowed);
 	}
 
 	for (start = name;
 	     (at = memchr(start, '@', namelen - (start - name)));
 	     start = at + 1) {
 
-		len = interpret_empty_at(name, namelen, at - name, buf);
-		if (len > 0)
-			return reinterpret(name, namelen, len, buf);
+		if (!allowed || (allowed & INTERPRET_BRANCH_HEAD)) {
+			len = interpret_empty_at(name, namelen, at - name, buf);
+			if (len > 0)
+				return reinterpret(name, namelen, len, buf,
+						   allowed);
+		}
 
 		len = interpret_branch_mark(name, namelen, at - name, buf,
-					    upstream_mark, branch_get_upstream);
+					    upstream_mark, branch_get_upstream,
+					    allowed);
 		if (len > 0)
 			return len;
 
 		len = interpret_branch_mark(name, namelen, at - name, buf,
-					    push_mark, branch_get_push);
+					    push_mark, branch_get_push,
+					    allowed);
 		if (len > 0)
 			return len;
 	}
@@ -1279,10 +1305,10 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
 	return -1;
 }
 
-void strbuf_branchname(struct strbuf *sb, const char *name)
+void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
 {
 	int len = strlen(name);
-	int used = interpret_branch_name(name, len, sb);
+	int used = interpret_branch_name(name, len, sb, allowed);
 
 	if (used < 0)
 		used = 0;
@@ -1291,7 +1317,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name)
 
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 {
-	strbuf_branchname(sb, name);
+	strbuf_branchname(sb, name, 0);
 	if (name[0] == '-')
 		return -1;
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
diff --git a/strbuf.h b/strbuf.h
index 6b51b2604..17e5f29a5 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -567,8 +567,12 @@ static inline void strbuf_complete_line(struct strbuf *sb)
  * "refs/remotes/origin/master").
  *
  * Note that the resulting name may not be a syntactically valid refname.
+ *
+ * If "allowed" is non-zero, restrict the set of allowed expansions. See
+ * interpret_branch_name() for details.
  */
-extern void strbuf_branchname(struct strbuf *sb, const char *name);
+extern void strbuf_branchname(struct strbuf *sb, const char *name,
+			      unsigned allowed);
 
 /*
  * Like strbuf_branchname() above, but confirm that the result is
-- 
2.12.0.359.gd4c8c42e9

[PATCH 5/8] t3204: test git-branch @-expansion corner cases

[Jeff King]

git-branch feeds the branch names from the command line to
strbuf_branchname(), but we do not yet tell that function
which kinds of expansions should be allowed. Let's create a
set of tests that cover both the allowed and disallowed
cases.

That shows off some breakages where we currently create or
delete the wrong ref (and will make sure that we do not
break any cases that _should_ be working when we do add more
restrictions).

Note that we check branch creation and deletion, but do not
bother with renames. Those follow the same code path as
creation.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t3204-branch-name-interpretation.sh | 112 ++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)
 create mode 100755 t/t3204-branch-name-interpretation.sh

diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
new file mode 100755
index 000000000..2fe696ba6
--- /dev/null
+++ b/t/t3204-branch-name-interpretation.sh
@@ -0,0 +1,112 @@
+#!/bin/sh
+
+test_description='interpreting exotic branch name arguments
+
+Branch name arguments are usually names which are taken to be inside of
+refs/heads/, but we interpret some magic syntax like @{-1}, @{upstream}, etc.
+This script aims to check the behavior of those corner cases.
+'
+. ./test-lib.sh
+
+expect_branch() {
+	git log -1 --format=%s "$1" >actual &&
+	echo "$2" >expect &&
+	test_cmp expect actual
+}
+
+expect_deleted() {
+	test_must_fail git rev-parse --verify "$1"
+}
+
+test_expect_success 'set up repo' '
+	test_commit one &&
+	test_commit two &&
+	git remote add origin foo.git
+'
+
+test_expect_success 'update branch via @{-1}' '
+	git branch previous one &&
+
+	git checkout previous &&
+	git checkout master &&
+
+	git branch -f @{-1} two &&
+	expect_branch previous two
+'
+
+test_expect_success 'update branch via local @{upstream}' '
+	git branch local one &&
+	git branch --set-upstream-to=local &&
+
+	git branch -f @{upstream} two &&
+	expect_branch local two
+'
+
+test_expect_failure 'disallow updating branch via remote @{upstream}' '
+	git update-ref refs/remotes/origin/remote one &&
+	git branch --set-upstream-to=origin/remote &&
+
+	test_must_fail git branch -f @{upstream} two
+'
+
+test_expect_success 'create branch with pseudo-qualified name' '
+	git branch refs/heads/qualified two &&
+	expect_branch refs/heads/refs/heads/qualified two
+'
+
+test_expect_success 'delete branch via @{-1}' '
+	git branch previous-del &&
+
+	git checkout previous-del &&
+	git checkout master &&
+
+	git branch -D @{-1} &&
+	expect_deleted previous-del
+'
+
+test_expect_success 'delete branch via local @{upstream}' '
+	git branch local-del &&
+	git branch --set-upstream-to=local-del &&
+
+	git branch -D @{upstream} &&
+	expect_deleted local-del
+'
+
+test_expect_success 'delete branch via remote @{upstream}' '
+	git update-ref refs/remotes/origin/remote-del two &&
+	git branch --set-upstream-to=origin/remote-del &&
+
+	git branch -r -D @{upstream} &&
+	expect_deleted origin/remote-del
+'
+
+# Note that we create two oddly named local branches here. We want to make
+# sure that we do not accidentally delete either of them, even if
+# shorten_unambiguous_ref() tweaks the name to avoid ambiguity.
+test_expect_failure 'delete @{upstream} expansion matches -r option' '
+	git update-ref refs/remotes/origin/remote-del two &&
+	git branch --set-upstream-to=origin/remote-del &&
+	git update-ref refs/heads/origin/remote-del two &&
+	git update-ref refs/heads/remotes/origin/remote-del two &&
+
+	test_must_fail git branch -D @{upstream} &&
+	expect_branch refs/heads/origin/remote-del two &&
+	expect_branch refs/heads/remotes/origin/remote-del two
+'
+
+# The thing we are testing here is that "@" is the real branch refs/heads/@,
+# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a
+# sane thing, but it _is_ technically allowed for now. If we disallow it, these
+# can be switched to test_must_fail.
+test_expect_failure 'create branch named "@"' '
+	git branch -f @ one &&
+	expect_branch refs/heads/@ one
+'
+
+test_expect_failure 'delete branch named "@"' '
+	git update-ref refs/heads/@ two &&
+	git branch -D @ &&
+	expect_deleted refs/heads/@
+'
+
+test_done
-- 
2.12.0.359.gd4c8c42e9

[PATCH 6/8] branch: restrict @-expansions when deleting

[Jeff King]

We use strbuf_branchname() to expand the branch name from
the command line, so you can delete the branch given by
@{-1}, for example.  However, we allow other nonsense like
"@", and we do not respect our "-r" flag (so we may end up
deleting an oddly-named local ref instead of a remote one).

We can fix this by passing the appropriate "allowed" flag to
strbuf_branchname().

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c                      | 5 ++++-
 t/t3204-branch-name-interpretation.sh | 4 ++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index cf0ece55d..291fe90de 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -191,17 +191,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 	int ret = 0;
 	int remote_branch = 0;
 	struct strbuf bname = STRBUF_INIT;
+	unsigned allowed_interpret;
 
 	switch (kinds) {
 	case FILTER_REFS_REMOTES:
 		fmt = "refs/remotes/%s";
 		/* For subsequent UI messages */
 		remote_branch = 1;
+		allowed_interpret = INTERPRET_BRANCH_REMOTE;
 
 		force = 1;
 		break;
 	case FILTER_REFS_BRANCHES:
 		fmt = "refs/heads/%s";
+		allowed_interpret = INTERPRET_BRANCH_LOCAL;
 		break;
 	default:
 		die(_("cannot use -a with -d"));
@@ -216,7 +219,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
 		char *target = NULL;
 		int flags = 0;
 
-		strbuf_branchname(&bname, argv[i], 0);
+		strbuf_branchname(&bname, argv[i], allowed_interpret);
 		free(name);
 		name = mkpathdup(fmt, bname.buf);
 
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 2fe696ba6..c8fec5b8c 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -83,7 +83,7 @@ test_expect_success 'delete branch via remote @{upstream}' '
 # Note that we create two oddly named local branches here. We want to make
 # sure that we do not accidentally delete either of them, even if
 # shorten_unambiguous_ref() tweaks the name to avoid ambiguity.
-test_expect_failure 'delete @{upstream} expansion matches -r option' '
+test_expect_success 'delete @{upstream} expansion matches -r option' '
 	git update-ref refs/remotes/origin/remote-del two &&
 	git branch --set-upstream-to=origin/remote-del &&
 	git update-ref refs/heads/origin/remote-del two &&
@@ -103,7 +103,7 @@ test_expect_failure 'create branch named "@"' '
 	expect_branch refs/heads/@ one
 '
 
-test_expect_failure 'delete branch named "@"' '
+test_expect_success 'delete branch named "@"' '
 	git update-ref refs/heads/@ two &&
 	git branch -D @ &&
 	expect_deleted refs/heads/@
-- 
2.12.0.359.gd4c8c42e9

[PATCH 7/8] strbuf_check_ref_format(): expand only local branches

[Jeff King]

This function asks strbuf_branchname() to expand any @-marks
in the branchname, and then we blindly stick refs/heads/ in
front of the result. This is obviously nonsense if the
expansion is "HEAD" or a ref in refs/remotes/.

The most obvious end-user effect is that creating or
renaming a branch with an expansion may have confusing
results (e.g., creating refs/heads/origin/master from
"@{upstream}" when the operation should be disallowed).

We can fix this by telling strbuf_branchname() that we are
only interested in local expansions. Any unexpanded bits are
then fed to check_ref_format(), which either disallows them
(in the case of "@{upstream}") or lets them through
("refs/heads/@" is technically valid, if a bit silly).

Signed-off-by: Jeff King <peff@peff.net>
---
 sha1_name.c                           | 2 +-
 t/t3204-branch-name-interpretation.sh | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index b21997c29..c0cfb69a4 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1317,7 +1317,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
 
 int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
 {
-	strbuf_branchname(sb, name, 0);
+	strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
 	if (name[0] == '-')
 		return -1;
 	strbuf_splice(sb, 0, 0, "refs/heads/", 11);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index c8fec5b8c..6115ad504 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -42,7 +42,7 @@ test_expect_success 'update branch via local @{upstream}' '
 	expect_branch local two
 '
 
-test_expect_failure 'disallow updating branch via remote @{upstream}' '
+test_expect_success 'disallow updating branch via remote @{upstream}' '
 	git update-ref refs/remotes/origin/remote one &&
 	git branch --set-upstream-to=origin/remote &&
 
@@ -98,7 +98,7 @@ test_expect_success 'delete @{upstream} expansion matches -r option' '
 # and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a
 # sane thing, but it _is_ technically allowed for now. If we disallow it, these
 # can be switched to test_must_fail.
-test_expect_failure 'create branch named "@"' '
+test_expect_success 'create branch named "@"' '
 	git branch -f @ one &&
 	expect_branch refs/heads/@ one
 '
-- 
2.12.0.359.gd4c8c42e9

[PATCH 8/8] checkout: restrict @-expansions when finding branch

[Jeff King]

When we parse "git checkout $NAME", we try to interpret
$NAME as a local branch-name. If it is, then we point HEAD
to that branch. Otherwise, we detach the HEAD at whatever
commit $NAME points to.

We do the interpretation by calling strbuf_branchname(), and
then blindly sticking "refs/heads/" on the front. This leads
to nonsense results when expansions like "@{upstream}" or
"@" point to something besides a local branch. We end up
with a local branch name like "refs/heads/origin/master" or
"refs/heads/HEAD".

Normally this has no user-visible effect because those
branches don't exist, and so we fallback to feeding the
result to get_sha1(), which resolves them correctly.

But as the new test in t3204 shows, there are corner cases
where the effect is observable, and we check out the wrong
local branch rather than detaching to the correct one.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/checkout.c                    |  2 +-
 t/t3204-branch-name-interpretation.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 05eefd994..81f07c3ef 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -452,7 +452,7 @@ static void setup_branch_path(struct branch_info *branch)
 {
 	struct strbuf buf = STRBUF_INIT;
 
-	strbuf_branchname(&buf, branch->name, 0);
+	strbuf_branchname(&buf, branch->name, INTERPRET_BRANCH_LOCAL);
 	if (strcmp(buf.buf, branch->name))
 		branch->name = xstrdup(buf.buf);
 	strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 6115ad504..b8e396009 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -109,4 +109,14 @@ test_expect_success 'delete branch named "@"' '
 	expect_deleted refs/heads/@
 '
 
+test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
+	git update-ref refs/remotes/origin/checkout one &&
+	git branch --set-upstream-to=origin/checkout &&
+	git update-ref refs/heads/origin/checkout two &&
+	git update-ref refs/heads/remotes/origin/checkout two &&
+
+	git checkout @{upstream} &&
+	expect_branch HEAD one
+'
+
 test_done
-- 
2.12.0.359.gd4c8c42e9

Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions

[Jeff King]

On Tue, Feb 28, 2017 at 07:14:34AM -0500, Jeff King wrote:

> However, out-parameters make calling interface somewhat
> cumbersome. Instead, let's do the opposite: let the caller
> tell us which elements to expand. That's easier to pass in,
> and none of the callers give more precise error messages
> than "@{upstream} isn't a valid branch name" anyway (which
> should be sufficient).

Two things to call attention to:

> -extern int interpret_branch_name(const char *str, int len, struct strbuf *);
> + *
> + * If "allowed" is non-zero, it is a treated as a bitfield of allowable
> + * expansions: local branches ("refs/heads/"), remote branches
> + * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
> + * allowed, even ones to refs outside of those namespaces.
> + */
> +#define INTERPRET_BRANCH_LOCAL (1<<0)
> +#define INTERPRET_BRANCH_REMOTE (1<<1)
> +#define INTERPRET_BRANCH_HEAD (1<<2)

Is the "0 allows everything, but any bit turns on restrictions"
convention too confusing? It's convenient to use in the callers which do
not need restrictions, but we could add an INTERPRET_BRANCH_ALL flag if
that is more clear (but note that it _isn't_ just bitwise-AND of the
other flags, because technically an @{upstream} could point to
"refs/foo" or some other location).

> -int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
> +int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,
> +			  unsigned allowed)
>  {
>  	char *at;
>  	const char *start;
> @@ -1254,24 +1275,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
>  		if (len == namelen)
>  			return len; /* consumed all */
>  		else
> -			return reinterpret(name, namelen, len, buf);
> +			return reinterpret(name, namelen, len, buf, allowed);
>  	}

It's hard to see from this context, but a careful reader may note that
we do not check "allowed" at all before calling
interpret_nth_prior_checkout(). This is looking for branch names via
HEAD, so I don't think it can ever return anything but a local name.

Which, hmm. I guess was valid when the flag was "only_branches", but
would not be valid under INTERPRET_BRANCH_REMOTE. I wonder if

  git branch -r -D @{-1}

incorrectly deletes refs/remotes/origin/master if you previously had
refs/heads/origin/master checked out.

-Peff

Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions

[Jeff King]

On Tue, Feb 28, 2017 at 07:23:38AM -0500, Jeff King wrote:

> > -int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
> > +int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,
> > +			  unsigned allowed)
> >  {
> >  	char *at;
> >  	const char *start;
> > @@ -1254,24 +1275,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
> >  		if (len == namelen)
> >  			return len; /* consumed all */
> >  		else
> > -			return reinterpret(name, namelen, len, buf);
> > +			return reinterpret(name, namelen, len, buf, allowed);
> >  	}
> 
> It's hard to see from this context, but a careful reader may note that
> we do not check "allowed" at all before calling
> interpret_nth_prior_checkout(). This is looking for branch names via
> HEAD, so I don't think it can ever return anything but a local name.
> 
> Which, hmm. I guess was valid when the flag was "only_branches", but
> would not be valid under INTERPRET_BRANCH_REMOTE. I wonder if
> 
>   git branch -r -D @{-1}
> 
> incorrectly deletes refs/remotes/origin/master if you previously had
> refs/heads/origin/master checked out.

The answer is "yes", it's broken. So interpret_branch_name() should do
an "allowed" check before expanding the nth-prior. The fix should be
easy, especially on top of the earlier 426f76595 (which, incidentally, I
already based this series on).

I'll hold off on re-rolling to see if it collects any other review.

-Peff

Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The original purpose of interpret_branch_name() was to be used by
> get_sha1() in resolving refs.  As such, some of its expansions may
> point to refs outside of the local "refs/heads" namespace.

I am not sure the reference to "get_sha1()" is entirely correct.

Until it was renamed at 431b1969fc ("Rename interpret/substitute
nth_last_branch functions", 2009-03-21), the function was called
interpret_nth_last_branch() which was originally introduced for the
name, not sha1, at ae5a6c3684 ("checkout: implement "@{-N}" shortcut
name for N-th last branch", 2009-01-17).  The use of the same syntax
and function for the object name came a bit later.

But I think that is an insignificant detail.  Let's read on.

> Over time, the function has been picked up by other callers
> who want to use the ref-expansion to give the user access to
> the same shortcuts (e.g., allowing "git branch" to delete
> via "@{-1}" or "@{upstream}").  These uses have confusing
> corner cases when the expansion isn't in refs/heads/ (for
> instance, deleting "@" tries to delete refs/heads/HEAD,
> which is nonsense).
>
> Callers can't know from the returned string how the
> expansion happened (e.g., did the user really ask for a
> branch named "HEAD", or did we do a bogus expansion?). One
> fix would be to return some out-parameters describing the
> types of expansion that occurred. This has the benefit that
> the caller can generate precise error messages ("I
> understood @{upstream} to mean origin/master, but that is a
> remote tracking branch, so you cannot create it as a local
> name").
>
> However, out-parameters make calling interface somewhat
> cumbersome. Instead, let's do the opposite: let the caller
> tell us which elements to expand. That's easier to pass in,
> and none of the callers give more precise error messages
> than "@{upstream} isn't a valid branch name" anyway (which
> should be sufficient).
>
> The strbuf_branchname() function needs a similar parameter,
> as most of the callers access interpret_branch_name()
> through it. For now, we'll pass "0" for "no restrictions" in
> each caller, and update them individually in subsequent
> patches.

OK.

> diff --git a/cache.h b/cache.h
> index c67995caa..a8816c914 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1383,8 +1383,17 @@ extern char *oid_to_hex(const struct object_id *oid);	/* same static buffer as s
>   *
>   * If the input was ok but there are not N branch switches in the
>   * reflog, it returns 0.
> - */
> -extern int interpret_branch_name(const char *str, int len, struct strbuf *);
> + *
> + * If "allowed" is non-zero, it is a treated as a bitfield of allowable
> + * expansions: local branches ("refs/heads/"), remote branches
> + * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
> + * allowed, even ones to refs outside of those namespaces.
> + */

Answering the question in your follow-up, I personally do not find
"0 means anything goes" too confusing, but for satisfying those who
do, spelling ~0 is not too bad, either.

> +#define INTERPRET_BRANCH_LOCAL (1<<0)
> +#define INTERPRET_BRANCH_REMOTE (1<<1)
> +#define INTERPRET_BRANCH_HEAD (1<<2)
> +extern int interpret_branch_name(const char *str, int len, struct strbuf *,
> +				 unsigned allowed);
>  extern int get_oid_mb(const char *str, struct object_id *oid);

> diff --git a/refs.c b/refs.c
> index 6d0961921..da62119c2 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -405,7 +405,7 @@ int refname_match(const char *abbrev_name, const char *full_name)
>  static char *substitute_branch_name(const char **string, int *len)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> -	int ret = interpret_branch_name(*string, *len, &buf);
> +	int ret = interpret_branch_name(*string, *len, &buf, 0);
>  
>  	if (ret == *len) {
>  		size_t size;

This is the one used by dwim_ref/log, so we'd need to allow it to
resolve to anything, e.g. "@" -> "HEAD", and pretend that the user
typed that expansion.  OK.

Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions

[Jeff King]

On Tue, Feb 28, 2017 at 12:27:45PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The original purpose of interpret_branch_name() was to be used by
> > get_sha1() in resolving refs.  As such, some of its expansions may
> > point to refs outside of the local "refs/heads" namespace.
> 
> I am not sure the reference to "get_sha1()" is entirely correct.
> 
> Until it was renamed at 431b1969fc ("Rename interpret/substitute
> nth_last_branch functions", 2009-03-21), the function was called
> interpret_nth_last_branch() which was originally introduced for the
> name, not sha1, at ae5a6c3684 ("checkout: implement "@{-N}" shortcut
> name for N-th last branch", 2009-01-17).  The use of the same syntax
> and function for the object name came a bit later.
> 
> But I think that is an insignificant detail.  Let's read on.

Yeah, sorry, I was lazy about digging up the history. I think the
problem actually started in ae0ba8e20a (Teach @{upstream} syntax to
strbuf_branchanme(), 2010-01-19), when the features were ported over
from get_sha1() to interpret_branch_name().

Since I need to re-roll anyway, I'll tweak this to be more accurate.

> > @@ -405,7 +405,7 @@ int refname_match(const char *abbrev_name, const char *full_name)
> >  static char *substitute_branch_name(const char **string, int *len)
> >  {
> >  	struct strbuf buf = STRBUF_INIT;
> > -	int ret = interpret_branch_name(*string, *len, &buf);
> > +	int ret = interpret_branch_name(*string, *len, &buf, 0);
> >  
> >  	if (ret == *len) {
> >  		size_t size;
> 
> This is the one used by dwim_ref/log, so we'd need to allow it to
> resolve to anything, e.g. "@" -> "HEAD", and pretend that the user
> typed that expansion.  OK.

Yeah. Left them all as "0" here, and then split the updates into their
own commits. So there's no commit that says "and we are leaving this
spot, because it is correct as-is". The other notable one is the
strbuf_branchname() call in merge_name().

I can mention those in the commit message here (I think I did in the
cover letter, but it would be nice to stick it in the history, since
that will be what comes up if you blame those lines).

-Peff

Re: [BUG] branch renamed to 'HEAD'

[Jacob Keller]

On Tue, Feb 28, 2017 at 4:06 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 27, 2017 at 07:53:02PM -0500, Jeff King wrote:
>
>> On Mon, Feb 27, 2017 at 04:33:36PM -0800, Junio C Hamano wrote:
>>
>> > A flag to affect the behaviour (as opposed to &flag as a secondary
>> > return value, like Peff's patch does) can be made to work.  Perhaps
>> > a flag that says "keep the input as is if the result is not a local
>> > branch name" would pass an input "@" intact and that may be
>> > sufficient to allow "git branch -m @" to rename the current branch
>> > to "@" (I do not think it is a sensible rename, though ;-).  But
>> > probably some callers need to keep the original input and compare
>> > with the result to see if we expanded anything if we go that route.
>> > At that point, I am not sure if there are much differences in the
>> > ease of use between the two approaches.
>>
>> I just went into more detail in my reply to Jacob, but I do think this
>> is a workable approach (and fortunately we seem to have banned bare "@"
>> as a name, along with anything containing "@{}", so I think we would end
>> up rejecting these nonsense names).
>>
>> I'll see if I can work up a patch. We'll still need to pass the flag
>> around through the various functions, but at least it will be a flag and
>> not a confusing negated out-parameter.
>
> OK, I have a series which fixes this (diffstat below). When I audited
> the other callers of interpret_branch_name() and strbuf_branchname(), it
> turned out to be even more complicated. The callers basically fall into
> a few buckets:
>
>   1. Callers like get_sha1() and merge_name() pass the result to
>      dwim_ref(), and are prepared to handle anything.
>
>   2. Some callers stick "refs/heads/" in front of the result, and
>      obviously only want local names. Most of git-branch and
>      git-checkout fall into this boat.
>
>   3. "git branch -d" can delete local _or_ remote branches, depending on
>      the "-r" flag. So the expansion it wants varies, and we need to
>      handle "just local" or "just remote".
>
> So I converted the "only_branch" flag to an "allowed" bit-field. No
> callers actually ask for more than a single type at once, but it was
> easy to do it that way. It serves all of the callers, and will easily
> adapt for the future (e.g., if "git branch -a -d" were ever allowed).
>
>   [1/8]: interpret_branch_name: move docstring to header file
>   [2/8]: strbuf_branchname: drop return value
>   [3/8]: strbuf_branchname: add docstring
>   [4/8]: interpret_branch_name: allow callers to restrict expansions
>   [5/8]: t3204: test git-branch @-expansion corner cases
>   [6/8]: branch: restrict @-expansions when deleting
>   [7/8]: strbuf_check_ref_format(): expand only local branches
>   [8/8]: checkout: restrict @-expansions when finding branch
>
>  builtin/branch.c                      |   5 +-
>  builtin/checkout.c                    |   2 +-
>  builtin/merge.c                       |   2 +-
>  cache.h                               |  32 ++++++++-
>  refs.c                                |   2 +-
>  revision.c                            |   2 +-
>  sha1_name.c                           |  76 ++++++++++-----------
>  strbuf.h                              |  21 +++++-
>  t/t3204-branch-name-interpretation.sh | 122 ++++++++++++++++++++++++++++++++++
>  9 files changed, 220 insertions(+), 44 deletions(-)
>  create mode 100755 t/t3204-branch-name-interpretation.sh
>
> -Peff

I didn't find any problems besides what you had already outlined
before I started reading the series. It looks pretty much like I
thought it would. I like the idea of saying "I want X" rather than the
command returning "This was a Y"

Re: [BUG] branch renamed to 'HEAD'

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> I didn't find any problems besides what you had already outlined
> before I started reading the series. It looks pretty much like I
> thought it would. I like the idea of saying "I want X" rather than the
> command returning "This was a Y"

Yeah, thanks for reading it through.  Except for one disambiguation
glitch Peff noticed himself, I found the entire series a pleasant
read.