[BUG] clone: regression in error messages in master

Hi,

So this should explain the problem:

  # using v1.8.3.1
  $ git clone https://google.com
  Cloning into 'google.com'...
  fatal: repository 'https://google.com/' not found

  # using master
  $ git clone https://google.com
  Cloning into 'google.com'...
  fatal: repository 'https://google.com/' not found
  fatal: Reading from helper 'git-remote-https' failed

To figure out where the regression was coming from, I ran a bisect with this script:

  #!/bin/sh
  make clean &&
  make -j 8 &&
  cd t &&
  sh -v -i clone-message.sh

where clone-message.sh is:

  test_description=clone-message

  . ./test-lib.sh

  test_expect_success setup '

  	rm -fr .git &&
  	test_create_repo src &&
  	(
  		cd src &&
  		>file &&
  		git add file &&
  		git commit -m initial &&
  		echo 1 >file &&
  		git add file &&
  		git commit -m updated
  	)

  '

  test_expect_success 'clone invalid URL' '
  	rm -fr dst &&
  	test_must_fail git clone https://google.com 2>msg &&
  	test_i18ngrep "repository .* not found" msg &&
  	! test_i18ngrep "git-remote-https" msg
  '

  test_done

The bisect pointed me to: 81d340d4 (transport-helper: report errors properly, 2013-04-10).

  $ git clone https://google.com
  Cloning into 'google.com'...
  fatal: https://google.com/info/refs?service=git-upload-pack not
found: did you run git update-server-info on the server?
  fatal: Reading from remote helper failed

What?! Okay, the last "Reading from remote helper failed" was introduced by this commit; my clone-message.sh has a bug. So I commented out the first test_i18ngrep and ran it. Result: c096955 (transport-helper: mention helper name when it dies, 2013-04-10). This is not the real culprit: it just changed the message string that 81d340d4 originally introduced.

Okay, so am I reporting a valid bug? Going through remote-curl, I can see that it dies in remote-curl.c:213 if HTTP_TARGET_MISSING. If that is the case, what is the point of printing the second message about the remote helper program not being present?

Thanks.

On Thu, Jun 20, 2013 at 06:46:55PM +0530, Ramkumar Ramachandra wrote:

Yeah, that is a not-so-great fallout from 81d340d4. The point of that commit was that we do not know whether the remote helper has printed anything useful; it died unexpectedly while we tried to read from it.

In this case, of course it has, and so the extra message is redundant and unwanted.

I'm not sure if there is a good way to distinguish the two cases (snooping on stderr would add complexity, and is not even robust, as we do not know the meaning of human-readable messages coming over stderr). Waiting for an "expected" time for the helper give us EOF does not work either; I think in this case we asked for a "list" or "fetch", and the helper died without giving us an answer (because there is no answer to give; there is no "oops, I could not complete your request" on the fetch side of the transport helper protocol).

So I'm not sure if there is a better option than reverting 81d340d4 and living with the lesser of two evils (no good message when the helper dies silently).

-Peff

Jeff King wrote:

I dug around, but I still can't justify that there is no better option. Could you write a commit message for this?

-- 8< --

Ramkumar Ramachandra wrote:

Oh, and we have to remove test 23 - "proper failure checks for pushing" from t5801-remote-helpers.sh.

On Fri, Jun 21, 2013 at 12:14:33PM +0530, Ramkumar Ramachandra wrote:

I think it is something like this:

-- >8 --
Subject: [PATCH] transport-helper: be quiet on read errors from helpers

Prior to commit 81d340d4, we did not print any error message if a remote transport helper died unexpectedly. If a helper did not print any error message (e.g., because it crashed), the user could be left confused. That commit tried to rectify the situation by printing a note that the helper exited unexpectedly.

However, this makes a much more common case worse: when a helper does die with a useful message, we print the extra "Reading from 'git-remote-foo failed" message. This can also end up confusing users, as they may not even know what remote helpers are (e.g., the fact that http support comes through git-remote-https is purely an implementation detail that most users do not know or care about).

Since we do not have a good way of knowing whether the helper printed a useful error, and since the common failure mode is for it to do so, let's default to remaining quiet. Debuggers can dig further by setting GIT_TRANSPORT_HELPER_DEBUG.

Signed-off-by: Jeff King <peff@peff.net>
---
Note that I haven't thought too hard about this; there may be a way to
detect for specific operations that we were expecting more data from the
helper and didn't get it. But even if we do want to go that route, I
think reverting the change to recvline_fh is probably going to be the
first step.

 t/t5801-remote-helpers.sh | 4 +---
 transport-helper.c        | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

Jeff King <peff@peff.net> writes:

Yeah, I agree with that assessment.  Will queue.

On Thu, Jun 20, 2013 at 9:16 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

I can see where this is confusing, but can also see how it's useful information to have. On clone, it's probably not that useful since you're looking right at the url, but I could see that information being more useful on a pull or push with the default arguments (when the source and destination aren't quite as obvious).

-John

John Szakmeister <john@szakmeister.net> writes:

The "extra" error messages is not the first one, but the last one, and the suggested revert is a proposal to remove the latter, not the "repository $URL not found".

On Fri, Jun 21, 2013 at 1:11 PM, Junio C Hamano <gitster@pobox.com> wrote:

[snip]

Sorry for the confusion. I realize they were talking about removing the remote helper line. What I meant was that with the url there, it's a bit easier to determine which part of git failed (http, hg, or bzr remote helper, for instance). What I was trying to say was perhaps there are paths through here where it's really helpful to know that things failed in the remote helper, so we may not want to wholesale remove it. Some of remote helpers, such as ones that talk to foreign VCSes, do quite a bit more than just transfer data, so it might be helpful to know that it's not core git that's failing but the remote helper. Seeing the URL is a reminder that you're interacting with a remote helper, and it may not be helpful there. But other commands don't necessarily show that to you, and it may be more helpful to have that reminder that it was a remote helper that failed.

I hope that makes more sense.

-John