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

[Jeff King <peff@peff.net>]

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

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

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


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