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

[Eric Sunshine <sunshine@sunshineco.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).

> 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.

>                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

> +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.

> +       /*
> +        * 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.

> @@ -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.

> +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

> +* 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.