| Message ID | b734425e3235651e738e6eac47eae0db7db92e7e.1609861567.git.matheus.bernardino@usp.br (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7a54dd92d887f5482a5b2b97b87f144faf1c01a9 |
| Headers | show |
| Series | [v2] t4129: don't fail if setgid is set in the test directory | expand |
Matheus Tavares <matheus.bernardino@usp.br> writes: > +# Get the modebits from a file or directory, ignoring the setgid bit (g+s). > +# This bit is inherited by subdirectories at their creation. So we remove it > +# from the returning string to prevent callers from having to worry about the > +# state of the bit in the test directory. > +# We probably do not use "chmod g+s" manually on regular files, so I may be being overly "correct", but shouldn't these be done only for directories? > test_modebits () { > - ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' > + ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' \ > + -e 's|^\(......\)S|\1-|' -e 's|^\(......\)s|\1x|' That is, -e 's|^\(d.....\)S|\1-|' -e 's|^\(d.....\)s|\1x|' instead of applying the rule to any filetype. Will queue as-is, as the distinction probably would not matter in practice. Thanks.
On Wed, Jan 6, 2021 at 8:59 PM Junio C Hamano <gitster@pobox.com> wrote: > > Matheus Tavares <matheus.bernardino@usp.br> writes: > > > +# Get the modebits from a file or directory, ignoring the setgid bit (g+s). > > +# This bit is inherited by subdirectories at their creation. So we remove it > > +# from the returning string to prevent callers from having to worry about the > > +# state of the bit in the test directory. > > +# > > We probably do not use "chmod g+s" manually on regular files, so I > may be being overly "correct", but shouldn't these be done only for > directories? > > > test_modebits () { > > - ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' > > + ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' \ > > + -e 's|^\(......\)S|\1-|' -e 's|^\(......\)s|\1x|' > > That is, > > -e 's|^\(d.....\)S|\1-|' -e 's|^\(d.....\)s|\1x|' > > instead of applying the rule to any filetype. Yeah, you're right. I ended up applying the rule on regular files as well just for standardization. That is, if some day, for some reason, a test script decides to use "chmod g+s" on a regular file and a directory, test_modebits would treat them equally to avoid any confusion. But I guess it's very unlikely that we will ever need to set the setgid bit on a test, anyway... > Will queue as-is, as the distinction probably would not matter in > practice. > > Thanks.
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 999982fe4a..2f08ce7cba 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -367,9 +367,14 @@ test_chmod () { git update-index --add "--chmod=$@" } -# Get the modebits from a file or directory. +# Get the modebits from a file or directory, ignoring the setgid bit (g+s). +# This bit is inherited by subdirectories at their creation. So we remove it +# from the returning string to prevent callers from having to worry about the +# state of the bit in the test directory. +# test_modebits () { - ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' + ls -ld "$1" | sed -e 's|^\(..........\).*|\1|' \ + -e 's|^\(......\)S|\1-|' -e 's|^\(......\)s|\1x|' } # Unset a configuration variable, but don't fail if it doesn't exist.
The last test of t4129 creates a directory and expects its setgid bit (g+s) to be off. But this makes the test fail when the parent directory has the bit set, as setgid's state is inherited by newly created subdirectories. One way to solve this problem is to allow the presence of this bit when comparing the return of `test_modebits` with the expected value. But then we may have the same problem in the future when other tests start using `test_modebits` on directories (currently t4129 is the only one) and forget about setgid. Instead, let's make the helper function more robust with respect to the state of the setgid bit in the test directory by removing this bit from the returning value. There should be no problem with existing callers as no one currently expects this bit to be on. Note that the sticky bit (+t) and the setuid bit (u+s) are not inherited, so we don't have to worry about those. Reported-by: Kevin Daudt <me@ikke.info> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> --- t/test-lib-functions.sh | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)