[PATCH 0/2] Simplify "commented" API functions

[PATCH 0/2] Simplify "commented" API functions

[Junio C Hamano]

We [earlier] noticed that a few helper functions that format strings
into a strbuf, prefixed with an arbitrary comment leading character,
are forcing their callers to always pass the same argument.  Instead
of keeping this excess flexibility nobody wants in practice, narrow
the API so that all callers of these functions will get the same
comment leading character.

Superficially it may appear that this goes against one of the recent
trend, "war on global variables", but I doubt it matters much in the
longer run.

These call sites all have already been relying on the global
"comment_line_str" anyway, and passing the variable from the top of
arbitrary deep call chain, which may not care specifically about
this single variable comment_line_str, would not scale as a
solution.  If we were to make it a convention to pass something from
the very top of the call chain everywhere, it should not be an
individual variable that is overly specific, like comment_line_str.
Rather, it has to be something that allows access to a bag of all
the global settings (possibly wider than "the_repository" struct).
We can also limit our reliance to the globals by allowing a single
global function call to obtaion such a bag of all global settings,
i.e. "get_the_environment()", and allow everybody, including
functions at the leaf level, to ask "what is the comment_line_str in
the environment I am working in?".  As part of the libification, we
can plug in an appropriate shim for get_the_environment().


[earlier] https://lore.kernel.org/git/xmqq7cn4g3nx.fsf@gitster.g/

Junio C Hamano (2):
  strbuf: retire strbuf_commented_addf()
  strbuf: retire strbuf_commented_lines()

 add-patch.c          |  8 ++++----
 builtin/branch.c     |  2 +-
 builtin/merge.c      |  8 ++++----
 builtin/notes.c      | 10 +++++-----
 builtin/stripspace.c |  2 +-
 builtin/tag.c        |  4 ++--
 fmt-merge-msg.c      | 17 +++++++----------
 rebase-interactive.c |  8 ++++----
 sequencer.c          | 16 +++++++---------
 strbuf.c             | 11 +++++------
 strbuf.h             | 13 +++++--------
 wt-status.c          |  6 +++---
 12 files changed, 48 insertions(+), 57 deletions(-)

-- 
2.46.0-717-g3841ff3f09

[PATCH 1/2] strbuf: retire strbuf_commented_addf()

[Junio C Hamano]

The API allows the caller to pass any string as the comment prefix,
but in reality, everybody passes the comment_line_str from the
environment.

Replace this overly flexible API with strbuf_comment_addf() that
does not allow customization of the comment_line_str (usually '#'
but can be configured via the core.commentChar).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 add-patch.c          | 8 ++++----
 builtin/branch.c     | 2 +-
 builtin/merge.c      | 8 ++++----
 builtin/tag.c        | 4 ++--
 rebase-interactive.c | 2 +-
 sequencer.c          | 4 ++--
 strbuf.c             | 6 +++---
 strbuf.h             | 6 +++---
 wt-status.c          | 2 +-
 9 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 46f6bddfe5..32c990cc18 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1114,11 +1114,11 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
 	size_t i;
 
 	strbuf_reset(&s->buf);
-	strbuf_commented_addf(&s->buf, comment_line_str,
+	strbuf_comment_addf(&s->buf,
 			      _("Manual hunk edit mode -- see bottom for "
 				"a quick guide.\n"));
 	render_hunk(s, hunk, 0, 0, &s->buf);
-	strbuf_commented_addf(&s->buf, comment_line_str,
+	strbuf_comment_addf(&s->buf,
 			      _("---\n"
 				"To remove '%c' lines, make them ' ' lines "
 				"(context).\n"
@@ -1127,13 +1127,13 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
 			      s->mode->is_reverse ? '+' : '-',
 			      s->mode->is_reverse ? '-' : '+',
 			      comment_line_str);
-	strbuf_commented_addf(&s->buf, comment_line_str, "%s",
+	strbuf_comment_addf(&s->buf,  "%s",
 			      _(s->mode->edit_hunk_hint));
 	/*
 	 * TRANSLATORS: 'it' refers to the patch mentioned in the previous
 	 * messages.
 	 */
-	strbuf_commented_addf(&s->buf, comment_line_str,
+	strbuf_comment_addf(&s->buf,
 			      _("If it does not apply cleanly, you will be "
 				"given an opportunity to\n"
 				"edit again.  If all lines of the hunk are "
diff --git a/builtin/branch.c b/builtin/branch.c
index 48cac74f97..b2a4206e1b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -681,7 +681,7 @@ static int edit_branch_description(const char *branch_name)
 	exists = !read_branch_desc(&buf, branch_name);
 	if (!buf.len || buf.buf[buf.len-1] != '\n')
 		strbuf_addch(&buf, '\n');
-	strbuf_commented_addf(&buf, comment_line_str,
+	strbuf_comment_addf(&buf,
 		    _("Please edit the description for the branch\n"
 		      "  %s\n"
 		      "Lines starting with '%s' will be stripped.\n"),
diff --git a/builtin/merge.c b/builtin/merge.c
index 9fba27d85d..8794fea28f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -863,15 +863,15 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 		strbuf_addch(&msg, '\n');
 		if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS) {
 			wt_status_append_cut_line(&msg);
-			strbuf_commented_addf(&msg, comment_line_str, "\n");
+			strbuf_comment_addf(&msg,  "\n");
 		}
-		strbuf_commented_addf(&msg, comment_line_str,
+		strbuf_comment_addf(&msg,
 				      _(merge_editor_comment));
 		if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
-			strbuf_commented_addf(&msg, comment_line_str,
+			strbuf_comment_addf(&msg,
 					      _(scissors_editor_comment));
 		else
-			strbuf_commented_addf(&msg, comment_line_str,
+			strbuf_comment_addf(&msg,
 				_(no_scissors_editor_comment), comment_line_str);
 	}
 	if (signoff)
diff --git a/builtin/tag.c b/builtin/tag.c
index a1fb218512..0929cfc158 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -334,10 +334,10 @@ static void create_tag(const struct object_id *object, const char *object_ref,
 			struct strbuf buf = STRBUF_INIT;
 			strbuf_addch(&buf, '\n');
 			if (opt->cleanup_mode == CLEANUP_ALL)
-				strbuf_commented_addf(&buf, comment_line_str,
+				strbuf_comment_addf(&buf,
 				      _(tag_template), tag, comment_line_str);
 			else
-				strbuf_commented_addf(&buf, comment_line_str,
+				strbuf_comment_addf(&buf,
 				      _(tag_template_nocleanup), tag, comment_line_str);
 			write_or_die(fd, buf.buf, buf.len);
 			strbuf_release(&buf);
diff --git a/rebase-interactive.c b/rebase-interactive.c
index cbeb864147..dd2ec363d8 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -73,7 +73,7 @@ void append_todo_help(int command_count,
 
 	if (!edit_todo) {
 		strbuf_addch(buf, '\n');
-		strbuf_commented_addf(buf, comment_line_str,
+		strbuf_comment_addf(buf,
 				      Q_("Rebase %s onto %s (%d command)",
 					 "Rebase %s onto %s (%d commands)",
 					 command_count),
diff --git a/sequencer.c b/sequencer.c
index a2284ac9e9..bf844ce98e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -731,11 +731,11 @@ void append_conflicts_hint(struct index_state *istate,
 	}
 
 	strbuf_addch(msgbuf, '\n');
-	strbuf_commented_addf(msgbuf, comment_line_str, "Conflicts:\n");
+	strbuf_comment_addf(msgbuf,  "Conflicts:\n");
 	for (i = 0; i < istate->cache_nr;) {
 		const struct cache_entry *ce = istate->cache[i++];
 		if (ce_stage(ce)) {
-			strbuf_commented_addf(msgbuf, comment_line_str,
+			strbuf_comment_addf(msgbuf,
 					      "\t%s\n", ce->name);
 			while (i < istate->cache_nr &&
 			       !strcmp(ce->name, istate->cache[i]->name))
diff --git a/strbuf.c b/strbuf.c
index 3d2189a7f6..6c525da7a6 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,4 +1,5 @@
 #include "git-compat-util.h"
+#include "environment.h"
 #include "gettext.h"
 #include "hex-ll.h"
 #include "strbuf.h"
@@ -384,8 +385,7 @@ void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
 	add_lines(out, comment_prefix, buf, size, 1);
 }
 
-void strbuf_commented_addf(struct strbuf *sb, const char *comment_prefix,
-			   const char *fmt, ...)
+void strbuf_comment_addf(struct strbuf *sb, const char *fmt, ...)
 {
 	va_list params;
 	struct strbuf buf = STRBUF_INIT;
@@ -395,7 +395,7 @@ void strbuf_commented_addf(struct strbuf *sb, const char *comment_prefix,
 	strbuf_vaddf(&buf, fmt, params);
 	va_end(params);
 
-	strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_prefix);
+	strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_line_str);
 	if (incomplete_line)
 		sb->buf[--sb->len] = '\0';
 
diff --git a/strbuf.h b/strbuf.h
index 003f880ff7..aebc8020ae 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -385,11 +385,11 @@ __attribute__((format (printf,2,3)))
 void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
 
 /**
- * Add a formatted string prepended by a comment character and a
+ * Add a formatted string prepended by the comment character and a
  * blank to the buffer.
  */
-__attribute__((format (printf, 3, 4)))
-void strbuf_commented_addf(struct strbuf *sb, const char *comment_prefix, const char *fmt, ...);
+__attribute__((format (printf,2,3)))
+void strbuf_comment_addf(struct strbuf *sb, const char *fmt, ...);
 
 __attribute__((format (printf,2,0)))
 void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
diff --git a/wt-status.c b/wt-status.c
index b778eef989..44d29b721f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1111,7 +1111,7 @@ void wt_status_append_cut_line(struct strbuf *buf)
 {
 	const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
 
-	strbuf_commented_addf(buf, comment_line_str, "%s", cut_line);
+	strbuf_comment_addf(buf,  "%s", cut_line);
 	strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_str);
 }
 
-- 
2.46.0-717-g3841ff3f09

[PATCH 2/2] strbuf: retire strbuf_commented_lines()

[Junio C Hamano]

The API allows the caller to pass any string as the comment prefix,
but in reality, everybody passes the comment_line_str from the
environment.

Replace this overly flexible API with strbuf_comment_lines() that
does not allow customization of the comment_line_str (usually '#'
but can be configured via the core.commentChar).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/notes.c      | 10 +++++-----
 builtin/stripspace.c |  2 +-
 fmt-merge-msg.c      | 17 +++++++----------
 rebase-interactive.c |  6 +++---
 sequencer.c          | 12 +++++-------
 strbuf.c             |  7 +++----
 strbuf.h             |  7 ++-----
 wt-status.c          |  4 ++--
 8 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 4cc5bfedc3..8c4d7f6f89 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -178,7 +178,7 @@ static void write_commented_object(int fd, const struct object_id *object)
 
 	if (strbuf_read(&buf, show.out, 0) < 0)
 		die_errno(_("could not read 'show' output"));
-	strbuf_add_commented_lines(&cbuf, buf.buf, buf.len, comment_line_str);
+	strbuf_add_comment_lines(&cbuf, buf.buf, buf.len);
 	write_or_die(fd, cbuf.buf, cbuf.len);
 
 	strbuf_release(&cbuf);
@@ -206,10 +206,10 @@ static void prepare_note_data(const struct object_id *object, struct note_data *
 			copy_obj_to_fd(fd, old_note);
 
 		strbuf_addch(&buf, '\n');
-		strbuf_add_commented_lines(&buf, "\n", strlen("\n"), comment_line_str);
-		strbuf_add_commented_lines(&buf, _(note_template), strlen(_(note_template)),
-					   comment_line_str);
-		strbuf_add_commented_lines(&buf, "\n", strlen("\n"), comment_line_str);
+		strbuf_add_comment_lines(&buf, "\n", strlen("\n"));
+		strbuf_add_comment_lines(&buf, _(note_template),
+					 strlen(_(note_template)));
+		strbuf_add_comment_lines(&buf, "\n", strlen("\n"));
 		write_or_die(fd, buf.buf, buf.len);
 
 		write_commented_object(fd, object);
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index e5626e5126..7be99a2ec0 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -13,7 +13,7 @@ static void comment_lines(struct strbuf *buf)
 	size_t len;
 
 	msg = strbuf_detach(buf, &len);
-	strbuf_add_commented_lines(buf, msg, len, comment_line_str);
+	strbuf_add_comment_lines(buf, msg, len);
 	free(msg);
 }
 
diff --git a/fmt-merge-msg.c b/fmt-merge-msg.c
index 6acb37b480..19d7dd10a0 100644
--- a/fmt-merge-msg.c
+++ b/fmt-merge-msg.c
@@ -511,8 +511,7 @@ static void fmt_tag_signature(struct strbuf *tagbuf,
 	strbuf_complete_line(tagbuf);
 	if (sig->len) {
 		strbuf_addch(tagbuf, '\n');
-		strbuf_add_commented_lines(tagbuf, sig->buf, sig->len,
-					   comment_line_str);
+		strbuf_add_comment_lines(tagbuf, sig->buf, sig->len);
 	}
 }
 
@@ -556,19 +555,17 @@ static void fmt_merge_msg_sigs(struct strbuf *out)
 			if (tag_number == 2) {
 				struct strbuf tagline = STRBUF_INIT;
 				strbuf_addch(&tagline, '\n');
-				strbuf_add_commented_lines(&tagline,
-						origins.items[first_tag].string,
-						strlen(origins.items[first_tag].string),
-						comment_line_str);
+				strbuf_add_comment_lines(&tagline,
+							 origins.items[first_tag].string,
+							 strlen(origins.items[first_tag].string));
 				strbuf_insert(&tagbuf, 0, tagline.buf,
 					      tagline.len);
 				strbuf_release(&tagline);
 			}
 			strbuf_addch(&tagbuf, '\n');
-			strbuf_add_commented_lines(&tagbuf,
-					origins.items[i].string,
-					strlen(origins.items[i].string),
-					comment_line_str);
+			strbuf_add_comment_lines(&tagbuf,
+						 origins.items[i].string,
+						 strlen(origins.items[i].string));
 			fmt_tag_signature(&tagbuf, &sig, buf, len);
 		}
 		strbuf_release(&payload);
diff --git a/rebase-interactive.c b/rebase-interactive.c
index dd2ec363d8..1670c51fef 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -80,7 +80,7 @@ void append_todo_help(int command_count,
 				      shortrevisions, shortonto, command_count);
 	}
 
-	strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);
+	strbuf_add_comment_lines(buf, msg, strlen(msg));
 
 	if (get_missing_commit_check_level() == MISSING_COMMIT_CHECK_ERROR)
 		msg = _("\nDo not remove any line. Use 'drop' "
@@ -89,7 +89,7 @@ void append_todo_help(int command_count,
 		msg = _("\nIf you remove a line here "
 			 "THAT COMMIT WILL BE LOST.\n");
 
-	strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);
+	strbuf_add_comment_lines(buf, msg, strlen(msg));
 
 	if (edit_todo)
 		msg = _("\nYou are editing the todo file "
@@ -100,7 +100,7 @@ void append_todo_help(int command_count,
 		msg = _("\nHowever, if you remove everything, "
 			"the rebase will be aborted.\n\n");
 
-	strbuf_add_commented_lines(buf, msg, strlen(msg), comment_line_str);
+	strbuf_add_comment_lines(buf, msg, strlen(msg));
 }
 
 int edit_todo_list(struct repository *r, struct replay_opts *opts,
diff --git a/sequencer.c b/sequencer.c
index bf844ce98e..77872d02f4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1911,7 +1911,7 @@ static int is_fixup_flag(enum todo_command command, unsigned flag)
 }
 
 /*
- * Wrapper around strbuf_add_commented_lines() which avoids double
+ * Wrapper around strbuf_add_comment_lines() which avoids double
  * commenting commit subjects.
  */
 static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)
@@ -1928,7 +1928,7 @@ static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)
 		s += count;
 		len -= count;
 	}
-	strbuf_add_commented_lines(buf, s, len, comment_line_str);
+	strbuf_add_comment_lines(buf, s, len);
 }
 
 /* Does the current fixup chain contain a squash command? */
@@ -2028,7 +2028,7 @@ static int append_squash_message(struct strbuf *buf, const char *body,
 	strbuf_addf(buf, _(nth_commit_msg_fmt),
 		    ++ctx->current_fixup_count + 1);
 	strbuf_addstr(buf, "\n\n");
-	strbuf_add_commented_lines(buf, body, commented_len, comment_line_str);
+	strbuf_add_comment_lines(buf, body, commented_len);
 	/* buf->buf may be reallocated so store an offset into the buffer */
 	fixup_off = buf->len;
 	strbuf_addstr(buf, body + commented_len);
@@ -2119,8 +2119,7 @@ static int update_squash_messages(struct repository *r,
 			      _(first_commit_msg_str));
 		strbuf_addstr(&buf, "\n\n");
 		if (is_fixup_flag(command, flag))
-			strbuf_add_commented_lines(&buf, body, strlen(body),
-						   comment_line_str);
+			strbuf_add_comment_lines(&buf, body, strlen(body));
 		else
 			strbuf_addstr(&buf, body);
 
@@ -2139,8 +2138,7 @@ static int update_squash_messages(struct repository *r,
 		strbuf_addf(&buf, _(skip_nth_commit_msg_fmt),
 			    ++ctx->current_fixup_count + 1);
 		strbuf_addstr(&buf, "\n\n");
-		strbuf_add_commented_lines(&buf, body, strlen(body),
-					   comment_line_str);
+		strbuf_add_comment_lines(&buf, body, strlen(body));
 	} else
 		return error(_("unknown command: %d"), command);
 	repo_unuse_commit_buffer(r, commit, message);
diff --git a/strbuf.c b/strbuf.c
index 6c525da7a6..3dfd25d94d 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -379,10 +379,9 @@ static void add_lines(struct strbuf *out,
 	strbuf_complete_line(out);
 }
 
-void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
-				size_t size, const char *comment_prefix)
+void strbuf_add_comment_lines(struct strbuf *out, const char *buf, size_t size)
 {
-	add_lines(out, comment_prefix, buf, size, 1);
+	add_lines(out, comment_line_str, buf, size, 1);
 }
 
 void strbuf_comment_addf(struct strbuf *sb, const char *fmt, ...)
@@ -395,7 +394,7 @@ void strbuf_comment_addf(struct strbuf *sb, const char *fmt, ...)
 	strbuf_vaddf(&buf, fmt, params);
 	va_end(params);
 
-	strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_line_str);
+	strbuf_add_comment_lines(sb, buf.buf, buf.len);
 	if (incomplete_line)
 		sb->buf[--sb->len] = '\0';
 
diff --git a/strbuf.h b/strbuf.h
index aebc8020ae..fcb38c3905 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -284,12 +284,9 @@ void strbuf_splice(struct strbuf *sb, size_t pos, size_t len,
 
 /**
  * Add a NUL-terminated string to the buffer. Each line will be prepended
- * by a comment character and a blank.
+ * by the comment character and a blank.
  */
-void strbuf_add_commented_lines(struct strbuf *out,
-				const char *buf, size_t size,
-				const char *comment_prefix);
-
+void strbuf_add_comment_lines(struct strbuf *out, const char *buf, size_t size);
 
 /**
  * Add data of given length to the buffer.
diff --git a/wt-status.c b/wt-status.c
index 44d29b721f..b94ee1f727 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1033,7 +1033,7 @@ static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncom
 	if (s->display_comment_prefix) {
 		size_t len;
 		summary_content = strbuf_detach(&summary, &len);
-		strbuf_add_commented_lines(&summary, summary_content, len, comment_line_str);
+		strbuf_add_comment_lines(&summary, summary_content, len);
 		free(summary_content);
 	}
 
@@ -1112,7 +1112,7 @@ void wt_status_append_cut_line(struct strbuf *buf)
 	const char *explanation = _("Do not modify or remove the line above.\nEverything below it will be ignored.");
 
 	strbuf_comment_addf(buf,  "%s", cut_line);
-	strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_str);
+	strbuf_add_comment_lines(buf, explanation, strlen(explanation));
 }
 
 void wt_status_add_cut_line(struct wt_status *s)
-- 
2.46.0-717-g3841ff3f09

Re: [PATCH 2/2] strbuf: retire strbuf_commented_lines()

[Kristoffer Haugsbakk]

On Thu, Sep 12, 2024, at 22:53, Junio C Hamano wrote:
> The API allows the caller to pass any string as the comment prefix,
> but in reality, everybody passes the comment_line_str from the
> environment.
>
> Replace this overly flexible API with strbuf_comment_lines() that
> does not allow customization of the comment_line_str (usually '#'
> but can be configured via the core.commentChar).

core.commentChar or core.commentString.

-- 
Kristoffer Haugsbakk

Re: [PATCH 0/2] Simplify "commented" API functions

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> [earlier] https://lore.kernel.org/git/xmqq7cn4g3nx.fsf@gitster.g/
>
> Junio C Hamano (2):
>   strbuf: retire strbuf_commented_addf()
>   strbuf: retire strbuf_commented_lines()

This of course has a semantic conflict with the change that hides
not just "the_repository" and functions that implicitly use that
variable but other unrelated globals from environment.h behind a
compatibility macro.

A trivial merge-fix to be used to annotate the merge to make it an
evil merge whose result compiles looks like this:

    merge-fix/jc/strbuf-commented-something

diff --git a/strbuf.c b/strbuf.c
index 05a6dc944b..cb972b07f7 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -7,6 +7,14 @@
 #include "utf8.h"
 #include "date.h"
 
+/*
+ * We do not need the_repository at all, but the comment_line_str is
+ * hidden behind USE_THE_REPOSITORY_VARIABLE CPP macro.  We could
+ * #define it before including "envirnoment.h".  Or we can make an
+ * external declaration ourselves here.
+ */
+extern const char *comment_line_str;
+
 int starts_with(const char *str, const char *prefix)
 {
 	for (; ; str++, prefix++)

Re: [PATCH 0/2] Simplify "commented" API functions

[Patrick Steinhardt]

On Thu, Sep 12, 2024 at 01:52:59PM -0700, Junio C Hamano wrote:
> We [earlier] noticed that a few helper functions that format strings
> into a strbuf, prefixed with an arbitrary comment leading character,
> are forcing their callers to always pass the same argument.  Instead
> of keeping this excess flexibility nobody wants in practice, narrow
> the API so that all callers of these functions will get the same
> comment leading character.
> 
> Superficially it may appear that this goes against one of the recent
> trend, "war on global variables", but I doubt it matters much in the
> longer run.

I'm not quite sure I agree. The comment strings we have are in theory
broken, because they can be configured differently per repository. So if
you happen to have a Git command that operates on multiple repositories
at once and that needs to pay attention to that config then it would now
use the same comment character for both repositories, which I'd argue is
the wrong thing to do.

The recent patch series that makes "environment.c" aware of
`USE_THE_REPOSITORY_VARIABLE` already converted some of the global
config variables to be per-repository, because ultimately all of them
are broken in the described way. So from my point of view we should aim
to convert remaining ones to be per-repository, as well.

> These call sites all have already been relying on the global
> "comment_line_str" anyway, and passing the variable from the top of
> arbitrary deep call chain, which may not care specifically about
> this single variable comment_line_str, would not scale as a
> solution.  If we were to make it a convention to pass something from
> the very top of the call chain everywhere, it should not be an
> individual variable that is overly specific, like comment_line_str.
> Rather, it has to be something that allows access to a bag of all
> the global settings (possibly wider than "the_repository" struct).
> We can also limit our reliance to the globals by allowing a single
> global function call to obtaion such a bag of all global settings,
> i.e. "get_the_environment()", and allow everybody, including
> functions at the leaf level, to ask "what is the comment_line_str in
> the environment I am working in?".  As part of the libification, we
> can plug in an appropriate shim for get_the_environment().

This here I can definitely agree with. I think the best fit we have in
general is the "repo-settings.c" subsystem, which is also where I've
moved some of the previously-global settings into. It still has some
downsides in its current format:

  - It depends on a repository, but I'd argue it shouldn't such that we
    can also query configuration in repo-less settings.

  - `prepare_repo_settings()` makes up for a bad calling convention. I
    think that lazy accessors are way easier to use.

But it is a start, and something we can continue to build on.

Patrick

Re: [PATCH 0/2] Simplify "commented" API functions

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I'm not quite sure I agree. The comment strings we have are in theory
> broken, because they can be configured differently per repository. So if
> you happen to have a Git command that operates on multiple repositories
> at once and that needs to pay attention to that config then it would now
> use the same comment character for both repositories, which I'd argue is
> the wrong thing to do.

Correct.  It needs a move from global to a member in a repository
instance, but the same "hey do not keep passing the same parameter"
motivation behind these patches applies, as the existing call sites
most likely will instead pass "the_repository->comment_line_str" to
these two functions.  The simplification would move that reference
to "the_repository->comment_line_str" down to these two functions.

> The recent patch series that makes "environment.c" aware of
> `USE_THE_REPOSITORY_VARIABLE` already converted some of the global
> config variables to be per-repository, because ultimately all of them
> are broken in the described way. So from my point of view we should aim
> to convert remaining ones to be per-repository, as well.

Yes, I view the environement change somewhat incomplete and it was
annoying to see things other than the_repository itself and those
that implicitly refer to the_repository covered by the CPP macro.

But we need to step back a bit in order to make the environment
change better.  Not everything works inside a repository and you may
not even have a repository but want to refer to a comment character
(say, "git bugreport" run outside a repository, perhaps, and the
bugreport may want to honor end-user configuration for commentChar
to mark its various sections).  Earlier I said it may make sense to
reimplement the global as a member of a repository instance, but
that is not entirely true.  Such a member in a repository struct may
be a good implementation detail for anybody who asks "what comment
character should I be using in the context I am called?", and there
may be "const char *get_comment_line_str(struct repository *)" that
accepts which repository to work with, but such a function would
need to be prepared to work without any repository, working out of
the system and per-user configuration files.

>   - It depends on a repository, but I'd argue it shouldn't such that we
>     can also query configuration in repo-less settings.
>
>   - `prepare_repo_settings()` makes up for a bad calling convention. I
>     think that lazy accessors are way easier to use.
>
> But it is a start, and something we can continue to build on.

Yup.