Re: [PATCH] merge-recursive: fix parsing of "diff-algorithm" option
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Sep 26, 2013, 20:47 UTC
- Message-ID
- <20130926204720.GH9464@google.com>
- In-Reply-To
- <689bf88b6f1d33e123cc786042cc6dba23464351.1380225743.git.john@keeping.me.uk>
John Keeping wrote:
Show 11 quoted lines
> The "diff-algorithm" option to the recursive merge strategy takes the > name of the algorithm as an option, but it uses strcmp on the option > string to check if it starts with "diff-algorithm=", meaning that this > options cannot actually be used. > > Fix this by switching to prefixcmp. At the same time, clarify the > following line by using strlen instead of a hard-coded length, which > also makes it consistent with nearby code. > > Reported-by: Luke Noel-Storr <luke.noel-storr@integrate.co.uk> > Signed-off-by: John Keeping <john@keeping.me.uk>
Thanks, both.
[...]
Show 12 quoted lines
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2069,8 +2069,8 @@ int parse_merge_opt(struct merge_options *o, const char *s)
> o->xdl_opts = DIFF_WITH_ALG(o, PATIENCE_DIFF);
> else if (!strcmp(s, "histogram"))
> o->xdl_opts = DIFF_WITH_ALG(o, HISTOGRAM_DIFF);
> - else if (!strcmp(s, "diff-algorithm=")) {
> - long value = parse_algorithm_value(s+15);
> + else if (!prefixcmp(s, "diff-algorithm=")) {
> + long value = parse_algorithm_value(s + strlen("diff-algorithm="));
> if (value < 0)
> return -1;While we're here:
Part of the problem is that there are no tests for this option (or for 'git diff --diff-algorithm', or for the '[diff] algorithm' configuration), so we didn't notice it didn't work until someone actually tried it.
Do you have any examples of a diff or merge that works better with some particular diff algorithm, that could be used in tests?
Ciao, Jonathan