Improve the readability of log --graph output
#383
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Welcome to GitGitGadgetHi @jcoglan, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that this Pull Request has a good description, as it will be used as cover letter. Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions. If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via: curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt |
jcoglan
force-pushed
the
jc/simplify-graph
branch
from
d0437ef to
ea0df1d
Compare
|
/allow |
|
User jcoglan is now allowed to use GitGitGadget. WARNING: jcoglan has no public email address set on GitHub |
|
/submit |
|
Submitted as [email protected] WARNING: jcoglan has no public email address set on GitHub |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
graph.c
Outdated
| @@ -115,11 +115,20 @@ static const char *column_get_color_code(unsigned short color) | |||
| static void strbuf_write_column(struct strbuf *sb, const struct column *c, | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 10/10/2019 12:13 PM, James Coglan via GitGitGadget wrote:
> From: James Coglan <[email protected]>
>
> 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
There was a problem hiding this comment.
On the Git mailing list, James Coglan wrote (reply to this):
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.
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
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
There was a problem hiding this comment.
On the Git mailing list, James Coglan wrote (reply to this):
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
graph.c
Outdated
| @@ -115,11 +115,20 @@ static const char *column_get_color_code(unsigned short color) | |||
| static void strbuf_write_column(struct strbuf *sb, const struct column *c, | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 10/10/2019 12:13 PM, James Coglan via GitGitGadget wrote:
> From: James Coglan <[email protected]>
>
> Following the introduction of "left-skewed" merges, which are merges
> whose first parent fuses with another edge to its left, we have some
> more edge cases to deal with in the display of commit and post-merge
> lines.
>
> The current graph code handles the following cases for edges appearing
> to the right of the commit (*) on commit lines. A 2-way merge is usually
> followed by vertical lines:
>
> | | |
> | * |
> | |\ \
>
> An octopus merge (more than two parents) is always followed by edges
> sloping to the right:
>
> | | \ | | \
> | *-. \ | *---. \
> | |\ \ \ | |\ \ \ \
>
> A 2-way merge is followed by a right-sloping edge if the commit line
> immediately follows a post-merge line for a commit that appears in the
> same column as the current commit, or any column to the left of that:
>
> | * | * |
> | |\ | |\ \
> | * \ | | * \
> | |\ \ | | |\ \
>
> This commit introduces the following new cases for commit lines. If a
> 2-way merge skews to the left, then the edges to its right are always
> vertical lines, even if the commit follows a post-merge line:
>
> | | | | |\
> | * | | * |
> |/| | |/| |
>
> A commit with 3 parents that skews left is followed by vertical edges:
>
> | | |
> | * |
> |/|\ \
>
> If a 3-way left-skewed merge commit appears immediately after a
> post-merge line, then it may be followed the right-sloping edges, just
> like a 2-way merge that is not skewed.
>
> | |\
> | * \
> |/|\ \
>
> Octopus merges with 4 or more parents that skew to the left will always
> be followed by right-sloping edges, because the existing columns need to
> expand around the merge.
>
> | | \
> | *-. \
> |/|\ \ \
>
> On post-merge lines, usually all edges following the current commit
> slope to the right:
>
> | * | |
> | |\ \ \
>
> However, if the commit is a left-skewed 2-way merge, the edges to its
> right remain vertical. We also need to display a space after the
> vertical line descending from the commit marker, whereas this line would
> normally be followed by a backslash.
>
> | * | |
> |/| | |
>
> If a left-skewed merge has more than 2 parents, then the edges to its
> right are still sloped as they bend around the edges introduced by the
> merge.
>
> | * | |
> |/|\ \ \
>
> To handle these new cases, we need to know not just how many parents
> each commit has, but how many new columns it adds to the display; this
> quantity is recorded in the `edges_added` field for the current commit,
> and `prev_edges_added` field for the previous commit.
>
> Here, "column" refers to visual columns, not the logical columns of the
> `columns` array. This is because even if all the commit's parents end up
> fusing with existing edges, they initially introduce distinct edges in
> the commit and post-merge lines before those edges collapse. For
> example, a 3-way merge whose 2nd and 3rd parents fuse with existing
> edges still introduces 2 visual columns that affect the display of edges
> to their right.
>
> | | | \
> | | *-. \
> | | |\ \ \
> | |_|/ / /
> |/| | / /
> | | |/ /
> | |/| |
> | | | |
>
> This merge does not introduce any *logical* columns; there are 4 edges
> before and after this commit once all edges have collapsed. But it does
> initially introduce 2 new edges that need to be accommodated by the
> edges to their right.
>
> Signed-off-by: James Coglan <[email protected]>
> ---
> graph.c | 63 +++++++++++++--
> t/t4215-log-skewed-merges.sh | 151 +++++++++++++++++++++++++++++++++++
> 2 files changed, 209 insertions(+), 5 deletions(-)
>
> diff --git a/graph.c b/graph.c
> index 9136173e03..fb2e42850f 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -197,6 +197,46 @@ struct git_graph {
> * |/| | | | | | | | | | *
> */
> int merge_layout;
> + /*
> + * The number of columns added to the graph by the current commit. For
> + * 2-way and octopus merges, this is is usually one less than the
> + * number of parents:
> + *
> + * | | | | | \
> + * | * | | *---. \
> + * | |\ \ | |\ \ \ \
> + * | | | | | | | | | |
> + *
> + * num_parents: 2 num_parents: 4
> + * edges_added: 1 edges_added: 3
> + *
> + * For left-skewed merges, the first parent fuses with its neighbor and
> + * so one less column is added:
> + *
> + * | | | | | \
> + * | * | | *-. \
> + * |/| | |/|\ \ \
> + * | | | | | | | |
> + *
> + * num_parents: 2 num_parents: 4
> + * edges_added: 0 edges_added: 2
> + *
> + * This number determines how edges to the right of the merge are
> + * displayed in commit and post-merge lines; if no columns have been
> + * added then a vertical line should be used where a right-tracking
> + * line would otherwise be used.
> + *
> + * | * \ | * |
> + * | |\ \ |/| |
> + * | | * \ | * |
> + */
> + int edges_added;
> + /*
> + * The number of columns added by the previous commit, which is used to
> + * smooth edges appearing to the right of a commit in a commit line
> + * following a post-merge line.
> + */
> + int prev_edges_added;
> /*
> * The maximum number of columns that can be stored in the columns
> * and new_columns arrays. This is also half the number of entries
> @@ -309,6 +349,8 @@ struct git_graph *graph_init(struct rev_info *opt)
> graph->commit_index = 0;
> graph->prev_commit_index = 0;
> graph->merge_layout = 0;
> + graph->edges_added = 0;
> + graph->prev_edges_added = 0;
> graph->num_columns = 0;
> graph->num_new_columns = 0;
> graph->mapping_size = 0;
> @@ -670,6 +712,9 @@ void graph_update(struct git_graph *graph, struct commit *commit)
> */
> graph_update_columns(graph);
>
> + graph->prev_edges_added = graph->edges_added;
> + graph->edges_added = graph->num_parents + graph->merge_layout - 2;
> +
> graph->expansion_row = 0;
>
> /*
> @@ -943,12 +988,13 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
>
> if (graph->num_parents > 2)
> graph_draw_octopus_merge(graph, sb);
> - } else if (seen_this && (graph->num_parents > 2)) {
> + } else if (seen_this && (graph->edges_added > 1)) {
> strbuf_write_column(sb, col, '\\');
> - } else if (seen_this && (graph->num_parents == 2)) {
> + } else if (seen_this && (graph->edges_added == 1)) {
> /*
> - * This is a 2-way merge commit.
> - * There is no GRAPH_PRE_COMMIT stage for 2-way
> + * This is either a right-skewed 2-way merge
> + * commit, or a left-skewed 3-way merge.
> + * There is no GRAPH_PRE_COMMIT stage for such
> * merges, so this is the first line of output
> * for this commit. Check to see what the previous
> * line of output was.
> @@ -960,6 +1006,7 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
> * makes the output look nicer.
> */
> if (graph->prev_state == GRAPH_POST_MERGE &&
> + graph->prev_edges_added > 0 &&
> graph->prev_commit_index < i)
> strbuf_write_column(sb, col, '\\');
> else
> @@ -1031,8 +1078,14 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf
> else
> idx++;
> }
> + if (graph->edges_added == 0)
> + strbuf_addch(sb, ' ');
> +
> } else if (seen_this) {
> - strbuf_write_column(sb, col, '\\');
> + if (graph->edges_added > 0)
> + strbuf_write_column(sb, col, '\\');
> + else
> + strbuf_write_column(sb, col, '|');
> strbuf_addch(sb, ' ');
> } else {
> strbuf_write_column(sb, col, '|');
> diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
> index cfaba40f97..e479d6e676 100755
> --- a/t/t4215-log-skewed-merges.sh
> +++ b/t/t4215-log-skewed-merges.sh
> @@ -39,4 +39,155 @@ test_expect_success 'log --graph with left-skewed merge' '
> test_cmp expect actual
> '
>
> +test_expect_success 'setup nested left-skewed merge' '
> + git checkout --orphan 1_p &&
> + test_commit 1_A &&
> + test_commit 1_B &&
> + test_commit 1_C &&
> + git checkout -b 1_q @^ && test_commit 1_D &&
> + git checkout 1_p && git merge --no-ff 1_q -m 1_E &&
> + git checkout -b 1_r @~3 && test_commit 1_F &&
> + git checkout 1_p && git merge --no-ff 1_r -m 1_G &&
> + git checkout @^^ && git merge --no-ff 1_p -m 1_H
> +'
> +
> +cat > expect <<\EOF
> +* 1_H
> +|\
> +| * 1_G
> +| |\
> +| | * 1_F
> +| * | 1_E
> +|/| |
> +| * | 1_D
> +* | | 1_C
> +|/ /
> +* | 1_B
> +|/
> +* 1_A
> +EOF
> +
> +test_expect_success 'log --graph with nested left-skewed merge' '
> + git log --graph --pretty=tformat:%s | sed "s/ *$//" > actual &&
> + test_cmp expect actual
> +'
I should have noticed in your earlier commits, but why don't you keep
the output inside the test suite? You can start with "cat >expect <<-EOF"
to have it ignore leading whitespace. Sorry if there's something else about
this that is causing issues.
> +
> +test_expect_success 'setup nested left-skewed merge following normal merge' '
> + git checkout --orphan 2_p &&
> + test_commit 2_A &&
> + test_commit 2_B &&
> + test_commit 2_C &&
> + git checkout -b 2_q @^^ &&
> + test_commit 2_D &&
> + test_commit 2_E &&
> + git checkout -b 2_r @^ && test_commit 2_F &&
> + git checkout 2_q &&
> + git merge --no-ff 2_r -m 2_G &&
> + git merge --no-ff 2_p^ -m 2_H &&
> + git checkout -b 2_s @^^ && git merge --no-ff 2_q -m 2_J &&
> + git checkout 2_p && git merge --no-ff 2_s -m 2_K
> +'
> +
> +cat > expect <<\EOF
> +* 2_K
> +|\
> +| * 2_J
> +| |\
> +| | * 2_H
> +| | |\
> +| | * | 2_G
> +| |/| |
> +| | * | 2_F
> +| * | | 2_E
> +| |/ /
> +| * | 2_D
> +* | | 2_C
> +| |/
> +|/|
> +* | 2_B
> +|/
> +* 2_A
> +EOF
> +
> +test_expect_success 'log --graph with nested left-skewed merge following normal merge' '
> + git log --graph --pretty=tformat:%s | sed "s/ *$//" > actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'setup nested right-skewed merge following left-skewed merge' '
> + git checkout --orphan 3_p &&
> + test_commit 3_A &&
> + git checkout -b 3_q &&
> + test_commit 3_B &&
> + test_commit 3_C &&
> + git checkout -b 3_r @^ &&
> + test_commit 3_D &&
> + git checkout 3_q && git merge --no-ff 3_r -m 3_E &&
> + git checkout 3_p && git merge --no-ff 3_q -m 3_F &&
> + git checkout 3_r && test_commit 3_G &&
> + git checkout 3_p && git merge --no-ff 3_r -m 3_H &&
> + git checkout @^^ && git merge --no-ff 3_p -m 3_J
> +'
> +
> +cat > expect <<\EOF
> +* 3_J
> +|\
> +| * 3_H
> +| |\
> +| | * 3_G
> +| * | 3_F
> +|/| |
> +| * | 3_E
> +| |\ \
> +| | |/
> +| | * 3_D
> +| * | 3_C
> +| |/
> +| * 3_B
> +|/
> +* 3_A
> +EOF
> +
> +test_expect_success 'log --graph with nested right-skewed merge following left-skewed merge' '
> + git log --graph --pretty=tformat:%s | sed "s/ *$//" > actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'setup right-skewed merge following a left-skewed one' '
> + git checkout --orphan 4_p &&
> + test_commit 4_A &&
> + test_commit 4_B &&
> + test_commit 4_C &&
> + git checkout -b 4_q @^^ && test_commit 4_D &&
> + git checkout -b 4_r 4_p^ && git merge --no-ff 4_q -m 4_E &&
> + git checkout -b 4_s 4_p^^ &&
> + git merge --no-ff 4_r -m 4_F &&
> + git merge --no-ff 4_p -m 4_G &&
> + git checkout @^^ && git merge --no-ff 4_s -m 4_H
> +'
> +
> +cat > expect <<\EOF
> +* 4_H
> +|\
> +| * 4_G
> +| |\
> +| * | 4_F
> +|/| |
> +| * | 4_E
> +| |\ \
> +| | * | 4_D
> +| |/ /
> +|/| |
> +| | * 4_C
> +| |/
> +| * 4_B
> +|/
> +* 4_A
> +EOF
> +
> +test_expect_success 'log --graph with right-skewed merge following a left-skewed one' '
> + git log --graph --date-order --pretty=tformat:%s | sed "s/ *$//" > actual &&
> + test_cmp expect actual
> +'
> +
> test_done
>
There was a problem hiding this comment.
On the Git mailing list, James Coglan wrote (reply to this):
On 10/10/2019 18:49, Derrick Stolee wrote:
> On 10/10/2019 12:13 PM, James Coglan via GitGitGadget wrote:
>> From: James Coglan <[email protected]>
>>
>> Following the introduction of "left-skewed" merges, which are merges
>> whose first parent fuses with another edge to its left, we have some
>> more edge cases to deal with in the display of commit and post-merge
>> lines.
>>
>> The current graph code handles the following cases for edges appearing
>> to the right of the commit (*) on commit lines. A 2-way merge is usually
>> followed by vertical lines:
>>
>> | | |
>> | * |
>> | |\ \
>>
>> An octopus merge (more than two parents) is always followed by edges
>> sloping to the right:
>>
>> | | \ | | \
>> | *-. \ | *---. \
>> | |\ \ \ | |\ \ \ \
>>
>> A 2-way merge is followed by a right-sloping edge if the commit line
>> immediately follows a post-merge line for a commit that appears in the
>> same column as the current commit, or any column to the left of that:
>>
>> | * | * |
>> | |\ | |\ \
>> | * \ | | * \
>> | |\ \ | | |\ \
>>
>> This commit introduces the following new cases for commit lines. If a
>> 2-way merge skews to the left, then the edges to its right are always
>> vertical lines, even if the commit follows a post-merge line:
>>
>> | | | | |\
>> | * | | * |
>> |/| | |/| |
>>
>> A commit with 3 parents that skews left is followed by vertical edges:
>>
>> | | |
>> | * |
>> |/|\ \
>>
>> If a 3-way left-skewed merge commit appears immediately after a
>> post-merge line, then it may be followed the right-sloping edges, just
>> like a 2-way merge that is not skewed.
>>
>> | |\
>> | * \
>> |/|\ \
>>
>> Octopus merges with 4 or more parents that skew to the left will always
>> be followed by right-sloping edges, because the existing columns need to
>> expand around the merge.
>>
>> | | \
>> | *-. \
>> |/|\ \ \
>>
>> On post-merge lines, usually all edges following the current commit
>> slope to the right:
>>
>> | * | |
>> | |\ \ \
>>
>> However, if the commit is a left-skewed 2-way merge, the edges to its
>> right remain vertical. We also need to display a space after the
>> vertical line descending from the commit marker, whereas this line would
>> normally be followed by a backslash.
>>
>> | * | |
>> |/| | |
>>
>> If a left-skewed merge has more than 2 parents, then the edges to its
>> right are still sloped as they bend around the edges introduced by the
>> merge.
>>
>> | * | |
>> |/|\ \ \
>>
>> To handle these new cases, we need to know not just how many parents
>> each commit has, but how many new columns it adds to the display; this
>> quantity is recorded in the `edges_added` field for the current commit,
>> and `prev_edges_added` field for the previous commit.
>>
>> Here, "column" refers to visual columns, not the logical columns of the
>> `columns` array. This is because even if all the commit's parents end up
>> fusing with existing edges, they initially introduce distinct edges in
>> the commit and post-merge lines before those edges collapse. For
>> example, a 3-way merge whose 2nd and 3rd parents fuse with existing
>> edges still introduces 2 visual columns that affect the display of edges
>> to their right.
>>
>> | | | \
>> | | *-. \
>> | | |\ \ \
>> | |_|/ / /
>> |/| | / /
>> | | |/ /
>> | |/| |
>> | | | |
>>
>> This merge does not introduce any *logical* columns; there are 4 edges
>> before and after this commit once all edges have collapsed. But it does
>> initially introduce 2 new edges that need to be accommodated by the
>> edges to their right.
>>
>> Signed-off-by: James Coglan <[email protected]>
>> ---
>> graph.c | 63 +++++++++++++--
>> t/t4215-log-skewed-merges.sh | 151 +++++++++++++++++++++++++++++++++++
>> 2 files changed, 209 insertions(+), 5 deletions(-)
>>
>> diff --git a/graph.c b/graph.c
>> index 9136173e03..fb2e42850f 100644
>> --- a/graph.c
>> +++ b/graph.c
>> @@ -197,6 +197,46 @@ struct git_graph {
>> * |/| | | | | | | | | | *
>> */
>> int merge_layout;
>> + /*
>> + * The number of columns added to the graph by the current commit. For
>> + * 2-way and octopus merges, this is is usually one less than the
>> + * number of parents:
>> + *
>> + * | | | | | \
>> + * | * | | *---. \
>> + * | |\ \ | |\ \ \ \
>> + * | | | | | | | | | |
>> + *
>> + * num_parents: 2 num_parents: 4
>> + * edges_added: 1 edges_added: 3
>> + *
>> + * For left-skewed merges, the first parent fuses with its neighbor and
>> + * so one less column is added:
>> + *
>> + * | | | | | \
>> + * | * | | *-. \
>> + * |/| | |/|\ \ \
>> + * | | | | | | | |
>> + *
>> + * num_parents: 2 num_parents: 4
>> + * edges_added: 0 edges_added: 2
>> + *
>> + * This number determines how edges to the right of the merge are
>> + * displayed in commit and post-merge lines; if no columns have been
>> + * added then a vertical line should be used where a right-tracking
>> + * line would otherwise be used.
>> + *
>> + * | * \ | * |
>> + * | |\ \ |/| |
>> + * | | * \ | * |
>> + */
>> + int edges_added;
>> + /*
>> + * The number of columns added by the previous commit, which is used to
>> + * smooth edges appearing to the right of a commit in a commit line
>> + * following a post-merge line.
>> + */
>> + int prev_edges_added;
>> /*
>> * The maximum number of columns that can be stored in the columns
>> * and new_columns arrays. This is also half the number of entries
>> @@ -309,6 +349,8 @@ struct git_graph *graph_init(struct rev_info *opt)
>> graph->commit_index = 0;
>> graph->prev_commit_index = 0;
>> graph->merge_layout = 0;
>> + graph->edges_added = 0;
>> + graph->prev_edges_added = 0;
>> graph->num_columns = 0;
>> graph->num_new_columns = 0;
>> graph->mapping_size = 0;
>> @@ -670,6 +712,9 @@ void graph_update(struct git_graph *graph, struct commit *commit)
>> */
>> graph_update_columns(graph);
>>
>> + graph->prev_edges_added = graph->edges_added;
>> + graph->edges_added = graph->num_parents + graph->merge_layout - 2;
>> +
>> graph->expansion_row = 0;
>>
>> /*
>> @@ -943,12 +988,13 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
>>
>> if (graph->num_parents > 2)
>> graph_draw_octopus_merge(graph, sb);
>> - } else if (seen_this && (graph->num_parents > 2)) {
>> + } else if (seen_this && (graph->edges_added > 1)) {
>> strbuf_write_column(sb, col, '\\');
>> - } else if (seen_this && (graph->num_parents == 2)) {
>> + } else if (seen_this && (graph->edges_added == 1)) {
>> /*
>> - * This is a 2-way merge commit.
>> - * There is no GRAPH_PRE_COMMIT stage for 2-way
>> + * This is either a right-skewed 2-way merge
>> + * commit, or a left-skewed 3-way merge.
>> + * There is no GRAPH_PRE_COMMIT stage for such
>> * merges, so this is the first line of output
>> * for this commit. Check to see what the previous
>> * line of output was.
>> @@ -960,6 +1006,7 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
>> * makes the output look nicer.
>> */
>> if (graph->prev_state == GRAPH_POST_MERGE &&
>> + graph->prev_edges_added > 0 &&
>> graph->prev_commit_index < i)
>> strbuf_write_column(sb, col, '\\');
>> else
>> @@ -1031,8 +1078,14 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf
>> else
>> idx++;
>> }
>> + if (graph->edges_added == 0)
>> + strbuf_addch(sb, ' ');
>> +
>> } else if (seen_this) {
>> - strbuf_write_column(sb, col, '\\');
>> + if (graph->edges_added > 0)
>> + strbuf_write_column(sb, col, '\\');
>> + else
>> + strbuf_write_column(sb, col, '|');
>> strbuf_addch(sb, ' ');
>> } else {
>> strbuf_write_column(sb, col, '|');
>> diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
>> index cfaba40f97..e479d6e676 100755
>> --- a/t/t4215-log-skewed-merges.sh
>> +++ b/t/t4215-log-skewed-merges.sh
>> @@ -39,4 +39,155 @@ test_expect_success 'log --graph with left-skewed merge' '
>> test_cmp expect actual
>> '
>>
>> +test_expect_success 'setup nested left-skewed merge' '
>> + git checkout --orphan 1_p &&
>> + test_commit 1_A &&
>> + test_commit 1_B &&
>> + test_commit 1_C &&
>> + git checkout -b 1_q @^ && test_commit 1_D &&
>> + git checkout 1_p && git merge --no-ff 1_q -m 1_E &&
>> + git checkout -b 1_r @~3 && test_commit 1_F &&
>> + git checkout 1_p && git merge --no-ff 1_r -m 1_G &&
>> + git checkout @^^ && git merge --no-ff 1_p -m 1_H
>> +'
>> +
>> +cat > expect <<\EOF
>> +* 1_H
>> +|\
>> +| * 1_G
>> +| |\
>> +| | * 1_F
>> +| * | 1_E
>> +|/| |
>> +| * | 1_D
>> +* | | 1_C
>> +|/ /
>> +* | 1_B
>> +|/
>> +* 1_A
>> +EOF
>> +
>> +test_expect_success 'log --graph with nested left-skewed merge' '
>> + git log --graph --pretty=tformat:%s | sed "s/ *$//" > actual &&
>> + test_cmp expect actual
>> +'
>
> I should have noticed in your earlier commits, but why don't you keep
> the output inside the test suite? You can start with "cat >expect <<-EOF"
> to have it ignore leading whitespace. Sorry if there's something else about
> this that is causing issues.
I was following a pattern used in t/t4202-log.sh. I believe it was easier to debug these tests with the setup and expectations split into separate blocks, but I wouldn't be opposed to merging them.
>> +
>> +test_expect_success 'setup nested left-skewed merge following normal merge' '
>> + git checkout --orphan 2_p &&
>> + test_commit 2_A &&
>> + test_commit 2_B &&
>> + test_commit 2_C &&
>> + git checkout -b 2_q @^^ &&
>> + test_commit 2_D &&
>> + test_commit 2_E &&
>> + git checkout -b 2_r @^ && test_commit 2_F &&
>> + git checkout 2_q &&
>> + git merge --no-ff 2_r -m 2_G &&
>> + git merge --no-ff 2_p^ -m 2_H &&
>> + git checkout -b 2_s @^^ && git merge --no-ff 2_q -m 2_J &&
>> + git checkout 2_p && git merge --no-ff 2_s -m 2_K
>> +'
>> +
>> +cat > expect <<\EOF
>> +* 2_K
>> +|\
>> +| * 2_J
>> +| |\
>> +| | * 2_H
>> +| | |\
>> +| | * | 2_G
>> +| |/| |
>> +| | * | 2_F
>> +| * | | 2_E
>> +| |/ /
>> +| * | 2_D
>> +* | | 2_C
>> +| |/
>> +|/|
>> +* | 2_B
>> +|/
>> +* 2_A
>> +EOF
>> +
>> +test_expect_success 'log --graph with nested left-skewed merge following normal merge' '
>> + git log --graph --pretty=tformat:%s | sed "s/ *$//" > actual &&
>> + test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'setup nested right-skewed merge following left-skewed merge' '
>> + git checkout --orphan 3_p &&
>> + test_commit 3_A &&
>> + git checkout -b 3_q &&
>> + test_commit 3_B &&
>> + test_commit 3_C &&
>> + git checkout -b 3_r @^ &&
>> + test_commit 3_D &&
>> + git checkout 3_q && git merge --no-ff 3_r -m 3_E &&
>> + git checkout 3_p && git merge --no-ff 3_q -m 3_F &&
>> + git checkout 3_r && test_commit 3_G &&
>> + git checkout 3_p && git merge --no-ff 3_r -m 3_H &&
>> + git checkout @^^ && git merge --no-ff 3_p -m 3_J
>> +'
>> +
>> +cat > expect <<\EOF
>> +* 3_J
>> +|\
>> +| * 3_H
>> +| |\
>> +| | * 3_G
>> +| * | 3_F
>> +|/| |
>> +| * | 3_E
>> +| |\ \
>> +| | |/
>> +| | * 3_D
>> +| * | 3_C
>> +| |/
>> +| * 3_B
>> +|/
>> +* 3_A
>> +EOF
>> +
>> +test_expect_success 'log --graph with nested right-skewed merge following left-skewed merge' '
>> + git log --graph --pretty=tformat:%s | sed "s/ *$//" > actual &&
>> + test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'setup right-skewed merge following a left-skewed one' '
>> + git checkout --orphan 4_p &&
>> + test_commit 4_A &&
>> + test_commit 4_B &&
>> + test_commit 4_C &&
>> + git checkout -b 4_q @^^ && test_commit 4_D &&
>> + git checkout -b 4_r 4_p^ && git merge --no-ff 4_q -m 4_E &&
>> + git checkout -b 4_s 4_p^^ &&
>> + git merge --no-ff 4_r -m 4_F &&
>> + git merge --no-ff 4_p -m 4_G &&
>> + git checkout @^^ && git merge --no-ff 4_s -m 4_H
>> +'
>> +
>> +cat > expect <<\EOF
>> +* 4_H
>> +|\
>> +| * 4_G
>> +| |\
>> +| * | 4_F
>> +|/| |
>> +| * | 4_E
>> +| |\ \
>> +| | * | 4_D
>> +| |/ /
>> +|/| |
>> +| | * 4_C
>> +| |/
>> +| * 4_B
>> +|/
>> +* 4_A
>> +EOF
>> +
>> +test_expect_success 'log --graph with right-skewed merge following a left-skewed one' '
>> + git log --graph --date-order --pretty=tformat:%s | sed "s/ *$//" > actual &&
>> + test_cmp expect actual
>> +'
>> +
>> test_done
>>
>
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Fri, Oct 11, 2019 at 06:04:21PM +0100, James Coglan wrote:
> > I should have noticed in your earlier commits, but why don't you keep
> > the output inside the test suite? You can start with "cat >expect <<-EOF"
> > to have it ignore leading whitespace. Sorry if there's something else about
> > this that is causing issues.
>
> I was following a pattern used in t/t4202-log.sh. I believe it was
> easier to debug these tests with the setup and expectations split into
> separate blocks, but I wouldn't be opposed to merging them.
Some of the older tests used that style, but we've been slowly
modernizing (I know, it's hard to pick up the style by example in such
cases!). The usual style these days is making sure everything goes in a
test_expect_* block, with "<<-" to indent here-documents.
Another minor style nit that you picked up from t4202:
> >> +cat > expect <<\EOF
We'd omit the space after ">" here.
-Peff
There was a problem hiding this comment.
On the Git mailing list, James Coglan wrote (reply to this):
On 13/10/2019 07:56, Jeff King wrote:
> On Fri, Oct 11, 2019 at 06:04:21PM +0100, James Coglan wrote:
>
>>> I should have noticed in your earlier commits, but why don't you keep
>>> the output inside the test suite? You can start with "cat >expect <<-EOF"
>>> to have it ignore leading whitespace. Sorry if there's something else about
>>> this that is causing issues.
>>
>> I was following a pattern used in t/t4202-log.sh. I believe it was
>> easier to debug these tests with the setup and expectations split into
>> separate blocks, but I wouldn't be opposed to merging them.
>
> Some of the older tests used that style, but we've been slowly
> modernizing (I know, it's hard to pick up the style by example in such
> cases!). The usual style these days is making sure everything goes in a
> test_expect_* block, with "<<-" to indent here-documents.
>
> Another minor style nit that you picked up from t4202:
>
>>>> +cat > expect <<\EOF
>
> We'd omit the space after ">" here.
Thanks, I've now made the style changes you've suggested on my branch. How should I go about sharing the current state of my patch series after I've incorporated all the changes suggested here? Should I post them as replies on this thread, or start a new pull request via GitGitGadget?
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 10/14/2019 11:38 AM, James Coglan wrote:
> On 13/10/2019 07:56, Jeff King wrote:
>> On Fri, Oct 11, 2019 at 06:04:21PM +0100, James Coglan wrote:
>>
>>>> I should have noticed in your earlier commits, but why don't you keep
>>>> the output inside the test suite? You can start with "cat >expect <<-EOF"
>>>> to have it ignore leading whitespace. Sorry if there's something else about
>>>> this that is causing issues.
>>>
>>> I was following a pattern used in t/t4202-log.sh. I believe it was
>>> easier to debug these tests with the setup and expectations split into
>>> separate blocks, but I wouldn't be opposed to merging them.
>>
>> Some of the older tests used that style, but we've been slowly
>> modernizing (I know, it's hard to pick up the style by example in such
>> cases!). The usual style these days is making sure everything goes in a
>> test_expect_* block, with "<<-" to indent here-documents.
>>
>> Another minor style nit that you picked up from t4202:
>>
>>>>> +cat > expect <<\EOF
>>
>> We'd omit the space after ">" here.
>
> Thanks, I've now made the style changes you've suggested on my branch. How should I go about sharing the current state of my patch series after I've incorporated all the changes suggested here? Should I post them as replies on this thread, or start a new pull request via GitGitGadget?
Since you sent v1 via GitGitGadget, all you need to do is
add another "/submit" comment on the same PR and it will
send a v2 to this thread.
It will auto-generate the range-diff from v1 and append it
to your cover letter.
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi James,
On Mon, 14 Oct 2019, James Coglan wrote:
> [...] How should I go about sharing the current state of my patch
> series after I've incorporated all the changes suggested here? Should
> I post them as replies on this thread, or start a new pull request via
> GitGitGadget?
Just force-push the branch, and tell GitGitGadget to `/submit`. It will
take care of tagging a new iteration, generating a range-diff, and
sending it off to the list.
Thanks,
Dscho
|
On the Git mailing list, Derrick Stolee wrote (reply to this): |
graph.c
Outdated
| @@ -115,11 +115,20 @@ static const char *column_get_color_code(unsigned short color) | |||
| static void strbuf_write_column(struct strbuf *sb, const struct column *c, | |||
There was a problem hiding this comment.
On the Git mailing list, Denton Liu wrote (reply to this):
Hi James,
Nicely done! This issue was bugging me for a while!
On Thu, Oct 10, 2019 at 09:13:52AM -0700, James Coglan via GitGitGadget wrote:
[...]
> diff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh
> index 9ada687628..fbd485d83a 100755
> --- a/t/t4214-log-graph-octopus.sh
> +++ b/t/t4214-log-graph-octopus.sh
> @@ -42,23 +42,74 @@ test_expect_success 'set up merge history' '
> test_tick &&
> git merge -m octopus-merge 1 2 3 4 &&
> git checkout 1 -b L &&
> - test_commit left
> + test_commit left &&
> + git checkout 4 -b R &&
> + test_commit right
> '
>
> 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 &&
> + git log --color=always --graph --date-order --pretty=tformat:%s L merge >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 &&
> + git log --color=never --graph --date-order --pretty=tformat:%s L merge >actual.raw &&
> sed "s/ *\$//" actual.raw >actual &&
> test_cmp expect.uncolored actual
> '
>
> -# Repeat the previous two tests with "normal" octopus merge (i.e.,
> +# Repeat the previous two tests with an octopus merge whose final parent skews left
> +
> +test_expect_success 'log --graph with left-skewed final parent, no color' '
> + cat >expect.uncolored <<-\EOF &&
> + * right
> + | *---. octopus-merge
> + | |\ \ \
> + | |_|_|/
> + |/| | |
> + * | | | 4
> + | | | * 3
> + | |_|/
> + |/| |
> + | | * 2
> + | |/
> + |/|
> + | * 1
> + |/
> + * initial
> + EOF
> + git log --color=never --graph --date-order --pretty=tformat:%s R merge >actual.raw &&
> + sed "s/ *\$//" actual.raw >actual &&
> + test_cmp expect.uncolored actual
> +'
> +
> +test_expect_success 'log --graph with left-skewed final parent with colors' '
> + cat >expect.colors <<-\EOF &&
> + * right
> + <RED>|<RESET> *<BLUE>-<RESET><BLUE>-<RESET><RED>-<RESET><RED>.<RESET> octopus-merge
> + <RED>|<RESET> <GREEN>|<RESET><YELLOW>\<RESET> <BLUE>\<RESET> <RED>\<RESET>
> + <RED>|<RESET> <GREEN>|<RESET><RED>_<RESET><YELLOW>|<RESET><RED>_<RESET><BLUE>|<RESET><RED>/<RESET>
> + <RED>|<RESET><RED>/<RESET><GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET>
> + * <GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> 4
> + <MAGENTA>|<RESET> <GREEN>|<RESET> <YELLOW>|<RESET> * 3
> + <MAGENTA>|<RESET> <GREEN>|<RESET><MAGENTA>_<RESET><YELLOW>|<RESET><MAGENTA>/<RESET>
> + <MAGENTA>|<RESET><MAGENTA>/<RESET><GREEN>|<RESET> <YELLOW>|<RESET>
> + <MAGENTA>|<RESET> <GREEN>|<RESET> * 2
> + <MAGENTA>|<RESET> <GREEN>|<RESET><MAGENTA>/<RESET>
> + <MAGENTA>|<RESET><MAGENTA>/<RESET><GREEN>|<RESET>
> + <MAGENTA>|<RESET> * 1
> + <MAGENTA>|<RESET><MAGENTA>/<RESET>
> + * initial
> + EOF
> + test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
> + git log --color=always --graph --date-order --pretty=tformat:%s R merge >actual.colors.raw &&
> + test_decode_color <actual.colors.raw | sed "s/ *\$//" >actual.colors &&
> + test_cmp expect.colors actual.colors
> +'
> +
> +# Repeat the first 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' '
So, I decided to merge 'dl/octopus-graph-bug' with your branch and I
couldn't be happier! I had to make a couple of minor tweaks to the
existing test cases but most of them only involved flipping
`test_expect_failure` to `test_expect_success`.
You can see the results of this by doing:
$ git fetch https://github.com/Denton-L/git.git testing/graph-output
$ git diff FETCH_HEAD^2 t/t4214-log-graph-octopus.sh
and the resulting diff is very pleasing imo.
Junio, when you pick this topic up, that branch should contain the
correct conflict resolution if that can help you out in any way.
Anyway, thanks for the work, James!
I'll give this patch my:
Tested-by: Denton Liu <[email protected]>
> --
> gitgitgadget
There was a problem hiding this comment.
On the Git mailing list, Denton Liu wrote (reply to this):
On Thu, Oct 10, 2019 at 11:16:24AM -0700, Denton Liu wrote:
> You can see the results of this by doing:
>
> $ git fetch https://github.com/Denton-L/git.git testing/graph-output
> $ git diff FETCH_HEAD^2 t/t4214-log-graph-octopus.sh
>
> and the resulting diff is very pleasing imo.
I guess it'd probably be nice if the result of that diff were viewable
without the extra work of fetching everything, so here it is:
diff --git a/t/t4214-log-graph-octopus.sh b/t/t4214-log-graph-octopus.sh
index 3ae8e51e50..40d27db674 100755
--- a/t/t4214-log-graph-octopus.sh
+++ b/t/t4214-log-graph-octopus.sh
@@ -26,15 +26,14 @@ test_expect_success 'set up merge history' '
test_expect_success 'log --graph with tricky octopus merge, no color' '
cat >expect.uncolored <<-\EOF &&
* left
- | *---. octopus-merge
- | |\ \ \
- |/ / / /
+ | *-. octopus-merge
+ |/|\ \
| | | * 4
| | * | 3
| | |/
- | * | 2
+ | * / 2
| |/
- * | 1
+ * / 1
|/
* initial
EOF
@@ -47,15 +46,14 @@ test_expect_success 'log --graph with tricky octopus merge with colors' '
test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
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>
- <RED>|<RESET> * <MAGENTA>|<RESET> 2
+ <RED>|<RESET> * <MAGENTA>/<RESET> 2
<RED>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
- * <MAGENTA>|<RESET> 1
+ * <MAGENTA>/<RESET> 1
<MAGENTA>|<RESET><MAGENTA>/<RESET>
* initial
EOF
@@ -74,9 +72,9 @@ test_expect_success 'log --graph with normal octopus merge, no color' '
| | | * 4
| | * | 3
| | |/
- | * | 2
+ | * / 2
| |/
- * | 1
+ * / 1
|/
* initial
EOF
@@ -92,9 +90,9 @@ test_expect_success 'log --graph with normal octopus merge with colors' '
<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> 2
<RED>|<RESET> <BLUE>|<RESET><BLUE>/<RESET>
- * <BLUE>|<RESET> 1
+ * <BLUE>/<RESET> 1
<BLUE>|<RESET><BLUE>/<RESET>
* initial
EOF
@@ -112,9 +110,9 @@ test_expect_success 'log --graph with normal octopus merge and child, no color'
| | | * 4
| | * | 3
| | |/
- | * | 2
+ | * / 2
| |/
- * | 1
+ * / 1
|/
* initial
EOF
@@ -123,7 +121,7 @@ test_expect_success 'log --graph with normal octopus merge and child, no color'
test_cmp expect.uncolored actual
'
-test_expect_failure 'log --graph with normal octopus and child merge with colors' '
+test_expect_success 'log --graph with normal octopus and child merge with colors' '
cat >expect.colors <<-\EOF &&
* after-merge
*<BLUE>-<RESET><BLUE>-<RESET><MAGENTA>-<RESET><MAGENTA>.<RESET> octopus-merge
@@ -131,9 +129,9 @@ test_expect_failure 'log --graph with normal octopus and child merge with colors
<GREEN>|<RESET> <YELLOW>|<RESET> <BLUE>|<RESET> * 4
<GREEN>|<RESET> <YELLOW>|<RESET> * <MAGENTA>|<RESET> 3
<GREEN>|<RESET> <YELLOW>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
- <GREEN>|<RESET> * <MAGENTA>|<RESET> 2
+ <GREEN>|<RESET> * <MAGENTA>/<RESET> 2
<GREEN>|<RESET> <MAGENTA>|<RESET><MAGENTA>/<RESET>
- * <MAGENTA>|<RESET> 1
+ * <MAGENTA>/<RESET> 1
<MAGENTA>|<RESET><MAGENTA>/<RESET>
* initial
EOF
@@ -147,15 +145,14 @@ test_expect_success 'log --graph with tricky octopus merge and its child, no col
cat >expect.uncolored <<-\EOF &&
* left
| * after-merge
- | *---. octopus-merge
- | |\ \ \
- |/ / / /
+ | *-. octopus-merge
+ |/|\ \
| | | * 4
| | * | 3
| | |/
- | * | 2
+ | * / 2
| |/
- * | 1
+ * / 1
|/
* initial
EOF
@@ -164,20 +161,19 @@ test_expect_success 'log --graph with tricky octopus merge and its child, no col
test_cmp expect.uncolored actual
'
-test_expect_failure 'log --graph with tricky octopus merge and its child with colors' '
+test_expect_success 'log --graph with tricky octopus merge and its child with colors' '
test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
cat >expect.colors <<-\EOF &&
* left
<RED>|<RESET> * after-merge
- <RED>|<RESET> *<MAGENTA>-<RESET><MAGENTA>-<RESET><CYAN>-<RESET><CYAN>.<RESET> octopus-merge
- <RED>|<RESET> <RED>|<RESET><BLUE>\<RESET> <MAGENTA>\<RESET> <CYAN>\<RESET>
- <RED>|<RESET><RED>/<RESET> <BLUE>/<RESET> <MAGENTA>/<RESET> <CYAN>/<RESET>
+ <RED>|<RESET> *<CYAN>-<RESET><CYAN>.<RESET> octopus-merge
+ <RED>|<RESET><RED>/<RESET><BLUE>|<RESET><MAGENTA>\<RESET> <CYAN>\<RESET>
<RED>|<RESET> <BLUE>|<RESET> <MAGENTA>|<RESET> * 4
<RED>|<RESET> <BLUE>|<RESET> * <CYAN>|<RESET> 3
<RED>|<RESET> <BLUE>|<RESET> <CYAN>|<RESET><CYAN>/<RESET>
- <RED>|<RESET> * <CYAN>|<RESET> 2
+ <RED>|<RESET> * <CYAN>/<RESET> 2
<RED>|<RESET> <CYAN>|<RESET><CYAN>/<RESET>
- * <CYAN>|<RESET> 1
+ * <CYAN>/<RESET> 1
<CYAN>|<RESET><CYAN>/<RESET>
* initial
EOF
@@ -209,7 +205,7 @@ test_expect_success 'log --graph with crossover in octopus merge, no color' '
test_cmp expect.uncolored actual
'
-test_expect_failure 'log --graph with crossover in octopus merge with colors' '
+test_expect_success 'log --graph with crossover in octopus merge with colors' '
test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
cat >expect.colors <<-\EOF &&
* after-4
@@ -257,7 +253,7 @@ test_expect_success 'log --graph with crossover in octopus merge and its child,
test_cmp expect.uncolored actual
'
-test_expect_failure 'log --graph with crossover in octopus merge and its child with colors' '
+test_expect_success 'log --graph with crossover in octopus merge and its child with colors' '
test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
cat >expect.colors <<-\EOF &&
* after-4
@@ -353,7 +349,7 @@ test_expect_success 'log --graph with unrelated commit and octopus child, no col
test_cmp expect.uncolored actual
'
-test_expect_failure 'log --graph with unrelated commit and octopus child with colors' '
+test_expect_success 'log --graph with unrelated commit and octopus child with colors' '
test_config log.graphColors red,green,yellow,blue,magenta,cyan &&
cat >expect.colors <<-\EOF &&
* after-initial
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Thu, Oct 10, 2019 at 11:16:24AM -0700, Denton Liu wrote:
> > test_expect_success 'log --graph with normal octopus merge, no color' '
>
> So, I decided to merge 'dl/octopus-graph-bug' with your branch and I
> couldn't be happier! I had to make a couple of minor tweaks to the
> existing test cases but most of them only involved flipping
> `test_expect_failure` to `test_expect_success`.
Thanks for doing that. I also checked the output on the git.git case
discussed in:
https://public-inbox.org/git/[email protected]/
and it looks great. Probably because it's the same as your test cases --
the "non-collapsing type" I called it there (or maybe it's all just the
out-of-order thing James brought up; I don't recall discussing that
before, but certainly his test case exhibits the same off-by-one
coloring weirdness without the code change).
-Peff
graph.c
Outdated
| @@ -115,11 +115,20 @@ static const char *column_get_color_code(unsigned short color) | |||
| static void strbuf_write_column(struct strbuf *sb, const struct column *c, | |||
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi James,
On Thu, 10 Oct 2019, James Coglan via GitGitGadget wrote:
> From: James Coglan <[email protected]>
>
> All the output functions in `graph.c` currently keep track of how many
> printable chars they've written to the buffer, before calling
> `graph_pad_horizontally()` to pad the line with spaces. Some functions
> do this by incrementing a counter whenever they write to the buffer, and
> others do it by encoding an assumption about how many chars are written,
> as in:
>
> graph_pad_horizontally(graph, sb, graph->num_columns * 2);
>
> This adds a fair amount of noise to the functions' logic and is easily
> broken if one forgets to increment the right counter or update the
> calculations used for padding.
>
> To make this easier to use, I'm adding a `width` field to `strbuf` that
> tracks the number of printing characters added after the line prefix.
This is a big heavy-handed: adding a `width` field to `struct strbuf`
and maintaining it _just_ for the purpose of `graph.c` puts an
unnecssary load on every other `strbuf` user (of which there are a
_lot_).
So my obvious question is: what makes `width` different from `len`?
Since we exclusively use ASCII characters for the graph part, we should
be able to use the already-existing `len`, for free, no?
I could imagine that the `strbuf` might receive more than one line, but
then we still would only need to remember the offset of the last newline
character in that `strbuf`, no?
Ciao,
Johannes
> It's set to 0 at the start of `graph_next_line()`, and then various
> `strbuf` functions update it as follows:
>
> - `strbuf_write_column()` increments `width` by 1
>
> - `strbuf_setlen()` changes `width` by the amount added to `len` if
> `len` is increased, or makes `width` and `len` the same if it's
> decreased
>
> - `strbuf_addch()` increments `width` by 1
>
> This is enough to ensure that the functions used by `graph.c` update
> `strbuf->width` correctly, and `graph_pad_horizontally()` can then use
> this field instead of taking `chars_written` as a parameter.
>
> Signed-off-by: James Coglan <[email protected]>
> ---
> graph.c | 68 ++++++++++++++++++++++----------------------------------
> strbuf.h | 8 ++++++-
> 2 files changed, 33 insertions(+), 43 deletions(-)
>
> diff --git a/graph.c b/graph.c
> index f53135485f..c56fdec1fc 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -115,11 +115,20 @@ 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)
> {
> + /*
> + * Remember the buffer's width as we're about to add non-printing
> + * content to it, and we want to avoid counting the byte length
> + * of this content towards the buffer's visible width
> + */
> + size_t prev_width =3D sb->width;
> +
> if (c->color < column_colors_max)
> strbuf_addstr(sb, column_get_color_code(c->color));
> strbuf_addch(sb, col_char);
> if (c->color < column_colors_max)
> strbuf_addstr(sb, column_get_color_code(column_colors_max));
> +
> + sb->width =3D prev_width + 1;
> }
>
> struct git_graph {
> @@ -686,8 +695,7 @@ static int graph_is_mapping_correct(struct git_graph=
*graph)
> return 1;
> }
>
> -static void graph_pad_horizontally(struct git_graph *graph, struct strb=
uf *sb,
> - int chars_written)
> +static void graph_pad_horizontally(struct git_graph *graph, struct strb=
uf *sb)
> {
> /*
> * Add additional spaces to the end of the strbuf, so that all
> @@ -696,8 +704,8 @@ static void graph_pad_horizontally(struct git_graph =
*graph, struct strbuf *sb,
> * This way, fields printed to the right of the graph will remain
> * aligned for the entire commit.
> */
> - if (chars_written < graph->width)
> - strbuf_addchars(sb, ' ', graph->width - chars_written);
> + if (sb->width < graph->width)
> + strbuf_addchars(sb, ' ', graph->width - sb->width);
> }
>
> static void graph_output_padding_line(struct git_graph *graph,
> @@ -723,7 +731,7 @@ static void graph_output_padding_line(struct git_gra=
ph *graph,
> strbuf_addch(sb, ' ');
> }
>
> - graph_pad_horizontally(graph, sb, graph->num_new_columns * 2);
> + graph_pad_horizontally(graph, sb);
> }
>
>
> @@ -740,7 +748,7 @@ static void graph_output_skip_line(struct git_graph =
*graph, struct strbuf *sb)
> * of the graph is missing.
> */
> strbuf_addstr(sb, "...");
> - graph_pad_horizontally(graph, sb, 3);
> + graph_pad_horizontally(graph, sb);
>
> if (graph->num_parents >=3D 3 &&
> graph->commit_index < (graph->num_columns - 1))
> @@ -754,7 +762,6 @@ static void graph_output_pre_commit_line(struct git_=
graph *graph,
> {
> int num_expansion_rows;
> int i, seen_this;
> - int chars_written;
>
> /*
> * This function formats a row that increases the space around a commi=
t
> @@ -777,14 +784,12 @@ static void graph_output_pre_commit_line(struct gi=
t_graph *graph,
> * Output the row
> */
> seen_this =3D 0;
> - chars_written =3D 0;
> for (i =3D 0; i < graph->num_columns; i++) {
> struct column *col =3D &graph->columns[i];
> if (col->commit =3D=3D graph->commit) {
> seen_this =3D 1;
> strbuf_write_column(sb, col, '|');
> strbuf_addchars(sb, ' ', graph->expansion_row);
> - chars_written +=3D 1 + graph->expansion_row;
> } else if (seen_this && (graph->expansion_row =3D=3D 0)) {
> /*
> * This is the first line of the pre-commit output.
> @@ -800,19 +805,15 @@ static void graph_output_pre_commit_line(struct gi=
t_graph *graph,
> strbuf_write_column(sb, col, '\\');
> else
> strbuf_write_column(sb, col, '|');
> - chars_written++;
> } else if (seen_this && (graph->expansion_row > 0)) {
> strbuf_write_column(sb, col, '\\');
> - chars_written++;
> } else {
> strbuf_write_column(sb, col, '|');
> - chars_written++;
> }
> strbuf_addch(sb, ' ');
> - chars_written++;
> }
>
> - graph_pad_horizontally(graph, sb, chars_written);
> + graph_pad_horizontally(graph, sb);
>
> /*
> * Increment graph->expansion_row,
> @@ -842,11 +843,9 @@ static void graph_output_commit_char(struct git_gra=
ph *graph, struct strbuf *sb)
> }
>
> /*
> - * Draw the horizontal dashes of an octopus merge and return the number=
of
> - * characters written.
> + * Draw the horizontal dashes of an octopus merge.
> */
> -static int graph_draw_octopus_merge(struct git_graph *graph,
> - struct strbuf *sb)
> +static void graph_draw_octopus_merge(struct git_graph *graph, struct st=
rbuf *sb)
> {
> /*
> * Here dashless_parents represents the number of parents which don't
> @@ -890,13 +889,12 @@ static int graph_draw_octopus_merge(struct git_gra=
ph *graph,
> strbuf_write_column(sb, &graph->new_columns[i+first_col],
> i =3D=3D dashful_parents-1 ? '.' : '-');
> }
> - return 2 * dashful_parents;
> }
>
> static void graph_output_commit_line(struct git_graph *graph, struct st=
rbuf *sb)
> {
> int seen_this =3D 0;
> - int i, chars_written;
> + int i;
>
> /*
> * Output the row containing this commit
> @@ -906,7 +904,6 @@ static void graph_output_commit_line(struct git_grap=
h *graph, struct strbuf *sb)
> * children that we have already processed.)
> */
> seen_this =3D 0;
> - chars_written =3D 0;
> for (i =3D 0; i <=3D graph->num_columns; i++) {
> struct column *col =3D &graph->columns[i];
> struct commit *col_commit;
> @@ -921,14 +918,11 @@ static void graph_output_commit_line(struct git_gr=
aph *graph, struct strbuf *sb)
> if (col_commit =3D=3D graph->commit) {
> seen_this =3D 1;
> graph_output_commit_char(graph, sb);
> - chars_written++;
>
> if (graph->num_parents > 2)
> - chars_written +=3D graph_draw_octopus_merge(graph,
> - sb);
> + graph_draw_octopus_merge(graph, sb);
> } else if (seen_this && (graph->num_parents > 2)) {
> strbuf_write_column(sb, col, '\\');
> - chars_written++;
> } else if (seen_this && (graph->num_parents =3D=3D 2)) {
> /*
> * This is a 2-way merge commit.
> @@ -948,16 +942,13 @@ static void graph_output_commit_line(struct git_gr=
aph *graph, struct strbuf *sb)
> strbuf_write_column(sb, col, '\\');
> else
> strbuf_write_column(sb, col, '|');
> - chars_written++;
> } else {
> strbuf_write_column(sb, col, '|');
> - chars_written++;
> }
> strbuf_addch(sb, ' ');
> - chars_written++;
> }
>
> - graph_pad_horizontally(graph, sb, chars_written);
> + graph_pad_horizontally(graph, sb);
>
> /*
> * Update graph->state
> @@ -984,12 +975,11 @@ static struct column *find_new_column_by_commit(st=
ruct git_graph *graph,
> static void graph_output_post_merge_line(struct git_graph *graph, struc=
t strbuf *sb)
> {
> int seen_this =3D 0;
> - int i, j, chars_written;
> + int i, j;
>
> /*
> * Output the post-merge row
> */
> - chars_written =3D 0;
> for (i =3D 0; i <=3D graph->num_columns; i++) {
> struct column *col =3D &graph->columns[i];
> struct commit *col_commit;
> @@ -1017,7 +1007,6 @@ static void graph_output_post_merge_line(struct gi=
t_graph *graph, struct strbuf
> assert(par_column);
>
> strbuf_write_column(sb, par_column, '|');
> - chars_written++;
> for (j =3D 0; j < graph->num_parents - 1; j++) {
> parents =3D next_interesting_parent(graph, parents);
> assert(parents);
> @@ -1026,19 +1015,16 @@ static void graph_output_post_merge_line(struct =
git_graph *graph, struct strbuf
> strbuf_write_column(sb, par_column, '\\');
> strbuf_addch(sb, ' ');
> }
> - chars_written +=3D j * 2;
> } else if (seen_this) {
> strbuf_write_column(sb, col, '\\');
> strbuf_addch(sb, ' ');
> - chars_written +=3D 2;
> } else {
> strbuf_write_column(sb, col, '|');
> strbuf_addch(sb, ' ');
> - chars_written +=3D 2;
> }
> }
>
> - graph_pad_horizontally(graph, sb, chars_written);
> + graph_pad_horizontally(graph, sb);
>
> /*
> * Update graph->state
> @@ -1181,7 +1167,7 @@ static void graph_output_collapsing_line(struct gi=
t_graph *graph, struct strbuf
> }
> }
>
> - graph_pad_horizontally(graph, sb, graph->mapping_size);
> + graph_pad_horizontally(graph, sb);
>
> /*
> * Swap mapping and new_mapping
> @@ -1199,6 +1185,8 @@ static void graph_output_collapsing_line(struct gi=
t_graph *graph, struct strbuf
>
> int graph_next_line(struct git_graph *graph, struct strbuf *sb)
> {
> + sb->width =3D 0;
> +
> switch (graph->state) {
> case GRAPH_PADDING:
> graph_output_padding_line(graph, sb);
> @@ -1227,7 +1215,6 @@ int graph_next_line(struct git_graph *graph, struc=
t strbuf *sb)
> static void graph_padding_line(struct git_graph *graph, struct strbuf *=
sb)
> {
> int i;
> - int chars_written =3D 0;
>
> if (graph->state !=3D GRAPH_COMMIT) {
> graph_next_line(graph, sb);
> @@ -1245,19 +1232,16 @@ static void graph_padding_line(struct git_graph =
*graph, struct strbuf *sb)
> struct column *col =3D &graph->columns[i];
>
> strbuf_write_column(sb, col, '|');
> - chars_written++;
>
> if (col->commit =3D=3D graph->commit && graph->num_parents > 2) {
> int len =3D (graph->num_parents - 2) * 2;
> strbuf_addchars(sb, ' ', len);
> - chars_written +=3D len;
> } else {
> strbuf_addch(sb, ' ');
> - chars_written++;
> }
> }
>
> - graph_pad_horizontally(graph, sb, chars_written);
> + graph_pad_horizontally(graph, sb);
>
> /*
> * Update graph->prev_state since we have output a padding line
> diff --git a/strbuf.h b/strbuf.h
> index f62278a0be..3a98147321 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -66,11 +66,12 @@ struct string_list;
> struct strbuf {
> size_t alloc;
> size_t len;
> + size_t width;
> char *buf;
> };
>
> extern char strbuf_slopbuf[];
> -#define STRBUF_INIT { .alloc =3D 0, .len =3D 0, .buf =3D strbuf_slopbu=
f }
> +#define STRBUF_INIT { .alloc =3D 0, .len =3D 0, .width =3D 0, .buf =3D=
strbuf_slopbuf }
>
> /*
> * Predeclare this here, since cache.h includes this file before it def=
ines the
> @@ -161,6 +162,10 @@ static inline void strbuf_setlen(struct strbuf *sb,=
size_t len)
> {
> if (len > (sb->alloc ? sb->alloc - 1 : 0))
> die("BUG: strbuf_setlen() beyond buffer");
> + if (len > sb->len)
> + sb->width +=3D len - sb->len;
> + else
> + sb->width =3D len;
> sb->len =3D len;
> if (sb->buf !=3D strbuf_slopbuf)
> sb->buf[len] =3D '\0';
> @@ -231,6 +236,7 @@ static inline void strbuf_addch(struct strbuf *sb, i=
nt c)
> strbuf_grow(sb, 1);
> sb->buf[sb->len++] =3D c;
> sb->buf[sb->len] =3D '\0';
> + sb->width++;
> }
>
> /**
> --
> gitgitgadget
>
>
There was a problem hiding this comment.
On the Git mailing list, Denton Liu wrote (reply to this):
Hi Dscho,
On Thu, Oct 10, 2019 at 11:07:35PM +0200, Johannes Schindelin wrote:
> Hi James,
>
> On Thu, 10 Oct 2019, James Coglan via GitGitGadget wrote:
>
> > From: James Coglan <[email protected]>
> >
> > All the output functions in `graph.c` currently keep track of how many
> > printable chars they've written to the buffer, before calling
> > `graph_pad_horizontally()` to pad the line with spaces. Some functions
> > do this by incrementing a counter whenever they write to the buffer, and
> > others do it by encoding an assumption about how many chars are written,
> > as in:
> >
> > graph_pad_horizontally(graph, sb, graph->num_columns * 2);
> >
> > This adds a fair amount of noise to the functions' logic and is easily
> > broken if one forgets to increment the right counter or update the
> > calculations used for padding.
> >
> > To make this easier to use, I'm adding a `width` field to `strbuf` that
> > tracks the number of printing characters added after the line prefix.
>
> This is a big heavy-handed: adding a `width` field to `struct strbuf`
> and maintaining it _just_ for the purpose of `graph.c` puts an
> unnecssary load on every other `strbuf` user (of which there are a
> _lot_).
>
> So my obvious question is: what makes `width` different from `len`?
> Since we exclusively use ASCII characters for the graph part, we should
> be able to use the already-existing `len`, for free, no?
From what I can gleam from looking at the code, `width` is different
from `len` because when we're printing with colours, there'll be a bunch
of termcodes that don't actually count for the width.
I think that we should either leave the `chars_written` variable as is
or maybe calculate it after the fact. Here's an untested and uncompiled
implementation of something that might do that:
static int calculate_width(const struct strbuf *row)
{
int in_termcode = 0;
int width = 0;
int i;
for (i = 0; i < row.len; i++) {
if (row.buf[i] == '\033')
in_termcode = 1;
if (!in_termcode)
width++;
else if (row.buf[i] == 'm')
in_termcode = 0;
}
}
If we include this, I'm not sure what kind of performance hit we might
take if the graph we're generating is particularly big, though.
>
> I could imagine that the `strbuf` might receive more than one line, but
> then we still would only need to remember the offset of the last newline
> character in that `strbuf`, no?
>
> Ciao,
> Johannes
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 10/10/2019 7:05 PM, Denton Liu wrote:
> Hi Dscho,
>
> On Thu, Oct 10, 2019 at 11:07:35PM +0200, Johannes Schindelin wrote:
>> Hi James,
>>
>> On Thu, 10 Oct 2019, James Coglan via GitGitGadget wrote:
>>
>>> From: James Coglan <[email protected]>
>>>
>>> All the output functions in `graph.c` currently keep track of how many
>>> printable chars they've written to the buffer, before calling
>>> `graph_pad_horizontally()` to pad the line with spaces. Some functions
>>> do this by incrementing a counter whenever they write to the buffer, and
>>> others do it by encoding an assumption about how many chars are written,
>>> as in:
>>>
>>> graph_pad_horizontally(graph, sb, graph->num_columns * 2);
>>>
>>> This adds a fair amount of noise to the functions' logic and is easily
>>> broken if one forgets to increment the right counter or update the
>>> calculations used for padding.
>>>
>>> To make this easier to use, I'm adding a `width` field to `strbuf` that
>>> tracks the number of printing characters added after the line prefix.
>>
>> This is a big heavy-handed: adding a `width` field to `struct strbuf`
>> and maintaining it _just_ for the purpose of `graph.c` puts an
>> unnecssary load on every other `strbuf` user (of which there are a
>> _lot_).
I was concerned about this, too.
>> So my obvious question is: what makes `width` different from `len`?
>> Since we exclusively use ASCII characters for the graph part, we should
>> be able to use the already-existing `len`, for free, no?
>
> From what I can gleam from looking at the code, `width` is different
> from `len` because when we're printing with colours, there'll be a bunch
> of termcodes that don't actually count for the width.
>
> I think that we should either leave the `chars_written` variable as is
> or maybe calculate it after the fact. Here's an untested and uncompiled
> implementation of something that might do that:
>
> static int calculate_width(const struct strbuf *row)
> {
> int in_termcode = 0;
> int width = 0;
> int i;
>
> for (i = 0; i < row.len; i++) {
> if (row.buf[i] == '\033')
> in_termcode = 1;
>
> if (!in_termcode)
> width++;
> else if (row.buf[i] == 'm')
> in_termcode = 0;
> }
> }
>
> If we include this, I'm not sure what kind of performance hit we might
> take if the graph we're generating is particularly big, though.
This is worth trying. In terms of CPU time, this should be a mere
blip compared to all of the commit walking that is going on. (Unless
we get to thousands of columns, but by then the output is useless.)
Of course, we should measure the impact, but it is worth a try.
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <[email protected]> writes:
> This is a big heavy-handed: adding a `width` field to `struct strbuf`
> and maintaining it _just_ for the purpose of `graph.c` puts an
> unnecssary load on every other `strbuf` user (of which there are a
> _lot_).
>
> So my obvious question is: what makes `width` different from `len`?
> Since we exclusively use ASCII characters for the graph part, we should
> be able to use the already-existing `len`, for free, no?
A red-colored piece on the line consumes <RED><RESET> bytes more
than the payload. Which is counted as part of "len". These bytes
do not consume any display width.
When the payload is a basic CJK char in UTF-8 it may typically use 3
byte, while consuming only two display columns.
So I can understand that this application may want to keep track of
<byte sequence, byte sequence length, display width> tuple.
I also understand that a programmer inexperienced/unfamiliar with
our codebase may find it an easy way to satisfiy the need to add an
extra field to strbuf. But as you pointed out, that is a hack
unacceptable in the larger picture. Use of strbuf as "auto resizing
byte array, represented as a <byte sequence, byte sequence length>
tuple" is everywhere and we do not want to bloat it.
Thanks for spotting and raising this unfortunate show-stopper issue.
The problem being solved is worth solving, but it needs to be done
without butchering a basic data structure used elsewhere.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Denton Liu <[email protected]> writes:
> static int calculate_width(const struct strbuf *row)
> {
> int in_termcode = 0;
> int width = 0;
> int i;
>
> for (i = 0; i < row.len; i++) {
> if (row.buf[i] == '\033')
> in_termcode = 1;
>
> if (!in_termcode)
> width++;
> else if (row.buf[i] == 'm')
> in_termcode = 0;
> }
> }
Not every byte that is outside the escape sequence contributes to
one display columns. You would want to take a look at utf8_width()
for inspiration.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
James Coglan <[email protected]> writes:
> - We don't want a general solution to this problem for everything
> `strbuf` could be used for; it only needs to address the graph
> padding problem.
Of course. Somebody may use strbuf to hold rows of a table and you
do not want to contaminate strbuf with fields like width of each
column etc, that are very specific to the application. IOW, strbuf
is merely _one_ component of a larger solution to each specific
problem, and the latter may be things like "struct graphbuf" like
Dscho suggested, which might use strbuf as an underlying
<byte-string, length> tuple mechanism, but that is an implementation
detail that should not be exposed to the users of the struct (and
that is why he did not call, and you should not call, the data
structure "graph-strbuf" or anything with "strbuf").
> - We only want to count printing characters, not color formatting sequences.
OK. But I'd phrase "count printing characters" as "measure display
width" for at least two reasons. Whitespaces are typically counted
as non-printing, but you do want to take them into account for this
application. Also the graph may not be limited to ASCII graphics
forever, and byte- or character-count may not match display width on
a fixed-width display.
> - We only need to consider the width of a small set of characters:
> { | / \ _ - . * }. We don't need to worry about Unicode, and the
> simple character counting in graph.c was working fine.
I have to warn you that we saw attempts to step outside these ASCII
graphics and use Unicode characters for prettier output in the past.
If you can do so without too much complexity, I'd suggest you try
not to close the door to those people who follow your footsteps to
further improve the system by pursuing the avenue further.
> To this end I've prepared a different implementation that
> introduces the indirection described above, and does not modify
> `strbuf`:
>
> +struct graph_strbuf {
> + struct strbuf *buf;
> + size_t width;
> +};
Is there a reason why you need a pointer to a strbuf that is
allocated separately? E.g. would it make it harder to manage
if the above were
struct graphbuf {
struct strbuf buf;
int width; /* display width in columns */
};
which is essentially what Dscho suggested?
> +static inline void graph_strbuf_addch(struct graph_strbuf *sb, int c)
> +{
> + strbuf_addch(sb->buf, c);
> + sb->width++;
> +}
> +
> +void graph_strbuf_addchars(struct graph_strbuf *sb, int c, size_t n)
> +{
> + strbuf_addchars(sb->buf, c, n);
> + sb->width += n;
> +}
> +
> +static inline void graph_strbuf_addstr(struct graph_strbuf *sb, const char *s)
> +{
> + strbuf_addstr(sb->buf, s);
> + sb->width += strlen(s);
> +}
I'd probably introduce another helper that takes color code and
graphbuf (also notice how I name the variables and types; calling
something sb that is not a strbuf is misleading and inviting
unnecessary bugs):
static inline void graphbuf_addcolor(struct graphbuf *gb, unsigned short color)
{
strbuf_addstr(gb->buf, column_get_color_code(color));
}
if I were writing this code. The goal is to make any strbuf_add*()
that is done directly on gb->buf outside these helpers a bug--that
way, grepping for strbuf_add* in this file would become a very easy
way to catch such a bug that bypasses the graphbuf_add*() layer and
potentially screw up the column counting.
Other than these three points (i.e. naming without "strbuf" that is
an implementation detail, embedded vs pointer of strbuf in the
graphbuf, and making sure everything can be done via graphbuf_add*()
wrappers to make it easier to spot bugs), it seems you are going in
the right direction. Thanks for working on this.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi,
I just realized that I completely failed to offer my enthusiasm not only
for the idea of this patch series and the resulting graphs, but also at
the really high quality of the contribution. Thanks, James, for working
on this!
And now just quickly...
On Sat, 12 Oct 2019, Junio C Hamano wrote:
> James Coglan <[email protected]> writes:
>
> > - We only need to consider the width of a small set of characters:
> > { | / \ _ - . * }. We don't need to worry about Unicode, and the
> > simple character counting in graph.c was working fine.
>
> I have to warn you that we saw attempts to step outside these ASCII
> graphics and use Unicode characters for prettier output in the past.
> If you can do so without too much complexity, I'd suggest you try
> not to close the door to those people who follow your footsteps to
> further improve the system by pursuing the avenue further.
That is a very valid point, I think. There are really pretty Unicode
characters out there that I could totally see in a nice commit graph.
At that stage, we would have to use `int utf8_strnwidth(const char
*string, int len, int skip_ansi)` anyway (because of the `skip_ansi`
that saves us _a lot_ of work). In this case, I doubt that it makes
sense to keep a running tally of the width. It would be faster, or at
least similarly fast, to just run `utf8_strnwidth()` on the final string
that we want to measure.
Ciao,
Dscho
There was a problem hiding this comment.
On the Git mailing list, James Coglan wrote (reply to this):
On 12/10/2019 01:27, Junio C Hamano wrote:
> James Coglan <[email protected]> writes:
>
>> - We don't want a general solution to this problem for everything
>> `strbuf` could be used for; it only needs to address the graph
>> padding problem.
>
> Of course. Somebody may use strbuf to hold rows of a table and you
> do not want to contaminate strbuf with fields like width of each
> column etc, that are very specific to the application. IOW, strbuf
> is merely _one_ component of a larger solution to each specific
> problem, and the latter may be things like "struct graphbuf" like
> Dscho suggested, which might use strbuf as an underlying
> <byte-string, length> tuple mechanism, but that is an implementation
> detail that should not be exposed to the users of the struct (and
> that is why he did not call, and you should not call, the data
> structure "graph-strbuf" or anything with "strbuf").
Thank you for the clarification. I've now modified my patch to call the wrapper struct `graph_line` to better reflect its use. This was informed by uncertainty on this thread about whether the width calculation needs to take line breaks into account, so I wanted to name it to indicate it contains a single display line.
>> - We only want to count printing characters, not color formatting sequences.
>
> OK. But I'd phrase "count printing characters" as "measure display
> width" for at least two reasons. Whitespaces are typically counted
> as non-printing, but you do want to take them into account for this
> application. Also the graph may not be limited to ASCII graphics
> forever, and byte- or character-count may not match display width on
> a fixed-width display.
>
>> - We only need to consider the width of a small set of characters:
>> { | / \ _ - . * }. We don't need to worry about Unicode, and the
>> simple character counting in graph.c was working fine.
>
> I have to warn you that we saw attempts to step outside these ASCII
> graphics and use Unicode characters for prettier output in the past.
> If you can do so without too much complexity, I'd suggest you try
> not to close the door to those people who follow your footsteps to
> further improve the system by pursuing the avenue further.
That makes sense. All I've done for now is to put the calculations that were already being done behind an interface, consisting of just the operations graph.c actually uses, namely:
void graph_line_addch(struct graph_line *line, int c);
void graph_line_addchars(struct graph_line *line, int c, size_t n);
void graph_line_addstr(struct graph_line *line, const char *s);
void graph_line_write_column(struct graph_line *line, const struct column *c, char col_char);
Having this interface in place should not preclude extending this functionality to Unicode later, and may make it simpler as it puts all the disply width calculations in one place, rather than its current state where it's distributed across all the output functions and makes the assumption that all chars contribute one display column. It especially removes the need for statements like this that encode an assumption about what was printed:
graph_pad_horizontally(graph, sb, graph->num_columns * 2);
>> To this end I've prepared a different implementation that
>> introduces the indirection described above, and does not modify
>> `strbuf`:
>>
>> +struct graph_strbuf {
>> + struct strbuf *buf;
>> + size_t width;
>> +};
>
> Is there a reason why you need a pointer to a strbuf that is
> allocated separately? E.g. would it make it harder to manage
> if the above were
>
> struct graphbuf {
> struct strbuf buf;
> int width; /* display width in columns */
> };
>
> which is essentially what Dscho suggested?
I used a pointer here because I create the wrapper struct in `graph_next_line()`, which is an external interface that takes a `struct strbuf *`:
int graph_next_line(struct git_graph *graph, struct strbuf *sb)
{
struct graph_line line = { .buf = sb, .width = 0 };
// ...
}
So I'm not allocating the strbuf here, just wrapping a pointer to it. I would prefer to define `graph_line` as having a `strbuf` inline but I've not found a way to do that without breaking the other functions that call `graph_next_line()`. Although, as far as I know both this wrapper struct and every `strbuf` it points to are stack-allocated so I'm more concerned about the extra dereference rather than allocation cost.
If there's a way to do this I'm open to suggestions.
>> +static inline void graph_strbuf_addch(struct graph_strbuf *sb, int c)
>> +{
>> + strbuf_addch(sb->buf, c);
>> + sb->width++;
>> +}
>> +
>> +void graph_strbuf_addchars(struct graph_strbuf *sb, int c, size_t n)
>> +{
>> + strbuf_addchars(sb->buf, c, n);
>> + sb->width += n;
>> +}
>> +
>> +static inline void graph_strbuf_addstr(struct graph_strbuf *sb, const char *s)
>> +{
>> + strbuf_addstr(sb->buf, s);
>> + sb->width += strlen(s);
>> +}
>
> I'd probably introduce another helper that takes color code and
> graphbuf (also notice how I name the variables and types; calling
> something sb that is not a strbuf is misleading and inviting
> unnecessary bugs):
>
> static inline void graphbuf_addcolor(struct graphbuf *gb, unsigned short color)
> {
> strbuf_addstr(gb->buf, column_get_color_code(color));
> }
>
> if I were writing this code. The goal is to make any strbuf_add*()
> that is done directly on gb->buf outside these helpers a bug--that
> way, grepping for strbuf_add* in this file would become a very easy
> way to catch such a bug that bypasses the graphbuf_add*() layer and
> potentially screw up the column counting.
That would be great, I'm just not sure how to fully decouple graph.c from `strbuf` so that I can properly block it from using the `strbuf` interface. What I have done is got a better separation between uses of `strbuf` and `graph_line`, so that the only functions that still use the `strbuf` interface are the `graph_line` interface, and functions that create a `strbuf` themselves (e.g. `graph_show_commit()`). Hopefully that separation makes the intent clearer and is a stepping stone to fully decoupling these interfaces.
> Other than these three points (i.e. naming without "strbuf" that is
> an implementation detail, embedded vs pointer of strbuf in the
> graphbuf, and making sure everything can be done via graphbuf_add*()
> wrappers to make it easier to spot bugs), it seems you are going in
> the right direction. Thanks for working on this.
Thanks! For completeness, here is the current state of this patch after I've been through the feedback up to this point:
From 241180570fbaae04a2263c5ff1ab3b92f54f8004 Mon Sep 17 00:00:00 2001
From: James Coglan <[email protected]>
Date: Thu, 22 Aug 2019 09:30:08 +0100
Subject: [PATCH] graph: automatically track display width of graph lines
All the output functions called by `graph_next_line()` currently keep
track of how many printable chars they've written to the buffer, before
calling `graph_pad_horizontally()` to pad the line with spaces. Some
functions do this by incrementing a counter whenever they write to the
buffer, and others do it by encoding an assumption about how many chars
are written, as in:
graph_pad_horizontally(graph, sb, graph->num_columns * 2);
This adds a fair amount of noise to the functions' logic and is easily
broken if one forgets to increment the right counter or update the
calculations used for padding.
To make this easier to use, I'm introducing a new struct called
`graph_line` that wraps a `strbuf` and keeps count of its display width
implicitly. `graph_next_line()` wraps this around the `struct strbuf *`
it's given and passes a `struct graph_line *` to the output functions,
which use its interface.
The `graph_line` interface wraps the `strbuf_addch()`,
`strbuf_addchars()` and `strbuf_addstr()` functions, and adds the
`graph_line_write_column()` function for adding a single character with
color formatting. The `graph_pad_horizontally()` function can then use
the `width` field from the struct rather than taking a character count
as a parameter.
Signed-off-by: James Coglan <[email protected]>
---
graph.c | 194 +++++++++++++++++++++++++++++---------------------------
1 file changed, 99 insertions(+), 95 deletions(-)
diff --git a/graph.c b/graph.c
index f53135485f..2f81a5d23d 100644
--- a/graph.c
+++ b/graph.c
@@ -112,14 +112,42 @@ static const char *column_get_color_code(unsigned short color)
return column_colors[color];
}
-static void strbuf_write_column(struct strbuf *sb, const struct column *c,
- char col_char)
+struct graph_line {
+ struct strbuf *buf;
+ size_t width;
+};
+
+static inline void graph_line_addch(struct graph_line *line, int c)
+{
+ strbuf_addch(line->buf, c);
+ line->width++;
+}
+
+static inline void graph_line_addchars(struct graph_line *line, int c, size_t n)
+{
+ strbuf_addchars(line->buf, c, n);
+ line->width += n;
+}
+
+static inline void graph_line_addstr(struct graph_line *line, const char *s)
+{
+ strbuf_addstr(line->buf, s);
+ line->width += strlen(s);
+}
+
+static inline void graph_line_addcolor(struct graph_line *line, unsigned short color)
+{
+ strbuf_addstr(line->buf, column_get_color_code(color));
+}
+
+static void graph_line_write_column(struct graph_line *line, const struct column *c,
+ char col_char)
{
if (c->color < column_colors_max)
- strbuf_addstr(sb, column_get_color_code(c->color));
- strbuf_addch(sb, col_char);
+ graph_line_addcolor(line, c->color);
+ graph_line_addch(line, col_char);
if (c->color < column_colors_max)
- strbuf_addstr(sb, column_get_color_code(column_colors_max));
+ graph_line_addcolor(line, column_colors_max);
}
struct git_graph {
@@ -686,8 +714,7 @@ static int graph_is_mapping_correct(struct git_graph *graph)
return 1;
}
-static void graph_pad_horizontally(struct git_graph *graph, struct strbuf *sb,
- int chars_written)
+static void graph_pad_horizontally(struct git_graph *graph, struct graph_line *line)
{
/*
* Add additional spaces to the end of the strbuf, so that all
@@ -696,12 +723,12 @@ static void graph_pad_horizontally(struct git_graph *graph, struct strbuf *sb,
* This way, fields printed to the right of the graph will remain
* aligned for the entire commit.
*/
- if (chars_written < graph->width)
- strbuf_addchars(sb, ' ', graph->width - chars_written);
+ if (line->width < graph->width)
+ graph_line_addchars(line, ' ', graph->width - line->width);
}
static void graph_output_padding_line(struct git_graph *graph,
- struct strbuf *sb)
+ struct graph_line *line)
{
int i;
@@ -719,11 +746,11 @@ static void graph_output_padding_line(struct git_graph *graph,
* Output a padding row, that leaves all branch lines unchanged
*/
for (i = 0; i < graph->num_new_columns; i++) {
- strbuf_write_column(sb, &graph->new_columns[i], '|');
- strbuf_addch(sb, ' ');
+ graph_line_write_column(line, &graph->new_columns[i], '|');
+ graph_line_addch(line, ' ');
}
- graph_pad_horizontally(graph, sb, graph->num_new_columns * 2);
+ graph_pad_horizontally(graph, line);
}
@@ -733,14 +760,14 @@ int graph_width(struct git_graph *graph)
}
-static void graph_output_skip_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_output_skip_line(struct git_graph *graph, struct graph_line *line)
{
/*
* Output an ellipsis to indicate that a portion
* of the graph is missing.
*/
- strbuf_addstr(sb, "...");
- graph_pad_horizontally(graph, sb, 3);
+ graph_line_addstr(line, "...");
+ graph_pad_horizontally(graph, line);
if (graph->num_parents >= 3 &&
graph->commit_index < (graph->num_columns - 1))
@@ -750,11 +777,10 @@ 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)
+ struct graph_line *line)
{
int num_expansion_rows;
int i, seen_this;
- int chars_written;
/*
* This function formats a row that increases the space around a commit
@@ -777,14 +803,12 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
* Output the row
*/
seen_this = 0;
- chars_written = 0;
for (i = 0; i < graph->num_columns; i++) {
struct column *col = &graph->columns[i];
if (col->commit == graph->commit) {
seen_this = 1;
- strbuf_write_column(sb, col, '|');
- strbuf_addchars(sb, ' ', graph->expansion_row);
- chars_written += 1 + graph->expansion_row;
+ graph_line_write_column(line, col, '|');
+ graph_line_addchars(line, ' ', graph->expansion_row);
} else if (seen_this && (graph->expansion_row == 0)) {
/*
* This is the first line of the pre-commit output.
@@ -797,22 +821,18 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
*/
if (graph->prev_state == GRAPH_POST_MERGE &&
graph->prev_commit_index < i)
- strbuf_write_column(sb, col, '\\');
+ graph_line_write_column(line, col, '\\');
else
- strbuf_write_column(sb, col, '|');
- chars_written++;
+ graph_line_write_column(line, col, '|');
} else if (seen_this && (graph->expansion_row > 0)) {
- strbuf_write_column(sb, col, '\\');
- chars_written++;
+ graph_line_write_column(line, col, '\\');
} else {
- strbuf_write_column(sb, col, '|');
- chars_written++;
+ graph_line_write_column(line, col, '|');
}
- strbuf_addch(sb, ' ');
- chars_written++;
+ graph_line_addch(line, ' ');
}
- graph_pad_horizontally(graph, sb, chars_written);
+ graph_pad_horizontally(graph, line);
/*
* Increment graph->expansion_row,
@@ -823,7 +843,7 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
graph_update_state(graph, GRAPH_COMMIT);
}
-static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
+static void graph_output_commit_char(struct git_graph *graph, struct graph_line *line)
{
/*
* For boundary commits, print 'o'
@@ -831,22 +851,20 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
*/
if (graph->commit->object.flags & BOUNDARY) {
assert(graph->revs->boundary);
- strbuf_addch(sb, 'o');
+ graph_line_addch(line, 'o');
return;
}
/*
* get_revision_mark() handles all other cases without assert()
*/
- strbuf_addstr(sb, get_revision_mark(graph->revs, graph->commit));
+ graph_line_addstr(line, get_revision_mark(graph->revs, graph->commit));
}
/*
- * Draw the horizontal dashes of an octopus merge and return the number of
- * characters written.
+ * Draw the horizontal dashes of an octopus merge.
*/
-static int graph_draw_octopus_merge(struct git_graph *graph,
- struct strbuf *sb)
+static void graph_draw_octopus_merge(struct git_graph *graph, struct graph_line *line)
{
/*
* Here dashless_parents represents the number of parents which don't
@@ -886,17 +904,16 @@ static int graph_draw_octopus_merge(struct git_graph *graph,
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 ? '.' : '-');
+ graph_line_write_column(line, &graph->new_columns[i+first_col], '-');
+ graph_line_write_column(line, &graph->new_columns[i+first_col],
+ i == dashful_parents-1 ? '.' : '-');
}
- return 2 * dashful_parents;
}
-static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_output_commit_line(struct git_graph *graph, struct graph_line *line)
{
int seen_this = 0;
- int i, chars_written;
+ int i;
/*
* Output the row containing this commit
@@ -906,7 +923,6 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
* children that we have already processed.)
*/
seen_this = 0;
- chars_written = 0;
for (i = 0; i <= graph->num_columns; i++) {
struct column *col = &graph->columns[i];
struct commit *col_commit;
@@ -920,15 +936,12 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
if (col_commit == graph->commit) {
seen_this = 1;
- graph_output_commit_char(graph, sb);
- chars_written++;
+ graph_output_commit_char(graph, line);
if (graph->num_parents > 2)
- chars_written += graph_draw_octopus_merge(graph,
- sb);
+ graph_draw_octopus_merge(graph, line);
} else if (seen_this && (graph->num_parents > 2)) {
- strbuf_write_column(sb, col, '\\');
- chars_written++;
+ graph_line_write_column(line, col, '\\');
} else if (seen_this && (graph->num_parents == 2)) {
/*
* This is a 2-way merge commit.
@@ -945,19 +958,16 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
*/
if (graph->prev_state == GRAPH_POST_MERGE &&
graph->prev_commit_index < i)
- strbuf_write_column(sb, col, '\\');
+ graph_line_write_column(line, col, '\\');
else
- strbuf_write_column(sb, col, '|');
- chars_written++;
+ graph_line_write_column(line, col, '|');
} else {
- strbuf_write_column(sb, col, '|');
- chars_written++;
+ graph_line_write_column(line, col, '|');
}
- strbuf_addch(sb, ' ');
- chars_written++;
+ graph_line_addch(line, ' ');
}
- graph_pad_horizontally(graph, sb, chars_written);
+ graph_pad_horizontally(graph, line);
/*
* Update graph->state
@@ -981,15 +991,14 @@ static struct column *find_new_column_by_commit(struct git_graph *graph,
return NULL;
}
-static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_output_post_merge_line(struct git_graph *graph, struct graph_line *line)
{
int seen_this = 0;
- int i, j, chars_written;
+ int i, j;
/*
* Output the post-merge row
*/
- chars_written = 0;
for (i = 0; i <= graph->num_columns; i++) {
struct column *col = &graph->columns[i];
struct commit *col_commit;
@@ -1016,29 +1025,25 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf
par_column = find_new_column_by_commit(graph, parents->item);
assert(par_column);
- strbuf_write_column(sb, par_column, '|');
- chars_written++;
+ graph_line_write_column(line, par_column, '|');
for (j = 0; j < graph->num_parents - 1; j++) {
parents = next_interesting_parent(graph, parents);
assert(parents);
par_column = find_new_column_by_commit(graph, parents->item);
assert(par_column);
- strbuf_write_column(sb, par_column, '\\');
- strbuf_addch(sb, ' ');
+ graph_line_write_column(line, par_column, '\\');
+ graph_line_addch(line, ' ');
}
- chars_written += j * 2;
} else if (seen_this) {
- strbuf_write_column(sb, col, '\\');
- strbuf_addch(sb, ' ');
- chars_written += 2;
+ graph_line_write_column(line, col, '\\');
+ graph_line_addch(line, ' ');
} else {
- strbuf_write_column(sb, col, '|');
- strbuf_addch(sb, ' ');
- chars_written += 2;
+ graph_line_write_column(line, col, '|');
+ graph_line_addch(line, ' ');
}
}
- graph_pad_horizontally(graph, sb, chars_written);
+ graph_pad_horizontally(graph, line);
/*
* Update graph->state
@@ -1049,7 +1054,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf
graph_update_state(graph, GRAPH_COLLAPSING);
}
-static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)
+static void graph_output_collapsing_line(struct git_graph *graph, struct graph_line *line)
{
int i;
short used_horizontal = 0;
@@ -1159,9 +1164,9 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
for (i = 0; i < graph->mapping_size; i++) {
int target = graph->new_mapping[i];
if (target < 0)
- strbuf_addch(sb, ' ');
+ graph_line_addch(line, ' ');
else if (target * 2 == i)
- strbuf_write_column(sb, &graph->new_columns[target], '|');
+ graph_line_write_column(line, &graph->new_columns[target], '|');
else if (target == horizontal_edge_target &&
i != horizontal_edge - 1) {
/*
@@ -1172,16 +1177,16 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
if (i != (target * 2)+3)
graph->new_mapping[i] = -1;
used_horizontal = 1;
- strbuf_write_column(sb, &graph->new_columns[target], '_');
+ graph_line_write_column(line, &graph->new_columns[target], '_');
} else {
if (used_horizontal && i < horizontal_edge)
graph->new_mapping[i] = -1;
- strbuf_write_column(sb, &graph->new_columns[target], '/');
+ graph_line_write_column(line, &graph->new_columns[target], '/');
}
}
- graph_pad_horizontally(graph, sb, graph->mapping_size);
+ graph_pad_horizontally(graph, line);
/*
* Swap mapping and new_mapping
@@ -1199,24 +1204,26 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
int graph_next_line(struct git_graph *graph, struct strbuf *sb)
{
+ struct graph_line line = { .buf = sb, .width = 0 };
+
switch (graph->state) {
case GRAPH_PADDING:
- graph_output_padding_line(graph, sb);
+ graph_output_padding_line(graph, &line);
return 0;
case GRAPH_SKIP:
- graph_output_skip_line(graph, sb);
+ graph_output_skip_line(graph, &line);
return 0;
case GRAPH_PRE_COMMIT:
- graph_output_pre_commit_line(graph, sb);
+ graph_output_pre_commit_line(graph, &line);
return 0;
case GRAPH_COMMIT:
- graph_output_commit_line(graph, sb);
+ graph_output_commit_line(graph, &line);
return 1;
case GRAPH_POST_MERGE:
- graph_output_post_merge_line(graph, sb);
+ graph_output_post_merge_line(graph, &line);
return 0;
case GRAPH_COLLAPSING:
- graph_output_collapsing_line(graph, sb);
+ graph_output_collapsing_line(graph, &line);
return 0;
}
@@ -1227,7 +1234,7 @@ int graph_next_line(struct git_graph *graph, struct strbuf *sb)
static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
{
int i;
- int chars_written = 0;
+ struct graph_line line = { .buf = sb, .width = 0 };
if (graph->state != GRAPH_COMMIT) {
graph_next_line(graph, sb);
@@ -1244,20 +1251,17 @@ static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
for (i = 0; i < graph->num_columns; i++) {
struct column *col = &graph->columns[i];
- strbuf_write_column(sb, col, '|');
- chars_written++;
+ graph_line_write_column(&line, col, '|');
if (col->commit == graph->commit && graph->num_parents > 2) {
int len = (graph->num_parents - 2) * 2;
- strbuf_addchars(sb, ' ', len);
- chars_written += len;
+ graph_line_addchars(&line, ' ', len);
} else {
- strbuf_addch(sb, ' ');
- chars_written++;
+ graph_line_addch(&line, ' ');
}
}
- graph_pad_horizontally(graph, sb, chars_written);
+ graph_pad_horizontally(graph, &line);
/*
* Update graph->prev_state since we have output a padding line
--
2.23.0
There was a problem hiding this comment.
On the Git mailing list, James Coglan wrote (reply to this):
On 14/10/2019 13:55, James Coglan wrote:
> Thanks! For completeness, here is the current state of this patch after I've been through the feedback up to this point:
>
> From 241180570fbaae04a2263c5ff1ab3b92f54f8004 Mon Sep 17 00:00:00 2001
> From: James Coglan <[email protected]>
> Date: Thu, 22 Aug 2019 09:30:08 +0100
> Subject: [PATCH] graph: automatically track display width of graph lines
>
> All the output functions called by `graph_next_line()` currently keep
> track of how many printable chars they've written to the buffer, before
> calling `graph_pad_horizontally()` to pad the line with spaces. Some
> functions do this by incrementing a counter whenever they write to the
> buffer, and others do it by encoding an assumption about how many chars
> are written, as in:
>
> graph_pad_horizontally(graph, sb, graph->num_columns * 2);
>
> This adds a fair amount of noise to the functions' logic and is easily
> broken if one forgets to increment the right counter or update the
> calculations used for padding.
>
> To make this easier to use, I'm introducing a new struct called
> `graph_line` that wraps a `strbuf` and keeps count of its display width
> implicitly. `graph_next_line()` wraps this around the `struct strbuf *`
> it's given and passes a `struct graph_line *` to the output functions,
> which use its interface.
>
> The `graph_line` interface wraps the `strbuf_addch()`,
> `strbuf_addchars()` and `strbuf_addstr()` functions, and adds the
> `graph_line_write_column()` function for adding a single character with
> color formatting. The `graph_pad_horizontally()` function can then use
> the `width` field from the struct rather than taking a character count
> as a parameter.
>
> Signed-off-by: James Coglan <[email protected]>
> ---
> graph.c | 194 +++++++++++++++++++++++++++++---------------------------
> 1 file changed, 99 insertions(+), 95 deletions(-)
>
> diff --git a/graph.c b/graph.c
> index f53135485f..2f81a5d23d 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -112,14 +112,42 @@ static const char *column_get_color_code(unsigned short color)
> return column_colors[color];
> }
>
> -static void strbuf_write_column(struct strbuf *sb, const struct column *c,
> - char col_char)
> +struct graph_line {
> + struct strbuf *buf;
> + size_t width;
> +};
> +
> +static inline void graph_line_addch(struct graph_line *line, int c)
> +{
> + strbuf_addch(line->buf, c);
> + line->width++;
> +}
> +
> +static inline void graph_line_addchars(struct graph_line *line, int c, size_t n)
> +{
> + strbuf_addchars(line->buf, c, n);
> + line->width += n;
> +}
> +
> +static inline void graph_line_addstr(struct graph_line *line, const char *s)
> +{
> + strbuf_addstr(line->buf, s);
> + line->width += strlen(s);
> +}
> +
> +static inline void graph_line_addcolor(struct graph_line *line, unsigned short color)
> +{
> + strbuf_addstr(line->buf, column_get_color_code(color));
> +}
> +
> +static void graph_line_write_column(struct graph_line *line, const struct column *c,
> + char col_char)
> {
> if (c->color < column_colors_max)
> - strbuf_addstr(sb, column_get_color_code(c->color));
> - strbuf_addch(sb, col_char);
> + graph_line_addcolor(line, c->color);
> + graph_line_addch(line, col_char);
> if (c->color < column_colors_max)
> - strbuf_addstr(sb, column_get_color_code(column_colors_max));
> + graph_line_addcolor(line, column_colors_max);
> }
>
> struct git_graph {
> @@ -686,8 +714,7 @@ static int graph_is_mapping_correct(struct git_graph *graph)
> return 1;
> }
>
> -static void graph_pad_horizontally(struct git_graph *graph, struct strbuf *sb,
> - int chars_written)
> +static void graph_pad_horizontally(struct git_graph *graph, struct graph_line *line)
> {
> /*
> * Add additional spaces to the end of the strbuf, so that all
> @@ -696,12 +723,12 @@ static void graph_pad_horizontally(struct git_graph *graph, struct strbuf *sb,
> * This way, fields printed to the right of the graph will remain
> * aligned for the entire commit.
> */
> - if (chars_written < graph->width)
> - strbuf_addchars(sb, ' ', graph->width - chars_written);
> + if (line->width < graph->width)
> + graph_line_addchars(line, ' ', graph->width - line->width);
> }
>
> static void graph_output_padding_line(struct git_graph *graph,
> - struct strbuf *sb)
> + struct graph_line *line)
> {
> int i;
>
> @@ -719,11 +746,11 @@ static void graph_output_padding_line(struct git_graph *graph,
> * Output a padding row, that leaves all branch lines unchanged
> */
> for (i = 0; i < graph->num_new_columns; i++) {
> - strbuf_write_column(sb, &graph->new_columns[i], '|');
> - strbuf_addch(sb, ' ');
> + graph_line_write_column(line, &graph->new_columns[i], '|');
> + graph_line_addch(line, ' ');
> }
>
> - graph_pad_horizontally(graph, sb, graph->num_new_columns * 2);
> + graph_pad_horizontally(graph, line);
> }
>
>
> @@ -733,14 +760,14 @@ int graph_width(struct git_graph *graph)
> }
>
>
> -static void graph_output_skip_line(struct git_graph *graph, struct strbuf *sb)
> +static void graph_output_skip_line(struct git_graph *graph, struct graph_line *line)
> {
> /*
> * Output an ellipsis to indicate that a portion
> * of the graph is missing.
> */
> - strbuf_addstr(sb, "...");
> - graph_pad_horizontally(graph, sb, 3);
> + graph_line_addstr(line, "...");
> + graph_pad_horizontally(graph, line);
>
> if (graph->num_parents >= 3 &&
> graph->commit_index < (graph->num_columns - 1))
> @@ -750,11 +777,10 @@ 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)
> + struct graph_line *line)
> {
> int num_expansion_rows;
> int i, seen_this;
> - int chars_written;
>
> /*
> * This function formats a row that increases the space around a commit
> @@ -777,14 +803,12 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
> * Output the row
> */
> seen_this = 0;
> - chars_written = 0;
> for (i = 0; i < graph->num_columns; i++) {
> struct column *col = &graph->columns[i];
> if (col->commit == graph->commit) {
> seen_this = 1;
> - strbuf_write_column(sb, col, '|');
> - strbuf_addchars(sb, ' ', graph->expansion_row);
> - chars_written += 1 + graph->expansion_row;
> + graph_line_write_column(line, col, '|');
> + graph_line_addchars(line, ' ', graph->expansion_row);
> } else if (seen_this && (graph->expansion_row == 0)) {
> /*
> * This is the first line of the pre-commit output.
> @@ -797,22 +821,18 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
> */
> if (graph->prev_state == GRAPH_POST_MERGE &&
> graph->prev_commit_index < i)
> - strbuf_write_column(sb, col, '\\');
> + graph_line_write_column(line, col, '\\');
> else
> - strbuf_write_column(sb, col, '|');
> - chars_written++;
> + graph_line_write_column(line, col, '|');
> } else if (seen_this && (graph->expansion_row > 0)) {
> - strbuf_write_column(sb, col, '\\');
> - chars_written++;
> + graph_line_write_column(line, col, '\\');
> } else {
> - strbuf_write_column(sb, col, '|');
> - chars_written++;
> + graph_line_write_column(line, col, '|');
> }
> - strbuf_addch(sb, ' ');
> - chars_written++;
> + graph_line_addch(line, ' ');
> }
>
> - graph_pad_horizontally(graph, sb, chars_written);
> + graph_pad_horizontally(graph, line);
>
> /*
> * Increment graph->expansion_row,
> @@ -823,7 +843,7 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
> graph_update_state(graph, GRAPH_COMMIT);
> }
>
> -static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
> +static void graph_output_commit_char(struct git_graph *graph, struct graph_line *line)
> {
> /*
> * For boundary commits, print 'o'
> @@ -831,22 +851,20 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
> */
> if (graph->commit->object.flags & BOUNDARY) {
> assert(graph->revs->boundary);
> - strbuf_addch(sb, 'o');
> + graph_line_addch(line, 'o');
> return;
> }
>
> /*
> * get_revision_mark() handles all other cases without assert()
> */
> - strbuf_addstr(sb, get_revision_mark(graph->revs, graph->commit));
> + graph_line_addstr(line, get_revision_mark(graph->revs, graph->commit));
> }
>
> /*
> - * Draw the horizontal dashes of an octopus merge and return the number of
> - * characters written.
> + * Draw the horizontal dashes of an octopus merge.
> */
> -static int graph_draw_octopus_merge(struct git_graph *graph,
> - struct strbuf *sb)
> +static void graph_draw_octopus_merge(struct git_graph *graph, struct graph_line *line)
> {
> /*
> * Here dashless_parents represents the number of parents which don't
> @@ -886,17 +904,16 @@ static int graph_draw_octopus_merge(struct git_graph *graph,
>
> 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 ? '.' : '-');
> + graph_line_write_column(line, &graph->new_columns[i+first_col], '-');
> + graph_line_write_column(line, &graph->new_columns[i+first_col],
> + i == dashful_parents-1 ? '.' : '-');
> }
> - return 2 * dashful_parents;
> }
>
> -static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
> +static void graph_output_commit_line(struct git_graph *graph, struct graph_line *line)
> {
> int seen_this = 0;
> - int i, chars_written;
> + int i;
>
> /*
> * Output the row containing this commit
> @@ -906,7 +923,6 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
> * children that we have already processed.)
> */
> seen_this = 0;
> - chars_written = 0;
> for (i = 0; i <= graph->num_columns; i++) {
> struct column *col = &graph->columns[i];
> struct commit *col_commit;
> @@ -920,15 +936,12 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
>
> if (col_commit == graph->commit) {
> seen_this = 1;
> - graph_output_commit_char(graph, sb);
> - chars_written++;
> + graph_output_commit_char(graph, line);
>
> if (graph->num_parents > 2)
> - chars_written += graph_draw_octopus_merge(graph,
> - sb);
> + graph_draw_octopus_merge(graph, line);
> } else if (seen_this && (graph->num_parents > 2)) {
> - strbuf_write_column(sb, col, '\\');
> - chars_written++;
> + graph_line_write_column(line, col, '\\');
> } else if (seen_this && (graph->num_parents == 2)) {
> /*
> * This is a 2-way merge commit.
> @@ -945,19 +958,16 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
> */
> if (graph->prev_state == GRAPH_POST_MERGE &&
> graph->prev_commit_index < i)
> - strbuf_write_column(sb, col, '\\');
> + graph_line_write_column(line, col, '\\');
> else
> - strbuf_write_column(sb, col, '|');
> - chars_written++;
> + graph_line_write_column(line, col, '|');
> } else {
> - strbuf_write_column(sb, col, '|');
> - chars_written++;
> + graph_line_write_column(line, col, '|');
> }
> - strbuf_addch(sb, ' ');
> - chars_written++;
> + graph_line_addch(line, ' ');
> }
>
> - graph_pad_horizontally(graph, sb, chars_written);
> + graph_pad_horizontally(graph, line);
>
> /*
> * Update graph->state
> @@ -981,15 +991,14 @@ static struct column *find_new_column_by_commit(struct git_graph *graph,
> return NULL;
> }
>
> -static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf *sb)
> +static void graph_output_post_merge_line(struct git_graph *graph, struct graph_line *line)
> {
> int seen_this = 0;
> - int i, j, chars_written;
> + int i, j;
>
> /*
> * Output the post-merge row
> */
> - chars_written = 0;
> for (i = 0; i <= graph->num_columns; i++) {
> struct column *col = &graph->columns[i];
> struct commit *col_commit;
> @@ -1016,29 +1025,25 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf
> par_column = find_new_column_by_commit(graph, parents->item);
> assert(par_column);
>
> - strbuf_write_column(sb, par_column, '|');
> - chars_written++;
> + graph_line_write_column(line, par_column, '|');
> for (j = 0; j < graph->num_parents - 1; j++) {
> parents = next_interesting_parent(graph, parents);
> assert(parents);
> par_column = find_new_column_by_commit(graph, parents->item);
> assert(par_column);
> - strbuf_write_column(sb, par_column, '\\');
> - strbuf_addch(sb, ' ');
> + graph_line_write_column(line, par_column, '\\');
> + graph_line_addch(line, ' ');
> }
> - chars_written += j * 2;
> } else if (seen_this) {
> - strbuf_write_column(sb, col, '\\');
> - strbuf_addch(sb, ' ');
> - chars_written += 2;
> + graph_line_write_column(line, col, '\\');
> + graph_line_addch(line, ' ');
> } else {
> - strbuf_write_column(sb, col, '|');
> - strbuf_addch(sb, ' ');
> - chars_written += 2;
> + graph_line_write_column(line, col, '|');
> + graph_line_addch(line, ' ');
> }
> }
>
> - graph_pad_horizontally(graph, sb, chars_written);
> + graph_pad_horizontally(graph, line);
>
> /*
> * Update graph->state
> @@ -1049,7 +1054,7 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct strbuf
> graph_update_state(graph, GRAPH_COLLAPSING);
> }
>
> -static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf *sb)
> +static void graph_output_collapsing_line(struct git_graph *graph, struct graph_line *line)
> {
> int i;
> short used_horizontal = 0;
> @@ -1159,9 +1164,9 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
> for (i = 0; i < graph->mapping_size; i++) {
> int target = graph->new_mapping[i];
> if (target < 0)
> - strbuf_addch(sb, ' ');
> + graph_line_addch(line, ' ');
> else if (target * 2 == i)
> - strbuf_write_column(sb, &graph->new_columns[target], '|');
> + graph_line_write_column(line, &graph->new_columns[target], '|');
> else if (target == horizontal_edge_target &&
> i != horizontal_edge - 1) {
> /*
> @@ -1172,16 +1177,16 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
> if (i != (target * 2)+3)
> graph->new_mapping[i] = -1;
> used_horizontal = 1;
> - strbuf_write_column(sb, &graph->new_columns[target], '_');
> + graph_line_write_column(line, &graph->new_columns[target], '_');
> } else {
> if (used_horizontal && i < horizontal_edge)
> graph->new_mapping[i] = -1;
> - strbuf_write_column(sb, &graph->new_columns[target], '/');
> + graph_line_write_column(line, &graph->new_columns[target], '/');
>
> }
> }
>
> - graph_pad_horizontally(graph, sb, graph->mapping_size);
> + graph_pad_horizontally(graph, line);
>
> /*
> * Swap mapping and new_mapping
> @@ -1199,24 +1204,26 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct strbuf
>
> int graph_next_line(struct git_graph *graph, struct strbuf *sb)
> {
> + struct graph_line line = { .buf = sb, .width = 0 };
> +
> switch (graph->state) {
> case GRAPH_PADDING:
> - graph_output_padding_line(graph, sb);
> + graph_output_padding_line(graph, &line);
> return 0;
> case GRAPH_SKIP:
> - graph_output_skip_line(graph, sb);
> + graph_output_skip_line(graph, &line);
> return 0;
> case GRAPH_PRE_COMMIT:
> - graph_output_pre_commit_line(graph, sb);
> + graph_output_pre_commit_line(graph, &line);
> return 0;
> case GRAPH_COMMIT:
> - graph_output_commit_line(graph, sb);
> + graph_output_commit_line(graph, &line);
> return 1;
> case GRAPH_POST_MERGE:
> - graph_output_post_merge_line(graph, sb);
> + graph_output_post_merge_line(graph, &line);
> return 0;
> case GRAPH_COLLAPSING:
> - graph_output_collapsing_line(graph, sb);
> + graph_output_collapsing_line(graph, &line);
> return 0;
> }
>
> @@ -1227,7 +1234,7 @@ int graph_next_line(struct git_graph *graph, struct strbuf *sb)
> static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
> {
> int i;
> - int chars_written = 0;
> + struct graph_line line = { .buf = sb, .width = 0 };
>
> if (graph->state != GRAPH_COMMIT) {
> graph_next_line(graph, sb);
> @@ -1244,20 +1251,17 @@ static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
> for (i = 0; i < graph->num_columns; i++) {
> struct column *col = &graph->columns[i];
>
> - strbuf_write_column(sb, col, '|');
> - chars_written++;
> + graph_line_write_column(&line, col, '|');
>
> if (col->commit == graph->commit && graph->num_parents > 2) {
> int len = (graph->num_parents - 2) * 2;
> - strbuf_addchars(sb, ' ', len);
> - chars_written += len;
> + graph_line_addchars(&line, ' ', len);
> } else {
> - strbuf_addch(sb, ' ');
> - chars_written++;
> + graph_line_addch(&line, ' ');
> }
> }
>
> - graph_pad_horizontally(graph, sb, chars_written);
> + graph_pad_horizontally(graph, &line);
>
> /*
> * Update graph->prev_state since we have output a padding line
In addition to this, I'd like to include the following patch that moves the `graph_pad_horizontally()` calls. By the way, what's the best way for me to submit the whole updated patch series after responding to all feedback?
From 97d230c8a8d9b677b331e37482039384589b096a Mon Sep 17 00:00:00 2001
From: James Coglan <[email protected]>
Date: Mon, 14 Oct 2019 12:10:06 +0100
Subject: [PATCH] graph: handle line padding in `graph_next_line()`
Now that the display width of graph lines is implicitly tracked via the
`graph_line` interface, the calls to `graph_pad_horizontally()` no
longer need to be located inside the individual output functions, where
the character counting was previously being done.
All the functions called by `graph_next_line()` generate a line of
output, then call `graph_pad_horizontally()`, and finally change the
graph state if necessary. As padding is the final change to the output
done by all these functions, it can be removed from all of them and done
in `graph_next_line()` instead.
I've also moved the guard in `graph_output_padding_line()` that checks
the graph has a commit; this function is only called by
`graph_next_line()` and we must not pad the `graph_line` if no commit is
set.
---
graph.c | 49 ++++++++++++++++++++-----------------------------
1 file changed, 20 insertions(+), 29 deletions(-)
diff --git a/graph.c b/graph.c
index 2f81a5d23d..4c68557b17 100644
--- a/graph.c
+++ b/graph.c
@@ -732,16 +732,6 @@ static void graph_output_padding_line(struct git_graph *graph,
{
int i;
- /*
- * We could conceivable be called with a NULL commit
- * if our caller has a bug, and invokes graph_next_line()
- * immediately after graph_init(), without first calling
- * graph_update(). Return without outputting anything in this
- * case.
- */
- if (!graph->commit)
- return;
-
/*
* Output a padding row, that leaves all branch lines unchanged
*/
@@ -749,8 +739,6 @@ static void graph_output_padding_line(struct git_graph *graph,
graph_line_write_column(line, &graph->new_columns[i], '|');
graph_line_addch(line, ' ');
}
-
- graph_pad_horizontally(graph, line);
}
@@ -767,7 +755,6 @@ static void graph_output_skip_line(struct git_graph *graph, struct graph_line *l
* of the graph is missing.
*/
graph_line_addstr(line, "...");
- graph_pad_horizontally(graph, line);
if (graph->num_parents >= 3 &&
graph->commit_index < (graph->num_columns - 1))
@@ -832,8 +819,6 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
graph_line_addch(line, ' ');
}
- graph_pad_horizontally(graph, line);
-
/*
* Increment graph->expansion_row,
* and move to state GRAPH_COMMIT if necessary
@@ -967,8 +952,6 @@ static void graph_output_commit_line(struct git_graph *graph, struct graph_line
graph_line_addch(line, ' ');
}
- graph_pad_horizontally(graph, line);
-
/*
* Update graph->state
*/
@@ -1043,8 +1026,6 @@ static void graph_output_post_merge_line(struct git_graph *graph, struct graph_l
}
}
- graph_pad_horizontally(graph, line);
-
/*
* Update graph->state
*/
@@ -1186,8 +1167,6 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l
}
}
- graph_pad_horizontally(graph, line);
-
/*
* Swap mapping and new_mapping
*/
@@ -1204,31 +1183,43 @@ static void graph_output_collapsing_line(struct git_graph *graph, struct graph_l
int graph_next_line(struct git_graph *graph, struct strbuf *sb)
{
+ int shown_commit_line = 0;
struct graph_line line = { .buf = sb, .width = 0 };
+ /*
+ * We could conceivable be called with a NULL commit
+ * if our caller has a bug, and invokes graph_next_line()
+ * immediately after graph_init(), without first calling
+ * graph_update(). Return without outputting anything in this
+ * case.
+ */
+ if (!graph->commit)
+ return -1;
+
switch (graph->state) {
case GRAPH_PADDING:
graph_output_padding_line(graph, &line);
- return 0;
+ break;
case GRAPH_SKIP:
graph_output_skip_line(graph, &line);
- return 0;
+ break;
case GRAPH_PRE_COMMIT:
graph_output_pre_commit_line(graph, &line);
- return 0;
+ break;
case GRAPH_COMMIT:
graph_output_commit_line(graph, &line);
- return 1;
+ shown_commit_line = 1;
+ break;
case GRAPH_POST_MERGE:
graph_output_post_merge_line(graph, &line);
- return 0;
+ break;
case GRAPH_COLLAPSING:
graph_output_collapsing_line(graph, &line);
- return 0;
+ break;
}
- assert(0);
- return 0;
+ graph_pad_horizontally(graph, &line);
+ return shown_commit_line;
}
static void graph_padding_line(struct git_graph *graph, struct strbuf *sb)
--
2.23.0
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
James Coglan <[email protected]> writes:
>> Is there a reason why you need a pointer to a strbuf that is
>> allocated separately? E.g. would it make it harder to manage
>> if the above were
>>
>> struct graphbuf {
>> struct strbuf buf;
>> int width; /* display width in columns */
>> };
>>
>> which is essentially what Dscho suggested?
>
> I used a pointer here because I create the wrapper struct in
> `graph_next_line()`, which is an external interface that takes a
> `struct strbuf *`:
>
> int graph_next_line(struct git_graph *graph, struct strbuf *sb)
> {
> struct graph_line line = { .buf = sb, .width = 0 };
> // ...
> }
>
> So I'm not allocating the strbuf here, just wrapping a pointer to
> it.
OK, so existing callers allocate strbuf, and you are merely adding a
wrapper structure to keep track of the width. The management of the
lifetime of the strbuf is not your business so there is no reason to
inline the structure in graph_line. Makes sense. Thanks.
|
On the Git mailing list, Jeff King wrote (reply to this): |
|
On the Git mailing list, James Coglan wrote (reply to this): |
jcoglan
force-pushed
the
jc/simplify-graph
branch
from
ea0df1d to
d1635aa
Compare
|
@jcoglan Since By the way, a merge conflict resolution for the two branches can be found here: https://github.com/Denton-L/git/tree/testing/graph-output Thanks again for your hard work! |
|
/submit |
|
Error: Refusing to submit a patch series that does not merge cleanly. |
|
@jcoglan I think you need to rebase on top of |
jcoglan
force-pushed
the
jc/simplify-graph
branch
from
d1635aa to
07ddd50
Compare
|
@Denton-L Thanks for the conflict resolution, it was a neat coincidence that we both added exactly the same history to that test. I've now rebased and updated my commits to amend |
|
/submit |
|
Submitted as [email protected] WARNING: jcoglan has no public email address set on GitHub |
All the output functions called by `graph_next_line()` currently keep
track of how many printable chars they've written to the buffer, before
calling `graph_pad_horizontally()` to pad the line with spaces. Some
functions do this by incrementing a counter whenever they write to the
buffer, and others do it by encoding an assumption about how many chars
are written, as in:
graph_pad_horizontally(graph, sb, graph->num_columns * 2);
This adds a fair amount of noise to the functions' logic and is easily
broken if one forgets to increment the right counter or update the
calculations used for padding.
To make this easier to use, I'm introducing a new struct called
`graph_line` that wraps a `strbuf` and keeps count of its display width
implicitly. `graph_next_line()` wraps this around the `struct strbuf *`
it's given and passes a `struct graph_line *` to the output functions,
which use its interface.
The `graph_line` interface wraps the `strbuf_addch()`,
`strbuf_addchars()` and `strbuf_addstr()` functions, and adds the
`graph_line_write_column()` function for adding a single character with
color formatting. The `graph_pad_horizontally()` function can then use
the `width` field from the struct rather than taking a character count
as a parameter.
Signed-off-by: James Coglan <[email protected]>
Now that the display width of graph lines is implicitly tracked via the `graph_line` interface, the calls to `graph_pad_horizontally()` no longer need to be located inside the individual output functions, where the character counting was previously being done. All the functions called by `graph_next_line()` generate a line of output, then call `graph_pad_horizontally()`, and finally change the graph state if necessary. As padding is the final change to the output done by all these functions, it can be removed from all of them and done in `graph_next_line()` instead. I've also moved the guard in `graph_output_padding_line()` that checks the graph has a commit; this function is only called by `graph_next_line()` and we must not pad the `graph_line` if no commit is set. Signed-off-by: James Coglan <[email protected]>
I will shortly be making some changes to `graph_insert_into_new_columns()` and so am trying to simplify it. One possible simplification is that we can extract the loop for finding the element in `new_columns` containing the given commit. `find_new_column_by_commit()` contains a very similar loop but it returns a `struct column *` rather than an `int` offset into the array. Here I'm introducing a version that returns `int` and using that in `graph_insert_into_new_columns()` and `graph_output_post_merge_line()`. Signed-off-by: James Coglan <[email protected]>
I will shortly be making some changes to this function and so am trying to simplify it. It currently contains some duplicated logic; both branches the function can take assign the commit's column index into the `mapping` array and increment `mapping_index`. Here I change the function so that the only conditional behaviour is that it appends the commit to `new_columns` if it's not present. All manipulation of `mapping` now happens on a single code path. Signed-off-by: James Coglan <[email protected]>
There's a duplication of logic between `graph_insert_into_new_columns()` and `graph_update_width()`. `graph_insert_into_new_columns()` is called repeatedly by `graph_update_columns()` with an `int *` that tracks the offset into the `mapping` array where we should write the next value. Each call to `graph_insert_into_new_columns()` effectively pushes one column index and one "null" value (-1) onto the `mapping` array and therefore increments `mapping_idx` by 2. `graph_update_width()` duplicates this process: the `width` of the graph is essentially the initial width of the `mapping` array before edges begin collapsing. The `graph_update_width()` function's logic effectively works out how many times `graph_insert_into_new_columns()` was called based on the relationship of the current commit to the rest of the graph. I'm about to make some changes that make the assignment of values into the `mapping` array more complicated. Rather than make `graph_update_width()` more complicated at the same time, we can simply remove this function and use `graph->width` to track the offset into the `mapping` array as we're building it. This removes the duplication and makes sure that `graph->width` is the same as the visual width of the `mapping` array once `graph_update_columns()` is complete. Signed-off-by: James Coglan <[email protected]>
This computation is repeated in a couple of places and I need to add another condition to it to implement a further improvement to the graph rendering, so I'm extracting this into a function. Signed-off-by: James Coglan <[email protected]>
The commits following this one introduce a series of improvements to the
layout of graphs, tidying up a few edge cases, namely:
- merge whose first parent fuses with an existing column to the left
- merge whose last parent fuses with its immediate neighbor on the right
- edges that collapse to the left above and below a commit line
This test case exemplifies these cases and provides a motivating example
of the kind of history I'm aiming to clear up.
The first parent of merge E is the same as the parent of H, so those
edges fuse together.
* H
|
| *-. E
| |\ \
|/ / /
|
* B
We can "skew" the display of this merge so that it doesn't introduce
additional columns that immediately collapse:
* H
|
| * E
|/|\
|
* B
The last parent of E is D, the same as the parent of F which is the edge
to the right of the merge.
* F
|
\
*-. \ E
|\ \ \
/ / / /
| /
|/
* D
The two edges leading to D could be fused sooner: rather than expanding
the F edge around the merge and then letting the edges collapse, the F
edge could fuse with the E edge in the post-merge line:
* F
|
\
*-. | E
|\ \|
/ / /
|
* D
If this is combined with the "skew" effect above, we get a much cleaner
graph display for these edges:
* F
|
* | E
/|\|
|
* D
Finally, the edge leading from C to A appears jagged as it passes
through the commit line for B:
| * | C
| |/
* | B
|/
* A
This can be smoothed out so that such edges are easier to read:
| * | C
| |/
* / B
|/
* A
Signed-off-by: James Coglan <[email protected]>
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 vertical 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:
| * | *
| |\ | |\
| | \ | * \
| | \ |/|\ \
| *-. \
| |\ \ \
Signed-off-by: James Coglan <[email protected]>
Following the introduction of "left-skewed" merges, which are merges
whose first parent fuses with another edge to its left, we have some
more edge cases to deal with in the display of commit and post-merge
lines.
The current graph code handles the following cases for edges appearing
to the right of the commit (*) on commit lines. A 2-way merge is usually
followed by vertical lines:
| | |
| * |
| |\ \
An octopus merge (more than two parents) is always followed by edges
sloping to the right:
| | \ | | \
| *-. \ | *---. \
| |\ \ \ | |\ \ \ \
A 2-way merge is followed by a right-sloping edge if the commit line
immediately follows a post-merge line for a commit that appears in the
same column as the current commit, or any column to the left of that:
| * | * |
| |\ | |\ \
| * \ | | * \
| |\ \ | | |\ \
This commit introduces the following new cases for commit lines. If a
2-way merge skews to the left, then the edges to its right are always
vertical lines, even if the commit follows a post-merge line:
| | | | |\
| * | | * |
|/| | |/| |
A commit with 3 parents that skews left is followed by vertical edges:
| | |
| * |
|/|\ \
If a 3-way left-skewed merge commit appears immediately after a
post-merge line, then it may be followed the right-sloping edges, just
like a 2-way merge that is not skewed.
| |\
| * \
|/|\ \
Octopus merges with 4 or more parents that skew to the left will always
be followed by right-sloping edges, because the existing columns need to
expand around the merge.
| | \
| *-. \
|/|\ \ \
On post-merge lines, usually all edges following the current commit
slope to the right:
| * | |
| |\ \ \
However, if the commit is a left-skewed 2-way merge, the edges to its
right remain vertical. We also need to display a space after the
vertical line descending from the commit marker, whereas this line would
normally be followed by a backslash.
| * | |
|/| | |
If a left-skewed merge has more than 2 parents, then the edges to its
right are still sloped as they bend around the edges introduced by the
merge.
| * | |
|/|\ \ \
To handle these new cases, we need to know not just how many parents
each commit has, but how many new columns it adds to the display; this
quantity is recorded in the `edges_added` field for the current commit,
and `prev_edges_added` field for the previous commit.
Here, "column" refers to visual columns, not the logical columns of the
`columns` array. This is because even if all the commit's parents end up
fusing with existing edges, they initially introduce distinct edges in
the commit and post-merge lines before those edges collapse. For
example, a 3-way merge whose 2nd and 3rd parents fuse with existing
edges still introduces 2 visual columns that affect the display of edges
to their right.
| | | \
| | *-. \
| | |\ \ \
| |_|/ / /
|/| | / /
| | |/ /
| |/| |
| | | |
This merge does not introduce any *logical* columns; there are 4 edges
before and after this commit once all edges have collapsed. But it does
initially introduce 2 new edges that need to be accommodated by the
edges to their right.
Signed-off-by: James Coglan <[email protected]>
The change I'm about to make requires being able to inspect the mapping array that was used to render the last GRAPH_COLLAPSING line while rendering a GRAPH_COMMIT line. The `new_mapping` array currently exists as a pre-allocated space for computing the next `mapping` array during `graph_output_collapsing_line()`, but we can repurpose it to let us see the previous `mapping` state. To support this use it will make more sense if this array is named `old_mapping`, as it will contain the mapping data for the previous line we rendered, at the point we're rendering a commit line. Signed-off-by: James Coglan <[email protected]>
When a graph contains edges that are in the process of collapsing to the
left, but those edges cross a commit line, the effect is that the edges
have a jagged appearance:
*
|\
| *
| \
*-. \
|\ \ \
| | * |
| * | |
| |/ /
* | |
|/ /
* |
|/
*
We already takes steps to smooth edges like this when they're expanding;
when an edge appears to the right of a merge commit marker on a
GRAPH_COMMIT line immediately following a GRAPH_POST_MERGE line, we
render it as a `\`:
* \
|\ \
| * \
| |\ \
We can make a similar improvement to collapsing edges, making them
easier to follow and giving the overall graph a feeling of increased
symmetry:
*
|\
| *
| \
*-. \
|\ \ \
| | * |
| * | |
| |/ /
* / /
|/ /
* /
|/
*
To do this, we introduce a new special case for edges on GRAPH_COMMIT
lines that immediately follow a GRAPH_COLLAPSING line. By retaining a
copy of the `mapping` array used to render the GRAPH_COLLAPSING line in
the `old_mapping` array, we can determine that an edge is collapsing
through the GRAPH_COMMIT line and should be smoothed.
Signed-off-by: James Coglan <[email protected]>
When a merge commit is printed and its final parent is the same commit
that occupies the column to the right of the merge, this results in a
kink in the displayed edges:
* |
|\ \
| |/
| *
Graphs containing these shapes can be hard to read, as the expansion to
the right followed immediately by collapsing back to the left creates a
lot of zig-zagging edges, especially when many columns are present.
We can improve this by eliminating the zig-zag and having the merge's
final parent edge fuse immediately with its neighbor:
* |
|\|
| *
This reduces the horizontal width for the current commit by 2, and
requires one less row, making the graph display more compact. Taken in
combination with other graph-smoothing enhancements, it greatly
compresses the space needed to display certain histories:
*
|\
| * *
| |\ |\
| | * | *
| | | | |\
| | \ | | *
| *-. \ | * |
| |\ \ \ => |/|\|
|/ / / / | | *
| | | / | * |
| | |/ | |/
| | * * /
| * | |/
| |/ *
* |
|/
*
One of the test cases here cannot be correctly rendered in Git v2.23.0;
it produces this output following commit E:
| | *-. \ 5_E
| | |\ \ \
| |/ / / /
| | | / _
| |_|/
|/| |
The new implementation makes sure that the rightmost edge in this
history is not left dangling as above.
Signed-off-by: James Coglan <[email protected]>
In 0400583 ("log: fix coloring of certain octopus merge shapes", 2018-09-01) there is a fix for the coloring of dashes following an octopus merge. It makes a distinction between the case where all parents introduce a new column, versus the case where the first parent collapses into an existing column: | *-. | *-. | |\ \ | |\ \ | | | | |/ / / The latter case means that the columns for the merge parents begin one place to the left in the `new_columns` array compared to the former case. However, the implementation only works if the commit's parents are kept in order as they map onto the visual columns, as we get the colors by iterating over `new_columns` as we print the dashes. In general, the commit's parents can arbitrarily merge with existing columns, and change their ordering in the process. For example, in the following diagram, the number of each column indicates which commit parent appears in each column. | | *---. | | |\ \ \ | | |/ / / | |/| | / | |_|_|/ |/| | | 3 1 0 2 If the columns are colored (red, green, yellow, blue), then the dashes will currently be colored yellow and blue, whereas they should be blue and red. To fix this, we need to look up each column in the `mapping` array, which before the `GRAPH_COLLAPSING` state indicates which logical column is displayed in each visual column. This implementation is simpler as it doesn't have any edge cases, and it also handles how left-skewed first parents are now displayed: | *-. |/|\ \ | | | | 0 1 2 3 The color of the first dashes is always the color found in `mapping` two columns to the right of the commit symbol. Because commits are displayed after all edges have been collapsed together and the visual columns match the logical ones, we can find the visual offset of the commit symbol using `commit_index`. Signed-off-by: James Coglan <[email protected]>
jcoglan
force-pushed
the
jc/simplify-graph
branch
from
07ddd50 to
cd761b3
Compare
|
/submit |
|
Submitted as [email protected] WARNING: jcoglan has no public email address set on GitHub |
| @@ -112,14 +112,42 @@ static const char *column_get_color_code(unsigned short color) | |||
| return column_colors[color]; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"James Coglan via GitGitGadget" <[email protected]> writes:
> +struct graph_line {
> + struct strbuf *buf;
> + size_t width;
> +};
> +
> +static inline void graph_line_addch(struct graph_line *line, int c)
> +{
> + strbuf_addch(line->buf, c);
> + line->width++;
> +}
> +
> +static inline void graph_line_addchars(struct graph_line *line, int c, size_t n)
> +{
> + strbuf_addchars(line->buf, c, n);
> + line->width += n;
> +}
> +
> +static inline void graph_line_addstr(struct graph_line *line, const char *s)
> +{
> + strbuf_addstr(line->buf, s);
> + line->width += strlen(s);
> +}
> +
> +static inline void graph_line_addcolor(struct graph_line *line, unsigned short color)
> +{
> + strbuf_addstr(line->buf, column_get_color_code(color));
> +}
All makes sense, and as long as nobody uses strbuf_add*() on
line->buf directly, it shouldn't be too hard to extend these
to support graph drawn characters outside ASCII in the future
after this series settles.
> static void graph_output_pre_commit_line(struct git_graph *graph,
> ...
> for (i = 0; i < graph->num_columns; i++) {
> struct column *col = &graph->columns[i];
> if (col->commit == graph->commit) {
> seen_this = 1;
> - strbuf_write_column(sb, col, '|');
> - strbuf_addchars(sb, ' ', graph->expansion_row);
> - chars_written += 1 + graph->expansion_row;
> + graph_line_write_column(line, col, '|');
> + graph_line_addchars(line, ' ', graph->expansion_row);
Nice reduction of noise, as the proposed log message promises.
> int graph_next_line(struct git_graph *graph, struct strbuf *sb)
I just noticed it but does this have to be extern? Nobody outside
graph.[ch] seems to have any reference to it. That might mean that
it may be easier than we thought earlier to change the second
parameter of this to "struct graph_line *", instead of us wrapping
an incoming strbuf (which external callers are aware of, as opposed
to graph_line which we prefer not to expose to outsiders). Whether
we change the second parameter or not, the first clean-up may be to
turn this function into a file-scope static, perhaps?
All the changes are very pleasing to see. Thanks.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Junio C Hamano <[email protected]> writes:
>> int graph_next_line(struct git_graph *graph, struct strbuf *sb)
>
> I just noticed it but does this have to be extern? Nobody outside
> graph.[ch] seems to have any reference to it.
I was stupid; strike this part out.
Thanks.
| @@ -112,14 +112,42 @@ static const char *column_get_color_code(unsigned short color) | |||
| return column_colors[color]; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"James Coglan via GitGitGadget" <[email protected]> writes:
> From: James Coglan <[email protected]>
>
> Now that the display width of graph lines is implicitly tracked via the
> `graph_line` interface, the calls to `graph_pad_horizontally()` no
> longer need to be located inside the individual output functions, where
> the character counting was previously being done.
>
> All the functions called by `graph_next_line()` generate a line of
> output, then call `graph_pad_horizontally()`, and finally change the
> graph state if necessary. As padding is the final change to the output
> done by all these functions, it can be removed from all of them and done
> in `graph_next_line()` instead.
Very well explained.
| @@ -112,14 +112,42 @@ static const char *column_get_color_code(unsigned short color) | |||
| return column_colors[color]; | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"James Coglan via GitGitGadget" <[email protected]> writes:
> 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:
>
> | * | *
> | |\ | |\
> | | \ | * \
> | | \ |/|\ \
> | *-. \
> | |\ \ \
Looking at these drawings reminded me of a tangent that is brought
up from time to time. We do not do great job when showing multiple
roots.
If you have a history like this:
A---D---E
/
B---C
drawing the graph _with_ the merge gives a reasonable representation
of the entire topology.
* 46f67dd E
* 6f89516 D
|\
| * e6277a9 C
| * 13ae9b2 B
* afee005 A
But if you start drawing from parents of D (excluding D), you'd get
this:
* e6277a9 C
* 13ae9b2 B
* afee005 A
and the fact that B and A do not share parent-child relationships is
lost. An easy way to show that would be to draw the bottom three
lines of the full history output we saw earlier:
| * e6277a9 C
| * 13ae9b2 B
* afee005 A
either with or without the vertical bar to imply that A may have a
child.
This is not something that has to be done as part of this series,
but I am hoping that the internal simplification and code
restructuring that is done by this series would make it easier to
enhance the system to allow such an output.
Thanks.
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 10/16/2019 12:00 AM, Junio C Hamano wrote:
> "James Coglan via GitGitGadget" <[email protected]> writes:
>
>> 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:
>>
>> | * | *
>> | |\ | |\
>> | | \ | * \
>> | | \ |/|\ \
>> | *-. \
>> | |\ \ \
>
> Looking at these drawings reminded me of a tangent that is brought
> up from time to time. We do not do great job when showing multiple
> roots.
>
> If you have a history like this:
>
> A---D---E
> /
> B---C
>
> drawing the graph _with_ the merge gives a reasonable representation
> of the entire topology.
>
> * 46f67dd E
> * 6f89516 D
> |\
> | * e6277a9 C
> | * 13ae9b2 B
> * afee005 A
>
> But if you start drawing from parents of D (excluding D), you'd get
> this:
>
> * e6277a9 C
> * 13ae9b2 B
> * afee005 A
I hit this very situation recently when I was experimenting with
'git fast-import' and accidentally created many parallel, independent
histories. Running "git log --graph --all --simplify-by-decoration"
made it look like all the refs were in a line, but they were not.
(The one way I knew something was up: the base commits also appeared
without a decoration. That was the only clue that the histories did
not continue in a line.)
>
> and the fact that B and A do not share parent-child relationships is
> lost. An easy way to show that would be to draw the bottom three
> lines of the full history output we saw earlier:
>
> | * e6277a9 C
> | * 13ae9b2 B
> * afee005 A
>
> either with or without the vertical bar to imply that A may have a
> child.
The natural extension of this would be multiple columns:
| | | | | *
| | | | *
| | | *
| | *
| *
*
This does not appear too cumbersome, and such a situation should
be rare.
> This is not something that has to be done as part of this series,
> but I am hoping that the internal simplification and code
> restructuring that is done by this series would make it easier to
> enhance the system to allow such an output.
I agree. An excellent idea for a follow-up.
Thanks,
-Stolee
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Derrick Stolee <[email protected]> writes:
> I hit this very situation recently when I was experimenting with
> 'git fast-import' and accidentally created many parallel, independent
> histories. Running "git log --graph --all --simplify-by-decoration"
> made it look like all the refs were in a line, but they were not.
> (The one way I knew something was up: the base commits also appeared
> without a decoration. That was the only clue that the histories did
> not continue in a line.)
>
>>
>> and the fact that B and A do not share parent-child relationships is
>> lost. An easy way to show that would be to draw the bottom three
>> lines of the full history output we saw earlier:
>>
>> | * e6277a9 C
>> | * 13ae9b2 B
>> * afee005 A
>>
>> either with or without the vertical bar to imply that A may have a
>> child.
> The natural extension of this would be multiple columns:
>
> | | | | | *
> | | | | *
> | | | *
> | | *
> | *
> *
After sleeping over it, I now think we shouldn't draw lines that
imply a child for each of these commits, as we haven't seen, but
I agree that this can be extended to 3 or more roots.
|
This branch is now known as |
|
This patch series was integrated into pu via git@0a19638. |
| @@ -0,0 +1,43 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 10/15/2019 7:47 PM, James Coglan via GitGitGadget wrote:
> diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
> new file mode 100755
> index 0000000000..4582ba066a
> --- /dev/null
> +++ b/t/t4215-log-skewed-merges.sh
> @@ -0,0 +1,43 @@
> +#!/bin/sh
> +
> +test_description='git log --graph of skewed merges'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'log --graph with merge fusing with its left and right neighbors' '
> + cat >expect <<-\EOF &&
> + * H
> + |\
> + | * G
> + | |\
> + | | * F
> + | | |
> + | | \
> + | *-. \ E
> + | |\ \ \
> + |/ / / /
> + | | | /
> + | | |/
> + | | * D
> + | * | C
> + | |/
> + * | B
> + |/
> + * A
> + EOF
Thanks for adding this test! I really think it helps show some
of your improvements later as this test is mutated.
-Stolee
> +
> + git checkout --orphan _p &&
> + test_commit A &&
> + test_commit B &&
> + git checkout -b _q @^ && test_commit C &&
> + git checkout -b _r @^ && test_commit D &&
> + git checkout _p && git merge --no-ff _q _r -m E &&
> + git checkout _r && test_commit F &&
> + git checkout _p && git merge --no-ff _r -m G &&
> + git checkout @^^ && git merge --no-ff _p -m H &&
> +
> + git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> + test_cmp expect actual
> +'
> +
> +test_done
>
|
@jcoglan I hope you did not close this PR because of all the reviews? We're all very excited about your work over here... |
| @@ -0,0 +1,43 @@ | |||
| #!/bin/sh | |||
There was a problem hiding this comment.
On the Git mailing list, SZEDER Gábor wrote (reply to this):
On Tue, Oct 15, 2019 at 11:47:53PM +0000, James Coglan via GitGitGadget wrote:
> diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
> new file mode 100755
> index 0000000000..4582ba066a
> --- /dev/null
> +++ b/t/t4215-log-skewed-merges.sh
> @@ -0,0 +1,43 @@
> +#!/bin/sh
> +
> +test_description='git log --graph of skewed merges'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'log --graph with merge fusing with its left and right neighbors' '
> + cat >expect <<-\EOF &&
> + * H
> + |\
> + | * G
> + | |\
> + | | * F
> + | | |
> + | | \
> + | *-. \ E
> + | |\ \ \
> + |/ / / /
> + | | | /
> + | | |/
> + | | * D
> + | * | C
> + | |/
> + * | B
> + |/
> + * A
> + EOF
> +
> + git checkout --orphan _p &&
> + test_commit A &&
> + test_commit B &&
> + git checkout -b _q @^ && test_commit C &&
> + git checkout -b _r @^ && test_commit D &&
> + git checkout _p && git merge --no-ff _q _r -m E &&
> + git checkout _r && test_commit F &&
> + git checkout _p && git merge --no-ff _r -m G &&
> + git checkout @^^ && git merge --no-ff _p -m H &&
> +
> + git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
Please don't pipe 'git log --graph's output, but use an intermediate
file instead:
git log --graph ... >out &&
sed s/// out >actual &&
test_cmp expect actual
The exit code of a pipeline is the exit code of the last process in
the pipeline, and the exit codes of processes upstream of a pipe are
ignored. Consequently, if 'git log --graph' produced the expected
output but were to fail during housekeeping before exiting (segfault,
double free(), whatever), then that failure would go unnoticed.
This applies to several (all?) new tests added in this patch series as
well.
I'd like to join the praises from others: this is one excellent
first-time submission, thanks.
There was a problem hiding this comment.
On the Git mailing list, Denton Liu wrote (reply to this):
When git commands are placed in the upstream of a pipe, their return
codes are lost. In this particular case, it is especially bad since we
are testing the intricacies of `git log --graph` behavior and if we hit
an unexpected failure or segfault, we want to know this.
Redirect the output of git commands upstream of pipe into a file and
have sed read from that file so that git failures are detected.
Signed-off-by: Denton Liu <[email protected]>
---
Thanks for the suggestion, Gábor. (Is that how I should refer to you? I
recently learned that some poeple write their names in ALL CAPS as a
convention.)
A little late to the party but since this cleanup hasn't been done yet,
let's do it now. We can apply this patch to the tip of
'jc/log-graph-simplify'.
t/t4215-log-skewed-merges.sh | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
index d33c6438d8..0a2555fb28 100755
--- a/t/t4215-log-skewed-merges.sh
+++ b/t/t4215-log-skewed-merges.sh
@@ -31,7 +31,8 @@ test_expect_success 'log --graph with merge fusing with its left and right neigh
git checkout _p && git merge --no-ff _r -m G &&
git checkout @^^ && git merge --no-ff _p -m H &&
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+ git log --graph --pretty=tformat:%s >actual.raw &&
+ sed "s/ *$//" actual.raw >actual &&
test_cmp expect actual
'
@@ -68,7 +69,8 @@ test_expect_success 'log --graph with left-skewed merge' '
git checkout 0_p && git merge --no-ff 0_s -m 0_G &&
git checkout @^ && git merge --no-ff 0_q 0_r 0_t 0_p -m 0_H &&
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+ git log --graph --pretty=tformat:%s >actual.raw &&
+ sed "s/ *$//" actual.raw >actual &&
test_cmp expect actual
'
@@ -99,7 +101,8 @@ test_expect_success 'log --graph with nested left-skewed merge' '
git checkout 1_p && git merge --no-ff 1_r -m 1_G &&
git checkout @^^ && git merge --no-ff 1_p -m 1_H &&
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+ git log --graph --pretty=tformat:%s >actual.raw &&
+ sed "s/ *$//" actual.raw >actual &&
test_cmp expect actual
'
@@ -139,7 +142,8 @@ test_expect_success 'log --graph with nested left-skewed merge following normal
git checkout -b 2_s @^^ && git merge --no-ff 2_q -m 2_J &&
git checkout 2_p && git merge --no-ff 2_s -m 2_K &&
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+ git log --graph --pretty=tformat:%s >actual.raw &&
+ sed "s/ *$//" actual.raw >actual &&
test_cmp expect actual
'
@@ -175,7 +179,8 @@ test_expect_success 'log --graph with nested right-skewed merge following left-s
git checkout 3_p && git merge --no-ff 3_r -m 3_H &&
git checkout @^^ && git merge --no-ff 3_p -m 3_J &&
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+ git log --graph --pretty=tformat:%s >actual.raw &&
+ sed "s/ *$//" actual.raw >actual &&
test_cmp expect actual
'
@@ -210,7 +215,8 @@ test_expect_success 'log --graph with right-skewed merge following a left-skewed
git merge --no-ff 4_p -m 4_G &&
git checkout @^^ && git merge --no-ff 4_s -m 4_H &&
- git log --graph --date-order --pretty=tformat:%s | sed "s/ *$//" >actual &&
+ git log --graph --date-order --pretty=tformat:%s >actual.raw &&
+ sed "s/ *$//" actual.raw >actual &&
test_cmp expect actual
'
@@ -250,7 +256,8 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
git checkout 5_r &&
git merge --no-ff 5_s -m 5_H &&
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
+ git log --graph --pretty=tformat:%s >actual.raw &&
+ sed "s/ *$//" actual.raw >actual &&
test_cmp expect actual
'
--
2.24.0.300.g722ba42680
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Denton Liu <[email protected]> writes:
> A little late to the party but since this cleanup hasn't been done yet,
> let's do it now. We can apply this patch to the tip of
> 'jc/log-graph-simplify'.
> ...
> - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> + git log --graph --pretty=tformat:%s >actual.raw &&
> + sed "s/ *$//" actual.raw >actual &&
Obviously good but I wonder if
show_graph () {
git log --graph --pretty=tformat:%s >actual.raw &&
sed "s/ *$//" actual.raw &&
rm actual.raw
}
would help to make it even more readable without too much repetition.
Will queue; thanks.
> test_cmp expect actual
> '
>
> @@ -68,7 +69,8 @@ test_expect_success 'log --graph with left-skewed merge' '
> git checkout 0_p && git merge --no-ff 0_s -m 0_G &&
> git checkout @^ && git merge --no-ff 0_q 0_r 0_t 0_p -m 0_H &&
>
> - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> + git log --graph --pretty=tformat:%s >actual.raw &&
> + sed "s/ *$//" actual.raw >actual &&
> test_cmp expect actual
> '
>
> @@ -99,7 +101,8 @@ test_expect_success 'log --graph with nested left-skewed merge' '
> git checkout 1_p && git merge --no-ff 1_r -m 1_G &&
> git checkout @^^ && git merge --no-ff 1_p -m 1_H &&
>
> - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> + git log --graph --pretty=tformat:%s >actual.raw &&
> + sed "s/ *$//" actual.raw >actual &&
> test_cmp expect actual
> '
>
> @@ -139,7 +142,8 @@ test_expect_success 'log --graph with nested left-skewed merge following normal
> git checkout -b 2_s @^^ && git merge --no-ff 2_q -m 2_J &&
> git checkout 2_p && git merge --no-ff 2_s -m 2_K &&
>
> - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> + git log --graph --pretty=tformat:%s >actual.raw &&
> + sed "s/ *$//" actual.raw >actual &&
> test_cmp expect actual
> '
>
> @@ -175,7 +179,8 @@ test_expect_success 'log --graph with nested right-skewed merge following left-s
> git checkout 3_p && git merge --no-ff 3_r -m 3_H &&
> git checkout @^^ && git merge --no-ff 3_p -m 3_J &&
>
> - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> + git log --graph --pretty=tformat:%s >actual.raw &&
> + sed "s/ *$//" actual.raw >actual &&
> test_cmp expect actual
> '
>
> @@ -210,7 +215,8 @@ test_expect_success 'log --graph with right-skewed merge following a left-skewed
> git merge --no-ff 4_p -m 4_G &&
> git checkout @^^ && git merge --no-ff 4_s -m 4_H &&
>
> - git log --graph --date-order --pretty=tformat:%s | sed "s/ *$//" >actual &&
> + git log --graph --date-order --pretty=tformat:%s >actual.raw &&
> + sed "s/ *$//" actual.raw >actual &&
> test_cmp expect actual
> '
>
> @@ -250,7 +256,8 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
> git checkout 5_r &&
> git merge --no-ff 5_s -m 5_H &&
>
> - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> + git log --graph --pretty=tformat:%s >actual.raw &&
> + sed "s/ *$//" actual.raw >actual &&
> test_cmp expect actual
> '
There was a problem hiding this comment.
On the Git mailing list, SZEDER Gábor wrote (reply to this):
On Tue, Nov 12, 2019 at 03:57:08PM +0900, Junio C Hamano wrote:
> Denton Liu <[email protected]> writes:
>
> > A little late to the party but since this cleanup hasn't been done yet,
> > let's do it now. We can apply this patch to the tip of
> > 'jc/log-graph-simplify'.
> > ...
> > - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > + git log --graph --pretty=tformat:%s >actual.raw &&
> > + sed "s/ *$//" actual.raw >actual &&
>
> Obviously good but I wonder if
>
> show_graph () {
> git log --graph --pretty=tformat:%s >actual.raw &&
> sed "s/ *$//" actual.raw &&
> rm actual.raw
> }
>
> would help to make it even more readable without too much repetition.
I think it would indeed, but then let's go one step further, and add
that 'test_cmp expect actual' to the function, too, and call it
'check_graph'.
> Will queue; thanks.
>
> > test_cmp expect actual
> > '
> >
> > @@ -68,7 +69,8 @@ test_expect_success 'log --graph with left-skewed merge' '
> > git checkout 0_p && git merge --no-ff 0_s -m 0_G &&
> > git checkout @^ && git merge --no-ff 0_q 0_r 0_t 0_p -m 0_H &&
> >
> > - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > + git log --graph --pretty=tformat:%s >actual.raw &&
> > + sed "s/ *$//" actual.raw >actual &&
> > test_cmp expect actual
> > '
> >
> > @@ -99,7 +101,8 @@ test_expect_success 'log --graph with nested left-skewed merge' '
> > git checkout 1_p && git merge --no-ff 1_r -m 1_G &&
> > git checkout @^^ && git merge --no-ff 1_p -m 1_H &&
> >
> > - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > + git log --graph --pretty=tformat:%s >actual.raw &&
> > + sed "s/ *$//" actual.raw >actual &&
> > test_cmp expect actual
> > '
> >
> > @@ -139,7 +142,8 @@ test_expect_success 'log --graph with nested left-skewed merge following normal
> > git checkout -b 2_s @^^ && git merge --no-ff 2_q -m 2_J &&
> > git checkout 2_p && git merge --no-ff 2_s -m 2_K &&
> >
> > - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > + git log --graph --pretty=tformat:%s >actual.raw &&
> > + sed "s/ *$//" actual.raw >actual &&
> > test_cmp expect actual
> > '
> >
> > @@ -175,7 +179,8 @@ test_expect_success 'log --graph with nested right-skewed merge following left-s
> > git checkout 3_p && git merge --no-ff 3_r -m 3_H &&
> > git checkout @^^ && git merge --no-ff 3_p -m 3_J &&
> >
> > - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > + git log --graph --pretty=tformat:%s >actual.raw &&
> > + sed "s/ *$//" actual.raw >actual &&
> > test_cmp expect actual
> > '
> >
> > @@ -210,7 +215,8 @@ test_expect_success 'log --graph with right-skewed merge following a left-skewed
> > git merge --no-ff 4_p -m 4_G &&
> > git checkout @^^ && git merge --no-ff 4_s -m 4_H &&
> >
> > - git log --graph --date-order --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > + git log --graph --date-order --pretty=tformat:%s >actual.raw &&
> > + sed "s/ *$//" actual.raw >actual &&
> > test_cmp expect actual
> > '
> >
> > @@ -250,7 +256,8 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
> > git checkout 5_r &&
> > git merge --no-ff 5_s -m 5_H &&
> >
> > - git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
> > + git log --graph --pretty=tformat:%s >actual.raw &&
> > + sed "s/ *$//" actual.raw >actual &&
> > test_cmp expect actual
> > '
There was a problem hiding this comment.
On the Git mailing list, Denton Liu wrote (reply to this):
When git commands are placed in the upstream of a pipe, their return
codes are lost. In this particular case, it is especially bad since we
are testing the intricacies of `git log --graph` behavior and if we hit
an unexpected failure or segfault, we want to know this.
Extract the common output checking logic into check_graph() where we
redirect the output of git commands upstream of pipe into a file and
have sed read from that file so that git failures are detected.
This patch is best viewed with `--color-moved`.
Signed-off-by: Denton Liu <[email protected]>
---
Thanks for the feedback, everyone. I decided to take Gábor's suggestion
and write the check_graph() helper function.
Unfortunately, even though in practice I was moving the here-doc lines
down, the diff shows up as me moving the commit and checkout lines up
and this might affect how the blame ends up looking. Is there any way to
force git to make the diff show up the other way in both this diff and
in the blame or is this the best that we can do?
Thanks!
t/t4215-log-skewed-merges.sh | 208 ++++++++++++++++-------------------
1 file changed, 97 insertions(+), 111 deletions(-)
diff --git a/t/t4215-log-skewed-merges.sh b/t/t4215-log-skewed-merges.sh
index d33c6438d8..18709a723e 100755
--- a/t/t4215-log-skewed-merges.sh
+++ b/t/t4215-log-skewed-merges.sh
@@ -4,8 +4,25 @@ test_description='git log --graph of skewed merges'
. ./test-lib.sh
+check_graph () {
+ cat >expect &&
+ git log --graph --pretty=tformat:%s "$@" >actual.raw &&
+ sed "s/ *$//" actual.raw >actual &&
+ test_cmp expect actual
+}
+
test_expect_success 'log --graph with merge fusing with its left and right neighbors' '
- cat >expect <<-\EOF &&
+ git checkout --orphan _p &&
+ test_commit A &&
+ test_commit B &&
+ git checkout -b _q @^ && test_commit C &&
+ git checkout -b _r @^ && test_commit D &&
+ git checkout _p && git merge --no-ff _q _r -m E &&
+ git checkout _r && test_commit F &&
+ git checkout _p && git merge --no-ff _r -m G &&
+ git checkout @^^ && git merge --no-ff _p -m H &&
+
+ check_graph <<-\EOF
* H
|\
| * G
@@ -20,23 +37,20 @@ test_expect_success 'log --graph with merge fusing with its left and right neigh
|/
* A
EOF
-
- git checkout --orphan _p &&
- test_commit A &&
- test_commit B &&
- git checkout -b _q @^ && test_commit C &&
- git checkout -b _r @^ && test_commit D &&
- git checkout _p && git merge --no-ff _q _r -m E &&
- git checkout _r && test_commit F &&
- git checkout _p && git merge --no-ff _r -m G &&
- git checkout @^^ && git merge --no-ff _p -m H &&
-
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
- test_cmp expect actual
'
test_expect_success 'log --graph with left-skewed merge' '
- cat >expect <<-\EOF &&
+ git checkout --orphan 0_p && test_commit 0_A &&
+ git checkout -b 0_q 0_p && test_commit 0_B &&
+ git checkout -b 0_r 0_p &&
+ test_commit 0_C &&
+ test_commit 0_D &&
+ git checkout -b 0_s 0_p && test_commit 0_E &&
+ git checkout -b 0_t 0_p && git merge --no-ff 0_r^ 0_s -m 0_F &&
+ git checkout 0_p && git merge --no-ff 0_s -m 0_G &&
+ git checkout @^ && git merge --no-ff 0_q 0_r 0_t 0_p -m 0_H &&
+
+ check_graph <<-\EOF
*-----. 0_H
|\ \ \ \
| | | | * 0_G
@@ -57,23 +71,20 @@ test_expect_success 'log --graph with left-skewed merge' '
|/
* 0_A
EOF
-
- git checkout --orphan 0_p && test_commit 0_A &&
- git checkout -b 0_q 0_p && test_commit 0_B &&
- git checkout -b 0_r 0_p &&
- test_commit 0_C &&
- test_commit 0_D &&
- git checkout -b 0_s 0_p && test_commit 0_E &&
- git checkout -b 0_t 0_p && git merge --no-ff 0_r^ 0_s -m 0_F &&
- git checkout 0_p && git merge --no-ff 0_s -m 0_G &&
- git checkout @^ && git merge --no-ff 0_q 0_r 0_t 0_p -m 0_H &&
-
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
- test_cmp expect actual
'
test_expect_success 'log --graph with nested left-skewed merge' '
- cat >expect <<-\EOF &&
+ git checkout --orphan 1_p &&
+ test_commit 1_A &&
+ test_commit 1_B &&
+ test_commit 1_C &&
+ git checkout -b 1_q @^ && test_commit 1_D &&
+ git checkout 1_p && git merge --no-ff 1_q -m 1_E &&
+ git checkout -b 1_r @~3 && test_commit 1_F &&
+ git checkout 1_p && git merge --no-ff 1_r -m 1_G &&
+ git checkout @^^ && git merge --no-ff 1_p -m 1_H &&
+
+ check_graph <<-\EOF
* 1_H
|\
| * 1_G
@@ -88,23 +99,24 @@ test_expect_success 'log --graph with nested left-skewed merge' '
|/
* 1_A
EOF
-
- git checkout --orphan 1_p &&
- test_commit 1_A &&
- test_commit 1_B &&
- test_commit 1_C &&
- git checkout -b 1_q @^ && test_commit 1_D &&
- git checkout 1_p && git merge --no-ff 1_q -m 1_E &&
- git checkout -b 1_r @~3 && test_commit 1_F &&
- git checkout 1_p && git merge --no-ff 1_r -m 1_G &&
- git checkout @^^ && git merge --no-ff 1_p -m 1_H &&
-
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
- test_cmp expect actual
'
test_expect_success 'log --graph with nested left-skewed merge following normal merge' '
- cat >expect <<-\EOF &&
+ git checkout --orphan 2_p &&
+ test_commit 2_A &&
+ test_commit 2_B &&
+ test_commit 2_C &&
+ git checkout -b 2_q @^^ &&
+ test_commit 2_D &&
+ test_commit 2_E &&
+ git checkout -b 2_r @^ && test_commit 2_F &&
+ git checkout 2_q &&
+ git merge --no-ff 2_r -m 2_G &&
+ git merge --no-ff 2_p^ -m 2_H &&
+ git checkout -b 2_s @^^ && git merge --no-ff 2_q -m 2_J &&
+ git checkout 2_p && git merge --no-ff 2_s -m 2_K &&
+
+ check_graph <<-\EOF
* 2_K
|\
| * 2_J
@@ -124,27 +136,23 @@ test_expect_success 'log --graph with nested left-skewed merge following normal
|/
* 2_A
EOF
-
- git checkout --orphan 2_p &&
- test_commit 2_A &&
- test_commit 2_B &&
- test_commit 2_C &&
- git checkout -b 2_q @^^ &&
- test_commit 2_D &&
- test_commit 2_E &&
- git checkout -b 2_r @^ && test_commit 2_F &&
- git checkout 2_q &&
- git merge --no-ff 2_r -m 2_G &&
- git merge --no-ff 2_p^ -m 2_H &&
- git checkout -b 2_s @^^ && git merge --no-ff 2_q -m 2_J &&
- git checkout 2_p && git merge --no-ff 2_s -m 2_K &&
-
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
- test_cmp expect actual
'
test_expect_success 'log --graph with nested right-skewed merge following left-skewed merge' '
- cat >expect <<-\EOF &&
+ git checkout --orphan 3_p &&
+ test_commit 3_A &&
+ git checkout -b 3_q &&
+ test_commit 3_B &&
+ test_commit 3_C &&
+ git checkout -b 3_r @^ &&
+ test_commit 3_D &&
+ git checkout 3_q && git merge --no-ff 3_r -m 3_E &&
+ git checkout 3_p && git merge --no-ff 3_q -m 3_F &&
+ git checkout 3_r && test_commit 3_G &&
+ git checkout 3_p && git merge --no-ff 3_r -m 3_H &&
+ git checkout @^^ && git merge --no-ff 3_p -m 3_J &&
+
+ check_graph <<-\EOF
* 3_J
|\
| * 3_H
@@ -161,26 +169,21 @@ test_expect_success 'log --graph with nested right-skewed merge following left-s
|/
* 3_A
EOF
-
- git checkout --orphan 3_p &&
- test_commit 3_A &&
- git checkout -b 3_q &&
- test_commit 3_B &&
- test_commit 3_C &&
- git checkout -b 3_r @^ &&
- test_commit 3_D &&
- git checkout 3_q && git merge --no-ff 3_r -m 3_E &&
- git checkout 3_p && git merge --no-ff 3_q -m 3_F &&
- git checkout 3_r && test_commit 3_G &&
- git checkout 3_p && git merge --no-ff 3_r -m 3_H &&
- git checkout @^^ && git merge --no-ff 3_p -m 3_J &&
-
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
- test_cmp expect actual
'
test_expect_success 'log --graph with right-skewed merge following a left-skewed one' '
- cat >expect <<-\EOF &&
+ git checkout --orphan 4_p &&
+ test_commit 4_A &&
+ test_commit 4_B &&
+ test_commit 4_C &&
+ git checkout -b 4_q @^^ && test_commit 4_D &&
+ git checkout -b 4_r 4_p^ && git merge --no-ff 4_q -m 4_E &&
+ git checkout -b 4_s 4_p^^ &&
+ git merge --no-ff 4_r -m 4_F &&
+ git merge --no-ff 4_p -m 4_G &&
+ git checkout @^^ && git merge --no-ff 4_s -m 4_H &&
+
+ check_graph --date-order <<-\EOF
* 4_H
|\
| * 4_G
@@ -198,24 +201,25 @@ test_expect_success 'log --graph with right-skewed merge following a left-skewed
|/
* 4_A
EOF
-
- git checkout --orphan 4_p &&
- test_commit 4_A &&
- test_commit 4_B &&
- test_commit 4_C &&
- git checkout -b 4_q @^^ && test_commit 4_D &&
- git checkout -b 4_r 4_p^ && git merge --no-ff 4_q -m 4_E &&
- git checkout -b 4_s 4_p^^ &&
- git merge --no-ff 4_r -m 4_F &&
- git merge --no-ff 4_p -m 4_G &&
- git checkout @^^ && git merge --no-ff 4_s -m 4_H &&
-
- git log --graph --date-order --pretty=tformat:%s | sed "s/ *$//" >actual &&
- test_cmp expect actual
'
test_expect_success 'log --graph with octopus merge with column joining its penultimate parent' '
- cat >expect <<-\EOF &&
+ git checkout --orphan 5_p &&
+ test_commit 5_A &&
+ git branch 5_q &&
+ git branch 5_r &&
+ test_commit 5_B &&
+ git checkout 5_q && test_commit 5_C &&
+ git checkout 5_r && test_commit 5_D &&
+ git checkout 5_p &&
+ git merge --no-ff 5_q 5_r -m 5_E &&
+ git checkout 5_q && test_commit 5_F &&
+ git checkout -b 5_s 5_p^ &&
+ git merge --no-ff 5_p 5_q -m 5_G &&
+ git checkout 5_r &&
+ git merge --no-ff 5_s -m 5_H &&
+
+ check_graph <<-\EOF
* 5_H
|\
| *-. 5_G
@@ -234,24 +238,6 @@ test_expect_success 'log --graph with octopus merge with column joining its penu
|/
* 5_A
EOF
-
- git checkout --orphan 5_p &&
- test_commit 5_A &&
- git branch 5_q &&
- git branch 5_r &&
- test_commit 5_B &&
- git checkout 5_q && test_commit 5_C &&
- git checkout 5_r && test_commit 5_D &&
- git checkout 5_p &&
- git merge --no-ff 5_q 5_r -m 5_E &&
- git checkout 5_q && test_commit 5_F &&
- git checkout -b 5_s 5_p^ &&
- git merge --no-ff 5_p 5_q -m 5_G &&
- git checkout 5_r &&
- git merge --no-ff 5_s -m 5_H &&
-
- git log --graph --pretty=tformat:%s | sed "s/ *$//" >actual &&
- test_cmp expect actual
'
test_done
--
2.24.0.300.g722ba42680
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Denton Liu <[email protected]> writes:
> .... Is there any way to
> force git to make the diff show up the other way in both this diff and
> in the blame or is this the best that we can do?
"git blame -M" is your friend, I think.
$ git blame -b -M HEAD^ t/t4215-log-skewed-merges.sh
would show what this patch did rather nicely.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This series of patches are designed to improve the output of the
log --graphcommand; their effect can be summed up in the following diagram:
These changes aim to make the edges in graph diagrams easier to read, by
straightening lines and making certain kinds of topologies display more
compactly. Three distinct changes are included.
First, if the first parent of a merge fuses with an edge to the left of the
commit, then we display that by making the edges fuse immediately rather than by
drawing a line straight down and then having it track to the left. That is,
where we currently display these graphs:
We will now display these merges as follows:
This transformation is applied to merges with any number of parents, for example
we currently display 3-parent merges like this:
And we will now display them like this:
If the edge the first parent fuses with is separated from the commit by multiple
columns, a horizontal edge is drawn just as we currently do in the 'collapsing'
state. This change also affects the display of commit and post-merge lines in
subtle ways that are more thoroughly described in the relevant commits.
The second change is that if the final parent of a merge fuses with the edge to
the right of the commit, then we can remove the zig-zag effect that currently
results. We currently display these merges like this:
After these changes, this merge will now be displayed like so:
If the final parent fuses with an edge that's further to the right, its display
is unchanged and it will still display like this:
The final structural change smooths out lines that are collapsing through commit
lines. For example, consider the following history:
This is now rendered so that commit lines display an edge using
/instead of|, if that edge is tracking to the left both above and below the commit line.That results in this improved display:
Taken together, these changes produce the change shown in the first diagram
above, with the current rendering on the left and the new rendering on the
right.
A final addition to that set of changes fixes the coloring of dashes that are
drawn next to octopus merges, in a manner compatible with all these changes. The
early commits in this set are refactorings that make the functional changes
easier to introduce.