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

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