Alex Henrie <alexhenrie24@xxxxxxxxx> 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.
