Re: [PATCH] config: Add documentation for writing config files

[Matthieu Moy <Matthieu.Moy@Grenoble-inp.fr>]

Tanay Abhra <tanayabh@gmail.com> writes:

> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> ---
>  Documentation/technical/api-config.txt | 31 ++++++++++++++++++++++++++++++-
>  1 file changed, 30 insertions(+), 1 deletion(-)

Even though the reason to replace a TODO with real stuff is rather
straigthforward, the reader appriates a short commit message (ideally
pointing to the commit introducing the TODO). My first thought reading
the patch was "wtf, is that a patch in the middle of a series, where
does this TODO come from" ;-).

> diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
> index 230b3a0..df08385 100644
> --- a/Documentation/technical/api-config.txt
> +++ b/Documentation/technical/api-config.txt
> @@ -137,4 +137,33 @@ int read_file_with_include(const char *file, config_fn_t fn, void *data)
>  Writing Config Files
>  --------------------
>  
> -TODO
> +Git gives multiple entry points in the Config API to write config values to
> +files namely `git_config_set_in_file` and `git_config_set`, which write to
> +a specific config file or to `.git/config` respectively. They both take a
> +key/value pair as parameter.

Sounds good.

> +In the end they both all call `git_config_set_multivar_in_file` which takes
> +four parameters:

Do we really want to document this as part of the config API? i.e. does
a normal user of the API want to know about this? My understanding is
that git_config_set_multivar_in_file is essentially a private function,
and then the best place to document is with comments near the function
definition (it already has such comment).

Your text is easier to understand and more detailed, but I would
personnally prefer improving the in-code comment (possibly just leaving
a mention of git_config_set_multivar_in_file and pointing the reader to
the code for details).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/