Thank you all for the helpful comments :-)
On Wed, 27 Feb 2019 at 07:08, Duy Nguyen <[email protected]> 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
