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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 1, 2016, 20:32 UTC
Message-ID: <xmqqpopsi61a.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160730094156.etvrzqbhcpg3is2z@x>

Josh Triplett <josh@joshtriplett.org> writes:

Show 19 quoted lines

> +static char *from;
>  static const char *signature = git_version_string;
>  static const char *signature_file;
>  static int config_cover_letter;
> @@ -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;
> +	}

One potential issue I see here is that if we ever plan to switch the default, we may want to issue a warning message to users who do not have any format.from configured when they do run the program on a commit that will get a different result before and after the switch in a release of Git before that default switch happens. The message would say something like "you are formatting somebody else's commit. the output will change in future versions of Git and show an explicit in-body From: header; if you want to keep the current behaviour, set format.from configuration variable to false".

But you cannot tell by looking at from that is NULL between two cases, it is NULL because we defaulted to it (in which case we do want to warn), or the user set it explicitly to false (we do not want to give the warning).

So "... makes it easier to change the default in the future." in the log message is a bit of exaggeration. The change in this patch is not there yet.

Show 14 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

I know you are only mimicking the style of the existing tests but the piping loses a potential error exit code from format-patch. I'd suggest leaving this as low-hanging fruit for later, not fixing them as part of this series.

This stops at the blank after the header, so there is no point doing master..side to format three patches. Just do "-1 side" instead?

More importantly, this only checks the From: in the header, which is just the half of what --from does. Shouldn't we be testing that the in-body From: is added as necessary?

Thanks.