Re: [PATCH 1/1] config: add documentation to config.h

[Heba Waly <heba.waly@gmail.com>]

On Sat, Oct 19, 2019 at 11:07 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > From: Heba Waly <heba.waly@gmail.com>
> >
> > This commit is copying and summarizing the documentation from
> > documentation/technical/api-config.txt to comments in config.h
>
> Thanks for this commit!
>
> As for your commit message, as far as I know, the idea is to move the
> documentation, not to copy it. Also, write this in imperative form,
> e.g.:
>
>   Move the documentation from Documentation/technical/api-config.txt
>   into config.h.
>
> Also change the title of the commit message accordingly, e.g.:
>
>   config: move documentation to header file
>
Ok.
> Also, include the deletion of api-config.txt in this commit.
I wasn't sure if the api-config.txt should be removed or not so I
decided to keep it
and wait for feedback. I assume I'll need to delete api-config.html as well?

> If you are doing any summarizing, describe what summarizing you are
> doing in the commit message too.
Ok, will do so.

> > + * A config callback function takes three parameters:
> > + *
> > + * - the name of the parsed variable. 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 found variable, as a string. If the variable had no
> > + *   value specified, the value will be NULL (typically this means it
> > + *   should be interpreted as boolean true).
> > + *
> > + * - a void pointer passed in by the caller of the config API; this can
> > + *   contain callback-specific data
> > + *
> > + * A config callback should return 0 for success, or -1 if the variable
> > + * could not be parsed properly.
> > + */
> > +
> >  struct object_id;
> >
> >  /* git_config_parse_key() returns these negated: */
> > @@ -73,6 +107,11 @@ struct config_options {
> >
> >  typedef int (*config_fn_t)(const char *, const char *, void *);
> >  int git_default_config(const char *, const char *, void *);
>
> The config callback is config_fn_t so that documentation should be
> placed above that typedef.
>
Cool, I couldn't find it, thanks!

> Other than that, this looks good to me. The result is perhaps not as
> tidy as we would like (especially with some functions being documented
> and others not) but I think, anyway, that a verbatim movement should be
> done in one commit (this one) and improvements, in a subsequent commit.
You're right, I would have loved to get all the functions documented, but that's
something I'm not able to do right now as I'm still getting familiar
with the code base.
But it's a good start!
I agree with you about moving the documentation and deleting the file
in one commit.
Will do so.

Thank you for the feedback!

Heba