On Sun, Apr 7, 2024 at 3:04 AM Quang Lê Duy <leduyquang753@xxxxxxxxx> wrote: > On Sun, Apr 7, 2024 at 12:47 PM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
