# [PATCH 1/5] Documentation: move diff.wordRegex from config.txt to diff-config.txt 14 messages from 2012-10-02 to 2012-10-30. Participants: Ramkumar Ramachandra, Jens Lehmann. Thread: https://gitlist.dev/t/31716 ## Ramkumar Ramachandra, 2012-10-02 16:51 Subject: [PATCH 0/5] submodule: introduce diff.submoduleFormat Message-ID: <1349196670-2844-1-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1349196670-2844-1-git-send-email-artagnon%40gmail.com ``` Hi, The fourth patch is the crux of the series. The first two patches clear up config.txt, diff-config.txt for the fourth patch. The third patch turned out to be a prerequisite. And the fifth patch is a bonus. Thanks for reading. Ram Ramkumar Ramachandra (5): Documentation: move diff.wordRegex from config.txt to diff-config.txt Documentation: sort diff-config.txt alphabetically diff: acknowledge --submodule=short command-line option diff: introduce diff.submoduleFormat configuration variable submodule: display summary header in bold Documentation/config.txt | 6 ------ Documentation/diff-config.txt | 21 +++++++++++++++++---- Documentation/diff-options.txt | 3 ++- diff.c | 29 +++++++++++++++++++---------- diff.h | 17 +++++++++-------- submodule.c | 8 ++++---- submodule.h | 2 +- t/t4041-diff-submodule-option.sh | 10 ++++++++++ 8 files changed, 62 insertions(+), 34 deletions(-) -- 1.7.8.1.362.g5d6df.dirty ``` ## Ramkumar Ramachandra, 2012-10-02 16:51 Subject: [PATCH 1/5] Documentation: move diff.wordRegex from config.txt to diff-config.txt Message-ID: <1349196670-2844-2-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1349196670-2844-2-git-send-email-artagnon%40gmail.com In-Reply-To: <1349196670-2844-1-git-send-email-artagnon@gmail.com> ``` 19299a8 (Documentation: Move diff..* from config.txt to diff-config.txt, 2011-04-07) moved the diff configuration options to diff-config.txt, but forgot about diff.wordRegex, which was left behind in config.txt. Fix this. Signed-off-by: Ramkumar Ramachandra --- Documentation/config.txt | 6 ------ Documentation/diff-config.txt | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 11f320b..d1de857 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -962,12 +962,6 @@ difftool..cmd:: difftool.prompt:: Prompt before each invocation of the diff tool. -diff.wordRegex:: - A POSIX Extended Regular Expression used to determine what is a "word" - when performing word-by-word difference calculations. Character - sequences that match the regular expression are "words", all other - characters are *ignorable* whitespace. - fetch.recurseSubmodules:: This option can be either set to a boolean value or to 'on-demand'. Setting it to a boolean changes the behavior of fetch and pull to diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt index 67a90a8..c2b94f9 100644 --- a/Documentation/diff-config.txt +++ b/Documentation/diff-config.txt @@ -103,6 +103,12 @@ diff.suppressBlankEmpty:: A boolean to inhibit the standard behavior of printing a space before each empty output line. Defaults to false. +diff.wordRegex:: + A POSIX Extended Regular Expression used to determine what is a "word" + when performing word-by-word difference calculations. Character + sequences that match the regular expression are "words", all other + characters are *ignorable* whitespace. + diff..command:: The custom diff driver command. See linkgit:gitattributes[5] for details. -- 1.7.8.1.362.g5d6df.dirty ``` ## Ramkumar Ramachandra, 2012-10-02 16:51 Subject: [PATCH 2/5] Documentation: sort diff-config.txt alphabetically Message-ID: <1349196670-2844-3-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1349196670-2844-3-git-send-email-artagnon%40gmail.com In-Reply-To: <1349196670-2844-1-git-send-email-artagnon@gmail.com> ``` df44483a (diff --stat: add config option to limit graph width, 2012-03-01) added the option diff.statGraphWidth, but did not place it in its appropriate lexical place in diff-config.txt. Fix this. Signed-off-by: Ramkumar Ramachandra --- Documentation/diff-config.txt | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt index c2b94f9..04574f9 100644 --- a/Documentation/diff-config.txt +++ b/Documentation/diff-config.txt @@ -52,10 +52,6 @@ directories with less than 10% of the total amount of changed files, and accumulating child directory counts in the parent directories: `files,10,cumulative`. -diff.statGraphWidth:: - Limit the width of the graph part in --stat output. If set, applies - to all commands generating --stat output except format-patch. - diff.external:: If this config variable is set, diff generation is not performed using the internal diff machinery, but using the @@ -99,6 +95,10 @@ diff.renames:: will enable basic rename detection. If set to "copies" or "copy", it will detect copies, as well. +diff.statGraphWidth:: + Limit the width of the graph part in --stat output. If set, applies + to all commands generating --stat output except format-patch. + diff.suppressBlankEmpty:: A boolean to inhibit the standard behavior of printing a space before each empty output line. Defaults to false. -- 1.7.8.1.362.g5d6df.dirty ``` ## Ramkumar Ramachandra, 2012-10-02 16:51 Subject: [PATCH 3/5] diff: acknowledge --submodule=short command-line option Message-ID: <1349196670-2844-4-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1349196670-2844-4-git-send-email-artagnon%40gmail.com In-Reply-To: <1349196670-2844-1-git-send-email-artagnon@gmail.com> ``` Currently, the diff code does not differentiate between an explicit '--submodule=short' being passed, and no submodule option being passed on the command line. Making this differentiation will be important when the command-line option can be used to override a "diff.submoduleFormat" configuration variable introduced in the next patch. Signed-off-by: Ramkumar Ramachandra --- diff.c | 4 +++- diff.h | 17 +++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index 35d3f07..8ea40f9 100644 --- a/diff.c +++ b/diff.c @@ -3647,7 +3647,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac) } else if (!strcmp(arg, "--submodule")) DIFF_OPT_SET(options, SUBMODULE_LOG); else if (!prefixcmp(arg, "--submodule=")) { - if (!strcmp(arg + 12, "log")) + if (!strcmp(arg + 12, "short")) + DIFF_OPT_SET(options, SUBMODULE_SHORT); + else if (!strcmp(arg + 12, "log")) DIFF_OPT_SET(options, SUBMODULE_LOG); } diff --git a/diff.h b/diff.h index a658f85..4115b49 100644 --- a/diff.h +++ b/diff.h @@ -77,14 +77,15 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data) #define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20) #define DIFF_OPT_ALLOW_TEXTCONV (1 << 21) #define DIFF_OPT_DIFF_FROM_CONTENTS (1 << 22) -#define DIFF_OPT_SUBMODULE_LOG (1 << 23) -#define DIFF_OPT_DIRTY_SUBMODULES (1 << 24) -#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25) -#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26) -#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27) -#define DIFF_OPT_DIRSTAT_BY_LINE (1 << 28) -#define DIFF_OPT_FUNCCONTEXT (1 << 29) -#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30) +#define DIFF_OPT_SUBMODULE_SHORT (1 << 23) +#define DIFF_OPT_SUBMODULE_LOG (1 << 24) +#define DIFF_OPT_DIRTY_SUBMODULES (1 << 25) +#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 26) +#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 27) +#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 28) +#define DIFF_OPT_DIRSTAT_BY_LINE (1 << 29) +#define DIFF_OPT_FUNCCONTEXT (1 << 30) +#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 31) #define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag) #define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag) -- 1.7.8.1.362.g5d6df.dirty ``` ## Ramkumar Ramachandra, 2012-10-02 16:51 Subject: [PATCH 4/5] diff: introduce diff.submoduleFormat configuration variable Message-ID: <1349196670-2844-5-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1349196670-2844-5-git-send-email-artagnon%40gmail.com In-Reply-To: <1349196670-2844-1-git-send-email-artagnon@gmail.com> ``` Introduce a diff.submoduleFormat configuration variable corresponding to the '--submodule' command-line option of 'git diff'. Signed-off-by: Ramkumar Ramachandra --- Documentation/diff-config.txt | 7 +++++++ Documentation/diff-options.txt | 3 ++- diff.c | 25 ++++++++++++++++--------- t/t4041-diff-submodule-option.sh | 10 ++++++++++ 4 files changed, 35 insertions(+), 10 deletions(-) diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt index 04574f9..e445cc8 100644 --- a/Documentation/diff-config.txt +++ b/Documentation/diff-config.txt @@ -103,6 +103,13 @@ diff.suppressBlankEmpty:: A boolean to inhibit the standard behavior of printing a space before each empty output line. Defaults to false. +diff.submoduleFormat:: + 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..034c4e7 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.submoduleFormat` configuration variable. --color[=]:: Show colored diff. diff --git a/diff.c b/diff.c index 8ea40f9..4cb8dd2 100644 --- a/diff.c +++ b/diff.c @@ -28,6 +28,7 @@ static int diff_suppress_blank_empty; static int diff_use_color_default = -1; static const char *diff_word_regex_cfg; static const char *external_diff_cmd_cfg; +static const char *submodule_format_cfg; int diff_auto_refresh_index = 1; static int diff_mnemonic_prefix; static int diff_no_prefix; @@ -161,6 +162,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.submoduleformat")) + 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")) @@ -2227,15 +2230,19 @@ static void builtin_diff(const char *name_a, line_prefix = msgbuf->buf; } - if (DIFF_OPT_TST(o, SUBMODULE_LOG) && - (!one->mode || S_ISGITLINK(one->mode)) && - (!two->mode || S_ISGITLINK(two->mode))) { - const char *del = diff_get_color_opt(o, DIFF_FILE_OLD); - const char *add = diff_get_color_opt(o, DIFF_FILE_NEW); - show_submodule_summary(o->file, one ? one->path : two->path, - one->sha1, two->sha1, two->dirty_submodule, - del, add, reset); - return; + if (!DIFF_OPT_TST(o, SUBMODULE_SHORT) && + ((!one->mode || S_ISGITLINK(one->mode)) && + (!two->mode || S_ISGITLINK(two->mode)))) { + if (DIFF_OPT_TST(o, SUBMODULE_LOG) || + (submodule_format_cfg && + !strcmp(submodule_format_cfg, "log"))) { + const char *del = diff_get_color_opt(o, DIFF_FILE_OLD); + const char *add = diff_get_color_opt(o, DIFF_FILE_NEW); + show_submodule_summary(o->file, one ? one->path : two->path, + one->sha1, two->sha1, two->dirty_submodule, + del, add, reset); + return; + } } if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh index 6c01d0c..ed4f3a8 100755 --- a/t/t4041-diff-submodule-option.sh +++ b/t/t4041-diff-submodule-option.sh @@ -43,6 +43,16 @@ EOF test_cmp expected actual " +test_expect_success 'added submodule, set diff.submoduleFormat' " + git config diff.submoduleFormat log && + git add sm1 && + git diff --cached >actual && + cat >expected <<-EOF && +Submodule sm1 0000000...$head1 (new submodule) +EOF + test_cmp expected actual +" + commit_file sm1 && head2=$(add_file sm1 foo3) -- 1.7.8.1.362.g5d6df.dirty ``` ## Ramkumar Ramachandra, 2012-10-02 16:51 Subject: [PATCH 5/5] submodule: display summary header in bold Message-ID: <1349196670-2844-6-git-send-email-artagnon@gmail.com> URL: https://gitlist.dev/e/1349196670-2844-6-git-send-email-artagnon%40gmail.com In-Reply-To: <1349196670-2844-1-git-send-email-artagnon@gmail.com> ``` Currently, 'git diff --submodule' displays output with a bold diff header for non-submodules. So this part is in bold: diff --git a/file1 b/file1 index 30b2f6c..2638038 100644 --- a/file1 +++ b/file1 For submodules, the header looks like this: Submodule submodule1 012b072..248d0fd: Unfortunately, it's easy to miss in the output because it's not bold. Change this. Signed-off-by: Ramkumar Ramachandra --- diff.c | 2 +- submodule.c | 8 ++++---- submodule.h | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/diff.c b/diff.c index 4cb8dd2..cf369c5 100644 --- a/diff.c +++ b/diff.c @@ -2240,7 +2240,7 @@ static void builtin_diff(const char *name_a, const char *add = diff_get_color_opt(o, DIFF_FILE_NEW); show_submodule_summary(o->file, one ? one->path : two->path, one->sha1, two->sha1, two->dirty_submodule, - del, add, reset); + set, del, add, reset); return; } } diff --git a/submodule.c b/submodule.c index 50f213e..be60e89 100644 --- a/submodule.c +++ b/submodule.c @@ -258,7 +258,7 @@ int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg) void show_submodule_summary(FILE *f, const char *path, unsigned char one[20], unsigned char two[20], - unsigned dirty_submodule, + unsigned dirty_submodule, const char *set, const char *del, const char *add, const char *reset) { struct rev_info rev; @@ -292,15 +292,15 @@ void show_submodule_summary(FILE *f, const char *path, return; } - strbuf_addf(&sb, "Submodule %s %s..", path, + strbuf_addf(&sb, "%sSubmodule %s %s..", set, path, find_unique_abbrev(one, DEFAULT_ABBREV)); if (!fast_backward && !fast_forward) strbuf_addch(&sb, '.'); strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV)); if (message) - strbuf_addf(&sb, " %s\n", message); + strbuf_addf(&sb, " %s%s\n", message, reset); else - strbuf_addf(&sb, "%s:\n", fast_backward ? " (rewind)" : ""); + strbuf_addf(&sb, "%s:%s\n", fast_backward ? " (rewind)" : "", reset); fwrite(sb.buf, sb.len, 1, f); if (!message) { diff --git a/submodule.h b/submodule.h index 594b50d..ef5aab6 100644 --- a/submodule.h +++ b/submodule.h @@ -20,7 +20,7 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *); int parse_fetch_recurse_submodules_arg(const char *opt, const char *arg); void show_submodule_summary(FILE *f, const char *path, unsigned char one[20], unsigned char two[20], - unsigned dirty_submodule, + unsigned dirty_submodule, const char *set, const char *del, const char *add, const char *reset); void set_config_fetch_recurse_submodules(int value); void check_for_new_submodule_commits(unsigned char new_sha1[20]); -- 1.7.8.1.362.g5d6df.dirty ``` ## Jens Lehmann, 2012-10-02 19:24 Subject: Re: [PATCH 3/5] diff: acknowledge --submodule=short command-line option Message-ID: <506B3F74.3020208@web.de> URL: https://gitlist.dev/e/506B3F74.3020208%40web.de In-Reply-To: <1349196670-2844-4-git-send-email-artagnon@gmail.com> ``` Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra: > Currently, the diff code does not differentiate between an explicit > '--submodule=short' being passed, and no submodule option being passed > on the command line. Making this differentiation will be important > when the command-line option can be used to override a > "diff.submoduleFormat" configuration variable introduced in the next > patch. Wouldn't it be sufficient here to simply reset the log flag by using "DIFF_OPT_CLR(options, SUBMODULE_LOG)"? This would avoid having to use the last bit of the diffopt flags. And if I read the code correctly, diff_opt_parse() is called by setup_revisions() which is called after git_config(), so that should be safe. (And "textconv" uses the same approach) > Signed-off-by: Ramkumar Ramachandra > --- > diff.c | 4 +++- > diff.h | 17 +++++++++-------- > 2 files changed, 12 insertions(+), 9 deletions(-) > > diff --git a/diff.c b/diff.c > index 35d3f07..8ea40f9 100644 > --- a/diff.c > +++ b/diff.c > @@ -3647,7 +3647,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac) > } else if (!strcmp(arg, "--submodule")) > DIFF_OPT_SET(options, SUBMODULE_LOG); > else if (!prefixcmp(arg, "--submodule=")) { > - if (!strcmp(arg + 12, "log")) > + if (!strcmp(arg + 12, "short")) > + DIFF_OPT_SET(options, SUBMODULE_SHORT); > + else if (!strcmp(arg + 12, "log")) > DIFF_OPT_SET(options, SUBMODULE_LOG); > } > > diff --git a/diff.h b/diff.h > index a658f85..4115b49 100644 > --- a/diff.h > +++ b/diff.h > @@ -77,14 +77,15 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data) > #define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20) > #define DIFF_OPT_ALLOW_TEXTCONV (1 << 21) > #define DIFF_OPT_DIFF_FROM_CONTENTS (1 << 22) > -#define DIFF_OPT_SUBMODULE_LOG (1 << 23) > -#define DIFF_OPT_DIRTY_SUBMODULES (1 << 24) > -#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25) > -#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26) > -#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27) > -#define DIFF_OPT_DIRSTAT_BY_LINE (1 << 28) > -#define DIFF_OPT_FUNCCONTEXT (1 << 29) > -#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30) > +#define DIFF_OPT_SUBMODULE_SHORT (1 << 23) > +#define DIFF_OPT_SUBMODULE_LOG (1 << 24) > +#define DIFF_OPT_DIRTY_SUBMODULES (1 << 25) > +#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 26) > +#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 27) > +#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 28) > +#define DIFF_OPT_DIRSTAT_BY_LINE (1 << 29) > +#define DIFF_OPT_FUNCCONTEXT (1 << 30) > +#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 31) > > #define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag) > #define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag) > ``` ## Jens Lehmann, 2012-10-02 19:44 Subject: Re: [PATCH 4/5] diff: introduce diff.submoduleFormat configuration variable Message-ID: <506B4418.2050700@web.de> URL: https://gitlist.dev/e/506B4418.2050700%40web.de In-Reply-To: <1349196670-2844-5-git-send-email-artagnon@gmail.com> ``` Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra: > Introduce a diff.submoduleFormat configuration variable corresponding > to the '--submodule' command-line option of 'git diff'. Nice. Maybe a better name would be "diff.submodule", as this sets the default for the "--submodule" option of diff? And I think you should also test in t4041 that "--submodule=short" overrides the config setting. > Signed-off-by: Ramkumar Ramachandra > --- > Documentation/diff-config.txt | 7 +++++++ > Documentation/diff-options.txt | 3 ++- > diff.c | 25 ++++++++++++++++--------- > t/t4041-diff-submodule-option.sh | 10 ++++++++++ > 4 files changed, 35 insertions(+), 10 deletions(-) > > diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt > index 04574f9..e445cc8 100644 > --- a/Documentation/diff-config.txt > +++ b/Documentation/diff-config.txt > @@ -103,6 +103,13 @@ diff.suppressBlankEmpty:: > A boolean to inhibit the standard behavior of printing a space > before each empty output line. Defaults to false. > > +diff.submoduleFormat:: > + 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..034c4e7 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.submoduleFormat` configuration variable. > > --color[=]:: > Show colored diff. > diff --git a/diff.c b/diff.c > index 8ea40f9..4cb8dd2 100644 > --- a/diff.c > +++ b/diff.c > @@ -28,6 +28,7 @@ static int diff_suppress_blank_empty; > static int diff_use_color_default = -1; > static const char *diff_word_regex_cfg; > static const char *external_diff_cmd_cfg; > +static const char *submodule_format_cfg; > int diff_auto_refresh_index = 1; > static int diff_mnemonic_prefix; > static int diff_no_prefix; > @@ -161,6 +162,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.submoduleformat")) > + 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")) > @@ -2227,15 +2230,19 @@ static void builtin_diff(const char *name_a, > line_prefix = msgbuf->buf; > } > > - if (DIFF_OPT_TST(o, SUBMODULE_LOG) && > - (!one->mode || S_ISGITLINK(one->mode)) && > - (!two->mode || S_ISGITLINK(two->mode))) { > - const char *del = diff_get_color_opt(o, DIFF_FILE_OLD); > - const char *add = diff_get_color_opt(o, DIFF_FILE_NEW); > - show_submodule_summary(o->file, one ? one->path : two->path, > - one->sha1, two->sha1, two->dirty_submodule, > - del, add, reset); > - return; > + if (!DIFF_OPT_TST(o, SUBMODULE_SHORT) && > + ((!one->mode || S_ISGITLINK(one->mode)) && > + (!two->mode || S_ISGITLINK(two->mode)))) { > + if (DIFF_OPT_TST(o, SUBMODULE_LOG) || > + (submodule_format_cfg && > + !strcmp(submodule_format_cfg, "log"))) { > + const char *del = diff_get_color_opt(o, DIFF_FILE_OLD); > + const char *add = diff_get_color_opt(o, DIFF_FILE_NEW); > + show_submodule_summary(o->file, one ? one->path : two->path, > + one->sha1, two->sha1, two->dirty_submodule, > + del, add, reset); > + return; > + } > } > > if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) { > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh > index 6c01d0c..ed4f3a8 100755 > --- a/t/t4041-diff-submodule-option.sh > +++ b/t/t4041-diff-submodule-option.sh > @@ -43,6 +43,16 @@ EOF > test_cmp expected actual > " > > +test_expect_success 'added submodule, set diff.submoduleFormat' " > + git config diff.submoduleFormat log && > + git add sm1 && > + git diff --cached >actual && > + cat >expected <<-EOF && > +Submodule sm1 0000000...$head1 (new submodule) > +EOF > + test_cmp expected actual > +" > + > commit_file sm1 && > head2=$(add_file sm1 foo3) > > ``` ## Jens Lehmann, 2012-10-03 13:45 Subject: Re: [PATCH 4/5] diff: introduce diff.submoduleFormat configuration variable Message-ID: <506C4161.3040201@web.de> URL: https://gitlist.dev/e/506C4161.3040201%40web.de In-Reply-To: <506B4418.2050700@web.de> ``` Am 02.10.2012 21:44, schrieb Jens Lehmann: > Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra: >> Introduce a diff.submoduleFormat configuration variable corresponding >> to the '--submodule' command-line option of 'git diff'. > > Nice. Maybe a better name would be "diff.submodule", as this sets the > default for the "--submodule" option of diff? > > And I think you should also test in t4041 that "--submodule=short" > overrides the config setting. We also need tests which show that setting that config to "log" does not break one of the many users of "git diff" ("stash", "rebase" and "format-patch" come to mind, most probably I missed some others). I suspect we'll have to add "--submodule=short" options to some call sites to keep them working with submodule changes. ``` ## Ramkumar Ramachandra, 2012-10-07 15:22 Subject: Re: [PATCH 3/5] diff: acknowledge --submodule=short command-line option Message-ID: URL: https://gitlist.dev/e/CALkWK0mbQZjYUR73rdFP2BtOzdL9gQqxnC5fjtsCYq2F29%3D8qg%40mail.gmail.com In-Reply-To: <506B3F74.3020208@web.de> ``` Jens Lehmann wrote: > Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra: >> Currently, the diff code does not differentiate between an explicit >> '--submodule=short' being passed, and no submodule option being passed >> on the command line. Making this differentiation will be important >> when the command-line option can be used to override a >> "diff.submoduleFormat" configuration variable introduced in the next >> patch. > > Wouldn't it be sufficient here to simply reset the log flag by using > "DIFF_OPT_CLR(options, SUBMODULE_LOG)"? This would avoid having to > use the last bit of the diffopt flags. And if I read the code correctly, > diff_opt_parse() is called by setup_revisions() which is called after > git_config(), so that should be safe. (And "textconv" uses the same > approach) How is it sufficient? In git_diff_ui_config(), I set submodule_format_cfg, which has nothing to do with SUBMODULE_LOG. In builtin_diff(), I'll have to check SUBMODULE_LOG and submodule_format_cfg. The tricky bit is that I should check submodule_format_cfg if and only if "--submodule=short" was NOT passed on the command line- now, that's not the same thing is checking if SUBMODULE_LOG is unset, because SUBMODULE_LOG is unset (or cleared) if no argument was passed or if "--submodule=short" is passed. Therefore, I need a SUBMODULE_SHORT to differentiate between the two cases. What am I missing? Ram ``` ## Jens Lehmann, 2012-10-07 19:49 Subject: Re: [PATCH 3/5] diff: acknowledge --submodule=short command-line option Message-ID: <5071DCC8.50004@web.de> URL: https://gitlist.dev/e/5071DCC8.50004%40web.de In-Reply-To: ``` Am 07.10.2012 17:22, schrieb Ramkumar Ramachandra: > Jens Lehmann wrote: >> Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra: >>> Currently, the diff code does not differentiate between an explicit >>> '--submodule=short' being passed, and no submodule option being passed >>> on the command line. Making this differentiation will be important >>> when the command-line option can be used to override a >>> "diff.submoduleFormat" configuration variable introduced in the next >>> patch. >> >> Wouldn't it be sufficient here to simply reset the log flag by using >> "DIFF_OPT_CLR(options, SUBMODULE_LOG)"? This would avoid having to >> use the last bit of the diffopt flags. And if I read the code correctly, >> diff_opt_parse() is called by setup_revisions() which is called after >> git_config(), so that should be safe. (And "textconv" uses the same >> approach) > > How is it sufficient? In git_diff_ui_config(), I set > submodule_format_cfg, which has nothing to do with SUBMODULE_LOG. In > builtin_diff(), I'll have to check SUBMODULE_LOG and > submodule_format_cfg. The tricky bit is that I should check > submodule_format_cfg if and only if "--submodule=short" was NOT passed > on the command line- now, that's not the same thing is checking if > SUBMODULE_LOG is unset, because SUBMODULE_LOG is unset (or cleared) if > no argument was passed or if "--submodule=short" is passed. > Therefore, I need a SUBMODULE_SHORT to differentiate between the two > cases. > > What am I missing? I forgot to mention that testing submodule_format_cfg would have to happen in cmd_diff() (between reading the config and parsing the command line options) instead of builtin_diff(). Something like this should do the trick (untested): diff --git a/builtin/diff.c b/builtin/diff.c index 9650be2..180bf44 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -297,6 +297,9 @@ 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); + if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log")) + DIFF_OPT_SET(options, SUBMODULE_LOG); + if (nongit) die(_("Not a git repository")); argc = setup_revisions(argc, argv, &rev, NULL); ``` ## Jens Lehmann, 2012-10-07 19:55 Subject: Re: [PATCH 3/5] diff: acknowledge --submodule=short command-line option Message-ID: <5071DE3C.5080206@web.de> URL: https://gitlist.dev/e/5071DE3C.5080206%40web.de In-Reply-To: <5071DCC8.50004@web.de> ``` Am 07.10.2012 21:49, schrieb Jens Lehmann: > I forgot to mention that testing submodule_format_cfg would have to > happen in cmd_diff() (between reading the config and parsing the > command line options) instead of builtin_diff(). Something like this > should do the trick (untested): > > diff --git a/builtin/diff.c b/builtin/diff.c > index 9650be2..180bf44 100644 > --- a/builtin/diff.c > +++ b/builtin/diff.c > @@ -297,6 +297,9 @@ 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); > > + if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log")) > + DIFF_OPT_SET(options, SUBMODULE_LOG); Obviously not even compile tested, the line above should read: + DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG); > + > if (nongit) > die(_("Not a git repository")); > argc = setup_revisions(argc, argv, &rev, NULL); ``` ## Ramkumar Ramachandra, 2012-10-29 10:30 Subject: Re: [PATCH 4/5] diff: introduce diff.submoduleFormat configuration variable Message-ID: URL: https://gitlist.dev/e/CALkWK0nPJ-8UvyYN800-Tj%2BmLQtv-vF3SJScb1O8EEiY238Z6w%40mail.gmail.com In-Reply-To: <506C4161.3040201@web.de> ``` Jens Lehmann wrote: > Am 02.10.2012 21:44, schrieb Jens Lehmann: >> Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra: >>> Introduce a diff.submoduleFormat configuration variable corresponding >>> to the '--submodule' command-line option of 'git diff'. >> >> Nice. Maybe a better name would be "diff.submodule", as this sets the >> default for the "--submodule" option of diff? >> >> And I think you should also test in t4041 that "--submodule=short" >> overrides the config setting. > > We also need tests which show that setting that config to "log" does > not break one of the many users of "git diff" ("stash", "rebase" and > "format-patch" come to mind, most probably I missed some others). I > suspect we'll have to add "--submodule=short" options to some call > sites to keep them working with submodule changes. Um, why would "stash", "rebase" or "format-patch" be affected by this setting? They don't operate on submodules at all. To be sure, I ran all the tests with the following diff and nothing broke. Confused, Ram -- 8< -- diff --git a/t/test-lib.sh b/t/test-lib.sh index 514282c..904a81c 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -608,6 +608,8 @@ fi # in subprocesses like git equals our $PWD (for pathname comparisons). cd -P "$test" || exit 1 +git config test.submodule log + this_test=${0##*/} this_test=${this_test%%-*} for skp in $GIT_SKIP_TESTS ``` ## Jens Lehmann, 2012-10-30 21:26 Subject: Re: [PATCH 4/5] diff: introduce diff.submoduleFormat configuration variable Message-ID: <509045EF.9030401@web.de> URL: https://gitlist.dev/e/509045EF.9030401%40web.de In-Reply-To: ``` Am 29.10.2012 11:30, schrieb Ramkumar Ramachandra: > Jens Lehmann wrote: >> Am 02.10.2012 21:44, schrieb Jens Lehmann: >>> Am 02.10.2012 18:51, schrieb Ramkumar Ramachandra: >>>> Introduce a diff.submoduleFormat configuration variable corresponding >>>> to the '--submodule' command-line option of 'git diff'. >>> >>> Nice. Maybe a better name would be "diff.submodule", as this sets the >>> default for the "--submodule" option of diff? >>> >>> And I think you should also test in t4041 that "--submodule=short" >>> overrides the config setting. >> >> We also need tests which show that setting that config to "log" does >> not break one of the many users of "git diff" ("stash", "rebase" and >> "format-patch" come to mind, most probably I missed some others). I >> suspect we'll have to add "--submodule=short" options to some call >> sites to keep them working with submodule changes. > > Um, why would "stash", "rebase" or "format-patch" be affected by this > setting? They don't operate on submodules at all. To be sure, I ran > all the tests with the following diff and nothing broke. They do operate on the submodule commits too (while they don't touch submodule work trees) and IIRC rebase applies diffs, so that could break when the output of diff changes due to the new config option. But it looks like your test did prove that nothing goes wrong there, I assume they they use plumbing diff commands which aren't affected by the new option. ```