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

[Jeff King <peff@peff.net>]

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

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