Re: [PATCH 1/2] revision: Denote root commits with '#'

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 19, 2021, 22:10 UTC
Message-ID: <xmqq1reginnq.fsf@gitster.c.googlers.com>
In-Reply-To: <237aeef3-239f-bff4-fa17-5581092c8f51@pdinc.us>

Kyle Marek <kmarek@pdinc.us> writes:

Show 9 quoted lines

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