{"thread":{"id":"32100","subject":"[PATCH v4 0/4] Introduce diff.submodule","startedAt":"2012-11-13T15:42:43Z","lastAt":"2012-11-22T09:44:18Z","messageCount":12,"participants":["Ramkumar Ramachandra","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":4},"messages":[{"id":"203093","messageId":"1352821367-3611-1-git-send-email-artagnon@gmail.com","threadId":"32100","inReplyTo":"http://mid.gmane.org/1352653146-3932-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v4 0/4] Introduce diff.submodule","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-13T15:42:43Z","receivedAt":"2012-11-13T15:42:43Z","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\nv3 is here: http://mid.gmane.org/1352653146-3932-1-git-send-email-artagnon@gmail.com\n\nThis version was prepared in response to Peff's review of v3.\nWhat changed:\n* Functional code simplified and moved to git_diff_ui_config.\n* Peff contributed one additional patch to the series.\n\nThanks.\n\nRam\n\nJeff King (1):\n  diff: rename \"set\" variable\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 diff.c                           |   46 ++++++++++++++++++++++++++++---------\n submodule.c                      |    8 +++---\n submodule.h                      |    2 +-\n t/t4041-diff-submodule-option.sh |   30 ++++++++++++++++++++++++-\n 7 files changed, 84 insertions(+), 24 deletions(-)\n\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"203094","messageId":"1352821367-3611-2-git-send-email-artagnon@gmail.com","threadId":"32100","inReplyTo":"1352821367-3611-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v4 1/4] Documentation: move diff.wordRegex from config.txt to diff-config.txt","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-13T15:42:44Z","receivedAt":"2012-11-13T15:42:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"19299a8 (Documentation: Move diff.<driver>.* 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 <artagnon@gmail.com>\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.<tool>.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.<driver>.command::\n \tThe custom diff driver command.  See linkgit:gitattributes[5]\n \tfor details.\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"203096","messageId":"1352821367-3611-3-git-send-email-artagnon@gmail.com","threadId":"32100","inReplyTo":"1352821367-3611-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v4 2/4] diff: introduce diff.submodule configuration variable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-13T15:42:45Z","receivedAt":"2012-11-13T15:42:45Z","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 <artagnon@gmail.com>\n---\n Documentation/diff-config.txt    |    7 +++++++\n Documentation/diff-options.txt   |    3 ++-\n diff.c                           |   32 ++++++++++++++++++++++++++++----\n t/t4041-diff-submodule-option.sh |   30 +++++++++++++++++++++++++++++-\n 4 files changed, 66 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[=<when>]::\n \tShow colored diff.\ndiff --git a/diff.c b/diff.c\nindex e89a201..7f2a255 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -123,6 +123,17 @@ 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+{\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\treturn -1;\n+\treturn 0;\n+}\n+\n static int git_config_rename(const char *var, const char *value)\n {\n \tif (!value)\n@@ -178,6 +189,13 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \tif (!strcmp(var, \"diff.ignoresubmodules\"))\n \t\thandle_ignore_submodules_arg(&default_diff_options, value);\n \n+\tif (!strcmp(var, \"diff.submodule\")) {\n+\t\tif (parse_submodule_params(&default_diff_options, value))\n+\t\t\twarning(_(\"Unknown value for 'diff.submodule' config variable: '%s'\"),\n+\t\t\t\tvalue);\n+\t\treturn 0;\n+\t}\n+\n \tif (git_color_config(var, value, cb) < 0)\n \t\treturn -1;\n \n@@ -3475,6 +3493,14 @@ 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 *value)\n+{\n+\tif (parse_submodule_params(options, value))\n+\t\tdie(_(\"Failed to parse --submodule option parameter: '%s'\"),\n+\t\t\tvalue);\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 +3681,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":"203095","messageId":"1352821367-3611-4-git-send-email-artagnon@gmail.com","threadId":"32100","inReplyTo":"1352821367-3611-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v4 3/4] diff: rename \"set\" variable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-13T15:42:46Z","receivedAt":"2012-11-13T15:42:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"From: Jeff King <peff@peff.net>\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 <peff@peff.net>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n diff.c |   12 ++++++------\n 1 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 7f2a255..ffaed72 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2240,7 +2240,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@@ -2287,24 +2287,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-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"203097","messageId":"1352821367-3611-5-git-send-email-artagnon@gmail.com","threadId":"32100","inReplyTo":"1352821367-3611-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v4 4/4] submodule: display summary header in bold","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-13T15:42:47Z","receivedAt":"2012-11-13T15:42:47Z","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 <artagnon@gmail.com>\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 ffaed72..1065978 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2261,7 +2261,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\tmeta, del, add, reset);\n \t\treturn;\n \t}\n \ndiff --git a/submodule.c b/submodule.c\nindex e3e0b45..2f55436 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 *meta,\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..\", meta, 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..3dc1b3f 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 *meta,\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":"203293","messageId":"20121115162331.GD6157@sigill.intra.peff.net","threadId":"32100","inReplyTo":"1352821367-3611-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v4 0/4] Introduce diff.submodule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T16:23:34Z","receivedAt":"2012-11-15T16:23:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 13, 2012 at 09:12:43PM +0530, Ramkumar Ramachandra wrote:\n\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> v3 is here: http://mid.gmane.org/1352653146-3932-1-git-send-email-artagnon@gmail.com\n> \n> This version was prepared in response to Peff's review of v3.\n> What changed:\n> * Functional code simplified and moved to git_diff_ui_config.\n> * Peff contributed one additional patch to the series.\n\nThanks, this version looks good to me.\n\n-Peff\n"},{"id":"203294","messageId":"20121115162524.GE6157@sigill.intra.peff.net","threadId":"32100","inReplyTo":"20121115162331.GD6157@sigill.intra.peff.net","subject":"Re: [PATCH v4 0/4] Introduce diff.submodule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T16:25:26Z","receivedAt":"2012-11-15T16:25:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 08:23:34AM -0800, Jeff King wrote:\n\n> On Tue, Nov 13, 2012 at 09:12:43PM +0530, Ramkumar Ramachandra wrote:\n> \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> > v3 is here: http://mid.gmane.org/1352653146-3932-1-git-send-email-artagnon@gmail.com\n> > \n> > This version was prepared in response to Peff's review of v3.\n> > What changed:\n> > * Functional code simplified and moved to git_diff_ui_config.\n> > * Peff contributed one additional patch to the series.\n> \n> Thanks, this version looks good to me.\n\nOh wait. I did not look closely enough. The point was to move the option\nparser _out_ of git_diff_ui_config into git_diff_basic_config, so that\nit only triggers for porcelain, not plumbing.\n\n-Peff\n"},{"id":"203295","messageId":"20121115163356.GF6157@sigill.intra.peff.net","threadId":"32100","inReplyTo":"20121115162524.GE6157@sigill.intra.peff.net","subject":"Re: [PATCH v4 0/4] Introduce diff.submodule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-15T16:33:57Z","receivedAt":"2012-11-15T16:33:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 15, 2012 at 08:25:26AM -0800, Jeff King wrote:\n\n> > Thanks, this version looks good to me.\n> \n> Oh wait. I did not look closely enough. The point was to move the option\n> parser _out_ of git_diff_ui_config into git_diff_basic_config, so that\n> it only triggers for porcelain, not plumbing.\n\nOh no, I am having a muddled morning. It _is_ right. We want to move it\nout of basic into ui. Sorry for the noise. I just woke up and am\nthinking backwards.\n\nIt may be worth squashing this test into patch 3:\n\ndiff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh\nindex e401814..023439f 100755\n--- a/t/t4041-diff-submodule-option.sh\n+++ b/t/t4041-diff-submodule-option.sh\n@@ -72,6 +72,21 @@ EOF\n \ttest_cmp expected actual\n \"\n \n+test_expect_success 'diff.submodule does not affect plumbing' '\n+\ttest_config diff.submodule log &&\n+\tgit diff-index -p HEAD >actual &&\n+\tcat >expected <<-EOF &&\n+\tdiff --git a/sm1 b/sm1\n+\tnew file mode 160000\n+\tindex 0000000..a2c4dab\n+\t--- /dev/null\n+\t+++ b/sm1\n+\t@@ -0,0 +1 @@\n+\t+Subproject commit $fullhead1\n+\tEOF\n+\ttest_cmp expected actual\n+'\n+\n commit_file sm1 &&\n head2=$(add_file sm1 foo3)\n \n\nBTW, while writing the test, I noticed two minor nits with your tests:\n\n  1. They can use test_config, which is simpler (you do not need to\n     unset yourself after the test) and safer (the unset happens via\n     test_when_finished, so it works even if the test fails).\n\n  2. You can still indent expected output when using <<-.\n\nI don't know if it is worth re-rolling for them.\n\n-Peff\n"},{"id":"203336","messageId":"CALkWK0k65oyz1FWdyjjHLjj-DS75cdfk_8Vd3NV4OwzS+--VaA@mail.gmail.com","threadId":"32100","inReplyTo":"20121115163356.GF6157@sigill.intra.peff.net","subject":"Re: [PATCH v4 0/4] Introduce diff.submodule","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-16T10:45:02Z","receivedAt":"2012-11-16T10:45:02Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> It may be worth squashing this test into patch 3:\n\nLooks good.  Thanks.\n\n> BTW, while writing the test, I noticed two minor nits with your tests:\n>\n>   1. They can use test_config, which is simpler (you do not need to\n>      unset yourself after the test) and safer (the unset happens via\n>      test_when_finished, so it works even if the test fails).\n\nI see.  Can you squash this in?\n\n-- 8< --\ndiff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh\nindex e401814..876800f 100755\n--- a/t/t4041-diff-submodule-option.sh\n+++ b/t/t4041-diff-submodule-option.sh\n@@ -56,7 +56,7 @@ EOF\n \"\n\n test_expect_success '--submodule=short overrides diff.submodule' \"\n-\tgit config diff.submodule log &&\n+\ttest_config diff.submodule log &&\n \tgit add sm1 &&\n \tgit diff --submodule=short --cached >actual &&\n \tcat >expected <<-EOF &&\n@@ -68,7 +68,6 @@ index 0000000..a2c4dab\n @@ -0,0 +1 @@\n +Subproject commit $fullhead1\n EOF\n-\tgit config --unset diff.submodule &&\n \ttest_cmp expected actual\n \"\n\n>   2. You can still indent expected output when using <<-.\n\nI know;  however, I wanted to be consistent with the surrounding code.\n\nRam\n"},{"id":"203532","messageId":"7vk3tht7yz.fsf@alter.siamese.dyndns.org","threadId":"32100","inReplyTo":"1352821367-3611-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v4 2/4] diff: introduce diff.submodule configuration variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-20T00:48:36Z","receivedAt":"2012-11-20T00:48:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh\n> index 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\nThat looks like quite a roundabout way to say\n\n\t$(cd sm1; git rev-parse --verify HEAD)\n\ndoesn't it?  I know this is just moved code from the original, so I\nam not saying this should be fixed in this commit.\n\nExisting code in t7401 may want to be cleaned up, perhaps after this\ntopic settles.  The add_file() function, for example, has at least\nthese points:\n\n - do we know 7 hexdigits is always the right precision?\n - what happens when it fails to create a commit in one of the steps\n   while looping over \"$@\" in its loop?\n - the function uses the \"cd there and then come back\", not\n   \"go there in a subshell and do whatever it needs to\" pattern.\n\n> +test_expect_success 'added submodule, set diff.submodule' \"\n\ns/added/add/;\n\nShouldn't it test the base case where no diff.submodule is set?  For\nthose people, the patch should not change the output when they do or\ndo not pass --submodule option, right?\n\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"},{"id":"203630","messageId":"CALkWK0kZ0p4ptmAySXKspU2qJm9gm6Wg4uyBhRnXQ4qdSC34FA@mail.gmail.com","threadId":"32100","inReplyTo":"7vk3tht7yz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 2/4] diff: introduce diff.submodule configuration variable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-22T09:40:06Z","receivedAt":"2012-11-22T09:40:06Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>\n>> diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh\n>> index 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> That looks like quite a roundabout way to say\n>\n>         $(cd sm1; git rev-parse --verify HEAD)\n>\n> doesn't it?  I know this is just moved code from the original, so I\n> am not saying this should be fixed in this commit.\n\nExactly what I was thinking.\n\n> Existing code in t7401 may want to be cleaned up, perhaps after this\n> topic settles.  The add_file() function, for example, has at least\n> these points:\n>\n>  - do we know 7 hexdigits is always the right precision?\n>  - what happens when it fails to create a commit in one of the steps\n>    while looping over \"$@\" in its loop?\n>  - the function uses the \"cd there and then come back\", not\n>    \"go there in a subshell and do whatever it needs to\" pattern.\n\nOkay, will do.\n\n>> +test_expect_success 'added submodule, set diff.submodule' \"\n>\n> s/added/add/;\n\nI see that the topic is already in `next`.  Do you want to fix it up there?\n\n> Shouldn't it test the base case where no diff.submodule is set?  For\n> those people, the patch should not change the output when they do or\n> do not pass --submodule option, right?\n\nWhen no diff.submodule is set, `git diff --submodule` should just work\nas before; isn't this tested by the other tests in the file?\n\nRam\n"},{"id":"203642","messageId":"CALkWK0nxcGrpsK__Jc-Q+Y9HDP4pVx6ySrDGgS+4DxO2Rd+xBQ@mail.gmail.com","threadId":"32100","inReplyTo":"CALkWK0kZ0p4ptmAySXKspU2qJm9gm6Wg4uyBhRnXQ4qdSC34FA@mail.gmail.com","subject":"Re: [PATCH v4 2/4] diff: introduce diff.submodule configuration variable","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-22T09:44:18Z","receivedAt":"2012-11-22T09:44:18Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Junio C Hamano wrote:\n>> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>>> +test_expect_success 'added submodule, set diff.submodule' \"\n>>\n>> s/added/add/;\n>\n> I see that the topic is already in `next`.  Do you want to fix it up there?\n\nI was simply following the example set by the previous test in the\nfile: \"added submodule\".  I can fix up these two after the topic\ngraduates to `master`, if you prefer that, instead of rewriting `next`\nfor a small change like this.\n\nRam\n"}]}