Re: [GSoC RFC PATCH 3/5] repo-info: add the field references.format

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

Karthik Nayak <karthik.188@gmail.com> writes:

>>  static void repo_info_init(struct repo_info *repo_info,
>>  			   struct repository *repo,
>>  			   char *format,
>> -			   int allow_empty UNUSED,
>> -			   int argc UNUSED,
>> -			   const char **argv UNUSED
>> +			   int allow_empty,
>> +			   int argc,
>> +			   const char **argv
>>  			   ) {
>>
>
> Nit: we wrap to 80 chars generally, so you can put multiple arguments on
> the same line.

Good point.  It is worth pointing out that we also group related
arguments together, and make it easier to later add new things at
the end and still keep related things together, e.g.,

static void repo_info_init(struct repo_info *repo_info,
			   struct repository *repo,
                           int argc, const char **argv,
			   const char *format,
			   int allow_empty)
{

Obviously argc, argv belong to each other, so it is OK to have on
the same line, and unlike repo_info (i.e. the out argument), repo
(i.e. the primary thing that is inspected), format and allow_empty
are something you would want to later extend when you "enrich" the
interface and functionality.  You may even gain "int display_width"
argument to allow line-wrapped output, for example, and you would
have to insert in the middle if you want to keep related things
together, if you had (argc, argv) at the end. 

Also note that {opening and closing braces} around the function body
sits on their own lines.

Why is "format" a string?  Shouldn't the caller do all the argument
parsing and pass an enum or something to this function?  After all,
I presume that allow_empty is set in response to "--allow-empty" or
something that the user gave to the command, and that parsing is done
by the caller before it calls this function, no?  Why not do the
same for the output format?

I'll stop here for now.