git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, Emily Shaffer <emilyshaffer@google.com>
Subject: Re: [PATCH] wt-status: expand, not dwim, a "detached from" ref
Date: Wed, 26 Aug 2020 18:47:23 -0700	[thread overview]
Message-ID: <20200827014723.GA750502@google.com> (raw)
In-Reply-To: <20200513004058.34456-1-jonathantanmy@google.com>

Hi,

Jonathan Tan wrote:

> 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

Interesting.  Even worse, it happens when I run "git status".

[...]
> 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.
[...]
> -	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 */

Alas, as Junio mentions downthread, this patch produces a regression
in behavior: before,

	$ git checkout --quiet master@{u}
	$ git status | head -1
	HEAD detached at origin/master

after,

	$ git checkout --quiet master@{u}
	$ git status | head -1
	HEAD detached at 675a4aaf3b2

This ends up being a bit subtle.

The current branch can be in one of three states: I can be on a branch
(HEAD symref pointing to a ref refs/heads/branchname), detached (HEAD
psuedoref pointing to a commit), or on a branch yet to be born.
__git_ps1 in contrib/completion/git-prompt.sh, for example, is able to
show these three states.

Since b397ea4863a (status: show more info than "currently not on any
branch", 2013-03-13), "git status", on the other hand, gives richer
information.  In the detached case, naming the commit that HEAD points
to doesn't really give a user enough information to orient themself,
so we look at the reflog (!) to find out what the user had intended to
check out.  Reflog entries look like

	checkout: moving from master to v1.0

and the "v1.0" can tell us that v1.0 is the tag that we detached HEAD
on.

This strikes me as always having been strange in a few ways:

On one hand, this is using the reflog.  reflog entries are
historically meant to be human-readable.  They are a raw string, not
structured data.  They can be translated to a different human
language.  Other tools interacting with git repositories are not
guaranteed to use the same string.  Changes such as the introduction
of "git switch" could be expected to lead to writing "switch:" instead
of "checkout:" some day.

Beyond that, it involves guesswork.  As b397ea4863a explains,

	When it cannot figure out the original ref, it shows an
	abbreviated SHA-1.

The reflog entry contains the parameter passed to "git checkout" in
the form the user provided --- it is not a full ref name or string
meant to be appended to refs/heads/ but it can be arbitrary syntax
supported by "git checkout".  At the time that the checkout happened,
we know what *commit* that name resolved to, but we do not know what
ref it resolved to:

- refs in the search path can have been created or deleted since then
- with @{u} syntax, the named branch's upstream can have been
  reconfigured
- and so on

If we want to know what ref HEAD was detached at, wouldn't we want
some reliable source of that information that records it at the time
it was known?

Another example is if I used "git checkout -" syntax.  In that case,
the reflog records the commit id that I am checking out, instead of
recording "-".  That's because (after "-" is replaced by "@{-1}") the
"magic branch" handler strbuf_branchname replaces @{-1} with with the
branch name being checked out.  That branch name *also* comes from the
reflog, this time from the <old> side of the

	checkout: moving from <old> to <new>

line, and <old> is populated as a simple commit id or branch name read
from HEAD.  So this creates a bit of an inconsistency: if I run "git
status", "git checkout -", "git checkout -" again, and then "git
status" again, the first "git status" tells me what ref I had used to
detach HEAD but the second does not.

Okay, so much for background.

The motivating error producing

	fatal: HEAD does not point to a branch

is a special case of the "we do not know what ref it resolved to"
problem described above: the string @{upstream} represents the
upstream of the branch that HEAD pointed to *at the time of the
checkout*, but since then HEAD has been detached.

[...]
> 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.

Given the context above, two possibilities seem appealing:

 A. Like you're hinting, could dwim_ref get a variant that returns -1
    instead of die()ing on failure?  That way, we could fulfill the
    intent described in b397ea48:

	When it cannot figure out the original ref, it shows an abbreviated
	SHA-1.

 B. Alternatively, in the @{u} case could we switch together the <old>
    and <new> pieces of the context from the

	checkout: moving from master to @{upstream}

    reflog line to make "master@{upstream}"?  It's still possible for
    the upstream to have changed since then, but at least in the
    common case this would match the lookup that happened at checkout
    time.

Thoughts?

Thanks,
Jonathan

  parent reply	other threads:[~2020-08-27  1:47 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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
2020-08-27  1:47 ` Jonathan Nieder [this message]
2020-08-27  2:10   ` Junio C Hamano
2020-08-29  1:02 ` [PATCH v2 0/2] Fix for git checkout @{u} (non-local) then git status Jonathan Tan
2020-08-29  1:02   ` [PATCH v2 1/2] sha1-name: replace unsigned int with option struct Jonathan Tan
2020-08-29 18:44     ` Junio C Hamano
2020-08-29  1:02   ` [PATCH v2 2/2] wt-status: tolerate dangling marks Jonathan Tan
2020-08-29 18:55     ` Junio C Hamano
2020-08-31 17:17       ` Jonathan Tan
2020-08-31 17:37         ` Junio C Hamano
2020-09-01 22:28 ` [PATCH v3 0/3] Fix for git checkout @{u} (non-local) then git status Jonathan Tan
2020-09-01 22:28   ` [PATCH v3 1/3] sha1-name: replace unsigned int with option struct Jonathan Tan
2020-09-01 22:28   ` [PATCH v3 2/3] refs: move dwim_ref() to header file Jonathan Tan
2020-09-01 22:28   ` [PATCH v3 3/3] wt-status: tolerate dangling marks Jonathan Tan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200827014723.GA750502@google.com \
    --to=jrnieder@gmail.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).