[PATCH] graph.c: visual difference on subsequent series

[PATCH] graph.c: visual difference on subsequent series

[Milton Soares Filho]

For projects with separate history lines and, thus, multiple root-commits, the
linear arrangement of `git log --graph --oneline` does not allow the user to
spot where the sequence ends, giving the impression that it's a contiguous
history. E.g.

History sequence A: a1 -- a2 -- a3 (root-commit)
History sequence B: b1 -- b2 -- b3 (root-commit)

    git log --graph --oneline
    * a1
    * a2
    * a3
    * b1
    * b2
    * b3

In a GUI tool, the root-commit of each series would stand out on the graph.

This modification changes the commit char to a different symbol ('x'), so users
of the command-line graph tool can easily identify root-commits and make sense
of where each series is limited to.

    git log --graph --oneline
    * a1
    * a2
    x a3
    * b1
    * b2
    x b3

Signed-off-by: Milton Soares Filho <milton.soares.filho@gmail.com>
---
 graph.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/graph.c b/graph.c
index b24d04c..ec8e960 100644
--- a/graph.c
+++ b/graph.c
@@ -780,6 +780,15 @@ static void graph_output_commit_char(struct git_graph *graph, struct strbuf *sb)
 	}
 
 	/*
+	 * Out-stand parentless commits to enforce non-continuity on subsequent
+	 * but separate series
+	 */
+	if (graph->commit->parents == NULL) {
+		strbuf_addch(sb, 'x');
+		return;
+	}
+
+	/*
 	 * get_revision_mark() handles all other cases without assert()
 	 */
 	strbuf_addstr(sb, get_revision_mark(graph->revs, graph->commit));
-- 
1.8.1.2

Re: [PATCH] graph.c: visual difference on subsequent series

[Junio C Hamano]

Milton Soares Filho <milton.soares.filho@gmail.com> writes:

>     git log --graph --oneline
>     * a1
>     * a2
>     x a3
>     * b1
>     * b2
>     x b3

I agree that the problem you are trying to solve is a good thing to
tackle, and I also agree that marking a root commit differently from
other commits is one way to solve it, but I am not sure if that is
the best way.  If the stretches of a's and b's in your history are
very long, wouldn't it be easier to spot if they are painted in
different colours, in addition to or instead of marking the roots
differently [*1*], for example?

>  	/*
> +	 * Out-stand parentless commits to enforce non-continuity on subsequent
> +	 * but separate series
> +	 */
> +	if (graph->commit->parents == NULL) {
> +		strbuf_addch(sb, 'x');
> +		return;
> +	}
> +
> +	/*
>  	 * get_revision_mark() handles all other cases without assert()
>  	 */
>  	strbuf_addstr(sb, get_revision_mark(graph->revs, graph->commit));

It is unclear why the update goes to this function. At the first
glance, I feel that it would be more sensible to add the equivalent
code to get_revision_mark()---we do not have to worry about what
else, other than calling get_revision_mark() and adding it to sb,
would be skipped by the added "return" when we later have to update
this function and add more code after the existing strbuf_addstr().

The change implemented your way will lose other information when a
root commit is at the boundary, marked as uninteresting, or on the
left/right side of traversal (when --left-right is requested).  I
think these pieces of information your patch seems to be losing are
a lot more relevant than "have we hit the root?", especially in the
majority of repositories where there is only one root commit.

Thanks.


[Footnote]

*1* Note that I am not saying "the change the patch introduces is
not sufficient and you have to paint the commits in different
colors" here. I myself think it would be a lot more work to do so,
and I even suspect that it may be asking for the moon---you may not
even know what root "a1" (and "b1") came from when you are showing
these commits without first digging down to the roots and then
walking the history backwards, which may not be practically
feasible.

Re: [PATCH] graph.c: visual difference on subsequent series

[Milton Soares Filho]

On 25 October 2013 15:13, Junio C Hamano <gitster@pobox.com> wrote:
> Milton Soares Filho <milton.soares.filho@gmail.com> writes:
>
>>     git log --graph --oneline
>>     * a1
>>     * a2
>>     x a3
>>     * b1
>>     * b2
>>     x b3
>
> I agree that the problem you are trying to solve is a good thing to
> tackle, and I also agree that marking a root commit differently from
> other commits is one way to solve it, but I am not sure if that is
> the best way.  If the stretches of a's and b's in your history are
> very long, wouldn't it be easier to spot if they are painted in
> different colours, in addition to or instead of marking the roots
> differently [*1*], for example?

Thanks for taking your time reviewing this patch, Junio. I didn't really thought
it would get any attention since multiple root-commits is not a very common
use-case[1]. However, if most people got excited with git-subtree new
features as I did, there is a good chance that multiple root-commits are
going to become a common-place in the near future ;-)

That said, I completely agree that painting with different colors would be
a much better fix, however I believe that it can be done in a separate
changeset by someone that understands better the impact on the rest
of the system. Personally, changing only the mark is sufficient because:

a) it'll work on terminal types without coloring support and configurations
    whose explicitly disable it
b) it'll spare myself of running a separate GUI program just
    to spot where each series begin
c) it won't require any visual design skills from a developer (me)
    without a minimal sense for it :-)

By the way, is there a visual or design guideline document for building
decorated log graphs? From where comes the inspiration of it?

> The change implemented your way will lose other information when a
> root commit is at the boundary, marked as uninteresting, or on the
> left/right side of traversal (when --left-right is requested).  I
> think these pieces of information your patch seems to be losing are
> a lot more relevant than "have we hit the root?", especially in the
> majority of repositories where there is only one root commit.

Nice. I'll try to move the logic into get_revision_mark() and hope
the priority on handling it is better suited.

> [...]
> and I even suspect that it may be asking for the moon---you may not
> even know what root "a1" (and "b1") came from when you are showing
> these commits without first digging down to the roots and then
> walking the history backwards, which may not be practically
> feasible.

It'd be nice to figure out a test-case to emerge it.

[]s, milton

[1]: In git  repository itself I could find only seven of them (root-commis)

Re: [PATCH] graph.c: visual difference on subsequent series

[Keshav Kini]

Milton Soares Filho <milton.soares.filho@gmail.com> writes:
> On 25 October 2013 15:13, Junio C Hamano <gitster@pobox.com> wrote:
>> Milton Soares Filho <milton.soares.filho@gmail.com> writes:
>>
>>>     git log --graph --oneline
>>>     * a1
>>>     * a2
>>>     x a3
>>>     * b1
>>>     * b2
>>>     x b3
>>
>> I agree that the problem you are trying to solve is a good thing to
>> tackle, and I also agree that marking a root commit differently from
>> other commits is one way to solve it, but I am not sure if that is
>> the best way.  If the stretches of a's and b's in your history are
>> very long, wouldn't it be easier to spot if they are painted in
>> different colours, in addition to or instead of marking the roots
>> differently [*1*], for example?
>
> Thanks for taking your time reviewing this patch, Junio. I didn't really thought
> it would get any attention since multiple root-commits is not a very common
> use-case[1]. However, if most people got excited with git-subtree new
> features as I did, there is a good chance that multiple root-commits are
> going to become a common-place in the near future ;-)

I don't think this is that obscure. I've often thought there should be
some way to distinguish root commits as well.  In fact when dealing with
multiple root commits I usually just don't use --oneline and instead use
the full --graph view so I can find root commits by grepping for '^  ' :)

I should also mention that there are lots of situations where you might
see multiple "root commits" not because there are truly multiple commits
with no parent in the repository, but because you're looking at some
subgraph of the history graph -- that is, you have multiple commits in
your display whose parents are purposely excluded. For example, you
might be looking at a revision list like 'C ^A ^B':

    master
    |  .---------------B
    | /       `-------------.
    O<                   .---`--C
    | \                 /
    |  `---------------A

The commits you were looking at would be these ones:

              `-------------.
                         .---`--C
                        /

So multiple "roots" can appear easily in such cases.

> That said, I completely agree that painting with different colors would be
> a much better fix, however I believe that it can be done in a separate
> changeset by someone that understands better the impact on the rest
> of the system. Personally, changing only the mark is sufficient because:
>
> a) it'll work on terminal types without coloring support and configurations
>     whose explicitly disable it
> b) it'll spare myself of running a separate GUI program just
>     to spot where each series begin
> c) it won't require any visual design skills from a developer (me)
>     without a minimal sense for it :-)

I'm a bit worried that if someone is parsing `git log --graph` output
looking for `*` lines they might suddenly start missing the root commits
that they were previously able to find.  I mean, not that anyone should
be doing that, but if we can avoid breaking that, why not do so?

What about just putting an extra blank line after every root commit line
(possibly except the last one)?  That should make it plenty easy to see
where the root commits are in --oneline mode.  I think it would actually
be easier to spot at a glance than replacing `*` with `x` because it
creates a gap in all columns of the output, rather than only in column
1.  Also, this is very subjective but I think it looks kind of ugly to
use "x" :P

By the by, you might want to use the `-v` argument to `git send-email`
so that people reading the list can tell at a glance which patch
versions are newer than which other patch versions.

-Keshav

Re: [PATCH] graph.c: visual difference on subsequent series

[Junio C Hamano]

[administrivia: please avoid culling addresses from To:/Cc: lines]

Keshav Kini <keshav.kini@gmail.com> writes:

> What about just putting an extra blank line after every root commit line
> (possibly except the last one)?  That should make it plenty easy to see
> where the root commits are in --oneline mode.  I think it would actually
> be easier to spot at a glance than replacing `*` with `x` because it
> creates a gap in all columns of the output, rather than only in column
> 1.  Also, this is very subjective but I think it looks kind of ugly to
> use "x" :P

I agree to all of the above, including the ugliness of 'x' ;-)

A "blank" may however be hard to spot, if the range is limited,
though.  For example,

    $ git log --graph --oneline a4..
      * HEAD
     /* a1
    | * a2
    | * a3
    * b1
    * b2
    * b3

where "a4", which is a root, is the sole parent of "a3" and HEAD is
a merge between "a1" and "b1" might produce something like this,
while we may get this from the same history, when shown unlimited:

    $ git log --graph --oneline
      * HEAD
     /* a1
    | * a2
    | * a3
    | * a4
    |
    * b1
    * b2
    * b3

A divider line might make it visually a lot more strong, i.e.

    $ git log --graph --oneline
      * HEAD
     /* a1
    | * a2
    | * a3
    | * a4
    |   ~~~~~~~~~~~~~~~~~~~~~~~
    * b1
    * b2
    * b3

but I am not sure if it is too distracting.

Re: [PATCH] graph.c: visual difference on subsequent series

[Keshav Kini]

Junio C Hamano <gitster@pobox.com> writes:
> [administrivia: please avoid culling addresses from To:/Cc: lines]

Yikes, sorry about that.  I've been sending messages through Gmane
rather than via email, and I didn't realize the list didn't
automatically send messages to the appropriate people who are only
reading the list via actual email (as I am not such a person).

> Keshav Kini <keshav.kini@gmail.com> writes:
>> What about just putting an extra blank line after every root commit line
>> (possibly except the last one)?  That should make it plenty easy to see
>> where the root commits are in --oneline mode.  I think it would actually
>> be easier to spot at a glance than replacing `*` with `x` because it
>> creates a gap in all columns of the output, rather than only in column
>> 1.  Also, this is very subjective but I think it looks kind of ugly to
>> use "x" :P
>
> I agree to all of the above, including the ugliness of 'x' ;-)
>
> A "blank" may however be hard to spot, if the range is limited,
> though.  For example,
>
>     $ git log --graph --oneline a4..
>       * HEAD
>      /* a1
>     | * a2
>     | * a3
>     * b1
>     * b2
>     * b3
>
> where "a4", which is a root, is the sole parent of "a3" and HEAD is
> a merge between "a1" and "b1" might produce something like this,
> while we may get this from the same history, when shown unlimited:
>
>     $ git log --graph --oneline
>       * HEAD
>      /* a1
>     | * a2
>     | * a3
>     | * a4
>     |
>     * b1
>     * b2
>     * b3
>
> A divider line might make it visually a lot more strong, i.e.
>
>     $ git log --graph --oneline
>       * HEAD
>      /* a1
>     | * a2
>     | * a3
>     | * a4
>     |   ~~~~~~~~~~~~~~~~~~~~~~~
>     * b1
>     * b2
>     * b3
>
> but I am not sure if it is too distracting.

I would be fine with that, fwiw.  We can also turn it on and off with a
config option if people really don't like it, I suppose...

-Keshav

Re: [PATCH] graph.c: visual difference on subsequent series

[Milton Soares Filho]

On 28 October 2013 13:41, Junio C Hamano <gitster@pobox.com> wrote:
> I agree to all of the above, including the ugliness of 'x' ;-)
>
> A "blank" may however be hard to spot, if the range is limited,
> though.  For example,

A 'x' looks like termination points in some specification languages
such as SDL and MSC and thus translates directly to the idea of a
root-commit, at least IMO. For sure it does not stand out as blatantly
as it should, but it gives a general idea without further
distractions, which seems to be the idea of a simple 'git log --graph
--oneline'.

An idea that have just come to mind is to have a decorator to enforce
this property, like this.

      * HEAD
     /* a1
    | * a2
    | * a3
    | x a4 (root-commit)
    * b1
    * b2
    x b3  (root-commit)

This way the user only gets 'distracted' if he explicitly asks for it
(--decorate), with all its colors and whatnot. What do you think?
Should I aim for it?

Besides anything else, this discussion is becoming very subjective.
I've received private feedbacks thanking for the changeset and not a
word against the poor 'x'. Maybe it's time to talk to a UI designer or
let a benevolent dictator set this quarrel off ;-)

[]s, milton

Re: [PATCH] graph.c: visual difference on subsequent series

[Junio C Hamano]

Milton Soares Filho <milton.soares.filho@gmail.com> writes:

> On 28 October 2013 13:41, Junio C Hamano <gitster@pobox.com> wrote:
>> I agree to all of the above, including the ugliness of 'x' ;-)
>>
>> A "blank" may however be hard to spot, if the range is limited,
>> though.  For example,
>
> A 'x' looks like termination points in some specification languages
> such as SDL and MSC and thus translates directly to the idea of a
> root-commit, at least IMO. For sure it does not stand out as blatantly
> as it should, but it gives a general idea without further
> distractions, which seems to be the idea of a simple 'git log --graph
> --oneline'.
>
> An idea that have just come to mind is to have a decorator to enforce
> this property, like this.
>
>       * HEAD
>      /* a1
>     | * a2
>     | * a3
>     | x a4 (root-commit)
>     * b1
>     * b2
>     x b3  (root-commit)
>
> This way the user only gets 'distracted' if he explicitly asks for it
> (--decorate), with all its colors and whatnot. What do you think?
> Should I aim for it?
>
> Besides anything else, this discussion is becoming very subjective.

If I have to choose, I'd rather avoid using 'x' or anything that
have to override '*', not just 'x' being ugly, but the approach to
_replace_ the "revision-mark" (usually '*' but sometimes '<', '^',
etc) forces us to give priority between "root-ness" and other kinds
of information (e.g. "left-ness").  That was the primary reason I
liked Keshav's suggestion to use one extra line _below_ the root,
which will allow us to still keep the existing information unlike
what we discussed in our back-and-forth during the initial review.

I also think a blank (or divider) below the root commits does make
it visually obvious that nothing comes _before_ the root commit in
the history, which probably even removes the need to paint the
tracks of histories leading to different roots in different colours.

I hope the above shows that my reaction was much less subjective
than my response sounded ;-)

Thanks.

[RFH/PATCH] graph: give an extra gap after showing root commit

[Junio C Hamano]

With a history with more than one root commit, if a root commit
falls on one display column and another commit that is unrelated to
that root's history is shown on the next line on the same column,
the resulting graph would appear as if the latter is a parent of the
former, like this (there are two histories a1-a2 & b1-b2):

        ------------------------------------------
        $ git log --graph --oneline a2 b2
        * a2
        * a1
        * b2
        * b1
        ------------------------------------------

which is misleading.  b2 is a tip of a history unrelated to the
history that has a1 as a root.

Force a gap line immediately before showing next unrelated commit if
we showed a root from one history, to make the above display look
like this instead:

        ------------------------------------------
        $ git log --graph --oneline a2 b2
        * a2
        * a1

        * b2
        * b1
        ------------------------------------------

but do not waste line when we do not have to.  E.g. with a history
that merges a2 and b2,

        ------------------------------------------
        $ git log --graph --oneline m
        * Merge a2 and b2
        |\
        | * a2
        | * a1
        * b2
        * b1
        ------------------------------------------

there is no need to show an extra blank line after showing a1, as
it is clear that it has no parents.

This takes inspiration from Milton Soares Filho's "graph.c: mark
root commit differently" from a few months ago ($gmane/236708),
which tried to show a root commit as 'x', but only if it would have
been shown as '*' in the original, but uses a different approach so
that a root that may have been painted differently from '*'
(e.g. '<' for "left root") can also be made distinguishable.

Signed-off-by: Junio C Hamano <gitster@pobox.com>

---

 Note that this still does not work very well for --boundary case
 (see the last test added to t6016).

 It may actually make sense to force the "next" commit after showing
 a root always occupy a different column, instead of wasting a blank
 line.  If we did so, the output from the first example may look like
 this:

        ------------------------------------------
        $ git log --graph --oneline a2 b2
        * a2
        * a1
          * b2
          * b1
        ------------------------------------------

 or it may even look like this:

        ------------------------------------------
        $ git log --graph --oneline a2 b2
        * a2
        * a1
          * b2
         /
        * b1
        ------------------------------------------

 I tried to follow graph_update_columns() logic but gave up for now;
 avoiding to place a commit whose descendant has already been seen
 to the same column as the root commit we are processing there is
 easy, but the current commit may not yet be in columns[], and
 graph_output_commit_line() needs to show the current commit beyond
 the end of the columns[] list in such a case, so teaching
 graph_update_columns() to tweak placement of the next line is not
 sufficient.

 graph.c                                    | 20 ++++++++++++++++--
 t/t6016-rev-list-graph-simplify-history.sh | 34 ++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/graph.c b/graph.c
index b24d04c..2c3f141 100644
--- a/graph.c
+++ b/graph.c
@@ -187,6 +187,10 @@ struct git_graph {
 	 * stored as an index into the array column_colors.
 	 */
 	unsigned short default_column_color;
+	/* The last one was a root and we haven't emitted an extra blank */
+	unsigned need_post_root_gap : 1;
+	/* Are we looking at the root? */
+	unsigned is_root : 1;
 };
 
 static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void *data)
@@ -205,7 +209,7 @@ static struct strbuf *diff_output_prefix_callback(struct diff_options *opt, void
 
 struct git_graph *graph_init(struct rev_info *opt)
 {
-	struct git_graph *graph = xmalloc(sizeof(struct git_graph));
+	struct git_graph *graph = xcalloc(1, sizeof(struct git_graph));
 
 	if (!column_colors)
 		graph_set_column_colors(column_colors_ansi,
@@ -552,11 +556,14 @@ static void graph_update_columns(struct git_graph *graph)
 void graph_update(struct git_graph *graph, struct commit *commit)
 {
 	struct commit_list *parent;
+	int was_root = graph->is_root;
 
 	/*
 	 * Set the new commit
 	 */
 	graph->commit = commit;
+	graph->is_root = !commit->parents;
+	graph->need_post_root_gap = 0;
 
 	/*
 	 * Count how many interesting parents this commit has
@@ -607,8 +614,12 @@ void graph_update(struct git_graph *graph, struct commit *commit)
 	else if (graph->num_parents >= 3 &&
 		 graph->commit_index < (graph->num_columns - 1))
 		graph->state = GRAPH_PRE_COMMIT;
-	else
+	else {
 		graph->state = GRAPH_COMMIT;
+		if (was_root &&
+		    graph->prev_commit_index == graph->commit_index)
+			graph->need_post_root_gap = 1;
+	}
 }
 
 static int graph_is_mapping_correct(struct git_graph *graph)
@@ -814,6 +825,11 @@ static void graph_output_commit_line(struct git_graph *graph, struct strbuf *sb)
 	int seen_this = 0;
 	int i, chars_written;
 
+	if (graph->need_post_root_gap) {
+		graph->need_post_root_gap = 0;
+		strbuf_addch(sb, '\n');
+	}
+
 	/*
 	 * Output the row containing this commit
 	 * Iterate up to and including graph->num_columns,
diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh
index f7181d1..ca53a80 100755
--- a/t/t6016-rev-list-graph-simplify-history.sh
+++ b/t/t6016-rev-list-graph-simplify-history.sh
@@ -264,4 +264,38 @@ test_expect_success '--graph --boundary ^C3' '
 	test_cmp expected actual
 	'
 
+one_independent_branch () {
+	git checkout --orphan root$1 A1 &&
+	test_commit root_$1 &&
+	test_commit then_$1 &&
+	test_commit further_$1
+}
+
+test_expect_success 'multi-root setup' '
+	one_independent_branch 0 &&
+	one_independent_branch 1 &&
+	one_independent_branch 2 &&
+
+	git checkout -b merge210 root2 &&
+	test_tick &&
+	git merge -s ours root1 &&
+	test_tick &&
+	git merge -s ours root0
+'
+
+test_expect_success 'multi-root does not emit unnecessary post-root gap' '
+	git log --oneline --graph >actual &&
+	! grep "^$" actual
+'
+
+test_expect_success 'multi-root does show necessary post-root gap' '
+	git log --oneline --graph root0 root1 root2 >actual &&
+	test $(grep -c "^$" actual) = 2
+'
+
+test_expect_failure 'multi-root does not emit unnecessary post-root gap' '
+	git log --oneline --graph merge210~1...merge210~1^2~2 >actual &&
+	! grep "^$" actual
+'
+
 test_done
-- 
1.8.5.2-297-g3e57c29

Re: [RFH/PATCH] graph: give an extra gap after showing root commit

[Junio C Hamano]

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

>  Note that this still does not work very well for --boundary case
>  (see the last test added to t6016).
> ...
> +test_expect_failure 'multi-root does not emit unnecessary post-root gap' '
> +	git log --oneline --graph merge210~1...merge210~1^2~2 >actual &&
> +	! grep "^$" actual
> +'

Obviously, this needs to be

    git log --oneline --graph --boundary merge210~1...merge210~1^2~2 >actual &&

for it to fail.

Re: [RFH/PATCH] graph: give an extra gap after showing root commit

[Thomas Rast]

Hi Junio,

I briefly looked at d84a3da (jc/graph-post-root-gap) in pu, and have
this nit:

> diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh
> [...]
> +one_independent_branch () {
> +	git checkout --orphan root$1 A1 &&
> +	test_commit root_$1 &&

The naming of root0 etc. makes the test below rather confusing to read,
because test_commit root_0 also creates a tag called root_0.  So you set
up history that has a tag root_0 that points *only* at the root, and a
branch root0 that includes two more commits.

> +test_expect_failure 'multi-root does show necessary post-root gap' '
> +	sed -e "s/ #$/ /" >expect <<-\EOF &&
> +	* further_2
> +	* then_2
> +	* root_2
> +	  * further_1
> +	  * then_1
> +	  * root_1
> +	* further_0
> +	* then_0
> +	* root_0
> +	EOF
> +	git log --graph --format=%s root0 root1 root2 >actual &&
> +	test_cmp expect actual
> +'

-- 
Thomas Rast
tr@thomasrast.ch

[PATCH] graph.c: visual difference on subsequent series

[Milton Soares Filho]

For projects with separate history lines and, thus, multiple root-commits, the
linear arrangement of `git log --graph --oneline` does not allow the user to
spot where the sequence ends, giving the impression that it's a contiguous
history. E.g.

History sequence A: a1 -- a2 -- a3 (root-commit)
History sequence B: b1 -- b2 -- b3 (root-commit)

    git log --graph --oneline
    * a1
    * a2
    * a3
    * b1
    * b2
    * b3

In a GUI tool, the root-commit of each series would stand out on the graph.

This modification changes the commit char to a different symbol ('x'), so users
of the command-line graph tool can easily identify root-commits and make sense
of where each series is limited to.

    git log --graph --oneline
    * a1
    * a2
    x a3
    * b1
    * b2
    x b3

UPDATE: dealing with the mark at get_revision_mark() to address Junio C Hamano
concerns and give it a proper priority:

> It is unclear why the update goes to this function. At the first
> glance, I feel that it would be more sensible to add the equivalent
> code to get_revision_mark()---we do not have to worry about what
> else, other than calling get_revision_mark() and adding it to sb,
> would be skipped by the added "return" when we later have to update
> this function and add more code after the existing strbuf_addstr().
>
> The change implemented your way will lose other information when a
> root commit is at the boundary, marked as uninteresting, or on the
> left/right side of traversal (when --left-right is requested).  I
> think these pieces of information your patch seems to be losing are
> a lot more relevant than "have we hit the root?", especially in the
> majority of repositories where there is only one root commit.

Signed-off-by: Milton Soares Filho <milton.soares.filho@gmail.com>
---
 revision.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 0173e01..ab0447f 100644
--- a/revision.c
+++ b/revision.c
@@ -3066,7 +3066,9 @@ char *get_revision_mark(const struct rev_info *revs, const struct commit *commit
 			return "<";
 		else
 			return ">";
-	} else if (revs->graph)
+	} else if (revs->graph && commit->parents == NULL)
+		return "x"; /* diverges root-commits in subsequent series */
+	else if (revs->graph)
 		return "*";
 	else if (revs->cherry_mark)
 		return "+";
-- 
1.8.1.2

[PATCH] graph.c: visual difference on subsequent series

[Antoine Beaupré]

For projects with separate history lines and, thus, multiple root-commits, the
linear arrangement of `git log --graph --oneline` does not allow the user to
spot where the sequence ends, giving the impression that it's a contiguous
history. E.g.

History sequence A: a1 -- a2 -- a3 (root-commit)
History sequence B: b1 -- b2 -- b3 (root-commit)

    git log --graph --oneline
    * a1
    * a2
    * a3
    * b1
    * b2
    * b3

In a GUI tool, the root-commit of each series would stand out on the graph.

This modification changes the commit char to a different symbol ('o'), so users
of the command-line graph tool can easily identify root-commits and make sense
of where each series is limited to.

    git log --graph --oneline
    * a1
    * a2
    o a3
    * b1
    * b2
    o b3

The 'o' character was chosen because it is the same character used in rev-list
to mark root commits.

This patch is similar than the one provided by Milton Soares Filho in
1382734287.31768.1.git.send.email.milton.soares.filho@gmail.com but was
implemented independently and uses the 'o' character instead of 'x'.

Other solutions were discarded for those reasons:

 * line delimiters: we want to keep one commit per line
 * tree indentation: it makes little sense with commit trees without
   common history, and is more complicated to implement

Signed-off-by: Antoine Beaupré <anarcat@koumbit.org>
---
 revision.c                                 |  8 ++++++--
 t/t4202-log.sh                             | 10 +++++-----
 t/t6016-rev-list-graph-simplify-history.sh | 14 +++++++-------
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/revision.c b/revision.c
index 75dda92..5f21e24 100644
--- a/revision.c
+++ b/revision.c
@@ -3246,8 +3246,12 @@ char *get_revision_mark(const struct rev_info *revs, const struct commit *commit
 			return "<";
 		else
 			return ">";
-	} else if (revs->graph)
-		return "*";
+	} else if (revs->graph) {
+		if (commit->parents)
+			return "*";
+		else
+			return "o";
+	}
 	else if (revs->cherry_mark)
 		return "+";
 	return "";
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 99ab7ca..d11876e 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -244,7 +244,7 @@ cat > expect <<EOF
 * fourth
 * third
 * second
-* initial
+o initial
 EOF
 
 test_expect_success 'simple log --graph' '
@@ -272,7 +272,7 @@ cat > expect <<\EOF
 |/
 * third
 * second
-* initial
+o initial
 EOF
 
 test_expect_success 'log --graph with merge' '
@@ -338,7 +338,7 @@ cat > expect <<\EOF
 |
 |     second
 |
-* commit tags/side-1~3
+o commit tags/side-1~3
   Author: A U Thor <author@example.com>
 
       initial
@@ -410,7 +410,7 @@ cat > expect <<\EOF
 * | third
 |/
 * second
-* initial
+o initial
 EOF
 
 test_expect_success 'log --graph with merge' '
@@ -799,7 +799,7 @@ cat >expect <<\EOF
 | -one
 | +ichi
 |
-* commit COMMIT_OBJECT_NAME
+o commit COMMIT_OBJECT_NAME
   Author: A U Thor <author@example.com>
 
       initial
diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh
index f7181d1..74b6fc3 100755
--- a/t/t6016-rev-list-graph-simplify-history.sh
+++ b/t/t6016-rev-list-graph-simplify-history.sh
@@ -81,7 +81,7 @@ test_expect_success '--graph --all' '
 	echo "|/|   " >> expected &&
 	echo "* | $A2" >> expected &&
 	echo "|/  " >> expected &&
-	echo "* $A1" >> expected &&
+	echo "o $A1" >> expected &&
 	git rev-list --graph --all > actual &&
 	test_cmp expected actual
 	'
@@ -111,7 +111,7 @@ test_expect_success '--graph --simplify-by-decoration' '
 	echo "|/|   " >> expected &&
 	echo "* | $A2" >> expected &&
 	echo "|/  " >> expected &&
-	echo "* $A1" >> expected &&
+	echo "o $A1" >> expected &&
 	git rev-list --graph --all --simplify-by-decoration > actual &&
 	test_cmp expected actual
 	'
@@ -139,7 +139,7 @@ test_expect_success '--graph --simplify-by-decoration prune branch B' '
 	echo "* | $A3" >> expected &&
 	echo "|/  " >> expected &&
 	echo "* $A2" >> expected &&
-	echo "* $A1" >> expected &&
+	echo "o $A1" >> expected &&
 	git rev-list --graph --simplify-by-decoration --all > actual &&
 	test_cmp expected actual
 	'
@@ -156,7 +156,7 @@ test_expect_success '--graph --full-history -- bar.txt' '
 	echo "| |/  " >> expected &&
 	echo "* | $A3" >> expected &&
 	echo "|/  " >> expected &&
-	echo "* $A2" >> expected &&
+	echo "o $A2" >> expected &&
 	git rev-list --graph --full-history --all -- bar.txt > actual &&
 	test_cmp expected actual
 	'
@@ -170,7 +170,7 @@ test_expect_success '--graph --full-history --simplify-merges -- bar.txt' '
 	echo "* | $A5" >> expected &&
 	echo "* | $A3" >> expected &&
 	echo "|/  " >> expected &&
-	echo "* $A2" >> expected &&
+	echo "o $A2" >> expected &&
 	git rev-list --graph --full-history --simplify-merges --all \
 		-- bar.txt > actual &&
 	test_cmp expected actual
@@ -183,7 +183,7 @@ test_expect_success '--graph -- bar.txt' '
 	echo "* $A3" >> expected &&
 	echo "| * $C4" >> expected &&
 	echo "|/  " >> expected &&
-	echo "* $A2" >> expected &&
+	echo "o $A2" >> expected &&
 	git rev-list --graph --all -- bar.txt > actual &&
 	test_cmp expected actual
 	'
@@ -201,7 +201,7 @@ test_expect_success '--graph --sparse -- bar.txt' '
 	echo "| * $C1" >> expected &&
 	echo "|/  " >> expected &&
 	echo "* $A2" >> expected &&
-	echo "* $A1" >> expected &&
+	echo "o $A1" >> expected &&
 	git rev-list --graph --sparse --all -- bar.txt > actual &&
 	test_cmp expected actual
 	'
-- 
2.1.1

Re: [PATCH] graph.c: visual difference on subsequent series

[Antoine Beaupré]

Any reason why this patch wasn't included / reviewed?

Thanks,

A.

On 2014-11-10 08:33:32, Antoine Beaupré wrote:
> For projects with separate history lines and, thus, multiple root-commits, the
> linear arrangement of `git log --graph --oneline` does not allow the user to
> spot where the sequence ends, giving the impression that it's a contiguous
> history. E.g.
>
> History sequence A: a1 -- a2 -- a3 (root-commit)
> History sequence B: b1 -- b2 -- b3 (root-commit)
>
>     git log --graph --oneline
>     * a1
>     * a2
>     * a3
>     * b1
>     * b2
>     * b3
>
> In a GUI tool, the root-commit of each series would stand out on the graph.
>
> This modification changes the commit char to a different symbol ('o'), so users
> of the command-line graph tool can easily identify root-commits and make sense
> of where each series is limited to.
>
>     git log --graph --oneline
>     * a1
>     * a2
>     o a3
>     * b1
>     * b2
>     o b3
>
> The 'o' character was chosen because it is the same character used in rev-list
> to mark root commits.
>
> This patch is similar than the one provided by Milton Soares Filho in
> 1382734287.31768.1.git.send.email.milton.soares.filho@gmail.com but was
> implemented independently and uses the 'o' character instead of 'x'.
>
> Other solutions were discarded for those reasons:
>
>  * line delimiters: we want to keep one commit per line
>  * tree indentation: it makes little sense with commit trees without
>    common history, and is more complicated to implement
>
> Signed-off-by: Antoine Beaupré <anarcat@koumbit.org>
> ---
>  revision.c                                 |  8 ++++++--
>  t/t4202-log.sh                             | 10 +++++-----
>  t/t6016-rev-list-graph-simplify-history.sh | 14 +++++++-------
>  3 files changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index 75dda92..5f21e24 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -3246,8 +3246,12 @@ char *get_revision_mark(const struct rev_info *revs, const struct commit *commit
>  			return "<";
>  		else
>  			return ">";
> -	} else if (revs->graph)
> -		return "*";
> +	} else if (revs->graph) {
> +		if (commit->parents)
> +			return "*";
> +		else
> +			return "o";
> +	}
>  	else if (revs->cherry_mark)
>  		return "+";
>  	return "";
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 99ab7ca..d11876e 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -244,7 +244,7 @@ cat > expect <<EOF
>  * fourth
>  * third
>  * second
> -* initial
> +o initial
>  EOF
>  
>  test_expect_success 'simple log --graph' '
> @@ -272,7 +272,7 @@ cat > expect <<\EOF
>  |/
>  * third
>  * second
> -* initial
> +o initial
>  EOF
>  
>  test_expect_success 'log --graph with merge' '
> @@ -338,7 +338,7 @@ cat > expect <<\EOF
>  |
>  |     second
>  |
> -* commit tags/side-1~3
> +o commit tags/side-1~3
>    Author: A U Thor <author@example.com>
>  
>        initial
> @@ -410,7 +410,7 @@ cat > expect <<\EOF
>  * | third
>  |/
>  * second
> -* initial
> +o initial
>  EOF
>  
>  test_expect_success 'log --graph with merge' '
> @@ -799,7 +799,7 @@ cat >expect <<\EOF
>  | -one
>  | +ichi
>  |
> -* commit COMMIT_OBJECT_NAME
> +o commit COMMIT_OBJECT_NAME
>    Author: A U Thor <author@example.com>
>  
>        initial
> diff --git a/t/t6016-rev-list-graph-simplify-history.sh b/t/t6016-rev-list-graph-simplify-history.sh
> index f7181d1..74b6fc3 100755
> --- a/t/t6016-rev-list-graph-simplify-history.sh
> +++ b/t/t6016-rev-list-graph-simplify-history.sh
> @@ -81,7 +81,7 @@ test_expect_success '--graph --all' '
>  	echo "|/|   " >> expected &&
>  	echo "* | $A2" >> expected &&
>  	echo "|/  " >> expected &&
> -	echo "* $A1" >> expected &&
> +	echo "o $A1" >> expected &&
>  	git rev-list --graph --all > actual &&
>  	test_cmp expected actual
>  	'
> @@ -111,7 +111,7 @@ test_expect_success '--graph --simplify-by-decoration' '
>  	echo "|/|   " >> expected &&
>  	echo "* | $A2" >> expected &&
>  	echo "|/  " >> expected &&
> -	echo "* $A1" >> expected &&
> +	echo "o $A1" >> expected &&
>  	git rev-list --graph --all --simplify-by-decoration > actual &&
>  	test_cmp expected actual
>  	'
> @@ -139,7 +139,7 @@ test_expect_success '--graph --simplify-by-decoration prune branch B' '
>  	echo "* | $A3" >> expected &&
>  	echo "|/  " >> expected &&
>  	echo "* $A2" >> expected &&
> -	echo "* $A1" >> expected &&
> +	echo "o $A1" >> expected &&
>  	git rev-list --graph --simplify-by-decoration --all > actual &&
>  	test_cmp expected actual
>  	'
> @@ -156,7 +156,7 @@ test_expect_success '--graph --full-history -- bar.txt' '
>  	echo "| |/  " >> expected &&
>  	echo "* | $A3" >> expected &&
>  	echo "|/  " >> expected &&
> -	echo "* $A2" >> expected &&
> +	echo "o $A2" >> expected &&
>  	git rev-list --graph --full-history --all -- bar.txt > actual &&
>  	test_cmp expected actual
>  	'
> @@ -170,7 +170,7 @@ test_expect_success '--graph --full-history --simplify-merges -- bar.txt' '
>  	echo "* | $A5" >> expected &&
>  	echo "* | $A3" >> expected &&
>  	echo "|/  " >> expected &&
> -	echo "* $A2" >> expected &&
> +	echo "o $A2" >> expected &&
>  	git rev-list --graph --full-history --simplify-merges --all \
>  		-- bar.txt > actual &&
>  	test_cmp expected actual
> @@ -183,7 +183,7 @@ test_expect_success '--graph -- bar.txt' '
>  	echo "* $A3" >> expected &&
>  	echo "| * $C4" >> expected &&
>  	echo "|/  " >> expected &&
> -	echo "* $A2" >> expected &&
> +	echo "o $A2" >> expected &&
>  	git rev-list --graph --all -- bar.txt > actual &&
>  	test_cmp expected actual
>  	'
> @@ -201,7 +201,7 @@ test_expect_success '--graph --sparse -- bar.txt' '
>  	echo "| * $C1" >> expected &&
>  	echo "|/  " >> expected &&
>  	echo "* $A2" >> expected &&
> -	echo "* $A1" >> expected &&
> +	echo "o $A1" >> expected &&
>  	git rev-list --graph --sparse --all -- bar.txt > actual &&
>  	test_cmp expected actual
>  	'
> -- 
> 2.1.1
>

-- 
Pour marcher au pas d'une musique militaire, il n'y a pas besoin de
cerveau, une moelle épinière suffit.
                        - Albert Enstein

Re: [PATCH] graph.c: visual difference on subsequent series

[Junio C Hamano]

Antoine Beaupré <anarcat@koumbit.org> writes:

> Any reason why this patch wasn't included / reviewed?
> ...
>> This patch is similar than the one provided by Milton Soares Filho in
>> 1382734287.31768.1.git.send.email.milton.soares.filho@gmail.com but was
>> implemented independently and uses the 'o' character instead of 'x'.

The reason why Milton's patch was not taken after discussion [*1*]
was not because its implementation was poor, but its design was not
good.  By overriding '*' '<' or '>' with x, it made it impossible to
distinguish a root on the left side branch and a root on the right
side branch.

Is the design of your independent implementation the same except
that 'o' is used instead of 'x'?  Independent implementation does
not make the same design magically better, if that is the case ;-)

If the design is different, please explain how your patch solves the
issue with Milton's design in your log message.

For example, you could use the column arrangement to solve it, e.g.

History sequence A: a1 -- a2 -- a3 (root-commit)
History sequence B: b1 -- b2 -- b3 (root-commit)

    $ git log --graph --oneline A B
    * a1
    * a2
    * a3
      * b1
      * b2
      * b3

    $ git log --graph --oneline --left-right A...B
    < a1
    < a2
    < a3
      > b1
      > b2
      > b3

I am not saying that the above would be the only way to do so; there
may be other ways to solve the issue.

[Reference]

*1* http://thread.gmane.org/gmane.comp.version-control.git/236708/focus=236843

Re: [PATCH] graph.c: visual difference on subsequent series

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 27.07.2015 22:17:
> Antoine Beaupré <anarcat@koumbit.org> writes:
> 
>> Any reason why this patch wasn't included / reviewed?
>> ...
>>> This patch is similar than the one provided by Milton Soares Filho in
>>> 1382734287.31768.1.git.send.email.milton.soares.filho@gmail.com but was
>>> implemented independently and uses the 'o' character instead of 'x'.
> 
> The reason why Milton's patch was not taken after discussion [*1*]
> was not because its implementation was poor, but its design was not
> good.  By overriding '*' '<' or '>' with x, it made it impossible to
> distinguish a root on the left side branch and a root on the right
> side branch.
> 
> Is the design of your independent implementation the same except
> that 'o' is used instead of 'x'?  Independent implementation does
> not make the same design magically better, if that is the case ;-)

Interestingly, the patch to the tests lists * to o changes only, no < or
> to o.

The reason is simply that the patch doesn't change anything for left nor
right commits. I would say that is the best compromise since it does not
change the overall layout, provides more information by default and does
not override information that is requested specifically.

If we want to put more information into log --graph simultaneously we
should really go beyond ASCII and look at how tig does it, e.g. using
unicode characters.

> If the design is different, please explain how your patch solves the
> issue with Milton's design in your log message.
> 
> For example, you could use the column arrangement to solve it, e.g.
> 
> History sequence A: a1 -- a2 -- a3 (root-commit)
> History sequence B: b1 -- b2 -- b3 (root-commit)
> 
>     $ git log --graph --oneline A B
>     * a1
>     * a2
>     * a3
>       * b1
>       * b2
>       * b3
> 
>     $ git log --graph --oneline --left-right A...B
>     < a1
>     < a2
>     < a3
>       > b1
>       > b2
>       > b3
> 
> I am not saying that the above would be the only way to do so; there
> may be other ways to solve the issue.
> 
> [Reference]
> 
> *1* http://thread.gmane.org/gmane.comp.version-control.git/236708/focus=236843
> 

Re: [PATCH] graph.c: visual difference on subsequent series

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>> Is the design of your independent implementation the same except
>> that 'o' is used instead of 'x'?  Independent implementation does
>> not make the same design magically better, if that is the case ;-)
>
> Interestingly, the patch to the tests lists * to o changes only, no < or
>> to o.

Well, in that case, then the opposite but an equivalent problem
exists in the design, no?  It promises to make roots stand out by
painting them as 'o', but it sometimes fails to do so.  In other
words, ...

> The reason is simply that the patch doesn't change anything for left nor
> right commits. I would say that is the best compromise since it does not
> change the overall layout, provides more information by default and does
> not override information that is requested specifically.

... it fails your last criteria.

> If we want to put more information into log --graph simultaneously we
> should really go beyond ASCII and look at how tig does it, e.g. using
> unicode characters.

That's another way to do so, but shifting columns to show where the
history is not connected also does not change the overall layout,
provides more information by default, etc., and a big plus is that
it would be an approach to do so without having to go beyond ASCII.

Re: [PATCH] graph.c: visual difference on subsequent series

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 03.09.2015 19:13:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>>> Is the design of your independent implementation the same except
>>> that 'o' is used instead of 'x'?  Independent implementation does
>>> not make the same design magically better, if that is the case ;-)
>>
>> Interestingly, the patch to the tests lists * to o changes only, no < or
>>> to o.
> 
> Well, in that case, then the opposite but an equivalent problem
> exists in the design, no?  It promises to make roots stand out by
> painting them as 'o', but it sometimes fails to do so.  In other
> words, ...
> 
>> The reason is simply that the patch doesn't change anything for left nor
>> right commits. I would say that is the best compromise since it does not
>> change the overall layout, provides more information by default and does
>> not override information that is requested specifically.
> 
> ... it fails your last criteria.

How would it? "--left-right" information is requested specifically and
not overridden. Root information is not requested specifically [by the
user].

>> If we want to put more information into log --graph simultaneously we
>> should really go beyond ASCII and look at how tig does it, e.g. using
>> unicode characters.
> 
> That's another way to do so, but shifting columns to show where the
> history is not connected also does not change the overall layout,
> provides more information by default, etc., and a big plus is that
> it would be an approach to do so without having to go beyond ASCII.

That would consume more horizontal space and annoy at least some people.
Alternatively, we could use more vertical space and annoy at least some
(other?) people.

In fact, I tend to think that horizontal space is more "precious" than
vertical space due to the common orientation of scrolling...

If we don't mind an increase in vertical spacing there are more issues
that could be solved, for example the merge point description sticking
visually to the commits above rather than the merged branch in the
typical old..new scenario.

In fact, both issues would be solved be adding an extra line after a
root commit (be it "real" root or boundary-wise).

Michael

Re: [PATCH] graph.c: visual difference on subsequent series

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> How would it? "--left-right" information is requested specifically and
> not overridden. Root information is not requested specifically [by the
> user].

If this "highlight root prominently" were a config, then using both
config and --left-right would mean one of them needs to give.  If
this were always on, then the act of the user running "git log"
alone is a sign that the user explicitly asked the log to be shown
with the new world order, i.e. the root is promised to be shown
visible.  Either way, the user is not getting what s/he asked.

>>> If we want to put more information into log --graph simultaneously we
>>> should really go beyond ASCII and look at how tig does it, e.g. using
>>> unicode characters.
>> 
>> That's another way to do so, but shifting columns to show where the
>> history is not connected also does not change the overall layout,
>> provides more information by default, etc., and a big plus is that
>> it would be an approach to do so without having to go beyond ASCII.
>
> That would consume more horizontal space and annoy at least some people.

I sense that we are working from different perceptions of what
"shifting columns" should look like.

A history that reaches two roots would be shown like this, with or
without any special treatment for root:

 * tip
 |\
 | * tip of the side branch
 * | tip of the trunk
 * | second of the trunk
 * | root of the trunk
   * second of the side branch
   * root of the side branch

so it does not give us any more "wasted space" issue with or without
"showing root more prominently".

The case where we would see differences is to have two or more
totally disjoint histories.  But "shifting columns" does not have to
draw that case like this:

 * tip of history A
 | * tip of history B
 * | second of history A
 * | root of history A
   * second of history B
   * third of history B
   * fourth of history B
   * fifth of history B
   * root of history B

It can do this instead to save horizontal space (which I agree with
you is the more precious one than the vertical one):

 * tip of history A
 | * tip of history B
 * | second of history A
 * | root of history A
   * second of history B
  /
 * third of history B
 * fourth of history B
 * fifth of history B
 * root of history B

It does spend more space around the root of each history (in this
case, history A) when it shifts the column for history B to the
space now vacated by history A in order to save horizontal space.
But drawing the graph around the root differently from other parts
is exactly to show roots more prominently; it draws the users' eyes.

Here is another example of drawing the same history.  If the
traversal is topologica:, "shifting columns" does not have to draw
this:

 * tip of history A
 * second of history A
 * root of history A
   * tip of history B
   * second of history B
   * third of history B
   * fourth of history B
   * fifth of history B
   * root of history B

It can do this instead:

 * tip of history A
 * second of history A
 * root of history A
   * tip of history B
  /
 * second of history B
 * third of history B
 * fourth of history B
 * fifth of history B
 * root of history B

Again I am not saying that "shifting columns" is the only way we can
do this, and there may be other ways to do this without losing
information.  Going beyond ASCII as you hinted would be one example.