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

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

 Michal Privoznik mpriv...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-01-15 Thread Junio C Hamano
Jeff King p...@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 ;-)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-01-14 Thread Michal Privoznik
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 mpriv...@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
@@ -155,3 +155,20 @@ 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`;;
+   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.
+--
++
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 383518c..33e25dc 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1839,6 +1839,7 @@ _git_config ()
diff.suppressBlankEmpty
diff.tool
diff.wordRegex
+   diff.algorithm
difftool.
difftool.prompt
fetch.recurseSubmodules
diff --git a/diff.c b/diff.c
index 732d4c2..e9a7e4d 100644
--- a/diff.c
+++ b/diff.c
@@ -36,6 +36,7 @@ static int diff_no_prefix;
 static int diff_stat_graph_width;
 static int diff_dirstat_permille_default = 30;
 static struct diff_options default_diff_options;
+static long diff_algorithm;
 
 static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_RESET,
@@ -143,6 +144,20 @@ 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)
+{
+   if (!value || !strcasecmp(value, myers))
+   return 0;
+   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
@@ -196,6 +211,13 @@ int git_diff_ui_config(const char *var, const char *value, 
void *cb)
return 0;
}
 
+   if (!strcmp(var, diff.algorithm)) {
+   diff_algorithm = parse_algorithm_value(value);
+   if (diff_algorithm  0)
+   return -1;
+   return 0;
+   }
+
if (git_color_config(var, value, cb)  0)
return -1;
 
@@ -3213,6 +3235,7 @@ void diff_setup(struct diff_options *options)
options-add_remove = diff_addremove;
options-use_color = diff_use_color_default;
options-detect_rename = diff_detect_rename_default;
+   options-xdl_opts |= diff_algorithm;
 
if (diff_no_prefix) {
options-a_prefix = options-b_prefix = ;
-- 
1.8.0.2

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-01-14 Thread Junio C Hamano
Michal Privoznik mpriv...@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
 @@ -196,6 +211,13 @@ int git_diff_ui_config(const char *var, const char 
 *value, void *cb)
   return 0;
   }
  
 + if (!strcmp(var, diff.algorithm)) {
 + diff_algorithm = parse_algorithm_value(value);
 + if (diff_algorithm  0)
 + return -1;
 + return 0;
 + }
 +
   if (git_color_config(var, value, cb)  0)
   return -1;
  
 @@ -3213,6 +3235,7 @@ void diff_setup(struct diff_options *options)
   options-add_remove = diff_addremove;
   options-use_color = diff_use_color_default;
   options-detect_rename = diff_detect_rename_default;
 + options-xdl_opts |= diff_algorithm;
  
   if (diff_no_prefix) {
   options-a_prefix = options-b_prefix = ;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html