Re: [PATCH v2 08/20] hash: require hash algorithm in `empty_tree_oid_hex()`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 13, 2024, 15:39 UTC
- Message-ID
- <xmqqv82cn97d.fsf@gitster.g>
- In-Reply-To
- <66763b5a-3ea6-481f-b4d3-5fad76f5da0c@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
Show 13 quoted lines
>> diff --git a/add-interactive.c b/add-interactive.c >> index b5d6cd689a..a0961096cd 100644 >> --- a/add-interactive.c >> +++ b/add-interactive.c >> @@ -557,7 +557,7 @@ static int get_modified_files(struct repository *r, >> s.skip_unseen = filter && i; >> opt.def = is_initial ? >> - empty_tree_oid_hex() : oid_to_hex(&head_oid); >> + empty_tree_oid_hex(the_repository->hash_algo) : oid_to_hex(&head_oid); > > The hunk fragment shows that we already have a struct repository > instance in this function which we should use instead of > "the_repository"
As an internal helper function in add-interactive.c, all of whose callers deal with "struct add_i_state *", it probably should not even take "struct repository *" as a parameter. The state knows what repository we are working with.
Show 13 quoted lines
>> diff --git a/add-patch.c b/add-patch.c
>> index 814de57c4a..86181770f2 100644
>> --- a/add-patch.c
>> +++ b/add-patch.c
>> @@ -420,7 +420,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps)
>> /* could be on an unborn branch */
>> !strcmp("HEAD", s->revision) &&
>> repo_get_oid(the_repository, "HEAD", &oid) ?
>> - empty_tree_oid_hex() : s->revision);
>> + empty_tree_oid_hex(the_repository->hash_algo) : s->revision);
>
> It's not obvious from this hunk but there is a repository instance in
> "s->s->r" which we should use instead of "the_repository"I agree it is the same issue.
Just like a previous effort, making a "faithful" conversion from the original that used the_repository implicitly by explicitly passing the_repository in one patch, and then making semantics corrections of the original (if we were ever working on a repository in s->r that is different from the_repository, the existing code is already buggy) in a separate patch, is a reasonable approach to limit the cognitive load while reviewing the first step, I would say.
Show 13 quoted lines
> diff --git a/sequencer.c b/sequencer.c >> index 68d62a12ff..823691e379 100644 >> --- a/sequencer.c >> +++ b/sequencer.c >> @@ -2263,7 +2263,7 @@ static int do_pick_commit(struct repository *r, >> unborn = 1; >> } else if (unborn) >> oidcpy(&head, the_hash_algo->empty_tree); >> - if (index_differs_from(r, unborn ? empty_tree_oid_hex() : "HEAD", >> + if (index_differs_from(r, unborn ? empty_tree_oid_hex(the_repository->hash_algo) : "HEAD", > > The hunk fragment shows that we already have a struct repository > instance in "r" which we should use instead of "the_repository" here.
Yes, but the same "it is better to make a faithful conversion first, corrections separately in a later step" would apply.
As the sequencer machinery is inherently stateful, I wonder if we should pass around not "repository" but a sequencer state object that may have a pointer to a repository in use. But that of course belongs to the latter, i.e., "making corrections", theme.