From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Mon, 01 Aug 2011 17:37:22 GMT
Subject: Re: [PATCH 10/18] revert: Don't create invalid replay_opts in parse_args
Message-ID: <CALkWK0n2j_jFzkwF_QrVUejchnh=zEbUkGxH1_EyhjjZLj3FPA@mail.gmail.com>
In-Reply-To: <201107311431.26187.chriscool@tuxfamily.org>

Hi Christian,

Christian Couder writes:
> On Wednesday 27 July 2011 05:19:07 Ramkumar Ramachandra wrote:
>> +static void verify_opt_compatible(const char *me, const char *base_opt,
>> ...) +{
>> +     const char *this_opt;
>> +     va_list ap;
>> +     int set;
>> +
>> +     va_start(ap, base_opt);
>> +     while ((this_opt = va_arg(ap, const char *))) {
>> +             set = va_arg(ap, int);
>> +             if (set)
>> +                     die(_("%s: %s cannot be used with %s"),
>> +                             me, this_opt, base_opt);
>> +     }
>> +     va_end(ap);
>> +}
>
> Question: returning in the middle of va_start() - va_end() may not be ok with
> some compilers, but I don't know how safe it is to exit()?

Interesting observation.  Even if it's not a problem, I suppose
there's no harm in putting a va_end before the die() statement --
Valgrind will probably be happier anyway.

>> +     /*
>> +      * Sequence of picks finished successfully; cleanup by
>> +      * removing the .git/sequencer directory
>> +      */
>> +     strbuf_reset(&buf);
>> +     strbuf_addf(&buf, "%s", git_path(SEQ_DIR));
>> +     remove_dir_recursively(&buf, 0);
>>       return 0;
>>  }
>
> The "strbuf_reset(&buf)" is not needed. But a "strbuf_release(&buf)" could be
> added before the return.

Right, thanks.  Fixed.

-- Ram