Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork
- From
René Scharfe <rene.scharfe@lsrfire.ath.cx>
- Date
- Aug 9, 2011, 10:25 UTC
- Message-ID
- <4E410B26.1080407@lsrfire.ath.cx>
- In-Reply-To
- <20110809050211.GA3588@sigill.intra.peff.net>
Am 09.08.2011 07:02, schrieb Jeff King:
Show 35 quoted lines
> 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!
Show 8 quoted lines
> 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/.
Show 10 quoted lines
> 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é