Kyle Marek <kmarek@xxxxxxxx> writes:
>> So the condition we saw in your patches, !commit->parents, which
>> attempted to see if it was root, needs to be replaced with a helper
>> function that checks if there is any parent that is shown in the
>> output.
>> ...
>> Hmm?
>
> Okay, I see what you mean. Fixing --graph to avoid implying ancestry
> sounds like a better approach to me.
Sorry, I do not know how you drew that conclusion from my
description.
All I meant to convey is "roots are not special at all, commits that
do not have parents in the parts of the history shown are, and care
must be taken to ensure that they do not appear to have parents".
And the argument applies equally to either of two approaches.
Whether the solution chosen is
(1) to use special set of markers "{#}" for commits that do not
have parents in the displayed part of the history instead of
the usual "<*>", or
(2) to stick to the normal set of markers "<*>" but shift the graph
to avoid false ancestry.
we shouldn't be special casing "root commits" just because they are
roots. Exactly the same issue exists for non-root commits whose
parents are not shown in the output, if commits from unrelated
ancestry is drawn directly below them.
> That being said, I spoke to Jason recently, and he expressed interest
> in optionally marking root commits so they are easy to search for in a
> graph with something like /# in `less`. I see value in this,
I do not mind to denote the "this commit may appear directly on top
of another commit, but there is no ancestry" situation with a
special set of markers that is different from the usual "<*>" (for
left, normal and right) set. I agree pagers are good ways to /search
things in the output.
> So would you be open to my modifying of the patch in question (patch
> 1+2 squashed, I guess) to instead use "--mark-roots=<mark>" to
> optionally mark root commits with a string <mark>, and pursue fixing
> the --graph rendering issue in another series?
I do not mind if the graph rendering fix does not happen yet again;
IIRC the past contributors couldn't implement it, either.
I think this new feature should be made opt-in by introducing a new
option (without giving it a configuration variable), with explicit
"--no-<option>" supported to countermand a "--<option>=#" that may
appear earlier on the command line (or prepare your scripts for
later introduction of such a configuration variable).
I do find it troubling if the <option> has "root" in its name, and I
would find it even more troubling if the feature somehow treated
root commits specially but not other commits that do not have their
parents shown. It was the primary point I wanted to stress in the
previous two message [*1*].
I am hoping that a single option can give three-tuple that replaces
the usual "<*>", with perhaps the default of "{#}" or something.
I however offhand do not think of a way to make "left root" appear
in the output, but because we'd need "right root" that looks
different from ">" anyway, it may make sense to allow specifying
"left root" just for symmetry.
[Footnote]
*1* But if we do not think of a good option name without the word
"root" in it, I might be talked into a name with "root", as long
as we clearly describe (1) that commits that has parents that
are not shown in the history are also shown with these letters,
and (2) that new contributors are welcome to try coming up with
a new name for the option to explain the behaviour better, but
are not welcome to argue that the option should special case
root commits only because the option is named with "root" in it.
