On 28/04/2013 02:02, Junio C Hamano wrote:
Kevin Bracey <kevin@xxxxxxxxx> writes:
In the event of an odd merge, we may find ourselves TREESAME to
apparently redundant parents. Prevent simplify_merges() from removing
every TREESAME parent - in the event of such a merge it's useful to see
where we came actually from came.
"came from" :)
@@ -2106,8 +2106,32 @@ static int remove_marked_parents(struct rev_info *revs, struct commit *commit)
{
struct treesame_state *ts = lookup_decoration(&revs->treesame, &commit->object);
struct commit_list **pp, *p;
+ struct commit *su = NULL, *sm = NULL;
What do "su" and "sm" stand for?
"same, unmarked" and "same, marked" were what was in my head.
Could you explain here a bit more the reason why we do not want to
remove them and why "-s ours" is so significant that it deserves to
be singled out? And why randomly picking one that is redundant
(because it is an ancestor of some other parent) is an improvement?
I feel it's consistent with the default non-full-history behaviour. The
parent that we choose not to remove is the same one that the default
log with "simplify_history==1" would have followed: the first parent we
are TREESAME to. Or at least that's the intent. So this parent would
normally be singled out, and it's not an arbitrary (or "random") choice.
It feels wrong to me that --full-history --simplify-merges could produce
a disjoint history from the default. I think it should include the
default simple history. If this code triggered, it should produce
something quite distinctive in a graphical view, which will help find
odd/broken merges.
The "remove-redundant" logic first marks commits that are ancestors
of other commits in the original set, without taking treesame[] into
account at all. If the final objective of the code is to keep paths
that consists of non-treesame[] commits, perhaps the logic needs to
be changed to reject non-treesame[] commits that are ancestors of
other non-treesame[] commits, or something?
Maybe - the main simplify_merges machinery could certainly make use of
the treesame[] information, and one of the motivations for introducing
it was to open up the possibility of richer analysis. I'll spend a
little time to think about whether we want a more fundamental change to
the original scan.
But this patch as it stands was an "easy" change to make with clearly
limited scope and relatively little risk - I specifically wanted just to
include our default "simple" parent.
Kevin
--
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