* [PATCH 1/3] Documentation: move diff.wordRegex from config.txt to diff-config.txt
2012-11-01 10:43 [PATCH v2 0/3] Introduce diff.submodule Ramkumar Ramachandra
@ 2012-11-01 10:43 ` Ramkumar Ramachandra
2012-11-01 10:43 ` [PATCH 2/3] diff: introduce diff.submodule configuration variable Ramkumar Ramachandra
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Ramkumar Ramachandra @ 2012-11-01 10:43 UTC (permalink / raw)
To: Git List; +Cc: Jens Lehmann
19299a8 (Documentation: Move diff.<driver>.* 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 <artagnon@gmail.com>
---
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.<tool>.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 75ab8a5..decd370 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -107,6 +107,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.<driver>.command::
The custom diff driver command. See linkgit:gitattributes[5]
for details.
--
1.7.8.1.362.g5d6df.dirty
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/3] diff: introduce diff.submodule configuration variable
2012-11-01 10:43 [PATCH v2 0/3] Introduce diff.submodule Ramkumar Ramachandra
2012-11-01 10:43 ` [PATCH 1/3] Documentation: move diff.wordRegex from config.txt to diff-config.txt Ramkumar Ramachandra
@ 2012-11-01 10:43 ` Ramkumar Ramachandra
2012-11-08 20:51 ` Jeff King
2012-11-01 10:43 ` [PATCH 3/3] submodule: display summary header in bold Ramkumar Ramachandra
2012-11-01 21:10 ` [PATCH v2 0/3] Introduce diff.submodule Jens Lehmann
3 siblings, 1 reply; 11+ messages in thread
From: Ramkumar Ramachandra @ 2012-11-01 10:43 UTC (permalink / raw)
To: Git List; +Cc: Jens Lehmann
Introduce a diff.submodule configuration variable corresponding to the
'--submodule' command-line option of 'git diff'.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
Documentation/diff-config.txt | 7 +++++++
Documentation/diff-options.txt | 3 ++-
builtin/diff.c | 4 ++++
cache.h | 1 +
diff.c | 5 +++++
t/t4041-diff-submodule-option.sh | 30 +++++++++++++++++++++++++++++-
6 files changed, 48 insertions(+), 2 deletions(-)
diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index decd370..89dd634 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -107,6 +107,13 @@ diff.suppressBlankEmpty::
A boolean to inhibit the standard behavior of printing a space
before each empty output line. Defaults to false.
+diff.submodule::
+ Specify the format in which differences in submodules are
+ shown. The "log" format lists the commits in the range like
+ linkgit:git-submodule[1] `summary` does. The "short" format
+ format just shows the names of the commits at the beginning
+ and end of the range. Defaults to short.
+
diff.wordRegex::
A POSIX Extended Regular Expression used to determine what is a "word"
when performing word-by-word difference calculations. Character
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index cf4b216..f4f7e25 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -170,7 +170,8 @@ any of those replacements occurred.
the commits in the range like linkgit:git-submodule[1] `summary` does.
Omitting the `--submodule` option or specifying `--submodule=short`,
uses the 'short' format. This format just shows the names of the commits
- at the beginning and end of the range.
+ at the beginning and end of the range. Can be tweaked via the
+ `diff.submodule` configuration variable.
--color[=<when>]::
Show colored diff.
diff --git a/builtin/diff.c b/builtin/diff.c
index 9650be2..6d00311 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
+ /* Set SUBMODULE_LOG if diff.submodule config var was set */
+ if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log"))
+ DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);
+
if (nongit)
die(_("Not a git repository"));
argc = setup_revisions(argc, argv, &rev, NULL);
diff --git a/cache.h b/cache.h
index a58df84..5fc7ba3 100644
--- a/cache.h
+++ b/cache.h
@@ -1220,6 +1220,7 @@ int add_files_to_cache(const char *prefix, const char **pathspec, int flags);
/* diff.c */
extern int diff_auto_refresh_index;
+extern const char *submodule_format_cfg;
/* match-trees.c */
void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, int);
diff --git a/diff.c b/diff.c
index 86e5f2a..835eb26 100644
--- a/diff.c
+++ b/diff.c
@@ -29,6 +29,7 @@ static int diff_use_color_default = -1;
static int diff_context_default = 3;
static const char *diff_word_regex_cfg;
static const char *external_diff_cmd_cfg;
+const char *submodule_format_cfg;
int diff_auto_refresh_index = 1;
static int diff_mnemonic_prefix;
static int diff_no_prefix;
@@ -168,6 +169,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
diff_stat_graph_width = git_config_int(var, value);
return 0;
}
+ if (!strcmp(var, "diff.submodule"))
+ return git_config_string(&submodule_format_cfg, var, value);
if (!strcmp(var, "diff.external"))
return git_config_string(&external_diff_cmd_cfg, var, value);
if (!strcmp(var, "diff.wordregex"))
@@ -3656,6 +3659,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
else if (!prefixcmp(arg, "--submodule=")) {
if (!strcmp(arg + 12, "log"))
DIFF_OPT_SET(options, SUBMODULE_LOG);
+ if (!strcmp(arg + 12, "short"))
+ DIFF_OPT_CLR(options, SUBMODULE_LOG);
}
/* misc options */
diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 6c01d0c..e401814 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -33,6 +33,7 @@ test_create_repo sm1 &&
add_file . foo >/dev/null
head1=$(add_file sm1 foo1 foo2)
+fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
test_expect_success 'added submodule' "
git add sm1 &&
@@ -43,6 +44,34 @@ EOF
test_cmp expected actual
"
+test_expect_success 'added submodule, set diff.submodule' "
+ git config diff.submodule log &&
+ git add sm1 &&
+ git diff --cached >actual &&
+ cat >expected <<-EOF &&
+Submodule sm1 0000000...$head1 (new submodule)
+EOF
+ git config --unset diff.submodule &&
+ test_cmp expected actual
+"
+
+test_expect_success '--submodule=short overrides diff.submodule' "
+ git config diff.submodule log &&
+ git add sm1 &&
+ git diff --submodule=short --cached >actual &&
+ cat >expected <<-EOF &&
+diff --git a/sm1 b/sm1
+new file mode 160000
+index 0000000..a2c4dab
+--- /dev/null
++++ b/sm1
+@@ -0,0 +1 @@
++Subproject commit $fullhead1
+EOF
+ git config --unset diff.submodule &&
+ test_cmp expected actual
+"
+
commit_file sm1 &&
head2=$(add_file sm1 foo3)
@@ -73,7 +102,6 @@ EOF
test_cmp expected actual
"
-fullhead1=$(cd sm1; git rev-list --max-count=1 $head1)
fullhead2=$(cd sm1; git rev-list --max-count=1 $head2)
test_expect_success 'modified submodule(forward) --submodule=short' "
git diff --submodule=short >actual &&
--
1.7.8.1.362.g5d6df.dirty
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
2012-11-01 10:43 ` [PATCH 2/3] diff: introduce diff.submodule configuration variable Ramkumar Ramachandra
@ 2012-11-08 20:51 ` Jeff King
2012-11-11 14:50 ` Ramkumar Ramachandra
0 siblings, 1 reply; 11+ messages in thread
From: Jeff King @ 2012-11-08 20:51 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Jens Lehmann
On Thu, Nov 01, 2012 at 04:13:49PM +0530, Ramkumar Ramachandra wrote:
> diff --git a/builtin/diff.c b/builtin/diff.c
> index 9650be2..6d00311 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
> DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
>
> + /* Set SUBMODULE_LOG if diff.submodule config var was set */
> + if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log"))
> + DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);
> +
Yuck. Why is this parsing happening in cmd_diff?
Wouldn't you want it to kick in for "git log --submodule", too? It seems
like it should go into diff_setup(), and the porcelain/plumbing aspect
should be controlled by parsing it in git_diff_ui_config or
git_diff_basic_config. See how diff_no_prefix and diff_mnemonic prefix
are handled for an example.
Then you can keep the parsing logic for "log" in diff.c. And you should
factor it out into a function so that the command-line option and the
config parser can share the same code. I know it's only one line now,
but anybody who wants to add an option will have to update both places.
See the parsing of diff.dirstat for an example.
> else if (!prefixcmp(arg, "--submodule=")) {
> if (!strcmp(arg + 12, "log"))
> DIFF_OPT_SET(options, SUBMODULE_LOG);
> + if (!strcmp(arg + 12, "short"))
> + DIFF_OPT_CLR(options, SUBMODULE_LOG);
> }
Much better (although arguably should go in a separate patch). Should we
also produce an error if somebody says "--submodule=foobar"?
-Peff
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
2012-11-08 20:51 ` Jeff King
@ 2012-11-11 14:50 ` Ramkumar Ramachandra
2012-11-11 15:09 ` Jeff King
0 siblings, 1 reply; 11+ messages in thread
From: Ramkumar Ramachandra @ 2012-11-11 14:50 UTC (permalink / raw)
To: Jeff King; +Cc: Git List, Jens Lehmann
Jeff King wrote:
> On Thu, Nov 01, 2012 at 04:13:49PM +0530, Ramkumar Ramachandra wrote:
>
>> diff --git a/builtin/diff.c b/builtin/diff.c
>> index 9650be2..6d00311 100644
>> --- a/builtin/diff.c
>> +++ b/builtin/diff.c
>> @@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>> DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
>> DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
>>
>> + /* Set SUBMODULE_LOG if diff.submodule config var was set */
>> + if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log"))
>> + DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);
>> +
>
> Yuck. Why is this parsing happening in cmd_diff?
Blame Jens- see this thread |
http://thread.gmane.org/gmane.comp.version-control.git/206816/focus=206815
> Then you can keep the parsing logic for "log" in diff.c. And you should
> factor it out into a function so that the command-line option and the
> config parser can share the same code. I know it's only one line now,
> but anybody who wants to add an option will have to update both places.
> See the parsing of diff.dirstat for an example.
Done. I'll send out the new series shortly.
Ram
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
2012-11-11 14:50 ` Ramkumar Ramachandra
@ 2012-11-11 15:09 ` Jeff King
0 siblings, 0 replies; 11+ messages in thread
From: Jeff King @ 2012-11-11 15:09 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Jens Lehmann
On Sun, Nov 11, 2012 at 08:20:27PM +0530, Ramkumar Ramachandra wrote:
> >> diff --git a/builtin/diff.c b/builtin/diff.c
> >> index 9650be2..6d00311 100644
> >> --- a/builtin/diff.c
> >> +++ b/builtin/diff.c
> >> @@ -297,6 +297,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
> >> DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
> >> DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
> >>
> >> + /* Set SUBMODULE_LOG if diff.submodule config var was set */
> >> + if (submodule_format_cfg && !strcmp(submodule_format_cfg, "log"))
> >> + DIFF_OPT_SET(&rev.diffopt, SUBMODULE_LOG);
> >> +
> >
> > Yuck. Why is this parsing happening in cmd_diff?
>
> Blame Jens- see this thread |
> http://thread.gmane.org/gmane.comp.version-control.git/206816/focus=206815
I don't think that is the right path, as at means that the option can
only ever affect diff, not other porcelains. I was thinking something
more like this (completely untested):
diff --git a/diff.c b/diff.c
index e89a201..74f4fc6 100644
--- a/diff.c
+++ b/diff.c
@@ -37,6 +37,13 @@ static int diff_stat_graph_width;
static int diff_dirstat_permille_default = 30;
static struct diff_options default_diff_options;
+/*
+ * 0 for "short", 1 for "log". This should probably just be an enum, and
+ * SUBMODULE_LOG lifted up from being a bit in the options to being its own
+ * struct member.
+ */
+static int diff_submodule_default;
+
static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_RESET,
GIT_COLOR_NORMAL, /* PLAIN */
@@ -178,6 +185,19 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
if (!strcmp(var, "diff.ignoresubmodules"))
handle_ignore_submodules_arg(&default_diff_options, value);
+ if (!strcmp(var, "diff.submodule")) {
+ /* XXX this should be factored out from the command-line parser */
+ if (!value)
+ return config_error_nonbool(var);
+ else if (!strcmp(var, "short"))
+ diff_submodule_default = 0;
+ else if (!strcmp(var, "log"))
+ diff_submodule_default = 1;
+ else
+ return error("unknown %s value: %s", var, value);
+ return 0;
+ }
+
if (git_color_config(var, value, cb) < 0)
return -1;
@@ -3193,6 +3213,9 @@ void diff_setup(struct diff_options *options)
options->a_prefix = "a/";
options->b_prefix = "b/";
}
+
+ if (diff_submodule_default)
+ DIFF_OPT_SET(options, SUBMODULE_LOG);
}
void diff_setup_done(struct diff_options *options)
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] submodule: display summary header in bold
2012-11-01 10:43 [PATCH v2 0/3] Introduce diff.submodule Ramkumar Ramachandra
2012-11-01 10:43 ` [PATCH 1/3] Documentation: move diff.wordRegex from config.txt to diff-config.txt Ramkumar Ramachandra
2012-11-01 10:43 ` [PATCH 2/3] diff: introduce diff.submodule configuration variable Ramkumar Ramachandra
@ 2012-11-01 10:43 ` Ramkumar Ramachandra
2012-11-01 21:10 ` [PATCH v2 0/3] Introduce diff.submodule Jens Lehmann
3 siblings, 0 replies; 11+ messages in thread
From: Ramkumar Ramachandra @ 2012-11-01 10:43 UTC (permalink / raw)
To: Git List; +Cc: Jens Lehmann
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 <artagnon@gmail.com>
---
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 835eb26..115f384 100644
--- a/diff.c
+++ b/diff.c
@@ -2244,7 +2244,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 e3e0b45..c10182e 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 f2e8271..997fd06 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 0/3] Introduce diff.submodule
2012-11-01 10:43 [PATCH v2 0/3] Introduce diff.submodule Ramkumar Ramachandra
` (2 preceding siblings ...)
2012-11-01 10:43 ` [PATCH 3/3] submodule: display summary header in bold Ramkumar Ramachandra
@ 2012-11-01 21:10 ` Jens Lehmann
2012-11-04 17:58 ` Ramkumar Ramachandra
3 siblings, 1 reply; 11+ messages in thread
From: Jens Lehmann @ 2012-11-01 21:10 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
Am 01.11.2012 11:43, schrieb Ramkumar Ramachandra:
> Hi,
>
> v1 is here:
> http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
>
> I've fixed the issues pointed out in the review by Jens.
Thanks, looking good to me.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 0/3] Introduce diff.submodule
2012-11-01 21:10 ` [PATCH v2 0/3] Introduce diff.submodule Jens Lehmann
@ 2012-11-04 17:58 ` Ramkumar Ramachandra
2012-11-08 20:43 ` Jeff King
0 siblings, 1 reply; 11+ messages in thread
From: Ramkumar Ramachandra @ 2012-11-04 17:58 UTC (permalink / raw)
To: Jens Lehmann, Jeff King; +Cc: Git List
Jens Lehmann wrote:
> Am 01.11.2012 11:43, schrieb Ramkumar Ramachandra:
>> Hi,
>>
>> v1 is here:
>> http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
>>
>> I've fixed the issues pointed out in the review by Jens.
>
> Thanks, looking good to me.
Peff, can we pick this up?
Ram
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 0/3] Introduce diff.submodule
2012-11-04 17:58 ` Ramkumar Ramachandra
@ 2012-11-08 20:43 ` Jeff King
0 siblings, 0 replies; 11+ messages in thread
From: Jeff King @ 2012-11-08 20:43 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Jens Lehmann, Git List
On Sun, Nov 04, 2012 at 11:28:17PM +0530, Ramkumar Ramachandra wrote:
> Jens Lehmann wrote:
> > Am 01.11.2012 11:43, schrieb Ramkumar Ramachandra:
> >> Hi,
> >>
> >> v1 is here:
> >> http://mid.gmane.org/1349196670-2844-1-git-send-email-artagnon@gmail.com
> >>
> >> I've fixed the issues pointed out in the review by Jens.
> >
> > Thanks, looking good to me.
>
> Peff, can we pick this up?
Thanks for prodding, I missed the original. I have a few comments, but
I'll respond directly.
Also, I was coincidentally looking at the same code today. You might
find this interesting:
http://article.gmane.org/gmane.comp.version-control.git/209188
-Peff
^ permalink raw reply [flat|nested] 11+ messages in thread