On 13 Feb 2016, at 19:15, Jeff King <p...@peff.net> 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.raw >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 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 @@ test_expect_success '--show-origin stdin' ' >> ' >> >> test_expect_success '--show-origin stdin with file include' ' >> - INCLUDE_DIR="$HOME/include" && >> - mkdir -p "$INCLUDE_DIR" && >> cat >"$INCLUDE_DIR"/stdin.include <<-EOF && >> [user] >> stdin = include > > Here we can assume that the setup block succeeded (if it didn't, all of > the tests are screwed anyway, so you'd want to fix that first). 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
