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

Message

Patrick Steinhardt Jan. 31, 2025, 10:53 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]

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, 407 insertions(+), 141 deletions(-)

Range-diff versus v3:

1:  5001da5515 = 1:  5a68112d63 t5504: modernize test by moving heredocs into test bodies
2:  de1b7eab6a ! 2:  bb3b9cd8b7 t5548: refactor to reuse setup_upstream() function
    @@ Commit message
         setup_upstream_and_workbench() functions.
     
         Signed-off-by: Jiang Xin <[e-mail]>
    +    Signed-off-by: Patrick Steinhardt <[e-mail]>
     
      ## t/t5548-push-porcelain.sh ##
     @@ t/t5548-push-porcelain.sh: format_and_save_expect () {
3:  be1daa20cd ! 3:  7268ab4fdf t5548: refactor test cases by resetting upstream
    @@ Commit message
            hook by moving the operations into the corresponding test case,
     
         Signed-off-by: Jiang Xin <[e-mail]>
    +    Signed-off-by: Patrick Steinhardt <[e-mail]>
     
      ## t/t5548-push-porcelain.sh ##
     @@ t/t5548-push-porcelain.sh: setup_upstream_and_workbench () {
4:  d353cc13d0 < -:  ---------- t5548: add new porcelain test cases
-:  ---------- > 4:  1b721338eb t5548: add new porcelain test cases
5:  2f4723d34f ! 5:  7be03b6fdd t5548: add porcelain push test cases for dry-run mode
    @@ Commit message
          - git push --porcelain --dry-run --atomic --force
     
         Signed-off-by: Jiang Xin <[e-mail]>
    +    Signed-off-by: Patrick Steinhardt <[e-mail]>
     
      ## t/t5548-push-porcelain.sh ##
     @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {
    @@ t/t5548-push-porcelain.sh: run_git_push_porcelain_output_test() {
     +		;;
     +	file)
     +		PROTOCOL="builtin protocol"
    -+		URL_PREFIX="/.*"
    ++		URL_PREFIX=".*"
     +		;;
     +	esac
     +
6:  73728ec873 ! 6:  ea0e885613 send-pack: new return code "ERROR_SEND_PACK_BAD_REF_STATUS"
    @@ Commit message
         We cannot ignore bad REF_STATUS directly in the "send_pack()" function,
         because the function is also used in "builtin/send-pack.c". So we add a
         new non-zero error code "SEND_PACK_ERROR_REF_STATUS" for "send_pack()".
    -    We can choose to ignore the specific error code in the
    -    "git_transport_push()" function to have the same behavior as
    -    "push_refs()" for HTTP protocol.
    +
    +    Ignore the specific error code in the "git_transport_push()" function to
    +    have the same behavior as "push_refs()" for HTTP protocol. Note that
    +    even though we ignore the error here, we'll ultimately still end up
    +    detecting that a subset of refs was not pushed in `transport_push()`
    +    because we eventually call `push_had_errors()` on the remote refs.
     
         Signed-off-by: Jiang Xin <[e-mail]>
    +    Signed-off-by: Patrick Steinhardt <[e-mail]>
     
      ## send-pack.c ##
    -@@ send-pack.c: int send_pack(struct send_pack_args *args,
    +@@ send-pack.c: int send_pack(struct repository *r,
      				reject_atomic_push(remote_refs, args->send_mirror);
      				error("atomic push failed for ref %s. status: %d",
      				      ref->name, ref->status);
    @@ send-pack.c: int send_pack(struct send_pack_args *args,
      				goto out;
      			}
      			/* else fallthrough */
    -@@ send-pack.c: int send_pack(struct send_pack_args *args,
    +@@ send-pack.c: int send_pack(struct repository *r,
      	if (ret < 0)
      		goto out;
      
    @@ send-pack.c: int send_pack(struct send_pack_args *args,
      	for (ref = remote_refs; ref; ref = ref->next) {
      		switch (ref->status) {
      		case REF_STATUS_NONE:
    -@@ send-pack.c: int send_pack(struct send_pack_args *args,
    +@@ send-pack.c: int send_pack(struct repository *r,
      		case REF_STATUS_OK:
      			break;
      		default:
    @@ send-pack.c: int send_pack(struct send_pack_args *args,
      	}
     
      ## send-pack.h ##
    -@@ send-pack.h: struct ref;
    +@@ send-pack.h: struct repository;
      #define SEND_PACK_PUSH_CERT_IF_ASKED 1
      #define SEND_PACK_PUSH_CERT_ALWAYS 2
      
    -+/* Custom exit code from send_pack. */
    ++/* At least one reference has been rejected by the remote side. */
     +#define ERROR_SEND_PACK_BAD_REF_STATUS 1
     +
      struct send_pack_args {
      	const char *url;
      	unsigned verbose:1,
    +@@ send-pack.h: struct option;
    + int option_parse_push_signed(const struct option *opt,
    + 			     const char *arg, int unset);
    + 
    ++/*
    ++ * Compute a packfile and write it to a file descriptor. The `fd` array needs
    ++ * to contain two file descriptors: `fd[0]` is the file descriptor used as
    ++ * input for the packet reader, whereas `fd[1]` is the file descriptor the
    ++ * packfile will be written to.
    ++ *
    ++ * Returns 0 on success, non-zero otherwise. Negative return values indicate a
    ++ * generic error, whereas positive return values indicate specific error
    ++ * conditions as documented with the `ERROR_SEND_PACK_*` constants.
    ++ */
    + int send_pack(struct repository *r, struct send_pack_args *args,
    + 	      int fd[], struct child_process *conn,
    + 	      struct ref *remote_refs, struct oid_array *extra_have);
     
      ## transport.c ##
     @@ transport.c: static int git_transport_push(struct transport *transport, struct ref *remote_re
      	case protocol_v0:
    - 		ret = send_pack(&args, data->fd, data->conn, remote_refs,
    + 		ret = send_pack(the_repository, &args, data->fd, data->conn, remote_refs,
      				&data->extra_have);
     +		/*
     +		 * Ignore the specific error code to maintain consistent behavior
7:  b26774fee4 = 7:  6930d7ec2b t5543: atomic push reports exit code failure
8:  ab4d0906f6 ! 8:  c2c82673fd send-pack: gracefully close the connection for atomic push
    @@ Commit message
     
         Reported-by: Patrick Steinhardt <[e-mail]>
         Signed-off-by: Jiang Xin <[e-mail]>
    +    Signed-off-by: Patrick Steinhardt <[e-mail]>
     
      ## send-pack.c ##
    -@@ send-pack.c: int send_pack(struct send_pack_args *args,
    +@@ send-pack.c: int send_pack(struct repository *r,
      				error("atomic push failed for ref %s. status: %d",
      				      ref->name, ref->status);
      				ret = ERROR_SEND_PACK_BAD_REF_STATUS;

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