Re: [PATCH 1/2] format-patch: Add a config option format.from to set the default for --from
- From
Josh Triplett <josh@joshtriplett.org>
- Date
- Jul 30, 2016, 18:12 UTC
- Message-ID
- <20160730181246.4aifnvqfeenddzdl@x>
- In-Reply-To
- <20160730154034.thjrfu6zeprzwvxb@sigill.intra.peff.net>
On Sat, Jul 30, 2016 at 11:40:34AM -0400, Jeff King wrote:
Show 23 quoted lines
> 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).
Show 20 quoted lines
> > 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