Re: [PATCH v2] refs: fix creation of corrupted reflogs for symrefs

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 23, 2025, 17:55 UTC
Message-ID: <xmqqv7u5s9cq.fsf@gitster.g>
In-Reply-To: <20250123112944.3922712-1-karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

> Subject: Re: [PATCH v2] refs: fix creation of corrupted reflogs for symrefs

This may be just me, but every time I see the above title, it read to me as if we are on purpose doing "creation of corrupted reflogs for symrefs", but we are failing to do so for some reason, and this commit is about improving the situation so that we can correctly create corrupted reflog entries for symbolic ref updates.

And because the only sensible reason why we may on purpose do "creation of corrupted reflogs" I can think of is perhaps we prepare such corrupted thing to test how robust the production code is when seeing such corrupted data, I would expect to see a change to t/ hierarchy.

But the patch touches the code, not just tests, which makes me doubly puzzled.

It happens every time I see this title and the change. Perhaps drop "corrupted" from the title?

> The commit 297c09eabb (refs: allow multiple reflog entries for the same
> refname, 2024-12-16) added logic for reflogs to exit early in
> `lock_ref_for_update()` after obtaining the required lock. This was

I do not think the actor, who "exits early", is not "reflogs". Should we have "for reflogs" in the above, or perhaps move it to the end of the sentence (i.e. the required lock gets obtained because we want to do some operation "for reflogs")?

Show 5 quoted lines

> added as a performance optimization as it was assumed that no further
> processing was required for reflog-only updates. However this was
> incorrect since for a symref's reflog entry, the update needs to be
> populated with the old_oid value. This is done right after the early
> exit.

"The early exit skipped this required work"?

Show 5 quoted lines

> This caused a bug in Git 2.48 in the files backend where target
> references of symrefs being updated would create a corrupted reflog
> entry for the symref since the old_oid is not populated. Undo the skip
> in logic to fix this issue and also add a test to ensure that such an
> issue doesn't arise in the future.

OK.

> The early exit was added as a performance optimization for reflog-only
> updates, and it wasn't essential to the original changes. As such,
> reverting it shouldn't cause any further issues.

I am not sure if this is even worth saying, as you already said that the early return was done incorrectly assuming that the remainder of the function can be skipped as an optimization. What may help readers is to state that all the rest of the code path that was skipped by a mistaken optimization is necessary and would not do anything unwanted.

> Reported-by: Nika Layzell <nika@thelayzells.com>
> Co-authored-by: Jeff King <peff@peff.net>
> Signed-off-by: Jeff King <peff@peff.net>
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>

Show 16 quoted lines

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index e2316f1dd4..29045aad43 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -2068,4 +2068,13 @@ do
>  
>  done
>  
> +test_expect_success 'update-ref should also create reflog for HEAD' '
> +	test_commit to-rewind &&
> +	git rev-parse HEAD >expect &&
> +	head=$(git symbolic-ref HEAD) &&
> +	git update-ref --create-reflog "$head" HEAD~ &&
> +	git rev-parse HEAD@{1} >actual &&
> +	test_cmp expect actual
> +'

Nice. We could rename "head" to something more meaningful (like "current branch") but I can live with the above version. It is much nicer than assuming on what branch we would be, which was what the previous iteration did.

Thanks.