Re: [PATCH 5/5] pack-objects: walk tag chains for --include-tag
On Wed, Sep 07, 2016 at 11:49:28AM -0700, Junio C Hamano wrote: > Jeff King 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
Re: [PATCH 5/5] pack-objects: walk tag chains for --include-tag
Jeff King 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.
Re: [PATCH 5/5] pack-objects: walk tag chains for --include-tag
On Mon, Sep 05, 2016 at 05:52:26PM -0400, Jeff King wrote: > When pack-objects is given --include-tag, it peels each tag > ref down to a non-tag object, and if that non-tag object is > going to be packed, we include the tag, too. But what > happens if we have a chain of tags (e.g., tag "A" points to > tag "B", which points to commit "C")? > > We'll peel down to "C" and realize that we want to include > tag "A", but we do not ever consider tag "B", leading to a > broken pack (assuming "B" was not otherwise selected). > Instead, we have to walk the whole chain, adding any tags we > find to the pack. So technically one might argue that this pack isn't "broken", in that it _is_ a valid pack. It's just that it doesn't contain what the user was asking for. 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 dunno. This seems like the most elegant place to fix it, though it does mean that pack-objects will go to some slight extra work when auto-packing a tag (it has to parse the tag to find out whether it's a chain). I'm doubt it matters much in practice. -Peff
