Re: [PATCH v2 3/3] diff: Introduce --diff-algorithm command line option

[Michal Privoznik <mprivozn@redhat.com>]

On 14.01.2013 22:06, Junio C Hamano wrote:
> Michal Privoznik <mprivozn@redhat.com> writes:
> 
>> +--diff-algorithm={patience|minimal|histogram|myers}::
>> +	Choose a diff algorithm. The variants are as follows:
>> ++
>> +--
>> +`myers`;;
>> +	The basic greedy diff algorithm.
>> +`minimal`;;
>> +	Spend extra time to make sure the smallest possible diff is
>> +	produced.
>> +`patience`;;
>> +	Use "patience diff" algorithm when generating patches.
>> +`histogram`;;
>> +	This algorithm extends the patience algorithm to "support
>> +	low-occurrence common elements".
>> +--
>> ++
>> +For instance, if you configured diff.algorithm variable to a
>> +non-default value and want to use the default one, then you
>> +have to use `--diff-algorithm=myers` option.
>> +
>> +You should prefer this option over the `--minimal`, `--patience` and
>> +`--histogram` which are kept just for backwards compatibility.
> 
> Much better; I'd drop the last paragraph, though.
> 
> I also think we really should consider "default" synonym for
> whichever happens to be the built-in default (currently myers).
> 
>> diff --git a/diff.c b/diff.c
>> index e9a7e4d..3e021d5 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -144,7 +144,7 @@ static int git_config_rename(const char *var, const char *value)
>>  	return git_config_bool(var,value) ? DIFF_DETECT_RENAME : 0;
>>  }
>>  
>> -static long parse_algorithm_value(const char *value)
>> +long parse_algorithm_value(const char *value)
>>  {
>>  	if (!value || !strcasecmp(value, "myers"))
>>  		return 0;
>> @@ -3633,6 +3633,16 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>>  		options->xdl_opts = DIFF_WITH_ALG(options, PATIENCE_DIFF);
>>  	else if (!strcmp(arg, "--histogram"))
>>  		options->xdl_opts = DIFF_WITH_ALG(options, HISTOGRAM_DIFF);
>> +	else if (!prefixcmp(arg, "--diff-algorithm=")) {
>> +		long value = parse_algorithm_value(arg+17);
>> +		if (value < 0)
>> +			return error("option diff-algorithm accepts \"myers\", "
>> +				     "\"minimal\", \"patience\" and \"histogram\"");
>> +		/* clear out previous settings */
>> +		DIFF_XDL_CLR(options, NEED_MINIMAL);
>> +		options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;
>> +		options->xdl_opts |= value;
> 
> This makes me wonder if other places that use DIFF_WITH_ALG() also
> need to worry about clearing NEED_MINIMAL?
> 

Not really. In my approach, --minimal looks at yet another algorithm.
However, current code sees it as orthogonal to the myers algorithm. The
flag doesn't have any effect on --patience or --histogram at all.

So I think we need to clear the flag only when using --diff-algorithm.

Michal