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@xxxxxxxxxxx>\$" 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html