* [BUG] A part of an edge from an octopus merge gets colored, even with --color=never @ 2016-05-15 13:05 Noam Postavsky 2016-05-17 19:07 ` Johannes Sixt 0 siblings, 1 reply; 22+ messages in thread From: Noam Postavsky @ 2016-05-15 13:05 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 617 bytes --] With a certain topology involving an octopus merge, git log --graph --oneline --all --color=never produces output which includes some ANSI escape code coloring. Attached is a script to reproduce the problem (creates a git repository in subdir log-format-test), along with sample graph and valgrind output (indicates some unitialialized memory access). I've observed the problem with Windows git versions 2.7.0, 2.5.3. I've NOT observed it with 1.9.5, On GNU/Linux the symptom only appears when running with valgrind, I tried versions 2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes from) [-- Attachment #2: graph.log --] [-- Type: text/plain, Size: 142 bytes --] * e98205a c | *^[[31m-^[[m^[[31m.^[[m 808603b merge a b | |\ \ |/ / / | | * 8886a4e b | * | 2d8743f a | |/ * | e09af19 1 |/ * 773004e 0 [-- Attachment #3: test-multiway-merge.sh --] [-- Type: application/x-sh, Size: 660 bytes --] [-- Attachment #4: valgrind.log --] [-- Type: text/plain, Size: 5512 bytes --] ==11287== Memcheck, a memory error detector ==11287== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al. ==11287== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info ==11287== Command: /home/npostavs/src/git/git log --oneline --graph --color=never --all ==11287== ==11287== Conditional jump or move depends on uninitialised value(s) ==11287== at 0x4B0A48: strbuf_write_column (graph.c:79) ==11287== by 0x4B0B3F: graph_draw_octopus_merge (graph.c:799) ==11287== by 0x4B1440: graph_output_commit_line (graph.c:837) ==11287== by 0x4B1726: graph_next_line (graph.c:1126) ==11287== by 0x4B1A5F: graph_show_commit (graph.c:1197) ==11287== by 0x4BCF37: show_log (log-tree.c:601) ==11287== by 0x4BD723: log_tree_commit (log-tree.c:879) ==11287== by 0x441E8C: cmd_log_walk (log.c:345) ==11287== by 0x443961: cmd_log (log.c:660) ==11287== by 0x4050F9: run_builtin (git.c:350) ==11287== by 0x405218: handle_builtin (git.c:536) ==11287== by 0x4055AA: run_argv (git.c:582) ==11287== ==11287== Use of uninitialised value of size 8 ==11287== at 0x4B05A0: column_get_color_code (graph.c:73) ==11287== by 0x4B0A51: strbuf_write_column (graph.c:80) ==11287== by 0x4B0B3F: graph_draw_octopus_merge (graph.c:799) ==11287== by 0x4B1440: graph_output_commit_line (graph.c:837) ==11287== by 0x4B1726: graph_next_line (graph.c:1126) ==11287== by 0x4B1A5F: graph_show_commit (graph.c:1197) ==11287== by 0x4BCF37: show_log (log-tree.c:601) ==11287== by 0x4BD723: log_tree_commit (log-tree.c:879) ==11287== by 0x441E8C: cmd_log_walk (log.c:345) ==11287== by 0x443961: cmd_log (log.c:660) ==11287== by 0x4050F9: run_builtin (git.c:350) ==11287== by 0x405218: handle_builtin (git.c:536) ==11287== ==11287== Conditional jump or move depends on uninitialised value(s) ==11287== at 0x4B0AC5: strbuf_write_column (graph.c:82) ==11287== by 0x4B0B3F: graph_draw_octopus_merge (graph.c:799) ==11287== by 0x4B1440: graph_output_commit_line (graph.c:837) ==11287== by 0x4B1726: graph_next_line (graph.c:1126) ==11287== by 0x4B1A5F: graph_show_commit (graph.c:1197) ==11287== by 0x4BCF37: show_log (log-tree.c:601) ==11287== by 0x4BD723: log_tree_commit (log-tree.c:879) ==11287== by 0x441E8C: cmd_log_walk (log.c:345) ==11287== by 0x443961: cmd_log (log.c:660) ==11287== by 0x4050F9: run_builtin (git.c:350) ==11287== by 0x405218: handle_builtin (git.c:536) ==11287== by 0x4055AA: run_argv (git.c:582) ==11287== ==11287== Conditional jump or move depends on uninitialised value(s) ==11287== at 0x4B0A48: strbuf_write_column (graph.c:79) ==11287== by 0x4B0B6F: graph_draw_octopus_merge (graph.c:802) ==11287== by 0x4B1440: graph_output_commit_line (graph.c:837) ==11287== by 0x4B1726: graph_next_line (graph.c:1126) ==11287== by 0x4B1A5F: graph_show_commit (graph.c:1197) ==11287== by 0x4BCF37: show_log (log-tree.c:601) ==11287== by 0x4BD723: log_tree_commit (log-tree.c:879) ==11287== by 0x441E8C: cmd_log_walk (log.c:345) ==11287== by 0x443961: cmd_log (log.c:660) ==11287== by 0x4050F9: run_builtin (git.c:350) ==11287== by 0x405218: handle_builtin (git.c:536) ==11287== by 0x4055AA: run_argv (git.c:582) ==11287== ==11287== Use of uninitialised value of size 8 ==11287== at 0x4B05A0: column_get_color_code (graph.c:73) ==11287== by 0x4B0A51: strbuf_write_column (graph.c:80) ==11287== by 0x4B0B6F: graph_draw_octopus_merge (graph.c:802) ==11287== by 0x4B1440: graph_output_commit_line (graph.c:837) ==11287== by 0x4B1726: graph_next_line (graph.c:1126) ==11287== by 0x4B1A5F: graph_show_commit (graph.c:1197) ==11287== by 0x4BCF37: show_log (log-tree.c:601) ==11287== by 0x4BD723: log_tree_commit (log-tree.c:879) ==11287== by 0x441E8C: cmd_log_walk (log.c:345) ==11287== by 0x443961: cmd_log (log.c:660) ==11287== by 0x4050F9: run_builtin (git.c:350) ==11287== by 0x405218: handle_builtin (git.c:536) ==11287== ==11287== Conditional jump or move depends on uninitialised value(s) ==11287== at 0x4B0AC5: strbuf_write_column (graph.c:82) ==11287== by 0x4B0B6F: graph_draw_octopus_merge (graph.c:802) ==11287== by 0x4B1440: graph_output_commit_line (graph.c:837) ==11287== by 0x4B1726: graph_next_line (graph.c:1126) ==11287== by 0x4B1A5F: graph_show_commit (graph.c:1197) ==11287== by 0x4BCF37: show_log (log-tree.c:601) ==11287== by 0x4BD723: log_tree_commit (log-tree.c:879) ==11287== by 0x441E8C: cmd_log_walk (log.c:345) ==11287== by 0x443961: cmd_log (log.c:660) ==11287== by 0x4050F9: run_builtin (git.c:350) ==11287== by 0x405218: handle_builtin (git.c:536) ==11287== by 0x4055AA: run_argv (git.c:582) ==11287== ==11287== ==11287== HEAP SUMMARY: ==11287== in use at exit: 651,170 bytes in 191 blocks ==11287== total heap usage: 414 allocs, 223 frees, 1,756,967 bytes allocated ==11287== ==11287== LEAK SUMMARY: ==11287== definitely lost: 825 bytes in 8 blocks ==11287== indirectly lost: 1,515 bytes in 10 blocks ==11287== possibly lost: 0 bytes in 0 blocks ==11287== still reachable: 648,830 bytes in 173 blocks ==11287== suppressed: 0 bytes in 0 blocks ==11287== Rerun with --leak-check=full to see details of leaked memory ==11287== ==11287== For counts of detected and suppressed errors, rerun with: -v ==11287== Use --track-origins=yes to see where uninitialised values come from ==11287== ERROR SUMMARY: 6 errors from 6 contexts (suppressed: 0 from 0) ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2016-05-15 13:05 [BUG] A part of an edge from an octopus merge gets colored, even with --color=never Noam Postavsky @ 2016-05-17 19:07 ` Johannes Sixt 2016-05-17 19:45 ` Jeff King 0 siblings, 1 reply; 22+ messages in thread From: Johannes Sixt @ 2016-05-17 19:07 UTC (permalink / raw) To: Noam Postavsky; +Cc: git Am 15.05.2016 um 15:05 schrieb Noam Postavsky: > With a certain topology involving an octopus merge, git log --graph > --oneline --all --color=never produces output which includes some ANSI > escape code coloring. Attached is a script to reproduce the problem > (creates a git repository in subdir log-format-test), along with > sample graph and valgrind output (indicates some unitialialized memory > access). > > I've observed the problem with Windows git versions 2.7.0, 2.5.3. > I've NOT observed it with 1.9.5, > > On GNU/Linux the symptom only appears when running with valgrind, I > tried versions > 2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes from) > Sorry, I can't reproduce your observation. I ran the script you provided with HOME=$PWD and a minimal .gitconfig that only sets user.email. But valgrind is happy with both 2.8.0 and v2.8.2-402-gedec370 on my Linux box. -- Hannes ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2016-05-17 19:07 ` Johannes Sixt @ 2016-05-17 19:45 ` Jeff King 2016-05-17 19:51 ` Jeff King 2016-05-17 20:02 ` Johannes Sixt 0 siblings, 2 replies; 22+ messages in thread From: Jeff King @ 2016-05-17 19:45 UTC (permalink / raw) To: Johannes Sixt; +Cc: Noam Postavsky, git On Tue, May 17, 2016 at 09:07:33PM +0200, Johannes Sixt wrote: > Am 15.05.2016 um 15:05 schrieb Noam Postavsky: > > With a certain topology involving an octopus merge, git log --graph > > --oneline --all --color=never produces output which includes some ANSI > > escape code coloring. Attached is a script to reproduce the problem > > (creates a git repository in subdir log-format-test), along with > > sample graph and valgrind output (indicates some unitialialized memory > > access). > > > > I've observed the problem with Windows git versions 2.7.0, 2.5.3. > > I've NOT observed it with 1.9.5, > > > > On GNU/Linux the symptom only appears when running with valgrind, I > > tried versions > > 2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes from) > > > > Sorry, I can't reproduce your observation. I ran the script you provided > with HOME=$PWD and a minimal .gitconfig that only sets user.email. But > valgrind is happy with both 2.8.0 and v2.8.2-402-gedec370 on my Linux box. Interesting. It replicates out of the box for me. It looks like the column pointer we are passing is bogus. If I instrument git like this: diff --git a/graph.c b/graph.c index 1350bdd..62a5810 100644 --- a/graph.c +++ b/graph.c @@ -76,6 +76,7 @@ static const char *column_get_color_code(unsigned short color) static void strbuf_write_column(struct strbuf *sb, const struct column *c, char col_char) { + warning("c=%p, c->color = %d, max=%d", c, c->color, column_colors_max); if (c->color < column_colors_max) strbuf_addstr(sb, column_get_color_code(c->color)); strbuf_addch(sb, col_char); @@ -390,6 +391,9 @@ static void graph_insert_into_new_columns(struct git_graph *graph, */ graph->new_columns[graph->num_new_columns].commit = commit; graph->new_columns[graph->num_new_columns].color = graph_find_commit_color(graph, commit); + warning("assigned %p, %d", + &graph->new_columns[graph->num_new_columns], + graph->new_columns[graph->num_new_columns].color); graph->mapping[*mapping_index] = graph->num_new_columns; *mapping_index += 2; graph->num_new_columns++; Then I get this output: warning: assigned 0x20d21d0, 12 * 163b784 (c) c warning: assigned 0x20d8f90, 12 warning: assigned 0x20d8fa0, 12 warning: assigned 0x20d8fb0, 12 warning: c=0x20d21d0, c->color = 12, max=12 warning: c=0x20d8fc0, c->color = 0, max=12 warning: c=0x20d8fc0, c->color = 0, max=12 | *-. a9a6975 (HEAD -> m) merge a b Note that we set up f90, fa0, and fb0, but then pass fc0 into strbuf_write_column (and it has bogus color values). It looks like we're reading one past the end of our array, but I haven't figured out where or why. -Peff ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2016-05-17 19:45 ` Jeff King @ 2016-05-17 19:51 ` Jeff King 2016-05-17 19:55 ` Jeff King 2016-05-17 20:02 ` Johannes Sixt 1 sibling, 1 reply; 22+ messages in thread From: Jeff King @ 2016-05-17 19:51 UTC (permalink / raw) To: Johannes Sixt; +Cc: Noam Postavsky, git On Tue, May 17, 2016 at 03:45:34PM -0400, Jeff King wrote: > Note that we set up f90, fa0, and fb0, but then pass fc0 into > strbuf_write_column (and it has bogus color values). It looks like we're > reading one past the end of our array, but I haven't figured out where > or why. Looking at the valgrind output reveals that. Here's an assert() that catches it reliably for me: diff --git a/graph.c b/graph.c index 1350bdd..964bbd1 100644 --- a/graph.c +++ b/graph.c @@ -794,9 +794,11 @@ static int graph_draw_octopus_merge(struct git_graph *graph, ((graph->num_parents - dashless_commits) * 2) - 1; for (i = 0; i < num_dashes; i++) { col_num = (i / 2) + dashless_commits + graph->commit_index; + assert(col_num < graph->num_new_columns); strbuf_write_column(sb, &graph->new_columns[col_num], '-'); } col_num = (i / 2) + dashless_commits + graph->commit_index; + assert(col_num < graph->num_new_columns); strbuf_write_column(sb, &graph->new_columns[col_num], '.'); return num_dashes + 1; } (It's actually the first one which triggers). I'm not familiar enough with the code to know whether the col_num computation is bogus, or whether we needed to earlier increase the size of the "new_columns" field. -Peff ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2016-05-17 19:51 ` Jeff King @ 2016-05-17 19:55 ` Jeff King 2016-05-20 22:12 ` Noam Postavsky 0 siblings, 1 reply; 22+ messages in thread From: Jeff King @ 2016-05-17 19:55 UTC (permalink / raw) To: Johannes Sixt; +Cc: Hemmo Nieminen, Noam Postavsky, git On Tue, May 17, 2016 at 03:51:37PM -0400, Jeff King wrote: > On Tue, May 17, 2016 at 03:45:34PM -0400, Jeff King wrote: > > > Note that we set up f90, fa0, and fb0, but then pass fc0 into > > strbuf_write_column (and it has bogus color values). It looks like we're > > reading one past the end of our array, but I haven't figured out where > > or why. > > Looking at the valgrind output reveals that. Here's an assert() that > catches it reliably for me: > > diff --git a/graph.c b/graph.c > index 1350bdd..964bbd1 100644 > --- a/graph.c > +++ b/graph.c > @@ -794,9 +794,11 @@ static int graph_draw_octopus_merge(struct git_graph *graph, > ((graph->num_parents - dashless_commits) * 2) - 1; > for (i = 0; i < num_dashes; i++) { > col_num = (i / 2) + dashless_commits + graph->commit_index; > + assert(col_num < graph->num_new_columns); > strbuf_write_column(sb, &graph->new_columns[col_num], '-'); > } > col_num = (i / 2) + dashless_commits + graph->commit_index; > + assert(col_num < graph->num_new_columns); > strbuf_write_column(sb, &graph->new_columns[col_num], '.'); > return num_dashes + 1; > } > > (It's actually the first one which triggers). I'm not familiar enough > with the code to know whether the col_num computation is bogus, or > whether we needed to earlier increase the size of the "new_columns" > field. And unsurprisingly, reverting 339c17bc7690b5436ac61c996cede3d52c85b50d seems to fix this (author cc'd). It's the extra "commit_index" addition that causes the problem. But I'm still not sure what the correct solution is. -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2016-05-17 19:55 ` Jeff King @ 2016-05-20 22:12 ` Noam Postavsky 2018-06-23 21:45 ` Noam Postavsky 0 siblings, 1 reply; 22+ messages in thread From: Noam Postavsky @ 2016-05-20 22:12 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Hemmo Nieminen, git On Tue, May 17, 2016 at 3:55 PM, Jeff King <peff@peff.net> wrote: >> (It's actually the first one which triggers). I'm not familiar enough >> with the code to know whether the col_num computation is bogus, or >> whether we needed to earlier increase the size of the "new_columns" >> field. > > And unsurprisingly, reverting 339c17bc7690b5436ac61c996cede3d52c85b50d > seems to fix this (author cc'd). It's the extra "commit_index" addition > that causes the problem. But I'm still not sure what the correct > solution is. Looking at the coloured output, for some octopus merges where the first parent edge immediately merges into the next column to the left, col_num should be decremented by 1 (otherwise the colour of the "-." doesn't match the rest of that edge). | | *-. | | |\ \ | |/ / / For the other case where the first parent edge stays straight, the current col_num computation is correct. | *-. | |\ \ | | | * I'm not sure how to distinguish these cases in the code though. Is it enough to just compare against graph->num_new_columns? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2016-05-20 22:12 ` Noam Postavsky @ 2018-06-23 21:45 ` Noam Postavsky 2018-06-25 16:23 ` Jeff King 0 siblings, 1 reply; 22+ messages in thread From: Noam Postavsky @ 2018-06-23 21:45 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Hemmo Nieminen, git [-- Attachment #1: Type: text/plain, Size: 791 bytes --] Archive link to previous discussion: https://marc.info/?l=git&m=146331754420554&w=2 On 20 May 2016 at 18:12, Noam Postavsky <npostavs@users.sourceforge.net> wrote: > Looking at the coloured output, for some octopus merges where the > first parent edge immediately merges into the next column to the left, > col_num should be decremented by 1 (otherwise the colour of the "-." > doesn't match the rest of that edge). > > | | *-. > | | |\ \ > | |/ / / > > For the other case where the first parent edge stays straight, the > current col_num computation is correct. > > | *-. > | |\ \ > | | | * > > I'm not sure how to distinguish these cases in the code though. Is it > enough to just compare against graph->num_new_columns? I was recently reminded of this, here's a patch which does that. [-- Attachment #2: v1-0001-log-Fix-coloring-of-certain-octupus-merge-shapes.patch --] [-- Type: text/x-diff, Size: 1512 bytes --] From d0c4f19ff162e63d5d23d456d0fc4fe9a32029ee Mon Sep 17 00:00:00 2001 From: Noam Postavsky <npostavs@users.sourceforge.net> Date: Sat, 23 Jun 2018 16:56:43 -0400 Subject: [PATCH v1] log: Fix coloring of certain octupus merge shapes For octopus merges where the first parent edge immediately merges into the next column to the left: | | *-. | | |\ \ | |/ / / then the number of columns should be one less than the usual case: | *-. | |\ \ | | | * Signed-off-by: Noam Postavsky <npostavs@users.sourceforge.net> --- graph.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/graph.c b/graph.c index e1f6d3bdd..c919c86e8 100644 --- a/graph.c +++ b/graph.c @@ -856,12 +856,16 @@ static int graph_draw_octopus_merge(struct git_graph *graph, int col_num, i; int num_dashes = ((graph->num_parents - dashless_commits) * 2) - 1; - for (i = 0; i < num_dashes; i++) { - col_num = (i / 2) + dashless_commits + graph->commit_index; + int first_col = dashless_commits + graph->commit_index; + int last_col = first_col + (num_dashes / 2); + if (last_col >= graph->num_new_columns) { + first_col--; + last_col--; + } + for (i = 0, col_num = first_col; i < num_dashes; i++, col_num++) { strbuf_write_column(sb, &graph->new_columns[col_num], '-'); } - col_num = (i / 2) + dashless_commits + graph->commit_index; - strbuf_write_column(sb, &graph->new_columns[col_num], '.'); + strbuf_write_column(sb, &graph->new_columns[last_col], '.'); return num_dashes + 1; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-06-23 21:45 ` Noam Postavsky @ 2018-06-25 16:23 ` Jeff King 2018-06-30 12:47 ` Noam Postavsky 0 siblings, 1 reply; 22+ messages in thread From: Jeff King @ 2018-06-25 16:23 UTC (permalink / raw) To: Noam Postavsky; +Cc: Johannes Sixt, Hemmo Nieminen, git On Sat, Jun 23, 2018 at 05:45:19PM -0400, Noam Postavsky wrote: > On 20 May 2016 at 18:12, Noam Postavsky <npostavs@users.sourceforge.net> wrote: My, this is a blast from the past. :) > Subject: [PATCH v1] log: Fix coloring of certain octupus merge shapes > > For octopus merges where the first parent edge immediately merges into > the next column to the left: > > | | *-. > | | |\ \ > | |/ / / > > then the number of columns should be one less than the usual case: > > | *-. > | |\ \ > | | | * These diagrams confused me for a minute, because I see two differences: 1. The first one has an extra apparently unrelated parallel branch on the far left. 2. The first has the first-parent of the "*" merge commit immediately join the branch. But if I understand correctly, we only care about the second property. So would it be accurate to show them as: | *-. | |\ \ |/ / / | *-. | |\ \ | | | * ? I think that makes it easier to compare them. I don't remember much about our prior discussion, so let me try to talk myself through the patch itself: > diff --git a/graph.c b/graph.c > index e1f6d3bdd..c919c86e8 100644 > --- a/graph.c > +++ b/graph.c > @@ -856,12 +856,16 @@ static int graph_draw_octopus_merge(struct git_graph *graph, > int col_num, i; > int num_dashes = > ((graph->num_parents - dashless_commits) * 2) - 1; > - for (i = 0; i < num_dashes; i++) { > - col_num = (i / 2) + dashless_commits + graph->commit_index; OK, so the old code emitted num_dashes, and every pair was done with the same column. Our highest iteration of this loop would use the column at (num_dashes-1) / 2. We know that num_dashes is always odd, so: num_dashes = 1 puts our last column at 0 num_dashes = 3 puts our last column at 1 And so on. So far so good. > + int first_col = dashless_commits + graph->commit_index; This corresponds to the i=0 case, makes sense. > + int last_col = first_col + (num_dashes / 2); But here our last_col misses the "-1". I don't think it matters because we know num_dashes is always odd, and therefore due to integer truncation (num_dashes-1)/2 == (num_dashes/2). > + if (last_col >= graph->num_new_columns) { > + first_col--; > + last_col--; > + } The shifting of last_col I expect as part of the fix. I was surprised by shifting first_col, though. Wouldn't it always start at 0 (offset by the previous commits)? It definitely seems to be necessary, but I'm not sure I understand why. > + for (i = 0, col_num = first_col; i < num_dashes; i++, col_num++) { > strbuf_write_column(sb, &graph->new_columns[col_num], '-'); > } > - col_num = (i / 2) + dashless_commits + graph->commit_index; > - strbuf_write_column(sb, &graph->new_columns[col_num], '.'); > + strbuf_write_column(sb, &graph->new_columns[last_col], '.'); In this new loop we count up our dashes and our columns. But now we have 1-to-1 correspondence as we increment! I don't think that can be right. And indeed, if I take your original problem report and add an extra "d" branch and make the octopus "a b d", then the problem comes back. You don't notice with a 3-parent merge because We need to increment col_num only half as much as num_dashes. Should we be doing: for (col_num = first_col; col_num < last_col; col_num++) { strbuf_write_column(sb, &graph->new_columns[col_num], '-'); strbuf_write_column(sb, &graph->new_columns[col_num], '-'); } strbuf_write_column(sb, &graph->new_columns[last_col], '-'); strbuf_write_column(sb, &graph->new_columns[last_col], '.'); I.e., write "--" for each interior column, and then "-." for the last one? -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-06-25 16:23 ` Jeff King @ 2018-06-30 12:47 ` Noam Postavsky 2018-08-06 18:34 ` Noam Postavsky 2018-08-06 21:26 ` Jeff King 0 siblings, 2 replies; 22+ messages in thread From: Noam Postavsky @ 2018-06-30 12:47 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Hemmo Nieminen, git [-- Attachment #1: Type: text/plain, Size: 1268 bytes --] On 25 June 2018 at 12:23, Jeff King <peff@peff.net> wrote: > These diagrams confused me for a minute, because I see two differences: > > 1. The first one has an extra apparently unrelated parallel branch on > the far left. > > 2. The first has the first-parent of the "*" merge commit immediately > join the branch. > > But if I understand correctly, we only care about the second property. Yeah, sorry about that, I just copied them from "natural" occurences and didn't remove all the non-relevant detail. > I don't remember much about our prior discussion, so let me try to talk > myself through the patch itself: I didn't remember all that much either, but I did know that I didn't have a very strong grasp on the code at the time. But your talk-through convinced me that I really have no clue what's going on :) I'm still having trouble getting a big picture understanding of how the graph struct relates the what gets drawn on screen, but through some poking around with the debugger + trial & error, I've arrived at a new patch which seems to work. It's also a lot simpler. I hope you can tell me if it makes sense. Also attached an updated test-multiway-merge.sh which allows adding more branches to test different sized merges more easily. [-- Attachment #2: v2-0001-log-Fix-coloring-of-certain-octupus-merge-shapes.patch --] [-- Type: text/x-diff, Size: 1098 bytes --] From ad40c5986264af1f5934b05082e16a3ce314caab Mon Sep 17 00:00:00 2001 From: Noam Postavsky <npostavs@users.sourceforge.net> Date: Sat, 23 Jun 2018 16:56:43 -0400 Subject: [PATCH v2] log: Fix coloring of certain octupus merge shapes The graph->new_columns index should depend on graph->commit_index. Signed-off-by: Noam Postavsky <npostavs@users.sourceforge.net> --- graph.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/graph.c b/graph.c index e1f6d3bdd..c78259020 100644 --- a/graph.c +++ b/graph.c @@ -857,10 +857,10 @@ static int graph_draw_octopus_merge(struct git_graph *graph, int num_dashes = ((graph->num_parents - dashless_commits) * 2) - 1; for (i = 0; i < num_dashes; i++) { - col_num = (i / 2) + dashless_commits + graph->commit_index; + col_num = (i / 2) + dashless_commits; strbuf_write_column(sb, &graph->new_columns[col_num], '-'); } - col_num = (i / 2) + dashless_commits + graph->commit_index; + col_num = (i / 2) + dashless_commits; strbuf_write_column(sb, &graph->new_columns[col_num], '.'); return num_dashes + 1; } -- 2.11.0 [-- Attachment #3: test-multiway-merge.sh --] [-- Type: application/x-sh, Size: 856 bytes --] ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-06-30 12:47 ` Noam Postavsky @ 2018-08-06 18:34 ` Noam Postavsky 2018-08-06 21:28 ` Jeff King 2018-08-06 21:26 ` Jeff King 1 sibling, 1 reply; 22+ messages in thread From: Noam Postavsky @ 2018-08-06 18:34 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Hemmo Nieminen, git On 30 June 2018 at 08:47, Noam Postavsky <npostavs@users.sourceforge.net> wrote: > I'm still having trouble getting a big picture understanding of how > the graph struct relates the what gets drawn on screen, but through > some poking around with the debugger + trial & error, I've arrived at > a new patch which seems to work. It's also a lot simpler. I hope you > can tell me if it makes sense. > > Also attached an updated test-multiway-merge.sh which allows adding > more branches to test different sized merges more easily. Ping? (I got some bounce message regarding test-multiway-merge.sh, but it does show up in the mailing list archive, so I think my message has gone through) https://marc.info/?l=git&m=153036284214253&w=2 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-08-06 18:34 ` Noam Postavsky @ 2018-08-06 21:28 ` Jeff King 0 siblings, 0 replies; 22+ messages in thread From: Jeff King @ 2018-08-06 21:28 UTC (permalink / raw) To: Noam Postavsky; +Cc: Johannes Sixt, Hemmo Nieminen, git On Mon, Aug 06, 2018 at 02:34:45PM -0400, Noam Postavsky wrote: > On 30 June 2018 at 08:47, Noam Postavsky <npostavs@users.sourceforge.net> wrote: > > > I'm still having trouble getting a big picture understanding of how > > the graph struct relates the what gets drawn on screen, but through > > some poking around with the debugger + trial & error, I've arrived at > > a new patch which seems to work. It's also a lot simpler. I hope you > > can tell me if it makes sense. > > > > Also attached an updated test-multiway-merge.sh which allows adding > > more branches to test different sized merges more easily. > > Ping? (I got some bounce message regarding test-multiway-merge.sh, but > it does show up in the mailing list archive, so I think my message has > gone through) > > https://marc.info/?l=git&m=153036284214253&w=2 No, it made it. It just got shuffled to the bottom of my pile (for future reference, you can ping more frequently than once a month if you think something was dropped ;) ). -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-06-30 12:47 ` Noam Postavsky 2018-08-06 18:34 ` Noam Postavsky @ 2018-08-06 21:26 ` Jeff King 2018-09-02 0:34 ` Noam Postavsky 1 sibling, 1 reply; 22+ messages in thread From: Jeff King @ 2018-08-06 21:26 UTC (permalink / raw) To: Noam Postavsky; +Cc: Johannes Sixt, Hemmo Nieminen, git On Sat, Jun 30, 2018 at 08:47:16AM -0400, Noam Postavsky wrote: > I'm still having trouble getting a big picture understanding of how > the graph struct relates the what gets drawn on screen, but through > some poking around with the debugger + trial & error, I've arrived at > a new patch which seems to work. It's also a lot simpler. I hope you > can tell me if it makes sense. > [...] > int num_dashes = > ((graph->num_parents - dashless_commits) * 2) - 1; > for (i = 0; i < num_dashes; i++) { > - col_num = (i / 2) + dashless_commits + graph->commit_index; > + col_num = (i / 2) + dashless_commits; > strbuf_write_column(sb, &graph->new_columns[col_num], '-'); > } > - col_num = (i / 2) + dashless_commits + graph->commit_index; > + col_num = (i / 2) + dashless_commits; Hmm. So this seems to work because in your example, we're showing the merge in slot 1. So the index is 1, and we have an off-by-one problem, and getting rid of that solves it. But what if we had another line of development to the left of that? I.e., if the "c" in your script was itself on the right-hand side of a merge. We can simulate that by adding this to your script, right before the invocation of log: # We traverse in commit-date order, so make sure the new commit is # more recent than the others. This is also the cause of your "calling # it x doesn't work" comment, I think (all of these commits are # created in a single second, so the order we hit the refs in --all # matters). sleep 1 git checkout -b a-prime master^ git commit --allow-empty -m a-prime That gives me a graph like this (for d-e-f): * d342ed8 (HEAD -> a-prime) a-prime | * 14aae3a (c) c | | *-------. 4bacae1 (m) merge a b d e f | | |\ \ \ \ \ | |/ / / / / / | | | | | | * f19c3a9 (f) f | |_|_|_|_|/ |/| | | | | | | | | | * 48fd961 (e) e | |_|_|_|/ |/| | | | | | | | * 3f4914f (d) d | |_|_|/ |/| | | | | | * 8bef98c (b) b | |_|/ |/| | | | * 253e7ba (a) a | |/ |/| | * 8a60f32 (master) 1 |/ * b00ba42 0 and graph->commit_index is 2. That doesn't trigger valgrind, but all the colors are off-by-one (which makes sense; we're off-by-one towards the beginning of the array now). Using "graph->commit_index - 1" seems to yield the right results, but it feels like we're just hacking around it. And my understanding was that the "straight edge" case actually works with the current code, and we'd probably be breaking that. I still think it makes more sense to iterate over the columns rather than over the dashes, which removes a lot of these confusing cases. This is what I came up with: diff --git a/graph.c b/graph.c index c782590202..d0a3e0858b 100644 --- a/graph.c +++ b/graph.c @@ -847,22 +847,24 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) static int graph_draw_octopus_merge(struct git_graph *graph, struct strbuf *sb) { + int col_num, first_col, last_col; + + /* Skip the current commit, since we've already drawn its asterisk. */ + first_col = graph->commit_index + 1; /* - * Here dashless_commits represents the number of parents - * which don't need to have dashes (because their edges fit - * neatly under the commit). + * We subtract three, one each for: + * - the commit we're directly on top of + * - the commit to our left that we're merged into + * - we want to point to the final column, not one past */ - const int dashless_commits = 2; - int col_num, i; - int num_dashes = - ((graph->num_parents - dashless_commits) * 2) - 1; - for (i = 0; i < num_dashes; i++) { - col_num = (i / 2) + dashless_commits; + last_col = first_col + graph->num_parents - 3; + + for (col_num = first_col; col_num <= last_col; col_num++) { strbuf_write_column(sb, &graph->new_columns[col_num], '-'); + strbuf_write_column(sb, &graph->new_columns[col_num], + col_num == last_col ? '.' : '-'); } - col_num = (i / 2) + dashless_commits; - strbuf_write_column(sb, &graph->new_columns[col_num], '.'); - return num_dashes + 1; + return 2 * (last_col - first_col + 1); } static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb) I suspect it still has a bug, which is that it is handling this first-parent-goes-left case, but probably gets the straight-parent case wrong. But at least in this form, I think it is obvious to see where that bug is (the "three" in the comment is not accurate in that latter case, and it should be two). Which I think is what your original fix was getting at: we need to set first/last to start off with, and then "shrink" them with a conditional depending on which form we're seeing. -Peff ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-08-06 21:26 ` Jeff King @ 2018-09-02 0:34 ` Noam Postavsky 2018-09-08 16:13 ` Jeff King 0 siblings, 1 reply; 22+ messages in thread From: Noam Postavsky @ 2018-09-02 0:34 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Hemmo Nieminen, git [-- Attachment #1: Type: text/plain, Size: 486 bytes --] On 6 August 2018 at 17:26, Jeff King <peff@peff.net> wrote: > I suspect it still has a bug, which is that it is handling this > first-parent-goes-left case, but probably gets the straight-parent case > wrong. But at least in this form, I think it is obvious to see where > that bug is (the "three" in the comment is not accurate in that latter > case, and it should be two). Yes, thanks, it makes a lot more sense this way. I believe the attached handles both parent types correctly. [-- Attachment #2: v3-0001-log-Fix-coloring-of-certain-octupus-merge-shapes.patch --] [-- Type: text/x-diff, Size: 2819 bytes --] From a841a50b016c0cfc9183384e6c3ca85a23d1e11f Mon Sep 17 00:00:00 2001 From: Noam Postavsky <npostavs@users.sourceforge.net> Date: Sat, 1 Sep 2018 20:07:16 -0400 Subject: [PATCH v3] log: Fix coloring of certain octupus merge shapes For octopus merges where the first parent edge immediately merges into the next column to the left: | *-. | |\ \ |/ / / then the number of columns should be one less than the usual case: | *-. | |\ \ | | | * Also refactor the code to iterate over columns rather than dashes, building from an initial patch suggestion by Jeff King. Signed-off-by: Noam Postavsky <npostavs@users.sourceforge.net> --- graph.c | 48 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 36 insertions(+), 12 deletions(-) diff --git a/graph.c b/graph.c index e1f6d3bdd..478c86dfb 100644 --- a/graph.c +++ b/graph.c @@ -848,21 +848,45 @@ static int graph_draw_octopus_merge(struct git_graph *graph, struct strbuf *sb) { /* - * Here dashless_commits represents the number of parents - * which don't need to have dashes (because their edges fit - * neatly under the commit). + * Here dashless_commits represents the number of parents which don't + * need to have dashes (because their edges fit neatly under the + * commit). And dashful_commits are the remaining ones. */ const int dashless_commits = 2; - int col_num, i; - int num_dashes = - ((graph->num_parents - dashless_commits) * 2) - 1; - for (i = 0; i < num_dashes; i++) { - col_num = (i / 2) + dashless_commits + graph->commit_index; - strbuf_write_column(sb, &graph->new_columns[col_num], '-'); + int dashful_commits = graph->num_parents - dashless_commits; + + /* + * Usually, each parent gets its own column, like this: + * + * | *-. + * | |\ \ + * | | | * + * + * Sometimes the first parent goes into an existing column, like this: + * + * | *-. + * | |\ \ + * |/ / / + * + */ + int parent_in_existing_cols = graph->num_parents - + (graph->num_new_columns - graph->num_columns); + + /* + * Draw the dashes. We start in the column following the + * dashless_commits, but subtract out the parent which goes to an + * existing column: we've already counted that column in commit_index. + */ + int first_col = graph->commit_index + dashless_commits + - parent_in_existing_cols; + int i; + + for (i = 0; i < dashful_commits; i++) { + strbuf_write_column(sb, &graph->new_columns[i+first_col], '-'); + strbuf_write_column(sb, &graph->new_columns[i+first_col], + i == dashful_commits-1 ? '.' : '-'); } - col_num = (i / 2) + dashless_commits + graph->commit_index; - strbuf_write_column(sb, &graph->new_columns[col_num], '.'); - return num_dashes + 1; + return 2 * dashful_commits; } static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb) -- 2.11.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-09-02 0:34 ` Noam Postavsky @ 2018-09-08 16:13 ` Jeff King 2018-09-25 0:27 ` Noam Postavsky 0 siblings, 1 reply; 22+ messages in thread From: Jeff King @ 2018-09-08 16:13 UTC (permalink / raw) To: Noam Postavsky; +Cc: Johannes Sixt, Hemmo Nieminen, git On Sat, Sep 01, 2018 at 08:34:41PM -0400, Noam Postavsky wrote: > On 6 August 2018 at 17:26, Jeff King <peff@peff.net> wrote: > > > I suspect it still has a bug, which is that it is handling this > > first-parent-goes-left case, but probably gets the straight-parent case > > wrong. But at least in this form, I think it is obvious to see where > > that bug is (the "three" in the comment is not accurate in that latter > > case, and it should be two). > > Yes, thanks, it makes a lot more sense this way. I believe the > attached handles both parent types correctly. Great (and sorry for the delayed response). Let me see if I understand it... > diff --git a/graph.c b/graph.c > index e1f6d3bdd..478c86dfb 100644 > --- a/graph.c > +++ b/graph.c > @@ -848,21 +848,45 @@ static int graph_draw_octopus_merge(struct git_graph *graph, > struct strbuf *sb) > { > /* > - * Here dashless_commits represents the number of parents > - * which don't need to have dashes (because their edges fit > - * neatly under the commit). > + * Here dashless_commits represents the number of parents which don't > + * need to have dashes (because their edges fit neatly under the > + * commit). And dashful_commits are the remaining ones. > */ > const int dashless_commits = 2; Why is dashless commits always going to be 2? I thought at first that it was representing the merge commit itself, plus the line of development to the left. But the latter could be arbitrarily sized. But that's not what it is, because we handle that by using graph->commit_index in the iteration below. So I get that the merge commit itself does not need a dash. What's the other one? > + int dashful_commits = graph->num_parents - dashless_commits; OK, this makes sense. > + /* > + * Usually, each parent gets its own column, like this: > + * > + * | *-. > + * | |\ \ > + * | | | * > + * > + * Sometimes the first parent goes into an existing column, like this: > + * > + * | *-. > + * | |\ \ > + * |/ / / > + * > + */ > + int parent_in_existing_cols = graph->num_parents - > + (graph->num_new_columns - graph->num_columns); Ah, OK, this is the magic part: we compare num_new_columns versus num_columns to see which case we have. Makes sense. And this comment is very welcome to explain it visually. > + /* > + * Draw the dashes. We start in the column following the > + * dashless_commits, but subtract out the parent which goes to an > + * existing column: we've already counted that column in commit_index. > + */ > + int first_col = graph->commit_index + dashless_commits > + - parent_in_existing_cols; > + int i; OK, so we start at the commit_index, which makes sense. We skip past the dashless commits (which includes the merge itself, plus the other mystery one). And then we go back by the parents in existing columns, which I think is either 1 or 2. And I think that may be the root of my confusion. The other "dashless" commit is the parent we've already printed before hitting this function, the left-hand line that goes all the way down below where we print the other parents. So I think this is doing the right thing. I'm not sure if there's a better way to explain "dashless" or not. > + for (i = 0; i < dashful_commits; i++) { > + strbuf_write_column(sb, &graph->new_columns[i+first_col], '-'); > + strbuf_write_column(sb, &graph->new_columns[i+first_col], > + i == dashful_commits-1 ? '.' : '-'); > } And this loop is nice and simple now. Good. So I think this patch looks right. This is all sufficiently complex that we probably want to add something to the test suite. For reference, here's how I hacked up your original script to put more commits on the left: --- test-multiway-merge.sh.orig 2018-09-08 12:04:23.007468601 -0400 +++ test-multiway-merge.sh 2018-09-08 12:11:02.267750789 -0400 @@ -25,6 +25,11 @@ "$GIT" init +for base in 1 2 3 4; do + echo base-$base >foo + git add foo + git commit -m base-$base +done echo 0 > foo "$GIT" add foo "$GIT" commit -m 0 @@ -47,4 +52,10 @@ "$GIT" checkout m "$GIT" merge -m "merge a b $*" a b "$@" +sleep 1 +for i in 1 2 3 4; do + git checkout -b $i-prime master~$i + git commit --allow-empty -m side-$i +done + valgrind "$GIT" log --oneline --graph --all Then running it as "test-multiway-merge d e f" gives a nice wide graph that should show any off-by-one mistakes. We should be able to do away with the "sleep" in a real test if we make use of test_commit(), which advances GIT_COMMITTER_DATE by one second for each commit. Do you feel comfortable trying to add something to the test suite for this? -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-09-08 16:13 ` Jeff King @ 2018-09-25 0:27 ` Noam Postavsky 2018-10-03 0:09 ` Noam Postavsky 2018-10-03 4:24 ` Jeff King 0 siblings, 2 replies; 22+ messages in thread From: Noam Postavsky @ 2018-09-25 0:27 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Hemmo Nieminen, git [-- Attachment #1: Type: text/plain, Size: 721 bytes --] On Sat, 8 Sep 2018 at 12:13, Jeff King <peff@peff.net> wrote: > Great (and sorry for the delayed response). No problem, I know it's not the most urgent bug ever :) > So I think this is doing the right thing. I'm not sure if there's a > better way to explain "dashless" or not. I've updated the comments and renamed a few variables, see if that helps. > Do you feel comfortable trying to add something to the test suite for > this? Um, sort of. I managed to recast my script into the framework of the other tests (see attached t4299-octopus.sh); it seems like it should go into t4202-log.sh, but it's not clear to me how I can do this without breaking all the other tests which expect a certain sequence of commits. [-- Attachment #2: t4299-octopus.sh --] [-- Type: application/x-sh, Size: 3887 bytes --] [-- Attachment #3: v4-0001-log-Fix-coloring-of-certain-octupus-merge-shapes.patch --] [-- Type: text/x-diff, Size: 3136 bytes --] From ade526d32f692cae06000bb413ff29dad3f6109e Mon Sep 17 00:00:00 2001 From: Noam Postavsky <npostavs@users.sourceforge.net> Date: Sat, 1 Sep 2018 20:07:16 -0400 Subject: [PATCH v4] log: Fix coloring of certain octupus merge shapes For octopus merges where the first parent edge immediately merges into the next column to the left: | *-. | |\ \ |/ / / then the number of columns should be one less than the usual case: | *-. | |\ \ | | | * Also refactor the code to iterate over columns rather than dashes, building from an initial patch suggestion by Jeff King. Signed-off-by: Noam Postavsky <npostavs@users.sourceforge.net> --- graph.c | 56 +++++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 15 deletions(-) diff --git a/graph.c b/graph.c index e1f6d3bdd..a3366f6da 100644 --- a/graph.c +++ b/graph.c @@ -842,27 +842,53 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) } /* - * Draw an octopus merge and return the number of characters written. + * Draw the horizontal dashes of an octopus merge and return the number of + * characters written. */ static int graph_draw_octopus_merge(struct git_graph *graph, struct strbuf *sb) { /* - * Here dashless_commits represents the number of parents - * which don't need to have dashes (because their edges fit - * neatly under the commit). - */ - const int dashless_commits = 2; - int col_num, i; - int num_dashes = - ((graph->num_parents - dashless_commits) * 2) - 1; - for (i = 0; i < num_dashes; i++) { - col_num = (i / 2) + dashless_commits + graph->commit_index; - strbuf_write_column(sb, &graph->new_columns[col_num], '-'); + * Here dashless_parents represents the number of parents which don't + * need to have dashes (the edges labeled "0" and "1"). And + * dashful_parents are the remaining ones. + * + * | *---. + * | |\ \ \ + * | | | | | + * x 0 1 2 3 + * + */ + const int dashless_parents = 2; + int dashful_parents = graph->num_parents - dashless_parents; + + /* + * Usually, each parent gets its own column, like the diagram above, but + * sometimes the first parent goes into an existing column, like this: + * + * | *---. + * | |\ \ \ + * |/ / / / + * x 0 1 2 + * + * In which case there will be more parents than the delta of columns. + */ + int delta_cols = (graph->num_new_columns - graph->num_columns); + int parent_in_old_cols = graph->num_parents - delta_cols; + + /* + * In both cases, commit_index corresponds to the edge labeled "0". + */ + int first_col = graph->commit_index + dashless_parents + - parent_in_old_cols; + + int i; + for (i = 0; i < dashful_parents; i++) { + strbuf_write_column(sb, &graph->new_columns[i+first_col], '-'); + strbuf_write_column(sb, &graph->new_columns[i+first_col], + i == dashful_parents-1 ? '.' : '-'); } - col_num = (i / 2) + dashless_commits + graph->commit_index; - strbuf_write_column(sb, &graph->new_columns[col_num], '.'); - return num_dashes + 1; + return 2 * dashful_parents; } static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb) -- 2.11.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-09-25 0:27 ` Noam Postavsky @ 2018-10-03 0:09 ` Noam Postavsky 2018-10-03 4:24 ` Jeff King 1 sibling, 0 replies; 22+ messages in thread From: Noam Postavsky @ 2018-10-03 0:09 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Hemmo Nieminen, git On Mon, 24 Sep 2018 at 20:27, Noam Postavsky <npostavs@users.sourceforge.net> wrote: > > On Sat, 8 Sep 2018 at 12:13, Jeff King <peff@peff.net> wrote: > > > Great (and sorry for the delayed response). > > No problem, I know it's not the most urgent bug ever :) Ping. :) > I managed to recast my script into the framework of the > other tests (see attached t4299-octopus.sh); it seems like it should > go into t4202-log.sh, but it's not clear to me how I can do this > without breaking all the other tests which expect a certain sequence > of commits. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-09-25 0:27 ` Noam Postavsky 2018-10-03 0:09 ` Noam Postavsky @ 2018-10-03 4:24 ` Jeff King 2018-10-03 22:32 ` Noam Postavsky 1 sibling, 1 reply; 22+ messages in thread From: Jeff King @ 2018-10-03 4:24 UTC (permalink / raw) To: Noam Postavsky; +Cc: Johannes Sixt, Hemmo Nieminen, git On Mon, Sep 24, 2018 at 08:27:47PM -0400, Noam Postavsky wrote: > > So I think this is doing the right thing. I'm not sure if there's a > > better way to explain "dashless" or not. > > I've updated the comments and renamed a few variables, see if that helps. Yeah, I really like your explanations/diagrams in the comments. It makes the logic very clear. > > Do you feel comfortable trying to add something to the test suite for > > this? > > Um, sort of. I managed to recast my script into the framework of the > other tests (see attached t4299-octopus.sh); it seems like it should > go into t4202-log.sh, but it's not clear to me how I can do this > without breaking all the other tests which expect a certain sequence > of commits. It's OK to start a new script if you have some tricky or complicated setup. Probably it ought to be t4214-log-graph-octopus to keep it near the other log tests. And it should be added in the actual patch, but I assume you just kept it out here since you weren't sure where to put it yet. I'll try to comment on the test script itself. > #!/bin/sh > > test_description='git log' This should actually describe what's going on in the test. Usually a one-sentence is OK, but I think it might be good to specifically mention that we're handling this special octopus case. > . ./test-lib.sh > > make_octopus_merge () { > for i ; do > git checkout master -b $i || return $? > # Make tag name different from branch name. > test_commit $i $i $i tag$i || return $? > done Please use: for i in "$@" which is a bit less subtle (there's also only one caller of this function, so it could be inlined; note that it's OK to use "return" in a test_expect block). Why do we need the tag name to be different? > git checkout 1 -b merge && This is assuming we just made a branch called "1", but that's one of the arguments. Probably this should be "$1" (or the whole thing should just be inlined so it is clear that what the set of parameters we expect is). > test_tick && > git merge -m octopus-merge "$@" Good use of test_tick so that we have predictable traversal ordering. > test_expect_success 'set up merge history' ' > test_commit initial && > make_octopus_merge 1 2 3 4 && > git checkout 1 -b L && > test_commit left > ' It might actually be worth setting up the uncolored expect file as part of this, since it so neatly diagrams the graph you're trying to produce. I.e., something like (completely untested; note that the leading indentation is all tabs, which will be removed by the "<<-" operator): test_expect_success 'set up merge history' ' # This is the graph we expect to generate here. cat >expect.uncolored <<-\EOF && * left | *---. octopus-merge | |\ \ \ |/ / / / | | | * 4 | | * | 3 | | |/ | * | 2 | |/ * | 1 |/ * initial EOF for i in 1 2 3 4; do git checkout -b $i $master || return $? # Make tag name different from branch name. test_commit $i $i $i tag$i || return $? done && git checkout -b merge 1 && test_tick && git merge -m octopus-merge 1 2 3 4 ' > cat > expect.colors <<\EOF A few style bits: we prefer to keep even setup steps like this inside a test_expect block (though you may see some very old tests which have not been fixed yet). Also, we omit the space after ">". > * left > <RED>|<RESET> *<BLUE>-<RESET><BLUE>-<RESET><MAGENTA>-<RESET><MAGENTA>.<RESET> octopus-merge > <RED>|<RESET> <RED>|<RESET><YELLOW>\<RESET> <BLUE>\<RESET> <MAGENTA>\<RESET> > <RED>|<RESET><RED>/<RESET> <YELLOW>/<RESET> <BLUE>/<RESET> <MAGENTA>/<RESET> > <RED>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 4 > <RED>|<RESET> <YELLOW>|<RESET> * <MAGENTA>|<RESET> 3 > <RED>|<RESET> <YELLOW>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET> > <RED>|<RESET> * <MAGENTA>|<RESET> 2 > <RED>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET> > * <MAGENTA>|<RESET> 1 > <MAGENTA>|<RESET><MAGENTA>/<RESET> > * initial Yikes. :) This one is pretty hard to read. I'm not sure if there's a good alternative. If you pipe the output of test_decode through this: sed ' s/<RED>.<RESET>/R/g; s/<BLUE>.<RESET>/B/g; s/<MAGENTA>.<RESET>/M/g; s/<YELLOW>.<RESET>/Y/g; ' you get this: * left R *BBMM octopus-merge R RY B M RR Y B M R Y B * 4 R Y * M 3 R Y MM R * M 2 R MM * M 1 MM * initial which is admittedly pretty horrible, too, but at least resembles a graph. I dunno. I'm also not thrilled that we depend on the exact sequence of default colors, but I suspect it's not the first time. And it wouldn't be too hard to update it if that default changes. > test_expect_success 'log --graph with tricky octopus merge' ' > git log --color=always --graph --date-order --pretty=tformat:%s --all | > test_decode_color | sed "s/ *\$//" >actual && Try not to put "git" on the left-hand side of a pipe, since it means we'll miss its exit code (and especially we'd miss its death due to ASan or Valgrind problems, which I think was one of the major ways of detecting the original problem). So: git log ... >actual.raw && test_decode_color <actual.raw | sed ... >actual && test_cmp expect.colors actual > cat > expect <<\EOF > * left > | *---. octopus-merge > | |\ \ \ > |/ / / / > | | | * 4 > | | * | 3 > | | |/ > | * | 2 > | |/ > * | 1 > |/ > * initial > EOF This is the expect output that I suggested showing earlier. :) > test_expect_success 'log --graph with tricky octopus merge' ' > debug git log --color=never --graph --date-order --pretty=tformat:%s --all | > sed "s/ *\$//" >actual && Leftover "debug" cruft? The same pipe comment applies as above. > test_done > test_done Two dones; we exit after the first one (so everything after this is ignored). I think it's OK to have a dedicated script for even these two tests, if it makes things easier to read. However, would we also want to test the octopus without the problematic graph here? I think if we just omit "left" we get that, don't we? -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-10-03 4:24 ` Jeff King @ 2018-10-03 22:32 ` Noam Postavsky 2018-10-09 4:51 ` Jeff King 0 siblings, 1 reply; 22+ messages in thread From: Noam Postavsky @ 2018-10-03 22:32 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Hemmo Nieminen, git [-- Attachment #1: Type: text/plain, Size: 3358 bytes --] On Wed, 3 Oct 2018 at 00:24, Jeff King <peff@peff.net> wrote: > Yeah, I really like your explanations/diagrams in the comments. It makes > the logic very clear. Ok good, I did have the feeling that the logic became actually clearer to me after I wrestled with the test code, so I think this means I didn't just imagine that. :) > (there's also only one caller of this > function, so it could be inlined; note that it's OK to use "return" in a > test_expect block). Oh, I think had run into some trouble with the test runner complaining about a broken &&-chain, but it seems to work fine now (perhaps I missing the && somewhere else that I fixed later). > Why do we need the tag name to be different? Otherwise the 'git checkout' command complains about an ambiguous ref (added that to the comment). > > git checkout 1 -b merge && > > This is assuming we just made a branch called "1", but that's one of the > arguments. Probably this should be "$1" (or the whole thing should just > be inlined so it is clear that what the set of parameters we expect is). Oops, right. I've inlined it. > It might actually be worth setting up the uncolored expect file as part > of this, since it so neatly diagrams the graph you're trying to produce. > > I.e., something like (completely untested; note that the leading > indentation is all tabs, which will be removed by the "<<-" operator): Yup, works (I again had run into some problems with &&-chaining earlier, but now it works fine) > > * left > > <RED>|<RESET> *<BLUE>-<RESET><BLUE>-<RESET><MAGENTA>-<RESET><MAGENTA>.<RESET> octopus-merge [...] > > Yikes. :) This one is pretty hard to read. I'm not sure if there's a > good alternative. If you pipe the output of test_decode through > this: > > sed ' > s/<RED>.<RESET>/R/g; [...] > you get this: > > * left > R *BBMM octopus-merge > R RY B M [...] > which is admittedly pretty horrible, too, but at least resembles a > graph. I dunno. Yeah, but it's lossy, so it doesn't seem usable for the test. Maybe doubling up some characters? ** left R| **B-B-M-M. octopus-merge R| R|Y\ B\ M\ R|R/ Y/ B/ M/ R| Y| B| ** 4 R| Y| ** M| 3 R| Y| M|M/ R| ** M| 2 R| M|M/ ** M| 1 M|M/ ** initial > I'm also not thrilled that we depend on the exact sequence of default > colors, but I suspect it's not the first time. And it wouldn't be too > hard to update it if that default changes. Well, it's easy enough to set the colors explicitly. After doing this I noticed that green seems to be skipped. Not sure if that's a bug or not. > Try not to put "git" on the left-hand side of a pipe, since it means > we'll miss its exit code Ok. > Leftover "debug" cruft? > > The same pipe comment applies as above. > > > test_done > > test_done > > Two dones; we exit after the first one (so everything after this is > ignored). Oops, yeah, this script was still a bit of a rough draft. > I think it's OK to have a dedicated script for even these two tests, if > it makes things easier to read. However, would we also want to test the > octopus without the problematic graph here? I think if we just omit > "left" we get that, don't we? t4202-log.sh already does test a "normal" octopus merge (starting around line 615, search for "octopus-a"). But that is only a 3-parent merge. And adding another test is easy enough. [-- Attachment #2: v5-0001-log-Fix-coloring-of-certain-octupus-merge-shapes.patch --] [-- Type: text/x-diff, Size: 6749 bytes --] From cd9415b524357c2c8b9b20a63032c94e01d46a15 Mon Sep 17 00:00:00 2001 From: Noam Postavsky <npostavs@users.sourceforge.net> Date: Sat, 1 Sep 2018 20:07:16 -0400 Subject: [PATCH v5] log: Fix coloring of certain octupus merge shapes For octopus merges where the first parent edge immediately merges into the next column to the left: | *-. | |\ \ |/ / / then the number of columns should be one less than the usual case: | *-. | |\ \ | | | * Also refactor the code to iterate over columns rather than dashes, building from an initial patch suggestion by Jeff King. Signed-off-by: Noam Postavsky <npostavs@users.sourceforge.net> --- graph.c | 56 +++++++++++++++++------- t/t4214-log-graph-octopus.sh | 102 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 15 deletions(-) create mode 100755 t/t4214-log-graph-octopus.sh diff --git a/graph.c b/graph.c index e1f6d3bdd..a3366f6da 100644 --- a/graph.c +++ b/graph.c @@ -842,27 +842,53 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb) } /* - * Draw an octopus merge and return the number of characters written. + * Draw the horizontal dashes of an octopus merge and return the number of + * characters written. */ static int graph_draw_octopus_merge(struct git_graph *graph, struct strbuf *sb) { /* - * Here dashless_commits represents the number of parents - * which don't need to have dashes (because their edges fit - * neatly under the commit). - */ - const int dashless_commits = 2; - int col_num, i; - int num_dashes = - ((graph->num_parents - dashless_commits) * 2) - 1; - for (i = 0; i < num_dashes; i++) { - col_num = (i / 2) + dashless_commits + graph->commit_index; - strbuf_write_column(sb, &graph->new_columns[col_num], '-'); + * Here dashless_parents represents the number of parents which don't + * need to have dashes (the edges labeled "0" and "1"). And + * dashful_parents are the remaining ones. + * + * | *---. + * | |\ \ \ + * | | | | | + * x 0 1 2 3 + * + */ + const int dashless_parents = 2; + int dashful_parents = graph->num_parents - dashless_parents; + + /* + * Usually, each parent gets its own column, like the diagram above, but + * sometimes the first parent goes into an existing column, like this: + * + * | *---. + * | |\ \ \ + * |/ / / / + * x 0 1 2 + * + * In which case there will be more parents than the delta of columns. + */ + int delta_cols = (graph->num_new_columns - graph->num_columns); + int parent_in_old_cols = graph->num_parents - delta_cols; + + /* + * In both cases, commit_index corresponds to the edge labeled "0". + */ + int first_col = graph->commit_index + dashless_parents + - parent_in_old_cols; + + int i; + for (i = 0; i < dashful_parents; i++) { + strbuf_write_column(sb, &graph->new_columns[i+first_col], '-'); + strbuf_write_column(sb, &graph->new_columns[i+first_col], + i == dashful_parents-1 ? '.' : '-'); } - col_num = (i / 2) + dashless_commits + graph->commit_index; - strbuf_write_column(sb, &graph->new_columns[col_num], '.'); - return num_dashes + 1; + return 2 * dashful_parents; } static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb) diff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh new file mode 100755 index 000000000..dab96c89a --- /dev/null +++ b/t/t4214-log-graph-octopus.sh @@ -0,0 +1,102 @@ +#!/bin/sh + +test_description='git log --graph of skewed left octopus merge.' + +. ./test-lib.sh + +test_expect_success 'set up merge history' ' + cat >expect.uncolored <<-\EOF && + * left + | *---. octopus-merge + | |\ \ \ + |/ / / / + | | | * 4 + | | * | 3 + | | |/ + | * | 2 + | |/ + * | 1 + |/ + * initial + EOF + cat >expect.colors <<-\EOF && + * left + <RED>|<RESET> *<BLUE>-<RESET><BLUE>-<RESET><MAGENTA>-<RESET><MAGENTA>.<RESET> octopus-merge + <RED>|<RESET> <RED>|<RESET><YELLOW>\<RESET> <BLUE>\<RESET> <MAGENTA>\<RESET> + <RED>|<RESET><RED>/<RESET> <YELLOW>/<RESET> <BLUE>/<RESET> <MAGENTA>/<RESET> + <RED>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 4 + <RED>|<RESET> <YELLOW>|<RESET> * <MAGENTA>|<RESET> 3 + <RED>|<RESET> <YELLOW>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET> + <RED>|<RESET> * <MAGENTA>|<RESET> 2 + <RED>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET> + * <MAGENTA>|<RESET> 1 + <MAGENTA>|<RESET><MAGENTA>/<RESET> + * initial + EOF + test_commit initial && + for i in 1 2 3 4 ; do + git checkout master -b $i || return $? + # Make tag name different from branch name, to avoid + # ambiguity error when calling checkout. + test_commit $i $i $i tag$i || return $? + done && + git checkout 1 -b merge && + test_tick && + git merge -m octopus-merge 1 2 3 4 && + git checkout 1 -b L && + test_commit left +' + +test_expect_success 'log --graph with tricky octopus merge with colors' ' + test_config log.graphColors red,green,yellow,blue,magenta,cyan && + git log --color=always --graph --date-order --pretty=tformat:%s --all >actual.colors.raw && + test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors && + test_cmp expect.colors actual.colors +' + +test_expect_success 'log --graph with tricky octopus merge, no color' ' + git log --color=never --graph --date-order --pretty=tformat:%s --all >actual.raw && + sed "s/ *\$//" actual.raw >actual && + test_cmp expect.uncolored actual +' + +# Repeat the previous two tests with "normal" octopus merge (i.e., +# without the first parent skewing to the "left" branch column). + +test_expect_success 'log --graph with normal octopus merge, no color' ' + cat >expect.uncolored <<-\EOF && + *---. octopus-merge + |\ \ \ + | | | * 4 + | | * | 3 + | | |/ + | * | 2 + | |/ + * | 1 + |/ + * initial + EOF + git log --color=never --graph --date-order --pretty=tformat:%s merge >actual.raw && + sed "s/ *\$//" actual.raw >actual && + test_cmp expect.uncolored actual +' + +test_expect_success 'log --graph with normal octopus merge with colors' ' + cat >expect.colors <<-\EOF && + *<YELLOW>-<RESET><YELLOW>-<RESET><BLUE>-<RESET><BLUE>.<RESET> octopus-merge + <RED>|<RESET><GREEN>\<RESET> <YELLOW>\<RESET> <BLUE>\<RESET> + <RED>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET> * 4 + <RED>|<RESET> <GREEN>|<RESET> * <BLUE>|<RESET> 3 + <RED>|<RESET> <GREEN>|<RESET> <BLUE>|<RESET><BLUE>/<RESET> + <RED>|<RESET> * <BLUE>|<RESET> 2 + <RED>|<RESET> <BLUE>|<RESET><BLUE>/<RESET> + * <BLUE>|<RESET> 1 + <BLUE>|<RESET><BLUE>/<RESET> + * initial + EOF + test_config log.graphColors red,green,yellow,blue,magenta,cyan && + git log --color=always --graph --date-order --pretty=tformat:%s merge >actual.colors.raw && + test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors && + test_cmp expect.colors actual.colors +' +test_done -- 2.11.0 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-10-03 22:32 ` Noam Postavsky @ 2018-10-09 4:51 ` Jeff King 2018-10-10 0:42 ` Noam Postavsky 0 siblings, 1 reply; 22+ messages in thread From: Jeff King @ 2018-10-09 4:51 UTC (permalink / raw) To: Noam Postavsky; +Cc: Johannes Sixt, Hemmo Nieminen, git On Wed, Oct 03, 2018 at 06:32:06PM -0400, Noam Postavsky wrote: > > which is admittedly pretty horrible, too, but at least resembles a > > graph. I dunno. > > Yeah, but it's lossy, so it doesn't seem usable for the test. Maybe > doubling up some characters? > > ** left > R| **B-B-M-M. octopus-merge > R| R|Y\ B\ M\ > R|R/ Y/ B/ M/ > R| Y| B| ** 4 > R| Y| ** M| 3 > R| Y| M|M/ > R| ** M| 2 > R| M|M/ > ** M| 1 > M|M/ > ** initial Yeah, I tried something similar, but it's hard to read as a graph since the alignment is lost between lines. I agree the single-char version is lossy, but I think in combination with checking the literal, uncolored version, we'd be OK. However, it may be best to just leave the original verbose version you had. It's hard to read and to modify, but we don't plan for people to do that very often. And it's at least simple. > > I'm also not thrilled that we depend on the exact sequence of default > > colors, but I suspect it's not the first time. And it wouldn't be too > > hard to update it if that default changes. > > Well, it's easy enough to set the colors explicitly. After doing this > I noticed that green seems to be skipped. Not sure if that's a bug or > not. Hmm, yeah, that is weird. I think it's an artifact of the way we increment the color selector, though, and not related to your patch (the same thing happens before your fix, as well). > > I think it's OK to have a dedicated script for even these two tests, if > > it makes things easier to read. However, would we also want to test the > > octopus without the problematic graph here? I think if we just omit > > "left" we get that, don't we? > > t4202-log.sh already does test a "normal" octopus merge (starting > around line 615, search for "octopus-a"). But that is only a 3-parent > merge. And adding another test is easy enough. > [...] Thanks, what you have here looks good. > From cd9415b524357c2c8b9b20a63032c94e01d46a15 Mon Sep 17 00:00:00 2001 > From: Noam Postavsky <npostavs@users.sourceforge.net> > Date: Sat, 1 Sep 2018 20:07:16 -0400 > Subject: [PATCH v5] log: Fix coloring of certain octupus merge shapes This whole version looks good to me. "git am" is supposed to understand attachments, but it seems to want to apply our whole conversation as the commit message. You may want to repost one more time with this subject in the email subject line to fix that and to get the maintainer's attention. Feel free to add my: Reviewed-by: Jeff King <peff@peff.net> after your signoff. Thanks for sticking with this topic! -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2018-10-09 4:51 ` Jeff King @ 2018-10-10 0:42 ` Noam Postavsky 0 siblings, 0 replies; 22+ messages in thread From: Noam Postavsky @ 2018-10-10 0:42 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Sixt, Hemmo Nieminen, git > This whole version looks good to me. "git am" is supposed to understand > attachments, but it seems to want to apply our whole conversation as the > commit message. > > You may want to repost one more time with this subject in the email > subject line to fix that and to get the maintainer's attention. Feel > free to add my: > > Reviewed-by: Jeff King <peff@peff.net> > > after your signoff. Resent with git send-email. https://marc.info/?l=git&m=153913190617067&w=2 > Thanks for sticking with this topic! Thank you for all your patient reviewing! ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2016-05-17 19:45 ` Jeff King 2016-05-17 19:51 ` Jeff King @ 2016-05-17 20:02 ` Johannes Sixt 2016-05-17 21:57 ` Jeff King 1 sibling, 1 reply; 22+ messages in thread From: Johannes Sixt @ 2016-05-17 20:02 UTC (permalink / raw) To: Jeff King; +Cc: Noam Postavsky, git Am 17.05.2016 um 21:45 schrieb Jeff King: > On Tue, May 17, 2016 at 09:07:33PM +0200, Johannes Sixt wrote: > >> Am 15.05.2016 um 15:05 schrieb Noam Postavsky: >>> With a certain topology involving an octopus merge, git log --graph >>> --oneline --all --color=never produces output which includes some ANSI >>> escape code coloring. Attached is a script to reproduce the problem >>> (creates a git repository in subdir log-format-test), along with >>> sample graph and valgrind output (indicates some unitialialized memory >>> access). >>> >>> I've observed the problem with Windows git versions 2.7.0, 2.5.3. >>> I've NOT observed it with 1.9.5, >>> >>> On GNU/Linux the symptom only appears when running with valgrind, I >>> tried versions >>> 2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes from) >>> >> >> Sorry, I can't reproduce your observation. I ran the script you provided >> with HOME=$PWD and a minimal .gitconfig that only sets user.email. But >> valgrind is happy with both 2.8.0 and v2.8.2-402-gedec370 on my Linux box. > > Interesting. It replicates out of the box for me. "Out of the box" are the magic words. I usually compile with -O0, which doesn't trigger the valgrind report. When I compile with a 3.x based gcc on Windows, I see these warnings: CC color.o color.c: In function 'color_parse_mem': color.c:203: warning: 'c.value' may be used uninitialized in this function color.c:203: warning: 'c.blue' may be used uninitialized in this function color.c:203: warning: 'c.green' may be used uninitialized in this function color.c:203: warning: 'c.red' may be used uninitialized in this function (which triggered my curiosity in this bug report). But they seem to be unrelated and are most likely false positives. -- Hannes ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never 2016-05-17 20:02 ` Johannes Sixt @ 2016-05-17 21:57 ` Jeff King 0 siblings, 0 replies; 22+ messages in thread From: Jeff King @ 2016-05-17 21:57 UTC (permalink / raw) To: Johannes Sixt; +Cc: Noam Postavsky, git On Tue, May 17, 2016 at 10:02:22PM +0200, Johannes Sixt wrote: > > Interesting. It replicates out of the box for me. > > "Out of the box" are the magic words. I usually compile with -O0, which > doesn't trigger the valgrind report. Heh, I meant that Noam's test worked out of the box. I also build with -O0. I was able to replicate with different optimization levels. I think the interesting thing here is actually the libc (and therefore possibly your valgrind version). I tried compiling with ASAN and get a color value of "48830". But no ASAN warning! I think what is happening is that we over-allocate the new_columns array based on a power of 2, but only initialize up to num_new_columns. So the off-by-one accesses heap memory that is allocated but which we have never written to. > When I compile with a 3.x based gcc on Windows, I see these warnings: > > CC color.o > color.c: In function 'color_parse_mem': > color.c:203: warning: 'c.value' may be used uninitialized in this function > color.c:203: warning: 'c.blue' may be used uninitialized in this function > color.c:203: warning: 'c.green' may be used uninitialized in this function > color.c:203: warning: 'c.red' may be used uninitialized in this function > > (which triggered my curiosity in this bug report). But they seem to be > unrelated and are most likely false positives. Yeah, I think that's unrelated. I'd be highly distrustful of -Wuninitialized in gcc 3.x. We had to mark quite a few false positives back then, that were later corrected in the 4.x series. -Peff ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-10-10 0:42 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-15 13:05 [BUG] A part of an edge from an octopus merge gets colored, even with --color=never Noam Postavsky 2016-05-17 19:07 ` Johannes Sixt 2016-05-17 19:45 ` Jeff King 2016-05-17 19:51 ` Jeff King 2016-05-17 19:55 ` Jeff King 2016-05-20 22:12 ` Noam Postavsky 2018-06-23 21:45 ` Noam Postavsky 2018-06-25 16:23 ` Jeff King 2018-06-30 12:47 ` Noam Postavsky 2018-08-06 18:34 ` Noam Postavsky 2018-08-06 21:28 ` Jeff King 2018-08-06 21:26 ` Jeff King 2018-09-02 0:34 ` Noam Postavsky 2018-09-08 16:13 ` Jeff King 2018-09-25 0:27 ` Noam Postavsky 2018-10-03 0:09 ` Noam Postavsky 2018-10-03 4:24 ` Jeff King 2018-10-03 22:32 ` Noam Postavsky 2018-10-09 4:51 ` Jeff King 2018-10-10 0:42 ` Noam Postavsky 2016-05-17 20:02 ` Johannes Sixt 2016-05-17 21:57 ` Jeff King
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).