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

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

Junio C Hamano <gitster@pobox.com> writes:

> 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));
>  }

If the parameter list fits on a single line not just helps readers
of a code who reads from top to bottom, but those who run "grep" for
the function name.

>  
> -
>  static void add_layout_bare(struct strbuf *buf, struct repository *repo UNUSED)
>  {
>  	add_bool(buf, "layout.bare", is_bare_repository());

No reason to give double-blank between these two functions; it is
not like add_string/bool/references/format are closer together than
this add_layout_bare

> @@ -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 */

We don't do // comments

>  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 },
>  };

Spaces are used inside {braces} like the above.

>  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)
> +{

"{" opening and "}" closing braces around a function body sit on
their own line alone without anybody else.  This is unlike the
multi-statement blocks in control structures (e.g. opening brace for
the block executed when condition holds in "if (cond) {" comes after
the closing parenthesis ")" for the condition after a SP on the same
line).

> +	const struct field search_key = { key, NULL };

Use of spaces inside brace pair again.

> +	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)
> +{

Ditto.

>  	const char *last = "";
>  	struct strbuf buf;
>  	strbuf_init(&buf, 256);
> @@ -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);

Extra SP before ",".

> +	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;