Re: `git update-ref` fails to set reflog old_oid in 2.48
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jan 22, 2025, 06:47 UTC
- Message-ID
- <CAOLa=ZQt8GFPpqnzVLNw0EfPOWDi8hZC0QOpNytRwDxYYDW0Ng@mail.gmail.com>
- In-Reply-To
- <20250121215235.GA2753621@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 48 quoted lines
>
> On Tue, Jan 21, 2025 at 03:40:06PM -0500, Nika Layzell wrote:
>
>> In git 2.48.1, the `git update-ref` subcommand no longer correctly
>> updates the reflog in some cases. Specifically, it appears that the
>> `old_oid` field will not be updated when modifying a branch referenced
>> by another symbolic ref (e.g. HEAD). This doesn't break the `git
>> reflog` subcommand, but does break references like `HEAD@{1}`, which
>> appear to read the `old_oid` field.
>>
>> STR (in a fresh directory):
>> ```
>> git init -b main
>> git commit --allow-empty -m "A"
>> git commit --allow-empty -m "B"
>> git update-ref -m "reason" refs/heads/main HEAD~ HEAD
>> ```
>> [...]
>> The `old_oid` field is empty (all zeroes). This is only the case in
>> derived reflogs (in this case .git/logs/HEAD). The reflog for
>> refs/heads/main appears to be updated correctly.
>
> Thanks for an easy reproduction recipe. Looks like it bisects to
> 297c09eabb (refs: allow multiple reflog entries for the same refname,
> 2024-12-16). Author cc'd.
>
> Just looking at the diff, the early return from lock_ref_for_update()
> seems suspicious to me. Doing this:
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 8953d1c6d3..1c0e24a855 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2611,9 +2611,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,
>
> update->backend_data = lock;
>
> - if (update->flags & REF_LOG_ONLY)
> - goto out;
> -
> if (update->type & REF_ISSYMREF) {
> if (update->flags & REF_NO_DEREF) {
> /*
>
> makes the problem go away, and doesn't fail any tests. But that is just
> me poking at it without understanding why those lines were there in the
> first place.
>The reason for that addition was that there was assumed the flow of `lock_ref_for_update()` for reflog only updates was to capture the lock only [1]. But this is wrong since this misses the old_oid population. As such your fix should do the trick. Thanks!
[1]: https://lore.kernel.org/git/CAOLa=ZQqQTEquJ0e5rG168-CVADR8K-uYma7Z8yiDCptyPgBQg@mail.gmail.com/
> -Peff