Re: [PATCH v2] git-archive: Add new option "--output" to write archive to a file instead of stdout.

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

 



carlos.duclos@xxxxxxxxx schrieb:
> NOTE: I can only use a webmail client, so some of the tabs might have
> overwritten by it. If  that's the case I'll resend the patch as MIME
> attachment.

Please do, as applying the patch as-is would be difficult, painful even.

> When archiving a repository there is no way to specify a file as output.
> This patch adds a new option "--output" that redirects the output to a
> file instead of stdout.
> 
> Signed-off by: Carlos Manuel Duclos Vergara <carlos.duclos@xxxxxxxxx>
> ---
>  Documentation/git-archive.txt |    3 +
>  archive-zip.c                 |    1 -
>  archive.c                     |   17 +++++
>  t/t0024-crlf-archive.sh       |   19 +++++
>  t/t5000-tar-tree.sh           |  148 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 187 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
> index 41cbf9c..04b05f3 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=<file>::
> +       Writes the archive to <file> instead of stdout.

Please use imperative ("Write") instead of present tense to match the
style used to describe other options.

> +
>  <extra>::
>         This can be any options that the archiver backend understand.
>         See next section.
> diff --git a/archive-zip.c b/archive-zip.c
> index cf28504..de5e540 100644
> --- a/archive-zip.c
> +++ b/archive-zip.c
> @@ -266,7 +266,6 @@ int write_zip_archive(struct archiver_args *args)
>         int err;
> 
>         dos_time(&args->time, &zip_date, &zip_time);
> -
>         zip_dir = xmalloc(ZIP_DIRECTORY_MIN_SIZE);
>         zip_dir_size = ZIP_DIRECTORY_MIN_SIZE;
> 

This hunk doesn't seem to belong in here.

> diff --git a/archive.c b/archive.c
> index e6de039..fde8602 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -239,6 +239,18 @@ static void parse_treeish_arg(const char **argv,
>         ar_args->time = archive_time;
>  }
> 
> +static void create_output_file(const char *output_file)
> +{
> +       int output_fd = creat(output_file, 0666);
> +       if (output_fd < 0)
> +               die("could not create archive file");

Reporting the file name here could help the user to spot typos.

> +       if (dup2(output_fd, 1) != 1)
> +       {
> +               close(output_fd);
> +               die("could not redirect output");
> +       }
> +}

Style:
	if (condition) {
		do(something);
		...
	}

output_fd can be closed after dup2()ing.

A successful dup2() call can return 0 on some systems (mingw here).

> +
>  #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 +265,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;

One tab indent, please.

>         int compression_level = -1;
>         int verbose = 0;
>         int i;
> @@ -262,6 +275,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, "file", "write the results to this file"),

Please wrap lines to fit into 80 columns, like it was done for the
preceding option.  And perhaps replace "results" with "archive"?

>                 OPT__VERBOSE(&verbose),
>                 OPT__COMPR('0', &compression_level, "store only", 0),
>                 OPT__COMPR('1', &compression_level, "compress faster", 1),
> @@ -294,6 +308,9 @@ static int parse_archive_args(int argc, const char **argv,
>         if (!base)
>                 base = "";
> 
> +       if(output)
> +               create_output_file(output);
> +

Ah, you make it possible to write the list of formats into a file,
too, as Junio mentioned.  So the "results" above is correct after
all.  But is this intended?

>         if (list) {
>                 for (i = 0; i < ARRAY_SIZE(archivers); i++)
>                         printf("%s\n", archivers[i].name);
> diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
> index e533039..237a8f6 100755
> --- a/t/t0024-crlf-archive.sh
> +++ b/t/t0024-crlf-archive.sh
> @@ -26,6 +26,15 @@ test_expect_success 'tar archive' '
> 
>  '
> 
> +test_expect_success 'tar archive output redirected' '
> +
> +       git archive --format=tar --output=test.tar HEAD &&
> +       ( mkdir untarred2 && cd untarred2 && "$TAR" -xf ../test.tar )
> +
> +       test_cmp sample untarred2/sample
> +
> +'
> +
>  "$UNZIP" -v >/dev/null 2>&1
>  if [ $? -eq 127 ]; then
>         echo "Skipping ZIP test, because unzip was not found"
> @@ -43,4 +52,14 @@ test_expect_success 'zip archive' '
> 
>  '
> 
> +test_expect_success 'zip archive output redirected' '
> +
> +       git archive --format=zip --output=test.zip HEAD &&
> +
> +       ( mkdir unzipped2 && cd unzipped2 && unzip ../test.zip ) &&
> +
> +       test_cmp sample unzipped2/sample
> +
> +'
> +
>  test_done
> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
> index c942c8b..b11e504 100755
> --- a/t/t5000-tar-tree.sh
> +++ b/t/t5000-tar-tree.sh
> @@ -66,6 +66,11 @@ test_expect_success \
>      'remove ignored file' \
>      'rm a/ignored'
> 
> +
> +#
> +# Tar tests
> +#
> +
>  test_expect_success \
>      'git archive' \
>      'git archive HEAD >b.tar'
> @@ -160,6 +165,91 @@ test_expect_success \
>        diff a/substfile2 g/prefix/a/substfile2
>  '
> 
> +#
> +# Same tests as above but now using redirection
> +#
> +
> +test_expect_success \
> +    'git archive' \
> +    'git archive --output=b20.tar HEAD'
> +
> +test_expect_success \
> +    'git tar-tree' \
> +    'git tar-tree HEAD >b21.tar'
> +
> +test_expect_success \
> +    'git archive vs. git tar-tree' \
> +    'diff b20.tar b21.tar'
> +
> +test_expect_success \
> +    'git archive in a bare repo' \
> +    '(cd bare.git && git archive --output=../b22.tar HEAD)'
> +
> +test_expect_success \
> +    'git archive vs. the same in a bare repo' \
> +    'test_cmp b20.tar b22.tar'
> +
> +test_expect_success \
> +    'validate file modification time' \
> +    'mkdir extract2 &&
> +     "$TAR" xf b20.tar -C extract2 a/a &&
> +     test-chmtime -v +0 extract2/a/a |cut -f 1 >b20.mtime &&
> +     echo "1117231200" >expected.mtime &&
> +     diff expected.mtime b20.mtime'
> +
> +test_expect_success \
> +    'git get-tar-commit-id' \
> +    'git get-tar-commit-id <b20.tar >b20.commitid &&
> +     diff .git/$(git symbolic-ref HEAD) b20.commitid'
> +
> +test_expect_success \
> +    'extract tar archive' \
> +    '(mkdir b20 && cd b20 && "$TAR" xf -) <b20.tar'
> +
> +test_expect_success \
> +    'validate filenames' \
> +    '(cd b20/a && find .) | sort >b20.lst &&
> +     diff a.lst b20.lst'
> +
> +test_expect_success \
> +    'validate file contents' \
> +    'diff -r a b20/a'
> +
> +test_expect_success \
> +    'create archives with substfiles' \
> +    'echo "substfile?" export-subst >a/.gitattributes &&
> +     git archive --output=f20.tar HEAD &&
> +     git archive --prefix=prefix/ --output=g20.tar HEAD &&
> +     rm a/.gitattributes'
> +
> +test_expect_success \
> +    'extract substfiles' \
> +    '(mkdir f20 && cd f20 && "$TAR" xf -) <f20.tar'
> +
> +test_expect_success \
> +     'validate substfile contents' \
> +     'git log --max-count=1 "--pretty=format:A${SUBSTFORMAT}O" HEAD \
> +      >f20/a/substfile1.expected &&
> +      diff f20/a/substfile1.expected f20/a/substfile1 &&
> +      diff a/substfile2 f20/a/substfile2
> +'
> +
> +test_expect_success \
> +    'extract substfiles from archive with prefix' \
> +    '(mkdir g20 && cd g20 && "$TAR" xf -) <g20.tar'
> +
> +test_expect_success \
> +     'validate substfile contents from archive with prefix' \
> +     'git log --max-count=1 "--pretty=format:A${SUBSTFORMAT}O" HEAD \
> +      >g20/prefix/a/substfile1.expected &&
> +      diff g20/prefix/a/substfile1.expected g20/prefix/a/substfile1 &&
> +      diff a/substfile2 g20/prefix/a/substfile2
> +'
> +
> +#
> +# Zip tests
> +#
> +
>  test_expect_success \
>      'git archive --format=zip' \
>      'git archive --format=zip HEAD >d.zip'
> @@ -172,6 +262,26 @@ test_expect_success \
>      'git archive --format=zip vs. the same in a bare repo' \
>      'test_cmp d.zip d1.zip'
> 
> +#
> +# Same tests as above but now using redirection
> +#
> +
> +test_expect_success \
> +    'git archive --format=zip --output=d10.zip' \
> +    'git archive --format=zip --output=d10.zip HEAD'
> +
> +test_expect_success \
> +    'git archive --format=zip --output=d11.zip in a bare repo' \
> +    '(cd bare.git && git archive --format=zip --output=../d11.zip HEAD)'
> +
> +test_expect_success \
> +    'git archive --format=zip redirected output vs. the same in a bare repo' \
> +    'test_cmp d10.zip d11.zip'
> +
> +#
> +# Zip tests
> +#
> +
>  $UNZIP -v >/dev/null 2>&1
>  if [ $? -eq 127 ]; then
>         echo "Skipping ZIP tests, because unzip was not found"
> @@ -213,4 +323,42 @@ test_expect_success \
>      'git archive --list outside of a git repo' \
>      'GIT_DIR=some/non-existing/directory git archive --list'
> 
> +#
> +# Same tests as above but now with redirected output
> +#
> +
> +test_expect_success \
> +    'extract ZIP archive from redirected output  archive' \
> +    '(mkdir d10 && cd d10 && $UNZIP ../d10.zip)'
> +
> +test_expect_success \
> +    'validate filenames from redirected output  archive' \
> +    '(cd d10/a && find .) | sort >d10.lst &&
> +     diff a.lst d10.lst'
> +
> +test_expect_success \
> +    'validate file contents from redirected output  archive' \
> +    'diff -r a d10/a'
> +
> +test_expect_success \
> +    'git archive --format=zip with prefix from redirected output  archive' \
> +    'git archive --format=zip --prefix=prefix/ --output=e10.zip HEAD'
> +
> +test_expect_success \
> +    'extract ZIP archive with prefix from redirected output  archive' \
> +    '(mkdir e10 && cd e10 && $UNZIP ../e10.zip)'
> +
> +test_expect_success \
> +    'validate filenames with prefix from redirected output  archive' \
> +    '(cd e10/prefix/a && find .) | sort >e10.lst &&
> +     diff a.lst e10.lst'
> +
> +test_expect_success \
> +    'validate file contents with prefix from redirected output  archive' \
> +    'diff -r a e10/prefix/a'
> +
> +test_expect_success \
> +    'git archive --list outside of a git repo' \
> +    'GIT_DIR=some/non-existing/directory git archive --list'
> +
>  test_done
> --
> 1.6.2.rc0.63.g7cbd0.dirty
> 

Lot's of tests, nice!  But I think comparing the created archive to
the one written to stdout using the same parameters is sufficient.
There's no need to untar, unzip or otherwise look at the archives
again if they are the the same.

René
--
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]

  Powered by Linux