Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable

2012-11-13 Thread Ramkumar Ramachandra 
Jeff King wrote:
 On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:
 @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char 
 *value, void *cb)
   return 0;
   }

 + if (!strcmp(var, diff.submodule)) {

 Shouldn't this be in git_diff_ui_config so it does not affect scripts
 calling plumbing?

I honestly didn't understand the difference between
git_diff_basic_config and git_diff_ui_config.  The latter function
calls the former function at the end of its body.  Why are they two
separate functions in the first place?

Btw, I just posted a follow-up.

Ram
--
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 2/3] diff: introduce diff.submodule configuration variable

2012-11-13 Thread Junio C Hamano 
Ramkumar Ramachandra artag...@gmail.com writes:

 Jeff King wrote:
 On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:
 @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char 
 *value, void *cb)
   return 0;
   }

 + if (!strcmp(var, diff.submodule)) {

 Shouldn't this be in git_diff_ui_config so it does not affect scripts
 calling plumbing?

 I honestly didn't understand the difference between
 git_diff_basic_config and git_diff_ui_config.  The latter function
 calls the former function at the end of its body.  Why are they two
 separate functions in the first place?

In case you meant s/didn't/don't/, git_diff_ui_config() should be
called only by human-facing Porcelain commands where their
behaviours can and should be affected by end user configuration
variables.

When a configuration variable should not affect output from plumbing
commands like diff-files, diff-index, and diff-tree, it must not be
read in git_diff_basic_config(), but in git_diff_ui_config().

The output from git format-patch is consumed by git apply that
expects Subproject commit %s\n with fully spelled object name, so
your configuration must not affect the output of format-patch,
either.
--
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 2/3] diff: introduce diff.submodule configuration variable

2012-11-12 Thread Jeff King 
On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:

 +static int parse_submodule_params(struct diff_options *options, const char 
 *value,
 + struct strbuf *errmsg)
 +{
 + if (!strcmp(value, log))
 + DIFF_OPT_SET(options, SUBMODULE_LOG);
 + else if (!strcmp(value, short))
 + DIFF_OPT_CLR(options, SUBMODULE_LOG);
 + else {
 + strbuf_addf(errmsg, _('%s'), value);
 + return 1;
 + }
 + return 0;
 +}

I think -1 would be the more normal error return.

 @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char 
 *value, void *cb)
   return 0;
   }
  
 + if (!strcmp(var, diff.submodule)) {

Shouldn't this be in git_diff_ui_config so it does not affect scripts
calling plumbing?

 + struct strbuf errmsg = STRBUF_INIT;
 + if (parse_submodule_params(default_diff_options, value, 
 errmsg))
 + warning(_(Unknown value for 'diff.submodule' config 
 variable: %s),
 + errmsg.buf);
 + strbuf_release(errmsg);
 + return 0;
 + }

Hmm. This strbuf error handling strikes me as very clunky, considering
that it does not pass any useful information out of the parse function
(it always just adds '$value' to the error string).  Wouldn't it be
simpler to just have parse_submodule_params return -1, and then let the
caller warn or generate an error as appropriate?

-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

[PATCH 2/3] diff: introduce diff.submodule configuration variable

2012-11-11 Thread Ramkumar Ramachandra 
Introduce a diff.submodule configuration variable corresponding to the
'--submodule' command-line option of 'git diff'.

Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
 Documentation/diff-config.txt|7 ++
 Documentation/diff-options.txt   |3 +-
 cache.h  |1 +
 diff.c   |   40 ++---
 t/t4041-diff-submodule-option.sh |   30 +++-
 5 files changed, 75 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index decd370..89dd634 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -107,6 +107,13 @@ diff.suppressBlankEmpty::
A boolean to inhibit the standard behavior of printing a space
before each empty output line. Defaults to false.
 
+diff.submodule::
+   Specify the format in which differences in submodules are
+   shown.  The log format lists the commits in the range like
+   linkgit:git-submodule[1] `summary` does.  The short format
+   format just shows the names of the commits at the beginning
+   and end of the range.  Defaults to short.
+
 diff.wordRegex::
A POSIX Extended Regular Expression used to determine what is a word
when performing word-by-word difference calculations.  Character
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index cf4b216..f4f7e25 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -170,7 +170,8 @@ any of those replacements occurred.
the commits in the range like linkgit:git-submodule[1] `summary` does.
Omitting the `--submodule` option or specifying `--submodule=short`,
uses the 'short' format. This format just shows the names of the commits
-   at the beginning and end of the range.
+   at the beginning and end of the range.  Can be tweaked via the
+   `diff.submodule` configuration variable.
 
 --color[=when]::
Show colored diff.
diff --git a/cache.h b/cache.h
index dbd8018..4d9dbc7 100644
--- a/cache.h
+++ b/cache.h
@@ -1221,6 +1221,7 @@ int add_files_to_cache(const char *prefix, const char 
**pathspec, int flags);
 
 /* diff.c */
 extern int diff_auto_refresh_index;
+extern const char *submodule_format_cfg;
 
 /* match-trees.c */
 void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, 
int);
diff --git a/diff.c b/diff.c
index e89a201..b486070 100644
--- a/diff.c
+++ b/diff.c
@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;
 static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
+const char *submodule_format_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 static int diff_no_prefix;
@@ -123,6 +124,20 @@ static int parse_dirstat_params(struct diff_options 
*options, const char *params
return ret;
 }
 
+static int parse_submodule_params(struct diff_options *options, const char 
*value,
+   struct strbuf *errmsg)
+{
+   if (!strcmp(value, log))
+   DIFF_OPT_SET(options, SUBMODULE_LOG);
+   else if (!strcmp(value, short))
+   DIFF_OPT_CLR(options, SUBMODULE_LOG);
+   else {
+   strbuf_addf(errmsg, _('%s'), value);
+   return 1;
+   }
+   return 0;
+}
+
 static int git_config_rename(const char *var, const char *value)
 {
if (!value)
@@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char 
*value, void *cb)
return 0;
}
 
+   if (!strcmp(var, diff.submodule)) {
+   struct strbuf errmsg = STRBUF_INIT;
+   if (parse_submodule_params(default_diff_options, value, 
errmsg))
+   warning(_(Unknown value for 'diff.submodule' config 
variable: %s),
+   errmsg.buf);
+   strbuf_release(errmsg);
+   return 0;
+   }
+
if (!prefixcmp(var, submodule.))
return parse_submodule_config_option(var, value);
 
@@ -3475,6 +3499,16 @@ static int parse_dirstat_opt(struct diff_options 
*options, const char *params)
return 1;
 }
 
+static int parse_submodule_opt(struct diff_options *options, const char 
*params)
+{
+   struct strbuf errmsg = STRBUF_INIT;
+   if (parse_submodule_params(options, params, errmsg))
+   die(_(Failed to parse --submodule option parameter: %s),
+   errmsg.buf);
+   strbuf_release(errmsg);
+   return 1;
+}
+
 int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 {
const char *arg = av[0];
@@ -3655,10 +3689,8 @@ int diff_opt_parse(struct diff_options *options, const 
char **av, int ac)
handle_ignore_submodules_arg(options, arg + 20);
} else if (!strcmp(arg, --submodule))
DIFF_OPT_SET(options,

Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable

2012-11-08 Thread Jeff King 
On Thu, Nov 01, 2012 at 04:13:49PM +0530, Ramkumar Ramachandra wrote:

 diff --git a/builtin/diff.c b/builtin/diff.c
 index 9650be2..6d00311 100644
 --- a/builtin/diff.c
 +++ b/builtin/diff.c
 @@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char 
 *prefix)
   DIFF_OPT_SET(rev.diffopt, ALLOW_EXTERNAL);
   DIFF_OPT_SET(rev.diffopt, ALLOW_TEXTCONV);
  
 + /* Set SUBMODULE_LOG if diff.submodule config var was set */
 + if (submodule_format_cfg  !strcmp(submodule_format_cfg, log))
 + DIFF_OPT_SET(rev.diffopt, SUBMODULE_LOG);
 +

Yuck. Why is this parsing happening in cmd_diff?

Wouldn't you want it to kick in for git log --submodule, too? It seems
like it should go into diff_setup(), and the porcelain/plumbing aspect
should be controlled by parsing it in git_diff_ui_config or
git_diff_basic_config. See how diff_no_prefix and diff_mnemonic prefix
are handled for an example.

Then you can keep the parsing logic for log in diff.c. And you should
factor it out into a function so that the command-line option and the
config parser can share the same code. I know it's only one line now,
but anybody who wants to add an option will have to update both places.
See the parsing of diff.dirstat for an example.

   else if (!prefixcmp(arg, --submodule=)) {
   if (!strcmp(arg + 12, log))
   DIFF_OPT_SET(options, SUBMODULE_LOG);
 + if (!strcmp(arg + 12, short))
 + DIFF_OPT_CLR(options, SUBMODULE_LOG);
   }

Much better (although arguably should go in a separate patch). Should we
also produce an error if somebody says --submodule=foobar?

-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

[PATCH 2/3] diff: introduce diff.submodule configuration variable

2012-11-01 Thread Ramkumar Ramachandra 
Introduce a diff.submodule configuration variable corresponding to the
'--submodule' command-line option of 'git diff'.

Signed-off-by: Ramkumar Ramachandra artag...@gmail.com
---
 Documentation/diff-config.txt|7 +++
 Documentation/diff-options.txt   |3 ++-
 builtin/diff.c   |4 
 cache.h  |1 +
 diff.c   |5 +
 t/t4041-diff-submodule-option.sh |   30 +-
 6 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index decd370..89dd634 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -107,6 +107,13 @@ diff.suppressBlankEmpty::
A boolean to inhibit the standard behavior of printing a space
before each empty output line. Defaults to false.
 
+diff.submodule::
+   Specify the format in which differences in submodules are
+   shown.  The log format lists the commits in the range like
+   linkgit:git-submodule[1] `summary` does.  The short format
+   format just shows the names of the commits at the beginning
+   and end of the range.  Defaults to short.
+
 diff.wordRegex::
A POSIX Extended Regular Expression used to determine what is a word
when performing word-by-word difference calculations.  Character
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index cf4b216..f4f7e25 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -170,7 +170,8 @@ any of those replacements occurred.
the commits in the range like linkgit:git-submodule[1] `summary` does.
Omitting the `--submodule` option or specifying `--submodule=short`,
uses the 'short' format. This format just shows the names of the commits
-   at the beginning and end of the range.
+   at the beginning and end of the range.  Can be tweaked via the
+   `diff.submodule` configuration variable.
 
 --color[=when]::
Show colored diff.
diff --git a/builtin/diff.c b/builtin/diff.c
index 9650be2..6d00311 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char 
*prefix)
DIFF_OPT_SET(rev.diffopt, ALLOW_EXTERNAL);
DIFF_OPT_SET(rev.diffopt, ALLOW_TEXTCONV);
 
+   /* Set SUBMODULE_LOG if diff.submodule config var was set */
+   if (submodule_format_cfg  !strcmp(submodule_format_cfg, log))
+   DIFF_OPT_SET(rev.diffopt, SUBMODULE_LOG);
+
if (nongit)
die(_(Not a git repository));
argc = setup_revisions(argc, argv, rev, NULL);
diff --git a/cache.h b/cache.h
index a58df84..5fc7ba3 100644
--- a/cache.h
+++ b/cache.h
@@ -1220,6 +1220,7 @@ int add_files_to_cache(const char *prefix, const char 
**pathspec, int flags);
 
 /* diff.c */
 extern int diff_auto_refresh_index;
+extern const char *submodule_format_cfg;
 
 /* match-trees.c */
 void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, 
int);
diff --git a/diff.c b/diff.c
index 86e5f2a..835eb26 100644
--- a/diff.c
+++ b/diff.c
@@ -29,6 +29,7 @@ static int diff_use_color_default = -1;
 static int diff_context_default = 3;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
+const char *submodule_format_cfg;
 int diff_auto_refresh_index = 1;
 static int diff_mnemonic_prefix;
 static int diff_no_prefix;
@@ -168,6 +169,8 @@ int git_diff_ui_config(const char *var, const char *value, 
void *cb)
diff_stat_graph_width = git_config_int(var, value);
return 0;
}
+   if (!strcmp(var, diff.submodule))
+   return git_config_string(submodule_format_cfg, var, value);
if (!strcmp(var, diff.external))
return git_config_string(external_diff_cmd_cfg, var, value);
if (!strcmp(var, diff.wordregex))
@@ -3656,6 +3659,8 @@ int diff_opt_parse(struct diff_options *options, const 
char **av, int ac)
else if (!prefixcmp(arg, --submodule=)) {
if (!strcmp(arg + 12, log))
DIFF_OPT_SET(options, SUBMODULE_LOG);
+   if (!strcmp(arg + 12, short))
+   DIFF_OPT_CLR(options, SUBMODULE_LOG);
}
 
/* misc options */
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 6c01d0c..e401814 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -33,6 +33,7 @@ test_create_repo sm1 
 add_file . foo /dev/null
 
 head1=$(add_file sm1 foo1 foo2)
+fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
 
 test_expect_success 'added submodule' 
git add sm1 
@@ -43,6 +44,34 @@ EOF
test_cmp expected actual
 
 
+test_expect_success 'added submodule, set diff.submodule' 
+   git config diff.submodule log 
+   git add sm1 
+   git diff --cached actual 
+