[RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions

[RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions

[Lê Duy Quang]

`git log --graph`, when invoked with multiple starting revisions and some
exclusions which cut the commit graph, may give a disconnected graph. In other
words, the resulting graph has more than one separate connected regions. The
command currently prints the connected regions on top of each other without any
separation.

This leads to a problem. Say there are two connected regions, each having two
commits, the graph would look like this:

* a2
* a1
* b2
* b1

which may lead to a misunderstanding that these four commits belong to the same
timeline, i.e. b2 is a parent of a1.

This patchset adds a separator line between each pair of connected regions to
clarify that they are not actually connected:

* a2
* a1
---
* b2
* b1

Lê Duy Quang (1):
  Add separator lines into `git log --graph`.

 graph.c                                |  55 +++++++++++-
 t/t4218-log-graph-connected-regions.sh | 119 +++++++++++++++++++++++++
 2 files changed, 170 insertions(+), 4 deletions(-)
 create mode 100755 t/t4218-log-graph-connected-regions.sh


base-commit: 19981daefd7c147444462739375462b49412ce33
-- 
2.44.0

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

[Lê Duy Quang]

This is to separate out connected regions of the resulting commit graph so as
to not have them confused as belonging to the same timeline.
---
 graph.c                                |  55 +++++++++++-
 t/t4218-log-graph-connected-regions.sh | 119 +++++++++++++++++++++++++
 2 files changed, 170 insertions(+), 4 deletions(-)
 create mode 100755 t/t4218-log-graph-connected-regions.sh

diff --git a/graph.c b/graph.c
index 1ca34770ee..c0107c02fa 100644
--- a/graph.c
+++ b/graph.c
@@ -69,6 +69,12 @@ enum graph_state {
 	GRAPH_COLLAPSING
 };
 
+enum connected_region_state {
+	CONNECTED_REGION_FIRST_COMMIT,
+	CONNECTED_REGION_USE_CURRENT,
+	CONNECTED_REGION_NEW_REGION
+};
+
 static void graph_show_line_prefix(const struct diff_options *diffopt)
 {
 	if (!diffopt || !diffopt->line_prefix)
@@ -310,6 +316,12 @@ struct git_graph {
 	 * stored as an index into the array column_colors.
 	 */
 	unsigned short default_column_color;
+	/*
+	 * The state of which connected region the current commit belongs to.
+	 * This is used to output a clarifying separator line between
+	 * connected regions.
+	 */
+	enum connected_region_state connected_region_state;
 };
 
 static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void *data)
@@ -380,6 +392,7 @@ struct git_graph *graph_init(struct rev_info *opt)
 	 * This way we start at 0 for the first commit.
 	 */
 	graph->default_column_color = column_colors_max - 1;
+	graph->connected_region_state = CONNECTED_REGION_FIRST_COMMIT;
 
 	/*
 	 * Allocate a reasonably large default number of columns
@@ -729,9 +742,9 @@ static int graph_num_expansion_rows(struct 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 &&
 	       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)
 	 * commit.
 	 */
 	graph->prev_commit_index = graph->commit_index;
+	
+	/*
+	 * 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;
 
 	/*
 	 * Call graph_update_columns() to update
@@ -865,8 +884,28 @@ static void graph_output_skip_line(struct git_graph *graph, struct graph_line *l
 		graph_update_state(graph, GRAPH_COMMIT);
 }
 
-static void graph_output_pre_commit_line(struct git_graph *graph,
-					 struct graph_line *line)
+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.
+	 */
+	assert(graph->connected_region_state == CONNECTED_REGION_NEW_REGION);
+
+	/*
+	 * Output the row.
+	 */
+	graph_line_addstr(line, "---");
+
+	/*
+	 * 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);
+}
+
+static void graph_output_parent_expansion_line(struct git_graph *graph, struct graph_line *line)
 {
 	int i, seen_this;
 
@@ -928,6 +967,14 @@ static void graph_output_pre_commit_line(struct git_graph *graph,
 		graph_update_state(graph, GRAPH_COMMIT);
 }
 
+static void graph_output_pre_commit_line(struct git_graph *graph, struct graph_line *line)
+{
+	if (graph->connected_region_state == CONNECTED_REGION_NEW_REGION)
+		graph_output_separator_line(graph, line);
+	else
+		graph_output_parent_expansion_line(graph, line);
+}
+
 static void graph_output_commit_char(struct git_graph *graph, struct graph_line *line)
 {
 	/*
diff --git a/t/t4218-log-graph-connected-regions.sh b/t/t4218-log-graph-connected-regions.sh
new file mode 100755
index 0000000000..4efe17827e
--- /dev/null
+++ b/t/t4218-log-graph-connected-regions.sh
@@ -0,0 +1,119 @@
+#!/bin/sh
+
+test_description="git log --graph connected regions"
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+. "$TEST_DIRECTORY/lib-terminal.sh"
+. "$TEST_DIRECTORY/lib-log-graph.sh"
+
+test_cmp_graph () {
+	lib_test_cmp_graph --format=%s "$@"
+}
+
+add_commit () {
+	touch $1 &&
+	git add $1 &&
+	git commit -m $1
+	git tag "$1-commit"
+}
+
+test_expect_success setup '
+	git checkout -b a &&
+	add_commit root &&
+	
+	add_commit a1 &&
+	add_commit a2 &&
+	add_commit a3 &&
+	
+	git checkout -b b root-commit &&
+	add_commit b1 &&
+	add_commit b2 &&
+	git checkout -b c &&
+	add_commit c3 &&
+	git checkout b &&
+	add_commit b3 &&
+	git merge c -m b4 &&
+	
+	git checkout -b d root-commit &&
+	add_commit d1 &&
+	add_commit d2 &&
+	git checkout -b e &&
+	add_commit e3 &&
+	git checkout d &&
+	add_commit d3 &&
+	add_commit d4
+'
+
+cat > expect <<\EOF
+* a3
+* a2
+* a1
+| *   b4
+| |\
+| | * c3
+| * | b3
+| |/
+| * b2
+| * b1
+|/
+| * d4
+| * d3
+| | * e3
+| |/
+| * d2
+| * d1
+|/
+* root
+EOF
+
+test_expect_success 'all commits' '
+	test_cmp_graph a b c d e
+'
+
+cat > expect <<\EOF
+* a3
+* a2
+* a1
+---
+*   b4
+|\
+| * c3
+* | b3
+|/
+* b2
+* b1
+---
+* d4
+* d3
+| * e3
+|/
+* d2
+* d1
+EOF
+
+test_expect_success 'without root commit' '
+	test_cmp_graph a b c d e ^root-commit
+'
+
+cat > expect <<\EOF
+* a3
+---
+*   b4
+|\
+| * c3
+* b3
+---
+* d4
+* d3
+---
+* e3
+EOF
+
+test_expect_success "branches' tips" '
+	test_cmp_graph a b c d e ^a2-commit ^b2-commit ^d2-commit
+'
+
+test_done
-- 
2.44.0

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

[Eric Sunshine]

On Sun, Apr 7, 2024 at 1:10 AM Lê Duy Quang <leduyquang753@gmail.com> wrote:
> This is to separate out connected regions of the resulting commit graph so as
> to not have them confused as belonging to the same timeline.
> ---

I'm not particularly a user of --graph, so I don't necessarily have an
opinion about the utility of this change or its mechanics, but I can
make a few observations to help you improve the patch to improve the
chances of it being accepted.

First, move the information from the cover letter into the commit
message of the patch itself since that information will be helpful to
future readers of the patch if it becomes part of the permanent
history.

Second, following Documentation/SubmittingPatches guidelines, the
subject could instead be written something like this:

    log: visually separate `git log --graph` regions

Third, add a Signed-off-by: trailer after the commit message (see
SubmittingPatches).

> diff --git a/graph.c b/graph.c
> @@ -729,9 +742,9 @@ static int graph_num_expansion_rows(struct 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

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

> +       assert(graph->connected_region_state == CONNECTED_REGION_NEW_REGION);

We tend to use BUG() rather than assert():

    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.

> +       /*
> +        * 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.

> @@ -0,0 +1,119 @@
> +#!/bin/sh
> +
> +test_description="git log --graph connected regions"
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +. "$TEST_DIRECTORY/lib-terminal.sh"
> +. "$TEST_DIRECTORY/lib-log-graph.sh"

"lib-terminal.sh" doesn't seem to be needed by these tests.

> +test_cmp_graph () {
> +       lib_test_cmp_graph --format=%s "$@"
> +}
> +
> +add_commit () {
> +       touch $1 &&

If the timestamp of the empty file being created is not significant,
we avoid `touch` and instead use `>` to create the file:

    >"$1" &&

> +       git add $1 &&
> +       git commit -m $1
> +       git tag "$1-commit"
> +}

Is this add_commit() function more or less duplicating the
functionality of test_commit() from t/test-lib-functions.sh?

> +cat > expect <<\EOF

Style: drop whitespace following redirect operators:

    cat >expect <<\EOF

> +* a3
> +* a2
> +* a1
> +| *   b4
> +| |\
> +| | * c3
> +| * | b3
> +| |/
> +| * b2
> +| * b1
> +|/
> +| * d4
> +| * d3
> +| | * e3
> +| |/
> +| * d2
> +| * d1
> +|/
> +* root
> +EOF
> +
> +test_expect_success 'all commits' '
> +       test_cmp_graph a b c d e
> +'

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.

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

[Eric Sunshine]

On Sun, Apr 7, 2024 at 1:47 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> I'm not particularly a user of --graph, so I don't necessarily have an
> opinion about the utility of this change or its mechanics, but I can
> make a few observations to help you improve the patch to improve the
> chances of it being accepted.

I forgot to mention that application of your patch results in some warnings:

    % git am add-sep-lines.patch
    Applying: Add separator lines into `git log --graph`.
    .git/rebase-apply/patch:61: trailing whitespace.
    .git/rebase-apply/patch:147: trailing whitespace.
    .git/rebase-apply/patch:151: trailing whitespace.
    .git/rebase-apply/patch:160: trailing whitespace.
    warning: 4 lines add whitespace errors.

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

[Quang Lê Duy]

On Sun, Apr 7, 2024 at 12:52 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> I forgot to mention that application of your patch results in some warnings:
>
>     % git am add-sep-lines.patch
>     Applying: Add separator lines into `git log --graph`.
>     .git/rebase-apply/patch:61: trailing whitespace.
>     .git/rebase-apply/patch:147: trailing whitespace.
>     .git/rebase-apply/patch:151: trailing whitespace.
>     .git/rebase-apply/patch:160: trailing whitespace.
>     warning: 4 lines add whitespace errors.

Indeed I failed to notice the whitespace `vim` added to the empty lines.
Appreciate your notice.

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

[Dragan Simic]

On 2024-04-07 09:06, Quang Lê Duy wrote:
> On Sun, Apr 7, 2024 at 12:52 PM Eric Sunshine <sunshine@sunshineco.com> 
> wrote:
>> I forgot to mention that application of your patch results in some 
>> warnings:
>> 
>>     % git am add-sep-lines.patch
>>     Applying: Add separator lines into `git log --graph`.
>>     .git/rebase-apply/patch:61: trailing whitespace.
>>     .git/rebase-apply/patch:147: trailing whitespace.
>>     .git/rebase-apply/patch:151: trailing whitespace.
>>     .git/rebase-apply/patch:160: trailing whitespace.
>>     warning: 4 lines add whitespace errors.
> 
> Indeed I failed to notice the whitespace `vim` added to the empty 
> lines.
> Appreciate your notice.

As a note, vim can be configured to highlight the trailing
whitespace, making it easy to spot.

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

[Quang Lê Duy]

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

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

[Eric Sunshine]

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.

Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions

[Eric Sunshine]

On Sun, Apr 7, 2024 at 1:10 AM Lê Duy Quang <leduyquang753@gmail.com> wrote:
> `git log --graph`, when invoked with multiple starting revisions and some
> exclusions which cut the commit graph, may give a disconnected graph. In other
> words, the resulting graph has more than one separate connected regions. The
> command currently prints the connected regions on top of each other without any
> separation.
>
> This leads to a problem. Say there are two connected regions, each having two
> commits, the graph would look like this:
>
> * a2
> * a1
> * b2
> * b1
>
> which may lead to a misunderstanding that these four commits belong to the same
> timeline, i.e. b2 is a parent of a1.
>
> This patchset adds a separator line between each pair of connected regions to
> clarify that they are not actually connected:
>
> * a2
> * a1
> ---
> * b2
> * b1

This sort of information which explains why the patch may be desirable
is not only helpful to reviewers of the submission, but will be
helpful to future readers of the patch once it becomes part of the
project's permanent history (assuming it is accepted). However, the
cover letter does not become part of the project's history (it exists
only in the mailing list). As such, please move this discussion into
the commit message of the patch itself.

Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> This leads to a problem. Say there are two connected regions, each having two
>> commits, the graph would look like this:
>>
>> * a2
>> * a1
>> * b2
>> * b1
>>
>> which may lead to a misunderstanding that these four commits belong to the same
>> timeline, i.e. b2 is a parent of a1.
>>
>> This patchset adds a separator line between each pair of connected regions to
>> clarify that they are not actually connected:
>>
>> * a2
>> * a1
>> ---
>> * b2
>> * b1
>
> This sort of information which explains why the patch may be desirable
> is not only helpful to reviewers of the submission, but will be
> helpful to future readers of the patch once it becomes part of the
> project's permanent history (assuming it is accepted). However, the
> cover letter does not become part of the project's history (it exists
> only in the mailing list). As such, please move this discussion into
> the commit message of the patch itself.

True.  But because we are doing graph, shouldn't we be able to do
better?  For example, you can draw the two lineage of histories
on different columns and ...

       * a2
       * a1
     * b2
     * b1

... that way, you do not need to lose one line of precious vertical
screen real estate.

Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions

[Quang Lê Duy]

On Sun, Apr 7, 2024 at 12:37 PM Junio C Hamano <gitster@pobox.com> wrote:
> True.  But because we are doing graph, shouldn't we be able to do
> better?  For example, you can draw the two lineage of histories
> on different columns and ...
>
>        * a2
>        * a1
>      * b2
>      * b1
>
> ... that way, you do not need to lose one line of precious vertical
> screen real estate.

I think horizontal screen real estate is even more precious than the vertical
one, since one usually doesn't scroll their terminal horizontally. And then it
would probably be a way more complicated implementation.

Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions

[Dragan Simic]

Hello Quang,

On 2024-04-07 08:40, Quang Lê Duy wrote:
> On Sun, Apr 7, 2024 at 12:37 PM Junio C Hamano <gitster@pobox.com> 
> wrote:
>> True.  But because we are doing graph, shouldn't we be able to do
>> better?  For example, you can draw the two lineage of histories
>> on different columns and ...
>> 
>>        * a2
>>        * a1
>>      * b2
>>      * b1
>> 
>> ... that way, you do not need to lose one line of precious vertical
>> screen real estate.
> 
> I think horizontal screen real estate is even more precious than the 
> vertical
> one, since one usually doesn't scroll their terminal horizontally. And 
> then it
> would probably be a way more complicated implementation.

These days, very few computer screens aren't in widescreen format,
so there's inevitably less vertical screen space available than the
horizontal space.

Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions

[Quang Lê Duy]

On Sun, Apr 7, 2024 at 3:34 PM Dragan Simic <dsimic@manjaro.org> wrote:
> These days, very few computer screens aren't in widescreen format,
> so there's inevitably less vertical screen space available than the
> horizontal space.

The ability to scroll makes all the diffence though, there is quite a chance any
non-trivial commit log is going to span more than one screen height already
anyway. There's no risk in sprinkling a few more lines into it; meanwhile if the
number of columns actually gets too many, in many cases you will not have a
horizontal scrollbar to play with.

But it depends on how this idea will be expanded, as it has been only an initial
hint from the original reply.

Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions

[Dragan Simic]

On 2024-04-07 10:46, Quang Lê Duy wrote:
> On Sun, Apr 7, 2024 at 3:34 PM Dragan Simic <dsimic@manjaro.org> wrote:
>> These days, very few computer screens aren't in widescreen format,
>> so there's inevitably less vertical screen space available than the
>> horizontal space.
> 
> The ability to scroll makes all the diffence though, there is quite a 
> chance any
> non-trivial commit log is going to span more than one screen height 
> already
> anyway. There's no risk in sprinkling a few more lines into it; 
> meanwhile if the
> number of columns actually gets too many, in many cases you will not 
> have a
> horizontal scrollbar to play with.
> 
> But it depends on how this idea will be expanded, as it has been only 
> an initial
> hint from the original reply.

Please note that less(1), which is commonly used as the pager, also
supports horizontal scrolling.  Perhaps that isn't a very well known
feature of less(1), but it is supported.

Re: [RFC PATCH 0/1] Add lines to `git log --graph` to separate connected regions

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> True.  But because we are doing graph, shouldn't we be able to do
> better?  For example, you can draw the two lineage of histories
> on different columns and ...
>
>        * a2
>        * a1
>      * b2
>      * b1
>
> ... that way, you do not need to lose one line of precious vertical
> screen real estate.

Just to save others time to go spelunking in the list archive,
"solutions" attempted and did not work in the past include

 (1) wasting a line to insert a "gap" in the output.
 (2) using marker different from '*' to mark each commit.

If taking the latter approach, it needs to designed to work well
with "boundary" and "left-right" options (the design to shift column
would not have to worry about these, which is another plus).

Thanks.