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

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

[carlos.duclos]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

Patch attached as MIME to avoid conversion problems.

Patch highlights:
1. Formatted to 80 columns.
2. Change the language in the documentation to match the style.
3. Modified create_output_file.
3.1 Changed from creat(2) to open(2).
3.2 Changed the logic so we only dup the file if the fd is different from 1.
3.3 Didn't change the closing of the file, since dup2 will close the file for us.
Only close the file if there was an error.
4. Didn't change the tests, it never hurts to have many tests :-)

Regards


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-git-archive-Add-new-option-output-to-write-arch.patch --]
[-- Type: text/x-diff; name="0001-git-archive-Add-new-option-output-to-write-arch.patch", Size: 8893 bytes --]

From b68d40dca34d45e2535c50879cce62e3b24a2f30 Mon Sep 17 00:00:00 2001
From: Carlos Manuel Duclos Vergara <carlos.duclos@nokia.com>
Date: Mon, 16 Feb 2009 18:20:25 +0100
Subject: [PATCH] git-archive: Add new option "--output" to write archive to a file instead of stdout.

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.
---
 Documentation/git-archive.txt |    3 +
 archive.c                     |   23 ++++++
 t/t0024-crlf-archive.sh       |   19 +++++
 t/t5000-tar-tree.sh           |  148 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 193 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
index 41cbf9c..5bde197 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>::
+	Write the archive to <file> instead of stdout.
+
 <extra>::
 	This can be any options that the archiver backend understand.
 	See next section.
diff --git a/archive.c b/archive.c
index e6de039..e6af4ec 100644
--- a/archive.c
+++ b/archive.c
@@ -239,6 +239,23 @@ 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 = open(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
+	if (output_fd < 0)
+		die("could not create archive file: %s ", output_file);
+	if (output_fd != 1)
+		if (dup2(output_fd, 1) < 0) {
+			/*
+			 * dup2 closes output_fd on success, if something 
+			 * goes wrong we close output_fd here to avoid
+			 * problems.
+			 */
+			close(output_fd);
+			die("could not redirect output");
+		}
+}
+
 #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 +270,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 +280,8 @@ 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 archive to this file"),
 		OPT__VERBOSE(&verbose),
 		OPT__COMPR('0', &compression_level, "store only", 0),
 		OPT__COMPR('1', &compression_level, "compress faster", 1),
@@ -294,6 +314,9 @@ static int parse_archive_args(int argc, const char **argv,
 	if (!base)
 		base = "";
 
+	if (output)
+		create_output_file(output);
+
 	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

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

[Junio C Hamano]

<carlos.duclos@nokia.com> writes:

> 4. Didn't change the tests, it never hurts to have many tests :-)

It actually hurts me a lot.  I run every tests before pushing out the
integration results, and duplicated tests costs me a lot without adding
much value to the process.

> From b68d40dca34d45e2535c50879cce62e3b24a2f30 Mon Sep 17 00:00:00 2001
> From: Carlos Manuel Duclos Vergara <carlos.duclos@nokia.com>
> Date: Mon, 16 Feb 2009 18:20:25 +0100
> Subject: [PATCH] git-archive: Add new option "--output" to write archive to a file instead of stdout.

Very long line that you can be a bit creative to shorten.  E.g.

    git-archive: add --output=<file> to send output to a file instead of stdout

Because anything you are adding is new, people can tell that --word is an
option from the context, and people who are reading "git log" output know
that archive command traditionally writes to standard output.

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

Very long line I can wrap locally so this by itself is not a grave enough
offence to ask you to fix and resubmit, but I'm asking you to redo the
tests anyway, so please wrap this while at it.

Needs a sign-off.

> diff --git a/archive.c b/archive.c
> index e6de039..e6af4ec 100644
> --- a/archive.c
> +++ b/archive.c
> @@ -239,6 +239,23 @@ 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 = open(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
> +	if (output_fd < 0)
> +		die("could not create archive file: %s ", output_file);
> +	if (output_fd != 1)
> +		if (dup2(output_fd, 1) < 0) {
> +			/*
> +			 * dup2 closes output_fd on success, if something 
> +			 * goes wrong we close output_fd here to avoid
> +			 * problems.
> +			 */
> +			close(output_fd);
> +			die("could not redirect output");

The comment and close() are probably unnecessary, as you will die()
immediately.

> +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
> +
> +'

I agree with René that the test should just create the same archive twice,
with or without --output=, and compare the output.

To protect your patch from future changes that may do something funny to
only one side of the codepath, which is not even very likely knowing the
structure of the code, I'd recommend doing two tests, one for --format=tar
and another for --format=zip.

Also you do not have to introduce an inconsistent way to give option to
"tar" like the above (note that "tar xf" is the preferred traditional
notation, not "tar -xf", in this script).

Other than that, I think this is ready for 'next'.

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

[René Scharfe]

Junio C Hamano schrieb:
> <carlos.duclos@nokia.com> writes:
>> +static void create_output_file(const char *output_file)
>> +{
>> +	int output_fd = open(output_file, O_CREAT | O_WRONLY | O_TRUNC, 0666);
>> +	if (output_fd < 0)
>> +		die("could not create archive file: %s ", output_file);
>> +	if (output_fd != 1)
>> +		if (dup2(output_fd, 1) < 0) {
>> +			/*
>> +			 * dup2 closes output_fd on success, if something 
>> +			 * goes wrong we close output_fd here to avoid
>> +			 * problems.
>> +			 */
>> +			close(output_fd);
>> +			die("could not redirect output");
> 
> The comment and close() are probably unnecessary, as you will die()
> immediately.

dup2() closes the second file, not the first one (but not if it's the
same as the first one), so the comment is incorrect.  And I agree it's
OK to just die() without cleaning up, as this is a fatal error anyway
and an unlikely one on top of that.  How about something like this?

	if (dup2(output_fd, 1) < 0)
		die("could not redirect output");
	close(output_fd);

René