From: Junio C Hamano <hidden> Date: 2016-06-15 22:58:43
Kevin Bracey [off-list ref] writes:
On 10/09/2013 20:19, Junio C Hamano wrote:
quoted
I am grumpy X-<.
It appears that we introduced a large breakage during 1.8.4 cycle to
the revision traversal machinery and made pathspec-limited "git log"
pretty much useless.
This command
$ git log v1.8.3.1..v1.8.4 -- git-cvsserver.perl
reports that a merge 766f0f8ef7 (which did not touch the specified
path at all) touches it.
Bisecting points at d0af663e (revision.c: Make --full-history
consider more merges, 2013-05-16).
That merge appearing *with* --full-history would seem like correct
behaviour to me. Or at least it's what I intended.
Oh, of course. "--full-history" is about showing any pointless
change, "the mainline was a lot more up-to-date and there were
changes relative to a fork based on an older baseline", so your
updated "log" should show that in the mainline git-cvsserver.perl
has been more fresh when that merge happened. But it shouldn't
appear if the user does not ask for "--full-history".
However, your particular example occurs *without*--full-history, which
suggests a problem.
Yes.
I note that "gitk v1.8.3^0..v1.8.4" and "git log --parents
v1.8.3..v1.8.4" show that merge in Git 1.8.3, but not in Git 1.8.4. So
we're going partially forwards, at least.
With the testcases demonstrating the cases your series fixed that
all look sensible, I think it is not really an option for us to
revert them; you do not have to defend it with "we are going
partially forwards" ;-).
From: Kevin Bracey <hidden> Date: 2016-06-15 22:58:43
On 11/09/2013 01:23, Junio C Hamano wrote:
Kevin Bracey [off-list ref] writes:
quoted
On 10/09/2013 20:19, Junio C Hamano wrote:
quoted
This command
$ git log v1.8.3.1..v1.8.4 -- git-cvsserver.perl
reports that a merge 766f0f8ef7 (which did not touch the specified
path at all) touches it.
Bisecting points at d0af663e (revision.c: Make --full-history
consider more merges, 2013-05-16).
That merge appearing *with* --full-history would seem like correct
behaviour to me. Or at least it's what I intended.
... But it shouldn't
appear if the user does not ask for "--full-history".
Well, there is a functioning semi-work-around for now: avoid difficult
non-linear questions like "v1.8.3.1..v1.8.4". A question like
"v1.8.3..v1.8.4" is a lot easier to visualise, and it does already omit
the merge.
On reflection I'm not sure what we should for the "simple history" view
of v1.8.3.1..v1.8.4. We're not rewriting parents, so we don't get a
chance to reconsider the merge as being zero-parent, and we do have this
little section of graph to traverse at the bottom:
1.8.3
o----x----x----x----x---x--- (x = included, o =
excluded, *=!treesame)
/
/*
o--x--x--x--x
In effect, we do have a linear section of history to follow, and the
file does change in the middle of that line. It may be quite hard to
come up with a solid rule to hide the merge that doesn't go wrong
somewhere else.
The current rules for this are
1) if identical to any on-graph parent, follow that one, and rewrite the
merge as a non-merge. We currently do not follow to an identical
off-graph parent. This long-standing comment in try_to_simplify_commit
applies: "Even if a merge with an uninteresting side branch brought the
entire change we are interested in, we do not want to lose the other
branches of this merge, so we just keep going." For this query, the
mainline link to 1.8.3 is the "uninteresting side branch"! If you do
specify v1.8.3..v1.8.4, then v1.8.3 becomes "on-graph" thanks to other
new rules, and this rule does kick in, hiding the merge.
2) If rule 1 doesn't activate, and it remains as a merge, hide it if
treesame to all on-graph parents. Previously this rule was "hide if
treesame to any parent", and so that would have hidden the merge.
Now, when I changed rule 2, I did not think this would affect the
default log. See my commit message:
"Now redefine a commit's TREESAME flag to be true only if a commit is
TREESAME to _all_ of its [later: on-graph] parent. This doesn't
affect ... the default
simplify_history behaviour (because partially TREESAME merges are
turned
into normal commits)..."
Whoops - partially TREESAME merges are not always turned into normal
commits.
Maybe the fix is to define TREESAME differently for simplify_history -
to use the old definition of "identical to any parent" in that case. I'm
not sure that's right though.
I currently feel instinctively more disposed to dropping the older
"don't follow off-graph identical parents" rule. Let the default history
go straight to v1.8.3 even though it goes off the graph, stopping us
traversing the topic branch.
Kevin
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:58:43
Kevin Bracey wrote:
On reflection I'm not sure what we should for the "simple history"
view of v1.8.3.1..v1.8.4. We're not rewriting parents, so we don't
get a chance to reconsider the merge as being zero-parent, and we do
have this little section of graph to traverse at the bottom:
1.8.3
o----x----x----x----x---x--- (x = included, o = excluded, *=!treesame)
/
/*
o--x--x--x--x
[...]
1) if identical to any on-graph parent, follow that one, and rewrite
the merge as a non-merge. We currently do not follow to an identical
off-graph parent. This long-standing comment in try_to_simplify_commit
applies: "Even if a merge with an uninteresting side branch brought
the entire change we are interested in, we do not want to lose the
other branches of this merge, so we just keep going."
[...]
2) If rule 1 doesn't activate, and it remains as a merge, hide it if
treesame to all on-graph parents. Previously this rule was "hide if
treesame to any parent", and so that would have hidden the merge.
Now, when I changed rule 2, I did not think this would affect the
default log. See my commit message:
[...]
I currently feel instinctively more disposed to dropping the older
"don't follow off-graph identical parents" rule. Let the default
history go straight to v1.8.3 even though it goes off the graph,
stopping us traversing the topic branch.
Thanks for this analysis. Interesting.
The rule (1) comes from v1.3.0-rc1~13^2~6:
commit f3219fbbba32b5100430c17468524b776eb869d6
Author: Junio C Hamano [off-list ref]
Date: Fri Mar 10 21:59:37 2006 -0800
try_to_simplify_commit(): do not skip inspecting tree change at boundary.
When git-rev-list (and git-log) collapsed ancestry chain to
commits that touch specified paths, we failed to inspect and
notice tree changes when we are about to hit uninteresting
parent. This resulted in "git rev-list since.. -- file" to
always show the child commit after the lower bound, even if it
does not touch the file. This commit fixes it.
Thanks for Catalin for reporting this.
See also:
461cf59f8924f174d7a0dcc3d77f576d93ed29a4
Signed-off-by: Junio C Hamano [off-list ref]
I think you're right that dropping the "don't follow off-graph
treesame parents" rule would be a sensible change. The usual point of
the "follow the treesame parent" rule is to avoid drawing undue
attention to merges of ancient history where some of the parents are
side-branches with an old version of the files being tracked and did
not actually change those files. That rationale applies just as much
for a merge on top of an UNINTERESTING rev as any other merge.
Thanks,
Jonathan
From: Kevin Bracey <hidden> Date: 2016-06-15 22:58:44
On 11/09/2013 21:24, Jonathan Nieder wrote:
Kevin Bracey wrote:
quoted
On reflection I'm not sure what we should for the "simple history"
view of v1.8.3.1..v1.8.4. We're not rewriting parents, so we don't
get a chance to reconsider the merge as being zero-parent, and we do
have this little section of graph to traverse at the bottom:
1.8.3
o----x----x----x----x---x--- (x = included, o = excluded, *=!treesame)
/
/*
o--x--x--x--x
[...]
quoted
1) if identical to any on-graph parent, follow that one, and rewrite
the merge as a non-merge. We currently do not follow to an identical
off-graph parent. This long-standing comment in try_to_simplify_commit
applies: "Even if a merge with an uninteresting side branch brought
the entire change we are interested in, we do not want to lose the
other branches of this merge, so we just keep going."
[...]
quoted
I currently feel instinctively more disposed to dropping the older
"don't follow off-graph identical parents" rule. Let the default
history go straight to v1.8.3 even though it goes off the graph,
stopping us traversing the topic branch.
Thanks for this analysis. Interesting.
The rule (1) comes from v1.3.0-rc1~13^2~6: ...
I think you're right that dropping the "don't follow off-graph
treesame parents" rule would be a sensible change. The usual point of
the "follow the treesame parent" rule is to avoid drawing undue
attention to merges of ancient history where some of the parents are
side-branches with an old version of the files being tracked and did
not actually change those files. That rationale applies just as much
for a merge on top of an UNINTERESTING rev as any other merge.
I agree about the rationale still applying - why not follow off-graph,
unless you're doing --ancestry-path? (Fortunately ancestry_path already
disables simplify_history). That makes more sense if you try to ignore
the misleading comment. In a typical "v1..v3" range, the temporal
limiting means that it's paths to the mainline that will tend to be
marked UNINTERESTING, not to the topic branches...
But I can imagine going off graph it may previously have tripped up
other parts of the code. It could be that this Git 1.3.0 rule ended up
covering over some of the older merge hiding logic flakiness. Maybe it's
no longer necessary. I'll do some experiments.
Now, one bit of news - I have just figured out why gitk is behaving
differently. It transforms ".." before it reaches git.
To see the effect at the command line: "git log v1.8.3..v.1.8.4" hides
the merge, but "git log ^v1.8.3 v1.8.4" shows it. Whoops. A new example
of a dotty shorthand not being exactly equivalent.
In the ".." case the v1.8.3 tag gets peeled before being sent to
add_rev_cmdline , and the "mark bottom commits" logic works. But in the
"^" case, the v1.8.3 doesn't get peeled. Junio - any thoughts on the
correct place to fix that? (And gitk actually does ^<tag-sha>, just to
be odd, so that needs to be handled too). Should these things be peeled
in revs->cmdline or not? We should be consistent.
Kevin