{"thread":{"id":"32077","subject":"[PATCH v3 0/3] Introduce diff.submodule","startedAt":"2012-11-11T16:59:03Z","lastAt":"2012-11-13T16:27:08Z","messageCount":9,"participants":["Ramkumar Ramachandra","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"202884","messageId":"1352653146-3932-1-git-send-email-artagnon@gmail.com","threadId":"32077","inReplyTo":null,"subject":"[PATCH v3 0/3] Introduce diff.submodule","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-11T16:59:03Z","receivedAt":"2012-11-11T16:59:03Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"v1 is here: http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com\nv2 is here: http://mid.gmane.org/1351766630-4837-1-git-send-email-artagnon@gmail.com\n\nThis version was prepared in response to Peff's review of v2. As\nsuggested, I've created a separate function which both '--submodule'\nand 'diff.submodule' use to set/ unset SUBMODULE_OPT.\n\nRam\n\nRamkumar Ramachandra (3):\n Documentation: move diff.wordRegex from config.txt to diff-config.txt\n diff: introduce diff.submodule configuration variable\n submodule: display summary header in bold\n\n Documentation/config.txt | 6 -----\n Documentation/diff-config.txt | 13 +++++++++++\n Documentation/diff-options.txt | 3 +-\n cache.h | 1 +\n diff.c | 42 +++++++++++++++++++++++++++++++++----\n submodule.c | 8 +++---\n submodule.h | 2 +-\n t/t4041-diff-submodule-option.sh | 30 ++++++++++++++++++++++++++-\n 8 files changed, 87 insertions(+), 18 deletions(-)\n\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"202885","messageId":"1352653146-3932-2-git-send-email-artagnon@gmail.com","threadId":"32077","inReplyTo":"1352653146-3932-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/3] Documentation: move diff.wordRegex from config.txt to diff-config.txt","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-11T16:59:04Z","receivedAt":"2012-11-11T16:59:04Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"19299a8 (Documentation: Move diff..* from config.txt to\ndiff-config.txt, 2011-04-07) moved the diff configuration options to\ndiff-config.txt, but forgot about diff.wordRegex, which was left\nbehind in config.txt. Fix this.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n Documentation/config.txt | 6 ------\n Documentation/diff-config.txt | 6 ++++++\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 9a0544c..e70216d 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -962,12 +962,6 @@ difftool..cmd::\n difftool.prompt::\n \tPrompt before each invocation of the diff tool.\n \n-diff.wordRegex::\n-\tA POSIX Extended Regular Expression used to determine what is a \"word\"\n-\twhen performing word-by-word difference calculations. Character\n-\tsequences that match the regular expression are \"words\", all other\n-\tcharacters are *ignorable* whitespace.\n-\n fetch.recurseSubmodules::\n \tThis option can be either set to a boolean value or to 'on-demand'.\n \tSetting it to a boolean changes the behavior of fetch and pull to\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex 75ab8a5..decd370 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -107,6 +107,12 @@ diff.suppressBlankEmpty::\n \tA boolean to inhibit the standard behavior of printing a space\n \tbefore each empty output line. Defaults to false.\n \n+diff.wordRegex::\n+\tA POSIX Extended Regular Expression used to determine what is a \"word\"\n+\twhen performing word-by-word difference calculations. Character\n+\tsequences that match the regular expression are \"words\", all other\n+\tcharacters are *ignorable* whitespace.\n+\n diff..command::\n \tThe custom diff driver command. See linkgit:gitattributes[5]\n \tfor details.\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"202886","messageId":"1352653146-3932-3-git-send-email-artagnon@gmail.com","threadId":"32077","inReplyTo":"1352653146-3932-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/3] diff: introduce diff.submodule configuration variable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-11T16:59:05Z","receivedAt":"2012-11-11T16:59:05Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Introduce a diff.submodule configuration variable corresponding to the\n'--submodule' command-line option of 'git diff'.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n Documentation/diff-config.txt | 7 ++++++\n Documentation/diff-options.txt | 3 +-\n cache.h | 1 +\n diff.c | 40 ++++++++++++++++++++++++++++++++++---\n t/t4041-diff-submodule-option.sh | 30 +++++++++++++++++++++++++++-\n 5 files changed, 75 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt\nindex decd370..89dd634 100644\n--- a/Documentation/diff-config.txt\n+++ b/Documentation/diff-config.txt\n@@ -107,6 +107,13 @@ diff.suppressBlankEmpty::\n \tA boolean to inhibit the standard behavior of printing a space\n \tbefore each empty output line. Defaults to false.\n \n+diff.submodule::\n+\tSpecify the format in which differences in submodules are\n+\tshown. The \"log\" format lists the commits in the range like\n+\tlinkgit:git-submodule[1] `summary` does. The \"short\" format\n+\tformat just shows the names of the commits at the beginning\n+\tand end of the range. Defaults to short.\n+\n diff.wordRegex::\n \tA POSIX Extended Regular Expression used to determine what is a \"word\"\n \twhen performing word-by-word difference calculations. Character\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex cf4b216..f4f7e25 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -170,7 +170,8 @@ any of those replacements occurred.\n \tthe commits in the range like linkgit:git-submodule[1] `summary` does.\n \tOmitting the `--submodule` option or specifying `--submodule=short`,\n \tuses the 'short' format. This format just shows the names of the commits\n-\tat the beginning and end of the range.\n+\tat the beginning and end of the range. Can be tweaked via the\n+\t`diff.submodule` configuration variable.\n \n --color[=]::\n \tShow colored diff.\ndiff --git a/cache.h b/cache.h\nindex dbd8018..4d9dbc7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1221,6 +1221,7 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags);\n \n /* diff.c */\n extern int diff_auto_refresh_index;\n+extern const char *submodule_format_cfg;\n \n /* match-trees.c */\n void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, int);\ndiff --git a/diff.c b/diff.c\nindex e89a201..b486070 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -30,6 +30,7 @@ static int diff_use_color_default = -1;\n static int diff_context_default = 3;\n static const char *diff_word_regex_cfg;\n static const char *external_diff_cmd_cfg;\n+const char *submodule_format_cfg;\n int diff_auto_refresh_index = 1;\n static int diff_mnemonic_prefix;\n static int diff_no_prefix;\n@@ -123,6 +124,20 @@ static int parse_dirstat_params(struct diff_options *options, const char *params\n \treturn ret;\n }\n \n+static int parse_submodule_params(struct diff_options *options, const char *value,\n+\t\t\t\tstruct strbuf *errmsg)\n+{\n+\tif (!strcmp(value, \"log\"))\n+\t\tDIFF_OPT_SET(options, SUBMODULE_LOG);\n+\telse if (!strcmp(value, \"short\"))\n+\t\tDIFF_OPT_CLR(options, SUBMODULE_LOG);\n+\telse {\n+\t\tstrbuf_addf(errmsg, _(\"'%s'\"), value);\n+\t\treturn 1;\n+\t}\n+\treturn 0;\n+}\n+\n static int git_config_rename(const char *var, const char *value)\n {\n \tif (!value)\n@@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"diff.submodule\")) {\n+\t\tstruct strbuf errmsg = STRBUF_INIT;\n+\t\tif (parse_submodule_params(&default_diff_options, value, &errmsg))\n+\t\t\twarning(_(\"Unknown value for 'diff.submodule' config variable: %s\"),\n+\t\t\t\terrmsg.buf);\n+\t\tstrbuf_release(&errmsg);\n+\t\treturn 0;\n+\t}\n+\n \tif (!prefixcmp(var, \"submodule.\"))\n \t\treturn parse_submodule_config_option(var, value);\n \n@@ -3475,6 +3499,16 @@ static int parse_dirstat_opt(struct diff_options *options, const char *params)\n \treturn 1;\n }\n \n+static int parse_submodule_opt(struct diff_options *options, const char *params)\n+{\n+\tstruct strbuf errmsg = STRBUF_INIT;\n+\tif (parse_submodule_params(options, params, &errmsg))\n+\t\tdie(_(\"Failed to parse --submodule option parameter: %s\"),\n+\t\t\terrmsg.buf);\n+\tstrbuf_release(&errmsg);\n+\treturn 1;\n+}\n+\n int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n {\n \tconst char *arg = av[0];\n@@ -3655,10 +3689,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\thandle_ignore_submodules_arg(options, arg + 20);\n \t} else if (!strcmp(arg, \"--submodule\"))\n \t\tDIFF_OPT_SET(options, SUBMODULE_LOG);\n-\telse if (!prefixcmp(arg, \"--submodule=\")) {\n-\t\tif (!strcmp(arg + 12, \"log\"))\n-\t\t\tDIFF_OPT_SET(options, SUBMODULE_LOG);\n-\t}\n+\telse if (!prefixcmp(arg, \"--submodule=\"))\n+\t\treturn parse_submodule_opt(options, arg + 12);\n \n \t/* misc options */\n \telse if (!strcmp(arg, \"-z\"))\ndiff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh\nindex 6c01d0c..e401814 100755\n--- a/t/t4041-diff-submodule-option.sh\n+++ b/t/t4041-diff-submodule-option.sh\n@@ -33,6 +33,7 @@ test_create_repo sm1 &&\n add_file . foo >/dev/null\n \n head1=$(add_file sm1 foo1 foo2)\n+fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)\n \n test_expect_success 'added submodule' \"\n \tgit add sm1 &&\n@@ -43,6 +44,34 @@ EOF\n \ttest_cmp expected actual\n \"\n \n+test_expect_success 'added submodule, set diff.submodule' \"\n+\tgit config diff.submodule log &&\n+\tgit add sm1 &&\n+\tgit diff --cached >actual &&\n+\tcat >expected <<-EOF &&\n+Submodule sm1 0000000...$head1 (new submodule)\n+EOF\n+\tgit config --unset diff.submodule &&\n+\ttest_cmp expected actual\n+\"\n+\n+test_expect_success '--submodule=short overrides diff.submodule' \"\n+\tgit config diff.submodule log &&\n+\tgit add sm1 &&\n+\tgit diff --submodule=short --cached >actual &&\n+\tcat >expected <<-EOF &&\n+diff --git a/sm1 b/sm1\n+new file mode 160000\n+index 0000000..a2c4dab\n+--- /dev/null\n++++ b/sm1\n+@@ -0,0 +1 @@\n++Subproject commit $fullhead1\n+EOF\n+\tgit config --unset diff.submodule &&\n+\ttest_cmp expected actual\n+\"\n+\n commit_file sm1 &&\n head2=$(add_file sm1 foo3)\n \n@@ -73,7 +102,6 @@ EOF\n \ttest_cmp expected actual\n \"\n \n-fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)\n fullhead2=$(cd sm1; git rev-list --max-count=1 $head2)\n test_expect_success 'modified submodule(forward) --submodule=short' \"\n \tgit diff --submodule=short >actual &&\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"202887","messageId":"1352653146-3932-4-git-send-email-artagnon@gmail.com","threadId":"32077","inReplyTo":"1352653146-3932-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/3] submodule: display summary header in bold","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-11T16:59:06Z","receivedAt":"2012-11-11T16:59:06Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Currently, 'git diff --submodule' displays output with a bold diff\nheader for non-submodules. So this part is in bold:\n\n diff --git a/file1 b/file1\n index 30b2f6c..2638038 100644\n --- a/file1\n +++ b/file1\n\nFor submodules, the header looks like this:\n\n Submodule submodule1 012b072..248d0fd:\n\nUnfortunately, it's easy to miss in the output because it's not bold.\nChange this.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n diff.c | 2 +-\n submodule.c | 8 ++++----\n submodule.h | 2 +-\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex b486070..51c0d6c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2267,7 +2267,7 @@ static void builtin_diff(const char *name_a,\n \t\tconst char *add = diff_get_color_opt(o, DIFF_FILE_NEW);\n \t\tshow_submodule_summary(o->file, one ? one->path : two->path,\n \t\t\t\tone->sha1, two->sha1, two->dirty_submodule,\n-\t\t\t\tdel, add, reset);\n+\t\t\t\tset, del, add, reset);\n \t\treturn;\n \t}\n \ndiff --git a/submodule.c b/submodule.c\nindex e3e0b45..c10182e 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -258,7 +258,7 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg)\n \n void show_submodule_summary(FILE *f, const char *path,\n \t\tunsigned char one[20], unsigned char two[20],\n-\t\tunsigned dirty_submodule,\n+\t\tunsigned dirty_submodule, const char *set,\n \t\tconst char *del, const char *add, const char *reset)\n {\n \tstruct rev_info rev;\n@@ -292,15 +292,15 @@ void show_submodule_summary(FILE *f, const char *path,\n \t\treturn;\n \t}\n \n-\tstrbuf_addf(&sb, \"Submodule %s %s..\", path,\n+\tstrbuf_addf(&sb, \"%sSubmodule %s %s..\", set, path,\n \t\t\tfind_unique_abbrev(one, DEFAULT_ABBREV));\n \tif (!fast_backward && !fast_forward)\n \t\tstrbuf_addch(&sb, '.');\n \tstrbuf_addf(&sb, \"%s\", find_unique_abbrev(two, DEFAULT_ABBREV));\n \tif (message)\n-\t\tstrbuf_addf(&sb, \" %s\\n\", message);\n+\t\tstrbuf_addf(&sb, \" %s%s\\n\", message, reset);\n \telse\n-\t\tstrbuf_addf(&sb, \"%s:\\n\", fast_backward ? \" (rewind)\" : \"\");\n+\t\tstrbuf_addf(&sb, \"%s:%s\\n\", fast_backward ? \" (rewind)\" : \"\", reset);\n \tfwrite(sb.buf, sb.len, 1, f);\n \n \tif (!message) {\ndiff --git a/submodule.h b/submodule.h\nindex f2e8271..997fd06 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -20,7 +20,7 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);\n int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg);\n void show_submodule_summary(FILE *f, const char *path,\n \t\tunsigned char one[20], unsigned char two[20],\n-\t\tunsigned dirty_submodule,\n+\t\tunsigned dirty_submodule, const char *set,\n \t\tconst char *del, const char *add, const char *reset);\n void set_config_fetch_recurse_submodules(int value);\n void check_for_new_submodule_commits(unsigned char new_sha1[20]);\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"203047","messageId":"CALkWK0ncA8R2NbPTCAJtiDgk3aRGwcAOCWLuwQLFR=cZZ+Zdxw@mail.gmail.com","threadId":"32077","inReplyTo":"1352653146-3932-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v3 0/3] Introduce diff.submodule","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-13T05:17:56Z","receivedAt":"2012-11-13T05:17:56Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Ramkumar Ramachandra wrote:\n> v1 is here: http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com\n> v2 is here: http://mid.gmane.org/1351766630-4837-1-git-send-email-artagnon@gmail.com\n>\n> This version was prepared in response to Peff's review of v2. As\n> suggested, I've created a separate function which both '--submodule'\n> and 'diff.submodule' use to set/ unset SUBMODULE_OPT.\n\nJunio, can we pick this up?\n\nRam\n"},{"id":"203050","messageId":"20121113053336.GA10995@sigill.intra.peff.net","threadId":"32077","inReplyTo":"1352653146-3932-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T05:33:36Z","receivedAt":"2012-11-13T05:33:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:\n\n> +static int parse_submodule_params(struct diff_options *options, const char *value,\n> +\t\t\t\tstruct strbuf *errmsg)\n> +{\n> +\tif (!strcmp(value, \"log\"))\n> +\t\tDIFF_OPT_SET(options, SUBMODULE_LOG);\n> +\telse if (!strcmp(value, \"short\"))\n> +\t\tDIFF_OPT_CLR(options, SUBMODULE_LOG);\n> +\telse {\n> +\t\tstrbuf_addf(errmsg, _(\"'%s'\"), value);\n> +\t\treturn 1;\n> +\t}\n> +\treturn 0;\n> +}\n\nI think \"-1\" would be the more normal error return.\n\n> @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n> \t\treturn 0;\n> \t}\n> \n> +\tif (!strcmp(var, \"diff.submodule\")) {\n\nShouldn't this be in git_diff_ui_config so it does not affect scripts\ncalling plumbing?\n\n> +\t\tstruct strbuf errmsg = STRBUF_INIT;\n> +\t\tif (parse_submodule_params(&default_diff_options, value, &errmsg))\n> +\t\t\twarning(_(\"Unknown value for 'diff.submodule' config variable: %s\"),\n> +\t\t\t\terrmsg.buf);\n> +\t\tstrbuf_release(&errmsg);\n> +\t\treturn 0;\n> +\t}\n\nHmm. This strbuf error handling strikes me as very clunky, considering\nthat it does not pass any useful information out of the parse function\n(it always just adds '$value' to the error string). Wouldn't it be\nsimpler to just have parse_submodule_params return -1, and then let the\ncaller warn or generate an error as appropriate?\n\n-Peff\n"},{"id":"203051","messageId":"20121113054447.GB10995@sigill.intra.peff.net","threadId":"32077","inReplyTo":"1352653146-3932-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 3/3] submodule: display summary header in bold","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-13T05:44:47Z","receivedAt":"2012-11-13T05:44:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 11, 2012 at 10:29:06PM +0530, Ramkumar Ramachandra wrote:\n\n> Currently, 'git diff --submodule' displays output with a bold diff\n> header for non-submodules. So this part is in bold:\n> \n> diff --git a/file1 b/file1\n> index 30b2f6c..2638038 100644\n> --- a/file1\n> +++ b/file1\n> \n> For submodules, the header looks like this:\n> \n> Submodule submodule1 012b072..248d0fd:\n> \n> Unfortunately, it's easy to miss in the output because it's not bold.\n> Change this.\n\nReading this, I was at first concerned that you were using \"bold\" and\nnot DIFF_METAINFO. But:\n\n> diff --git a/diff.c b/diff.c\n> index b486070..51c0d6c 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2267,7 +2267,7 @@ static void builtin_diff(const char *name_a,\n> \t\tconst char *add = diff_get_color_opt(o, DIFF_FILE_NEW);\n> \t\tshow_submodule_summary(o->file, one ? one->path : two->path,\n> \t\t\t\tone->sha1, two->sha1, two->dirty_submodule,\n> -\t\t\t\tdel, add, reset);\n> +\t\t\t\tset, del, add, reset);\n\nThis is correctly passing the DIFF_METAINFO color, which is in the \"set\"\nvariable. Good. I looked at the output on a sample repository, and it is\nmuch easier to read (though I use magenta for my metainfo).\n\n\"set\" is a terrible name for that variable (not your fault, of course).\nWhen you re-roll (and I think you need to because of the diff_ui thing\nin patch 2), would you mind throwing this cleanup in the middle?\n\n-- >8 --\nSubject: builtin_diff: rename \"set\" variable\n\nOnce upon a time the builtin_diff function used one color, and the color\nvariables were called \"set\" and \"reset\". Nowadays it is a much longer\nfunction and we use several colors (e.g., \"add\", \"del\"). Rename \"set\" to\n\"meta\" to show that it is the color for showing diff meta-info (it still\ndoes not indicate that it is a \"color\", but at least it matches the\nscheme of the other color variables).\n\nSigned-off-by: Jeff King \n---\n diff.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex e89a201..978e1af 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2222,7 +2222,7 @@ static void builtin_diff(const char *name_a,\n \tmmfile_t mf1, mf2;\n \tconst char *lbl[2];\n \tchar *a_one, *b_two;\n-\tconst char *set = diff_get_color_opt(o, DIFF_METAINFO);\n+\tconst char *meta = diff_get_color_opt(o, DIFF_METAINFO);\n \tconst char *reset = diff_get_color_opt(o, DIFF_RESET);\n \tconst char *a_prefix, *b_prefix;\n \tstruct userdiff_driver *textconv_one = NULL;\n@@ -2269,24 +2269,24 @@ static void builtin_diff(const char *name_a,\n \tb_two = quote_two(b_prefix, name_b + (*name_b == '/'));\n \tlbl[0] = DIFF_FILE_VALID(one) ? a_one : \"/dev/null\";\n \tlbl[1] = DIFF_FILE_VALID(two) ? b_two : \"/dev/null\";\n-\tstrbuf_addf(&header, \"%s%sdiff --git %s %s%s\\n\", line_prefix, set, a_one, b_two, reset);\n+\tstrbuf_addf(&header, \"%s%sdiff --git %s %s%s\\n\", line_prefix, meta, a_one, b_two, reset);\n \tif (lbl[0][0] == '/') {\n \t\t/* /dev/null */\n-\t\tstrbuf_addf(&header, \"%s%snew file mode %06o%s\\n\", line_prefix, set, two->mode, reset);\n+\t\tstrbuf_addf(&header, \"%s%snew file mode %06o%s\\n\", line_prefix, meta, two->mode, reset);\n \t\tif (xfrm_msg)\n \t\t\tstrbuf_addstr(&header, xfrm_msg);\n \t\tmust_show_header = 1;\n \t}\n \telse if (lbl[1][0] == '/') {\n-\t\tstrbuf_addf(&header, \"%s%sdeleted file mode %06o%s\\n\", line_prefix, set, one->mode, reset);\n+\t\tstrbuf_addf(&header, \"%s%sdeleted file mode %06o%s\\n\", line_prefix, meta, one->mode, reset);\n \t\tif (xfrm_msg)\n \t\t\tstrbuf_addstr(&header, xfrm_msg);\n \t\tmust_show_header = 1;\n \t}\n \telse {\n \t\tif (one->mode != two->mode) {\n-\t\t\tstrbuf_addf(&header, \"%s%sold mode %06o%s\\n\", line_prefix, set, one->mode, reset);\n-\t\t\tstrbuf_addf(&header, \"%s%snew mode %06o%s\\n\", line_prefix, set, two->mode, reset);\n+\t\t\tstrbuf_addf(&header, \"%s%sold mode %06o%s\\n\", line_prefix, meta, one->mode, reset);\n+\t\t\tstrbuf_addf(&header, \"%s%snew mode %06o%s\\n\", line_prefix, meta, two->mode, reset);\n \t\t\tmust_show_header = 1;\n \t\t}\n \t\tif (xfrm_msg)\n"},{"id":"203098","messageId":"CALkWK0nJwznx36yoAUKXRnyA+32143tBVJHnnrosz-Ht7VhwHw@mail.gmail.com","threadId":"32077","inReplyTo":"20121113053336.GA10995@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-13T15:45:30Z","receivedAt":"2012-11-13T15:45:30Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:\n>> @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n>> return 0;\n>> }\n>>\n>> + if (!strcmp(var, \"diff.submodule\")) {\n>\n> Shouldn't this be in git_diff_ui_config so it does not affect scripts\n> calling plumbing?\n\nI honestly didn't understand the difference between\ngit_diff_basic_config and git_diff_ui_config. The latter function\ncalls the former function at the end of its body. Why are they two\nseparate functions in the first place?\n\nBtw, I just posted a follow-up.\n\nRam\n"},{"id":"203102","messageId":"7vlie5foyr.fsf@alter.siamese.dyndns.org","threadId":"32077","inReplyTo":"CALkWK0nJwznx36yoAUKXRnyA+32143tBVJHnnrosz-Ht7VhwHw@mail.gmail.com","subject":"Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-13T16:27:08Z","receivedAt":"2012-11-13T16:27:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Jeff King wrote:\n>> On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:\n>>> @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)\n>>> return 0;\n>>> }\n>>>\n>>> + if (!strcmp(var, \"diff.submodule\")) {\n>>\n>> Shouldn't this be in git_diff_ui_config so it does not affect scripts\n>> calling plumbing?\n>\n> I honestly didn't understand the difference between\n> git_diff_basic_config and git_diff_ui_config. The latter function\n> calls the former function at the end of its body. Why are they two\n> separate functions in the first place?\n\nIn case you meant s/didn't/don't/, git_diff_ui_config() should be\ncalled only by human-facing Porcelain commands where their\nbehaviours can and should be affected by end user configuration\nvariables.\n\nWhen a configuration variable should not affect output from plumbing\ncommands like diff-files, diff-index, and diff-tree, it must not be\nread in git_diff_basic_config(), but in git_diff_ui_config().\n\nThe output from \"git format-patch\" is consumed by \"git apply\" that\nexpects \"Subproject commit %s\\n\" with fully spelled object name, so\nyour configuration must not affect the output of format-patch,\neither.\n"}]}