Re: [PATCH 1/1] config: add documentation to config.h
On Sun, Oct 20, 2019 at 09:35:17PM +1300, Heba Waly wrote: > On Sat, Oct 19, 2019 at 11:52 AM Emily Shaffer > wrote: > > > > On Fri, Oct 18, 2019 at 12:06:59AM +, Heba Waly via GitGitGadget wrote: > > > From: Heba Waly > > > > Hi Heba, > > > > Thanks for the patch! > > > > I'd like to highlight to the community that this is an Outreachy > > applicant and microproject. Heba, when you send the next version, I > > think you can add [Outreachy] manually to the PR subject line - that > > should draw the attention of those in the community who are invested in > > helping Outreachy applicants. > Good idea! I wanted to add it to the email subject but as I decided to > use gitgadget > I had no control over the subject. Hm, it looks like you already figured out how to add it to the title of the PR. :) > > > > > > This commit is copying and summarizing the documentation from > > > documentation/technical/api-config.txt to comments in config.h > > > > I think in the GitGitGadget PR you've got some great comments from Dscho > > about how to format your commit message; please take a look at those and > > feel free to reach out to me if you're still not sure what's missing or > > not. > Will do. > > > Signed-off-by: Heba Waly > > > > One thing I miss in this change is the removal of the contents of > > Documentation/technical/api-config.txt (or maybe the removal of the file > > itself). I'd prefer to see at least for api-config.txt to say something > > like "Please refer to comments in 'config.h'"; or, more drastically, for > > api-config.txt to be removed entirely. > > > > Having both pieces of documentation standing independently means that > > someone who's trying to add new information about the config API won't > > know where to add it; eventually they'll add something to config.h but > > not api-config.txt, or vice versa, and the two documents will go out of > > sync. So we want to move the documentation, rather than copy it. > That makes sense, thanks for the explanation. > I wasn't sure if it should be removed or not so I decided to leave it > until I'm asked otherwise. > So I assume api-config.html will be removed too? That shouldn't be tracked - this is generated from api-config.txt as part of the build. So don't worry about this part. > > > + > > > +/** > > > + * Value Parsing Helpers > > > + * - > > > > It may not make sense to have the header here in the middle of the doc. > > > > I wonder whether we need the headers at all anymore; or, whether it > > makes more sense to put this header in the long comment at the top with > > just the list of function names (so someone knows where to look), and > > leave the per-function explanations inline with the function they > > describe? > I see your point Emily, but in the CodingGuidelines file it was > advised to refer to strbuf.h > as a model for documentation, I noticed that strbuf.h used headers > this way so I decided > to replicate that. Ok! Sure. > > I made a couple of smallish comments about general formatting, but I'm > > also interested to know whether you were able to move the entire > > contents of api-config.txt across to here. Was there anything that you > > couldn't find a place for? > Yes, everything is moved. > > Thanks a lot for this change, and congrats on getting your first review > > out! Welcome! :) > > > > - Emily > > > Thanks a lot Emily for the detailed and helpful feedback! > > Heba
Re: [PATCH 1/1] config: add documentation to config.h
On Sat, Oct 19, 2019 at 11:52 AM Emily Shaffer wrote:
>
> On Fri, Oct 18, 2019 at 12:06:59AM +, Heba Waly via GitGitGadget wrote:
> > From: Heba Waly
>
> Hi Heba,
>
> Thanks for the patch!
>
> I'd like to highlight to the community that this is an Outreachy
> applicant and microproject. Heba, when you send the next version, I
> think you can add [Outreachy] manually to the PR subject line - that
> should draw the attention of those in the community who are invested in
> helping Outreachy applicants.
Good idea! I wanted to add it to the email subject but as I decided to
use gitgadget
I had no control over the subject.
> >
> > This commit is copying and summarizing the documentation from
> > documentation/technical/api-config.txt to comments in config.h
>
> I think in the GitGitGadget PR you've got some great comments from Dscho
> about how to format your commit message; please take a look at those and
> feel free to reach out to me if you're still not sure what's missing or
> not.
Will do.
> > Signed-off-by: Heba Waly
>
> One thing I miss in this change is the removal of the contents of
> Documentation/technical/api-config.txt (or maybe the removal of the file
> itself). I'd prefer to see at least for api-config.txt to say something
> like "Please refer to comments in 'config.h'"; or, more drastically, for
> api-config.txt to be removed entirely.
>
> Having both pieces of documentation standing independently means that
> someone who's trying to add new information about the config API won't
> know where to add it; eventually they'll add something to config.h but
> not api-config.txt, or vice versa, and the two documents will go out of
> sync. So we want to move the documentation, rather than copy it.
That makes sense, thanks for the explanation.
I wasn't sure if it should be removed or not so I decided to leave it
until I'm asked otherwise.
So I assume api-config.html will be removed too?
> Plus, having the removed doc as part of this change means I can more
> easily look at where the lines of content are coming from and see if you
> made any significant changes from the old contents of api-config.txt.
> Having a smaller amount of change to review means your review will be
> quicker - I don't feel as strong a need to check the grammar, spelling,
> etc, because that text has already been reviewed before, and can just
> make sure that the placement of each piece of documentation makes sense.
yes!
>
> > ---
> > config.h | 327 +++
> > 1 file changed, 327 insertions(+)
> >
> > diff --git a/config.h b/config.h
> > index f0ed464004..fa999a2ba0 100644
> > --- a/config.h
> > +++ b/config.h
> > @@ -4,6 +4,40 @@
> > #include "hashmap.h"
> > #include "string-list.h"
> >
> > +
> > +/**
> > + * The config API gives callers a way to access Git configuration files
> > + * (and files which have the same syntax). See linkgit:git-config[1] for a
> > + * discussion of the config file syntax.
> > + *
> > + * General Usage
> > + * -
> > + *
> > + * Config files are parsed linearly, and each variable found is passed to a
> > + * caller-provided callback function. The callback function is responsible
> > + * for any actions to be taken on the config option, and is free to ignore
> > + * some options. It is not uncommon for the configuration to be parsed
> > + * several times during the run of a Git program, with different callbacks
> > + * picking out different variables useful to themselves.
> > + *
> > + * 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 *);
> > +
> > +/**
> > + * Read a specific file in git-config format.
> > + * This function takes the same callback and data parameters as
> > `git_config`.
> > + */
> > int git_config_from_file(config_fn_t fn, const char *, void *);
> > int git_config_from_file_with_options(config_fn_t fn, const char *,
> > void *,
> > @@ -88,33 +127,152 @@
Re: [PATCH 1/1] config: add documentation to config.h
On Sat, Oct 19, 2019 at 11:07 AM Jonathan Tan wrote:
>
> > From: Heba Waly
> >
> > 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
Re: [PATCH 1/1] config: add documentation to config.h
On Fri, Oct 18, 2019 at 12:06:59AM +, Heba Waly via GitGitGadget wrote:
> From: Heba Waly
Hi Heba,
Thanks for the patch!
I'd like to highlight to the community that this is an Outreachy
applicant and microproject. Heba, when you send the next version, I
think you can add [Outreachy] manually to the PR subject line - that
should draw the attention of those in the community who are invested in
helping Outreachy applicants.
>
> This commit is copying and summarizing the documentation from
> documentation/technical/api-config.txt to comments in config.h
I think in the GitGitGadget PR you've got some great comments from Dscho
about how to format your commit message; please take a look at those and
feel free to reach out to me if you're still not sure what's missing or
not.
> Signed-off-by: Heba Waly
One thing I miss in this change is the removal of the contents of
Documentation/technical/api-config.txt (or maybe the removal of the file
itself). I'd prefer to see at least for api-config.txt to say something
like "Please refer to comments in 'config.h'"; or, more drastically, for
api-config.txt to be removed entirely.
Having both pieces of documentation standing independently means that
someone who's trying to add new information about the config API won't
know where to add it; eventually they'll add something to config.h but
not api-config.txt, or vice versa, and the two documents will go out of
sync. So we want to move the documentation, rather than copy it.
Plus, having the removed doc as part of this change means I can more
easily look at where the lines of content are coming from and see if you
made any significant changes from the old contents of api-config.txt.
Having a smaller amount of change to review means your review will be
quicker - I don't feel as strong a need to check the grammar, spelling,
etc, because that text has already been reviewed before, and can just
make sure that the placement of each piece of documentation makes sense.
> ---
> config.h | 327 +++
> 1 file changed, 327 insertions(+)
>
> diff --git a/config.h b/config.h
> index f0ed464004..fa999a2ba0 100644
> --- a/config.h
> +++ b/config.h
> @@ -4,6 +4,40 @@
> #include "hashmap.h"
> #include "string-list.h"
>
> +
> +/**
> + * The config API gives callers a way to access Git configuration files
> + * (and files which have the same syntax). See linkgit:git-config[1] for a
> + * discussion of the config file syntax.
> + *
> + * General Usage
> + * -
> + *
> + * Config files are parsed linearly, and each variable found is passed to a
> + * caller-provided callback function. The callback function is responsible
> + * for any actions to be taken on the config option, and is free to ignore
> + * some options. It is not uncommon for the configuration to be parsed
> + * several times during the run of a Git program, with different callbacks
> + * picking out different variables useful to themselves.
> + *
> + * 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 *);
> +
> +/**
> + * Read a specific file in git-config format.
> + * This function takes the same callback and data parameters as `git_config`.
> + */
> int git_config_from_file(config_fn_t fn, const char *, void *);
> int git_config_from_file_with_options(config_fn_t fn, const char *,
> void *,
> @@ -88,33 +127,152 @@ void git_config_push_parameter(const char *text);
> int git_config_from_parameters(config_fn_t fn, void *data);
> void read_early_config(config_fn_t cb, void *data);
> void read_very_early_config(config_fn_t cb, void *data);
> +
> +/**
> + * Most programs will simply want to look up variables in all config files
> + * that Git knows about, using the normal precedence rules. To do this,
> + * call `git_config` with a callback function and void data pointer.
> + *
> + * `git_config` will read all config sources in order of increasing
> + * priority. Thus a callback sh
Re: [PATCH 1/1] config: add documentation to config.h
> From: Heba Waly
>
> 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
Also, include the deletion of api-config.txt in this commit.
If you are doing any summarizing, describe what summarizing you are
doing in the commit message too.
> + * 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.
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.
