From: Lars Hjemli <hjemli@gmail.com>
Date: Sun, 10 Jun 2007 08:58:29 GMT
Subject: Re: [PATCH 2/3] Add gitmodules(5)
Message-ID: <8c5c35580706100158n7dabfce4y5f79f8943d8abb87@mail.gmail.com>
In-Reply-To: <20070610002802.GD31707@planck.djpig.de>

On 6/10/07, Frank Lichtenheld <frank@lichtenheld.de> wrote:
> The unofficial git documentation nitpicker at work ;)

That's good, keep it up :)

>
> On Sat, Jun 09, 2007 at 11:38:51PM +0200, Lars Hjemli wrote:
> > +The `.gitmodules` file, located in the top-level directory of a
> > +gitlink:git[7] working tree, is a text file with a layout matching the
>
> That link seems superfluous to me.

Yeah, what I really wanted here was a link to the definition of "git
working tree". I'll drop it.

> I would have used "syntax" instead of "layout".

Agreed

>
> > +requirements of gitlink:git-config[1].
> > +
> > +The file consists of sections named `module`, divided into one subsection
> > +per submodule. The subsections are named with the logical name of the
> > +submodule it describes.
>
> "sections named module" sounds confusing to me. Why are there multiple
> sections named module? (for the record: I know what you mean, I just
> don't know if it couldn't be said simpler)
> Maybe better "subsections of section `module`, one per submodule"?
> Hmm, sounds ugly too.

Good documentation is hard, so I'll work on it some more...

>
> > +Each submodule can contain the following keys.
> > +
> > +module.$name.path::
> > +     Define a path, relative to the top-level directory of the git
> > +     working tree, where the submodule is expected to be checked out.
> > +
> > +module.$name.url::
> > +     Define a url from where the submodule repository can be cloned.
>
> For .path a "Defaults to name of submodule" probably wouldn't hurt.

True. But there might be some issues with this rule, so I'll leave it
as is for now.

>
> For the sake of documentation consistency I would suggest
> module.<name>.path. You can compare the output of
> $ grep "\.<[a-z]" Documentation/*.txt
> with
> $ grep "\.\$[a-z]" Documentation/*.txt
> to see what I mean.

That was very descriptive, thanks!

>
> > +     [module 'libfoo']
> > +             path = include/foo
> > +             url = git://example1.com/git/libfoo.git
> > +
> > +     [module 'libbar']
> > +             url = git://example2.com/pub/git/libbar.git
>
> This would actually be a syntax error in a git config file
> (subsection names can be enclosed in "" but not '').

Shame on me for drinking while documenting ;-)

Thanks for the review, I'll try to send a fixed-up patch later today.

--
larsh