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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 1, 2011 at 7:46 PM, Jeff King <peff@xxxxxxxx> wrote:
> 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 ;)

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

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

>> 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 ;)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]