Re: [PATCH 1/2] format-patch: Add a config option format.from to set the default for --from

[Josh Triplett <josh@joshtriplett.org>]

On Sat, Jul 30, 2016 at 11:40:34AM -0400, Jeff King wrote:
> On Sat, Jul 30, 2016 at 02:41:56AM -0700, Josh Triplett wrote:
> 
> > @@ -807,6 +808,17 @@ static int git_format_config(const char *var, const char *value, void *cb)
> >  		base_auto = git_config_bool(var, value);
> >  		return 0;
> >  	}
> > +	if (!strcmp(var, "format.from")) {
> > +		int b = git_config_maybe_bool(var, value);
> > +		free(from);
> > +		if (b < 0)
> > +			from = xstrdup(value);
> > +		else if (b)
> > +			from = xstrdup(git_committer_info(IDENT_NO_DATE));
> > +		else
> > +			from = NULL;
> > +		return 0;
> > +	}
> 
> This "free old, then handle tri-state" mirrors the code in the parseopt
> callback pretty closely. I wonder if they could share the logic (it is
> not many lines, but we would want the logic to stay identical). I
> suspect the helper function would end up with more boilerplate than it's
> worth, though, trying to handle the unset and default cases.

I looked at trying to share that code for exactly that reason, but
didn't find a convenient way to share the two, because from_callback
checked two separate variables (unset and arg), while the logic above
checks one.  So, while the *bodies* of the three-way if are duplicated,
the *conditions* aren't.

However, if you'd like to avoid the duplication between the three
values, I can do that with a set_from function that takes an enum and a
new value; it'll actually increase lines of code, but remove the
duplication (as well as the second patch's third copy of setting from to
the committer info).

> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index 1206c48..b0579dd 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -229,6 +229,46 @@ check_patch () {
> >  	grep -e "^Subject:" "$1"
> >  }
> >  
> > +test_expect_success 'format.from=false' '
> > +
> > +	git -c format.from=false format-patch --stdout master..side |
> > +	sed -e "/^\$/q" >patch &&
> > +	check_patch patch &&
> > +	! grep "^From: C O Mitter <committer@example.com>\$" patch
> > +'
> 
> These tests follow a different style from the "--from" tests later in
> the script (and your second patch does follow it, and puts its test
> close there). Any reason not to have all of the "from" tests together,
> and using the same style?

The tests covered different things.  The later --from tests made sure
that --from behaved as expected.  These tests made sure that format.from
and --from/--no-from interacted in the expected way, with the
command-line options overriding the configuration.  So, I put them next
to the tests for other options like format.to and format.cc, which
tested the same thing (overriding those with --no-to, --no-cc, etc).

> Overall, the whole thing looks cleanly done, and I don't mind it going
> in as-is. These are just two things I noticed while reading it over.

I'll send a v2 with the code duplication fixed.

- Josh Triplett