Skip to contentSkip to conversation

log: fix coloring of certain octupus merge shapes

PATCH In review Noam Postavsky 2 participants 4 messages Oct 10, 2018 – Oct 12, 2018

lore
Share & tools
lore

Noam Postavsky

author
#0 lore raw

PATCH v6 1/1 log: fix coloring of certain octupus merge shapes

For octopus merges where the first parent edge immediately merges into the next column to the left:

| *-.
| |\ \
|/ / /

then the number of columns should be one less than the usual case:

| *-.
| |\ \
| | | *

Also refactor the code to iterate over columns rather than dashes, building from an initial patch suggestion by Jeff King.

Signed-off-by Noam Postavsky <[email protected]>
Reviewed-by Jeff King <[email protected]>
---
 graph.c                      |  56 +++++++++++++++++-------
 t/t4214-log-graph-octopus.sh | 102 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+), 15 deletions(-)
 create mode 100755 t/t4214-log-graph-octopus.sh
graph.c +41 -15
index e1f6d3bddb..a3366f6dac 100644
@@ -842,27 +842,53 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 }
 
 /*
- * Draw an octopus merge and return the number of characters written.
+ * Draw the horizontal dashes of an octopus merge and return the number of
+ * characters written.
  */
 static int graph_draw_octopus_merge(struct git_graph *graph,
 				    struct strbuf *sb)
 {
 	/*
-	 * Here dashless_commits represents the number of parents
-	 * which don't need to have dashes (because their edges fit
-	 * neatly under the commit).
-	 */
-	const int dashless_commits = 2;
-	int col_num, i;
-	int num_dashes =
-		((graph->num_parents - dashless_commits) * 2) - 1;
-	for (i = 0; i < num_dashes; i++) {
-		col_num = (i / 2) + dashless_commits + graph->commit_index;
-		strbuf_write_column(sb, &graph->new_columns[col_num], '-');
+	 * Here dashless_parents represents the number of parents which don't
+	 * need to have dashes (the edges labeled "0" and "1").  And
+	 * dashful_parents are the remaining ones.
+	 *
+	 * | *---.
+	 * | |\ \ \
+	 * | | | | |
+	 * x 0 1 2 3
+	 *
+	 */
+	const int dashless_parents = 2;
+	int dashful_parents = graph->num_parents - dashless_parents;
+
+	/*
+	 * Usually, each parent gets its own column, like the diagram above, but
+	 * sometimes the first parent goes into an existing column, like this:
+	 *
+	 * | *---.
+	 * | |\ \ \
+	 * |/ / / /
+	 * x 0 1 2
+	 *
+	 * In which case there will be more parents than the delta of columns.
+	 */
+	int delta_cols = (graph->num_new_columns - graph->num_columns);
+	int parent_in_old_cols = graph->num_parents - delta_cols;
+
+	/*
+	 * In both cases, commit_index corresponds to the edge labeled "0".
+	 */
+	int first_col = graph->commit_index + dashless_parents
+	    - parent_in_old_cols;
+
+	int i;
+	for (i = 0; i < dashful_parents; i++) {
+		strbuf_write_column(sb, &graph->new_columns[i+first_col], '-');
+		strbuf_write_column(sb, &graph->new_columns[i+first_col],
+				    i == dashful_parents-1 ? '.' : '-');
 	}
-	col_num = (i / 2) + dashless_commits + graph->commit_index;
-	strbuf_write_column(sb, &graph->new_columns[col_num], '.');
-	return num_dashes + 1;
+	return 2 * dashful_parents;
 }
 
 static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
t/t4214-log-graph-octopus.sh new file +102 -0

102 lines not on this page · raw message

Signature · 1 line
2.11.0
#1 lore raw
Quoted 7 lines · Noam Postavsky

For octopus merges where the first parent edge immediately merges into the next column to the left:

| *-.
| |\ \
|/ / /
Quoted 5 lines

then the number of columns should be one less than the usual case:

| *-.
| |\ \
| | | *

I had a bit hard time parsing the above, especially with "then", which probably would make it easier to read if it is not there.

Quoted 2 lines

Also refactor the code to iterate over columns rather than dashes, building from an initial patch suggestion by Jeff King.

s/suggestion/suggested/ perhaps?

Quoted 4 lines
Signed-off-by Noam Postavsky <[email protected]>
Reviewed-by Jeff King <[email protected]>

---

Thanks, both.

graph.c static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) · line 854

Quoted 11 lines

/*
+ * Draw the horizontal dashes of an octopus merge and return the number of
+ * characters written.
*/
static int graph_draw_octopus_merge(struct git_graph *graph,

graph.c +3 -0
 				    struct strbuf *sb)
 {
 	/*
+	 * Here dashless_parents represents the number of parents which don't
+	 * need to have dashes (the edges labeled "0" and "1").  And
+	 * dashful_parents are the remaining ones.

Here "dash" refers to that horizontal line on the same line as the resulting merge. A very clearly explained definition. OK.

graph.c static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) · line 862

Quoted 7 lines
graph.c +7 -0
+	 * | *---.
+	 * | |\ \ \
+	 * | | | | |
+	 * x 0 1 2 3
+	 *
+	 */
+	const int dashless_parents = 2;

That counts parent #0 (the first parent) and parent #1.

graph.c static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) · line 863

Quoted 1 line
graph.c +1 -0
+	int dashful_parents = graph->num_parents - dashless_parents;

When a mistaken caller calls this function on a commit that is not an octopus, this can underflow. dashful_parents would be -1 for a non-merge, dashful_parents would be 0 for a normal merge, and then dashful_parents would be 1 for a merge of three histories. OK.

graph.c static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) · line 875

Quoted 11 lines
graph.c +11 -0
+	/*
+	 * Usually, each parent gets its own column, like the diagram above, but
+	 * sometimes the first parent goes into an existing column, like this:
+	 *
+	 * | *---.
+	 * | |\ \ \
+	 * |/ / / /
+	 * x 0 1 2
+	 *
+	 * In which case there will be more parents than the delta of columns.
+	 */

It is unclear to me what "delta of columns" means here. Is this because I am unfamiliar with the internal of graph.[ch] API (and 'delta of columns' is used elsewhere in the API internals already)?

graph.c static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) · line 876

Quoted 1 line
graph.c +1 -0
+	int delta_cols = (graph->num_new_columns - graph->num_columns);

So in the second picture above, new-columns (which is the columns used after showing the current line) is narrower (because 'x' reuses an already allocated column without getting a new one) than columns (which is the columns for the octopus merge we are showing)?

I am not sure I follow what is going on around here, sorry.

graph.c static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) · line 889

Quoted 12 lines
graph.c +12 -0
+	int parent_in_old_cols = graph->num_parents - delta_cols;
+	/*
+	 * In both cases, commit_index corresponds to the edge labeled "0".
+	 */
+	int first_col = graph->commit_index + dashless_parents
+	    - parent_in_old_cols;
+
+	int i;
+	for (i = 0; i < dashful_parents; i++) {
+		strbuf_write_column(sb, &graph->new_columns[i+first_col], '-');
+		strbuf_write_column(sb, &graph->new_columns[i+first_col],
+				    i == dashful_parents-1 ? '.' : '-');

Draw a dash-dash for each, except we show dash-dot only for the last one. OK. It is interesting that dashful_parents does not have to change between the two examples you gave above, and it is understandable because it only depends on the shape of the graph near the octopus merge itself (in other words, the placement of the parent commits does not contribute to it at all). Makes sense.

graph.c static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) · line 891

Quoted 5 lines
graph.c +1 -3
 	}
-	col_num = (i / 2) + dashless_commits + graph->commit_index;
-	strbuf_write_column(sb, &graph->new_columns[col_num], '.');
-	return num_dashes + 1;
+	return 2 * dashful_parents;

This is natural, as we showed either dash-dash or dash-dot only for dashful_parents after the merge itself. OK.

Thanks, will queue.

author Reviewed-by Jeff King
#2 lore raw
Quoted 2 lines · Junio C Hamano

I had a bit hard time parsing the above, especially with "then", which probably would make it easier to read if it is not there.

Okay, I guess better to separate the explanation from the diagrams, rather than weaving them together:

    For octopus merges where the first parent edge immediately merges into
    the next column to the left, the number of columns should be one less
    than the usual case.

    First parent to the left case:

    | *-.
    | |\ \
    |/ / /

    The usual case:

    | *-.
    | |\ \
    | | | *
Quoted 4 lines
Quoted 2 lines

Also refactor the code to iterate over columns rather than dashes, building from an initial patch suggestion by Jeff King.

s/suggestion/suggested/ perhaps?

Ok.

Quoted 3 lines

It is unclear to me what "delta of columns" means here. Is this because I am unfamiliar with the internal of graph.[ch] API (and 'delta of columns' is used elsewhere in the API internals already)?

No, I just meant difference in number of columns from the previous line to the next. Actually, I had kind of wanted to use "new columns", but that would be confusing with the num_new_columns variable actually meaning the total number of columns in the next line.

Quoted 8 lines
Quoted 1 line

+ int delta_cols = (graph->num_new_columns - graph->num_columns);

So in the second picture above, new-columns (which is the columns used after showing the current line) is narrower (because 'x' reuses an already allocated column without getting a new one) than columns (which is the columns for the octopus merge we are showing)?

I am not sure I follow what is going on around here, sorry.

Maybe it's clearer by saying "added columns" (also expanded the comments a bit)?

/*
* Usually, we add one new column for each parent (like the diagram
* above) but sometimes the first parent goes into an existing column,
* like this:
*
* | *---.
* | |\ \ \
* |/ / / /
* x 0 1 2
*
* In which case the number of parents will be one greater than the
* number of added columns.
*/
int added_cols = (graph->num_new_columns - graph->num_columns); int parent_in_old_cols = graph->num_parents - added_cols;

From a9c90605c062b30273dad35adbf319905028cacc Mon Sep 17 00:00:00 2001
From: Noam Postavsky <[email protected]>
Date: Sat, 1 Sep 2018 20:07:16 -0400
Subject: [PATCH v7] log: fix coloring of certain octupus merge shapes

For octopus merges where the first parent edge immediately merges into the next column to the left, the number of columns should be one less than the usual case.

First parent to the left case:

| *-.
| |\ \
|/ / /

The usual case:

| *-.
| |\ \
| | | *

Also refactor the code to iterate over columns rather than dashes, building from an initial patch suggested by Jeff King.

Signed-off-by Noam Postavsky <[email protected]>
Reviewed-by Jeff King <[email protected]>
---
 graph.c                      |  58 +++++++++++++++++-------
 t/t4214-log-graph-octopus.sh | 102 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 145 insertions(+), 15 deletions(-)
 create mode 100755 t/t4214-log-graph-octopus.sh
graph.c +43 -15
index e1f6d3bddb..f53135485f 100644
@@ -842,27 +842,55 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 }
 
 /*
- * Draw an octopus merge and return the number of characters written.
+ * Draw the horizontal dashes of an octopus merge and return the number of
+ * characters written.
  */
 static int graph_draw_octopus_merge(struct git_graph *graph,
 				    struct strbuf *sb)
 {
 	/*
-	 * Here dashless_commits represents the number of parents
-	 * which don't need to have dashes (because their edges fit
-	 * neatly under the commit).
-	 */
-	const int dashless_commits = 2;
-	int col_num, i;
-	int num_dashes =
-		((graph->num_parents - dashless_commits) * 2) - 1;
-	for (i = 0; i < num_dashes; i++) {
-		col_num = (i / 2) + dashless_commits + graph->commit_index;
-		strbuf_write_column(sb, &graph->new_columns[col_num], '-');
+	 * Here dashless_parents represents the number of parents which don't
+	 * need to have dashes (the edges labeled "0" and "1").  And
+	 * dashful_parents are the remaining ones.
+	 *
+	 * | *---.
+	 * | |\ \ \
+	 * | | | | |
+	 * x 0 1 2 3
+	 *
+	 */
+	const int dashless_parents = 2;
+	int dashful_parents = graph->num_parents - dashless_parents;
+
+	/*
+	 * Usually, we add one new column for each parent (like the diagram
+	 * above) but sometimes the first parent goes into an existing column,
+	 * like this:
+	 *
+	 * | *---.
+	 * | |\ \ \
+	 * |/ / / /
+	 * x 0 1 2
+	 *
+	 * In which case the number of parents will be one greater than the
+	 * number of added columns.
+	 */
+	int added_cols = (graph->num_new_columns - graph->num_columns);
+	int parent_in_old_cols = graph->num_parents - added_cols;
+
+	/*
+	 * In both cases, commit_index corresponds to the edge labeled "0".
+	 */
+	int first_col = graph->commit_index + dashless_parents
+	    - parent_in_old_cols;
+
+	int i;
+	for (i = 0; i < dashful_parents; i++) {
+		strbuf_write_column(sb, &graph->new_columns[i+first_col], '-');
+		strbuf_write_column(sb, &graph->new_columns[i+first_col],
+				    i == dashful_parents-1 ? '.' : '-');
 	}
-	col_num = (i / 2) + dashless_commits + graph->commit_index;
-	strbuf_write_column(sb, &graph->new_columns[col_num], '.');
-	return num_dashes + 1;
+	return 2 * dashful_parents;
 }
 
 static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
t/t4214-log-graph-octopus.sh new file +102 -0

102 lines not on this page · raw message

Signature · 1 line
2.11.0
#3 lore raw

I'll do the s/octu/octo/; again on the title while queuing.

Let's merge this to 'next'.

Thanks.