Re: [PATCH v4] transport-helper: report errors properly

From: Jeff King <peff@peff.net>
Date: Apr 8, 2013, 19:28 UTC
Message-ID: <20130408192829.GC7337@sigill.intra.peff.net>
In-Reply-To: <1365432004-20132-1-git-send-email-felipe.contreras@gmail.com>

On Mon, Apr 08, 2013 at 09:40:04AM -0500, Felipe Contreras wrote:

Show 5 quoted lines

> If a push fails because the remote-helper died (with fast-export), the
> user won't see any error message. So let's add one.
> 
> At the same time lets add tests to ensure this error is reported, and
> while we are at it, check the error from fast-import

Thanks, I think this patch is definitely the right direction.

It seems like there is a lot of back-story that had to be clarified during the review/discussion. Is there a reason not to summarize it here so later readers of this commit are enlightened?

I'm thinking something like:

  If a push fails because the remote-helper died (with fast-export), the
  user does not see any error message. We do correctly die with a failed
  exit code, as we notice that the helper has died while reading back
  the ref status from the helper. However, we don't print any message.
  This is OK if the helper itself printed a useful error message, but we
  cannot count on that; let's let the user know that the helper failed.

  In the long run, it may make more sense to propagate the error back up
  to push, so that it can present the usual status table and give a
  nicer message. But this is a much simpler fix that can help
  immediately.

  While we're adding tests, let's also confirm that the remote-helper
  dying is also detect when importing refs. We currently do so robustly
  when the helper uses the "done" feature (and that is what we test). We
  cannot do so reliably when the helper does not use the "done" feature,
  but it is not even worth testing; the right solution is for the helper
  to start using "done".

Show 6 quoted lines

>  	export)
> +		if test -n "$GIT_REMOTE_TESTGIT_FAILURE"
> +		then
> +			sleep 1 # don't let fast-export get SIGPIPE
> +			exit 1
> +		fi

We can do away with this sleep with:

  while read line; do
          test "$line" = "done" && break
  done

The version I posted yesterday had both the read and the sleep, but the sleep was only necessary there to demonstrate the race with check_command.

> +# We sleep to give fast-export a chance to catch the SIGPIPE
> +test_expect_success 'proper failure checks for pushing' '

I think we can drop this comment now, right?

-Peff