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

2019-10-22 Thread Heba Waly
On Wed, Oct 23, 2019 at 9:59 AM Emily Shaffer  wrote:
>
> On Tue, Oct 22, 2019 at 07:05:06AM +, Heba Waly via GitGitGadget wrote:
> > From: Heba Waly 
> >
> > Move the documentation from Documentation/technical/api-config.txt into
> > config.h
>
> This is still a little thin for what we usually want from commit
> messages. Try to imagine that five years from now, you find this commit
> by running `git blame` on config.h and then examining the commit which
> introduced all these comments with `git show ` - what would
> you want to know?
>
> Typically we want to know "why" the change was made, because the diff
> shows "what". We can see from the diff that you're moving comments from
> A to B, but if you explain why you did so (not "because my Outreachy
> mentor told me to" ;) but "because it is useful to see usage information
> next to code" or "this is best practice as described by blah blah") - I
> wouldn't be able to know that reasoning just from looking at your diff.
Ok, got it.

>
> > diff --git a/config.h b/config.h
> > index f0ed464004..02f78ffc2b 100644
> > --- a/config.h
> > +++ b/config.h
> > @@ -4,6 +4,23 @@
> >  #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
>
> Ah, here's another place where the Asciidoc link isn't going to do
> anything anymore.
yep!
> Otherwise I didn't still see anything jumping out. When the commit
> message is cleaned up I'm ready to add my Reviewed-by line.
great!

>  - Emily

Thanks :)


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

2019-10-22 Thread Junio C Hamano
Emily Shaffer  writes:

>> ...
>> +/**
>> + * 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
>
> Ah, here's another place where the Asciidoc link isn't going to do
> anything anymore.
>
> Otherwise I didn't still see anything jumping out. When the commit
> message is cleaned up I'm ready to add my Reviewed-by line.

Thanks.  Your review(s) have been quite sensible and helpful.




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

2019-10-22 Thread Emily Shaffer
On Tue, Oct 22, 2019 at 07:05:06AM +, Heba Waly via GitGitGadget wrote:
> From: Heba Waly 
> 
> Move the documentation from Documentation/technical/api-config.txt into
> config.h

This is still a little thin for what we usually want from commit
messages. Try to imagine that five years from now, you find this commit
by running `git blame` on config.h and then examining the commit which
introduced all these comments with `git show ` - what would
you want to know?

Typically we want to know "why" the change was made, because the diff
shows "what". We can see from the diff that you're moving comments from
A to B, but if you explain why you did so (not "because my Outreachy
mentor told me to" ;) but "because it is useful to see usage information
next to code" or "this is best practice as described by blah blah") - I
wouldn't be able to know that reasoning just from looking at your diff.


> diff --git a/config.h b/config.h
> index f0ed464004..02f78ffc2b 100644
> --- a/config.h
> +++ b/config.h
> @@ -4,6 +4,23 @@
>  #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

Ah, here's another place where the Asciidoc link isn't going to do
anything anymore.

Otherwise I didn't still see anything jumping out. When the commit
message is cleaned up I'm ready to add my Reviewed-by line.

 - Emily