From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Mon, 01 Aug 2011 14:45:03 GMT
Subject: Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork
Message-ID: <CABPQNSaqyD+rhWPRbtVdnkweuXSycBahKEsasGZkEg3mi4SaxQ@mail.gmail.com>
In-Reply-To: <20110728170222.GB15931@sigill.intra.peff.net>

On Thu, Jul 28, 2011 at 7:02 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Jul 28, 2011 at 10:47:58AM -0600, Jeff King wrote:
>
>> Ah, nevermind. I see now; the command-line option interface to "git
>> archive" is not as rich as what you can pass via write_archive.
>>
>> I think you can get around it by adding an option to "git archive" to
>> indicate that we are filling a remote request, which is the only
>> extra information that my series adds.
>
> 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?

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

> Applied on top of the merge of your series into next, this
> passes t5000.

Unfortunately, it doesn't pass on Windows, but this doesn't seem to be
directly related to the remote-stuff: Here's the tests that fail:


$ 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

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?

>
> -Peff
>
> ---
> diff --git a/builtin/archive.c b/builtin/archive.c
> index 883c009..6668340 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -85,6 +85,7 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
>        const char *exec = "git-upload-archive";
>        const char *output = NULL;
>        const char *remote = NULL;
> +       int is_remote = 0;
>        struct option local_opts[] = {
>                OPT_STRING('o', "output", &output, "file",
>                        "write the archive to this file"),
> @@ -92,6 +93,9 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
>                        "retrieve the archive from remote repository <repo>"),
>                OPT_STRING(0, "exec", &exec, "cmd",
>                        "path to the remote git-upload-archive command"),
> +               { OPTION_BOOLEAN, 0, "remote-request", &is_remote, NULL,
> +                       "indicate we are serving a remote request",
> +                       PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
>                OPT_END()
>        };
>
> @@ -106,5 +110,5 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
>
>        setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
>
> -       return write_archive(argc, argv, prefix, 1, output, 0);
> +       return write_archive(argc, argv, prefix, 1, output, is_remote);
>  }
> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
> index 0c192b5..c57e8bd 100644
> --- a/builtin/upload-archive.c
> +++ b/builtin/upload-archive.c
> @@ -27,8 +27,9 @@ static void prepare_argv(const char **sent_argv, const char **argv)
>        int len;
>
>        /* put received options in sent_argv[] */
> -       sent_argc = 1;
> +       sent_argc = 2;
>        sent_argv[0] = "archive";
> +       sent_argv[1] = "--remote-request";
>        for (p = buf;;) {
>                /* This will die if not enough free space in buf */
>                len = packet_read_line(0, p, (buf + sizeof buf) - p);
>