Re: [PATCH v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Junio C Hamano]

Lars Schneider  writes:

>> There is no need for this patch series to address this anomaly; it's
>> perhaps low-hanging fruit for someone wanting to join the project. The
>> only very minor wrinkle is that we'd still need to recognize --null as
>> a deprecated (and undocumented) alias for --nul.
>
> Does the list have a place to document these ideas for newbies to be found?

I do not know of one offhand, but if somebody (or like-minded group
of people) starts to collect one, I am sure it would be very much
appreciated.

A search for "low hanging fruit" and "hint hint" in the list archive
used to work well as a first-order approximation; using these
keywords takes conscious effort on those who write them, though ;-).
--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Lars Schneider]

On 15 Feb 2016, at 21:58, Eric Sunshine  wrote:

> On Mon, Feb 15, 2016 at 5:17 AM,   wrote:
>> If config values are queried using 'git config' (e.g. via --get,
>> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
>> find the configuration file where the values were defined.
>> 
>> Teach 'git config' the '--show-origin' option to print the source
>> configuration file for every printed value.
>> 
>> Based-on-patch-by: Jeff King 
>> Signed-off-by: Lars Schneider 
>> ---
>> diff --git a/builtin/config.c b/builtin/config.c
>> @@ -27,6 +28,7 @@ static int actions, types;
>> static const char *get_color_slot, *get_colorbool_slot;
>> static int end_null;
> 
> Not related to your changes, but I just realized that this variable
> really ought to be named 'end_nul' since we're talking about the
> character NUL, not a NULL pointer.
> 
>> static int respect_includes = -1;
>> +static int show_origin;
>> @@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
>>OPT_BOOL('z', "null", _null, N_("terminate values with NUL 
>> byte")),
> 
> Likewise, the long option name should be --nul rather than --null, or
> the long name could be dropped altogether since some other commands
> just recognize short option -z.
> 
> There is no need for this patch series to address this anomaly; it's
> perhaps low-hanging fruit for someone wanting to join the project. The
> only very minor wrinkle is that we'd still need to recognize --null as
> a deprecated (and undocumented) alias for --nul.

Does the list have a place to document these ideas for newbies to be found?

Thanks,
Lars--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Tue, Feb 16, 2016 at 10:14:45PM +, Ramsay Jones wrote:

> > I think it's more than that one-liner. This patch shows "type:name"
> > verbatim from what is passed into do_config_from_file, as does the error
> > message. If they are going to have different output formats (e.g.,
> > "" versus "stdin"), there needs to be logic transforming them in
> > at least one of the spots.
> 
> Ugh, yes you are right.
> 
> Hmm, I just hacked something up (see below) and, since its a bit
> ugly, I'm now in two minds! (it could be improved, of course). ;-)
> 
> So, I'll leave it to yourself and Lars to decide.
> [...]
> + if (!strcmp(cftype, "stdin")) {
> + cftype = "file";
> + cfname = "";
> + }

I think if we go this route it would be cleaner to just make "type" an
enum and convert it to the appropriate string in the callers. But other
than that, I think your patch is along the correct lines.

I dunno. Personally I am fine with the change in error messages done by
Lars. I could go either way.

-Peff

PS Thanks also for your patch fixing the prototypes. I completely missed
   that.
--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Ramsay Jones]

On 16/02/16 17:38, Jeff King wrote:
> On Tue, Feb 16, 2016 at 04:46:07PM +, Ramsay Jones wrote:
> 
>>> OK, I am happy to add the extra code. 
>>
>> Unless I've missed something (quite possible), this patch does not
>> need to change. (you have (both) convinced me that your current
>> solution is the best).
>>
>> The only change that I would suggest is the one-liner I already
>> suggested to the previous patch (plus the one-liner in the test,
>> of course. err ... so the two-liner!). Having said that, I didn't
>> try it out - I was just typing into my email client, so ...
> 
> I think it's more than that one-liner. This patch shows "type:name"
> verbatim from what is passed into do_config_from_file, as does the error
> message. If they are going to have different output formats (e.g.,
> "" versus "stdin"), there needs to be logic transforming them in
> at least one of the spots.

Ugh, yes you are right.

Hmm, I just hacked something up (see below) and, since its a bit
ugly, I'm now in two minds! (it could be improved, of course). ;-)

So, I'll leave it to yourself and Lars to decide.

ATB,
Ramsay Jones

-- >8 --
From: Ramsay Jones 
Date: Tue, 16 Feb 2016 21:39:47 +
Subject: [PATCH] config: fixup '' error messages

Signed-off-by: Ramsay Jones 
---
 config.c   | 22 ++
 t/t1300-repo-config.sh |  2 +-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/config.c b/config.c
index 0a35323..adc808c 100644
--- a/config.c
+++ b/config.c
@@ -417,6 +417,8 @@ static int git_parse_source(config_fn_t fn, void *data)
int comment = 0;
int baselen = 0;
struct strbuf *var = >var;
+   const char *cftype = cf->type;
+   const char *cfname = cf->name;
 
/* U+FEFF Byte Order Mark in UTF8 */
const char *bomptr = utf8_bom;
@@ -471,10 +473,14 @@ static int git_parse_source(config_fn_t fn, void *data)
if (get_value(fn, data, var) < 0)
break;
}
+   if (!strcmp(cftype, "stdin")) {
+   cftype = "file";
+   cfname = "";
+   }
if (cf->die_on_error)
-   die(_("bad config line %d in %s %s"), cf->linenr, cf->type, 
cf->name);
+   die(_("bad config line %d in %s %s"), cf->linenr, cftype, 
cfname);
else
-   return error(_("bad config line %d in %s %s"), cf->linenr, 
cf->type, cf->name);
+   return error(_("bad config line %d in %s %s"), cf->linenr, 
cftype, cfname);
 }
 
 static int parse_unit_factor(const char *end, uintmax_t *val)
@@ -589,9 +595,17 @@ static void die_bad_number(const char *name, const char 
*value)
if (!value)
value = "";
 
-   if (cf && cf->type && cf->name)
+   if (cf && cf->type && cf->name) {
+   const char *cftype = cf->type;
+   const char *cfname = cf->name;
+
+   if (!strcmp(cftype, "stdin")) {
+   cftype = "file";
+   cfname = "";
+   }
die(_("bad numeric config value '%s' for '%s' in %s %s: %s"),
-   value, name, cf->type, cf->name, reason);
+   value, name, cftype, cfname, reason);
+   }
die(_("bad numeric config value '%s' for '%s': %s"), value, name, 
reason);
 }
 
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 4abbdf9..4497688 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -707,7 +707,7 @@ test_expect_success 'invalid unit' '
 '
 
 test_expect_success 'invalid stdin config' '
-   echo "fatal: bad config line 1 in stdin " >expect &&
+   echo "fatal: bad config line 1 in file " >expect &&
echo "[broken" | test_must_fail git config --list --file - >output 2>&1 
&&
test_cmp expect output
 '
-- 
2.7.0



--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Tue, Feb 16, 2016 at 04:46:07PM +, Ramsay Jones wrote:

> > OK, I am happy to add the extra code. 
> 
> Unless I've missed something (quite possible), this patch does not
> need to change. (you have (both) convinced me that your current
> solution is the best).
> 
> The only change that I would suggest is the one-liner I already
> suggested to the previous patch (plus the one-liner in the test,
> of course. err ... so the two-liner!). Having said that, I didn't
> try it out - I was just typing into my email client, so ...

I think it's more than that one-liner. This patch shows "type:name"
verbatim from what is passed into do_config_from_file, as does the error
message. If they are going to have different output formats (e.g.,
"" versus "stdin"), there needs to be logic transforming them in
at least one of the spots.

-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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Ramsay Jones]

On 16/02/16 09:51, Lars Schneider wrote:
> 
> On 15 Feb 2016, at 23:39, Ramsay Jones  wrote:
> 
>>
>>
>> On 15/02/16 21:40, Jeff King wrote:
>>> On Mon, Feb 15, 2016 at 09:36:23PM +, Ramsay Jones wrote:
>>>
> +test_expect_success '--show-origin stdin' '
> + cat >expect <<-\EOF &&
> + stdin:  user.custom=true

 So, as with the previous patch, I think this should be:
file:user.custom=true
>>>
>>> That's ambiguous with a file named "", which was the point of
>>> having the two separate prefixes in the first place.
>>>
>>> I think in practice we _could_ get by with an ambiguous output (it's not
>>> like "" is a common filename), but that was discussed earlier in
>>> the thread, and Lars decided to go for something unambiguous.
>>
>> sure, I just don't think it would cause a problem in practice.
>> How about using '-' for ? Hmm, you can actually create
>> such a file in the filesystem! Oh well, I guess its not a big deal.
>>
>>>
>>> That doesn't necessarily have to bleed over into the error messages,
>>> though (which could continue to use "" if we want to put in a
>>> little extra code to covering the cases separately.
>>
>> Yep.

Sorry for not replying earlier - today has been hectic, so far.

> OK, I am happy to add the extra code. 

Unless I've missed something (quite possible), this patch does not
need to change. (you have (both) convinced me that your current
solution is the best).

The only change that I would suggest is the one-liner I already
suggested to the previous patch (plus the one-liner in the test,
of course. err ... so the two-liner!). Having said that, I didn't
try it out - I was just typing into my email client, so ...

>   However, out of curiosity, can
> you explain in what cases you actually use configs from stdin? I wasn't
> aware of this feature before working on this patch and I still wonder
> when I would use it.

Personally, I can't imagine ever using it. (I don't have a great
imagination. ;-)

Since I couldn't recall when this feature was added, I looked for
the commit that added it and found it was merged in commit 08f36302.
In particular, commit 3caec73b ("config: teach "git config --file -"
to read from the standard input", 19-02-2014) does not seem to
include any motivation for the change. The corresponding release
notes for v2.2.0 do not seem to add anything either.

So, I'm not much help here. :(

[Ah, looking at the date on the merge explains why I didn't
notice this.]

>  If it is only a seldom used feature then I am not
> sure if adding the extra code to restore the existing error message
> is worth the effort?

ATB,
Ramsay Jones


--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Lars Schneider]

On 16 Feb 2016, at 00:56, Junio C Hamano  wrote:

> Jeff King  writes:
> 
>>> An existing test t3300 tells me that a test that uses a path with a
>>> tab needs to be skipped on FAT/NTFS.  If your goal is to make sure
>>> dquote is exercised, can't we just do with a path with a SP in it or
>>> something?
>> 
>> It has to trigger quote_c_style(). You can see the complete set of
>> quoted characters in quote.c:sq_lookup, but space is not one of them.
>> Probably double-quote or backslash is the best bet, as the rest are all
>> control characters.
> 
> Yeah, 3300 seems to use dq for that.
> 
> Thanks for checking.

Thanks for the pointer Junio. I didn't think of FAT/NTFS. I will mimic
3300 and use double quotes.

Thanks,
Lars
--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Lars Schneider]

On 15 Feb 2016, at 23:39, Ramsay Jones  wrote:

> 
> 
> On 15/02/16 21:40, Jeff King wrote:
>> On Mon, Feb 15, 2016 at 09:36:23PM +, Ramsay Jones wrote:
>> 
 +test_expect_success '--show-origin stdin' '
 +  cat >expect <<-\EOF &&
 +  stdin:  user.custom=true
>>> 
>>> So, as with the previous patch, I think this should be:
>>> file:user.custom=true
>> 
>> That's ambiguous with a file named "", which was the point of
>> having the two separate prefixes in the first place.
>> 
>> I think in practice we _could_ get by with an ambiguous output (it's not
>> like "" is a common filename), but that was discussed earlier in
>> the thread, and Lars decided to go for something unambiguous.
> 
> sure, I just don't think it would cause a problem in practice.
> How about using '-' for ? Hmm, you can actually create
> such a file in the filesystem! Oh well, I guess its not a big deal.
> 
>> 
>> That doesn't necessarily have to bleed over into the error messages,
>> though (which could continue to use "" if we want to put in a
>> little extra code to covering the cases separately.
> 
> Yep.
OK, I am happy to add the extra code. However, out of curiosity, can
you explain in what cases you actually use configs from stdin? I wasn't
aware of this feature before working on this patch and I still wonder
when I would use it. If it is only a seldom used feature then I am not
sure if adding the extra code to restore the existing error message
is worth the effort?

Thanks,
Lars
--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Lars Schneider]

On 15 Feb 2016, at 22:41, Junio C Hamano  wrote:

> Ramsay Jones  writes:
> 
>>> +--show-origin::
>>> +   Augment the output of all queried config options with the
>>> +   origin type (file, stdin, blob, cmdline) and the actual origin
>> 
>> file, blob, cmdline (hmm, maybe cmd-line? ;-) )
> 
> If we are going to spell it out, "command-line" would be the way to
> go.  "cmdline" is probably OK.

I think cmdline is OK, too. However, if the list thinks that there is a chance
that it could be incomprehensible to the user then I would prefer 
"command-line".

- Lars--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Mon, Feb 15, 2016 at 02:18:18PM -0800, Junio C Hamano wrote:

> larsxschnei...@gmail.com writes:
> 
> > +test_expect_success 'set up custom config file' '
> > +   CUSTOM_CONFIG_FILE=$(printf "file\twith\ttabs.conf") &&
> > +   cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> > +   [user]
> > +   custom = true
> > +   EOF
> > +'
> > +
> > +test_expect_success '--show-origin escape special file name characters' '
> > +   cat >expect <<-\EOF &&
> > +   file:"file\twith\ttabs.conf"user.custom=true
> > +   EOF
> > +   git config --file "$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
> > +   test_cmp expect output
> > +'
> 
> Do we really need to use a file with such a name?
> 
> An existing test t3300 tells me that a test that uses a path with a
> tab needs to be skipped on FAT/NTFS.  If your goal is to make sure
> dquote is exercised, can't we just do with a path with a SP in it or
> something?

It has to trigger quote_c_style(). You can see the complete set of
quoted characters in quote.c:sq_lookup, but space is not one of them.
Probably double-quote or backslash is the best bet, as the rest are all
control characters.

-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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Ramsay Jones]

On 15/02/16 21:40, Jeff King wrote:
> On Mon, Feb 15, 2016 at 09:36:23PM +, Ramsay Jones wrote:
> 
>>> +test_expect_success '--show-origin stdin' '
>>> +   cat >expect <<-\EOF &&
>>> +   stdin:  user.custom=true
>>
>> So, as with the previous patch, I think this should be:
>>  file:user.custom=true
> 
> That's ambiguous with a file named "", which was the point of
> having the two separate prefixes in the first place.
> 
> I think in practice we _could_ get by with an ambiguous output (it's not
> like "" is a common filename), but that was discussed earlier in
> the thread, and Lars decided to go for something unambiguous.

sure, I just don't think it would cause a problem in practice.
How about using '-' for ? Hmm, you can actually create
such a file in the filesystem! Oh well, I guess its not a big deal.

> 
> That doesn't necessarily have to bleed over into the error messages,
> though (which could continue to use "" if we want to put in a
> little extra code to covering the cases separately.

Yep.

ATB,
Ramsay Jones


--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> +test_expect_success 'set up custom config file' '
> + CUSTOM_CONFIG_FILE=$(printf "file\twith\ttabs.conf") &&
> + cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> + [user]
> + custom = true
> + EOF
> +'
> +
> +test_expect_success '--show-origin escape special file name characters' '
> + cat >expect <<-\EOF &&
> + file:"file\twith\ttabs.conf"user.custom=true
> + EOF
> + git config --file "$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
> + test_cmp expect output
> +'

Do we really need to use a file with such a name?

An existing test t3300 tells me that a test that uses a path with a
tab needs to be skipped on FAT/NTFS.  If your goal is to make sure
dquote is exercised, can't we just do with a path with a SP in it or
something?

Thanks.
--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Junio C Hamano]

Ramsay Jones  writes:

>> +--show-origin::
>> +Augment the output of all queried config options with the
>> +origin type (file, stdin, blob, cmdline) and the actual origin
>
> file, blob, cmdline (hmm, maybe cmd-line? ;-) )

If we are going to spell it out, "command-line" would be the way to
go.  "cmdline" is probably OK.
--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Mon, Feb 15, 2016 at 09:36:23PM +, Ramsay Jones wrote:

> > +test_expect_success '--show-origin stdin' '
> > +   cat >expect <<-\EOF &&
> > +   stdin:  user.custom=true
> 
> So, as with the previous patch, I think this should be:
>   file:user.custom=true

That's ambiguous with a file named "", which was the point of
having the two separate prefixes in the first place.

I think in practice we _could_ get by with an ambiguous output (it's not
like "" is a common filename), but that was discussed earlier in
the thread, and Lars decided to go for something unambiguous.

That doesn't necessarily have to bleed over into the error messages,
though (which could continue to use "" if we want to put in a
little extra code to covering the cases separately.

-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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Ramsay Jones]

On 15/02/16 10:17, larsxschnei...@gmail.com wrote:
> From: Lars Schneider 
> 
> If config values are queried using 'git config' (e.g. via --get,
> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
> find the configuration file where the values were defined.
> 
> Teach 'git config' the '--show-origin' option to print the source
> configuration file for every printed value.
> 
> Based-on-patch-by: Jeff King 
> Signed-off-by: Lars Schneider 
> ---
>  Documentation/git-config.txt |  15 +++--
>  builtin/config.c |  33 ++
>  t/t1300-repo-config.sh   | 153 
> +++
>  3 files changed, 196 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 2608ca7..6374997 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -9,18 +9,18 @@ git-config - Get and set repository or global options
>  SYNOPSIS
>  
>  [verse]
> -'git config' [] [type] [-z|--null] name [value [value_regex]]
> +'git config' [] [type] [--show-origin] [-z|--null] name [value 
> [value_regex]]
>  'git config' [] [type] --add name value
>  'git config' [] [type] --replace-all name value [value_regex]
> -'git config' [] [type] [-z|--null] --get name [value_regex]
> -'git config' [] [type] [-z|--null] --get-all name [value_regex]
> -'git config' [] [type] [-z|--null] [--name-only] --get-regexp 
> name_regex [value_regex]
> +'git config' [] [type] [--show-origin] [-z|--null] --get name 
> [value_regex]
> +'git config' [] [type] [--show-origin] [-z|--null] --get-all 
> name [value_regex]
> +'git config' [] [type] [--show-origin] [-z|--null] 
> [--name-only] --get-regexp name_regex [value_regex]
>  'git config' [] [type] [-z|--null] --get-urlmatch name URL
>  'git config' [] --unset name [value_regex]
>  'git config' [] --unset-all name [value_regex]
>  'git config' [] --rename-section old_name new_name
>  'git config' [] --remove-section name
> -'git config' [] [-z|--null] [--name-only] -l | --list
> +'git config' [] [--show-origin] [-z|--null] [--name-only] -l | 
> --list
>  'git config' [] --get-color name [default]
>  'git config' [] --get-colorbool name [stdout-is-tty]
>  'git config' [] -e | --edit
> @@ -194,6 +194,11 @@ See also <>.
>   Output only the names of config variables for `--list` or
>   `--get-regexp`.
>  
> +--show-origin::
> + Augment the output of all queried config options with the
> + origin type (file, stdin, blob, cmdline) and the actual origin

file, blob, cmdline (hmm, maybe cmd-line? ;-) )

> + (config file path, ref, or blob id if applicable).
> +
>  --get-colorbool name [stdout-is-tty]::
>  
>   Find the color setting for `name` (e.g. `color.diff`) and output
> diff --git a/builtin/config.c b/builtin/config.c
> index adc7727..7bad0c0 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -3,6 +3,7 @@
>  #include "color.h"
>  #include "parse-options.h"
>  #include "urlmatch.h"
> +#include "quote.h"
>  
>  static const char *const builtin_config_usage[] = {
>   N_("git config []"),
> @@ -27,6 +28,7 @@ static int actions, types;
>  static const char *get_color_slot, *get_colorbool_slot;
>  static int end_null;
>  static int respect_includes = -1;
> +static int show_origin;
>  
>  #define ACTION_GET (1<<0)
>  #define ACTION_GET_ALL (1<<1)
> @@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
>   OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
>   OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
>   OPT_BOOL(0, "includes", _includes, N_("respect include 
> directives on lookup")),
> + OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
> (file, stdin, blob, cmdline)")),
>   OPT_END(),
>  };
>  
> @@ -91,8 +94,28 @@ static void check_argc(int argc, int min, int max) {
>   usage_with_options(builtin_config_usage, builtin_config_options);
>  }
>  
> +static void show_config_origin(struct strbuf *buf)
> +{
> + const char term = end_null ? '\0' : '\t';
> +
> + strbuf_addstr(buf, current_config_type());
> + strbuf_addch(buf, ':');
> + if (end_null)
> + strbuf_addstr(buf, current_config_name());
> + else
> + quote_c_style(current_config_name(), buf, NULL, 0);
> + strbuf_addch(buf, term);
> +}
> +
>  static int show_all_config(const char *key_, const char *value_, void *cb)
>  {
> + if (show_origin) {
> + struct strbuf buf = STRBUF_INIT;
> + show_config_origin();
> + /* Use fwrite as "buf" can contain \0's if "end_null" is set. */
> + fwrite(buf.buf, 1, buf.len, stdout);
> + strbuf_release();
> + }
>   if (!omit_values && value_)
>   printf("%s%c%s%c", key_, delim, value_, term);
>   else
> @@ -108,6 +131,8 @@ struct strbuf_list {
>  
>  

Re: [PATCH v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Mon, Feb 15, 2016 at 03:58:12PM -0500, Eric Sunshine wrote:

> > diff --git a/builtin/config.c b/builtin/config.c
> > @@ -27,6 +28,7 @@ static int actions, types;
> >  static const char *get_color_slot, *get_colorbool_slot;
> >  static int end_null;
> 
> Not related to your changes, but I just realized that this variable
> really ought to be named 'end_nul' since we're talking about the
> character NUL, not a NULL pointer.

Yeah, I noticed that, too. We just went through a round of related fixes
here, and the "usual" name is now "nul_term_line". I don't especially
like that one either, but at least it is correct and consistent. :)

> >  static int respect_includes = -1;
> > +static int show_origin;
> > @@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
> > OPT_BOOL('z', "null", _null, N_("terminate values with NUL 
> > byte")),
> 
> Likewise, the long option name should be --nul rather than --null, or
> the long name could be dropped altogether since some other commands
> just recognize short option -z.
> 
> There is no need for this patch series to address this anomaly; it's
> perhaps low-hanging fruit for someone wanting to join the project. The
> only very minor wrinkle is that we'd still need to recognize --null as
> a deprecated (and undocumented) alias for --nul.

I think that would be OK as long as we keep the compatible option.

-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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Eric Sunshine]

On Mon, Feb 15, 2016 at 5:17 AM,   wrote:
> If config values are queried using 'git config' (e.g. via --get,
> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
> find the configuration file where the values were defined.
>
> Teach 'git config' the '--show-origin' option to print the source
> configuration file for every printed value.
>
> Based-on-patch-by: Jeff King 
> Signed-off-by: Lars Schneider 
> ---
> diff --git a/builtin/config.c b/builtin/config.c
> @@ -27,6 +28,7 @@ static int actions, types;
>  static const char *get_color_slot, *get_colorbool_slot;
>  static int end_null;

Not related to your changes, but I just realized that this variable
really ought to be named 'end_nul' since we're talking about the
character NUL, not a NULL pointer.

>  static int respect_includes = -1;
> +static int show_origin;
> @@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
> OPT_BOOL('z', "null", _null, N_("terminate values with NUL 
> byte")),

Likewise, the long option name should be --nul rather than --null, or
the long name could be dropped altogether since some other commands
just recognize short option -z.

There is no need for this patch series to address this anomaly; it's
perhaps low-hanging fruit for someone wanting to join the project. The
only very minor wrinkle is that we'd still need to recognize --null as
a deprecated (and undocumented) alias for --nul.

> OPT_BOOL(0, "name-only", _values, N_("show variable names 
> only")),
> OPT_BOOL(0, "includes", _includes, N_("respect include 
> directives on lookup")),
> +   OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
> (file, stdin, blob, cmdline)")),
> OPT_END(),
>  };
--
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 v4 3/3] config: add '--show-origin' option to print the origin of a config value

[Jeff King]

On Mon, Feb 15, 2016 at 11:17:46AM +0100, larsxschnei...@gmail.com wrote:

> From: Lars Schneider 
> 
> If config values are queried using 'git config' (e.g. via --get,
> --get-all, --get-regexp, or --list flag) then it is sometimes hard to
> find the configuration file where the values were defined.
> 
> Teach 'git config' the '--show-origin' option to print the source
> configuration file for every printed value.
> 
> Based-on-patch-by: Jeff King 
> Signed-off-by: Lars Schneider 
> ---
>  Documentation/git-config.txt |  15 +++--
>  builtin/config.c |  33 ++
>  t/t1300-repo-config.sh   | 153 
> +++
>  3 files changed, 196 insertions(+), 5 deletions(-)

I packed all of my thoughts on test tear-down into my response to the
cover letter. :)

Other than that minor point, this version looks good to me.

-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

[PATCH v4 3/3] config: add '--show-origin' option to print the origin of a config value

[larsxschneider]

From: Lars Schneider 

If config values are queried using 'git config' (e.g. via --get,
--get-all, --get-regexp, or --list flag) then it is sometimes hard to
find the configuration file where the values were defined.

Teach 'git config' the '--show-origin' option to print the source
configuration file for every printed value.

Based-on-patch-by: Jeff King 
Signed-off-by: Lars Schneider 
---
 Documentation/git-config.txt |  15 +++--
 builtin/config.c |  33 ++
 t/t1300-repo-config.sh   | 153 +++
 3 files changed, 196 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2608ca7..6374997 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,18 +9,18 @@ git-config - Get and set repository or global options
 SYNOPSIS
 
 [verse]
-'git config' [] [type] [-z|--null] name [value [value_regex]]
+'git config' [] [type] [--show-origin] [-z|--null] name [value 
[value_regex]]
 'git config' [] [type] --add name value
 'git config' [] [type] --replace-all name value [value_regex]
-'git config' [] [type] [-z|--null] --get name [value_regex]
-'git config' [] [type] [-z|--null] --get-all name [value_regex]
-'git config' [] [type] [-z|--null] [--name-only] --get-regexp 
name_regex [value_regex]
+'git config' [] [type] [--show-origin] [-z|--null] --get name 
[value_regex]
+'git config' [] [type] [--show-origin] [-z|--null] --get-all name 
[value_regex]
+'git config' [] [type] [--show-origin] [-z|--null] [--name-only] 
--get-regexp name_regex [value_regex]
 'git config' [] [type] [-z|--null] --get-urlmatch name URL
 'git config' [] --unset name [value_regex]
 'git config' [] --unset-all name [value_regex]
 'git config' [] --rename-section old_name new_name
 'git config' [] --remove-section name
-'git config' [] [-z|--null] [--name-only] -l | --list
+'git config' [] [--show-origin] [-z|--null] [--name-only] -l | 
--list
 'git config' [] --get-color name [default]
 'git config' [] --get-colorbool name [stdout-is-tty]
 'git config' [] -e | --edit
@@ -194,6 +194,11 @@ See also <>.
Output only the names of config variables for `--list` or
`--get-regexp`.
 
+--show-origin::
+   Augment the output of all queried config options with the
+   origin type (file, stdin, blob, cmdline) and the actual origin
+   (config file path, ref, or blob id if applicable).
+
 --get-colorbool name [stdout-is-tty]::
 
Find the color setting for `name` (e.g. `color.diff`) and output
diff --git a/builtin/config.c b/builtin/config.c
index adc7727..7bad0c0 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -3,6 +3,7 @@
 #include "color.h"
 #include "parse-options.h"
 #include "urlmatch.h"
+#include "quote.h"
 
 static const char *const builtin_config_usage[] = {
N_("git config []"),
@@ -27,6 +28,7 @@ static int actions, types;
 static const char *get_color_slot, *get_colorbool_slot;
 static int end_null;
 static int respect_includes = -1;
+static int show_origin;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -81,6 +83,7 @@ static struct option builtin_config_options[] = {
OPT_BOOL('z', "null", _null, N_("terminate values with NUL byte")),
OPT_BOOL(0, "name-only", _values, N_("show variable names only")),
OPT_BOOL(0, "includes", _includes, N_("respect include 
directives on lookup")),
+   OPT_BOOL(0, "show-origin", _origin, N_("show origin of config 
(file, stdin, blob, cmdline)")),
OPT_END(),
 };
 
@@ -91,8 +94,28 @@ static void check_argc(int argc, int min, int max) {
usage_with_options(builtin_config_usage, builtin_config_options);
 }
 
+static void show_config_origin(struct strbuf *buf)
+{
+   const char term = end_null ? '\0' : '\t';
+
+   strbuf_addstr(buf, current_config_type());
+   strbuf_addch(buf, ':');
+   if (end_null)
+   strbuf_addstr(buf, current_config_name());
+   else
+   quote_c_style(current_config_name(), buf, NULL, 0);
+   strbuf_addch(buf, term);
+}
+
 static int show_all_config(const char *key_, const char *value_, void *cb)
 {
+   if (show_origin) {
+   struct strbuf buf = STRBUF_INIT;
+   show_config_origin();
+   /* Use fwrite as "buf" can contain \0's if "end_null" is set. */
+   fwrite(buf.buf, 1, buf.len, stdout);
+   strbuf_release();
+   }
if (!omit_values && value_)
printf("%s%c%s%c", key_, delim, value_, term);
else
@@ -108,6 +131,8 @@ struct strbuf_list {
 
 static int format_config(struct strbuf *buf, const char *key_, const char 
*value_)
 {
+   if (show_origin)
+   show_config_origin(buf);
if (show_keys)
strbuf_addstr(buf, key_);
if (!omit_values) {
@@ -538,6 +563,14 @@ int cmd_config(int argc,