Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] wt-status: expand, not dwim, a "detached from" ref
@ 2020-05-13  0:40 Jonathan Tan
  2020-05-13  5:33 ` Junio C Hamano
  0 siblings, 1 reply; 4+ messages in thread
From: Jonathan Tan @ 2020-05-13  0:40 UTC (permalink / raw)
  To: git; +Cc: Jonathan Tan

When a user checks out the upstream branch of HEAD and then runs "git
branch", like this:

  git checkout @{u}
  git branch

an error message is printed:

  fatal: HEAD does not point to a branch

(This error message when running "git branch" persists even after
checking out other things - it only stops after checking out a branch.)

This is because "git branch" attempts to DWIM "@{u}" when determining
the "HEAD detached" message, but that doesn't work because HEAD no
longer points to a branch.

Therefore, when calculating the status of a worktree, only expand such a
string, not DWIM it. Such strings would not match a ref, and "git
branch" would report "HEAD detached at <hash>" instead.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
I tried to track down other uses of wt_status_get_detached_from()
(called from wt_status_get_state() with get_detached_from=1) but there
seemed to be quite a few, so I stopped.

One alternative is to plumb down a flag from dwim_ref() to
interpret_branch_mark() that indicates that (1) don't read HEAD when
processing, and (2) when encountering a ref of the form "<optional
ref>@{u}", don't die when the optional ref doesn't exist, is not a
branch, or does not have an upstream. (We cannot change the
functionality outright because other parts of Git rely on such a message
being printed.) But this seems too complicated just for diagnosis.
---
 t/t3203-branch-output.sh | 10 ++++++++++
 wt-status.c              |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 71818b90f0..c5d3d739e5 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -209,6 +209,16 @@ EOF
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'git branch shows detached HEAD properly after checking out upstream branch' '
+	git init upstream &&
+	test_commit -C upstream foo &&
+
+	git clone upstream downstream &&
+	git -C downstream checkout @{u} &&
+	git -C downstream branch >actual &&
+	test_i18ngrep "HEAD detached at [0-9a-f]\\+" actual
+'
+
 test_expect_success 'git branch `--sort` option' '
 	cat >expect <<-\EOF &&
 	* (HEAD detached from fromtag)
diff --git a/wt-status.c b/wt-status.c
index 98dfa6f73f..f84ebc3e2c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1562,7 +1562,7 @@ static void wt_status_get_detached_from(struct repository *r,
 		return;
 	}
 
-	if (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 &&
+	if (expand_ref(the_repository, cb.buf.buf, cb.buf.len, &oid, &ref) == 1 &&
 	    /* sha1 is a commit? match without further lookup */
 	    (oideq(&cb.noid, &oid) ||
 	     /* perhaps sha1 is a tag, try to dereference to a commit */
-- 
2.26.2.645.ge9eca65c58-goog


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] wt-status: expand, not dwim, a "detached from" ref
  2020-05-13  0:40 [PATCH] wt-status: expand, not dwim, a "detached from" ref Jonathan Tan
@ 2020-05-13  5:33 ` Junio C Hamano
  2020-05-13 14:59   ` Junio C Hamano
  0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2020-05-13  5:33 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git

Jonathan Tan <jonathantanmy@google.com> writes:

> When a user checks out the upstream branch of HEAD and then runs "git
> branch", like this:
>
>   git checkout @{u}
>   git branch
>
> an error message is printed:
>
>   fatal: HEAD does not point to a branch

I had to spend unreasonable amount of time reproducing this.

    $ git branch --show-current
    master
    $ git checkout -b -t naster master
    $ git branch --show-current
    naster
    $ git checkout @{u}
    $ git branch --show-current
    master
    $ git branch
    * master
      naster

What was missing was that the current branch must be forked from a
remote-tracking branch; with a fork from a local branch, the last
step, i.e. "git branch", works just fine.

> (This error message when running "git branch" persists even after
> checking out other things - it only stops after checking out a branch.)

Correct.  And it is also worth pointing out that this is not "git
branch"; the users would see it primarily as a problem with "git
status", which would die the same way.

> This is because "git branch" attempts to DWIM "@{u}" when determining
> the "HEAD detached" message, but that doesn't work because HEAD no
> longer points to a branch.
>
> Therefore, when calculating the status of a worktree, only expand such a
> string, not DWIM it. Such strings would not match a ref, and "git
> branch" would report "HEAD detached at <hash>" instead.

OK.  "Detached at <hash>" is somewhat unsatisfactory (you most
likely have detached from refs/remotes/origin/<something>), but
it is much better than "fatal" X-<.

> One alternative is to plumb down a flag from dwim_ref() to
> interpret_branch_mark() that indicates that (1) don't read HEAD when
> processing, and (2) when encountering a ref of the form "<optional
> ref>@{u}", don't die when the optional ref doesn't exist, is not a
> branch, or does not have an upstream. (We cannot change the
> functionality outright because other parts of Git rely on such a message
> being printed.)

Thanks for a good analysis.  That likely is a good direction for a
longer term.  @{upstream} is "tell me the upstream of the branch
that is currently checked out", right?  So it is reasonable to
expect that it has no good answer in a detached HEAD state.  And
when on a branch, that branch may not have an upstream, and we
should prepare for such a case, too.

> diff --git a/wt-status.c b/wt-status.c
> index 98dfa6f73f..f84ebc3e2c 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1562,7 +1562,7 @@ static void wt_status_get_detached_from(struct repository *r,
>  		return;
>  	}
>  
> -	if (dwim_ref(cb.buf.buf, cb.buf.len, &oid, &ref) == 1 &&
> +	if (expand_ref(the_repository, cb.buf.buf, cb.buf.len, &oid, &ref) == 1 &&
>  	    /* sha1 is a commit? match without further lookup */
>  	    (oideq(&cb.noid, &oid) ||
>  	     /* perhaps sha1 is a tag, try to dereference to a commit */

Hmph, I just did this:

    $ git branch -b -t next origin/next
    $ git checkout next@{upstream}
    $ git status

It used to say "HEAD detached at origin/next" without this change,
but now it says "HEAD detached at 046d49d455", which smells like a
regression.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] wt-status: expand, not dwim, a "detached from" ref
  2020-05-13  5:33 ` Junio C Hamano
@ 2020-05-13 14:59   ` Junio C Hamano
  2020-05-18 22:24     ` Jonathan Tan
  0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2020-05-13 14:59 UTC (permalink / raw)
  To: Jonathan Tan; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

> OK.  "Detached at <hash>" is somewhat unsatisfactory (you most
> likely have detached from refs/remotes/origin/<something>), but
> it is much better than "fatal" X-<.
>
>> One alternative is to plumb down a flag from dwim_ref() to
>> interpret_branch_mark() that indicates that (1) don't read HEAD when
>> processing, and (2) when encountering a ref of the form "<optional
>> ref>@{u}", don't die when the optional ref doesn't exist, is not a
>> branch, or does not have an upstream. (We cannot change the
>> functionality outright because other parts of Git rely on such a message
>> being printed.)
>
> Thanks for a good analysis.  That likely is a good direction for a
> longer term.  @{upstream} is "tell me the upstream of the branch
> that is currently checked out", right?  So it is reasonable to
> expect that it has no good answer in a detached HEAD state.  And
> when on a branch, that branch may not have an upstream, and we
> should prepare for such a case, too.

Well, I have to take this back.

The real cause of the problem is that we record in reflog that we
switched to @{u}, but when get_detached_from() tries to "dwim" it,
the "current branch" that is implicitly there to be applied to @{u}
is different one from what used to compute which branch to check
out, isn't it?  And it probably is not limited to @{upstream} but
other @{<stuff>} that implicitly apply to the "current branch",
e.g. @{push}.

This causes a few potential problems:

 - The "current" branch may be different from the one when such a
   reflog entry that refers to @{<stuff>} when we later yank
   @{<stuff>} out of the reflog and try to use it.  This is the
   problem the patch under discussion tries to hide, but the patch
   also breaks cases that are working fine.

 - Even if the user gave the branch name (i.e. 'next@{upstream}' in
   the example this patch would have introduced a regression) or if
   we updated get_detached_from() to correctly infer the branch that
   was current when the reflog entry in which we found @{upstream}
   was recorded, the upstream for the branch may have been
   reconfigured between the time when the reflog entry was written
   and get_detached_from() is called.  'next@{upstream}' may have
   been 'origin/next' back then but it may be a different
   remote-tracking branch now.  This issue is not solved by the
   patch---the issue is unfixable unless we log the changes to the
   branch configuration so that we can figure out what was the
   upstream of 'next' back then, which we won't do.

Assuming that is the root cause, I think the right solution likely
lies along these three lines:

 (1) record not @{<stuff>} (or <branch>@{<stuff>} for that matter),
     but the actual starting point in the reflog (e.g. in the
     example this patch would have introduced a regression,
     i.e. next@{u}, we should record 'origin/next'.  In the example
     this patch would have used degraded output to prevent dying,
     i.e. @{u}, we should also record 'origin/next')---this also
     will fix the "the branch's upstream may be different now"
     problem.

 (2) a patch like yours to use expand_ref() as a fallback, but only
     for the "@{<stuff>}" notation that depends on what the "then
     current" branch was---this is a mere band-aid, like the patch
     under discussion is, but at least it would not regress the
     cases that are "working".  "The upstream may be different now"
     problem is still there.

 (3) when we have to interpret @{<stuff>} found in the reflog, go
     back to see which branch was current, and compute @{<stuff>}
     relative to that branch---this would "fix" more cases than (2)
     would, but it won't fix "the upstream can change" problem, and
     I think the trade-off is not all that great.

I think the combination of (1) and (2) is likely to be the best way
to go.  (1) will remove the root cause, and (2) will allow us to
cope with reflog entries before (1) is applied.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] wt-status: expand, not dwim, a "detached from" ref
  2020-05-13 14:59   ` Junio C Hamano
@ 2020-05-18 22:24     ` Jonathan Tan
  0 siblings, 0 replies; 4+ messages in thread
From: Jonathan Tan @ 2020-05-18 22:24 UTC (permalink / raw)
  To: gitster; +Cc: jonathantanmy, git

>  (1) record not @{<stuff>} (or <branch>@{<stuff>} for that matter),
>      but the actual starting point in the reflog (e.g. in the
>      example this patch would have introduced a regression,
>      i.e. next@{u}, we should record 'origin/next'.  In the example
>      this patch would have used degraded output to prevent dying,
>      i.e. @{u}, we should also record 'origin/next')---this also
>      will fix the "the branch's upstream may be different now"
>      problem.

This sounds reasonable. I took a look at this.

The part that converts the user-given refname (e.g. "@{u}") into an OID
is the invocation of get_oid_mb() in parse_branchname_arg() in
builtin/checkout.c, and get_oid_mb() eventually calls repo_dwim_ref()
which has access to the absolute branch name ("origin/master"). I did
not try plumbing it all the way, but I tried overriding "arg" with
"refs/remotes/origin/master" after the call to get_oid_mb() and it
worked.

For reference, the stack between get_oid_mb() and repo_dwim_ref() is as
follows (the line numbers may not be accurate because of some debug
statements I added):

  repo_dwim_ref (refs.c:597)                                                                                
  get_oid_basic (sha1-name.c:875)                                                                           
  get_oid_1 (sha1-name.c:1195)                                                                              
  get_oid_with_context_1 (sha1-name.c:1812)                                                         
  get_oid_with_context (sha1-name.c:1959)
  repo_get_oid (sha1-name.c:1610)
  repo_get_oid_mb (sha1-name.c:1382)

Besides the increase in complicatedness of all the listed functions that
we would need in order to plumb the absolute branch name through, I
haven't checked if the absolute branch name is the one that we should
use whenever we write to the reflog, or if there are some times that we
still want to use the user-specified name. I'll take a further look, but
any ideas are welcome.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-13  0:40 [PATCH] wt-status: expand, not dwim, a "detached from" ref Jonathan Tan
2020-05-13  5:33 ` Junio C Hamano
2020-05-13 14:59   ` Junio C Hamano
2020-05-18 22:24     ` Jonathan Tan

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git