René Scharfe <rene.scharfe@xxxxxxxxxxxxxx> writes:
> carlos.duclos@xxxxxxxxx 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)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
