On Sun, Apr 7, 2024 at 12:47 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> > diff --git a/graph.c b/graph.c
> > @@ -729,9 +742,9 @@ static int graph_num_expansion_rows(struc
t 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
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.
> > +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?
> 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.
> > + /*
> > + * 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.
> 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.)
