Jiang Xin <worldhello.net@xxxxxxxxx> writes:
> 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.
> 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?
>
> 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.
> @@ -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")?
> 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?
> - 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+"$@"}
}
