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

René Scharfe [off-list ref] writes:

carlos.duclos@nokia.com schrieb:

quoted

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.

Thanks.  I agree with everything you said in your review, and adding a bit
more.

quoted

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);

"git grep -n -w -e 'creat' -- '*.c'" shows nothing; we seem to prefer
using a longhand:

	open(path, O_CREAT | O_WRONLY | O_TRUNC, 0666);

instead.  Personally I see nothing wrong in creat(2) per-se, but let's be
consistent.

quoted

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

If the user is sick enough to close fd#1 from the shell when running
archive, it could already be pointing at fd#1 ;-)

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

Yikes.

The logic would become:

	fd = creat();
        if (fd < 0)
        	die();
	if (fd != 1) {
        	if (dup2(fd, 1) < 0 || close(fd))
	        	die();
	}                        

quoted

@@ -294,6 +308,9 @@ static int parse_archive_args(int argc, const char **argv,
        if (!base)
                base = "";

+       if(output)

Style: if (output)