Re: [GSoC RFC PATCH v4 0/4] repo: add new command for retrieving repository info

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

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

> This patch, then:
>
> - Renames the command to `repo` instead of `repo-info`. All the functionality
>   of `repo-info` will now be under `repo info`. The functionality of `survey`
>   will be moved to another subcommand of `git repo`.
>
> - Removes the JSON support. Given that after the previous feedback we already
>   have a nice machine-readable format for outputting this data, JSON would not
>   be so useful as it seemed to be at first (when the "other format" was just
>   returning the values without the keys). This makes the code far more simpler,
>   as we don't need to deal with the details of both formats.
>
> - Uses a simpler representation of the fields, based in their keys instead of
>   declaring multiple enums and using nested switches. This new solution is
>   based in a table mapping the keys and the callbacks for retrieving the data.
>
> - Provide a simple infrastructure for extending with the second command.
>
> Given that this v4 is almost a rewrite, I think it isn't worth to send a
> range-diff.

OK.  If it is almost a rewrite, then perhaps we should tart by
making sure that it won't have too many irritating style gotchas to
discourage reviewers from reading it ;-)

The below is from 

    $ git clang-format --diff $(git merge-base master HEAD) -- builtin/repo.c

I removed some obviously bad suggestions but many were improvements.

I'll follow this message up as if I were reviewing what clang-format
produced.

Thanks.


 builtin/repo.c | 47 ++++++++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 27 deletions(-)

diff --git c/builtin/repo.c w/builtin/repo.c
index d75417a48b..e8cf465da6 100644
--- c/builtin/repo.c
+++ w/builtin/repo.c
@@ -14,27 +14,23 @@ struct field {
 	add_field_fn *add_field_callback;
 };
 
-static void add_string(struct strbuf *buf,
-		       const char *key, const char *value)
+static void add_string(struct strbuf *buf, const char *key, const char *value)
 {
 	strbuf_addf(buf, "%s\n%s%c", key, value, '\0');
 }
 
-static void add_bool(struct strbuf *buf,
-		     const char *key, const int value)
+static void add_bool(struct strbuf *buf, const char *key, const int value)
 {
 	const char *output_value = value ? "true" : "false";
 	strbuf_addf(buf, "%s\n%s%c", key, output_value, '\0');
 }
 
-static void add_references_format(struct strbuf *buf,
-				  struct repository *repo)
+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));
 }
 
-
 static void add_layout_bare(struct strbuf *buf, struct repository *repo UNUSED)
 {
 	add_bool(buf, "layout.bare", is_bare_repository());
@@ -45,11 +41,11 @@ static void add_layout_shallow(struct strbuf *buf, struct repository *repo)
 	add_bool(buf, "layout.shallow", is_repository_shallow(repo));
 }
 
-// repo_info_fields keys should be in lexicographical order
+/* repo_info_fields keys should be in lexicographical order */
 static const struct field repo_info_fields[] = {
-	{"layout.bare", add_layout_bare},
-	{"layout.shallow", add_layout_shallow},
-	{"references.format", add_references_format},
+	{ "layout.bare", add_layout_bare },
+	{ "layout.shallow", add_layout_shallow },
+	{ "references.format", add_references_format },
 };
 
 static int repo_info_fields_cmp(const void *va, const void *vb)
@@ -60,12 +56,13 @@ static int repo_info_fields_cmp(const void *va, const void *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);
+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;
 }
 
@@ -77,7 +74,8 @@ static int qsort_strcmp(const void *va, const void *vb)
 	return strcmp(a, b);
 }
 
-static void print_fields(int argc, const char **argv, struct repository *repo) {
+static void print_fields(int argc, const char **argv, struct repository *repo)
+{
 	const char *last = "";
 	struct strbuf buf;
 	strbuf_init(&buf, 256);
@@ -91,7 +89,7 @@ static void print_fields(int argc, const char **argv, struct repository *repo) {
 		if (!strcmp(key, last))
 			continue;
 
-		callback = get_append_callback(key);
+		callback = *get_append_callback(key);
 
 		if (!callback) {
 			error("key %s not found", key);
@@ -107,19 +105,14 @@ static void print_fields(int argc, const char **argv, struct repository *repo) {
 	strbuf_release(&buf);
 }
 
-static int repo_info(int argc,
-		     const char **argv,
-		     const char *prefix UNUSED,
+static int repo_info(int argc, const char **argv, const char *prefix UNUSED,
 		     struct repository *repo)
 {
-
-	print_fields(argc - 1 , argv + 1, repo);
+	print_fields(argc - 1, argv + 1, repo);
 	return 0;
 }
 
-int cmd_repo(int argc,
-	     const char **argv,
-	     const char *prefix,
+int cmd_repo(int argc, const char **argv, const char *prefix,
 	     struct repository *repo)
 {
 	parse_opt_subcommand_fn *fn = NULL;