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

From: Quang Lê Duy <leduyquang753@gmail.com>
Date: Apr 7, 2024, 07:03 UTC
Message-ID: <CACXAH50jwngEQLF612FMyZ36yB5Nd7vVHS98KMWYjXqcNzvpwg@mail.gmail.com>
In-Reply-To: <CAPig+cROH8Ebu9CgR87-48+Rk0H3maN+dwB+Y-N2FTvy5shE1Q@mail.gmail.com>

On Sun, Apr 7, 2024 at 12:47 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

> > diff --git a/graph.c b/graph.c
> > @@ -729,9 +742,9 @@ static int graph_num_expansion_rows(struc

t git_graph *graph)

Show 21 quoted lines

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

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.

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

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.

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

Show 10 quoted lines

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

Also same thing that I followed for consistency.

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

I tried looking and didn't see a script that these tests would fit nicely into. I would really appreciate having a second set of eyes.

Show 11 quoted lines

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

(My apologies, the email client doesn't automatically add CC to the mailing list in the reply and I forgot to do it myself, so I have to resend this message.)