Re: [PATCH v2 08/20] hash: require hash algorithm in `empty_tree_oid_hex()`

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

>> 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.

>> 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.

> 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.