Re: [GSoC RFC PATCH v2 3/7] repo-info: add plaintext as an output format

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

Lucas Seiki Oshiro <lucasseikioshiro@gmail.com> writes:

> Add 'plaintext' as an output format of repo-info. This output format is
> composed zero or more key=value pairs, one per line.
>
> Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> Mentored-by Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> ---
>  builtin/repo-info.c  | 12 +++++++++++-
>  t/t1900-repo-info.sh |  4 ++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/repo-info.c b/builtin/repo-info.c
> index cbe1475e30..cd7c110f47 100644
> --- a/builtin/repo-info.c
> +++ b/builtin/repo-info.c
> @@ -3,7 +3,8 @@
>  #include "parse-options.h"
>  
>  enum output_format {
> -	FORMAT_JSON
> +	FORMAT_JSON,
> +	FORMAT_PLAINTEXT
>  };

Give a trailing comma after "_PLAINTEXT".

>  struct repo_info {
> @@ -19,10 +20,16 @@ static void repo_info_init(struct repo_info *repo_info,
>  
>  	if (format == NULL || !strcmp(format, "json"))
>  		repo_info->format = FORMAT_JSON;

It is somewhat strange for this helper function deciding/hardcoding
what the default format is.  Would it make it easier to understand
if you lost "if given NULL, use JSON" from here, and instead
initialize the local variable format to "json" in cmd_repo_info()?

> +	else if (!strcmp(format, "plaintext"))
> +		repo_info->format = FORMAT_PLAINTEXT;
>  	else
>  		die("invalid format %s", format);
>  }
>  
> +static void repo_info_print_plaintext(struct repo_info *repo_info UNUSED)
> +{
> +}
> +
>  static void repo_info_print_json(struct repo_info *repo_info UNUSED)
>  {
>  	struct json_writer jw;
> @@ -44,6 +51,9 @@ static void repo_info_print(struct repo_info *repo_info)
>  	case FORMAT_JSON:
>  		repo_info_print_json(repo_info);
>  		break;
> +	case FORMAT_PLAINTEXT:
> +		repo_info_print_plaintext(repo_info);
> +		break;
>  	}
>  }
>  
> diff --git a/t/t1900-repo-info.sh b/t/t1900-repo-info.sh
> index f634e1a285..998c835795 100755
> --- a/t/t1900-repo-info.sh
> +++ b/t/t1900-repo-info.sh
> @@ -18,5 +18,9 @@ test_expect_success PERLJSON 'json: returns empty output with allow-empty' '
>  	git repo-info --format=json >output &&
>  	test_line_count = 2 output
>  '
> +test_expect_success 'plaintext: returns empty output with allow-empty' '

allow-empty?

> +	git repo-info --format=plaintext >output &&
> +	test_line_count = 0 output
> +'

In any case, I do not know if it makes sense to add this test before
you have a single line of repo_info_print_plaintext() implemented.