Re: Multiple fetches when unshallowing a shallow clone

From: Jason Paller-Rzepka <jasonpr@google.com>
Date: Dec 7, 2015, 19:57 UTC
Message-ID: <CACs8u9TvUKiPN9Brgym_zDea+_-MWWP9r4wZDa_qib6QxJ6YcA@mail.gmail.com>
In-Reply-To: <CACsJy8DA2Xg9bRmudsRgwy9k=BCawTRU=7bm+rEXv2KoZgoXJw@mail.gmail.com>

I should point out that I never saw a "deepen 0" line. Rather, I saw git send "option depth 0" to the remote helper. Duy, you mentioned that depth=0 means "do not change depth". I assume that means the server should use exactly the shallows that the client sent, and it does not need to traverse the tree or modify the shallow or unshallow sets at all. Right? Duy, you also mentioned that "those lines should be rejected any way". You just mean that a "deepen 0" line should be rejected, right? And that's because the right way to tell git-upload-pack not to change the depth is to omit the "deepen" line after the "shallow" lines, so there's never a need to send "deepen 0"?

Thanks!

On Sun, Dec 6, 2015 at 5:46 AM, Duy Nguyen <pclouds@gmail.com> wrote:

Show 64 quoted lines

> On Sun, Dec 6, 2015 at 8:01 AM, Jeff King <peff@peff.net> wrote:
>> On Sun, Dec 06, 2015 at 01:37:18AM -0500, Jeff King wrote:
>>
>>> And indeed, replacing the logic with what I wrote does make the backfill
>>> go away in my test case. But it's so far from what is there that I feel
>>> like I must be missing something.
>>
>> I think one thing I was missing is that we need to just grab the
>> _object_, but we need to realize that the ref needs updating[1]. So we
>> cannot skip backfill of any tag that we do not already have, even if we
>> already have the tag object.
>
> It's probably worth adding a few comment lines about this. I did
> search back commit history but did not get this.
>
>> Which made me wonder why this:
>>
>>   git init parent &&
>>   git -C parent commit --allow-empty -m one &&
>>   git clone parent child &&
>>   git -C parent commit --allow-empty -m two &&
>>   git -C parent tag -m mytag foo &&
>>   git -C parent commit --allow-empty -m three &&
>>   git -C child fetch
>>
>> does not appear to need to backfill to pick up refs/tags/foo. But it
>> does. It's just that it hits the quickfetch() code path and does not
>> have to ask the other side for a pack. And that explains why it does hit
>> in the --shallow case: we explicitly disable quickfetch in such cases.
>>
>> For the unshallow case, of course we could use it (but only for the
>> second, backfill fetch). Something like this seems to work for me:
>>
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index ed84963..b33b90f 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -881,6 +881,8 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)
>>
>>         transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
>>         transport_set_option(transport, TRANS_OPT_DEPTH, "0");
>> +       if (unshallow)
>> +               depth = NULL;
>>         fetch_refs(transport, ref_map);
>>
>>         if (gsecondary) {
>>
>> But I admit I am not at all confident that it doesn't cause other
>> problems, or that it covers all cases. Even in a shallow repo, we should
>> be able to quickfetch individual tags, shouldn't we?
>
> Yes. depth is only non-NULL when you pass --depth (or --unshallow).
> quickfetch should happen when you fetch without those options.
>
>> I wonder if we could just always set "depth = NULL" here.
>
> --unshallow is essentially --depth=<max>, so I don't see why
> --unshallow should be singled out here. We probably want to restore
> depth back (or pass a flag to explicitly ignore the "depth" exception
> in quickfetch). For multiple fetches, we spawn new commands so depth
> being NULL does not harm. Just in case somebody tries to fetch a
> couple more times in the same process in future.
> --
> Duy