* Re: git-upload-archive help was not shown correctly
2012-01-14 15:46 ` Carlos Martín Nieto
@ 2012-01-14 20:56 ` Jeff King
0 siblings, 0 replies; 3+ messages in thread
From: Jeff King @ 2012-01-14 20:56 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: devendra, git
On Sat, Jan 14, 2012 at 04:46:33PM +0100, Carlos Martín Nieto wrote:
> On Sat, Jan 14, 2012 at 07:10:16PM +0530, devendra wrote:
> > Hi git folks,
> >
> > the command git-upload-archive is not properly showing usage info when
> > ran barely with out any args.
>
> git-upload-package is not for human use. It's what gets run on the
> remote end when you run e.g. 'git archive --remote=origin HEAD'
Yeah, but notice that it does detect the error condition; it just wraps
it in a bunch of cruft. More explanation (and a patch) are below, though
I'm lukewarm on the patch.
> > it shows some kind of unwanted garbage instead of showing a nice help
> > message.
>
> It's trying to talk to git. What you see is the "Git Smart
> Protocol". What were you trying to do?
Yeah, this is the more important question. You shouldn't generally ever
be running git-upload-* yourself.
-- >8 --
Subject: upload-archive: detect bad command-line arguments early
The upload-archive command spawns a helper process to
actually generate the archive (passing along all of its
command-line arguments), and the parent process then
multiplexes the stdout and stderr of the helper back to the
client.
This means an incorrect invocation (which in this case means
failing to provide a single <repo> argument) will cause the
helper to complain, and the error will be sent to the client
over the multiplexed channel.
This is not ideal for two reasons:
1. An invocation error in upload-pack is almost certainly
not something the client can do anything about. It
generally implies a bug in either git-daemon or
git-archive. The one exception is something like:
git archive --remote=ssh_host:repo.git \
--exec="git upload-archive bogus" \
2. For local users who are experimenting with git
commands, running "upload-archive" appears to generate
a bunch of barely-readable garbage (which is in fact
the git protocol intermingled with stderr). It's nicer
if we provide them with a readable error message.
Instead, we can detect a bad command-line before we spawn
the helper and give a more human-readable complaint (i.e.,
helping reason (2) above). This doesn't hurt case (1) as
much as you might think, because in the ssh case, the user
should be receiving stderr from the parent process anyway.
So before they saw:
$ git archive --remote=... --exec='git upload-archive bogus'
fatal: sent error to the client: git upload-archive: archiver died with error
remote: usage: git upload-archive <repo>
remote: git upload-archive: archiver died with error
and now they see:
usage: git upload-archive <repo>
fatal: The remote end hung up unexpectedly
Signed-off-by: Jeff King <peff@peff.net>
---
I'm lukewarm on this patch. I actually think the original output is
slightly nicer, so you are hurting case (1) for people in case (2). But
case (2), running random programs to see what happens, is not something
we probably need to be encouraging.
builtin/upload-archive.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index b928beb..0733775 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -10,6 +10,8 @@
static const char upload_archive_usage[] =
"git upload-archive <repo>";
+static const char upload_archive_writer_usage[] =
+ "git upload-archive--writer <repo>";
static const char deadchild[] =
"git upload-archive: archiver died with error";
@@ -25,7 +27,7 @@ int cmd_upload_archive_writer(int argc, const char **argv, const char *prefix)
int len;
if (argc != 2)
- usage(upload_archive_usage);
+ usage(upload_archive_writer_usage);
if (strlen(argv[1]) + 1 > sizeof(buf))
die("insanely long repository name");
@@ -96,6 +98,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
{
struct child_process writer = { argv };
+ if (argc != 2)
+ usage(upload_archive_usage);
+
/*
* Set up sideband subprocess.
*
--
1.7.9.rc0.33.gd3c17
^ permalink raw reply related [flat|nested] 3+ messages in thread