| Message ID | 12c0916cb1ef033f917dc065cc1f18c0477296b8.1570724021.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve the readability of log --graph output | expand |
On 10/10/2019 12:13 PM, James Coglan via GitGitGadget wrote: > From: James Coglan <jcoglan@gmail.com> > > Currently, when we display a merge whose first parent is already present > in a column to the left of the merge commit, we display the first parent > as a veritcal pipe `|` in the GRAPH_POST_MERGE line and then immediately > enter the GRAPH_COLLAPSING state. The first-parent line tracks to the > left and all the other parent lines follow it; this creates a "kink" in > those lines: > > | *---. > | |\ \ \ > |/ / / / > | | | * > > This change tidies the display of such commits such that if the first > parent appears to the left of the merge, we render it as a `/` and the > second parent as a `|`. This reduces the horizontal and vertical space > needed to render the merge, and makes the resulting lines easier to > read. > > | *-. > |/|\ \ > | | | * > > If the first parent is separated from the merge by several columns, a > horizontal line is drawn in a similar manner to how the GRAPH_COLLAPSING > state displays the line. > > | | | *-. > | |_|/|\ \ > |/| | | | * > > This effect is applied to both "normal" two-parent merges, and to > octopus merges. It also reduces the vertical space needed for pre-commit > lines, as the merge occupies one less column than usual. > > Before: After: > > | * | * > | |\ | |\ > | | \ | * \ > | | \ |/|\ \ > | *-. \ > | |\ \ \ > Thank you for adding these careful diagrams both to the message and the code. These concepts are hard to track without a visual aid. [snip] > +++ b/t/t4215-log-skewed-merges.sh > @@ -0,0 +1,42 @@ > +#!/bin/sh > + > +test_description='git log --graph of skewed merges' > + > +. ./test-lib.sh > + > +test_expect_success 'setup left-skewed merge' ' Could you skew this example to include a left-skewed octopus merge (and use fewer Git processes) with the following: git checkout --orphan _a && test_commit A && git switch -c _b _a && test_commit B && git switch -c _c _a && test_commit C && git switch -c _d _a && test_commit D && git switch -c _e _b && git merge --no-ff _c _d E && git switch -c _f _a && git merge --no-ff _d -m F && git checkout _a && git merge --no-ff _b _c _e _f -m G and I think the resulting output will be: *-----. G |\ \ \ \ | | | | * F | |_|_|/| |/| | | | | | | * | E | |_|/|\| |/| | | | | | |/ * D | |_|__/ |/| | | | * C | |/ |/| | * B |/ * A
On 10/10/2019 18:19, Derrick Stolee wrote: > On 10/10/2019 12:13 PM, James Coglan via GitGitGadget wrote: >> +++ b/t/t4215-log-skewed-merges.sh >> @@ -0,0 +1,42 @@ >> +#!/bin/sh >> + >> +test_description='git log --graph of skewed merges' >> + >> +. ./test-lib.sh >> + >> +test_expect_success 'setup left-skewed merge' ' > > > Could you skew this example to include a left-skewed octopus merge > (and use fewer Git processes) with the following: > > git checkout --orphan _a && test_commit A && > git switch -c _b _a && test_commit B && > git switch -c _c _a && test_commit C && > git switch -c _d _a && test_commit D && git switch -c _e _b && git merge --no-ff _c _d E && > git switch -c _f _a && git merge --no-ff _d -m F && git checkout _a && git merge --no-ff _b _c _e _f -m G > and I think the resulting output will be: > > *-----. G > |\ \ \ \ > | | | | * F > | |_|_|/| > |/| | | | > | | | * | E > | |_|/|\| > |/| | | | > | | |/ * D > | |_|__/ > |/| | > | | * C > | |/ > |/| > | * B > |/ > * A At this point in the history, commit E won't render like that -- this is before the change that flattens edges that fuse with the merge's last parent. I think the display of this history at this point will be: *-----. G |\ \ \ \ | | | | * F | |_|_|/| |/| | | | | | | * | E | |_|/|\ \ |/| |/ / / | | | | / | | | |/ | | | * D | |_|/ |/| | | | * C | |/ |/| | * B |/ * A Is there a particular reason for wanting to include this test case? What particular combination of states is it designed to test? (My guess is that it includes an octopus merge where the original test does not.) I'd be happy to add it at the appropriate point in the history if it's adding coverage not provided by the other tests.
On 10/11/2019 12:50 PM, James Coglan wrote: > On 10/10/2019 18:19, Derrick Stolee wrote: >> On 10/10/2019 12:13 PM, James Coglan via GitGitGadget wrote: >>> +++ b/t/t4215-log-skewed-merges.sh >>> @@ -0,0 +1,42 @@ >>> +#!/bin/sh >>> + >>> +test_description='git log --graph of skewed merges' >>> + >>> +. ./test-lib.sh >>> + >>> +test_expect_success 'setup left-skewed merge' ' >> >> >> Could you skew this example to include a left-skewed octopus merge >> (and use fewer Git processes) with the following: >> >> git checkout --orphan _a && test_commit A && >> git switch -c _b _a && test_commit B && >> git switch -c _c _a && test_commit C && >> git switch -c _d _a && test_commit D && git switch -c _e _b && git merge --no-ff _c _d E && >> git switch -c _f _a && git merge --no-ff _d -m F && git checkout _a && git merge --no-ff _b _c _e _f -m G >> and I think the resulting output will be: >> >> *-----. G >> |\ \ \ \ >> | | | | * F >> | |_|_|/| >> |/| | | | >> | | | * | E >> | |_|/|\| >> |/| | | | >> | | |/ * D >> | |_|__/ >> |/| | >> | | * C >> | |/ >> |/| >> | * B >> |/ >> * A > > At this point in the history, commit E won't render like that -- this is before the change that flattens edges that fuse with the merge's last parent. I think the display of this history at this point will be: > > *-----. G > |\ \ \ \ > | | | | * F > | |_|_|/| > |/| | | | > | | | * | E > | |_|/|\ \ > |/| |/ / / > | | | | / > | | | |/ > | | | * D > | |_|/ > |/| | > | | * C > | |/ > |/| > | * B > |/ > * A > > Is there a particular reason for wanting to include this test case? What particular combination of states is it designed to test? (My guess is that it includes an octopus merge where the original test does not.) I'd be happy to add it at the appropriate point in the history if it's adding coverage not provided by the other tests. Thanks for correcting my test case. It also helps that you would show the change in behavior in your later commits. My reason to include this test is because it includes a regular merge and an octopus merge, both of which have a skewed render. Many times logic that applies to a normal merge breaks with octopus merges, so I try to include them whenever possible. Thanks, -Stolee
On 12/10/2019 02:36, Derrick Stolee wrote: > On 10/11/2019 12:50 PM, James Coglan wrote: >> On 10/10/2019 18:19, Derrick Stolee wrote: >>> On 10/10/2019 12:13 PM, James Coglan via GitGitGadget wrote: >>>> +++ b/t/t4215-log-skewed-merges.sh >>>> @@ -0,0 +1,42 @@ >>>> +#!/bin/sh >>>> + >>>> +test_description='git log --graph of skewed merges' >>>> + >>>> +. ./test-lib.sh >>>> + >>>> +test_expect_success 'setup left-skewed merge' ' >>> >>> >>> Could you skew this example to include a left-skewed octopus merge >>> (and use fewer Git processes) with the following: >>> >>> git checkout --orphan _a && test_commit A && >>> git switch -c _b _a && test_commit B && >>> git switch -c _c _a && test_commit C && >>> git switch -c _d _a && test_commit D && git switch -c _e _b && git merge --no-ff _c _d E && >>> git switch -c _f _a && git merge --no-ff _d -m F && git checkout _a && git merge --no-ff _b _c _e _f -m G >>> and I think the resulting output will be: >>> >>> *-----. G >>> |\ \ \ \ >>> | | | | * F >>> | |_|_|/| >>> |/| | | | >>> | | | * | E >>> | |_|/|\| >>> |/| | | | >>> | | |/ * D >>> | |_|__/ >>> |/| | >>> | | * C >>> | |/ >>> |/| >>> | * B >>> |/ >>> * A >> >> At this point in the history, commit E won't render like that -- this is before the change that flattens edges that fuse with the merge's last parent. I think the display of this history at this point will be: >> >> *-----. G >> |\ \ \ \ >> | | | | * F >> | |_|_|/| >> |/| | | | >> | | | * | E >> | |_|/|\ \ >> |/| |/ / / >> | | | | / >> | | | |/ >> | | | * D >> | |_|/ >> |/| | >> | | * C >> | |/ >> |/| >> | * B >> |/ >> * A >> >> Is there a particular reason for wanting to include this test case? What particular combination of states is it designed to test? (My guess is that it includes an octopus merge where the original test does not.) I'd be happy to add it at the appropriate point in the history if it's adding coverage not provided by the other tests. > > Thanks for correcting my test case. It also helps that you would show the change in behavior in your later commits. > > My reason to include this test is because it includes a regular merge and an octopus merge, both of which have a skewed render. Many times logic that applies to a normal merge breaks with octopus merges, so I try to include them whenever possible. Thanks, I've now incorporated your suggested test into my patch. I had to amend it slightly as it turns out the above history is not valid; G is not a possible merge because one of its parents (C) is an ancestor of another (E). The actual example I've added is this: *-----. 0_H |\ \ \ \ | | | | * 0_G | |_|_|/| |/| | | | | | | * \ 0_F | |_|/|\ \ |/| | | |/ | | | | * 0_E | |_|_|/ |/| | | | | * | 0_D | | |/ | | * 0_C | |/ |/| | * 0_B |/ * 0_A I've also added another commit before beginning this work that adds the example from the cover letter, so you can see it changing with each commit, namely this history: * H |\ | * G | |\ | | * F | | | | | \ | *-. \ E | |\ \ \ |/ / / / | | | / | | |/ | | * D | * | C | |/ * | B |/ * A
diff --git a/graph.c b/graph.c index 98e8777db4..9136173e03 100644 --- a/graph.c +++ b/graph.c @@ -183,6 +183,20 @@ struct git_graph { * previous commit. */ int prev_commit_index; + /* + * Which layout variant to use to display merge commits. If the + * commit's first parent is known to be in a column to the left of the + * merge, then this value is 0 and we use the layout on the left. + * Otherwise, the value is 1 and the layout on the right is used. This + * field tells us how many columns the first parent occupies. + * + * 0) 1) + * + * | | | *-. | | *---. + * | |_|/|\ \ | | |\ \ \ + * |/| | | | | | | | | | * + */ + int merge_layout; /* * The maximum number of columns that can be stored in the columns * and new_columns arrays. This is also half the number of entries @@ -294,6 +308,7 @@ struct git_graph *graph_init(struct rev_info *opt) graph->prev_state = GRAPH_PADDING; graph->commit_index = 0; graph->prev_commit_index = 0; + graph->merge_layout = 0; graph->num_columns = 0; graph->num_new_columns = 0; graph->mapping_size = 0; @@ -453,9 +468,11 @@ static int graph_find_new_column_by_commit(struct git_graph *graph, } static void graph_insert_into_new_columns(struct git_graph *graph, - struct commit *commit) + struct commit *commit, + int idx) { int i = graph_find_new_column_by_commit(graph, commit); + int mapping_idx; /* * If the commit is not already in the new_columns array, then add it @@ -467,8 +484,26 @@ static void graph_insert_into_new_columns(struct git_graph *graph, graph->new_columns[i].color = graph_find_commit_color(graph, commit); } - graph->mapping[graph->width] = i; - graph->width += 2; + if (graph->num_parents > 1 && idx > -1 && graph->merge_layout == -1) { + /* + * If this is the first parent of a merge, choose a layout for + * the merge line based on whether the parent appears in a + * column to the left of the merge + */ + int dist, shift; + + dist = idx - i; + shift = (dist > 1) ? 2 * dist - 3 : 1; + + graph->merge_layout = (dist > 0) ? 0 : 1; + mapping_idx = graph->width + (graph->merge_layout - 1) * shift; + graph->width += 2 * graph->merge_layout; + } else { + mapping_idx = graph->width; + graph->width += 2; + } + + graph->mapping[mapping_idx] = i; } static void graph_update_columns(struct git_graph *graph) @@ -534,6 +569,7 @@ static void graph_update_columns(struct git_graph *graph) if (col_commit == graph->commit) { seen_this = 1; graph->commit_index = i; + graph->merge_layout = -1; for (parent = first_interesting_parent(graph); parent; parent = next_interesting_parent(graph, parent)) { @@ -546,7 +582,7 @@ static void graph_update_columns(struct git_graph *graph) !is_commit_in_columns) { graph_increment_column_color(graph); } - graph_insert_into_new_columns(graph, parent->item); + graph_insert_into_new_columns(graph, parent->item, i); } /* * We always need to increment graph->width by at @@ -557,7 +593,7 @@ static void graph_update_columns(struct git_graph *graph) if (graph->num_parents == 0) graph->width += 2; } else { - graph_insert_into_new_columns(graph, col_commit); + graph_insert_into_new_columns(graph, col_commit, -1); } } @@ -569,10 +605,36 @@ static void graph_update_columns(struct git_graph *graph) graph->mapping_size--; } +static int graph_num_expansion_rows(struct git_graph *graph) +{ + /* + * Normally, we need two expansion rows for each dashed parent line from + * an octopus merge: + * + * | * + * | |\ + * | | \ + * | | \ + * | *-. \ + * | |\ \ \ + * + * If the merge is skewed to the left, then its parents occupy one less + * column, and we don't need as many expansion rows to route around it; + * in some cases that means we don't need any expansion rows at all: + * + * | * + * | |\ + * | * \ + * |/|\ \ + */ + return (graph->num_parents + graph->merge_layout - 3) * 2; +} + static int graph_needs_pre_commit_line(struct git_graph *graph) { return graph->num_parents >= 3 && - graph->commit_index < (graph->num_columns - 1); + graph->commit_index < (graph->num_columns - 1) && + graph->expansion_row < graph_num_expansion_rows(graph); } void graph_update(struct git_graph *graph, struct commit *commit) @@ -722,7 +784,6 @@ static void graph_output_skip_line(struct git_graph *graph, struct strbuf *sb) static void graph_output_pre_commit_line(struct git_graph *graph, struct strbuf *sb) { - int num_expansion_rows; int i, seen_this; /* @@ -733,14 +794,13 @@ static void graph_output_pre_commit_line(struct git_graph *graph, * We need 2 extra rows for every parent over 2. */ assert(graph->num_parents >= 3); - num_expansion_rows = (graph->num_parents - 2) * 2; /* * graph->expansion_row tracks the current expansion row we are on. * It should be in the range [0, num_expansion_rows - 1] */ assert(0 <= graph->expansion_row && - graph->expansion_row < num_expansion_rows); + graph->expansion_row < graph_num_expansion_rows(graph)); /* * Output the row @@ -782,7 +842,7 @@ static void graph_output_pre_commit_line(struct git_graph *graph, * and move to state GRAPH_COMMIT if necessary */ graph->expansion_row++; - if (graph->expansion_row >= num_expansion_rows) + if (!graph_needs_pre_commit_line(graph)) graph_update_state(graph, GRAPH_COMMIT); } @@ -820,7 +880,7 @@ static void graph_draw_octopus_merge(struct git_graph *graph, struct strbuf *sb) * x 0 1 2 3 * */ - const int dashless_parents = 2; + const int dashless_parents = 3 - graph->merge_layout; int dashful_parents = graph->num_parents - dashless_parents; /* @@ -828,9 +888,9 @@ static void graph_draw_octopus_merge(struct git_graph *graph, struct strbuf *sb) * above) but sometimes the first parent goes into an existing column, * like this: * - * | *---. - * | |\ \ \ - * |/ / / / + * | *-. + * |/|\ \ + * | | | | * x 0 1 2 * * In which case the number of parents will be one greater than the @@ -923,10 +983,15 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb) graph_update_state(graph, GRAPH_COLLAPSING); } +const char merge_chars[] = {'/', '|', '\\'}; + static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb) { int seen_this = 0; - int i, j; + int i; + + struct commit_list *first_parent = first_interesting_parent(graph); + int seen_parent = 0; /* * Output the post-merge row @@ -949,30 +1014,34 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf * new_columns and use those to format the * edges. */ - struct commit_list *parents = NULL; + struct commit_list *parents = first_parent; int par_column; + int idx = graph->merge_layout; + char c; seen_this = 1; - parents = first_interesting_parent(graph); - assert(parents); - par_column = graph_find_new_column_by_commit(graph, parents->item); - assert(par_column >= 0); - - strbuf_write_column(sb, &graph->new_columns[par_column], '|'); - for (j = 0; j < graph->num_parents - 1; j++) { - parents = next_interesting_parent(graph, parents); - assert(parents); + + for (; parents; parents = next_interesting_parent(graph, parents)) { par_column = graph_find_new_column_by_commit(graph, parents->item); assert(par_column >= 0); - strbuf_write_column(sb, &graph->new_columns[par_column], '\\'); - strbuf_addch(sb, ' '); + + c = merge_chars[idx]; + strbuf_write_column(sb, &graph->new_columns[par_column], c); + if (idx == 2) + strbuf_addch(sb, ' '); + else + idx++; } } else if (seen_this) { strbuf_write_column(sb, col, '\\'); strbuf_addch(sb, ' '); } else { strbuf_write_column(sb, col, '|'); - strbuf_addch(sb, ' '); + if (graph->merge_layout != 0 || i != graph->commit_index - 1) + strbuf_addch(sb, seen_parent ? '_' : ' '); } + + if (col_commit == first_parent->item) + seen_parent = 1; } graph_pad_horizontally(graph, sb); diff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh index dab96c89aa..40f420877b 100755 --- a/t/t4214-log-graph-octopus.sh +++ b/t/t4214-log-graph-octopus.sh @@ -7,9 +7,8 @@ test_description='git log --graph of skewed left octopus merge.' test_expect_success 'set up merge history' ' cat >expect.uncolored <<-\EOF && * left - | *---. octopus-merge - | |\ \ \ - |/ / / / + | *-. octopus-merge + |/|\ \ | | | * 4 | | * | 3 | | |/ @@ -21,9 +20,8 @@ test_expect_success 'set up merge history' ' 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> *<MAGENTA>-<RESET><MAGENTA>.<RESET> octopus-merge + <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> diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh new file mode 100755 index 0000000000..cfaba40f97 --- /dev/null +++ b/t/t4215-log-skewed-merges.sh @@ -0,0 +1,42 @@ +#!/bin/sh + +test_description='git log --graph of skewed merges' + +. ./test-lib.sh + +test_expect_success 'setup left-skewed merge' ' + git checkout --orphan _a && test_commit A && + git branch _b && + git branch _c && + git branch _d && + git branch _e && + git checkout _b && test_commit B && + git checkout _c && test_commit C && + git checkout _d && test_commit D && + git checkout _e && git merge --no-ff _d -m E && + git checkout _a && git merge --no-ff _b _c _e -m F +' + +cat > expect <<\EOF +*---. F +|\ \ \ +| | | * E +| |_|/| +|/| | | +| | | * D +| |_|/ +|/| | +| | * C +| |/ +|/| +| * B +|/ +* A +EOF + +test_expect_success 'log --graph with left-skewed merge' ' + git log --graph --pretty=tformat:%s | sed "s/ *$//" > actual && + test_cmp expect actual +' + +test_done