Multiple fetches when unshallowing a shallow clone

> Hi all,
>
> Would anyone be willing to help me understand some shallow-clone
> behavior?  (I found a bug in Dulwich, and I'm looking for some context
> so I can determine how to fix it.)
>
> I learned that cgit sometimes performs two fetches for a `git fetch
> --unshallow`: one with depth 'infinity', and a subsequent one with
> depth zero.

>
> Could anyone answer:
> 1) What is the purpose of the second fetch?
> 2) What does this depth of zero mean? Is it the same as a depth of
> infinity?  (I assume not... but, since I thought the smallest
> meaningful depth was 1, I don't know what else it might mean.)
>
> Thank you!
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

> On Mon, Nov 30, 2015 at 11:35 AM, Jason Paller-Rzepka
> <jasonpr@google.com> wrote:
> > Hi all,
> >
> > Would anyone be willing to help me understand some shallow-clone
> > behavior?  (I found a bug in Dulwich, and I'm looking for some context
> > so I can determine how to fix it.)
> >
> > I learned that cgit sometimes performs two fetches for a `git fetch
> > --unshallow`: one with depth 'infinity', and a subsequent one with
> > depth zero.
> 
> Is there a condition to trigger this 'sometimes' ?
> 
> I just tried reproducing via
> $ GIT_TRACE=1 git fetch --unshallow
> 
> and could not see a second fetch, but only a
> fetch-pack with --depth=2147483647

>>
>> and could not see a second fetch, but only a
>> fetch-pack with --depth=2147483647
>
> This seems to reproduce consistently for me:
>
>   $ git clone --depth=1 git://github.com/git/git

> On Fri, Dec 04, 2015 at 12:46:59PM -0800, Stefan Beller wrote:
>
>> On Mon, Nov 30, 2015 at 11:35 AM, Jason Paller-Rzepka
>> <jasonpr@google.com> wrote:
>> > Hi all,
>> >
>> > Would anyone be willing to help me understand some shallow-clone
>> > behavior?  (I found a bug in Dulwich, and I'm looking for some context
>> > so I can determine how to fix it.)
>> >
>> > I learned that cgit sometimes performs two fetches for a `git fetch
>> > --unshallow`: one with depth 'infinity', and a subsequent one with
>> > depth zero.
>>
>> Is there a condition to trigger this 'sometimes' ?
>>
>> I just tried reproducing via
>> $ GIT_TRACE=1 git fetch --unshallow
>>
>> and could not see a second fetch, but only a
>> fetch-pack with --depth=2147483647
>
> This seems to reproduce consistently for me:
>
>   $ git clone --depth=1 git://github.com/git/git
>   Cloning into 'git'...
>   remote: Counting objects: 2925, done.
>   remote: Compressing objects: 100% (2602/2602), done.
>   remote: Total 2925 (delta 230), reused 2329 (delta 206), pack-reused 0
>   Receiving objects: 100% (2925/2925), 6.17 MiB | 10.80 MiB/s, done.
>   Resolving deltas: 100% (230/230), done.
>
>   $ cd git
>   $ git fetch --unshallow
>   remote: Counting objects: 185430, done.
>   remote: Compressing objects: 100% (46933/46933), done.
>   remote: Total 185430 (delta 140505), reused 181589 (delta 136694), pack-reused 0
>   Receiving objects: 100% (185430/185430), 52.80 MiB | 10.84 MiB/s, done.
>   Resolving deltas: 100% (140505/140505), completed with 1784 local objects.
>   remote: Counting objects: 579, done.
>   remote: Compressing objects: 100% (579/579), done.
>   remote: Total 579 (delta 0), reused 579 (delta 0), pack-reused 0
>   Receiving objects: 100% (579/579), 266.85 KiB | 0 bytes/s, done.
>   [... fetch output ...]
>
> That looks like two packs being received for the --unshallow case.
>
> -Peff

>> But why would fetching a tag (or set of tags) merit a depth of zero?
>> Doesn't depth 1 mean "give me the the objects, and none of their
>> descendants"?  Why use 0?
>
> That comes from this line:
>
>   transport_set_option(transport, TRANS_OPT_DEPTH, "0");
>
> That line blame back to b888d61 (Make fetch a builtin, 2007-09-10),
> which isn't incredibly helpful.

> I think that comes from the original git-fetch.sh, which had:
>
>   ?*)
>           # do not deepen a shallow tree when following tags
>           shallow_depth=
>
> Which makes sense. I think the code at that point is not aware that we
> just "unshallowed" and can therefore drop the depth parameter
> altogether. But I admit I am not all that familiar with the shallow
> code.
>
> +cc Duy, who can probably say something way more intelligent about this
> off the top of his head. :)
>
> -Peff

> Jeff King <peff@peff.net> writes:
>
>>> But why would fetching a tag (or set of tags) merit a depth of zero?
>>> Doesn't depth 1 mean "give me the the objects, and none of their
>>> descendants"?  Why use 0?
>>
>> That comes from this line:
>>
>>   transport_set_option(transport, TRANS_OPT_DEPTH, "0");
>>
>> That line blame back to b888d61 (Make fetch a builtin, 2007-09-10),
>> which isn't incredibly helpful.
>
> Hmm, "0" means "no depth limitations", which is exactly what we want
> in this "unshallow" case, I would think.  The behaviour observed is

> just like a regular fetch that auto-follows tags, where it has to
> make a second fetch if the primary fetch fails to include everything
> that is needed for propagating the tag for whatever reason.
>
> Having said that, IIRC, these days a depth limited clone is created
> implicitly with --single-branch option, and I am not sure what the
> right behaviour for the auto-following of tags in such a repository.

> On Fri, Dec 4, 2015 at 11:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> just like a regular fetch that auto-follows tags, where it has to
>> make a second fetch if the primary fetch fails to include everything
>> that is needed for propagating the tag for whatever reason.
>>
>> Having said that, IIRC, these days a depth limited clone is created
>> implicitly with --single-branch option, and I am not sure what the
>> right behaviour for the auto-following of tags in such a repository.
>
> I suppose followtags feature has been around long enough that we can
> simply trust that and skip the second fetch?

> > I suppose followtags feature has been around long enough that we can
> > simply trust that and skip the second fetch?
> 
> Hmmmm, I wonder why the code needs the backfill fetch while talking
> to a server that has the include-tag capability, then?

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

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

> 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

> 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.
> ...
> [1] I'm still puzzled why find_non_local_tags uses has_sha1_file() on
>     the tag object at all, then.

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> > ...
> > [1] I'm still puzzled why find_non_local_tags uses has_sha1_file() on
> >     the tag object at all, then.
> 
> The designed semantics of auto-following tags (not necessarily as
> implemented or documented, i.e. there may be implementation or
> documentation bugs), I think, is to arrive at the same state as
> doing a fetch (or a push) without the auto-following and then doing
> a separate fetch (or a push) of tags that point at the objects that
> are reachable from the tips of refs after finishing the first
> (i.e. without auto-follow) fetch (or a push).  In a scenario where
> we already have a commit reachable from existing remote-tracking
> branch and the current transfer (be it a fetch or a push, with or
> without auto-follow) does not update any remote-tracking branch
> (because the source side did not have any changes), if the source
> side added a tag that refers to that commit that the receiving end
> lacks, that tag needs to be transferred and then stored.
> 
> So has_sha1_file() is not the right test---if anything, it needs to
> be checking if the object being checked is reachable from a tip of
> some ref.
> 
> But of course, that test is rather expensive, so perhaps the
> implementation cheated and uses has_sha1_file() instead?  The only
> case it would misidentify would be after an aborted fetch (or push)
> left unconnected island of objects and some of these objects that
> are not reachable are pointed at by tags the receiving end does not
> have.

> This seems to reproduce consistently for me:
>
>   $ git clone --depth=1 git://github.com/git/git
>   Cloning into 'git'...
>   remote: Counting objects: 2925, done.
>   remote: Compressing objects: 100% (2602/2602), done.
>   remote: Total 2925 (delta 230), reused 2329 (delta 206), pack-reused 0
>   Receiving objects: 100% (2925/2925), 6.17 MiB | 10.80 MiB/s, done.
>   Resolving deltas: 100% (230/230), done.
>
>   $ cd git
>   $ git fetch --unshallow
>   remote: Counting objects: 185430, done.
>   remote: Compressing objects: 100% (46933/46933), done.
>   remote: Total 185430 (delta 140505), reused 181589 (delta 136694), pack-reused 0
>   Receiving objects: 100% (185430/185430), 52.80 MiB | 10.84 MiB/s, done.
>   Resolving deltas: 100% (140505/140505), completed with 1784 local objects.
>   remote: Counting objects: 579, done.
>   remote: Compressing objects: 100% (579/579), done.
>   remote: Total 579 (delta 0), reused 579 (delta 0), pack-reused 0
>   Receiving objects: 100% (579/579), 266.85 KiB | 0 bytes/s, done.
>   [... fetch output ...]
>
> That looks like two packs being received for the --unshallow case.

> Jeff King <peff@peff.net> writes:
> 
> > This seems to reproduce consistently for me:
> >
> >   $ git clone --depth=1 git://github.com/git/git
> >   Cloning into 'git'...
> >   remote: Counting objects: 2925, done.
> >   remote: Compressing objects: 100% (2602/2602), done.
> >   remote: Total 2925 (delta 230), reused 2329 (delta 206), pack-reused 0
> >   Receiving objects: 100% (2925/2925), 6.17 MiB | 10.80 MiB/s, done.
> >   Resolving deltas: 100% (230/230), done.
> >
> >   $ cd git
> >   $ git fetch --unshallow
> >   remote: Counting objects: 185430, done.
> >   remote: Compressing objects: 100% (46933/46933), done.
> >   remote: Total 185430 (delta 140505), reused 181589 (delta 136694), pack-reused 0
> >   Receiving objects: 100% (185430/185430), 52.80 MiB | 10.84 MiB/s, done.
> >   Resolving deltas: 100% (140505/140505), completed with 1784 local objects.
> >   remote: Counting objects: 579, done.
> >   remote: Compressing objects: 100% (579/579), done.
> >   remote: Total 579 (delta 0), reused 579 (delta 0), pack-reused 0
> >   Receiving objects: 100% (579/579), 266.85 KiB | 0 bytes/s, done.
> >   [... fetch output ...]
> >
> > That looks like two packs being received for the --unshallow case.
> 
> What is puzzling is that I do not seem to see this "two fetches"
> with the local transport.  I only see "deepen 2147483647" in the
> protocol log.

> 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"?