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

[Junio C Hamano <gitster@pobox.com>]

Josh Triplett <josh@joshtriplett.org> writes:

> +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.

> 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.