Lucas Seiki Oshiro <lucasseikioshiro@xxxxxxxxx> writes:
> Add the --format flag to the repo-info command, allowing the user to
> choose betwen the 'json' and 'plaintext' formats as output.
>
s/betwen/between
> Also add a flag --allow-empty, which will force the output data to be
> empty when no field is requested.
>
Why do you suppose we need this, I'm not against it, but it would be
nice to state why this is necessary. The idea is to have a default
output when a user runs `git repo-info`, so I'm missing why this would
be useful.
> Mentored-by: Karthik Nayak <karthik.188@xxxxxxxxx>
> Mentored-by Patrick Steinhardt <ps@xxxxxx>
> Signed-off-by: Lucas Seiki Oshiro <lucasseikioshiro@xxxxxxxxx>
> ---
> builtin/repo-info.c | 79 ++++++++++++++++++++++++++++++++++++++++++--
> t/meson.build | 1 +
> t/t1518-repo-info.sh | 49 +++++++++++++++++++++++++++
> 3 files changed, 127 insertions(+), 2 deletions(-)
> create mode 100755 t/t1518-repo-info.sh
>
> diff --git a/builtin/repo-info.c b/builtin/repo-info.c
> index 4615b988d8..4d539a17fb 100644
> --- a/builtin/repo-info.c
> +++ b/builtin/repo-info.c
> @@ -1,22 +1,97 @@
> #include "builtin.h"
> +#include "hash.h"
This header doesn't seem to be required for compiling this patch.
> +#include "json-writer.h"
> #include "parse-options.h"
> +#include "refs.h"
Similar, this header too doesn't seem to be required.
> +
> +enum output_format {
> + FORMAT_PLAINTEXT,
> + FORMAT_JSON
> +};
> +
> +struct repo_info {
> + struct repository *repo;
> + enum output_format format;
> +};
> +
> +static void repo_info_init(struct repo_info *repo_info,
> + struct repository *repo,
> + char *format,
> + int allow_empty UNUSED,
So allow_empty isn't even used in this patch, let's separate it out to a
patch of itself then.
> + int argc UNUSED,
> + const char **argv UNUSED
> + ) {
> + repo_info->repo = repo;
> +
> + if (format == NULL || !strcmp(format, "json"))
> + repo_info->format = FORMAT_JSON;
> + 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) {
> +}
Since we don't implement this in this patch, perhaps we can only
introduce the json formatting in this patch? That also allows us to
completely skip the 'plaintext' formatting for now and implement it in
the future.
> +static void repo_info_print_json(struct repo_info *repo_info UNUSED)
> +{
> + struct json_writer jw;
> +
> + jw_init(&jw);
> +
> + jw_object_begin(&jw, 1);
> + jw_end(&jw);
> +
> + puts(jw.json.buf);
> + jw_release(&jw);
> +}
> +
> +static void repo_info_print(struct repo_info *repo_info)
> +{
> + enum output_format format = repo_info->format;
> +
> + switch (format) {
> + case FORMAT_PLAINTEXT:
> + repo_info_print_plaintext(repo_info);
> + break;
> + case FORMAT_JSON:
> + repo_info_print_json(repo_info);
> + break;
> + }
> +}
>
> int cmd_repo_info(
> int argc,
> const char **argv,
> const char *prefix,
> - struct repository *repo UNUSED
> + struct repository *repo
> )
> {
> const char *const repo_info_usage[] = {
> "git repo-info",
> NULL
> };
> + struct repo_info repo_info;
> + char *format = NULL;
> + int allow_empty = 0;
> struct option options[] = {
> + OPT_STRING(0,
> + "format",
> + &format,
> + N_("format"),
> + N_("output format")),
> + OPT_BOOL(0,
> + "allow-empty",
> + &allow_empty,
> + "when set, it will use an empty set of fields if no field is requested"),
> OPT_END()
> };
>
> - argc = parse_options(argc, argv, prefix, options, repo_info_usage, 0);
> + argc = parse_options(argc, argv, prefix, options, repo_info_usage,
> + PARSE_OPT_KEEP_UNKNOWN_OPT);
I think I understand why we use 'PARSE_OPT_KEEP_UNKNOWN_OPT', this is to
allow users to provide custom fields to print. But this doesn't seem to
be mentioned in the commit message or used in this patch, could we do
either?
[snip]
> diff --git a/t/t1518-repo-info.sh b/t/t1518-repo-info.sh
> new file mode 100755
> index 0000000000..2e1a6f0c34
> --- /dev/null
> +++ b/t/t1518-repo-info.sh
> @@ -0,0 +1,49 @@
> +#!/bin/sh
> +
> +test_description='test git repo-info'
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +
> +parse_json () {
> + tr '\n' ' ' | "$PERL_PATH" "$TEST_DIRECTORY/t0019/parse_json.perl"
> +}
> +
If I recall correctly, in our previous internal review, it was mentioned
that since we use perl here, we should add a PERL pre-requisite. Similar
to the one in 't/t0019-json-writer.sh'.
> +test_repo_info () {
Nit: would be nice to have some documentation on this function,
regarding:
1. Its purpose and use case
2. The fields and what they mean
You can take some hints from 'test_migration' in
't/t1460-refs-migrate.sh'.
> + label=$1
> + init_args=$2
> + key=$3
> + expected_value=$4
> +
> + test_expect_success "json: $label" "
> + test_when_finished 'rm -rf repo' &&
> + git init $init_args repo &&
> + cd repo &&
> + echo '$expected_value' >expect &&
> + git repo-info '$key'| parse_json >output &&
> + grep -F 'row[0].$key' output | cut -d ' ' -f 2 >actual &&
> + test_cmp expect actual
> + "
> +
> + test_expect_success "plaintext: $label" "
> + test_when_finished 'rm -rf repo' &&
> + git init $init_args repo &&
> + cd repo &&
> + echo '$expected_value' >expect &&
> + git repo-info --format=plaintext '$key' >actual &&
> + test_cmp expect actual
> + "
> +}
> +
> +test_expect_success 'json: returns empty output with allow-empty' '
> + git repo-info --allow-empty --format=json >output &&
> + test_line_count = 2 output
> +'
> +
As of this patch,
$ ~/code/git/build/bin-wrappers/git repo-info
{
}
$ ~/code/git/build/bin-wrappers/git repo-info --allow-empty
{
}
So what differentiates the former from the latter?
> +test_expect_success 'plaintext: returns empty output with allow-empty' '
> + git repo-info --allow-empty --format=plaintext >output &&
> + test_line_count = 0 output
> +'
>
This is because we didn't implement plainttext no?
> +
> +test_done
> --
> 2.39.5 (Apple Git-154)
Overall, I think the changes here make sense to me. But the commit
can be broken up and divided into individual bits:
1. Commit to introduce the --format=json flag
2. Commit to introduce the --format=plaintext flag (can be done later as
needed)
3. Commit to add '--allow-empty' flag (is this needed?).
This way the tests and code can also be split as per these commits, this
would make reviews much easier since the current patch has some dead
code which presumably will be used in upcoming patches.
- Karthik
Attachment:
signature.asc
Description: PGP signature
