{"thread":{"id":"61285","subject":"[RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions","startedAt":"2024-04-07T05:10:41Z","lastAt":"2024-04-08T15:49:33Z","messageCount":15,"participants":["Lê Duy Quang","Eric Sunshine","Junio C Hamano","Quang Lê Duy","Dragan Simic"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"492438","messageId":"20240407051031.6018-1-leduyquang753@gmail.com","threadId":"61285","inReplyTo":null,"subject":"[RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions","fromName":"Lê Duy Quang","fromEmail":"leduyquang753@gmail.com","sentAt":"2024-04-07T05:10:30Z","receivedAt":"2024-04-07T05:10:41Z","isPatch":true,"sender":{"key":"leduyquang753@gmail.com","avatar":"https://gravatar.com/avatar/f15e7b4c0bd656dd16b43b810d31934f4775f409784afdacd5e387ac4f5fbe59?d=mp&s=160"},"body":"`git log --graph`, when invoked with multiple starting revisions and some\nexclusions which cut the commit graph, may give a disconnected graph. In other\nwords, the resulting graph has more than one separate connected regions. The\ncommand currently prints the connected regions on top of each other without any\nseparation.\n\nThis leads to a problem. Say there are two connected regions, each having two\ncommits, the graph would look like this:\n\n* a2\n* a1\n* b2\n* b1\n\nwhich may lead to a misunderstanding that these four commits belong to the same\ntimeline, i.e. b2 is a parent of a1.\n\nThis patchset adds a separator line between each pair of connected regions to\nclarify that they are not actually connected:\n\n* a2\n* a1\n---\n* b2\n* b1\n\nLê Duy Quang (1):\n  Add separator lines into `git log --graph`.\n\n graph.c                                |  55 +++++++++++-\n t/t4218-log-graph-connected-regions.sh | 119 +++++++++++++++++++++++++\n 2 files changed, 170 insertions(+), 4 deletions(-)\n create mode 100755 t/t4218-log-graph-connected-regions.sh\n\n\nbase-commit: 19981daefd7c147444462739375462b49412ce33\n-- \n2.44.0\n\n"},{"id":"492439","messageId":"20240407051031.6018-2-leduyquang753@gmail.com","threadId":"61285","inReplyTo":"20240407051031.6018-1-leduyquang753@gmail.com","subject":"[RFC PATCH 1/1] Add separator lines into `git log --graph`.","fromName":"Lê Duy Quang","fromEmail":"leduyquang753@gmail.com","sentAt":"2024-04-07T05:10:31Z","receivedAt":"2024-04-07T05:10:46Z","isPatch":true,"sender":{"key":"leduyquang753@gmail.com","avatar":"https://gravatar.com/avatar/f15e7b4c0bd656dd16b43b810d31934f4775f409784afdacd5e387ac4f5fbe59?d=mp&s=160"},"body":"This is to separate out connected regions of the resulting commit graph so as\nto not have them confused as belonging to the same timeline.\n---\n graph.c                                |  55 +++++++++++-\n t/t4218-log-graph-connected-regions.sh | 119 +++++++++++++++++++++++++\n 2 files changed, 170 insertions(+), 4 deletions(-)\n create mode 100755 t/t4218-log-graph-connected-regions.sh\n\ndiff --git a/graph.c b/graph.c\nindex 1ca34770ee..c0107c02fa 100644\n--- a/graph.c\n+++ b/graph.c\n@@ -69,6 +69,12 @@ enum graph_state {\n \tGRAPH_COLLAPSING\n };\n \n+enum connected_region_state {\n+\tCONNECTED_REGION_FIRST_COMMIT,\n+\tCONNECTED_REGION_USE_CURRENT,\n+\tCONNECTED_REGION_NEW_REGION\n+};\n+\n static void graph_show_line_prefix(const struct diff_options *diffopt)\n {\n \tif (!diffopt || !diffopt->line_prefix)\n@@ -310,6 +316,12 @@ struct git_graph {\n \t * stored as an index into the array column_colors.\n \t */\n \tunsigned short default_column_color;\n+\t/*\n+\t * The state of which connected region the current commit belongs to.\n+\t * This is used to output a clarifying separator line between\n+\t * connected regions.\n+\t */\n+\tenum connected_region_state connected_region_state;\n };\n \n static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void *data)\n@@ -380,6 +392,7 @@ struct git_graph *graph_init(struct rev_info *opt)\n \t * This way we start at 0 for the first commit.\n \t */\n \tgraph->default_column_color = column_colors_max - 1;\n+\tgraph->connected_region_state = CONNECTED_REGION_FIRST_COMMIT;\n \n \t/*\n \t * Allocate a reasonably large default number of columns\n@@ -729,9 +742,9 @@ static int graph_num_expansion_rows(struct git_graph *graph)\n \n static int graph_needs_pre_commit_line(struct git_graph *graph)\n {\n-\treturn graph->num_parents >= 3 &&\n+\treturn graph->connected_region_state == CONNECTED_REGION_NEW_REGION || (graph->num_parents >= 3 &&\n \t       graph->commit_index < (graph->num_columns - 1) &&\n-\t       graph->expansion_row < graph_num_expansion_rows(graph);\n+\t       graph->expansion_row < graph_num_expansion_rows(graph));\n }\n \n void graph_update(struct git_graph *graph, struct commit *commit)\n@@ -760,6 +773,12 @@ void graph_update(struct git_graph *graph, struct commit *commit)\n \t * commit.\n \t */\n \tgraph->prev_commit_index = graph->commit_index;\n+\t\n+\t/*\n+\t * Determine whether this commit belongs to a new connected region.\n+\t */\n+\tgraph->connected_region_state = (graph->connected_region_state != CONNECTED_REGION_FIRST_COMMIT &&\n+\t\tgraph->num_new_columns == 0) ? CONNECTED_REGION_NEW_REGION : CONNECTED_REGION_USE_CURRENT;\n \n \t/*\n \t * Call graph_update_columns() to update\n@@ -865,8 +884,28 @@ static void graph_output_skip_line(struct git_graph *graph, struct graph_line *l\n \t\tgraph_update_state(graph, GRAPH_COMMIT);\n }\n \n-static void graph_output_pre_commit_line(struct git_graph *graph,\n-\t\t\t\t\t struct graph_line *line)\n+static void graph_output_separator_line(struct git_graph *graph, struct graph_line *line)\n+{\n+\t/*\n+\t * This function adds a row that separates two disconnected graphs,\n+\t * as the appearance of multiple separate commits on top of each other\n+\t * may cause a misunderstanding that they belong to a timeline.\n+\t */\n+\tassert(graph->connected_region_state == CONNECTED_REGION_NEW_REGION);\n+\n+\t/*\n+\t * Output the row.\n+\t */\n+\tgraph_line_addstr(line, \"---\");\n+\n+\t/*\n+\t * Immediately move to GRAPH_COMMIT state as there for sure aren't going to be\n+\t * any more pre-commit lines.\n+\t */\n+\tgraph_update_state(graph, GRAPH_COMMIT);\n+}\n+\n+static void graph_output_parent_expansion_line(struct git_graph *graph, struct graph_line *line)\n {\n \tint i, seen_this;\n \n@@ -928,6 +967,14 @@ static void graph_output_pre_commit_line(struct git_graph *graph,\n \t\tgraph_update_state(graph, GRAPH_COMMIT);\n }\n \n+static void graph_output_pre_commit_line(struct git_graph *graph, struct graph_line *line)\n+{\n+\tif (graph->connected_region_state == CONNECTED_REGION_NEW_REGION)\n+\t\tgraph_output_separator_line(graph, line);\n+\telse\n+\t\tgraph_output_parent_expansion_line(graph, line);\n+}\n+\n static void graph_output_commit_char(struct git_graph *graph, struct graph_line *line)\n {\n \t/*\ndiff --git a/t/t4218-log-graph-connected-regions.sh b/t/t4218-log-graph-connected-regions.sh\nnew file mode 100755\nindex 0000000000..4efe17827e\n--- /dev/null\n+++ b/t/t4218-log-graph-connected-regions.sh\n@@ -0,0 +1,119 @@\n+#!/bin/sh\n+\n+test_description=\"git log --graph connected regions\"\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+. \"$TEST_DIRECTORY/lib-terminal.sh\"\n+. \"$TEST_DIRECTORY/lib-log-graph.sh\"\n+\n+test_cmp_graph () {\n+\tlib_test_cmp_graph --format=%s \"$@\"\n+}\n+\n+add_commit () {\n+\ttouch $1 &&\n+\tgit add $1 &&\n+\tgit commit -m $1\n+\tgit tag \"$1-commit\"\n+}\n+\n+test_expect_success setup '\n+\tgit checkout -b a &&\n+\tadd_commit root &&\n+\t\n+\tadd_commit a1 &&\n+\tadd_commit a2 &&\n+\tadd_commit a3 &&\n+\t\n+\tgit checkout -b b root-commit &&\n+\tadd_commit b1 &&\n+\tadd_commit b2 &&\n+\tgit checkout -b c &&\n+\tadd_commit c3 &&\n+\tgit checkout b &&\n+\tadd_commit b3 &&\n+\tgit merge c -m b4 &&\n+\t\n+\tgit checkout -b d root-commit &&\n+\tadd_commit d1 &&\n+\tadd_commit d2 &&\n+\tgit checkout -b e &&\n+\tadd_commit e3 &&\n+\tgit checkout d &&\n+\tadd_commit d3 &&\n+\tadd_commit d4\n+'\n+\n+cat > expect <<\\EOF\n+* a3\n+* a2\n+* a1\n+| *   b4\n+| |\\\n+| | * c3\n+| * | b3\n+| |/\n+| * b2\n+| * b1\n+|/\n+| * d4\n+| * d3\n+| | * e3\n+| |/\n+| * d2\n+| * d1\n+|/\n+* root\n+EOF\n+\n+test_expect_success 'all commits' '\n+\ttest_cmp_graph a b c d e\n+'\n+\n+cat > expect <<\\EOF\n+* a3\n+* a2\n+* a1\n+---\n+*   b4\n+|\\\n+| * c3\n+* | b3\n+|/\n+* b2\n+* b1\n+---\n+* d4\n+* d3\n+| * e3\n+|/\n+* d2\n+* d1\n+EOF\n+\n+test_expect_success 'without root commit' '\n+\ttest_cmp_graph a b c d e ^root-commit\n+'\n+\n+cat > expect <<\\EOF\n+* a3\n+---\n+*   b4\n+|\\\n+| * c3\n+* b3\n+---\n+* d4\n+* d3\n+---\n+* e3\n+EOF\n+\n+test_expect_success \"branches' tips\" '\n+\ttest_cmp_graph a b c d e ^a2-commit ^b2-commit ^d2-commit\n+'\n+\n+test_done\n-- \n2.44.0\n\n"},{"id":"492440","messageId":"CAPig+cQVQrrXrDqys-TyCqFynR6=pmbxs-NK-ca-s0K8mjADaA@mail.gmail.com","threadId":"61285","inReplyTo":"20240407051031.6018-1-leduyquang753@gmail.com","subject":"Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-07T05:30:48Z","receivedAt":"2024-04-07T05:31:00Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 7, 2024 at 1:10 AM Lê Duy Quang <leduyquang753@gmail.com> wrote:\n> `git log --graph`, when invoked with multiple starting revisions and some\n> exclusions which cut the commit graph, may give a disconnected graph. In other\n> words, the resulting graph has more than one separate connected regions. The\n> command currently prints the connected regions on top of each other without any\n> separation.\n>\n> This leads to a problem. Say there are two connected regions, each having two\n> commits, the graph would look like this:\n>\n> * a2\n> * a1\n> * b2\n> * b1\n>\n> which may lead to a misunderstanding that these four commits belong to the same\n> timeline, i.e. b2 is a parent of a1.\n>\n> This patchset adds a separator line between each pair of connected regions to\n> clarify that they are not actually connected:\n>\n> * a2\n> * a1\n> ---\n> * b2\n> * b1\n\nThis sort of information which explains why the patch may be desirable\nis not only helpful to reviewers of the submission, but will be\nhelpful to future readers of the patch once it becomes part of the\nproject's permanent history (assuming it is accepted). However, the\ncover letter does not become part of the project's history (it exists\nonly in the mailing list). As such, please move this discussion into\nthe commit message of the patch itself.\n"},{"id":"492441","messageId":"xmqq4jcdivgw.fsf@gitster.g","threadId":"61285","inReplyTo":"CAPig+cQVQrrXrDqys-TyCqFynR6=pmbxs-NK-ca-s0K8mjADaA@mail.gmail.com","subject":"Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-07T05:37:51Z","receivedAt":"2024-04-07T05:37:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n>> This leads to a problem. Say there are two connected regions, each having two\n>> commits, the graph would look like this:\n>>\n>> * a2\n>> * a1\n>> * b2\n>> * b1\n>>\n>> which may lead to a misunderstanding that these four commits belong to the same\n>> timeline, i.e. b2 is a parent of a1.\n>>\n>> This patchset adds a separator line between each pair of connected regions to\n>> clarify that they are not actually connected:\n>>\n>> * a2\n>> * a1\n>> ---\n>> * b2\n>> * b1\n>\n> This sort of information which explains why the patch may be desirable\n> is not only helpful to reviewers of the submission, but will be\n> helpful to future readers of the patch once it becomes part of the\n> project's permanent history (assuming it is accepted). However, the\n> cover letter does not become part of the project's history (it exists\n> only in the mailing list). As such, please move this discussion into\n> the commit message of the patch itself.\n\nTrue.  But because we are doing graph, shouldn't we be able to do\nbetter?  For example, you can draw the two lineage of histories\non different columns and ...\n\n       * a2\n       * a1\n     * b2\n     * b1\n\n... that way, you do not need to lose one line of precious vertical\nscreen real estate.\n"},{"id":"492443","messageId":"CAPig+cROH8Ebu9CgR87-48+Rk0H3maN+dwB+Y-N2FTvy5shE1Q@mail.gmail.com","threadId":"61285","inReplyTo":"20240407051031.6018-2-leduyquang753@gmail.com","subject":"Re: [RFC PATCH 1/1] Add separator lines into `git log --graph`.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-07T05:47:35Z","receivedAt":"2024-04-07T05:47:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 7, 2024 at 1:10 AM Lê Duy Quang <leduyquang753@gmail.com> wrote:\n> This is to separate out connected regions of the resulting commit graph so as\n> to not have them confused as belonging to the same timeline.\n> ---\n\nI'm not particularly a user of --graph, so I don't necessarily have an\nopinion about the utility of this change or its mechanics, but I can\nmake a few observations to help you improve the patch to improve the\nchances of it being accepted.\n\nFirst, move the information from the cover letter into the commit\nmessage of the patch itself since that information will be helpful to\nfuture readers of the patch if it becomes part of the permanent\nhistory.\n\nSecond, following Documentation/SubmittingPatches guidelines, the\nsubject could instead be written something like this:\n\n    log: visually separate `git log --graph` regions\n\nThird, add a Signed-off-by: trailer after the commit message (see\nSubmittingPatches).\n\n> diff --git a/graph.c b/graph.c\n> @@ -729,9 +742,9 @@ static int graph_num_expansion_rows(struct git_graph *graph)\n>  static int graph_needs_pre_commit_line(struct git_graph *graph)\n>  {\n> -       return graph->num_parents >= 3 &&\n> +       return graph->connected_region_state == CONNECTED_REGION_NEW_REGION || (graph->num_parents >= 3 &&\n\nStyle: This line is overly long and should be wrapped; we aim (as much\nas possible) to fit within an 80-column limit.\n\n>                graph->commit_index < (graph->num_columns - 1) &&\n> -              graph->expansion_row < graph_num_expansion_rows(graph);\n> +              graph->expansion_row < graph_num_expansion_rows(graph));\n>  void graph_update(struct git_graph *graph, struct commit *commit)\n> @@ -760,6 +773,12 @@ void graph_update(struct git_graph *graph, struct commit *commit)\n> +\n> +       /*\n> +        * Determine whether this commit belongs to a new connected region.\n> +        */\n> +       graph->connected_region_state = (graph->connected_region_state != CONNECTED_REGION_FIRST_COMMIT &&\n> +               graph->num_new_columns == 0) ? CONNECTED_REGION_NEW_REGION : CONNECTED_REGION_USE_CURRENT;\n\nStyle: overly long lines\n\n> +static void graph_output_separator_line(struct git_graph *graph, struct graph_line *line)\n> +{\n> +       /*\n> +        * This function adds a row that separates two disconnected graphs,\n> +        * as the appearance of multiple separate commits on top of each other\n> +        * may cause a misunderstanding that they belong to a timeline.\n> +        */\n\nThis comment seems to explain the purpose of the function itself. As\nsuch, it should precede the function definition rather than being\nembedded within it.\n\n> +       assert(graph->connected_region_state == CONNECTED_REGION_NEW_REGION);\n\nWe tend to use BUG() rather than assert():\n\n    if (graph->connected_region_state != CONNECTED_REGION_NEW_REGION)\n        BUG(\"explain the failure here\");\n\n> +       /*\n> +        * Output the row.\n> +        */\n> +       graph_line_addstr(line, \"---\");\n\nThe code itself is obvious enough without the comment, so the comment\nis mere noise, thus should be dropped.\n\n> +       /*\n> +        * Immediately move to GRAPH_COMMIT state as there for sure aren't going to be\n> +        * any more pre-commit lines.\n> +        */\n> +       graph_update_state(graph, GRAPH_COMMIT);\n> +}\n> diff --git a/t/t4218-log-graph-connected-regions.sh b/t/t4218-log-graph-connected-regions.sh\n> new file mode 100755\n\nWe typically try to avoid creating new test scripts if an existing\nscript would be a logical place to house the new tests. I haven't\npersonally checked if such a script already exists, but if so, it\nwould be good to add new tests to it. If not, then creating a new\nscript, as you do here, may be fine.\n\n> @@ -0,0 +1,119 @@\n> +#!/bin/sh\n> +\n> +test_description=\"git log --graph connected regions\"\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n> +. ./test-lib.sh\n> +. \"$TEST_DIRECTORY/lib-terminal.sh\"\n> +. \"$TEST_DIRECTORY/lib-log-graph.sh\"\n\n\"lib-terminal.sh\" doesn't seem to be needed by these tests.\n\n> +test_cmp_graph () {\n> +       lib_test_cmp_graph --format=%s \"$@\"\n> +}\n> +\n> +add_commit () {\n> +       touch $1 &&\n\nIf the timestamp of the empty file being created is not significant,\nwe avoid `touch` and instead use `>` to create the file:\n\n    >\"$1\" &&\n\n> +       git add $1 &&\n> +       git commit -m $1\n> +       git tag \"$1-commit\"\n> +}\n\nIs this add_commit() function more or less duplicating the\nfunctionality of test_commit() from t/test-lib-functions.sh?\n\n> +cat > expect <<\\EOF\n\nStyle: drop whitespace following redirect operators:\n\n    cat >expect <<\\EOF\n\n> +* a3\n> +* a2\n> +* a1\n> +| *   b4\n> +| |\\\n> +| | * c3\n> +| * | b3\n> +| |/\n> +| * b2\n> +| * b1\n> +|/\n> +| * d4\n> +| * d3\n> +| | * e3\n> +| |/\n> +| * d2\n> +| * d1\n> +|/\n> +* root\n> +EOF\n> +\n> +test_expect_success 'all commits' '\n> +       test_cmp_graph a b c d e\n> +'\n\nModern test style is to perform all actions inside the\ntest_expect_success body itself, so:\n\n    test_expect_success 'all commits' '\n        cat >expect <<-\\EOF\n        ...\n        EOF\n        test_cmp_graph a b c d e\n    '\n\nNote the use of <<- to allow you to indent the here-doc body.\n"},{"id":"492444","messageId":"CAPig+cSfpWaanknSqMGEza7rPhXwgQ0k3V9svfuBsZ_C2EaCmw@mail.gmail.com","threadId":"61285","inReplyTo":"CAPig+cROH8Ebu9CgR87-48+Rk0H3maN+dwB+Y-N2FTvy5shE1Q@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] Add separator lines into `git log --graph`.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-07T05:52:42Z","receivedAt":"2024-04-07T05:52:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 7, 2024 at 1:47 AM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> I'm not particularly a user of --graph, so I don't necessarily have an\n> opinion about the utility of this change or its mechanics, but I can\n> make a few observations to help you improve the patch to improve the\n> chances of it being accepted.\n\nI forgot to mention that application of your patch results in some warnings:\n\n    % git am add-sep-lines.patch\n    Applying: Add separator lines into `git log --graph`.\n    .git/rebase-apply/patch:61: trailing whitespace.\n    .git/rebase-apply/patch:147: trailing whitespace.\n    .git/rebase-apply/patch:151: trailing whitespace.\n    .git/rebase-apply/patch:160: trailing whitespace.\n    warning: 4 lines add whitespace errors.\n"},{"id":"492446","messageId":"CACXAH50KHrnmc1At_DHX5O=BGuGT_pe9n5nGLSJYt7fXTbe_Yg@mail.gmail.com","threadId":"61285","inReplyTo":"xmqq4jcdivgw.fsf@gitster.g","subject":"Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions","fromName":"Quang Lê Duy","fromEmail":"leduyquang753@gmail.com","sentAt":"2024-04-07T06:40:36Z","receivedAt":"2024-04-07T06:41:14Z","isPatch":true,"sender":{"key":"leduyquang753@gmail.com","avatar":"https://gravatar.com/avatar/f15e7b4c0bd656dd16b43b810d31934f4775f409784afdacd5e387ac4f5fbe59?d=mp&s=160"},"body":"On Sun, Apr 7, 2024 at 12:37 PM Junio C Hamano <gitster@pobox.com> wrote:\n> True.  But because we are doing graph, shouldn't we be able to do\n> better?  For example, you can draw the two lineage of histories\n> on different columns and ...\n>\n>        * a2\n>        * a1\n>      * b2\n>      * b1\n>\n> ... that way, you do not need to lose one line of precious vertical\n> screen real estate.\n\nI think horizontal screen real estate is even more precious than the vertical\none, since one usually doesn't scroll their terminal horizontally. And then it\nwould probably be a way more complicated implementation.\n"},{"id":"492447","messageId":"CACXAH50jwngEQLF612FMyZ36yB5Nd7vVHS98KMWYjXqcNzvpwg@mail.gmail.com","threadId":"61285","inReplyTo":"CAPig+cROH8Ebu9CgR87-48+Rk0H3maN+dwB+Y-N2FTvy5shE1Q@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] Add separator lines into `git log --graph`.","fromName":"Quang Lê Duy","fromEmail":"leduyquang753@gmail.com","sentAt":"2024-04-07T07:03:50Z","receivedAt":"2024-04-07T07:04:28Z","isPatch":true,"sender":{"key":"leduyquang753@gmail.com","avatar":"https://gravatar.com/avatar/f15e7b4c0bd656dd16b43b810d31934f4775f409784afdacd5e387ac4f5fbe59?d=mp&s=160"},"body":"On Sun, Apr 7, 2024 at 12:47 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > diff --git a/graph.c b/graph.c\n> > @@ -729,9 +742,9 @@ static int graph_num_expansion_rows(struc\nt git_graph *graph)\n> >  static int graph_needs_pre_commit_line(struct git_graph *graph)\n> >  {\n> > -       return graph->num_parents >= 3 &&\n> > +       return graph->connected_region_state == CONNECTED_REGION_NEW_REGION || (graph->num_parents >= 3 &&\n>\n> Style: This line is overly long and should be wrapped; we aim (as much\n> as possible) to fit within an 80-column limit.\n>\n> >                graph->commit_index < (graph->num_columns - 1) &&\n> > -              graph->expansion_row < graph_num_expansion_rows(graph);\n> > +              graph->expansion_row < graph_num_expansion_rows(graph));\n> >  void graph_update(struct git_graph *graph, struct commit *commit)\n> > @@ -760,6 +773,12 @@ void graph_update(struct git_graph *graph, struct commit *commit)\n> > +\n> > +       /*\n> > +        * Determine whether this commit belongs to a new connected region.\n> > +        */\n> > +       graph->connected_region_state = (graph->connected_region_state != CONNECTED_REGION_FIRST_COMMIT &&\n> > +               graph->num_new_columns == 0) ? CONNECTED_REGION_NEW_REGION : CONNECTED_REGION_USE_CURRENT;\n>\n> Style: overly long lines\n\nMay I ask how am I expected to place the line breaks? The Linux kernel style\nguide I consulted\n(https://www.kernel.org/doc/html/v4.10/process/coding-style.html) doesn't seem\nto go into too much detail on this.\n\n> > +static void graph_output_separator_line(struct git_graph *graph, struct graph_line *line)\n> > +{\n> > +       /*\n> > +        * This function adds a row that separates two disconnected graphs,\n> > +        * as the appearance of multiple separate commits on top of each other\n> > +        * may cause a misunderstanding that they belong to a timeline.\n> > +        */\n>\n> This comment seems to explain the purpose of the function itself. As\n> such, it should precede the function definition rather than being\n> embedded within it.\n\nI just followed what the surrounding code did (particularly in the original\n`graph_output_pre_commit_line` function), but on second look that functionality\ncomment seems to only serve as context for the sentence below that so OK.\n\n> > +       assert(graph->connected_region_state == CONNECTED_REGION_NEW_REGION);\n>\n> We tend to use BUG() rather than assert():\n\nSame thing, I just followed that `graph_output_pre_commit_line` did. So I should\nforgo the consistency here? Or is that usage of `assert` in the existing code\nalso to be updated?\n\n>     if (graph->connected_region_state != CONNECTED_REGION_NEW_REGION)\n>         BUG(\"explain the failure here\");\n>\n> > +       /*\n> > +        * Output the row.\n> > +        */\n> > +       graph_line_addstr(line, \"---\");\n>\n> The code itself is obvious enough without the comment, so the comment\n> is mere noise, thus should be dropped.\n\nAlso same thing that I followed for consistency.\n\n> > +       /*\n> > +        * Immediately move to GRAPH_COMMIT state as there for sure aren't going to be\n> > +        * any more pre-commit lines.\n> > +        */\n> > +       graph_update_state(graph, GRAPH_COMMIT);\n> > +}\n> > diff --git a/t/t4218-log-graph-connected-regions.sh b/t/t4218-log-graph-connected-regions.sh\n> > new file mode 100755\n>\n> We typically try to avoid creating new test scripts if an existing\n> script would be a logical place to house the new tests. I haven't\n> personally checked if such a script already exists, but if so, it\n> would be good to add new tests to it. If not, then creating a new\n> script, as you do here, may be fine.\n\nI tried looking and didn't see a script that these tests would fit nicely into.\nI would really appreciate having a second set of eyes.\n\n> Modern test style is to perform all actions inside the\n> test_expect_success body itself, so:\n>\n>     test_expect_success 'all commits' '\n>         cat >expect <<-\\EOF\n>         ...\n>         EOF\n>         test_cmp_graph a b c d e\n>     '\n>\n> Note the use of <<- to allow you to indent the here-doc body.\n\nThis is also because I followed what `t4202-log.sh` did, but if that represents\noutdated practice then I'll change.\n\n(My apologies, the email client doesn't automatically add CC to the mailing list\nin the reply and I forgot to do it myself, so I have to resend this message.)\n"},{"id":"492448","messageId":"CACXAH50uHNreqhK9hn9_SPARCCq2gfAOs4krGS-Xa0-5GKSvtg@mail.gmail.com","threadId":"61285","inReplyTo":"CAPig+cSfpWaanknSqMGEza7rPhXwgQ0k3V9svfuBsZ_C2EaCmw@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] Add separator lines into `git log --graph`.","fromName":"Quang Lê Duy","fromEmail":"leduyquang753@gmail.com","sentAt":"2024-04-07T07:06:07Z","receivedAt":"2024-04-07T07:06:46Z","isPatch":true,"sender":{"key":"leduyquang753@gmail.com","avatar":"https://gravatar.com/avatar/f15e7b4c0bd656dd16b43b810d31934f4775f409784afdacd5e387ac4f5fbe59?d=mp&s=160"},"body":"On Sun, Apr 7, 2024 at 12:52 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> I forgot to mention that application of your patch results in some warnings:\n>\n>     % git am add-sep-lines.patch\n>     Applying: Add separator lines into `git log --graph`.\n>     .git/rebase-apply/patch:61: trailing whitespace.\n>     .git/rebase-apply/patch:147: trailing whitespace.\n>     .git/rebase-apply/patch:151: trailing whitespace.\n>     .git/rebase-apply/patch:160: trailing whitespace.\n>     warning: 4 lines add whitespace errors.\n\nIndeed I failed to notice the whitespace `vim` added to the empty lines.\nAppreciate your notice.\n"},{"id":"492449","messageId":"1e5dfc946c38809f159776f41e23c7c6@manjaro.org","threadId":"61285","inReplyTo":"CACXAH50KHrnmc1At_DHX5O=BGuGT_pe9n5nGLSJYt7fXTbe_Yg@mail.gmail.com","subject":"Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-07T08:34:19Z","receivedAt":"2024-04-07T08:34:24Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Quang,\n\nOn 2024-04-07 08:40, Quang Lê Duy wrote:\n> On Sun, Apr 7, 2024 at 12:37 PM Junio C Hamano <gitster@pobox.com> \n> wrote:\n>> True.  But because we are doing graph, shouldn't we be able to do\n>> better?  For example, you can draw the two lineage of histories\n>> on different columns and ...\n>> \n>>        * a2\n>>        * a1\n>>      * b2\n>>      * b1\n>> \n>> ... that way, you do not need to lose one line of precious vertical\n>> screen real estate.\n> \n> I think horizontal screen real estate is even more precious than the \n> vertical\n> one, since one usually doesn't scroll their terminal horizontally. And \n> then it\n> would probably be a way more complicated implementation.\n\nThese days, very few computer screens aren't in widescreen format,\nso there's inevitably less vertical screen space available than the\nhorizontal space.\n"},{"id":"492450","messageId":"559a32ce4bd3060ff2a1c352c62c6161@manjaro.org","threadId":"61285","inReplyTo":"CACXAH50uHNreqhK9hn9_SPARCCq2gfAOs4krGS-Xa0-5GKSvtg@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] Add separator lines into `git log --graph`.","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-07T08:35:37Z","receivedAt":"2024-04-07T08:35:39Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-07 09:06, Quang Lê Duy wrote:\n> On Sun, Apr 7, 2024 at 12:52 PM Eric Sunshine <sunshine@sunshineco.com> \n> wrote:\n>> I forgot to mention that application of your patch results in some \n>> warnings:\n>> \n>>     % git am add-sep-lines.patch\n>>     Applying: Add separator lines into `git log --graph`.\n>>     .git/rebase-apply/patch:61: trailing whitespace.\n>>     .git/rebase-apply/patch:147: trailing whitespace.\n>>     .git/rebase-apply/patch:151: trailing whitespace.\n>>     .git/rebase-apply/patch:160: trailing whitespace.\n>>     warning: 4 lines add whitespace errors.\n> \n> Indeed I failed to notice the whitespace `vim` added to the empty \n> lines.\n> Appreciate your notice.\n\nAs a note, vim can be configured to highlight the trailing\nwhitespace, making it easy to spot.\n"},{"id":"492451","messageId":"CACXAH52+ZZGW7xwutZ8Chrap3e07Xm-5zwDg=L2PBV2nVEvOXQ@mail.gmail.com","threadId":"61285","inReplyTo":"1e5dfc946c38809f159776f41e23c7c6@manjaro.org","subject":"Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions","fromName":"Quang Lê Duy","fromEmail":"leduyquang753@gmail.com","sentAt":"2024-04-07T08:46:13Z","receivedAt":"2024-04-07T08:46:51Z","isPatch":true,"sender":{"key":"leduyquang753@gmail.com","avatar":"https://gravatar.com/avatar/f15e7b4c0bd656dd16b43b810d31934f4775f409784afdacd5e387ac4f5fbe59?d=mp&s=160"},"body":"On Sun, Apr 7, 2024 at 3:34 PM Dragan Simic <dsimic@manjaro.org> wrote:\n> These days, very few computer screens aren't in widescreen format,\n> so there's inevitably less vertical screen space available than the\n> horizontal space.\n\nThe ability to scroll makes all the diffence though, there is quite a chance any\nnon-trivial commit log is going to span more than one screen height already\nanyway. There's no risk in sprinkling a few more lines into it; meanwhile if the\nnumber of columns actually gets too many, in many cases you will not have a\nhorizontal scrollbar to play with.\n\nBut it depends on how this idea will be expanded, as it has been only an initial\nhint from the original reply.\n"},{"id":"492453","messageId":"CAPig+cSD7ONA-MUt=dxt5nh2FQ1nJjbcdDb0ko0j0sBrr0reqg@mail.gmail.com","threadId":"61285","inReplyTo":"CACXAH50jwngEQLF612FMyZ36yB5Nd7vVHS98KMWYjXqcNzvpwg@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] Add separator lines into `git log --graph`.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-04-07T09:07:37Z","receivedAt":"2024-04-07T09:07:50Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Apr 7, 2024 at 3:04 AM Quang Lê Duy <leduyquang753@gmail.com> wrote:\n> On Sun, Apr 7, 2024 at 12:47 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > > +       return graph->connected_region_state == CONNECTED_REGION_NEW_REGION || (graph->num_parents >= 3 &&\n> > > +       graph->connected_region_state = (graph->connected_region_state != CONNECTED_REGION_FIRST_COMMIT &&\n> > > +               graph->num_new_columns == 0) ? CONNECTED_REGION_NEW_REGION : CONNECTED_REGION_USE_CURRENT;\n> >\n> > Style: overly long lines\n>\n> May I ask how am I expected to place the line breaks? The Linux kernel style\n> guide I consulted\n> (https://www.kernel.org/doc/html/v4.10/process/coding-style.html) doesn't seem\n> to go into too much detail on this.\n\nI don't have a precise answer other than \"use good taste\". One\nreasonably solid rule is that when wrapping at `&&` and `||`, those\noperators should appear at the end of the line rather than the\nbeginning of the next line. So, a possible wrapping for these two\ncases might be:\n\n    return graph->connected_region_state == CONNECTED_REGION_NEW_REGION ||\n        (graph->num_parents >= 3 &&\n        graph->commit_index < (graph->num_columns - 1) &&\n        graph->expansion_row < graph_num_expansion_rows(graph));\n\n    graph->connected_region_state =\n        (graph->connected_region_state != CONNECTED_REGION_FIRST_COMMIT &&\n        graph->num_new_columns == 0) ?\n        CONNECTED_REGION_NEW_REGION : CONNECTED_REGION_USE_CURRENT;\n\nSince this enum is private to the C file and not part of an expressive\npublic API, another possibility for reducing the line length is to\nshorten some of the names. For instance:\n\n    enum connected_region_state {\n        CONNREG_FIRST_COMMIT,\n        CONNREG_USE_CURRENT,\n        CONNREG_NEW_REGION\n    };\n\n> > > +static void graph_output_separator_line(struct git_graph *graph, struct graph_line *line)\n> > > +{\n> > > +       /*\n> > > +        * This function adds a row that separates two disconnected graphs,\n> > > +        * as the appearance of multiple separate commits on top of each other\n> > > +        * may cause a misunderstanding that they belong to a timeline.\n> > > +        */\n> >\n> > This comment seems to explain the purpose of the function itself. As\n> > such, it should precede the function definition rather than being\n> > embedded within it.\n>\n> I just followed what the surrounding code did (particularly in the original\n> `graph_output_pre_commit_line` function), but on second look that functionality\n> comment seems to only serve as context for the sentence below that so OK.\n\nIndeed, looking at graph_output_pre_commit_line(), the comment seems\nto be explaining the reason for the assert() in that function, whereas\nthe comment you wrote here seems to be explaining the purpose of the\nfunction itself.\n\n> > > +       assert(graph->connected_region_state == CONNECTED_REGION_NEW_REGION);\n> >\n> > We tend to use BUG() rather than assert():\n>\n> Same thing, I just followed that `graph_output_pre_commit_line` did. So I should\n> forgo the consistency here? Or is that usage of `assert` in the existing code\n> also to be updated?\n\nI see what you mean, now that I'm looking at graph.c. Since assert()\nis used so heavily in this file already (and there are no BUG()\ninvocations at all), it probably makes sense to be consistent and use\nassert() here, as well. Adding a sentence to the commit message\nexplaining that you're using assert() for consistency rather than\nBUG() will be helpful to reviewers.\n\nWhile it might be a nice cleanup to eventually swap out assert() in\nfavor of BUG(), we should leave that for another day in order to keep\nthis patch well-focused. (We don't want to add a bunch of \"while at\nit, let's also change this\" items, thus losing focus on what you\nactually want to achieve.)\n\n> > > +       /*\n> > > +        * Output the row.\n> > > +        */\n> > > +       graph_line_addstr(line, \"---\");\n> >\n> > The code itself is obvious enough without the comment, so the comment\n> > is mere noise, thus should be dropped.\n>\n> Also same thing that I followed for consistency.\n\nUnderstandable. In this case, I don't personally feel that this\ncomment is adding any value, thus would drop it, but others (including\nyourself) may feel differently.\n\n> > Modern test style is to perform all actions inside the\n> > test_expect_success body itself, so:\n> >\n> >     test_expect_success 'all commits' '\n> >         cat >expect <<-\\EOF\n> >         ...\n> >         EOF\n> >         test_cmp_graph a b c d e\n> >     '\n> >\n> > Note the use of <<- to allow you to indent the here-doc body.\n>\n> This is also because I followed what `t4202-log.sh` did, but if that represents\n> outdated practice then I'll change.\n\nUnderstood.\n\nGenerally speaking, when adding new tests, we do want to follow modern\npractice; that's especially true when creating a brand new test\nscript, but even when adding new tests to an existing script.\n\nIf you're modifying an existing test, then being consistent with the\nsurrounding code is a good idea. Consistency may also be reasonable\nsometimes when inserting a new test into a block of existing\nclosely-related tests. Saying so in the commit message will help\nreviewers understand.\n"},{"id":"492454","messageId":"e4919f6afe9db5fa62769bb52c6f1e00@manjaro.org","threadId":"61285","inReplyTo":"CACXAH52+ZZGW7xwutZ8Chrap3e07Xm-5zwDg=L2PBV2nVEvOXQ@mail.gmail.com","subject":"Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-04-07T09:13:45Z","receivedAt":"2024-04-07T09:13:47Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-04-07 10:46, Quang Lê Duy wrote:\n> On Sun, Apr 7, 2024 at 3:34 PM Dragan Simic <dsimic@manjaro.org> wrote:\n>> These days, very few computer screens aren't in widescreen format,\n>> so there's inevitably less vertical screen space available than the\n>> horizontal space.\n> \n> The ability to scroll makes all the diffence though, there is quite a \n> chance any\n> non-trivial commit log is going to span more than one screen height \n> already\n> anyway. There's no risk in sprinkling a few more lines into it; \n> meanwhile if the\n> number of columns actually gets too many, in many cases you will not \n> have a\n> horizontal scrollbar to play with.\n> \n> But it depends on how this idea will be expanded, as it has been only \n> an initial\n> hint from the original reply.\n\nPlease note that less(1), which is commonly used as the pager, also\nsupports horizontal scrolling.  Perhaps that isn't a very well known\nfeature of less(1), but it is supported.\n"},{"id":"492545","messageId":"xmqqy19netx1.fsf@gitster.g","threadId":"61285","inReplyTo":"xmqq4jcdivgw.fsf@gitster.g","subject":"Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-08T15:49:30Z","receivedAt":"2024-04-08T15:49:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> True.  But because we are doing graph, shouldn't we be able to do\n> better?  For example, you can draw the two lineage of histories\n> on different columns and ...\n>\n>        * a2\n>        * a1\n>      * b2\n>      * b1\n>\n> ... that way, you do not need to lose one line of precious vertical\n> screen real estate.\n\nJust to save others time to go spelunking in the list archive,\n\"solutions\" attempted and did not work in the past include\n\n (1) wasting a line to insert a \"gap\" in the output.\n (2) using marker different from '*' to mark each commit.\n\nIf taking the latter approach, it needs to designed to work well\nwith \"boundary\" and \"left-right\" options (the design to shift column\nwould not have to worry about these, which is another plus).\n\nThanks.\n"}]}