| Message ID | 20191226195747.GC170890@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | avoid use of "VAR= cmd" with a shell function (Re: [PATCH 3/5] test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate) | expand |
Jonathan Nieder <jrnieder@gmail.com> writes: > Just like assigning a nonempty value, assigning an empty value to a > shell variable when calling a function produces non-portable behavior: > in some shells, the assignment lasts for the duration of the function > invocation, and in others, it persists after the function returns. > ... > diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl > index 38bfeebd88..fd3303552b 100755 > --- a/t/check-non-portable-shell.pl > +++ b/t/check-non-portable-shell.pl > @@ -46,7 +46,7 @@ sub err { > /(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)'; > /\bgrep\b.*--file\b/ and err 'grep --file FILE is not portable (use grep -f FILE)'; > /\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)'; > - /^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and > + /^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and Thanks for a quick fix. It is kind-of surprising that there was only one existing offender.
Jonathan Nieder <jrnieder@gmail.com> writes: > Just like assigning a nonempty value, assigning an empty value to a > shell variable when calling a function produces non-portable behavior: > in some shells, the assignment lasts for the duration of the function > invocation, and in others, it persists after the function returns. > > Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> > --- > If it would be useful for me to send a copy of the "Enable protocol v2 > by default" series rebased on top of this, let me know. When rebased, t5552 passes (including the test lint) at the "request v0 explicitly for some tests" step now. The tip of "promote proto v2 to default" series fails at 5552.5 with or without these two patches, though.
On Thu, Dec 26, 2019 at 2:57 PM Jonathan Nieder <jrnieder@gmail.com> wrote: > Just like assigning a nonempty value, assigning an empty value to a > shell variable when calling a function produces non-portable behavior: > in some shells, the assignment lasts for the duration of the function > invocation, and in others, it persists after the function returns. > > Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> > --- > diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl > @@ -46,7 +46,7 @@ sub err { > - /^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and > + /^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and > err '"FOO=bar shell_func" assignment extends beyond "shell_func"'; Thanks, the change makes sense. I suspect that I simply overlooked this case when implementing this. Acked-by: me
Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: >> Just like assigning a nonempty value, assigning an empty value to a >> shell variable when calling a function produces non-portable behavior: >> in some shells, the assignment lasts for the duration of the function >> invocation, and in others, it persists after the function returns. >> >> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> >> --- >> If it would be useful for me to send a copy of the "Enable protocol v2 >> by default" series rebased on top of this, let me know. > > When rebased, t5552 passes (including the test lint) at the "request > v0 explicitly for some tests" step now. > > The tip of "promote proto v2 to default" series fails at 5552.5 > with or without these two patches, though. Oh, subtle. With shells that leak variable assignments after a function returns (such as bash when run as 'sh'), 5552.5 was running with GIT_TEST_PROTOCOL_VERSION=0, masking the issue. In protocol v2, there is no "stateful" mode: negotiation always uses the stateless-rpc path, and the stateless-rpc path involves more care to avoid chatter during negotiation (since request size increases with each round). This is why b1.c14 and b1.c9 don't show up in the v2 trace. Processing the trace with "git name-rev --stdin" yields packet: fetch> want 184bd23dc533e1e63153e7e181411bd29acca918 packet: fetch> have f65fc9b4d5c1cb76494a7f8df0230d8d29a33e67 (tags/b8.c19) packet: fetch> have 334d40a157dec5d93023976c30cd22b24bdc279a (tags/b7.c19) [...] packet: fetch> have e3496f08debed7528bd7e4c4a12b71d1a99d697f (tags/b1.c19) packet: fetch> have e7bb01cb25bebd0341c9d62f4c7e929a99b6ed4b (tags/b8.c17) packet: fetch> have 7f5656e94770d527d4f909fd5e2ea274ec63177a (tags/b7.c17) [...] packet: fetch> have 17639a004fe8511fe1de57dd9ddabf2ee0de902d (tags/b1.c17) packet: fetch> 0000 packet: fetch< acknowledgments packet: fetch< ACK e3496f08debed7528bd7e4c4a12b71d1a99d697f (tags/b1.c19) packet: fetch< ACK 17639a004fe8511fe1de57dd9ddabf2ee0de902d (tags/b1.c17) By comparison, with protocol v0 over stateful bidirectional transports, there's an additional round-trip folded in: packet: fetch> have f65fc9b4d5c1cb76494a7f8df0230d8d29a33e67 (tags/b8.c19) packet: fetch> have 334d40a157dec5d93023976c30cd22b24bdc279a (tags/b7.c19) [...] packet: fetch> have e3496f08debed7528bd7e4c4a12b71d1a99d697f (tags/b1.c19) packet: fetch> have e7bb01cb25bebd0341c9d62f4c7e929a99b6ed4b (tags/b8.c17) packet: fetch> have 7f5656e94770d527d4f909fd5e2ea274ec63177a (tags/b7.c17) [...] packet: fetch> have 17639a004fe8511fe1de57dd9ddabf2ee0de902d (tags/b1.c17) packet: fetch> 0000 packet: fetch> have a1d75daa2f482f89171f092778da506803e54531 (tags/b8.c14) packet: fetch> have b2e9b68d2650b77283421888be8a950c18bab29d (tags/b7.c14) [...] packet: fetch> have b89f6499d7cee40ef422edb15433a10f82de0206 (tags/b1.c14) packet: fetch> have e4190b433240834c895347214d29426a094f2fe2 (tags/b8.c9) packet: fetch> have 5f1aa7f016defcf74e5e1d4991342987c9d4b447 (tags/b7.c9) [...] packet: fetch> have b76868e654ce45adb9e06f638e48a72556843361 (tags/b1.c9) packet: fetch> 0000 packet: fetch< ACK e3496f08debed7528bd7e4c4a12b71d1a99d697f (tags/b1.c19) common packet: fetch< ACK 17639a004fe8511fe1de57dd9ddabf2ee0de902d (tags/b1.c17) common Patch coming in a moment to force v0 here with a comment. Thanks, Jonathan
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl index 38bfeebd88..fd3303552b 100755 --- a/t/check-non-portable-shell.pl +++ b/t/check-non-portable-shell.pl @@ -46,7 +46,7 @@ sub err { /(?:\$\(seq|^\s*seq\b)/ and err 'seq is not portable (use test_seq)'; /\bgrep\b.*--file\b/ and err 'grep --file FILE is not portable (use grep -f FILE)'; /\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (use FOO=bar && export FOO)'; - /^\s*([A-Z0-9_]+=(\w+|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and + /^\s*([A-Z0-9_]+=(\w*|(["']).*?\3)\s+)+(\w+)/ and exists($func{$4}) and err '"FOO=bar shell_func" assignment extends beyond "shell_func"'; $line = ''; # this resets our $. for each file
Just like assigning a nonempty value, assigning an empty value to a shell variable when calling a function produces non-portable behavior: in some shells, the assignment lasts for the duration of the function invocation, and in others, it persists after the function returns. Signed-off-by: Jonathan Nieder <jrnieder@gmail.com> --- If it would be useful for me to send a copy of the "Enable protocol v2 by default" series rebased on top of this, let me know. Thanks again for catching it. Sincerely, Jonathan t/check-non-portable-shell.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)