subject:"\[PATCH v3 3\/3\] config\: add '\-\-show\-origin' option to print the origin of a config value"

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

2016-02-15 Thread Lars Schneider


On 13 Feb 2016, at 19:15, Jeff King  wrote:

> On Sat, Feb 13, 2016 at 01:04:12PM -0500, Jeff King wrote:
> 
>> On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:
>> 
 +test_expect_success '--show-origin' '
>>> [...]
>>> I see you split this up more, but there's still quite a bit going on in
>>> this one block. IMHO, it would be more customary in our tests to put the
>>> setup into one test_expect_success block, then each of these
>>> expect-run-cmp blocks into their own test_expect_success.
>> 
>> Here's a squashable patch that shows what I mean.
> 
> And here are a few comments on the changes...
> 
>> -test_expect_success '--show-origin' '
>> ->.git/config &&
>> ->"$HOME"/.gitconfig &&
>> +test_expect_success 'set up --show-origin tests' '
>>  INCLUDE_DIR="$HOME/include" &&
>>  mkdir -p "$INCLUDE_DIR" &&
>> -cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
>> +cat >"$INCLUDE_DIR"/absolute.include <<-\EOF &&
>>  [user]
>>  absolute = include
>>  EOF
>> -cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
>> +cat >"$INCLUDE_DIR"/relative.include <<-\EOF &&
>>  [user]
>>  relative = include
>>  EOF
>> -test_config_global user.global "true" &&
>> -test_config_global user.override "global" &&
>> -test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
>> -test_config include.path ../include/relative.include &&
>> -test_config user.local "true" &&
>> -test_config user.override "local" &&
>> +cat >"$HOME"/.gitconfig <<-EOF &&
>> +[user]
>> +global = true
>> +override = global
>> +[include]
>> +path = "$INCLUDE_DIR/absolute.include"
>> +EOF
>> +cat >.git/config <<-\EOF
>> +[include]
>> +path = ../include/relative.include
>> +[user]
>> +local = true
>> +override = local
>> +EOF
> 
> I preserved your ordering here (as the later "--list" tests care). But
> it might be worth ordering both files the same way, so that a reader
> does not wonder if it is significant (and just update the --list
> output expectation later).
OK, fixed!


> 
>> @@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '
>>  localQcmdline:Quser.cmdline
>>  trueQ
>>  EOF
>> -git -c user.cmdline=true config --null --list --show-origin | nul_to_q 
>> >output &&
>> +git -c user.cmdline=true config --null --list --show-origin >output.raw 
>> &&
>> +nul_to_q output &&
> 
> We usually try to avoid putting git on the left-hand side of a pipe,
> because it hides the exit code, and we want to make sure git does not
> fail. I won't be surprised if you copied the style from elsewhere in the
> script, though, as this is an old one and we were not always consistent.
Make sense, fixed!


> 
>>  echo >>output &&
>> -test_cmp expect output &&
>> +test_cmp expect output
> 
> This "echo" might be worth a comment. I think we are just adding the
> extra newline that the here-doc insists on, but that --null output would
> not include.
Done.

> 
> Overall, I find the "--show-origin --null" output pretty weird to read.
> We use a newline to split the config key/value, a NUL between config
> entries, but now also a NUL between the filename and the rest of the
> config entry.
> 
> That makes parsing pretty weird, as you cannot just use something like
> 
>  git config --show-origin --list --null | perl -0ne ...
> 
> to process entries; every other entry you get will be a filename. But at
> the same time, we do not go all out and say "there is a NUL between each
> field", because the key/value separator is a newline in this case, and
> the reader has to parse that separately after splitting on NULs.
> 
> But I think it's too late to do anything about it now. The weirdness is
> really the mixed NUL/newline thing, and you are not introducing that.
> 
>> -CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
>> -cat >"$CUSTOM_CONFIG_FILE" <<-\EOF &&
>> -[user]
>> -custom = true
>> -EOF
>> +test_expect_success 'set up custom config file' '
>> +CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
>> +cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
>> +[user]
>> +custom = true
>> +EOF
>> +'
> 
> Everything, even mundane setup, should generally go in a test_expect
> block. That means we'll notice unexpected failures, and any output will
> follow the usual "--verbose" rules.
> 
> Arguably this setup could just go into the initial setup block.
> 
> Also, you may not that the filename does _not_ actually have tabs in it,
> because the shell does not expand "\t". It does have backslashes in it,
> though, which is enough to trigger our C-style quoting.
Oh, thanks for the explanation. I was already wondering about the double
backslash earlier...

> 
> So the

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

2016-02-14 Thread Johannes Schindelin

Hi Peff,

tl;dr let's keep an eye on adding only test cases that do not depend on
earlier test cases' output ('setup' excluded, of course).

On Sun, 14 Feb 2016, Jeff King wrote:

> In general, my opinion is that skipping arbitrary leading tests is a
> losing strategy. It's just too easy to introduce hidden dependencies,
> and not worth the programmer time to make sure each test runs in
> isolation. But others on the list may disagree.

Yes, I disagree. And you would, too, if you had to run as many tests as I
had to do by way of js/mingw-tests. I did not keep precise track of time,
but I am certain that I had to run one of those bloody tests (forgot which
one) around 100 times, each taking roughly 3 minutes to complete, and of
course, it was the *last* test case failing, and *of course* it depended
on earlier tests to run.

It gets even worse when you think about those test cases that depend on
some prereq such as SYMLINKS or POSIXPERM. In *most* cases does the
developer who adds them not even notice that these prerequisites are
required. And subsequent test cases that do *not* share those
prerequisites depend (of course!) on the previous test cases' output.

Don't get me wrong, I think it would be too much pain for little gain to
clean up our act now. But I think that we have ample evidence that it
would be a plenty good idea to try pretty hard to avoid adding to the
pile.

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

2016-02-14 Thread Lars Schneider


On 13 Feb 2016, at 18:44, Jeff King  wrote:

> On Sat, Feb 13, 2016 at 03:24:16PM +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.
> 
> Thanks, I think this version fixes the correctness issues I mentioned
> earlier. I do still have nits to pick (of course :) ), that we may or
> may not want to deal with.
> 
>> +static void show_config_origin(struct strbuf *buf)
>> +{
>> +const char term = end_null ? '\0' : '\t';
>> +const char *type;
>> +const char *name;
>> +
>> +current_config_type_name(, );
> 
> This double out-parameter feels like a clunky interface.
> 
> I was tempted to suggest that we simply make the "struct config_source"
> available to builtin/config.c (which is already pretty intimate with the
> rest of the config code), and then it can pick out what it wants. But
> there _is_ some logic in the function to convert the NULL "cf" into
> "cmdline".
> 
> Perhaps it would be simpler to just have two accessor functions, and do:
> 
>  strbuf_addstr(buf, current_config_type());
>  ...
>  strbuf_addstr(buf, current_config_name());
> 
> I admit it is a pretty minor point, though.
Agreed, this looks nicer.


> 
>> static int show_all_config(const char *key_, const char *value_, void *cb)
>> {
>> +if (show_origin) {
>> +   struct strbuf buf = STRBUF_INIT;
>> +   show_config_origin();
>> +   fwrite(buf.buf, 1, buf.len, stdout);
>> +   strbuf_release();
>> +}
> 
> The indentation is funky here.
True! Indentation without intention :-) 

> 
> The use of fwrite() to catch the embedded NULs is subtle enough that it
> might be worth a comment.
> 
> It also made me wonder how format_config() handles this. It looks like
> we send the result eventually to fwrite() there, so it all works (and it
> does _not_ have the comment I mentioned :) ).
I will add a comment in both places :-)


> 
>> +test_expect_success '--show-origin' '
>> +>.git/config &&
>> +>"$HOME"/.gitconfig &&
>> +INCLUDE_DIR="$HOME/include" &&
>> +mkdir -p "$INCLUDE_DIR" &&
>> +cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
>> +[user]
>> +absolute = include
>> +EOF
>> +cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
>> +[user]
>> +relative = include
>> +EOF
>> +test_config_global user.global "true" &&
>> +test_config_global user.override "global" &&
>> +test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
>> +test_config include.path ../include/relative.include &&
>> +test_config user.local "true" &&
>> +test_config user.override "local" &&
>> +
>> +cat >expect <<-EOF &&
>> +file:$HOME/.gitconfig   user.global=true
>> +file:$HOME/.gitconfig   user.override=global
>> +file:$HOME/.gitconfig   
>> include.path=$INCLUDE_DIR/absolute.include
>> +file:$INCLUDE_DIR/absolute.include  user.absolute=include
>> +file:.git/configinclude.path=../include/relative.include
>> +file:.git/../include/relative.include   user.relative=include
>> +file:.git/configuser.local=true
>> +file:.git/configuser.override=local
>> +cmdline:user.cmdline=true
>> +EOF
>> +git -c user.cmdline=true config --list --show-origin >output &&
>> +test_cmp expect output &&
>> +
>> +cat >expect <<-EOF &&
>> +file:$HOME/.gitconfigQuser.global
>> +trueQfile:$HOME/.gitconfigQuser.override
>> +globalQfile:$HOME/.gitconfigQinclude.path
>> +
>> $INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
>> +includeQfile:.git/configQinclude.path
>> +
>> ../include/relative.includeQfile:.git/../include/relative.includeQuser.relative
>> +includeQfile:.git/configQuser.local
>> +trueQfile:.git/configQuser.override
>> +localQcmdline:Quser.cmdline
>> +trueQ
>> +EOF
> 
> I see you split this up more, but there's still quite a bit going on in
> this one block. IMHO, it would be more customary in our tests to put the
> setup into one test_expect_success block, then each of these
> expect-run-cmp blocks into their own test_expect_success.
> 
> It does mean that the setup mutates the global test state for further
> tests (and you should stop using test_config_*, which clean up at the
> end of the block), but I think that's the right thing here. The point of
> test_config is "flip on this switch just for a moment, so we can test
> its effect without hurting further

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

2016-02-14 Thread Jeff King

On Sun, Feb 14, 2016 at 01:48:59PM +0100, Lars Schneider wrote:

> > I see you split this up more, but there's still quite a bit going on in
> > this one block. IMHO, it would be more customary in our tests to put the
> > setup into one test_expect_success block, then each of these
> > expect-run-cmp blocks into their own test_expect_success.
> > 
> > It does mean that the setup mutates the global test state for further
> > tests (and you should stop using test_config_*, which clean up at the
> > end of the block), but I think that's the right thing here. The point of
> > test_config is "flip on this switch just for a moment, so we can test
> > its effect without hurting further tests". But these are config tests in
> > the first place, and it is OK for them to show a progression of
> > mutations of the config (you'll note that like the other tests in this
> > script, you are clearing out .git/config in the first place).
> > 
> TBH I am always a little annoyed if Git tests depend on each other. It makes
> it harder to just disable all uninteresting tests and only focus on the one 
> that 
> I am working with. However, I agree with your point that the test block does 
> too
> many things. Would it be OK if I write a bash function that performs the test
> setup? Then I would call this function in the beginning of every individual 
> test. Or do you prefer the global state strategy?

In general, my opinion is that skipping arbitrary leading tests is a
losing strategy. It's just too easy to introduce hidden dependencies,
and not worth the programmer time to make sure each test runs in
isolation. But others on the list may disagree.

That being said, I think what I am proposing is a much milder form of
that. With what I am proposing, you can skip everything _except_ tests
which match /set.?up/ in their description. We do not perfectly adhere
to that in our tests, but I suspect it works a majority of the time.

If it is taking too long to get to a particular test in a test script,
maybe that is a sign we need to break up the script. There are also a
few tricks you can use to still _run_ the earlier blocks, but not have
them interfere with debugging a particular test:

  1. Use --verbose-only=123 to get verbose output only from a single
 test.

  2. Use "-i" to stop running tests at the first failure. Usually it is
 worth fixing that one, and then seeing if other tests fail, too, or
 were simply dependent.

  3. If you are using --valgrind, the tests run very slowly (normally
 t1300 takes 400ms to run on my machine, so I don't mind waiting
 that long to get to a new test at the end. With valgrind it is more
 like 90 seconds). You can use --valgrind-only=123 to test only the
 block you are debugging, and run the rest quickly.

We do use shell functions in some places to do repeated setup. In
general, I prefer setting up the global state. It's more efficient
(which does add up when running the whole suite), and I find it easier
to debug failing tests (it's just one less thing the failing block is
doing that you have to look at; and you can generally "cd" into the
leftover trash directory to investigate the global state).

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

2016-02-13 Thread 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 |  35 
 cache.h  |   1 +
 config.c |   6 ++
 t/t1300-repo-config.sh   | 128 +++
 5 files changed, 180 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 59b1c95..2bf4d4e 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..302b265 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,30 @@ 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';
+   const char *type;
+   const char *name;
+
+   current_config_type_name(, );
+   strbuf_addstr(buf, type);
+   strbuf_addch(buf, ':');
+   if (end_null)
+   strbuf_addstr(buf, name);
+   else
+   quote_c_style(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();
+   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 +133,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 +565,14 @@ int cmd_config(int argc,

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

2016-02-13 Thread Jeff King

On Sat, Feb 13, 2016 at 03:24:16PM +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.

Thanks, I think this version fixes the correctness issues I mentioned
earlier. I do still have nits to pick (of course :) ), that we may or
may not want to deal with.

> +static void show_config_origin(struct strbuf *buf)
> +{
> + const char term = end_null ? '\0' : '\t';
> + const char *type;
> + const char *name;
> +
> + current_config_type_name(, );

This double out-parameter feels like a clunky interface.

I was tempted to suggest that we simply make the "struct config_source"
available to builtin/config.c (which is already pretty intimate with the
rest of the config code), and then it can pick out what it wants. But
there _is_ some logic in the function to convert the NULL "cf" into
"cmdline".

Perhaps it would be simpler to just have two accessor functions, and do:

  strbuf_addstr(buf, current_config_type());
  ...
  strbuf_addstr(buf, current_config_name());

I admit it is a pretty minor point, though.

>  static int show_all_config(const char *key_, const char *value_, void *cb)
>  {
> + if (show_origin) {
> +   struct strbuf buf = STRBUF_INIT;
> +   show_config_origin();
> +   fwrite(buf.buf, 1, buf.len, stdout);
> +   strbuf_release();
> + }

The indentation is funky here.

The use of fwrite() to catch the embedded NULs is subtle enough that it
might be worth a comment.

It also made me wonder how format_config() handles this. It looks like
we send the result eventually to fwrite() there, so it all works (and it
does _not_ have the comment I mentioned :) ).

> +test_expect_success '--show-origin' '
> + >.git/config &&
> + >"$HOME"/.gitconfig &&
> + INCLUDE_DIR="$HOME/include" &&
> + mkdir -p "$INCLUDE_DIR" &&
> + cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
> + [user]
> + absolute = include
> + EOF
> + cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
> + [user]
> + relative = include
> + EOF
> + test_config_global user.global "true" &&
> + test_config_global user.override "global" &&
> + test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
> + test_config include.path ../include/relative.include &&
> + test_config user.local "true" &&
> + test_config user.override "local" &&
> +
> + cat >expect <<-EOF &&
> + file:$HOME/.gitconfig   user.global=true
> + file:$HOME/.gitconfig   user.override=global
> + file:$HOME/.gitconfig   
> include.path=$INCLUDE_DIR/absolute.include
> + file:$INCLUDE_DIR/absolute.include  user.absolute=include
> + file:.git/configinclude.path=../include/relative.include
> + file:.git/../include/relative.include   user.relative=include
> + file:.git/configuser.local=true
> + file:.git/configuser.override=local
> + cmdline:user.cmdline=true
> + EOF
> + git -c user.cmdline=true config --list --show-origin >output &&
> + test_cmp expect output &&
> +
> + cat >expect <<-EOF &&
> + file:$HOME/.gitconfigQuser.global
> + trueQfile:$HOME/.gitconfigQuser.override
> + globalQfile:$HOME/.gitconfigQinclude.path
> + 
> $INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
> + includeQfile:.git/configQinclude.path
> + 
> ../include/relative.includeQfile:.git/../include/relative.includeQuser.relative
> + includeQfile:.git/configQuser.local
> + trueQfile:.git/configQuser.override
> + localQcmdline:Quser.cmdline
> + trueQ
> + EOF

I see you split this up more, but there's still quite a bit going on in
this one block. IMHO, it would be more customary in our tests to put the
setup into one test_expect_success block, then each of these
expect-run-cmp blocks into their own test_expect_success.

It does mean that the setup mutates the global test state for further
tests (and you should stop using test_config_*, which clean up at the
end of the block), but I think that's the right thing here. The point of
test_config is "flip on this switch just for a moment, so we can test
its effect without hurting further tests". But these are config tests in
the first place, and it is OK for them to show a progression of
mutations of the config (you'll note that like the other tests in this
script, you are clearing out .git/config in the first place).

-Peff
--
To unsubscribe from this list: send

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

2016-02-13 Thread Jeff King

On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:

> > +test_expect_success '--show-origin' '
> [...]
> I see you split this up more, but there's still quite a bit going on in
> this one block. IMHO, it would be more customary in our tests to put the
> setup into one test_expect_success block, then each of these
> expect-run-cmp blocks into their own test_expect_success.

Here's a squashable patch that shows what I mean.

---
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 112f7c8..c7496aa 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1207,26 +1207,34 @@ test_expect_success POSIXPERM,PERL 'preserves existing 
permissions' '
  "die q(badrename) if ((stat(q(.git/config)))[2] & 0) != 0600"
 '
 
-test_expect_success '--show-origin' '
-   >.git/config &&
-   >"$HOME"/.gitconfig &&
+test_expect_success 'set up --show-origin tests' '
INCLUDE_DIR="$HOME/include" &&
mkdir -p "$INCLUDE_DIR" &&
-   cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
+   cat >"$INCLUDE_DIR"/absolute.include <<-\EOF &&
[user]
absolute = include
EOF
-   cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
+   cat >"$INCLUDE_DIR"/relative.include <<-\EOF &&
[user]
relative = include
EOF
-   test_config_global user.global "true" &&
-   test_config_global user.override "global" &&
-   test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
-   test_config include.path ../include/relative.include &&
-   test_config user.local "true" &&
-   test_config user.override "local" &&
+   cat >"$HOME"/.gitconfig <<-EOF &&
+   [user]
+   global = true
+   override = global
+   [include]
+   path = "$INCLUDE_DIR/absolute.include"
+   EOF
+   cat >.git/config <<-\EOF
+   [include]
+   path = ../include/relative.include
+   [user]
+   local = true
+   override = local
+   EOF
+'
 
+test_expect_success '--show-origin with --list' '
cat >expect <<-EOF &&
file:$HOME/.gitconfig   user.global=true
file:$HOME/.gitconfig   user.override=global
@@ -1239,8 +1247,10 @@ test_expect_success '--show-origin' '
cmdline:user.cmdline=true
EOF
git -c user.cmdline=true config --list --show-origin >output &&
-   test_cmp expect output &&
+   test_cmp expect output
+'
 
+test_expect_success '--show-origin with --list --null' '
cat >expect <<-EOF &&
file:$HOME/.gitconfigQuser.global
trueQfile:$HOME/.gitconfigQuser.override
@@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '
localQcmdline:Quser.cmdline
trueQ
EOF
-   git -c user.cmdline=true config --null --list --show-origin | nul_to_q 
>output &&
+   git -c user.cmdline=true config --null --list --show-origin >output.raw 
&&
+   nul_to_q output &&
echo >>output &&
-   test_cmp expect output &&
+   test_cmp expect output
+'
 
+test_expect_success '--show-origin with single file' '
cat >expect <<-\EOF &&
file:.git/configinclude.path=../include/relative.include
file:.git/configuser.local=true
file:.git/configuser.override=local
EOF
git config --local --list --show-origin >output &&
-   test_cmp expect output &&
+   test_cmp expect output
+'
 
+test_expect_success '--show-origin with --get-regexp' '
cat >expect <<-EOF &&
file:$HOME/.gitconfig   user.global true
file:.git/configuser.local true
EOF
git config --show-origin --get-regexp "user\.[g|l].*" >output &&
-   test_cmp expect output &&
+   test_cmp expect output
+'
 
+test_expect_success '--show-origin getting a single key' '
cat >expect <<-\EOF &&
file:.git/configlocal
EOF
@@ -1279,11 +1296,13 @@ test_expect_success '--show-origin' '
test_cmp expect output
 '
 
-CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
-cat >"$CUSTOM_CONFIG_FILE" <<-\EOF &&
-   [user]
-   custom = true
-EOF
+test_expect_success 'set up custom config file' '
+   CUSTOM_CONFIG_FILE="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 &&
@@ -1302,8 +1321,6 @@ test_expect_success '--show-origin stdin' '
 '
 
 test_expect_success '--show-origin stdin with file include' '
-   INCLUDE_DIR="$HOME/include" &&
-   mkdir -p "$INCLUDE_DIR" &&
cat

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

2016-02-13 Thread Jeff King

On Sat, Feb 13, 2016 at 01:04:12PM -0500, Jeff King wrote:

> On Sat, Feb 13, 2016 at 12:44:49PM -0500, Jeff King wrote:
> 
> > > +test_expect_success '--show-origin' '
> > [...]
> > I see you split this up more, but there's still quite a bit going on in
> > this one block. IMHO, it would be more customary in our tests to put the
> > setup into one test_expect_success block, then each of these
> > expect-run-cmp blocks into their own test_expect_success.
> 
> Here's a squashable patch that shows what I mean.

And here are a few comments on the changes...

> -test_expect_success '--show-origin' '
> - >.git/config &&
> - >"$HOME"/.gitconfig &&
> +test_expect_success 'set up --show-origin tests' '
>   INCLUDE_DIR="$HOME/include" &&
>   mkdir -p "$INCLUDE_DIR" &&
> - cat >"$INCLUDE_DIR"/absolute.include <<-EOF &&
> + cat >"$INCLUDE_DIR"/absolute.include <<-\EOF &&
>   [user]
>   absolute = include
>   EOF
> - cat >"$INCLUDE_DIR"/relative.include <<-EOF &&
> + cat >"$INCLUDE_DIR"/relative.include <<-\EOF &&
>   [user]
>   relative = include
>   EOF
> - test_config_global user.global "true" &&
> - test_config_global user.override "global" &&
> - test_config_global include.path "$INCLUDE_DIR"/absolute.include &&
> - test_config include.path ../include/relative.include &&
> - test_config user.local "true" &&
> - test_config user.override "local" &&
> + cat >"$HOME"/.gitconfig <<-EOF &&
> + [user]
> + global = true
> + override = global
> + [include]
> + path = "$INCLUDE_DIR/absolute.include"
> + EOF
> + cat >.git/config <<-\EOF
> + [include]
> + path = ../include/relative.include
> + [user]
> + local = true
> + override = local
> + EOF

I preserved your ordering here (as the later "--list" tests care). But
it might be worth ordering both files the same way, so that a reader
does not wonder if it is significant (and just update the --list
output expectation later).

> @@ -1253,25 +1263,32 @@ test_expect_success '--show-origin' '
>   localQcmdline:Quser.cmdline
>   trueQ
>   EOF
> - git -c user.cmdline=true config --null --list --show-origin | nul_to_q 
> >output &&
> + git -c user.cmdline=true config --null --list --show-origin >output.raw 
> &&
> + nul_to_q output &&

We usually try to avoid putting git on the left-hand side of a pipe,
because it hides the exit code, and we want to make sure git does not
fail. I won't be surprised if you copied the style from elsewhere in the
script, though, as this is an old one and we were not always consistent.

>   echo >>output &&
> - test_cmp expect output &&
> + test_cmp expect output

This "echo" might be worth a comment. I think we are just adding the
extra newline that the here-doc insists on, but that --null output would
not include.

Overall, I find the "--show-origin --null" output pretty weird to read.
We use a newline to split the config key/value, a NUL between config
entries, but now also a NUL between the filename and the rest of the
config entry.

That makes parsing pretty weird, as you cannot just use something like

  git config --show-origin --list --null | perl -0ne ...

to process entries; every other entry you get will be a filename. But at
the same time, we do not go all out and say "there is a NUL between each
field", because the key/value separator is a newline in this case, and
the reader has to parse that separately after splitting on NULs.

But I think it's too late to do anything about it now. The weirdness is
really the mixed NUL/newline thing, and you are not introducing that.

> -CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
> -cat >"$CUSTOM_CONFIG_FILE" <<-\EOF &&
> - [user]
> - custom = true
> -EOF
> +test_expect_success 'set up custom config file' '
> + CUSTOM_CONFIG_FILE="file\twith\ttabs.conf" &&
> + cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
> + [user]
> + custom = true
> + EOF
> +'

Everything, even mundane setup, should generally go in a test_expect
block. That means we'll notice unexpected failures, and any output will
follow the usual "--verbose" rules.

Arguably this setup could just go into the initial setup block.

Also, you may not that the filename does _not_ actually have tabs in it,
because the shell does not expand "\t". It does have backslashes in it,
though, which is enough to trigger our C-style quoting.

So the test isn't wrong, but the filename is misleading. If you really
want tabs, you'd have to do:

  CUSTOM_CONFIG_FILE="$(printf "file\twith\ttabs.conf")

or similar.

>  test_expect_success '--show-origin escape special file name characters' '
>   cat >expect <<-\EOF &&
> @@ -1302,8 +1321,6 @@

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

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

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

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

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

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

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

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

8 matches

Site Navigation

Mail list logo

Footer information