From: Junio C Hamano <gitster@pobox.com>
Date: Mon, 16 Feb 2009 21:38:02 GMT
Subject: Re: [PATCH v2] git-archive: Add new option "--output" to write archive to a file instead of stdout.
Message-ID: <7v8wo6f4g5.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <4999C884.5050209@lsrfire.ath.cx>

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> carlos.duclos@nokia.com 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.

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

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

>> +       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();
	}                        

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

Style: if (output)