Re: [PATCH 2/3] log: add a config option for --graph

From: Alex Henrie <alexhenrie24@gmail.com>
Date: Feb 10, 2022, 16:49 UTC
Message-ID: <CAMMLpeS9MrHdq4bNwDYUJ0ctAafsjLskXStcsO_vBnfngwor-A@mail.gmail.com>
In-Reply-To: <xmqq4k5799sf.fsf@gitster.g>

On Wed, Feb 9, 2022 at 11:25 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 28 quoted lines

>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > +     if (default_graph &&
> > +         !rev->graph &&
>
> This part I can see why we want to check ;-)
>
> > +         !rev->track_linear &&
> > +         !rev->reverse &&
> > +         !rev->reflog_info &&
> > +         !rev->no_walk) {
>
> But this makes me wonder how we plan to keep this list of "if the
> user asked for any of these options, we shouldn't turn graph on by
> default" up-to-date.  The scheme looks brittle.
>
> Also, what would happen when a developer wants to add, say
> log.reverse, configuration variable in the future?  I can see the
> block to do the equivalent of this for .log.reverse begins with
>
>         if (default_reverse &&
>             !rev->reverse &&
>
> but I am not sure what other conditions need to be checked,
> especially with 'graph'---should it check for !rev->graph or
> default_graph of both?  Are we playing with a potential
> combinatorial explosion?

In principle there's no reason why --graph and --reverse can't be compatible. However, if both log.graph and log.reverse config options are added while they are still incompatible, I think using them together should trigger the existing option compatibility error in the setup_revisions function:

        if (revs->reverse && revs->graph)
                die(_("options '%s' and '%s' cannot be used
together"), "--reverse", "--graph");

That said, we should make sure that --graph on the command line overrides log.reverse in the config file (instead of dying), and the same for --reverse overriding log.graph. How about this:

1. In git_log_config, initialize default_graph and default_reverse by
parsing log.graph and log.reverse.

2. In cmd_log_init_defaults, initialize rev->graph_default to
default_graph and rev->reverse_default to default_reverse.

3. In handle_revision_opt, set revs->graph if --graph is given, clear
revs->graph if --no-graph is given, and in either case clear
revs->graph_default. Set revs->reverse and revs->reverse_default
likewise.

4. In setup_revisions, if revs->graph_default is still true, set
revs->graph based on revs->reverse and revs->reverse_default. Set
revs->reverse likewise.

5. In setup_revisions, call a new revision_opts_finish function that
sets revs->topo_order and revs->rewrite_parents if necessary based on
the final value of revs->graph.

I think that would ensure that command line options interact correctly with config options, plus it would allow adding a --no-graph command line option, and the logic to handle defaults would be clearly linked to the existing incompatible option checks. I realize that it will take a bit of refactoring though. I'll send new patches shortly to make it more clear.

Please let me know if any of your other feedback is still relevant in v2.

-Alex