Re: [PATCH 2/2] t/check-non-portable-shell: detect "FOO= shell_func", too

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Dec 26, 2019, 22:37 UTC
Message-ID: <20191226223727.GB186931@google.com>
In-Reply-To: <xmqq7e2ilu1j.fsf@gitster-ct.c.googlers.com>

Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:

Show 15 quoted lines

>> 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