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

[Jeff King <peff@peff.net>]

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.

> 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