[PATCH] config: Add documentation for writing config files

[PATCH] config: Add documentation for writing config files

[Tanay Abhra]

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

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.
+In the end they both all call `git_config_set_multivar_in_file` which takes
+four parameters:
+
+- the name of the file, as a string, to which key/value pairs will be written.
+
+- the name of key, as a string. This is in canonical "flat" form: the section,
+  subsection, and variable segments will be separated by dots, and the section
+  and variable segments will be all lowercase.
+  E.g., `core.ignorecase`, `diff.SomeType.textconv`.
+
+- the value of the variable, as a string. If value is equal to NULL, it will
+  remove the matching key from the config file.
+
+- the value regex, as a string. It will disregard key/value pairs where value
+  does not match.
+
+- a multi_replace value, as an int. If value is equal to zero, nothing or only
+  one matching key/value is replaced, else all matching key/values (regardless
+  how many) are removed, before the new pair is written.
+
+It returns 0 on success.
+
+Also, there are functions `git_config_rename_section` and
+`git_config_rename_section_in_file` with parameters `old_name` and `new_name`
+for renaming or removing sections in the config files. If NULL is passed
+through `new_name` parameter, the section will be removed from the config file.
-- 
1.9.0.GIT

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

[Matthieu Moy]

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/

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

[Tanay Abhra]

On 06/02/2014 12:37 PM, Matthieu Moy wrote:
> 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" ;-).

Ok, I will send a new patch with the updated commit message. .

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

Sorry, but I don't think so. In cache.h, the following functions have been provided
as externs,
git_config_set_in_file()
git_config_set()
git_config_set_multivar()
git_config_set_multivar_in_file()
extern int git_config_rename_section()
extern int git_config_rename_section_in_file()

Thus, they seem to be the part of the API. In most of the technical API docs I have
read there is at least a description of all parameters of the main function also,
relevant data structures if any.
Also a certain amount of redundancy about details is allowed.

A good example is api-hashmap.txt which provides detailed descriptions of each
function and data structure which was very much helpful to me.

Reverse is api-string-list.txt which was useless to me, so I had to go through
the whole code to understand how to use it.

Thanks for the review.