[v5,0/8] transport: don't ignore git-receive-pack(1) exit code on atomic push

Message

Patrick Steinhardt Feb. 3, 2025, 6:29 a.m. UTC

Hi,

we've hit an edge case at GitLab where an atomic push will not notice an
error when git-receive-pack(1) updates the refs, but otherwise fails
with a non-zero exit code. The push would be successful and no error
would be printed even though some things have gone wrong on the remote
side.

As promised last week, I've now adopted the patch series from Jiang Xin
to make some progress on the issue. The series is now based on the
latest master branch at 3b0d05c4a7 (The fifth batch, 2025-01-29).

Changes in v4:
  - Rewrite the commit that adds two new tests for `--porcelain`.
    Previously, the commit both reordered existing tests and added new
    ones, which made it hard to see what actually changed. The reorder
    wasn't necessary though, so I've adapted it to only add new tests
    now.
  - Fix the URL prefix in t5548 to also work on Windows.
  - Document why it's fine to start ignoring the return value of
    `git_transport_push()`.
  - Improve comments to document behaviour better.
  - Link to v3: https://lore.kernel.org/r/[e-mail]

Changes in v5:
  - Escape heredocs where possible.
  - Link to v4: https://lore.kernel.org/r/[e-mail]

Thanks!

Patrick

---
Jiang Xin (5):
      t5548: refactor to reuse setup_upstream() function
      t5548: refactor test cases by resetting upstream
      t5548: add porcelain push test cases for dry-run mode
      send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"
      send-pack: gracefully close the connection for atomic push

Patrick Steinhardt (3):
      t5504: modernize test by moving heredocs into test bodies
      t5548: add new porcelain test cases
      t5543: atomic push reports exit code failure

 send-pack.c                     |  10 +-
 send-pack.h                     |  13 ++
 t/t5504-fetch-receive-strict.sh |  35 ++--
 t/t5543-atomic-push.sh          |  30 +++
 t/t5548-push-porcelain.sh       | 443 ++++++++++++++++++++++++++++++----------
 transport.c                     |  17 +-
 6 files changed, 406 insertions(+), 142 deletions(-)

Range-diff versus v4:

1:  ef3732a280 ! 1:  2d04ec6ca3 t5504: modernize test by moving heredocs into test bodies
    @@ t/t5504-fetch-receive-strict.sh: test_expect_success 'push without strict' '
      		git config fetch.fsckobjects false &&
      		git config transfer.fsckobjects false
      	) &&
    -+	cat >exp <<-EOF &&
    ++	cat >exp <<-\EOF &&
     +	To dst
     +	!	refs/heads/main:refs/heads/test	[remote rejected] (missing necessary objects)
     +	Done
    @@ t/t5504-fetch-receive-strict.sh: test_expect_success 'push with receive.fsckobje
      		git config receive.fsckobjects true &&
      		git config transfer.fsckobjects false
      	) &&
    -+	cat >exp <<-EOF &&
    ++	cat >exp <<-\EOF &&
     +	To dst
     +	!	refs/heads/main:refs/heads/test	[remote rejected] (unpacker error)
     +	EOF
2:  c4a57bf457 ! 2:  31c8b10e7d t5548: refactor to reuse setup_upstream() function
    @@ t/t5548-push-porcelain.sh: format_and_save_expect () {
     +	if test $# -ne 1
     +	then
     +		BUG "location of upstream repository is not provided"
    -+	fi &&
    -+	# Assign the first argument to the variable upstream;
    -+	# we will use it in the subsequent test cases.
    ++	fi
     +	upstream="$1"
     +
      	# Upstream  after setup : main(B)  foo(A)  bar(A)  baz(A)
3:  67928693cc ! 3:  a8de197677 t5548: refactor test cases by resetting upstream
    @@ Commit message
     
      ## t/t5548-push-porcelain.sh ##
     @@ t/t5548-push-porcelain.sh: setup_upstream_and_workbench () {
    - 	# we will use it in the subsequent test cases.
    + 	fi
      	upstream="$1"
      
     -	# Upstream  after setup : main(B)  foo(A)  bar(A)  baz(A)
4:  6e0f0f791f ! 4:  9e4f3be9e0 t5548: add new porcelain test cases
    @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {
     +			baz \
     +			next >out &&
     +		make_user_friendly_and_stable_output <out >actual &&
    -+		format_and_save_expect <<-EOF &&
    ++		format_and_save_expect <<-\EOF &&
     +		> To <URL/of/upstream.git>
     +		> =	refs/heads/baz:refs/heads/baz	[up to date]
     +		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
    @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {
     +			baz \
     +			next >out &&
     +		make_user_friendly_and_stable_output <out >actual &&
    -+		format_and_save_expect <<-EOF &&
    ++		format_and_save_expect <<-\EOF &&
     +		> To <URL/of/upstream.git>
     +		> =	refs/heads/baz:refs/heads/baz	[up to date]
     +		>  	<COMMIT-B>:refs/heads/bar	<COMMIT-A>..<COMMIT-B>
5:  cb985baec9 = 5:  093b50d785 t5548: add porcelain push test cases for dry-run mode
6:  68ae698e4b = 6:  9a1851f06e send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"
7:  525cefd4f2 = 7:  e789913922 t5543: atomic push reports exit code failure
8:  19cdbf991f = 8:  52101a1f14 send-pack: gracefully close the connection for atomic push

---
base-commit: 3b0d05c4a79d0e441283680a864529b02dca5f08
change-id: 20241113-pks-push-atomic-respect-exit-code-436c443a657d

Comments

Junio C Hamano Feb. 3, 2025, 11:26 p.m. UTC | #1

Patrick Steinhardt <[e-mail]> writes:

> Changes in v5:
>   - Escape heredocs where possible.
>   - Link to v4: https://lore.kernel.org/r/[e-mail]
>
> Thanks!

Changes since the last round looked trivially correct ;-)

Will replace.