Hi Paul, On 2015-05-21 11:48, Paul Tan wrote: > > On Tue, May 19, 2015 at 1:41 AM, Johannes Schindelin > <johannes.schindelin@xxxxxx> wrote: >> On 2015-05-18 17:05, Paul Tan wrote: >>> diff --git a/builtin/pull.c b/builtin/pull.c >>> index 0b771b9..a4d9c92 100644 >>> --- a/builtin/pull.c >>> +++ b/builtin/pull.c >>> @@ -11,16 +11,64 @@ >>> #include "argv-array.h" >>> #include "run-command.h" >>> >>> +/** >>> + * Given an option opt, where opt->value points to a char* and opt->defval is a >>> + * string, sets opt->value to the evaluation of "--$defval=$arg". If `arg` is >>> + * NULL, then opt->value is set to "--$defval". If unset is true, then >>> + * opt->value is set to "--no-$defval". >>> + */ >>> +static int parse_opt_passthru(const struct option *opt, const char >>> *arg, int unset) >> >> Implied by my suggested description, I also propose to put the re-recreated command-line option into a strbuf instead of a char *, to make memory management easier (read: avoid memory leaks). > > Unfortunately, the usage of strbuf means that we lose the ability to > know if an option was not provided at all (the value is NULL). This is > important as some of these options are used to override the default > configured behavior. Would this not be implied by the strbuf's len being 0? >> You might also want to verify that arg is `NULL` when `unset != 0`. Something like this: > > Hmm, would there be a situation where arg is NULL when `unset != 0`? I forgot to say that my suggestion was purely meant as defensive coding. Yes, `arg` *should* be `NULL` when `unset != 0`. Should ;-) By the way, just to make sure: My comments and suggestions are no demands; you should feel very free to reject them when you feel that your code is better without the suggested changes. I am just trying to be helpful. Ciao, Dscho -- 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