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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Apr 7, 2024 at 3:04 AM Quang Lê Duy <leduyquang753@gmail.com> wrote:
> On Sun, Apr 7, 2024 at 12:47 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > +       return graph->connected_region_state == CONNECTED_REGION_NEW_REGION || (graph->num_parents >= 3 &&
> > > +       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
>
> May I ask how am I expected to place the line breaks? The Linux kernel style
> guide I consulted
> (https://www.kernel.org/doc/html/v4.10/process/coding-style.html) doesn't seem
> to go into too much detail on this.

I don't have a precise answer other than "use good taste". One
reasonably solid rule is that when wrapping at `&&` and `||`, those
operators should appear at the end of the line rather than the
beginning of the next line. So, a possible wrapping for these two
cases might be:

    return graph->connected_region_state == CONNECTED_REGION_NEW_REGION ||
        (graph->num_parents >= 3 &&
        graph->commit_index < (graph->num_columns - 1) &&
        graph->expansion_row < graph_num_expansion_rows(graph));

    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;

Since this enum is private to the C file and not part of an expressive
public API, another possibility for reducing the line length is to
shorten some of the names. For instance:

    enum connected_region_state {
        CONNREG_FIRST_COMMIT,
        CONNREG_USE_CURRENT,
        CONNREG_NEW_REGION
    };

> > > +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.
>
> I just followed what the surrounding code did (particularly in the original
> `graph_output_pre_commit_line` function), but on second look that functionality
> comment seems to only serve as context for the sentence below that so OK.

Indeed, looking at graph_output_pre_commit_line(), the comment seems
to be explaining the reason for the assert() in that function, whereas
the comment you wrote here seems to be explaining the purpose of the
function itself.

> > > +       assert(graph->connected_region_state == CONNECTED_REGION_NEW_REGION);
> >
> > We tend to use BUG() rather than assert():
>
> Same thing, I just followed that `graph_output_pre_commit_line` did. So I should
> forgo the consistency here? Or is that usage of `assert` in the existing code
> also to be updated?

I see what you mean, now that I'm looking at graph.c. Since assert()
is used so heavily in this file already (and there are no BUG()
invocations at all), it probably makes sense to be consistent and use
assert() here, as well. Adding a sentence to the commit message
explaining that you're using assert() for consistency rather than
BUG() will be helpful to reviewers.

While it might be a nice cleanup to eventually swap out assert() in
favor of BUG(), we should leave that for another day in order to keep
this patch well-focused. (We don't want to add a bunch of "while at
it, let's also change this" items, thus losing focus on what you
actually want to achieve.)

> > > +       /*
> > > +        * 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.
>
> Also same thing that I followed for consistency.

Understandable. In this case, I don't personally feel that this
comment is adding any value, thus would drop it, but others (including
yourself) may feel differently.

> > 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.
>
> This is also because I followed what `t4202-log.sh` did, but if that represents
> outdated practice then I'll change.

Understood.

Generally speaking, when adding new tests, we do want to follow modern
practice; that's especially true when creating a brand new test
script, but even when adding new tests to an existing script.

If you're modifying an existing test, then being consistent with the
surrounding code is a good idea. Consistency may also be reasonable
sometimes when inserting a new test into a block of existing
closely-related tests. Saying so in the commit message will help
reviewers understand.