git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 14, 2013, 18:44 UTC
Message-ID
<7vsj63bo4n.fsf@alter.siamese.dyndns.org>
In-Reply-To
<6c058f13c6bb83985e8651a8dde99e7b17879d4e.1358006339.git.mprivozn@redhat.com>
Michal Privoznik <mprivozn@redhat.com> writes:
> Since command line options have higher priority than config file
> variables and taking previous commit into account, we need a way
> how to specify myers algorithm on command line.
Yes.  We cannot stop at [2/3] without this patch.
> However,
> inventing `--myers` is not the right answer. We need far more
> general option, and that is `--diff-algorithm`.

Yes, --diff-algorithm=default would let people defeat a configured algo without knowing how exactly to spell myers.

> The older options
> (`--minimal`, `--patience` and `--histogram`) are kept for
> backward compatibility.
That is phrasing it too strongly to be acceptable.

People who do not care any configuration can keep using --histogram without having to retrain their fingers to type a much longer form you introduced (i.e. --diff-algorithm=histogram). It is and will always be just as valid a way to ask for --histogram as the new longhand is.

Show 20 quoted lines
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  Documentation/diff-options.txt         | 22 ++++++++++++++++++++++
>  contrib/completion/git-completion.bash | 11 +++++++++++
>  diff.c                                 | 12 +++++++++++-
>  diff.h                                 |  2 ++
>  merge-recursive.c                      |  6 ++++++
>  5 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index 39f2c50..4091f52 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -45,6 +45,9 @@ ifndef::git-format-patch[]
>  	Synonym for `-p --raw`.
>  endif::git-format-patch[]
>  
> +The next three options are kept for compatibility reason. You should use the
> +`--diff-algorithm` option instead.
> +
Drop this.
Show 22 quoted lines
> @@ -55,6 +58,25 @@ endif::git-format-patch[]
>  --histogram::
>  	Generate a diff using the "histogram diff" algorithm.
>  
> +--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".
> +--
> ++
> +You should prefer this option over the `--minimal`, `--patience` and
> +`--histogram` which are kept just for backwards compatibility.
Drop the last one, and replace it with something like:
	If you configured diff.algorithm to a non-default value and
	want to use the default one, you have to use this form and
	choose myers, i.e. --diff-algorithm=myers, as we do not have
	a short-and-sweet --myers option.
(but the above is a bit too verbose, so please shorten it appropriately).
Show 99 quoted lines
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 33e25dc..d592cf9 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1021,6 +1021,8 @@ _git_describe ()
>  	__gitcomp_nl "$(__git_refs)"
>  }
>  
> +__git_diff_algorithms="myers minimal patience histogram"
> +
>  __git_diff_common_options="--stat --numstat --shortstat --summary
>  			--patch-with-stat --name-only --name-status --color
>  			--no-color --color-words --no-renames --check
> @@ -1035,6 +1037,7 @@ __git_diff_common_options="--stat --numstat --shortstat --summary
>  			--raw
>  			--dirstat --dirstat= --dirstat-by-file
>  			--dirstat-by-file= --cumulative
> +			--diff-algorithm=
>  "
>  
>  _git_diff ()
> @@ -1042,6 +1045,10 @@ _git_diff ()
>  	__git_has_doubledash && return
>  
>  	case "$cur" in
> +	--diff-algorithm=*)
> +		__gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
> +		return
> +		;;
>  	--*)
>  		__gitcomp "--cached --staged --pickaxe-all --pickaxe-regex
>  			--base --ours --theirs --no-index
> @@ -2114,6 +2121,10 @@ _git_show ()
>  			" "" "${cur#*=}"
>  		return
>  		;;
> +	--diff-algorithm=*)
> +		__gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
> +		return
> +		;;
>  	--*)
>  		__gitcomp "--pretty= --format= --abbrev-commit --oneline
>  			$__git_diff_common_options
> diff --git a/diff.c b/diff.c
> index ddae5c4..6418076 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;
> +	}
>  
>  	/* flags options */
>  	else if (!strcmp(arg, "--binary")) {
> diff --git a/diff.h b/diff.h
> index a47bae4..54c2590 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -333,6 +333,8 @@ extern struct userdiff_driver *get_textconv(struct diff_filespec *one);
>  
>  extern int parse_rename_score(const char **cp_p);
>  
> +extern long parse_algorithm_value(const char *value);
> +
>  extern int print_stat_summary(FILE *fp, int files,
>  			      int insertions, int deletions);
>  extern void setup_diff_pager(struct diff_options *);
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d882060..53d8feb 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -2068,6 +2068,12 @@ 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);
> +		if (value < 0)
> +			return -1;
> +		o->xdl_opts |= value;

Isn't this new hunk wrong? DIFF_WITH_ALG() removes the previously chosen algorithm choice before OR'ing the new one in, so that

	diff --histogram --patience

would not end up with a value PATIENCE|HISTOGRAM OR'ed together in the algo field.

Previous: Michal PrivoznikNext: Junio C Hamano
Message 6 of 7 in “Rework git-diff algorithm selection”
  1. 0/3 Rework git-diff algorithm selectionMichal Privoznik, Jan 12, 2013
  2. 1/3 git-completion.bash: Autocomplete --minimal and --histogram for git-diffMichal Privoznik, Jan 12, 2013
  3. 2/3 config: Introduce diff.algorithm variableMichal Privoznik, Jan 12, 2013
  4. Junio C HamanoJan 14, 2013
  5. 3/3 diff: Introduce --diff-algorithm command line optionMichal Privoznik, Jan 12, 2013
  6. Junio C HamanoJan 14, 2013
  7. Junio C HamanoJan 14, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.