Re: [PATCH v4] format-patch: --rfc honors what --subject-prefix sets

From: Jeff King <peff@peff.net>
Date: Aug 31, 2023, 21:29 UTC
Message-ID: <20230831212950.GA949706@coredump.intra.peff.net>
In-Reply-To: <xmqqsf808h4g.fsf@gitster.g>

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.

Show 15 quoted lines

> >  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