From: Josh Triplett <josh@joshtriplett.org>
Date: Sun, 07 Aug 2016 22:57:01 GMT
Subject: Re: [PATCH v2 1/2] format-patch: Add a config option format.from to set the default for --from
Message-ID: <20160807225701.ucv2xunq5vs4uedk@x>
In-Reply-To: <20160801173847.qph2tora75h6ebsk@sigill.intra.peff.net>

On Mon, Aug 01, 2016 at 01:38:47PM -0400, Jeff King wrote:
> On Sat, Jul 30, 2016 at 12:11:11PM -0700, Josh Triplett wrote:
> 
> > +enum from {
> > +	FROM_AUTHOR,
> > +	FROM_USER,
> > +	FROM_VALUE,
> > +};
> > +
> > +static void set_from(enum from type, const char *value)
> > +{
> > +	free(from);
> > +	switch (type) {
> > +	case FROM_AUTHOR:
> > +		from = NULL;
> > +		break;
> > +	case FROM_USER:
> > +		from = xstrdup(git_committer_info(IDENT_NO_DATE));
> > +		break;
> > +	case FROM_VALUE:
> > +		from = xstrdup(value);
> > +		break;
> > +	}
> > +}
> 
> Thanks for looking into reducing the duplication. TBH, I am not sure it
> is really an improvement, just because of the amount of boilerplate (and
> this function interface is kind of weird, because of the rules for when
> "value" should or should not be NULL).
> 
> I guess another way to do it would be:
> 
>   #define FROM_AUTO_IDENT ((const char *)(intptr_t)1))
>   void set_from(const char *value)
>   {
> 	if (value == FROM_AUTO_IDENT)
> 		value = git_committer_info(IDENT_NO_DATE);
> 	free(from);
> 	from = xstrdup_or_null(value);
>   }

I'd actually seriously considered this exact approach, which I preferred
as well, but I'd discarded it because I figured it'd get rejected.
Given your suggestion, and Junio's comment, I'll go with this version.

- Josh Triplett