When simplifying an odd merge, such as one that used "-s ours", we may
find ourselves TREESAME to apparently redundant parents. Prevent
simplify_merges() from removing every TREESAME parent; if this would
happen reinstate the first TREESAME parent - the one that the default
log would have followed.
This avoids producing a totally disjoint history from the default log
when the default log is a better explanation of the end result, and aids
visualisation of odd merges.
Signed-off-by: Kevin Bracey <kevin@xxxxxxxxx>
---
Documentation/rev-list-options.txt | 3 +-
revision.c | 69 ++++++++++++++++++++++++++++++++++++++
t/t6111-rev-list-treesame.sh | 2 +-
3 files changed, 72 insertions(+), 2 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 1dad341..e23bdb0 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -471,7 +471,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 c88ded8..7535757 100644
--- a/revision.c
+++ b/revision.c
@@ -2119,6 +2119,73 @@ static int mark_redundant_parents(struct rev_info *revs, struct commit *commit)
return marked;
}
+/*
+ * Awkward naming - this means one parent we are TREESAME to.
+ * cf mark_treesame_root_parents: root parents that are TREESAME (to an
+ * empty tree). Better name suggestions?
+ */
+static int leave_one_treesame_to_parent(struct rev_info *revs, struct commit *commit)
+{
+ struct treesame_state *ts = lookup_decoration(&revs->treesame, &commit->object);
+ struct commit *unmarked = NULL, *marked = NULL;
+ struct commit_list *p;
+ unsigned n;
+
+ for (p = commit->parents, n = 0; p; p = p->next, n++) {
+ if (ts->treesame[n]) {
+ if (p->item->object.flags & TMP_MARK) {
+ if (!marked)
+ marked = p->item;
+ } else {
+ if (!unmarked) {
+ unmarked = p->item;
+ break;
+ }
+ }
+ }
+ }
+
+ /*
+ * If we are TREESAME to a marked-for-deletion parent, but not to any
+ * unmarked parents, unmark the first TREESAME parent. This is the
+ * parent that the default simplify_history==1 scan would have followed,
+ * and it doesn't make sense to omit that path when asking for a
+ * simplified full history. Retaining it improves the chances of
+ * understanding odd missed merges that took an old version of a file.
+ *
+ * Example:
+ *
+ * I---------X A modified the file, but mainline merge X used
+ * \ / "-s ours", so took the version from I. X is
+ * `--A--' TREESAME to I and !TREESAME to A.
+ *
+ * Default log from X would produce "I". Without this check,
+ * --full-history --simplify-merges would produce "I-A-X", showing
+ * the merge commit X and that it changed A, but not making clear that
+ * it had just taken the I version. With this check, the topology above
+ * is retained.
+ *
+ * Note that it is possible that the simplification chooses a different
+ * TREESAME parent from the default, in which case this test doesn't
+ * activate, and we _do_ drop the default parent. Example:
+ *
+ * I------X A modified the file, but it was reverted in B,
+ * \ / meaning mainline merge X is TREESAME to both
+ * A--B parents.
+ *
+ * Default log would produce "I" by following the first parent;
+ * --full-history --simplify-merges will produce "I-A-B". But this is a
+ * reasonable result - it presents a logical full history leading from
+ * I to X, and X is not an important merge.
+ */
+ if (!unmarked && marked) {
+ marked->object.flags &= ~TMP_MARK;
+ return 1;
+ }
+
+ return 0;
+}
+
static int remove_marked_parents(struct rev_info *revs, struct commit *commit)
{
struct commit_list **pp, *p;
@@ -2222,6 +2289,8 @@ static struct commit_list **simplify_one(struct rev_info *revs, struct commit *c
if (1 < cnt) {
int marked = mark_redundant_parents(revs, commit);
if (marked)
+ marked -= leave_one_treesame_to_parent(revs, commit);
+ if (marked)
cnt = remove_marked_parents(revs, commit);
}
diff --git a/t/t6111-rev-list-treesame.sh b/t/t6111-rev-list-treesame.sh
index efc2442..5cc0c34 100755
--- a/t/t6111-rev-list-treesame.sh
+++ b/t/t6111-rev-list-treesame.sh
@@ -104,7 +104,7 @@ check_result 'M L H B A' -- file
check_result 'M L H B A' --parents -- file
check_result 'M L J I H G F D B A' --full-history -- file
check_result 'M L K J I H G F D B A' --full-history --parents -- file
-check_result 'M H L J I G F B A' --simplify-merges -- file
+check_result 'M H L J I G F B A' --simplify-merges -- file # should check that G has parent B
check_result 'M L K G F D B A' --first-parent
check_result 'M L G F B A' --first-parent -- file
--
1.8.3.rc0.28.g682c2d9
--
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