Re: [PATCH v3 12/12] hash: stop depending on `the_repository` in `null_oid()`

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 10, 2025, 15:38 UTC
Message-ID: <xmqqjz8waoax.fsf@gitster.g>
In-Reply-To: <CABPp-BG0AtKdUH2g8+_f0J0ViWs5xZKM9kCcbWYZ=uEu6bswdQ@mail.gmail.com>

Elijah Newren <newren@gmail.com> writes:

Show 12 quoted lines

>> @@ -4304,7 +4304,7 @@ static int process_entry(struct merge_options *opt,
>>                 /* Deleted on both sides */
>>                 ci->merged.is_null = 1;
>>                 ci->merged.result.mode = 0;
>> -               oidcpy(&ci->merged.result.oid, null_oid());
>> +               oidcpy(&ci->merged.result.oid, null_oid(the_hash_algo));
>>                 assert(!ci->df_conflict);
>>                 ci->merged.clean = !ci->path_conflict;
>>         }
>
> What you have is an improvement since it's at least making things
> explicit, but these should really be opt->repo->hash_algo.

I think this is following the typical pattern in recent topics of this sort, i.e. first make things explicit and use "the" singleton instance, with the plan to find if there are instances more appropriate in the context. When we passed a repo instance through the call chain, the caller at the root may have passed the_repository down the call chain, to make sure everybody down below will be bug-to-bug compatible (because they either used the_repository or used a helper that implicitly always used the_repository). This is doing the same, and your "but these should really be" belongs to the "with the plan to find" phase, which may come later but outside the scope of this series.

Incidentally, that is the reason why the use of superproject repository instead of the_repository was singled out in the proposed commit log message. It should have been mechanical replacement to make this step mechanical and easier to audit, but with another topic in flight that semantically conflicts the changes in ths patch, it took an exception.