Skip to content

Commit

Permalink
send-pack: gracefully close the connection for atomic push
Browse files Browse the repository at this point in the history
Patrick reported an issue that the exit code of git-receive-pack(1) is
ignored during atomic push with "--porcelain" flag, and added new test
cases in t5543.

This issue originated from commit 7dcbeaa (send-pack: fix
inconsistent porcelain output, 2020-04-17). At that time, I chose to
ignore the exit code of "finish_connect()" without investigating the
root cause of the abnormal termination of git-receive-pack. That was an
incorrect solution.

The root cause is that an atomic push operation terminates early without
sending a flush packet to git-receive-pack. As a result,
git-receive-pack continues waiting for commands without exiting. By
sending a flush packet at the appropriate location in "send_pack()", we
ensure that the git-receive-pack process closes properly, avoiding an
erroneous exit code for git-push. At the same time, revert the changes
to the "transport.c" file made in commit 7dcbeaa.

Reported-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Jiang Xin <[email protected]>
  • Loading branch information
jiangxin committed Dec 10, 2024
1 parent 621887f commit f51e4df
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 11 deletions.
1 change: 1 addition & 0 deletions send-pack.c
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,7 @@ int send_pack(struct send_pack_args *args,
error("atomic push failed for ref %s. status: %d",
ref->name, ref->status);
ret = ERROR_SEND_PACK_BAD_REF_STATUS;
packet_flush(out);
goto out;
}
/* else fallthrough */
Expand Down
4 changes: 2 additions & 2 deletions t/t5543-atomic-push.sh
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ test_expect_success 'atomic push reports (reject by non-ff)' '
test_cmp expect actual
'

test_expect_failure 'atomic push reports exit code failure' '
test_expect_success 'atomic push reports exit code failure' '
write_script receive-pack-wrapper <<-\EOF &&
git-receive-pack "$@"
exit 1
Expand All @@ -296,7 +296,7 @@ test_expect_failure 'atomic push reports exit code failure' '
test_cmp expect err
'

test_expect_failure 'atomic push reports exit code failure with porcelain' '
test_expect_success 'atomic push reports exit code failure with porcelain' '
write_script receive-pack-wrapper <<-\EOF &&
git-receive-pack "$@"
exit 1
Expand Down
10 changes: 1 addition & 9 deletions transport.c
Original file line number Diff line number Diff line change
Expand Up @@ -928,15 +928,7 @@ static int git_transport_push(struct transport *transport, struct ref *remote_re

close(data->fd[1]);
close(data->fd[0]);
/*
* Atomic push may abort the connection early and close the pipe,
* which may cause an error for `finish_connect()`. Ignore this error
* for atomic git-push.
*/
if (ret || args.atomic)
finish_connect(data->conn);
else
ret = finish_connect(data->conn);
ret |= finish_connect(data->conn);
data->conn = NULL;
data->finished_handshake = 0;

Expand Down

0 comments on commit f51e4df

Please sign in to comment.