Re: [PATCH v2] git-archive: Add new option "--output" to write archive to a file instead of stdout.
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 16, 2009, 21:38 UTC
- Message-ID
- <7v8wo6f4g5.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <4999C884.5050209@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 6 quoted lines
> 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.
Show 11 quoted lines
>> 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.
Show 14 quoted lines
>> + 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();
} Show 5 quoted lines
>> @@ -294,6 +308,9 @@ static int parse_archive_args(int argc, const char **argv, >> if (!base) >> base = ""; >> >> + if(output)
Style: if (output)