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

[Junio C Hamano <gitster@pobox.com>]

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.