Re: [PATCH v3 8/8] refs: add support for migrating reflogs
- From
karthik nayak <karthik.188@gmail.com>
- Date
- Dec 16, 2024, 15:59 UTC
- Message-ID
- <CAOLa=ZQniLNyxmvypm8BwMHyBRNb6E+SM8AXZiJpprpda5A4bg@mail.gmail.com>
- In-Reply-To
- <xmqqo71b7hkn.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 32 quoted lines
> Patrick Steinhardt <ps@pks.im> writes:
>
>>> +static int migrate_one_reflog(const char *refname, void *cb_data)
>>> +{
>>> + struct migration_data *migration_data = cb_data;
>>> + struct reflog_migration_data data;
>>> +
>>> + data.refname = refname;
>>> + data.old_refs = migration_data->old_refs;
>>> + data.transaction = migration_data->transaction;
>>> + data.errbuf = migration_data->errbuf;
>>> + data.sb = &migration_data->sb;
>>
>> The `index` variable isn't getting initialized here anymore, so its
>> value is essenitally random. I'd propose to use designated initializers
>> for `data` to fix this:
>>
>> struct reflog_migration_data data = {
>> .refname = refname,
>> .old_refs = migration_data->old_refs,
>> .transaction = migration_data->transaction,
>> .errbuf = migration_data->errbuf,
>> .sb = &migration_data->sb,
>> };
>
> GOod. As long as it is sensible to null-initialize the relevant
> field and all the other fields not mentioned above, that certainly
> would give us more predicitable behaviour ;-). I do not offhand
> know if 0 is the right value to initialize the .index member with,
> though; didn't you two recently had an exchange about starting with
> 0 or 1 or something?
>We did [1] indeed, context is that I set it to '1' to distinguish between indexed and non-indexed updates. But it wasn't logically needed and was confusing so I decided to remove that change (which caused the issue here!).
[1]: https://lore.kernel.org/r/CAOLa=ZQ9SHD3gzTVaznGhkCBjrrJbHm1fDyi1F-h6VZvtdpxgw@mail.gmail.com
> Thanks.