{"thread":{"id":"31993","subject":"[PATCH v2 0/3] Introduce diff.submodule","startedAt":"2012-11-01T10:43:47Z","lastAt":"2012-11-11T15:09:22Z","messageCount":10,"participants":["Ramkumar Ramachandra","Jens Lehmann","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"202360","messageId":"1351766630-4837-1-git-send-email-artagnon@gmail.com","threadId":"31993","inReplyTo":null,"subject":"[PATCH v2 0/3] Introduce diff.submodule","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-01T10:43:47Z","receivedAt":"2012-11-01T10:43:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nv1 is here:\nhttp://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com\n\nI've fixed the issues pointed out in the review by Jens.\n\nSorry about the delay; need to find a new dayjob.\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 builtin/diff.c                   |    4 ++++\n cache.h                          |    1 +\n diff.c                           |    7 ++++++-\n submodule.c                      |    8 ++++----\n submodule.h                      |    2 +-\n t/t4041-diff-submodule-option.sh |   30 +++++++++++++++++++++++++++++-\n 9 files changed, 60 insertions(+), 14 deletions(-)\n\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"202361","messageId":"1351766630-4837-2-git-send-email-artagnon@gmail.com","threadId":"31993","inReplyTo":"1351766630-4837-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-01T10:43:48Z","receivedAt":"2012-11-01T10:43:48Z","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 11f320b..d1de857 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":"202362","messageId":"1351766630-4837-3-git-send-email-artagnon@gmail.com","threadId":"31993","inReplyTo":"1351766630-4837-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-01T10:43:49Z","receivedAt":"2012-11-01T10:43:49Z","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 builtin/diff.c                   |    4 ++++\n cache.h                          |    1 +\n diff.c                           |    5 +++++\n t/t4041-diff-submodule-option.sh |   30 +++++++++++++++++++++++++++++-\n 6 files changed, 48 insertions(+), 2 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/builtin/diff.c b/builtin/diff.c\nindex 9650be2..6d00311 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \tDIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n \tDIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);\n \n+\t/* Set SUBMODULE_LOG if diff.submodule config var was set */\n+\tif (submodule_format_cfg && !strcmp(submodule_format_cfg, \"log\"))\n+\t\tDIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);\n+\n \tif (nongit)\n \t\tdie(_(\"Not a git repository\"));\n \targc = setup_revisions(argc, argv, &rev, NULL);\ndiff --git a/cache.h b/cache.h\nindex a58df84..5fc7ba3 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1220,6 +1220,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 86e5f2a..835eb26 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -29,6 +29,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@@ -168,6 +169,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)\n \t\tdiff_stat_graph_width = git_config_int(var, value);\n \t\treturn 0;\n \t}\n+\tif (!strcmp(var, \"diff.submodule\"))\n+\t\treturn git_config_string(&submodule_format_cfg, var, value);\n \tif (!strcmp(var, \"diff.external\"))\n \t\treturn git_config_string(&external_diff_cmd_cfg, var, value);\n \tif (!strcmp(var, \"diff.wordregex\"))\n@@ -3656,6 +3659,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \telse if (!prefixcmp(arg, \"--submodule=\")) {\n \t\tif (!strcmp(arg + 12, \"log\"))\n \t\t\tDIFF_OPT_SET(options, SUBMODULE_LOG);\n+\t\tif (!strcmp(arg + 12, \"short\"))\n+\t\t\tDIFF_OPT_CLR(options, SUBMODULE_LOG);\n \t}\n \n \t/* misc options */\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":"202363","messageId":"1351766630-4837-4-git-send-email-artagnon@gmail.com","threadId":"31993","inReplyTo":"1351766630-4837-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-01T10:43:50Z","receivedAt":"2012-11-01T10:43:50Z","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 835eb26..115f384 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2244,7 +2244,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":"202379","messageId":"5092E535.3010701@web.de","threadId":"31993","inReplyTo":"1351766630-4837-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v2 0/3] Introduce diff.submodule","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2012-11-01T21:10:13Z","receivedAt":"2012-11-01T21:10:13Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 01.11.2012 11:43, schrieb Ramkumar Ramachandra:\n> Hi,\n> \n> v1 is here:\n> http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com\n> \n> I've fixed the issues pointed out in the review by Jens.\n\nThanks, looking good to me.\n"},{"id":"202520","messageId":"CALkWK0kdMbiTUYcSmY1OZOt6fjLTesi9y3S2LZvagjhu-0fn4g@mail.gmail.com","threadId":"31993","inReplyTo":"5092E535.3010701@web.de","subject":"Re: [PATCH v2 0/3] Introduce diff.submodule","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-11-04T17:58:17Z","receivedAt":"2012-11-04T17:58:17Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jens Lehmann wrote:\n> Am 01.11.2012 11:43, schrieb Ramkumar Ramachandra:\n>> Hi,\n>>\n>> v1 is here:\n>> http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com\n>>\n>> I've fixed the issues pointed out in the review by Jens.\n>\n> Thanks, looking good to me.\n\nPeff, can we pick this up?\n\nRam\n"},{"id":"202669","messageId":"20121108204312.GA8376@sigill.intra.peff.net","threadId":"31993","inReplyTo":"CALkWK0kdMbiTUYcSmY1OZOt6fjLTesi9y3S2LZvagjhu-0fn4g@mail.gmail.com","subject":"Re: [PATCH v2 0/3] Introduce diff.submodule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-08T20:43:12Z","receivedAt":"2012-11-08T20:43:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 04, 2012 at 11:28:17PM +0530, Ramkumar Ramachandra wrote:\n\n> Jens Lehmann wrote:\n> > Am 01.11.2012 11:43, schrieb Ramkumar Ramachandra:\n> >> Hi,\n> >>\n> >> v1 is here:\n> >> http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com\n> >>\n> >> I've fixed the issues pointed out in the review by Jens.\n> >\n> > Thanks, looking good to me.\n> \n> Peff, can we pick this up?\n\nThanks for prodding, I missed the original. I have a few comments, but\nI'll respond directly.\n\nAlso, I was coincidentally looking at the same code today. You might\nfind this interesting:\n\n  http://article.gmane.org/gmane.comp.version-control.git/209188\n\n-Peff\n"},{"id":"202670","messageId":"20121108205110.GB8376@sigill.intra.peff.net","threadId":"31993","inReplyTo":"1351766630-4837-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-08T20:51:10Z","receivedAt":"2012-11-08T20:51:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 01, 2012 at 04:13:49PM +0530, Ramkumar Ramachandra wrote:\n\n> diff --git a/builtin/diff.c b/builtin/diff.c\n> index 9650be2..6d00311 100644\n> --- a/builtin/diff.c\n> +++ b/builtin/diff.c\n> @@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>  \tDIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n>  \tDIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);\n>  \n> +\t/* Set SUBMODULE_LOG if diff.submodule config var was set */\n> +\tif (submodule_format_cfg && !strcmp(submodule_format_cfg, \"log\"))\n> +\t\tDIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);\n> +\n\nYuck. Why is this parsing happening in cmd_diff?\n\nWouldn't you want it to kick in for \"git log --submodule\", too? It seems\nlike it should go into diff_setup(), and the porcelain/plumbing aspect\nshould be controlled by parsing it in git_diff_ui_config or\ngit_diff_basic_config. See how diff_no_prefix and diff_mnemonic prefix\nare handled for an example.\n\nThen you can keep the parsing logic for \"log\" in diff.c. And you should\nfactor it out into a function so that the command-line option and the\nconfig parser can share the same code. I know it's only one line now,\nbut anybody who wants to add an option will have to update both places.\nSee the parsing of diff.dirstat for an example.\n\n>  \telse if (!prefixcmp(arg, \"--submodule=\")) {\n>  \t\tif (!strcmp(arg + 12, \"log\"))\n>  \t\t\tDIFF_OPT_SET(options, SUBMODULE_LOG);\n> +\t\tif (!strcmp(arg + 12, \"short\"))\n> +\t\t\tDIFF_OPT_CLR(options, SUBMODULE_LOG);\n>  \t}\n\nMuch better (although arguably should go in a separate patch). Should we\nalso produce an error if somebody says \"--submodule=foobar\"?\n\n-Peff\n"},{"id":"202864","messageId":"CALkWK0=zTCXki2c=ugRXE485ps2=OWag7mdzVJW93cnsypwkiA@mail.gmail.com","threadId":"31993","inReplyTo":"20121108205110.GB8376@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-11T14:50:27Z","receivedAt":"2012-11-11T14:50:27Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> On Thu, Nov 01, 2012 at 04:13:49PM +0530, Ramkumar Ramachandra wrote:\n>\n>> diff --git a/builtin/diff.c b/builtin/diff.c\n>> index 9650be2..6d00311 100644\n>> --- a/builtin/diff.c\n>> +++ b/builtin/diff.c\n>> @@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n>>       DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n>>       DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);\n>>\n>> +     /* Set SUBMODULE_LOG if diff.submodule config var was set */\n>> +     if (submodule_format_cfg && !strcmp(submodule_format_cfg, \"log\"))\n>> +             DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);\n>> +\n>\n> Yuck. Why is this parsing happening in cmd_diff?\n\nBlame Jens- see this thread |\nhttp://thread.gmane.org/gmane.comp.version-control.git/206816/focus=206815\n\n> Then you can keep the parsing logic for \"log\" in diff.c. And you should\n> factor it out into a function so that the command-line option and the\n> config parser can share the same code. I know it's only one line now,\n> but anybody who wants to add an option will have to update both places.\n> See the parsing of diff.dirstat for an example.\n\nDone.  I'll send out the new series shortly.\n\nRam\n"},{"id":"202867","messageId":"20121111150922.GA10140@sigill.intra.peff.net","threadId":"31993","inReplyTo":"CALkWK0=zTCXki2c=ugRXE485ps2=OWag7mdzVJW93cnsypwkiA@mail.gmail.com","subject":"Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-11-11T15:09:22Z","receivedAt":"2012-11-11T15:09:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 11, 2012 at 08:20:27PM +0530, Ramkumar Ramachandra wrote:\n\n> >> diff --git a/builtin/diff.c b/builtin/diff.c\n> >> index 9650be2..6d00311 100644\n> >> --- a/builtin/diff.c\n> >> +++ b/builtin/diff.c\n> >> @@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n> >>       DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);\n> >>       DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);\n> >>\n> >> +     /* Set SUBMODULE_LOG if diff.submodule config var was set */\n> >> +     if (submodule_format_cfg && !strcmp(submodule_format_cfg, \"log\"))\n> >> +             DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);\n> >> +\n> >\n> > Yuck. Why is this parsing happening in cmd_diff?\n> \n> Blame Jens- see this thread |\n> http://thread.gmane.org/gmane.comp.version-control.git/206816/focus=206815\n\nI don't think that is the right path, as at means that the option can\nonly ever affect diff, not other porcelains. I was thinking something\nmore like this (completely untested):\n\ndiff --git a/diff.c b/diff.c\nindex e89a201..74f4fc6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -37,6 +37,13 @@ static int diff_stat_graph_width;\n static int diff_dirstat_permille_default = 30;\n static struct diff_options default_diff_options;\n \n+/*\n+ * 0 for \"short\", 1 for \"log\". This should probably just be an enum, and\n+ * SUBMODULE_LOG lifted up from being a bit in the options to being its own\n+ * struct member.\n+ */\n+static int diff_submodule_default;\n+\n static char diff_colors[][COLOR_MAXLEN] = {\n \tGIT_COLOR_RESET,\n \tGIT_COLOR_NORMAL,\t/* PLAIN */\n@@ -178,6 +185,19 @@ 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\t/* XXX this should be factored out from the command-line parser */\n+\t\tif (!value)\n+\t\t\treturn config_error_nonbool(var);\n+\t\telse if (!strcmp(var, \"short\"))\n+\t\t\tdiff_submodule_default = 0;\n+\t\telse if (!strcmp(var, \"log\"))\n+\t\t\tdiff_submodule_default = 1;\n+\t\telse\n+\t\t\treturn error(\"unknown %s value: %s\", var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (git_color_config(var, value, cb) < 0)\n \t\treturn -1;\n \n@@ -3193,6 +3213,9 @@ void diff_setup(struct diff_options *options)\n \t\toptions->a_prefix = \"a/\";\n \t\toptions->b_prefix = \"b/\";\n \t}\n+\n+\tif (diff_submodule_default)\n+\t\tDIFF_OPT_SET(options, SUBMODULE_LOG);\n }\n \n void diff_setup_done(struct diff_options *options)\n"}]}