Tanay Abhra <tanayabh@xxxxxxxxx> writes: > Signed-off-by: Tanay Abhra <tanayabh@xxxxxxxxx> > --- > 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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html