threads / patch / 30751

patch, 3 partsrevision: "simplify" options imply topo-order sort

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

## tl;dr

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

replies: 3people: 1as markdown or json

Junio C Hamano· Jun 8, 2012, 22:53 UTC · lore

[PATCH 0/3] rev-list --simplify-{merges,by-decoration} --first-parent

The deeper history simplification using merge information (not to be confused with "merge simplification") was not prepared to be used with the --first-parent option, but it was possible to trigger this combination from gitk.

The fix is fairly straight-forward. When showing only first-parent chain of history, the later parents won't matter in the result, so we can just rewrite the parents and make the history a single strand of pearls.

Showing first-parent-only history and then attempting to simplify merges does not make much sense, but simplify-by-decoration may be a valid thing to ask.

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

 revision.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

-- 
1.7.11.rc2.29.g85b30f3

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

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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.rc2.29.g85b30f3

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

[PATCH 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.rc2.29.g85b30f3

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

[PATCH 3/3] revision: cull side parents before 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.

Remove these side parents from parents list before starting to further simplifying the result.

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

Show changes to revision.c +3 −0

diff --git a/revision.c b/revision.c
index 814b96f..d93eb7c 100644
--- a/revision.c
+++ b/revision.c
@@ -2031,6 +2031,9 @@ static void simplify_merges(struct rev_info *revs)
 		 * Do not free(list) here yet; the original list
 		 * is used later in this function.
 		 */
+		if (revs->first_parent_only &&
+		    commit->parents && commit->parents->next)
+			commit->parents->next = NULL;
 		commit_list_insert(commit, &yet_to_do);
 	}
 	while (yet_to_do) {

-- 
1.7.11.rc2.29.g85b30f3