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

From: Jeff King <peff@peff.net>
Date: Aug 9, 2011, 20:05 UTC
Message-ID: <20110809200504.GA31944@sigill.intra.peff.net>
In-Reply-To: <4E410B26.1080407@lsrfire.ath.cx>

On Tue, Aug 09, 2011 at 12:25:42PM +0200, René Scharfe wrote:

Show 6 quoted lines

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

Also, is adding "| cat" also sufficient for arbitrary shell code (i.e., whatever the user hands us via the config)? I couldn't think of an example that wouldn't work.

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

Yeah, I like all of those solutions better than hacking an extra pipe into git. I don't know enough to say how painful they are in practice, though.

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

Too bad. I guess the first msys program to be run sets the mode of the output descriptor, and then everybody else inherits that. If there were a helper or shell builtin to switch to binary mode, we could put it at the start of the shell pipeline.

If you write a C program that does:

  setmode(1, O_BINARY);

and call it like:

  ./setmode-helper; gzip -cn

does that work? I suspect not, as from my brief reading of the msys gzip source, it actually calls setmode() itself.

-Peff