Re: [GSOC PATCH 2/2] builtin/fmt-merge-msg: stop depending on 'the_repository'

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

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

> Refactor builtin/fmt-merge-msg.c to remove the dependancy on the global
> 'the_repository'. Replace all the occurrences of 'the_repository' with
> 'repo', where 'repo' is a pointer to 'struct repository' passed to the
> function 'cmd_fmt_merge_msg()' and thus remove the definition '#define
> USE_THE_REPOSITORY_VARIABLE'. Also, add a test to make sure that "git
> fmt-merge-msg -h" can be called outside a repository.

This also moves the call to git_config()/repo_config() after
parse_options().

It generally is a bad idea to read command line options first and
then read the configuration (it is a bug if such a flow causes
values from configuration to overwrite values from command line).
THe current set of options and configuration variables may not
overlap, in which case such a questionable arrangement happen to be
without bug right now, but it would prevent future developers from
adding new options and configuration variables and make them
interact with each other in the most natural way.

In any case, the reason for this change of the order between config
and parse-options is not explained at all in the proposed log
message.

> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---
>  builtin/fmt-merge-msg.c | 7 +++----
>  t/t1517-outside-repo.sh | 7 +++++++
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
> index fed8163825..848498b8e6 100644
> --- a/builtin/fmt-merge-msg.c
> +++ b/builtin/fmt-merge-msg.c
> @@ -1,4 +1,3 @@
> -#define USE_THE_REPOSITORY_VARIABLE
>  #include "builtin.h"
>  #include "config.h"
>  #include "fmt-merge-msg.h"
> @@ -13,7 +12,7 @@ static const char * const fmt_merge_msg_usage[] = {
>  int cmd_fmt_merge_msg(int argc,
>  		      const char **argv,
>  		      const char *prefix,
> -		      struct repository *repo UNUSED)
> +		      struct repository *repo)
>  {
>  	char *inpath = NULL;
>  	const char *message = NULL;
> @@ -53,13 +52,13 @@ int cmd_fmt_merge_msg(int argc,
>  	int ret;
>  	struct fmt_merge_msg_opts opts;
>  
> -	git_config(fmt_merge_msg_config, NULL);
>  	argc = parse_options(argc, argv, prefix, options, fmt_merge_msg_usage,
>  			     0);
>  	if (argc > 0)
>  		usage_with_options(fmt_merge_msg_usage, options);
> +	repo_config(repo, fmt_merge_msg_config, NULL);
>  
> -	adjust_shortlog_len(the_repository, &shortlog_len);
> +	adjust_shortlog_len(repo, &shortlog_len);
>  
>  	if (inpath && strcmp(inpath, "-")) {
>  		in = fopen(inpath, "r");
> diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
> index 8f59b867f2..4b4e645860 100755
> --- a/t/t1517-outside-repo.sh
> +++ b/t/t1517-outside-repo.sh
> @@ -121,4 +121,11 @@ test_expect_success 'prune does not crash with -h' '
>  	test_grep "[Uu]sage: git prune " usage
>  '
>  
> +test_expect_success 'fmt-merge-msg does not crash with -h' '
> +	test_expect_code 129 git fmt-merge-msg -h >usage &&
> +	test_grep "[Uu]sage: git fmt-merge-msg " usage &&
> +	test_expect_code 129 nongit git fmt-merge-msg -h >usage &&
> +	test_grep "[Uu]sage: git fmt-merge-msg " usage
> +'
> +
>  test_done