[PATCH] get_oid_basic: detached HEAD and @{-1}

[PATCH] get_oid_basic: detached HEAD and @{-1}

[Rubén Justo]

We discourage the use of branch names that resemble commit ids; but
for convenience not as a limitation:

	$ git commit --allow-empty -m first
	[... 000001] first
	$ oid=$(git rev-parse HEAD)
	$ git commit --allow-empty -m second
	[... 000002] second
	$ git checkout -b $oid
	Switched to a new branch '000001...'

With this usage, removing that risky-named branch and trying to re-use
the branch name again, the result expected is an error but:

	$ git checkout -
	$ git branch -d $oid
	Deleted branch 000001... (was 000002).
	$ git checkout $oid
	... You are in 'detached HEAD' state...
	HEAD is now at 000001...

With @{-1} shortcuts the result is the same, but here we can do better
and give an error as a result:  from the reflog we know @{-1} refers
to a branch named '000001...' which pointed to commit '000002...'.

Let's use this unused-yet information to avoid the ambiguity in the
result.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 object-name.c                         | 20 ++++++++++++++------
 t/t3204-branch-name-interpretation.sh | 10 ++++++++++
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/object-name.c b/object-name.c
index 2dd1a0f56e..3fae19de2f 100644
--- a/object-name.c
+++ b/object-name.c
@@ -878,7 +878,8 @@ static inline int push_mark(const char *string, int len)
 }
 
 static enum get_oid_result get_oid_1(struct repository *r, const char *name, int len, struct object_id *oid, unsigned lookup_flags);
-static int interpret_nth_prior_checkout(struct repository *r, const char *name, int namelen, struct strbuf *buf);
+static int interpret_nth_prior_checkout(struct repository *r, const char *name,
+		int namelen, struct strbuf *buf, struct object_id *ooid);
 
 static int get_oid_basic(struct repository *r, const char *str, int len,
 			 struct object_id *oid, unsigned int flags)
@@ -940,10 +941,12 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 
 	if (nth_prior) {
 		struct strbuf buf = STRBUF_INIT;
+		struct object_id ooid;
 		int detached;
 
-		if (interpret_nth_prior_checkout(r, str, len, &buf) > 0) {
-			detached = (buf.len == r->hash_algo->hexsz && !get_oid_hex(buf.buf, oid));
+		if (interpret_nth_prior_checkout(r, str, len, &buf, &ooid) > 0) {
+			detached = (buf.len == r->hash_algo->hexsz &&
+				!get_oid_hex(buf.buf, oid)) && !oidcmp(oid, &ooid);
 			strbuf_release(&buf);
 			if (detached)
 				return 0;
@@ -1383,9 +1386,10 @@ static int get_oid_oneline(struct repository *r,
 struct grab_nth_branch_switch_cbdata {
 	int remaining;
 	struct strbuf *sb;
+	struct object_id *ooid;
 };
 
-static int grab_nth_branch_switch(struct object_id *ooid UNUSED,
+static int grab_nth_branch_switch(struct object_id *ooid,
 				  struct object_id *noid UNUSED,
 				  const char *email UNUSED,
 				  timestamp_t timestamp UNUSED,
@@ -1405,6 +1409,8 @@ static int grab_nth_branch_switch(struct object_id *ooid UNUSED,
 		len = target - match;
 		strbuf_reset(cb->sb);
 		strbuf_add(cb->sb, match, len);
+		if (cb->ooid)
+			oidcpy(cb->ooid, ooid);
 		return 1; /* we are done */
 	}
 	return 0;
@@ -1416,7 +1422,8 @@ static int grab_nth_branch_switch(struct object_id *ooid UNUSED,
  */
 static int interpret_nth_prior_checkout(struct repository *r,
 					const char *name, int namelen,
-					struct strbuf *buf)
+					struct strbuf *buf,
+					struct object_id *ooid)
 {
 	long nth;
 	int retval;
@@ -1438,6 +1445,7 @@ static int interpret_nth_prior_checkout(struct repository *r,
 		return -1;
 	cb.remaining = nth;
 	cb.sb = buf;
+	cb.ooid = ooid;
 
 	retval = refs_for_each_reflog_ent_reverse(get_main_ref_store(r),
 			"HEAD", grab_nth_branch_switch, &cb);
@@ -1621,7 +1629,7 @@ int repo_interpret_branch_name(struct repository *r,
 		namelen = strlen(name);
 
 	if (!options->allowed || (options->allowed & INTERPRET_BRANCH_LOCAL)) {
-		len = interpret_nth_prior_checkout(r, name, namelen, buf);
+		len = interpret_nth_prior_checkout(r, name, namelen, buf, NULL);
 		if (!len) {
 			return len; /* syntax Ok, not enough switches */
 		} else if (len > 0) {
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 3399344f25..5839884ae4 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -167,4 +167,14 @@ test_expect_success 'modify branch upstream via "@{-1}" and "@{-1}@{upstream}"'
 	test_must_fail git config branch.upstream-other.merge
 '
 
+test_expect_success '@{-1} might look erroneously like a detached HEAD' '
+	oid=$(git rev-parse HEAD) &&
+	git checkout -b $oid &&
+	test_commit new-oid &&
+	git checkout - &&
+	git branch -D $oid &&
+	test_must_fail git checkout @{-1} &&
+	test_must_fail git rev-parse @{-1}
+'
+
 test_done
-- 
2.39.0