[RFC,2/3] strbuf_commented_addf(): drop the comment_line_char parameter

Commit Message

Jonathan Tan Oct. 30, 2023, 8:22 p.m. UTC

From: Junio C Hamano <[e-mail]>

All the callers of this function supply the global variable
comment_line_char as an argument to its second parameter.  Remove
the parameter to allow us in the future to change the reference to
the global variable with something else, like a function call.

Signed-off-by: Junio C Hamano <[e-mail]>
Signed-off-by: Jonathan Tan <[e-mail]>
---
 add-patch.c          |  8 ++++----
 builtin/branch.c     |  2 +-
 builtin/merge.c      |  8 ++++----
 builtin/tag.c        |  4 ++--
 environment.c        | 18 ++++++++++++++++++
 environment.h        |  7 +++++++
 rebase-interactive.c |  2 +-
 sequencer.c          |  4 ++--
 strbuf.c             | 19 +------------------
 strbuf.h             |  7 -------
 wt-status.c          |  2 +-
 11 files changed, 41 insertions(+), 40 deletions(-)

Comments

Junio C Hamano Oct. 31, 2023, 5:19 a.m. UTC | #1

Jonathan Tan <[e-mail]> writes:

> From: Junio C Hamano <[e-mail]>
>
> All the callers of this function supply the global variable
> comment_line_char as an argument to its second parameter.  Remove
> the parameter to allow us in the future to change the reference to
> the global variable with something else, like a function call.
>
> Signed-off-by: Junio C Hamano <[e-mail]>
> Signed-off-by: Jonathan Tan <[e-mail]>
> ---

> diff --git a/environment.c b/environment.c
> index bb3c2a96a3..d9f64cffa0 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -416,3 +416,21 @@ int print_sha1_ellipsis(void)
>  	}
>  	return cached_result;
>  }
> +
> +void strbuf_commented_addf(struct strbuf *sb,
> +			   const char *fmt, ...)
> +{
> +	va_list params;
> +	struct strbuf buf = STRBUF_INIT;
> +	int incomplete_line = sb->len && sb->buf[sb->len - 1] != '\n';
> +
> +	va_start(params, fmt);
> +	strbuf_vaddf(&buf, fmt, params);
> +	va_end(params);
> +
> +	strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_line_char);
> +	if (incomplete_line)
> +		sb->buf[--sb->len] = '\0';
> +
> +	strbuf_release(&buf);
> +}

This moving of the helper function does not belong to the "fix
commented_addf() not to take the comment_line_char" step.

The series should be restructured to have the two patches from me
first, and then your moving some stuff to environment.c, probably.

Jonathan Tan Oct. 31, 2023, 10:24 p.m. UTC | #2

Junio C Hamano <[e-mail]> writes:
> This moving of the helper function does not belong to the "fix
> commented_addf() not to take the comment_line_char" step.
> 
> The series should be restructured to have the two patches from me
> first, and then your moving some stuff to environment.c, probably.

This means that #include "environment.h" will be added and then removed
in the same series, but I don't feel too strongly about that. I'll send
an updated set of patches.

Junio C Hamano Oct. 31, 2023, 11:54 p.m. UTC | #3

Jonathan Tan <[e-mail]> writes:

> Junio C Hamano <[e-mail]> writes:
>> This moving of the helper function does not belong to the "fix
>> commented_addf() not to take the comment_line_char" step.
>> 
>> The series should be restructured to have the two patches from me
>> first, and then your moving some stuff to environment.c, probably.
>
> This means that #include "environment.h" will be added and then removed
> in the same series,

I do prefer it that way, because that is exactly what we are doing.
First we fix the duplicated parameter in the API by relying on the
global, and then we move things around to hide the dependence on the
global.

Thanks.

Patch

diff --git a/add-patch.c b/add-patch.c
index bfe19876cd..471a0037be 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1106,11 +1106,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_char,
+	strbuf_commented_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_char,
+	strbuf_commented_addf(&s->buf,
 			      _("---\n"
 				"To remove '%c' lines, make them ' ' lines "
 				"(context).\n"
@@ -1119,13 +1119,13 @@  static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
 			      s->mode->is_reverse ? '+' : '-',
 			      s->mode->is_reverse ? '-' : '+',
 			      comment_line_char);
-	strbuf_commented_addf(&s->buf, comment_line_char, "%s",
+	strbuf_commented_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_char,
+	strbuf_commented_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 2ec190b14a..b2f171e10b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -668,7 +668,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_char,
+	strbuf_commented_addf(&buf,
 		    _("Please edit the description for the branch\n"
 		      "  %s\n"
 		      "Lines starting with '%c' will be stripped.\n"),
diff --git a/builtin/merge.c b/builtin/merge.c
index d748d46e13..8f0e8be7c3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -857,15 +857,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_char, "\n");
+			strbuf_commented_addf(&msg, "\n");
 		}
-		strbuf_commented_addf(&msg, comment_line_char,
+		strbuf_commented_addf(&msg,
 				      _(merge_editor_comment));
 		if (cleanup_mode == COMMIT_MSG_CLEANUP_SCISSORS)
-			strbuf_commented_addf(&msg, comment_line_char,
+			strbuf_commented_addf(&msg,
 					      _(scissors_editor_comment));
 		else
-			strbuf_commented_addf(&msg, comment_line_char,
+			strbuf_commented_addf(&msg,
 				_(no_scissors_editor_comment), comment_line_char);
 	}
 	if (signoff)
diff --git a/builtin/tag.c b/builtin/tag.c
index 3918eacbb5..a85a0d8def 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -314,10 +314,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_char,
+				strbuf_commented_addf(&buf,
 				      _(tag_template), tag, comment_line_char);
 			else
-				strbuf_commented_addf(&buf, comment_line_char,
+				strbuf_commented_addf(&buf,
 				      _(tag_template_nocleanup), tag, comment_line_char);
 			write_or_die(fd, buf.buf, buf.len);
 			strbuf_release(&buf);
diff --git a/environment.c b/environment.c
index bb3c2a96a3..d9f64cffa0 100644
--- a/environment.c
+++ b/environment.c
@@ -416,3 +416,21 @@  int print_sha1_ellipsis(void)
 	}
 	return cached_result;
 }
+
+void strbuf_commented_addf(struct strbuf *sb,
+			   const char *fmt, ...)
+{
+	va_list params;
+	struct strbuf buf = STRBUF_INIT;
+	int incomplete_line = sb->len && sb->buf[sb->len - 1] != '\n';
+
+	va_start(params, fmt);
+	strbuf_vaddf(&buf, fmt, params);
+	va_end(params);
+
+	strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_line_char);
+	if (incomplete_line)
+		sb->buf[--sb->len] = '\0';
+
+	strbuf_release(&buf);
+}
diff --git a/environment.h b/environment.h
index e5351c9dd9..5778f5a8e4 100644
--- a/environment.h
+++ b/environment.h
@@ -229,4 +229,11 @@  extern const char *excludes_file;
  */
 int print_sha1_ellipsis(void);
 
+/**
+ * Add a formatted string prepended by a comment character and a
+ * blank to the buffer.
+ */
+__attribute__((format (printf, 2, 3)))
+void strbuf_commented_addf(struct strbuf *sb, const char *fmt, ...);
+
 #endif
diff --git a/rebase-interactive.c b/rebase-interactive.c
index d9718409b3..3f33da7f03 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -71,7 +71,7 @@  void append_todo_help(int command_count,
 
 	if (!edit_todo) {
 		strbuf_addch(buf, '\n');
-		strbuf_commented_addf(buf, comment_line_char,
+		strbuf_commented_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 d584cac8ed..5d348a3f12 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -675,11 +675,11 @@  void append_conflicts_hint(struct index_state *istate,
 	}
 
 	strbuf_addch(msgbuf, '\n');
-	strbuf_commented_addf(msgbuf, comment_line_char, "Conflicts:\n");
+	strbuf_commented_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_char,
+			strbuf_commented_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 9ee639519a..b1717270a2 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"
@@ -352,24 +353,6 @@  void strbuf_add_commented_lines(struct strbuf *out, const char *buf,
 	strbuf_add_lines(out, prefix1, prefix2, buf, size);
 }
 
-void strbuf_commented_addf(struct strbuf *sb, char comment_line_char,
-			   const char *fmt, ...)
-{
-	va_list params;
-	struct strbuf buf = STRBUF_INIT;
-	int incomplete_line = sb->len && sb->buf[sb->len - 1] != '\n';
-
-	va_start(params, fmt);
-	strbuf_vaddf(&buf, fmt, params);
-	va_end(params);
-
-	strbuf_add_commented_lines(sb, buf.buf, buf.len, comment_line_char);
-	if (incomplete_line)
-		sb->buf[--sb->len] = '\0';
-
-	strbuf_release(&buf);
-}
-
 void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
 {
 	int len;
diff --git a/strbuf.h b/strbuf.h
index 3559e73dd8..4c58dc25e9 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -374,13 +374,6 @@  void strbuf_humanise_rate(struct strbuf *buf, off_t bytes);
 __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
- * blank to the buffer.
- */
-__attribute__((format (printf, 3, 4)))
-void strbuf_commented_addf(struct strbuf *sb, char comment_line_char, 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 9f45bf6949..54b2775730 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1102,7 +1102,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_char, "%s", cut_line);
+	strbuf_commented_addf(buf, "%s", cut_line);
 	strbuf_add_commented_lines(buf, explanation, strlen(explanation), comment_line_char);
 }