| Message ID | 67d5b93fdaab7f73f352293372ee3d71fb7c1409.1593529394.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: replace incorrect test_must_fail usage (part 6) | expand |
On Tue, Jun 30, 2020 at 11:03 AM Denton Liu <liu.denton@gmail.com> wrote: > In a future patch, we plan on making the test_must_fail()-family of > functions accept only git commands. Even though force_color() wraps an > invocation of `env git`, test_must_fail() will not be able to figure > this out since it will assume that force_color() is just some random > function which is disallowed. > > Instead of using `env` in force_color() (which does not support shell > functions), export the environment variables in a subshell. Write the > invocation as `force_color test_must_fail git ...` since shell functions > are now supported. > > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > @@ -31,7 +31,13 @@ diff_cmp () { > force_color () { > - env GIT_PAGER_IN_USE=true TERM=vt100 "$@" > + ( > + GIT_PAGER_IN_USE=true && > + export GIT_PAGER_IN_USE && > + TERM=vt100 && > + export TERM && > + "$@" > + ) > } I'm having trouble understanding why this function was transformed the way it was. I presume the subshell is to ensure that the variable assignments don't escape the function context since you're dropping 'env', however, it seems the following would be simpler: force_color () { (GIT_PAGER_IN_USE=true TERM=vt100 "$@") } Or, is there something non-obvious going on that I'm missing? By the way, I'm wondering if the subshell deserves an in-code comment. Whereas, we have somewhat settled upon the idiom 'test_must_fail env FOO=bar ...' when we need to make sure variable assignments don't escape the local context -- since 'FOO=bar test_must_fail ...' doesn't make that guarantee under all shells -- the use of a subshell here to achieve the same (if I'm understanding correctly) is not nearly so obvious. The non-obviousness is due to "$@" being abstract -- someone reading the code won't necessarily realize that the first element of "$@" might be a shell function, thus would not necessarily understand the use of a subshell.
Eric Sunshine <sunshine@sunshineco.com> writes: > On Tue, Jun 30, 2020 at 11:03 AM Denton Liu <liu.denton@gmail.com> wrote: >> In a future patch, we plan on making the test_must_fail()-family of >> functions accept only git commands. Even though force_color() wraps an >> invocation of `env git`, test_must_fail() will not be able to figure >> this out since it will assume that force_color() is just some random >> function which is disallowed. >> >> Instead of using `env` in force_color() (which does not support shell >> functions), export the environment variables in a subshell. Write the >> invocation as `force_color test_must_fail git ...` since shell functions >> are now supported. >> >> Signed-off-by: Denton Liu <liu.denton@gmail.com> >> --- >> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh >> @@ -31,7 +31,13 @@ diff_cmp () { >> force_color () { >> - env GIT_PAGER_IN_USE=true TERM=vt100 "$@" >> + ( >> + GIT_PAGER_IN_USE=true && >> + export GIT_PAGER_IN_USE && >> + TERM=vt100 && >> + export TERM && >> + "$@" >> + ) >> } > > I'm having trouble understanding why this function was transformed the > way it was. I presume the subshell is to ensure that the variable > assignments don't escape the function context since you're dropping > 'env', however, it seems the following would be simpler: > > force_color () { > (GIT_PAGER_IN_USE=true TERM=vt100 "$@") > } > > Or, is there something non-obvious going on that I'm missing? Denton wants to be able to write force_color test_must_fail git foo blah The only known kind of POSIX breaking behaviour by some shells makes the assignment persist after the "$@" returns, and the subshell would be a good way to protect the caller of force_color from the effect of the assignment leaking, so if we only care about that known breakage, both Denton's and your simplification should work OK, but it is probably clearer to see that we are *not* relying on any one-shot environment assignment at all in Denton's version. I'd list two variables on a single export command, though ;-) > By the way, I'm wondering if the subshell deserves an in-code comment. > Whereas, we have somewhat settled upon the idiom 'test_must_fail env > FOO=bar ...' when we need to make sure variable assignments don't > escape the local context -- since 'FOO=bar test_must_fail ...' doesn't > make that guarantee under all shells -- the use of a subshell here to > achieve the same (if I'm understanding correctly) is not nearly so > obvious. The non-obviousness is due to "$@" being abstract -- someone > reading the code won't necessarily realize that the first element of > "$@" might be a shell function, thus would not necessarily understand > the use of a subshell. Probably.
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 49decbac71..c4c1e9b603 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -31,7 +31,13 @@ diff_cmp () { # indicates a dumb terminal, so we set that variable, too. force_color () { - env GIT_PAGER_IN_USE=true TERM=vt100 "$@" + ( + GIT_PAGER_IN_USE=true && + export GIT_PAGER_IN_USE && + TERM=vt100 && + export TERM && + "$@" + ) } test_expect_success 'setup (initial)' ' @@ -604,7 +610,7 @@ test_expect_success 'detect bogus diffFilter output' ' echo content >test && test_config interactive.diffFilter "sed 1d" && printf y >y && - test_must_fail force_color git add -p <y + force_color test_must_fail git add -p <y ' test_expect_success 'diff.algorithm is passed to `git diff-files`' '
In a future patch, we plan on making the test_must_fail()-family of functions accept only git commands. Even though force_color() wraps an invocation of `env git`, test_must_fail() will not be able to figure this out since it will assume that force_color() is just some random function which is disallowed. Instead of using `env` in force_color() (which does not support shell functions), export the environment variables in a subshell. Write the invocation as `force_color test_must_fail git ...` since shell functions are now supported. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- t/t3701-add-interactive.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)