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

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

[off-list ref] 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 <redacted>
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.

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

Junio C Hamano schrieb:

[off-list ref] writes:

quoted

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