Michal Privoznik | 14 Jan 20:58 2013
Picon

[PATCH v2 0/3] Rework git-diff algorithm selection

It's been a while I was trying to get this in. Recently, I realized how
important this is.

Please keep me CC'ed as I am not subscribed to the list.

diff to v1:
-Junio's suggestions worked in

Michal Privoznik (3):
  git-completion.bash: Autocomplete --minimal and --histogram for
    git-diff
  config: Introduce diff.algorithm variable
  diff: Introduce --diff-algorithm command line option

 Documentation/diff-config.txt          | 17 +++++++++++++++++
 Documentation/diff-options.txt         | 23 +++++++++++++++++++++++
 contrib/completion/git-completion.bash | 14 +++++++++++++-
 diff.c                                 | 33 +++++++++++++++++++++++++++++++++
 diff.h                                 |  2 ++
 merge-recursive.c                      |  9 +++++++++
 6 files changed, 97 insertions(+), 1 deletion(-)

--

-- 
1.8.0.2

Michal Privoznik | 14 Jan 20:58 2013
Picon

[PATCH v2 2/3] config: Introduce diff.algorithm variable

Some users or projects prefer different algorithms over others, e.g.
patience over myers or similar. However, specifying appropriate
argument every time diff is to be used is impractical. Moreover,
creating an alias doesn't play nicely with other tools based on diff
(git-show for instance). Hence, a configuration variable which is able
to set specific algorithm is needed. For now, these four values are
accepted: 'myers' (which has the same effect as not setting the config
variable at all), 'minimal', 'patience' and 'histogram'.

Signed-off-by: Michal Privoznik <mprivozn <at> redhat.com>
---
 Documentation/diff-config.txt          | 17 +++++++++++++++++
 contrib/completion/git-completion.bash |  1 +
 diff.c                                 | 23 +++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 4314ad0..be31276 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
 <at>  <at>  -155,3 +155,20  <at>  <at>  diff.tool::
 	"kompare".  Any other value is treated as a custom diff tool,
 	and there must be a corresponding `difftool.<tool>.cmd`
 	option.
+
+diff.algorithm::
+	Choose a diff algorithm.  The variants are as follows:
++
+--
+`myers`;;
(Continue reading)

Junio C Hamano | 14 Jan 22:05 2013
Picon
Picon

Re: [PATCH v2 2/3] config: Introduce diff.algorithm variable

Michal Privoznik <mprivozn <at> redhat.com> writes:

> +static long parse_algorithm_value(const char *value)
> +{
> +	if (!value || !strcasecmp(value, "myers"))
> +		return 0;

[diff]
	algorithm

should probably error out.  Also it is rather unusual to parse the
keyword values case insensitively.

> +	else if (!strcasecmp(value, "minimal"))
> +		return XDF_NEED_MINIMAL;
> +	else if (!strcasecmp(value, "patience"))
> +		return XDF_PATIENCE_DIFF;
> +	else if (!strcasecmp(value, "histogram"))
> +		return XDF_HISTOGRAM_DIFF;
> +	else
> +		return -1;
> +}
> +
>  /*
>   * These are to give UI layer defaults.
>   * The core-level commands such as git-diff-files should
>  <at>  <at>  -196,6 +211,13  <at>  <at>  int git_diff_ui_config(const char *var, const char *value, void *cb)
>  		return 0;
>  	}
>  
(Continue reading)

Jeff King | 15 Jan 17:58 2013
Picon

Re: [PATCH v2 2/3] config: Introduce diff.algorithm variable

On Mon, Jan 14, 2013 at 01:05:27PM -0800, Junio C Hamano wrote:

> Michal Privoznik <mprivozn <at> redhat.com> writes:
> 
> > +static long parse_algorithm_value(const char *value)
> > +{
> > +	if (!value || !strcasecmp(value, "myers"))
> > +		return 0;
> 
> [diff]
> 	algorithm
> 
> should probably error out.

Definitely.

> Also it is rather unusual to parse the keyword values case insensitively.

Is it? "git grep strcasecmp" shows that we already do so in many cases
(e.g., any bool option, core.autocrlf, receive.deny*, etc). Is there a
reason to reject "Myers" here?

-Peff
Junio C Hamano | 15 Jan 18:09 2013
Picon
Picon

Re: [PATCH v2 2/3] config: Introduce diff.algorithm variable

Jeff King <peff <at> peff.net> writes:

>> Also it is rather unusual to parse the keyword values case insensitively.
>
> Is it? "git grep strcasecmp" shows that we already do so in many cases
> (e.g., any bool option, core.autocrlf, receive.deny*, etc).

Yeah, I did the same grep after I wrote the message.  Thanks for
saving me the trouble of reporting the findings ;-)
Michal Privoznik | 14 Jan 20:58 2013
Picon

[PATCH v2 1/3] git-completion.bash: Autocomplete --minimal and --histogram for git-diff

Even though --patience was already there, we missed --minimal and
--histogram for some reason.

Signed-off-by: Michal Privoznik <mprivozn <at> redhat.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index a4c48e1..383518c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
 <at>  <at>  -1031,7 +1031,7  <at>  <at>  __git_diff_common_options="--stat --numstat --shortstat --summary
 			--no-ext-diff
 			--no-prefix --src-prefix= --dst-prefix=
 			--inter-hunk-context=
-			--patience
+			--patience --histogram --minimal
 			--raw
 			--dirstat --dirstat= --dirstat-by-file
 			--dirstat-by-file= --cumulative
--

-- 
1.8.0.2

Michal Privoznik | 14 Jan 20:58 2013
Picon

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

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. However,
inventing `--myers` is not the right answer. We need far more
general option, and that is `--diff-algorithm`.

Signed-off-by: Michal Privoznik <mprivozn <at> redhat.com>
---
 Documentation/diff-options.txt         | 23 +++++++++++++++++++++++
 contrib/completion/git-completion.bash | 11 +++++++++++
 diff.c                                 | 12 +++++++++++-
 diff.h                                 |  2 ++
 merge-recursive.c                      |  9 +++++++++
 5 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 39f2c50..01b97b3 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
 <at>  <at>  -55,6 +55,29  <at>  <at>  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`;;
(Continue reading)

Junio C Hamano | 14 Jan 22:06 2013
Picon
Picon

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

Michal Privoznik <mprivozn <at> 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).

(Continue reading)

Michal Privoznik | 15 Jan 11:07 2013
Picon

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

On 14.01.2013 22:06, Junio C Hamano wrote:
> Michal Privoznik <mprivozn <at> 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).
(Continue reading)


Gmane