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

git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed

From: Jonathan Nieder <jrnieder@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jonathan Tan <jonathantanmy@google.com>,
	Jeff King <peff@peff.net>,
	Jeff Hostetler <jeffhost@microsoft.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH 2/2] t/check-non-portable-shell: detect "FOO= shell_func", too
Date: Thu, 26 Dec 2019 14:37:27 -0800	[thread overview]
Message-ID: <20191226223727.GB186931@google.com> (raw)
In-Reply-To: <xmqq7e2ilu1j.fsf@gitster-ct.c.googlers.com>

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

next prev parent reply	other threads:[~2019-12-26 22:37 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-24  0:58 [PATCH 0/5] Enable protocol v2 by default Jonathan Nieder
2019-12-24  0:59 ` [PATCH 1/5] fetch test: use more robust test for filtered objects Jonathan Nieder
2019-12-26 14:29   ` Derrick Stolee
2019-12-24  1:00 ` [PATCH 2/5] config doc: protocol.version is not experimental Jonathan Nieder
2019-12-24  1:01 ` [PATCH 3/5] test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate Jonathan Nieder
2019-12-26 19:26   ` Junio C Hamano
2019-12-26 19:53     ` [PATCH 0/2] avoid use of "VAR= cmd" with a shell function (Re: [PATCH 3/5] test: request GIT_TEST_PROTOCOL_VERSION=0 when appropriate) Jonathan Nieder
2019-12-26 19:55       ` [PATCH 1/2] fetch test: avoid use of "VAR= cmd" with a shell function Jonathan Nieder
2019-12-26 19:57       ` [PATCH 2/2] t/check-non-portable-shell: detect "FOO= shell_func", too Jonathan Nieder
2019-12-26 20:08         ` Junio C Hamano
2019-12-26 20:18         ` Junio C Hamano
2019-12-26 22:37           ` Jonathan Nieder [this message]
2019-12-26 23:12           ` [PATCH jn/test-lint-one-shot-export-to-shell-function] fetch test: mark test of "skipping" haves as v0-only Jonathan Nieder
2019-12-26 20:39         ` [PATCH 2/2] t/check-non-portable-shell: detect "FOO= shell_func", too Eric Sunshine
2019-12-24  1:02 ` [PATCH 4/5] protocol test: let protocol.version override GIT_TEST_PROTOCOL_VERSION Jonathan Nieder
2019-12-24  1:04 ` [PATCH 5/5] fetch: default to protocol version 2 Jonathan Nieder
2019-12-26 14:30 ` [PATCH 0/5] Enable protocol v2 by default Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191226223727.GB186931@google.com \
    --to=jrnieder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jeffhost@microsoft.com \
    --cc=jonathantanmy@google.com \
    --cc=peff@peff.net \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Be sure your reply has a Subject: header at the top and a blank line before the message body.

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).