Re: ksh, test: test -t file descriptor isn't optional
I just checked and it seems that while zsh supports the pre-POSIX behavior of "test -t" using fd 1, both dash and bash treat "-t" as a string by default when there is no fd specified. So I think this change is fine. OK millert@ for your revised dif if you add the missing ';' after the "return 0". - todd
Re: ksh, test: test -t file descriptor isn't optional
Omar Polo wrote: > sorry for the delay, this is another mail that I meant to take a look > earlier... Thanks for the review, Omar! > > > together with a small > > > rewrite in test_eval TO_FILTT case, as it was a bit too difficult for me > > > to read: it seems that, for legacy reason (haven't looked at the code > > > history), opnd1 could be NULL for TO_FILTT, the only test with such > > > behaviour. My understanding is that ptest_getopnd avoids that by > > > returning "1". So, opnd1 can't be NULL. bi_getn writes to res, but took > > > me a while to understand that looking only at the conditional call of > > > bi_getn in the if condition. Finally, for some reason, is opnd1 managed > > > to be NULL, isatty(0) is called. I believe that the intention was to do > > > > > > res = opnd1 ? isatty(res) : 0; > > > > > > instead. I think the proposed rewrite is easier to follow. > > I think that the rewrite is more complex than needed. I'm attaching a > slightly revised diff that is closer to the current code. I agree and I like your chunk better. I tried to write as dummy-proof as possible so it was easier to follow for me.
Re: ksh, test: test -t file descriptor isn't optional
sorry for the delay, this is another mail that I meant to take a look earlier... On 2023/05/29 16:36:45 +, Lucas wrote: > Ping. > > > Hi tech@, > > > > Both test.1 and ksh.1 (under the non-POSIX compatibility flag) state > > that `test -t` will default to test whether fd 1 is a TTY if the > > argument is omitted. This isn't the case, and both treat `-t` as the > > equivalent of `test -n -t`, ie, test if `-t` is a non-empty string, > > trivially true. > > > > It's easy to see it in `test`: it exits right away in switch(argc). agreed. The "(default 1)" in test.1 is completely wrong. > > `ksh` is a bit more difficult to follow: builtins eventually call c_test > > in c_test.c. For `test -t`, c_test will be called with wp being "test", > > "-t". It'll eventually call test_parse, with the test environment set > > with > > > > te.pos.wp = wp + 1; > > te.wp_end = wp + argc; > > > > For this particular case, argc is 2. Bearing that in mind, test_parse > > will make a call stack of test_aexpr -> test_oexpr -> test_nexpr -> > > test_primary. The first 2 ifs are skipped (asuming no TEF_ERROR is set) > > and the next if, which would handle `test -t` gracefully, isn't entered: > > one of the && operands, &te->pos.wp[1] < te->wp_end, is false. > > Afterwards, `-t` is evaled as TO_SNTZE, ie if it's a non-empty string. Agree on this too. > > The following diff amends the manpages, removing the "file descriptor > > is optional" parts, and removes the unused machinery of bin/ksh/c_test.c > > to deal with an optional argument in `test -t`, for what is worth I agree with you. It's a historic behaviour that has been broken for some time already and noone seems to have noticed, I think it could go away. > > together with a small > > rewrite in test_eval TO_FILTT case, as it was a bit too difficult for me > > to read: it seems that, for legacy reason (haven't looked at the code > > history), opnd1 could be NULL for TO_FILTT, the only test with such > > behaviour. My understanding is that ptest_getopnd avoids that by > > returning "1". So, opnd1 can't be NULL. bi_getn writes to res, but took > > me a while to understand that looking only at the conditional call of > > bi_getn in the if condition. Finally, for some reason, is opnd1 managed > > to be NULL, isatty(0) is called. I believe that the intention was to do > > > > res = opnd1 ? isatty(res) : 0; > > > > instead. I think the proposed rewrite is easier to follow. I think that the rewrite is more complex than needed. I'm attaching a slightly revised diff that is closer to the current code. > > Regress is happy, although nothing in there tests `test -t` or `test -t > > n`. Existing scripts shouldn't break with this change: `test -t` will > > keep being true, whether fd 1 is a TTY or not. The diff can be further > > split on man changes and code changes if needed. > > > > btw, the easy test for `test -t` being wrong, whether under POSIX compat > > or not, is > > > > $ test -t 1; echo $? # 1 is TTY in an interactive session > > 0 > > $ test -t 1 >&-; echo $? # 1 isn't a TTY because it's closed > > 1 > > $ test -t >&-; echo $? > > 0 bash and ash behave the same out-of-the-box (i.e. without any eventual posix flag set.) I'd prefer some more feedback on this before going on. diff /usr/src commit - 15eb8637ab039139400e655284e2e2d8ca898a03 path + /usr/src blob - 7038a52bfa432d515d9683187930407c4d6bd6d5 file + bin/ksh/c_test.c --- bin/ksh/c_test.c +++ bin/ksh/c_test.c @@ -156,12 +156,6 @@ c_test(char **wp) } if (argc == 1) { opnd1 = (*te.getopnd)(&te, TO_NONOP, 1); - /* Historically, -t by itself test if fd 1 -* is a file descriptor, but POSIX says its -* a string test... -*/ - if (!Flag(FPOSIX) && strcmp(opnd1, "-t") == 0) - break; res = (*te.eval)(&te, TO_STNZE, opnd1, NULL, 1); if (invert & 1) @@ -271,14 +265,11 @@ test_eval(Test_env *te, Test_op op, const char *opnd1, case TO_FILGZ: /* -s */ return stat(opnd1, &b1) == 0 && b1.st_size > 0L; case TO_FILTT: /* -t */ - if (opnd1 && !bi_getn(opnd1, &res)) { + if (!bi_getn(opnd1, &res)) { te->flags |= TEF_ERROR; - res = 0; - } else { - /* generate error if in FPOSIX mode? */ - res = isatty(opnd1 ? res : 0); + return 0 } - return res; + return isatty(res); case TO_FILUID: /* -O */ return stat(opnd1, &b1) == 0 && b1.st_uid == ksheuid; case TO_FILGID: /* -G
Re: ksh, test: test -t file descriptor isn't optional
Ping. > Hi tech@, > > Both test.1 and ksh.1 (under the non-POSIX compatibility flag) state > that `test -t` will default to test whether fd 1 is a TTY if the > argument is omitted. This isn't the case, and both treat `-t` as the > equivalent of `test -n -t`, ie, test if `-t` is a non-empty string, > trivially true. > > It's easy to see it in `test`: it exits right away in switch(argc). > `ksh` is a bit more difficult to follow: builtins eventually call c_test > in c_test.c. For `test -t`, c_test will be called with wp being "test", > "-t". It'll eventually call test_parse, with the test environment set > with > > te.pos.wp = wp + 1; > te.wp_end = wp + argc; > > For this particular case, argc is 2. Bearing that in mind, test_parse > will make a call stack of test_aexpr -> test_oexpr -> test_nexpr -> > test_primary. The first 2 ifs are skipped (asuming no TEF_ERROR is set) > and the next if, which would handle `test -t` gracefully, isn't entered: > one of the && operands, &te->pos.wp[1] < te->wp_end, is false. > Afterwards, `-t` is evaled as TO_SNTZE, ie if it's a non-empty string. > > The following diff amends the manpages, removing the "file descriptor > is optional" parts, and removes the unused machinery of bin/ksh/c_test.c > to deal with an optional argument in `test -t`, together with a small > rewrite in test_eval TO_FILTT case, as it was a bit too difficult for me > to read: it seems that, for legacy reason (haven't looked at the code > history), opnd1 could be NULL for TO_FILTT, the only test with such > behaviour. My understanding is that ptest_getopnd avoids that by > returning "1". So, opnd1 can't be NULL. bi_getn writes to res, but took > me a while to understand that looking only at the conditional call of > bi_getn in the if condition. Finally, for some reason, is opnd1 managed > to be NULL, isatty(0) is called. I believe that the intention was to do > > res = opnd1 ? isatty(res) : 0; > > instead. I think the proposed rewrite is easier to follow. > > Regress is happy, although nothing in there tests `test -t` or `test -t > n`. Existing scripts shouldn't break with this change: `test -t` will > keep being true, whether fd 1 is a TTY or not. The diff can be further > split on man changes and code changes if needed. > > btw, the easy test for `test -t` being wrong, whether under POSIX compat > or not, is > > $ test -t 1; echo $? # 1 is TTY in an interactive session > 0 > $ test -t 1 >&-; echo $? # 1 isn't a TTY because it's closed > 1 > $ test -t >&-; echo $? > 0 diff refs/heads/master refs/heads/ksh-t-fd commit - 55055d619d36cc45f8c6891404c51cd405214e86 commit + 00fa44f8caccc78154e093b5fa719e392716b81e blob - 7038a52bfa432d515d9683187930407c4d6bd6d5 blob + db16b71a9b64afb7047803c65ec620f03e18e42d --- bin/ksh/c_test.c +++ bin/ksh/c_test.c @@ -156,12 +156,6 @@ c_test(char **wp) } if (argc == 1) { opnd1 = (*te.getopnd)(&te, TO_NONOP, 1); - /* Historically, -t by itself test if fd 1 -* is a file descriptor, but POSIX says its -* a string test... -*/ - if (!Flag(FPOSIX) && strcmp(opnd1, "-t") == 0) - break; res = (*te.eval)(&te, TO_STNZE, opnd1, NULL, 1); if (invert & 1) @@ -271,14 +265,18 @@ test_eval(Test_env *te, Test_op op, const char *opnd1, case TO_FILGZ: /* -s */ return stat(opnd1, &b1) == 0 && b1.st_size > 0L; case TO_FILTT: /* -t */ - if (opnd1 && !bi_getn(opnd1, &res)) { - te->flags |= TEF_ERROR; - res = 0; - } else { - /* generate error if in FPOSIX mode? */ - res = isatty(opnd1 ? res : 0); + { + int s, v; + + s = bi_getn(opnd1, &v); + if (!s) { + te->flags |= TEF_ERROR; + res = 0; + } else { + res = isatty(v); + } + return res; } - return res; case TO_FILUID: /* -O */ return stat(opnd1, &b1) == 0 && b1.st_uid == ksheuid; case TO_FILGID: /* -G */ @@ -527,7 +525,7 @@ ptest_getopnd(Test_env *te, Test_op op, int do_eval) ptest_getopnd(Test_env *te, Test_op op, int do_eval) { if (te->pos.wp >= te->wp_end) - return op == TO_FILTT ? "1" : NULL; + return NULL; return *te->pos.wp++; } blob - cd3bfc3ebf41217b46c69eaff634cd8e9771c425 blob + c8406305e706cff12711bbcb934f13d67f49ebdf ---
Re: ksh, test: test -t file descriptor isn't optional
Several months bump. Lucas wrote: > Hi tech@, > > Both test.1 and ksh.1 (under the non-POSIX compatibility flag) state > that `test -t` will default to test whether fd 1 is a TTY if the > argument is omitted. This isn't the case, and both treat `-t` as the > equivalent of `test -n -t`, ie, test if `-t` is a non-empty string, > trivially true. > > It's easy to see it in `test`: it exits right away in switch(argc). > `ksh` is a bit more difficult to follow: builtins eventually call c_test > in c_test.c. For `test -t`, c_test will be called with wp being "test", > "-t". It'll eventually call test_parse, with the test environment set > with > > te.pos.wp = wp + 1; > te.wp_end = wp + argc; > > For this particular case, argc is 2. Bearing that in mind, test_parse > will make a call stack of test_aexpr -> test_oexpr -> test_nexpr -> > test_primary. The first 2 ifs are skipped (asuming no TEF_ERROR is set) > and the next if, which would handle `test -t` gracefully, isn't entered: > one of the && operands, &te->pos.wp[1] < te->wp_end, is false. > Afterwards, `-t` is evaled as TO_SNTZE, ie if it's a non-empty string. > > The following diff amends the manpages, removing the "file descriptor > is optional" parts, and removes the unused machinery of bin/ksh/c_test.c > to deal with an optional argument in `test -t`, together with a small > rewrite in test_eval TO_FILTT case, as it was a bit too difficult for me > to read: it seems that, for legacy reason (haven't looked at the code > history), opnd1 could be NULL for TO_FILTT, the only test with such > behaviour. My understanding is that ptest_getopnd avoids that by > returning "1". So, opnd1 can't be NULL. bi_getn writes to res, but took > me a while to understand that looking only at the conditional call of > bi_getn in the if condition. Finally, for some reason, is opnd1 managed > to be NULL, isatty(0) is called. I believe that the intention was to do > > res = opnd1 ? isatty(res) : 0; > > instead. I think the proposed rewrite is easier to follow. > > Regress is happy, although nothing in there tests `test -t` or `test -t > n`. Existing scripts shouldn't break with this change: `test -t` will > keep being true, whether fd 1 is a TTY or not. The diff can be further > split on man changes and code changes if needed. > > btw, the easy test for `test -t` being wrong, whether under POSIX compat > or not, is > > $ test -t 1; echo $? # 1 is TTY in an interactive session > 0 > $ test -t 1 >&-; echo $? # 1 isn't a TTY because it's closed > 1 > $ test -t >&-; echo $? > 0 diff refs/heads/master refs/heads/ksh-t-fd commit - 55055d619d36cc45f8c6891404c51cd405214e86 commit + 00fa44f8caccc78154e093b5fa719e392716b81e blob - 7038a52bfa432d515d9683187930407c4d6bd6d5 blob + db16b71a9b64afb7047803c65ec620f03e18e42d --- bin/ksh/c_test.c +++ bin/ksh/c_test.c @@ -156,12 +156,6 @@ c_test(char **wp) } if (argc == 1) { opnd1 = (*te.getopnd)(&te, TO_NONOP, 1); - /* Historically, -t by itself test if fd 1 -* is a file descriptor, but POSIX says its -* a string test... -*/ - if (!Flag(FPOSIX) && strcmp(opnd1, "-t") == 0) - break; res = (*te.eval)(&te, TO_STNZE, opnd1, NULL, 1); if (invert & 1) @@ -271,14 +265,18 @@ test_eval(Test_env *te, Test_op op, const char *opnd1, case TO_FILGZ: /* -s */ return stat(opnd1, &b1) == 0 && b1.st_size > 0L; case TO_FILTT: /* -t */ - if (opnd1 && !bi_getn(opnd1, &res)) { - te->flags |= TEF_ERROR; - res = 0; - } else { - /* generate error if in FPOSIX mode? */ - res = isatty(opnd1 ? res : 0); + { + int s, v; + + s = bi_getn(opnd1, &v); + if (!s) { + te->flags |= TEF_ERROR; + res = 0; + } else { + res = isatty(v); + } + return res; } - return res; case TO_FILUID: /* -O */ return stat(opnd1, &b1) == 0 && b1.st_uid == ksheuid; case TO_FILGID: /* -G */ @@ -527,7 +525,7 @@ ptest_getopnd(Test_env *te, Test_op op, int do_eval) ptest_getopnd(Test_env *te, Test_op op, int do_eval) { if (te->pos.wp >= te->wp_end) - return op == TO_FILTT ? "1" : NULL; + return NULL; return *te->pos.wp++; } blob - cd3bfc3ebf41217b46c69eaff634cd8e9771c425 blob + c8406305e706cff1
Re: ksh, test: test -t file descriptor isn't optional
Bump. In the meantime I did some digging. The issue was introduced by r1.18, 2009-03-01. There, `test -t` should've worked as advertised, as the first check in test_primary would be whether the operand is unary. > Hi tech@, > > Both test.1 and ksh.1 (under the non-POSIX compatibility flag) state > that `test -t` will default to test whether fd 1 is a TTY if the > argument is omitted. This isn't the case, and both treat `-t` as the > equivalent of `test -n -t`, ie, test if `-t` is a non-empty string, > trivially true. > > It's easy to see it in `test`: it exits right away in switch(argc). > `ksh` is a bit more difficult to follow: builtins eventually call c_test > in c_test.c. For `test -t`, c_test will be called with wp being "test", > "-t". It'll eventually call test_parse, with the test environment set > with > > te.pos.wp = wp + 1; > te.wp_end = wp + argc; > > For this particular case, argc is 2. Bearing that in mind, test_parse > will make a call stack of test_aexpr -> test_oexpr -> test_nexpr -> > test_primary. The first 2 ifs are skipped (asuming no TEF_ERROR is set) > and the next if, which would handle `test -t` gracefully, isn't entered: > one of the && operands, &te->pos.wp[1] < te->wp_end, is false. > Afterwards, `-t` is evaled as TO_SNTZE, ie if it's a non-empty string. > > The following diff amends the manpages, removing the "file descriptor > is optional" parts, and removes the unused machinery of bin/ksh/c_test.c > to deal with an optional argument in `test -t`, together with a small > rewrite in test_eval TO_FILTT case, as it was a bit too difficult for me > to read: it seems that, for legacy reason (haven't looked at the code > history), opnd1 could be NULL for TO_FILTT, the only test with such > behaviour. My understanding is that ptest_getopnd avoids that by > returning "1". So, opnd1 can't be NULL. bi_getn writes to res, but took > me a while to understand that looking only at the conditional call of > bi_getn in the if condition. Finally, for some reason, is opnd1 managed > to be NULL, isatty(0) is called. I believe that the intention was to do > > res = opnd1 ? isatty(res) : 0; > > instead. I think the proposed rewrite is easier to follow. > > Regress is happy, although nothing in there tests `test -t` or `test -t > n`. Existing scripts shouldn't break with this change: `test -t` will > keep being true, whether fd 1 is a TTY or not. The diff can be further > split on man changes and code changes if needed. > > btw, the easy test for `test -t` being wrong, whether under POSIX compat > or not, is > > $ test -t 1; echo $? # 1 is TTY in an interactive session > 0 > $ test -t 1 >&-; echo $? # 1 isn't a TTY because it's closed > 1 > $ test -t >&-; echo $? > 0 bye, Lucas diff 45d281fcfba6e40007d9a498265cdbf711d94ed0 ebffbda379cd24f3bb8c9e43b712b9d699c9980c commit - 45d281fcfba6e40007d9a498265cdbf711d94ed0 commit + ebffbda379cd24f3bb8c9e43b712b9d699c9980c blob - 7038a52bfa432d515d9683187930407c4d6bd6d5 blob + db16b71a9b64afb7047803c65ec620f03e18e42d --- bin/ksh/c_test.c +++ bin/ksh/c_test.c @@ -156,12 +156,6 @@ c_test(char **wp) } if (argc == 1) { opnd1 = (*te.getopnd)(&te, TO_NONOP, 1); - /* Historically, -t by itself test if fd 1 -* is a file descriptor, but POSIX says its -* a string test... -*/ - if (!Flag(FPOSIX) && strcmp(opnd1, "-t") == 0) - break; res = (*te.eval)(&te, TO_STNZE, opnd1, NULL, 1); if (invert & 1) @@ -271,14 +265,18 @@ test_eval(Test_env *te, Test_op op, const char *opnd1, case TO_FILGZ: /* -s */ return stat(opnd1, &b1) == 0 && b1.st_size > 0L; case TO_FILTT: /* -t */ - if (opnd1 && !bi_getn(opnd1, &res)) { - te->flags |= TEF_ERROR; - res = 0; - } else { - /* generate error if in FPOSIX mode? */ - res = isatty(opnd1 ? res : 0); + { + int s, v; + + s = bi_getn(opnd1, &v); + if (!s) { + te->flags |= TEF_ERROR; + res = 0; + } else { + res = isatty(v); + } + return res; } - return res; case TO_FILUID: /* -O */ return stat(opnd1, &b1) == 0 && b1.st_uid == ksheuid; case TO_FILGID: /* -G */ @@ -527,7 +525,7 @@ ptest_getopnd(Test_env *te, Test_op op, int do_eval) ptest_getopnd(Test_env *te, Test_op op, int do_
ksh, test: test -t file descriptor isn't optional
Hi tech@, Both test.1 and ksh.1 (under the non-POSIX compatibility flag) state that `test -t` will default to test whether fd 1 is a TTY if the argument is omitted. This isn't the case, and both treat `-t` as the equivalent of `test -n -t`, ie, test if `-t` is a non-empty string, trivially true. It's easy to see it in `test`: it exits right away in switch(argc). `ksh` is a bit more difficult to follow: builtins eventually call c_test in c_test.c. For `test -t`, c_test will be called with wp being "test", "-t". It'll eventually call test_parse, with the test environment set with te.pos.wp = wp + 1; te.wp_end = wp + argc; For this particular case, argc is 2. Bearing that in mind, test_parse will make a call stack of test_aexpr -> test_oexpr -> test_nexpr -> test_primary. The first 2 ifs are skipped (asuming no TEF_ERROR is set) and the next if, which would handle `test -t` gracefully, isn't entered: one of the && operands, &te->pos.wp[1] < te->wp_end, is false. Afterwards, `-t` is evaled as TO_SNTZE, ie if it's a non-empty string. The following diff amends the manpages, removing the "file descriptor is optional" parts, and removes the unused machinery of bin/ksh/c_test.c to deal with an optional argument in `test -t`, together with a small rewrite in test_eval TO_FILTT case, as it was a bit too difficult for me to read: it seems that, for legacy reason (haven't looked at the code history), opnd1 could be NULL for TO_FILTT, the only test with such behaviour. My understanding is that ptest_getopnd avoids that by returning "1". So, opnd1 can't be NULL. bi_getn writes to res, but took me a while to understand that looking only at the conditional call of bi_getn in the if condition. Finally, for some reason, is opnd1 managed to be NULL, isatty(0) is called. I believe that the intention was to do res = opnd1 ? isatty(res) : 0; instead. I think the proposed rewrite is easier to follow. Regress is happy, although nothing in there tests `test -t` or `test -t n`. Existing scripts shouldn't break with this change: `test -t` will keep being true, whether fd 1 is a TTY or not. The diff can be further split on man changes and code changes if needed. btw, the easy test for `test -t` being wrong, whether under POSIX compat or not, is $ test -t 1; echo $? # 1 is TTY in an interactive session 0 $ test -t 1 >&-; echo $? # 1 isn't a TTY because it's closed 1 $ test -t >&-; echo $? 0 bye, Lucas diff 45d281fcfba6e40007d9a498265cdbf711d94ed0 ebffbda379cd24f3bb8c9e43b712b9d699c9980c commit - 45d281fcfba6e40007d9a498265cdbf711d94ed0 commit + ebffbda379cd24f3bb8c9e43b712b9d699c9980c blob - 7038a52bfa432d515d9683187930407c4d6bd6d5 blob + db16b71a9b64afb7047803c65ec620f03e18e42d --- bin/ksh/c_test.c +++ bin/ksh/c_test.c @@ -156,12 +156,6 @@ c_test(char **wp) } if (argc == 1) { opnd1 = (*te.getopnd)(&te, TO_NONOP, 1); - /* Historically, -t by itself test if fd 1 -* is a file descriptor, but POSIX says its -* a string test... -*/ - if (!Flag(FPOSIX) && strcmp(opnd1, "-t") == 0) - break; res = (*te.eval)(&te, TO_STNZE, opnd1, NULL, 1); if (invert & 1) @@ -271,14 +265,18 @@ test_eval(Test_env *te, Test_op op, const char *opnd1, case TO_FILGZ: /* -s */ return stat(opnd1, &b1) == 0 && b1.st_size > 0L; case TO_FILTT: /* -t */ - if (opnd1 && !bi_getn(opnd1, &res)) { - te->flags |= TEF_ERROR; - res = 0; - } else { - /* generate error if in FPOSIX mode? */ - res = isatty(opnd1 ? res : 0); + { + int s, v; + + s = bi_getn(opnd1, &v); + if (!s) { + te->flags |= TEF_ERROR; + res = 0; + } else { + res = isatty(v); + } + return res; } - return res; case TO_FILUID: /* -O */ return stat(opnd1, &b1) == 0 && b1.st_uid == ksheuid; case TO_FILGID: /* -G */ @@ -527,7 +525,7 @@ ptest_getopnd(Test_env *te, Test_op op, int do_eval) ptest_getopnd(Test_env *te, Test_op op, int do_eval) { if (te->pos.wp >= te->wp_end) - return op == TO_FILTT ? "1" : NULL; + return NULL; return *te->pos.wp++; } blob - 0e97b9b49a65d19722f1762d4cc0f36ffa6fae72 blob + 8d1542590b44b8b2aff63d44abe575d6845ed549 --- bin/ksh/ksh.1 +++ bin/ksh/ksh.1 @@ -2569