Skip to content

Commit

Permalink
transport-helper: enforce atomic in push_refs_with_push
Browse files Browse the repository at this point in the history
Teach transport-helper how to notice if skipping a ref during push would
violate atomicity on the client side. We notice that a ref would be
rejected, and choose not to send it, but don't notice that if the client
has asked for --atomic we are violating atomicity if all the other
pushes we are sending would succeed. Asking the server end to uphold
atomicity wouldn't work here as the server doesn't have any idea that we
tried to update a ref that's broken.

The added test-case is a succinct way to reproduce this issue that fails
today. The same steps work fine when we aren't using a transport-helper
to get to the upstream, i.e. when we've added a local repository as a
remote:

  git remote add ~/upstream upstream

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
nasamuffin authored and gitster committed Jul 12, 2019
1 parent b697d92 commit 3bca1e7
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 0 deletions.
49 changes: 49 additions & 0 deletions t/t5541-http-push-smart.sh
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,55 @@ test_expect_success 'push (chunked)' '
test $HEAD = $(git rev-parse --verify HEAD))
'

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" &&
test_config -C "$d" http.receivepack true &&
up="$HTTPD_URL"/smart/atomic-branches.git &&
# Tell "$up" about two branches for now
test_commit atomic1 &&
test_commit atomic2 &&
git branch collateral &&
git push "$up" master collateral &&
# collateral is a valid push, but should be failed by atomic push
git checkout collateral &&
test_commit collateral1 &&
# Make master incompatible with upstream to provoke atomic
git checkout master &&
git reset --hard HEAD^ &&
# Add a new branch which should be failed by atomic push. This is a
# regression case.
git branch atomic &&
# --atomic should cause entire push to be rejected
test_must_fail git push --atomic "$up" master atomic collateral 2>output &&
# the new branch should not have been created upstream
test_must_fail git -C "$d" show-ref --verify refs/heads/atomic &&
# upstream should still reflect atomic2, the last thing we pushed
# successfully
git rev-parse atomic2 >expected &&
# on master...
git -C "$d" rev-parse refs/heads/master >actual &&
test_cmp expected actual &&
# ...and collateral.
git -C "$d" rev-parse refs/heads/collateral >actual &&
test_cmp expected actual &&
# the failed refs should be indicated to the user
grep "^ ! .*rejected.* master -> master" output &&
# 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
'

test_expect_success 'push --all can push to empty repo' '
d=$HTTPD_DOCUMENT_ROOT_PATH/empty-all.git &&
git init --bare "$d" &&
Expand Down
6 changes: 6 additions & 0 deletions transport-helper.c
Original file line number Diff line number Diff line change
Expand Up @@ -853,6 +853,7 @@ static int push_refs_with_push(struct transport *transport,
{
int force_all = flags & TRANSPORT_PUSH_FORCE;
int mirror = flags & TRANSPORT_PUSH_MIRROR;
int atomic = flags & TRANSPORT_PUSH_ATOMIC;
struct helper_data *data = transport->data;
struct strbuf buf = STRBUF_INIT;
struct ref *ref;
Expand All @@ -872,6 +873,11 @@ static int push_refs_with_push(struct transport *transport,
case REF_STATUS_REJECT_NONFASTFORWARD:
case REF_STATUS_REJECT_STALE:
case REF_STATUS_REJECT_ALREADY_EXISTS:
if (atomic) {
string_list_clear(&cas_options, 0);
return 0;
} else
continue;
case REF_STATUS_UPTODATE:
continue;
default:
Expand Down
13 changes: 13 additions & 0 deletions transport.c
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,19 @@ int transport_push(struct repository *r,
err = push_had_errors(remote_refs);
ret = push_ret | err;

if ((flags & TRANSPORT_PUSH_ATOMIC) && err) {
for (struct ref *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,
Expand Down

0 comments on commit 3bca1e7

Please sign in to comment.