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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2022, 18:25 UTC
Message-ID: <xmqq4k5799sf.fsf@gitster.g>
In-Reply-To: <20220209162350.169971-2-alexhenrie24@gmail.com>

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?

> +		rev->topo_order = 1;
> +		rev->rewrite_parents = 1;
> +		rev->graph = graph_init(rev);

In any case, it probably makes sense to encapsulate these three lines in a small helper function "when --graph is asked for on a rev, call this function" in the PREVIOUS step of the series, and change this patch to read more like

	if (default_graph &&
	    /* check for incompatible options */
	    !rev->track_linear && !rev->reverse && ...)
		rev_add_graph_option(rev);

Most importantly, the helper should be one that handles "wow, we see that rev->graph is already populated, what should we do?", and not this caller. And that helper can be called unconditionally by the command line parser when it finds !strcmp(arg, "--graph") yields true.

Thanks.