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

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Aug 1, 2011, 18:02 UTC
Message-ID: <CABPQNSbyp2hEgXvzo3bMHHxrbc9fQLqaABweqfjP7xQzBUdj6A@mail.gmail.com>
In-Reply-To: <20110801174603.GB10302@sigill.intra.peff.net>

On Mon, Aug 1, 2011 at 7:46 PM, Jeff King <peff@peff.net> wrote:

Show 12 quoted lines

> On Mon, Aug 01, 2011 at 04:45:03PM +0200, Erik Faye-Lund wrote:
>
>> > And that patch would look something like the one below.
>>
>> This is the most straight forward way of doing this, thanks. I'll post
>> a new version with this squashed in soon.
>>
>> What's the proper way of attributing you for the work?
>
> If you're squashing, just make a note in the text, or add a Helped-by
> header at the end.
>

OK, that's basically what I did already ;)

Show 24 quoted lines

>> > You can also now
>> > drop the "remote" parameter to write_archive entirely, but I didn't do
>> > so here.
>>
>> I'm not entirely sure how you propose we do this... do you mean by
>> hoisting the relevant logic from write_archive up to cmd_archive or
>> something?
>
> Sorry, I wrote that comment, then totally changed how my patch was
> implemented and failed to update the comment. :)
>
> The arguments to git-archive actually get parsed in two places:
>
>  1. cmd_archive passes them to decide on meta stuff like remote vs
>     local
>
>  2. write_archive has a parse_archive_args function to actually parse
>     the arguments that both upload-archive and archive share
>
> My patch puts the new argument in (1). We could put it in (2) and do
> away with passing the flag all the way down the call stack. But it seems
> conceptually cleaner to me to have it in (1) (and the diff is much
> shorter, too).
>

Makes sense. Thanks for the explanation, I agree.

Show 36 quoted lines

>> $ make t5000-tar-tree
>> *** t5000-tar-tree.sh ***
>> ok 1 - populate workdir
>> <snip>
>> ok 53 - infer tgz from .tar.gz filename
>> not ok - 54 extract tgz file
>> #
>> #               $GUNZIP -c <j.tgz >j.tar &&
>> #               test_cmp b.tar j.tar
>> #
>> not ok - 55 remote tar.gz is allowed by default
>> #
>> #               git archive --remote=. --format=tar.gz HEAD >remote.tar.gz &&
>> #               test_cmp j.tgz remote.tar.gz
>> #
>> ok 56 - remote tar.gz can be disabled
>> # failed 2 among 56 test(s)
>> 1..56
>> make: *** [t5000-tar-tree.sh] Error 1
>>
>> It seems "git archive --format=tgz HEAD" is broken on Windows:
>> $ git archive --format=tgz HEAD > j.tgz
>> $ gunzip -c j.tgz > j.tar && echo ok
>>
>> gzip: j.tgz: invalid compressed data--format violated
>>
>> But if I perform the compression manually, the archive is fine:
>> $ git archive --format=tar HEAD | gzip -cn > j.tgz
>> $ gunzip -c j.tgz > j.tar && echo ok
>> ok
>
> Weird. What does j.tgz end up looking like? Is it empty, or does it have
> bogus data in it? Does gzip actually get invoked at all? Try running
> with GIT_TRACE=1. I don't suppose you guys have something like strace,
> which might be helpful.
>

It does have data, and gzip does get invoked: $ gunzip -c j.tgz | wc -c

gzip: j.tgz: invalid compressed data--format violated
 131072

So it seems there are around 130k of valid data before it barfs.

Hmm, but when I try the same after re-running the test, I get a different amount of valid data (491520 bytes this time)... Is this a timing-related issue?

Yeah, strace would have been useful, but we don't have that on Windows. There are probably other alternatives, but I'm not aware... perhaps procmon can be of help?

Show 7 quoted lines

>> So, the big question is, are all tar-filters broken on Windows? It
>> seems not; the tests for the foo-filter works fine... any clues? Could
>> it somehow be newline-related, perhaps?
>
> I'm not sure that newlines would make a difference. We use straight
> write() everywhere in the archive code, which should be binary safe.
>

OK, so that hunch was a bad one ;)