Re: [BUG] clone: regression in error messages in master

2013-06-22 Thread John Szakmeister 
On Fri, Jun 21, 2013 at 1:11 PM, Junio C Hamano gits...@pobox.com wrote:
 John Szakmeister j...@szakmeister.net writes:
[snip]
 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).

 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.

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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] clone: regression in error messages in master

2013-06-21 Thread Ramkumar Ramachandra 
Jeff King wrote:
 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).

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

-- 8 --
diff --git a/transport-helper.c b/transport-helper.c
index 06c08a1..db9bd18 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -56,7 +56,7 @@ static int recvline_fh(FILE *helper, struct strbuf
if (strbuf_getline(buffer, helper, '\n') == EOF) {
if (debug)
fprintf(stderr, Debug: Remote helper quit.\n);
-   die(Reading from helper 'git-remote-%s' failed, name);
+   exit(128);
}

if (debug)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] clone: regression in error messages in master

2013-06-21 Thread Ramkumar Ramachandra 
Ramkumar Ramachandra wrote:
 diff --git a/transport-helper.c b/transport-helper.c
 index 06c08a1..db9bd18 100644
 --- a/transport-helper.c
 +++ b/transport-helper.c

Oh, and we have to remove test 23 - proper failure checks for
pushing from t5801-remote-helpers.sh.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] clone: regression in error messages in master

2013-06-21 Thread Jeff King 
On Fri, Jun 21, 2013 at 12:14:33PM +0530, Ramkumar Ramachandra wrote:

 Jeff King wrote:
  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).
 
 I dug around, but I still can't justify that there is no better
 option.  Could you write a commit message for this?

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 p...@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(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 4899af3..8c4c539 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -210,9 +210,7 @@ test_expect_success 'proper failure checks for pushing' '
(GIT_REMOTE_TESTGIT_FAILURE=1 
export GIT_REMOTE_TESTGIT_FAILURE 
cd local 
-   test_must_fail git push --all 2 error 
-   cat error 
-   grep -q Reading from helper .git-remote-testgit. failed error
+   test_must_fail git push --all
)
 '
 
diff --git a/transport-helper.c b/transport-helper.c
index 06c08a1..db9bd18 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -56,7 +56,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, 
const char *name)
if (strbuf_getline(buffer, helper, '\n') == EOF) {
if (debug)
fprintf(stderr, Debug: Remote helper quit.\n);
-   die(Reading from helper 'git-remote-%s' failed, name);
+   exit(128);
}
 
if (debug)
-- 
1.8.3.rc2.14.g7eee6b3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] clone: regression in error messages in master

2013-06-21 Thread John Szakmeister 
On Thu, Jun 20, 2013 at 9:16 AM, Ramkumar Ramachandra
artag...@gmail.com wrote:
 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

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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] clone: regression in error messages in master

2013-06-21 Thread Junio C Hamano 
Jeff King p...@peff.net writes:

 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.

Yeah, I agree with that assessment.  Will queue.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] clone: regression in error messages in master

2013-06-21 Thread Junio C Hamano 
John Szakmeister j...@szakmeister.net writes:

 On Thu, Jun 20, 2013 at 9:16 AM, Ramkumar Ramachandra
 artag...@gmail.com wrote:
 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

 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).

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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] clone: regression in error messages in master

2013-06-20 Thread Ramkumar Ramachandra 
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 2msg 
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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html