threads / discuss / 13701

log --graph: extra space with --pretty=oneline

Subject: log --graph: extra space with --pretty=oneline

## tl;dr

12 messages between May 28, 2008 and Jun 2, 2008.

replies: 11people: 4as markdown or json

Teemu Likonen· May 28, 2008, 11:24 UTC · lore

Sometimes "log --graph --pretty=oneline" prints a sort of broken graph line. In the git repository try this:

$ git log --graph --pretty=oneline --abbrev-commit -4 8366b7b
M   8366b7b... Merge branch 'maint'
|\  
| M   a2f5be5... Merge branch 'jk/maint-send-email-compose' into maint
| |\  
| M  \  93c7b9c... Merge branch 'hb/maint-send-email-quote-recipients' into maint
| |\  | 
| M  \ \  6abf189... Merge branch 'maint-1.5.4' into maint
| |\  | |
    ^

Extra spaces there. I don't mind that myself but to some users it may look like a bug. Maybe one would expect output like this:

M   8366b7b... Merge branch 'maint'
|\  
| M   a2f5be5... Merge branch 'jk/maint-send-email-compose' into maint
| |\  
| | \
| M  \  93c7b9c... Merge branch 'hb/maint-send-email-quote-recipients' into maint
| |\  \ 
| | \  \
| M  \  |  6abf189... Merge branch 'maint-1.5.4' into maint
| |\  | |
It requires more lines though.
Wincent Colaiuta· May 28, 2008, 11:34 UTC · re: Teemu Likonen · lore

Re: log --graph: extra space with --pretty=oneline

El 28/5/2008, a las 13:24, Teemu Likonen escribió:
Show 35 quoted lines
> Sometimes "log --graph --pretty=oneline" prints a sort of broken graph
> line. In the git repository try this:
>
> $ git log --graph --pretty=oneline --abbrev-commit -4 8366b7b
>
>
> M   8366b7b... Merge branch 'maint'
> |\
> | M   a2f5be5... Merge branch 'jk/maint-send-email-compose' into maint
> | |\
> | M  \  93c7b9c... Merge branch 'hb/maint-send-email-quote- 
> recipients' into maint
> | |\  |
> | M  \ \  6abf189... Merge branch 'maint-1.5.4' into maint
> | |\  | |
>    ^
>
> Extra spaces there. I don't mind that myself but to some users it may
> look like a bug. Maybe one would expect output like this:
>
>
> M   8366b7b... Merge branch 'maint'
> |\
> | M   a2f5be5... Merge branch 'jk/maint-send-email-compose' into maint
> | |\
> | | \
> | M  \  93c7b9c... Merge branch 'hb/maint-send-email-quote- 
> recipients' into maint
> | |\  \
> | | \  \
> | M  \  |  6abf189... Merge branch 'maint-1.5.4' into maint
> | |\  | |
>
>
> It requires more lines though.
Yes it does, but it definitely looks more readable to me.
W
Adam Simpkins· May 29, 2008, 08:57 UTC · re: Teemu Likonen · lore

Re: log --graph: extra space with --pretty=oneline

On Wed, May 28, 2008 at 02:24:05PM +0300, Teemu Likonen wrote:
Show 23 quoted lines
> Sometimes "log --graph --pretty=oneline" prints a sort of broken graph
> line. In the git repository try this:
> 
> $ git log --graph --pretty=oneline --abbrev-commit -4 8366b7b
> 
> [current graph output removed]
> 
> Extra spaces there. I don't mind that myself but to some users it may
> look like a bug. Maybe one would expect output like this:
> 
> 
> M   8366b7b... Merge branch 'maint'
> |\  
> | M   a2f5be5... Merge branch 'jk/maint-send-email-compose' into maint
> | |\  
> | | \
> | M  \  93c7b9c... Merge branch 'hb/maint-send-email-quote-recipients' into maint
> | |\  \ 
> | | \  \
> | M  \  |  6abf189... Merge branch 'maint-1.5.4' into maint
> | |\  | |
> 
> It requires more lines though.

Hmm. Yes, the output could definitely be improved. This problem occurs for merges with 2 parents (not octopus merges), and only when the branch line to the right of the merge was moving right at the end of the previous commit (i.e., it was displayed with '\' instead of '|' or '/').

Note that this doesn't require extra lines to fix:
M   8366b7b... Merge branch 'maint'
|\  
| M   a2f5be5... Merge branch 'jk/maint-send-email-compose' into maint
| |\  
| M \  93c7b9c... Merge branch 'hb/maint-send-email-quote-recipients' into maint
| |\ \ 
| M \ \  6abf189... Merge branch 'maint-1.5.4' into maint
| |\ \ \

This can easily be implemented by changing the output for 2-way merges from something like this:

| M  \
| |\  |
to this:
| M \
| |\ \

However, I find this makes the graph slightly uglier when the incoming branch to the right of the merge wasn't '\' on the previous line. The following change seems to look better when the branch line was '|' or '/' on the previous line:

| M |
| |\ \

For comparison, here's a comparison of several scenarios of how the output looks now, and how it would look with these fixes.

Current behavior        Option 1                 Option 2
| |\                    | |\                     | |\
| M  \                  | M \                    | M |
| |\  |                 | |\ \                   | |\ \
| | |                   | | |                    | | |
| M  \                  | M \                    | M |
| |\  |                 | |\ \                   | |\ \
| |  /                  | |  /                   | |  /
| M  \                  | M \                    | M |
| |\  |                 | |\ \                   | |\ \
| | |/                  | | |/                   | | |/
| M  \                  | M \                    | M |
| |\  |                 | |\ \                   | |\ \

Note that in the case of octopus merges, the current code already produces output like that of option 1.

However, I find that both options 1 and 2 look a little uglier than the current behavior when the branch lines need to be collapsed again after the merge. The graph has more pointy angles, but it is still readable in all cases:

Current behavior        Option 1                 Option 2
| * | |                 | * | |                  | * | |
| M  \ \                | M \ \                  | M | |
| |\  | |               | |\ \ \                 | |\ \ \
|/ / / /                |/ / / /                 |/ / / /

For sections of the graph where there are several merge commits in a row, I think option 1 looks the best:

Current behavior        Option 1                 Option 2
* |                     * |                      * |
M  \                    M \                      M |
|\  |                   |\ \                     |\ \
M  \ \                  M \ \                    M | |
|\  | |                 |\ \ \                   |\ \ \
M  \ \ \                M \ \ \                  M | | |
|\  | | |               |\ \ \ \                 |\ \ \ \

What is everyone's preference between the 3 options? Personally, I'm leaning towards Option 2.

I'll send out some informal patches of both option 1 and option 2, for comparison.

In the future, the code could even be improved to dynamically choose between these three options based on the output printed for the previous commit. Currently it doesn't store enough information from the previous commit to do this.

-- 
Adam Simpkins
adam@adamsimpkins.net
Adam Simpkins· May 29, 2008, 09:03 UTC · re: Adam Simpkins · lore

[PATCH] graph API: improve output for merge commits (option 1)

This eliminates the extra space that sometimes appeared in branch lines to the right of 2-way merge commits. (It appeared when the branch line was displayed as '\' on the line just before the merge commit.)

For example,
| |\
| M  \
| |\  |
is now displayed as
| |\
| M \
| |\ \
Signed-off-by: Adam Simpkins <adam@adamsimpkins.net>
---
 graph.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/graph.c b/graph.c
index 26b8c52..c3babcb 100644
--- a/graph.c
+++ b/graph.c
@@ -625,10 +625,8 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 			seen_this = 1;
 			graph_output_commit_char(graph, sb);
 
-			if (graph->num_parents < 2)
+			if (graph->num_parents < 3)
 				strbuf_addch(sb, ' ');
-			else if (graph->num_parents == 2)
-				strbuf_addstr(sb, "  ");
 			else {
 				int num_dashes =
 					((graph->num_parents - 2) * 2) - 1;
@@ -679,9 +677,7 @@ void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
 			strbuf_addch(sb, '|');
 			for (j = 0; j < graph->num_parents - 1; j++)
 				strbuf_addstr(sb, "\\ ");
-			if (graph->num_parents == 2)
-				strbuf_addch(sb, ' ');
-		} else if (seen_this && (graph->num_parents > 2)) {
+		} else if (seen_this) {
 			strbuf_addstr(sb, "\\ ");
 		} else {
 			strbuf_addstr(sb, "| ");
-- 
1.5.6.rc0.46.gd2b3.dirty
Adam Simpkins· May 29, 2008, 09:04 UTC · re: Adam Simpkins · lore

[PATCH] graph API: improve output for merge commits (option 2)

This eliminates the extra space that sometimes appeared in branch lines to the right of 2-way merge commits. (It appeared when the branch line was displayed as '\' on the line just before the merge commit.)

For example,
| |\
| M  \
| |\  |
is now displayed as
| |\
| M |
| |\ \

The output for octopus merges was also updated to be more similar to that for 2-way merges.

Signed-off-by: Adam Simpkins <adam@adamsimpkins.net>
---
 graph.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/graph.c b/graph.c
index 26b8c52..92f5b1a 100644
--- a/graph.c
+++ b/graph.c
@@ -535,7 +535,7 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
 		if (col->commit == graph->commit) {
 			seen_this = 1;
 			strbuf_addf(sb, "| %*s", graph->expansion_row, "");
-		} else if (seen_this) {
+		} else if (seen_this && (graph->expansion_row > 0)) {
 			strbuf_addstr(sb, "\\ ");
 		} else {
 			strbuf_addstr(sb, "| ");
@@ -625,10 +625,8 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 			seen_this = 1;
 			graph_output_commit_char(graph, sb);
 
-			if (graph->num_parents < 2)
+			if (graph->num_parents < 3)
 				strbuf_addch(sb, ' ');
-			else if (graph->num_parents == 2)
-				strbuf_addstr(sb, "  ");
 			else {
 				int num_dashes =
 					((graph->num_parents - 2) * 2) - 1;
@@ -636,7 +634,7 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 					strbuf_addch(sb, '-');
 				strbuf_addstr(sb, ". ");
 			}
-		} else if (seen_this && (graph->num_parents > 1)) {
+		} else if (seen_this && (graph->num_parents > 2)) {
 			strbuf_addstr(sb, "\\ ");
 		} else {
 			strbuf_addstr(sb, "| ");
@@ -679,9 +677,7 @@ void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
 			strbuf_addch(sb, '|');
 			for (j = 0; j < graph->num_parents - 1; j++)
 				strbuf_addstr(sb, "\\ ");
-			if (graph->num_parents == 2)
-				strbuf_addch(sb, ' ');
-		} else if (seen_this && (graph->num_parents > 2)) {
+		} else if (seen_this) {
 			strbuf_addstr(sb, "\\ ");
 		} else {
 			strbuf_addstr(sb, "| ");
-- 
1.5.6.rc0.46.gd2b3.dirty
Teemu Likonen· May 29, 2008, 10:25 UTC · re: Adam Simpkins · lore

Re: log --graph: extra space with --pretty=oneline

Adam Simpkins wrote (2008-05-29 01:57 -0700):
> What is everyone's preference between the 3 options?  Personally, I'm
> leaning towards Option 2.

I prefer the option 2 too. To me it never looks too ugly whereas the other two have some broken line or buggy-ish cases.

Damn, this log --graph is so nice feature. I like it more than gitk, regardless of the fact that I have two monitors and could easily have gitk open and visible all the time.

Adam Simpkins· Jun 1, 2008, 20:56 UTC · re: Adam Simpkins · lore

[PATCH 0/2] graph API: improve printing of merges

This is two minor patches to improve the display of merge commits in the graph output. The first fixes the "extra space" that appears sometimes, as pointed out by Teemu Likonen. I had previously posted 2 simple options for fixing the problem, but neither one was best in all cases. This patch is an improved version that dynamically chooses how the merge commit should be displayed, based on the last line of the previous commit's output.

For example, with the new changes, the code now prints:
$ git log --graph --pretty=format:%h -10 8d6afc1
M   8d6afc1
|\  
| M   f2fea68
| |\  
| M \   21dbe12
| |\ \  
M | \ \   41094b8
|\ \ \ \  
| M \ \ \   061ad5f
| |\ \ \ \  
| M \ \ \ \   fe041ad
| |\ \ \ \ \  
| | \ \ \ \ \     
| |  \ \ \ \ \    
| M-. \ \ \ \ \   cd1333d
| |\ \ \ \ \ \ \  
| | * | | | | | | cfcbd34
| | * | | | | | | 5398fed
| M | | | | | | |   539d84f
| |\ \ \ \ \ \ \ \  

The second patch improves the output for octopus merges, by avoiding printing unnecessary padding lines before the commit when there aren't any existing branch lines to the right of the merge.

Adam Simpkins (2):
  graph API: improve display of merge commits
  graph API: avoid printing unnecessary padding before some octopus
    merges
 graph.c |  123 +++++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 101 insertions(+), 22 deletions(-)
Adam Simpkins· Jun 1, 2008, 20:56 UTC · re: Adam Simpkins · lore

[PATCH 1/2] graph API: improve display of merge commits

This change improves the way merge commits are displayed, to eliminate a few visual artifacts. Previously, merge commits were displayed as:

| M  \
| |\  |

As pointed out by Teemu Likonen, this didn't look nice if the rightmost branch line was displayed as '\' on the previous line, as it then appeared to have an extra space in it:

| |\
| M  \
| |\  |

This change updates the code so that branch lines to the right of merge commits are printed slightly differently depending on how the previous line was displayed:

| |\          | | |        | |  /
| M \         | M |        | M |
| |\ \        | |\ \       | |\ \
Signed-off-by: Adam Simpkins <adam@adamsimpkins.net>
---
 graph.c |  110 +++++++++++++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 93 insertions(+), 17 deletions(-)
diff --git a/graph.c b/graph.c
index 26b8c52..332d1e8 100644
--- a/graph.c
+++ b/graph.c
@@ -81,6 +81,27 @@ struct git_graph {
 	 */
 	enum graph_state state;
 	/*
+	 * The output state for the previous line of output.
+	 * This is primarily used to determine how the first merge line
+	 * should appear, based on the last line of the previous commit.
+	 */
+	enum graph_state prev_state;
+	/*
+	 * The index of the column that refers to this commit.
+	 *
+	 * If none of the incoming columns refer to this commit,
+	 * this will be equal to num_columns.
+	 */
+	int commit_index;
+	/*
+	 * The commit_index for the previously displayed commit.
+	 *
+	 * This is used to determine how the first line of a merge
+	 * graph output should appear, based on the last line of the
+	 * previous commit.
+	 */
+	int prev_commit_index;
+	/*
 	 * The maximum number of columns that can be stored in the columns
 	 * and new_columns arrays.  This is also half the number of entries
 	 * that can be stored in the mapping and new_mapping arrays.
@@ -137,6 +158,9 @@ struct git_graph *graph_init(struct rev_info *opt)
 	graph->num_parents = 0;
 	graph->expansion_row = 0;
 	graph->state = GRAPH_PADDING;
+	graph->prev_state = GRAPH_PADDING;
+	graph->commit_index = 0;
+	graph->prev_commit_index = 0;
 	graph->num_columns = 0;
 	graph->num_new_columns = 0;
 	graph->mapping_size = 0;
@@ -164,6 +188,12 @@ void graph_release(struct git_graph *graph)
 	free(graph);
 }
 
+static void graph_update_state(struct git_graph *graph, enum graph_state s)
+{
+	graph->prev_state = graph->state;
+	graph->state = s;
+}
+
 static void graph_ensure_capacity(struct git_graph *graph, int num_columns)
 {
 	if (graph->column_capacity >= num_columns)
@@ -342,6 +372,7 @@ static void graph_update_columns(struct git_graph *graph)
 		if (col_commit == graph->commit) {
 			int old_mapping_idx = mapping_idx;
 			seen_this = 1;
+			graph->commit_index = i;
 			for (parent = graph->commit->parents;
 			     parent;
 			     parent = parent->next) {
@@ -395,6 +426,13 @@ void graph_update(struct git_graph *graph, struct commit *commit)
 	}
 
 	/*
+	 * Store the old commit_index in prev_commit_index.
+	 * graph_update_columns() will update graph->commit_index for this
+	 * commit.
+	 */
+	graph->prev_commit_index = graph->commit_index;
+
+	/*
 	 * Call graph_update_columns() to update
 	 * columns, new_columns, and mapping.
 	 */
@@ -404,6 +442,9 @@ void graph_update(struct git_graph *graph, struct commit *commit)
 
 	/*
 	 * Update graph->state.
+	 * Note that we don't call graph_update_state() here, since
+	 * we don't want to update graph->prev_state.  No line for
+	 * graph->state was ever printed.
 	 *
 	 * If the previous commit didn't get to the GRAPH_PADDING state,
 	 * it never finished its output.  Goto GRAPH_SKIP, to print out
@@ -498,9 +539,9 @@ static void graph_output_skip_line(struct git_graph *graph, struct strbuf *sb)
 	graph_pad_horizontally(graph, sb);
 
 	if (graph->num_parents >= 3)
-		graph->state = GRAPH_PRE_COMMIT;
+		graph_update_state(graph, GRAPH_PRE_COMMIT);
 	else
-		graph->state = GRAPH_COMMIT;
+		graph_update_state(graph, GRAPH_COMMIT);
 }
 
 static void graph_output_pre_commit_line(struct git_graph *graph,
@@ -535,7 +576,22 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
 		if (col->commit == graph->commit) {
 			seen_this = 1;
 			strbuf_addf(sb, "| %*s", graph->expansion_row, "");
-		} else if (seen_this) {
+		} else if (seen_this && (graph->expansion_row == 0)) {
+			/*
+			 * This is the first line of the pre-commit output.
+			 * If the previous commit was a merge commit and
+			 * ended in the GRAPH_POST_MERGE state, all branch
+			 * lines after graph->prev_commit_index were
+			 * printed as "\" on the previous line.  Continue
+			 * to print them as "\" on this line.  Otherwise,
+			 * print the branch lines as "|".
+			 */
+			if (graph->prev_state == GRAPH_POST_MERGE &&
+			    graph->prev_commit_index < i)
+				strbuf_addstr(sb, "\\ ");
+			else
+				strbuf_addstr(sb, "| ");
+		} else if (seen_this && (graph->expansion_row > 0)) {
 			strbuf_addstr(sb, "\\ ");
 		} else {
 			strbuf_addstr(sb, "| ");
@@ -550,7 +606,7 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
 	 */
 	graph->expansion_row++;
 	if (graph->expansion_row >= num_expansion_rows)
-		graph->state = GRAPH_COMMIT;
+		graph_update_state(graph, GRAPH_COMMIT);
 }
 
 static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
@@ -625,10 +681,8 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 			seen_this = 1;
 			graph_output_commit_char(graph, sb);
 
-			if (graph->num_parents < 2)
+			if (graph->num_parents < 3)
 				strbuf_addch(sb, ' ');
-			else if (graph->num_parents == 2)
-				strbuf_addstr(sb, "  ");
 			else {
 				int num_dashes =
 					((graph->num_parents - 2) * 2) - 1;
@@ -636,8 +690,27 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 					strbuf_addch(sb, '-');
 				strbuf_addstr(sb, ". ");
 			}
-		} else if (seen_this && (graph->num_parents > 1)) {
+		} else if (seen_this && (graph->num_parents > 2)) {
 			strbuf_addstr(sb, "\\ ");
+		} else if (seen_this && (graph->num_parents == 2)) {
+			/*
+			 * This is a 2-way merge commit.
+			 * There is no GRAPH_PRE_COMMIT stage for 2-way
+			 * merges, so this is the first line of output
+			 * for this commit.  Check to see what the previous
+			 * line of output was.
+			 *
+			 * If it was GRAPH_POST_MERGE, the branch line
+			 * coming into this commit may have been '\',
+			 * and not '|' or '/'.  If so, output the branch
+			 * line as '\' on this line, instead of '|'.  This
+			 * makes the output look nicer.
+			 */
+			if (graph->prev_state == GRAPH_POST_MERGE &&
+			    graph->prev_commit_index < i)
+				strbuf_addstr(sb, "\\ ");
+			else
+				strbuf_addstr(sb, "| ");
 		} else {
 			strbuf_addstr(sb, "| ");
 		}
@@ -649,11 +722,11 @@ void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 	 * Update graph->state
 	 */
 	if (graph->num_parents > 1)
-		graph->state = GRAPH_POST_MERGE;
+		graph_update_state(graph, GRAPH_POST_MERGE);
 	else if (graph_is_mapping_correct(graph))
-		graph->state = GRAPH_PADDING;
+		graph_update_state(graph, GRAPH_PADDING);
 	else
-		graph->state = GRAPH_COLLAPSING;
+		graph_update_state(graph, GRAPH_COLLAPSING);
 }
 
 void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
@@ -679,9 +752,7 @@ void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
 			strbuf_addch(sb, '|');
 			for (j = 0; j < graph->num_parents - 1; j++)
 				strbuf_addstr(sb, "\\ ");
-			if (graph->num_parents == 2)
-				strbuf_addch(sb, ' ');
-		} else if (seen_this && (graph->num_parents > 2)) {
+		} else if (seen_this) {
 			strbuf_addstr(sb, "\\ ");
 		} else {
 			strbuf_addstr(sb, "| ");
@@ -694,9 +765,9 @@ void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
 	 * Update graph->state
 	 */
 	if (graph_is_mapping_correct(graph))
-		graph->state = GRAPH_PADDING;
+		graph_update_state(graph, GRAPH_PADDING);
 	else
-		graph->state = GRAPH_COLLAPSING;
+		graph_update_state(graph, GRAPH_COLLAPSING);
 }
 
 void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)
@@ -801,7 +872,7 @@ void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)
 	 * Otherwise, we need to collapse some branch lines together.
 	 */
 	if (graph_is_mapping_correct(graph))
-		graph->state = GRAPH_PADDING;
+		graph_update_state(graph, GRAPH_PADDING);
 }
 
 int graph_next_line(struct git_graph *graph, struct strbuf *sb)
@@ -865,6 +936,11 @@ void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
 	}
 
 	graph_pad_horizontally(graph, sb);
+
+	/*
+	 * Update graph->prev_state since we have output a padding line
+	 */
+	graph->prev_state = GRAPH_PADDING;
 }
 
 int graph_is_commit_finished(struct git_graph const *graph)
-- 
1.5.6.rc0.54.g04bfd
Adam Simpkins· Jun 1, 2008, 20:56 UTC · re: Adam Simpkins · lore

[PATCH 2/2] graph API: avoid printing unnecessary padding before some octopus merges

When an octopus merge is printed, several lines are printed before it to move over existing branch lines to its right. This is needed to make room for the children of the octopus merge. For example:

| | | |
| |  \ \
| |   \ \
| |    \ \
| M---. \ \
| |\ \ \ \ \

However, this step isn't necessary if there are no branch lines to the right of the octopus merge. Therefore, skip this step when it is not needed, to avoid printing extra lines that don't really serve any purpose.

Signed-off-by: Adam Simpkins <adam@adamsimpkins.net>
---
 graph.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/graph.c b/graph.c
index 332d1e8..0531716 100644
--- a/graph.c
+++ b/graph.c
@@ -450,16 +450,18 @@ void graph_update(struct git_graph *graph, struct commit *commit)
 	 * it never finished its output.  Goto GRAPH_SKIP, to print out
 	 * a line to indicate that portion of the graph is missing.
 	 *
-	 * Otherwise, if there are 3 or more parents, we need to print
-	 * extra rows before the commit, to expand the branch lines around
-	 * it and make room for it.
+	 * If there are 3 or more parents, we may need to print extra rows
+	 * before the commit, to expand the branch lines around it and make
+	 * room for it.  We need to do this unless there aren't any branch
+	 * rows to the right of this commit.
 	 *
 	 * If there are less than 3 parents, we can immediately print the
 	 * commit line.
 	 */
 	if (graph->state != GRAPH_PADDING)
 		graph->state = GRAPH_SKIP;
-	else if (graph->num_parents >= 3)
+	else if (graph->num_parents >= 3 &&
+		 graph->commit_index < (graph->num_columns - 1))
 		graph->state = GRAPH_PRE_COMMIT;
 	else
 		graph->state = GRAPH_COMMIT;
@@ -538,7 +540,8 @@ static void graph_output_skip_line(struct git_graph *graph, struct strbuf *sb)
 	strbuf_addstr(sb, "...");
 	graph_pad_horizontally(graph, sb);
 
-	if (graph->num_parents >= 3)
+	if (graph->num_parents >= 3 &&
+	    graph->commit_index < (graph->num_columns - 1))
 		graph_update_state(graph, GRAPH_PRE_COMMIT);
 	else
 		graph_update_state(graph, GRAPH_COMMIT);
-- 
1.5.6.rc0.54.g04bfd
Junio C Hamano· Jun 1, 2008, 21:50 UTC · re: Adam Simpkins · lore

Re: [PATCH 2/2] graph API: avoid printing unnecessary padding before some octopus merges

Adam Simpkins <adam@adamsimpkins.net> writes:
Show 36 quoted lines
> When an octopus merge is printed, several lines are printed before it to
> move over existing branch lines to its right.  This is needed to make
> room for the children of the octopus merge.  For example:
>
> | | | |
> | |  \ \
> | |   \ \
> | |    \ \
> | M---. \ \
> | |\ \ \ \ \
>
> However, this step isn't necessary if there are no branch lines to the
> right of the octopus merge.  Therefore, skip this step when it is not
> needed, to avoid printing extra lines that don't really serve any
> purpose.
>
> Signed-off-by: Adam Simpkins <adam@adamsimpkins.net>
> ---
>  graph.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/graph.c b/graph.c
> index 332d1e8..0531716 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -450,16 +450,18 @@ void graph_update(struct git_graph *graph, struct commit *commit)
>  	 * it never finished its output.  Goto GRAPH_SKIP, to print out
>  	 * a line to indicate that portion of the graph is missing.
>  	 *
> -	 * Otherwise, if there are 3 or more parents, we need to print
> -	 * extra rows before the commit, to expand the branch lines around
> -	 * it and make room for it.
> +	 * If there are 3 or more parents, we may need to print extra rows
> +	 * before the commit, to expand the branch lines around it and make
> +	 * room for it.  We need to do this unless there aren't any branch
> +	 * rows to the right of this commit.
Double negation like this is confusing, isn't it?

"We do not have to do this if there isn't any branch row to the right of this commit" may be better. "We need to do this only if there is a branch row (or more) to the right of this commit" would probably be better.

Other than that, the code looks sane to me.
Adam Simpkins· Jun 2, 2008, 00:04 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] graph API: avoid printing unnecessary padding before some octopus merges

On Sun, Jun 01, 2008 at 02:50:34PM -0700, Junio C Hamano wrote:
Show 23 quoted lines
> Adam Simpkins <adam@adamsimpkins.net> writes:
> 
> > diff --git a/graph.c b/graph.c
> > index 332d1e8..0531716 100644
> > --- a/graph.c
> > +++ b/graph.c
> > @@ -450,16 +450,18 @@ void graph_update(struct git_graph *graph, struct commit *commit)
> >  	 * it never finished its output.  Goto GRAPH_SKIP, to print out
> >  	 * a line to indicate that portion of the graph is missing.
> >  	 *
> > -	 * Otherwise, if there are 3 or more parents, we need to print
> > -	 * extra rows before the commit, to expand the branch lines around
> > -	 * it and make room for it.
> > +	 * If there are 3 or more parents, we may need to print extra rows
> > +	 * before the commit, to expand the branch lines around it and make
> > +	 * room for it.  We need to do this unless there aren't any branch
> > +	 * rows to the right of this commit.
> 
> Double negation like this is confusing, isn't it?
> 
> "We do not have to do this if there isn't any branch row to the right of
> this commit" may be better.  "We need to do this only if there is a branch
> row (or more) to the right of this commit" would probably be better.

Yes, I agree it is less confusing without the double negation. Your second choice of wording sounds best.

How do you prefer to fix simple things like this? Do you want to just apply the fix yourself, or is it easier for you if I submit an amended patch?

-- 
Adam Simpkins
adam@adamsimpkins.net
Junio C Hamano· Jun 2, 2008, 04:41 UTC · re: Adam Simpkins · lore

Re: [PATCH 2/2] graph API: avoid printing unnecessary padding before some octopus merges

Adam Simpkins <adam@adamsimpkins.net> writes:
> How do you prefer to fix simple things like this?  Do you want to just
> apply the fix yourself, or is it easier for you if I submit an amended
> patch?

For a small thing like this, it's probably easiest if you said: "Yeah, use that phrasing" (or "It would be even better to say this way: ...") would be good enough. I know how to operate my editor ;-).

← back to recent threads