Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
Ramkumar Ramachandra writes:
> Jeff King wrote:
>> On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:
>>> @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char
>>> *value, void *cb)
>>> return 0;
>>> }
>>>
>>> + if (!strcmp(var, "diff.submodule")) {
>>
>> Shouldn't this be in git_diff_ui_config so it does not affect scripts
>> calling plumbing?
>
> I honestly didn't understand the difference between
> git_diff_basic_config and git_diff_ui_config. The latter function
> calls the former function at the end of its body. Why are they two
> separate functions in the first place?
In case you meant s/didn't/don't/, git_diff_ui_config() should be
called only by human-facing Porcelain commands where their
behaviours can and should be affected by end user configuration
variables.
When a configuration variable should not affect output from plumbing
commands like diff-files, diff-index, and diff-tree, it must not be
read in git_diff_basic_config(), but in git_diff_ui_config().
The output from "git format-patch" is consumed by "git apply" that
expects "Subproject commit %s\n" with fully spelled object name, so
your configuration must not affect the output of format-patch,
either.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
Jeff King wrote:
> On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:
>> @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char
>> *value, void *cb)
>> return 0;
>> }
>>
>> + if (!strcmp(var, "diff.submodule")) {
>
> Shouldn't this be in git_diff_ui_config so it does not affect scripts
> calling plumbing?
I honestly didn't understand the difference between
git_diff_basic_config and git_diff_ui_config. The latter function
calls the former function at the end of its body. Why are they two
separate functions in the first place?
Btw, I just posted a follow-up.
Ram
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
On Sun, Nov 11, 2012 at 10:29:05PM +0530, Ramkumar Ramachandra wrote:
> +static int parse_submodule_params(struct diff_options *options, const char
> *value,
> + struct strbuf *errmsg)
> +{
> + if (!strcmp(value, "log"))
> + DIFF_OPT_SET(options, SUBMODULE_LOG);
> + else if (!strcmp(value, "short"))
> + DIFF_OPT_CLR(options, SUBMODULE_LOG);
> + else {
> + strbuf_addf(errmsg, _("'%s'"), value);
> + return 1;
> + }
> + return 0;
> +}
I think "-1" would be the more normal error return.
> @@ -223,6 +238,15 @@ int git_diff_basic_config(const char *var, const char
> *value, void *cb)
> return 0;
> }
>
> + if (!strcmp(var, "diff.submodule")) {
Shouldn't this be in git_diff_ui_config so it does not affect scripts
calling plumbing?
> + struct strbuf errmsg = STRBUF_INIT;
> + if (parse_submodule_params(&default_diff_options, value,
> &errmsg))
> + warning(_("Unknown value for 'diff.submodule' config
> variable: %s"),
> + errmsg.buf);
> + strbuf_release(&errmsg);
> + return 0;
> + }
Hmm. This strbuf error handling strikes me as very clunky, considering
that it does not pass any useful information out of the parse function
(it always just adds '$value' to the error string). Wouldn't it be
simpler to just have parse_submodule_params return -1, and then let the
caller warn or generate an error as appropriate?
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] diff: introduce diff.submodule configuration variable
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
