From: Kevin Bracey <kevin@bracey.fi>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org
Subject: Re: [RFC/PATCH 2/3] simplify-merges: never remove all TREESAME parents
Date: Sun, 28 Apr 2013 10:10:15 +0300 [thread overview]
Message-ID: <517CCB57.3060807@bracey.fi> (raw)
In-Reply-To: <7vli83shk0.fsf@alter.siamese.dyndns.org>
On 28/04/2013 02:02, Junio C Hamano wrote:
> 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" :)
>>
>> @@ -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
next prev parent reply other threads:[~2013-04-28 7:10 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-09 18:00 Locating merge that dropped a change Kevin Bracey
2013-04-11 17:28 ` Kevin Bracey
2013-04-11 19:21 ` Junio C Hamano
2013-04-22 19:23 ` [RFC/PATCH] Make --full-history consider more merges Kevin Bracey
2013-04-22 19:49 ` Junio C Hamano
2013-04-23 16:35 ` Kevin Bracey
2013-04-24 22:34 ` Junio C Hamano
2013-04-25 1:59 ` Junio C Hamano
2013-04-25 15:48 ` Kevin Bracey
2013-04-25 16:51 ` Junio C Hamano
2013-04-25 17:11 ` Kevin Bracey
2013-04-25 18:19 ` Junio C Hamano
2013-04-26 19:18 ` Kevin Bracey
2013-04-26 19:31 ` [RFC/PATCH 1/3] revision.c: tighten up TREESAME handling of merges Kevin Bracey
2013-04-26 19:31 ` [RFC/PATCH 2/3] simplify-merges: never remove all TREESAME parents Kevin Bracey
2013-04-27 23:02 ` Junio C Hamano
2013-04-28 7:10 ` Kevin Bracey [this message]
2013-04-28 18:09 ` Junio C Hamano
2013-04-26 19:31 ` [RFC/PATCH 3/3] simplify-merges: drop merge from irrelevant side branch Kevin Bracey
2013-04-27 22:36 ` [RFC/PATCH 1/3] revision.c: tighten up TREESAME handling of merges Junio C Hamano
2013-04-27 22:57 ` David Aguilar
2013-04-28 7:03 ` Kevin Bracey
2013-04-28 18:38 ` Junio C Hamano
2013-04-29 17:46 ` Kevin Bracey
2013-04-29 18:11 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=517CCB57.3060807@bracey.fi \
--to=kevin@bracey.fi \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).