git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v4 10/15] simplify-merges: never remove all TREESAME parents

From
Kevin Bracey <kevin@bracey.fi>
Date
May 16, 2013, 15:32 UTC
Message-ID
<1368718361-27859-11-git-send-email-kevin@bracey.fi>
In-Reply-To
<1368718361-27859-1-git-send-email-kevin@bracey.fi>

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@bracey.fi>
---
 Documentation/rev-list-options.txt |  3 +-
 revision.c                         | 69 ++++++++++++++++++++++++++++++++++++++
 t/t6111-rev-list-treesame.sh       |  4 +--
 3 files changed, 73 insertions(+), 3 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index d166384..f41e865 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 64b86ae..62f399c 100644
--- a/revision.c
+++ b/revision.c
@@ -2136,6 +2136,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;
@@ -2239,6 +2306,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 0047b54..484b69b 100755
--- a/t/t6111-rev-list-treesame.sh
+++ b/t/t6111-rev-list-treesame.sh
@@ -116,7 +116,7 @@ check_result 'M L H B A' -- file
 check_result '(LH)M (B)L (B)H (A)B A' --parents -- file
 check_result 'M L J I H G F D B A' --full-history -- file
 check_result '(LH)M (K)L (GJ)K (I)J (G)I (G)H (FB)G (D)F (BA)D (A)B A' --full-history --parents -- file
-check_outcome failure '(LH)M (G)H (J)L (I)J (G)I (FB)G (B)F (A)B A' --simplify-merges -- file # drops parent B from G
+check_result '(LH)M (G)H (J)L (I)J (G)I (FB)G (B)F (A)B A' --simplify-merges -- file
 check_result 'M L K G F D B A' --first-parent
 check_result 'M L G F B A' --first-parent -- file
 
@@ -127,7 +127,7 @@ check_result 'M L H' F..M -- file
 check_result '(LH)M (B)L (B)H' --parents F..M -- file
 check_result 'M L J I H G' F..M --full-history -- file
 check_result '(LH)M (K)L (GJ)K (I)J (G)I (G)H (FB)G' F..M --full-history --parents -- file
-check_outcome failure '(LH)M (G)H (J)L (I)J (G)I (FB)G' F..M --simplify-merges -- file # drops parent B from G
+check_result '(LH)M (G)H (J)L (I)J (G)I (FB)G' F..M --simplify-merges -- file
 check_result 'M L K J I H G' F..M --ancestry-path
 check_result 'M L J I H G' F..M --ancestry-path -- file
 check_result '(LH)M (K)L (GJ)K (I)J (G)I (G)H (FE)G' F..M --ancestry-path --parents -- file

-- 
1.8.3.rc0.28.g4b02ef5
Previous: Kevin BraceyNext: Kevin Bracey
Message 11 of 16 in “History traversal refinements”
  1. 00/15 History traversal refinementsKevin Bracey, May 16, 2013
  2. 01/15 decorate.c: compact table when growingKevin Bracey, May 16, 2013
  3. 02/15 t6019: test file dropped in -s ours mergeKevin Bracey, May 16, 2013
  4. 03/15 t6111: new TREESAME test setKevin Bracey, May 16, 2013
  5. 04/15 t6111: allow checking the parents as wellKevin Bracey, May 16, 2013
  6. 05/15 t6111: add parents to testsKevin Bracey, May 16, 2013
  7. 06/15 rev-list-options.txt: correct TREESAME for PKevin Bracey, May 16, 2013
  8. 07/15 Documentation: avoid "uninteresting"Kevin Bracey, May 16, 2013
  9. 08/15 revision.c: Make --full-history consider more mergesKevin Bracey, May 16, 2013
  10. 09/15 t6012: update test for tweaked full-history traversalKevin Bracey, May 16, 2013
  11. 10/15 simplify-merges: never remove all TREESAME parentsKevin Bracey, May 16, 2013
  12. 11/15 simplify-merges: drop merge from irrelevant side branchKevin Bracey, May 16, 2013
  13. 12/15 revision.c: add BOTTOM flag for commitsKevin Bracey, May 16, 2013
  14. 13/15 revision.c: discount side branches when computing TREESAMEKevin Bracey, May 16, 2013
  15. 14/15 revision.c: don't show all merges for --parentsKevin Bracey, May 16, 2013
  16. 15/15 revision.c: make default history consider bottom commitsKevin Bracey, May 16, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.