Re: [PATCH] config: Introduce --patience config variable

From: Jeff King <peff@peff.net>
Date: Mar 6, 2012, 11:49 UTC
Message-ID: <20120306114914.GB6733@sigill.intra.peff.net>
In-Reply-To: <a87ed689ddfb06601dd639541199fc72d829bdaf.1331031473.git.mprivozn@redhat.com>

On Tue, Mar 06, 2012 at 11:59:42AM +0100, Michal Privoznik wrote:

Show 5 quoted lines

> Some users like the patience diff more than the bare diff. However,
> specifying the '--patience' argument every time diff is to be used
> is impractical. Moreover, creating an alias doesn't play with other
> tools nice, e.g. git-show; Therefore we need a configuration variable
> to turn this on/off across whole git tools.

The idea of turning on patience diff via config makes sense to me, and it is not a problem for plumbing tools to have patience diff on, since patience diffs are syntactically identical to non-patience diffs. So I think the intent is good.

A few comments on the patch itself:

Show 9 quoted lines

> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -86,6 +86,9 @@ diff.mnemonicprefix::
>  diff.noprefix::
>  	If set, 'git diff' does not show any source or destination prefix.
>  
> +diff.patience:
> +    If set, 'git diff' will use patience algorithm.
> +

Should this be a boolean? Or should we actually have a diff.algorithm option where you specify the algorithm you want (e.g., "diff.algorithm = patience")? That would free us up later to more easily add new values.

In particular, I am thinking about --minimal. It is mutually exclusive with --patience, and is simply ignored if you use patience diff. we perhaps have "diff.algorithm" which can be one of "myers", "minimal" (which is really myers + the minimal flag), and "patience".

Or I suppose you could think of it as "--minimal" being orthogonal to the algorithm chosen, and it is simply that "--minimal" does nothing (currently) with the patience algorithm.

Show 8 quoted lines

> @@ -3202,6 +3208,9 @@ int diff_setup_done(struct diff_options *options)
>  		DIFF_OPT_SET(options, EXIT_WITH_STATUS);
>  	}
>  
> +    if (diff_patience)
> +		DIFF_XDL_SET(options, PATIENCE_DIFF);
> +
>  	return 0;

Hmm. Usually for a boolean config we would have "-1" mean "not specified", and otherwise 0/1 for true/false. But in your case, setting diff.patience to "false" will be the same as not setting it at all. I think this is probably OK. "false" is the default, so you would only want to specify it to override an earlier setting, but there is nothing earlier than config that you could possibly be overriding.

Speaking of overriding, you may want to actually override the config option from the command line. You probably should also add a "--no-patience" option, so that one can turn off "diff.patience = true" for a particular invocation.

-Peff