Re: [PATCH 1/2] format-patch: Add a config option format.from to set the default for --from
- From
Jeff King <peff@peff.net>
- Date
- Jul 30, 2016, 15:40 UTC
- Message-ID
- <20160730154034.thjrfu6zeprzwvxb@sigill.intra.peff.net>
- In-Reply-To
- <20160730094156.etvrzqbhcpg3is2z@x>
On Sat, Jul 30, 2016 at 02:41:56AM -0700, Josh Triplett wrote:
Show 15 quoted lines
> @@ -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.
Show 15 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?
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.
-Peff