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

From: Kevin Bracey <kevin@bracey.fi>
Date: Apr 28, 2013, 07:10 UTC
Message-ID: <517CCB57.3060807@bracey.fi>
In-Reply-To: <7vli83shk0.fsf@alter.siamese.dyndns.org>

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

Show 6 quoted lines

> Kevin Bracey <kevin@bracey.fi> 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" :)

Show 7 quoted lines

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

Show 6 quoted lines

> 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