[PATCH v4 0/3] jx/t5411-flake-fix
- From
Jiang Xin <worldhello.net@gmail.com>
- Date
- Nov 11, 2020, 11:31 UTC
- Message-ID
- <20201111113202.24911-1-zhiyou.jx@alibaba-inc.com>
- In-Reply-To
- <CANYiYbH-x6khgTkkFV29+7AjghOZmG69_6-sQcm2489WMHOWAA@mail.gmail.com>
The exchange between receive-pack and proc-receive hook did not carefully check for errors.
## Changes since v3
1. Patch 1/3:
Add new helper function `filter_out_user_friendly_and_stable_output`, and use it for 5411-0000, 5411-0001, 5411-0013 and 5411-0014.
2. Patch 2/3:
Check PACKET_READ_EOF status from reader for broken pipe.
3. Patch 3/3:
Add a new patch, which uses version 0 for proc-receive by default.
## Range-diff of v3...v4
1: 785b28333f ! 1: a3bdb5a5d6 t5411: refactor make_user_friendly_and_stable_output
@@ Metadata
Author: Jiang Xin <worldhello.net@gmail.com>
## Commit message ##
- t5411: refactor make_user_friendly_and_stable_output
+ t5411: new helper filter_out_user_friendly_and_stable_output
- Common function `make_user_friendly_and_stable_output()` can take
- additional arguments to format messages for special cases.
+ New helper `filter_out_user_friendly_and_stable_output` will call
+ common helpr function `make_user_friendly_and_stable_output` and use
+ additional arguments to filter out messages for specific test cases.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## t/t5411/common-functions.sh ##
@@ t/t5411/common-functions.sh: create_commits_in () {
@@ t/t5411/common-functions.sh: create_commits_in () {
-e "s/ / /g" \
-e "s/$A/<COMMIT-A>/g" \
@@ t/t5411/common-functions.sh: 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" | \
-+ if test $# -eq 0
-+ then
-+ cat
-+ else
-+ sed ${1+"$@"}
-+ fi
+ -e "/^error: / d"
}
++
++filter_out_user_friendly_and_stable_output () {
++ make_user_friendly_and_stable_output |
++ sed -n ${1+"$@"}
++}
## t/t5411/test-0000-standard-git-push.sh ##
@@ t/t5411/test-0000-standard-git-push.sh: test_expect_success "git-push --atomic ($PROTOCOL)" '
@@ t/t5411/test-0000-standard-git-push.sh: test_expect_success "git-push --atomic (
- -e "/^To / { s/ */ /g; p; }" \
- -e "/^ ! / { s/ */ /g; p; }" \
- >actual &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "/^To / { p; }" \
+ -e "/^ ! / { p; }" \
+ <out >actual &&
@@ t/t5411/test-0001-standard-git-push--porcelain.sh: test_expect_success "git-push
- -e "/^To / { s/ */ /g; p; }" \
- -e "/^! / { s/ */ /g; p; }" \
- >actual &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "s/^# GETTEXT POISON #//" \
+ -e "/^To / { p; }" \
+ -e "/^! / { p; }" \
2: d83d4519db ! 2: 4ca1c6e2bc receive-pack: gently write messages to proc-receive
@@ Commit message
Reported-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com>
## builtin/receive-pack.c ##
@@ builtin/receive-pack.c: static int read_proc_receive_report(struct packet_reader *reader,
- struct command *cmd;
- struct command *hint = NULL;
- struct ref_push_report *report = NULL;
-- int new_report = 0;
+ int new_report = 0;
int code = 0;
-+ int new_report = 0;
int once = 0;
+ int response = 0;
for (;;) {
struct object_id old_oid, new_oid;
-@@ builtin/receive-pack.c: static int read_proc_receive_report(struct packet_reader *reader,
+ const char *head;
const char *refname;
char *p;
-
+-
- if (packet_reader_read(reader) != PACKET_READ_NORMAL)
-+ if (packet_reader_read(reader) != PACKET_READ_NORMAL) {
-+ if (!response) {
-+ strbuf_addstr(errmsg, "no response from proc-receive hook");
++ enum packet_read_status status;
++
++ status = packet_reader_read(reader);
++ if (status != PACKET_READ_NORMAL) {
++ /* Check whether proc-receive exited abnormally */
++ if (status == PACKET_READ_EOF && !response) {
++ strbuf_addstr(errmsg, "proc-receive exited abnormally");
+ return -1;
+ }
break;
@@ builtin/receive-pack.c: static int read_proc_receive_report(struct packet_reader
head = reader->line;
p = strchr(head, ' ');
-@@ builtin/receive-pack.c: static int run_proc_receive_hook(struct command *commands,
- struct strbuf cap = STRBUF_INIT;
- struct strbuf errmsg = STRBUF_INIT;
- int hook_use_push_options = 0;
-- int version = 0;
-+ int version = -1;
- int code;
-
- argv[0] = find_hook("proc-receive");
@@ builtin/receive-pack.c: static int run_proc_receive_hook(struct command *commands,
if (use_push_options)
strbuf_addstr(&cap, " push-options");
@@ builtin/receive-pack.c: static int run_proc_receive_hook(struct command *command
} else {
- packet_write_fmt(proc.in, "version=1\n");
+ code = packet_write_fmt_gently(proc.in, "version=1\n");
-+ }
+ }
+- packet_flush(proc.in);
+ if (!code)
+ code = packet_flush_gently(proc.in);
+
+- for (;;) {
+- int linelen;
++ if (!code)
++ for (;;) {
++ int linelen;
++ enum packet_read_status status;
+
+- if (packet_reader_read(&reader) != PACKET_READ_NORMAL)
+- break;
++ status = packet_reader_read(&reader);
++ if (status != PACKET_READ_NORMAL) {
++ /* Check whether proc-receive exited abnormally */
++ if (status == PACKET_READ_EOF)
++ code = -1;
++ break;
++ }
+
+- if (reader.pktlen > 8 && starts_with(reader.line, "version=")) {
+- version = atoi(reader.line + 8);
+- linelen = strlen(reader.line);
+- if (linelen < reader.pktlen) {
+- const char *feature_list = reader.line + linelen + 1;
+- if (parse_feature_request(feature_list, "push-options"))
+- hook_use_push_options = 1;
++ if (reader.pktlen > 8 && starts_with(reader.line, "version=")) {
++ version = atoi(reader.line + 8);
++ linelen = strlen(reader.line);
++ if (linelen < reader.pktlen) {
++ const char *feature_list = reader.line + linelen + 1;
++ if (parse_feature_request(feature_list, "push-options"))
++ hook_use_push_options = 1;
++ }
+ }
+ }
++
+ if (code) {
+ strbuf_addstr(&errmsg, "fail to negotiate version with proc-receive hook");
+ goto cleanup;
}
-- packet_flush(proc.in);
- for (;;) {
- int linelen;
-@@ builtin/receive-pack.c: static int run_proc_receive_hook(struct command *commands,
- }
- }
-
-- if (version != 1) {
-+ if (version == -1) {
-+ strbuf_addstr(&errmsg, "fail to negotiate version with proc-receive hook");
-+ code = -1;
-+ goto cleanup;
-+ } else if (version != 1) {
- strbuf_addf(&errmsg, "proc-receive version '%d' is not supported",
- version);
- code = -1;
+ if (version != 1) {
@@ builtin/receive-pack.c: static int run_proc_receive_hook(struct command *commands,
for (cmd = commands; cmd; cmd = cmd->next) {
if (!cmd->run_proc_receive || cmd->skip_update || cmd->error_string)
@@ t/t5411/test-0013-bad-protocol.sh: test_expect_success "proc-receive: bad protoc
test_must_fail git -C workbench push origin \
HEAD:refs/for/main/topic \
>out 2>&1 &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "/^To / { p; }" \
+ -e "/^ ! / { p; }" \
+ <out >actual &&
@@ t/t5411/test-0013-bad-protocol.sh: test_expect_success "proc-receive: bad protoc
+ test-tool proc-receive -v --die-write-version
+ EOF
+'
-+
+
+# Refs of upstream : main(A)
+# Refs of workbench: main(A) tags/v123
+# git push : refs/for/main/topic(A)
@@ t/t5411/test-0013-bad-protocol.sh: test_expect_success "proc-receive: bad protoc
+ test_must_fail git -C workbench push origin \
+ HEAD:refs/for/main/topic \
+ >out 2>&1 &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "/^To / { p; }" \
+ -e "/^ ! / { p; }" \
+ <out >actual &&
@@ t/t5411/test-0013-bad-protocol.sh: test_expect_success "proc-receive: bad protoc
+ test_cmp expect actual &&
+ grep "remote: fatal: die with the --die-write-version option" out &&
+ grep "remote: error: fail to negotiate version with proc-receive hook" out &&
-
++
+ git -C "$upstream" show-ref >out &&
+ make_user_friendly_and_stable_output <out >actual &&
+ cat >expect <<-EOF &&
@@ t/t5411/test-0013-bad-protocol.sh: test_expect_success "proc-receive: bad protoc
+ test_must_fail git -C workbench push origin \
+ HEAD:refs/for/main/topic \
+ >out 2>&1 &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "/^To / { p; }" \
+ -e "/^ ! / { p; }" \
+ <out >actual &&
@@ t/t5411/test-0013-bad-protocol.sh: test_expect_success "proc-receive: bad protoc
+ -o reviewers=user1,user2 \
HEAD:refs/for/main/topic \
>out 2>&1 &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "/^To / { p; }" \
+ -e "/^ ! / { p; }" \
+ <out >actual &&
@@ t/t5411/test-0013-bad-protocol.sh: test_expect_success "proc-receive: bad protoc
+ EOF
+ test_cmp expect actual
+'
-
-- grep "remote: fatal: protocol error: expected \"old new ref\", got \"<ZERO-OID> <COMMIT-A> refs/for/main/topic\"" actual &&
++
+test_expect_success "setup proc-receive hook (hook --die-write-report, $PROTOCOL)" '
+ write_script "$upstream/hooks/proc-receive" <<-EOF
+ printf >&2 "# proc-receive hook\n"
+ test-tool proc-receive -v --die-write-report
+ EOF
+'
-+
+
+- grep "remote: fatal: protocol error: expected \"old new ref\", got \"<ZERO-OID> <COMMIT-A> refs/for/main/topic\"" actual &&
+# Refs of upstream : main(A)
+# Refs of workbench: main(A) tags/v123
+# git push : refs/for/main/topic(A)
@@ t/t5411/test-0013-bad-protocol.sh: test_expect_success "proc-receive: bad protoc
+ test_must_fail git -C workbench push origin \
+ HEAD:refs/for/main/topic \
+ >out 2>&1 &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "/^To / { p; }" \
+ -e "/^ ! / { p; }" \
+ <out >actual &&
@@ t/t5411/test-0013-bad-protocol.sh: test_expect_success "proc-receive: bad protoc
git -C "$upstream" show-ref >out &&
make_user_friendly_and_stable_output <out >actual &&
@@ t/t5411/test-0013-bad-protocol.sh: test_expect_success "proc-receive: bad protocol (no report, $PROTOCOL)" '
- remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic
- remote: # proc-receive hook
- remote: proc-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic
-+ remote: error: no response from proc-receive hook
- remote: # post-receive hook
- remote: post-receive< <ZERO-OID> <COMMIT-A> refs/heads/next
- To <URL/of/upstream.git>
- * [new branch] HEAD -> next
-- ! [remote rejected] HEAD -> refs/for/main/topic (proc-receive failed to report status)
-+ ! [remote rejected] HEAD -> refs/for/main/topic (fail to run proc-receive hook)
+ ! [remote rejected] HEAD -> refs/for/main/topic (proc-receive failed to report status)
EOF
test_cmp expect actual &&
+
@@ t/t5411/test-0014-bad-protocol--porcelain.sh: test_expect_success "proc-receive:
+ test_must_fail git -C workbench push --porcelain origin \
+ HEAD:refs/for/main/topic \
+ >out 2>&1 &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "/^To / { p; n; p; n; p; }" \
+ <out >actual &&
+ cat >expect <<-EOF &&
@@ t/t5411/test-0014-bad-protocol--porcelain.sh: test_expect_success "proc-receive:
+ test_must_fail git -C workbench push --porcelain origin \
+ HEAD:refs/for/main/topic \
+ >out 2>&1 &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "/^To / { p; n; p; n; p; }" \
+ <out >actual &&
+ cat >expect <<-EOF &&
@@ t/t5411/test-0014-bad-protocol--porcelain.sh: test_expect_success "proc-receive:
+ test_must_fail git -C workbench push --porcelain origin \
+ HEAD:refs/for/main/topic \
+ >out 2>&1 &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "/^To / { p; n; p; n; p; }" \
+ <out >actual &&
+ cat >expect <<-EOF &&
@@ t/t5411/test-0014-bad-protocol--porcelain.sh: test_expect_success "proc-receive:
+ -o reviewers=user1,user2 \
+ HEAD:refs/for/main/topic \
+ >out 2>&1 &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "/^To / { p; n; p; n; p; }" \
+ <out >actual &&
+ cat >expect <<-EOF &&
@@ t/t5411/test-0014-bad-protocol--porcelain.sh: test_expect_success "proc-receive:
+ test_must_fail git -C workbench push --porcelain origin \
+ HEAD:refs/for/main/topic \
+ >out 2>&1 &&
-+ make_user_friendly_and_stable_output -n \
++ filter_out_user_friendly_and_stable_output \
+ -e "/^To / { p; n; p; n; p; }" \
+ <out >actual &&
+ cat >expect <<-EOF &&
@@ t/t5411/test-0014-bad-protocol--porcelain.sh: test_expect_success "proc-receive:
write_script "$upstream/hooks/proc-receive" <<-EOF
printf >&2 "# proc-receive hook\n"
@@ t/t5411/test-0014-bad-protocol--porcelain.sh: test_expect_success "proc-receive: bad protocol (no report, $PROTOCOL/porcelain)
- remote: pre-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic
- remote: # proc-receive hook
- remote: proc-receive< <ZERO-OID> <COMMIT-A> refs/for/main/topic
-+ remote: error: no response from proc-receive hook
- remote: # post-receive hook
- remote: post-receive< <ZERO-OID> <COMMIT-A> refs/heads/next
- To <URL/of/upstream.git>
- * HEAD:refs/heads/next [new branch]
-- ! HEAD:refs/for/main/topic [remote rejected] (proc-receive failed to report status)
-+ ! HEAD:refs/for/main/topic [remote rejected] (fail to run proc-receive hook)
Done
EOF
test_cmp expect actual &&
-: ---------- > 3: 82d03e6508 receive-pack: use default version 0 for proc-receive---
Jiang Xin (3): t5411: new helper filter_out_user_friendly_and_stable_output receive-pack: gently write messages to proc-receive receive-pack: use default version 0 for proc-receive
builtin/receive-pack.c | 94 +++++++--- t/helper/test-proc-receive.c | 54 ++++-- t/t5411/common-functions.sh | 7 +- t/t5411/test-0000-standard-git-push.sh | 9 +- .../test-0001-standard-git-push--porcelain.sh | 15 +- t/t5411/test-0013-bad-protocol.sh | 138 ++++++++++++-- t/t5411/test-0014-bad-protocol--porcelain.sh | 173 +++++++++++++++++- t/t5411/test-0026-push-options.sh | 60 ++++++ t/t5411/test-0027-push-options--porcelain.sh | 62 +++++++ 9 files changed, 545 insertions(+), 67 deletions(-)
-- 2.29.0.dirty