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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html