[PATCH] When archiving a repository there is no way to specify a file as output.

[PATCH] When archiving a repository there is no way to specify a file as output.

[carlos.duclos]

Hi,

Short description:
To enable that I added a new option "--output" that will redirect the output to a file instead to stdout.

Long description:
When archiving a repository there is no way to send the result to a file without using redirection at shell level. Since there are situations where redirection is not available, for instance when running git inside a continuous integration system which is already redirecting the output, I added an option to write the archive to a file directly.
In order to do that I added a new option to archiver_args, int output_fd, which holds the file descriptor where the resulting archive should be written. If no option is specified in command line that option defaults to 1 and no behavior change, however if the "--output" option is specified and the file was created that file descriptor points to the new file. I also modified the write_or_die calls to use "output_fd" instead of 1, so they write to the file descriptor.

>From 10e09bf828c18f2846651262b7f647b630e09872 Mon Sep 17 00:00:00 2001
From: Carlos Manuel Duclos Vergara <carlos.duclos@trolltech.com>
Date: Fri, 13 Feb 2009 16:09:39 +0100
Subject: [PATCH] When archiving a repository there is no way to specify a file as output.
 To enable that I added a new option "--output" that will redirect the output to a file instead to stdout.

Signed-off-by: Carlos Manuel Duclos Vergara <carlos.duclos@nokia.com>

---
 archive-tar.c |   16 ++++++++++++----
 archive-zip.c |   21 ++++++++++++++-------
 archive.c     |   20 ++++++++++++++++++++
 archive.h     |    1 +
 4 files changed, 47 insertions(+), 11 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index ba890eb..4d8fc03 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -13,11 +13,13 @@ static unsigned long offset;

 static int tar_umask = 002;

+static int output_fd = 1;
+
 /* writes out the whole block, but only if it is full */
 static void write_if_needed(void)
 {
        if (offset == BLOCKSIZE) {
-               write_or_die(1, block, BLOCKSIZE);
+               write_or_die(output_fd, block, BLOCKSIZE);
                offset = 0;
        }
 }
@@ -42,7 +44,7 @@ static void write_blocked(const void *data, unsigned long size)
                write_if_needed();
        }
        while (size >= BLOCKSIZE) {
-               write_or_die(1, buf, BLOCKSIZE);
+               write_or_die(output_fd, buf, BLOCKSIZE);
                size -= BLOCKSIZE;
                buf += BLOCKSIZE;
        }
@@ -66,10 +68,10 @@ static void write_trailer(void)
 {
        int tail = BLOCKSIZE - offset;
        memset(block + offset, 0, tail);
-       write_or_die(1, block, BLOCKSIZE);
+       write_or_die(output_fd, block, BLOCKSIZE);
        if (tail < 2 * RECORDSIZE) {
                memset(block, 0, offset);
-               write_or_die(1, block, BLOCKSIZE);
+               write_or_die(output_fd, block, BLOCKSIZE);
        }
 }

@@ -234,11 +236,17 @@ static int git_tar_config(const char *var, const char *value, void *cb)
        return git_default_config(var, value, cb);
 }

+static void setup_write_backend(struct archiver_args *args)
+{
+       output_fd = args->output_fd;
+}
+
 int write_tar_archive(struct archiver_args *args)
 {
        int err = 0;

        git_config(git_tar_config, NULL);
+       setup_write_backend(args);

        if (args->commit_sha1)
                err = write_global_extended_header(args);
diff --git a/archive-zip.c b/archive-zip.c
index cf28504..dd3ba27 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -14,6 +14,8 @@ static unsigned int zip_offset;
 static unsigned int zip_dir_offset;
 static unsigned int zip_dir_entries;

+static int output_fd = 1;
+
 #define ZIP_DIRECTORY_MIN_SIZE (1024 * 1024)

 struct zip_local_header {
@@ -219,12 +221,12 @@ static int write_zip_entry(struct archiver_args *args,
        copy_le32(header.size, uncompressed_size);
        copy_le16(header.filename_length, pathlen);
        copy_le16(header.extra_length, 0);
-       write_or_die(1, &header, ZIP_LOCAL_HEADER_SIZE);
+       write_or_die(output_fd, &header, ZIP_LOCAL_HEADER_SIZE);
        zip_offset += ZIP_LOCAL_HEADER_SIZE;
-       write_or_die(1, path, pathlen);
+       write_or_die(output_fd, path, pathlen);
        zip_offset += pathlen;
        if (compressed_size > 0) {
-               write_or_die(1, out, compressed_size);
+               write_or_die(output_fd, out, compressed_size);
                zip_offset += compressed_size;
        }

@@ -246,10 +248,10 @@ static void write_zip_trailer(const unsigned char *sha1)
        copy_le32(trailer.offset, zip_offset);
        copy_le16(trailer.comment_length, sha1 ? 40 : 0);

-       write_or_die(1, zip_dir, zip_dir_offset);
-       write_or_die(1, &trailer, ZIP_DIR_TRAILER_SIZE);
+       write_or_die(output_fd, zip_dir, zip_dir_offset);
+       write_or_die(output_fd, &trailer, ZIP_DIR_TRAILER_SIZE);
        if (sha1)
-               write_or_die(1, sha1_to_hex(sha1), 40);
+               write_or_die(output_fd, sha1_to_hex(sha1), 40);
 }

 static void dos_time(time_t *time, int *dos_date, int *dos_time)
@@ -261,12 +263,17 @@ static void dos_time(time_t *time, int *dos_date, int *dos_time)
        *dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;
 }

+static void setup_write_backend(struct archiver_args *args)
+{
+       output_fd = args->output_fd;
+}
+
 int write_zip_archive(struct archiver_args *args)
 {
        int err;

        dos_time(&args->time, &zip_date, &zip_time);
-
+       setup_write_backend(args);
        zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
        zip_dir_size = ZIP_DIRECTORY_MIN_SIZE;

diff --git a/archive.c b/archive.c
index e6de039..420b853 100644
--- a/archive.c
+++ b/archive.c
@@ -239,6 +239,16 @@ static void parse_treeish_arg(const char **argv,
        ar_args->time = archive_time;
 }

+static void create_output_file(const char *output_file, struct archiver_args *ar_args)
+{
+       int fd = -1;
+
+       fd = creat(output_file, S_IRUSR | S_IWUSR);
+       if(fd < 0)
+               die("could not create archive file");
+       ar_args->output_fd = fd;
+}
+
 #define OPT__COMPR(s, v, h, p) \
        { OPTION_SET_INT, (s), NULL, (v), NULL, (h), \
          PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (p) }
@@ -253,6 +263,7 @@ static int parse_archive_args(int argc, const char **argv,
        const char *base = NULL;
        const char *remote = NULL;
        const char *exec = NULL;
+    const char *output = NULL;
        int compression_level = -1;
        int verbose = 0;
        int i;
@@ -262,6 +273,7 @@ static int parse_archive_args(int argc, const char **argv,
                OPT_STRING(0, "format", &format, "fmt", "archive format"),
                OPT_STRING(0, "prefix", &base, "prefix",
                        "prepend prefix to each pathname in the archive"),
+               OPT_STRING(0, "output", &output, "output file", "write the results to this file"),
                OPT__VERBOSE(&verbose),
                OPT__COMPR('0', &compression_level, "store only", 0),
                OPT__COMPR('1', &compression_level, "compress faster", 1),
@@ -294,6 +306,14 @@ static int parse_archive_args(int argc, const char **argv,
        if (!base)
                base = "";

+       if(output)
+               create_output_file(output, args);
+       else
+               /*
+                * Default to stdout
+                */
+               args->output_fd = 1;
+
        if (list) {
                for (i = 0; i < ARRAY_SIZE(archivers); i++)
                        printf("%s\n", archivers[i].name);
diff --git a/archive.h b/archive.h
index 0b15b35..67524fa 100644
--- a/archive.h
+++ b/archive.h
@@ -9,6 +9,7 @@ struct archiver_args {
        const struct commit *commit;
        time_t time;
        const char **pathspec;
+       int output_fd;
        unsigned int verbose : 1;
        int compression_level;
 };
--
1.6.0.2

>From 7cbd0a3edb1cf75b5a0644263e1755fd18a5c37d Mon Sep 17 00:00:00 2001
From: Carlos Manuel Duclos Vergara <carlos.duclos@trolltech.com>
Date: Fri, 13 Feb 2009 16:22:21 +0100
Subject: [PATCH] Updating documentation for git-archive in order to reflect the new "--output" option.

Signed-off-by: Carlos Manuel Duclos Vergara <carlos.duclos@nokia.com>

---
 Documentation/git-archive.txt |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index 41cbf9c..d1a9d95 100644
--- a/Documentation/git-archive.txt
+++ b/Documentation/git-archive.txt
@@ -47,6 +47,9 @@ OPTIONS
 --prefix=<prefix>/::
        Prepend <prefix>/ to each filename in the archive.

+--output=<output file>::
+       Writes the archive to <output file> instead of stdout.
+
 <extra>::
        This can be any options that the archiver backend understand.
        See next section.
--
1.6.0.2

Re: [PATCH] When archiving a repository there is no way to specify a file as output.

[Jakub Narebski]

<carlos.duclos@nokia.com> writes:

First, the comments below are not indication that this patch is not a
good idea, because in my opinion it is.  This review is about
_technical_ shortcomings...

> Short description:
> ==================
> To enable that I added a new option "--output" that will redirect
> the output to a file instead to stdout.
> 
> Long description:
> =================
>
> When archiving a repository there is no way to send the result to a
> file without using redirection at shell level. Since there are
> situations where redirection is not available, for instance when
> running git inside a continuous integration system which is already
> redirecting the output, I added an option to write the archive to a
> file directly.
>
> In order to do that I added a new option to archiver_args, int
> output_fd, which holds the file descriptor where the resulting
> archive should be written. If no option is specified in command line
> that option defaults to 1 and no behavior change, however if the
> "--output" option is specified and the file was created that file
> descriptor points to the new file. I also modified the write_or_die
> calls to use "output_fd" instead of 1, so they write to the file
> descriptor.
>

There should be separator, e.g.
-- >8 --
to mark where comments ends and commit message begins.
 
> From 10e09bf828c18f2846651262b7f647b630e09872 Mon Sep 17 00:00:00 2001

The above line is not necessary.

> From: Carlos Manuel Duclos Vergara <carlos.duclos@trolltech.com>
> Date: Fri, 13 Feb 2009 16:09:39 +0100

Usually 'From:' is required only if the value from email cannot be
used, either because you send email on behalf of somebody else
(somebody else is author of patch), or because you are sending from
alternate email address.

The 'Date:' header is very rarely required: the one from email should
be good enough in most cases.

> Subject: [PATCH] When archiving a repository there is no way to specify a file as output.
>  To enable that I added a new option "--output" that will redirect the output to a file instead to stdout.

Git commit message conventions (see Documentation/SubmittingPatches)
call for short description or a commit/patch (commit summary) in first
line: that is what it would be in email subject ('Subject:' line
above), followed by a more detailed description.

Your summary (subject of patch) is both too long, and not standalone.
I think the better choice would be:

  Subject: [PATCH] git-archive: Add new option "--output" to write to a file

  When archiving a repository there is no way to specify a file as
  output.  This patch a new option "--output" that will redirect the
  output to a file instead to stdout.

I think however that some of 'long description' above, or perhaps even
whole of it, should be put in commit message itself, and not be only
as a comment (present only in mailing list archives).

> 
> Signed-off-by: Carlos Manuel Duclos Vergara <carlos.duclos@nokia.com>

Why do you use different email address for signoff, and for authorship
('From:' line)?  Was it intended, or was it an accident?

[...]
> From 7cbd0a3edb1cf75b5a0644263e1755fd18a5c37d Mon Sep 17 00:00:00 2001
> From: Carlos Manuel Duclos Vergara <carlos.duclos@trolltech.com>
> Date: Fri, 13 Feb 2009 16:22:21 +0100
> Subject: [PATCH] Updating documentation for git-archive in order to reflect the new "--output" option.
> 
> Signed-off-by: Carlos Manuel Duclos Vergara <carlos.duclos@nokia.com>

First, this should really be together in one, single patch.

Second, there is convention of sending patch series as a _series_ of
emails, eother as responses to cover letter message, or 'chained' so
subsequent patches (emails) are replies to earlier ones.  For better
readibility, and in case some patches get lost or be sorted out of
order by email/news client, there is convention of using [PATCH n/m]
prefix.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] When archiving a repository there is no way to specify a file as output.

[René Scharfe]

carlos.duclos@nokia.com schrieb:
> Hi,
> 
> Short description:
> To enable that I added a new option "--output" that will redirect the output to a file instead to stdout.
> 
> Long description:
> When archiving a repository there is no way to send the result to a file without using redirection at shell level. Since there are situations where redirection is not available, for instance when running git inside a continuous integration system which is already redirecting the output, I added an option to write the archive to a file directly.
> In order to do that I added a new option to archiver_args, int output_fd, which holds the file descriptor where the resulting archive should be written. If no option is specified in command line that option defaults to 1 and no behavior change, however if the "--output" option is specified and the file was created that file descriptor points to the new file. I also modified the write_or_die calls to use "output_fd" instead of 1, so they write to the file descriptor.
> 
> From 10e09bf828c18f2846651262b7f647b630e09872 Mon Sep 17 00:00:00 2001
> From: Carlos Manuel Duclos Vergara <carlos.duclos@trolltech.com>
> Date: Fri, 13 Feb 2009 16:09:39 +0100
> Subject: [PATCH] When archiving a repository there is no way to specify a file as output.
>  To enable that I added a new option "--output" that will redirect the output to a file instead to stdout.
> 
> Signed-off-by: Carlos Manuel Duclos Vergara <carlos.duclos@nokia.com>
> 
> ---
>  archive-tar.c |   16 ++++++++++++----
>  archive-zip.c |   21 ++++++++++++++-------
>  archive.c     |   20 ++++++++++++++++++++
>  archive.h     |    1 +
>  4 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/archive-tar.c b/archive-tar.c
> index ba890eb..4d8fc03 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -13,11 +13,13 @@ static unsigned long offset;
> 
>  static int tar_umask = 002;
> 
> +static int output_fd = 1;
> +

Static variables hinder libification, but there are already lots of them in
the two archive backends, so I'm neutral on that.

>  /* writes out the whole block, but only if it is full */
>  static void write_if_needed(void)
>  {
>         if (offset == BLOCKSIZE) {
> -               write_or_die(1, block, BLOCKSIZE);
> +               write_or_die(output_fd, block, BLOCKSIZE);
>                 offset = 0;
>         }
>  }
> @@ -42,7 +44,7 @@ static void write_blocked(const void *data, unsigned long size)
>                 write_if_needed();
>         }
>         while (size >= BLOCKSIZE) {
> -               write_or_die(1, buf, BLOCKSIZE);
> +               write_or_die(output_fd, buf, BLOCKSIZE);
>                 size -= BLOCKSIZE;
>                 buf += BLOCKSIZE;
>         }
> @@ -66,10 +68,10 @@ static void write_trailer(void)
>  {
>         int tail = BLOCKSIZE - offset;
>         memset(block + offset, 0, tail);
> -       write_or_die(1, block, BLOCKSIZE);
> +       write_or_die(output_fd, block, BLOCKSIZE);
>         if (tail < 2 * RECORDSIZE) {
>                 memset(block, 0, offset);
> -               write_or_die(1, block, BLOCKSIZE);
> +               write_or_die(output_fd, block, BLOCKSIZE);
>         }
>  }
> 
> @@ -234,11 +236,17 @@ static int git_tar_config(const char *var, const char *value, void *cb)
>         return git_default_config(var, value, cb);
>  }
> 
> +static void setup_write_backend(struct archiver_args *args)
> +{
> +       output_fd = args->output_fd;
> +}
> +

This wrapper doesn't buy you much and is only called once.  Please
just set output_fd directly below.

>  int write_tar_archive(struct archiver_args *args)
>  {
>         int err = 0;
> 
>         git_config(git_tar_config, NULL);
> +       setup_write_backend(args);
> 
>         if (args->commit_sha1)
>                 err = write_global_extended_header(args);
> diff --git a/archive-zip.c b/archive-zip.c
> index cf28504..dd3ba27 100644
> --- a/archive-zip.c
> +++ b/archive-zip.c
> @@ -14,6 +14,8 @@ static unsigned int zip_offset;
>  static unsigned int zip_dir_offset;
>  static unsigned int zip_dir_entries;
> 
> +static int output_fd = 1;
> +
>  #define ZIP_DIRECTORY_MIN_SIZE (1024 * 1024)
> 
>  struct zip_local_header {
> @@ -219,12 +221,12 @@ static int write_zip_entry(struct archiver_args *args,
>         copy_le32(header.size, uncompressed_size);
>         copy_le16(header.filename_length, pathlen);
>         copy_le16(header.extra_length, 0);
> -       write_or_die(1, &header, ZIP_LOCAL_HEADER_SIZE);
> +       write_or_die(output_fd, &header, ZIP_LOCAL_HEADER_SIZE);
>         zip_offset += ZIP_LOCAL_HEADER_SIZE;
> -       write_or_die(1, path, pathlen);
> +       write_or_die(output_fd, path, pathlen);
>         zip_offset += pathlen;
>         if (compressed_size > 0) {
> -               write_or_die(1, out, compressed_size);
> +               write_or_die(output_fd, out, compressed_size);
>                 zip_offset += compressed_size;
>         }
> 
> @@ -246,10 +248,10 @@ static void write_zip_trailer(const unsigned char *sha1)
>         copy_le32(trailer.offset, zip_offset);
>         copy_le16(trailer.comment_length, sha1 ? 40 : 0);
> 
> -       write_or_die(1, zip_dir, zip_dir_offset);
> -       write_or_die(1, &trailer, ZIP_DIR_TRAILER_SIZE);
> +       write_or_die(output_fd, zip_dir, zip_dir_offset);
> +       write_or_die(output_fd, &trailer, ZIP_DIR_TRAILER_SIZE);
>         if (sha1)
> -               write_or_die(1, sha1_to_hex(sha1), 40);
> +               write_or_die(output_fd, sha1_to_hex(sha1), 40);
>  }
> 
>  static void dos_time(time_t *time, int *dos_date, int *dos_time)
> @@ -261,12 +263,17 @@ static void dos_time(time_t *time, int *dos_date, int *dos_time)
>         *dos_time = t->tm_sec / 2 + t->tm_min * 32 + t->tm_hour * 2048;
>  }
> 
> +static void setup_write_backend(struct archiver_args *args)
> +{
> +       output_fd = args->output_fd;
> +}
> +

Same here.

>  int write_zip_archive(struct archiver_args *args)
>  {
>         int err;
> 
>         dos_time(&args->time, &zip_date, &zip_time);
> -
> +       setup_write_backend(args);
>         zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
>         zip_dir_size = ZIP_DIRECTORY_MIN_SIZE;
> 
> diff --git a/archive.c b/archive.c
> index e6de039..420b853 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -239,6 +239,16 @@ static void parse_treeish_arg(const char **argv,
>         ar_args->time = archive_time;
>  }
> 
> +static void create_output_file(const char *output_file, struct archiver_args *ar_args)
> +{
> +       int fd = -1;
> +
> +       fd = creat(output_file, S_IRUSR | S_IWUSR);
> +       if(fd < 0)
> +               die("could not create archive file");
> +       ar_args->output_fd = fd;
> +}
> +

The local variable fd is initialized immediately before it's set again.
In fact, you don't need even need it at all -- use ar_args->output_fd
directly.

0600 (-rw-------) is a nice and secure permissions for newly created
files, but I think we should apply the users umask here.  tar does,
at least.

>  #define OPT__COMPR(s, v, h, p) \
>         { OPTION_SET_INT, (s), NULL, (v), NULL, (h), \
>           PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, (p) }
> @@ -253,6 +263,7 @@ static int parse_archive_args(int argc, const char **argv,
>         const char *base = NULL;
>         const char *remote = NULL;
>         const char *exec = NULL;
> +    const char *output = NULL;

Please indent using tabs, a single one in this line.  Indeed, your
message doesn't contain a single tab, which makes the patch difficult
to apply.  Please see Documentation/SubmittingPatches for how to
make and send patches for git.

>         int compression_level = -1;
>         int verbose = 0;
>         int i;
> @@ -262,6 +273,7 @@ static int parse_archive_args(int argc, const char **argv,
>                 OPT_STRING(0, "format", &format, "fmt", "archive format"),
>                 OPT_STRING(0, "prefix", &base, "prefix",
>                         "prepend prefix to each pathname in the archive"),
> +               OPT_STRING(0, "output", &output, "output file", "write the results to this file"),

Please wrap lines to make them fit into 80 columns.  You can mimic the style
of the option just above the one you're adding.

I think "file" is better as the fourth parameter, as this is, well, just
another command line argument specifying a file.  That it's used for output
doesn't place it in a new class of arguments.  (See comment about argh in
parse-options.h.)

>                 OPT__VERBOSE(&verbose),
>                 OPT__COMPR('0', &compression_level, "store only", 0),
>                 OPT__COMPR('1', &compression_level, "compress faster", 1),
> @@ -294,6 +306,14 @@ static int parse_archive_args(int argc, const char **argv,
>         if (!base)
>                 base = "";
> 
> +       if(output)
> +               create_output_file(output, args);
> +       else
> +               /*
> +                * Default to stdout
> +                */
> +               args->output_fd = 1;
> +
>         if (list) {
>                 for (i = 0; i < ARRAY_SIZE(archivers); i++)
>                         printf("%s\n", archivers[i].name);

You don't close the file, which works now because archive terminates
shortly after the last file has been written, but it's unclean.  I
think the best place to put creat() and close() is before and after
the last line of write_archive().

> diff --git a/archive.h b/archive.h
> index 0b15b35..67524fa 100644
> --- a/archive.h
> +++ b/archive.h
> @@ -9,6 +9,7 @@ struct archiver_args {
>         const struct commit *commit;
>         time_t time;
>         const char **pathspec;
> +       int output_fd;
>         unsigned int verbose : 1;
>         int compression_level;
>  };
> --
> 1.6.0.2
> 
> From 7cbd0a3edb1cf75b5a0644263e1755fd18a5c37d Mon Sep 17 00:00:00 2001
> From: Carlos Manuel Duclos Vergara <carlos.duclos@trolltech.com>
> Date: Fri, 13 Feb 2009 16:22:21 +0100
> Subject: [PATCH] Updating documentation for git-archive in order to reflect the new "--output" option.
> 
> Signed-off-by: Carlos Manuel Duclos Vergara <carlos.duclos@nokia.com>
> 
> ---
>  Documentation/git-archive.txt |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
> index 41cbf9c..d1a9d95 100644
> --- a/Documentation/git-archive.txt
> +++ b/Documentation/git-archive.txt
> @@ -47,6 +47,9 @@ OPTIONS
>  --prefix=<prefix>/::
>         Prepend <prefix>/ to each filename in the archive.
> 
> +--output=<output file>::
> +       Writes the archive to <output file> instead of stdout.
> +
>  <extra>::
>         This can be any options that the archiver backend understand.
>         See next section.

This patch should be part of the one that changes the code.  You
don't need to (and should not) split this up, as this is one
consistent change.  Except for the missing test. :)

> --
> 1.6.0.2
> 

Thanks,
René

Re: [PATCH] When archiving a repository there is no way to specify a file as output.

[Junio C Hamano]

<carlos.duclos@nokia.com> writes:

> Short description:
> To enable that I added a new option "--output" that will redirect the
> output to a file instead to stdout.

I won't discuss patch submission technicalities here; they are important
to help you get understood by others, but Jakub covered the issues very
well already.

> When archiving a repository there is no way to send the result to a file
> without using redirection at shell level. Since there are situations
> where redirection is not available, for instance when running git inside
> a continuous integration system which is already redirecting the output,
> I added an option to write the archive to a file directly.

If your CI can run 'git' with four strings 'archive', '--output',
'myout.tar', and 'HEAD' as its parameters, it can certainly run 'sh' with
parameters '-c', 'git archive "$0" >"$1"', 'HEAD' and 'myout.tar'.

In other words, saying "sometimes redirection is not available, and we
must support output file parameter" is not a convincing justification.

The patch is in the "It would be nice if the command allowed --output=file
option, and here is a patch to do so" category.

Which is still a good reason that justifies the change, but it is more
honest and does not overstate its importance.

> diff --git a/archive-zip.c b/archive-zip.c
> index cf28504..dd3ba27 100644
> --- a/archive-zip.c
> +++ b/archive-zip.c
> @@ -14,6 +14,8 @@ static unsigned int zip_offset;
>  static unsigned int zip_dir_offset;
>  static unsigned int zip_dir_entries;
>
> +static int output_fd = 1;
> +
>  #define ZIP_DIRECTORY_MIN_SIZE (1024 * 1024)
>
>  struct zip_local_header {
> @@ -219,12 +221,12 @@ static int write_zip_entry(struct archiver_args
> *args,
>         copy_le32(header.size, uncompressed_size);
>         copy_le16(header.filename_length, pathlen);
>         copy_le16(header.extra_length, 0);
> -       write_or_die(1, &header, ZIP_LOCAL_HEADER_SIZE);
> +       write_or_die(output_fd, &header, ZIP_LOCAL_HEADER_SIZE);

This function takes archiver_args, so args->output_fd is available to you
without introducing a new global variable.  Woudln't it make more sense to
pass archiver_args around to functions that do write() but do not
currently receive it, and use args->output_fd throughout, getting rid of
the global variable?

> +static void create_output_file(const char *output_file, struct
> archiver_args *ar_args)
> +{
> +       int fd = -1;
> +
> +       fd = creat(output_file, S_IRUSR | S_IWUSR);

When the resulting mode does not matter, we try hard to let the user's
umask take effect by not limiting the mode like this ourselves.

> +       if(fd < 0)

Style... "if (fd < 0)".  They are everywhere...

> +static void create_output_file(const char *output_file, struct
> archiver_args *ar_args)
> +{
> +       int fd = -1;
> +
> +       fd = creat(output_file, S_IRUSR | S_IWUSR);
> +       if(fd < 0)
> +               die("could not create archive file");
> +       ar_args->output_fd = fd;
> +}
> +
> ...
> @@ -294,6 +306,14 @@ static int parse_archive_args(int argc, const
> char **argv,
>         if (!base)
>                 base = "";
>
> +       if(output)
> +               create_output_file(output, args);
> +       else
> ...

This is not wrong per-se but I do not like it very much.  Should "parse"
function make an externally observable side effect of creating a file?

It is plausible that sufficiently sick person may want to run

	git archive --output=msgfile --list

to get the list of supported formats in the message file, but if that is
the use case you are trying to support, obviously we would need to open
the output file and dup it to fd=1 instead ;-).

No, I am not seriously suggesting to support the above as a valid use
case, but open-to-dup trick might make the patch much smaller.

Also the correctness of this code is a bit tricky; parse_archive_args() is
called by write_archive() before setup_git_directory() is called, and it
depends on this initialization order to work correctly from inside a
subdirectory.  Otherwise you would be prepending the "prefix" to "output"
before calling create_output_file() here.  It might warrant a comment to
note this fact here.

> ---
>  Documentation/git-archive.txt |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)

Squash this into the main patch so that the code change and documentation
updates go hand in hand, please.

Also this needs a new test.