Re: [PATCH v7 4/5] format-patch: add commitListFormat config

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

Phillip Wood <phillip.wood123@gmail.com> writes:

>> Possible values:
>>    - commitListFormat is set but no string is passed: it will default to
>>      "[%(count)/%(total)] %s"
>
> It is unusual for an empty config value to mean something different from 
> it not being set. The reason for this is that it allows
>
>      git -c config.key some-command
>
> to act as though config.key was not set.

That syntax is the same as setting config.key=true; disabling the
feature triggered by config.key is quite counter-intuitive, isn't
it?

We are by default using "shortlog", but use of this configuration
variable is a sign that the user wants to use a more modern custom
format that is not the traditional "shortlog".  It would be quite
natural to invoke the modern default by setting it to "true" (i.e.,
"I want to enable the new format.commitlistformat feature, but I am
not saying which format, and the "log:[%(count)/%(total)] %s" format
is used).

Perhaps "format.commitlistformat = false" should disable the modern
format and fall back to "shortlog", setting it to true (including
the use of "valueless true" syntax) should enable it and use the
modern default "log:[%c/%t] %s" format, and non-bool text should be
used as a custom specification ("shortlog", or "log:<format>")?

I.e.

	switch (git_parse_maybe_bool_text(value)) {
        case 0: /* false */
		fmt_cover_letter_commit_list = "shortlog";
		break;
	case 1: /* true - use the modern default format */
		fmt_cover_letter_commit_list = "log:[%c/%t] %s";
		break;		
	default:
		fmt_cover_letter_commit_list = value;
		break;
	}

Hmm?


> It would be nice to support a default format on the commandline as well.


>
>>    - if a string is passed: will use it as a format spec. Note that this
>>      is either "shortlog" or a format spec prefixed by "log:"
>>      e.g."log:%s (%an)"
>
> Having the config value behave like --cover-letter-format=<value> is 
> sensible
>
>>    - if commitListFormat is not set: it will default to the shortlog
>>      format.
>
> makes sense
>
> Thanks
>
> Phillip
>
>> Signed-off-by: Mirko Faina <mroik@delayed.space>
>> ---
>>   builtin/log.c           | 21 ++++++++++++++++
>>   t/t4014-format-patch.sh | 53 +++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 74 insertions(+)
>> 
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 95e5d9755f..5fec0ddaf9 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -886,6 +886,7 @@ struct format_config {
>>   	char *signature;
>>   	char *signature_file;
>>   	enum cover_setting config_cover_letter;
>> +	char *fmt_cover_letter_commit_list;
>>   	char *config_output_directory;
>>   	enum cover_from_description cover_from_description_mode;
>>   	int show_notes;
>> @@ -930,6 +931,7 @@ static void format_config_release(struct format_config *cfg)
>>   	string_list_clear(&cfg->extra_cc, 0);
>>   	strbuf_release(&cfg->sprefix);
>>   	free(cfg->fmt_patch_suffix);
>> +	free(cfg->fmt_cover_letter_commit_list);
>>   }
>>   
>>   static enum cover_from_description parse_cover_from_description(const char *arg)
>> @@ -1052,6 +1054,19 @@ static int git_format_config(const char *var, const char *value,
>>   		cfg->config_cover_letter = git_config_bool(var, value) ? COVER_ON : COVER_OFF;
>>   		return 0;
>>   	}
>> +	if (!strcmp(var, "format.commitlistformat")) {
>> +		struct strbuf tmp = STRBUF_INIT;
>> +		strbuf_init(&tmp, 0);
>> +		if (value)
>> +			strbuf_addstr(&tmp, value);
>> +		else
>> +			strbuf_addstr(&tmp, "log:[%(count)/%(total)] %s");
>> +
>> +		FREE_AND_NULL(cfg->fmt_cover_letter_commit_list);
>> +		git_config_string(&cfg->fmt_cover_letter_commit_list, var, tmp.buf);
>
>
>
>> +		strbuf_release(&tmp);
>> +		return 0;
>> +	}
>>   	if (!strcmp(var, "format.outputdirectory")) {
>>   		FREE_AND_NULL(cfg->config_output_directory);
>>   		return git_config_string(&cfg->config_output_directory, var, value);
>> @@ -2329,6 +2344,12 @@ int cmd_format_patch(int argc,
>>   		goto done;
>>   	total = list.nr;
>>   
>> +	if (!cover_letter_fmt) {
>> +		cover_letter_fmt = cfg.fmt_cover_letter_commit_list;
>> +		if (!cover_letter_fmt)
>> +			cover_letter_fmt = "shortlog";
>> +	}
>> +
>>   	if (cover_letter == -1) {
>>   		if (cfg.config_cover_letter == COVER_AUTO)
>>   			cover_letter = (total > 1);
>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>> index 458da80721..4891389a53 100755
>> --- a/t/t4014-format-patch.sh
>> +++ b/t/t4014-format-patch.sh
>> @@ -428,6 +428,59 @@ test_expect_success 'cover letter no format' '
>>   	test_line_count = 1 result
>>   '
>>   
>> +test_expect_success 'cover letter config with count, subject and author' '
>> +	test_when_finished "rm -rf patches result" &&
>> +	test_when_finished "git config unset format.coverletter" &&
>> +	test_when_finished "git config unset format.commitlistformat" &&
>> +	git config set format.coverletter true &&
>> +	git config set format.commitlistformat "log:[%(count)/%(total)] %s (%an)" &&
>> +	git format-patch -o patches HEAD~2 &&
>> +	grep -E "^[[[:digit:]]+/[[:digit:]]+] .* \(A U Thor\)" patches/0000-cover-letter.patch >result &&
>> +	test_line_count = 2 result
>> +'
>> +
>> +test_expect_success 'cover letter config with count and author' '
>> +	test_when_finished "rm -rf patches result" &&
>> +	test_when_finished "git config unset format.coverletter" &&
>> +	test_when_finished "git config unset format.commitlistformat" &&
>> +	git config set format.coverletter true &&
>> +	git config set format.commitlistformat "log:[%(count)/%(total)] (%an)" &&
>> +	git format-patch -o patches HEAD~2 &&
>> +	grep -E "^[[[:digit:]]+/[[:digit:]]+] \(A U Thor\)" patches/0000-cover-letter.patch >result &&
>> +	test_line_count = 2 result
>> +'
>> +
>> +test_expect_success 'cover letter config commitlistformat set but no format' '
>> +	test_when_finished "rm -rf patches result" &&
>> +	test_when_finished "git config unset format.coverletter" &&
>> +	test_when_finished "git config unset format.commitlistformat" &&
>> +	git config set format.coverletter true &&
>> +	printf "\tcommitlistformat" >> .git/config &&
>> +	git format-patch -o patches HEAD~2 &&
>> +	grep -E "^[[[:digit:]]+/[[:digit:]]+] .*" patches/0000-cover-letter.patch >result &&
>> +	test_line_count = 2 result
>> +'
>> +
>> +test_expect_success 'cover letter config commitlistformat set to shortlog' '
>> +	test_when_finished "rm -rf patches result" &&
>> +	test_when_finished "git config unset format.coverletter" &&
>> +	test_when_finished "git config unset format.commitlistformat" &&
>> +	git config set format.coverletter true &&
>> +	git config set format.commitlistformat shortlog &&
>> +	git format-patch -o patches HEAD~2 &&
>> +	grep -E "^A U Thor \([[:digit:]]+\)" patches/0000-cover-letter.patch >result &&
>> +	test_line_count = 1 result
>> +'
>> +
>> +test_expect_success 'cover letter config commitlistformat not set' '
>> +	test_when_finished "rm -rf patches result" &&
>> +	test_when_finished "git config unset format.coverletter" &&
>> +	git config set format.coverletter true &&
>> +	git format-patch -o patches HEAD~2 &&
>> +	grep -E "^A U Thor \([[:digit:]]+\)" patches/0000-cover-letter.patch >result &&
>> +	test_line_count = 1 result
>> +'
>> +
>>   test_expect_success 'reroll count' '
>>   	rm -fr patches &&
>>   	git format-patch -o patches --cover-letter --reroll-count 4 main..side >list &&