| Message ID | 20190314214740.23360-1-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | commit-graph: segfault & other fixes for broken graphs | expand |
On Thu, Mar 14, 2019 at 5:47 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > I fixed a test to avoid the pattern a0a630192d > (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13) > tests for. The new pattern is more obvious. > > As an aside I don't get how that doesn't work as intended from the > commit message of that commit & its series. > > $ cat /tmp/x.sh > sayit() { echo "saying '$SAY'"; }; > SAY=hi sayit; sayit; > $ sh /tmp/x.sh > saying 'hi' > saying '' > > I get the same thing on bash, dash, NetBSD ksh, Solaris's shell & > AIX's. I.e. it's explicitly not assigning $SAY for the duration of the > shell [...] The shells you tested may all behave "sanely" given that input, but not all shells do. For instance, on MacOS, the factory-supplied bash 3.2.57 behaves in the "broken" way in 'sh' compatibility mode: $ cat /tmp/x.sh sayit() { echo "saying '$SAY'"; }; SAY=hi sayit; sayit; $ /bin/sh /tmp/x.sh saying 'hi' saying 'hi' $ /bin/bash /tmp/x.sh saying 'hi' saying ''
On Fri, Mar 15 2019, Eric Sunshine wrote: > On Thu, Mar 14, 2019 at 5:47 PM Ævar Arnfjörð Bjarmason > <avarab@gmail.com> wrote: >> I fixed a test to avoid the pattern a0a630192d >> (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13) >> tests for. The new pattern is more obvious. >> >> As an aside I don't get how that doesn't work as intended from the >> commit message of that commit & its series. >> >> $ cat /tmp/x.sh >> sayit() { echo "saying '$SAY'"; }; >> SAY=hi sayit; sayit; >> $ sh /tmp/x.sh >> saying 'hi' >> saying '' >> >> I get the same thing on bash, dash, NetBSD ksh, Solaris's shell & >> AIX's. I.e. it's explicitly not assigning $SAY for the duration of the >> shell [...] > > The shells you tested may all behave "sanely" given that input, but > not all shells do. For instance, on MacOS, the factory-supplied bash > 3.2.57 behaves in the "broken" way in 'sh' compatibility mode: > > $ cat /tmp/x.sh > sayit() { echo "saying '$SAY'"; }; > SAY=hi sayit; sayit; > > $ /bin/sh /tmp/x.sh > saying 'hi' > saying 'hi' > > $ /bin/bash /tmp/x.sh > saying 'hi' > saying '' Thanks. Also I found (with help from Freenode ##POSIX) that this part of the spec[1] talks about it: BEGIN QUOTE Variable assignments shall be performed as follows: [...] * If the command name is a function that is not a standard utility implemented as a function, variable assignments shall affect the current execution environment during the execution of the function. It is unspecified: - Whether or not the variable assignments persist after the completion of the function - Whether or not the variables gain the export attribute during the execution of the function - Whether or not export attributes gained as a result of the variable assignments persist after the completion of the function (if variable assignments persist after the completion of the function) END QUOTE I.e. this is undefined behavior that just happened to work on the various shells I tested. Makes sense, was just wondering if I'd entirely missed out on what behavior was, turns out it's just fairly rare. FWIW My bash 5.0.2 on Debian behaves as I noted in both sh and bash mode. 1. http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
See the v1 cover letter for details: https://public-inbox.org/git/20190221223753.20070-1-avarab@gmail.com/ I'd forgotten this after 2.21 was released. This addresses all the comments on v1 and rebases it. A range-diff is below. I also improved 7/8's commit message a bit. I fixed a test to avoid the pattern a0a630192d (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13) tests for. The new pattern is more obvious. As an aside I don't get how that doesn't work as intended from the commit message of that commit & its series. $ cat /tmp/x.sh sayit() { echo "saying '$SAY'"; }; SAY=hi sayit; sayit; $ sh /tmp/x.sh saying 'hi' saying '' I get the same thing on bash, dash, NetBSD ksh, Solaris's shell & AIX's. I.e. it's explicitly not assigning $SAY for the duration of the shell as this would do: $ cat /tmp/y.sh sayit() { echo "saying '$SAY'"; }; SAY=hi; sayit; sayit; $ sh /tmp/y.sh saying 'hi' saying 'hi' Which is the impression I get from the commit message. Ævar Arnfjörð Bjarmason (8): commit-graph tests: split up corrupt_graph_and_verify() commit-graph tests: test a graph that's too small commit-graph: fix segfault on e.g. "git status" commit-graph: don't early exit(1) on e.g. "git status" commit-graph: don't pass filename to load_commit_graph_one_fd_st() commit-graph verify: detect inability to read the graph commit-graph write: don't die if the existing graph is corrupt commit-graph: improve & i18n error messages builtin/commit-graph.c | 23 +++++-- commit-graph.c | 132 +++++++++++++++++++++++++++------------- commit-graph.h | 4 ++ commit.h | 6 ++ t/t5318-commit-graph.sh | 42 +++++++++++-- 5 files changed, 154 insertions(+), 53 deletions(-) Range-diff: 1: 9d318d5106 ! 1: 2f8ba0adf8 commit-graph tests: split up corrupt_graph_and_verify() @@ -49,7 +49,7 @@ - test_when_finished mv commit-graph-backup $objdir/info/commit-graph && - cp $objdir/info/commit-graph commit-graph-backup && printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc && - dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" count=0 && + dd of="$objdir/info/commit-graph" bs=1 seek="$zero_pos" if=/dev/null && generate_zero_bytes $(($orig_size - $zero_pos)) >>"$objdir/info/commit-graph" && - test_must_fail git commit-graph verify 2>test_err && - grep -v "^+" test_err >err && 2: 73849add5e = 2: 800b17edde commit-graph tests: test a graph that's too small 3: 6bfce758e1 = 3: 7083ab81c7 commit-graph: fix segfault on e.g. "git status" 4: ac07ff415e = 4: d00564ae89 commit-graph: don't early exit(1) on e.g. "git status" 5: b2dd394cc7 = 5: 25ee185bf7 commit-graph: don't pass filename to load_commit_graph_one_fd_st() 6: 9987149e5c ! 6: 7619b46987 commit-graph verify: detect inability to read the graph @@ -37,16 +37,10 @@ } -+test_expect_success 'detect permission problem' ' ++test_expect_success POSIXPERM,SANITY 'detect permission problem' ' + corrupt_graph_setup && + chmod 000 $objdir/info/commit-graph && -+ -+ # Skip as root, or in other cases (odd fs or OS) where a -+ # "chmod 000 file" does not yield EACCES on e.g. "cat file" -+ if ! test -r $objdir/info/commit-graph -+ then -+ corrupt_graph_verify "Could not open" -+ fi ++ corrupt_graph_verify "Could not open" +' + test_expect_success 'detect too small' ' 7: 0e35b12a1a ! 7: 17ee4fc050 commit-graph write: don't die if the existing graph is corrupt @@ -18,6 +18,10 @@ use_commit_graph=1 as seen in 177722b344 ("commit: integrate commit graph with commit parsing", 2018-04-10). + Not using the old graph at all slows down the writing of the new graph + by some small amount, but is a sensible way to prevent an error in the + existing commit-graph from spreading. + Just fixing the current issue would be likely to result in code that's inadvertently broken in the future. New code might use the commit-graph at a distance. To detect such cases introduce a @@ -36,7 +40,12 @@ corruption. This might need to be re-visited if we learn to write the commit-graph - incrementally. + incrementally, but probably not. Hopefully we'll just start by finding + out what commits we have in total, then read the old graph(s) to see + what they cover, and finally write a new graph file with everything + that's missing. In that case the new graph writing code just needs to + continue to use e.g. a parse_commit() that doesn't consult the + existing commit-graphs. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> @@ -119,7 +128,7 @@ grep -v "^+" test_err >err && test_i18ngrep "$grepstr" err && - git status --short -+ if test -z "$NO_WRITE_TEST_BACKUP" ++ if test "$2" != "no-copy" + then + cp $objdir/info/commit-graph commit-graph-pre-write-test + fi && @@ -130,14 +139,14 @@ # usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>] @@ - # "chmod 000 file" does not yield EACCES on e.g. "cat file" - if ! test -r $objdir/info/commit-graph - then -- corrupt_graph_verify "Could not open" -+ NO_WRITE_TEST_BACKUP=1 corrupt_graph_verify "Could not open" - fi + test_expect_success POSIXPERM,SANITY 'detect permission problem' ' + corrupt_graph_setup && + chmod 000 $objdir/info/commit-graph && +- corrupt_graph_verify "Could not open" ++ corrupt_graph_verify "Could not open" "no-copy" ' + test_expect_success 'detect too small' ' @@ git fsck && corrupt_graph_and_verify $GRAPH_BYTE_FOOTER "\00" \ 8: a74d0f0f6f = 8: 29ab2895b7 commit-graph: improve & i18n error messages