Re: breakage in revision traversal with pathspec

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> One question, though. With your patch, if I do "tag1..tag2", I get both
> the tags and the peeled commits in the pending object list. Whereas with
> "^tag1 tag2", we put only the tags into the list, and we expect the
> traversal machinery to peel them later. I cannot off-hand think of a
> reason this difference should be a problem, but I am wondering if there
> is some code path that does not traverse, but just looks at pending
> objects, that might care.

Did I really do that?

I thought that the original was pushing peeled tag1^0 and tag2^0
(and nothing else) for "tag1..tag2", and the intent of the patch was
to see if "a" (which is "tag1^0" in this case) has the same object
name as the object originally given on the side of the dots
(i.e. "tag1").  If they differ, that means "a" is the peeled object,
and instead use the original "tag1" for "a_obj" that is pushed into
the pending (and if they are the same, "a_obj" is just "&a->object",
the object itself).  The same for "b", "tag2" and "b_obj".  So at
least I didn't mean to push four objects into the pending list
before prepare_revision_walk() kicks in.

Perhaps I missed something?

Now, when prepare_revision_walk() picks up objects from the pending
list, they are fed to handle_commit(), and these two tags will be
peeled and their commits are returned to be queued in revs->commits
linked list, while the tags themselves are sent to the pending list
to be emitted in "--objects" output. But that should be the same
between "tag1..tag2" and "^tag1 tag2".

A possible difference in behaviour is that with "^tag1 tag2", we do
not instantiate the commit objects pointed at by these tags until
prepare_revision_walk() sends these tags to handle_commit(), while
with "tag1..tag2", these tags and the commit objects would already
be parsed when setup_revisions() returns (and the updated code does
rely on this behaviour by saying "if a->object.sha1 and from_sha1
are different, we know the tag whose name is from_sha1 is already
parsed, so we can just call lookup_object() on from_sha1 to grab
it").  But I do not think any code just tries to grab an object
using a random object name outside the revision traversal and decide
to do things that results in semantically different behaviour if the
resulting object has (or has not) already been parsed.