Re: [PATCH 5/5] pack-objects: walk tag chains for --include-tag

From: Jeff King <peff@peff.net>
Date: Sep 7, 2016, 19:45 UTC
Message-ID: <20160907194542.rubdx37mwnho6lrr@sigill.intra.peff.net>
In-Reply-To: <xmqqzinjlf47.fsf@gitster.mtv.corp.google.com>

On Wed, Sep 07, 2016 at 11:49:28AM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > As explained further in the commit message, "fetch" is robust to this,
> > because it does a real connectivity check and follow-on fetch before
> > writing anything it thinks it got via include-tag. So perhaps one could
> > argue that pack-objects is correct; include-tag is best-effort, and it
> > is the client's job to make sure it has everything it needs. And that
> > would mean the bug is in git-clone, which should be doing the
> > connectivity check and follow-on fetch.
> 
> I think that is probably a more technically correct interpretation
> of the history.
> 
> I think upgrading "best-effort" to "guarantee" like you did is a
> right approach nevertheless.  I think the "best-effort" we initially
> did was merely us being lazy.

Yeah, after sleeping on it, the conclusion I came to was that it does not _hurt_ to have include-tag be a bit more careful.

I also wondered about the corner case I noted in the commit message. If you have a tag chain of A->B->C, and you already have "C" (a commit), but are fetching "B" (a tag), then include-tag does not notice "A".

That's OK for git-fetch. It will collect "A" during its backfill phase (not because of "B" at all, but because it knows that "A" eventually peels to "C", which it already has). "git-clone" does not have a backfill, of course. But neither can it "already have" a commit. So either we get "C" as part of the clone (in which case include-tag will include "A"), or it does not (in which case we cannot be getting "B" either, because "C" is reachable from it).

And of course that's only when single-branch is in use. Normally git-clone just grabs all the tags blindly. :)

So I think everything Just Works after my patch, though we do still rely on fetch backfill to pick up some obscure cases.

-Peff