Re: [PATCH v2] t5411: consistent result for proc-receive broken test
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 9, 2020, 20:59 UTC
- Message-ID
- <xmqqh7pyb61f.fsf@gitster.c.googlers.com>
- In-Reply-To
- <20201109105846.64303-1-zhiyou.jx@alibaba-inc.com>
Jiang Xin <worldhello.net@gmail.com> writes:
Show 26 quoted lines
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index bb9909c52e..6bd402897c 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1172,6 +1172,7 @@ static int run_proc_receive_hook(struct command *commands,
> if (version != 1) {
> strbuf_addf(&errmsg, "proc-receive version '%d' is not supported",
> version);
> + close(proc.in);
> code = -1;
> goto cleanup;
> }
> @@ -1196,11 +1197,12 @@ static int run_proc_receive_hook(struct command *commands,
> packet_flush(proc.in);
> }
>
> + close(proc.in);
> +
> /* Read result from proc-receive */
> code = read_proc_receive_report(&reader, commands, &errmsg);
>
> cleanup:
> - close(proc.in);
> close(proc.out);
> if (use_sideband)
> finish_async(&muxer);OK, without us closing our end, the hook cannot even tell that it read to the end of our input.
Show 21 quoted lines
> diff --git a/t/helper/test-proc-receive.c b/t/helper/test-proc-receive.c
> index 42164d9898..9f7fbc5b7a 100644
> --- a/t/helper/test-proc-receive.c
> +++ b/t/helper/test-proc-receive.c
> @@ -12,6 +12,7 @@ static const char *proc_receive_usage[] = {
>
> static int die_version;
> static int die_readline;
> +static int die_report;
> static int no_push_options;
> static int use_atomic;
> static int use_push_options;
> @@ -52,8 +53,10 @@ static void proc_receive_verison(struct packet_reader *reader) {
> }
> }
>
> - if (server_version != 1 || die_version)
> + if (server_version != 1)
> die("bad protocol version: %d", server_version);
> + if (die_version)
> + die("die with the --die-version option");If any of these trigger, wouldn't we end up dying without consuming what receive-pack said?
Show 18 quoted lines
>
> packet_write_fmt(1, "version=%d%c%s\n",
> version, '\0',
> @@ -79,9 +82,15 @@ static void proc_receive_read_commands(struct packet_reader *reader,
> *p++ != ' ' ||
> parse_oid_hex(p, &new_oid, &p) ||
> *p++ != ' ' ||
> - die_readline)
> + die_readline) {
> + char *bad_line = xstrdup(reader->line);
> + while (packet_reader_read(reader) != PACKET_READ_EOF)
> + ; /* do nothing */
> + if (die_readline)
> + die("die with the --die-readline option");
> die("protocol error: expected 'old new ref', got '%s'",
> - reader->line);
> + bad_line);
> + }This part is different from the previous one in that it slurps all the input before dying evein in die_readline case.
Show 6 quoted lines
> @@ -166,6 +177,8 @@ int cmd__proc_receive(int argc, const char **argv)
> fprintf(stderr, "proc-receive> %s\n", item->string);
> }
>
> + if (die_report)
> + die("die with the --die-report option");And at this point we have already read everything the other end said (if so, there is no need for the artificial "read everything before we die")?
Show 8 quoted lines
> diff --git a/t/t5411/test-0013-bad-protocol.sh b/t/t5411/test-0013-bad-protocol.sh > index c5fe4cb37b..5c5241bc95 100644 > --- a/t/t5411/test-0013-bad-protocol.sh > +++ b/t/t5411/test-0013-bad-protocol.sh > @@ -55,19 +55,16 @@ test_expect_success "proc-receive: bad protocol (hook --die-version, $PROTOCOL)" > test_must_fail git -C workbench push origin \ > HEAD:refs/for/master/topic \ > >out 2>&1 &&
Are these expected to conflict with Dscho's changes to move 'master' around?
Show 7 quoted lines
> - make_user_friendly_and_stable_output <out >actual &&
> -
> + make_user_friendly_and_stable_output <out |
> + sed -n \
> + -e "/^To / { s/ */ /g; p; }" \
> + -e "/^ ! / { s/ */ /g; p; }" \
> + >actual &&It's the same thing but I somehow find "s/ */ /g" easier to read. The comparison is between "there may be two things or more---squish them down to one" and "After one thing, there may be any number of things---remove all the extra ones".
Makes me wonder if make_user_friendly should optionally have an option to do something like this for us. I doubt it that it is worth to do something like the attached patch.
t/t5411/common-functions.sh | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git c/t/t5411/common-functions.sh w/t/t5411/common-functions.sh index 6580bebd8e..6919639c60 100644 --- c/t/t5411/common-functions.sh +++ w/t/t5411/common-functions.sh @@ -40,7 +40,9 @@ create_commits_in () { # `GIT_TEST_GETTEXT_POISON=true` in order to test unintentional translations # on plumbing commands. make_user_friendly_and_stable_output () { - sed \ + local en= + case "$#" in 0) ;; *) en=-n ;; esac + sed $en \ -e "s/ *\$//" \ -e "s/ */ /g" \ -e "s/'/\"/g" \ @@ -52,5 +54,6 @@ make_user_friendly_and_stable_output () { -e "s/$(echo $A | cut -c1-7)[0-9a-f]*/<OID-A>/g" \ -e "s/$(echo $B | cut -c1-7)[0-9a-f]*/<OID-B>/g" \ -e "s#To $URL_PREFIX/upstream.git#To <URL/of/upstream.git>#" \ - -e "/^error: / d" + -e "/^error: / d" \ + ${1+"$@"} }