Re: [PATCH 2/2] apply: do not use the_repository
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 28, 2024, 16:33 UTC
- Message-ID
- <xmqqh6eh7v7g.fsf@gitster.g>
- In-Reply-To
- <ZlWHgBROsPBrmM0D@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
Show 28 quoted lines
> On Tue, May 28, 2024 at 06:32:01AM +0000, Philip Peterson via GitGitGadget wrote:
>> diff --git a/apply.c b/apply.c
>> index 901b67e6255..364c05fbd06 100644
>> --- a/apply.c
>> +++ b/apply.c
>> @@ -3218,13 +3218,13 @@ static int apply_binary(struct apply_state *state,
>> return 0; /* deletion patch */
>> }
>>
>> - if (has_object(the_repository, &oid, 0)) {
>> + if (has_object(state->repo, &oid, 0)) {
>> /* We already have the postimage */
>> enum object_type type;
>> unsigned long size;
>> char *result;
>>
>> - result = repo_read_object_file(the_repository, &oid, &type,
>> + result = repo_read_object_file(state->repo, &oid, &type,
>> &size);
>> if (!result)
>> return error(_("the necessary postimage %s for "
>
> We call `get_oid_hex()` in this function, which ultimately ends up
> accessing `the_repository` via `the_hash_algo`. We should likely convert
> those to `repo_get_oid()`.
>
> There are also other accesses to `the_hash_algo` in this function, which
> should be converted to use `state->repo->hash_algo`, as well.We as a more experienced developers should come up with a way to help developers who are less familiar with the API set, so that they can chip in this effort to wean ourselves off of globals.
Would a bug like the ones you pointed out be easily caught by say running "GIT_TEST_DEFAULT_HASH=sha256 make test"?
By the way, for commands like "git apply" that can and does work outside a repository, a change to use state->repo instead of the_repository is only half a story. Dealing with cases where there is no repository is the other half. I _think_ we have drawn a reasonable line to punt and refuse binary patches and three-way fallback outside a repository, but there may be use cases that benefit from being able to do these things in a tarball extract.