Re: [RFC/PATCH 2/3] simplify-merges: never remove all TREESAME parents

On 28/04/2013 02:02, Junio C Hamano wrote:

Kevin Bracey [off-list ref] writes:

quoted

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" :)

quoted

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