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

[Junio C Hamano <gitster@pobox.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.