Re: [RFC PATCH 1/1] Add separator lines into `git log --graph`.

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 7, 2024, 05:47 UTC
Message-ID: <CAPig+cROH8Ebu9CgR87-48+Rk0H3maN+dwB+Y-N2FTvy5shE1Q@mail.gmail.com>
In-Reply-To: <20240407051031.6018-2-leduyquang753@gmail.com>

On Sun, Apr 7, 2024 at 1:10 AM Lê Duy Quang <leduyquang753@gmail.com> wrote:

> This is to separate out connected regions of the resulting commit graph so as
> to not have them confused as belonging to the same timeline.
> ---

I'm not particularly a user of --graph, so I don't necessarily have an opinion about the utility of this change or its mechanics, but I can make a few observations to help you improve the patch to improve the chances of it being accepted.

First, move the information from the cover letter into the commit message of the patch itself since that information will be helpful to future readers of the patch if it becomes part of the permanent history.

Second, following Documentation/SubmittingPatches guidelines, the subject could instead be written something like this:

    log: visually separate `git log --graph` regions

Third, add a Signed-off-by: trailer after the commit message (see SubmittingPatches).

Show 6 quoted lines

> diff --git a/graph.c b/graph.c
> @@ -729,9 +742,9 @@ static int graph_num_expansion_rows(struct git_graph *graph)
>  static int graph_needs_pre_commit_line(struct git_graph *graph)
>  {
> -       return graph->num_parents >= 3 &&
> +       return graph->connected_region_state == CONNECTED_REGION_NEW_REGION || (graph->num_parents >= 3 &&

Style: This line is overly long and should be wrapped; we aim (as much
as possible) to fit within an 80-column limit.

Show 11 quoted lines

>                graph->commit_index < (graph->num_columns - 1) &&
> -              graph->expansion_row < graph_num_expansion_rows(graph);
> +              graph->expansion_row < graph_num_expansion_rows(graph));
>  void graph_update(struct git_graph *graph, struct commit *commit)
> @@ -760,6 +773,12 @@ void graph_update(struct git_graph *graph, struct commit *commit)
> +
> +       /*
> +        * Determine whether this commit belongs to a new connected region.
> +        */
> +       graph->connected_region_state = (graph->connected_region_state != CONNECTED_REGION_FIRST_COMMIT &&
> +               graph->num_new_columns == 0) ? CONNECTED_REGION_NEW_REGION : CONNECTED_REGION_USE_CURRENT;

Style: overly long lines

Show 7 quoted lines

> +static void graph_output_separator_line(struct git_graph *graph, struct graph_line *line)
> +{
> +       /*
> +        * This function adds a row that separates two disconnected graphs,
> +        * as the appearance of multiple separate commits on top of each other
> +        * may cause a misunderstanding that they belong to a timeline.
> +        */

This comment seems to explain the purpose of the function itself. As such, it should precede the function definition rather than being embedded within it.

> +       assert(graph->connected_region_state == CONNECTED_REGION_NEW_REGION);

We tend to use BUG() rather than assert():

    if (graph->connected_region_state != CONNECTED_REGION_NEW_REGION)
        BUG("explain the failure here");

> +       /*
> +        * Output the row.
> +        */
> +       graph_line_addstr(line, "---");

The code itself is obvious enough without the comment, so the comment is mere noise, thus should be dropped.

Show 8 quoted lines

> +       /*
> +        * Immediately move to GRAPH_COMMIT state as there for sure aren't going to be
> +        * any more pre-commit lines.
> +        */
> +       graph_update_state(graph, GRAPH_COMMIT);
> +}
> diff --git a/t/t4218-log-graph-connected-regions.sh b/t/t4218-log-graph-connected-regions.sh
> new file mode 100755

We typically try to avoid creating new test scripts if an existing script would be a logical place to house the new tests. I haven't personally checked if such a script already exists, but if so, it would be good to add new tests to it. If not, then creating a new script, as you do here, may be fine.

Show 11 quoted lines

> @@ -0,0 +1,119 @@
> +#!/bin/sh
> +
> +test_description="git log --graph connected regions"
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-terminal.sh"
> +. "$TEST_DIRECTORY/lib-log-graph.sh"

"lib-terminal.sh" doesn't seem to be needed by these tests.

Show 6 quoted lines

> +test_cmp_graph () {
> +       lib_test_cmp_graph --format=%s "$@"
> +}
> +
> +add_commit () {
> +       touch $1 &&

If the timestamp of the empty file being created is not significant, we avoid `touch` and instead use `>` to create the file:

    >"$1" &&

> +       git add $1 &&
> +       git commit -m $1
> +       git tag "$1-commit"
> +}

Is this add_commit() function more or less duplicating the functionality of test_commit() from t/test-lib-functions.sh?

> +cat > expect <<\EOF

Style: drop whitespace following redirect operators:

    cat >expect <<\EOF

Show 24 quoted lines

> +* a3
> +* a2
> +* a1
> +| *   b4
> +| |\
> +| | * c3
> +| * | b3
> +| |/
> +| * b2
> +| * b1
> +|/
> +| * d4
> +| * d3
> +| | * e3
> +| |/
> +| * d2
> +| * d1
> +|/
> +* root
> +EOF
> +
> +test_expect_success 'all commits' '
> +       test_cmp_graph a b c d e
> +'

Modern test style is to perform all actions inside the test_expect_success body itself, so:

    test_expect_success 'all commits' '
        cat >expect <<-\EOF
        ...
        EOF
        test_cmp_graph a b c d e
    '

Note the use of <<- to allow you to indent the here-doc body.