Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork

From: Jeff King <peff@peff.net>
Date: Aug 9, 2011, 05:02 UTC
Message-ID: <20110809050211.GA3588@sigill.intra.peff.net>
In-Reply-To: <4E401869.8060702@lsrfire.ath.cx>

On Mon, Aug 08, 2011 at 07:10:01PM +0200, René Scharfe wrote:

Show 28 quoted lines

> > -	tar_filter_config("tar.tgz.command", "gzip -cn", NULL);
> > +	tar_filter_config("tar.tgz.command", "gzip -cn | cat", NULL);
> >  	tar_filter_config("tar.tgz.remote", "true", NULL);
> > -	tar_filter_config("tar.tar.gz.command", "gzip -cn", NULL);
> > +	tar_filter_config("tar.tar.gz.command", "gzip -cn | cat", NULL);
> >  	tar_filter_config("tar.tar.gz.remote", "true", NULL);
> >  	git_config(git_tar_config, NULL);
> >  	for (i = 0; i < nr_tar_filters; i++) {
> > 
> > (provided that 'cat' magically does not suffer from the same problem,
> > and I do think that it does not.)
> 
> The external cat can indeed be used.  We'd need to do that for user
> supplied commands as well, though, like this (ugh):
> 
> diff --git a/archive-tar.c b/archive-tar.c
> index 20af005..eaa9a1c 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -326,6 +326,9 @@ static int write_tar_filter_archive(const struct archiver *ar,
>  		die("BUG: tar-filter archiver called with no filter defined");
>  
>  	strbuf_addstr(&cmd, ar->data);
> +#ifdef WIN32
> +	strbuf_addstr(&cmd, " | cat");
> +#endif
>  	if (args->compression_level >= 0)
>  		strbuf_addf(&cmd, " -%d", args->compression_level);

Do we need to? It seems to me that defaulting to "gzip -cn | cat" is not "we are on Windows, a platform that needs a special workaround in git", but rather "this gzip is horribly broken, but at build-time you can set a gzip that works".

So if the user wants to specify some broken filter, it is up to them to add "| cat" if their filter merits it.

But that is somewhat a matter of perception, and it won't make a user on Windows who does "git config archive.bz2 bzip2 -c" any happier when they are told it is their responsibility to deal with it.

BTW, as nice as this "gzip -cn | cat" idea is, I think it needs to be wrapped in a shell script. With the code above, we will generate "gzip -cn | cat -9". So we really need:

  $ cat `which gzip`
  #!/bin/sh
  gzip.real -cn "$@" | cat

and then no hacks need to go into git at all. The fix is about providing a sane gzip, not fixing git.

BTW, from what Johannes said, the issue is about a non-msys program calling an msys one. Does that mean that having git run:

  sh -c 'gzip -cn'

would work? If so, then could the solution be as simple as turning off the "don't bother with the shell" optimization that run-command uses? Something like "gzip -cn" gets split by git and run via spawn now (because it has no metacharacters). But we could easily make it always run through the shell.

-Peff