On Wed, 3 Oct 2018 at 00:24, Jeff King <peff@xxxxxxxx> 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.
From cd9415b524357c2c8b9b20a63032c94e01d46a15 Mon Sep 17 00:00:00 2001 From: Noam Postavsky <npostavs@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> --- 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