* Re: [PATCH] remote-curl: do not complain on EOF from parent git
2014-07-09 21:20 ` [PATCH] remote-curl: do not complain on EOF from parent git Jeff King
@ 2014-07-09 21:25 ` Keller, Jacob E
2014-07-09 21:42 ` Jeff King
2014-07-09 21:45 ` [PATCH 1/3] " Jeff King
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Keller, Jacob E @ 2014-07-09 21:25 UTC (permalink / raw)
To: peff; +Cc: git, wipedout
On Wed, 2014-07-09 at 17:20 -0400, Jeff King wrote:
> The parent git process is supposed to send us an empty line
> to indicate that the conversation is over. However, the
> parent process may die() if there is a problem with the
> operaiton (e.g., we try to fetch a ref that does not exist).
Nitpick, but you probably meant operation here.
Thanks,
Jake
> In this case, it produces a useful message, but then
> remote-curl _also_ produces an unhelpful message:
>
> $ git pull origin matser
> fatal: couldn't find remote ref matser
> Unexpected end of command stream
>
> The "right" way to fix this is to teach the parent git to
> always cleanly close the connection to the helper, letting
> it know that we are done. Implementing that is rather
> clunky, though, as it would involve either replacing die()
> operations with returning errors up the stack (until we
> disconnect the transport), or adding an atexit handler to
> clean up any transport helpers left open.
>
> It's much simpler to just suppress the EOF message in
> remote-curl. It was not added to address any real-world
> situation in the first place, but rather a "we should
> probably report unexpected things" suggestion[1].
>
> It is the parent git which drives the operation, and whose
> exit value actually matters. If the parent dies, then the
> helper has no need to complain (except as a debugging aid).
> In the off chance that the pipe is closed without the parent
> dying, the parent can still notice the non-zero exit code.
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/176036
>
> Reported-by: Dmitry <wipedout@yandex.ru>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The original discussion that led to this code being implemented was due
> to us checking the helper's exit code in the first place. However, we
> seem to be inconsistent about doing so. I'm not inclined to pursue it
> further, though, as these subtle details of the transport helper code
> usually turn into a can of worms, and more importantly, I don't think it
> hurts anything in the real world. Either the parent git gets the
> expected protocol output from the helper or it doesn't, and we report
> errors on that. An error from a helper after the operation completes is
> not really important to the parent git either way.
>
> remote-curl.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 4493b38..0454ffc 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -971,8 +971,6 @@ int main(int argc, const char **argv)
> if (strbuf_getline(&buf, stdin, '\n') == EOF) {
> if (ferror(stdin))
> fprintf(stderr, "Error reading command stream\n");
> - else
> - fprintf(stderr, "Unexpected end of command stream\n");
> return 1;
> }
> if (buf.len == 0)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] remote-curl: do not complain on EOF from parent git
2014-07-09 21:25 ` Keller, Jacob E
@ 2014-07-09 21:42 ` Jeff King
0 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2014-07-09 21:42 UTC (permalink / raw)
To: Keller, Jacob E; +Cc: git, wipedout
On Wed, Jul 09, 2014 at 09:25:18PM +0000, Keller, Jacob E wrote:
> On Wed, 2014-07-09 at 17:20 -0400, Jeff King wrote:
> > The parent git process is supposed to send us an empty line
> > to indicate that the conversation is over. However, the
> > parent process may die() if there is a problem with the
> > operaiton (e.g., we try to fetch a ref that does not exist).
>
> Nitpick, but you probably meant operation here.
I did (and I probably meant to turn on spellcheck, too...).
I'll repost in a minute, as I have some follow-on patches. Thanks.
-Peff
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] remote-curl: do not complain on EOF from parent git
2014-07-09 21:20 ` [PATCH] remote-curl: do not complain on EOF from parent git Jeff King
2014-07-09 21:25 ` Keller, Jacob E
@ 2014-07-09 21:45 ` Jeff King
2014-07-09 21:47 ` [PATCH 2/3] remote-curl: use error instead of fprintf(stderr) Jeff King
2014-07-09 21:47 ` [PATCH 3/3] remote-curl: mark helper-protocol errors more clearly Jeff King
3 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2014-07-09 21:45 UTC (permalink / raw)
To: Dmitry; +Cc: git, Junio C Hamano
The parent git process is supposed to send us an empty line
to indicate that the conversation is over. However, the
parent process may die() if there is a problem with the
operation (e.g., we try to fetch a ref that does not exist).
In this case, it produces a useful message, but then
remote-curl _also_ produces an unhelpful message:
$ git pull origin matser
fatal: couldn't find remote ref matser
Unexpected end of command stream
The "right" way to fix this is to teach the parent git to
always cleanly close the connection to the helper, letting
it know that we are done. Implementing that is rather
clunky, though, as it would involve either replacing die()
operations with returning errors up the stack (until we
disconnect the transport), or adding an atexit handler to
clean up any transport helpers left open.
It's much simpler to just suppress the EOF message in
remote-curl. It was not added to address any real-world
situation in the first place, but rather a "we should
probably report unexpected things" suggestion[1].
It is the parent git which drives the operation, and whose
exit value actually matters. If the parent dies, then the
helper has no need to complain (except as a debugging aid).
In the off chance that the pipe is closed without the parent
dying, it can still notice the non-zero exit code.
[1] http://article.gmane.org/gmane.comp.version-control.git/176036
Signed-off-by: Jeff King <peff@peff.net>
---
Repost fixing commit message typo (and indicating it as the first of 3
patches).
remote-curl.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 4493b38..0454ffc 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -971,8 +971,6 @@ int main(int argc, const char **argv)
if (strbuf_getline(&buf, stdin, '\n') == EOF) {
if (ferror(stdin))
fprintf(stderr, "Error reading command stream\n");
- else
- fprintf(stderr, "Unexpected end of command stream\n");
return 1;
}
if (buf.len == 0)
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] remote-curl: use error instead of fprintf(stderr)
2014-07-09 21:20 ` [PATCH] remote-curl: do not complain on EOF from parent git Jeff King
2014-07-09 21:25 ` Keller, Jacob E
2014-07-09 21:45 ` [PATCH 1/3] " Jeff King
@ 2014-07-09 21:47 ` Jeff King
2014-07-09 21:47 ` [PATCH 3/3] remote-curl: mark helper-protocol errors more clearly Jeff King
3 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2014-07-09 21:47 UTC (permalink / raw)
To: Dmitry; +Cc: git
We usually prefix our error messages with "error: ", but
many error messages from remote-curl are simply printed with
fprintf. This can make the output a little harder to read
(especially because such message may be intermingled with
errors from the parent git process).
There is no reason to avoid error(), as we are already
calling it many places (in addition to libgit.a functions
which use it).
While we're adjusting the messages, we can also drop the
capitalization which makes them unlike other git error
messages.
Signed-off-by: Jeff King <peff@peff.net>
---
I suspect there may be some more improvements we can make (e.g., the
"fetch failed" below seems like it might be redundant with what "git
fetch" will print). But hunting them is a lot of work for little gain;
I'd rather wait to see them in practice and fix them on a case by case
basis.
remote-curl.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 0454ffc..a619b64 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -399,7 +399,7 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
rpc->pos = 0;
return CURLIOE_OK;
}
- fprintf(stderr, "Unable to rewind rpc post data - try increasing http.postBuffer\n");
+ error("unable to rewind rpc post data - try increasing http.postBuffer");
return CURLIOE_FAILRESTART;
default:
@@ -709,7 +709,7 @@ static int fetch_dumb(int nr_heads, struct ref **to_fetch)
free(targets[i]);
free(targets);
- return ret ? error("Fetch failed.") : 0;
+ return ret ? error("fetch failed.") : 0;
}
static int fetch_git(struct discovery *heads,
@@ -949,7 +949,7 @@ int main(int argc, const char **argv)
git_extract_argv0_path(argv[0]);
setup_git_directory_gently(&nongit);
if (argc < 2) {
- fprintf(stderr, "Remote needed\n");
+ error("remote needed");
return 1;
}
@@ -970,7 +970,7 @@ int main(int argc, const char **argv)
do {
if (strbuf_getline(&buf, stdin, '\n') == EOF) {
if (ferror(stdin))
- fprintf(stderr, "Error reading command stream\n");
+ error("error reading command stream");
return 1;
}
if (buf.len == 0)
@@ -1014,7 +1014,7 @@ int main(int argc, const char **argv)
printf("\n");
fflush(stdout);
} else {
- fprintf(stderr, "Unknown command '%s'\n", buf.buf);
+ error("unknown command '%s'", buf.buf);
return 1;
}
strbuf_reset(&buf);
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] remote-curl: mark helper-protocol errors more clearly
2014-07-09 21:20 ` [PATCH] remote-curl: do not complain on EOF from parent git Jeff King
` (2 preceding siblings ...)
2014-07-09 21:47 ` [PATCH 2/3] remote-curl: use error instead of fprintf(stderr) Jeff King
@ 2014-07-09 21:47 ` Jeff King
3 siblings, 0 replies; 8+ messages in thread
From: Jeff King @ 2014-07-09 21:47 UTC (permalink / raw)
To: Dmitry; +Cc: git, Junio C Hamano
When we encounter an error in remote-curl, we generally just
report it to stderr. There is no need for the user to care
that the "could not connect to server" error was generated
by git-remote-https rather than a function in the parent
git-fetch process.
However, when the error is in the protocol between git and
the helper, it makes sense to clearly identify which side is
complaining. These cases shouldn't ever happen, but when
they do, we can make them less confusing by being more
verbose.
Signed-off-by: Jeff King <peff@peff.net>
---
remote-curl.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index a619b64..1f6905d 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -949,7 +949,7 @@ int main(int argc, const char **argv)
git_extract_argv0_path(argv[0]);
setup_git_directory_gently(&nongit);
if (argc < 2) {
- error("remote needed");
+ error("remote-curl: usage: git remote-curl <remote> [<url>]");
return 1;
}
@@ -970,14 +970,14 @@ int main(int argc, const char **argv)
do {
if (strbuf_getline(&buf, stdin, '\n') == EOF) {
if (ferror(stdin))
- error("error reading command stream");
+ error("remote-curl: error reading command stream from git");
return 1;
}
if (buf.len == 0)
break;
if (starts_with(buf.buf, "fetch ")) {
if (nongit)
- die("Fetch attempted without a local repo");
+ die("remote-curl: fetch attempted without a local repo");
parse_fetch(&buf);
} else if (!strcmp(buf.buf, "list") || starts_with(buf.buf, "list ")) {
@@ -1014,7 +1014,7 @@ int main(int argc, const char **argv)
printf("\n");
fflush(stdout);
} else {
- error("unknown command '%s'", buf.buf);
+ error("remote-curl: unknown command '%s' from git", buf.buf);
return 1;
}
strbuf_reset(&buf);
--
2.0.0.566.gfe3e6b2
^ permalink raw reply related [flat|nested] 8+ messages in thread