Re: [GSoC RFC PATCH v4 2/4] repo: add the field references.format

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7296 bytes --]

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

> This commit is part of the series that introduce the new subcommand
> git-repo-info.
>
> The flag `--show-ref-format` from git-rev-parse is used for retrieving
> the reference format (i.e. `files` or `reftable`). This way, it is
> used for querying repository metadata, fitting in the purpose of
> git-repo-info.
>
> Then, add a new field `references.format` to the repo-info subcommand
> containing that information.
>

Nit: s/Then, add/Add

>
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Justin Tobler <jltobler@gmail.com>
> Mentored-by: Karthik Nayak <karthik.188@gmail.com>
> Mentored-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com>
> ---
>  Documentation/git-repo.adoc |  4 ++
>  builtin/repo.c              | 92 +++++++++++++++++++++++++++++++++++--
>  t/meson.build               |  1 +
>  t/t1900-repo.sh             | 47 +++++++++++++++++++
>  4 files changed, 140 insertions(+), 4 deletions(-)
>  create mode 100755 t/t1900-repo.sh
>
> diff --git a/Documentation/git-repo.adoc b/Documentation/git-repo.adoc
> index 6f8fe3f6ea..b7af6f45a4 100644
> --- a/Documentation/git-repo.adoc
> +++ b/Documentation/git-repo.adoc
> @@ -45,6 +45,10 @@ INFO KEYS
>  The set of data that `git repo` can return is grouped into the following
>  categories:
>
> +`references`::
> +Reference-related data:
> +* `format`: the reference storage format, either `files` or `reftable`.
> +

Nit: I would omit the '`files` or `reftable`' here, because while this
is currently true. This might not hold up in the future. So better to
not go into the details of the supported systems.

>  SEE ALSO
>  --------
>  linkgit:git-rev-parse[1]
> diff --git a/builtin/repo.c b/builtin/repo.c
> index a1787a3cc5..dcda0d6d61 100644
> --- a/builtin/repo.c
> +++ b/builtin/repo.c
> @@ -1,11 +1,95 @@
>  #include "builtin.h"
>  #include "parse-options.h"
> +#include "strbuf.h"
> +#include "refs.h"
>
> -static int repo_info(int argc UNUSED,
> -		     const char **argv UNUSED,
> +typedef void add_field_fn(struct strbuf *buf, struct repository *repo);
> +
> +struct field {
> +	const char *key;
> +	add_field_fn *add_field_callback;
> +};
> +
> +static void add_string(struct strbuf *buf,
> +		       const char *key, const char *value)
> +{
> +	strbuf_addf(buf, "%s\n%s%c", key, value, '\0');
> +}
> +

I like the table design used here, makes things much simpler. I do think
that each field shouldn't worry about the formatting, in fact, I would
say that we can move all of this logic to `print_fields`.

So each field would only be incharge of providing the output data. Then
`print_fields` would take the key, the output data and format it as
needed. This would also make it much easier to use a new format if
needed in the future.

> +static void add_references_format(struct strbuf *buf,
> +				  struct repository *repo)
> +{
> +	add_string(buf, "references.format",
> +		   ref_storage_format_to_name(repo->ref_storage_format));
> +}
> +
> +// repo_info_fields keys should be in lexicographical order
> +static const struct field repo_info_fields[] = {
> +	{"references.format", add_references_format},
> +};
> +
> +static int repo_info_fields_cmp(const void *va, const void *vb)
> +{
> +	const struct field *a = va;
> +	const struct field *b = vb;
> +
> +	return strcmp(a->key, b->key);
> +}
> +
> +static add_field_fn *get_append_callback(const char *key) {
> +	const struct field search_key = {key, NULL};
> +	const struct field *found = bsearch(&search_key, repo_info_fields,
> +					    ARRAY_SIZE(repo_info_fields),
> +					    sizeof(struct field),
> +					    repo_info_fields_cmp);
> +	return found ? found->add_field_callback : NULL;
> +}
> +
> +static int qsort_strcmp(const void *va, const void *vb)
> +{
> +	const char *a = *(const char **)va;
> +	const char *b = *(const char **)vb;
> +
> +	return strcmp(a, b);
> +}
> +
> +static void print_fields(int argc, const char **argv, struct repository *repo) {
> +	const char *last = "";
> +	struct strbuf buf;
> +	strbuf_init(&buf, 256);
> +
> +	QSORT(argv, argc, qsort_strcmp);
> +
> +	for (int i = 0; i < argc; i++) {
> +		add_field_fn *callback;
> +		const char *key = argv[i];
> +
> +		if (!strcmp(key, last))
> +			continue;
> +
> +		callback = get_append_callback(key);
> +
> +		if (!callback) {
> +			error("key %s not found", key);
> +			strbuf_release(&buf);
> +			exit(1);
> +		}
> +
> +		callback(&buf, repo);
> +		last = key;
> +	}
> +
> +	fwrite(buf.buf, 1, buf.len, stdout);
> +	strbuf_release(&buf);
> +}
> +
> +static int repo_info(int argc,
> +		     const char **argv,
>  		     const char *prefix UNUSED,
> -		     struct repository *repo UNUSED)
> +		     struct repository *repo)
>  {
> +
> +	print_fields(argc - 1 , argv + 1, repo);
>  	return 0;
>  }
>
> @@ -16,7 +100,7 @@ int cmd_repo(int argc,
>  {
>  	parse_opt_subcommand_fn *fn = NULL;
>  	const char *const repo_usage[] = {
> -		"git repo info",
> +		"git repo info [<key>...]",

Shouldn't this be part of the previous commit?

>  		NULL
>  	};
>  	struct option options[] = {
> diff --git a/t/meson.build b/t/meson.build
> index 1af289425d..8693e6abc4 100644
> --- a/t/meson.build
> +++ b/t/meson.build
> @@ -245,6 +245,7 @@ integration_tests = [
>    't1700-split-index.sh',
>    't1701-racy-split-index.sh',
>    't1800-hook.sh',
> +  't1900-repo.sh',
>    't2000-conflict-when-checking-files-out.sh',
>    't2002-checkout-cache-u.sh',
>    't2003-checkout-cache-mkdir.sh',
> diff --git a/t/t1900-repo.sh b/t/t1900-repo.sh
> new file mode 100755
> index 0000000000..b80fc6b78b
> --- /dev/null
> +++ b/t/t1900-repo.sh
> @@ -0,0 +1,47 @@
> +#!/bin/sh
> +
> +test_description='test git repo-info'
> +
> +. ./test-lib.sh
> +
> +# Test if a field is correctly returned in the null-terminated format
> +#
> +# Usage: test_repo_info <label> <init command> <key> <expected value>
> +#
> +# Arguments:
> +#   label: the label of the test
> +#   init command: a command that creates a repository called 'repo', configured
> +#      accordingly to what is being tested
> +#   key: the key of the field that is being tested
> +#   expected value: the value that the field should contain
> +test_repo_info () {
> +	label=$1
> +	init_command=$2
> +	key=$3
> +	expected_value=$4
> +
> +	test_expect_success "$label" '
> +		test_when_finished "rm -rf repo" &&
> +		eval "$init_command" &&
> +		echo "$expected_value" | lf_to_nul >expected &&
> +		git -C repo repo info "$key" >output &&
> +		tail -n 1 output >actual &&
> +		test_cmp expected actual
> +	'
> +}
> +
> +test_repo_info 'ref format files is retrieved correctly' '
> +	git init --ref-format=files repo' 'references.format' 'files'
> +
> +test_repo_info 'ref format reftable is retrieved correctly' '
> +	git init --ref-format=reftable repo' 'references.format' 'reftable'
> +
> +test_expect_success "only one value is returned if the same key is requested twice" '
> +	echo "references.format" > expected &&
> +	git rev-parse --show-ref-format > ref-format &&
> +	lf_to_nul <ref-format >>expected &&
> +	git repo info references.format references.format > actual &&
> +	test_cmp expected actual
> +'
> +
> +test_done
> --
> 2.39.5 (Apple Git-154)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]