Re: git-log --cherry-pick gives different results when using tag or tag^{}

From: Jeff King <peff@peff.net>
Date: Jan 15, 2014, 21:53 UTC
Message-ID: <20140115215343.GA16401@sigill.intra.peff.net>
In-Reply-To: <xmqq1u092f2k.fsf@gitster.dls.corp.google.com>

On Wed, Jan 15, 2014 at 11:57:39AM -0800, Junio C Hamano wrote:

Show 13 quoted lines

> Where do we pass down other flags from tags to commits?  For
> example, if we do this:
> 
> 	$ git log ^v1.8.5 master
> 
> we mark v1.8.5 tag as UNINTERESTING, and throw that tag (not commit
> v1.8.5^0) into revs->pending.objects[].  We do the same for 'master',
> which is a commit.
> 
> Later, in prepare_revision_walk(), we call handle_commit() on them,
> and unwrap the tag v1.8.5 to get v1.8.5^0, and then handles that
> commit object with flags obtained from the tag object.  This code
> only cares about UNINTERESTING and manually propagates it.

Thanks for picking up this line of thought. I had some notion that the right solution would be in propagating the flags later from the pending tags to the commits, but I didn't quite know where to look. Knowing that we explicitly propagate UNINTERESTING but nothing else makes what I was seeing make a lot more sense.

Show 7 quoted lines

> Perhaps that code needs to propagate at least SYMMETRIC_LEFT down to
> the commit object as well, no?  With your patch, the topmost level
> of tag object and the eventual commit object are marked with the
> flag, but if we were dealing with a tag that points at another tag
> that in turn points at a commit, the intermediate tag will not be
> marked with SYMMETRIC_LEFT (nor UNINTERESTING for that matter),
> which may not affect the final outcome, but it somewhat feels wrong.

Agreed. I think the lack of flags on intermediate tags has always been that way, even before 895c5ba, and I do not know of any case where it currently matters. But it seems like the obvious right thing to mark those intermediate tags.

> How about doing it this way instead (totally untested, though)?

Makes sense. It also means we will propagate flags down to any pointed-to trees and blobs. I can't think of a case where that will matter either (and they cannot be SYMMETRIC_LEFT, as that only makes sense for commit objects).

I do notice that when we have a tree, we explicitly propagate UNINTERESTING to the rest of the tree. Should we be propagating all flags instead? Again, I can't think of a reason to do so (and if it is not UNINTERESTING, it is a non-trivial amount of time to mark all paths in the tree).

Show 8 quoted lines

> @@ -287,7 +288,6 @@ static struct commit *handle_commit(struct rev_info *revs,
>  		if (parse_commit(commit) < 0)
>  			die("unable to parse commit %s", name);
>  		if (flags & UNINTERESTING) {
> -			commit->object.flags |= UNINTERESTING;
>  			mark_parents_uninteresting(commit);
>  			revs->limited = 1;
>  		}

We don't need to propagate the UNINTERESTING flag here, because either:

  - "object" pointed to the commit, in which case flags comes from
    object->flags, and we already have it set

or

  - "object" was a tag, and we propagated the flags as we peeled (from
    your earlier hunk)

Makes sense. I think the "mark_blob_uninteresting" call later in the function is now irrelevant for the same reasons. The mark_tree_uninteresting call is not, though, because it recurses.

-Peff