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

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

[Teemu Likonen]

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.

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

[Wincent Colaiuta]

El 28/5/2008, a las 13:24, Teemu Likonen escribió:
> 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

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

[Adam Simpkins]

On Wed, May 28, 2008 at 02:24:05PM +0300, Teemu Likonen wrote:
> 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

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

[Adam Simpkins]

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

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

[Adam Simpkins]

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

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

[Teemu Likonen]

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.

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

[Adam Simpkins]

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(-)

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

[Adam Simpkins]

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

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

[Adam Simpkins]

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

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

[Junio C Hamano]

Adam Simpkins <adam@adamsimpkins.net> writes:

> 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.

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

[Adam Simpkins]

On Sun, Jun 01, 2008 at 02:50:34PM -0700, Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

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 ;-).