patchrefs: allow @{n} to work with n-sized reflog

This sequence works

	$ git checkout -b newbranch
	$ git commit --allow-empty -m one
	$ git show -s newbranch@{1}

and shows the state that was immediately after the newbranch was created.

But then if you do

	$ git reflog expire --expire=now refs/heads/newbranch
	$ git commit --allow=empty -m two
	$ git show -s newbranch@{1}

you'd be scolded with

	fatal: log for 'newbranch' only has 1 entries

While it is true that it has only 1 entry, we have enough information in that single entry that records the transition between the state in which the tip of the branch was pointing at commit 'one' to the new commit 'two' built on it, so we should be able to answer "what object newbranch was pointing at?". But we refuse to do so.

Make @{0} the special case where we use the new side to look up that entry. Otherwise, look up @{n} using the old side of the (n-1)th entry of the reflog.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 refs.c                      | 48 +++++++++++++++++++++++++++----------
 t/t1503-rev-parse-verify.sh | 17 +++++++++++--
 2 files changed, 50 insertions(+), 15 deletions(-)

On Sat, 2 Jan 2021 at 02:41, Denton Liu <liu.denton@gmail.com> wrote:

The basic idea seems to make sense to me...

... but I can't really say anything about the implementation.

Minor nit: not sure about "expired" -- maybe "first after expiration".

Should this capture the output and compare it to, e.g., `git rev-parse newbranch^`?

Same here, but comparing to `git rev-parse newbranch`? Both of these checks seem worthwhile to make sure that we don't just answer *something*, but that we actually get the right answer, as per your "redefinition".

Speaking of redefinition, does this warrant an update of the documentation? That's a genuine question -- having browsed git-reflog(1) and gitrevisions(7) a bit, I'm not sure.

Martin

On Fri, Jan 01, 2021 at 05:36:06PM -0800, Denton Liu wrote:

I forgot to mention that the original thread that spawned this idea is here:

	https://lore.kernel.org/git/xmqqzh8zgcfp.fsf@gitster.c.googlers.com/

On Fri, Jan 01, 2021 at 05:36:06PM -0800, Denton Liu wrote:

Great! I've run into this issue quite a while ago with regular 'git gc' expiring too old reflog entries, and wondered while the @{N} notation errored out while the information was clearly still there in the reflog.

https://public-inbox.org/git/20130619125059.GD20052@goldbirke/T/#u

I agree with Martin about these tests: not failing is one thing, but we should make sure that the right value is printed.

Denton Liu <liu.denton@gmail.com> writes:

Yeah, I am often hit and irritated by this behaviour.

OK.

The code treats two cases identically (i.e. the case where cb->cnt was originally zero, and one). Is that intended?

I thought the code was to special case only <ref>@{0}, but with this conditional decrement, cb->cnt==0 would not be usable by the rest of the code as the "we must read the new side instead" signal. Is that why null-ness of ooid is also tested here? It is hard to tell the intention because "at_indexed_ent" does not quite tell me what the code wants to use the variable for.

The similarity of this to read_ref_at_ent_oldest is somehow striking. Do we really need to invent a new callback?

Hi Junio,

On Tue, Jan 05, 2021 at 09:55:29PM -0800, Junio C Hamano wrote:

Yep, this was inspired by one of your emails ;)

It shouldn't be possible for cb->cnt == 0 on the first iteration because there's a special-case check at [0]. As a result, it can only be -1 or >= 1 on the first iteration.

The -1 case happens when we're doing date-based lookup and that's what this if is intended to handle.

In the case where it's >= 1, we will always enter the if and it will always pre-decrement. This essentially gets us the n-1 behaviour.

The null-ness of the ooid is needed because on the last entry of the reflog, ooid will be null so we should skip that.

"at_indexed_ent" is meant to signal when we are indexing the reflog numerically (as opposed to by date), we have arrived at the correct entry. If you have a more fitting name, I'm open to suggestions.

Unfortunately, yes. The alternative is to add a flag into `struct read_ref_at_cb` and we could conditionally choose whether or not to copy noid or ooid but this seems like the lesser of two evils.

The duplicated part,

	if (cb->msg)
		*cb->msg = xstrdup(message);
	if (cb->cutoff_time)
		*cb->cutoff_time = timestamp;
	if (cb->cutoff_tz)
		*cb->cutoff_tz = tz;
	if (cb->cutoff_cnt)
		*cb->cutoff_cnt = cb->reccnt;

is actually repeated three times -- once in each of the callbacks. I considered extracting factoring it out into a function but I was on the fence because the function would still have some duplication since it'd still require cb, message, timestamp and tz to all be passed in.

[0]

Thanks, Denton

Denton Liu <liu.denton@gmail.com> writes:

I knew about -1; it wasn't apparent that the caller won't call us with cnt==0. Perhaps it deserves a mention in an in-code comment.

When querying for <ref>@{24}, all the entries are indexed numerically (counted), not just the 24th one, and that contributed to my puzzlement.

I offhand do not think of a "name", but "at target", "found", "reached count", are phrases that come to my mind as starting points.

Thanks.

When there is only one reflog entry (perhaps caused by expiring the reflog and then making a single commit) @{1} errors out even though there is technically enough information to do the lookup. Look at the old side of the reflog instead of the new side so that this does not fail. This is explained in more detail in the commit of the last patch.

This idea was given by Junio at [0].

[0]: https://lore.kernel.org/git/xmqqzh8zgcfp.fsf@gitster.c.googlers.com/

Changes since v1:

* Factor out set_read_ref_cutoffs()

* Check the output of rev-parse to ensure that the intended commit is
  returned

Denton Liu (2):
  refs: factor out set_read_ref_cutoffs()
  refs: allow @{n} to work with n-sized reflog

 refs.c                      | 118 ++++++++++++++++++++----------------
 t/t1503-rev-parse-verify.sh |   4 +-
 t/t1508-at-combinations.sh  |  16 +++++
 3 files changed, 84 insertions(+), 54 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  8f14ec3997 refs: factor out set_read_ref_cutoffs()
1:  0c6885f15f ! 2:  18a35506b8 refs: allow @{n} to work with n-sized reflog
    @@ refs.c: static int read_ref_at_ent(struct object_id *ooid, struct object_id *noi
      	struct read_ref_at_cb *cb = cb_data;
     +	int at_indexed_ent;
      
    - 	cb->reccnt++;
      	cb->tz = tz;
      	cb->date = timestamp;
      
    @@ refs.c: static int read_ref_at_ent(struct object_id *ooid, struct object_id *noi
     +		cb->cnt--;
     +	at_indexed_ent = cb->cnt == 0 && !is_null_oid(ooid);
     +	if (timestamp <= cb->at_time || at_indexed_ent) {
    - 		if (cb->msg)
    - 			*cb->msg = xstrdup(message);
    - 		if (cb->cutoff_time)
    -@@ refs.c: static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
    + 		set_read_ref_cutoffs(cb, timestamp, tz, message);
    + 		/*
      		 * we have not yet updated cb->[n|o]oid so they still
      		 * hold the values for the previous record.
      		 */
    @@ refs.c: static int read_ref_at_ent(struct object_id *ooid, struct object_id *noi
      			warning(_("log for ref %s unexpectedly ended on %s"),
      				cb->refname, show_date(cb->date, cb->tz,
      						       DATE_MODE(RFC2822)));
    +-		cb->reccnt++;
     -		oidcpy(&cb->ooid, ooid);
     -		oidcpy(&cb->noid, noid);
      		cb->found_it = 1;
     -		return 1;
      	}
    + 	cb->reccnt++;
      	oidcpy(&cb->ooid, ooid);
      	oidcpy(&cb->noid, noid);
     -	if (cb->cnt > 0)
    @@ refs.c: static int read_ref_at_ent(struct object_id *ooid, struct object_id *noi
     +{
     +	struct read_ref_at_cb *cb = cb_data;
     +
    -+	if (cb->msg)
    -+		*cb->msg = xstrdup(message);
    -+	if (cb->cutoff_time)
    -+		*cb->cutoff_time = timestamp;
    -+	if (cb->cutoff_tz)
    -+		*cb->cutoff_tz = tz;
    -+	if (cb->cutoff_cnt)
    -+		*cb->cutoff_cnt = cb->reccnt;
    ++	set_read_ref_cutoffs(cb, timestamp, tz, message);
     +	oidcpy(cb->oid, noid);
     +	/* We just want the first entry */
     +	return 1;
    @@ t/t1503-rev-parse-verify.sh: test_expect_success 'fails silently when using -q'
      	test_must_be_empty error
      '
      
    -@@ t/t1503-rev-parse-verify.sh: test_expect_success 'master@{n} for various n' '
    - 	test_must_fail git rev-parse --verify master@{$Np1}
    - '
    +
    + ## t/t1508-at-combinations.sh ##
    +@@ t/t1508-at-combinations.sh: test_expect_success 'create path with @' '
    + check "@:normal" blob content
    + check "@:fun@ny" blob content
      
     +test_expect_success '@{1} works with only one reflog entry' '
    -+	git checkout -B newbranch &&
    ++	git checkout -B newbranch master &&
     +	git reflog expire --expire=now refs/heads/newbranch &&
    -+	git commit --allow-empty -mexpired &&
    -+	git rev-parse --verify newbranch@{1}
    ++	git commit --allow-empty -m "first after expiration" &&
    ++	git rev-parse newbranch~ >expect &&
    ++	git rev-parse newbranch@{1} >actual &&
    ++	test_cmp expect actual
     +'
     +
     +test_expect_success '@{0} works with empty reflog' '
    -+	git checkout -B newbranch &&
    ++	git checkout -B newbranch master &&
     +	git reflog expire --expire=now refs/heads/newbranch &&
    -+	git rev-parse --verify newbranch@{0}
    ++	git rev-parse newbranch >expect &&
    ++	git rev-parse newbranch@{0} >actual &&
    ++	test_cmp expect actual
     +'
    -+
    - test_expect_success SYMLINKS 'ref resolution not confused by broken symlinks' '
    - 	ln -s does-not-exist .git/refs/heads/broken &&
    - 	test_must_fail git rev-parse --verify broken
    + test_done

This block of code is duplicated twice. In a future commit, it will be duplicated for a third time. Factor out the common functionality into set_read_ref_cutoffs().

In the case of read_ref_at_ent(), we are incrementing `cb->reccnt` at the beginning of the function. Move these to right before the return so that the `cb->reccnt - 1` is changed to `cb->reccnt` and it can be cleanly factored out into set_read_ref_cutoffs(). The duplication of the increment statements will be removed in a future patch.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 refs.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

This sequence works

	$ git checkout -b newbranch
	$ git commit --allow-empty -m one
	$ git show -s newbranch@{1}

and shows the state that was immediately after the newbranch was created.

But then if you do

	$ git reflog expire --expire=now refs/heads/newbranch
	$ git commit --allow=empty -m two
	$ git show -s newbranch@{1}

you'd be scolded with

	fatal: log for 'newbranch' only has 1 entries

While it is true that it has only 1 entry, we have enough information in that single entry that records the transition between the state in which the tip of the branch was pointing at commit 'one' to the new commit 'two' built on it, so we should be able to answer "what object newbranch was pointing at?". But we refuse to do so.

Make @{0} the special case where we use the new side to look up that entry. Otherwise, look up @{n} using the old side of the (n-1)th entry of the reflog.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 refs.c                      | 42 ++++++++++++++++++++++++-------------
 t/t1503-rev-parse-verify.sh |  4 ++--
 t/t1508-at-combinations.sh  | 16 ++++++++++++++
 3 files changed, 46 insertions(+), 16 deletions(-)

On Wed, Jan 06, 2021 at 01:01:54AM -0800, Denton Liu wrote:

You could use 'test_cmp_rev' in these two tests to spare a few lines and to get a bit friendlier error message on failure.

When there is only one reflog entry (perhaps caused by expiring the reflog and then making a single commit) @{1} errors out even though there is technically enough information to do the lookup. Look at the old side of the reflog instead of the new side so that this does not fail. This is explained in more detail in the commit of the last patch.

This idea was given by Junio at [0].

[0]: https://lore.kernel.org/git/xmqqzh8zgcfp.fsf@gitster.c.googlers.com/

Changes since v1:

* Factor out set_read_ref_cutoffs()

* Check the output of rev-parse to ensure that the intended commit is
  returned

Changes since v2:

* Rename at_indexed_ent -> reached_count

* Add an in-code comment to document that cb->cnt can't be 0 in the first
  iteration of read_ref_at_ent()

* Make test cases use test_cmp_rev() for brevity and better errors

Denton Liu (2):
  refs: factor out set_read_ref_cutoffs()
  refs: allow @{n} to work with n-sized reflog

 refs.c                      | 122 +++++++++++++++++++++---------------
 t/t1503-rev-parse-verify.sh |   4 +-
 t/t1508-at-combinations.sh  |  12 ++++
 3 files changed, 84 insertions(+), 54 deletions(-)

Range-diff against v2:
1:  8f14ec3997 = 1:  8f14ec3997 refs: factor out set_read_ref_cutoffs()
2:  18a35506b8 ! 2:  c88c997eab refs: allow @{n} to work with n-sized reflog
    @@ refs.c: static int read_ref_at_ent(struct object_id *ooid, struct object_id *noi
      		const char *message, void *cb_data)
      {
      	struct read_ref_at_cb *cb = cb_data;
    -+	int at_indexed_ent;
    ++	int reached_count;
      
      	cb->tz = tz;
      	cb->date = timestamp;
      
     -	if (timestamp <= cb->at_time || cb->cnt == 0) {
    ++	/*
    ++	 * It is not possible for cb->cnt == 0 on the first itertion because
    ++	 * that special case is handled in read_ref_at().
    ++	 */
     +	if (cb->cnt > 0)
     +		cb->cnt--;
    -+	at_indexed_ent = cb->cnt == 0 && !is_null_oid(ooid);
    -+	if (timestamp <= cb->at_time || at_indexed_ent) {
    ++	reached_count = cb->cnt == 0 && !is_null_oid(ooid);
    ++	if (timestamp <= cb->at_time || reached_count) {
      		set_read_ref_cutoffs(cb, timestamp, tz, message);
      		/*
      		 * we have not yet updated cb->[n|o]oid so they still
    @@ refs.c: static int read_ref_at_ent(struct object_id *ooid, struct object_id *noi
      					cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
     -		}
     -		else if (cb->date == cb->at_time)
    -+		if (at_indexed_ent)
    ++		if (reached_count)
     +			oidcpy(cb->oid, ooid);
     +		else if (!is_null_oid(&cb->ooid) || cb->date == cb->at_time)
      			oidcpy(cb->oid, noid);
    @@ t/t1508-at-combinations.sh: test_expect_success 'create path with @' '
     +	git checkout -B newbranch master &&
     +	git reflog expire --expire=now refs/heads/newbranch &&
     +	git commit --allow-empty -m "first after expiration" &&
    -+	git rev-parse newbranch~ >expect &&
    -+	git rev-parse newbranch@{1} >actual &&
    -+	test_cmp expect actual
    ++	test_cmp_rev newbranch~ newbranch@{1}
     +'
     +
     +test_expect_success '@{0} works with empty reflog' '
     +	git checkout -B newbranch master &&
     +	git reflog expire --expire=now refs/heads/newbranch &&
    -+	git rev-parse newbranch >expect &&
    -+	git rev-parse newbranch@{0} >actual &&
    -+	test_cmp expect actual
    ++	test_cmp_rev newbranch newbranch@{0}
     +'
      test_done

This block of code is duplicated twice. In a future commit, it will be duplicated for a third time. Factor out the common functionality into set_read_ref_cutoffs().

In the case of read_ref_at_ent(), we are incrementing `cb->reccnt` at the beginning of the function. Move these to right before the return so that the `cb->reccnt - 1` is changed to `cb->reccnt` and it can be cleanly factored out into set_read_ref_cutoffs(). The duplication of the increment statements will be removed in a future patch.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 refs.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

This sequence works

	$ git checkout -b newbranch
	$ git commit --allow-empty -m one
	$ git show -s newbranch@{1}

and shows the state that was immediately after the newbranch was created.

But then if you do

	$ git reflog expire --expire=now refs/heads/newbranch
	$ git commit --allow=empty -m two
	$ git show -s newbranch@{1}

you'd be scolded with

	fatal: log for 'newbranch' only has 1 entries

While it is true that it has only 1 entry, we have enough information in that single entry that records the transition between the state in which the tip of the branch was pointing at commit 'one' to the new commit 'two' built on it, so we should be able to answer "what object newbranch was pointing at?". But we refuse to do so.

Make @{0} the special case where we use the new side to look up that entry. Otherwise, look up @{n} using the old side of the (n-1)th entry of the reflog.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 refs.c                      | 46 ++++++++++++++++++++++++++-----------
 t/t1503-rev-parse-verify.sh |  4 ++--
 t/t1508-at-combinations.sh  | 12 ++++++++++
 3 files changed, 46 insertions(+), 16 deletions(-)

On Thu, Jan 07, 2021 at 02:36:59AM -0800, Denton Liu wrote:

s/itertion/iteration/

Regards Simon

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Please disregard, I see that the fixup has already been included.

-Denton

On Mon, Jan 11, 2021 at 10:14:14PM -0800, Denton Liu wrote:

Denton Liu <liu.denton@gmail.com> writes:

Being careful never hurts; it may make duplicated work from time to time, but that is better than fixes falling thru the cracks.

Thanks.

Junio,

On Thu, Jan 07, 2021 at 02:36:57AM -0800, Denton Liu wrote:

Topic 'dl/reflog-with-single-entry', i.e. these two patches queued directly on top of v2.29.2, break the test case "61 - valid ref of the form "n", n < N" in 't3903-stash.sh'. Queueing them on top of something already containing commit 4f44c5659b (stash: simplify reflog emptiness check, 2020-10-24) fixes this issue.

SZEDER Gábor <szeder.dev@gmail.com> writes:

Thanks for carefully watching ;-)

There is no reason why this fix needs to be backported down to 2.29 track, I would think.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t1508-at-combinations.sh | 1 +
 1 file changed, 1 insertion(+)