{"thread":{"id":"18980","subject":"[RFC/PATCH] graph API: Use horizontal lines for more compact graphs","startedAt":"2009-04-21T00:40:27Z","lastAt":"2009-04-27T16:35:18Z","messageCount":17,"participants":["Allan Caffee","Johannes Schindelin","Teemu Likonen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"111799","messageId":"20090421004027.GA12330@linux.vnet","threadId":"18980","inReplyTo":null,"subject":"[RFC/PATCH] graph API: Use horizontal lines for more compact graphs","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2009-04-21T00:40:27Z","receivedAt":"2009-04-21T00:40:27Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"Use horizontal lines instead of long diagonal during graph collapsing\nand precommit for more compact and legible graphs.\n\nSigned-off-by: Allan Caffee \n---\n graph.c | 57 +++++++++++++++++++++++++++++++++++++++++--------------\n t/t4202-log.sh | 6 +---\n 2 files changed, 44 insertions(+), 19 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex d4571cf..597e545 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -47,20 +47,6 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n * - Limit the number of columns, similar to the way gitk does.\n * If we reach more than a specified number of columns, omit\n * sections of some columns.\n- *\n- * - The output during the GRAPH_PRE_COMMIT and GRAPH_COLLAPSING states\n- * could be made more compact by printing horizontal lines, instead of\n- * long diagonal lines. For example, during collapsing, something like\n- * this: instead of this:\n- * | | | | | | | | | |\n- * | |_|_|/ | | | |/\n- * |/| | | | | |/|\n- * | | | | | |/| |\n- * |/| | |\n- * | | | |\n- *\n- * If there are several parallel diagonal lines, they will need to be\n- * replaced with horizontal lines on subsequent rows.\n */\n \n struct column {\n@@ -982,6 +968,9 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n {\n \tint i;\n \tint *tmp_mapping;\n+\tshort used_horizontal = 0;\n+\tint horizontal_edge = -1;\n+\tint horizontal_edge_target = -1;\n \n \t/*\n \t * Clear out the new_mapping array\n@@ -1019,6 +1008,17 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n \t\t\t * Move to the left by one\n \t\t\t */\n \t\t\tgraph->new_mapping[i - 1] = target;\n+\t\t\t/*\n+\t\t\t * If there isn't already an edge moving horizontally\n+\t\t\t * select this one.\n+\t\t\t */\n+\t\t\tif (horizontal_edge == -1) {\n+\t\t\t\tint j;\n+\t\t\t\thorizontal_edge = i;\n+\t\t\t\thorizontal_edge_target = target;\n+\t\t\t\tfor (j = (target * 2)+3; j < (i - 2); j += 2)\n+\t\t\t\t\tgraph->new_mapping[j] = target;\n+\t\t\t}\n \t\t} else if (graph->new_mapping[i - 1] == target) {\n \t\t\t/*\n \t\t\t * There is a branch line to our left\n@@ -1039,10 +1039,21 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n \t\t\t *\n \t\t\t * The space just to the left of this\n \t\t\t * branch should always be empty.\n+\t\t\t *\n+\t\t\t * The branch to the left of that space\n+\t\t\t * should be our eventual target.\n \t\t\t */\n \t\t\tassert(graph->new_mapping[i - 1] > target);\n \t\t\tassert(graph->new_mapping[i - 2] < 0);\n+\t\t\tassert(graph->new_mapping[i - 3] == target);\n \t\t\tgraph->new_mapping[i - 2] = target;\n+\t\t\t/*\n+\t\t\t * Mark this branch as the horizontal edge to\n+\t\t\t * prevent any other edges from moving\n+\t\t\t * horizontally.\n+\t\t\t */\n+\t\t\tif (horizontal_edge == -1)\n+\t\t\t\thorizontal_edge = i;\n \t\t}\n \t}\n \n@@ -1061,8 +1072,24 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n \t\t\tstrbuf_addch(sb, ' ');\n \t\telse if (target * 2 == i)\n \t\t\tstrbuf_write_column(sb, &graph->new_columns[target], '|');\n-\t\telse\n+\t\telse if (target == horizontal_edge_target &&\n+\t\t\t i != horizontal_edge - 1) {\n+\t\t\t\t/*\n+\t\t\t\t * Set the mappings for all but the\n+\t\t\t\t * first segment to -1 so that they\n+\t\t\t\t * won't continue into the next line.\n+\t\t\t\t */\n+\t\t\t\tif (i != (target * 2)+3)\n+\t\t\t\t\tgraph->new_mapping[i] = -1;\n+\t\t\t\tused_horizontal = 1;\n+\t\t\tstrbuf_write_column(sb, &graph->new_columns[target], '_');\n+\t\t}\n+\t\telse {\n+\t\t\tif (used_horizontal && i < horizontal_edge)\n+\t\t\t\tgraph->new_mapping[i] = -1;\n \t\t\tstrbuf_write_column(sb, &graph->new_columns[target], '/');\n+\n+\t\t}\n \t}\n \n \tgraph_pad_horizontally(graph, sb, graph->mapping_size);\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex b986190..a3b0cb8 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -298,14 +298,12 @@ cat > expect <<\\EOF\n * | | | Merge branch 'side'\n |\\ \\ \\ \\\n | * | | | side-2\n-| | | |/\n-| | |/|\n+| | |_|/\n | |/| |\n | * | | side-1\n * | | | Second\n * | | | sixth\n-| | |/\n-| |/|\n+| |_|/\n |/| |\n * | | fifth\n * | | fourth\n-- \n1.5.6.3\n"},{"id":"111807","messageId":"alpine.DEB.1.00.0904210255280.10279@pacific.mpi-cbg.de","threadId":"18980","inReplyTo":"20090421004027.GA12330@linux.vnet","subject":"Re: [RFC/PATCH] graph API: Use horizontal lines for more compact graphs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-21T00:56:42Z","receivedAt":"2009-04-21T00:56:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 20 Apr 2009, Allan Caffee wrote:\n\n> diff --git a/graph.c b/graph.c\n> index d4571cf..597e545 100644\n> --- a/graph.c\n> +++ b/graph.c\n> @@ -47,20 +47,6 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n> * - Limit the number of columns, similar to the way gitk does.\n> * If we reach more than a specified number of columns, omit\n> * sections of some columns.\n> - *\n> - * - The output during the GRAPH_PRE_COMMIT and GRAPH_COLLAPSING states\n> - * could be made more compact by printing horizontal lines, instead of\n> - * long diagonal lines. For example, during collapsing, something like\n> - * this: instead of this:\n> - * | | | | | | | | | |\n> - * | |_|_|/ | | | |/\n> - * |/| | | | | |/|\n> - * | | | | | |/| |\n> - * |/| | |\n> - * | | | |\n> - *\n> - * If there are several parallel diagonal lines, they will need to be\n> - * replaced with horizontal lines on subsequent rows.\n\nI like it!\n\n> +\t\t\t\tfor (j = (target * 2)+3; j < (i - 2); j += 2)\n\nThis (target*2)+3 is a bit too magical for me to understand. But maybe I \nam just too tired?\n\nCiao,\nDscho\n"},{"id":"111824","messageId":"b2e43f8f0904201923hd97f3e3v66addf59daa3956f@mail.gmail.com","threadId":"18980","inReplyTo":"alpine.DEB.1.00.0904210255280.10279@pacific.mpi-cbg.de","subject":"Re: [RFC/PATCH] graph API: Use horizontal lines for more compact graphs","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2009-04-21T02:23:20Z","receivedAt":"2009-04-21T02:23:20Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"On Mon, Apr 20, 2009 at 8:56 PM, Johannes Schindelin\n wrote:\n> Hi,\n>\n> On Mon, 20 Apr 2009, Allan Caffee wrote:\n>\n>> diff --git a/graph.c b/graph.c\n>> index d4571cf..597e545 100644\n>> --- a/graph.c\n>> +++ b/graph.c\n>> @@ -47,20 +47,6 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n>>   * - Limit the number of columns, similar to the way gitk does.\n>>   *   If we reach more than a specified number of columns, omit\n>>   *   sections of some columns.\n>> - *\n>> - * - The output during the GRAPH_PRE_COMMIT and GRAPH_COLLAPSING states\n>> - *   could be made more compact by printing horizontal lines, instead of\n>> - *   long diagonal lines.  For example, during collapsing, something like\n>> - *   this:          instead of this:\n>> - *   | | | | |      | | | | |\n>> - *   | |_|_|/       | | | |/\n>> - *   |/| | |        | | |/|\n>> - *   | | | |        | |/| |\n>> - *                  |/| | |\n>> - *                  | | | |\n>> - *\n>> - *   If there are several parallel diagonal lines, they will need to be\n>> - *   replaced with horizontal lines on subsequent rows.\n>\n> I like it!\n\n:) Good!\n\n>> +                             for (j = (target * 2)+3; j < (i - 2); j += 2)\n>\n> This (target*2)+3 is a bit too magical for me to understand.  But maybe I\n> am just too tired?\n\nIt is a little magical. Here target is an index into\ngraph->new_columns so we double that to get the actual location of the\nedge in the string for this line. So if we take the example that was\nin the original TODO:\n\nt(c)\n| t(c) + 3 (i.e. the first horizontal edge)\n| |\nv..v c\n| | | | |\n| |_|_|/\n|/| | |\n| | | |\n\nWhere c is the \"horizontal_edge\", t(c) is the target of the\n\"horizontal_edge\" and t(c) + 3 is the location of the first horizontal\nsegment. And then of course the += 2 is because we don't want to\nchange the mappings of the existing vertical edges. This could really\nprobably use a comment (suggestions welcome).\n\nHope that clears things up,\n~Allan\n"},{"id":"111850","messageId":"alpine.DEB.1.00.0904211010410.10279@pacific.mpi-cbg.de","threadId":"18980","inReplyTo":"b2e43f8f0904201923hd97f3e3v66addf59daa3956f@mail.gmail.com","subject":"Re: [RFC/PATCH] graph API: Use horizontal lines for more compact graphs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-21T08:13:56Z","receivedAt":"2009-04-21T08:13:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 20 Apr 2009, Allan Caffee wrote:\n\n> On Mon, Apr 20, 2009 at 8:56 PM, Johannes Schindelin\n> wrote:\n>\n> > On Mon, 20 Apr 2009, Allan Caffee wrote:\n> >\n> >> +                             for (j = (target * 2)+3; j < (i - 2); j += 2)\n> >\n> > This (target*2)+3 is a bit too magical for me to understand.  But \n> > maybe I am just too tired?\n> \n> It is a little magical. Here target is an index into\n> graph->new_columns so we double that to get the actual location of the\n> edge in the string for this line.\n\nAh. So how about\n\t\t\t\t /*\n\t\t\t\t * The variable target is the index of the graph\n\t\t\t\t * column, and therefore target*2+3 is the actual\n\t\t\t\t * screen column of the first horizontal line.\n\t\t\t\t */\n\nHmm?\n\nCiao,\nDscho"},{"id":"111876","messageId":"20090421124701.GA25982@linux.vnet","threadId":"18980","inReplyTo":"alpine.DEB.1.00.0904211010410.10279@pacific.mpi-cbg.de","subject":"[RFC/PATCH v2] graph API: Use horizontal lines for more compact graphs","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2009-04-21T12:47:01Z","receivedAt":"2009-04-21T12:47:01Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"Use horizontal lines instead of long diagonal during graph collapsing\nand precommit for more compact and legible graphs.\n\nSigned-off-by: Allan Caffee \n---\n graph.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------------\n t/t4202-log.sh | 6 +---\n 2 files changed, 50 insertions(+), 19 deletions(-)\n\nOn Tue, 21 Apr 2009, Johannes Schindelin wrote:\n> Hi,\n> \n> On Mon, 20 Apr 2009, Allan Caffee wrote:\n> \n> > On Mon, Apr 20, 2009 at 8:56 PM, Johannes Schindelin\n> > wrote:\n> >\n> > > On Mon, 20 Apr 2009, Allan Caffee wrote:\n> > >\n> > >> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? for (j = (target * 2)+3; j < (i - 2); j += 2)\n> > >\n> > > This (target*2)+3 is a bit too magical for me to understand. ?But \n> > > maybe I am just too tired?\n> > \n> > It is a little magical. Here target is an index into\n> > graph->new_columns so we double that to get the actual location of the\n> > edge in the string for this line.\n> \n> Ah. So how about\n> \t\t\t\t /*\n> \t\t\t\t * The variable target is the index of the graph\n> \t\t\t\t * column, and therefore target*2+3 is the actual\n> \t\t\t\t * screen column of the first horizontal line.\n> \t\t\t\t */\n\nSounds good to me. Everything else look good? \n\nActually now that I look at it, it might be a good idea to put an assert\nstatement in that for loop like `assert(graph->new_mapping[j] < 0)' to\nmake sure we don't clobber any existing lines. But that seems like\noverkill since we're already assured to be the first collapsing edge at\nthat point, which would imply that all previous odd indeces are empty.\nWDYT?\n\ndiff --git a/graph.c b/graph.c\nindex d4571cf..86577b4 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -47,20 +47,6 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n * - Limit the number of columns, similar to the way gitk does.\n * If we reach more than a specified number of columns, omit\n * sections of some columns.\n- *\n- * - The output during the GRAPH_PRE_COMMIT and GRAPH_COLLAPSING states\n- * could be made more compact by printing horizontal lines, instead of\n- * long diagonal lines. For example, during collapsing, something like\n- * this: instead of this:\n- * | | | | | | | | | |\n- * | |_|_|/ | | | |/\n- * |/| | | | | |/|\n- * | | | | | |/| |\n- * |/| | |\n- * | | | |\n- *\n- * If there are several parallel diagonal lines, they will need to be\n- * replaced with horizontal lines on subsequent rows.\n */\n \n struct column {\n@@ -982,6 +968,9 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n {\n \tint i;\n \tint *tmp_mapping;\n+\tshort used_horizontal = 0;\n+\tint horizontal_edge = -1;\n+\tint horizontal_edge_target = -1;\n \n \t/*\n \t * Clear out the new_mapping array\n@@ -1019,6 +1008,23 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n \t\t\t * Move to the left by one\n \t\t\t */\n \t\t\tgraph->new_mapping[i - 1] = target;\n+\t\t\t/*\n+\t\t\t * If there isn't already an edge moving horizontally\n+\t\t\t * select this one.\n+\t\t\t */\n+\t\t\tif (horizontal_edge == -1) {\n+\t\t\t\tint j;\n+\t\t\t\thorizontal_edge = i;\n+\t\t\t\thorizontal_edge_target = target;\n+\t\t\t\t/*\n+\t\t\t\t * The variable target is the index of the graph\n+\t\t\t\t * column, and therefore target*2+3 is the\n+\t\t\t\t * actual screen column of the first horizontal\n+\t\t\t\t * line.\n+\t\t\t\t */\n+\t\t\t\tfor (j = (target * 2)+3; j < (i - 2); j += 2)\n+\t\t\t\t\tgraph->new_mapping[j] = target;\n+\t\t\t}\n \t\t} else if (graph->new_mapping[i - 1] == target) {\n \t\t\t/*\n \t\t\t * There is a branch line to our left\n@@ -1039,10 +1045,21 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n \t\t\t *\n \t\t\t * The space just to the left of this\n \t\t\t * branch should always be empty.\n+\t\t\t *\n+\t\t\t * The branch to the left of that space\n+\t\t\t * should be our eventual target.\n \t\t\t */\n \t\t\tassert(graph->new_mapping[i - 1] > target);\n \t\t\tassert(graph->new_mapping[i - 2] < 0);\n+\t\t\tassert(graph->new_mapping[i - 3] == target);\n \t\t\tgraph->new_mapping[i - 2] = target;\n+\t\t\t/*\n+\t\t\t * Mark this branch as the horizontal edge to\n+\t\t\t * prevent any other edges from moving\n+\t\t\t * horizontally.\n+\t\t\t */\n+\t\t\tif (horizontal_edge == -1)\n+\t\t\t\thorizontal_edge = i;\n \t\t}\n \t}\n \n@@ -1061,8 +1078,24 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n \t\t\tstrbuf_addch(sb, ' ');\n \t\telse if (target * 2 == i)\n \t\t\tstrbuf_write_column(sb, &graph->new_columns[target], '|');\n-\t\telse\n+\t\telse if (target == horizontal_edge_target &&\n+\t\t\t i != horizontal_edge - 1) {\n+\t\t\t\t/*\n+\t\t\t\t * Set the mappings for all but the\n+\t\t\t\t * first segment to -1 so that they\n+\t\t\t\t * won't continue into the next line.\n+\t\t\t\t */\n+\t\t\t\tif (i != (target * 2)+3)\n+\t\t\t\t\tgraph->new_mapping[i] = -1;\n+\t\t\t\tused_horizontal = 1;\n+\t\t\tstrbuf_write_column(sb, &graph->new_columns[target], '_');\n+\t\t}\n+\t\telse {\n+\t\t\tif (used_horizontal && i < horizontal_edge)\n+\t\t\t\tgraph->new_mapping[i] = -1;\n \t\t\tstrbuf_write_column(sb, &graph->new_columns[target], '/');\n+\n+\t\t}\n \t}\n \n \tgraph_pad_horizontally(graph, sb, graph->mapping_size);\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex b986190..a3b0cb8 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -298,14 +298,12 @@ cat > expect <<\\EOF\n * | | | Merge branch 'side'\n |\\ \\ \\ \\\n | * | | | side-2\n-| | | |/\n-| | |/|\n+| | |_|/\n | |/| |\n | * | | side-1\n * | | | Second\n * | | | sixth\n-| | |/\n-| |/|\n+| |_|/\n |/| |\n * | | fifth\n * | | fourth\n-- \n1.5.6.3\n"},{"id":"111877","messageId":"alpine.DEB.1.00.0904211517201.6559@intel-tinevez-2-302","threadId":"18980","inReplyTo":"20090421124701.GA25982@linux.vnet","subject":"Re: [RFC/PATCH v2] graph API: Use horizontal lines for more compact graphs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-04-21T13:17:59Z","receivedAt":"2009-04-21T13:17:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n> Everything else look good?\n\nNo objection from my side.\n\n> Actually now that I look at it, it might be a good idea to put an assert\n> statement in that for loop like `assert(graph->new_mapping[j] < 0)' to\n> make sure we don't clobber any existing lines. But that seems like\n> overkill since we're already assured to be the first collapsing edge at\n> that point, which would imply that all previous odd indeces are empty.\n> WDYT?\n\nYep, sounds like overkill to me, too.\n\nThanks!\nDscho\n"},{"id":"111878","messageId":"87zlea9lit.fsf_-_@iki.fi","threadId":"18980","inReplyTo":"20090421124701.GA25982@linux.vnet","subject":"Bug in colored \"log --graph\" implementation","fromName":"Teemu Likonen","fromEmail":"tlikonen@iki.fi","sentAt":"2009-04-21T13:36:10Z","receivedAt":"2009-04-21T13:36:10Z","isPatch":false,"sender":{"key":"tlikonen@iki.fi","avatar":null},"body":"The colored log graph implementation (commit 427fc5b) introduces an\nalignment bug which looks like this:\n\n\n| | * | edf2e37 git-apply: work from subdirectory.\n| | * | 4ca0660 working from subdirectory: preparation\n| | | | \n| | \\ \\ \n| | \\ \\ \n| | \\ \\ \n| | \\ \\ \n| | \\ \\ \n| *-----. \\ \\ 5401f30 Merge branches 'jc/apply', 'lt/ls-tree', [...]\n| |\\ \\ \\ \\ \\ \\ \n| | | | | * | | 0501c24 Tutorial: adjust merge example to recursive [...]\n\n\nIn other words, the diagonal lines after this octopus merge are aligned\nwrong. To see it yourself type\n\n git log --graph --oneline a957207\n\nin the Git repository and scroll the output down a bit. Note that the\nbug exists with both --color _and_ --no-color.\n"},{"id":"111895","messageId":"20090421183412.GA8499@linux.vnet","threadId":"18980","inReplyTo":"87zlea9lit.fsf_-_@iki.fi","subject":"[PATCH] graph API: fix extra space during pre_commit_line state","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2009-04-21T18:34:12Z","receivedAt":"2009-04-21T18:34:12Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"An extra space is being inserted between the \"commit\" column and all of\nthe successive edges. Remove this space. This regression was\nintroduced by 427fc5b.\n\nSigned-off-by: Allan Caffee \n---\n graph.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\nOn Tue, 21 Apr 2009, Teemu Likonen wrote:\n> The colored log graph implementation (commit 427fc5b) introduces an\n> alignment bug which looks like this:\n> \n> | | * | edf2e37 git-apply: work from subdirectory.\n> | | * | 4ca0660 working from subdirectory: preparation\n> | | | | \n> | | \\ \\ \n> | | \\ \\ \n> | | \\ \\ \n> | | \\ \\ \n> | | \\ \\ \n> | *-----. \\ \\ 5401f30 Merge branches 'jc/apply', 'lt/ls-tree', [...]\n> | |\\ \\ \\ \\ \\ \\ \n> | | | | | * | | 0501c24 Tutorial: adjust merge example to recursive [...]\n> \n> \n> In other words, the diagonal lines after this octopus merge are aligned\n> wrong. To see it yourself type\n> \n> git log --graph --oneline a957207\n> \n> in the Git repository and scroll the output down a bit. Note that the\n> bug exists with both --color _and_ --no-color.\n\nIt's actually the lines before the merge that are shifted to the right\nby one. This patch should fix that.\n\nThis issue exposes a gap in the existing test coverage, which doesn't\nexercise the pre_commit_line code. Maybe another patch is in order to\nextend t4202-log to cover pre-commit lines and octopus merges.\n\ndiff --git a/graph.c b/graph.c\nindex d4571cf..31e09eb 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -727,8 +727,8 @@ static void graph_output_pre_commit_line(struct git_graph *graph,\n \t\tif (col->commit == graph->commit) {\n \t\t\tseen_this = 1;\n \t\t\tstrbuf_write_column(sb, col, '|');\n-\t\t\tstrbuf_addf(sb, \" %*s\", graph->expansion_row, \"\");\n-\t\t\tchars_written += 2 + graph->expansion_row;\n+\t\t\tstrbuf_addf(sb, \"%*s\", graph->expansion_row, \"\");\n+\t\t\tchars_written += 1 + graph->expansion_row;\n \t\t} else if (seen_this && (graph->expansion_row == 0)) {\n \t\t\t/*\n \t\t\t * This is the first line of the pre-commit output.\n-- \n1.5.6.3\n"},{"id":"111914","messageId":"87y6tt2wuq.fsf@iki.fi","threadId":"18980","inReplyTo":"20090421183412.GA8499@linux.vnet","subject":"Re: [PATCH] graph API: fix extra space during pre_commit_line state","fromName":"Teemu Likonen","fromEmail":"tlikonen@iki.fi","sentAt":"2009-04-22T03:25:33Z","receivedAt":"2009-04-22T03:25:33Z","isPatch":true,"sender":{"key":"tlikonen@iki.fi","avatar":null},"body":"On 2009-04-21 14:34 (-0400), Allan Caffee wrote:\n\n> An extra space is being inserted between the \"commit\" column and all of\n> the successive edges. Remove this space. This regression was\n> introduced by 427fc5b.\n>\n> Signed-off-by: Allan Caffee \n\nLooks like it's working now, thanks. Let's Cc to Junio so that he\ndoesn't miss the fix.\n\n> This issue exposes a gap in the existing test coverage, which doesn't\n> exercise the pre_commit_line code. Maybe another patch is in order to\n> extend t4202-log to cover pre-commit lines and octopus merges.\n\nI think that's a good idea. I like \"log --graph\" very much and when\nsomeone alters that part of the code I run my own visual \"test suites\"\nto notice if my pet feature has been broken. :-) Automatic tests would\nbe helpful.\n"},{"id":"111987","messageId":"20090422193838.GA1841@linux.vnet","threadId":"18980","inReplyTo":"87y6tt2wuq.fsf@iki.fi","subject":"Re: [PATCH] graph API: fix extra space during pre_commit_line state","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2009-04-22T19:38:38Z","receivedAt":"2009-04-22T19:38:38Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"On Wed, 22 Apr 2009, Teemu Likonen wrote:\n\n> On 2009-04-21 14:34 (-0400), Allan Caffee wrote:\n> \n> > An extra space is being inserted between the \"commit\" column and all of\n> > the successive edges. Remove this space. This regression was\n> > introduced by 427fc5b.\n> >\n> > Signed-off-by: Allan Caffee \n> \n> Looks like it's working now, thanks. Let's Cc to Junio so that he\n> doesn't miss the fix.\n\nActually, Junio, please disregard this patch for the moment. I'll\nresend it in a series along with another minor fix related to octopus\nmerges.\n\n> > This issue exposes a gap in the existing test coverage, which doesn't\n> > exercise the pre_commit_line code. Maybe another patch is in order to\n> > extend t4202-log to cover pre-commit lines and octopus merges.\n> \n> I think that's a good idea. I like \"log --graph\" very much and when\n> someone alters that part of the code I run my own visual \"test suites\"\n> to notice if my pet feature has been broken. :-) Automatic tests would\n> be helpful.\n\nI'll include a test patch in that series as well.\n"},{"id":"111991","messageId":"49ef7572.1e038e0a.2347.3896@mx.google.com","threadId":"18980","inReplyTo":"20090422193838.GA1841@linux.vnet","subject":"[PATCH 2/3] graph API: fix extra space during pre_commit_line state","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2009-04-22T19:52:13Z","receivedAt":"2009-04-22T19:52:13Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"An extra space is being inserted between the \"commit\" column and all of\nthe successive edges. Remove this space. This regression was\nintroduced by 427fc5b.\n\nSigned-off-by: Allan Caffee \n---\n graph.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex d4571cf..31e09eb 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -727,8 +727,8 @@ static void graph_output_pre_commit_line(struct git_graph *graph,\n \t\tif (col->commit == graph->commit) {\n \t\t\tseen_this = 1;\n \t\t\tstrbuf_write_column(sb, col, '|');\n-\t\t\tstrbuf_addf(sb, \" %*s\", graph->expansion_row, \"\");\n-\t\t\tchars_written += 2 + graph->expansion_row;\n+\t\t\tstrbuf_addf(sb, \"%*s\", graph->expansion_row, \"\");\n+\t\t\tchars_written += 1 + graph->expansion_row;\n \t\t} else if (seen_this && (graph->expansion_row == 0)) {\n \t\t\t/*\n \t\t\t * This is the first line of the pre-commit output.\n-- \n1.5.6.3\n"},{"id":"112015","messageId":"20090422212715.GA30442@linux.vnet","threadId":"18980","inReplyTo":"20090422193838.GA1841@linux.vnet","subject":"[PATCH 1/3] t4202-log: extend test coverage of graphing","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2009-04-22T21:27:15Z","receivedAt":"2009-04-22T21:27:15Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"Extend this test to cover the rendering of graphs with octopus merges\nand pre_commit lines.\n\nSigned-off-by: Allan Caffee \n---\n t/t4202-log.sh | 28 +++++++++++++++++++++++++++-\n 1 files changed, 27 insertions(+), 1 deletions(-)\n\nThis patch by itself should cause the test to fail. Proving that the\nfollowing two patches are required too return us to sane behaviour.\n\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex b986190..64502e2 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -284,10 +284,36 @@ test_expect_success 'set up more tangled history' '\n \tgit merge master~3 &&\n \tgit merge side~1 &&\n \tgit checkout master &&\n-\tgit merge tangle\n+\tgit merge tangle &&\n+\tgit checkout -b reach &&\n+\ttest_commit reach &&\n+\tgit checkout master &&\n+\tgit checkout -b octopus-a &&\n+\ttest_commit octopus-a &&\n+\tgit checkout master &&\n+\tgit checkout -b octopus-b &&\n+\ttest_commit octopus-b &&\n+\tgit checkout master &&\n+\ttest_commit seventh &&\n+\tgit merge octopus-a octopus-b\n+\tgit merge reach\n '\n \n cat > expect <<\\EOF\n+* Merge branch 'reach'\n+|\\\n+| \\\n+| \\\n+*-. \\ Merge branches 'octopus-a' and 'octopus-b'\n+|\\ \\ \\\n+* | | | seventh\n+| | * | octopus-b\n+| |/ /\n+|/| |\n+| * | octopus-a\n+|/ /\n+| * reach\n+|/\n * Merge branch 'tangle'\n |\\\n | * Merge branch 'side' (early part) into tangle\n-- \n1.5.6.3\n"},{"id":"112016","messageId":"20090422212728.GA30484@linux.vnet","threadId":"18980","inReplyTo":"20090422193838.GA1841@linux.vnet","subject":"[PATCH 2/3] graph API: fix extra space during pre_commit_line state","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2009-04-22T21:27:28Z","receivedAt":"2009-04-22T21:27:28Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"An extra space is being inserted between the \"commit\" column and all of\nthe successive edges. Remove this space. This regression was\nintroduced by 427fc5b.\n\nSigned-off-by: Allan Caffee \n---\n graph.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex d4571cf..31e09eb 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -727,8 +727,8 @@ static void graph_output_pre_commit_line(struct git_graph *graph,\n \t\tif (col->commit == graph->commit) {\n \t\t\tseen_this = 1;\n \t\t\tstrbuf_write_column(sb, col, '|');\n-\t\t\tstrbuf_addf(sb, \" %*s\", graph->expansion_row, \"\");\n-\t\t\tchars_written += 2 + graph->expansion_row;\n+\t\t\tstrbuf_addf(sb, \"%*s\", graph->expansion_row, \"\");\n+\t\t\tchars_written += 1 + graph->expansion_row;\n \t\t} else if (seen_this && (graph->expansion_row == 0)) {\n \t\t\t/*\n \t\t\t * This is the first line of the pre-commit output.\n-- \n1.5.6.3\n"},{"id":"112017","messageId":"20090422212759.GA30512@linux.vnet","threadId":"18980","inReplyTo":"20090422193838.GA1841@linux.vnet","subject":"[PATCH 3/3] graph API: fix a bug in the rendering of octopus merges","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2009-04-22T21:27:59Z","receivedAt":"2009-04-22T21:27:59Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"An off by one error was causing octopus merges with 3 parents to not be\nrendered correctly. This regression was introduced by 427fc5.\n\nSigned-off-by: Allan Caffee \n---\n graph.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex 31e09eb..b7879f8 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -852,7 +852,7 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)\n \t\t\tgraph_output_commit_char(graph, sb);\n \t\t\tchars_written++;\n \n-\t\t\tif (graph->num_parents > 3)\n+\t\t\tif (graph->num_parents > 2)\n \t\t\t\tchars_written += graph_draw_octopus_merge(graph,\n \t\t\t\t\t\t\t\t\t sb);\n \t\t} else if (seen_this && (graph->num_parents > 2)) {\n-- \n1.5.6.3\n"},{"id":"112018","messageId":"20090422212812.GA30830@linux.vnet","threadId":"18980","inReplyTo":"20090421124701.GA25982@linux.vnet","subject":"Re: [RFC/PATCH v2] graph API: Use horizontal lines for more compact graphs","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2009-04-22T21:28:12Z","receivedAt":"2009-04-22T21:28:12Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"On Tue, 21 Apr 2009, Allan Caffee wrote:\n\n> Use horizontal lines instead of long diagonal during graph collapsing\n> and precommit for more compact and legible graphs.\n\nPlease replace this message with:\n----8<----------------\nUse horizontal lines instead of long diagonal lines during the\ncollapsing state of graph rendering. For example what used to be:\n\n | | | | |\n | | | |/\n | | |/|\n | |/| |\n |/| | |\n | | | |\nis now\n | | | | |\n | |_|_|/\n |/| | |\n | | | |\n\nThis results in more compact and legible graphs.\n---->8----------------\n\nNotice that I dropped out the part about precommits. Originally I\nintended to utilize horizontal lines in pre_commit_line's as well. But\npartway through I decided to just do collapsing_line's for now since (a)\ncollapsing_line edges tend to extend much longer and (b)\npre_commit_line's are less common since they only occur when there is a\nmerge with at least three parents, and (c) the change for\npre_commit_lines is liable to be more complicated and error-prone.\n"},{"id":"112426","messageId":"20090427154341.GA9818@linux.vnet","threadId":"18980","inReplyTo":"20090422212812.GA30830@linux.vnet","subject":"[PATCH v2 (resend)] graph API: Use horizontal lines for more compact graphs","fromName":"Allan Caffee","fromEmail":"allan.caffee@gmail.com","sentAt":"2009-04-27T15:43:41Z","receivedAt":"2009-04-27T15:43:41Z","isPatch":true,"sender":{"key":"allan.caffee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/114759?v=4"},"body":"Use horizontal lines instead of long diagonal lines during the\ncollapsing state of graph rendering. For example what used to be:\n\n | | | | |\n | | | |/\n | | |/|\n | |/| |\n |/| | |\n | | | |\nis now\n | | | | |\n | |_|_|/\n |/| | |\n | | | |\n\nresulting in more compact and legible graphs.\n\nSigned-off-by: Allan Caffee \n---\n\nJunio, is this patch acceptable for inclusion?\n\n graph.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------------\n t/t4202-log.sh | 6 +---\n 2 files changed, 50 insertions(+), 19 deletions(-)\n\ndiff --git a/graph.c b/graph.c\nindex 06fbeb6..f3fa253 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -47,20 +47,6 @@ static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);\n * - Limit the number of columns, similar to the way gitk does.\n * If we reach more than a specified number of columns, omit\n * sections of some columns.\n- *\n- * - The output during the GRAPH_PRE_COMMIT and GRAPH_COLLAPSING states\n- * could be made more compact by printing horizontal lines, instead of\n- * long diagonal lines. For example, during collapsing, something like\n- * this: instead of this:\n- * | | | | | | | | | |\n- * | |_|_|/ | | | |/\n- * |/| | | | | |/|\n- * | | | | | |/| |\n- * |/| | |\n- * | | | |\n- *\n- * If there are several parallel diagonal lines, they will need to be\n- * replaced with horizontal lines on subsequent rows.\n */\n \n struct column {\n@@ -982,6 +968,9 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n {\n \tint i;\n \tint *tmp_mapping;\n+\tshort used_horizontal = 0;\n+\tint horizontal_edge = -1;\n+\tint horizontal_edge_target = -1;\n \n \t/*\n \t * Clear out the new_mapping array\n@@ -1019,6 +1008,23 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n \t\t\t * Move to the left by one\n \t\t\t */\n \t\t\tgraph->new_mapping[i - 1] = target;\n+\t\t\t/*\n+\t\t\t * If there isn't already an edge moving horizontally\n+\t\t\t * select this one.\n+\t\t\t */\n+\t\t\tif (horizontal_edge == -1) {\n+\t\t\t\tint j;\n+\t\t\t\thorizontal_edge = i;\n+\t\t\t\thorizontal_edge_target = target;\n+\t\t\t\t/*\n+\t\t\t\t * The variable target is the index of the graph\n+\t\t\t\t * column, and therefore target*2+3 is the\n+\t\t\t\t * actual screen column of the first horizontal\n+\t\t\t\t * line.\n+\t\t\t\t */\n+\t\t\t\tfor (j = (target * 2)+3; j < (i - 2); j += 2)\n+\t\t\t\t\tgraph->new_mapping[j] = target;\n+\t\t\t}\n \t\t} else if (graph->new_mapping[i - 1] == target) {\n \t\t\t/*\n \t\t\t * There is a branch line to our left\n@@ -1039,10 +1045,21 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n \t\t\t *\n \t\t\t * The space just to the left of this\n \t\t\t * branch should always be empty.\n+\t\t\t *\n+\t\t\t * The branch to the left of that space\n+\t\t\t * should be our eventual target.\n \t\t\t */\n \t\t\tassert(graph->new_mapping[i - 1] > target);\n \t\t\tassert(graph->new_mapping[i - 2] < 0);\n+\t\t\tassert(graph->new_mapping[i - 3] == target);\n \t\t\tgraph->new_mapping[i - 2] = target;\n+\t\t\t/*\n+\t\t\t * Mark this branch as the horizontal edge to\n+\t\t\t * prevent any other edges from moving\n+\t\t\t * horizontally.\n+\t\t\t */\n+\t\t\tif (horizontal_edge == -1)\n+\t\t\t\thorizontal_edge = i;\n \t\t}\n \t}\n \n@@ -1061,8 +1078,24 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf\n \t\t\tstrbuf_addch(sb, ' ');\n \t\telse if (target * 2 == i)\n \t\t\tstrbuf_write_column(sb, &graph->new_columns[target], '|');\n-\t\telse\n+\t\telse if (target == horizontal_edge_target &&\n+\t\t\t i != horizontal_edge - 1) {\n+\t\t\t\t/*\n+\t\t\t\t * Set the mappings for all but the\n+\t\t\t\t * first segment to -1 so that they\n+\t\t\t\t * won't continue into the next line.\n+\t\t\t\t */\n+\t\t\t\tif (i != (target * 2)+3)\n+\t\t\t\t\tgraph->new_mapping[i] = -1;\n+\t\t\t\tused_horizontal = 1;\n+\t\t\tstrbuf_write_column(sb, &graph->new_columns[target], '_');\n+\t\t}\n+\t\telse {\n+\t\t\tif (used_horizontal && i < horizontal_edge)\n+\t\t\t\tgraph->new_mapping[i] = -1;\n \t\t\tstrbuf_write_column(sb, &graph->new_columns[target], '/');\n+\n+\t\t}\n \t}\n \n \tgraph_pad_horizontally(graph, sb, graph->mapping_size);\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 67f983f..076f79e 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -324,14 +324,12 @@ cat > expect <<\\EOF\n * | | | Merge branch 'side'\n |\\ \\ \\ \\\n | * | | | side-2\n-| | | |/\n-| | |/|\n+| | |_|/\n | |/| |\n | * | | side-1\n * | | | Second\n * | | | sixth\n-| | |/\n-| |/|\n+| |_|/\n |/| |\n * | | fifth\n * | | fourth\n-- \n1.5.6.3\n"},{"id":"112435","messageId":"7veive6omx.fsf@gitster.siamese.dyndns.org","threadId":"18980","inReplyTo":"20090427154341.GA9818@linux.vnet","subject":"Re: [PATCH v2 (resend)] graph API: Use horizontal lines for more compact graphs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-04-27T16:35:18Z","receivedAt":"2009-04-27T16:35:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Allan Caffee writes:\n\n> Junio, is this patch acceptable for inclusion?\n\nI think your patch is an improvement, and it is queued as part of the\nfirst batch post 1.6.3.\n\nAs a general guideline:\n\n - after -rc0, we do not take any feature enhancement patches that has not\n been discussed on the list before -rc0 was tagged;\n\n - after -rc1, we only take documentation updates, trivial fixes, and\n fixes (not necessarily trivial) to issues introduced since the last\n release (i.e. regression fix);\n\n - after -rc2, we only take documentation updates and regression fixes;\n\n\nBy the way, your Mail-Followup-To seems to be misconfigured.\n"}]}