| Message ID | 20220427170649.4949-3-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 424f315d9f15338ee950f343e01becd6f087121b |
| Headers | show |
| Series | t0033-safe-directory: test improvements and a documentation clarification | expand |
SZEDER Gábor <szeder.dev@gmail.com> writes: > According to the documentation 'safe.directory' "is only respected > when specified in a system or global config, not when it is specified > in a repository config or via the command line option -c > safe.directory=<path>". > > Add tests to check that 'safe.directory' in the repository config or > on the command line is indeed ignored. > > Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > --- > t/t0033-safe-directory.sh | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh > index 6f9680e8b0..82dac0eb93 100755 > --- a/t/t0033-safe-directory.sh > +++ b/t/t0033-safe-directory.sh > @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' ' > expect_rejected_dir > ' > > +test_expect_success 'ignoring safe.directory on the command line' ' > + test_must_fail git -c safe.directory="$(pwd)" status 2>err && > + grep "unsafe repository" err > +' > + > +test_expect_success 'ignoring safe.directory in repo config' ' > + ( > + unset GIT_TEST_ASSUME_DIFFERENT_OWNER && > + git config safe.directory "$(pwd)" > + ) && > + expect_rejected_dir > +' I am debating myself if we want to remove the in-repository safe.directory configuration setting after this test piece is done, with test_when_finished. We just made sure, with this test, that having the variable does not affect anything, so the subsequent tests should not care hence it is probably OK. On the other hand, to make sure those settings they make (e.g. setting it globally is what the next test we can see below does) is what affects their outcome, it removes one more thing we need to worry about if we clean after ourselves. I dunno. Other than that, looking good so far; both [1/3] and [2/3] look good. > test_expect_success 'safe.directory does not match' ' > git config --global safe.directory bogus && > expect_rejected_dir
On 4/27/2022 4:37 PM, Junio C Hamano wrote: > SZEDER Gábor <szeder.dev@gmail.com> writes: > >> According to the documentation 'safe.directory' "is only respected >> when specified in a system or global config, not when it is specified >> in a repository config or via the command line option -c >> safe.directory=<path>". >> >> Add tests to check that 'safe.directory' in the repository config or >> on the command line is indeed ignored. >> >> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> >> --- >> t/t0033-safe-directory.sh | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh >> index 6f9680e8b0..82dac0eb93 100755 >> --- a/t/t0033-safe-directory.sh >> +++ b/t/t0033-safe-directory.sh >> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' ' >> expect_rejected_dir >> ' >> >> +test_expect_success 'ignoring safe.directory on the command line' ' >> + test_must_fail git -c safe.directory="$(pwd)" status 2>err && >> + grep "unsafe repository" err >> +' >> + >> +test_expect_success 'ignoring safe.directory in repo config' ' >> + ( >> + unset GIT_TEST_ASSUME_DIFFERENT_OWNER && >> + git config safe.directory "$(pwd)" >> + ) && >> + expect_rejected_dir >> +' > > I am debating myself if we want to remove the in-repository > safe.directory configuration setting after this test piece is done, > with test_when_finished. We just made sure, with this test, that > having the variable does not affect anything, so the subsequent > tests should not care hence it is probably OK. On the other hand, > to make sure those settings they make (e.g. setting it globally is > what the next test we can see below does) is what affects their > outcome, it removes one more thing we need to worry about if we > clean after ourselves. I dunno. test_config would do the same, right? I think it automatically does the test_when_finished for us. Thanks, -Stolee
Derrick Stolee <derrickstolee@github.com> writes: > On 4/27/2022 4:37 PM, Junio C Hamano wrote: >> SZEDER Gábor <szeder.dev@gmail.com> writes: >> >>> According to the documentation 'safe.directory' "is only respected >>> when specified in a system or global config, not when it is specified >>> in a repository config or via the command line option -c >>> safe.directory=<path>". >>> >>> Add tests to check that 'safe.directory' in the repository config or >>> on the command line is indeed ignored. >>> >>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> >>> --- >>> t/t0033-safe-directory.sh | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh >>> index 6f9680e8b0..82dac0eb93 100755 >>> --- a/t/t0033-safe-directory.sh >>> +++ b/t/t0033-safe-directory.sh >>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' ' >>> expect_rejected_dir >>> ' >>> >>> +test_expect_success 'ignoring safe.directory on the command line' ' >>> + test_must_fail git -c safe.directory="$(pwd)" status 2>err && >>> + grep "unsafe repository" err >>> +' >>> + >>> +test_expect_success 'ignoring safe.directory in repo config' ' >>> + ( >>> + unset GIT_TEST_ASSUME_DIFFERENT_OWNER && >>> + git config safe.directory "$(pwd)" >>> + ) && >>> + expect_rejected_dir >>> +' >> >> I am debating myself if we want to remove the in-repository >> safe.directory configuration setting after this test piece is done, >> with test_when_finished. We just made sure, with this test, that >> having the variable does not affect anything, so the subsequent >> tests should not care hence it is probably OK. On the other hand, >> to make sure those settings they make (e.g. setting it globally is >> what the next test we can see below does) is what affects their >> outcome, it removes one more thing we need to worry about if we >> clean after ourselves. I dunno. > > test_config would do the same, right? I think it automatically > does the test_when_finished for us. I thought it (specifically, anything depends on test_when_finished) has trouble working well from inside a subprocess?
On Fri, Apr 29, 2022 at 10:57:58AM -0700, Junio C Hamano wrote: > Derrick Stolee <derrickstolee@github.com> writes: > > > On 4/27/2022 4:37 PM, Junio C Hamano wrote: > >> SZEDER Gábor <szeder.dev@gmail.com> writes: > >> > >>> According to the documentation 'safe.directory' "is only respected > >>> when specified in a system or global config, not when it is specified > >>> in a repository config or via the command line option -c > >>> safe.directory=<path>". > >>> > >>> Add tests to check that 'safe.directory' in the repository config or > >>> on the command line is indeed ignored. > >>> > >>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> > >>> --- > >>> t/t0033-safe-directory.sh | 13 +++++++++++++ > >>> 1 file changed, 13 insertions(+) > >>> > >>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh > >>> index 6f9680e8b0..82dac0eb93 100755 > >>> --- a/t/t0033-safe-directory.sh > >>> +++ b/t/t0033-safe-directory.sh > >>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' ' > >>> expect_rejected_dir > >>> ' > >>> > >>> +test_expect_success 'ignoring safe.directory on the command line' ' > >>> + test_must_fail git -c safe.directory="$(pwd)" status 2>err && > >>> + grep "unsafe repository" err > >>> +' > >>> + > >>> +test_expect_success 'ignoring safe.directory in repo config' ' > >>> + ( > >>> + unset GIT_TEST_ASSUME_DIFFERENT_OWNER && > >>> + git config safe.directory "$(pwd)" > >>> + ) && > >>> + expect_rejected_dir > >>> +' > >> > >> I am debating myself if we want to remove the in-repository > >> safe.directory configuration setting after this test piece is done, > >> with test_when_finished. We just made sure, with this test, that > >> having the variable does not affect anything, so the subsequent > >> tests should not care hence it is probably OK. On the other hand, > >> to make sure those settings they make (e.g. setting it globally is > >> what the next test we can see below does) is what affects their > >> outcome, it removes one more thing we need to worry about if we > >> clean after ourselves. I dunno. > > > > test_config would do the same, right? I think it automatically > > does the test_when_finished for us. > > I thought it (specifically, anything depends on test_when_finished) > has trouble working well from inside a subprocess? Yeah, test_config doesn't work in a subshell, because modifying the variable holding the cleanup commands won't be visible after leaving the subshell, and the protection added in 0968f12a99 (test-lib-functions: detect test_when_finished in subshell, 2015-09-05) will kick in. And we do need a subshell to set the config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git config' would refuse to touch the config file. I think something like test_when_finished "( unset GIT_TEST_ASSUME_DIFFERENT_USER && git config --unset safe.directory )" would work, though. I considered cleaning up the config file, but all subsequent tests leave behind config settings for later tests as well (in the global config file, though).
On 4/29/2022 3:06 PM, SZEDER Gábor wrote: > On Fri, Apr 29, 2022 at 10:57:58AM -0700, Junio C Hamano wrote: >> Derrick Stolee <derrickstolee@github.com> writes: >>> test_config would do the same, right? I think it automatically >>> does the test_when_finished for us. >> >> I thought it (specifically, anything depends on test_when_finished) >> has trouble working well from inside a subprocess? > > Yeah, test_config doesn't work in a subshell, because modifying > the variable holding the cleanup commands won't be visible after > leaving the subshell, and the protection added in 0968f12a99 > (test-lib-functions: detect test_when_finished in subshell, > 2015-09-05) will kick in. And we do need a subshell to set the > config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git > config' would refuse to touch the config file. Ah yes, of course. > I think something like > > test_when_finished "( > unset GIT_TEST_ASSUME_DIFFERENT_USER && > git config --unset safe.directory > )" > > would work, though. Would it be simpler to use this? GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory Thanks, -Stolee
On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote: > > And we do need a subshell to set the > > config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git > > config' would refuse to touch the config file. > > Ah yes, of course. > > > I think something like > > > > test_when_finished "( > > unset GIT_TEST_ASSUME_DIFFERENT_USER && > > git config --unset safe.directory > > )" > > > > would work, though. > > Would it be simpler to use this? > > GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory Oh, wow. This is so obvious, no wonder it didn't occur to me :)
On Tue, May 10, 2022 at 08:33:21PM +0200, SZEDER Gábor wrote: > On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote: > > > And we do need a subshell to set the > > > config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git > > > config' would refuse to touch the config file. > > > > Ah yes, of course. > > > > > I think something like > > > > > > test_when_finished "( > > > unset GIT_TEST_ASSUME_DIFFERENT_USER && > > > git config --unset safe.directory > > > )" > > > > > > would work, though. > > > > Would it be simpler to use this? > > > > GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory > > Oh, wow. This is so obvious, no wonder it didn't occur to me :) Don't we consider this one-shot environment variable to be sticky on some shells (i.e., that it persists beyond just the "git config" invocation here)? Thanks, Taylor
On Tue, May 10, 2022 at 03:55:23PM -0400, Taylor Blau wrote: > > > > > > GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory > > > > Oh, wow. This is so obvious, no wonder it didn't occur to me :) > > Don't we consider this one-shot environment variable to be sticky on > some shells (i.e., that it persists beyond just the "git config" > invocation here)? do you have an example of such a shell?, I would assume that since the mechanism to implement these would be similar to local and we already require local for running our tests, that shouldn't be an issue (at least in the test suite), right? any such variables should be only set as part of the environment used by the posix shell before it call execve to invoke the next command IMHO. Carlo
On Tue, May 10, 2022 at 01:06:58PM -0700, Carlo Marcelo Arenas Belón wrote: > On Tue, May 10, 2022 at 03:55:23PM -0400, Taylor Blau wrote: > > > > > > > > GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory > > > > > > Oh, wow. This is so obvious, no wonder it didn't occur to me :) > > > > Don't we consider this one-shot environment variable to be sticky on > > some shells (i.e., that it persists beyond just the "git config" > > invocation here)? > > do you have an example of such a shell?, I would assume that since the > mechanism to implement these would be similar to local and we already > require local for running our tests, that shouldn't be an issue (at > least in the test suite), right? > > any such variables should be only set as part of the environment used > by the posix shell before it call execve to invoke the next command IMHO. This is completely my mistake, that stickiness exists only when invoking shell _functions_, not other commands (like "git"). I have gotten so used to looking for the former, that I didn't read carefully enough to realize that we are in the latter situation instead. > Carlo Thanks, Taylor
On 5/10/22 3:55 PM, Taylor Blau wrote: > On Tue, May 10, 2022 at 08:33:21PM +0200, SZEDER Gábor wrote: >> On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote: >>> Would it be simpler to use this? >>> >>> GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory >> >> Oh, wow. This is so obvious, no wonder it didn't occur to me :) > > Don't we consider this one-shot environment variable to be sticky on > some shells (i.e., that it persists beyond just the "git config" > invocation here)? Almost. Some old/buggy shells incorrectly allow the one-shot environment variable assignment to bleed past command invocation when the "command" is a shell function, like this: SOME_ENV_VAR=somevalue shell_function_call We do "lint"[*] for that sort of problem in t/check-non-portable-shell.pl. Stolee's example, however, is invoking a normal program (`git`, in this case), so isn't subject to that unwanted behavior from buggy shells. [*]: a0a630192d (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13)
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh index 6f9680e8b0..82dac0eb93 100755 --- a/t/t0033-safe-directory.sh +++ b/t/t0033-safe-directory.sh @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' ' expect_rejected_dir ' +test_expect_success 'ignoring safe.directory on the command line' ' + test_must_fail git -c safe.directory="$(pwd)" status 2>err && + grep "unsafe repository" err +' + +test_expect_success 'ignoring safe.directory in repo config' ' + ( + unset GIT_TEST_ASSUME_DIFFERENT_OWNER && + git config safe.directory "$(pwd)" + ) && + expect_rejected_dir +' + test_expect_success 'safe.directory does not match' ' git config --global safe.directory bogus && expect_rejected_dir
According to the documentation 'safe.directory' "is only respected when specified in a system or global config, not when it is specified in a repository config or via the command line option -c safe.directory=<path>". Add tests to check that 'safe.directory' in the repository config or on the command line is indeed ignored. Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com> --- t/t0033-safe-directory.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+)