{"thread":{"id":"62102","subject":"[PATCH 0/2] Simplify \"commented\" API functions","startedAt":"2024-09-12T20:53:05Z","lastAt":"2024-10-29T20:53:59Z","messageCount":7,"participants":["Junio C Hamano","Patrick Steinhardt","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"502719","messageId":"20240912205301.1809355-1-gitster@pobox.com","threadId":"62102","inReplyTo":null,"subject":"[PATCH 0/2] Simplify \"commented\" API functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-12T20:52:59Z","receivedAt":"2024-09-12T20:53:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We [earlier] noticed that a few helper functions that format strings\ninto a strbuf, prefixed with an arbitrary comment leading character,\nare forcing their callers to always pass the same argument.  Instead\nof keeping this excess flexibility nobody wants in practice, narrow\nthe API so that all callers of these functions will get the same\ncomment leading character.\n\nSuperficially it may appear that this goes against one of the recent\ntrend, \"war on global variables\", but I doubt it matters much in the\nlonger run.\n\nThese call sites all have already been relying on the global\n\"comment_line_str\" anyway, and passing the variable from the top of\narbitrary deep call chain, which may not care specifically about\nthis single variable comment_line_str, would not scale as a\nsolution.  If we were to make it a convention to pass something from\nthe very top of the call chain everywhere, it should not be an\nindividual variable that is overly specific, like comment_line_str.\nRather, it has to be something that allows access to a bag of all\nthe global settings (possibly wider than \"the_repository\" struct).\nWe can also limit our reliance to the globals by allowing a single\nglobal function call to obtaion such a bag of all global settings,\ni.e. \"get_the_environment()\", and allow everybody, including\nfunctions at the leaf level, to ask \"what is the comment_line_str in\nthe environment I am working in?\".  As part of the libification, we\ncan plug in an appropriate shim for get_the_environment().\n\n\n[earlier] https://lore.kernel.org/git/xmqq7cn4g3nx.fsf@gitster.g/\n\nJunio C Hamano (2):\n  strbuf: retire strbuf_commented_addf()\n  strbuf: retire strbuf_commented_lines()\n\n add-patch.c          |  8 ++++----\n builtin/branch.c     |  2 +-\n builtin/merge.c      |  8 ++++----\n builtin/notes.c      | 10 +++++-----\n builtin/stripspace.c |  2 +-\n builtin/tag.c        |  4 ++--\n fmt-merge-msg.c      | 17 +++++++----------\n rebase-interactive.c |  8 ++++----\n sequencer.c          | 16 +++++++---------\n strbuf.c             | 11 +++++------\n strbuf.h             | 13 +++++--------\n wt-status.c          |  6 +++---\n 12 files changed, 48 insertions(+), 57 deletions(-)\n\n-- \n2.46.0-717-g3841ff3f09\n\n"},{"id":"502720","messageId":"20240912205301.1809355-2-gitster@pobox.com","threadId":"62102","inReplyTo":"20240912205301.1809355-1-gitster@pobox.com","subject":"[PATCH 1/2] strbuf: retire strbuf_commented_addf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-12T20:53:00Z","receivedAt":"2024-09-12T20:53:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The API allows the caller to pass any string as the comment prefix,\nbut in reality, everybody passes the comment_line_str from the\nenvironment.\n\nReplace this overly flexible API with strbuf_comment_addf() that\ndoes not allow customization of the comment_line_str (usually '#'\nbut can be configured via the core.commentChar).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n add-patch.c          | 8 ++++----\n builtin/branch.c     | 2 +-\n builtin/merge.c      | 8 ++++----\n builtin/tag.c        | 4 ++--\n rebase-interactive.c | 2 +-\n sequencer.c          | 4 ++--\n strbuf.c             | 6 +++---\n strbuf.h             | 6 +++---\n wt-status.c          | 2 +-\n 9 files changed, 21 insertions(+), 21 deletions(-)\n\ndiff --git a/add-patch.c b/add-patch.c\nindex 46f6bddfe5..32c990cc18 100644\n--- a/add-patch.c\n+++ b/add-patch.c\n@@ -1114,11 +1114,11 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n \tsize_t i;\n \n \tstrbuf_reset(&s->buf);\n-\tstrbuf_commented_addf(&s->buf, comment_line_str,\n+\tstrbuf_comment_addf(&s->buf,\n \t\t\t      _(\"Manual hunk edit mode -- see bottom for \"\n \t\t\t\t\"a quick guide.\\n\"));\n \trender_hunk(s, hunk, 0, 0, &s->buf);\n-\tstrbuf_commented_addf(&s->buf, comment_line_str,\n+\tstrbuf_comment_addf(&s->buf,\n \t\t\t      _(\"---\\n\"\n \t\t\t\t\"To remove '%c' lines, make them ' ' lines \"\n \t\t\t\t\"(context).\\n\"\n@@ -1127,13 +1127,13 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)\n \t\t\t      s->mode->is_reverse ? '+' : '-',\n \t\t\t      s->mode->is_reverse ? '-' : '+',\n \t\t\t      comment_line_str);\n-\tstrbuf_commented_addf(&s->buf, comment_line_str, \"%s\",\n+\tstrbuf_comment_addf(&s->buf,  \"%s\",\n \t\t\t      _(s->mode->edit_hunk_hint));\n \t/*\n \t * TRANSLATORS: 'it' refers to the patch mentioned in the previous\n \t * messages.\n \t */\n-\tstrbuf_commented_addf(&s->buf, comment_line_str,\n+\tstrbuf_comment_addf(&s->buf,\n \t\t\t      _(\"If it does not apply cleanly, you will be \"\n \t\t\t\t\"given an opportunity to\\n\"\n \t\t\t\t\"edit again.  If all lines of the hunk are \"\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 48cac74f97..b2a4206e1b 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -681,7 +681,7 @@ static int edit_branch_description(const char *branch_name)\n \texists = !read_branch_desc(&buf, branch_name);\n \tif (!buf.len || buf.buf[buf.len-1] != '\\n')\n \t\tstrbuf_addch(&buf, '\\n');\n-\tstrbuf_commented_addf(&buf, comment_line_str,\n+\tstrbuf_comment_addf(&buf,\n \t\t    _(\"Please edit the description for the branch\\n\"\n \t\t      \"  %s\\n\"\n \t\t      \"Lines starting with '%s' will be stripped.\\n\"),\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 9fba27d85d..8794fea28f 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -863,15 +863,15 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tstrbuf_addch(&msg, '\\n');\n \t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {\n \t\t\twt_status_append_cut_line(&msg);\n-\t\t\tstrbuf_commented_addf(&msg, comment_line_str, \"\\n\");\n+\t\t\tstrbuf_comment_addf(&msg,  \"\\n\");\n \t\t}\n-\t\tstrbuf_commented_addf(&msg, comment_line_str,\n+\t\tstrbuf_comment_addf(&msg,\n \t\t\t\t      _(merge_editor_comment));\n \t\tif (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)\n-\t\t\tstrbuf_commented_addf(&msg, comment_line_str,\n+\t\t\tstrbuf_comment_addf(&msg,\n \t\t\t\t\t      _(scissors_editor_comment));\n \t\telse\n-\t\t\tstrbuf_commented_addf(&msg, comment_line_str,\n+\t\t\tstrbuf_comment_addf(&msg,\n \t\t\t\t_(no_scissors_editor_comment), comment_line_str);\n \t}\n \tif (signoff)\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex a1fb218512..0929cfc158 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -334,10 +334,10 @@ static void create_tag(const struct object_id *object, const char *object_ref,\n \t\t\tstruct strbuf buf = STRBUF_INIT;\n \t\t\tstrbuf_addch(&buf, '\\n');\n \t\t\tif (opt->cleanup_mode == CLEANUP_ALL)\n-\t\t\t\tstrbuf_commented_addf(&buf, comment_line_str,\n+\t\t\t\tstrbuf_comment_addf(&buf,\n \t\t\t\t      _(tag_template), tag, comment_line_str);\n \t\t\telse\n-\t\t\t\tstrbuf_commented_addf(&buf, comment_line_str,\n+\t\t\t\tstrbuf_comment_addf(&buf,\n \t\t\t\t      _(tag_template_nocleanup), tag, comment_line_str);\n \t\t\twrite_or_die(fd, buf.buf, buf.len);\n \t\t\tstrbuf_release(&buf);\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex cbeb864147..dd2ec363d8 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -73,7 +73,7 @@ void append_todo_help(int command_count,\n \n \tif (!edit_todo) {\n \t\tstrbuf_addch(buf, '\\n');\n-\t\tstrbuf_commented_addf(buf, comment_line_str,\n+\t\tstrbuf_comment_addf(buf,\n \t\t\t\t      Q_(\"Rebase %s onto %s (%d command)\",\n \t\t\t\t\t \"Rebase %s onto %s (%d commands)\",\n \t\t\t\t\t command_count),\ndiff --git a/sequencer.c b/sequencer.c\nindex a2284ac9e9..bf844ce98e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -731,11 +731,11 @@ void append_conflicts_hint(struct index_state *istate,\n \t}\n \n \tstrbuf_addch(msgbuf, '\\n');\n-\tstrbuf_commented_addf(msgbuf, comment_line_str, \"Conflicts:\\n\");\n+\tstrbuf_comment_addf(msgbuf,  \"Conflicts:\\n\");\n \tfor (i = 0; i < istate->cache_nr;) {\n \t\tconst struct cache_entry *ce = istate->cache[i++];\n \t\tif (ce_stage(ce)) {\n-\t\t\tstrbuf_commented_addf(msgbuf, comment_line_str,\n+\t\t\tstrbuf_comment_addf(msgbuf,\n \t\t\t\t\t      \"\\t%s\\n\", ce->name);\n \t\t\twhile (i < istate->cache_nr &&\n \t\t\t       !strcmp(ce->name, istate->cache[i]->name))\ndiff --git a/strbuf.c b/strbuf.c\nindex 3d2189a7f6..6c525da7a6 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -1,4 +1,5 @@\n #include \"git-compat-util.h\"\n+#include \"environment.h\"\n #include \"gettext.h\"\n #include \"hex-ll.h\"\n #include \"strbuf.h\"\n@@ -384,8 +385,7 @@ void strbuf_add_commented_lines(struct strbuf *out, const char *buf,\n \tadd_lines(out, comment_prefix, buf, size, 1);\n }\n \n-void strbuf_commented_addf(struct strbuf *sb, const char *comment_prefix,\n-\t\t\t   const char *fmt, ...)\n+void strbuf_comment_addf(struct strbuf *sb, const char *fmt, ...)\n {\n \tva_list params;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -395,7 +395,7 @@ void strbuf_commented_addf(struct strbuf *sb, const char *comment_prefix,\n \tstrbuf_vaddf(&buf, fmt, params);\n \tva_end(params);\n \n-\tstrbuf_add_commented_lines(sb, buf.buf, buf.len, comment_prefix);\n+\tstrbuf_add_commented_lines(sb, buf.buf, buf.len, comment_line_str);\n \tif (incomplete_line)\n \t\tsb->buf[--sb->len] = '\\0';\n \ndiff --git a/strbuf.h b/strbuf.h\nindex 003f880ff7..aebc8020ae 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -385,11 +385,11 @@ __attribute__((format (printf,2,3)))\n void strbuf_addf(struct strbuf *sb, const char *fmt, ...);\n \n /**\n- * Add a formatted string prepended by a comment character and a\n+ * Add a formatted string prepended by the comment character and a\n  * blank to the buffer.\n  */\n-__attribute__((format (printf, 3, 4)))\n-void strbuf_commented_addf(struct strbuf *sb, const char *comment_prefix, const char *fmt, ...);\n+__attribute__((format (printf,2,3)))\n+void strbuf_comment_addf(struct strbuf *sb, const char *fmt, ...);\n \n __attribute__((format (printf,2,0)))\n void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\ndiff --git a/wt-status.c b/wt-status.c\nindex b778eef989..44d29b721f 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1111,7 +1111,7 @@ void wt_status_append_cut_line(struct strbuf *buf)\n {\n \tconst char *explanation = _(\"Do not modify or remove the line above.\\nEverything below it will be ignored.\");\n \n-\tstrbuf_commented_addf(buf, comment_line_str, \"%s\", cut_line);\n+\tstrbuf_comment_addf(buf,  \"%s\", cut_line);\n \tstrbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_str);\n }\n \n-- \n2.46.0-717-g3841ff3f09\n\n"},{"id":"502721","messageId":"20240912205301.1809355-3-gitster@pobox.com","threadId":"62102","inReplyTo":"20240912205301.1809355-1-gitster@pobox.com","subject":"[PATCH 2/2] strbuf: retire strbuf_commented_lines()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-12T20:53:01Z","receivedAt":"2024-09-12T20:53:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The API allows the caller to pass any string as the comment prefix,\nbut in reality, everybody passes the comment_line_str from the\nenvironment.\n\nReplace this overly flexible API with strbuf_comment_lines() that\ndoes not allow customization of the comment_line_str (usually '#'\nbut can be configured via the core.commentChar).\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/notes.c      | 10 +++++-----\n builtin/stripspace.c |  2 +-\n fmt-merge-msg.c      | 17 +++++++----------\n rebase-interactive.c |  6 +++---\n sequencer.c          | 12 +++++-------\n strbuf.c             |  7 +++----\n strbuf.h             |  7 ++-----\n wt-status.c          |  4 ++--\n 8 files changed, 28 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 4cc5bfedc3..8c4d7f6f89 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -178,7 +178,7 @@ static void write_commented_object(int fd, const struct object_id *object)\n \n \tif (strbuf_read(&buf, show.out, 0) < 0)\n \t\tdie_errno(_(\"could not read 'show' output\"));\n-\tstrbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);\n+\tstrbuf_add_comment_lines(&cbuf, buf.buf, buf.len);\n \twrite_or_die(fd, cbuf.buf, cbuf.len);\n \n \tstrbuf_release(&cbuf);\n@@ -206,10 +206,10 @@ static void prepare_note_data(const struct object_id *object, struct note_data *\n \t\t\tcopy_obj_to_fd(fd, old_note);\n \n \t\tstrbuf_addch(&buf, '\\n');\n-\t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"), comment_line_str);\n-\t\tstrbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template)),\n-\t\t\t\t\t   comment_line_str);\n-\t\tstrbuf_add_commented_lines(&buf, \"\\n\", strlen(\"\\n\"), comment_line_str);\n+\t\tstrbuf_add_comment_lines(&buf, \"\\n\", strlen(\"\\n\"));\n+\t\tstrbuf_add_comment_lines(&buf, _(note_template),\n+\t\t\t\t\t strlen(_(note_template)));\n+\t\tstrbuf_add_comment_lines(&buf, \"\\n\", strlen(\"\\n\"));\n \t\twrite_or_die(fd, buf.buf, buf.len);\n \n \t\twrite_commented_object(fd, object);\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex e5626e5126..7be99a2ec0 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -13,7 +13,7 @@ static void comment_lines(struct strbuf *buf)\n \tsize_t len;\n \n \tmsg = strbuf_detach(buf, &len);\n-\tstrbuf_add_commented_lines(buf, msg, len, comment_line_str);\n+\tstrbuf_add_comment_lines(buf, msg, len);\n \tfree(msg);\n }\n \ndiff --git a/fmt-merge-msg.c b/fmt-merge-msg.c\nindex 6acb37b480..19d7dd10a0 100644\n--- a/fmt-merge-msg.c\n+++ b/fmt-merge-msg.c\n@@ -511,8 +511,7 @@ static void fmt_tag_signature(struct strbuf *tagbuf,\n \tstrbuf_complete_line(tagbuf);\n \tif (sig->len) {\n \t\tstrbuf_addch(tagbuf, '\\n');\n-\t\tstrbuf_add_commented_lines(tagbuf, sig->buf, sig->len,\n-\t\t\t\t\t   comment_line_str);\n+\t\tstrbuf_add_comment_lines(tagbuf, sig->buf, sig->len);\n \t}\n }\n \n@@ -556,19 +555,17 @@ static void fmt_merge_msg_sigs(struct strbuf *out)\n \t\t\tif (tag_number == 2) {\n \t\t\t\tstruct strbuf tagline = STRBUF_INIT;\n \t\t\t\tstrbuf_addch(&tagline, '\\n');\n-\t\t\t\tstrbuf_add_commented_lines(&tagline,\n-\t\t\t\t\t\torigins.items[first_tag].string,\n-\t\t\t\t\t\tstrlen(origins.items[first_tag].string),\n-\t\t\t\t\t\tcomment_line_str);\n+\t\t\t\tstrbuf_add_comment_lines(&tagline,\n+\t\t\t\t\t\t\t origins.items[first_tag].string,\n+\t\t\t\t\t\t\t strlen(origins.items[first_tag].string));\n \t\t\t\tstrbuf_insert(&tagbuf, 0, tagline.buf,\n \t\t\t\t\t      tagline.len);\n \t\t\t\tstrbuf_release(&tagline);\n \t\t\t}\n \t\t\tstrbuf_addch(&tagbuf, '\\n');\n-\t\t\tstrbuf_add_commented_lines(&tagbuf,\n-\t\t\t\t\torigins.items[i].string,\n-\t\t\t\t\tstrlen(origins.items[i].string),\n-\t\t\t\t\tcomment_line_str);\n+\t\t\tstrbuf_add_comment_lines(&tagbuf,\n+\t\t\t\t\t\t origins.items[i].string,\n+\t\t\t\t\t\t strlen(origins.items[i].string));\n \t\t\tfmt_tag_signature(&tagbuf, &sig, buf, len);\n \t\t}\n \t\tstrbuf_release(&payload);\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex dd2ec363d8..1670c51fef 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -80,7 +80,7 @@ void append_todo_help(int command_count,\n \t\t\t\t      shortrevisions, shortonto, command_count);\n \t}\n \n-\tstrbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);\n+\tstrbuf_add_comment_lines(buf, msg, strlen(msg));\n \n \tif (get_missing_commit_check_level() == MISSING_COMMIT_CHECK_ERROR)\n \t\tmsg = _(\"\\nDo not remove any line. Use 'drop' \"\n@@ -89,7 +89,7 @@ void append_todo_help(int command_count,\n \t\tmsg = _(\"\\nIf you remove a line here \"\n \t\t\t \"THAT COMMIT WILL BE LOST.\\n\");\n \n-\tstrbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);\n+\tstrbuf_add_comment_lines(buf, msg, strlen(msg));\n \n \tif (edit_todo)\n \t\tmsg = _(\"\\nYou are editing the todo file \"\n@@ -100,7 +100,7 @@ void append_todo_help(int command_count,\n \t\tmsg = _(\"\\nHowever, if you remove everything, \"\n \t\t\t\"the rebase will be aborted.\\n\\n\");\n \n-\tstrbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);\n+\tstrbuf_add_comment_lines(buf, msg, strlen(msg));\n }\n \n int edit_todo_list(struct repository *r, struct replay_opts *opts,\ndiff --git a/sequencer.c b/sequencer.c\nindex bf844ce98e..77872d02f4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1911,7 +1911,7 @@ static int is_fixup_flag(enum todo_command command, unsigned flag)\n }\n \n /*\n- * Wrapper around strbuf_add_commented_lines() which avoids double\n+ * Wrapper around strbuf_add_comment_lines() which avoids double\n  * commenting commit subjects.\n  */\n static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)\n@@ -1928,7 +1928,7 @@ static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)\n \t\ts += count;\n \t\tlen -= count;\n \t}\n-\tstrbuf_add_commented_lines(buf, s, len, comment_line_str);\n+\tstrbuf_add_comment_lines(buf, s, len);\n }\n \n /* Does the current fixup chain contain a squash command? */\n@@ -2028,7 +2028,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,\n \tstrbuf_addf(buf, _(nth_commit_msg_fmt),\n \t\t    ++ctx->current_fixup_count + 1);\n \tstrbuf_addstr(buf, \"\\n\\n\");\n-\tstrbuf_add_commented_lines(buf, body, commented_len, comment_line_str);\n+\tstrbuf_add_comment_lines(buf, body, commented_len);\n \t/* buf->buf may be reallocated so store an offset into the buffer */\n \tfixup_off = buf->len;\n \tstrbuf_addstr(buf, body + commented_len);\n@@ -2119,8 +2119,7 @@ static int update_squash_messages(struct repository *r,\n \t\t\t      _(first_commit_msg_str));\n \t\tstrbuf_addstr(&buf, \"\\n\\n\");\n \t\tif (is_fixup_flag(command, flag))\n-\t\t\tstrbuf_add_commented_lines(&buf, body, strlen(body),\n-\t\t\t\t\t\t   comment_line_str);\n+\t\t\tstrbuf_add_comment_lines(&buf, body, strlen(body));\n \t\telse\n \t\t\tstrbuf_addstr(&buf, body);\n \n@@ -2139,8 +2138,7 @@ static int update_squash_messages(struct repository *r,\n \t\tstrbuf_addf(&buf, _(skip_nth_commit_msg_fmt),\n \t\t\t    ++ctx->current_fixup_count + 1);\n \t\tstrbuf_addstr(&buf, \"\\n\\n\");\n-\t\tstrbuf_add_commented_lines(&buf, body, strlen(body),\n-\t\t\t\t\t   comment_line_str);\n+\t\tstrbuf_add_comment_lines(&buf, body, strlen(body));\n \t} else\n \t\treturn error(_(\"unknown command: %d\"), command);\n \trepo_unuse_commit_buffer(r, commit, message);\ndiff --git a/strbuf.c b/strbuf.c\nindex 6c525da7a6..3dfd25d94d 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -379,10 +379,9 @@ static void add_lines(struct strbuf *out,\n \tstrbuf_complete_line(out);\n }\n \n-void strbuf_add_commented_lines(struct strbuf *out, const char *buf,\n-\t\t\t\tsize_t size, const char *comment_prefix)\n+void strbuf_add_comment_lines(struct strbuf *out, const char *buf, size_t size)\n {\n-\tadd_lines(out, comment_prefix, buf, size, 1);\n+\tadd_lines(out, comment_line_str, buf, size, 1);\n }\n \n void strbuf_comment_addf(struct strbuf *sb, const char *fmt, ...)\n@@ -395,7 +394,7 @@ void strbuf_comment_addf(struct strbuf *sb, const char *fmt, ...)\n \tstrbuf_vaddf(&buf, fmt, params);\n \tva_end(params);\n \n-\tstrbuf_add_commented_lines(sb, buf.buf, buf.len, comment_line_str);\n+\tstrbuf_add_comment_lines(sb, buf.buf, buf.len);\n \tif (incomplete_line)\n \t\tsb->buf[--sb->len] = '\\0';\n \ndiff --git a/strbuf.h b/strbuf.h\nindex aebc8020ae..fcb38c3905 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -284,12 +284,9 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,\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+ * by the comment character and a blank.\n  */\n-void strbuf_add_commented_lines(struct strbuf *out,\n-\t\t\t\tconst char *buf, size_t size,\n-\t\t\t\tconst char *comment_prefix);\n-\n+void strbuf_add_comment_lines(struct strbuf *out, const char *buf, size_t size);\n \n /**\n  * Add data of given length to the buffer.\ndiff --git a/wt-status.c b/wt-status.c\nindex 44d29b721f..b94ee1f727 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1033,7 +1033,7 @@ static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncom\n \tif (s->display_comment_prefix) {\n \t\tsize_t len;\n \t\tsummary_content = strbuf_detach(&summary, &len);\n-\t\tstrbuf_add_commented_lines(&summary, summary_content, len, comment_line_str);\n+\t\tstrbuf_add_comment_lines(&summary, summary_content, len);\n \t\tfree(summary_content);\n \t}\n \n@@ -1112,7 +1112,7 @@ void wt_status_append_cut_line(struct strbuf *buf)\n \tconst char *explanation = _(\"Do not modify or remove the line above.\\nEverything below it will be ignored.\");\n \n \tstrbuf_comment_addf(buf,  \"%s\", cut_line);\n-\tstrbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_str);\n+\tstrbuf_add_comment_lines(buf, explanation, strlen(explanation));\n }\n \n void wt_status_add_cut_line(struct wt_status *s)\n-- \n2.46.0-717-g3841ff3f09\n\n"},{"id":"502722","messageId":"xmqq8qvw357v.fsf@gitster.g","threadId":"62102","inReplyTo":"20240912205301.1809355-1-gitster@pobox.com","subject":"Re: [PATCH 0/2] Simplify \"commented\" API functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-12T21:57:40Z","receivedAt":"2024-09-12T21:57:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> [earlier] https://lore.kernel.org/git/xmqq7cn4g3nx.fsf@gitster.g/\n>\n> Junio C Hamano (2):\n>   strbuf: retire strbuf_commented_addf()\n>   strbuf: retire strbuf_commented_lines()\n\nThis of course has a semantic conflict with the change that hides\nnot just \"the_repository\" and functions that implicitly use that\nvariable but other unrelated globals from environment.h behind a\ncompatibility macro.\n\nA trivial merge-fix to be used to annotate the merge to make it an\nevil merge whose result compiles looks like this:\n\n    merge-fix/jc/strbuf-commented-something\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 05a6dc944b..cb972b07f7 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -7,6 +7,14 @@\n #include \"utf8.h\"\n #include \"date.h\"\n \n+/*\n+ * We do not need the_repository at all, but the comment_line_str is\n+ * hidden behind USE_THE_REPOSITORY_VARIABLE CPP macro.  We could\n+ * #define it before including \"envirnoment.h\".  Or we can make an\n+ * external declaration ourselves here.\n+ */\n+extern const char *comment_line_str;\n+\n int starts_with(const char *str, const char *prefix)\n {\n \tfor (; ; str++, prefix++)\n"},{"id":"502734","messageId":"ZuPYZW3jYas4kJzC@pks.im","threadId":"62102","inReplyTo":"20240912205301.1809355-1-gitster@pobox.com","subject":"Re: [PATCH 0/2] Simplify \"commented\" API functions","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-13T06:15:01Z","receivedAt":"2024-09-13T06:15:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 12, 2024 at 01:52:59PM -0700, Junio C Hamano wrote:\n> We [earlier] noticed that a few helper functions that format strings\n> into a strbuf, prefixed with an arbitrary comment leading character,\n> are forcing their callers to always pass the same argument.  Instead\n> of keeping this excess flexibility nobody wants in practice, narrow\n> the API so that all callers of these functions will get the same\n> comment leading character.\n> \n> Superficially it may appear that this goes against one of the recent\n> trend, \"war on global variables\", but I doubt it matters much in the\n> longer run.\n\nI'm not quite sure I agree. The comment strings we have are in theory\nbroken, because they can be configured differently per repository. So if\nyou happen to have a Git command that operates on multiple repositories\nat once and that needs to pay attention to that config then it would now\nuse the same comment character for both repositories, which I'd argue is\nthe wrong thing to do.\n\nThe recent patch series that makes \"environment.c\" aware of\n`USE_THE_REPOSITORY_VARIABLE` already converted some of the global\nconfig variables to be per-repository, because ultimately all of them\nare broken in the described way. So from my point of view we should aim\nto convert remaining ones to be per-repository, as well.\n\n> These call sites all have already been relying on the global\n> \"comment_line_str\" anyway, and passing the variable from the top of\n> arbitrary deep call chain, which may not care specifically about\n> this single variable comment_line_str, would not scale as a\n> solution.  If we were to make it a convention to pass something from\n> the very top of the call chain everywhere, it should not be an\n> individual variable that is overly specific, like comment_line_str.\n> Rather, it has to be something that allows access to a bag of all\n> the global settings (possibly wider than \"the_repository\" struct).\n> We can also limit our reliance to the globals by allowing a single\n> global function call to obtaion such a bag of all global settings,\n> i.e. \"get_the_environment()\", and allow everybody, including\n> functions at the leaf level, to ask \"what is the comment_line_str in\n> the environment I am working in?\".  As part of the libification, we\n> can plug in an appropriate shim for get_the_environment().\n\nThis here I can definitely agree with. I think the best fit we have in\ngeneral is the \"repo-settings.c\" subsystem, which is also where I've\nmoved some of the previously-global settings into. It still has some\ndownsides in its current format:\n\n  - It depends on a repository, but I'd argue it shouldn't such that we\n    can also query configuration in repo-less settings.\n\n  - `prepare_repo_settings()` makes up for a bad calling convention. I\n    think that lazy accessors are way easier to use.\n\nBut it is a start, and something we can continue to build on.\n\nPatrick\n"},{"id":"502765","messageId":"xmqqjzffzcvk.fsf@gitster.g","threadId":"62102","inReplyTo":"ZuPYZW3jYas4kJzC@pks.im","subject":"Re: [PATCH 0/2] Simplify \"commented\" API functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-13T17:23:27Z","receivedAt":"2024-09-13T17:25:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I'm not quite sure I agree. The comment strings we have are in theory\n> broken, because they can be configured differently per repository. So if\n> you happen to have a Git command that operates on multiple repositories\n> at once and that needs to pay attention to that config then it would now\n> use the same comment character for both repositories, which I'd argue is\n> the wrong thing to do.\n\nCorrect.  It needs a move from global to a member in a repository\ninstance, but the same \"hey do not keep passing the same parameter\"\nmotivation behind these patches applies, as the existing call sites\nmost likely will instead pass \"the_repository->comment_line_str\" to\nthese two functions.  The simplification would move that reference\nto \"the_repository->comment_line_str\" down to these two functions.\n\n> The recent patch series that makes \"environment.c\" aware of\n> `USE_THE_REPOSITORY_VARIABLE` already converted some of the global\n> config variables to be per-repository, because ultimately all of them\n> are broken in the described way. So from my point of view we should aim\n> to convert remaining ones to be per-repository, as well.\n\nYes, I view the environement change somewhat incomplete and it was\nannoying to see things other than the_repository itself and those\nthat implicitly refer to the_repository covered by the CPP macro.\n\nBut we need to step back a bit in order to make the environment\nchange better.  Not everything works inside a repository and you may\nnot even have a repository but want to refer to a comment character\n(say, \"git bugreport\" run outside a repository, perhaps, and the\nbugreport may want to honor end-user configuration for commentChar\nto mark its various sections).  Earlier I said it may make sense to\nreimplement the global as a member of a repository instance, but\nthat is not entirely true.  Such a member in a repository struct may\nbe a good implementation detail for anybody who asks \"what comment\ncharacter should I be using in the context I am called?\", and there\nmay be \"const char *get_comment_line_str(struct repository *)\" that\naccepts which repository to work with, but such a function would\nneed to be prepared to work without any repository, working out of\nthe system and per-user configuration files.\n\n>   - It depends on a repository, but I'd argue it shouldn't such that we\n>     can also query configuration in repo-less settings.\n>\n>   - `prepare_repo_settings()` makes up for a bad calling convention. I\n>     think that lazy accessors are way easier to use.\n>\n> But it is a start, and something we can continue to build on.\n\nYup.\n"},{"id":"506302","messageId":"7a0520b4-2530-4660-a858-1def0c8d6de2@app.fastmail.com","threadId":"62102","inReplyTo":"20240912205301.1809355-3-gitster@pobox.com","subject":"Re: [PATCH 2/2] strbuf: retire strbuf_commented_lines()","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-29T20:53:37Z","receivedAt":"2024-10-29T20:53:59Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Thu, Sep 12, 2024, at 22:53, Junio C Hamano wrote:\n> The API allows the caller to pass any string as the comment prefix,\n> but in reality, everybody passes the comment_line_str from the\n> environment.\n>\n> Replace this overly flexible API with strbuf_comment_lines() that\n> does not allow customization of the comment_line_str (usually '#'\n> but can be configured via the core.commentChar).\n\ncore.commentChar or core.commentString.\n\n-- \nKristoffer Haugsbakk\n\n"}]}