[PATCH 1/2] object-name: detect and report empty reflogs

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 7090 bytes --]

The `ref@{n}` syntax allows the user to request the n'th reflog entry
for a ref. This syntax is zero-indexed, meaning that entry zero refers
to the first entry in the reflog. The behaviour here is quite confusing
though and depends on the state of the corresponding reflog:

  - If the reflog is not empty, we return the first reflog entry.

  - If the reflog is empty, we return the object ID of the ref.

  - If the reflog is missing, we return an error.

This is inconsistent and quite misleading.

This behaviour goes back to 6436a20284 (refs: allow @{n} to work with
n-sized reflog, 2021-01-07), which fixed a bug that wouldn't allow a
user to return the n'th reflog entry with an n-sized reflog. With this
commit, `read_ref_at()` started to special case reading the first entry
of the reflog via a separate `read_ref_at_ent_newest()` function. The
problem here is that we forgot to check whether the callback was invoked
at all, and thus we don't notice empty reflogs.

The commit in question added a test for `ref@{0}` when the reflog is
empty. But that test only works by chance: while `read_ref_at()` won't
initialize the object ID passed in by the pointer, all callers of this
function happen to call `repo_ref_dwim()` and thus pre-populate the
object ID. Thus, the consequence is that we indeed return the object ID
of the refname when the reflog is empty.

This behaviour is documented nowhere, and the fact that we return a
somewhat sensible result to the caller by sheer luck further stresses
the point that this behaviour is only accidental, even if there is a
test covering it.

Furthermore, this behaviour causes problems for the git-show-branch(1)
command. When executing `git show-branch --reflog` for a ref that either
has no or an empty reflog we run into a segfault. This is because the
`read_ref_at()` function doesn't report the error to us, and thus parts
of its out-parameters are not initialized.

Start to detect and report empty or missing reflogs in `read_ref_at()`
and report them to the caller. This results in a change in behaviour
when asking for `ref@{0}` with an empty or missing reflog because we now
die instead of returning the object ID of the ref itself. This adapted
behaviour should lead to less surprises as we now really only report
object IDs to the caller that actually come from the reflog, thus making
the user experience a whole lot more consistent.

This change also fixes the segfault in git-show-branch(1). Note that
this commit does not add a test yet -- this will be handled in the next
commit.

Reported-by: Yasushi SHOJI <yasushi.shoji@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 object-name.c                  | 10 ++++++----
 refs.c                         |  3 ++-
 t/t1506-rev-parse-diagnosis.sh |  8 ++++++++
 t/t1508-at-combinations.sh     | 29 +++++++++++++++++++++++++----
 4 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/object-name.c b/object-name.c
index 3a2ef5d680..e2a6c9d2ec 100644
--- a/object-name.c
+++ b/object-name.c
@@ -994,8 +994,8 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 	if (reflog_len) {
 		int nth, i;
 		timestamp_t at_time;
-		timestamp_t co_time;
-		int co_tz, co_cnt;
+		timestamp_t co_time = 0;
+		int co_tz = 0, co_cnt = 0;
 
 		/* Is it asking for N-th entry, or approxidate? */
 		for (i = nth = 0; 0 <= nth && i < reflog_len; i++) {
@@ -1020,6 +1020,7 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 				return -1;
 			}
 		}
+
 		if (read_ref_at(get_main_ref_store(r),
 				real_ref, flags, at_time, nth, oid, NULL,
 				&co_time, &co_tz, &co_cnt)) {
@@ -1035,9 +1036,10 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
 						show_date(co_time, co_tz, DATE_MODE(RFC2822)));
 				}
 			} else {
-				if (flags & GET_OID_QUIETLY) {
+				if (flags & GET_OID_QUIETLY)
 					exit(128);
-				}
+				if (!co_cnt)
+					die(_("log for '%.*s' is empty"), len, str);
 				die(_("log for '%.*s' only has %d entries"),
 				    len, str, co_cnt);
 			}
diff --git a/refs.c b/refs.c
index c633abf284..a2369e7835 100644
--- a/refs.c
+++ b/refs.c
@@ -1084,6 +1084,7 @@ static int read_ref_at_ent_newest(struct object_id *ooid UNUSED,
 	struct read_ref_at_cb *cb = cb_data;
 
 	set_read_ref_cutoffs(cb, timestamp, tz, message);
+	cb->found_it = 1;
 	oidcpy(cb->oid, noid);
 	/* We just want the first entry */
 	return 1;
@@ -1123,7 +1124,7 @@ int read_ref_at(struct ref_store *refs, const char *refname,
 
 	if (cb.cnt == 0) {
 		refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent_newest, &cb);
-		return 0;
+		return !cb.found_it;
 	}
 
 	refs_for_each_reflog_ent_reverse(refs, refname, read_ref_at_ent, &cb);
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index ef40511d89..9d147c4ade 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -140,6 +140,14 @@ test_expect_success 'incorrect file in :path and :N:path' '
 	test_grep "path .disk-only.txt. exists on disk, but not in the index" error
 '
 
+test_expect_success '@{0} reference with empty reflog' '
+	git checkout -B empty-reflog main &&
+	git reflog expire --expire=now refs/heads/empty-reflog &&
+	test_must_fail git rev-parse empty-reflog@{0} >output 2>error &&
+	test_must_be_empty output &&
+	test_grep "log for ${SQ}empty-reflog${SQ} is empty" error
+'
+
 test_expect_success 'invalid @{n} reference' '
 	test_must_fail git rev-parse main@{99999} >output 2>error &&
 	test_must_be_empty output &&
diff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh
index e841309d0e..020106a1cc 100755
--- a/t/t1508-at-combinations.sh
+++ b/t/t1508-at-combinations.sh
@@ -110,10 +110,31 @@ test_expect_success '@{1} works with only one reflog entry' '
 	test_cmp_rev newbranch~ newbranch@{1}
 '
 
-test_expect_success '@{0} works with empty reflog' '
-	git checkout -B newbranch main &&
-	git reflog expire --expire=now refs/heads/newbranch &&
-	test_cmp_rev newbranch newbranch@{0}
+test_expect_success '@{0} fails with empty reflog' '
+	git checkout -B empty-reflog main &&
+	git reflog expire --expire=now refs/heads/empty-reflog &&
+	cat >expect <<-EOF &&
+	fatal: Needed a single revision
+	EOF
+	test_must_fail git rev-parse --verify missing-reflog@{0} 2>err &&
+	test_cmp expect err
+'
+
+test_expect_success '@{0} fails with missing reflog' '
+	git -c core.logAllRefUpdates=false checkout -B missing-reflog main &&
+	cat >expect <<-EOF &&
+	fatal: Needed a single revision
+	EOF
+	test_must_fail git rev-parse --verify missing-reflog@{0} 2>err &&
+	test_cmp expect err
+'
+
+test_expect_success '@{0} favors first reflog entry with diverged reflog' '
+	git checkout -B diverged-reflog main &&
+	test_commit A &&
+	test_commit B &&
+	git reflog delete diverged-reflog@{0} &&
+	test_cmp_rev diverged-reflog~ diverged-reflog@{0}
 '
 
 test_done
-- 
2.44.0-rc1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]