[PATCH] document string_list_clear

[PATCH] document string_list_clear

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---

 Just stumbled accross this one and wasn't sure if it also frees up
 the memory involved.

 string-list.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/string-list.h b/string-list.h
index 494eb5d..99ecc44 100644
--- a/string-list.h
+++ b/string-list.h
@@ -21,6 +21,11 @@ struct string_list {
 void string_list_init(struct string_list *list, int strdup_strings);
 
 void print_string_list(const struct string_list *p, const char *text);
+
+/*
+ * Clears the string list, so it has zero items. All former items are freed.
+ * If free_util is true, all util pointers are also freed.
+ */
 void string_list_clear(struct string_list *list, int free_util);
 
 /* Use this function to call a custom clear function on each util pointer */
-- 
2.2.0

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Stefan Beller wrote:

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>  Just stumbled accross this one and wasn't sure if it also frees up
>  the memory involved.
>
>  string-list.h | 5 +++++
>  1 file changed, 5 insertions(+)

Sounds reasonable.  Documentation/technical/api-string-list.txt
documents these functions more fully.  The right balance between
documenting things in two places vs. adding "see also" pointers vs.
just putting the highlights in one of the two places isn't obvious to
me.

[...]
> --- a/string-list.h
> +++ b/string-list.h
> @@ -21,6 +21,11 @@ struct string_list {
>  void string_list_init(struct string_list *list, int strdup_strings);
>  
>  void print_string_list(const struct string_list *p, const char *text);
> +
> +/*
> + * Clears the string list, so it has zero items. All former items are freed.
> + * If free_util is true, all util pointers are also freed.
> + */
>  void string_list_clear(struct string_list *list, int free_util);

The api doc says

	Free a string_list. The `string` pointer of the items will be freed in
	case the `strdup_strings` member of the string_list is set. The second
	parameter controls if the `util` pointer of the items should be freed
	or not.

One option here would be to say

	Free a string_list.  See Documentation/technical/api-string-list.txt
	for details.

That reminds me: why do we call this string_list_clear instead of
string_list_free?

Thanks,
Jonathan

Re: [PATCH] document string_list_clear

[Jeff King]

On Fri, Dec 05, 2014 at 06:04:58PM -0800, Jonathan Nieder wrote:

> That reminds me: why do we call this string_list_clear instead of
> string_list_free?

Because it does not free the struct itself, and because you can then use
the result again. I think we try to draw a distinction between:

  /* Free resources, but do not reinitialize! */
  void string_list_free_data(struct string_list *s)
  {
	int i;
	for (i = 0; i < s->nr; i++)
		free(s->items[i].string);
	free(s->items[i];
  }

  /* Free resources, and go back to initialized but empty state */
  void string_list_clear(struct string_list *s)
  {
	string_list_free_data(s);
	s->nr = s->alloc = 0;
	s->items = NULL;
  }

  /* Free all resources from dynamically allocated structure */
  void string_list_free(struct string_list *s)
  {
	string_list_clear(s);
	free(s);
  }

Ideally we use consistent names to distinguish between them. We are not
always consistent, though (probably strbuf_release should be called
strbuf_clear for consistency). But I think we are fairly consistent that
"_free()" means "...and free the pointer, too".

In general, we try to avoid the first as a public interface (because it
is error-prone when somebody tries to reuse the list, and the extra work
of zero-ing the pointer is not enough to care about).

We also tend to avoid the third, because it is quite often not the
business of the object whether it was dynamically constructed or not
(exceptions tend to be node-oriented structures like linked lists and
trees; c.f. cache_tree and commit_list).

Maybe that should go into CodingGuidelines? It's not really style,
exactly, but it is convention.

-Peff

Re: [PATCH] document string_list_clear

[Jeff King]

On Fri, Dec 05, 2014 at 06:04:58PM -0800, Jonathan Nieder wrote:

> Stefan Beller wrote:
> 
> > Signed-off-by: Stefan Beller <sbeller@google.com>
> > ---
> >
> >  Just stumbled accross this one and wasn't sure if it also frees up
> >  the memory involved.
> >
> >  string-list.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> 
> Sounds reasonable.  Documentation/technical/api-string-list.txt
> documents these functions more fully.  The right balance between
> documenting things in two places vs. adding "see also" pointers vs.
> just putting the highlights in one of the two places isn't obvious to
> me.

Also, I forgot to mention: if we consistently put the API docs into the
header files and extracted it automatically into standalone documents,
we would not need to have two places.

This is something I've been meaning to look at for a long time, but it
never quite makes the priority list. And my past experiences with tools
like doxygen has been that they are complicated and have a lot of
dependencies. It's been a long time since I've tried, though.

-Peff

Re: [PATCH] document string_list_clear

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> +/*
>> + * Clears the string list, so it has zero items. All former items are freed.
>> + * If free_util is true, all util pointers are also freed.
>> + */
>>  void string_list_clear(struct string_list *list, int free_util);
>
> The api doc says
>
> 	Free a string_list. The `string` pointer of the items will be freed in
> 	case the `strdup_strings` member of the string_list is set. The second
> 	parameter controls if the `util` pointer of the items should be freed
> 	or not.
>
> One option here would be to say
>
> 	Free a string_list.  See Documentation/technical/api-string-list.txt
> 	for details.

If we later introduce string_list_free() that is in line with the
distinction between "free" and "clear" discussed on this thread, the
comment for this function needs to be fixed to "Clear the string
list.  See $doc". and that is not much different from "See $doc"
without the first sentence which is the function name.

Perhaps the API doc that currently says "Free" is the only thing
that needs fixing?  And perhaps add "See $doc" at the beginning of
the header and remove duplicated comments we already have in the
file?

Re: [PATCH] document string_list_clear

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Also, I forgot to mention: if we consistently put the API docs into the
> header files and extracted it automatically into standalone documents,
> we would not need to have two places.
>
> This is something I've been meaning to look at for a long time, but it
> never quite makes the priority list. And my past experiences with tools
> like doxygen has been that they are complicated and have a lot of
> dependencies. It's been a long time since I've tried, though.

Yeah, that is a thought.

Some existing documentation pages like allocation-growing may not be
a good match for this strategy, though; cache.h has a lot more than
just alloc-grow, and there needs to be a way to generate more than
one API docs from a single header (and vice versa, taking text from
more than one source for a single API, I suspect).

Re: [PATCH] document string_list_clear

[Stefan Beller]

On Tue, Dec 9, 2014 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Perhaps the API doc that currently says "Free" is the only thing
> that needs fixing?  And perhaps add "See $doc" at the beginning of
> the header and remove duplicated comments we already have in the
> file?

The reason I wrote this patch originally was because I seem to forget we have
more than one place to document our APIs. If there are comments in the header
I seem to have thought it were the only place where we have documentation.
But as we have some comments in the header for about half the functions there,
I missed looking into the Documentation at all.

I'd support Jeffs efforts to only write doc into the headers and then
export it or just
have documentation in the Documentation/technical dir. If there is
only one way to obtain
the information I wouldn't send in patches duplicating information.

Personally I first visit the header files to see if there is
documentation/comments on the
desired function. Looking at Documentation/technical/ is only reserved
for high lievel discussion on
API choices, so everything not prefixed with "api-" there.

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>>> +/*
>>> + * Clears the string list, so it has zero items. All former items are freed.
>>> + * If free_util is true, all util pointers are also freed.
>>> + */
>>>  void string_list_clear(struct string_list *list, int free_util);
>>
>> The api doc says
>>
>> 	Free a string_list. The `string` pointer of the items will be freed in
>> 	case the `strdup_strings` member of the string_list is set. The second
>> 	parameter controls if the `util` pointer of the items should be freed
>> 	or not.
>>
>> One option here would be to say
>>
>> 	Free a string_list.  See Documentation/technical/api-string-list.txt
>> 	for details.
>
> If we later introduce string_list_free() that is in line with the
> distinction between "free" and "clear" discussed on this thread, the
> comment for this function needs to be fixed to "Clear the string
> list.  See $doc". and that is not much different from "See $doc"
> without the first sentence which is the function name.

I still find the term "clear" to be confusing here.  It makes me think
the function will be analagous to strbuf_reset, when it's actually
analagous to strbuf_release.

In other words, the important thing is that this frees all members of
the string_list.  There might be a clearer way to say that.  The
string_list itself may not be dynamically allocated --- I'm not sure
what it would mean to free it.

How about string_list_release?

I think I could get used to "clear" if we used it consistently though.
I suspect anything we do will be confusing, unless we make them
consistent.

> Perhaps the API doc that currently says "Free" is the only thing
> that needs fixing?

I find the API doc to be pretty clear, actually.  If someone was
confused in practice then I'd be happy to try to debug the wording.

>                     And perhaps add "See $doc" at the beginning of
> the header and remove duplicated comments we already have in the
> file?

Yes, that sounds like a good way to go about it.

Thanks,
Jonathan

Re: [PATCH] document string_list_clear

[Jeff King]

On Tue, Dec 09, 2014 at 11:41:44AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Also, I forgot to mention: if we consistently put the API docs into the
> > header files and extracted it automatically into standalone documents,
> > we would not need to have two places.
> >
> > This is something I've been meaning to look at for a long time, but it
> > never quite makes the priority list. And my past experiences with tools
> > like doxygen has been that they are complicated and have a lot of
> > dependencies. It's been a long time since I've tried, though.
> 
> Yeah, that is a thought.
> 
> Some existing documentation pages like allocation-growing may not be
> a good match for this strategy, though; cache.h has a lot more than
> just alloc-grow, and there needs to be a way to generate more than
> one API docs from a single header (and vice versa, taking text from
> more than one source for a single API, I suspect).

I think that would be a matter of tool support for saying "now I am
outputting api-foo" in the inline documentation[1]. It does make writing
Makefiles a lot harder, though, if there isn't a one-to-one
correspondence between targets and their sources. Perhaps it is a sign
that we should split our include files more along functional boundaries.
If we cannot make sane documentation out of the file, then it is
probably confusing for humans to read it.

Opening cache.h shows it starting with git_{in,de}flate_*, and then
DEFAULT_GIT_PORT!? It has long been a dumping ground for random global
functions and definitions. If we are going to document subsystems,
splitting them into their own files seems sensible. Skimming through the
file, some of the splits seem pretty easy and obvious.

-Peff

[1] I do not have a tool in mind, so this is something to think about
    while evaluating them. It seems like just parsing:

    /**
     * All of this will get dumped in a file (sans asterisks),
     * which can then be fed to asciidoc. The "**" marker
     * starts the docstring, which ends at the normal comment
     * boundary.
     */

    would be an easy place to start, and by implementing it ourselves
    with sed we would be free to implement any other rules we wanted.
    That is probably too naive, though. It would be nice if the tool
    actually understood C function declarations, so that we would not
    have to repeat them manually in the comments.

    I guess all tools like doxygen probably started off as "just pull
    out and format the comments", and then grew and grew in complexity. :)

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Stefan Beller wrote:
> On Tue, Dec 9, 2014 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:

>> Perhaps the API doc that currently says "Free" is the only thing
>> that needs fixing?  And perhaps add "See $doc" at the beginning of
>> the header and remove duplicated comments we already have in the
>> file?
>
> The reason I wrote this patch originally was because I seem to forget we have
> more than one place to document our APIs. If there are comments in the header
> I seem to have thought it were the only place where we have documentation.

How about this patch?

-- >8 --
Subject: put strbuf API documentation in one place

v1.8.1-rc0~61^2 (strbuf_split*(): document functions, 2012-11-04)
added some nice API documentation for a few functions to strbuf.h, to
complement the documentation at Documentation/technical/api-strbuf.
That was helpful because it meant one less hop for someone reading the
header to find API documentation.

In practice, unfortunately, it is too hard to remember that there
is documentation in two places.  The longer documentation comments
in the header made Documentation/technical/api-strbuf less
discoverable.  So move the information to
Documentation/technical/api-strbuf and drop the long comments.

Hopefully in the long term we will find a good way to
generate well organized Documentation/technical/api-* documents
from comments in headers and this problem will be eliminated
completely.

Short reminders in the header file are still okay.

Reported-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/technical/api-strbuf.txt | 20 +++++++++++++++--
 strbuf.h                               | 40 ++--------------------------------
 2 files changed, 20 insertions(+), 40 deletions(-)

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
index cca6543..a43facc 100644
--- a/Documentation/technical/api-strbuf.txt
+++ b/Documentation/technical/api-strbuf.txt
@@ -252,6 +252,11 @@ which can be used by the programmer of the callback as she sees fit.
 	destination. This is useful for literal data to be fed to either
 	strbuf_expand or to the *printf family of functions.
 
+`strbuf_addstr_xml_quoted`::
+
+	Append a string to a strbuf with the characters '<', '>', '&', and
+	'"' converted into XML entities.
+
 `strbuf_humanise_bytes`::
 
 	Append the given byte size as a human-readable string (i.e. 12.23 KiB,
@@ -266,6 +271,13 @@ which can be used by the programmer of the callback as she sees fit.
 	Add a formatted string prepended by a comment character and a
 	blank to the buffer.
 
+`xstrfmt`::
+`xstrvfmt`::
+
+	Create a newly allocated string using printf format.  You can do
+	this easily with a strbuf, but this provides a shortcut to save a
+	few lines.
+
 `strbuf_fread`::
 
 	Read a given size of data from a FILE* pointer to the buffer.
@@ -336,11 +348,15 @@ same behaviour as well.
 	terminator characters.  Some of these functions take a `max`
 	parameter, which, if positive, limits the output to that
 	number of substrings.
++
+For lighter-weight alternatives, see `string_list_split` and
+`string_list_split_in_place` from the
+link:api-string-list.html[string list API].
 
 `strbuf_list_free`::
 
-	Free a list of strbufs (for example, the return values of the
-	`strbuf_split()` functions).
+	Free a NULL-terminated list of strbufs (for example, the return
+	values of the `strbuf_split()` functions).
 
 `launch_editor`::
 
diff --git a/strbuf.h b/strbuf.h
index 652b6c4..f3c9bb6 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -58,56 +58,27 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 		return 0;
 }
 
-/*
- * Split str (of length slen) at the specified terminator character.
- * Return a null-terminated array of pointers to strbuf objects
- * holding the substrings.  The substrings include the terminator,
- * except for the last substring, which might be unterminated if the
- * original string did not end with a terminator.  If max is positive,
- * then split the string into at most max substrings (with the last
- * substring containing everything following the (max-1)th terminator
- * character).
- *
- * For lighter-weight alternatives, see string_list_split() and
- * string_list_split_in_place().
- */
-extern struct strbuf **strbuf_split_buf(const char *, size_t,
+extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,
 					int terminator, int max);
 
-/*
- * Split a NUL-terminated string at the specified terminator
- * character.  See strbuf_split_buf() for more information.
- */
 static inline struct strbuf **strbuf_split_str(const char *str,
 					       int terminator, int max)
 {
 	return strbuf_split_buf(str, strlen(str), terminator, max);
 }
 
-/*
- * Split a strbuf at the specified terminator character.  See
- * strbuf_split_buf() for more information.
- */
 static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,
 						int terminator, int max)
 {
 	return strbuf_split_buf(sb->buf, sb->len, terminator, max);
 }
 
-/*
- * Split a strbuf at the specified terminator character.  See
- * strbuf_split_buf() for more information.
- */
 static inline struct strbuf **strbuf_split(const struct strbuf *sb,
 					   int terminator)
 {
 	return strbuf_split_max(sb, terminator, 0);
 }
 
-/*
- * Free a NULL-terminated list of strbufs (for example, the return
- * values of the strbuf_split*() functions).
- */
 extern void strbuf_list_free(struct strbuf **);
 
 /*----- add data in your buffer -----*/
@@ -158,10 +129,7 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);
 
-/*
- * Append s to sb, with the characters '<', '>', '&' and '"' converted
- * into XML entities.
- */
+/* append s with <, >, &, and " converted to XML entities */
 extern void strbuf_addstr_xml_quoted(struct strbuf *sb, const char *s);
 
 static inline void strbuf_complete_line(struct strbuf *sb)
@@ -200,10 +168,6 @@ extern int fprintf_ln(FILE *fp, const char *fmt, ...);
 
 char *xstrdup_tolower(const char *);
 
-/*
- * Create a newly allocated string using printf format. You can do this easily
- * with a strbuf, but this provides a shortcut to save a few lines.
- */
 __attribute__((format (printf, 1, 0)))
 char *xstrvfmt(const char *fmt, va_list ap);
 __attribute__((format (printf, 1, 2)))
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Jeff King wrote:
> On Tue, Dec 09, 2014 at 11:41:44AM -0800, Junio C Hamano wrote:

>> Yeah, that is a thought.
>>
>> Some existing documentation pages like allocation-growing may not be
>> a good match for this strategy, though; cache.h has a lot more than
>> just alloc-grow, and there needs to be a way to generate more than
>> one API docs from a single header (and vice versa, taking text from
>> more than one source for a single API, I suspect).
>
> I think that would be a matter of tool support for saying "now I am
> outputting api-foo" in the inline documentation[1]. It does make writing
> Makefiles a lot harder, though, if there isn't a one-to-one
> correspondence between targets and their sources. Perhaps it is a sign
> that we should split our include files more along functional boundaries.

Yeah, I think a separate alloc-grow.h would be fine.

Thanks,
Jonathan

Re: [PATCH] document string_list_clear

[Jeff King]

On Tue, Dec 09, 2014 at 12:17:13PM -0800, Jonathan Nieder wrote:

> Stefan Beller wrote:
> > On Tue, Dec 9, 2014 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> >> Perhaps the API doc that currently says "Free" is the only thing
> >> that needs fixing?  And perhaps add "See $doc" at the beginning of
> >> the header and remove duplicated comments we already have in the
> >> file?
> >
> > The reason I wrote this patch originally was because I seem to forget we have
> > more than one place to document our APIs. If there are comments in the header
> > I seem to have thought it were the only place where we have documentation.
> 
> How about this patch?
> 
> -- >8 --
> Subject: put strbuf API documentation in one place
> 
> v1.8.1-rc0~61^2 (strbuf_split*(): document functions, 2012-11-04)
> added some nice API documentation for a few functions to strbuf.h, to
> complement the documentation at Documentation/technical/api-strbuf.
> That was helpful because it meant one less hop for someone reading the
> header to find API documentation.
> 
> In practice, unfortunately, it is too hard to remember that there
> is documentation in two places.  The longer documentation comments
> in the header made Documentation/technical/api-strbuf less
> discoverable.  So move the information to
> Documentation/technical/api-strbuf and drop the long comments.
> 
> Hopefully in the long term we will find a good way to
> generate well organized Documentation/technical/api-* documents
> from comments in headers and this problem will be eliminated
> completely.
> 
> Short reminders in the header file are still okay.

I somewhat feel this goes in the opposite direction of where we want to
be (all data in one place, but that place is the header). Your patch
might make api-strbuf more discoverable, but it also vastly increases
the chances of function getting out of sync with their documentation, or
new functions being added without getting documented (very often the
presence of other documentation in the comments is enough to guilt me
into writing it for new ones).

Elsewhere I mentioned a tool to extract comments and format them. But do
people actually care about the formatting step? Does anybody asciidoc
the technical/api-* files? We did not even support building them until
sometime in 2012. Personally, I only ever view them as text.

In which case can we simply start migrating api-strbuf.txt into
in-header comments, without worrying about a parsing tool?

-Peff

Re: [PATCH] document string_list_clear

[Stefan Beller]

On Tue, Dec 9, 2014 at 12:27 PM, Jeff King <peff@peff.net> wrote:

>
> In which case can we simply start migrating api-strbuf.txt into
> in-header comments, without worrying about a parsing tool?
>
> -Peff

That would be my preference as well. Putting the API documentation
into the header files. Optionally if you're fancy you may enjoy the generated
API documentation. But I personally would rather look into the header for
the most up to date information.

As said above the Documentation/technical would only serve a purpose to
explain very high level concepts. So racy-git.txt or
pack-heuristics.txt will fit
in there very good.

Re: [PATCH] document string_list_clear

[Jeff King]

On Tue, Dec 09, 2014 at 12:32:15PM -0800, Stefan Beller wrote:

> As said above the Documentation/technical would only serve a purpose to
> explain very high level concepts. So racy-git.txt or
> pack-heuristics.txt will fit
> in there very good.

Yeah, I think we would leave anything that isn't api-*, with the goal
that all of api-* would eventually migrate into headers (it does not all
have to happen on day one, of course). There are a few spots which do
refer to api-credential, which is where you can find information on the
credential protocol. But that could be split out into a separate
technical document (which is probably an improvement, anyway; people who
are writing a helper do not know or care about our internal C API; they
only care about the protocol).

-Peff

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Jeff King wrote:

> Elsewhere I mentioned a tool to extract comments and format them. But do
> people actually care about the formatting step?

If formatting means "convert to a straightforward text document that I
can read through", then I do.

>                                                 Does anybody asciidoc
> the technical/api-* files? We did not even support building them until
> sometime in 2012. Personally, I only ever view them as text.

I also view them as text.  I tend to find it easier to read the
technical/api-* files than headers.  That might be for the same reason
that some other people prefer header files --- a
Documentation/technical/ file tends to be written in one chunk
together and ends up saying exactly what I need to know about the
calling sequence in a coherent way, while header files tend to evolve
over time to match the implementation without maintaining that
organization and usability.

I suspect a simple tool to extract the comments and produce something
like the technical/api-* files would help, since then when someone
writes a patch, they could try the tool and see if the result is still
easy to read.

Anyway, the patch I wrote was an attempt at a minimal fix (and an
attempt at getting out of a conversation that seemed to be moving
toward bikeshedding).  If someone wants to move the documentation from
api-strbuf.txt to strbuf.h, I won't object, since I hope that a simple
tool like described above isn't too far away.

Thanks,
Jonathan

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Jonathan Nieder wrote:
> Stefan Beller wrote:
>> On Tue, Dec 9, 2014 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:

>>> Perhaps the API doc that currently says "Free" is the only thing
>>> that needs fixing?  And perhaps add "See $doc" at the beginning of
>>> the header and remove duplicated comments we already have in the
>>> file?
>>
>> The reason I wrote this patch originally was because I seem to forget we have
>> more than one place to document our APIs. If there are comments in the header
>> I seem to have thought it were the only place where we have documentation.
>
> How about this patch?

And another:

-- >8--
Subject: put string-list API documentation in one place

Until recently (v1.8.0-rc0~46^2~5, 2012-09-12), the string-list API
was documented in detail in Documentation/technical/api-string-list.txt
and the header file contained section markers and some short reminders
but little other documentation.

Since then, the header has acquired some more comments that are mostly
identical to the documentation from technical/.  In principle that
should help convenience, since it means one less hop for someone
reading the header to find API documentation.  In practice,
unfortunately, it is hard to remember that there is documentation in
two places, and the comprehensive documentation of some functions in
the header makes it too easy to forget that the other functions are
documented at all (and where).

Add a comment pointing to Documentation/technical/ and remove the
comments that duplicate what is written there.  Longer term, we may
want to move all of the technical docs to header files and generate
printer-ready API documentation another way, but that is a larger
change for another day.

Short reminders in the header file are still okay.

Reported-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/technical/api-string-list.txt | 12 ++++++-
 string-list.h                               | 56 ++---------------------------
 2 files changed, 14 insertions(+), 54 deletions(-)

diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
index d51a657..a85e713 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -185,7 +185,17 @@ overwriting the delimiter characters with NULs and creating new
 string_list_items that point into the original string (the original
 string must therefore not be modified or freed while the `string_list`
 is in use).
-
++
+Examples:
++
+----
+string_list_split(l, "foo:bar:baz", ':', -1) -> ["foo", "bar", "baz"]
+string_list_split(l, "foo:bar:baz", ':', 0) -> ["foo:bar:baz"]
+string_list_split(l, "foo:bar:baz", ':', 1) -> ["foo", "bar:baz"]
+string_list_split(l, "foo:bar:", ':', -1) -> ["foo", "bar", ""]
+string_list_split(l, "", ':', -1) -> [""]
+string_list_split(l, ":", ':', -1) -> ["", ""]
+----
 
 Data structures
 ---------------
diff --git a/string-list.h b/string-list.h
index 494eb5d..e6a7841 100644
--- a/string-list.h
+++ b/string-list.h
@@ -1,6 +1,8 @@
 #ifndef STRING_LIST_H
 #define STRING_LIST_H
 
+/* See Documentation/technical/api-string-list.txt */
+
 struct string_list_item {
 	char *string;
 	void *util;
@@ -35,20 +37,9 @@ int for_each_string_list(struct string_list *list,
 #define for_each_string_list_item(item,list) \
 	for (item = (list)->items; item < (list)->items + (list)->nr; ++item)
 
-/*
- * Apply want to each item in list, retaining only the ones for which
- * the function returns true.  If free_util is true, call free() on
- * the util members of any items that have to be deleted.  Preserve
- * the order of the items that are retained.
- */
 void filter_string_list(struct string_list *list, int free_util,
 			string_list_each_func_t want, void *cb_data);
 
-/*
- * Remove any empty strings from the list.  If free_util is true, call
- * free() on the util members of any items that have to be deleted.
- * Preserve the order of the items that are retained.
- */
 void string_list_remove_empty_items(struct string_list *list, int free_util);
 
 /* Use these functions only on sorted lists: */
@@ -59,30 +50,12 @@ struct string_list_item *string_list_insert(struct string_list *list, const char
 struct string_list_item *string_list_insert_at_index(struct string_list *list,
 						     int insert_at, const char *string);
 struct string_list_item *string_list_lookup(struct string_list *list, const char *string);
-
-/*
- * Remove all but the first of consecutive entries with the same
- * string value.  If free_util is true, call free() on the util
- * members of any items that have to be deleted.
- */
 void string_list_remove_duplicates(struct string_list *sorted_list, int free_util);
 
 
 /* Use these functions only on unsorted lists: */
 
-/*
- * Add string to the end of list.  If list->strdup_string is set, then
- * string is copied; otherwise the new string_list_entry refers to the
- * input string.
- */
 struct string_list_item *string_list_append(struct string_list *list, const char *string);
-
-/*
- * Like string_list_append(), except string is never copied.  When
- * list->strdup_strings is set, this function can be used to hand
- * ownership of a malloc()ed string to list without making an extra
- * copy.
- */
 struct string_list_item *string_list_append_nodup(struct string_list *list, char *string);
 
 void sort_string_list(struct string_list *list);
@@ -92,32 +65,9 @@ struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
 
 void unsorted_string_list_delete_item(struct string_list *list, int i, int free_util);
 
-/*
- * Split string into substrings on character delim and append the
- * substrings to list.  The input string is not modified.
- * list->strdup_strings must be set, as new memory needs to be
- * allocated to hold the substrings.  If maxsplit is non-negative,
- * then split at most maxsplit times.  Return the number of substrings
- * appended to list.
- *
- * Examples:
- *   string_list_split(l, "foo:bar:baz", ':', -1) -> ["foo", "bar", "baz"]
- *   string_list_split(l, "foo:bar:baz", ':', 0) -> ["foo:bar:baz"]
- *   string_list_split(l, "foo:bar:baz", ':', 1) -> ["foo", "bar:baz"]
- *   string_list_split(l, "foo:bar:", ':', -1) -> ["foo", "bar", ""]
- *   string_list_split(l, "", ':', -1) -> [""]
- *   string_list_split(l, ":", ':', -1) -> ["", ""]
- */
 int string_list_split(struct string_list *list, const char *string,
 		      int delim, int maxsplit);
-
-/*
- * Like string_list_split(), except that string is split in-place: the
- * delimiter characters in string are overwritten with NULs, and the
- * new string_list_items point into string (which therefore must not
- * be modified or freed while the string_list is in use).
- * list->strdup_strings must *not* be set.
- */
 int string_list_split_in_place(struct string_list *list, char *string,
 			       int delim, int maxsplit);
+
 #endif /* STRING_LIST_H */
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH] document string_list_clear

[Stefan Beller]

On Tue, Dec 09, 2014 at 02:49:50PM -0800, Jonathan Nieder wrote:
> Jonathan Nieder wrote:
> > Stefan Beller wrote:
> >> On Tue, Dec 9, 2014 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> >>> Perhaps the API doc that currently says "Free" is the only thing
> >>> that needs fixing?  And perhaps add "See $doc" at the beginning of
> >>> the header and remove duplicated comments we already have in the
> >>> file?
> >>
> >> The reason I wrote this patch originally was because I seem to forget we have
> >> more than one place to document our APIs. If there are comments in the header
> >> I seem to have thought it were the only place where we have documentation.
> >
> > How about this patch?
> 
> And another:

this goes on top or replaces the previous one? (This applies cleanly on origin/master) 

> -- >8--
> Subject: put string-list API documentation in one place
> 
> Until recently (v1.8.0-rc0~46^2~5, 2012-09-12), the string-list API
> was documented in detail in Documentation/technical/api-string-list.txt
> and the header file contained section markers and some short reminders
> but little other documentation.
> 
> Since then, the header has acquired some more comments that are mostly
> identical to the documentation from technical/.  In principle that
> should help convenience, since it means one less hop for someone
> reading the header to find API documentation.  In practice,
> unfortunately, it is hard to remember that there is documentation in
> two places, and the comprehensive documentation of some functions in
> the header makes it too easy to forget that the other functions are
> documented at all (and where).
> 
> Add a comment pointing to Documentation/technical/ and remove the
> comments that duplicate what is written there.  Longer term, we may
> want to move all of the technical docs to header files and generate
> printer-ready API documentation another way, but that is a larger
> change for another day.
> 
> Short reminders in the header file are still okay.
> 
> Reported-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---

Thanks a lot for this patch. It's improving the situation a lot.
No information is lost albeit many deleted lines, have a 

Reviewed-by: Stefan Beller <sbeller@google.com>

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Stefan Beller wrote:
> On Tue, Dec 09, 2014 at 02:49:50PM -0800, Jonathan Nieder wrote:

>> And another:
>
> this goes on top or replaces the previous one? (This applies cleanly on origin/master) 

They're independent. :)

[...]
>> Reported-by: Stefan Beller <sbeller@google.com>
>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>> ---
>
> Thanks a lot for this patch. It's improving the situation a lot.
> No information is lost albeit many deleted lines, have a
>
> Reviewed-by: Stefan Beller <sbeller@google.com>

Thanks for looking it over.

Re: [PATCH] document string_list_clear

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jeff King wrote:
>
>> Elsewhere I mentioned a tool to extract comments and format them. But do
>> people actually care about the formatting step?
>
> If formatting means "convert to a straightforward text document that I
> can read through", then I do.

If the result becomes unparsable by AsciiDoc(or), those who
AsciiDoc'ified the api-*.txt may feel unhappy.  I however strongly
suspect that the primary consumer of these api-*.txt documents are
consuming the text version, not AsciiDoc-to-html output.

>>                                                 Does anybody asciidoc
>> the technical/api-* files? We did not even support building them until
>> sometime in 2012. Personally, I only ever view them as text.
>
> I also view them as text.  I tend to find it easier to read the
> technical/api-* files than headers.  That might be for the same reason
> that some other people prefer header files --- a
> Documentation/technical/ file tends to be written in one chunk
> together and ends up saying exactly what I need to know about the
> calling sequence in a coherent way, while header files tend to evolve
> over time to match the implementation without maintaining that
> organization and usability.

The documentation that was written with an explicit purpose to serve
as documentation would explain how each pieces fit in the whole
system much better than a list of pieces extracted from per-function
comments, unless the header comment that serves as the source of
generated document is written carefully.  But as people pointed out,
having the same set of information in two places in sync forces us
to be extra careful in a different way.  There is no free lunch.

We would want (1) documentation in one place, (2) easy access to it
when deciding which API function to call with what information, and
(3) make it hard to forget updating the documentation when making
changes to the API.  If that single source of truth is the header
file, perhaps we would become more careful commenting our headers
(i.e. "do not just comment each functions and structures, but
enhance introductory part that describes how things fit together"),
so I think it would be OK in the end.  We may need to reorganize and
reorder the headers so that associated comments to functions and
data types match desirable presentation order, but that effort will
kill two birds with one stone.  The resulting header would also be
easier to read by humans.

I am a bit hesitant to see us spending extra cycles either to
reinvent doxygen (if we do our own) or working around quirks in
third-party tools (if we decide to use existing ones).

Thanks.

Re: [PATCH] document string_list_clear

[Jeff King]

On Tue, Dec 09, 2014 at 02:23:37PM -0800, Jonathan Nieder wrote:

> Jeff King wrote:
> 
> > Elsewhere I mentioned a tool to extract comments and format them. But do
> > people actually care about the formatting step?
> 
> If formatting means "convert to a straightforward text document that I
> can read through", then I do.

I meant "run asciidoc in api-*". I am confused by your response. Here it
implies to me that you asciidoc them into a text format to make it
easier to read. But below you say "I also view [the api-* files] as
text", which implies you just look at them in your editor.

> >                                                 Does anybody asciidoc
> > the technical/api-* files? We did not even support building them until
> > sometime in 2012. Personally, I only ever view them as text.
> 
> I also view them as text.  I tend to find it easier to read the
> technical/api-* files than headers.  That might be for the same reason
> that some other people prefer header files --- a
> Documentation/technical/ file tends to be written in one chunk
> together and ends up saying exactly what I need to know about the
> calling sequence in a coherent way, while header files tend to evolve
> over time to match the implementation without maintaining that
> organization and usability.

We could probably do a better job of keeping our header files neat and
well-ordered. And perhaps would so if they had a coherent narrative in
the first place.

My feeling is that it is at least as much work to keep an api-*.txt file
neat as it would be to keep the header file neat (generally more,
because of the duplication of effort). The one exception might be that
ordering in the header file may require some forward declarations. I'd
guess that to be pretty rare, though (and certainly I did not run into
it when converting the strbuf documentation below).

> I suspect a simple tool to extract the comments and produce something
> like the technical/api-* files would help, since then when someone
> writes a patch, they could try the tool and see if the result is still
> easy to read.

That seems like wishful thinking to me. Some subset of developers will
be happy reading the documentation in the header file, and will not
commonly run the tool. Therefore they will also not bother to examine
the output of the tool when making a change (either because they forget,
or because it is simply a hassle that they do not care about).

> Anyway, the patch I wrote was an attempt at a minimal fix (and an
> attempt at getting out of a conversation that seemed to be moving
> toward bikeshedding).  If someone wants to move the documentation from
> api-strbuf.txt to strbuf.h, I won't object, since I hope that a simple
> tool like described above isn't too far away.

I do not mind a partial or minimal fix. This conversion is tedious, and
it does not have to be done all at once. But IMHO, your patch is
actively making things worse. The api-* files have slowly grown out of
date over the years, and I believe that is because they are too far
removed from the code people are actually touching. I've been making an
active effort recently to document function interfaces in comments, and
I feel like the presence of other comments has encouraged me to do so.

Here's strbuf.h, with the data from api-strbuf.txt moved into it. I
followed the ordering of api-strbuf.txt, since that has a more coherent
grouping of functions (but it does make the latter parts of the diff
messier, as we have to move function declarations around). I left the
bottom part of strbuf.h, where there are several undocumented functions,
largely untouched. As further work, it would make sense to document
those functions and also fit them into the right spot amidst the
already-grouped functions.

This has almost every bit of text from api-strbuf.txt. I omitted a few
descriptions where the documentation already in strbuf.h was actually
_better_. I did remove the function names, sine they are redundant with
the declarations below. E.g., rather than:

  /**
    * `foo`::
    *         Do something.
    */
  void foo(int x);

just:

  /**
    * Do something.
    */
  void foo(int x);

The declaration is nearby and has strictly more information, and the
less boilerplate the better for convincing people to actually write
something. An extraction tool can pretty-print the function name if it
wants.

I did drop some asciidoc-specific formatting from the main text (e.g.,
"+" lines for connecting blocks), which means that extracting the result
and running it through asciidoc won't work. I think it makes the actual
text much more readable, though. But we could do it the other way if
people really want to asciidoc it.

---
diff --git a/strbuf.h b/strbuf.h
index 652b6c4..6143680 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,22 +1,105 @@
 #ifndef STRBUF_H
 #define STRBUF_H
 
-/* See Documentation/technical/api-strbuf.txt */
-
-extern char strbuf_slopbuf[];
+/**
+ * strbuf's are meant to be used with all the usual C string and memory
+ * APIs. Given that the length of the buffer is known, it's often better to
+ * use the mem* functions than a str* one (memchr vs. strchr e.g.).
+ * Though, one has to be careful about the fact that str* functions often
+ * stop on NULs and that strbufs may have embedded NULs.
+ *
+ * A strbuf is NUL terminated for convenience, but no function in the
+ * strbuf API actually relies on the string being free of NULs.
+ *
+ * strbufs have some invariants that are very important to keep in mind:
+ *
+ * . The `buf` member is never NULL, so it can be used in any usual C
+ * string operations safely. strbuf's _have_ to be initialized either by
+ * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.
+ *
+ * Do *not* assume anything on what `buf` really is (e.g. if it is
+ * allocated memory or not), use `strbuf_detach()` to unwrap a memory
+ * buffer from its strbuf shell in a safe way. That is the sole supported
+ * way. This will give you a malloced buffer that you can later `free()`.
+ *
+ * However, it is totally safe to modify anything in the string pointed by
+ * the `buf` member, between the indices `0` and `len-1` (inclusive).
+ *
+ * . The `buf` member is a byte array that has at least `len + 1` bytes
+ *   allocated. The extra byte is used to store a `'\0'`, allowing the
+ *   `buf` member to be a valid C-string. Every strbuf function ensure this
+ *   invariant is preserved.
+ *
+ * NOTE: It is OK to "play" with the buffer directly if you work it this
+ *       way:
+ *
+ * ----
+ * strbuf_grow(sb, SOME_SIZE); <1>
+ * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);
+ * ----
+ * <1> Here, the memory array starting at `sb->buf`, and of length
+ * `strbuf_avail(sb)` is all yours, and you can be sure that
+ * `strbuf_avail(sb)` is at least `SOME_SIZE`.
+ *
+ * NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.
+ *
+ * Doing so is safe, though if it has to be done in many places, adding the
+ * missing API to the strbuf module is the way to go.
+ *
+ * WARNING: Do _not_ assume that the area that is yours is of size `alloc
+ * - 1` even if it's true in the current implementation. Alloc is somehow a
+ * "private" member that should not be messed with. Use `strbuf_avail()`
+ * instead.
+*/
+
+/**
+ * This is the string buffer structure. The `len` member can be used to
+ * determine the current length of the string, and `buf` member provides
+ * access to the string itself.
+ */
 struct strbuf {
 	size_t alloc;
 	size_t len;
 	char *buf;
 };
 
+extern char strbuf_slopbuf[];
 #define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
 
-/*----- strbuf life cycle -----*/
+/** strbuf lifecycle */
+
+/**
+ *	Initialize the structure. The second parameter can be zero or a bigger
+ *	number to allocate memory, in case you want to prevent further reallocs.
+ */
 extern void strbuf_init(struct strbuf *, size_t);
+
+/**
+ *	Release a string buffer and the memory it used. You should not use the
+ *	string buffer after using this function, unless you initialize it again.
+ */
 extern void strbuf_release(struct strbuf *);
+
+/**
+ * Detach the string from the strbuf and returns it; you now own the
+ * storage the string occupies and it is your responsibility from then on
+ * to release it with `free(3)` when you are done with it.
+ */
 extern char *strbuf_detach(struct strbuf *, size_t *);
+
+/**
+ * Attach a string to a buffer. You should specify the string to attach,
+ * the current length of the string and the amount of allocated memory.
+ * The amount must be larger than the string length, because the string you
+ * pass is supposed to be a NUL-terminated string.  This string _must_ be
+ * malloc()ed, and after attaching, the pointer cannot be relied upon
+ * anymore, and neither be free()d directly.
+ */
 extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);
+
+/**
+ * Swap the contents of two string buffers.
+ */
 static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)
 {
 	struct strbuf tmp = *a;
@@ -24,14 +107,33 @@ static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)
 	*b = tmp;
 }
 
-/*----- strbuf size related -----*/
+
+/** Related to the size of the buffer */
+
+/**
+ * Determine the amount of allocated but unused memory.
+ */
 static inline size_t strbuf_avail(const struct strbuf *sb)
 {
 	return sb->alloc ? sb->alloc - sb->len - 1 : 0;
 }
 
+/**
+ * Ensure that at least this amount of unused memory is available after
+ * `len`. This is used when you know a typical size for what you will add
+ * and want to avoid repetitive automatic resizing of the underlying buffer.
+ * This is never a needed operation, but can be critical for performance in
+ * some cases.
+ */
 extern void strbuf_grow(struct strbuf *, size_t);
 
+/**
+ * Set the length of the buffer to a given value. This function does *not*
+ * allocate new memory, so you should not perform a `strbuf_setlen()` to a
+ * length that is larger than `len + strbuf_avail()`. `strbuf_setlen()` is
+ * just meant as a 'please fix invariants from this strbuf I just messed
+ * with'.
+ */
 static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 {
 	if (len > (sb->alloc ? sb->alloc - 1 : 0))
@@ -39,16 +141,279 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 	sb->len = len;
 	sb->buf[len] = '\0';
 }
+
+/**
+ * Empty the buffer by setting the size of it to zero.
+ */
 #define strbuf_reset(sb)  strbuf_setlen(sb, 0)
 
-/*----- content related -----*/
+
+/** Related to the contents of the buffer */
+
+/**
+ * Strip whitespace from the beginning and end of a string.
+ * Equivalent to performing `strbuf_rtrim()` followed by `strbuf_ltrim()`.
+ */
 extern void strbuf_trim(struct strbuf *);
+
+/**
+ * Strip whitespace from the end of a string.
+ */
 extern void strbuf_rtrim(struct strbuf *);
+
+/**
+ * Strip whitespace from the beginning of a string.
+ */
 extern void strbuf_ltrim(struct strbuf *);
+
+/**
+ * Replace the contents of the strbuf with a reencoded form.  Returns -1
+ * on error, 0 on success.
+ */
 extern int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);
+
+/**
+ * Lowercase each character in the buffer using `tolower`.
+ */
 extern void strbuf_tolower(struct strbuf *sb);
+
+/**
+ * Compare two buffers. Returns an integer less than, equal to, or greater
+ * than zero if the first buffer is found, respectively, to be less than,
+ * to match, or be greater than the second buffer.
+ */
 extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
 
+
+/** Adding data to the buffer
+ *
+ * NOTE: All of the functions in this section will grow the buffer as
+ * necessary.  If they fail for some reason other than memory shortage and the
+ * buffer hadn't been allocated before (i.e. the `struct strbuf` was set to
+ * `STRBUF_INIT`), then they will free() it.
+ */
+
+/**
+ * Add a single character to the buffer.
+ */
+static inline void strbuf_addch(struct strbuf *sb, int c)
+{
+	strbuf_grow(sb, 1);
+	sb->buf[sb->len++] = c;
+	sb->buf[sb->len] = '\0';
+}
+
+/**
+ * Add a character the specified number of times to the buffer.
+ */
+extern void strbuf_addchars(struct strbuf *sb, int c, size_t n);
+
+/**
+ * Insert data to the given position of the buffer. The remaining contents
+ * will be shifted, not overwritten.
+ */
+extern void strbuf_insert(struct strbuf *, size_t pos, const void *, size_t);
+
+/**
+ * Remove given amount of data from a given position of the buffer.
+ */
+extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);
+
+/**
+ * Remove the bytes between `pos..pos+len` and replace it with the given
+ * data.
+ */
+extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,
+			  const void *, size_t);
+
+/**
+ * Add a NUL-terminated string to the buffer. Each line will be prepended
+ * by a comment character and a blank.
+ */
+extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);
+
+
+/**
+ * Add data of given length to the buffer.
+ */
+extern void strbuf_add(struct strbuf *, const void *, size_t);
+
+/**
+ * Add a NUL-terminated string to the buffer.
+ *
+ * NOTE: This function will *always* be implemented as an inline or a macro
+ * using strlen, meaning that this is efficient to write things like:
+ *
+ *     strbuf_addstr(sb, "immediate string");
+ */
+static inline void strbuf_addstr(struct strbuf *sb, const char *s)
+{
+	strbuf_add(sb, s, strlen(s));
+}
+
+/**
+ * Copy the contents of another buffer at the end of the current one.
+ */
+static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
+{
+	strbuf_grow(sb, sb2->len);
+	strbuf_add(sb, sb2->buf, sb2->len);
+}
+
+/**
+ * Copy part of the buffer from a given position till a given length to the
+ * end of the buffer.
+ */
+extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);
+
+/**
+ * This function can be used to expand a format string containing
+ * placeholders. To that end, it parses the string and calls the specified
+ * function for every percent sign found.
+ *
+ * The callback function is given a pointer to the character after the `%`
+ * and a pointer to the struct strbuf.  It is expected to add the expanded
+ * version of the placeholder to the strbuf, e.g. to add a newline
+ * character if the letter `n` appears after a `%`.  The function returns
+ * the length of the placeholder recognized and `strbuf_expand()` skips
+ * over it.
+ *
+ * The format `%%` is automatically expanded to a single `%` as a quoting
+ * mechanism; callers do not need to handle the `%` placeholder themselves,
+ * and the callback function will not be invoked for this placeholder.
+ *
+ * All other characters (non-percent and not skipped ones) are copied
+ * verbatim to the strbuf.  If the callback returned zero, meaning that the
+ * placeholder is unknown, then the percent sign is copied, too.
+ *
+ * In order to facilitate caching and to make it possible to give
+ * parameters to the callback, `strbuf_expand()` passes a context pointer,
+ * which can be used by the programmer of the callback as she sees fit.
+ */
+typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);
+extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);
+
+/**
+ * Used as callback for `strbuf_expand()`, expects an array of
+ * struct strbuf_expand_dict_entry as context, i.e. pairs of
+ * placeholder and replacement string.  The array needs to be
+ * terminated by an entry with placeholder set to NULL.
+ */
+struct strbuf_expand_dict_entry {
+	const char *placeholder;
+	const char *value;
+};
+extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context);
+
+/**
+ * Append the contents of one strbuf to another, quoting any
+ * percent signs ("%") into double-percents ("%%") in the
+ * destination. This is useful for literal data to be fed to either
+ * strbuf_expand or to the *printf family of functions.
+ */
+extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);
+
+/**
+ * Append the given byte size as a human-readable string (i.e. 12.23 KiB,
+ * 3.50 MiB).
+ */
+extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);
+
+/**
+ * Add a formatted string to the buffer.
+ */
+__attribute__((format (printf,2,3)))
+extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+
+/**
+ * Add a formatted string prepended by a comment character and a
+ * blank to the buffer.
+ */
+__attribute__((format (printf, 2, 3)))
+extern void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);
+
+__attribute__((format (printf,2,0)))
+extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
+
+/**
+ * Read a given size of data from a FILE* pointer to the buffer.
+ *
+ * NOTE: The buffer is rewound if the read fails. If -1 is returned,
+ * `errno` must be consulted, like you would do for `read(3)`.
+ * `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline()` has the
+ * same behaviour as well.
+ */
+extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
+
+/**
+ * Read the contents of a given file descriptor. The third argument can be
+ * used to give a hint about the file size, to avoid reallocs.  If read fails,
+ * any partial read is undone */
+extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
+
+/**
+ * Read the contents of a file, specified by its path. The third argument
+ * can be used to give a hint about the file size, to avoid reallocs.
+ */
+extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
+
+/**
+ * Read the target of a symbolic link, specified by its path.  The third
+ * argument can be used to give a hint about the size, to avoid reallocs.
+ */
+extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
+
+/**
+ * Set the buffer to the path of the current working directory.
+ */
+extern int strbuf_getcwd(struct strbuf *sb);
+
+/**
+ * Read a line from a FILE *, overwriting the existing contents
+ * of the strbuf. The second argument specifies the line
+ * terminator character, typically `'\n'`.
+ * Reading stops after the terminator or at EOF.  The terminator
+ * is removed from the buffer before returning.  Returns 0 unless
+ * there was nothing left before EOF, in which case it returns `EOF`.
+ */
+extern int strbuf_getline(struct strbuf *, FILE *, int);
+
+/**
+ * Like `strbuf_getline`, but keeps the trailing terminator (if
+ * any) in the buffer.
+ */
+extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
+
+/**
+ * Like `strbuf_getwholeline`, but operates on a file descriptor.
+ * It reads one character at a time, so it is very slow.  Do not
+ * use it unless you need the correct position in the file
+ * descriptor.
+ */
+extern int strbuf_getwholeline_fd(struct strbuf *, int, int);
+
+/**
+ * Add a path to a buffer, converting a relative path to an
+ * absolute one in the process.  Symbolic links are not
+ * resolved.
+ */
+extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
+
+/**
+ * Strip whitespace from a buffer. The second parameter controls if
+ * comments are considered contents to be removed or not.
+ */
+extern void stripspace(struct strbuf *buf, int skip_comments);
+
+/**
+ * Launch the user preferred editor to edit a file and fill the buffer
+ * with the file's contents upon the user completing their editing. The
+ * third argument can be used to set the environment which the editor is
+ * run in. If the buffer is NULL the editor is launched as usual but the
+ * file's contents are not read into the buffer upon completion.
+ */
+extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
+
 static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 {
 	if (strip_suffix_mem(sb->buf, &sb->len, suffix)) {
@@ -110,52 +475,6 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
  */
 extern void strbuf_list_free(struct strbuf **);
 
-/*----- add data in your buffer -----*/
-static inline void strbuf_addch(struct strbuf *sb, int c)
-{
-	strbuf_grow(sb, 1);
-	sb->buf[sb->len++] = c;
-	sb->buf[sb->len] = '\0';
-}
-
-extern void strbuf_insert(struct strbuf *, size_t pos, const void *, size_t);
-extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);
-
-/* splice pos..pos+len with given data */
-extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,
-                          const void *, size_t);
-
-extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);
-
-extern void strbuf_add(struct strbuf *, const void *, size_t);
-static inline void strbuf_addstr(struct strbuf *sb, const char *s)
-{
-	strbuf_add(sb, s, strlen(s));
-}
-static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
-{
-	strbuf_grow(sb, sb2->len);
-	strbuf_add(sb, sb2->buf, sb2->len);
-}
-extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);
-extern void strbuf_addchars(struct strbuf *sb, int c, size_t n);
-
-typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);
-extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);
-struct strbuf_expand_dict_entry {
-	const char *placeholder;
-	const char *value;
-};
-extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context);
-extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);
-
-__attribute__((format (printf,2,3)))
-extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
-__attribute__((format (printf, 2, 3)))
-extern void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);
-__attribute__((format (printf,2,0)))
-extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
-
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);
 
 /*
@@ -170,28 +489,11 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 		strbuf_addch(sb, '\n');
 }
 
-extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
-/* XXX: if read fails, any partial read is undone */
-extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
-extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
-extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
-extern int strbuf_getcwd(struct strbuf *sb);
-
-extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
-extern int strbuf_getline(struct strbuf *, FILE *, int);
-extern int strbuf_getwholeline_fd(struct strbuf *, int, int);
-
-extern void stripspace(struct strbuf *buf, int skip_comments);
-extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
-
 extern int strbuf_branchname(struct strbuf *sb, const char *name);
 extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
 
 extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
 				    int reserved);
-extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);
-
-extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
 
 __attribute__((format (printf,1,2)))
 extern int printf_ln(const char *fmt, ...);

Re: [PATCH] document string_list_clear

[Jeff King]

On Tue, Dec 09, 2014 at 03:18:28PM -0800, Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> writes:
> 
> > Jeff King wrote:
> >
> >> Elsewhere I mentioned a tool to extract comments and format them. But do
> >> people actually care about the formatting step?
> >
> > If formatting means "convert to a straightforward text document that I
> > can read through", then I do.
> 
> If the result becomes unparsable by AsciiDoc(or), those who
> AsciiDoc'ified the api-*.txt may feel unhappy.  I however strongly
> suspect that the primary consumer of these api-*.txt documents are
> consuming the text version, not AsciiDoc-to-html output.

Yeah, that was my point. They were not even asciidoc'd for a long time,
then one contributor, who does not otherwise work on the code,
volunteered to asciidoc them. AFAIK there are no regular developers who
wanted this feature (but I do not know for sure, which is why I asked).

I do not _mind_ if they can be asciidoc'd, but if nobody does so, they
are bound to develop formatting errors. And matching asciidoc syntax
does come at a readability cost to the text.

> The documentation that was written with an explicit purpose to serve
> as documentation would explain how each pieces fit in the whole
> system much better than a list of pieces extracted from per-function
> comments, unless the header comment that serves as the source of
> generated document is written carefully.

If we split the header files sanely (i.e., to match what would be one
api-* file), then I had imagined each header file having a long
free-form comment at the top giving the overview, followed by commented
structures and functions, with possibly other free-form comments in the
middle as necessary. That's what I did for the strbuf.h conversion I
just sent.

> I am a bit hesitant to see us spending extra cycles either to
> reinvent doxygen (if we do our own) or working around quirks in
> third-party tools (if we decide to use existing ones).

Me too. That's what I hoped that we could come up with a form whose
in-header source is readable enough that people would use it. Then there
is really no tool necessary.

-Peff

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Jeff King wrote:
>> Jeff King wrote:

> We could probably do a better job of keeping our header files neat and
> well-ordered. And perhaps would so if they had a coherent narrative in
> the first place.

The example I was looking at before was refs.h.  It is still a mess.
Hopefully strbuf.h will work better.

[...]
> On Tue, Dec 09, 2014 at 02:23:37PM -0800, Jonathan Nieder wrote:

>> I suspect a simple tool to extract the comments and produce something
>> like the technical/api-* files would help, since then when someone
>> writes a patch, they could try the tool and see if the result is still
>> easy to read.
>
> That seems like wishful thinking to me. Some subset of developers will
> be happy reading the documentation in the header file, and will not
> commonly run the tool. Therefore they will also not bother to examine
> the output of the tool when making a change (either because they forget,
> or because it is simply a hassle that they do not care about).

That is okay as long as enough people extract documentation to ensure
the result is sane.

It is similar to what happens with Documentation/*.txt.  A subset of
developers just work with the source and make local changes without
looking at the big picture (and sometimes buggy markup or documentation
organized in a confusing way results).  But that doesn't change much.
When people forget to look at the manpage and write text that is
awkward in context or markup errors, one of a few things happens:

 * a reviewer notices.  The patch is fixed, and the patch author
   develops better habits for next time.

 * someone using the documentation notices later, and it gets fixed
   then, which is still fine.

Both outcomes are much, much better than documentation that never gets
organized.

[...]
>                               The api-* files have slowly grown out of
> date over the years, and I believe that is because they are too far
> removed from the code people are actually touching.

I don't think they ever were very comprehensive or up to date.

As far as I understand, the api-* files are intended to be usually out
of date.  Their main purpose is to explain the gist of how to use the
API.  They were usually written way after the API was introduced (so
they are behind already).  They are clearly written and as long as
they mostly match the code, that is enough for them to be useful and
educational.

Then every few years or so, someone updates the file with the latest
API changes, still with an eye to overall organization and readability
of the document.

In other words, they prioritize clarity over currency.

Of course if it's possible to have clear documentation that also
matches the current code, that would be even better.  I have hope that
moving the documentation to the header files will get us there if we
do it right.

> I did drop some asciidoc-specific formatting from the main text (e.g.,
> "+" lines for connecting blocks), which means that extracting the result
> and running it through asciidoc won't work.

This unfortunately lost some structure (e.g., which paragraphs were
part of the same bulleted list).  It would be more common to use a
hanging indent:

 - The `buf` member is never NULL, so ...
   string operations ...
   by `strbuf_init()` ...

   Do not assume anything about what `buf` really is ...
   ...

 - ...

[...]
>                                             I think it makes the actual
> text much more readable, though. But we could do it the other way if
> people really want to asciidoc it.

I also don't mind losing the asciidoc markup, especially given that it
would be a pain to reconstruct and render.

My reaction at first glance is that this is trying to do something
good but the current implementation is less readable than
Documentation/technical/api-strbuf.txt.  Am I looking at a work in
progress, or is this meant to be ready for application?  Which aspects
would you be interested in comments on?

Thanks,
Jonathan

Re: [PATCH] document string_list_clear

[Michael Haggerty]

My vote is for putting the API docs in the header files:

* Functions are documented right next to their declarations (which the
compiler guarantees are up-to-date), removing ambiguity and avoiding
some redundancy.

* It is obvious at a glance which functions are documented and which
still need docs.

* It is easier to remember to update the documentation when it is near
the code, and easier for reviewers to remember to pester authors to do so.

I also like it when function declarations in header files include the
names of their parameters, as (1) it's an efficient way to give the
reader a good hint about how the function should be used, and (2) it
makes it easier to refer to the parameters from the docstring.

I agree that there is an important need for introductory/conceptual
documentation about and API, but I think this fits fine as a large
docstring at the top of the header file.

I personally don't have much use for formatted documentation but if that
were a need I would think that a standard tool like doxygen could be used.

I have been trying hard to add good header-file docstrings for code that
I have worked on, and even for code that I had to read because I needed
to figure out how to use it. I would find it a pity for that work to be
squashed into Documentation/technical/api-*.txt, where in my opinion it
is less discoverable and more likely to fall into disrepair.

Michael

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Michael Haggerty wrote:

>                              I would find it a pity for that work to be
> squashed into Documentation/technical/api-*.txt, where in my opinion it
> is less discoverable and more likely to fall into disrepair.

I think we're in violent agreement and keep repeating ourselves.

All I said is that api-strbuf.txt is currently the most readable
documentation of the strbuf API I can find.  The patch to move the
text to strbuf.h looked rough and incomplete.  Therefore I don't think
it's ready to be applied as is.  If you'd like more details about why
I say that, feel free to ask.

Jonathan

Re: [PATCH] document string_list_clear

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Michael Haggerty wrote:
>
>>                              I would find it a pity for that work to be
>> squashed into Documentation/technical/api-*.txt, where in my opinion it
>> is less discoverable and more likely to fall into disrepair.
>
> I think we're in violent agreement and keep repeating ourselves.

Hmph, I am confused.

I somehow had an impression that the "move to doc and remove from
header" patch was to illustrate how unpleasant the result will be as
a whole (i.e. results in a nice documentation as a starting point,
but we can see that it will be hard to motivate and help people to
keep it up to date during further development).  Which would suggest
that you are in favor of moving the other way around, to keep the
header rich with documentation only at the higher level.  Am I
reading you correctly?

> All I said is that api-strbuf.txt is currently the most readable
> documentation of the strbuf API I can find.  The patch to move the
> text to strbuf.h looked rough and incomplete.  Therefore I don't think
> it's ready to be applied as is.  If you'd like more details about why
> I say that, feel free to ask.

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Michael Haggerty wrote:

>>>                              I would find it a pity for that work to be
>>> squashed into Documentation/technical/api-*.txt, where in my opinion it
>>> is less discoverable and more likely to fall into disrepair.
>>
>> I think we're in violent agreement and keep repeating ourselves.
>
> Hmph, I am confused.
>
> I somehow had an impression that the "move to doc and remove from
> header" patch was to illustrate how unpleasant the result will be as
> a whole (i.e. results in a nice documentation as a starting point,
> but we can see that it will be hard to motivate and help people to
> keep it up to date during further development).  Which would suggest
> that you are in favor of moving the other way around, to keep the
> header rich with documentation only at the higher level.  Am I
> reading you correctly?

Sorry, I think I was unclear.

Some possibilities, in order of my preference (earlier items are better):

 1. Move documentation to header and provide a program to generate a nice
    standalone document.

 2. Move documentation to header, being careful enough that the header
    sort of works as a standalone document.

 3. Move documentation to Documentation/technical/ and keep the header
    bare-bones.

 4. Status quo (comprehensive documentation for some functions in both
    places, for others in only one place, no reliable way for someone
    to find the information they need in one place).

Since (3) is better than (4), I wrote simple patches to do that for
strbuf.h and string-list.h.  I meant them in earnest --- I hope they
get applied.

I think peff was working on (2), which is an admirable goal.  The
patch seemed to be incomplete.

Thanks,
Jonathan

Re: [PATCH] document string_list_clear

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Some possibilities, in order of my preference (earlier items are better):
>
>  1. Move documentation to header and provide a program to generate a nice
>     standalone document.
>
>  2. Move documentation to header, being careful enough that the header
>     sort of works as a standalone document.
>
>  3. Move documentation to Documentation/technical/ and keep the header
>     bare-bones.
>
>  4. Status quo (comprehensive documentation for some functions in both
>     places, for others in only one place, no reliable way for someone
>     to find the information they need in one place).
>
> Since (3) is better than (4), I wrote simple patches to do that for
> strbuf.h and string-list.h.  I meant them in earnest --- I hope they
> get applied.
>
> I think peff was working on (2), which is an admirable goal.  The
> patch seemed to be incomplete.

Yeah, I agree with the above preferred ordering, and also think once
we get to (2) it would be "usable" by the intended audience.  Those
who prefer (1) over (2) are the ones who somehow want to read hard
copies ;-) and are likely to be different audiences and are better
served by people with different skill-set and inclination.

I am not sure if (2) and (3) are that incompatible, though.  If you
had an acceptable version of (3), wouldn't it be just the matter of
indenting the whole thing with "s/^/ */", sprinkle "/**" and "*/" at
strategic paragraph breaks, and move them back to the corresponding
header?

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>>  2. Move documentation to header, being careful enough that the header
>>     sort of works as a standalone document.
>>
>>  3. Move documentation to Documentation/technical/ and keep the header
>>     bare-bones.
[...]
> I am not sure if (2) and (3) are that incompatible, though.  If you
> had an acceptable version of (3), wouldn't it be just the matter of
> indenting the whole thing with "s/^/ */", sprinkle "/**" and "*/" at
> strategic paragraph breaks, and move them back to the corresponding
> header?

Presumably.  There's also the question of whether to use asciidoc
markup, which got mixed in somehow (but I don't see why it has to be ---
a header with asciidoc would be fine with me, as would a file in
Documentation/technical/ without asciidoc).

Re: [PATCH] document string_list_clear

[Jeff King]

On Wed, Dec 10, 2014 at 01:18:15AM -0800, Jonathan Nieder wrote:

> > That seems like wishful thinking to me. Some subset of developers will
> > be happy reading the documentation in the header file, and will not
> > commonly run the tool. Therefore they will also not bother to examine
> > the output of the tool when making a change (either because they forget,
> > or because it is simply a hassle that they do not care about).
> 
> That is okay as long as enough people extract documentation to ensure
> the result is sane.

I have a guess that the "subset" I mentioned above is "most", but I
admit that is my intuition, not backed up by data.

> >                               The api-* files have slowly grown out of
> > date over the years, and I believe that is because they are too far
> > removed from the code people are actually touching.
> 
> I don't think they ever were very comprehensive or up to date.

Surely at least some of them were up to date when they were first
written? Take api-strbuf, for example. It started in dd613e6b, which
claims almost complete coverage. And then it slowly grew stale as
functions were added and not documented.

> As far as I understand, the api-* files are intended to be usually out
> of date.  Their main purpose is to explain the gist of how to use the
> API.  They were usually written way after the API was introduced (so
> they are behind already).  They are clearly written and as long as
> they mostly match the code, that is enough for them to be useful and
> educational.

I'm not sure any such thought as "intended to be out of date" went into
it. I think it was more "geez, these really need some documentation at
all". Out-of-date documentation can be dangerous and misleading, and is
one of the reasons I think it is a good idea to keep the documentation
closer to the code. It's not a silver bullet, but it helps make it more
obvious when both need to be updated.

> Then every few years or so, someone updates the file with the latest
> API changes, still with an eye to overall organization and readability
> of the document.

Again, look at api-strbuf.txt. The changes after the initial checkin are
almost entirely adding functions to the giant list. They are (hopefully)
added to the most appropriate spot, but that is also (again, hopefully)
done in the header file, too. I don't see any real effort towards
organization or readability.

Maybe I'm not understanding what you mean by organization or
readability. It seems to me like the api docs look like:

  some intro text and overall description

  some header (optional)

   - some-func

     description of some-func

   - some-func2

     description of some-func2


and I am proposing basically:

  /*
   * some intro text and overall description
   */

  /* some header (optional) */

  /* description of some-func */
  int some_func(...);

  /* description of some-func2 */
  int some_func2(...);

The readability and organization of those do not seem substantially
different to me.

> > I did drop some asciidoc-specific formatting from the main text (e.g.,
> > "+" lines for connecting blocks), which means that extracting the result
> > and running it through asciidoc won't work.
> 
> This unfortunately lost some structure (e.g., which paragraphs were
> part of the same bulleted list).  It would be more common to use a
> hanging indent:
> 
>  - The `buf` member is never NULL, so ...
>    string operations ...
>    by `strbuf_init()` ...
> 
>    Do not assume anything about what `buf` really is ...
>    ...
> 
>  - ...

Yeah, I think a hanging indent would be fine. I didn't actually spend
much effort on formatting. My point was to say "is this really so bad to
read inline?"

We could stick with asciidoc, too, but I really would like to keep the
barrier to writing documentation as low as possible, in order to
encourage it happening. I find that asciidoc syntax often ends up making
both writing and reading the source more tedious than it needs to be.

> >                                             I think it makes the actual
> > text much more readable, though. But we could do it the other way if
> > people really want to asciidoc it.
> 
> I also don't mind losing the asciidoc markup, especially given that it
> would be a pain to reconstruct and render.

I'm not sure it would be that hard if you can extract to text (which you
seem to want). But in the patch I posted (and the outline I showed
above), I chose to turn:

   foo::
      description of foo

into just:

   /* description of foo */
   void foo(void);

to avoid writers having to repeat themselves. But it does mean that the
extracted comments do not stand alone. You can either hack your own
parser (which is what doxygen does), or build something on top of a real
parser (there is cldoc, which uses libclang to extract comments next to
declarations).

That's why I suggested the possibility of punting on extraction
entirely, and just treating the header file as both the source and
finished document.

> My reaction at first glance is that this is trying to do something
> good but the current implementation is less readable than
> Documentation/technical/api-strbuf.txt.  Am I looking at a work in
> progress, or is this meant to be ready for application?  Which aspects
> would you be interested in comments on?

It was more "this is the direction I think we should go, and I will try
to implement a complete conversion to convince myself and others that I
am not forgetting any corner case". I probably should have given the
"outline" I gave above along with it.

I think the end result that I posted is still strictly better than what
we have currently, with the exception that I should have reformatted the
hanging indents. What is it that you don't like about it?

I'm not super interested in going back and forth over minor markup
issues if there are overall issues with the concept that might block it
from happening (just because they are tedious and time-consuming, and I
do not want to spend a lot of time formatting something that will get
thrown away). So please list your complaints in order of increasing
specificity. :)

-Peff

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Jeff King wrote:

> I'm not sure any such thought as "intended to be out of date" went into
> it.

Junio started the documentation in v1.5.4-rc1~49 (2007-11-24).  I'm
not sure if there was a discussion preceding that commit.  My
understanding was always that putting the documentation out-of-line
was an intentional decision and that it was understood that the
documentation would have cycles of falling out of date and needing to
be updated, but I may be misunderstanding the history.

Separate from the question of history, I honestly prefer this way of
doing API documentation relative to 90% of the API documentation in
headers I've seen in other projects.  I suspect you don't.  That's
okay --- it's possible for rational people to disagree about things.

It's moot given that we don't seem to disagree about what should be
done about it.  Why keep arguing?

> I think the end result that I posted is still strictly better than what
> we have currently, with the exception that I should have reformatted the
> hanging indents. What is it that you don't like about it?

Other issues of inconsistent markup.  For example, some comments are
strangely indented, and some look like

	/* First line
	 * second line
	 * third line */

> I'm not super interested in going back and forth over minor markup
> issues if there are overall issues with the concept that might block it
> from happening (just because they are tedious and time-consuming, and I
> do not want to spend a lot of time formatting something that will get
> thrown away). So please list your complaints in order of increasing
> specificity. :)

The overall concept is good.

I chose not to list markup issues for the same reason (it's more
tedious to go back and forth than for someone to spend some time to
get it mostly-right first on their own).  I am kind of confused about
the current status, since I've said again and again that I'd be happy
to see the documentation in the header but you still seem to be trying
to convince me of that.  What am I missing?

Jonathan

Re: [PATCH] document string_list_clear

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jeff King wrote:
>
>> I'm not sure any such thought as "intended to be out of date" went into
>> it.
>
> Junio started the documentation in v1.5.4-rc1~49 (2007-11-24).  I'm
> not sure if there was a discussion preceding that commit.  My
> understanding was always that putting the documentation out-of-line
> was an intentional decision and that it was understood that the
> documentation would have cycles of falling out of date and needing to
> be updated, but I may be misunderstanding the history.

There of course was not "intended to be out of date".  It was just
that the prevailing style of comments in the header back then was
sporadic per-function explanation that would have served only as a
reminder for those who have read the corresponding *.c file, without
any overall structure of the API described.

We could have gone to "in header description" route back then and
the commit you referred to could have added

	/*
         * NEEDSWORK: describe the overall structure of the API
         * here, so that people can use it without reading the
         * implementation of the API itself.
         */

to each header file instead.  I chose not to primarily because those
who wanted to have documentation pages that are pointed from api-index
were audience different from those who updated *.[ch] files, and also
I had slight preference to have a set of *.txt files that do not force
me to squelch /* * * */ while reading them.

> Separate from the question of history, I honestly prefer this way of
> doing API documentation relative to 90% of the API documentation in
> headers I've seen in other projects.  I suspect you don't.  That's
> okay --- it's possible for rational people to disagree about things.

Yes.

> The overall concept is good.

OK, then let's agree to go in the "comprehensive doc in the *.h and
then those who want *.txt can help the machinery to extract"
direction.

Thanks.

Re: [PATCH] document string_list_clear

[Stefan Beller]

On Fri, Dec 12, 2014 at 10:31 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jeff King wrote:
>
>> I'm not sure any such thought as "intended to be out of date" went into
>> it.
>
> Junio started the documentation in v1.5.4-rc1~49 (2007-11-24).  I'm
> not sure if there was a discussion preceding that commit.  My
> understanding was always that putting the documentation out-of-line
> was an intentional decision and that it was understood that the
> documentation would have cycles of falling out of date and needing to
> be updated, but I may be misunderstanding the history.
>
> Separate from the question of history, I honestly prefer this way of
> doing API documentation relative to 90% of the API documentation in
> headers I've seen in other projects.  I suspect you don't.  That's
> okay --- it's possible for rational people to disagree about things.
>
> It's moot given that we don't seem to disagree about what should be
> done about it.  Why keep arguing?
>

So you proposed an order of desirability w.r.t. the way we want to go
documenting things.

> Some possibilities, in order of my preference (earlier items are better):
>
>  1. Move documentation to header and provide a program to generate a nice
>     standalone document.

Junio agrees with that order. Michael said

> My vote is for putting the API docs in the header files:

which seems to be an agreement with your most desired way to do it.

Jeff said earlier when reviewing your patch:

> I somewhat feel this goes in the opposite direction of where we want to
> be *(all data in one place, but that place is the header)*.

which sounds to me as if he prefers also the header as the one place of truth.

I do agree on your proposed order of desirability as well. So nobody
showed up yet,
who dislikes the headers as the one place of truth.

So the open question is:
* The historic reasoning to introduce technical/api-$header at all. As
Junio mainly
  put persons to ask into the stubs in 530e741c726 (2007-11-24, Start
preparing the API documents.),
  it was probably improving the status quo back then. Maybe it was
easier to have all names in place
  than using git blame/git pickaxe at the time?
  Answering this question makes sure we don't oversee some reasons.

* Assuming we put everything into headers now, we'd need to discuss
  -> Do we want to extract it to technical/api-$somedoc later at all?
(There seems to be some disagreement?)
  -> How do we extract (plain sed for lines starting with "*", or
doxygen, or a self cooked script)
  -> How do we ensure the generated documentation doesn't get stale over time?

As I personally just look up things in the header anyway, I'd rather
get started on polishing the comments in the
headers as on arguing how to extract it. Though that's a required step
to know which format we want in the headers.

Thanks,
Stefan

Re: [PATCH] document string_list_clear

[Jeff King]

On Fri, Dec 12, 2014 at 10:31:14AM -0800, Jonathan Nieder wrote:

> Separate from the question of history, I honestly prefer this way of
> doing API documentation relative to 90% of the API documentation in
> headers I've seen in other projects.  I suspect you don't.  That's
> okay --- it's possible for rational people to disagree about things.
> 
> It's moot given that we don't seem to disagree about what should be
> done about it.  Why keep arguing?

Fair enough.

> > I think the end result that I posted is still strictly better than what
> > we have currently, with the exception that I should have reformatted the
> > hanging indents. What is it that you don't like about it?
> 
> Other issues of inconsistent markup.  For example, some comments are
> strangely indented, and some look like
> 
> 	/* First line
> 	 * second line
> 	 * third line */

Yeah, I had to strip the indent from:

  strbuf_init::
	description of strbuf_init...

when "strbuf_init::" went away, but it looks like I missed two of them.
And I see one "/* First line" where the "header" from "Adding data to
the buffer" got merged with a note. I agree those should be fixed.

I used our normal single-line comment style for most of those:

  /* Related to the size of the buffer */

in some places. But I'd be happy, too, with:

  /*
   * Related to the size of the buffer
   */

(and probably using a full sentence, or caps or underlining or something
to indicate that the comment begins a new section of the header file).

> I chose not to list markup issues for the same reason (it's more
> tedious to go back and forth than for someone to spend some time to
> get it mostly-right first on their own).  I am kind of confused about
> the current status, since I've said again and again that I'd be happy
> to see the documentation in the header but you still seem to be trying
> to convince me of that.  What am I missing?

I think partially it was crossing mails. While I was writing the
response that you responded to just now, I ended up postponing it for
other work, and in the meantime you sent several more messages
indicating you were OK with moving documentation into the headers. I
almost scrapped my response, but frankly I was left a bit confused by
your position, since it seemed the opposite of the 2 patches you had
sent moving things out of headers.

Do not feel obligated to make me unconfused. If you are on board with
putting documentation in the headers, then I think we can move forward
with that plan.

So what next?

I agree there are some formatting problems in the strbuf.h patch I sent
earlier. I'm happy to fix them and resend, but I'm not 100% sure that
fixing all the problems I see will not leave problems for you. I can fix
them and you can review if you want. Or alternatively, if you have more
drastic formatting or wording changes in mind, maybe it would make sense
for you to take a pass?

I _don't_ want to commit to moving all of api-* into headers myself.
It's something I would rather have happen over time as people touch
areas[1], because it's tedious and time-consuming. That does create an
inconsistency, in the sense that some APIs will be documented in
Documentation/technical, and others in their header files. But as long
as each _individual_ API is documented in one or the other, I don't see
that as a big problem (of course the status quo is a mix for some APIs,
so we will live with that until they are touched, but that is no worse
than today's state).

-Peff

[1] I had a vague plan to put new documentation into headers, and slowly
    migrate that way until we had a critical mass. But that was moving
    somewhat slowly, and obviously escaped the notice of many people. :)

Re: [PATCH] document string_list_clear

[Jeff King]

On Fri, Dec 12, 2014 at 11:19:23AM -0800, Stefan Beller wrote:

> * Assuming we put everything into headers now, we'd need to discuss
>   -> Do we want to extract it to technical/api-$somedoc later at all?
> (There seems to be some disagreement?)
>   -> How do we extract (plain sed for lines starting with "*", or
> doxygen, or a self cooked script)
>   -> How do we ensure the generated documentation doesn't get stale over time?

I think it would be _nice_ to extract it, and that should be a
consideration when doing the manual work of moving it into headers
(you'll notice I used the "/**" convention to start comments which
should be extractable, but I am happy to use any similar convention[1]).

But unless somebody is excited about working on the extractor
immediately, I think we should punt on it for now. Moving the
documentation is already a big job, and I do not want to add an extra
dependency to the task.

The exact extractor choice might influence formatting decisions (e.g.,
should extracted text stand alone, or must it be viewed in context of the
function it prefaces), but we might be able to do a mechanical
conversion later if those facts change.

-Peff

Re: [PATCH] document string_list_clear

[Jonathan Nieder]

Jeff King wrote:

> I agree there are some formatting problems in the strbuf.h patch I sent
> earlier. I'm happy to fix them and resend, but I'm not 100% sure that
> fixing all the problems I see will not leave problems for you. I can fix
> them and you can review if you want. Or alternatively, if you have more
> drastic formatting or wording changes in mind, maybe it would make sense
> for you to take a pass?

For the sake of reviewability, I think the less that happens in a
single working patch, the better. :)

If I were doing it, I would first de-asciidoc within technical/ and
then move into the header in a separate patch.  Or first move with
asciidoc intact and then de-asciidoc in a separate patch.  Combining
the two into a single patch is also fine.  Please don't change wording
at the same time.

> I _don't_ want to commit to moving all of api-* into headers myself.

Yeah, I think that's fine.  Moving one at a time when people want to
do it doesn't hurt consistency at all relative to the status quo.

Jonathan

Re: [PATCH] document string_list_clear

[Jeff King]

On Fri, Dec 12, 2014 at 11:35:52AM -0800, Jonathan Nieder wrote:

> If I were doing it, I would first de-asciidoc within technical/ and
> then move into the header in a separate patch.  Or first move with
> asciidoc intact and then de-asciidoc in a separate patch.  Combining
> the two into a single patch is also fine.  Please don't change wording
> at the same time.

Here's what I came up with. The first patch probably does more than
you'd like (and more than I would have done if I were starting from
scratch today). But I didn't want to get into undoing the stripping of
each function-name list item that I showed earlier, as it would be a lot
of tedious manual work. If we decide we want to keep those, I'm happy to
do the work to restore them, but it didn't seem like a good tradeoff
just to create an intermediate state to make the patch prettier.

I did split out some of the other formatting decisions, though, so they
can be evaluated individually.

  [1/4]: strbuf: migrate api-strbuf.txt documentation to strbuf.h
  [2/4]: strbuf.h: drop asciidoc list formatting from API docs
  [3/4]: strbuf.h: format asciidoc code blocks as 4-space indent
  [4/4]: strbuf.h: reorganize api function grouping headers

-Peff

[PATCH 1/4] strbuf: migrate api-strbuf.txt documentation to strbuf.h

[Jeff King]

Some of strbuf is documented as comments above functions,
and some is separate in Documentation/technical/api-strbuf.txt.
This makes it annoying to find the appropriate documentation.
We'd rather have it all in one place, which means all in the
text document, or all in the header.

Let's choose the header as that place. Even though the
formatting is not quite as pretty, this keeps the
documentation close to the related code.  The hope is that
this makes it easier to find what you want (human-readable
comments are right next to the C declarations), and easier
for writers to keep the documentation up to date.

This is more or less a straight import of the text from
api-strbuf.txt into C comments, complete with asciidoc
formatting. The exceptions are:

 1. All comments created in this way are started with "/**"
    to indicate they are part of the API documentation. This
    may help later with extracting the text to pretty-print
    it.

 2. Function descriptions do not repeat the function name,
    as it is available in the context directly below.  So:

      `strbuf_add`::

          Add data of given length to the buffer.

    from api-strbuf.txt becomes:

      /**
       * Add data of given length to the buffer.
       */
      void strbuf_add(struct strbuf *sb, const void *, size_t);

    As a result, any block-continuation required in asciidoc
    for that list item was dropped in favor of straight
    blank-line paragraph (since it is not necessary when we
    are not in a list item).

 3. There is minor re-wording to integrate existing comments
    and api-strbuf text. In each case, I took whichever
    version was more descriptive, and eliminated any
    redundancies. In one case, for strbuf_addstr, the api
    documentation gave its inline definition; I eliminated
    this as redundant with the actual definition, which can
    be seen directly below the comment.

 4. The functions in the header file are re-ordered to match
    the ordering of the API documentation, under the
    assumption that more thought went into the grouping
    there.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/technical/api-strbuf.txt | 351 -------------------------
 strbuf.h                               | 458 ++++++++++++++++++++++++++++-----
 2 files changed, 390 insertions(+), 419 deletions(-)
 delete mode 100644 Documentation/technical/api-strbuf.txt

diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
deleted file mode 100644
index cca6543..0000000
--- a/Documentation/technical/api-strbuf.txt
+++ /dev/null
@@ -1,351 +0,0 @@
-strbuf API
-==========
-
-strbuf's are meant to be used with all the usual C string and memory
-APIs. Given that the length of the buffer is known, it's often better to
-use the mem* functions than a str* one (memchr vs. strchr e.g.).
-Though, one has to be careful about the fact that str* functions often
-stop on NULs and that strbufs may have embedded NULs.
-
-A strbuf is NUL terminated for convenience, but no function in the
-strbuf API actually relies on the string being free of NULs.
-
-strbufs have some invariants that are very important to keep in mind:
-
-. The `buf` member is never NULL, so it can be used in any usual C
-string operations safely. strbuf's _have_ to be initialized either by
-`strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.
-+
-Do *not* assume anything on what `buf` really is (e.g. if it is
-allocated memory or not), use `strbuf_detach()` to unwrap a memory
-buffer from its strbuf shell in a safe way. That is the sole supported
-way. This will give you a malloced buffer that you can later `free()`.
-+
-However, it is totally safe to modify anything in the string pointed by
-the `buf` member, between the indices `0` and `len-1` (inclusive).
-
-. The `buf` member is a byte array that has at least `len + 1` bytes
-  allocated. The extra byte is used to store a `'\0'`, allowing the
-  `buf` member to be a valid C-string. Every strbuf function ensure this
-  invariant is preserved.
-+
-NOTE: It is OK to "play" with the buffer directly if you work it this
-      way:
-+
-----
-strbuf_grow(sb, SOME_SIZE); <1>
-strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);
-----
-<1> Here, the memory array starting at `sb->buf`, and of length
-`strbuf_avail(sb)` is all yours, and you can be sure that
-`strbuf_avail(sb)` is at least `SOME_SIZE`.
-+
-NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.
-+
-Doing so is safe, though if it has to be done in many places, adding the
-missing API to the strbuf module is the way to go.
-+
-WARNING: Do _not_ assume that the area that is yours is of size `alloc
-- 1` even if it's true in the current implementation. Alloc is somehow a
-"private" member that should not be messed with. Use `strbuf_avail()`
-instead.
-
-Data structures
----------------
-
-* `struct strbuf`
-
-This is the string buffer structure. The `len` member can be used to
-determine the current length of the string, and `buf` member provides
-access to the string itself.
-
-Functions
----------
-
-* Life cycle
-
-`strbuf_init`::
-
-	Initialize the structure. The second parameter can be zero or a bigger
-	number to allocate memory, in case you want to prevent further reallocs.
-
-`strbuf_release`::
-
-	Release a string buffer and the memory it used. You should not use the
-	string buffer after using this function, unless you initialize it again.
-
-`strbuf_detach`::
-
-	Detach the string from the strbuf and returns it; you now own the
-	storage the string occupies and it is your responsibility from then on
-	to release it with `free(3)` when you are done with it.
-
-`strbuf_attach`::
-
-	Attach a string to a buffer. You should specify the string to attach,
-	the current length of the string and the amount of allocated memory.
-	The amount must be larger than the string length, because the string you
-	pass is supposed to be a NUL-terminated string.  This string _must_ be
-	malloc()ed, and after attaching, the pointer cannot be relied upon
-	anymore, and neither be free()d directly.
-
-`strbuf_swap`::
-
-	Swap the contents of two string buffers.
-
-* Related to the size of the buffer
-
-`strbuf_avail`::
-
-	Determine the amount of allocated but unused memory.
-
-`strbuf_grow`::
-
-	Ensure that at least this amount of unused memory is available after
-	`len`. This is used when you know a typical size for what you will add
-	and want to avoid repetitive automatic resizing of the underlying buffer.
-	This is never a needed operation, but can be critical for performance in
-	some cases.
-
-`strbuf_setlen`::
-
-	Set the length of the buffer to a given value. This function does *not*
-	allocate new memory, so you should not perform a `strbuf_setlen()` to a
-	length that is larger than `len + strbuf_avail()`. `strbuf_setlen()` is
-	just meant as a 'please fix invariants from this strbuf I just messed
-	with'.
-
-`strbuf_reset`::
-
-	Empty the buffer by setting the size of it to zero.
-
-* Related to the contents of the buffer
-
-`strbuf_trim`::
-
-	Strip whitespace from the beginning and end of a string.
-	Equivalent to performing `strbuf_rtrim()` followed by `strbuf_ltrim()`.
-
-`strbuf_rtrim`::
-
-	Strip whitespace from the end of a string.
-
-`strbuf_ltrim`::
-
-	Strip whitespace from the beginning of a string.
-
-`strbuf_reencode`::
-
-	Replace the contents of the strbuf with a reencoded form.  Returns -1
-	on error, 0 on success.
-
-`strbuf_tolower`::
-
-	Lowercase each character in the buffer using `tolower`.
-
-`strbuf_cmp`::
-
-	Compare two buffers. Returns an integer less than, equal to, or greater
-	than zero if the first buffer is found, respectively, to be less than,
-	to match, or be greater than the second buffer.
-
-* Adding data to the buffer
-
-NOTE: All of the functions in this section will grow the buffer as necessary.
-If they fail for some reason other than memory shortage and the buffer hadn't
-been allocated before (i.e. the `struct strbuf` was set to `STRBUF_INIT`),
-then they will free() it.
-
-`strbuf_addch`::
-
-	Add a single character to the buffer.
-
-`strbuf_addchars`::
-
-	Add a character the specified number of times to the buffer.
-
-`strbuf_insert`::
-
-	Insert data to the given position of the buffer. The remaining contents
-	will be shifted, not overwritten.
-
-`strbuf_remove`::
-
-	Remove given amount of data from a given position of the buffer.
-
-`strbuf_splice`::
-
-	Remove the bytes between `pos..pos+len` and replace it with the given
-	data.
-
-`strbuf_add_commented_lines`::
-
-	Add a NUL-terminated string to the buffer. Each line will be prepended
-	by a comment character and a blank.
-
-`strbuf_add`::
-
-	Add data of given length to the buffer.
-
-`strbuf_addstr`::
-
-Add a NUL-terminated string to the buffer.
-+
-NOTE: This function will *always* be implemented as an inline or a macro
-that expands to:
-+
-----
-strbuf_add(..., s, strlen(s));
-----
-+
-Meaning that this is efficient to write things like:
-+
-----
-strbuf_addstr(sb, "immediate string");
-----
-
-`strbuf_addbuf`::
-
-	Copy the contents of another buffer at the end of the current one.
-
-`strbuf_adddup`::
-
-	Copy part of the buffer from a given position till a given length to the
-	end of the buffer.
-
-`strbuf_expand`::
-
-	This function can be used to expand a format string containing
-	placeholders. To that end, it parses the string and calls the specified
-	function for every percent sign found.
-+
-The callback function is given a pointer to the character after the `%`
-and a pointer to the struct strbuf.  It is expected to add the expanded
-version of the placeholder to the strbuf, e.g. to add a newline
-character if the letter `n` appears after a `%`.  The function returns
-the length of the placeholder recognized and `strbuf_expand()` skips
-over it.
-+
-The format `%%` is automatically expanded to a single `%` as a quoting
-mechanism; callers do not need to handle the `%` placeholder themselves,
-and the callback function will not be invoked for this placeholder.
-+
-All other characters (non-percent and not skipped ones) are copied
-verbatim to the strbuf.  If the callback returned zero, meaning that the
-placeholder is unknown, then the percent sign is copied, too.
-+
-In order to facilitate caching and to make it possible to give
-parameters to the callback, `strbuf_expand()` passes a context pointer,
-which can be used by the programmer of the callback as she sees fit.
-
-`strbuf_expand_dict_cb`::
-
-	Used as callback for `strbuf_expand()`, expects an array of
-	struct strbuf_expand_dict_entry as context, i.e. pairs of
-	placeholder and replacement string.  The array needs to be
-	terminated by an entry with placeholder set to NULL.
-
-`strbuf_addbuf_percentquote`::
-
-	Append the contents of one strbuf to another, quoting any
-	percent signs ("%") into double-percents ("%%") in the
-	destination. This is useful for literal data to be fed to either
-	strbuf_expand or to the *printf family of functions.
-
-`strbuf_humanise_bytes`::
-
-	Append the given byte size as a human-readable string (i.e. 12.23 KiB,
-	3.50 MiB).
-
-`strbuf_addf`::
-
-	Add a formatted string to the buffer.
-
-`strbuf_commented_addf`::
-
-	Add a formatted string prepended by a comment character and a
-	blank to the buffer.
-
-`strbuf_fread`::
-
-	Read a given size of data from a FILE* pointer to the buffer.
-+
-NOTE: The buffer is rewound if the read fails. If -1 is returned,
-`errno` must be consulted, like you would do for `read(3)`.
-`strbuf_read()`, `strbuf_read_file()` and `strbuf_getline()` has the
-same behaviour as well.
-
-`strbuf_read`::
-
-	Read the contents of a given file descriptor. The third argument can be
-	used to give a hint about the file size, to avoid reallocs.
-
-`strbuf_read_file`::
-
-	Read the contents of a file, specified by its path. The third argument
-	can be used to give a hint about the file size, to avoid reallocs.
-
-`strbuf_readlink`::
-
-	Read the target of a symbolic link, specified by its path.  The third
-	argument can be used to give a hint about the size, to avoid reallocs.
-
-`strbuf_getline`::
-
-	Read a line from a FILE *, overwriting the existing contents
-	of the strbuf. The second argument specifies the line
-	terminator character, typically `'\n'`.
-	Reading stops after the terminator or at EOF.  The terminator
-	is removed from the buffer before returning.  Returns 0 unless
-	there was nothing left before EOF, in which case it returns `EOF`.
-
-`strbuf_getwholeline`::
-
-	Like `strbuf_getline`, but keeps the trailing terminator (if
-	any) in the buffer.
-
-`strbuf_getwholeline_fd`::
-
-	Like `strbuf_getwholeline`, but operates on a file descriptor.
-	It reads one character at a time, so it is very slow.  Do not
-	use it unless you need the correct position in the file
-	descriptor.
-
-`strbuf_getcwd`::
-
-	Set the buffer to the path of the current working directory.
-
-`strbuf_add_absolute_path`
-
-	Add a path to a buffer, converting a relative path to an
-	absolute one in the process.  Symbolic links are not
-	resolved.
-
-`stripspace`::
-
-	Strip whitespace from a buffer. The second parameter controls if
-	comments are considered contents to be removed or not.
-
-`strbuf_split_buf`::
-`strbuf_split_str`::
-`strbuf_split_max`::
-`strbuf_split`::
-
-	Split a string or strbuf into a list of strbufs at a specified
-	terminator character.  The returned substrings include the
-	terminator characters.  Some of these functions take a `max`
-	parameter, which, if positive, limits the output to that
-	number of substrings.
-
-`strbuf_list_free`::
-
-	Free a list of strbufs (for example, the return values of the
-	`strbuf_split()` functions).
-
-`launch_editor`::
-
-	Launch the user preferred editor to edit a file and fill the buffer
-	with the file's contents upon the user completing their editing. The
-	third argument can be used to set the environment which the editor is
-	run in. If the buffer is NULL the editor is launched as usual but the
-	file's contents are not read into the buffer upon completion.
diff --git a/strbuf.h b/strbuf.h
index 652b6c4..0a83f9a 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,22 +1,117 @@
 #ifndef STRBUF_H
 #define STRBUF_H
 
-/* See Documentation/technical/api-strbuf.txt */
+/**
+ * strbuf's are meant to be used with all the usual C string and memory
+ * APIs. Given that the length of the buffer is known, it's often better to
+ * use the mem* functions than a str* one (memchr vs. strchr e.g.).
+ * Though, one has to be careful about the fact that str* functions often
+ * stop on NULs and that strbufs may have embedded NULs.
+ *
+ * A strbuf is NUL terminated for convenience, but no function in the
+ * strbuf API actually relies on the string being free of NULs.
+ *
+ * strbufs have some invariants that are very important to keep in mind:
+ *
+ * . The `buf` member is never NULL, so it can be used in any usual C
+ * string operations safely. strbuf's _have_ to be initialized either by
+ * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.
+ * +
+ * Do *not* assume anything on what `buf` really is (e.g. if it is
+ * allocated memory or not), use `strbuf_detach()` to unwrap a memory
+ * buffer from its strbuf shell in a safe way. That is the sole supported
+ * way. This will give you a malloced buffer that you can later `free()`.
+ * +
+ * However, it is totally safe to modify anything in the string pointed by
+ * the `buf` member, between the indices `0` and `len-1` (inclusive).
+ *
+ * . The `buf` member is a byte array that has at least `len + 1` bytes
+ *   allocated. The extra byte is used to store a `'\0'`, allowing the
+ *   `buf` member to be a valid C-string. Every strbuf function ensure this
+ *   invariant is preserved.
+ * +
+ * NOTE: It is OK to "play" with the buffer directly if you work it this
+ *       way:
+ * +
+ * ----
+ * strbuf_grow(sb, SOME_SIZE); <1>
+ * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);
+ * ----
+ * <1> Here, the memory array starting at `sb->buf`, and of length
+ * `strbuf_avail(sb)` is all yours, and you can be sure that
+ * `strbuf_avail(sb)` is at least `SOME_SIZE`.
+ * +
+ * NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.
+ * +
+ * Doing so is safe, though if it has to be done in many places, adding the
+ * missing API to the strbuf module is the way to go.
+ * +
+ * WARNING: Do _not_ assume that the area that is yours is of size `alloc
+ * - 1` even if it's true in the current implementation. Alloc is somehow a
+ * "private" member that should not be messed with. Use `strbuf_avail()`
+ * instead.
+ */
 
-extern char strbuf_slopbuf[];
+/*
+ * Data Structures
+ * ---------------
+ */
+
+/**
+ * This is the string buffer structure. The `len` member can be used to
+ * determine the current length of the string, and `buf` member provides
+ * access to the string itself.
+ */
 struct strbuf {
 	size_t alloc;
 	size_t len;
 	char *buf;
 };
 
+extern char strbuf_slopbuf[];
 #define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
 
-/*----- strbuf life cycle -----*/
+/**
+ * Functions
+ * ---------
+ */
+
+/**
+ * * Life Cycle
+ */
+
+/**
+ * Initialize the structure. The second parameter can be zero or a bigger
+ * number to allocate memory, in case you want to prevent further reallocs.
+ */
 extern void strbuf_init(struct strbuf *, size_t);
+
+/**
+ * Release a string buffer and the memory it used. You should not use the
+ * string buffer after using this function, unless you initialize it again.
+ */
 extern void strbuf_release(struct strbuf *);
+
+/**
+ * Detach the string from the strbuf and returns it; you now own the
+ * storage the string occupies and it is your responsibility from then on
+ * to release it with `free(3)` when you are done with it.
+ */
 extern char *strbuf_detach(struct strbuf *, size_t *);
+
+/**
+ * Attach a string to a buffer. You should specify the string to attach,
+ * the current length of the string and the amount of allocated memory.
+ * The amount must be larger than the string length, because the string you
+ * pass is supposed to be a NUL-terminated string.  This string _must_ be
+ * malloc()ed, and after attaching, the pointer cannot be relied upon
+ * anymore, and neither be free()d directly.
+ */
 extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);
+
+/**
+ * Swap the contents of two string buffers.
+ */
 static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)
 {
 	struct strbuf tmp = *a;
@@ -24,14 +119,35 @@ static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)
 	*b = tmp;
 }
 
-/*----- strbuf size related -----*/
+
+/**
+ * * Related to the size of the buffer
+ */
+
+/**
+ * Determine the amount of allocated but unused memory.
+ */
 static inline size_t strbuf_avail(const struct strbuf *sb)
 {
 	return sb->alloc ? sb->alloc - sb->len - 1 : 0;
 }
 
+/**
+ * Ensure that at least this amount of unused memory is available after
+ * `len`. This is used when you know a typical size for what you will add
+ * and want to avoid repetitive automatic resizing of the underlying buffer.
+ * This is never a needed operation, but can be critical for performance in
+ * some cases.
+ */
 extern void strbuf_grow(struct strbuf *, size_t);
 
+/**
+ * Set the length of the buffer to a given value. This function does *not*
+ * allocate new memory, so you should not perform a `strbuf_setlen()` to a
+ * length that is larger than `len + strbuf_avail()`. `strbuf_setlen()` is
+ * just meant as a 'please fix invariants from this strbuf I just messed
+ * with'.
+ */
 static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 {
 	if (len > (sb->alloc ? sb->alloc - 1 : 0))
@@ -39,16 +155,285 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 	sb->len = len;
 	sb->buf[len] = '\0';
 }
+
+/**
+ * Empty the buffer by setting the size of it to zero.
+ */
 #define strbuf_reset(sb)  strbuf_setlen(sb, 0)
 
-/*----- content related -----*/
+
+/**
+ * * Related to the contents of the buffer
+ */
+
+/**
+ * Strip whitespace from the beginning and end of a string.
+ * Equivalent to performing `strbuf_rtrim()` followed by `strbuf_ltrim()`.
+ */
 extern void strbuf_trim(struct strbuf *);
+
+/**
+ * Strip whitespace from the end of a string.
+ */
 extern void strbuf_rtrim(struct strbuf *);
+
+/**
+ * Strip whitespace from the beginning of a string.
+ */
 extern void strbuf_ltrim(struct strbuf *);
+
+/**
+ * Replace the contents of the strbuf with a reencoded form.  Returns -1
+ * on error, 0 on success.
+ */
 extern int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);
+
+/**
+ * Lowercase each character in the buffer using `tolower`.
+ */
 extern void strbuf_tolower(struct strbuf *sb);
+
+/**
+ * Compare two buffers. Returns an integer less than, equal to, or greater
+ * than zero if the first buffer is found, respectively, to be less than,
+ * to match, or be greater than the second buffer.
+ */
 extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
 
+
+/**
+ * Adding data to the buffer
+ *
+ * NOTE: All of the functions in this section will grow the buffer as
+ * necessary.  If they fail for some reason other than memory shortage and the
+ * buffer hadn't been allocated before (i.e. the `struct strbuf` was set to
+ * `STRBUF_INIT`), then they will free() it.
+ */
+
+/**
+ * Add a single character to the buffer.
+ */
+static inline void strbuf_addch(struct strbuf *sb, int c)
+{
+	strbuf_grow(sb, 1);
+	sb->buf[sb->len++] = c;
+	sb->buf[sb->len] = '\0';
+}
+
+/**
+ * Add a character the specified number of times to the buffer.
+ */
+extern void strbuf_addchars(struct strbuf *sb, int c, size_t n);
+
+/**
+ * Insert data to the given position of the buffer. The remaining contents
+ * will be shifted, not overwritten.
+ */
+extern void strbuf_insert(struct strbuf *, size_t pos, const void *, size_t);
+
+/**
+ * Remove given amount of data from a given position of the buffer.
+ */
+extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);
+
+/**
+ * Remove the bytes between `pos..pos+len` and replace it with the given
+ * data.
+ */
+extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,
+			  const void *, size_t);
+
+/**
+ * Add a NUL-terminated string to the buffer. Each line will be prepended
+ * by a comment character and a blank.
+ */
+extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);
+
+
+/**
+ * Add data of given length to the buffer.
+ */
+extern void strbuf_add(struct strbuf *, const void *, size_t);
+
+/**
+ * Add a NUL-terminated string to the buffer.
+ *
+ * NOTE: This function will *always* be implemented as an inline or a macro
+ * using strlen, meaning that this is efficient to write things like:
+ *
+ * ----
+ * strbuf_addstr(sb, "immediate string");
+ * ----
+ *
+ */
+static inline void strbuf_addstr(struct strbuf *sb, const char *s)
+{
+	strbuf_add(sb, s, strlen(s));
+}
+
+/**
+ * Copy the contents of another buffer at the end of the current one.
+ */
+static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
+{
+	strbuf_grow(sb, sb2->len);
+	strbuf_add(sb, sb2->buf, sb2->len);
+}
+
+/**
+ * Copy part of the buffer from a given position till a given length to the
+ * end of the buffer.
+ */
+extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);
+
+/**
+ * This function can be used to expand a format string containing
+ * placeholders. To that end, it parses the string and calls the specified
+ * function for every percent sign found.
+ *
+ * The callback function is given a pointer to the character after the `%`
+ * and a pointer to the struct strbuf.  It is expected to add the expanded
+ * version of the placeholder to the strbuf, e.g. to add a newline
+ * character if the letter `n` appears after a `%`.  The function returns
+ * the length of the placeholder recognized and `strbuf_expand()` skips
+ * over it.
+ *
+ * The format `%%` is automatically expanded to a single `%` as a quoting
+ * mechanism; callers do not need to handle the `%` placeholder themselves,
+ * and the callback function will not be invoked for this placeholder.
+ *
+ * All other characters (non-percent and not skipped ones) are copied
+ * verbatim to the strbuf.  If the callback returned zero, meaning that the
+ * placeholder is unknown, then the percent sign is copied, too.
+ *
+ * In order to facilitate caching and to make it possible to give
+ * parameters to the callback, `strbuf_expand()` passes a context pointer,
+ * which can be used by the programmer of the callback as she sees fit.
+ */
+typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);
+extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);
+
+/**
+ * Used as callback for `strbuf_expand()`, expects an array of
+ * struct strbuf_expand_dict_entry as context, i.e. pairs of
+ * placeholder and replacement string.  The array needs to be
+ * terminated by an entry with placeholder set to NULL.
+ */
+struct strbuf_expand_dict_entry {
+	const char *placeholder;
+	const char *value;
+};
+extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context);
+
+/**
+ * Append the contents of one strbuf to another, quoting any
+ * percent signs ("%") into double-percents ("%%") in the
+ * destination. This is useful for literal data to be fed to either
+ * strbuf_expand or to the *printf family of functions.
+ */
+extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);
+
+/**
+ * Append the given byte size as a human-readable string (i.e. 12.23 KiB,
+ * 3.50 MiB).
+ */
+extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);
+
+/**
+ * Add a formatted string to the buffer.
+ */
+__attribute__((format (printf,2,3)))
+extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+
+/**
+ * Add a formatted string prepended by a comment character and a
+ * blank to the buffer.
+ */
+__attribute__((format (printf, 2, 3)))
+extern void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);
+
+__attribute__((format (printf,2,0)))
+extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
+
+/**
+ * Read a given size of data from a FILE* pointer to the buffer.
+ *
+ * NOTE: The buffer is rewound if the read fails. If -1 is returned,
+ * `errno` must be consulted, like you would do for `read(3)`.
+ * `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline()` has the
+ * same behaviour as well.
+ */
+extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
+
+/**
+ * Read the contents of a given file descriptor. The third argument can be
+ * used to give a hint about the file size, to avoid reallocs.  If read fails,
+ * any partial read is undone */
+extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
+
+/**
+ * Read the contents of a file, specified by its path. The third argument
+ * can be used to give a hint about the file size, to avoid reallocs.
+ */
+extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
+
+/**
+ * Read the target of a symbolic link, specified by its path.  The third
+ * argument can be used to give a hint about the size, to avoid reallocs.
+ */
+extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
+
+/**
+ * Set the buffer to the path of the current working directory.
+ */
+extern int strbuf_getcwd(struct strbuf *sb);
+
+/**
+ * Read a line from a FILE *, overwriting the existing contents
+ * of the strbuf. The second argument specifies the line
+ * terminator character, typically `'\n'`.
+ * Reading stops after the terminator or at EOF.  The terminator
+ * is removed from the buffer before returning.  Returns 0 unless
+ * there was nothing left before EOF, in which case it returns `EOF`.
+ */
+extern int strbuf_getline(struct strbuf *, FILE *, int);
+
+/**
+ * Like `strbuf_getline`, but keeps the trailing terminator (if
+ * any) in the buffer.
+ */
+extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
+
+/**
+ * Like `strbuf_getwholeline`, but operates on a file descriptor.
+ * It reads one character at a time, so it is very slow.  Do not
+ * use it unless you need the correct position in the file
+ * descriptor.
+ */
+extern int strbuf_getwholeline_fd(struct strbuf *, int, int);
+
+/**
+ * Add a path to a buffer, converting a relative path to an
+ * absolute one in the process.  Symbolic links are not
+ * resolved.
+ */
+extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
+
+/**
+ * Strip whitespace from a buffer. The second parameter controls if
+ * comments are considered contents to be removed or not.
+ */
+extern void stripspace(struct strbuf *buf, int skip_comments);
+
+/**
+ * Launch the user preferred editor to edit a file and fill the buffer
+ * with the file's contents upon the user completing their editing. The
+ * third argument can be used to set the environment which the editor is
+ * run in. If the buffer is NULL the editor is launched as usual but the
+ * file's contents are not read into the buffer upon completion.
+ */
+extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
+
 static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 {
 	if (strip_suffix_mem(sb->buf, &sb->len, suffix)) {
@@ -110,52 +495,6 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
  */
 extern void strbuf_list_free(struct strbuf **);
 
-/*----- add data in your buffer -----*/
-static inline void strbuf_addch(struct strbuf *sb, int c)
-{
-	strbuf_grow(sb, 1);
-	sb->buf[sb->len++] = c;
-	sb->buf[sb->len] = '\0';
-}
-
-extern void strbuf_insert(struct strbuf *, size_t pos, const void *, size_t);
-extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);
-
-/* splice pos..pos+len with given data */
-extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,
-                          const void *, size_t);
-
-extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);
-
-extern void strbuf_add(struct strbuf *, const void *, size_t);
-static inline void strbuf_addstr(struct strbuf *sb, const char *s)
-{
-	strbuf_add(sb, s, strlen(s));
-}
-static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
-{
-	strbuf_grow(sb, sb2->len);
-	strbuf_add(sb, sb2->buf, sb2->len);
-}
-extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);
-extern void strbuf_addchars(struct strbuf *sb, int c, size_t n);
-
-typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);
-extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);
-struct strbuf_expand_dict_entry {
-	const char *placeholder;
-	const char *value;
-};
-extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context);
-extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);
-
-__attribute__((format (printf,2,3)))
-extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
-__attribute__((format (printf, 2, 3)))
-extern void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);
-__attribute__((format (printf,2,0)))
-extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
-
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);
 
 /*
@@ -170,28 +509,11 @@ static inline void strbuf_complete_line(struct strbuf *sb)
 		strbuf_addch(sb, '\n');
 }
 
-extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
-/* XXX: if read fails, any partial read is undone */
-extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
-extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
-extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
-extern int strbuf_getcwd(struct strbuf *sb);
-
-extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
-extern int strbuf_getline(struct strbuf *, FILE *, int);
-extern int strbuf_getwholeline_fd(struct strbuf *, int, int);
-
-extern void stripspace(struct strbuf *buf, int skip_comments);
-extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
-
 extern int strbuf_branchname(struct strbuf *sb, const char *name);
 extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
 
 extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
 				    int reserved);
-extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);
-
-extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
 
 __attribute__((format (printf,1,2)))
 extern int printf_ln(const char *fmt, ...);
-- 
2.2.0.454.g7eca6b7

[PATCH 2/4] strbuf.h: drop asciidoc list formatting from API docs

[Jeff King]

Using a hanging indent is much more readable. This means we
won't format as asciidoc anymore, but since we don't have a
working system for extracting these comments anyway, it's
probably more important to just make the source readable.

Signed-off-by: Jeff King <peff@peff.net>
---
 strbuf.h | 74 ++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 37 insertions(+), 37 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 0a83f9a..8f63b38 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -13,44 +13,44 @@
  *
  * strbufs have some invariants that are very important to keep in mind:
  *
- * . The `buf` member is never NULL, so it can be used in any usual C
- * string operations safely. strbuf's _have_ to be initialized either by
- * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.
- * +
- * Do *not* assume anything on what `buf` really is (e.g. if it is
- * allocated memory or not), use `strbuf_detach()` to unwrap a memory
- * buffer from its strbuf shell in a safe way. That is the sole supported
- * way. This will give you a malloced buffer that you can later `free()`.
- * +
- * However, it is totally safe to modify anything in the string pointed by
- * the `buf` member, between the indices `0` and `len-1` (inclusive).
+ *  - The `buf` member is never NULL, so it can be used in any usual C
+ *    string operations safely. strbuf's _have_ to be initialized either by
+ *    `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.
  *
- * . The `buf` member is a byte array that has at least `len + 1` bytes
- *   allocated. The extra byte is used to store a `'\0'`, allowing the
- *   `buf` member to be a valid C-string. Every strbuf function ensure this
- *   invariant is preserved.
- * +
- * NOTE: It is OK to "play" with the buffer directly if you work it this
- *       way:
- * +
- * ----
- * strbuf_grow(sb, SOME_SIZE); <1>
- * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);
- * ----
- * <1> Here, the memory array starting at `sb->buf`, and of length
- * `strbuf_avail(sb)` is all yours, and you can be sure that
- * `strbuf_avail(sb)` is at least `SOME_SIZE`.
- * +
- * NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.
- * +
- * Doing so is safe, though if it has to be done in many places, adding the
- * missing API to the strbuf module is the way to go.
- * +
- * WARNING: Do _not_ assume that the area that is yours is of size `alloc
- * - 1` even if it's true in the current implementation. Alloc is somehow a
- * "private" member that should not be messed with. Use `strbuf_avail()`
- * instead.
- */
+ *    Do *not* assume anything on what `buf` really is (e.g. if it is
+ *    allocated memory or not), use `strbuf_detach()` to unwrap a memory
+ *    buffer from its strbuf shell in a safe way. That is the sole supported
+ *    way. This will give you a malloced buffer that you can later `free()`.
+ *
+ *    However, it is totally safe to modify anything in the string pointed by
+ *    the `buf` member, between the indices `0` and `len-1` (inclusive).
+ *
+ *  - The `buf` member is a byte array that has at least `len + 1` bytes
+ *    allocated. The extra byte is used to store a `'\0'`, allowing the
+ *    `buf` member to be a valid C-string. Every strbuf function ensure this
+ *    invariant is preserved.
+ *
+ *    NOTE: It is OK to "play" with the buffer directly if you work it this
+ *    way:
+ *
+ *    ----
+ *    strbuf_grow(sb, SOME_SIZE); <1>
+ *    strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);
+ *    ----
+ *    <1> Here, the memory array starting at `sb->buf`, and of length
+ *    `strbuf_avail(sb)` is all yours, and you can be sure that
+ *    `strbuf_avail(sb)` is at least `SOME_SIZE`.
+ *
+ *    NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.
+ *
+ *    Doing so is safe, though if it has to be done in many places, adding the
+ *    missing API to the strbuf module is the way to go.
+ *
+ *    WARNING: Do _not_ assume that the area that is yours is of size `alloc
+ *    - 1` even if it's true in the current implementation. Alloc is somehow a
+ *    "private" member that should not be messed with. Use `strbuf_avail()`
+ *    instead.
+*/
 
 /*
  * Data Structures
-- 
2.2.0.454.g7eca6b7

[PATCH 3/4] strbuf.h: format asciidoc code blocks as 4-space indent

[Jeff King]

This is much easier to read when the whole thing is stuffed
inside a comment block. And there is precedent for this
convention in markdown (and just in general ascii text).

Signed-off-by: Jeff King <peff@peff.net>
---
As a side note, I actually find markdown much more pleasant to read and
write than asciidoc. I think the output is not always as nice, as it
does not have some of the structured elements that let us produce nice
manpages, for example. But I wonder if it would be a better choice for
formatting API documentation.

Not something that is really relevant to this series, but just food for
thought if we do end up with an extract+format tool.

 strbuf.h | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 8f63b38..078b805 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -33,10 +33,9 @@
  *    NOTE: It is OK to "play" with the buffer directly if you work it this
  *    way:
  *
- *    ----
- *    strbuf_grow(sb, SOME_SIZE); <1>
- *    strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);
- *    ----
+ *        strbuf_grow(sb, SOME_SIZE); <1>
+ *        strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);
+ *
  *    <1> Here, the memory array starting at `sb->buf`, and of length
  *    `strbuf_avail(sb)` is all yours, and you can be sure that
  *    `strbuf_avail(sb)` is at least `SOME_SIZE`.
@@ -261,9 +260,7 @@ extern void strbuf_add(struct strbuf *, const void *, size_t);
  * NOTE: This function will *always* be implemented as an inline or a macro
  * using strlen, meaning that this is efficient to write things like:
  *
- * ----
- * strbuf_addstr(sb, "immediate string");
- * ----
+ *     strbuf_addstr(sb, "immediate string");
  *
  */
 static inline void strbuf_addstr(struct strbuf *sb, const char *s)
-- 
2.2.0.454.g7eca6b7

[PATCH 4/4] strbuf.h: reorganize api function grouping headers

[Jeff King]

The original API doc had something like:

    Functions
    ---------

    * Life cycle

      ... some life-cycle functions ...

    * Related to the contents of the buffer

      ... functions related to contents ....

    etc

This grouping can be hard to read in the comment sources,
given the "*" in the comment lines, and the amount of text
between each section.

Instead, let's make a flat list of groupings, and underline
each as a section header. That makes them standout, and
eliminates the weird half-phrase of "Related to...". Like:

    Functions related to the contents of the buffer
    -----------------------------------------------

Signed-off-by: Jeff King <peff@peff.net>
---

If you look at the original header file, these groupings actually did
exist (though we did not remotely follow them as functions were added),
and looked like:

  /* ---- content related ---- */

I'd be happy with something like that, too, but went with the style that
matched the "Data Structures" header.

 strbuf.h | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 078b805..8649a0a 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -71,12 +71,8 @@ extern char strbuf_slopbuf[];
 #define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
 
 /**
- * Functions
- * ---------
- */
-
-/**
- * * Life Cycle
+ * Life Cycle Functions
+ * --------------------
  */
 
 /**
@@ -120,7 +116,8 @@ static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)
 
 
 /**
- * * Related to the size of the buffer
+ * Functions related to the size of the buffer
+ * -------------------------------------------
  */
 
 /**
@@ -162,7 +159,8 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)
 
 
 /**
- * * Related to the contents of the buffer
+ * Functions related to the contents of the buffer
+ * -----------------------------------------------
  */
 
 /**
-- 
2.2.0.454.g7eca6b7

Re: [PATCH 1/4] strbuf: migrate api-strbuf.txt documentation to strbuf.h

[Jeff King]

On Fri, Dec 12, 2014 at 04:28:00PM -0500, Jeff King wrote:

>  1. All comments created in this way are started with "/**"
>     to indicate they are part of the API documentation. This
>     may help later with extracting the text to pretty-print
>     it.

By the way, two thoughts I had while working on this.

One, I wonder if it would look nicer to drop the leading " * " from our
comments, especially for long, free-form text blocks. Like:

/**

Data Structures
---------------

This is actual asciidoc that can be rendered. It's not _nearly_ as
ugly to leave the asciidoc intact if you avoid the leading asterisks.

*/

I also wondered if you could spell it as:

#ifdef API_DOCUMENTATION
... some asciidoc goes here...
#endif

but the preprocessor does still look at the contents. I didn't check the
standard, but I suspect an implementation doesn't _have_ to, but gcc
will warn about things like unmatched single-quotes in the content.

And two, the editor support for writing comments like this is not as
good. For instance, vim's "reformat paragraph" command considers the
whole comment to be the paragraph. If you're writing asciidoc inside
the comment field, this quickly becomes annoying. This is a minor issue
that can be fixed (I am sure there is probably a vim package to do it
better already, and that emacs people will tsk tsk and explain why their
editor is better), but I thought I would note it as a potential "con" of
this approach.

-Peff

Re: [PATCH 1/4] strbuf: migrate api-strbuf.txt documentation to strbuf.h

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +/**
> + * * Related to the contents of the buffer
> + */
> +
> +/**
> + * Strip whitespace from the beginning and end of a string.
> + * Equivalent to performing `strbuf_rtrim()` followed by `strbuf_ltrim()`.
> + */
>  extern void strbuf_trim(struct strbuf *);
> +
> +/**
> + * Strip whitespace from the end of a string.
> + */
>  extern void strbuf_rtrim(struct strbuf *);
> +
> +/**
> + * Strip whitespace from the beginning of a string.
> + */
>  extern void strbuf_ltrim(struct strbuf *);

This part reminds me why I often find the current "out of header"
documentation when writing code _without_ the need for _learning_
the API.  The corresponding part in the original is merely these:

    /*----- content related -----*/
    extern void strbuf_trim(struct strbuf *);
    extern void strbuf_rtrim(struct strbuf *);
    extern void strbuf_ltrim(struct strbuf *);
    extern int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);
    ...

It is a lot quicker, scanning this with eyeball, to notice that
there are three related "trim" functions and how they are spelled.
If I wanted to "trim from both ends", knew there were some ways to
"trim" but didn't remember if we had r/ltrim without trim, and/or
was not sure if the "both" version was was _trim or _btrim, scanning
a bland list tends to be much more pleasant.

When you are _learning_ the API, the format that spreads each
individual function into its own section with explanation would be
easier to read through.

Not a strong objection to the overall direction, but I think that we
need to be aware that we are making things harder for some use
cases.

Re: [PATCH 2/4] strbuf.h: drop asciidoc list formatting from API docs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Using a hanging indent is much more readable. This means we
> won't format as asciidoc anymore, but since we don't have a
> working system for extracting these comments anyway, it's
> probably more important to just make the source readable.

Yeah.

Those who want to strip the leading " * " to turn it into AsciiDoc
would need to also parse out the hanging indent and format into
AsciiDoc, which is not entirely an unreasonable thing to ask.

Re: [PATCH 1/4] strbuf: migrate api-strbuf.txt documentation to strbuf.h

[Jonathan Nieder]

Jeff King wrote:

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/technical/api-strbuf.txt | 351 -------------------------
>  strbuf.h                               | 458 ++++++++++++++++++++++++++++-----
>  2 files changed, 390 insertions(+), 419 deletions(-)
>  delete mode 100644 Documentation/technical/api-strbuf.txt

Thanks.  This looks very sensible.  Some suggested tweaks below (see patch at
bottom).

> +++ b/strbuf.h
[...]
> +/**
> + * * Life Cycle
[...]
> +/**
> + * Adding data to the buffer

These were headings like

	* Life Cycle
	* Adding data to the buffer

Most ended up as ' * *' after the conversion, keeping the extra '*', but
'Adding data to the buffer' didn't.

After patch 4/4 they look the same again, so it doesn't matter.

[...]
>   * NOTE: This function will *always* be implemented as an inline or a macro
> - * that expands to:
> - * +
> - * ----
> - * strbuf_add(..., s, strlen(s));
> - * ----
> - * +
> - * Meaning that this is efficient to write things like:
> + * using strlen, meaning that this is efficient to write things like:
>   *
>   * ----
>   * strbuf_addstr(sb, "immediate string");
>   * ----

Makes sense.

[...]
>  /**
>   * Read the contents of a given file descriptor. The third argument can be
> - *     used to give a hint about the file size, to avoid reallocs.
> - */
> + * used to give a hint about the file size, to avoid reallocs.  If read fails,
> + * any partial read is undone */

Missing period at end of sentence.  "*/" should go on the next line.

[...]
> +extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
> +
> +/**
> + * Set the buffer to the path of the current working directory.
> + */
> +extern int strbuf_getcwd(struct strbuf *sb);

This snuck up a few lines --- it was between getwholeline_fd and
add_absolute_path before.  I think it makes sense for getcwd to be
next to add_absolute_path.

[...]
> +extern void stripspace(struct strbuf *buf, int skip_comments);
> +
> +/**
> + * Launch the user preferred editor to edit a file and fill the buffer
> + * with the file's contents upon the user completing their editing. The
> + * third argument can be used to set the environment which the editor is
> + * run in. If the buffer is NULL the editor is launched as usual but the
> + * file's contents are not read into the buffer upon completion.
> + */
> +extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);

split_* were before this before.  Probably launch_editor belongs with
read_file and co instead, but during this conversion it makes sense to
just keep the order from before.

[...]
> - * `strbuf_split_buf`::
> - * `strbuf_split_str`::
> - * `strbuf_split_max`::
> - * `strbuf_split`::
> - * 
> - *     Split a string or strbuf into a list of strbufs at a specified
> - *     terminator character.  The returned substrings include the
> - *     terminator characters.  Some of these functions take a `max`
> - *     parameter, which, if positive, limits the output to that
> - *     number of substrings.
> - * 
> - * `strbuf_list_free`::
> - * 
> - *     Free a list of strbufs (for example, the return values of the
> - *     `strbuf_split()` functions).
> +/*
> + * Split str (of length slen) at the specified terminator character.
> + * Return a null-terminated array of pointers to strbuf objects
> + * holding the substrings.  The substrings include the terminator,
> + * except for the last substring, which might be unterminated if the
[...]
> + */
> +extern struct strbuf **strbuf_split_buf(const char *, size_t,
> +                                       int terminator, int max);
> +
> +/*
> + * Split a NUL-terminated string at the specified terminator
> + * character.  See strbuf_split_buf() for more information.
> + */
> +static inline struct strbuf **strbuf_split_str(const char *str,
> +                                              int terminator, int max)
> +{
> +       return strbuf_split_buf(str, strlen(str), terminator, max);
> +}
> +
> +/*
> + * Split a strbuf at the specified terminator character.  See
> + * strbuf_split_buf() for more information.
> + */
> +static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,

This is something that the flat file format did better.  Would it work to
do something similar, for example by dropping the boilerplate comment
before the inline functions (not shown in the patch below)?

	/**
	 * Split a string or strbuf into a list of strbufs at a specified
	 * terminator character.  [...]
	 */
	extern struct strbuf **strbuf_split_buf(const char *, size_t, int terminator, int max);

	static inline struct strbuf **strbuf_split_str(const char *str, int terminator, int max)
	{
		return strbuf_split_buf(str, strlen(str), terminator, max);
	}

	static inline struct strbuf **strbuf_split_max(const struct strbuf *sb, int terminator, int max)
	{
		return strbuf_split_buf(sb->buf, sb->len, terminator, max);
	}

	static inline struct strbuf **strbuf_split(const struct strbuf *sb, int terminator)
	{
		return strbuf_split_max(sb, terminator, 0);
	}

	/**
	 * Free a list of strbufs (for example, the return values of the
	 * `strbuf_split()` functions).
	 */
	extern void strbuf_list_free(struct strbuf **);

 strbuf.h | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 0a83f9a..108644e 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -202,7 +202,7 @@ extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
 
 
 /**
- * Adding data to the buffer
+ * * Adding data to the buffer
  *
  * NOTE: All of the functions in this section will grow the buffer as
  * necessary.  If they fail for some reason other than memory shortage and the
@@ -368,7 +368,8 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 /**
  * Read the contents of a given file descriptor. The third argument can be
  * used to give a hint about the file size, to avoid reallocs.  If read fails,
- * any partial read is undone */
+ * any partial read is undone.
+ */
 extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);
 
 /**
@@ -384,11 +385,6 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
 extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);
 
 /**
- * Set the buffer to the path of the current working directory.
- */
-extern int strbuf_getcwd(struct strbuf *sb);
-
-/**
  * Read a line from a FILE *, overwriting the existing contents
  * of the strbuf. The second argument specifies the line
  * terminator character, typically `'\n'`.
@@ -413,6 +409,11 @@ extern int strbuf_getwholeline(struct strbuf *, FILE *, int);
 extern int strbuf_getwholeline_fd(struct strbuf *, int, int);
 
 /**
+ * Set the buffer to the path of the current working directory.
+ */
+extern int strbuf_getcwd(struct strbuf *sb);
+
+/**
  * Add a path to a buffer, converting a relative path to an
  * absolute one in the process.  Symbolic links are not
  * resolved.
@@ -425,15 +426,6 @@ extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
  */
 extern void stripspace(struct strbuf *buf, int skip_comments);
 
-/**
- * Launch the user preferred editor to edit a file and fill the buffer
- * with the file's contents upon the user completing their editing. The
- * third argument can be used to set the environment which the editor is
- * run in. If the buffer is NULL the editor is launched as usual but the
- * file's contents are not read into the buffer upon completion.
- */
-extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
-
 static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 {
 	if (strip_suffix_mem(sb->buf, &sb->len, suffix)) {
@@ -495,6 +487,15 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
  */
 extern void strbuf_list_free(struct strbuf **);
 
+/**
+ * Launch the user preferred editor to edit a file and fill the buffer
+ * with the file's contents upon the user completing their editing. The
+ * third argument can be used to set the environment which the editor is
+ * run in. If the buffer is NULL the editor is launched as usual but the
+ * file's contents are not read into the buffer upon completion.
+ */
+extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
+
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);
 
 /*

Re: [PATCH] document string_list_clear

[Stefan Beller]

On Fri, Dec 12, 2014 at 04:27:26PM -0500, Jeff King wrote:
> On Fri, Dec 12, 2014 at 11:35:52AM -0800, Jonathan Nieder wrote:
> 
> > If I were doing it, I would first de-asciidoc within technical/ and
> > then move into the header in a separate patch.  Or first move with
> > asciidoc intact and then de-asciidoc in a separate patch.  Combining
> > the two into a single patch is also fine.  Please don't change wording
> > at the same time.
> 
> Here's what I came up with. The first patch probably does more than
> you'd like (and more than I would have done if I were starting from
> scratch today). But I didn't want to get into undoing the stripping of
> each function-name list item that I showed earlier, as it would be a lot
> of tedious manual work. If we decide we want to keep those, I'm happy to
> do the work to restore them, but it didn't seem like a good tradeoff
> just to create an intermediate state to make the patch prettier.
> 
> I did split out some of the other formatting decisions, though, so they
> can be evaluated individually.
> 
>   [1/4]: strbuf: migrate api-strbuf.txt documentation to strbuf.h

(optional nit):
The subject of this patch is slightly different than the others.
How about: 
strbuf.h: integrate api-strbuf.txt documentation

>   [2/4]: strbuf.h: drop asciidoc list formatting from API docs
>   [3/4]: strbuf.h: format asciidoc code blocks as 4-space indent
>   [4/4]: strbuf.h: reorganize api function grouping headers
> 
> -Peff

I have reviewed the series and I personally like it as I am more
often in the learners/discovery stage than the fast lookup stage
as Junio mentioned.

Another inconsistency I found specifically related to the strbuf.h
documentation is the apparent changeing style.

At the beginning we have 
	extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);

Even with the documentation attached, which size_t is the length and what the 
amount of allocated memory? Later we have more explaining names, such as

extern struct strbuf **strbuf_split_buf(const char *, size_t,
					int terminator, int max);


Please append the following patch to your series. I noticed this as
the text editor I use uses different colors for 

	/** comments starting with double stars used with i.e. doxygen */
	/* random comments in the file */

Thanks,
Stefan


-----------8<------------------
From 5d0f98aba65e8b1415ebfcd8e06b67203e9305a7 Mon Sep 17 00:00:00 2001
From: Stefan Beller <sbeller@google.com>
Date: Fri, 12 Dec 2014 14:27:15 -0800
Subject: [PATCH] strbuf.h: Unify documentation comments beginnings

We want to preserve the meaning of "/**" to start a documentary
in depth comment, so all of the documenting comments should start with
a "/**".

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 strbuf.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/strbuf.h b/strbuf.h
index 8649a0a..95d49e1 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -438,7 +438,7 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 		return 0;
 }
 
-/*
+/**
  * Split str (of length slen) at the specified terminator character.
  * Return a null-terminated array of pointers to strbuf objects
  * holding the substrings.  The substrings include the terminator,
@@ -454,7 +454,7 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
 extern struct strbuf **strbuf_split_buf(const char *, size_t,
 					int terminator, int max);
 
-/*
+/**
  * Split a NUL-terminated string at the specified terminator
  * character.  See strbuf_split_buf() for more information.
  */
@@ -464,7 +464,7 @@ static inline struct strbuf **strbuf_split_str(const char *str,
 	return strbuf_split_buf(str, strlen(str), terminator, max);
 }
 
-/*
+/**
  * Split a strbuf at the specified terminator character.  See
  * strbuf_split_buf() for more information.
  */
@@ -474,7 +474,7 @@ static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,
 	return strbuf_split_buf(sb->buf, sb->len, terminator, max);
 }
 
-/*
+/**
  * Split a strbuf at the specified terminator character.  See
  * strbuf_split_buf() for more information.
  */
@@ -484,7 +484,7 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,
 	return strbuf_split_max(sb, terminator, 0);
 }
 
-/*
+/**
  * Free a NULL-terminated list of strbufs (for example, the return
  * values of the strbuf_split*() functions).
  */
@@ -492,7 +492,7 @@ extern void strbuf_list_free(struct strbuf **);
 
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);
 
-/*
+/**
  * Append s to sb, with the characters '<', '>', '&' and '"' converted
  * into XML entities.
  */
-- 
2.2.0.38.g71d6cb7

Re: [PATCH 2/4] strbuf.h: drop asciidoc list formatting from API docs

[Jonathan Nieder]

Jeff King wrote:

> Using a hanging indent is much more readable. This means we
> won't format as asciidoc anymore, but since we don't have a
> working system for extracting these comments anyway, it's
> probably more important to just make the source readable.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  strbuf.h | 74 ++++++++++++++++++++++++++++++++--------------------------------
>  1 file changed, 37 insertions(+), 37 deletions(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH 3/4] strbuf.h: format asciidoc code blocks as 4-space indent

[Jonathan Nieder]

Jeff King wrote:

> This is much easier to read when the whole thing is stuffed
> inside a comment block. And there is precedent for this
> convention in markdown (and just in general ascii text).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

> As a side note, I actually find markdown much more pleasant to read and
> write than asciidoc.

I do, too.  Quoting in asciidoc is a nightmare.

Thanks,
Jonathan

Re: [PATCH 4/4] strbuf.h: reorganize api function grouping headers

[Jonathan Nieder]

Jeff King wrote:

> If you look at the original header file, these groupings actually did
> exist (though we did not remotely follow them as functions were added),
> and looked like:
>
>   /* ---- content related ---- */
>
> I'd be happy with something like that, too, but went with the style that
> matched the "Data Structures" header.

I'd be happy either way too.

>  strbuf.h | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)

This is missing the "Adding data to the buffer" section.

Except for that,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 3/4] strbuf.h: format asciidoc code blocks as 4-space indent

[Michael Haggerty]

On 12/12/2014 11:39 PM, Jonathan Nieder wrote:
> Jeff King wrote:
> 
>> This is much easier to read when the whole thing is stuffed
>> inside a comment block. And there is precedent for this
>> convention in markdown (and just in general ascii text).
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
>> As a side note, I actually find markdown much more pleasant to read and
>> write than asciidoc.
> 
> I do, too.  Quoting in asciidoc is a nightmare.

Peff, thanks for working on this. I think it is a definite improvement.

I suggest that we accept the use of asciidoc/markdown's convention of
using backwards quotes to mark code snippets (especially identifier
names) within comments *anywhere* in our code base. For example, this
appears in refs.c:

    /*
     * Create a struct ref_entry object for the specified dirname.
     * dirname is the name of the directory with a trailing slash
     * (e.g., "refs/heads/") or "" for the top-level directory.
     */

I claim that it is more readable with a tiny bit of markup:

    /*
     * Create a `struct ref_entry` object for the specified `dirname`.
     * `dirname` is the name of the directory with a trailing slash
     * (e.g., "refs/heads/") or "" for the top-level directory.
     */

Marking up `struct ref_entry` helps make it clear that the two words
belong together, and marking up `dirname` makes it clear that we are
talking about a specific identifier (in this case, a function parameter).

Currently, comments use a mix of unadorned text, single-quoted text, and
double-quoted text when talking about code. I think the
asciidoc/markdown convention is clearer [1].

I think we shouldn't be pedantic about this. When a comment is readable
with no markup, there's no need to add markup. And "incorrect" markup
shouldn't by itself be reason to reject a patch. But in many examples, a
little bit of markup makes the text less ambiguous and easier to read.

Michael

[1] Yes, I see the irony in trying to improve a mixture of three
    conventions by adding a fourth one.

-- 
Michael Haggerty
mhagger@alum.mit.edu