[PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head

[PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head

[Junio C Hamano]

The original API read "checkout: moving from (.*) to ..." from the
reflog of the HEAD, and returned the substring between "from" and
"to", but there was no way, if the substring was a 40-hex string, to
tell if we were on a detached HEAD at that commit object, or on a
branch whose name happened to be the 40-hex string.

At this point, we cannot afford to change the format recorded in the
reflog, so introduce a heuristics to see if the 40-hex matches the
object name of the commit we are switching out of.  This will
unfortunately mishandle this case:

	HEX=$(git rev-parse master)
	git checkout -b $HEX master
	git checkout master

where we were indeed on a non-detached $HEX branch (i.e. HEAD was
pointing at refs/heads/$HEX, not storing $HEX), of course, but
otherwise should be fairly reliable.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This is a preparatory step for the beginning of a much larger
   series.  Peff is Cc'ed because one of the most tricky issue
   involves what d46a8301930a (fix parsing of @{-1}@{u} combination,
   2010-01-28) did.

 sha1_name.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3820f28..1473bb6 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -862,6 +862,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,
 struct grab_nth_branch_switch_cbdata {
 	int remaining;
 	struct strbuf buf;
+	int detached;
 };
 
 static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
@@ -880,9 +881,14 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
 	if (!match || !target)
 		return 0;
 	if (--(cb->remaining) == 0) {
+		unsigned char sha1[20];
+
 		len = target - match;
 		strbuf_reset(&cb->buf);
 		strbuf_add(&cb->buf, match, len);
+		cb->detached = (len == 40 &&
+				!get_sha1_hex(match, sha1) &&
+				!hashcmp(osha1, sha1));
 		return 1; /* we are done */
 	}
 	return 0;
@@ -891,8 +897,12 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,
 /*
  * Parse @{-N} syntax, return the number of characters parsed
  * if successful; otherwise signal an error with negative value.
+ * The string in buf.buf is either a branch name (needs to be
+ * prefixed with "refs/heads/" if the caller wants to make it
+ * a fully spelled refname) or 40-hex object name of the detached
+ * HEAD, and *detached is set to true for the latter.
  */
-static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)
+static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf, int *detached)
 {
 	long nth;
 	int retval;
@@ -917,6 +927,8 @@ static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)
 	if (0 < for_each_reflog_ent_reverse("HEAD", grab_nth_branch_switch, &cb)) {
 		strbuf_reset(buf);
 		strbuf_add(buf, cb.buf.buf, cb.buf.len);
+		if (detached)
+			*detached = cb.detached;
 		retval = brace - name + 1;
 	}
 
@@ -992,7 +1004,7 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 	char *cp;
 	struct branch *upstream;
 	int namelen = strlen(name);
-	int len = interpret_nth_prior_checkout(name, buf);
+	int len = interpret_nth_prior_checkout(name, buf, NULL);
 	int tmp_len;
 
 	if (!len)
-- 
1.8.3-rc1-182-gc61d106

[PATCH 2/2] interpret_branch_name(): unconfuse @{-1}@{u} when @{-1} is detached

[Junio C Hamano]

Now interpret_nth_prior_checkout() can tell the caller if the result
of expansion of @{-1} is a real branch name or the commit object
name for a detached HEAD state, let's avoid re-interpreting $HEX@{u}
in the latter case.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This prevents us from mistakenly refering to the upstream of an
   unrelated branch in this sequence:

     HEX=$(git rev-parse --verify HEAD)
     git branch $HEX     
     git checkout HEAD^0
     git checkout -
     git log @{-1}@{u}

   The branch created in the first step has never been checked out,
   and @{-1} does not refer to it.  @{-1}@{u} would first turn into 
   $HEX@{u} but we should not look for upstream of refs/heads/$HEX.

 sha1_name.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 1473bb6..d3b6897 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1004,15 +1004,21 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
 	char *cp;
 	struct branch *upstream;
 	int namelen = strlen(name);
-	int len = interpret_nth_prior_checkout(name, buf, NULL);
+	int detached;
+	int len = interpret_nth_prior_checkout(name, buf, &detached);
 	int tmp_len;
 
 	if (!len)
 		return len; /* syntax Ok, not enough switches */
 	if (0 < len && len == namelen)
 		return len; /* consumed all */
-	else if (0 < len) {
-		/* we have extra data, which might need further processing */
+	else if (0 < len && !detached) {
+		/*
+		 * We have extra data, which might need further
+		 * processing.  E.g. for the original "@{-1}@{u}" we
+		 * have converted @{-1} into buf and yet to process
+		 * the remaining @{u} part.
+		 */
 		struct strbuf tmp = STRBUF_INIT;
 		int used = buf->len;
 		int ret;
-- 
1.8.3-rc1-182-gc61d106

[non PATCH */2] preparatory analysis of remaining @{...} issues

[Junio C Hamano]

Currently interpret_branch_name() takes either @{-N}, @{u}, or
some@{u} and returns an abbreviated refname, or a detached HEAD if
@{-N} resolves to such a state.  Local branch names coming back for
@{-N} are returned as branch names without "refs/heads/", upstream
names coming back for @{u} are given after cleaning it up with
shorten_unambiguous_ref().  @{-N} resolved to a detached HEAD yields
a bare 40-HEX.

This makes the caller unnecessarily fragile.  I started asking

(1) perhaps interpret_branch_name() can be updated to return a full
    refname when it does return a ref;

(2) perhaps it can also be updated to say if its input @{-N} refers
    to a detached HEAD state.

I looked at all existing callers of interpret_branch_name() to see
how feasible such a change is.  Here is a preparatory analysis.


refs.c: substitute_branch_name()

    The call to interpret_branch_name() by this function only wants to
    see the branch name replaced to a full refname for the consumption
    of dwim_ref() and dwim_log(), and does not want to see a detached
    HEAD state at all.

    Changing @{-N} that returns the bare branch name to return
    refs/heads/$name is welcome.  Changing $name@{u} that returns an
    abbreviation to return a full refname is also welcome.
    And we can error out early if @{-N} referred to a detached HEAD.

    [answer: Yes/Yes]


revision.c: add_pending_object_with_mode()

    The call to interpret_branch_name() by this function wants to turn
    @{-N} and $name@{u} to a refname so that it can be passed to
    add_reflog_for_walk().  Obviously it does not want to see a
    detached state.

    Again, this benefits if interpret_branch_name returned a full
    refname, as add_reflog_for_walk() eventually ends up passing the
    name to dwim_log().

    [answer: Yes/Yes]


sha1_name.c: get_sha1_basic()

    The call to interpret_branch_name() by this function wants to turn
    @{-N} and $name@{u} to a string that can be fed to get_sha1_1() to
    get an object name.  Returning full refname or full object name is
    a good change.

    [answer: Yes/Yes]


sha1_name.c: interpret_branch_name()

    @{-N}@{u} is first given to interpret_nth_prior_checkout() to turn
    @{-N} into a branch name, and then appends @{u} to the result and
    recursively call this function to expand $branch@{u}.  Expansion
    of @{-N} to full refname can break the second expansion, and needs
    to be adjusted, but if @{-N} turns out to be a detaches state, we
    would want to error out the resulting $HEX@{u} anyway, so updating
    the API would result in a pair of real bug fixes.

    [answer: Yes/Yes -- but this caller needs further adjustment]


sha1_name.c: strbuf_branchname()

    The function wants to return an abbreviated branch name.
    The callers of this function are:

    builtin/branch.c: delete_branches()

	"git branch -d -r master@{u}" may expand to "git branch -d -r
	origin/master" and delete "refs/remotes/origin/master", and "git
	branch -d @{-2}" would expand to "git branch -d next" and delete
	"refs/heads/next".

	strbuf_branchname() that returns a full refname would break
	this caller and the caller needs to be updated to shorten
	its output.

        But if strbuf_branchname() can tell @{-N} were detached, we
	can prevent removal of an unintended local branch.

    builtin/checkout.c: setup_branch_path()

        With the current code, "git checkout -b master@{u}" would
        expand to "git checkout -b origin/master" and end up creating
        "refs/heads/origin/master", which may not be a good thing.

	strbuf_branchname() that returns a full refname would break
	this caller and the caller needs to be updated to shorten
	its output.

        If strbuf_branchname() can tell @{-N} were detached, we can
	error out instead of creating a bogus local branch.

    builtin/merge.c: merge_name()

	Uses strbuf_branchname() to turn @{-N} and $name@{u} into
	something that can be fed to dwim_ref().

        This does benefit if strbuf_branchname() can find out the
	full refname.

        If strbuf_branchname() can tell @{-N} were detached, we can
	error out instead of creating a bogus local branch.

    sha1_name.c: strbuf_check_branch_ref()

	Used to see if the name is reasonable when appended to
	refs/heads/.

	strbuf_branchname() that returns a full refname would break
	this caller and the caller needs to be updated to shorten
	its output.

    So strbuf_branchname() itself can tolerate an updated
    interpret_branch_name() that returns full refname and it will
    benefit if it can learn about detached state (many of its
    callers would appreciate the latter).

    [answer: Yes/Yes -- but this caller and its callers need some
    adjustment].

Re: [PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head

[Jeff King]

On Wed, May 08, 2013 at 02:12:29PM -0700, Junio C Hamano wrote:

> The original API read "checkout: moving from (.*) to ..." from the
> reflog of the HEAD, and returned the substring between "from" and
> "to", but there was no way, if the substring was a 40-hex string, to
> tell if we were on a detached HEAD at that commit object, or on a
> branch whose name happened to be the 40-hex string.
> 
> At this point, we cannot afford to change the format recorded in the
> reflog, so introduce a heuristics to see if the 40-hex matches the
> object name of the commit we are switching out of.  This will
> unfortunately mishandle this case:
> 
> 	HEX=$(git rev-parse master)
> 	git checkout -b $HEX master
> 	git checkout master

I do not think I've ever seen a 40-hex branch name in practice, but I
would think a branch named after the commit tip would be a reasonably
common reason to have one, and would trigger this case.

Since the point of marking the detached HEAD is to turn off things like
"@{-1}@{u}", we would want to be generous and err on the side of
assuming it is a branch if it _might_ be one. IOW, shouldn't we treat
the above sequence as a branch, and therefore mishandle:

  git checkout $HEX^0 master
  git checkout master

by erroneously assuming that we moved to the branch $HEX?

-Peff

Re: [PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Since the point of marking the detached HEAD is to turn off things like
> "@{-1}@{u}", we would want to be generous and err on the side of
> assuming it is a branch if it _might_ be one.

I am not sure X and Y mesh well in your "Since X, we would want Y".
It seems to argue for erring on the side of detached HEAD to me.

Checking the "from" name $HEX against old_sha1 is a local and cheap
measure I added there for the first level of disambiguation.  If
they do not match, we _know_ we didn't come back from a detached
HEAD state.

In order to err on the "favor branch when it could have been one",
you could additionally look for the reflog .git/logs/refs/heads/$HEX
when the "from" name $HEX matches old_sha1 (which is likely to be
detached, but it is possible that we were on the $HEX branch when
its tip was at $HEX) and making sure the tip of that $HEX branch
once used to be at $HEX at the time recorded for @{-N} in the HEAD
reflog in question.

That is far more expensive, though; I doubt it is worth doing.

Re: [PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head

[Jeff King]

On Thu, May 09, 2013 at 10:08:24AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Since the point of marking the detached HEAD is to turn off things like
> > "@{-1}@{u}", we would want to be generous and err on the side of
> > assuming it is a branch if it _might_ be one.
> 
> I am not sure X and Y mesh well in your "Since X, we would want Y".
> It seems to argue for erring on the side of detached HEAD to me.

Thinking on it more, I don't see that one is actually better than the
other. If you claim a detached HEAD when there isn't one, the user says
"stupid git, that was a branch, and you should tell me its upstream".
But if you claim an undetached HEAD when there isn't one, asking for the
upstream provides wildly inaccurate results (e.g., "git checkout
@{-1}@{u}" taking you somewhere unexpected).

> Checking the "from" name $HEX against old_sha1 is a local and cheap
> measure I added there for the first level of disambiguation.  If
> they do not match, we _know_ we didn't come back from a detached
> HEAD state.
> 
> In order to err on the "favor branch when it could have been one",
> you could additionally look for the reflog .git/logs/refs/heads/$HEX
> when the "from" name $HEX matches old_sha1 (which is likely to be
> detached, but it is possible that we were on the $HEX branch when
> its tip was at $HEX) and making sure the tip of that $HEX branch
> once used to be at $HEX at the time recorded for @{-N} in the HEAD
> reflog in question.

I was thinking in terms of @{-1}@{u}, so that you could say "well, do we
have upstream config for such a branch currently?". Because even though
we are digging into history (and it _may_ have been a branch at the
time, but isn't now), if we are ultimately going to ask about the
upstream config (as it is _now_, not when the entry was made), then it
does not matter if the branch was detached or not: we are still going to
return failure either way.

But there are _other_ uses for @{-1}, and I am probably being too
focused on this one use-case.

So given all of the above, I think I am fine with the direction of the
series.

-Peff

Re: [PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head

[Eric Sunshine]

On Wed, May 8, 2013 at 5:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> sha1_name.c: signal if @{-N} was a true branch nameor a detached head

s/nameor/name or/

> The original API read "checkout: moving from (.*) to ..." from the
> reflog of the HEAD, and returned the substring between "from" and
> "to", but there was no way, if the substring was a 40-hex string, to
> tell if we were on a detached HEAD at that commit object, or on a
> branch whose name happened to be the 40-hex string.
>
> At this point, we cannot afford to change the format recorded in the
> reflog, so introduce a heuristics to see if the 40-hex matches the
> object name of the commit we are switching out of.  This will
> unfortunately mishandle this case:
>
>         HEX=$(git rev-parse master)
>         git checkout -b $HEX master
>         git checkout master
>
> where we were indeed on a non-detached $HEX branch (i.e. HEAD was
> pointing at refs/heads/$HEX, not storing $HEX), of course, but
> otherwise should be fairly reliable.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>