On Wed, Aug 30, 2023 at 12:28:15PM -0700, Junio C Hamano wrote:
> Will queue. Let's wait to see if others find something fishy for a
> day or two and then merge it down to 'next'.
It looks good to me, and I'm much happier with where the refactoring
ended up compared to the earlier versions. I did have two nits, but I'm
content if neither is addressed.
One is that the commit message doesn't really describe the refactoring
of --subject-prefix. I'm OK with that rationale being in the list
archive, though.
> > static int subject_prefix_callback(const struct option *opt, const char *arg,
> > int unset)
> > {
> > + struct strbuf *sprefix;
> > +
> > BUG_ON_OPT_NEG(unset);
> > + sprefix = (struct strbuf *)opt->value;
> > subject_prefix = 1;
> > - ((struct rev_info *)opt->value)->subject_prefix = arg;
> > + strbuf_reset(sprefix);
> > + strbuf_addstr(sprefix, arg);
> > return 0;
> > }
>
> OK.
The cast is unnecessary here, since opt->value is a void pointer which
allows implicit casts. Just:
struct strbuf *sprefix = opt->value;
is IMHO a little more readable. But as we're just passing it along to
strbuf functions anyway, it would also work to do:
strbuf_reset(opt->value);
strbuf_addstr(opt->value, arg);
I think we're deep into questions of style / preference here, so I'm OK
with any of them. It's probably only that I've recently been refactoring
so many parseopt callbacks with the same pattern that I have opinions at
all. ;)
-Peff
