branch: name detached HEAD analogous to status

"git status" carefully names a detached HEAD "at" resp. "from" a rev or ref depending on whether the detached HEAD has moved since. "git branch" always uses "from", which can be confusing, because a status-aware user would interpret this as moved detached HEAD.

Make "git branch" use the same logic and wording.

---

Notes:
The wording is still different:

HEAD detached at %s
* (detached at %s)

for status (line 1) resp. branch (line 2). Maybe it's worthwhile to use the exact same string so that l10n output is guaranteed to be the same? E.g.

a)
HEAD detached at %s
* (HEAD detached at %s)

or
b)
HEAD (detached at %s)
* (detached at %s)

In case b), "git status" strings would need to change, in case a) only "git branch" strings which change anyway by this patch.

 builtin/branch.c         | 13 ++++++++++---
 t/t3203-branch-output.sh | 39 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 4 deletions(-)

index d8949cb..be391ee 100644
@@ -589,9 +589,16 @@ static char *get_head_description(void)
 	else if (state.bisect_in_progress)
 		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
 			    state.branch);
-	else if (state.detached_from)
-		strbuf_addf(&desc, _("(detached from %s)"),
-			    state.detached_from);
+	else if (state.detached_from) {
+		unsigned char sha1[20];
+		if (!get_sha1("HEAD", sha1) &&
+		    !hashcmp(sha1, state.detached_sha1))
+			strbuf_addf(&desc, _("(detached at %s)"),
+				state.detached_from);
+		else
+			strbuf_addf(&desc, _("(detached from %s)"),
+				state.detached_from);
+	}
 	else
 		strbuf_addstr(&desc, _("(no branch)"));
 	free(state.branch);

index ba4f98e..aaff885 100755
@@ -96,7 +96,7 @@ test_expect_success 'git branch -v pattern does not show branch summaries' '
 
 test_expect_success 'git branch shows detached HEAD properly' '
 	cat >expect <<EOF &&
-* (detached from $(git rev-parse --short HEAD^0))
+* (detached at $(git rev-parse --short HEAD^0))
   branch-one
   branch-two
   master

@@ -106,4 +106,41 @@ EOF
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'git branch shows detached HEAD properly after moving' '
+	cat >expect <<EOF &&
+* (detached from $(git rev-parse --short HEAD))
+  branch-one
+  branch-two
+  master
+EOF
+	git reset --hard HEAD^1 &&
+	git branch >actual &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success 'git branch shows detached HEAD properly from tag' '
+	cat >expect <<EOF &&
+* (detached at fromtag)
+  branch-one
+  branch-two
+  master
+EOF
+	git tag fromtag master &&
+	git checkout fromtag &&
+	git branch >actual &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success 'git branch shows detached HEAD properly after moving from tag' '
+	cat >expect <<EOF &&
+* (detached from fromtag)
+  branch-one
+  branch-two
+  master
+EOF
+	git reset --hard HEAD^1 &&
+	git branch >actual &&
+	test_i18ncmp expect actual
+'
+
 test_done

2.3.0.296.g32c87e1

Except that the wording in "git branch" is more correct, especially if the detached HEAD contains new commits.

In other words, "at" is only correct if the detached HEAD matches the ref. If the HEAD has other commits, it is no longer "at" that ref but instead it has grown "from" it.

But even if the detached HEAD matches the ref, saying it came "from" that ref (with 0 extra commits) is still better than saying detached-HEAD-with-extra-commits is "at" the ref.

		M.

Marc Branchaud venit, vidit, dixit 23.02.2015 16:12:

Sure, but that's exactly what git status does. Haven't you tried out?

And it's exactly what I suggest for git branch. It conveys more information.

Why? Both are true. "at" conveys the additional information that HEAD is still at the that rev.

Michael

Oops, right. Sorry, I got blinded by the various "detached at" examples in your patch's notes.

		M.

Yeah, otherwise the user would wonder why sometimes the object name after that "from" matches "git rev-parse HEAD" and sometimes does not.

In order to make sure that it will be easy for us to maintain that these two commands will keep using the same logic and wording after this "fix" is applied, should this patch do a bit more? Or is it worth doing that for such a small piece of code to be shared?

The following is a tangent and I do not think it is likely we would do anything about it, but I wonder what value we give the end users by showing the "from" information, both in "status" and "branch" in the first place. When I am on a detached HEAD, I'd be doing one of these three things:

(1) I am on some kind of sequencing machinery (e.g. "rebase -i", "cherry-pick A..B", or "bisect"). It does not matter to me at all if I am at the same commit at which I started the sequenced operations or the sequencing machinery has moved me one or more commits along its planned course of action, or where the original point the sequencing machinery detached the HEAD at. I suspect that I would not use "git status" or "git branch" in this mode anyway.

(2) I am sight-seeing, starting with e.g. "git checkout v2.0.0", and moving around with "git checkout $some_other_commit". I'd always see that I am "at" the commit I last checked out, so the distinctions would not be even shown to me.

(3) I am experimenting to fix or enhance an existing thing that is meant to eventually hit a concrete branch, but I do not know if the experiment would pan out. "git checkout $topic~$n" would be to start from near the tip of that $topic ($n may often be 0 but not always) and then I would "git commit" my experiments. When I assess my progress, I'd be interested in what I have that is not in $topic and vice versa since I started that experiment, so

$ git log ...$topic
$ git show-branch HEAD $topic

would be a lot more useful than having to learn "where did I detach" from either "status" or "branch" and then do something about that the abbreviated object name (like feeding it to "describe" or "log").

Of course, the decision to make the point the HEAD was originally detached at is not an issue this patch introduces, but it makes me wonder if that existing "at vs from" logic is an overall win or a loss.

Junio C Hamano venit, vidit, dixit 22.02.2015 20:21:

Yes, I guess I meant RFD when I meant RFC. If that consistency is deemed worthwhile it should at least be guaranteed by the tests, which the test amendments somehow do, but better by a shared code in wt-status.c.

That could possibly be reused by the decorate code - which is how I came about this: In order to decide about consistent HEAD decorations I checked what we have and got confused by status vs. branch.

Not for you nor anyone who routinely detaches heads :)

Despite HEAD reflog and delayed pruning and all that, "detached HEAD" is a state the average user may feel slightly uncomfortable with, and may not even have gotten into on purpose. "git checkout tag" and "git checkout remotebranch" are very easy ways to get there, even "git checkout HEAD^1" and such when mistaking "checkout" for "reset". Therefore, I think about that "at/from" as information (or rather: quick guesstimate) on two things:

- How did I get there? (For this it might be better to say 'at/from HEAD^1' which was sha1" rather than resolving that to a sha1 only. I dunno. Detached heads move so easily...)

- Has something (that could get lost) happened since?

We take a quick and overly cautious approach to answering the 2nd question, of course.

Maybe a "git head" command would be really a better place for all that information:

git head
"master" or "HEAD" (on branch resp. detached state)
git head -v
"master at..." or "HEAD at ..., detached from/at..."
git head -l
list of sha1s of childless prunable commits from HEAD's reflog
git head -d|--detach
alias for "git checkout -detach HEAD"

(just brainstorming)

Michael

I find it very useful, because I often work on HEADs detached from remote branches ("git checkout origin/foo"). If I'm sight-seeing, I like the reminder of which remote branch I checked out, especially because I often have several working tress going at the same time. I also often make trivial changes, like typo fixes, on such detached HEADs, and here too I appreciate the reminder of which remote branch I should push HEAD to.

		M.

wt_status_print() is the only caller of wt_status_get_detached_from(). The latter performs most of the analysis of a detached HEAD, including finding state->detached_from; the caller checks whether the detached HEAD is still at state->detached_from or has moved away.

Move that last bit of analysis to wt_status_get_detached_from(), too, and store the boolean result in state->detached_at.

---
 wt-status.c | 6 +++---
 wt-status.h | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

index 29666d0..e7c1a4b 100644
@@ -1222,6 +1222,8 @@ static void wt_status_get_detached_from(struct wt_status_state *state)
 		state->detached_from =
 			xstrdup(find_unique_abbrev(cb.nsha1, DEFAULT_ABBREV));
 	hashcpy(state->detached_sha1, cb.nsha1);
+	state->detached_at = !get_sha1("HEAD", sha1) &&
+			     !hashcmp(sha1, state->detached_sha1);
 
 	free(ref);
 	strbuf_release(&cb.buf);

@@ -1310,10 +1312,8 @@ void wt_status_print(struct wt_status *s)
 				on_what = _("rebase in progress; onto ");
 				branch_name = state.onto;
 			} else if (state.detached_from) {
-				unsigned char sha1[20];
 				branch_name = state.detached_from;
-				if (!get_sha1("HEAD", sha1) &&
-				    !hashcmp(sha1, state.detached_sha1))
+				if (state.detached_at)
 					on_what = _("HEAD detached at ");
 				else
 					on_what = _("HEAD detached from ");

index 283a9fe..e0a99f7 100644
@@ -84,6 +84,7 @@ struct wt_status_state {
 	int cherry_pick_in_progress;
 	int bisect_in_progress;
 	int revert_in_progress;
+	int detached_at;
 	char *branch;
 	char *onto;
 	char *detached_from;

2.3.1.303.g5174db1

So here's a little refactoring of wt-status, to help branch use the same logic regarding from/at for a detached HEAD.

Michael J Gruber (2):
wt-status: refactor detached HEAD analysis
branch: name detached HEAD analogous to status

 builtin/branch.c         | 13 ++++++++++---
 t/t3203-branch-output.sh | 39 ++++++++++++++++++++++++++++++++++++++-
 wt-status.c              |  6 +++---
 wt-status.h              |  1 +
 4 files changed, 52 insertions(+), 7 deletions(-)

2.3.1.303.g5174db1

"git status" carefully names a detached HEAD "at" resp. "from" a rev or ref depending on whether the detached HEAD has moved since. "git branch" always uses "from", which can be confusing, because a status-aware user would interpret this as moved detached HEAD.

Make "git branch" use the same logic and wording.

---

Notes:
v2 uses the info from refactored wt-status.

In addition, it tries to make sure that branch and status use the same strings:

HEAD detached at %s
* (HEAD detached at %s)

(status first line, branch second line)

Unfortunately, status strings are broken into pieces, so this can be achieved by comments only.

 builtin/branch.c         | 13 ++++++++++---
 t/t3203-branch-output.sh | 39 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 4 deletions(-)

index d8949cb..4c54240 100644
@@ -589,9 +589,16 @@ static char *get_head_description(void)
 	else if (state.bisect_in_progress)
 		strbuf_addf(&desc, _("(no branch, bisect started on %s)"),
 			    state.branch);
-	else if (state.detached_from)
-		strbuf_addf(&desc, _("(detached from %s)"),
-			    state.detached_from);
+	else if (state.detached_from) {
+		/* TRANSLATORS: make sure these match _("HEAD detached at ")
+		   and _("HEAD detached from ") in wt-status.c */
+		if (state.detached_at)
+			strbuf_addf(&desc, _("(HEAD detached at %s)"),
+				state.detached_from);
+		else
+			strbuf_addf(&desc, _("(HEAD detached from %s)"),
+				state.detached_from);
+	}
 	else
 		strbuf_addstr(&desc, _("(no branch)"));
 	free(state.branch);

index ba4f98e..f51d0f3 100755
@@ -96,7 +96,7 @@ test_expect_success 'git branch -v pattern does not show branch summaries' '
 
 test_expect_success 'git branch shows detached HEAD properly' '
 	cat >expect <<EOF &&
-* (detached from $(git rev-parse --short HEAD^0))
+* (HEAD detached at $(git rev-parse --short HEAD^0))
   branch-one
   branch-two
   master

@@ -106,4 +106,41 @@ EOF
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'git branch shows detached HEAD properly after moving' '
+	cat >expect <<EOF &&
+* (HEAD detached from $(git rev-parse --short HEAD))
+  branch-one
+  branch-two
+  master
+EOF
+	git reset --hard HEAD^1 &&
+	git branch >actual &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success 'git branch shows detached HEAD properly from tag' '
+	cat >expect <<EOF &&
+* (HEAD detached at fromtag)
+  branch-one
+  branch-two
+  master
+EOF
+	git tag fromtag master &&
+	git checkout fromtag &&
+	git branch >actual &&
+	test_i18ncmp expect actual
+'
+
+test_expect_success 'git branch shows detached HEAD properly after moving from tag' '
+	cat >expect <<EOF &&
+* (HEAD detached from fromtag)
+  branch-one
+  branch-two
+  master
+EOF
+	git reset --hard HEAD^1 &&
+	git branch >actual &&
+	test_i18ncmp expect actual
+'
+
 test_done

2.3.1.303.g5174db1

It feels somewhat strange to have fields called detached-at and detached-from in wt_status_state.

With this patch,

* branch is either a string "HEAD" or the name of the branch we are on.

* detached_from is the name of the branch HEAD detached from, or NULL if HEAD is on a branch.

* detached_sha1[] is the object name of the commit we last moved to with "checkout --detach".

* detached_at is a boolean that says the tip of detached_from and detached_sha1[] is the same.

There aren't that many users of these fields, so if we were to rename them and clarify what they mean in order to make the code more readable, we may be able to do so without too much churn.

Random conflicting/incoherent thoughts include:

- perhaps branch can be NULL when detached?

- perhaps branch can always point at the branch or the current detached-from?

- perhaps detached_from can be renamed to make it more clear that it is the name of a branch (detached_sha1[] is fine, because it is clear what it is by having "sha1" in its name)?

- perhaps detached_from/detached_at can be made into a single string that is set up by wt-status to hold either "HEAD detached at %s" or "HEAD detached from %s" to be used by its callers?

but I cannot yet reach a coherent whole to be called a suggestion for a better organization X-<.

Will replace what was queued with this version.

Thanks.