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

[Junio C Hamano <gitster@pobox.com>]

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