Re: [PATCH] commit-tree: utilize parse-options api

[Brandon Richardson <brandon1024.br@gmail.com>]

Thank you all for the helpful comments :-)

On Wed, 27 Feb 2019 at 07:08, Duy Nguyen <pclouds@gmail.com> wrote:
> > It was discovered that the --no-gpg-sign option was documented
> > but not implemented in 55ca3f99, and the existing implementation
>
> Most people refer to a commit with this format
>
> 55ca3f99ae (commit-tree: add and document --no-gpg-sign - 2013-12-13)
>
> It gives the reader some context without actually looking at the
> commit in question. And in the event that 55ca3f99 is ambiguous, it's
> easier to find the correct one.

I didn't know this, thank you for the tip. I'll start doing this from now on.
I will also reread through the SubmittingPatches doc.

> > +static int parse_parent_arg_callback(const struct option *opt,
> > +               const char *arg, int unset)
> > +{
> > +       struct object_id oid;
> > +       struct commit_list **parents = opt->value;
> > +
> > +       BUG_ON_OPT_NEG(unset);
> > +
> > +       if (!arg)
> > +               return 1;
>
> This "return 1;" surprises me because I think we often just return 0
> or -1. I know !arg cannot happen here, so maybe just drop it. Or if
> you want t play absolutely safe, maybe add a new macro like
>
> BUG_ON_NO_ARG(arg);
>
> which conveys the intention much better.

I like the BUG_ON_NO_ARG approach. I will go that route.

> > +static int parse_file_arg_callback(const struct option *opt,
> > +               const char *arg, int unset)
>
> I would suggest you do the same for -F, i.e. collect a string list of
> paths then do the heavy lifting afterwards _IF_ we don't support
> mixing -m and -F. If we do, then we have to handle both in callbacks
> to make sure we compose the message correctly.

I opted to use callbacks here to allow mixing -m and -F so that messages
are composed correctly, as you mentioned. I did so in an attempt to match
the existing functionality of commit-tree.

>
> > +               OPT_END()
> > +    };
>
> I think you're using spaces here to indent instead of TABs.

Good eye on the whitespace issue. I'm still dialling in my environment,
so please forgive me.

I will address all comments in a v2. Thanks again.

Brandon