| Message ID | 20200325143608.45141-4-zhiyou.jx@alibaba-inc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] t5543: never report what we do not push | expand |
Jiang Xin <worldhello.net@gmail.com> writes: > diff --git a/transport-helper.c b/transport-helper.c > index 20a7185ec4..ab3b52eb14 100644 > --- a/transport-helper.c > +++ b/transport-helper.c > @@ -894,6 +894,21 @@ 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 */ > + } > + } OK, so this is more in line with the check done in send_pack() that fails the push before we even send any pack data. I wonder if it is worth considering to move the logic of this loop into a helper function so that this and the other one in 2/3 can call it and stay in sync, something along the lines of void reject_push_to_other_refs(struct ref *ref, int mirror_mode) { for (; ref; ref = ref->next) { ... one iteration of the above loop ... } } Then the above part would become case REF_STATUS_REJECT_FOO: if (atomic) reject_push_to_other_refs(remote_refs, mirror); and the part modified by 2/3 would also become static int atomic_push_failure(...) { reject_push_to_other_refs(remote_refs, args->send_mirror); return error("atomic push failed ..."); } I am not sure if it is better to keep 2/3 and 3/3 separate or make them into a single step, but perhaps it is because I am not getting the true reason why you made them separate in the first place. Thanks.
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh index 2c2c3fb0f5..afc680d5e3 100755 --- a/t/t5541-http-push-smart.sh +++ b/t/t5541-http-push-smart.sh @@ -177,7 +177,10 @@ test_expect_success 'push (chunked)' ' test $HEAD = $(git rev-parse --verify HEAD)) ' -test_expect_failure 'push --atomic also prevents branch creation, reports collateral' ' +## References of remote: atomic1(1) master(2) collateral(2) other(2) +## References of local : atomic2(2) master(1) collateral(3) other(2) collateral1(3) atomic(1) +## Atomic push : master(1) collateral(3) atomic(1) +test_expect_success '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" && @@ -189,7 +192,8 @@ test_expect_failure 'push --atomic also prevents branch creation, reports collat test_commit atomic2 && git branch collateral && git branch other && - git push "$up" master collateral other && + git push "$up" atomic1 master collateral other && + git tag -d atomic1 && # collateral is a valid push, but should be failed by atomic push git checkout collateral && @@ -224,7 +228,11 @@ test_expect_failure 'push --atomic also prevents branch creation, reports collat # the collateral failure refs should be indicated to the user grep "^ ! .*rejected.* atomic -> atomic .*atomic push failed" output && - grep "^ ! .*rejected.* collateral -> collateral .*atomic push failed" output + grep "^ ! .*rejected.* collateral -> collateral .*atomic push failed" output && + + # never report what we do not push + ! grep "^ ! .*rejected.* atomic1 " output && + ! grep "^ ! .*rejected.* other " output ' test_expect_success 'push --atomic fails on server-side errors' ' diff --git a/transport-helper.c b/transport-helper.c index 20a7185ec4..ab3b52eb14 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -894,6 +894,21 @@ 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 */ + } + } string_list_clear(&cas_options, 0); return 0; } else
Commit v2.22.0-1-g3bca1e7f9f (transport-helper: enforce atomic in push_refs_with_push, 2019-07-11) noticed the incomplete report of failure of an atomic push for HTTP protocol. But the implementation has a flaw that mark all remote references as failure. Only mark necessary references as failure in `push_refs_with_push()` of transport-helper. Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> --- t/t5541-http-push-smart.sh | 14 +++++++++++--- transport-helper.c | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-)