Re: [PATCH 1/2] fetch-pack: remove fetch_if_missing=0
- From
- Jonathan Tan <jonathantanmy@google.com>
- Date
- May 7, 2020, 19:43 UTC
- Message-ID
- <20200507194354.33347-1-jonathantanmy@google.com>
- In-Reply-To
- <eb0cbeeeed080596c130f657186894999ae6121b.1587412477.git.gitgitgadget@gmail.com>
Show 8 quoted lines
> From: Hariom Verma <hariom18599@gmail.com>
>
> Commit 6462d5e ("fetch: remove fetch_if_missing=0", 2019-11-08)
> strove to remove the need for fetch_if_missing=0 from the fetching
> mechanism, so it is plausible to attempt removing fetch_if_missing=0
> from fetch-pack as well.
>
> Signed-off-by: Hariom Verma <hariom18599@gmail.com>As Christian said [1], please include tests like in the commit you mentioned. For a change like this, I think that the test is the most important part.
Also include a justification for why it's safe to remove fetch_if_missing=0. You can probably cite the aforementioned commit to say that it covers the fetch_pack() method, and then go through the rest of the code to see if any may inadvertently fetch an object.
Also, the fetch-pack and index-pack parts can be sent in separate patch sets, so you might want to concentrate on one command first.
[1] https://lore.kernel.org/git/CAP8UFD2SNnpKWtYUztZ76OU7zBsrXyYhG_Zds1wi+NqBKCv+Qw@mail.gmail.com/
Show 13 quoted lines
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 1734a573b01..1ca643f6491 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1649,7 +1649,7 @@ static void update_shallow(struct fetch_pack_args *args,
> struct oid_array extra = OID_ARRAY_INIT;
> struct object_id *oid = si->shallow->oid;
> for (i = 0; i < si->shallow->nr; i++)
> - if (has_object_file(&oid[i]))
> + if (has_object_file_with_flags(&oid[i], OBJECT_INFO_SKIP_FETCH_OBJECT))
> oid_array_append(&extra, &oid[i]);
> if (extra.nr) {
> setup_alternate_shallow(&shallow_lock,Hmm...this triggers when the user requests a clone that is both partial and shallow, and the server reports a shallow object that it didn't send back as a packfile; and it causes another fetch to be sent. This is a separate issue, but Hariom, if you'd like to take a look at this, that would work out too. You'll need to figure out how to make the server send back shallow lines referencing objects that are not in the packfile - one way to do it is to use one-time-perl. (Search the codebase to see how it is used.) This is probably more complex, though.