Phillip Wood <phillip.wood123@xxxxxxxxx> 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.
