Re: git-log --cherry-pick gives different results when using tag or tag^{}
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-log --cherry-pick gives different results when using tag or tag^{}
Jeff King writes: > [+cc Junio, as the bug blames to him] > ... > I think what is happening is that we used to apply the SYMMETRIC_LEFT > flag directly to the commit. Now we apply it to the tag, and it does not > seem to get propagated. The patch below fixes it for me, but I have no > idea if we actually need to be setting the other flags, or just > SYMMETRIC_LEFT. I also wonder if the non-symmetric two-dot case needs to > access any pointed-to commit and propagate flags in a similar way. Thanks. 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. 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. How about doing it this way instead (totally untested, though)? revision.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/revision.c b/revision.c index a68fde6..def070e 100644 --- a/revision.c +++ b/revision.c @@ -276,6 +276,7 @@ static struct commit *handle_commit(struct rev_info *revs, return NULL; die("bad object %s", sha1_to_hex(tag->tagged->sha1)); } + object->flags |= flags; } /* @@ -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; } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-log --cherry-pick gives different results when using tag or tag^{}
On 01/15/2014 10:49 AM, Jeff King wrote: > [+cc Junio, as the bug blames to him] > > On Fri, Jan 10, 2014 at 02:15:40PM +0100, Francis Moreau wrote: > >> In mykernel repository, I'm having 2 different behaviours with git-log >> but I don't understand why: >> >> Doing: >> >> $ git log --oneline --cherry-pick --left-right v3.4.71-1^{}...next >> >> and >> >> $ git log --oneline --cherry-pick --left-right v3.4.71-1...next >> >> give something different (where v3.4.71-1 is a tag). >> >> The command using ^{} looks the one that gives correct result I think. > > Yeah, this looks like a bug. Here's a simple reproduction recipe: Thanks a lot Jeff for your good analyze. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git-log --cherry-pick gives different results when using tag or tag^{}
[+cc Junio, as the bug blames to him] On Fri, Jan 10, 2014 at 02:15:40PM +0100, Francis Moreau wrote: > In mykernel repository, I'm having 2 different behaviours with git-log > but I don't understand why: > > Doing: > > $ git log --oneline --cherry-pick --left-right v3.4.71-1^{}...next > > and > > $ git log --oneline --cherry-pick --left-right v3.4.71-1...next > > give something different (where v3.4.71-1 is a tag). > > The command using ^{} looks the one that gives correct result I think. Yeah, this looks like a bug. Here's a simple reproduction recipe: commit() { echo content >$1 && git add $1 && git commit -m $1 } git init repo && cd repo && commit one && commit two && sleep 1 && git tag -m foo mytag && git checkout -b side HEAD^ && git cherry-pick mytag && commit three The sleep seems to be necessary, to give the commit and its cherry-picked version different commit times (presumably because it impacts the order in which we visit them during the traversal). Running: git log --oneline --decorate --cherry-pick --left-right mytag^{}...HEAD produces the expected: > e36cc32 (HEAD, side) three but running it with the tag, as: git log --oneline --decorate --cherry-pick --left-right mytag...HEAD yields: > e36cc32 (HEAD, side) three > 5e96f7d two > db92fca (tag: mytag, master) two Not only do we get the cherry-pick wrong (we should omit both "twos"), but we seem to erroneously count the tagged "two" as being on the right-hand side, which it clearly is not (and which is probably why we don't find the match via --cherry-pick). This worked in v1.8.4, but is broken in v1.8.5. It bisects to Junio's 895c5ba (revision: do not peel tags used in range notation, 2013-09-19), which sounds promising. I think what is happening is that we used to apply the SYMMETRIC_LEFT flag directly to the commit. Now we apply it to the tag, and it does not seem to get propagated. The patch below fixes it for me, but I have no idea if we actually need to be setting the other flags, or just SYMMETRIC_LEFT. I also wonder if the non-symmetric two-dot case needs to access any pointed-to commit and propagate flags in a similar way. diff --git a/revision.c b/revision.c index 7010aff..1d99bfc 100644 --- a/revision.c +++ b/revision.c @@ -1197,6 +1197,8 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi free_commit_list(exclude); a_flags = flags | SYMMETRIC_LEFT; + a->object.flags |= a_flags; + b->object.flags |= flags; } a_obj->flags |= a_flags; -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html