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

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Am 09.08.2011 07:02, schrieb Jeff King:
> 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".

It's an MSYS platform problem IMHO, and we can work around it.  That
extra cat is nothing to be proud of, sure.  And msysgit avoiding the
issue by distibuting a gzip that never converts line endings would solve
it without any runtime overhead.

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

Well, I'd rather take this responsibility from the user, who might
struggle a while before finding out what is going on.  However...

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

If all packers shipped with msysgit are usable then we're probably good.

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

Yes, the three added lines in the patch above would have to be moved
down two lines, after the compression level is added.  D'oh!

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

OK, that's one way to do it; another would be let gzip (and bzip2 etc.)
do whatever cat does to avoid end of line conversions.  And yet another
is to take them from http://unxutils.sourceforge.net/.

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

Just checked -- it doesn't work.  I assume that's because the shell is
also an MSYS program.

René