{"thread":{"id":"38124","subject":"[PATCH] document string_list_clear","startedAt":"2014-12-06T01:51:18Z","lastAt":"2014-12-14T17:42:26Z","messageCount":49,"participants":["Stefan Beller","Jonathan Nieder","Jeff King","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"253215","messageId":"1417830678-16115-1-git-send-email-sbeller@google.com","threadId":"38124","inReplyTo":null,"subject":"[PATCH] document string_list_clear","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-06T01:51:18Z","receivedAt":"2014-12-06T01:51:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n\n Just stumbled accross this one and wasn't sure if it also frees up\n the memory involved.\n\n string-list.h | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/string-list.h b/string-list.h\nindex 494eb5d..99ecc44 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -21,6 +21,11 @@ struct string_list {\n void string_list_init(struct string_list *list, int strdup_strings);\n \n void print_string_list(const struct string_list *p, const char *text);\n+\n+/*\n+ * Clears the string list, so it has zero items. All former items are freed.\n+ * If free_util is true, all util pointers are also freed.\n+ */\n void string_list_clear(struct string_list *list, int free_util);\n \n /* Use this function to call a custom clear function on each util pointer */\n-- \n2.2.0\n"},{"id":"253216","messageId":"20141206020458.GR16345@google.com","threadId":"38124","inReplyTo":"1417830678-16115-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-06T02:04:58Z","receivedAt":"2014-12-06T02:04:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Signed-off-by: Stefan Beller \n> ---\n>\n> Just stumbled accross this one and wasn't sure if it also frees up\n> the memory involved.\n>\n> string-list.h | 5 +++++\n> 1 file changed, 5 insertions(+)\n\nSounds reasonable. Documentation/technical/api-string-list.txt\ndocuments these functions more fully. The right balance between\ndocumenting things in two places vs. adding \"see also\" pointers vs.\njust putting the highlights in one of the two places isn't obvious to\nme.\n\n[...]\n> --- a/string-list.h\n> +++ b/string-list.h\n> @@ -21,6 +21,11 @@ struct string_list {\n> void string_list_init(struct string_list *list, int strdup_strings);\n> \n> void print_string_list(const struct string_list *p, const char *text);\n> +\n> +/*\n> + * Clears the string list, so it has zero items. All former items are freed.\n> + * If free_util is true, all util pointers are also freed.\n> + */\n> void string_list_clear(struct string_list *list, int free_util);\n\nThe api doc says\n\n\tFree a string_list. The `string` pointer of the items will be freed in\n\tcase the `strdup_strings` member of the string_list is set. The second\n\tparameter controls if the `util` pointer of the items should be freed\n\tor not.\n\nOne option here would be to say\n\n\tFree a string_list. See Documentation/technical/api-string-list.txt\n\tfor details.\n\nThat reminds me: why do we call this string_list_clear instead of\nstring_list_free?\n\nThanks,\nJonathan\n"},{"id":"253230","messageId":"20141206052752.GD31301@peff.net","threadId":"38124","inReplyTo":"20141206020458.GR16345@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-06T05:27:52Z","receivedAt":"2014-12-06T05:27:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 05, 2014 at 06:04:58PM -0800, Jonathan Nieder wrote:\n\n> That reminds me: why do we call this string_list_clear instead of\n> string_list_free?\n\nBecause it does not free the struct itself, and because you can then use\nthe result again. I think we try to draw a distinction between:\n\n /* Free resources, but do not reinitialize! */\n void string_list_free_data(struct string_list *s)\n {\n\tint i;\n\tfor (i = 0; i < s->nr; i++)\n\t\tfree(s->items[i].string);\n\tfree(s->items[i];\n }\n\n /* Free resources, and go back to initialized but empty state */\n void string_list_clear(struct string_list *s)\n {\n\tstring_list_free_data(s);\n\ts->nr = s->alloc = 0;\n\ts->items = NULL;\n }\n\n /* Free all resources from dynamically allocated structure */\n void string_list_free(struct string_list *s)\n {\n\tstring_list_clear(s);\n\tfree(s);\n }\n\nIdeally we use consistent names to distinguish between them. We are not\nalways consistent, though (probably strbuf_release should be called\nstrbuf_clear for consistency). But I think we are fairly consistent that\n\"_free()\" means \"...and free the pointer, too\".\n\nIn general, we try to avoid the first as a public interface (because it\nis error-prone when somebody tries to reuse the list, and the extra work\nof zero-ing the pointer is not enough to care about).\n\nWe also tend to avoid the third, because it is quite often not the\nbusiness of the object whether it was dynamically constructed or not\n(exceptions tend to be node-oriented structures like linked lists and\ntrees; c.f. cache_tree and commit_list).\n\nMaybe that should go into CodingGuidelines? It's not really style,\nexactly, but it is convention.\n\n-Peff\n"},{"id":"253231","messageId":"20141206053024.GE31301@peff.net","threadId":"38124","inReplyTo":"20141206020458.GR16345@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-06T05:30:24Z","receivedAt":"2014-12-06T05:30:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 05, 2014 at 06:04:58PM -0800, Jonathan Nieder wrote:\n\n> Stefan Beller wrote:\n> \n> > Signed-off-by: Stefan Beller \n> > ---\n> >\n> > Just stumbled accross this one and wasn't sure if it also frees up\n> > the memory involved.\n> >\n> > string-list.h | 5 +++++\n> > 1 file changed, 5 insertions(+)\n> \n> Sounds reasonable. Documentation/technical/api-string-list.txt\n> documents these functions more fully. The right balance between\n> documenting things in two places vs. adding \"see also\" pointers vs.\n> just putting the highlights in one of the two places isn't obvious to\n> me.\n\nAlso, I forgot to mention: if we consistently put the API docs into the\nheader files and extracted it automatically into standalone documents,\nwe would not need to have two places.\n\nThis is something I've been meaning to look at for a long time, but it\nnever quite makes the priority list. And my past experiences with tools\nlike doxygen has been that they are complicated and have a lot of\ndependencies. It's been a long time since I've tried, though.\n\n-Peff\n"},{"id":"253432","messageId":"xmqq7fy0mx70.fsf@gitster.dls.corp.google.com","threadId":"38124","inReplyTo":"20141206020458.GR16345@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-09T19:37:23Z","receivedAt":"2014-12-09T19:37:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> +/*\n>> + * Clears the string list, so it has zero items. All former items are freed.\n>> + * If free_util is true, all util pointers are also freed.\n>> + */\n>> void string_list_clear(struct string_list *list, int free_util);\n>\n> The api doc says\n>\n> \tFree a string_list. The `string` pointer of the items will be freed in\n> \tcase the `strdup_strings` member of the string_list is set. The second\n> \tparameter controls if the `util` pointer of the items should be freed\n> \tor not.\n>\n> One option here would be to say\n>\n> \tFree a string_list. See Documentation/technical/api-string-list.txt\n> \tfor details.\n\nIf we later introduce string_list_free() that is in line with the\ndistinction between \"free\" and \"clear\" discussed on this thread, the\ncomment for this function needs to be fixed to \"Clear the string\nlist. See $doc\". and that is not much different from \"See $doc\"\nwithout the first sentence which is the function name.\n\nPerhaps the API doc that currently says \"Free\" is the only thing\nthat needs fixing? And perhaps add \"See $doc\" at the beginning of\nthe header and remove duplicated comments we already have in the\nfile?\n"},{"id":"253433","messageId":"xmqq388omwzr.fsf@gitster.dls.corp.google.com","threadId":"38124","inReplyTo":"20141206053024.GE31301@peff.net","subject":"Re: [PATCH] document string_list_clear","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-09T19:41:44Z","receivedAt":"2014-12-09T19:41:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Also, I forgot to mention: if we consistently put the API docs into the\n> header files and extracted it automatically into standalone documents,\n> we would not need to have two places.\n>\n> This is something I've been meaning to look at for a long time, but it\n> never quite makes the priority list. And my past experiences with tools\n> like doxygen has been that they are complicated and have a lot of\n> dependencies. It's been a long time since I've tried, though.\n\nYeah, that is a thought.\n\nSome existing documentation pages like allocation-growing may not be\na good match for this strategy, though; cache.h has a lot more than\njust alloc-grow, and there needs to be a way to generate more than\none API docs from a single header (and vice versa, taking text from\nmore than one source for a single API, I suspect).\n"},{"id":"253435","messageId":"CAGZ79kbk4SXEXKzn-V8c4zCQU8m8ub+VkKhmub-bFoLZT1WWpA@mail.gmail.com","threadId":"38124","inReplyTo":"xmqq7fy0mx70.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-09T19:48:02Z","receivedAt":"2014-12-09T19:48:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 9, 2014 at 11:37 AM, Junio C Hamano wrote:\n>\n> Perhaps the API doc that currently says \"Free\" is the only thing\n> that needs fixing? And perhaps add \"See $doc\" at the beginning of\n> the header and remove duplicated comments we already have in the\n> file?\n\nThe reason I wrote this patch originally was because I seem to forget we have\nmore than one place to document our APIs. If there are comments in the header\nI seem to have thought it were the only place where we have documentation.\nBut as we have some comments in the header for about half the functions there,\nI missed looking into the Documentation at all.\n\nI'd support Jeffs efforts to only write doc into the headers and then\nexport it or just\nhave documentation in the Documentation/technical dir. If there is\nonly one way to obtain\nthe information I wouldn't send in patches duplicating information.\n\nPersonally I first visit the header files to see if there is\ndocumentation/comments on the\ndesired function. Looking at Documentation/technical/ is only reserved\nfor high lievel discussion on\nAPI choices, so everything not prefixed with \"api-\" there.\n"},{"id":"253438","messageId":"20141209200014.GX16345@google.com","threadId":"38124","inReplyTo":"xmqq7fy0mx70.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-09T20:00:14Z","receivedAt":"2014-12-09T20:00:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>>> +/*\n>>> + * Clears the string list, so it has zero items. All former items are freed.\n>>> + * If free_util is true, all util pointers are also freed.\n>>> + */\n>>> void string_list_clear(struct string_list *list, int free_util);\n>>\n>> The api doc says\n>>\n>> \tFree a string_list. The `string` pointer of the items will be freed in\n>> \tcase the `strdup_strings` member of the string_list is set. The second\n>> \tparameter controls if the `util` pointer of the items should be freed\n>> \tor not.\n>>\n>> One option here would be to say\n>>\n>> \tFree a string_list. See Documentation/technical/api-string-list.txt\n>> \tfor details.\n>\n> If we later introduce string_list_free() that is in line with the\n> distinction between \"free\" and \"clear\" discussed on this thread, the\n> comment for this function needs to be fixed to \"Clear the string\n> list. See $doc\". and that is not much different from \"See $doc\"\n> without the first sentence which is the function name.\n\nI still find the term \"clear\" to be confusing here. It makes me think\nthe function will be analagous to strbuf_reset, when it's actually\nanalagous to strbuf_release.\n\nIn other words, the important thing is that this frees all members of\nthe string_list. There might be a clearer way to say that. The\nstring_list itself may not be dynamically allocated --- I'm not sure\nwhat it would mean to free it.\n\nHow about string_list_release?\n\nI think I could get used to \"clear\" if we used it consistently though.\nI suspect anything we do will be confusing, unless we make them\nconsistent.\n\n> Perhaps the API doc that currently says \"Free\" is the only thing\n> that needs fixing?\n\nI find the API doc to be pretty clear, actually. If someone was\nconfused in practice then I'd be happy to try to debug the wording.\n\n> And perhaps add \"See $doc\" at the beginning of\n> the header and remove duplicated comments we already have in the\n> file?\n\nYes, that sounds like a good way to go about it.\n\nThanks,\nJonathan\n"},{"id":"253441","messageId":"20141209201551.GA12001@peff.net","threadId":"38124","inReplyTo":"xmqq388omwzr.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-09T20:15:51Z","receivedAt":"2014-12-09T20:15:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 09, 2014 at 11:41:44AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Also, I forgot to mention: if we consistently put the API docs into the\n> > header files and extracted it automatically into standalone documents,\n> > we would not need to have two places.\n> >\n> > This is something I've been meaning to look at for a long time, but it\n> > never quite makes the priority list. And my past experiences with tools\n> > like doxygen has been that they are complicated and have a lot of\n> > dependencies. It's been a long time since I've tried, though.\n> \n> Yeah, that is a thought.\n> \n> Some existing documentation pages like allocation-growing may not be\n> a good match for this strategy, though; cache.h has a lot more than\n> just alloc-grow, and there needs to be a way to generate more than\n> one API docs from a single header (and vice versa, taking text from\n> more than one source for a single API, I suspect).\n\nI think that would be a matter of tool support for saying \"now I am\noutputting api-foo\" in the inline documentation[1]. It does make writing\nMakefiles a lot harder, though, if there isn't a one-to-one\ncorrespondence between targets and their sources. Perhaps it is a sign\nthat we should split our include files more along functional boundaries.\nIf we cannot make sane documentation out of the file, then it is\nprobably confusing for humans to read it.\n\nOpening cache.h shows it starting with git_{in,de}flate_*, and then\nDEFAULT_GIT_PORT!? It has long been a dumping ground for random global\nfunctions and definitions. If we are going to document subsystems,\nsplitting them into their own files seems sensible. Skimming through the\nfile, some of the splits seem pretty easy and obvious.\n\n-Peff\n\n[1] I do not have a tool in mind, so this is something to think about\n while evaluating them. It seems like just parsing:\n\n /**\n * All of this will get dumped in a file (sans asterisks),\n * which can then be fed to asciidoc. The \"**\" marker\n * starts the docstring, which ends at the normal comment\n * boundary.\n */\n\n would be an easy place to start, and by implementing it ourselves\n with sed we would be free to implement any other rules we wanted.\n That is probably too naive, though. It would be nice if the tool\n actually understood C function declarations, so that we would not\n have to repeat them manually in the comments.\n\n I guess all tools like doxygen probably started off as \"just pull\n out and format the comments\", and then grew and grew in complexity. :)\n"},{"id":"253442","messageId":"20141209201713.GY16345@google.com","threadId":"38124","inReplyTo":"CAGZ79kbk4SXEXKzn-V8c4zCQU8m8ub+VkKhmub-bFoLZT1WWpA@mail.gmail.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-09T20:17:13Z","receivedAt":"2014-12-09T20:17:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Tue, Dec 9, 2014 at 11:37 AM, Junio C Hamano wrote:\n\n>> Perhaps the API doc that currently says \"Free\" is the only thing\n>> that needs fixing? And perhaps add \"See $doc\" at the beginning of\n>> the header and remove duplicated comments we already have in the\n>> file?\n>\n> The reason I wrote this patch originally was because I seem to forget we have\n> more than one place to document our APIs. If there are comments in the header\n> I seem to have thought it were the only place where we have documentation.\n\nHow about this patch?\n\n-- >8 --\nSubject: put strbuf API documentation in one place\n\nv1.8.1-rc0~61^2 (strbuf_split*(): document functions, 2012-11-04)\nadded some nice API documentation for a few functions to strbuf.h, to\ncomplement the documentation at Documentation/technical/api-strbuf.\nThat was helpful because it meant one less hop for someone reading the\nheader to find API documentation.\n\nIn practice, unfortunately, it is too hard to remember that there\nis documentation in two places. The longer documentation comments\nin the header made Documentation/technical/api-strbuf less\ndiscoverable. So move the information to\nDocumentation/technical/api-strbuf and drop the long comments.\n\nHopefully in the long term we will find a good way to\ngenerate well organized Documentation/technical/api-* documents\nfrom comments in headers and this problem will be eliminated\ncompletely.\n\nShort reminders in the header file are still okay.\n\nReported-by: Stefan Beller \nSigned-off-by: Jonathan Nieder \n---\n Documentation/technical/api-strbuf.txt | 20 +++++++++++++++--\n strbuf.h | 40 ++--------------------------------\n 2 files changed, 20 insertions(+), 40 deletions(-)\n\ndiff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\nindex cca6543..a43facc 100644\n--- a/Documentation/technical/api-strbuf.txt\n+++ b/Documentation/technical/api-strbuf.txt\n@@ -252,6 +252,11 @@ which can be used by the programmer of the callback as she sees fit.\n \tdestination. This is useful for literal data to be fed to either\n \tstrbuf_expand or to the *printf family of functions.\n \n+`strbuf_addstr_xml_quoted`::\n+\n+\tAppend a string to a strbuf with the characters '<', '>', '&', and\n+\t'\"' converted into XML entities.\n+\n `strbuf_humanise_bytes`::\n \n \tAppend the given byte size as a human-readable string (i.e. 12.23 KiB,\n@@ -266,6 +271,13 @@ which can be used by the programmer of the callback as she sees fit.\n \tAdd a formatted string prepended by a comment character and a\n \tblank to the buffer.\n \n+`xstrfmt`::\n+`xstrvfmt`::\n+\n+\tCreate a newly allocated string using printf format. You can do\n+\tthis easily with a strbuf, but this provides a shortcut to save a\n+\tfew lines.\n+\n `strbuf_fread`::\n \n \tRead a given size of data from a FILE* pointer to the buffer.\n@@ -336,11 +348,15 @@ same behaviour as well.\n \tterminator characters. Some of these functions take a `max`\n \tparameter, which, if positive, limits the output to that\n \tnumber of substrings.\n++\n+For lighter-weight alternatives, see `string_list_split` and\n+`string_list_split_in_place` from the\n+link:api-string-list.html[string list API].\n \n `strbuf_list_free`::\n \n-\tFree a list of strbufs (for example, the return values of the\n-\t`strbuf_split()` functions).\n+\tFree a NULL-terminated list of strbufs (for example, the return\n+\tvalues of the `strbuf_split()` functions).\n \n `launch_editor`::\n \ndiff --git a/strbuf.h b/strbuf.h\nindex 652b6c4..f3c9bb6 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -58,56 +58,27 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n \t\treturn 0;\n }\n \n-/*\n- * Split str (of length slen) at the specified terminator character.\n- * Return a null-terminated array of pointers to strbuf objects\n- * holding the substrings. The substrings include the terminator,\n- * except for the last substring, which might be unterminated if the\n- * original string did not end with a terminator. If max is positive,\n- * then split the string into at most max substrings (with the last\n- * substring containing everything following the (max-1)th terminator\n- * character).\n- *\n- * For lighter-weight alternatives, see string_list_split() and\n- * string_list_split_in_place().\n- */\n-extern struct strbuf **strbuf_split_buf(const char *, size_t,\n+extern struct strbuf **strbuf_split_buf(const char *str, size_t slen,\n \t\t\t\t\tint terminator, int max);\n \n-/*\n- * Split a NUL-terminated string at the specified terminator\n- * character. See strbuf_split_buf() for more information.\n- */\n static inline struct strbuf **strbuf_split_str(const char *str,\n \t\t\t\t\t int terminator, int max)\n {\n \treturn strbuf_split_buf(str, strlen(str), terminator, max);\n }\n \n-/*\n- * Split a strbuf at the specified terminator character. See\n- * strbuf_split_buf() for more information.\n- */\n static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,\n \t\t\t\t\t\tint terminator, int max)\n {\n \treturn strbuf_split_buf(sb->buf, sb->len, terminator, max);\n }\n \n-/*\n- * Split a strbuf at the specified terminator character. See\n- * strbuf_split_buf() for more information.\n- */\n static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n \t\t\t\t\t int terminator)\n {\n \treturn strbuf_split_max(sb, terminator, 0);\n }\n \n-/*\n- * Free a NULL-terminated list of strbufs (for example, the return\n- * values of the strbuf_split*() functions).\n- */\n extern void strbuf_list_free(struct strbuf **);\n \n /*----- add data in your buffer -----*/\n@@ -158,10 +129,7 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n \n extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);\n \n-/*\n- * Append s to sb, with the characters '<', '>', '&' and '\"' converted\n- * into XML entities.\n- */\n+/* append s with <, >, &, and \" converted to XML entities */\n extern void strbuf_addstr_xml_quoted(struct strbuf *sb, const char *s);\n \n static inline void strbuf_complete_line(struct strbuf *sb)\n@@ -200,10 +168,6 @@ extern int fprintf_ln(FILE *fp, const char *fmt, ...);\n \n char *xstrdup_tolower(const char *);\n \n-/*\n- * Create a newly allocated string using printf format. You can do this easily\n- * with a strbuf, but this provides a shortcut to save a few lines.\n- */\n __attribute__((format (printf, 1, 0)))\n char *xstrvfmt(const char *fmt, va_list ap);\n __attribute__((format (printf, 1, 2)))\n-- \n2.2.0.rc0.207.ga3a616c\n"},{"id":"253446","messageId":"20141209202152.GZ16345@google.com","threadId":"38124","inReplyTo":"20141209201551.GA12001@peff.net","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-09T20:21:52Z","receivedAt":"2014-12-09T20:21:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Dec 09, 2014 at 11:41:44AM -0800, Junio C Hamano wrote:\n\n>> Yeah, that is a thought.\n>>\n>> Some existing documentation pages like allocation-growing may not be\n>> a good match for this strategy, though; cache.h has a lot more than\n>> just alloc-grow, and there needs to be a way to generate more than\n>> one API docs from a single header (and vice versa, taking text from\n>> more than one source for a single API, I suspect).\n>\n> I think that would be a matter of tool support for saying \"now I am\n> outputting api-foo\" in the inline documentation[1]. It does make writing\n> Makefiles a lot harder, though, if there isn't a one-to-one\n> correspondence between targets and their sources. Perhaps it is a sign\n> that we should split our include files more along functional boundaries.\n\nYeah, I think a separate alloc-grow.h would be fine.\n\nThanks,\nJonathan\n"},{"id":"253449","messageId":"20141209202738.GC12001@peff.net","threadId":"38124","inReplyTo":"20141209201713.GY16345@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-09T20:27:38Z","receivedAt":"2014-12-09T20:27:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 09, 2014 at 12:17:13PM -0800, Jonathan Nieder wrote:\n\n> Stefan Beller wrote:\n> > On Tue, Dec 9, 2014 at 11:37 AM, Junio C Hamano wrote:\n> \n> >> Perhaps the API doc that currently says \"Free\" is the only thing\n> >> that needs fixing? And perhaps add \"See $doc\" at the beginning of\n> >> the header and remove duplicated comments we already have in the\n> >> file?\n> >\n> > The reason I wrote this patch originally was because I seem to forget we have\n> > more than one place to document our APIs. If there are comments in the header\n> > I seem to have thought it were the only place where we have documentation.\n> \n> How about this patch?\n> \n> -- >8 --\n> Subject: put strbuf API documentation in one place\n> \n> v1.8.1-rc0~61^2 (strbuf_split*(): document functions, 2012-11-04)\n> added some nice API documentation for a few functions to strbuf.h, to\n> complement the documentation at Documentation/technical/api-strbuf.\n> That was helpful because it meant one less hop for someone reading the\n> header to find API documentation.\n> \n> In practice, unfortunately, it is too hard to remember that there\n> is documentation in two places. The longer documentation comments\n> in the header made Documentation/technical/api-strbuf less\n> discoverable. So move the information to\n> Documentation/technical/api-strbuf and drop the long comments.\n> \n> Hopefully in the long term we will find a good way to\n> generate well organized Documentation/technical/api-* documents\n> from comments in headers and this problem will be eliminated\n> completely.\n> \n> Short reminders in the header file are still okay.\n\nI somewhat feel this goes in the opposite direction of where we want to\nbe (all data in one place, but that place is the header). Your patch\nmight make api-strbuf more discoverable, but it also vastly increases\nthe chances of function getting out of sync with their documentation, or\nnew functions being added without getting documented (very often the\npresence of other documentation in the comments is enough to guilt me\ninto writing it for new ones).\n\nElsewhere I mentioned a tool to extract comments and format them. But do\npeople actually care about the formatting step? Does anybody asciidoc\nthe technical/api-* files? We did not even support building them until\nsometime in 2012. Personally, I only ever view them as text.\n\nIn which case can we simply start migrating api-strbuf.txt into\nin-header comments, without worrying about a parsing tool?\n\n-Peff\n"},{"id":"253451","messageId":"CAGZ79kZa5sPxbzURtGScCrfbTaCX_cHiRj7uew6Xk+1Df7oTEQ@mail.gmail.com","threadId":"38124","inReplyTo":"20141209202738.GC12001@peff.net","subject":"Re: [PATCH] document string_list_clear","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-09T20:32:15Z","receivedAt":"2014-12-09T20:32:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 9, 2014 at 12:27 PM, Jeff King wrote:\n\n>\n> In which case can we simply start migrating api-strbuf.txt into\n> in-header comments, without worrying about a parsing tool?\n>\n> -Peff\n\nThat would be my preference as well. Putting the API documentation\ninto the header files. Optionally if you're fancy you may enjoy the generated\nAPI documentation. But I personally would rather look into the header for\nthe most up to date information.\n\nAs said above the Documentation/technical would only serve a purpose to\nexplain very high level concepts. So racy-git.txt or\npack-heuristics.txt will fit\nin there very good.\n"},{"id":"253453","messageId":"20141209204644.GE12001@peff.net","threadId":"38124","inReplyTo":"CAGZ79kZa5sPxbzURtGScCrfbTaCX_cHiRj7uew6Xk+1Df7oTEQ@mail.gmail.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-09T20:46:44Z","receivedAt":"2014-12-09T20:46:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 09, 2014 at 12:32:15PM -0800, Stefan Beller wrote:\n\n> As said above the Documentation/technical would only serve a purpose to\n> explain very high level concepts. So racy-git.txt or\n> pack-heuristics.txt will fit\n> in there very good.\n\nYeah, I think we would leave anything that isn't api-*, with the goal\nthat all of api-* would eventually migrate into headers (it does not all\nhave to happen on day one, of course). There are a few spots which do\nrefer to api-credential, which is where you can find information on the\ncredential protocol. But that could be split out into a separate\ntechnical document (which is probably an improvement, anyway; people who\nare writing a helper do not know or care about our internal C API; they\nonly care about the protocol).\n\n-Peff\n"},{"id":"253458","messageId":"20141209222337.GA16345@google.com","threadId":"38124","inReplyTo":"20141209202738.GC12001@peff.net","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-09T22:23:37Z","receivedAt":"2014-12-09T22:23:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Elsewhere I mentioned a tool to extract comments and format them. But do\n> people actually care about the formatting step?\n\nIf formatting means \"convert to a straightforward text document that I\ncan read through\", then I do.\n\n> Does anybody asciidoc\n> the technical/api-* files? We did not even support building them until\n> sometime in 2012. Personally, I only ever view them as text.\n\nI also view them as text. I tend to find it easier to read the\ntechnical/api-* files than headers. That might be for the same reason\nthat some other people prefer header files --- a\nDocumentation/technical/ file tends to be written in one chunk\ntogether and ends up saying exactly what I need to know about the\ncalling sequence in a coherent way, while header files tend to evolve\nover time to match the implementation without maintaining that\norganization and usability.\n\nI suspect a simple tool to extract the comments and produce something\nlike the technical/api-* files would help, since then when someone\nwrites a patch, they could try the tool and see if the result is still\neasy to read.\n\nAnyway, the patch I wrote was an attempt at a minimal fix (and an\nattempt at getting out of a conversation that seemed to be moving\ntoward bikeshedding). If someone wants to move the documentation from\napi-strbuf.txt to strbuf.h, I won't object, since I hope that a simple\ntool like described above isn't too far away.\n\nThanks,\nJonathan\n"},{"id":"253460","messageId":"20141209224949.GB16345@google.com","threadId":"38124","inReplyTo":"20141209201713.GY16345@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-09T22:49:50Z","receivedAt":"2014-12-09T22:49:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Stefan Beller wrote:\n>> On Tue, Dec 9, 2014 at 11:37 AM, Junio C Hamano wrote:\n\n>>> Perhaps the API doc that currently says \"Free\" is the only thing\n>>> that needs fixing? And perhaps add \"See $doc\" at the beginning of\n>>> the header and remove duplicated comments we already have in the\n>>> file?\n>>\n>> The reason I wrote this patch originally was because I seem to forget we have\n>> more than one place to document our APIs. If there are comments in the header\n>> I seem to have thought it were the only place where we have documentation.\n>\n> How about this patch?\n\nAnd another:\n\n-- >8--\nSubject: put string-list API documentation in one place\n\nUntil recently (v1.8.0-rc0~46^2~5, 2012-09-12), the string-list API\nwas documented in detail in Documentation/technical/api-string-list.txt\nand the header file contained section markers and some short reminders\nbut little other documentation.\n\nSince then, the header has acquired some more comments that are mostly\nidentical to the documentation from technical/. In principle that\nshould help convenience, since it means one less hop for someone\nreading the header to find API documentation. In practice,\nunfortunately, it is hard to remember that there is documentation in\ntwo places, and the comprehensive documentation of some functions in\nthe header makes it too easy to forget that the other functions are\ndocumented at all (and where).\n\nAdd a comment pointing to Documentation/technical/ and remove the\ncomments that duplicate what is written there. Longer term, we may\nwant to move all of the technical docs to header files and generate\nprinter-ready API documentation another way, but that is a larger\nchange for another day.\n\nShort reminders in the header file are still okay.\n\nReported-by: Stefan Beller \nSigned-off-by: Jonathan Nieder \n---\n Documentation/technical/api-string-list.txt | 12 ++++++-\n string-list.h | 56 ++---------------------------\n 2 files changed, 14 insertions(+), 54 deletions(-)\n\ndiff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\nindex d51a657..a85e713 100644\n--- a/Documentation/technical/api-string-list.txt\n+++ b/Documentation/technical/api-string-list.txt\n@@ -185,7 +185,17 @@ overwriting the delimiter characters with NULs and creating new\n string_list_items that point into the original string (the original\n string must therefore not be modified or freed while the `string_list`\n is in use).\n-\n++\n+Examples:\n++\n+----\n+string_list_split(l, \"foo:bar:baz\", ':', -1) -> [\"foo\", \"bar\", \"baz\"]\n+string_list_split(l, \"foo:bar:baz\", ':', 0) -> [\"foo:bar:baz\"]\n+string_list_split(l, \"foo:bar:baz\", ':', 1) -> [\"foo\", \"bar:baz\"]\n+string_list_split(l, \"foo:bar:\", ':', -1) -> [\"foo\", \"bar\", \"\"]\n+string_list_split(l, \"\", ':', -1) -> [\"\"]\n+string_list_split(l, \":\", ':', -1) -> [\"\", \"\"]\n+----\n \n Data structures\n ---------------\ndiff --git a/string-list.h b/string-list.h\nindex 494eb5d..e6a7841 100644\n--- a/string-list.h\n+++ b/string-list.h\n@@ -1,6 +1,8 @@\n #ifndef STRING_LIST_H\n #define STRING_LIST_H\n \n+/* See Documentation/technical/api-string-list.txt */\n+\n struct string_list_item {\n \tchar *string;\n \tvoid *util;\n@@ -35,20 +37,9 @@ int for_each_string_list(struct string_list *list,\n #define for_each_string_list_item(item,list) \\\n \tfor (item = (list)->items; item < (list)->items + (list)->nr; ++item)\n \n-/*\n- * Apply want to each item in list, retaining only the ones for which\n- * the function returns true. If free_util is true, call free() on\n- * the util members of any items that have to be deleted. Preserve\n- * the order of the items that are retained.\n- */\n void filter_string_list(struct string_list *list, int free_util,\n \t\t\tstring_list_each_func_t want, void *cb_data);\n \n-/*\n- * Remove any empty strings from the list. If free_util is true, call\n- * free() on the util members of any items that have to be deleted.\n- * Preserve the order of the items that are retained.\n- */\n void string_list_remove_empty_items(struct string_list *list, int free_util);\n \n /* Use these functions only on sorted lists: */\n@@ -59,30 +50,12 @@ struct string_list_item *string_list_insert(struct string_list *list, const char\n struct string_list_item *string_list_insert_at_index(struct string_list *list,\n \t\t\t\t\t\t int insert_at, const char *string);\n struct string_list_item *string_list_lookup(struct string_list *list, const char *string);\n-\n-/*\n- * Remove all but the first of consecutive entries with the same\n- * string value. If free_util is true, call free() on the util\n- * members of any items that have to be deleted.\n- */\n void string_list_remove_duplicates(struct string_list *sorted_list, int free_util);\n \n \n /* Use these functions only on unsorted lists: */\n \n-/*\n- * Add string to the end of list. If list->strdup_string is set, then\n- * string is copied; otherwise the new string_list_entry refers to the\n- * input string.\n- */\n struct string_list_item *string_list_append(struct string_list *list, const char *string);\n-\n-/*\n- * Like string_list_append(), except string is never copied. When\n- * list->strdup_strings is set, this function can be used to hand\n- * ownership of a malloc()ed string to list without making an extra\n- * copy.\n- */\n struct string_list_item *string_list_append_nodup(struct string_list *list, char *string);\n \n void sort_string_list(struct string_list *list);\n@@ -92,32 +65,9 @@ struct string_list_item *unsorted_string_list_lookup(struct string_list *list,\n \n void unsorted_string_list_delete_item(struct string_list *list, int i, int free_util);\n \n-/*\n- * Split string into substrings on character delim and append the\n- * substrings to list. The input string is not modified.\n- * list->strdup_strings must be set, as new memory needs to be\n- * allocated to hold the substrings. If maxsplit is non-negative,\n- * then split at most maxsplit times. Return the number of substrings\n- * appended to list.\n- *\n- * Examples:\n- * string_list_split(l, \"foo:bar:baz\", ':', -1) -> [\"foo\", \"bar\", \"baz\"]\n- * string_list_split(l, \"foo:bar:baz\", ':', 0) -> [\"foo:bar:baz\"]\n- * string_list_split(l, \"foo:bar:baz\", ':', 1) -> [\"foo\", \"bar:baz\"]\n- * string_list_split(l, \"foo:bar:\", ':', -1) -> [\"foo\", \"bar\", \"\"]\n- * string_list_split(l, \"\", ':', -1) -> [\"\"]\n- * string_list_split(l, \":\", ':', -1) -> [\"\", \"\"]\n- */\n int string_list_split(struct string_list *list, const char *string,\n \t\t int delim, int maxsplit);\n-\n-/*\n- * Like string_list_split(), except that string is split in-place: the\n- * delimiter characters in string are overwritten with NULs, and the\n- * new string_list_items point into string (which therefore must not\n- * be modified or freed while the string_list is in use).\n- * list->strdup_strings must *not* be set.\n- */\n int string_list_split_in_place(struct string_list *list, char *string,\n \t\t\t int delim, int maxsplit);\n+\n #endif /* STRING_LIST_H */\n-- \n2.2.0.rc0.207.ga3a616c\n"},{"id":"253462","messageId":"20141209230748.GA3442@google.com","threadId":"38124","inReplyTo":"20141209224949.GB16345@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-09T23:07:48Z","receivedAt":"2014-12-09T23:07:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 09, 2014 at 02:49:50PM -0800, Jonathan Nieder wrote:\n> Jonathan Nieder wrote:\n> > Stefan Beller wrote:\n> >> On Tue, Dec 9, 2014 at 11:37 AM, Junio C Hamano wrote:\n> \n> >>> Perhaps the API doc that currently says \"Free\" is the only thing\n> >>> that needs fixing? And perhaps add \"See $doc\" at the beginning of\n> >>> the header and remove duplicated comments we already have in the\n> >>> file?\n> >>\n> >> The reason I wrote this patch originally was because I seem to forget we have\n> >> more than one place to document our APIs. If there are comments in the header\n> >> I seem to have thought it were the only place where we have documentation.\n> >\n> > How about this patch?\n> \n> And another:\n\nthis goes on top or replaces the previous one? (This applies cleanly on origin/master) \n\n> -- >8--\n> Subject: put string-list API documentation in one place\n> \n> Until recently (v1.8.0-rc0~46^2~5, 2012-09-12), the string-list API\n> was documented in detail in Documentation/technical/api-string-list.txt\n> and the header file contained section markers and some short reminders\n> but little other documentation.\n> \n> Since then, the header has acquired some more comments that are mostly\n> identical to the documentation from technical/. In principle that\n> should help convenience, since it means one less hop for someone\n> reading the header to find API documentation. In practice,\n> unfortunately, it is hard to remember that there is documentation in\n> two places, and the comprehensive documentation of some functions in\n> the header makes it too easy to forget that the other functions are\n> documented at all (and where).\n> \n> Add a comment pointing to Documentation/technical/ and remove the\n> comments that duplicate what is written there. Longer term, we may\n> want to move all of the technical docs to header files and generate\n> printer-ready API documentation another way, but that is a larger\n> change for another day.\n> \n> Short reminders in the header file are still okay.\n> \n> Reported-by: Stefan Beller \n> Signed-off-by: Jonathan Nieder \n> ---\n\nThanks a lot for this patch. It's improving the situation a lot.\nNo information is lost albeit many deleted lines, have a \n\nReviewed-by: Stefan Beller \n"},{"id":"253463","messageId":"20141209231557.GC16345@google.com","threadId":"38124","inReplyTo":"20141209230748.GA3442@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-09T23:15:57Z","receivedAt":"2014-12-09T23:15:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Tue, Dec 09, 2014 at 02:49:50PM -0800, Jonathan Nieder wrote:\n\n>> And another:\n>\n> this goes on top or replaces the previous one? (This applies cleanly on origin/master) \n\nThey're independent. :)\n\n[...]\n>> Reported-by: Stefan Beller \n>> Signed-off-by: Jonathan Nieder \n>> ---\n>\n> Thanks a lot for this patch. It's improving the situation a lot.\n> No information is lost albeit many deleted lines, have a\n>\n> Reviewed-by: Stefan Beller \n\nThanks for looking it over.\n"},{"id":"253464","messageId":"xmqq61dkl8e3.fsf@gitster.dls.corp.google.com","threadId":"38124","inReplyTo":"20141209222337.GA16345@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-09T23:18:28Z","receivedAt":"2014-12-09T23:18:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jeff King wrote:\n>\n>> Elsewhere I mentioned a tool to extract comments and format them. But do\n>> people actually care about the formatting step?\n>\n> If formatting means \"convert to a straightforward text document that I\n> can read through\", then I do.\n\nIf the result becomes unparsable by AsciiDoc(or), those who\nAsciiDoc'ified the api-*.txt may feel unhappy. I however strongly\nsuspect that the primary consumer of these api-*.txt documents are\nconsuming the text version, not AsciiDoc-to-html output.\n\n>> Does anybody asciidoc\n>> the technical/api-* files? We did not even support building them until\n>> sometime in 2012. Personally, I only ever view them as text.\n>\n> I also view them as text. I tend to find it easier to read the\n> technical/api-* files than headers. That might be for the same reason\n> that some other people prefer header files --- a\n> Documentation/technical/ file tends to be written in one chunk\n> together and ends up saying exactly what I need to know about the\n> calling sequence in a coherent way, while header files tend to evolve\n> over time to match the implementation without maintaining that\n> organization and usability.\n\nThe documentation that was written with an explicit purpose to serve\nas documentation would explain how each pieces fit in the whole\nsystem much better than a list of pieces extracted from per-function\ncomments, unless the header comment that serves as the source of\ngenerated document is written carefully. But as people pointed out,\nhaving the same set of information in two places in sync forces us\nto be extra careful in a different way. There is no free lunch.\n\nWe would want (1) documentation in one place, (2) easy access to it\nwhen deciding which API function to call with what information, and\n(3) make it hard to forget updating the documentation when making\nchanges to the API. If that single source of truth is the header\nfile, perhaps we would become more careful commenting our headers\n(i.e. \"do not just comment each functions and structures, but\nenhance introductory part that describes how things fit together\"),\nso I think it would be OK in the end. We may need to reorganize and\nreorder the headers so that associated comments to functions and\ndata types match desirable presentation order, but that effort will\nkill two birds with one stone. The resulting header would also be\neasier to read by humans.\n\nI am a bit hesitant to see us spending extra cycles either to\nreinvent doxygen (if we do our own) or working around quirks in\nthird-party tools (if we decide to use existing ones).\n\nThanks.\n"},{"id":"253483","messageId":"20141210084351.GA29776@peff.net","threadId":"38124","inReplyTo":"20141209222337.GA16345@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T08:43:51Z","receivedAt":"2014-12-10T08:43:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 09, 2014 at 02:23:37PM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Elsewhere I mentioned a tool to extract comments and format them. But do\n> > people actually care about the formatting step?\n> \n> If formatting means \"convert to a straightforward text document that I\n> can read through\", then I do.\n\nI meant \"run asciidoc in api-*\". I am confused by your response. Here it\nimplies to me that you asciidoc them into a text format to make it\neasier to read. But below you say \"I also view [the api-* files] as\ntext\", which implies you just look at them in your editor.\n\n> > Does anybody asciidoc\n> > the technical/api-* files? We did not even support building them until\n> > sometime in 2012. Personally, I only ever view them as text.\n> \n> I also view them as text. I tend to find it easier to read the\n> technical/api-* files than headers. That might be for the same reason\n> that some other people prefer header files --- a\n> Documentation/technical/ file tends to be written in one chunk\n> together and ends up saying exactly what I need to know about the\n> calling sequence in a coherent way, while header files tend to evolve\n> over time to match the implementation without maintaining that\n> organization and usability.\n\nWe could probably do a better job of keeping our header files neat and\nwell-ordered. And perhaps would so if they had a coherent narrative in\nthe first place.\n\nMy feeling is that it is at least as much work to keep an api-*.txt file\nneat as it would be to keep the header file neat (generally more,\nbecause of the duplication of effort). The one exception might be that\nordering in the header file may require some forward declarations. I'd\nguess that to be pretty rare, though (and certainly I did not run into\nit when converting the strbuf documentation below).\n\n> I suspect a simple tool to extract the comments and produce something\n> like the technical/api-* files would help, since then when someone\n> writes a patch, they could try the tool and see if the result is still\n> easy to read.\n\nThat seems like wishful thinking to me. Some subset of developers will\nbe happy reading the documentation in the header file, and will not\ncommonly run the tool. Therefore they will also not bother to examine\nthe output of the tool when making a change (either because they forget,\nor because it is simply a hassle that they do not care about).\n\n> Anyway, the patch I wrote was an attempt at a minimal fix (and an\n> attempt at getting out of a conversation that seemed to be moving\n> toward bikeshedding). If someone wants to move the documentation from\n> api-strbuf.txt to strbuf.h, I won't object, since I hope that a simple\n> tool like described above isn't too far away.\n\nI do not mind a partial or minimal fix. This conversion is tedious, and\nit does not have to be done all at once. But IMHO, your patch is\nactively making things worse. The api-* files have slowly grown out of\ndate over the years, and I believe that is because they are too far\nremoved from the code people are actually touching. I've been making an\nactive effort recently to document function interfaces in comments, and\nI feel like the presence of other comments has encouraged me to do so.\n\nHere's strbuf.h, with the data from api-strbuf.txt moved into it. I\nfollowed the ordering of api-strbuf.txt, since that has a more coherent\ngrouping of functions (but it does make the latter parts of the diff\nmessier, as we have to move function declarations around). I left the\nbottom part of strbuf.h, where there are several undocumented functions,\nlargely untouched. As further work, it would make sense to document\nthose functions and also fit them into the right spot amidst the\nalready-grouped functions.\n\nThis has almost every bit of text from api-strbuf.txt. I omitted a few\ndescriptions where the documentation already in strbuf.h was actually\n_better_. I did remove the function names, sine they are redundant with\nthe declarations below. E.g., rather than:\n\n /**\n * `foo`::\n * Do something.\n */\n void foo(int x);\n\njust:\n\n /**\n * Do something.\n */\n void foo(int x);\n\nThe declaration is nearby and has strictly more information, and the\nless boilerplate the better for convincing people to actually write\nsomething. An extraction tool can pretty-print the function name if it\nwants.\n\nI did drop some asciidoc-specific formatting from the main text (e.g.,\n\"+\" lines for connecting blocks), which means that extracting the result\nand running it through asciidoc won't work. I think it makes the actual\ntext much more readable, though. But we could do it the other way if\npeople really want to asciidoc it.\n\n---\ndiff --git a/strbuf.h b/strbuf.h\nindex 652b6c4..6143680 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -1,22 +1,105 @@\n #ifndef STRBUF_H\n #define STRBUF_H\n \n-/* See Documentation/technical/api-strbuf.txt */\n-\n-extern char strbuf_slopbuf[];\n+/**\n+ * strbuf's are meant to be used with all the usual C string and memory\n+ * APIs. Given that the length of the buffer is known, it's often better to\n+ * use the mem* functions than a str* one (memchr vs. strchr e.g.).\n+ * Though, one has to be careful about the fact that str* functions often\n+ * stop on NULs and that strbufs may have embedded NULs.\n+ *\n+ * A strbuf is NUL terminated for convenience, but no function in the\n+ * strbuf API actually relies on the string being free of NULs.\n+ *\n+ * strbufs have some invariants that are very important to keep in mind:\n+ *\n+ * . The `buf` member is never NULL, so it can be used in any usual C\n+ * string operations safely. strbuf's _have_ to be initialized either by\n+ * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.\n+ *\n+ * Do *not* assume anything on what `buf` really is (e.g. if it is\n+ * allocated memory or not), use `strbuf_detach()` to unwrap a memory\n+ * buffer from its strbuf shell in a safe way. That is the sole supported\n+ * way. This will give you a malloced buffer that you can later `free()`.\n+ *\n+ * However, it is totally safe to modify anything in the string pointed by\n+ * the `buf` member, between the indices `0` and `len-1` (inclusive).\n+ *\n+ * . The `buf` member is a byte array that has at least `len + 1` bytes\n+ * allocated. The extra byte is used to store a `'\\0'`, allowing the\n+ * `buf` member to be a valid C-string. Every strbuf function ensure this\n+ * invariant is preserved.\n+ *\n+ * NOTE: It is OK to \"play\" with the buffer directly if you work it this\n+ * way:\n+ *\n+ * ----\n+ * strbuf_grow(sb, SOME_SIZE); <1>\n+ * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);\n+ * ----\n+ * <1> Here, the memory array starting at `sb->buf`, and of length\n+ * `strbuf_avail(sb)` is all yours, and you can be sure that\n+ * `strbuf_avail(sb)` is at least `SOME_SIZE`.\n+ *\n+ * NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.\n+ *\n+ * Doing so is safe, though if it has to be done in many places, adding the\n+ * missing API to the strbuf module is the way to go.\n+ *\n+ * WARNING: Do _not_ assume that the area that is yours is of size `alloc\n+ * - 1` even if it's true in the current implementation. Alloc is somehow a\n+ * \"private\" member that should not be messed with. Use `strbuf_avail()`\n+ * instead.\n+*/\n+\n+/**\n+ * This is the string buffer structure. The `len` member can be used to\n+ * determine the current length of the string, and `buf` member provides\n+ * access to the string itself.\n+ */\n struct strbuf {\n \tsize_t alloc;\n \tsize_t len;\n \tchar *buf;\n };\n \n+extern char strbuf_slopbuf[];\n #define STRBUF_INIT { 0, 0, strbuf_slopbuf }\n \n-/*----- strbuf life cycle -----*/\n+/** strbuf lifecycle */\n+\n+/**\n+ *\tInitialize the structure. The second parameter can be zero or a bigger\n+ *\tnumber to allocate memory, in case you want to prevent further reallocs.\n+ */\n extern void strbuf_init(struct strbuf *, size_t);\n+\n+/**\n+ *\tRelease a string buffer and the memory it used. You should not use the\n+ *\tstring buffer after using this function, unless you initialize it again.\n+ */\n extern void strbuf_release(struct strbuf *);\n+\n+/**\n+ * Detach the string from the strbuf and returns it; you now own the\n+ * storage the string occupies and it is your responsibility from then on\n+ * to release it with `free(3)` when you are done with it.\n+ */\n extern char *strbuf_detach(struct strbuf *, size_t *);\n+\n+/**\n+ * Attach a string to a buffer. You should specify the string to attach,\n+ * the current length of the string and the amount of allocated memory.\n+ * The amount must be larger than the string length, because the string you\n+ * pass is supposed to be a NUL-terminated string. This string _must_ be\n+ * malloc()ed, and after attaching, the pointer cannot be relied upon\n+ * anymore, and neither be free()d directly.\n+ */\n extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);\n+\n+/**\n+ * Swap the contents of two string buffers.\n+ */\n static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)\n {\n \tstruct strbuf tmp = *a;\n@@ -24,14 +107,33 @@ static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)\n \t*b = tmp;\n }\n \n-/*----- strbuf size related -----*/\n+\n+/** Related to the size of the buffer */\n+\n+/**\n+ * Determine the amount of allocated but unused memory.\n+ */\n static inline size_t strbuf_avail(const struct strbuf *sb)\n {\n \treturn sb->alloc ? sb->alloc - sb->len - 1 : 0;\n }\n \n+/**\n+ * Ensure that at least this amount of unused memory is available after\n+ * `len`. This is used when you know a typical size for what you will add\n+ * and want to avoid repetitive automatic resizing of the underlying buffer.\n+ * This is never a needed operation, but can be critical for performance in\n+ * some cases.\n+ */\n extern void strbuf_grow(struct strbuf *, size_t);\n \n+/**\n+ * Set the length of the buffer to a given value. This function does *not*\n+ * allocate new memory, so you should not perform a `strbuf_setlen()` to a\n+ * length that is larger than `len + strbuf_avail()`. `strbuf_setlen()` is\n+ * just meant as a 'please fix invariants from this strbuf I just messed\n+ * with'.\n+ */\n static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n {\n \tif (len > (sb->alloc ? sb->alloc - 1 : 0))\n@@ -39,16 +141,279 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n \tsb->len = len;\n \tsb->buf[len] = '\\0';\n }\n+\n+/**\n+ * Empty the buffer by setting the size of it to zero.\n+ */\n #define strbuf_reset(sb) strbuf_setlen(sb, 0)\n \n-/*----- content related -----*/\n+\n+/** Related to the contents of the buffer */\n+\n+/**\n+ * Strip whitespace from the beginning and end of a string.\n+ * Equivalent to performing `strbuf_rtrim()` followed by `strbuf_ltrim()`.\n+ */\n extern void strbuf_trim(struct strbuf *);\n+\n+/**\n+ * Strip whitespace from the end of a string.\n+ */\n extern void strbuf_rtrim(struct strbuf *);\n+\n+/**\n+ * Strip whitespace from the beginning of a string.\n+ */\n extern void strbuf_ltrim(struct strbuf *);\n+\n+/**\n+ * Replace the contents of the strbuf with a reencoded form. Returns -1\n+ * on error, 0 on success.\n+ */\n extern int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);\n+\n+/**\n+ * Lowercase each character in the buffer using `tolower`.\n+ */\n extern void strbuf_tolower(struct strbuf *sb);\n+\n+/**\n+ * Compare two buffers. Returns an integer less than, equal to, or greater\n+ * than zero if the first buffer is found, respectively, to be less than,\n+ * to match, or be greater than the second buffer.\n+ */\n extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n \n+\n+/** Adding data to the buffer\n+ *\n+ * NOTE: All of the functions in this section will grow the buffer as\n+ * necessary. If they fail for some reason other than memory shortage and the\n+ * buffer hadn't been allocated before (i.e. the `struct strbuf` was set to\n+ * `STRBUF_INIT`), then they will free() it.\n+ */\n+\n+/**\n+ * Add a single character to the buffer.\n+ */\n+static inline void strbuf_addch(struct strbuf *sb, int c)\n+{\n+\tstrbuf_grow(sb, 1);\n+\tsb->buf[sb->len++] = c;\n+\tsb->buf[sb->len] = '\\0';\n+}\n+\n+/**\n+ * Add a character the specified number of times to the buffer.\n+ */\n+extern void strbuf_addchars(struct strbuf *sb, int c, size_t n);\n+\n+/**\n+ * Insert data to the given position of the buffer. The remaining contents\n+ * will be shifted, not overwritten.\n+ */\n+extern void strbuf_insert(struct strbuf *, size_t pos, const void *, size_t);\n+\n+/**\n+ * Remove given amount of data from a given position of the buffer.\n+ */\n+extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);\n+\n+/**\n+ * Remove the bytes between `pos..pos+len` and replace it with the given\n+ * data.\n+ */\n+extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,\n+\t\t\t const void *, size_t);\n+\n+/**\n+ * Add a NUL-terminated string to the buffer. Each line will be prepended\n+ * by a comment character and a blank.\n+ */\n+extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);\n+\n+\n+/**\n+ * Add data of given length to the buffer.\n+ */\n+extern void strbuf_add(struct strbuf *, const void *, size_t);\n+\n+/**\n+ * Add a NUL-terminated string to the buffer.\n+ *\n+ * NOTE: This function will *always* be implemented as an inline or a macro\n+ * using strlen, meaning that this is efficient to write things like:\n+ *\n+ * strbuf_addstr(sb, \"immediate string\");\n+ */\n+static inline void strbuf_addstr(struct strbuf *sb, const char *s)\n+{\n+\tstrbuf_add(sb, s, strlen(s));\n+}\n+\n+/**\n+ * Copy the contents of another buffer at the end of the current one.\n+ */\n+static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)\n+{\n+\tstrbuf_grow(sb, sb2->len);\n+\tstrbuf_add(sb, sb2->buf, sb2->len);\n+}\n+\n+/**\n+ * Copy part of the buffer from a given position till a given length to the\n+ * end of the buffer.\n+ */\n+extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);\n+\n+/**\n+ * This function can be used to expand a format string containing\n+ * placeholders. To that end, it parses the string and calls the specified\n+ * function for every percent sign found.\n+ *\n+ * The callback function is given a pointer to the character after the `%`\n+ * and a pointer to the struct strbuf. It is expected to add the expanded\n+ * version of the placeholder to the strbuf, e.g. to add a newline\n+ * character if the letter `n` appears after a `%`. The function returns\n+ * the length of the placeholder recognized and `strbuf_expand()` skips\n+ * over it.\n+ *\n+ * The format `%%` is automatically expanded to a single `%` as a quoting\n+ * mechanism; callers do not need to handle the `%` placeholder themselves,\n+ * and the callback function will not be invoked for this placeholder.\n+ *\n+ * All other characters (non-percent and not skipped ones) are copied\n+ * verbatim to the strbuf. If the callback returned zero, meaning that the\n+ * placeholder is unknown, then the percent sign is copied, too.\n+ *\n+ * In order to facilitate caching and to make it possible to give\n+ * parameters to the callback, `strbuf_expand()` passes a context pointer,\n+ * which can be used by the programmer of the callback as she sees fit.\n+ */\n+typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);\n+extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);\n+\n+/**\n+ * Used as callback for `strbuf_expand()`, expects an array of\n+ * struct strbuf_expand_dict_entry as context, i.e. pairs of\n+ * placeholder and replacement string. The array needs to be\n+ * terminated by an entry with placeholder set to NULL.\n+ */\n+struct strbuf_expand_dict_entry {\n+\tconst char *placeholder;\n+\tconst char *value;\n+};\n+extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context);\n+\n+/**\n+ * Append the contents of one strbuf to another, quoting any\n+ * percent signs (\"%\") into double-percents (\"%%\") in the\n+ * destination. This is useful for literal data to be fed to either\n+ * strbuf_expand or to the *printf family of functions.\n+ */\n+extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n+\n+/**\n+ * Append the given byte size as a human-readable string (i.e. 12.23 KiB,\n+ * 3.50 MiB).\n+ */\n+extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);\n+\n+/**\n+ * Add a formatted string to the buffer.\n+ */\n+__attribute__((format (printf,2,3)))\n+extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n+\n+/**\n+ * Add a formatted string prepended by a comment character and a\n+ * blank to the buffer.\n+ */\n+__attribute__((format (printf, 2, 3)))\n+extern void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);\n+\n+__attribute__((format (printf,2,0)))\n+extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n+\n+/**\n+ * Read a given size of data from a FILE* pointer to the buffer.\n+ *\n+ * NOTE: The buffer is rewound if the read fails. If -1 is returned,\n+ * `errno` must be consulted, like you would do for `read(3)`.\n+ * `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline()` has the\n+ * same behaviour as well.\n+ */\n+extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n+\n+/**\n+ * Read the contents of a given file descriptor. The third argument can be\n+ * used to give a hint about the file size, to avoid reallocs. If read fails,\n+ * any partial read is undone */\n+extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n+\n+/**\n+ * Read the contents of a file, specified by its path. The third argument\n+ * can be used to give a hint about the file size, to avoid reallocs.\n+ */\n+extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n+\n+/**\n+ * Read the target of a symbolic link, specified by its path. The third\n+ * argument can be used to give a hint about the size, to avoid reallocs.\n+ */\n+extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n+\n+/**\n+ * Set the buffer to the path of the current working directory.\n+ */\n+extern int strbuf_getcwd(struct strbuf *sb);\n+\n+/**\n+ * Read a line from a FILE *, overwriting the existing contents\n+ * of the strbuf. The second argument specifies the line\n+ * terminator character, typically `'\\n'`.\n+ * Reading stops after the terminator or at EOF. The terminator\n+ * is removed from the buffer before returning. Returns 0 unless\n+ * there was nothing left before EOF, in which case it returns `EOF`.\n+ */\n+extern int strbuf_getline(struct strbuf *, FILE *, int);\n+\n+/**\n+ * Like `strbuf_getline`, but keeps the trailing terminator (if\n+ * any) in the buffer.\n+ */\n+extern int strbuf_getwholeline(struct strbuf *, FILE *, int);\n+\n+/**\n+ * Like `strbuf_getwholeline`, but operates on a file descriptor.\n+ * It reads one character at a time, so it is very slow. Do not\n+ * use it unless you need the correct position in the file\n+ * descriptor.\n+ */\n+extern int strbuf_getwholeline_fd(struct strbuf *, int, int);\n+\n+/**\n+ * Add a path to a buffer, converting a relative path to an\n+ * absolute one in the process. Symbolic links are not\n+ * resolved.\n+ */\n+extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n+\n+/**\n+ * Strip whitespace from a buffer. The second parameter controls if\n+ * comments are considered contents to be removed or not.\n+ */\n+extern void stripspace(struct strbuf *buf, int skip_comments);\n+\n+/**\n+ * Launch the user preferred editor to edit a file and fill the buffer\n+ * with the file's contents upon the user completing their editing. The\n+ * third argument can be used to set the environment which the editor is\n+ * run in. If the buffer is NULL the editor is launched as usual but the\n+ * file's contents are not read into the buffer upon completion.\n+ */\n+extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);\n+\n static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n {\n \tif (strip_suffix_mem(sb->buf, &sb->len, suffix)) {\n@@ -110,52 +475,6 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n */\n extern void strbuf_list_free(struct strbuf **);\n \n-/*----- add data in your buffer -----*/\n-static inline void strbuf_addch(struct strbuf *sb, int c)\n-{\n-\tstrbuf_grow(sb, 1);\n-\tsb->buf[sb->len++] = c;\n-\tsb->buf[sb->len] = '\\0';\n-}\n-\n-extern void strbuf_insert(struct strbuf *, size_t pos, const void *, size_t);\n-extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);\n-\n-/* splice pos..pos+len with given data */\n-extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,\n- const void *, size_t);\n-\n-extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);\n-\n-extern void strbuf_add(struct strbuf *, const void *, size_t);\n-static inline void strbuf_addstr(struct strbuf *sb, const char *s)\n-{\n-\tstrbuf_add(sb, s, strlen(s));\n-}\n-static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)\n-{\n-\tstrbuf_grow(sb, sb2->len);\n-\tstrbuf_add(sb, sb2->buf, sb2->len);\n-}\n-extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);\n-extern void strbuf_addchars(struct strbuf *sb, int c, size_t n);\n-\n-typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);\n-extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);\n-struct strbuf_expand_dict_entry {\n-\tconst char *placeholder;\n-\tconst char *value;\n-};\n-extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context);\n-extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n-\n-__attribute__((format (printf,2,3)))\n-extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n-__attribute__((format (printf, 2, 3)))\n-extern void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);\n-__attribute__((format (printf,2,0)))\n-extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n-\n extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);\n \n /*\n@@ -170,28 +489,11 @@ static inline void strbuf_complete_line(struct strbuf *sb)\n \t\tstrbuf_addch(sb, '\\n');\n }\n \n-extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n-/* XXX: if read fails, any partial read is undone */\n-extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n-extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n-extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n-extern int strbuf_getcwd(struct strbuf *sb);\n-\n-extern int strbuf_getwholeline(struct strbuf *, FILE *, int);\n-extern int strbuf_getline(struct strbuf *, FILE *, int);\n-extern int strbuf_getwholeline_fd(struct strbuf *, int, int);\n-\n-extern void stripspace(struct strbuf *buf, int skip_comments);\n-extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);\n-\n extern int strbuf_branchname(struct strbuf *sb, const char *name);\n extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);\n \n extern void strbuf_addstr_urlencode(struct strbuf *, const char *,\n \t\t\t\t int reserved);\n-extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);\n-\n-extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n \n __attribute__((format (printf,1,2)))\n extern int printf_ln(const char *fmt, ...);\n"},{"id":"253484","messageId":"20141210085201.GB29776@peff.net","threadId":"38124","inReplyTo":"xmqq61dkl8e3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T08:52:01Z","receivedAt":"2014-12-10T08:52:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 09, 2014 at 03:18:28PM -0800, Junio C Hamano wrote:\n\n> Jonathan Nieder writes:\n> \n> > Jeff King wrote:\n> >\n> >> Elsewhere I mentioned a tool to extract comments and format them. But do\n> >> people actually care about the formatting step?\n> >\n> > If formatting means \"convert to a straightforward text document that I\n> > can read through\", then I do.\n> \n> If the result becomes unparsable by AsciiDoc(or), those who\n> AsciiDoc'ified the api-*.txt may feel unhappy. I however strongly\n> suspect that the primary consumer of these api-*.txt documents are\n> consuming the text version, not AsciiDoc-to-html output.\n\nYeah, that was my point. They were not even asciidoc'd for a long time,\nthen one contributor, who does not otherwise work on the code,\nvolunteered to asciidoc them. AFAIK there are no regular developers who\nwanted this feature (but I do not know for sure, which is why I asked).\n\nI do not _mind_ if they can be asciidoc'd, but if nobody does so, they\nare bound to develop formatting errors. And matching asciidoc syntax\ndoes come at a readability cost to the text.\n\n> The documentation that was written with an explicit purpose to serve\n> as documentation would explain how each pieces fit in the whole\n> system much better than a list of pieces extracted from per-function\n> comments, unless the header comment that serves as the source of\n> generated document is written carefully.\n\nIf we split the header files sanely (i.e., to match what would be one\napi-* file), then I had imagined each header file having a long\nfree-form comment at the top giving the overview, followed by commented\nstructures and functions, with possibly other free-form comments in the\nmiddle as necessary. That's what I did for the strbuf.h conversion I\njust sent.\n\n> I am a bit hesitant to see us spending extra cycles either to\n> reinvent doxygen (if we do our own) or working around quirks in\n> third-party tools (if we decide to use existing ones).\n\nMe too. That's what I hoped that we could come up with a form whose\nin-header source is readable enough that people would use it. Then there\nis really no tool necessary.\n\n-Peff\n"},{"id":"253487","messageId":"20141210091815.GA18372@google.com","threadId":"38124","inReplyTo":"20141210084351.GA29776@peff.net","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-10T09:18:15Z","receivedAt":"2014-12-10T09:18:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n>> Jeff King wrote:\n\n> We could probably do a better job of keeping our header files neat and\n> well-ordered. And perhaps would so if they had a coherent narrative in\n> the first place.\n\nThe example I was looking at before was refs.h. It is still a mess.\nHopefully strbuf.h will work better.\n\n[...]\n> On Tue, Dec 09, 2014 at 02:23:37PM -0800, Jonathan Nieder wrote:\n\n>> I suspect a simple tool to extract the comments and produce something\n>> like the technical/api-* files would help, since then when someone\n>> writes a patch, they could try the tool and see if the result is still\n>> easy to read.\n>\n> That seems like wishful thinking to me. Some subset of developers will\n> be happy reading the documentation in the header file, and will not\n> commonly run the tool. Therefore they will also not bother to examine\n> the output of the tool when making a change (either because they forget,\n> or because it is simply a hassle that they do not care about).\n\nThat is okay as long as enough people extract documentation to ensure\nthe result is sane.\n\nIt is similar to what happens with Documentation/*.txt. A subset of\ndevelopers just work with the source and make local changes without\nlooking at the big picture (and sometimes buggy markup or documentation\norganized in a confusing way results). But that doesn't change much.\nWhen people forget to look at the manpage and write text that is\nawkward in context or markup errors, one of a few things happens:\n\n * a reviewer notices. The patch is fixed, and the patch author\n develops better habits for next time.\n\n * someone using the documentation notices later, and it gets fixed\n then, which is still fine.\n\nBoth outcomes are much, much better than documentation that never gets\norganized.\n\n[...]\n> The api-* files have slowly grown out of\n> date over the years, and I believe that is because they are too far\n> removed from the code people are actually touching.\n\nI don't think they ever were very comprehensive or up to date.\n\nAs far as I understand, the api-* files are intended to be usually out\nof date. Their main purpose is to explain the gist of how to use the\nAPI. They were usually written way after the API was introduced (so\nthey are behind already). They are clearly written and as long as\nthey mostly match the code, that is enough for them to be useful and\neducational.\n\nThen every few years or so, someone updates the file with the latest\nAPI changes, still with an eye to overall organization and readability\nof the document.\n\nIn other words, they prioritize clarity over currency.\n\nOf course if it's possible to have clear documentation that also\nmatches the current code, that would be even better. I have hope that\nmoving the documentation to the header files will get us there if we\ndo it right.\n\n> I did drop some asciidoc-specific formatting from the main text (e.g.,\n> \"+\" lines for connecting blocks), which means that extracting the result\n> and running it through asciidoc won't work.\n\nThis unfortunately lost some structure (e.g., which paragraphs were\npart of the same bulleted list). It would be more common to use a\nhanging indent:\n\n - The `buf` member is never NULL, so ...\n string operations ...\n by `strbuf_init()` ...\n\n Do not assume anything about what `buf` really is ...\n ...\n\n - ...\n\n[...]\n> I think it makes the actual\n> text much more readable, though. But we could do it the other way if\n> people really want to asciidoc it.\n\nI also don't mind losing the asciidoc markup, especially given that it\nwould be a pain to reconstruct and render.\n\nMy reaction at first glance is that this is trying to do something\ngood but the current implementation is less readable than\nDocumentation/technical/api-strbuf.txt. Am I looking at a work in\nprogress, or is this meant to be ready for application? Which aspects\nwould you be interested in comments on?\n\nThanks,\nJonathan\n"},{"id":"253531","messageId":"5488A87C.4030505@alum.mit.edu","threadId":"38124","inReplyTo":"20141210084351.GA29776@peff.net","subject":"Re: [PATCH] document string_list_clear","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-10T20:09:32Z","receivedAt":"2014-12-10T20:09:32Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"My vote is for putting the API docs in the header files:\n\n* Functions are documented right next to their declarations (which the\ncompiler guarantees are up-to-date), removing ambiguity and avoiding\nsome redundancy.\n\n* It is obvious at a glance which functions are documented and which\nstill need docs.\n\n* It is easier to remember to update the documentation when it is near\nthe code, and easier for reviewers to remember to pester authors to do so.\n\nI also like it when function declarations in header files include the\nnames of their parameters, as (1) it's an efficient way to give the\nreader a good hint about how the function should be used, and (2) it\nmakes it easier to refer to the parameters from the docstring.\n\nI agree that there is an important need for introductory/conceptual\ndocumentation about and API, but I think this fits fine as a large\ndocstring at the top of the header file.\n\nI personally don't have much use for formatted documentation but if that\nwere a need I would think that a standard tool like doxygen could be used.\n\nI have been trying hard to add good header-file docstrings for code that\nI have worked on, and even for code that I had to read because I needed\nto figure out how to use it. I would find it a pity for that work to be\nsquashed into Documentation/technical/api-*.txt, where in my opinion it\nis less discoverable and more likely to fall into disrepair.\n\nMichael\n"},{"id":"253537","messageId":"20141210215154.GF16345@google.com","threadId":"38124","inReplyTo":"5488A87C.4030505@alum.mit.edu","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-10T21:51:54Z","receivedAt":"2014-12-10T21:51:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Michael Haggerty wrote:\n\n> I would find it a pity for that work to be\n> squashed into Documentation/technical/api-*.txt, where in my opinion it\n> is less discoverable and more likely to fall into disrepair.\n\nI think we're in violent agreement and keep repeating ourselves.\n\nAll I said is that api-strbuf.txt is currently the most readable\ndocumentation of the strbuf API I can find. The patch to move the\ntext to strbuf.h looked rough and incomplete. Therefore I don't think\nit's ready to be applied as is. If you'd like more details about why\nI say that, feel free to ask.\n\nJonathan\n"},{"id":"253539","messageId":"xmqqlhmff8cs.fsf@gitster.dls.corp.google.com","threadId":"38124","inReplyTo":"20141210215154.GF16345@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-10T22:28:03Z","receivedAt":"2014-12-10T22:28:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Michael Haggerty wrote:\n>\n>> I would find it a pity for that work to be\n>> squashed into Documentation/technical/api-*.txt, where in my opinion it\n>> is less discoverable and more likely to fall into disrepair.\n>\n> I think we're in violent agreement and keep repeating ourselves.\n\nHmph, I am confused.\n\nI somehow had an impression that the \"move to doc and remove from\nheader\" patch was to illustrate how unpleasant the result will be as\na whole (i.e. results in a nice documentation as a starting point,\nbut we can see that it will be hard to motivate and help people to\nkeep it up to date during further development). Which would suggest\nthat you are in favor of moving the other way around, to keep the\nheader rich with documentation only at the higher level. Am I\nreading you correctly?\n\n> All I said is that api-strbuf.txt is currently the most readable\n> documentation of the strbuf API I can find. The patch to move the\n> text to strbuf.h looked rough and incomplete. Therefore I don't think\n> it's ready to be applied as is. If you'd like more details about why\n> I say that, feel free to ask.\n"},{"id":"253540","messageId":"20141210223721.GG16345@google.com","threadId":"38124","inReplyTo":"xmqqlhmff8cs.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-10T22:37:21Z","receivedAt":"2014-12-10T22:37:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>> Michael Haggerty wrote:\n\n>>> I would find it a pity for that work to be\n>>> squashed into Documentation/technical/api-*.txt, where in my opinion it\n>>> is less discoverable and more likely to fall into disrepair.\n>>\n>> I think we're in violent agreement and keep repeating ourselves.\n>\n> Hmph, I am confused.\n>\n> I somehow had an impression that the \"move to doc and remove from\n> header\" patch was to illustrate how unpleasant the result will be as\n> a whole (i.e. results in a nice documentation as a starting point,\n> but we can see that it will be hard to motivate and help people to\n> keep it up to date during further development). Which would suggest\n> that you are in favor of moving the other way around, to keep the\n> header rich with documentation only at the higher level. Am I\n> reading you correctly?\n\nSorry, I think I was unclear.\n\nSome possibilities, in order of my preference (earlier items are better):\n\n 1. Move documentation to header and provide a program to generate a nice\n standalone document.\n\n 2. Move documentation to header, being careful enough that the header\n sort of works as a standalone document.\n\n 3. Move documentation to Documentation/technical/ and keep the header\n bare-bones.\n\n 4. Status quo (comprehensive documentation for some functions in both\n places, for others in only one place, no reliable way for someone\n to find the information they need in one place).\n\nSince (3) is better than (4), I wrote simple patches to do that for\nstrbuf.h and string-list.h. I meant them in earnest --- I hope they\nget applied.\n\nI think peff was working on (2), which is an admirable goal. The\npatch seemed to be incomplete.\n\nThanks,\nJonathan\n"},{"id":"253543","messageId":"xmqq61djf6nq.fsf@gitster.dls.corp.google.com","threadId":"38124","inReplyTo":"20141210223721.GG16345@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-10T23:04:41Z","receivedAt":"2014-12-10T23:04:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Some possibilities, in order of my preference (earlier items are better):\n>\n> 1. Move documentation to header and provide a program to generate a nice\n> standalone document.\n>\n> 2. Move documentation to header, being careful enough that the header\n> sort of works as a standalone document.\n>\n> 3. Move documentation to Documentation/technical/ and keep the header\n> bare-bones.\n>\n> 4. Status quo (comprehensive documentation for some functions in both\n> places, for others in only one place, no reliable way for someone\n> to find the information they need in one place).\n>\n> Since (3) is better than (4), I wrote simple patches to do that for\n> strbuf.h and string-list.h. I meant them in earnest --- I hope they\n> get applied.\n>\n> I think peff was working on (2), which is an admirable goal. The\n> patch seemed to be incomplete.\n\nYeah, I agree with the above preferred ordering, and also think once\nwe get to (2) it would be \"usable\" by the intended audience. Those\nwho prefer (1) over (2) are the ones who somehow want to read hard\ncopies ;-) and are likely to be different audiences and are better\nserved by people with different skill-set and inclination.\n\nI am not sure if (2) and (3) are that incompatible, though. If you\nhad an acceptable version of (3), wouldn't it be just the matter of\nindenting the whole thing with \"s/^/ */\", sprinkle \"/**\" and \"*/\" at\nstrategic paragraph breaks, and move them back to the corresponding\nheader?\n"},{"id":"253544","messageId":"20141210230827.GH16345@google.com","threadId":"38124","inReplyTo":"xmqq61djf6nq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-10T23:08:27Z","receivedAt":"2014-12-10T23:08:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> 2. Move documentation to header, being careful enough that the header\n>> sort of works as a standalone document.\n>>\n>> 3. Move documentation to Documentation/technical/ and keep the header\n>> bare-bones.\n[...]\n> I am not sure if (2) and (3) are that incompatible, though. If you\n> had an acceptable version of (3), wouldn't it be just the matter of\n> indenting the whole thing with \"s/^/ */\", sprinkle \"/**\" and \"*/\" at\n> strategic paragraph breaks, and move them back to the corresponding\n> header?\n\nPresumably. There's also the question of whether to use asciidoc\nmarkup, which got mixed in somehow (but I don't see why it has to be ---\na header with asciidoc would be fine with me, as would a file in\nDocumentation/technical/ without asciidoc).\n"},{"id":"253632","messageId":"20141212091625.GA9049@peff.net","threadId":"38124","inReplyTo":"20141210091815.GA18372@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-12T09:16:26Z","receivedAt":"2014-12-12T09:16:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 10, 2014 at 01:18:15AM -0800, Jonathan Nieder wrote:\n\n> > That seems like wishful thinking to me. Some subset of developers will\n> > be happy reading the documentation in the header file, and will not\n> > commonly run the tool. Therefore they will also not bother to examine\n> > the output of the tool when making a change (either because they forget,\n> > or because it is simply a hassle that they do not care about).\n> \n> That is okay as long as enough people extract documentation to ensure\n> the result is sane.\n\nI have a guess that the \"subset\" I mentioned above is \"most\", but I\nadmit that is my intuition, not backed up by data.\n\n> > The api-* files have slowly grown out of\n> > date over the years, and I believe that is because they are too far\n> > removed from the code people are actually touching.\n> \n> I don't think they ever were very comprehensive or up to date.\n\nSurely at least some of them were up to date when they were first\nwritten? Take api-strbuf, for example. It started in dd613e6b, which\nclaims almost complete coverage. And then it slowly grew stale as\nfunctions were added and not documented.\n\n> As far as I understand, the api-* files are intended to be usually out\n> of date. Their main purpose is to explain the gist of how to use the\n> API. They were usually written way after the API was introduced (so\n> they are behind already). They are clearly written and as long as\n> they mostly match the code, that is enough for them to be useful and\n> educational.\n\nI'm not sure any such thought as \"intended to be out of date\" went into\nit. I think it was more \"geez, these really need some documentation at\nall\". Out-of-date documentation can be dangerous and misleading, and is\none of the reasons I think it is a good idea to keep the documentation\ncloser to the code. It's not a silver bullet, but it helps make it more\nobvious when both need to be updated.\n\n> Then every few years or so, someone updates the file with the latest\n> API changes, still with an eye to overall organization and readability\n> of the document.\n\nAgain, look at api-strbuf.txt. The changes after the initial checkin are\nalmost entirely adding functions to the giant list. They are (hopefully)\nadded to the most appropriate spot, but that is also (again, hopefully)\ndone in the header file, too. I don't see any real effort towards\norganization or readability.\n\nMaybe I'm not understanding what you mean by organization or\nreadability. It seems to me like the api docs look like:\n\n some intro text and overall description\n\n some header (optional)\n\n - some-func\n\n description of some-func\n\n - some-func2\n\n description of some-func2\n\n\nand I am proposing basically:\n\n /*\n * some intro text and overall description\n */\n\n /* some header (optional) */\n\n /* description of some-func */\n int some_func(...);\n\n /* description of some-func2 */\n int some_func2(...);\n\nThe readability and organization of those do not seem substantially\ndifferent to me.\n\n> > I did drop some asciidoc-specific formatting from the main text (e.g.,\n> > \"+\" lines for connecting blocks), which means that extracting the result\n> > and running it through asciidoc won't work.\n> \n> This unfortunately lost some structure (e.g., which paragraphs were\n> part of the same bulleted list). It would be more common to use a\n> hanging indent:\n> \n> - The `buf` member is never NULL, so ...\n> string operations ...\n> by `strbuf_init()` ...\n> \n> Do not assume anything about what `buf` really is ...\n> ...\n> \n> - ...\n\nYeah, I think a hanging indent would be fine. I didn't actually spend\nmuch effort on formatting. My point was to say \"is this really so bad to\nread inline?\"\n\nWe could stick with asciidoc, too, but I really would like to keep the\nbarrier to writing documentation as low as possible, in order to\nencourage it happening. I find that asciidoc syntax often ends up making\nboth writing and reading the source more tedious than it needs to be.\n\n> > I think it makes the actual\n> > text much more readable, though. But we could do it the other way if\n> > people really want to asciidoc it.\n> \n> I also don't mind losing the asciidoc markup, especially given that it\n> would be a pain to reconstruct and render.\n\nI'm not sure it would be that hard if you can extract to text (which you\nseem to want). But in the patch I posted (and the outline I showed\nabove), I chose to turn:\n\n foo::\n description of foo\n\ninto just:\n\n /* description of foo */\n void foo(void);\n\nto avoid writers having to repeat themselves. But it does mean that the\nextracted comments do not stand alone. You can either hack your own\nparser (which is what doxygen does), or build something on top of a real\nparser (there is cldoc, which uses libclang to extract comments next to\ndeclarations).\n\nThat's why I suggested the possibility of punting on extraction\nentirely, and just treating the header file as both the source and\nfinished document.\n\n> My reaction at first glance is that this is trying to do something\n> good but the current implementation is less readable than\n> Documentation/technical/api-strbuf.txt. Am I looking at a work in\n> progress, or is this meant to be ready for application? Which aspects\n> would you be interested in comments on?\n\nIt was more \"this is the direction I think we should go, and I will try\nto implement a complete conversion to convince myself and others that I\nam not forgetting any corner case\". I probably should have given the\n\"outline\" I gave above along with it.\n\nI think the end result that I posted is still strictly better than what\nwe have currently, with the exception that I should have reformatted the\nhanging indents. What is it that you don't like about it?\n\nI'm not super interested in going back and forth over minor markup\nissues if there are overall issues with the concept that might block it\nfrom happening (just because they are tedious and time-consuming, and I\ndo not want to spend a lot of time formatting something that will get\nthrown away). So please list your complaints in order of increasing\nspecificity. :)\n\n-Peff\n"},{"id":"253641","messageId":"20141212183114.GA29365@google.com","threadId":"38124","inReplyTo":"20141212091625.GA9049@peff.net","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-12T18:31:14Z","receivedAt":"2014-12-12T18:31:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I'm not sure any such thought as \"intended to be out of date\" went into\n> it.\n\nJunio started the documentation in v1.5.4-rc1~49 (2007-11-24). I'm\nnot sure if there was a discussion preceding that commit. My\nunderstanding was always that putting the documentation out-of-line\nwas an intentional decision and that it was understood that the\ndocumentation would have cycles of falling out of date and needing to\nbe updated, but I may be misunderstanding the history.\n\nSeparate from the question of history, I honestly prefer this way of\ndoing API documentation relative to 90% of the API documentation in\nheaders I've seen in other projects. I suspect you don't. That's\nokay --- it's possible for rational people to disagree about things.\n\nIt's moot given that we don't seem to disagree about what should be\ndone about it. Why keep arguing?\n\n> I think the end result that I posted is still strictly better than what\n> we have currently, with the exception that I should have reformatted the\n> hanging indents. What is it that you don't like about it?\n\nOther issues of inconsistent markup. For example, some comments are\nstrangely indented, and some look like\n\n\t/* First line\n\t * second line\n\t * third line */\n\n> I'm not super interested in going back and forth over minor markup\n> issues if there are overall issues with the concept that might block it\n> from happening (just because they are tedious and time-consuming, and I\n> do not want to spend a lot of time formatting something that will get\n> thrown away). So please list your complaints in order of increasing\n> specificity. :)\n\nThe overall concept is good.\n\nI chose not to list markup issues for the same reason (it's more\ntedious to go back and forth than for someone to spend some time to\nget it mostly-right first on their own). I am kind of confused about\nthe current status, since I've said again and again that I'd be happy\nto see the documentation in the header but you still seem to be trying\nto convince me of that. What am I missing?\n\nJonathan\n"},{"id":"253647","messageId":"xmqqoar8brto.fsf@gitster.dls.corp.google.com","threadId":"38124","inReplyTo":"20141212183114.GA29365@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-12T19:17:55Z","receivedAt":"2014-12-12T19:17:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jeff King wrote:\n>\n>> I'm not sure any such thought as \"intended to be out of date\" went into\n>> it.\n>\n> Junio started the documentation in v1.5.4-rc1~49 (2007-11-24). I'm\n> not sure if there was a discussion preceding that commit. My\n> understanding was always that putting the documentation out-of-line\n> was an intentional decision and that it was understood that the\n> documentation would have cycles of falling out of date and needing to\n> be updated, but I may be misunderstanding the history.\n\nThere of course was not \"intended to be out of date\". It was just\nthat the prevailing style of comments in the header back then was\nsporadic per-function explanation that would have served only as a\nreminder for those who have read the corresponding *.c file, without\nany overall structure of the API described.\n\nWe could have gone to \"in header description\" route back then and\nthe commit you referred to could have added\n\n\t/*\n * NEEDSWORK: describe the overall structure of the API\n * here, so that people can use it without reading the\n * implementation of the API itself.\n */\n\nto each header file instead. I chose not to primarily because those\nwho wanted to have documentation pages that are pointed from api-index\nwere audience different from those who updated *.[ch] files, and also\nI had slight preference to have a set of *.txt files that do not force\nme to squelch /* * * */ while reading them.\n\n> Separate from the question of history, I honestly prefer this way of\n> doing API documentation relative to 90% of the API documentation in\n> headers I've seen in other projects. I suspect you don't. That's\n> okay --- it's possible for rational people to disagree about things.\n\nYes.\n\n> The overall concept is good.\n\nOK, then let's agree to go in the \"comprehensive doc in the *.h and\nthen those who want *.txt can help the machinery to extract\"\ndirection.\n\nThanks.\n"},{"id":"253648","messageId":"CAGZ79kbKCRKyugTnnx34AkeqN0NDBL_J2yGky06E+263oQ_6hg@mail.gmail.com","threadId":"38124","inReplyTo":"20141212183114.GA29365@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-12T19:19:23Z","receivedAt":"2014-12-12T19:19:23Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 12, 2014 at 10:31 AM, Jonathan Nieder wrote:\n> Jeff King wrote:\n>\n>> I'm not sure any such thought as \"intended to be out of date\" went into\n>> it.\n>\n> Junio started the documentation in v1.5.4-rc1~49 (2007-11-24). I'm\n> not sure if there was a discussion preceding that commit. My\n> understanding was always that putting the documentation out-of-line\n> was an intentional decision and that it was understood that the\n> documentation would have cycles of falling out of date and needing to\n> be updated, but I may be misunderstanding the history.\n>\n> Separate from the question of history, I honestly prefer this way of\n> doing API documentation relative to 90% of the API documentation in\n> headers I've seen in other projects. I suspect you don't. That's\n> okay --- it's possible for rational people to disagree about things.\n>\n> It's moot given that we don't seem to disagree about what should be\n> done about it. Why keep arguing?\n>\n\nSo you proposed an order of desirability w.r.t. the way we want to go\ndocumenting things.\n\n> Some possibilities, in order of my preference (earlier items are better):\n>\n> 1. Move documentation to header and provide a program to generate a nice\n> standalone document.\n\nJunio agrees with that order. Michael said\n\n> My vote is for putting the API docs in the header files:\n\nwhich seems to be an agreement with your most desired way to do it.\n\nJeff said earlier when reviewing your patch:\n\n> I somewhat feel this goes in the opposite direction of where we want to\n> be *(all data in one place, but that place is the header)*.\n\nwhich sounds to me as if he prefers also the header as the one place of truth.\n\nI do agree on your proposed order of desirability as well. So nobody\nshowed up yet,\nwho dislikes the headers as the one place of truth.\n\nSo the open question is:\n* The historic reasoning to introduce technical/api-$header at all. As\nJunio mainly\n put persons to ask into the stubs in 530e741c726 (2007-11-24, Start\npreparing the API documents.),\n it was probably improving the status quo back then. Maybe it was\neasier to have all names in place\n than using git blame/git pickaxe at the time?\n Answering this question makes sure we don't oversee some reasons.\n\n* Assuming we put everything into headers now, we'd need to discuss\n -> Do we want to extract it to technical/api-$somedoc later at all?\n(There seems to be some disagreement?)\n -> How do we extract (plain sed for lines starting with \"*\", or\ndoxygen, or a self cooked script)\n -> How do we ensure the generated documentation doesn't get stale over time?\n\nAs I personally just look up things in the header anyway, I'd rather\nget started on polishing the comments in the\nheaders as on arguing how to extract it. Though that's a required step\nto know which format we want in the headers.\n\nThanks,\nStefan\n"},{"id":"253649","messageId":"20141212192417.GA21132@peff.net","threadId":"38124","inReplyTo":"20141212183114.GA29365@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-12T19:24:17Z","receivedAt":"2014-12-12T19:24:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 12, 2014 at 10:31:14AM -0800, Jonathan Nieder wrote:\n\n> Separate from the question of history, I honestly prefer this way of\n> doing API documentation relative to 90% of the API documentation in\n> headers I've seen in other projects. I suspect you don't. That's\n> okay --- it's possible for rational people to disagree about things.\n> \n> It's moot given that we don't seem to disagree about what should be\n> done about it. Why keep arguing?\n\nFair enough.\n\n> > I think the end result that I posted is still strictly better than what\n> > we have currently, with the exception that I should have reformatted the\n> > hanging indents. What is it that you don't like about it?\n> \n> Other issues of inconsistent markup. For example, some comments are\n> strangely indented, and some look like\n> \n> \t/* First line\n> \t * second line\n> \t * third line */\n\nYeah, I had to strip the indent from:\n\n strbuf_init::\n\tdescription of strbuf_init...\n\nwhen \"strbuf_init::\" went away, but it looks like I missed two of them.\nAnd I see one \"/* First line\" where the \"header\" from \"Adding data to\nthe buffer\" got merged with a note. I agree those should be fixed.\n\nI used our normal single-line comment style for most of those:\n\n /* Related to the size of the buffer */\n\nin some places. But I'd be happy, too, with:\n\n /*\n * Related to the size of the buffer\n */\n\n(and probably using a full sentence, or caps or underlining or something\nto indicate that the comment begins a new section of the header file).\n\n> I chose not to list markup issues for the same reason (it's more\n> tedious to go back and forth than for someone to spend some time to\n> get it mostly-right first on their own). I am kind of confused about\n> the current status, since I've said again and again that I'd be happy\n> to see the documentation in the header but you still seem to be trying\n> to convince me of that. What am I missing?\n\nI think partially it was crossing mails. While I was writing the\nresponse that you responded to just now, I ended up postponing it for\nother work, and in the meantime you sent several more messages\nindicating you were OK with moving documentation into the headers. I\nalmost scrapped my response, but frankly I was left a bit confused by\nyour position, since it seemed the opposite of the 2 patches you had\nsent moving things out of headers.\n\nDo not feel obligated to make me unconfused. If you are on board with\nputting documentation in the headers, then I think we can move forward\nwith that plan.\n\nSo what next?\n\nI agree there are some formatting problems in the strbuf.h patch I sent\nearlier. I'm happy to fix them and resend, but I'm not 100% sure that\nfixing all the problems I see will not leave problems for you. I can fix\nthem and you can review if you want. Or alternatively, if you have more\ndrastic formatting or wording changes in mind, maybe it would make sense\nfor you to take a pass?\n\nI _don't_ want to commit to moving all of api-* into headers myself.\nIt's something I would rather have happen over time as people touch\nareas[1], because it's tedious and time-consuming. That does create an\ninconsistency, in the sense that some APIs will be documented in\nDocumentation/technical, and others in their header files. But as long\nas each _individual_ API is documented in one or the other, I don't see\nthat as a big problem (of course the status quo is a mix for some APIs,\nso we will live with that until they are touched, but that is no worse\nthan today's state).\n\n-Peff\n\n[1] I had a vague plan to put new documentation into headers, and slowly\n migrate that way until we had a critical mass. But that was moving\n somewhat slowly, and obviously escaped the notice of many people. :)\n"},{"id":"253650","messageId":"20141212192938.GB21132@peff.net","threadId":"38124","inReplyTo":"CAGZ79kbKCRKyugTnnx34AkeqN0NDBL_J2yGky06E+263oQ_6hg@mail.gmail.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-12T19:29:38Z","receivedAt":"2014-12-12T19:29:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 12, 2014 at 11:19:23AM -0800, Stefan Beller wrote:\n\n> * Assuming we put everything into headers now, we'd need to discuss\n> -> Do we want to extract it to technical/api-$somedoc later at all?\n> (There seems to be some disagreement?)\n> -> How do we extract (plain sed for lines starting with \"*\", or\n> doxygen, or a self cooked script)\n> -> How do we ensure the generated documentation doesn't get stale over time?\n\nI think it would be _nice_ to extract it, and that should be a\nconsideration when doing the manual work of moving it into headers\n(you'll notice I used the \"/**\" convention to start comments which\nshould be extractable, but I am happy to use any similar convention[1]).\n\nBut unless somebody is excited about working on the extractor\nimmediately, I think we should punt on it for now. Moving the\ndocumentation is already a big job, and I do not want to add an extra\ndependency to the task.\n\nThe exact extractor choice might influence formatting decisions (e.g.,\nshould extracted text stand alone, or must it be viewed in context of the\nfunction it prefaces), but we might be able to do a mechanical\nconversion later if those facts change.\n\n-Peff\n"},{"id":"253651","messageId":"20141212193552.GB29365@google.com","threadId":"38124","inReplyTo":"20141212192417.GA21132@peff.net","subject":"Re: [PATCH] document string_list_clear","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-12T19:35:52Z","receivedAt":"2014-12-12T19:35:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I agree there are some formatting problems in the strbuf.h patch I sent\n> earlier. I'm happy to fix them and resend, but I'm not 100% sure that\n> fixing all the problems I see will not leave problems for you. I can fix\n> them and you can review if you want. Or alternatively, if you have more\n> drastic formatting or wording changes in mind, maybe it would make sense\n> for you to take a pass?\n\nFor the sake of reviewability, I think the less that happens in a\nsingle working patch, the better. :)\n\nIf I were doing it, I would first de-asciidoc within technical/ and\nthen move into the header in a separate patch. Or first move with\nasciidoc intact and then de-asciidoc in a separate patch. Combining\nthe two into a single patch is also fine. Please don't change wording\nat the same time.\n\n> I _don't_ want to commit to moving all of api-* into headers myself.\n\nYeah, I think that's fine. Moving one at a time when people want to\ndo it doesn't hurt consistency at all relative to the status quo.\n\nJonathan\n"},{"id":"253661","messageId":"20141212212726.GA26284@peff.net","threadId":"38124","inReplyTo":"20141212193552.GB29365@google.com","subject":"Re: [PATCH] document string_list_clear","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-12T21:27:26Z","receivedAt":"2014-12-12T21:27:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 12, 2014 at 11:35:52AM -0800, Jonathan Nieder wrote:\n\n> If I were doing it, I would first de-asciidoc within technical/ and\n> then move into the header in a separate patch. Or first move with\n> asciidoc intact and then de-asciidoc in a separate patch. Combining\n> the two into a single patch is also fine. Please don't change wording\n> at the same time.\n\nHere's what I came up with. The first patch probably does more than\nyou'd like (and more than I would have done if I were starting from\nscratch today). But I didn't want to get into undoing the stripping of\neach function-name list item that I showed earlier, as it would be a lot\nof tedious manual work. If we decide we want to keep those, I'm happy to\ndo the work to restore them, but it didn't seem like a good tradeoff\njust to create an intermediate state to make the patch prettier.\n\nI did split out some of the other formatting decisions, though, so they\ncan be evaluated individually.\n\n [1/4]: strbuf: migrate api-strbuf.txt documentation to strbuf.h\n [2/4]: strbuf.h: drop asciidoc list formatting from API docs\n [3/4]: strbuf.h: format asciidoc code blocks as 4-space indent\n [4/4]: strbuf.h: reorganize api function grouping headers\n\n-Peff\n"},{"id":"253662","messageId":"20141212212800.GA27451@peff.net","threadId":"38124","inReplyTo":"20141212212726.GA26284@peff.net","subject":"[PATCH 1/4] strbuf: migrate api-strbuf.txt documentation to strbuf.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-12T21:28:00Z","receivedAt":"2014-12-12T21:28:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Some of strbuf is documented as comments above functions,\nand some is separate in Documentation/technical/api-strbuf.txt.\nThis makes it annoying to find the appropriate documentation.\nWe'd rather have it all in one place, which means all in the\ntext document, or all in the header.\n\nLet's choose the header as that place. Even though the\nformatting is not quite as pretty, this keeps the\ndocumentation close to the related code. The hope is that\nthis makes it easier to find what you want (human-readable\ncomments are right next to the C declarations), and easier\nfor writers to keep the documentation up to date.\n\nThis is more or less a straight import of the text from\napi-strbuf.txt into C comments, complete with asciidoc\nformatting. The exceptions are:\n\n 1. All comments created in this way are started with \"/**\"\n to indicate they are part of the API documentation. This\n may help later with extracting the text to pretty-print\n it.\n\n 2. Function descriptions do not repeat the function name,\n as it is available in the context directly below. So:\n\n `strbuf_add`::\n\n Add data of given length to the buffer.\n\n from api-strbuf.txt becomes:\n\n /**\n * Add data of given length to the buffer.\n */\n void strbuf_add(struct strbuf *sb, const void *, size_t);\n\n As a result, any block-continuation required in asciidoc\n for that list item was dropped in favor of straight\n blank-line paragraph (since it is not necessary when we\n are not in a list item).\n\n 3. There is minor re-wording to integrate existing comments\n and api-strbuf text. In each case, I took whichever\n version was more descriptive, and eliminated any\n redundancies. In one case, for strbuf_addstr, the api\n documentation gave its inline definition; I eliminated\n this as redundant with the actual definition, which can\n be seen directly below the comment.\n\n 4. The functions in the header file are re-ordered to match\n the ordering of the API documentation, under the\n assumption that more thought went into the grouping\n there.\n\nSigned-off-by: Jeff King \n---\n Documentation/technical/api-strbuf.txt | 351 -------------------------\n strbuf.h | 458 ++++++++++++++++++++++++++++-----\n 2 files changed, 390 insertions(+), 419 deletions(-)\n delete mode 100644 Documentation/technical/api-strbuf.txt\n\ndiff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt\ndeleted file mode 100644\nindex cca6543..0000000\n--- a/Documentation/technical/api-strbuf.txt\n+++ /dev/null\n@@ -1,351 +0,0 @@\n-strbuf API\n-==========\n-\n-strbuf's are meant to be used with all the usual C string and memory\n-APIs. Given that the length of the buffer is known, it's often better to\n-use the mem* functions than a str* one (memchr vs. strchr e.g.).\n-Though, one has to be careful about the fact that str* functions often\n-stop on NULs and that strbufs may have embedded NULs.\n-\n-A strbuf is NUL terminated for convenience, but no function in the\n-strbuf API actually relies on the string being free of NULs.\n-\n-strbufs have some invariants that are very important to keep in mind:\n-\n-. The `buf` member is never NULL, so it can be used in any usual C\n-string operations safely. strbuf's _have_ to be initialized either by\n-`strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.\n-+\n-Do *not* assume anything on what `buf` really is (e.g. if it is\n-allocated memory or not), use `strbuf_detach()` to unwrap a memory\n-buffer from its strbuf shell in a safe way. That is the sole supported\n-way. This will give you a malloced buffer that you can later `free()`.\n-+\n-However, it is totally safe to modify anything in the string pointed by\n-the `buf` member, between the indices `0` and `len-1` (inclusive).\n-\n-. The `buf` member is a byte array that has at least `len + 1` bytes\n- allocated. The extra byte is used to store a `'\\0'`, allowing the\n- `buf` member to be a valid C-string. Every strbuf function ensure this\n- invariant is preserved.\n-+\n-NOTE: It is OK to \"play\" with the buffer directly if you work it this\n- way:\n-+\n-----\n-strbuf_grow(sb, SOME_SIZE); <1>\n-strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);\n-----\n-<1> Here, the memory array starting at `sb->buf`, and of length\n-`strbuf_avail(sb)` is all yours, and you can be sure that\n-`strbuf_avail(sb)` is at least `SOME_SIZE`.\n-+\n-NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.\n-+\n-Doing so is safe, though if it has to be done in many places, adding the\n-missing API to the strbuf module is the way to go.\n-+\n-WARNING: Do _not_ assume that the area that is yours is of size `alloc\n-- 1` even if it's true in the current implementation. Alloc is somehow a\n-\"private\" member that should not be messed with. Use `strbuf_avail()`\n-instead.\n-\n-Data structures\n----------------\n-\n-* `struct strbuf`\n-\n-This is the string buffer structure. The `len` member can be used to\n-determine the current length of the string, and `buf` member provides\n-access to the string itself.\n-\n-Functions\n----------\n-\n-* Life cycle\n-\n-`strbuf_init`::\n-\n-\tInitialize the structure. The second parameter can be zero or a bigger\n-\tnumber to allocate memory, in case you want to prevent further reallocs.\n-\n-`strbuf_release`::\n-\n-\tRelease a string buffer and the memory it used. You should not use the\n-\tstring buffer after using this function, unless you initialize it again.\n-\n-`strbuf_detach`::\n-\n-\tDetach the string from the strbuf and returns it; you now own the\n-\tstorage the string occupies and it is your responsibility from then on\n-\tto release it with `free(3)` when you are done with it.\n-\n-`strbuf_attach`::\n-\n-\tAttach a string to a buffer. You should specify the string to attach,\n-\tthe current length of the string and the amount of allocated memory.\n-\tThe amount must be larger than the string length, because the string you\n-\tpass is supposed to be a NUL-terminated string. This string _must_ be\n-\tmalloc()ed, and after attaching, the pointer cannot be relied upon\n-\tanymore, and neither be free()d directly.\n-\n-`strbuf_swap`::\n-\n-\tSwap the contents of two string buffers.\n-\n-* Related to the size of the buffer\n-\n-`strbuf_avail`::\n-\n-\tDetermine the amount of allocated but unused memory.\n-\n-`strbuf_grow`::\n-\n-\tEnsure that at least this amount of unused memory is available after\n-\t`len`. This is used when you know a typical size for what you will add\n-\tand want to avoid repetitive automatic resizing of the underlying buffer.\n-\tThis is never a needed operation, but can be critical for performance in\n-\tsome cases.\n-\n-`strbuf_setlen`::\n-\n-\tSet the length of the buffer to a given value. This function does *not*\n-\tallocate new memory, so you should not perform a `strbuf_setlen()` to a\n-\tlength that is larger than `len + strbuf_avail()`. `strbuf_setlen()` is\n-\tjust meant as a 'please fix invariants from this strbuf I just messed\n-\twith'.\n-\n-`strbuf_reset`::\n-\n-\tEmpty the buffer by setting the size of it to zero.\n-\n-* Related to the contents of the buffer\n-\n-`strbuf_trim`::\n-\n-\tStrip whitespace from the beginning and end of a string.\n-\tEquivalent to performing `strbuf_rtrim()` followed by `strbuf_ltrim()`.\n-\n-`strbuf_rtrim`::\n-\n-\tStrip whitespace from the end of a string.\n-\n-`strbuf_ltrim`::\n-\n-\tStrip whitespace from the beginning of a string.\n-\n-`strbuf_reencode`::\n-\n-\tReplace the contents of the strbuf with a reencoded form. Returns -1\n-\ton error, 0 on success.\n-\n-`strbuf_tolower`::\n-\n-\tLowercase each character in the buffer using `tolower`.\n-\n-`strbuf_cmp`::\n-\n-\tCompare two buffers. Returns an integer less than, equal to, or greater\n-\tthan zero if the first buffer is found, respectively, to be less than,\n-\tto match, or be greater than the second buffer.\n-\n-* Adding data to the buffer\n-\n-NOTE: All of the functions in this section will grow the buffer as necessary.\n-If they fail for some reason other than memory shortage and the buffer hadn't\n-been allocated before (i.e. the `struct strbuf` was set to `STRBUF_INIT`),\n-then they will free() it.\n-\n-`strbuf_addch`::\n-\n-\tAdd a single character to the buffer.\n-\n-`strbuf_addchars`::\n-\n-\tAdd a character the specified number of times to the buffer.\n-\n-`strbuf_insert`::\n-\n-\tInsert data to the given position of the buffer. The remaining contents\n-\twill be shifted, not overwritten.\n-\n-`strbuf_remove`::\n-\n-\tRemove given amount of data from a given position of the buffer.\n-\n-`strbuf_splice`::\n-\n-\tRemove the bytes between `pos..pos+len` and replace it with the given\n-\tdata.\n-\n-`strbuf_add_commented_lines`::\n-\n-\tAdd a NUL-terminated string to the buffer. Each line will be prepended\n-\tby a comment character and a blank.\n-\n-`strbuf_add`::\n-\n-\tAdd data of given length to the buffer.\n-\n-`strbuf_addstr`::\n-\n-Add a NUL-terminated string to the buffer.\n-+\n-NOTE: This function will *always* be implemented as an inline or a macro\n-that expands to:\n-+\n-----\n-strbuf_add(..., s, strlen(s));\n-----\n-+\n-Meaning that this is efficient to write things like:\n-+\n-----\n-strbuf_addstr(sb, \"immediate string\");\n-----\n-\n-`strbuf_addbuf`::\n-\n-\tCopy the contents of another buffer at the end of the current one.\n-\n-`strbuf_adddup`::\n-\n-\tCopy part of the buffer from a given position till a given length to the\n-\tend of the buffer.\n-\n-`strbuf_expand`::\n-\n-\tThis function can be used to expand a format string containing\n-\tplaceholders. To that end, it parses the string and calls the specified\n-\tfunction for every percent sign found.\n-+\n-The callback function is given a pointer to the character after the `%`\n-and a pointer to the struct strbuf. It is expected to add the expanded\n-version of the placeholder to the strbuf, e.g. to add a newline\n-character if the letter `n` appears after a `%`. The function returns\n-the length of the placeholder recognized and `strbuf_expand()` skips\n-over it.\n-+\n-The format `%%` is automatically expanded to a single `%` as a quoting\n-mechanism; callers do not need to handle the `%` placeholder themselves,\n-and the callback function will not be invoked for this placeholder.\n-+\n-All other characters (non-percent and not skipped ones) are copied\n-verbatim to the strbuf. If the callback returned zero, meaning that the\n-placeholder is unknown, then the percent sign is copied, too.\n-+\n-In order to facilitate caching and to make it possible to give\n-parameters to the callback, `strbuf_expand()` passes a context pointer,\n-which can be used by the programmer of the callback as she sees fit.\n-\n-`strbuf_expand_dict_cb`::\n-\n-\tUsed as callback for `strbuf_expand()`, expects an array of\n-\tstruct strbuf_expand_dict_entry as context, i.e. pairs of\n-\tplaceholder and replacement string. The array needs to be\n-\tterminated by an entry with placeholder set to NULL.\n-\n-`strbuf_addbuf_percentquote`::\n-\n-\tAppend the contents of one strbuf to another, quoting any\n-\tpercent signs (\"%\") into double-percents (\"%%\") in the\n-\tdestination. This is useful for literal data to be fed to either\n-\tstrbuf_expand or to the *printf family of functions.\n-\n-`strbuf_humanise_bytes`::\n-\n-\tAppend the given byte size as a human-readable string (i.e. 12.23 KiB,\n-\t3.50 MiB).\n-\n-`strbuf_addf`::\n-\n-\tAdd a formatted string to the buffer.\n-\n-`strbuf_commented_addf`::\n-\n-\tAdd a formatted string prepended by a comment character and a\n-\tblank to the buffer.\n-\n-`strbuf_fread`::\n-\n-\tRead a given size of data from a FILE* pointer to the buffer.\n-+\n-NOTE: The buffer is rewound if the read fails. If -1 is returned,\n-`errno` must be consulted, like you would do for `read(3)`.\n-`strbuf_read()`, `strbuf_read_file()` and `strbuf_getline()` has the\n-same behaviour as well.\n-\n-`strbuf_read`::\n-\n-\tRead the contents of a given file descriptor. The third argument can be\n-\tused to give a hint about the file size, to avoid reallocs.\n-\n-`strbuf_read_file`::\n-\n-\tRead the contents of a file, specified by its path. The third argument\n-\tcan be used to give a hint about the file size, to avoid reallocs.\n-\n-`strbuf_readlink`::\n-\n-\tRead the target of a symbolic link, specified by its path. The third\n-\targument can be used to give a hint about the size, to avoid reallocs.\n-\n-`strbuf_getline`::\n-\n-\tRead a line from a FILE *, overwriting the existing contents\n-\tof the strbuf. The second argument specifies the line\n-\tterminator character, typically `'\\n'`.\n-\tReading stops after the terminator or at EOF. The terminator\n-\tis removed from the buffer before returning. Returns 0 unless\n-\tthere was nothing left before EOF, in which case it returns `EOF`.\n-\n-`strbuf_getwholeline`::\n-\n-\tLike `strbuf_getline`, but keeps the trailing terminator (if\n-\tany) in the buffer.\n-\n-`strbuf_getwholeline_fd`::\n-\n-\tLike `strbuf_getwholeline`, but operates on a file descriptor.\n-\tIt reads one character at a time, so it is very slow. Do not\n-\tuse it unless you need the correct position in the file\n-\tdescriptor.\n-\n-`strbuf_getcwd`::\n-\n-\tSet the buffer to the path of the current working directory.\n-\n-`strbuf_add_absolute_path`\n-\n-\tAdd a path to a buffer, converting a relative path to an\n-\tabsolute one in the process. Symbolic links are not\n-\tresolved.\n-\n-`stripspace`::\n-\n-\tStrip whitespace from a buffer. The second parameter controls if\n-\tcomments are considered contents to be removed or not.\n-\n-`strbuf_split_buf`::\n-`strbuf_split_str`::\n-`strbuf_split_max`::\n-`strbuf_split`::\n-\n-\tSplit a string or strbuf into a list of strbufs at a specified\n-\tterminator character. The returned substrings include the\n-\tterminator characters. Some of these functions take a `max`\n-\tparameter, which, if positive, limits the output to that\n-\tnumber of substrings.\n-\n-`strbuf_list_free`::\n-\n-\tFree a list of strbufs (for example, the return values of the\n-\t`strbuf_split()` functions).\n-\n-`launch_editor`::\n-\n-\tLaunch the user preferred editor to edit a file and fill the buffer\n-\twith the file's contents upon the user completing their editing. The\n-\tthird argument can be used to set the environment which the editor is\n-\trun in. If the buffer is NULL the editor is launched as usual but the\n-\tfile's contents are not read into the buffer upon completion.\ndiff --git a/strbuf.h b/strbuf.h\nindex 652b6c4..0a83f9a 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -1,22 +1,117 @@\n #ifndef STRBUF_H\n #define STRBUF_H\n \n-/* See Documentation/technical/api-strbuf.txt */\n+/**\n+ * strbuf's are meant to be used with all the usual C string and memory\n+ * APIs. Given that the length of the buffer is known, it's often better to\n+ * use the mem* functions than a str* one (memchr vs. strchr e.g.).\n+ * Though, one has to be careful about the fact that str* functions often\n+ * stop on NULs and that strbufs may have embedded NULs.\n+ *\n+ * A strbuf is NUL terminated for convenience, but no function in the\n+ * strbuf API actually relies on the string being free of NULs.\n+ *\n+ * strbufs have some invariants that are very important to keep in mind:\n+ *\n+ * . The `buf` member is never NULL, so it can be used in any usual C\n+ * string operations safely. strbuf's _have_ to be initialized either by\n+ * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.\n+ * +\n+ * Do *not* assume anything on what `buf` really is (e.g. if it is\n+ * allocated memory or not), use `strbuf_detach()` to unwrap a memory\n+ * buffer from its strbuf shell in a safe way. That is the sole supported\n+ * way. This will give you a malloced buffer that you can later `free()`.\n+ * +\n+ * However, it is totally safe to modify anything in the string pointed by\n+ * the `buf` member, between the indices `0` and `len-1` (inclusive).\n+ *\n+ * . The `buf` member is a byte array that has at least `len + 1` bytes\n+ * allocated. The extra byte is used to store a `'\\0'`, allowing the\n+ * `buf` member to be a valid C-string. Every strbuf function ensure this\n+ * invariant is preserved.\n+ * +\n+ * NOTE: It is OK to \"play\" with the buffer directly if you work it this\n+ * way:\n+ * +\n+ * ----\n+ * strbuf_grow(sb, SOME_SIZE); <1>\n+ * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);\n+ * ----\n+ * <1> Here, the memory array starting at `sb->buf`, and of length\n+ * `strbuf_avail(sb)` is all yours, and you can be sure that\n+ * `strbuf_avail(sb)` is at least `SOME_SIZE`.\n+ * +\n+ * NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.\n+ * +\n+ * Doing so is safe, though if it has to be done in many places, adding the\n+ * missing API to the strbuf module is the way to go.\n+ * +\n+ * WARNING: Do _not_ assume that the area that is yours is of size `alloc\n+ * - 1` even if it's true in the current implementation. Alloc is somehow a\n+ * \"private\" member that should not be messed with. Use `strbuf_avail()`\n+ * instead.\n+ */\n \n-extern char strbuf_slopbuf[];\n+/*\n+ * Data Structures\n+ * ---------------\n+ */\n+\n+/**\n+ * This is the string buffer structure. The `len` member can be used to\n+ * determine the current length of the string, and `buf` member provides\n+ * access to the string itself.\n+ */\n struct strbuf {\n \tsize_t alloc;\n \tsize_t len;\n \tchar *buf;\n };\n \n+extern char strbuf_slopbuf[];\n #define STRBUF_INIT { 0, 0, strbuf_slopbuf }\n \n-/*----- strbuf life cycle -----*/\n+/**\n+ * Functions\n+ * ---------\n+ */\n+\n+/**\n+ * * Life Cycle\n+ */\n+\n+/**\n+ * Initialize the structure. The second parameter can be zero or a bigger\n+ * number to allocate memory, in case you want to prevent further reallocs.\n+ */\n extern void strbuf_init(struct strbuf *, size_t);\n+\n+/**\n+ * Release a string buffer and the memory it used. You should not use the\n+ * string buffer after using this function, unless you initialize it again.\n+ */\n extern void strbuf_release(struct strbuf *);\n+\n+/**\n+ * Detach the string from the strbuf and returns it; you now own the\n+ * storage the string occupies and it is your responsibility from then on\n+ * to release it with `free(3)` when you are done with it.\n+ */\n extern char *strbuf_detach(struct strbuf *, size_t *);\n+\n+/**\n+ * Attach a string to a buffer. You should specify the string to attach,\n+ * the current length of the string and the amount of allocated memory.\n+ * The amount must be larger than the string length, because the string you\n+ * pass is supposed to be a NUL-terminated string. This string _must_ be\n+ * malloc()ed, and after attaching, the pointer cannot be relied upon\n+ * anymore, and neither be free()d directly.\n+ */\n extern void strbuf_attach(struct strbuf *, void *, size_t, size_t);\n+\n+/**\n+ * Swap the contents of two string buffers.\n+ */\n static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)\n {\n \tstruct strbuf tmp = *a;\n@@ -24,14 +119,35 @@ static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)\n \t*b = tmp;\n }\n \n-/*----- strbuf size related -----*/\n+\n+/**\n+ * * Related to the size of the buffer\n+ */\n+\n+/**\n+ * Determine the amount of allocated but unused memory.\n+ */\n static inline size_t strbuf_avail(const struct strbuf *sb)\n {\n \treturn sb->alloc ? sb->alloc - sb->len - 1 : 0;\n }\n \n+/**\n+ * Ensure that at least this amount of unused memory is available after\n+ * `len`. This is used when you know a typical size for what you will add\n+ * and want to avoid repetitive automatic resizing of the underlying buffer.\n+ * This is never a needed operation, but can be critical for performance in\n+ * some cases.\n+ */\n extern void strbuf_grow(struct strbuf *, size_t);\n \n+/**\n+ * Set the length of the buffer to a given value. This function does *not*\n+ * allocate new memory, so you should not perform a `strbuf_setlen()` to a\n+ * length that is larger than `len + strbuf_avail()`. `strbuf_setlen()` is\n+ * just meant as a 'please fix invariants from this strbuf I just messed\n+ * with'.\n+ */\n static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n {\n \tif (len > (sb->alloc ? sb->alloc - 1 : 0))\n@@ -39,16 +155,285 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n \tsb->len = len;\n \tsb->buf[len] = '\\0';\n }\n+\n+/**\n+ * Empty the buffer by setting the size of it to zero.\n+ */\n #define strbuf_reset(sb) strbuf_setlen(sb, 0)\n \n-/*----- content related -----*/\n+\n+/**\n+ * * Related to the contents of the buffer\n+ */\n+\n+/**\n+ * Strip whitespace from the beginning and end of a string.\n+ * Equivalent to performing `strbuf_rtrim()` followed by `strbuf_ltrim()`.\n+ */\n extern void strbuf_trim(struct strbuf *);\n+\n+/**\n+ * Strip whitespace from the end of a string.\n+ */\n extern void strbuf_rtrim(struct strbuf *);\n+\n+/**\n+ * Strip whitespace from the beginning of a string.\n+ */\n extern void strbuf_ltrim(struct strbuf *);\n+\n+/**\n+ * Replace the contents of the strbuf with a reencoded form. Returns -1\n+ * on error, 0 on success.\n+ */\n extern int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);\n+\n+/**\n+ * Lowercase each character in the buffer using `tolower`.\n+ */\n extern void strbuf_tolower(struct strbuf *sb);\n+\n+/**\n+ * Compare two buffers. Returns an integer less than, equal to, or greater\n+ * than zero if the first buffer is found, respectively, to be less than,\n+ * to match, or be greater than the second buffer.\n+ */\n extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n \n+\n+/**\n+ * Adding data to the buffer\n+ *\n+ * NOTE: All of the functions in this section will grow the buffer as\n+ * necessary. If they fail for some reason other than memory shortage and the\n+ * buffer hadn't been allocated before (i.e. the `struct strbuf` was set to\n+ * `STRBUF_INIT`), then they will free() it.\n+ */\n+\n+/**\n+ * Add a single character to the buffer.\n+ */\n+static inline void strbuf_addch(struct strbuf *sb, int c)\n+{\n+\tstrbuf_grow(sb, 1);\n+\tsb->buf[sb->len++] = c;\n+\tsb->buf[sb->len] = '\\0';\n+}\n+\n+/**\n+ * Add a character the specified number of times to the buffer.\n+ */\n+extern void strbuf_addchars(struct strbuf *sb, int c, size_t n);\n+\n+/**\n+ * Insert data to the given position of the buffer. The remaining contents\n+ * will be shifted, not overwritten.\n+ */\n+extern void strbuf_insert(struct strbuf *, size_t pos, const void *, size_t);\n+\n+/**\n+ * Remove given amount of data from a given position of the buffer.\n+ */\n+extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);\n+\n+/**\n+ * Remove the bytes between `pos..pos+len` and replace it with the given\n+ * data.\n+ */\n+extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,\n+\t\t\t const void *, size_t);\n+\n+/**\n+ * Add a NUL-terminated string to the buffer. Each line will be prepended\n+ * by a comment character and a blank.\n+ */\n+extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);\n+\n+\n+/**\n+ * Add data of given length to the buffer.\n+ */\n+extern void strbuf_add(struct strbuf *, const void *, size_t);\n+\n+/**\n+ * Add a NUL-terminated string to the buffer.\n+ *\n+ * NOTE: This function will *always* be implemented as an inline or a macro\n+ * using strlen, meaning that this is efficient to write things like:\n+ *\n+ * ----\n+ * strbuf_addstr(sb, \"immediate string\");\n+ * ----\n+ *\n+ */\n+static inline void strbuf_addstr(struct strbuf *sb, const char *s)\n+{\n+\tstrbuf_add(sb, s, strlen(s));\n+}\n+\n+/**\n+ * Copy the contents of another buffer at the end of the current one.\n+ */\n+static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)\n+{\n+\tstrbuf_grow(sb, sb2->len);\n+\tstrbuf_add(sb, sb2->buf, sb2->len);\n+}\n+\n+/**\n+ * Copy part of the buffer from a given position till a given length to the\n+ * end of the buffer.\n+ */\n+extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);\n+\n+/**\n+ * This function can be used to expand a format string containing\n+ * placeholders. To that end, it parses the string and calls the specified\n+ * function for every percent sign found.\n+ *\n+ * The callback function is given a pointer to the character after the `%`\n+ * and a pointer to the struct strbuf. It is expected to add the expanded\n+ * version of the placeholder to the strbuf, e.g. to add a newline\n+ * character if the letter `n` appears after a `%`. The function returns\n+ * the length of the placeholder recognized and `strbuf_expand()` skips\n+ * over it.\n+ *\n+ * The format `%%` is automatically expanded to a single `%` as a quoting\n+ * mechanism; callers do not need to handle the `%` placeholder themselves,\n+ * and the callback function will not be invoked for this placeholder.\n+ *\n+ * All other characters (non-percent and not skipped ones) are copied\n+ * verbatim to the strbuf. If the callback returned zero, meaning that the\n+ * placeholder is unknown, then the percent sign is copied, too.\n+ *\n+ * In order to facilitate caching and to make it possible to give\n+ * parameters to the callback, `strbuf_expand()` passes a context pointer,\n+ * which can be used by the programmer of the callback as she sees fit.\n+ */\n+typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);\n+extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);\n+\n+/**\n+ * Used as callback for `strbuf_expand()`, expects an array of\n+ * struct strbuf_expand_dict_entry as context, i.e. pairs of\n+ * placeholder and replacement string. The array needs to be\n+ * terminated by an entry with placeholder set to NULL.\n+ */\n+struct strbuf_expand_dict_entry {\n+\tconst char *placeholder;\n+\tconst char *value;\n+};\n+extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context);\n+\n+/**\n+ * Append the contents of one strbuf to another, quoting any\n+ * percent signs (\"%\") into double-percents (\"%%\") in the\n+ * destination. This is useful for literal data to be fed to either\n+ * strbuf_expand or to the *printf family of functions.\n+ */\n+extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n+\n+/**\n+ * Append the given byte size as a human-readable string (i.e. 12.23 KiB,\n+ * 3.50 MiB).\n+ */\n+extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);\n+\n+/**\n+ * Add a formatted string to the buffer.\n+ */\n+__attribute__((format (printf,2,3)))\n+extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n+\n+/**\n+ * Add a formatted string prepended by a comment character and a\n+ * blank to the buffer.\n+ */\n+__attribute__((format (printf, 2, 3)))\n+extern void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);\n+\n+__attribute__((format (printf,2,0)))\n+extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n+\n+/**\n+ * Read a given size of data from a FILE* pointer to the buffer.\n+ *\n+ * NOTE: The buffer is rewound if the read fails. If -1 is returned,\n+ * `errno` must be consulted, like you would do for `read(3)`.\n+ * `strbuf_read()`, `strbuf_read_file()` and `strbuf_getline()` has the\n+ * same behaviour as well.\n+ */\n+extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n+\n+/**\n+ * Read the contents of a given file descriptor. The third argument can be\n+ * used to give a hint about the file size, to avoid reallocs. If read fails,\n+ * any partial read is undone */\n+extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n+\n+/**\n+ * Read the contents of a file, specified by its path. The third argument\n+ * can be used to give a hint about the file size, to avoid reallocs.\n+ */\n+extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n+\n+/**\n+ * Read the target of a symbolic link, specified by its path. The third\n+ * argument can be used to give a hint about the size, to avoid reallocs.\n+ */\n+extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n+\n+/**\n+ * Set the buffer to the path of the current working directory.\n+ */\n+extern int strbuf_getcwd(struct strbuf *sb);\n+\n+/**\n+ * Read a line from a FILE *, overwriting the existing contents\n+ * of the strbuf. The second argument specifies the line\n+ * terminator character, typically `'\\n'`.\n+ * Reading stops after the terminator or at EOF. The terminator\n+ * is removed from the buffer before returning. Returns 0 unless\n+ * there was nothing left before EOF, in which case it returns `EOF`.\n+ */\n+extern int strbuf_getline(struct strbuf *, FILE *, int);\n+\n+/**\n+ * Like `strbuf_getline`, but keeps the trailing terminator (if\n+ * any) in the buffer.\n+ */\n+extern int strbuf_getwholeline(struct strbuf *, FILE *, int);\n+\n+/**\n+ * Like `strbuf_getwholeline`, but operates on a file descriptor.\n+ * It reads one character at a time, so it is very slow. Do not\n+ * use it unless you need the correct position in the file\n+ * descriptor.\n+ */\n+extern int strbuf_getwholeline_fd(struct strbuf *, int, int);\n+\n+/**\n+ * Add a path to a buffer, converting a relative path to an\n+ * absolute one in the process. Symbolic links are not\n+ * resolved.\n+ */\n+extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n+\n+/**\n+ * Strip whitespace from a buffer. The second parameter controls if\n+ * comments are considered contents to be removed or not.\n+ */\n+extern void stripspace(struct strbuf *buf, int skip_comments);\n+\n+/**\n+ * Launch the user preferred editor to edit a file and fill the buffer\n+ * with the file's contents upon the user completing their editing. The\n+ * third argument can be used to set the environment which the editor is\n+ * run in. If the buffer is NULL the editor is launched as usual but the\n+ * file's contents are not read into the buffer upon completion.\n+ */\n+extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);\n+\n static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n {\n \tif (strip_suffix_mem(sb->buf, &sb->len, suffix)) {\n@@ -110,52 +495,6 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n */\n extern void strbuf_list_free(struct strbuf **);\n \n-/*----- add data in your buffer -----*/\n-static inline void strbuf_addch(struct strbuf *sb, int c)\n-{\n-\tstrbuf_grow(sb, 1);\n-\tsb->buf[sb->len++] = c;\n-\tsb->buf[sb->len] = '\\0';\n-}\n-\n-extern void strbuf_insert(struct strbuf *, size_t pos, const void *, size_t);\n-extern void strbuf_remove(struct strbuf *, size_t pos, size_t len);\n-\n-/* splice pos..pos+len with given data */\n-extern void strbuf_splice(struct strbuf *, size_t pos, size_t len,\n- const void *, size_t);\n-\n-extern void strbuf_add_commented_lines(struct strbuf *out, const char *buf, size_t size);\n-\n-extern void strbuf_add(struct strbuf *, const void *, size_t);\n-static inline void strbuf_addstr(struct strbuf *sb, const char *s)\n-{\n-\tstrbuf_add(sb, s, strlen(s));\n-}\n-static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)\n-{\n-\tstrbuf_grow(sb, sb2->len);\n-\tstrbuf_add(sb, sb2->buf, sb2->len);\n-}\n-extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);\n-extern void strbuf_addchars(struct strbuf *sb, int c, size_t n);\n-\n-typedef size_t (*expand_fn_t) (struct strbuf *sb, const char *placeholder, void *context);\n-extern void strbuf_expand(struct strbuf *sb, const char *format, expand_fn_t fn, void *context);\n-struct strbuf_expand_dict_entry {\n-\tconst char *placeholder;\n-\tconst char *value;\n-};\n-extern size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder, void *context);\n-extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *src);\n-\n-__attribute__((format (printf,2,3)))\n-extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n-__attribute__((format (printf, 2, 3)))\n-extern void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);\n-__attribute__((format (printf,2,0)))\n-extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n-\n extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);\n \n /*\n@@ -170,28 +509,11 @@ static inline void strbuf_complete_line(struct strbuf *sb)\n \t\tstrbuf_addch(sb, '\\n');\n }\n \n-extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n-/* XXX: if read fails, any partial read is undone */\n-extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n-extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n-extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n-extern int strbuf_getcwd(struct strbuf *sb);\n-\n-extern int strbuf_getwholeline(struct strbuf *, FILE *, int);\n-extern int strbuf_getline(struct strbuf *, FILE *, int);\n-extern int strbuf_getwholeline_fd(struct strbuf *, int, int);\n-\n-extern void stripspace(struct strbuf *buf, int skip_comments);\n-extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);\n-\n extern int strbuf_branchname(struct strbuf *sb, const char *name);\n extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);\n \n extern void strbuf_addstr_urlencode(struct strbuf *, const char *,\n \t\t\t\t int reserved);\n-extern void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes);\n-\n-extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n \n __attribute__((format (printf,1,2)))\n extern int printf_ln(const char *fmt, ...);\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253663","messageId":"20141212212820.GB27451@peff.net","threadId":"38124","inReplyTo":"20141212212726.GA26284@peff.net","subject":"[PATCH 2/4] strbuf.h: drop asciidoc list formatting from API docs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-12T21:28:20Z","receivedAt":"2014-12-12T21:28:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Using a hanging indent is much more readable. This means we\nwon't format as asciidoc anymore, but since we don't have a\nworking system for extracting these comments anyway, it's\nprobably more important to just make the source readable.\n\nSigned-off-by: Jeff King \n---\n strbuf.h | 74 ++++++++++++++++++++++++++++++++--------------------------------\n 1 file changed, 37 insertions(+), 37 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 0a83f9a..8f63b38 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -13,44 +13,44 @@\n *\n * strbufs have some invariants that are very important to keep in mind:\n *\n- * . The `buf` member is never NULL, so it can be used in any usual C\n- * string operations safely. strbuf's _have_ to be initialized either by\n- * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.\n- * +\n- * Do *not* assume anything on what `buf` really is (e.g. if it is\n- * allocated memory or not), use `strbuf_detach()` to unwrap a memory\n- * buffer from its strbuf shell in a safe way. That is the sole supported\n- * way. This will give you a malloced buffer that you can later `free()`.\n- * +\n- * However, it is totally safe to modify anything in the string pointed by\n- * the `buf` member, between the indices `0` and `len-1` (inclusive).\n+ * - The `buf` member is never NULL, so it can be used in any usual C\n+ * string operations safely. strbuf's _have_ to be initialized either by\n+ * `strbuf_init()` or by `= STRBUF_INIT` before the invariants, though.\n *\n- * . The `buf` member is a byte array that has at least `len + 1` bytes\n- * allocated. The extra byte is used to store a `'\\0'`, allowing the\n- * `buf` member to be a valid C-string. Every strbuf function ensure this\n- * invariant is preserved.\n- * +\n- * NOTE: It is OK to \"play\" with the buffer directly if you work it this\n- * way:\n- * +\n- * ----\n- * strbuf_grow(sb, SOME_SIZE); <1>\n- * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);\n- * ----\n- * <1> Here, the memory array starting at `sb->buf`, and of length\n- * `strbuf_avail(sb)` is all yours, and you can be sure that\n- * `strbuf_avail(sb)` is at least `SOME_SIZE`.\n- * +\n- * NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.\n- * +\n- * Doing so is safe, though if it has to be done in many places, adding the\n- * missing API to the strbuf module is the way to go.\n- * +\n- * WARNING: Do _not_ assume that the area that is yours is of size `alloc\n- * - 1` even if it's true in the current implementation. Alloc is somehow a\n- * \"private\" member that should not be messed with. Use `strbuf_avail()`\n- * instead.\n- */\n+ * Do *not* assume anything on what `buf` really is (e.g. if it is\n+ * allocated memory or not), use `strbuf_detach()` to unwrap a memory\n+ * buffer from its strbuf shell in a safe way. That is the sole supported\n+ * way. This will give you a malloced buffer that you can later `free()`.\n+ *\n+ * However, it is totally safe to modify anything in the string pointed by\n+ * the `buf` member, between the indices `0` and `len-1` (inclusive).\n+ *\n+ * - The `buf` member is a byte array that has at least `len + 1` bytes\n+ * allocated. The extra byte is used to store a `'\\0'`, allowing the\n+ * `buf` member to be a valid C-string. Every strbuf function ensure this\n+ * invariant is preserved.\n+ *\n+ * NOTE: It is OK to \"play\" with the buffer directly if you work it this\n+ * way:\n+ *\n+ * ----\n+ * strbuf_grow(sb, SOME_SIZE); <1>\n+ * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);\n+ * ----\n+ * <1> Here, the memory array starting at `sb->buf`, and of length\n+ * `strbuf_avail(sb)` is all yours, and you can be sure that\n+ * `strbuf_avail(sb)` is at least `SOME_SIZE`.\n+ *\n+ * NOTE: `SOME_OTHER_SIZE` must be smaller or equal to `strbuf_avail(sb)`.\n+ *\n+ * Doing so is safe, though if it has to be done in many places, adding the\n+ * missing API to the strbuf module is the way to go.\n+ *\n+ * WARNING: Do _not_ assume that the area that is yours is of size `alloc\n+ * - 1` even if it's true in the current implementation. Alloc is somehow a\n+ * \"private\" member that should not be messed with. Use `strbuf_avail()`\n+ * instead.\n+*/\n \n /*\n * Data Structures\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253664","messageId":"20141212213055.GC27451@peff.net","threadId":"38124","inReplyTo":"20141212212726.GA26284@peff.net","subject":"[PATCH 3/4] strbuf.h: format asciidoc code blocks as 4-space indent","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-12T21:30:56Z","receivedAt":"2014-12-12T21:30:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is much easier to read when the whole thing is stuffed\ninside a comment block. And there is precedent for this\nconvention in markdown (and just in general ascii text).\n\nSigned-off-by: Jeff King \n---\nAs a side note, I actually find markdown much more pleasant to read and\nwrite than asciidoc. I think the output is not always as nice, as it\ndoes not have some of the structured elements that let us produce nice\nmanpages, for example. But I wonder if it would be a better choice for\nformatting API documentation.\n\nNot something that is really relevant to this series, but just food for\nthought if we do end up with an extract+format tool.\n\n strbuf.h | 11 ++++-------\n 1 file changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 8f63b38..078b805 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -33,10 +33,9 @@\n * NOTE: It is OK to \"play\" with the buffer directly if you work it this\n * way:\n *\n- * ----\n- * strbuf_grow(sb, SOME_SIZE); <1>\n- * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);\n- * ----\n+ * strbuf_grow(sb, SOME_SIZE); <1>\n+ * strbuf_setlen(sb, sb->len + SOME_OTHER_SIZE);\n+ *\n * <1> Here, the memory array starting at `sb->buf`, and of length\n * `strbuf_avail(sb)` is all yours, and you can be sure that\n * `strbuf_avail(sb)` is at least `SOME_SIZE`.\n@@ -261,9 +260,7 @@ extern void strbuf_add(struct strbuf *, const void *, size_t);\n * NOTE: This function will *always* be implemented as an inline or a macro\n * using strlen, meaning that this is efficient to write things like:\n *\n- * ----\n- * strbuf_addstr(sb, \"immediate string\");\n- * ----\n+ * strbuf_addstr(sb, \"immediate string\");\n *\n */\n static inline void strbuf_addstr(struct strbuf *sb, const char *s)\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253665","messageId":"20141212213235.GD27451@peff.net","threadId":"38124","inReplyTo":"20141212212726.GA26284@peff.net","subject":"[PATCH 4/4] strbuf.h: reorganize api function grouping headers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-12T21:32:36Z","receivedAt":"2014-12-12T21:32:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The original API doc had something like:\n\n Functions\n ---------\n\n * Life cycle\n\n ... some life-cycle functions ...\n\n * Related to the contents of the buffer\n\n ... functions related to contents ....\n\n etc\n\nThis grouping can be hard to read in the comment sources,\ngiven the \"*\" in the comment lines, and the amount of text\nbetween each section.\n\nInstead, let's make a flat list of groupings, and underline\neach as a section header. That makes them standout, and\neliminates the weird half-phrase of \"Related to...\". Like:\n\n Functions related to the contents of the buffer\n -----------------------------------------------\n\nSigned-off-by: Jeff King \n---\n\nIf you look at the original header file, these groupings actually did\nexist (though we did not remotely follow them as functions were added),\nand looked like:\n\n /* ---- content related ---- */\n\nI'd be happy with something like that, too, but went with the style that\nmatched the \"Data Structures\" header.\n\n strbuf.h | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 078b805..8649a0a 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -71,12 +71,8 @@ extern char strbuf_slopbuf[];\n #define STRBUF_INIT { 0, 0, strbuf_slopbuf }\n \n /**\n- * Functions\n- * ---------\n- */\n-\n-/**\n- * * Life Cycle\n+ * Life Cycle Functions\n+ * --------------------\n */\n \n /**\n@@ -120,7 +116,8 @@ static inline void strbuf_swap(struct strbuf *a, struct strbuf *b)\n \n \n /**\n- * * Related to the size of the buffer\n+ * Functions related to the size of the buffer\n+ * -------------------------------------------\n */\n \n /**\n@@ -162,7 +159,8 @@ static inline void strbuf_setlen(struct strbuf *sb, size_t len)\n \n \n /**\n- * * Related to the contents of the buffer\n+ * Functions related to the contents of the buffer\n+ * -----------------------------------------------\n */\n \n /**\n-- \n2.2.0.454.g7eca6b7\n"},{"id":"253666","messageId":"20141212214010.GA27530@peff.net","threadId":"38124","inReplyTo":"20141212212800.GA27451@peff.net","subject":"Re: [PATCH 1/4] strbuf: migrate api-strbuf.txt documentation to strbuf.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-12T21:40:10Z","receivedAt":"2014-12-12T21:40:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 12, 2014 at 04:28:00PM -0500, Jeff King wrote:\n\n> 1. All comments created in this way are started with \"/**\"\n> to indicate they are part of the API documentation. This\n> may help later with extracting the text to pretty-print\n> it.\n\nBy the way, two thoughts I had while working on this.\n\nOne, I wonder if it would look nicer to drop the leading \" * \" from our\ncomments, especially for long, free-form text blocks. Like:\n\n/**\n\nData Structures\n---------------\n\nThis is actual asciidoc that can be rendered. It's not _nearly_ as\nugly to leave the asciidoc intact if you avoid the leading asterisks.\n\n*/\n\nI also wondered if you could spell it as:\n\n#ifdef API_DOCUMENTATION\n... some asciidoc goes here...\n#endif\n\nbut the preprocessor does still look at the contents. I didn't check the\nstandard, but I suspect an implementation doesn't _have_ to, but gcc\nwill warn about things like unmatched single-quotes in the content.\n\nAnd two, the editor support for writing comments like this is not as\ngood. For instance, vim's \"reformat paragraph\" command considers the\nwhole comment to be the paragraph. If you're writing asciidoc inside\nthe comment field, this quickly becomes annoying. This is a minor issue\nthat can be fixed (I am sure there is probably a vim package to do it\nbetter already, and that emacs people will tsk tsk and explain why their\neditor is better), but I thought I would note it as a potential \"con\" of\nthis approach.\n\n-Peff\n"},{"id":"253669","messageId":"xmqqoar8ecov.fsf@gitster.dls.corp.google.com","threadId":"38124","inReplyTo":"20141212212800.GA27451@peff.net","subject":"Re: [PATCH 1/4] strbuf: migrate api-strbuf.txt documentation to strbuf.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-12T22:16:32Z","receivedAt":"2014-12-12T22:16:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> +/**\n> + * * Related to the contents of the buffer\n> + */\n> +\n> +/**\n> + * Strip whitespace from the beginning and end of a string.\n> + * Equivalent to performing `strbuf_rtrim()` followed by `strbuf_ltrim()`.\n> + */\n> extern void strbuf_trim(struct strbuf *);\n> +\n> +/**\n> + * Strip whitespace from the end of a string.\n> + */\n> extern void strbuf_rtrim(struct strbuf *);\n> +\n> +/**\n> + * Strip whitespace from the beginning of a string.\n> + */\n> extern void strbuf_ltrim(struct strbuf *);\n\nThis part reminds me why I often find the current \"out of header\"\ndocumentation when writing code _without_ the need for _learning_\nthe API. The corresponding part in the original is merely these:\n\n /*----- content related -----*/\n extern void strbuf_trim(struct strbuf *);\n extern void strbuf_rtrim(struct strbuf *);\n extern void strbuf_ltrim(struct strbuf *);\n extern int strbuf_reencode(struct strbuf *sb, const char *from, const char *to);\n ...\n\nIt is a lot quicker, scanning this with eyeball, to notice that\nthere are three related \"trim\" functions and how they are spelled.\nIf I wanted to \"trim from both ends\", knew there were some ways to\n\"trim\" but didn't remember if we had r/ltrim without trim, and/or\nwas not sure if the \"both\" version was was _trim or _btrim, scanning\na bland list tends to be much more pleasant.\n\nWhen you are _learning_ the API, the format that spreads each\nindividual function into its own section with explanation would be\neasier to read through.\n\nNot a strong objection to the overall direction, but I think that we\nneed to be aware that we are making things harder for some use\ncases.\n"},{"id":"253670","messageId":"xmqqk31weckr.fsf@gitster.dls.corp.google.com","threadId":"38124","inReplyTo":"20141212212820.GB27451@peff.net","subject":"Re: [PATCH 2/4] strbuf.h: drop asciidoc list formatting from API docs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-12T22:19:00Z","receivedAt":"2014-12-12T22:19:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Using a hanging indent is much more readable. This means we\n> won't format as asciidoc anymore, but since we don't have a\n> working system for extracting these comments anyway, it's\n> probably more important to just make the source readable.\n\nYeah.\n\nThose who want to strip the leading \" * \" to turn it into AsciiDoc\nwould need to also parse out the hanging indent and format into\nAsciiDoc, which is not entirely an unreasonable thing to ask.\n"},{"id":"253671","messageId":"20141212223012.GC29365@google.com","threadId":"38124","inReplyTo":"20141212212800.GA27451@peff.net","subject":"Re: [PATCH 1/4] strbuf: migrate api-strbuf.txt documentation to strbuf.h","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-12T22:30:12Z","receivedAt":"2014-12-12T22:30:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Signed-off-by: Jeff King \n> ---\n> Documentation/technical/api-strbuf.txt | 351 -------------------------\n> strbuf.h | 458 ++++++++++++++++++++++++++++-----\n> 2 files changed, 390 insertions(+), 419 deletions(-)\n> delete mode 100644 Documentation/technical/api-strbuf.txt\n\nThanks. This looks very sensible. Some suggested tweaks below (see patch at\nbottom).\n\n> +++ b/strbuf.h\n[...]\n> +/**\n> + * * Life Cycle\n[...]\n> +/**\n> + * Adding data to the buffer\n\nThese were headings like\n\n\t* Life Cycle\n\t* Adding data to the buffer\n\nMost ended up as ' * *' after the conversion, keeping the extra '*', but\n'Adding data to the buffer' didn't.\n\nAfter patch 4/4 they look the same again, so it doesn't matter.\n\n[...]\n> * NOTE: This function will *always* be implemented as an inline or a macro\n> - * that expands to:\n> - * +\n> - * ----\n> - * strbuf_add(..., s, strlen(s));\n> - * ----\n> - * +\n> - * Meaning that this is efficient to write things like:\n> + * using strlen, meaning that this is efficient to write things like:\n> *\n> * ----\n> * strbuf_addstr(sb, \"immediate string\");\n> * ----\n\nMakes sense.\n\n[...]\n> /**\n> * Read the contents of a given file descriptor. The third argument can be\n> - * used to give a hint about the file size, to avoid reallocs.\n> - */\n> + * used to give a hint about the file size, to avoid reallocs. If read fails,\n> + * any partial read is undone */\n\nMissing period at end of sentence. \"*/\" should go on the next line.\n\n[...]\n> +extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n> +\n> +/**\n> + * Set the buffer to the path of the current working directory.\n> + */\n> +extern int strbuf_getcwd(struct strbuf *sb);\n\nThis snuck up a few lines --- it was between getwholeline_fd and\nadd_absolute_path before. I think it makes sense for getcwd to be\nnext to add_absolute_path.\n\n[...]\n> +extern void stripspace(struct strbuf *buf, int skip_comments);\n> +\n> +/**\n> + * Launch the user preferred editor to edit a file and fill the buffer\n> + * with the file's contents upon the user completing their editing. The\n> + * third argument can be used to set the environment which the editor is\n> + * run in. If the buffer is NULL the editor is launched as usual but the\n> + * file's contents are not read into the buffer upon completion.\n> + */\n> +extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);\n\nsplit_* were before this before. Probably launch_editor belongs with\nread_file and co instead, but during this conversion it makes sense to\njust keep the order from before.\n\n[...]\n> - * `strbuf_split_buf`::\n> - * `strbuf_split_str`::\n> - * `strbuf_split_max`::\n> - * `strbuf_split`::\n> - * \n> - * Split a string or strbuf into a list of strbufs at a specified\n> - * terminator character. The returned substrings include the\n> - * terminator characters. Some of these functions take a `max`\n> - * parameter, which, if positive, limits the output to that\n> - * number of substrings.\n> - * \n> - * `strbuf_list_free`::\n> - * \n> - * Free a list of strbufs (for example, the return values of the\n> - * `strbuf_split()` functions).\n> +/*\n> + * Split str (of length slen) at the specified terminator character.\n> + * Return a null-terminated array of pointers to strbuf objects\n> + * holding the substrings. The substrings include the terminator,\n> + * except for the last substring, which might be unterminated if the\n[...]\n> + */\n> +extern struct strbuf **strbuf_split_buf(const char *, size_t,\n> + int terminator, int max);\n> +\n> +/*\n> + * Split a NUL-terminated string at the specified terminator\n> + * character. See strbuf_split_buf() for more information.\n> + */\n> +static inline struct strbuf **strbuf_split_str(const char *str,\n> + int terminator, int max)\n> +{\n> + return strbuf_split_buf(str, strlen(str), terminator, max);\n> +}\n> +\n> +/*\n> + * Split a strbuf at the specified terminator character. See\n> + * strbuf_split_buf() for more information.\n> + */\n> +static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,\n\nThis is something that the flat file format did better. Would it work to\ndo something similar, for example by dropping the boilerplate comment\nbefore the inline functions (not shown in the patch below)?\n\n\t/**\n\t * Split a string or strbuf into a list of strbufs at a specified\n\t * terminator character. [...]\n\t */\n\textern struct strbuf **strbuf_split_buf(const char *, size_t, int terminator, int max);\n\n\tstatic inline struct strbuf **strbuf_split_str(const char *str, int terminator, int max)\n\t{\n\t\treturn strbuf_split_buf(str, strlen(str), terminator, max);\n\t}\n\n\tstatic inline struct strbuf **strbuf_split_max(const struct strbuf *sb, int terminator, int max)\n\t{\n\t\treturn strbuf_split_buf(sb->buf, sb->len, terminator, max);\n\t}\n\n\tstatic inline struct strbuf **strbuf_split(const struct strbuf *sb, int terminator)\n\t{\n\t\treturn strbuf_split_max(sb, terminator, 0);\n\t}\n\n\t/**\n\t * Free a list of strbufs (for example, the return values of the\n\t * `strbuf_split()` functions).\n\t */\n\textern void strbuf_list_free(struct strbuf **);\n\n strbuf.h | 33 +++++++++++++++++----------------\n 1 file changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 0a83f9a..108644e 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -202,7 +202,7 @@ extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);\n \n \n /**\n- * Adding data to the buffer\n+ * * Adding data to the buffer\n *\n * NOTE: All of the functions in this section will grow the buffer as\n * necessary. If they fail for some reason other than memory shortage and the\n@@ -368,7 +368,8 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n /**\n * Read the contents of a given file descriptor. The third argument can be\n * used to give a hint about the file size, to avoid reallocs. If read fails,\n- * any partial read is undone */\n+ * any partial read is undone.\n+ */\n extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n \n /**\n@@ -384,11 +385,6 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n \n /**\n- * Set the buffer to the path of the current working directory.\n- */\n-extern int strbuf_getcwd(struct strbuf *sb);\n-\n-/**\n * Read a line from a FILE *, overwriting the existing contents\n * of the strbuf. The second argument specifies the line\n * terminator character, typically `'\\n'`.\n@@ -413,6 +409,11 @@ extern int strbuf_getwholeline(struct strbuf *, FILE *, int);\n extern int strbuf_getwholeline_fd(struct strbuf *, int, int);\n \n /**\n+ * Set the buffer to the path of the current working directory.\n+ */\n+extern int strbuf_getcwd(struct strbuf *sb);\n+\n+/**\n * Add a path to a buffer, converting a relative path to an\n * absolute one in the process. Symbolic links are not\n * resolved.\n@@ -425,15 +426,6 @@ extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n */\n extern void stripspace(struct strbuf *buf, int skip_comments);\n \n-/**\n- * Launch the user preferred editor to edit a file and fill the buffer\n- * with the file's contents upon the user completing their editing. The\n- * third argument can be used to set the environment which the editor is\n- * run in. If the buffer is NULL the editor is launched as usual but the\n- * file's contents are not read into the buffer upon completion.\n- */\n-extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);\n-\n static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n {\n \tif (strip_suffix_mem(sb->buf, &sb->len, suffix)) {\n@@ -495,6 +487,15 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n */\n extern void strbuf_list_free(struct strbuf **);\n \n+/**\n+ * Launch the user preferred editor to edit a file and fill the buffer\n+ * with the file's contents upon the user completing their editing. The\n+ * third argument can be used to set the environment which the editor is\n+ * run in. If the buffer is NULL the editor is launched as usual but the\n+ * file's contents are not read into the buffer upon completion.\n+ */\n+extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);\n+\n extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);\n \n /*\n"},{"id":"253672","messageId":"20141212223241.GA19972@google.com","threadId":"38124","inReplyTo":"20141212212726.GA26284@peff.net","subject":"Re: [PATCH] document string_list_clear","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-12T22:32:41Z","receivedAt":"2014-12-12T22:32:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 12, 2014 at 04:27:26PM -0500, Jeff King wrote:\n> On Fri, Dec 12, 2014 at 11:35:52AM -0800, Jonathan Nieder wrote:\n> \n> > If I were doing it, I would first de-asciidoc within technical/ and\n> > then move into the header in a separate patch. Or first move with\n> > asciidoc intact and then de-asciidoc in a separate patch. Combining\n> > the two into a single patch is also fine. Please don't change wording\n> > at the same time.\n> \n> Here's what I came up with. The first patch probably does more than\n> you'd like (and more than I would have done if I were starting from\n> scratch today). But I didn't want to get into undoing the stripping of\n> each function-name list item that I showed earlier, as it would be a lot\n> of tedious manual work. If we decide we want to keep those, I'm happy to\n> do the work to restore them, but it didn't seem like a good tradeoff\n> just to create an intermediate state to make the patch prettier.\n> \n> I did split out some of the other formatting decisions, though, so they\n> can be evaluated individually.\n> \n> [1/4]: strbuf: migrate api-strbuf.txt documentation to strbuf.h\n\n(optional nit):\nThe subject of this patch is slightly different than the others.\nHow about: \nstrbuf.h: integrate api-strbuf.txt documentation\n\n> [2/4]: strbuf.h: drop asciidoc list formatting from API docs\n> [3/4]: strbuf.h: format asciidoc code blocks as 4-space indent\n> [4/4]: strbuf.h: reorganize api function grouping headers\n> \n> -Peff\n\nI have reviewed the series and I personally like it as I am more\noften in the learners/discovery stage than the fast lookup stage\nas Junio mentioned.\n\nAnother inconsistency I found specifically related to the strbuf.h\ndocumentation is the apparent changeing style.\n\nAt the beginning we have \n\textern void strbuf_attach(struct strbuf *, void *, size_t, size_t);\n\nEven with the documentation attached, which size_t is the length and what the \namount of allocated memory? Later we have more explaining names, such as\n\nextern struct strbuf **strbuf_split_buf(const char *, size_t,\n\t\t\t\t\tint terminator, int max);\n\n\nPlease append the following patch to your series. I noticed this as\nthe text editor I use uses different colors for \n\n\t/** comments starting with double stars used with i.e. doxygen */\n\t/* random comments in the file */\n\nThanks,\nStefan\n\n\n-----------8<------------------\nFrom 5d0f98aba65e8b1415ebfcd8e06b67203e9305a7 Mon Sep 17 00:00:00 2001\nFrom: Stefan Beller \nDate: Fri, 12 Dec 2014 14:27:15 -0800\nSubject: [PATCH] strbuf.h: Unify documentation comments beginnings\n\nWe want to preserve the meaning of \"/**\" to start a documentary\nin depth comment, so all of the documenting comments should start with\na \"/**\".\n\nSigned-off-by: Stefan Beller \n---\n strbuf.h | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/strbuf.h b/strbuf.h\nindex 8649a0a..95d49e1 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -438,7 +438,7 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n \t\treturn 0;\n }\n \n-/*\n+/**\n * Split str (of length slen) at the specified terminator character.\n * Return a null-terminated array of pointers to strbuf objects\n * holding the substrings. The substrings include the terminator,\n@@ -454,7 +454,7 @@ static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)\n extern struct strbuf **strbuf_split_buf(const char *, size_t,\n \t\t\t\t\tint terminator, int max);\n \n-/*\n+/**\n * Split a NUL-terminated string at the specified terminator\n * character. See strbuf_split_buf() for more information.\n */\n@@ -464,7 +464,7 @@ static inline struct strbuf **strbuf_split_str(const char *str,\n \treturn strbuf_split_buf(str, strlen(str), terminator, max);\n }\n \n-/*\n+/**\n * Split a strbuf at the specified terminator character. See\n * strbuf_split_buf() for more information.\n */\n@@ -474,7 +474,7 @@ static inline struct strbuf **strbuf_split_max(const struct strbuf *sb,\n \treturn strbuf_split_buf(sb->buf, sb->len, terminator, max);\n }\n \n-/*\n+/**\n * Split a strbuf at the specified terminator character. See\n * strbuf_split_buf() for more information.\n */\n@@ -484,7 +484,7 @@ static inline struct strbuf **strbuf_split(const struct strbuf *sb,\n \treturn strbuf_split_max(sb, terminator, 0);\n }\n \n-/*\n+/**\n * Free a NULL-terminated list of strbufs (for example, the return\n * values of the strbuf_split*() functions).\n */\n@@ -492,7 +492,7 @@ extern void strbuf_list_free(struct strbuf **);\n \n extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);\n \n-/*\n+/**\n * Append s to sb, with the characters '<', '>', '&' and '\"' converted\n * into XML entities.\n */\n-- \n2.2.0.38.g71d6cb7\n"},{"id":"253673","messageId":"20141212223701.GD29365@google.com","threadId":"38124","inReplyTo":"20141212212820.GB27451@peff.net","subject":"Re: [PATCH 2/4] strbuf.h: drop asciidoc list formatting from API docs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-12T22:37:01Z","receivedAt":"2014-12-12T22:37:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Using a hanging indent is much more readable. This means we\n> won't format as asciidoc anymore, but since we don't have a\n> working system for extracting these comments anyway, it's\n> probably more important to just make the source readable.\n> \n> Signed-off-by: Jeff King \n> ---\n> strbuf.h | 74 ++++++++++++++++++++++++++++++++--------------------------------\n> 1 file changed, 37 insertions(+), 37 deletions(-)\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"253674","messageId":"20141212223903.GE29365@google.com","threadId":"38124","inReplyTo":"20141212213055.GC27451@peff.net","subject":"Re: [PATCH 3/4] strbuf.h: format asciidoc code blocks as 4-space indent","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-12T22:39:03Z","receivedAt":"2014-12-12T22:39:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> This is much easier to read when the whole thing is stuffed\n> inside a comment block. And there is precedent for this\n> convention in markdown (and just in general ascii text).\n>\n> Signed-off-by: Jeff King \n> ---\n\nReviewed-by: Jonathan Nieder \n\n> As a side note, I actually find markdown much more pleasant to read and\n> write than asciidoc.\n\nI do, too. Quoting in asciidoc is a nightmare.\n\nThanks,\nJonathan\n"},{"id":"253675","messageId":"20141212224608.GF29365@google.com","threadId":"38124","inReplyTo":"20141212213235.GD27451@peff.net","subject":"Re: [PATCH 4/4] strbuf.h: reorganize api function grouping headers","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-12T22:46:08Z","receivedAt":"2014-12-12T22:46:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> If you look at the original header file, these groupings actually did\n> exist (though we did not remotely follow them as functions were added),\n> and looked like:\n>\n> /* ---- content related ---- */\n>\n> I'd be happy with something like that, too, but went with the style that\n> matched the \"Data Structures\" header.\n\nI'd be happy either way too.\n\n> strbuf.h | 14 ++++++--------\n> 1 file changed, 6 insertions(+), 8 deletions(-)\n\nThis is missing the \"Adding data to the buffer\" section.\n\nExcept for that,\nReviewed-by: Jonathan Nieder \n"},{"id":"253684","messageId":"548DCC02.60100@alum.mit.edu","threadId":"38124","inReplyTo":"20141212223903.GE29365@google.com","subject":"Re: [PATCH 3/4] strbuf.h: format asciidoc code blocks as 4-space indent","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-14T17:42:26Z","receivedAt":"2014-12-14T17:42:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/12/2014 11:39 PM, Jonathan Nieder wrote:\n> Jeff King wrote:\n> \n>> This is much easier to read when the whole thing is stuffed\n>> inside a comment block. And there is precedent for this\n>> convention in markdown (and just in general ascii text).\n>>\n>> Signed-off-by: Jeff King \n>> ---\n> \n> Reviewed-by: Jonathan Nieder \n> \n>> As a side note, I actually find markdown much more pleasant to read and\n>> write than asciidoc.\n> \n> I do, too. Quoting in asciidoc is a nightmare.\n\nPeff, thanks for working on this. I think it is a definite improvement.\n\nI suggest that we accept the use of asciidoc/markdown's convention of\nusing backwards quotes to mark code snippets (especially identifier\nnames) within comments *anywhere* in our code base. For example, this\nappears in refs.c:\n\n /*\n * Create a struct ref_entry object for the specified dirname.\n * dirname is the name of the directory with a trailing slash\n * (e.g., \"refs/heads/\") or \"\" for the top-level directory.\n */\n\nI claim that it is more readable with a tiny bit of markup:\n\n /*\n * Create a `struct ref_entry` object for the specified `dirname`.\n * `dirname` is the name of the directory with a trailing slash\n * (e.g., \"refs/heads/\") or \"\" for the top-level directory.\n */\n\nMarking up `struct ref_entry` helps make it clear that the two words\nbelong together, and marking up `dirname` makes it clear that we are\ntalking about a specific identifier (in this case, a function parameter).\n\nCurrently, comments use a mix of unadorned text, single-quoted text, and\ndouble-quoted text when talking about code. I think the\nasciidoc/markdown convention is clearer [1].\n\nI think we shouldn't be pedantic about this. When a comment is readable\nwith no markup, there's no need to add markup. And \"incorrect\" markup\nshouldn't by itself be reason to reject a patch. But in many examples, a\nlittle bit of markup makes the text less ambiguous and easier to read.\n\nMichael\n\n[1] Yes, I see the irony in trying to improve a mixture of three\n conventions by adding a fourth one.\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}