[PATCH 0/3] Remove sq_quote_print() in favor of *_buf

[PATCH 0/3] Remove sq_quote_print() in favor of *_buf

[Ramkumar Ramachandra]

Hi,

While going through the for-each-ref-pretty series that Duy and I were
developing, I noticed that this cleanup was independent and good
as-it-is.

So here it is.

Nguyễn Thái Ngọc Duy (1):
  for-each-ref, quote: convert *_quote_print -> *_quote_buf

Ramkumar Ramachandra (2):
  tar-tree: remove dependency on sq_quote_print()
  quote: remove sq_quote_print()

 builtin/for-each-ref.c | 13 +++++++----
 builtin/tar-tree.c     | 11 +++++----
 quote.c                | 61 ++++++++++++++++++--------------------------------
 quote.h                |  8 +++----
 4 files changed, 39 insertions(+), 54 deletions(-)

-- 
1.8.4.rc0.4.g4634265

[PATCH 1/3] for-each-ref, quote: convert *_quote_print -> *_quote_buf

[Ramkumar Ramachandra]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

for-each-ref.c:print_value() currently prints values to stdout
immediately using {sq|perl|python|tcl}_quote_print, giving us no
opportunity to do any further processing.  In preparation for getting
print_value() to accept an additional strbuf argument to write to,
convert the *_quote_print functions and callers to *_quote_buf.

[rr: commit message, minor modifications]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/for-each-ref.c | 13 +++++++++----
 quote.c                | 44 ++++++++++++++++++++++----------------------
 quote.h                |  6 +++---
 3 files changed, 34 insertions(+), 29 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 7f059c3..1d4083c 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -867,24 +867,29 @@ static void sort_refs(struct ref_sort *sort, struct refinfo **refs, int num_refs
 static void print_value(struct refinfo *ref, int atom, int quote_style)
 {
 	struct atom_value *v;
+	struct strbuf sb = STRBUF_INIT;
 	get_value(ref, atom, &v);
 	switch (quote_style) {
 	case QUOTE_NONE:
 		fputs(v->s, stdout);
 		break;
 	case QUOTE_SHELL:
-		sq_quote_print(stdout, v->s);
+		sq_quote_buf(&sb, v->s);
 		break;
 	case QUOTE_PERL:
-		perl_quote_print(stdout, v->s);
+		perl_quote_buf(&sb, v->s);
 		break;
 	case QUOTE_PYTHON:
-		python_quote_print(stdout, v->s);
+		python_quote_buf(&sb, v->s);
 		break;
 	case QUOTE_TCL:
-		tcl_quote_print(stdout, v->s);
+		tcl_quote_buf(&sb, v->s);
 		break;
 	}
+	if (quote_style != QUOTE_NONE) {
+		fputs(sb.buf, stdout);
+		strbuf_release(&sb);
+	}
 }
 
 static int hex1(char ch)
diff --git a/quote.c b/quote.c
index 5c88081..9fd66c6 100644
--- a/quote.c
+++ b/quote.c
@@ -408,72 +408,72 @@ int unquote_c_style(struct strbuf *sb, const char *quoted, const char **endp)
 
 /* quoting as a string literal for other languages */
 
-void perl_quote_print(FILE *stream, const char *src)
+void perl_quote_buf(struct strbuf *sb, const char *src)
 {
 	const char sq = '\'';
 	const char bq = '\\';
 	char c;
 
-	fputc(sq, stream);
+	strbuf_addch(sb, sq);
 	while ((c = *src++)) {
 		if (c == sq || c == bq)
-			fputc(bq, stream);
-		fputc(c, stream);
+			strbuf_addch(sb, bq);
+		strbuf_addch(sb, c);
 	}
-	fputc(sq, stream);
+	strbuf_addch(sb, sq);
 }
 
-void python_quote_print(FILE *stream, const char *src)
+void python_quote_buf(struct strbuf *sb, const char *src)
 {
 	const char sq = '\'';
 	const char bq = '\\';
 	const char nl = '\n';
 	char c;
 
-	fputc(sq, stream);
+	strbuf_addch(sb, sq);
 	while ((c = *src++)) {
 		if (c == nl) {
-			fputc(bq, stream);
-			fputc('n', stream);
+			strbuf_addch(sb, bq);
+			strbuf_addch(sb, 'n');
 			continue;
 		}
 		if (c == sq || c == bq)
-			fputc(bq, stream);
-		fputc(c, stream);
+			strbuf_addch(sb, bq);
+		strbuf_addch(sb, c);
 	}
-	fputc(sq, stream);
+	strbuf_addch(sb, sq);
 }
 
-void tcl_quote_print(FILE *stream, const char *src)
+void tcl_quote_buf(struct strbuf *sb, const char *src)
 {
 	char c;
 
-	fputc('"', stream);
+	strbuf_addch(sb, '"');
 	while ((c = *src++)) {
 		switch (c) {
 		case '[': case ']':
 		case '{': case '}':
 		case '$': case '\\': case '"':
-			fputc('\\', stream);
+			strbuf_addch(sb, '\\');
 		default:
-			fputc(c, stream);
+			strbuf_addch(sb, c);
 			break;
 		case '\f':
-			fputs("\\f", stream);
+			strbuf_addstr(sb, "\\f");
 			break;
 		case '\r':
-			fputs("\\r", stream);
+			strbuf_addstr(sb, "\\r");
 			break;
 		case '\n':
-			fputs("\\n", stream);
+			strbuf_addstr(sb, "\\n");
 			break;
 		case '\t':
-			fputs("\\t", stream);
+			strbuf_addstr(sb, "\\t");
 			break;
 		case '\v':
-			fputs("\\v", stream);
+			strbuf_addstr(sb, "\\v");
 			break;
 		}
 	}
-	fputc('"', stream);
+	strbuf_addch(sb, '"');
 }
diff --git a/quote.h b/quote.h
index ed110a5..6996ebd 100644
--- a/quote.h
+++ b/quote.h
@@ -68,8 +68,8 @@ extern char *quote_path_relative(const char *in, const char *prefix,
 			  struct strbuf *out);
 
 /* quoting as a string literal for other languages */
-extern void perl_quote_print(FILE *stream, const char *src);
-extern void python_quote_print(FILE *stream, const char *src);
-extern void tcl_quote_print(FILE *stream, const char *src);
+extern void perl_quote_buf(struct strbuf *sb, const char *src);
+extern void python_quote_buf(struct strbuf *sb, const char *src);
+extern void tcl_quote_buf(struct strbuf *sb, const char *src);
 
 #endif
-- 
1.8.4.rc0.4.g4634265

[PATCH 2/3] tar-tree: remove dependency on sq_quote_print()

[Ramkumar Ramachandra]

Currently, there is exactly one caller of sq_quote_print(), namely
cmd_tar_tree().  In the interest of removing sq_quote_print() and
simplification, replace it with an equivalent call to sq_quote_argv().
No functional changes intended.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/tar-tree.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/tar-tree.c b/builtin/tar-tree.c
index 3f1e701..ba3ffe6 100644
--- a/builtin/tar-tree.c
+++ b/builtin/tar-tree.c
@@ -26,8 +26,8 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)
 	 * $0 tree-ish basedir ==>
 	 * 	git archive --format-tar --prefix=basedir tree-ish
 	 */
-	int i;
 	const char **nargv = xcalloc(sizeof(*nargv), argc + 3);
+	struct strbuf sb = STRBUF_INIT;
 	char *basedir_arg;
 	int nargc = 0;
 
@@ -65,11 +65,10 @@ int cmd_tar_tree(int argc, const char **argv, const char *prefix)
 	fprintf(stderr,
 		"*** \"git tar-tree\" is now deprecated.\n"
 		"*** Running \"git archive\" instead.\n***");
-	for (i = 0; i < nargc; i++) {
-		fputc(' ', stderr);
-		sq_quote_print(stderr, nargv[i]);
-	}
-	fputc('\n', stderr);
+	sq_quote_argv(&sb, nargv, 0);
+	strbuf_addch(&sb, '\n');
+	fputs(sb.buf, stderr);
+	strbuf_release(&sb);
 	return cmd_archive(nargc, nargv, prefix);
 }
 
-- 
1.8.4.rc0.4.g4634265

[PATCH 3/3] quote: remove sq_quote_print()

[Ramkumar Ramachandra]

Remove sq_quote_print() since it has no callers.  A nicer alternative
sq_quote_buf() exists: its callers aren't forced to print immediately.

For historical context, sq_quote_print() was first introduced in
575ba9d6 (GIT_TRACE: show which built-in/external commands are executed,
2006-06-25) for the purpose of printing argv for $GIT_TRACE.  Today, we
achieve this using trace_argv_printf() -> sq_quote_argv() ->
sq_quote_buf(), which ultimately fills in a strbuf.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 quote.c | 17 -----------------
 quote.h |  2 --
 2 files changed, 19 deletions(-)

diff --git a/quote.c b/quote.c
index 9fd66c6..45e3db1 100644
--- a/quote.c
+++ b/quote.c
@@ -42,23 +42,6 @@ void sq_quote_buf(struct strbuf *dst, const char *src)
 	free(to_free);
 }
 
-void sq_quote_print(FILE *stream, const char *src)
-{
-	char c;
-
-	fputc('\'', stream);
-	while ((c = *src++)) {
-		if (need_bs_quote(c)) {
-			fputs("'\\", stream);
-			fputc(c, stream);
-			fputc('\'', stream);
-		} else {
-			fputc(c, stream);
-		}
-	}
-	fputc('\'', stream);
-}
-
 void sq_quote_argv(struct strbuf *dst, const char** argv, size_t maxlen)
 {
 	int i;
diff --git a/quote.h b/quote.h
index 6996ebd..71dcc3a 100644
--- a/quote.h
+++ b/quote.h
@@ -27,8 +27,6 @@ struct strbuf;
  * excluding the final null regardless of the buffer size.
  */
 
-extern void sq_quote_print(FILE *stream, const char *src);
-
 extern void sq_quote_buf(struct strbuf *, const char *src);
 extern void sq_quote_argv(struct strbuf *, const char **argv, size_t maxlen);
 
-- 
1.8.4.rc0.4.g4634265

Re: [PATCH 0/3] Remove sq_quote_print() in favor of *_buf

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> While going through the for-each-ref-pretty series that Duy and I were
> developing, I noticed that this cleanup was independent and good
> as-it-is.
>
> So here it is.

You always can first allocate a piece of memory and write into it
instead of writing things out directly.  The patch shows it _can_ be
done, but that is not a news.

And such a change is hardly a "clean-up".  It just wastes more
memory you do not have to waste, in order to do what you are doing.

When there is a reason why you need an in-memory representation,
this change starts to become the first step refactoring for an
enhancement.

> Nguyễn Thái Ngọc Duy (1):
>   for-each-ref, quote: convert *_quote_print -> *_quote_buf

The log message for this one explains it very well.  This change by
itself is not useful, but it will become necessary once you start
needing to access an in-memory result.

Re: [PATCH 0/3] Remove sq_quote_print() in favor of *_buf

[Junio C Hamano]

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

> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>> While going through the for-each-ref-pretty series that Duy and I were
>> developing, I noticed that this cleanup was independent and good
>> as-it-is.
>>
>> So here it is.
>
> You always can first allocate a piece of memory and write into it
> instead of writing things out directly.  The patch shows it _can_ be
> done, but that is not a news.
>
> And such a change is hardly a "clean-up".  It just wastes more
> memory you do not have to waste, in order to do what you are doing.
>
> When there is a reason why you need an in-memory representation,
> this change starts to become the first step refactoring for an
> enhancement.

Having said all that, the patch texts all look OK, so I'll queue
them with updated log messages.  It was the usual me reacting to
unjustified value judgement made in log messages and cover letters.

It would save me a lot of work if people stopped doing that and
instead stuck to facts.  For example, between print_to_buf() and
print_to_stdout(), the former is *not* necessarily "nicer".  It is
more flexible but that flexibility comes with a price, and the
caller needs a demonstrative need for that flexibility to justify
the cost.

Thanks.

Re: [PATCH 0/3] Remove sq_quote_print() in favor of *_buf

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Having said all that, the patch texts all look OK, so I'll queue
> them with updated log messages.  It was the usual me reacting to
> unjustified value judgement made in log messages and cover letters.

Thanks.  I'll look at how the log messages are different in the queued
version, and adapt accordingly next time.