threads / patch / 30876

v2, 3 parts"log --first-parent --simplify-merges/by-decoration"

Subject: [PATCH v2 0/3] "log --first-parent --simplify-merges/by-decoration"

## tl;dr

4 messages between Jun 22, 2012 and Jun 22, 2012. Diffs are folded; open one to read it.

replies: 3people: 1as markdown or json

Junio C Hamano· Jun 22, 2012, 22:27 UTC · lore

It is unclear what it means to "simplify-merges" while traversing only the "first-parent" ancestry chain, but the combination of the options makes the simplification logic to use in-core commit objects that haven't been examined for relevance, either producing incorrect result or taking too long to produce any output.

Teach the simplification logic to ignore commits that the first-parent traversal logic ignored when both are in effect to work around the issue.

Junio C Hamano (3):
  revision: "simplify" options imply topo-order sort
  revision: note the lack of free() in simplify_merges()
  revision: ignore side parents while running simplify-merges

 revision.c | 41 +++++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 12 deletions(-)

-- 
1.7.11.1.29.gf71be5c

Junio C Hamano· Jun 22, 2012, 22:27 UTC · re: Junio C Hamano · lore

[PATCH v2 1/3] revision: "simplify" options imply topo-order sort

The code internally runs sort_in_topo_order() already; it is more clear
to spell it out in the option parsing phase, instead of adding a special
case in simplify_merges() function.
---
 revision.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Show changes to revision.c +2 −2

diff --git a/revision.c b/revision.c
index 935e7a7..00aaefe 100644
--- a/revision.c
+++ b/revision.c
@@ -1358,11 +1358,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->topo_order = 1;
 	} else if (!strcmp(arg, "--simplify-merges")) {
 		revs->simplify_merges = 1;
+		revs->topo_order = 1;
 		revs->rewrite_parents = 1;
 		revs->simplify_history = 0;
 		revs->limited = 1;
 	} else if (!strcmp(arg, "--simplify-by-decoration")) {
 		revs->simplify_merges = 1;
+		revs->topo_order = 1;
 		revs->rewrite_parents = 1;
 		revs->simplify_history = 0;
 		revs->simplify_by_decoration = 1;
@@ -2016,8 +2018,6 @@ static void simplify_merges(struct rev_info *revs)
 	struct commit_list *list;
 	struct commit_list *yet_to_do, **tail;
 
-	if (!revs->topo_order)
-		sort_in_topological_order(&revs->commits, revs->lifo);
 	if (!revs->prune)
 		return;

-- 
1.7.11.1.29.gf71be5c

Junio C Hamano· Jun 22, 2012, 22:27 UTC · re: Junio C Hamano · lore

[PATCH v2 2/3] revision: note the lack of free() in simplify_merges()

Among the three similar-looking loops that walk singly linked commit_list, the first one is only peeking and the same list is later used for real work. Leave a comment not to mistakenly free its elements there.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

Show changes to revision.c +16 −7

diff --git a/revision.c b/revision.c
index 00aaefe..814b96f 100644
--- a/revision.c
+++ b/revision.c
@@ -2015,23 +2015,31 @@ static struct commit_list **simplify_one(struct rev_info *revs, struct commit *c
 
 static void simplify_merges(struct rev_info *revs)
 {
-	struct commit_list *list;
+	struct commit_list *list, *next;
 	struct commit_list *yet_to_do, **tail;
+	struct commit *commit;
 
 	if (!revs->prune)
 		return;
 
 	/* feed the list reversed */
 	yet_to_do = NULL;
-	for (list = revs->commits; list; list = list->next)
-		commit_list_insert(list->item, &yet_to_do);
+	for (list = revs->commits; list; list = next) {
+		commit = list->item;
+		next = list->next;
+		/*
+		 * Do not free(list) here yet; the original list
+		 * is used later in this function.
+		 */
+		commit_list_insert(commit, &yet_to_do);
+	}
 	while (yet_to_do) {
 		list = yet_to_do;
 		yet_to_do = NULL;
 		tail = &yet_to_do;
 		while (list) {
-			struct commit *commit = list->item;
-			struct commit_list *next = list->next;
+			commit = list->item;
+			next = list->next;
 			free(list);
 			list = next;
 			tail = simplify_one(revs, commit, tail);
@@ -2043,9 +2051,10 @@ static void simplify_merges(struct rev_info *revs)
 	revs->commits = NULL;
 	tail = &revs->commits;
 	while (list) {
-		struct commit *commit = list->item;
-		struct commit_list *next = list->next;
 		struct merge_simplify_state *st;
+
+		commit = list->item;
+		next = list->next;
 		free(list);
 		list = next;
 		st = locate_simplify_state(revs, commit);

-- 
1.7.11.1.29.gf71be5c

Junio C Hamano· Jun 22, 2012, 22:27 UTC · re: Junio C Hamano · lore

[PATCH v2 3/3] revision: ignore side parents while running simplify-merges

The simplify_merges() function needs to look at all history chain to find the closest ancestor that is relevant after the simplification, but after --first-parent traversal, side parents haven't been marked for relevance (they are irrelevant by definition due to the nature of first-parent-only traversal) nor culled from the parents list of resulting commits.

We cannot simply remove these side parents from the parents list, as the output phase still wants to see the parents. Instead, teach simplify_one() and its callees to ignore the later parents.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 revision.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Show changes to revision.c +11 −3

diff --git a/revision.c b/revision.c
index 814b96f..d1a4ef5 100644
--- a/revision.c
+++ b/revision.c
@@ -1949,8 +1949,9 @@ static struct commit_list **simplify_one(struct rev_info *revs, struct commit *c
 	}
 
 	/*
-	 * Do we know what commit all of our parents should be rewritten to?
-	 * Otherwise we are not ready to rewrite this one yet.
+	 * Do we know what commit all of our parents that matter
+	 * should be rewritten to?  Otherwise we are not ready to
+	 * rewrite this one yet.
 	 */
 	for (cnt = 0, p = commit->parents; p; p = p->next) {
 		pst = locate_simplify_state(revs, p->item);
@@ -1958,6 +1959,8 @@ static struct commit_list **simplify_one(struct rev_info *revs, struct commit *c
 			tail = &commit_list_insert(p->item, tail)->next;
 			cnt++;
 		}
+		if (revs->first_parent_only)
+			break;
 	}
 	if (cnt) {
 		tail = &commit_list_insert(commit, tail)->next;
@@ -1970,8 +1973,13 @@ static struct commit_list **simplify_one(struct rev_info *revs, struct commit *c
 	for (p = commit->parents; p; p = p->next) {
 		pst = locate_simplify_state(revs, p->item);
 		p->item = pst->simplified;
+		if (revs->first_parent_only)
+			break;
 	}
-	cnt = remove_duplicate_parents(commit);
+	if (!revs->first_parent_only)
+		cnt = remove_duplicate_parents(commit);
+	else
+		cnt = 1;
 
 	/*
 	 * It is possible that we are a merge and one side branch

-- 
1.7.11.1.29.gf71be5c