Re: [RFC/PATCH 2/3] simplify-merges: never remove all TREESAME parents
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:57:03
Kevin Bracey [off-list ref] writes:
quoted hunk
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. Signed-off-by: Kevin Bracey <redacted> --- Documentation/rev-list-options.txt | 3 ++- revision.c | 24 ++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-)diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 380db48..0832004 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt@@ -472,7 +472,8 @@ history according to the following rules: + * Replace each parent `P` of `C'` with its simplification `P'`. In the process, drop parents that are ancestors of other parents, and - remove duplicates. + remove duplicates, but take care to never drop all parents that + we are TREESAME to. + * If after this parent rewriting, `C'` is a root or merge commit (has zero or >1 parents), a boundary commit, or !TREESAME, it remains.diff --git a/revision.c b/revision.c index 176eb7b..4e27c9a 100644 --- a/revision.c +++ b/revision.c@@ -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?
int n, removed = 0;
+ /* Prescan - look for both marked and unmarked TREESAME parents */
+ for (p = commit->parents, n = 0; p; p = p->next, n++) {
+ if (ts->treesame[n]) {
+ if (p->item->object.flags & TMP_MARK) {
+ if (!sm) sm = p->item;
+ }
+ else {
+ if (!su) {
+ su = p->item;
+ break;
+ }
+ }
+ }
+ }
+
+ /* If we are TREESAME to a marked-for-deletion parent, but not to any
+ * unmarked parents, unmark the first TREESAME parent. We don't want
+ * to remove our "real" parent in the event of an "-s ours" type
+ * merge.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? 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?
+ */
+ if (!su && sm)
+ sm->object.flags &= ~TMP_MARK;
+
pp = &commit->parents;
n = 0;
while ((p = *pp) != NULL) {