From 7dcbeaa0df01661f31a0d93e5d5787a5d3767358 Mon Sep 17 00:00:00 2001 From: Jiang Xin Date: Fri, 17 Apr 2020 05:45:32 -0400 Subject: send-pack: fix inconsistent porcelain output The porcelain output of a failed `git-push` command is inconsistent for different protocols. For example, the following `git-push` command may fail due to the failure of the `pre-receive` hook. git push --porcelain origin HEAD:refs/heads/master For SSH protocol, the porcelain output does not end with a "Done" message: To ! HEAD:refs/heads/master [remote rejected] (pre-receive hook declined) While for HTTP protocol, the porcelain output does end with a "Done" message: To ! HEAD:refs/heads/master [remote rejected] (pre-receive hook declined) Done The following code at the end of function `send_pack()` indicates that `send_pack()` should not return an error if some references are rejected in porcelain mode. int send_pack(...) ... ... if (args->porcelain) return 0; for (ref = remote_refs; ref; ref = ref->next) { switch (ref->status) { case REF_STATUS_NONE: case REF_STATUS_UPTODATE: case REF_STATUS_OK: break; default: return -1; } } return 0; } So if atomic push failed, must check the porcelain mode before return an error. And `receive_status()` should not return an error for a failed updated reference, because `send_pack()` will check them instead. Signed-off-by: Jiang Xin Signed-off-by: Junio C Hamano --- send-pack.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) (limited to 'send-pack.c') diff --git a/send-pack.c b/send-pack.c index 0407841ae8..1835cd5582 100644 --- a/send-pack.c +++ b/send-pack.c @@ -190,10 +190,8 @@ static int receive_status(struct packet_reader *reader, struct ref *refs) if (reader->line[0] == 'o' && reader->line[1] == 'k') hint->status = REF_STATUS_OK; - else { + else hint->status = REF_STATUS_REMOTE_REJECT; - ret = -1; - } hint->remote_status = xstrdup_or_null(msg); /* start our next search from the next ref */ hint = hint->next; @@ -489,7 +487,8 @@ int send_pack(struct send_pack_args *args, if (use_atomic) { strbuf_release(&req_buf); strbuf_release(&cap_buf); - return atomic_push_failure(args, remote_refs, ref); + atomic_push_failure(args, remote_refs, ref); + return args->porcelain ? 0 : -1; } /* else fallthrough */ default: -- cgit v1.3 From 46701bde690f94fb1532bce110eae93d5f6b68a1 Mon Sep 17 00:00:00 2001 From: Jiang Xin Date: Fri, 17 Apr 2020 05:45:34 -0400 Subject: send-pack: mark failure of atomic push properly When pushing with SSH or other smart protocol, references are validated by function `check_to_send_update()` before they are sent in commands to `send_pack()` of "receve-pack". For atomic push, if a reference is rejected after the validation, only references pushed by user should be marked as failure, instead of report failure on all remote references. Commit v2.22.0-1-g3bca1e7f9f (transport-helper: enforce atomic in push_refs_with_push, 2019-07-11) wanted to fix report issue of HTTP protocol, but marked all remote references failure for atomic push. In order to fix the issue of status report for SSH or other built-in smart protocol, revert part of that commit and add additional status for function `atomic_push_failure()`. The additional status for it except the "REF_STATUS_EXPECTING_REPORT" status are: - REF_STATUS_NONE : Not marked as "REF_STATUS_EXPECTING_REPORT" yet. - REF_STATUS_OK : Assume OK for dryrun or status_report is disabled. This fix won't resolve the issue of status report in transport-helper for HTTP or other protocols, and breaks test case in t5541. Will fix it in additional commit. Signed-off-by: Jiang Xin Signed-off-by: Junio C Hamano --- send-pack.c | 2 ++ t/t5541-http-push-smart.sh | 2 +- t/t5543-atomic-push.sh | 6 +++--- t/t5548-push-porcelain.sh | 2 +- transport.c | 14 -------------- 5 files changed, 7 insertions(+), 19 deletions(-) (limited to 'send-pack.c') diff --git a/send-pack.c b/send-pack.c index 1835cd5582..efefb687b2 100644 --- a/send-pack.c +++ b/send-pack.c @@ -332,6 +332,8 @@ static int atomic_push_failure(struct send_pack_args *args, continue; switch (ref->status) { + case REF_STATUS_NONE: + case REF_STATUS_OK: case REF_STATUS_EXPECTING_REPORT: ref->status = REF_STATUS_ATOMIC_PUSH_FAILED; continue; diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh index 23be8ce92d..2c2c3fb0f5 100755 --- a/t/t5541-http-push-smart.sh +++ b/t/t5541-http-push-smart.sh @@ -177,7 +177,7 @@ test_expect_success 'push (chunked)' ' test $HEAD = $(git rev-parse --verify HEAD)) ' -test_expect_success 'push --atomic also prevents branch creation, reports collateral' ' +test_expect_failure 'push --atomic also prevents branch creation, reports collateral' ' # Setup upstream repo - empty for now d=$HTTPD_DOCUMENT_ROOT_PATH/atomic-branches.git && git init --bare "$d" && diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh index 001240eec7..620c30d58f 100755 --- a/t/t5543-atomic-push.sh +++ b/t/t5543-atomic-push.sh @@ -200,7 +200,7 @@ test_expect_success 'atomic push is not advertised if configured' ' # References in upstream : master(1) one(1) foo(1) # References in workbench: master(2) foo(1) two(2) bar(2) # Atomic push : master(2) two(2) bar(2) -test_expect_failure 'atomic push reports (reject by update hook)' ' +test_expect_success 'atomic push reports (reject by update hook)' ' mk_repo_pair && ( cd workbench && @@ -241,7 +241,7 @@ test_expect_failure 'atomic push reports (reject by update hook)' ' # References in upstream : master(1) one(1) foo(1) # References in workbench: master(2) foo(1) two(2) bar(2) -test_expect_failure 'atomic push reports (mirror, but reject by update hook)' ' +test_expect_success 'atomic push reports (mirror, but reject by update hook)' ' ( cd workbench && git remote remove up && @@ -262,7 +262,7 @@ test_expect_failure 'atomic push reports (mirror, but reject by update hook)' ' # References in upstream : master(2) one(1) foo(1) # References in workbench: master(1) foo(1) two(2) bar(2) -test_expect_failure 'atomic push reports (reject by non-ff)' ' +test_expect_success 'atomic push reports (reject by non-ff)' ' rm upstream/.git/hooks/update && ( cd workbench && diff --git a/t/t5548-push-porcelain.sh b/t/t5548-push-porcelain.sh index 96ba449a38..9f4b7de74b 100755 --- a/t/t5548-push-porcelain.sh +++ b/t/t5548-push-porcelain.sh @@ -136,7 +136,7 @@ run_git_push_porcelain_output_test() { # Refs of upstream : master(A) bar(B) baz(A) next(A) # Refs of workbench: master(B) bar(A) baz(A) next(A) # git-push : master(B) bar(A) NULL next(A) - test_expect_success "atomic push failed ($PROTOCOL)" ' + test_expect_failure "atomic push failed ($PROTOCOL)" ' ( cd workbench && git update-ref refs/heads/master $B && diff --git a/transport.c b/transport.c index 13d638d5fe..a07e39564c 100644 --- a/transport.c +++ b/transport.c @@ -1248,20 +1248,6 @@ int transport_push(struct repository *r, err = push_had_errors(remote_refs); ret = push_ret | err; - if ((flags & TRANSPORT_PUSH_ATOMIC) && err) { - struct ref *it; - for (it = remote_refs; it; it = it->next) - switch (it->status) { - case REF_STATUS_NONE: - case REF_STATUS_UPTODATE: - case REF_STATUS_OK: - it->status = REF_STATUS_ATOMIC_PUSH_FAILED; - break; - default: - break; - } - } - if (!quiet || err) transport_print_push_status(transport->url, remote_refs, verbose | porcelain, porcelain, -- cgit v1.3 From dfe1b7f19c04fd10e9bafce91bc1da0c18bbb194 Mon Sep 17 00:00:00 2001 From: Jiang Xin Date: Fri, 17 Apr 2020 05:45:36 -0400 Subject: transport-helper: new method reject_atomic_push() Add new method in transport-helper to reject all references if any reference is failed for atomic push. This method is reused in "send-pack.c" and "transport-helper.c", one for SSH, git and file protocols, and the other for HTTP protocol. Signed-off-by: Jiang Xin Signed-off-by: Junio C Hamano --- send-pack.c | 29 +++-------------------------- transport-helper.c | 38 +++++++++++++++++++++++--------------- transport.h | 3 +++ 3 files changed, 29 insertions(+), 41 deletions(-) (limited to 'send-pack.c') diff --git a/send-pack.c b/send-pack.c index efefb687b2..a7c53193c9 100644 --- a/send-pack.c +++ b/send-pack.c @@ -320,31 +320,6 @@ free_return: return update_seen; } - -static int atomic_push_failure(struct send_pack_args *args, - struct ref *remote_refs, - struct ref *failing_ref) -{ - struct ref *ref; - /* Mark other refs as failed */ - for (ref = remote_refs; ref; ref = ref->next) { - if (!ref->peer_ref && !args->send_mirror) - continue; - - switch (ref->status) { - case REF_STATUS_NONE: - case REF_STATUS_OK: - case REF_STATUS_EXPECTING_REPORT: - ref->status = REF_STATUS_ATOMIC_PUSH_FAILED; - continue; - default: - break; /* do nothing */ - } - } - return error("atomic push failed for ref %s. status: %d\n", - failing_ref->name, failing_ref->status); -} - #define NONCE_LEN_LIMIT 256 static void reject_invalid_nonce(const char *nonce, int len) @@ -489,7 +464,9 @@ int send_pack(struct send_pack_args *args, if (use_atomic) { strbuf_release(&req_buf); strbuf_release(&cap_buf); - atomic_push_failure(args, remote_refs, ref); + reject_atomic_push(remote_refs, args->send_mirror); + error("atomic push failed for ref %s. status: %d\n", + ref->name, ref->status); return args->porcelain ? 0 : -1; } /* else fallthrough */ diff --git a/transport-helper.c b/transport-helper.c index ab3b52eb14..a46afcb69d 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -894,21 +894,7 @@ static int push_refs_with_push(struct transport *transport, case REF_STATUS_REJECT_STALE: case REF_STATUS_REJECT_ALREADY_EXISTS: if (atomic) { - /* Mark other refs as failed */ - for (ref = remote_refs; ref; ref = ref->next) { - if (!ref->peer_ref && !mirror) - continue; - - switch (ref->status) { - case REF_STATUS_NONE: - case REF_STATUS_OK: - case REF_STATUS_EXPECTING_REPORT: - ref->status = REF_STATUS_ATOMIC_PUSH_FAILED; - continue; - default: - break; /* do nothing */ - } - } + reject_atomic_push(remote_refs, mirror); string_list_clear(&cas_options, 0); return 0; } else @@ -1503,3 +1489,25 @@ int bidirectional_transfer_loop(int input, int output) return tloop_spawnwait_tasks(&state); } + +void reject_atomic_push(struct ref *remote_refs, int mirror_mode) +{ + struct ref *ref; + + /* Mark other refs as failed */ + for (ref = remote_refs; ref; ref = ref->next) { + if (!ref->peer_ref && !mirror_mode) + continue; + + switch (ref->status) { + case REF_STATUS_NONE: + case REF_STATUS_OK: + case REF_STATUS_EXPECTING_REPORT: + ref->status = REF_STATUS_ATOMIC_PUSH_FAILED; + continue; + default: + break; /* do nothing */ + } + } + return; +} diff --git a/transport.h b/transport.h index e0131daab9..4298c855be 100644 --- a/transport.h +++ b/transport.h @@ -265,4 +265,7 @@ int transport_refs_pushed(struct ref *ref); void transport_print_push_status(const char *dest, struct ref *refs, int verbose, int porcelain, unsigned int *reject_reasons); +/* common method used by transport-helper.c and send-pack.c */ +void reject_atomic_push(struct ref *refs, int mirror_mode); + #endif -- cgit v1.3