{"thread":{"id":"61834","subject":"[PATCH 0/3] Documentation: some coding guideline updates","startedAt":"2024-07-24T11:05:15Z","lastAt":"2024-07-31T09:12:42Z","messageCount":29,"participants":["Patrick Steinhardt","Karthik Nayak","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"499246","messageId":"cover.1721818488.git.ps@pks.im","threadId":"61834","inReplyTo":null,"subject":"[PATCH 0/3] Documentation: some coding guideline updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T11:05:08Z","receivedAt":"2024-07-24T11:05:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nI've had it in my mind for a long time to update our coding guideline\nfor some things that I frequently stumble over during code reviews. This\nsmall patch series fills in those gaps.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (3):\n Documentation: clarify indentation style for C preprocessor directives\n Documentation: document naming schema for struct-related functions\n Documentation: document difference between release and free\n\n Documentation/CodingGuidelines | 41 ++++++++++++++++++++++++++++++++++\n 1 file changed, 41 insertions(+)\n\n-- \n2.46.0.rc1.dirty\n\n"},{"id":"499247","messageId":"64e0b449936a6828ead98438d621f7e7684546c8.1721818488.git.ps@pks.im","threadId":"61834","inReplyTo":"cover.1721818488.git.ps@pks.im","subject":"[PATCH 1/3] Documentation: clarify indentation style for C preprocessor directives","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T11:05:12Z","receivedAt":"2024-07-24T11:05:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There has recently been some discussion around how C preprocessor\ndirectives shall be indented [1]. This discussion was settled towards\nindenting after the hash by two spaces [2]. Document it as such.\n\n[1]: https://lore.kernel.org/all/xmqqwmmm1bw6.fsf@gitster.g/\n[2]: https://lore.kernel.org/all/20240708092317.267915-2-karthik.188@gmail.com/\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/CodingGuidelines | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 1d92b2da03..1d09384f28 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -214,6 +214,16 @@ For C programs:\n - We use tabs to indent, and interpret tabs as taking up to\n 8 spaces.\n \n+ - Nested C preprocessor directives are indented after the hash by two\n+ spaces per nesting level.\n+\n+\t#if FOO\n+\t# include \n+\t# if BAR\n+\t# include \n+\t# endif\n+\t#endif\n+\n - We try to keep to at most 80 characters per line.\n \n - As a Git developer we assume you have a reasonably modern compiler\n-- \n2.46.0.rc1.dirty\n\n"},{"id":"499248","messageId":"7f07bf1f3beee2f74a3572d2b9a8d28b6535053e.1721818488.git.ps@pks.im","threadId":"61834","inReplyTo":"cover.1721818488.git.ps@pks.im","subject":"[PATCH 2/3] Documentation: document naming schema for struct-related functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T11:05:17Z","receivedAt":"2024-07-24T11:05:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We nowadays have a proper mishmash of struct-related functions that are\ncalled `_` (e.g. `clear_prio_queue()`) versus functions\nthat are called `_` (e.g. `strbuf_clear()`). While the\nformer style may be easier to tie into a spoken conversation, most of\nour communication happens in text anyway. Furthermore, prefixing\nfunctions with the name of the structure they operate on makes it way\neasier to group them together, see which functions are related, and will\nalso help folks who are using code completion.\n\nLet's thus settle on one style, namely the one where functions start\nwith the name of the structure they operate on.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/CodingGuidelines | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 1d09384f28..34fcbcb5a4 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -541,6 +541,25 @@ For C programs:\n use your own debugger and arguments. Example: `GIT_DEBUGGER=\"ddd --gdb\"\n ./bin-wrappers/git log` (See `wrap-for-bin.sh`.)\n \n+ - Functions that operate on a specific structure and which are used by\n+ other subsystems shall be named after the structure. The function\n+ name should start with the name of the structure followed by a verb.\n+ E.g.\n+\n+\tstruct strbuf;\n+\n+\tvoid strbuf_add(struct strbuf *buf, ...);\n+\n+\tvoid strbuf_reset(struct strbuf *buf);\n+\n+ is preferred over:\n+\n+\tstruct strbuf;\n+\n+\tvoid add_string(struct strbuf *buf, ...);\n+\n+\tvoid reset_strbuf(struct strbuf *buf);\n+\n For Perl programs:\n \n - Most of the C guidelines above apply.\n-- \n2.46.0.rc1.dirty\n\n"},{"id":"499249","messageId":"5e1de3c3159968e897a83c05dae5e8504d37a16c.1721818488.git.ps@pks.im","threadId":"61834","inReplyTo":"cover.1721818488.git.ps@pks.im","subject":"[PATCH 3/3] Documentation: document difference between release and free","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T11:05:22Z","receivedAt":"2024-07-24T11:05:26Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We semi-regularly have discussions around whether a function shall be\nnamed `release()` or `free()`. For most of the part we use these two\nterminologies quite consistently though:\n\n - `release()` only frees internal state of a structure, whereas the\n structure itself is not free'd.\n\n - `free()` frees both internal state and the structure itself.\n\nCarve out a space where we can add idiomatic names for common functions\nin our coding guidelines. This space can get extended in the future when\nwe feel the need to document more idiomatic names.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/CodingGuidelines | 12 ++++++++++++\n 1 file changed, 12 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 34fcbcb5a4..ace4c4ad0c 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -560,6 +560,18 @@ For C programs:\n \n \tvoid reset_strbuf(struct strbuf *buf);\n \n+ - There are several common idiomatic names for functions performing\n+ specific tasks on structures:\n+\n+ - `_init()` initializes a structure without allocating the\n+ structure itself.\n+\n+ - `_release()` releases a structure's contents without\n+ freeing the structure.\n+\n+ - `_free()` releases a structure's contents and frees the\n+ structure.\n+\n For Perl programs:\n \n - Most of the C guidelines above apply.\n-- \n2.46.0.rc1.dirty\n\n"},{"id":"499252","messageId":"CAOLa=ZQn1uYSd0089+jOguVBSnKfZjak4766V3B8mXn2hBgAkg@mail.gmail.com","threadId":"61834","inReplyTo":"7f07bf1f3beee2f74a3572d2b9a8d28b6535053e.1721818488.git.ps@pks.im","subject":"Re: [PATCH 2/3] Documentation: document naming schema for struct-related functions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-24T11:42:29Z","receivedAt":"2024-07-24T11:42:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> We nowadays have a proper mishmash of struct-related functions that are\n> called `_` (e.g. `clear_prio_queue()`) versus functions\n> that are called `_` (e.g. `strbuf_clear()`). While the\n> former style may be easier to tie into a spoken conversation, most of\n> our communication happens in text anyway. Furthermore, prefixing\n> functions with the name of the structure they operate on makes it way\n> easier to group them together, see which functions are related, and will\n> also help folks who are using code completion.\n>\n> Let's thus settle on one style, namely the one where functions start\n> with the name of the structure they operate on.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> Documentation/CodingGuidelines | 19 +++++++++++++++++++\n> 1 file changed, 19 insertions(+)\n>\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 1d09384f28..34fcbcb5a4 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -541,6 +541,25 @@ For C programs:\n> use your own debugger and arguments. Example: `GIT_DEBUGGER=\"ddd --gdb\"\n> ./bin-wrappers/git log` (See `wrap-for-bin.sh`.)\n>\n> + - Functions that operate on a specific structure and which are used by\n> + other subsystems shall be named after the structure. The function\n> + name should start with the name of the structure followed by a verb.\n> + E.g.\n> +\n\nNit: It would be nice to add `_` here, since we do something\nsimilar in the next patch.\n\n> +\tstruct strbuf;\n> +\n> +\tvoid strbuf_add(struct strbuf *buf, ...);\n> +\n> +\tvoid strbuf_reset(struct strbuf *buf);\n> +\n> + is preferred over:\n> +\n> +\tstruct strbuf;\n> +\n> +\tvoid add_string(struct strbuf *buf, ...);\n> +\n\nI first thought this was a typo and should've been `add_strbuf` instead,\nbut I'm sure your intention was to show _other forms_ instead of\n`_` here.\n\nMaybe we could do s/is preferred over/is preferred over other forms,\nlike/. I dunno. It is probably fine as is :)\n\n> +\tvoid reset_strbuf(struct strbuf *buf);\n> +\n> For Perl programs:\n>\n> - Most of the C guidelines above apply.\n> --\n> 2.46.0.rc1.dirty\n"},{"id":"499254","messageId":"CAOLa=ZScBn+sMB7BWMpsS=Ld0sUW14Li5JVuKPGKtY91hVo4jA@mail.gmail.com","threadId":"61834","inReplyTo":"5e1de3c3159968e897a83c05dae5e8504d37a16c.1721818488.git.ps@pks.im","subject":"Re: [PATCH 3/3] Documentation: document difference between release and free","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-24T11:46:20Z","receivedAt":"2024-07-24T11:46:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> We semi-regularly have discussions around whether a function shall be\n> named `release()` or `free()`. For most of the part we use these two\n> terminologies quite consistently though:\n>\n\nI noticed there is also `clear()` used in some places. Should we also\nmention that we don't recommend using `clear()` WRT freeing memory?\n\n> - `release()` only frees internal state of a structure, whereas the\n> structure itself is not free'd.\n>\n> - `free()` frees both internal state and the structure itself.\n>\n> Carve out a space where we can add idiomatic names for common functions\n> in our coding guidelines. This space can get extended in the future when\n> we feel the need to document more idiomatic names.\n>\n> Signed-off-by: Patrick Steinhardt \n> ---\n> Documentation/CodingGuidelines | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 34fcbcb5a4..ace4c4ad0c 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -560,6 +560,18 @@ For C programs:\n>\n> \tvoid reset_strbuf(struct strbuf *buf);\n>\n> + - There are several common idiomatic names for functions performing\n> + specific tasks on structures:\n> +\n> + - `_init()` initializes a structure without allocating the\n> + structure itself.\n> +\n> + - `_release()` releases a structure's contents without\n> + freeing the structure.\n> +\n> + - `_free()` releases a structure's contents and frees the\n> + structure.\n> +\n> For Perl programs:\n>\n> - Most of the C guidelines above apply.\n> --\n> 2.46.0.rc1.dirty\n\nThe patch itself looks good.\n"},{"id":"499255","messageId":"CAOLa=ZS_Lfqa+Z5JQYuJ-maMgW0=DWAouZC3QfFcEEbX4kq_JA@mail.gmail.com","threadId":"61834","inReplyTo":"cover.1721818488.git.ps@pks.im","subject":"Re: [PATCH 0/3] Documentation: some coding guideline updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-24T11:47:08Z","receivedAt":"2024-07-24T11:47:09Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> I've had it in my mind for a long time to update our coding guideline\n> for some things that I frequently stumble over during code reviews. This\n> small patch series fills in those gaps.\n>\n\nThanks for the initiative. I've left some small comments, but overall\nthis already looks good to me.\n\n> Thanks!\n>\n> Patrick\n>\n> Patrick Steinhardt (3):\n> Documentation: clarify indentation style for C preprocessor directives\n> Documentation: document naming schema for struct-related functions\n> Documentation: document difference between release and free\n>\n> Documentation/CodingGuidelines | 41 ++++++++++++++++++++++++++++++++++\n> 1 file changed, 41 insertions(+)\n>\n> --\n> 2.46.0.rc1.dirty\n\nKarthik\n"},{"id":"499256","messageId":"ZqD9keTtimiqJnJP@tanuki","threadId":"61834","inReplyTo":"CAOLa=ZScBn+sMB7BWMpsS=Ld0sUW14Li5JVuKPGKtY91hVo4jA@mail.gmail.com","subject":"Re: [PATCH 3/3] Documentation: document difference between release and free","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T13:11:45Z","receivedAt":"2024-07-24T13:11:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 24, 2024 at 04:46:20AM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > We semi-regularly have discussions around whether a function shall be\n> > named `release()` or `free()`. For most of the part we use these two\n> > terminologies quite consistently though:\n> >\n> \n> I noticed there is also `clear()` used in some places. Should we also\n> mention that we don't recommend using `clear()` WRT freeing memory?\n\nIn any case I think we should decide on eithe using `clear()` or using\n`release()` for consistency's sake. Which of both we use I don't quite\ncare, but the following very shoddy analysis clearly favors `release()`:\n\n $ git grep '_clear(' | wc -l\n 844\n $ git grep '_release(' | wc -l\n 2126\n\nSo yeah, I'm happy to explicitly mention that `clear()` shouldn't be\nused in favor of `release()`.\n\nPatrick\n"},{"id":"499257","messageId":"ZqD9xMeFiaTUKPQw@tanuki","threadId":"61834","inReplyTo":"CAOLa=ZQn1uYSd0089+jOguVBSnKfZjak4766V3B8mXn2hBgAkg@mail.gmail.com","subject":"Re: [PATCH 2/3] Documentation: document naming schema for struct-related functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-24T13:12:36Z","receivedAt":"2024-07-24T13:12:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 24, 2024 at 04:42:29AM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> > diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> > index 1d09384f28..34fcbcb5a4 100644\n> > --- a/Documentation/CodingGuidelines\n> > +++ b/Documentation/CodingGuidelines\n> > @@ -541,6 +541,25 @@ For C programs:\n> > use your own debugger and arguments. Example: `GIT_DEBUGGER=\"ddd --gdb\"\n> > ./bin-wrappers/git log` (See `wrap-for-bin.sh`.)\n> >\n> > + - Functions that operate on a specific structure and which are used by\n> > + other subsystems shall be named after the structure. The function\n> > + name should start with the name of the structure followed by a verb.\n> > + E.g.\n> > +\n> \n> Nit: It would be nice to add `_` here, since we do something\n> similar in the next patch.\n\nMakes sense, will do.\n\nPatrick\n"},{"id":"499259","messageId":"1841a256-5c01-4892-99c7-ad7df14e6e0e@gmail.com","threadId":"61834","inReplyTo":"ZqD9keTtimiqJnJP@tanuki","subject":"Re: [PATCH 3/3] Documentation: document difference between release and free","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-24T14:30:07Z","receivedAt":"2024-07-24T14:30:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Patrick\n\nOn 24/07/2024 14:11, Patrick Steinhardt wrote:\n> On Wed, Jul 24, 2024 at 04:46:20AM -0700, Karthik Nayak wrote:\n>> Patrick Steinhardt writes:\n>>\n>>> We semi-regularly have discussions around whether a function shall be\n>>> named `release()` or `free()`. For most of the part we use these two\n>>> terminologies quite consistently though:\n>>>\n>>\n>> I noticed there is also `clear()` used in some places. Should we also\n>> mention that we don't recommend using `clear()` WRT freeing memory?\n> \n> In any case I think we should decide on eithe using `clear()` or using\n> `release()` for consistency's sake. Which of both we use I don't quite\n> care, but the following very shoddy analysis clearly favors `release()`:\n> \n> $ git grep '_clear(' | wc -l\n> 844\n> $ git grep '_release(' | wc -l\n> 2126\n\nI think a fairer comparison would be to look at function declarations, \nnot all the call sites.\n\n$ { git grep 'void [a-z_]*_release(' '*.h'\n git grep 'static void [a-z_]*_release(' '*.c'\n } | wc -l\n47\n$ { git grep 'void [a-z_]*_clear(' '*.h'\n git grep 'static void [a-z_]*_clear(' '*.c'\n } | wc -l\n58\n\nSo we have more _clear() functions than _release() functions. I think \nthere may sometimes be a semantic difference between _clear() and \n_release() as well where some _clear() functions zero out the struct \nafter freeing the members.\n\nThanks for working on this it will be a useful addition to our coding \nguidelines\n\nBest Wishes\n\nPhillip\n\n> So yeah, I'm happy to explicitly mention that `clear()` shouldn't be\n> used in favor of `release()`.\n> \n> Patrick\n"},{"id":"499282","messageId":"xmqqv80uwxn2.fsf@gitster.g","threadId":"61834","inReplyTo":"64e0b449936a6828ead98438d621f7e7684546c8.1721818488.git.ps@pks.im","subject":"Re: [PATCH 1/3] Documentation: clarify indentation style for C preprocessor directives","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-24T16:41:21Z","receivedAt":"2024-07-24T16:41:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> There has recently been some discussion around how C preprocessor\n> directives shall be indented [1]. This discussion was settled towards\n> indenting after the hash by two spaces [2]. Document it as such.\n\nIt was settled to have space after and not before the hash, but I do\nnot recall ever agreeing to two spaces. I prefer to increment by 1\nfor each level instead to keep the whole thing less distracting\nwhile carrying meaningful information.\n\nThanks.\n"},{"id":"499283","messageId":"xmqqikwuwx7j.fsf@gitster.g","threadId":"61834","inReplyTo":"7f07bf1f3beee2f74a3572d2b9a8d28b6535053e.1721818488.git.ps@pks.im","subject":"Re: [PATCH 2/3] Documentation: document naming schema for struct-related functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-24T16:50:40Z","receivedAt":"2024-07-24T16:50:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> + - Functions that operate on a specific structure and which are used by\n> + other subsystems shall be named after the structure.\n\nI am not sure if this is a good guideline. In the case of strbuf_,\nyou could say it is named after the structure, but I would actually\nthink that both structure and the functions are named after the\nsubsystem/API (i.e. we have \"strbuf\" that other subsystems can use).\n\n> + The function\n> + name should start with the name of the structure followed by a verb.\n> + E.g.\n> +\n> +\tstruct strbuf;\n> +\n> +\tvoid strbuf_add(struct strbuf *buf, ...);\n> +\n> +\tvoid strbuf_reset(struct strbuf *buf);\n> +\n> + is preferred over:\n> +\n> +\tstruct strbuf;\n> +\n> +\tvoid add_string(struct strbuf *buf, ...);\n> +\n> +\tvoid reset_strbuf(struct strbuf *buf);\n\nDo we want to rename start_command(), finish_command(),\nrun_command() and pipe_command()? child_process_start() sounds\nsomewhat ungrammatical.\n\nBy the way, some functions that have strbuf_ in their names do not\nhave anything to do with managing strings using the strbuf\nstructure, but they do things that are *not* about strings, but\nhappen to use strbuf as a way to either feed input to them or carry\noutput out of them. They should be renamed away to lose \"strbuf_\"\nin their names (e.g. strbuf_realpath() is about pathnames; it is\nimmaterial that the function happens to use strbuf to hold its\noutput but takes input from \"const char *\").\n"},{"id":"499284","messageId":"xmqqed7iwx4r.fsf@gitster.g","threadId":"61834","inReplyTo":"5e1de3c3159968e897a83c05dae5e8504d37a16c.1721818488.git.ps@pks.im","subject":"Re: [PATCH 3/3] Documentation: document difference between release and free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-24T16:52:20Z","receivedAt":"2024-07-24T16:52:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> We semi-regularly have discussions around whether a function shall be\n> named `release()` or `free()`. For most of the part we use these two\n> terminologies quite consistently though:\n>\n> - `release()` only frees internal state of a structure, whereas the\n> structure itself is not free'd.\n>\n> - `free()` frees both internal state and the structure itself.\n>\n> Carve out a space where we can add idiomatic names for common functions\n> in our coding guidelines. This space can get extended in the future when\n> we feel the need to document more idiomatic names.\n\nWe have _clear() in some subsystem/API. Are we sure the listed two\nare sufficient and _clear() can be replaced with one of them\n(perhaps _release())?\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> Documentation/CodingGuidelines | 12 ++++++++++++\n> 1 file changed, 12 insertions(+)\n>\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> index 34fcbcb5a4..ace4c4ad0c 100644\n> --- a/Documentation/CodingGuidelines\n> +++ b/Documentation/CodingGuidelines\n> @@ -560,6 +560,18 @@ For C programs:\n> \n> \tvoid reset_strbuf(struct strbuf *buf);\n> \n> + - There are several common idiomatic names for functions performing\n> + specific tasks on structures:\n> +\n> + - `_init()` initializes a structure without allocating the\n> + structure itself.\n> +\n> + - `_release()` releases a structure's contents without\n> + freeing the structure.\n> +\n> + - `_free()` releases a structure's contents and frees the\n> + structure.\n> +\n> For Perl programs:\n> \n> - Most of the C guidelines above apply.\n"},{"id":"499286","messageId":"xmqq8qxqwwyl.fsf@gitster.g","threadId":"61834","inReplyTo":"xmqqikwuwx7j.fsf@gitster.g","subject":"Re: [PATCH 2/3] Documentation: document naming schema for struct-related functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-24T16:56:02Z","receivedAt":"2024-07-24T16:56:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Patrick Steinhardt writes:\n>\n>> + - Functions that operate on a specific structure and which are used by\n>> + other subsystems shall be named after the structure.\n> ...\n>> + The function\n>> + name should start with the name of the structure followed by a verb.\n>> + E.g.\n>> +\n>> +\tstruct strbuf;\n>> +\n>> +\tvoid strbuf_add(struct strbuf *buf, ...);\n>> + ...\n>> +\tvoid strbuf_reset(struct strbuf *buf);\n\nAnother thing we may want to say about these \"The primary data\nstructure subsystem 'S' deals with is called 'struct S' and the\nfunctions that operate on 'struct S' are named S_()\" theme is\nthat the convention for S_() functions is to have the operand,\nwhich is always 'struct S *', of the verb as the first parameter.\n"},{"id":"499292","messageId":"xmqqed7iu0qt.fsf@gitster.g","threadId":"61834","inReplyTo":"1841a256-5c01-4892-99c7-ad7df14e6e0e@gmail.com","subject":"Re: [PATCH 3/3] Documentation: document difference between release and free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-24T18:02:34Z","receivedAt":"2024-07-24T18:02:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>>> I noticed there is also `clear()` used in some places. Should we also\n>>> mention that we don't recommend using `clear()` WRT freeing memory?\n>> In any case I think we should decide on eithe using `clear()` or\n>> using\n>> `release()` for consistency's sake. Which of both we use I don't quite\n>> care, but the following very shoddy analysis clearly favors `release()`:\n>> $ git grep '_clear(' | wc -l\n>> 844\n>> $ git grep '_release(' | wc -l\n>> 2126\n>\n> I think a fairer comparison would be to look at function declarations,\n> not all the call sites.\n>\n> $ { git grep 'void [a-z_]*_release(' '*.h'\n> git grep 'static void [a-z_]*_release(' '*.c'\n> } | wc -l\n> 47\n> $ { git grep 'void [a-z_]*_clear(' '*.h'\n> git grep 'static void [a-z_]*_clear(' '*.c'\n> } | wc -l\n> 58\n>\n> So we have more _clear() functions than _release() functions. I think\n> there may sometimes be a semantic difference between _clear() and\n> _release() as well where some _clear() functions zero out the struct\n> after freeing the members.\n>\n> Thanks for working on this it will be a useful addition to our coding\n> guidelines\n\nThanks for doing a more thorough study of the current codebase. I\ntend to agree that the number of actual _clear() functions matter a\nlot more than how many callsites call _clear(), and it would make\nsense to standardise on it. If everything else being equal, it does\nnot matter which one we pick, but it rarely happens that everything\nelse is equal.\n\n - \"release\" is a bit more cumbersome to type and read than \"clear\".\n\n - \"clear\" at least to me says more about the state of the thing\n after it got cleared (e.g., I would expect it would be filled\n with NUL bytes)\n\n - \"release\" places a lot more stress on what happens to the things\n that were contained before the release takes place.\n\nFor example, upon either \"clear\" or \"release\", I would expect\neverything pointed by elements in an array member of the struct, and\nthe array pointed at by the member, are free'd when we are\n\"clearing/releasing\" a strvec. But I may not care what is left in\nit after \"release\". It can be left to hold all the bytes the struct\nhad before \"release\" got called, as anybody who called the function\nare not supposed to look at the struct again anyway. But we may\nchoose not to have such a variant and always clear the struct after\nreleasing resources it held, just for good hygiene.\n\nSo in short, I would consider that \"clear = release + init\". If we\nwant to have both \"clear\" and \"release\" and have them distinct\nmeaning, that is fine. If we want to simplify and do without \"just\nrelease and leave them dirty\" variant, then we need only one name\nfor it, and I do not mind if we called it \"release\", even though\nI would think \"clear\" is a better name for the action that behaves\nas if \"init\" was done at the end to make it reusable.\n\nThanks.\n"},{"id":"499308","messageId":"xmqq1q3irrgk.fsf@gitster.g","threadId":"61834","inReplyTo":"xmqqv80uwxn2.fsf@gitster.g","subject":"Re: [PATCH 1/3] Documentation: clarify indentation style for C preprocessor directives","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-25T05:06:03Z","receivedAt":"2024-07-25T05:06:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Patrick Steinhardt writes:\n>\n>> There has recently been some discussion around how C preprocessor\n>> directives shall be indented [1]. This discussion was settled towards\n>> indenting after the hash by two spaces [2]. Document it as such.\n>\n> It was settled to have space after and not before the hash, but I do\n> not recall ever agreeing to two spaces. I prefer to increment by 1\n> for each level instead to keep the whole thing less distracting\n> while carrying meaningful information.\n\nUsing the indentation consistently is a good thing I do not object\nto.\n\nAmong our existing header and source files (excluding compat/ that\nis full of borrowed sources), scanning output from\n\n $ git grep -e '# *' '*.[ch]' ':!compat/'\n\ntells me that\n\n - builtin/gc.c (3 lines)\n - git-compat-util.h (52 lines)\n - trace.c (2 lines)\n - wildmatch.c (6 lines)\n\nuse one space indent after '#' per level, while\n\n - hash.h (10 lines, inconsistently 2 uses 1 per level)\n - thread-utils.c (8 lines)\n\nuse two space indent after '#' per level.\n\nIn compat/ directory, only compat/nedmalloc uses 2-space indent.\nThere in the hierarchy are so many files we borrowed from GNU, whose\ncoding style sticks to one-space indent.\n\n"},{"id":"499565","messageId":"ZqiI8H0Zbu_Zl2xS@tanuki","threadId":"61834","inReplyTo":"xmqq1q3irrgk.fsf@gitster.g","subject":"Re: [PATCH 1/3] Documentation: clarify indentation style for C preprocessor directives","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T06:32:16Z","receivedAt":"2024-07-30T06:32:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 24, 2024 at 10:06:03PM -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > Patrick Steinhardt writes:\n> >\n> >> There has recently been some discussion around how C preprocessor\n> >> directives shall be indented [1]. This discussion was settled towards\n> >> indenting after the hash by two spaces [2]. Document it as such.\n> >\n> > It was settled to have space after and not before the hash, but I do\n> > not recall ever agreeing to two spaces. I prefer to increment by 1\n> > for each level instead to keep the whole thing less distracting\n> > while carrying meaningful information.\n> \n> Using the indentation consistently is a good thing I do not object\n> to.\n> \n> Among our existing header and source files (excluding compat/ that\n> is full of borrowed sources), scanning output from\n> \n> $ git grep -e '# *' '*.[ch]' ':!compat/'\n> \n> tells me that\n> \n> - builtin/gc.c (3 lines)\n> - git-compat-util.h (52 lines)\n> - trace.c (2 lines)\n> - wildmatch.c (6 lines)\n> \n> use one space indent after '#' per level, while\n> \n> - hash.h (10 lines, inconsistently 2 uses 1 per level)\n> - thread-utils.c (8 lines)\n> \n> use two space indent after '#' per level.\n> \n> In compat/ directory, only compat/nedmalloc uses 2-space indent.\n> There in the hierarchy are so many files we borrowed from GNU, whose\n> coding style sticks to one-space indent.\n\nThanks for checking. I don't really mind whether we use one or two\nspaces, as long as we are being consistent. I actually took the 2-space\nindent from the comment for \"IndentPPDirectives\" in \".clang-format\",\nwhere it looks as if clang-format was using two spaces. So I took this\nas being the official style we settled on.\n\nOne thing I noticed though is that clang-format is inconsistent with\neither of those styles because it uses a tab character to indent nested\npreprocessor directives. So the comment we have in that file is quite\nmisleading. We can fix this with the below change though. I'll add that\nto this series.\n\nPatrick\n\ndiff --git a/.clang-format b/.clang-format\nindex 16fd12253e..0e3606a8bb 100644\n--- a/.clang-format\n+++ b/.clang-format\n@@ -105,6 +105,7 @@ IndentCaseLabels: false\n # # include \n # #endif\n IndentPPDirectives: AfterHash\n+PPIndentWidth: 1\n \n # Don't indent a function definition or declaration if it is wrapped after the\n # type\n"},{"id":"499566","messageId":"ZqiLA0bGYZfH1OWD@tanuki","threadId":"61834","inReplyTo":"xmqqikwuwx7j.fsf@gitster.g","subject":"Re: [PATCH 2/3] Documentation: document naming schema for struct-related functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T06:41:07Z","receivedAt":"2024-07-30T06:41:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 24, 2024 at 09:50:40AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > + - Functions that operate on a specific structure and which are used by\n> > + other subsystems shall be named after the structure.\n> \n> I am not sure if this is a good guideline. In the case of strbuf_,\n> you could say it is named after the structure, but I would actually\n> think that both structure and the functions are named after the\n> subsystem/API (i.e. we have \"strbuf\" that other subsystems can use).\n\nWell, in most cases I'd expect that the structure is named after the\nsubsystem/API, itself. I'm happy to relax this statement though and say\nthat functions should be named after the subsystem.\n\n> > + The function\n> > + name should start with the name of the structure followed by a verb.\n> > + E.g.\n> > +\n> > +\tstruct strbuf;\n> > +\n> > +\tvoid strbuf_add(struct strbuf *buf, ...);\n> > +\n> > +\tvoid strbuf_reset(struct strbuf *buf);\n> > +\n> > + is preferred over:\n> > +\n> > +\tstruct strbuf;\n> > +\n> > +\tvoid add_string(struct strbuf *buf, ...);\n> > +\n> > +\tvoid reset_strbuf(struct strbuf *buf);\n> \n> Do we want to rename start_command(), finish_command(),\n> run_command() and pipe_command()? \n\nI wouldn't quite go that far for now. We may want to slowly adapt some\nparts of our interfaces over time. But my main goal is rather to make\nthe style consistent for _new_ interfaces we add.\n\n> child_process_start() sounds somewhat ungrammatical.\n\nIt does, but I would argue that it is no different from `strbuf_reset()`\nand other functions where we have the verb as a trailer. And I have to\nsay that I find it a ton easier to reason about code where we have the\nsubsystem it belongs to as a prefix as it neatly groups together things\nand immediately sets you into the correct mindset of what to expect.\nThat is of course a question of preference, I'm not claiming that my\npreferral is objectively the best.\n\nBut again, what I do want to see is consistency. Nobody is helped when\nwe mix both styles in my opinion. It makes writing, reading and\nreviewing code harder than it has to be because you always have to\nremember whether it is `string_list_free()`, `free_string_list()`,\n`string_list_clear()` or `clear_string_list()`.\n\n> By the way, some functions that have strbuf_ in their names do not\n> have anything to do with managing strings using the strbuf\n> structure, but they do things that are *not* about strings, but\n> happen to use strbuf as a way to either feed input to them or carry\n> output out of them. They should be renamed away to lose \"strbuf_\"\n> in their names (e.g. strbuf_realpath() is about pathnames; it is\n> immaterial that the function happens to use strbuf to hold its\n> output but takes input from \"const char *\").\n\nYeah, that's fair indeed.\n\nPatrick\n"},{"id":"499567","messageId":"ZqiLn6JR5BBf9sJU@tanuki","threadId":"61834","inReplyTo":"xmqqed7iwx4r.fsf@gitster.g","subject":"Re: [PATCH 3/3] Documentation: document difference between release and free","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T06:43:43Z","receivedAt":"2024-07-30T06:43:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 24, 2024 at 09:52:20AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > We semi-regularly have discussions around whether a function shall be\n> > named `release()` or `free()`. For most of the part we use these two\n> > terminologies quite consistently though:\n> >\n> > - `release()` only frees internal state of a structure, whereas the\n> > structure itself is not free'd.\n> >\n> > - `free()` frees both internal state and the structure itself.\n> >\n> > Carve out a space where we can add idiomatic names for common functions\n> > in our coding guidelines. This space can get extended in the future when\n> > we feel the need to document more idiomatic names.\n> \n> We have _clear() in some subsystem/API. Are we sure the listed two\n> are sufficient and _clear() can be replaced with one of them\n> (perhaps _release())?\n\nI'd think that `clear()` can be replaced by `release()`, yes. But in\nanother branch I heard the argument that `clear()` is equivalent to\n`release()` followed by `init()`, which I do like. The only downside is\nthat `init()` must not allocate memory in this case, as otherwise the\n`clear()` function would lose the ability to release all resources\nassociated with its structure.\n\nPatrick\n"},{"id":"499568","messageId":"ZqiNCdI_ls6NE6iM@tanuki","threadId":"61834","inReplyTo":"xmqqed7iu0qt.fsf@gitster.g","subject":"Re: [PATCH 3/3] Documentation: document difference between release and free","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T06:49:45Z","receivedAt":"2024-07-30T06:49:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 24, 2024 at 11:02:34AM -0700, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n> >>> I noticed there is also `clear()` used in some places. Should we also\n> >>> mention that we don't recommend using `clear()` WRT freeing memory?\n> >> In any case I think we should decide on eithe using `clear()` or\n> >> using\n> >> `release()` for consistency's sake. Which of both we use I don't quite\n> >> care, but the following very shoddy analysis clearly favors `release()`:\n> >> $ git grep '_clear(' | wc -l\n> >> 844\n> >> $ git grep '_release(' | wc -l\n> >> 2126\n> >\n> > I think a fairer comparison would be to look at function declarations,\n> > not all the call sites.\n> >\n> > $ { git grep 'void [a-z_]*_release(' '*.h'\n> > git grep 'static void [a-z_]*_release(' '*.c'\n> > } | wc -l\n> > 47\n> > $ { git grep 'void [a-z_]*_clear(' '*.h'\n> > git grep 'static void [a-z_]*_clear(' '*.c'\n> > } | wc -l\n> > 58\n> >\n> > So we have more _clear() functions than _release() functions. I think\n> > there may sometimes be a semantic difference between _clear() and\n> > _release() as well where some _clear() functions zero out the struct\n> > after freeing the members.\n> >\n> > Thanks for working on this it will be a useful addition to our coding\n> > guidelines\n> \n> Thanks for doing a more thorough study of the current codebase. I\n> tend to agree that the number of actual _clear() functions matter a\n> lot more than how many callsites call _clear(), and it would make\n> sense to standardise on it. If everything else being equal, it does\n> not matter which one we pick, but it rarely happens that everything\n> else is equal.\n\nI'm not quite sure that I agree with this. I think coding style is most\nheavily influenced by what you see most in a codebase. So I'd argue that\nit is both declarations/definitions and callsites that influence the\ngeneral shape.\n\nThis of course means that interfaces like `struct strbuf` have way more\nimpact on our coding style than others, simply because it is being used\nall over the place. But in my opinion that follows naturally, because\nthe coding style that we use should work best for what is being used\nmost often.\n\nBut anyway, this is splitting hairs :)\n\n> - \"release\" is a bit more cumbersome to type and read than \"clear\".\n> \n> - \"clear\" at least to me says more about the state of the thing\n> after it got cleared (e.g., I would expect it would be filled\n> with NUL bytes)\n> \n> - \"release\" places a lot more stress on what happens to the things\n> that were contained before the release takes place.\n> \n> For example, upon either \"clear\" or \"release\", I would expect\n> everything pointed by elements in an array member of the struct, and\n> the array pointed at by the member, are free'd when we are\n> \"clearing/releasing\" a strvec. But I may not care what is left in\n> it after \"release\". It can be left to hold all the bytes the struct\n> had before \"release\" got called, as anybody who called the function\n> are not supposed to look at the struct again anyway. But we may\n> choose not to have such a variant and always clear the struct after\n> releasing resources it held, just for good hygiene.\n> \n> So in short, I would consider that \"clear = release + init\". If we\n> want to have both \"clear\" and \"release\" and have them distinct\n> meaning, that is fine. If we want to simplify and do without \"just\n> release and leave them dirty\" variant, then we need only one name\n> for it, and I do not mind if we called it \"release\", even though\n> I would think \"clear\" is a better name for the action that behaves\n> as if \"init\" was done at the end to make it reusable.\n\nI actually like this definition. The only downside I see of defining\n`clear = release + init` is that `init()` probably shouldn't be allowed\nto allocate any memory in this case. Otherwise, calling `clear()` on a\nstructure would not cause us to free all resources associated with it,\nwhich would be unexpected to me.\n\nPatrick\n"},{"id":"499569","messageId":"cover.1722323818.git.ps@pks.im","threadId":"61834","inReplyTo":"cover.1721818488.git.ps@pks.im","subject":"[PATCH v2 0/5] Documentation: some coding guideline updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T07:24:29Z","receivedAt":"2024-07-30T07:24:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that aims to improve our\ncoding guidelines such that we arrive at a more consistent coding style.\n\nChanges compared to v1:\n\n - Fix clang-format to use a single space to indent preprocessor\n directives instead of using tabs. Thus, this series is now built\n with kn/ci-clang-format at 1b8f306612 (ci/style-check: add\n `RemoveBracesLLVM` in CI job, 2024-07-23) merged into v2.46.0.\n\n - Adapt the coding guidelines accordingly to also only use a single\n space for indentation of nested preprocessor directives.\n\n - Adopt a proposal by Junio to more clearly spell out the relationship\n between a subsystem `S`, `struct S` and its functions `S_()`.\n\n - Document `S_clear()`-style functions. I have adopted the proposal by\n Junio hear, where `clear = release + init` with the restriction that\n `S_init()` must not allocate any resources.\n\n - Add another patch on top that makes variable initializers consistent\n in our coding guidelines. Our style is to add spaces between the\n curly brace and the initializers (`struct foo bar = { something };`).\n\nI think I captured everything that came out of the discussion, but\nplease let me know in case I misinterpreted or forgot anything.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (5):\n clang-format: fix indentation width for preprocessor directives\n Documentation: clarify indentation style for C preprocessor directives\n Documentation: document naming schema for structs and their functions\n Documentation: document idiomatic function names\n Documentation: consistently use spaces inside initializers\n\n .clang-format | 6 +++--\n Documentation/CodingGuidelines | 48 +++++++++++++++++++++++++++++++++-\n 2 files changed, 51 insertions(+), 3 deletions(-)\n\nRange-diff against v1:\n-: ---------- > 1: c33ad700d6 clang-format: fix indentation width for preprocessor directives\n1: 64e0b44993 ! 2: e3baf01234 Documentation: clarify indentation style for C preprocessor directives\n @@ Metadata\n ## Commit message ##\n Documentation: clarify indentation style for C preprocessor directives\n \n - There has recently been some discussion around how C preprocessor\n - directives shall be indented [1]. This discussion was settled towards\n - indenting after the hash by two spaces [2]. Document it as such.\n -\n - [1]: https://lore.kernel.org/all/xmqqwmmm1bw6.fsf@gitster.g/\n - [2]: https://lore.kernel.org/all/20240708092317.267915-2-karthik.188@gmail.com/\n + In the preceding commit, we have settled on using a single space per\n + nesting level to indent preprocessor directives. Clarify our coding\n + guidelines accordingly.\n \n Signed-off-by: Patrick Steinhardt \n \n @@ Documentation/CodingGuidelines: For C programs:\n - We use tabs to indent, and interpret tabs as taking up to\n 8 spaces.\n \n -+ - Nested C preprocessor directives are indented after the hash by two\n -+ spaces per nesting level.\n ++ - Nested C preprocessor directives are indented after the hash by one\n ++ space per nesting level.\n +\n +\t#if FOO\n -+\t# include \n -+\t# if BAR\n -+\t# include \n -+\t# endif\n ++\t# include \n ++\t# if BAR\n ++\t# include \n ++\t# endif\n +\t#endif\n +\n - We try to keep to at most 80 characters per line.\n2: 7f07bf1f3b ! 3: 25f73b970d Documentation: document naming schema for struct-related functions\n @@ Metadata\n Author: Patrick Steinhardt \n \n ## Commit message ##\n - Documentation: document naming schema for struct-related functions\n + Documentation: document naming schema for structs and their functions\n \n We nowadays have a proper mishmash of struct-related functions that are\n called `_` (e.g. `clear_prio_queue()`) versus functions\n @@ Documentation/CodingGuidelines: For C programs:\n use your own debugger and arguments. Example: `GIT_DEBUGGER=\"ddd --gdb\"\n ./bin-wrappers/git log` (See `wrap-for-bin.sh`.)\n \n -+ - Functions that operate on a specific structure and which are used by\n -+ other subsystems shall be named after the structure. The function\n -+ name should start with the name of the structure followed by a verb.\n -+ E.g.\n ++ - The primary data structure that a subsystem 'S' deals with is called\n ++ `struct S`. Functions that operate on `struct S` are named\n ++ `S_()` and should generally receive a pointer to `struct S` as\n ++ first parameter. E.g.\n +\n +\tstruct strbuf;\n +\n3: 5e1de3c315 ! 4: d4ce00303f Documentation: document difference between release and free\n @@ Metadata\n Author: Patrick Steinhardt \n \n ## Commit message ##\n - Documentation: document difference between release and free\n + Documentation: document idiomatic function names\n \n We semi-regularly have discussions around whether a function shall be\n - named `release()` or `free()`. For most of the part we use these two\n - terminologies quite consistently though:\n -\n - - `release()` only frees internal state of a structure, whereas the\n - structure itself is not free'd.\n -\n - - `free()` frees both internal state and the structure itself.\n + named `S_release()`, `S_clear()` or `S_free()`. Indeed, it may not be\n + obvious which of these is preferable as we never really defined what\n + each of these variants means exactly.\n \n Carve out a space where we can add idiomatic names for common functions\n - in our coding guidelines. This space can get extended in the future when\n - we feel the need to document more idiomatic names.\n + in our coding guidelines and define each of those functions. Like this,\n + we can get to a shared understanding of their respective semantics and\n + can easily point towards our style guide in future discussions such that\n + our codebase becomes more consistent over time.\n +\n + Note that the intent is not to rename all functions which violate these\n + semantics right away. Rather, the intent is to slowly converge towards a\n + common style over time.\n \n Signed-off-by: Patrick Steinhardt \n \n @@ Documentation/CodingGuidelines: For C programs:\n \tvoid reset_strbuf(struct strbuf *buf);\n \n + - There are several common idiomatic names for functions performing\n -+ specific tasks on structures:\n ++ specific tasks on a structure `S`:\n +\n -+ - `_init()` initializes a structure without allocating the\n ++ - `S_init()` initializes a structure without allocating the\n + structure itself.\n +\n -+ - `_release()` releases a structure's contents without\n -+ freeing the structure.\n ++ - `S_release()` releases a structure's contents without freeing the\n ++ structure.\n ++\n ++ - `S_clear()` is equivalent to `S_release()` followed by `S_init()`\n ++ such that the structure is directly usable after clearing it. When\n ++ `S_clear()` is provided, `S_init()` shall not allocate resources\n ++ that need to be released again.\n +\n -+ - `_free()` releases a structure's contents and frees the\n ++ - `S_free()` releases a structure's contents and frees the\n + structure.\n +\n For Perl programs:\n-: ---------- > 5: 8789323ac7 Documentation: consistently use spaces inside initializers\n\n-- \n2.46.0.dirty\n\n"},{"id":"499570","messageId":"c33ad700d68b5d8b5940dd803d116ec19bb75c7e.1722323818.git.ps@pks.im","threadId":"61834","inReplyTo":"cover.1722323818.git.ps@pks.im","subject":"[PATCH v2 1/5] clang-format: fix indentation width for preprocessor directives","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T07:24:33Z","receivedAt":"2024-07-30T07:24:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In [1], we have improved our clang-format configuration to also specify\nthe style for how to indent preprocessor directives. But while we have\nsettled the question of where to put the indentation, either before or\nafter the hash sign, we didn't specify exactly how to indent.\n\nWith the current configuration, clang-format uses tabs to indent each\nlevel of nested preprocessor directives, which is in fact unintentional\nand never done in our codebase. Instead, we use a mixture of indenting\nby either one or two spaces, where using a single space is somewhat more\ncommon.\n\nAdapt our clang-format configuration accordingly by specifying an\nindentation width of one space.\n\n[1]: <20240708092317.267915-1-karthik.188@gmail.com>\n\nSigned-off-by: Patrick Steinhardt \n---\n .clang-format | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/.clang-format b/.clang-format\nindex 16fd12253e..0b82f3c776 100644\n--- a/.clang-format\n+++ b/.clang-format\n@@ -100,11 +100,13 @@ BreakStringLiterals: false\n # Switch statement body is always indented one level more than case labels.\n IndentCaseLabels: false\n \n-# Indents directives before the hash.\n+# Indents directives before the hash. Each level uses a single space for\n+# indentation.\n # #if FOO\n-# # include \n+# # include \n # #endif\n IndentPPDirectives: AfterHash\n+PPIndentWidth: 1\n \n # Don't indent a function definition or declaration if it is wrapped after the\n # type\n-- \n2.46.0.dirty\n\n"},{"id":"499571","messageId":"e3baf012344df07a464dba997e8e32f521c8c195.1722323818.git.ps@pks.im","threadId":"61834","inReplyTo":"cover.1722323818.git.ps@pks.im","subject":"[PATCH v2 2/5] Documentation: clarify indentation style for C preprocessor directives","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T07:24:38Z","receivedAt":"2024-07-30T07:24:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the preceding commit, we have settled on using a single space per\nnesting level to indent preprocessor directives. Clarify our coding\nguidelines accordingly.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/CodingGuidelines | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 1d92b2da03..65fba3b810 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -214,6 +214,16 @@ For C programs:\n - We use tabs to indent, and interpret tabs as taking up to\n 8 spaces.\n \n+ - Nested C preprocessor directives are indented after the hash by one\n+ space per nesting level.\n+\n+\t#if FOO\n+\t# include \n+\t# if BAR\n+\t# include \n+\t# endif\n+\t#endif\n+\n - We try to keep to at most 80 characters per line.\n \n - As a Git developer we assume you have a reasonably modern compiler\n-- \n2.46.0.dirty\n\n"},{"id":"499572","messageId":"25f73b970dcf8b1d1e7629f75c157ee02cd10809.1722323818.git.ps@pks.im","threadId":"61834","inReplyTo":"cover.1722323818.git.ps@pks.im","subject":"[PATCH v2 3/5] Documentation: document naming schema for structs and their functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T07:24:43Z","receivedAt":"2024-07-30T07:24:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We nowadays have a proper mishmash of struct-related functions that are\ncalled `_` (e.g. `clear_prio_queue()`) versus functions\nthat are called `_` (e.g. `strbuf_clear()`). While the\nformer style may be easier to tie into a spoken conversation, most of\nour communication happens in text anyway. Furthermore, prefixing\nfunctions with the name of the structure they operate on makes it way\neasier to group them together, see which functions are related, and will\nalso help folks who are using code completion.\n\nLet's thus settle on one style, namely the one where functions start\nwith the name of the structure they operate on.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/CodingGuidelines | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 65fba3b810..a6a1ede204 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -541,6 +541,25 @@ For C programs:\n use your own debugger and arguments. Example: `GIT_DEBUGGER=\"ddd --gdb\"\n ./bin-wrappers/git log` (See `wrap-for-bin.sh`.)\n \n+ - The primary data structure that a subsystem 'S' deals with is called\n+ `struct S`. Functions that operate on `struct S` are named\n+ `S_()` and should generally receive a pointer to `struct S` as\n+ first parameter. E.g.\n+\n+\tstruct strbuf;\n+\n+\tvoid strbuf_add(struct strbuf *buf, ...);\n+\n+\tvoid strbuf_reset(struct strbuf *buf);\n+\n+ is preferred over:\n+\n+\tstruct strbuf;\n+\n+\tvoid add_string(struct strbuf *buf, ...);\n+\n+\tvoid reset_strbuf(struct strbuf *buf);\n+\n For Perl programs:\n \n - Most of the C guidelines above apply.\n-- \n2.46.0.dirty\n\n"},{"id":"499573","messageId":"d4ce00303f230e6ebc7099e07bb451811e6eabab.1722323818.git.ps@pks.im","threadId":"61834","inReplyTo":"cover.1722323818.git.ps@pks.im","subject":"[PATCH v2 4/5] Documentation: document idiomatic function names","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T07:24:47Z","receivedAt":"2024-07-30T07:24:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We semi-regularly have discussions around whether a function shall be\nnamed `S_release()`, `S_clear()` or `S_free()`. Indeed, it may not be\nobvious which of these is preferable as we never really defined what\neach of these variants means exactly.\n\nCarve out a space where we can add idiomatic names for common functions\nin our coding guidelines and define each of those functions. Like this,\nwe can get to a shared understanding of their respective semantics and\ncan easily point towards our style guide in future discussions such that\nour codebase becomes more consistent over time.\n\nNote that the intent is not to rename all functions which violate these\nsemantics right away. Rather, the intent is to slowly converge towards a\ncommon style over time.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/CodingGuidelines | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex a6a1ede204..b63a8f9a44 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -560,6 +560,23 @@ For C programs:\n \n \tvoid reset_strbuf(struct strbuf *buf);\n \n+ - There are several common idiomatic names for functions performing\n+ specific tasks on a structure `S`:\n+\n+ - `S_init()` initializes a structure without allocating the\n+ structure itself.\n+\n+ - `S_release()` releases a structure's contents without freeing the\n+ structure.\n+\n+ - `S_clear()` is equivalent to `S_release()` followed by `S_init()`\n+ such that the structure is directly usable after clearing it. When\n+ `S_clear()` is provided, `S_init()` shall not allocate resources\n+ that need to be released again.\n+\n+ - `S_free()` releases a structure's contents and frees the\n+ structure.\n+\n For Perl programs:\n \n - Most of the C guidelines above apply.\n-- \n2.46.0.dirty\n\n"},{"id":"499574","messageId":"8789323ac770e00707df4708994f7767bf57b00f.1722323818.git.ps@pks.im","threadId":"61834","inReplyTo":"cover.1722323818.git.ps@pks.im","subject":"[PATCH v2 5/5] Documentation: consistently use spaces inside initializers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-07-30T07:24:52Z","receivedAt":"2024-07-30T07:24:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Our coding guide is inconsistent with how it uses spaces inside of\ninitializers (`struct foo bar = { something }`). While we mostly carry\nthe space between open and closing braces and the initialized members,\nin one case we don't.\n\nFix this one instance such that we consistently carry the space. This is\nalso consistent with how clang-format formats such initializers.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/CodingGuidelines | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex b63a8f9a44..3daa1f82bb 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -244,7 +244,7 @@ For C programs:\n . since around 2007 with 2b6854c863a, we have been using\n initializer elements which are not computable at load time. E.g.:\n \n-\tconst char *args[] = {\"constant\", variable, NULL};\n+\tconst char *args[] = { \"constant\", variable, NULL };\n \n . since early 2012 with e1327023ea, we have been using an enum\n definition whose last element is followed by a comma. This, like\n-- \n2.46.0.dirty\n\n"},{"id":"499610","messageId":"CAOLa=ZRrUMawq_K=_4OUQrV7V67LhCUrBGtk2G4Z5Zq7+h=hAg@mail.gmail.com","threadId":"61834","inReplyTo":"c33ad700d68b5d8b5940dd803d116ec19bb75c7e.1722323818.git.ps@pks.im","subject":"Re: [PATCH v2 1/5] clang-format: fix indentation width for preprocessor directives","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-30T14:19:05Z","receivedAt":"2024-07-30T14:19:07Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> In [1], we have improved our clang-format configuration to also specify\n> the style for how to indent preprocessor directives. But while we have\n> settled the question of where to put the indentation, either before or\n> after the hash sign, we didn't specify exactly how to indent.\n>\n> With the current configuration, clang-format uses tabs to indent each\n> level of nested preprocessor directives, which is in fact unintentional\n> and never done in our codebase. Instead, we use a mixture of indenting\n> by either one or two spaces, where using a single space is somewhat more\n> common.\n>\n> Adapt our clang-format configuration accordingly by specifying an\n> indentation width of one space.\n>\n> [1]: <20240708092317.267915-1-karthik.188@gmail.com>\n>\n\nI totally missed this, thanks for fixing it up. The patch looks good.\n"},{"id":"499644","messageId":"xmqq34nqbnwu.fsf@gitster.g","threadId":"61834","inReplyTo":"cover.1722323818.git.ps@pks.im","subject":"Re: [PATCH v2 0/5] Documentation: some coding guideline updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-30T20:55:13Z","receivedAt":"2024-07-30T20:55:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> this is the second version of my patch series that aims to improve our\n> coding guidelines such that we arrive at a more consistent coding style.\n> ...\n> I think I captured everything that came out of the discussion, but\n> please let me know in case I misinterpreted or forgot anything.\n\nNothing jumps out at me as wrong/missing in this version.\n\nWill queue. Thanks.\n"},{"id":"499674","messageId":"CAOLa=ZS1JS5YyguYhyeAeWuv7Sb2tB0ze_5=GyQgRmhmVDNWyw@mail.gmail.com","threadId":"61834","inReplyTo":"cover.1722323818.git.ps@pks.im","subject":"Re: [PATCH v2 0/5] Documentation: some coding guideline updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-31T09:12:41Z","receivedAt":"2024-07-31T09:12:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> Hi,\n>\n> this is the second version of my patch series that aims to improve our\n> coding guidelines such that we arrive at a more consistent coding style.\n>\n> Changes compared to v1:\n>\n> - Fix clang-format to use a single space to indent preprocessor\n> directives instead of using tabs. Thus, this series is now built\n> with kn/ci-clang-format at 1b8f306612 (ci/style-check: add\n> `RemoveBracesLLVM` in CI job, 2024-07-23) merged into v2.46.0.\n>\n> - Adapt the coding guidelines accordingly to also only use a single\n> space for indentation of nested preprocessor directives.\n>\n> - Adopt a proposal by Junio to more clearly spell out the relationship\n> between a subsystem `S`, `struct S` and its functions `S_()`.\n>\n> - Document `S_clear()`-style functions. I have adopted the proposal by\n> Junio hear, where `clear = release + init` with the restriction that\n> `S_init()` must not allocate any resources.\n>\n> - Add another patch on top that makes variable initializers consistent\n> in our coding guidelines. Our style is to add spaces between the\n> curly brace and the initializers (`struct foo bar = { something };`).\n>\n> I think I captured everything that came out of the discussion, but\n> please let me know in case I misinterpreted or forgot anything.\n>\n> Thanks!\n>\n\nThis series seems good. I did a read-through and have no changes to\nsuggest! I'm really happy with these fixes which slowly improve the\nquality of the codebase.\n\nThanks!\n"}]}