| Message ID | 20230308192050.1291-3-royeldar0@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | status: improve info for detached HEAD | expand |
Roy Eldar <royeldar0@gmail.com> writes: > diff --git a/wt-status.c b/wt-status.c > index 3162241a57..f0a5fb578a 100644 > --- a/wt-status.c > +++ b/wt-status.c > @@ -1632,6 +1632,13 @@ static int grab_1st_switch(struct object_id *ooid UNUSED, > struct grab_1st_switch_cbdata *cb = cb_data; > const char *target = NULL, *end; > > + if (skip_prefix(message, "clone: from ", &message)) { > + oidcpy(&cb->noid, noid); > + strbuf_reset(&cb->buf); > + strbuf_add_unique_abbrev(&cb->buf, noid, DEFAULT_ABBREV); > + return 1; > + } > + > if (!skip_prefix(message, "checkout: moving from ", &message)) > return 0; > target = strstr(message, " to "); Two comments: - The original seems to duplicate the logic already in use for parsing @{-1} in object-name.c::grab_nth_branch_switch(). - Adding new code here would mean that the result of parsing @{-1} and what wt_status_get_detached_from() will report becomes inconsistent, no? After such a clone, "git checkout @{-1}" would say "there is no @{-1}" but "git status" would say it was detached from some hexadecimal object. Thinking about the latter, I think it does not add much value to say that we detached from something that is not a ref, so not adding "clone: from " logic to grab_nth_branch_switch() is probably the right thing to do anyway. But then does it even make sense to have this new logic here? Yes, the head may be detached at some object that is not a local or remote branch. But what is so bad about reporting the fact faithfully, i.e., that we are not on any branch? What commit object we are at can be seen by "git show" or "git rev-parse HEAD" or any other usual ways anyway, so... I personally do not very much appreciate the extra info that is given by saying "HEAD detached at X" and "HEAD detached from X", compared to saying just "Not currently on any branch", especially when these X are not concrete branch names or tag names but just hexadecimal string that needs to be fed to "git describe" to be turned into something that makes sense to humans, and that is probably the reason why I am not a good judge about the change this patch makes. Others who like the "detached at/from X" may be better judges to decide if this change makes sense. Thanks.
Hi, First of all, thanks for the thoughtful response. Junio C Hamano <gitster@pobox.com> writes: > - Adding new code here would mean that the result of parsing @{-1} > and what wt_status_get_detached_from() will report becomes > inconsistent, no? If I understand correctly, the result of parsing @{-1} is the commit checked out before the current one, so grab_nth_branch_switch() gets the commit we've moved _from_, whereas wt-status::grab_1st_switch() gets the commit we've moved _to_. After a clone, there is no commit we've moved _from_. > Yes, the head may be detached at some object that is not a local or > remote branch. But what is so bad about reporting the fact > faithfully, i.e., that we are not on any branch? I thought that we try to avoid showing "Not currently on any branch" as this message is not very user-friendly (see commit b397ea4). Furthermore, showing "HEAD detached at X" where X is the abbreviated hash is more consistent with the behavior of the detached HEAD advice in "git clone", which says Note: switching to 'X' > I personally do not very much appreciate the extra info that is > given by saying "HEAD detached at X" and "HEAD detached from X", > compared to saying just "Not currently on any branch", especially > when these X are not concrete branch names or tag names but just > hexadecimal string that needs to be fed to "git describe" to be > turned into something that makes sense to humans It might be better to show "HEAD detached at X" where X is the concrete tag name which was cloned; but since "grab_1st_switch" digs in the reflog for that information, one cannot figure out the tag name that was used when the repository was cloned. I didn't want to complicate the current logic too much, and IMHO showing the abbreviated hash is the best thing we can do, and it is already what we do in certain cases (e.g. after "git checkout --detach").
diff --git a/t/t7508-status.sh b/t/t7508-status.sh index d279157d28..0ab5bdc1e0 100755 --- a/t/t7508-status.sh +++ b/t/t7508-status.sh @@ -894,7 +894,7 @@ test_expect_success 'status shows detached HEAD properly after cloning a reposit git clone -b test_tag upstream downstream && git -C downstream status >actual && - grep -E "Not currently on any branch." actual + grep -E "HEAD detached at [0-9a-f]+" actual ' test_expect_success 'setup status submodule summary' ' diff --git a/wt-status.c b/wt-status.c index 3162241a57..f0a5fb578a 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1632,6 +1632,13 @@ static int grab_1st_switch(struct object_id *ooid UNUSED, struct grab_1st_switch_cbdata *cb = cb_data; const char *target = NULL, *end; + if (skip_prefix(message, "clone: from ", &message)) { + oidcpy(&cb->noid, noid); + strbuf_reset(&cb->buf); + strbuf_add_unique_abbrev(&cb->buf, noid, DEFAULT_ABBREV); + return 1; + } + if (!skip_prefix(message, "checkout: moving from ", &message)) return 0; target = strstr(message, " to ");
When a remote ref or a tag is checked out, HEAD is automatically detached, and "git status" says 'HEAD detached at ...', instead of 'Not currently on any branch.'; this is done by traversing the reflog and parsing an entry like 'checkout: moving from ... to ...'. In certain situations, HEAD can be detached after "git clone": for example, when "--branch" specifies a non-branch (e.g. a tag). It is preferable to avoid displaying 'Not currently on any branch.', so 'HEAD detached at $sha1' is shown instead. Signed-off-by: Roy Eldar <royeldar0@gmail.com> --- t/t7508-status.sh | 2 +- wt-status.c | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-)