threads / patch / 34568

patch, 3 partsRemove sq_quote_print() in favor of *_buf

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

## tl;dr

7 messages between Jul 30, 2013 and Jul 30, 2013. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Ramkumar Ramachandra· Jul 30, 2013, 08:31 UTC · lore
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
Ramkumar Ramachandra· Jul 30, 2013, 08:31 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 3 files +34 −29

builtin/for-each-ref.c, quote.c, quote.h

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
Ramkumar Ramachandra· Jul 30, 2013, 08:31 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to builtin/tar-tree.c +5 −6
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
Ramkumar Ramachandra· Jul 30, 2013, 08:31 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 2 files +0 −19

quote.c, quote.h

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
Junio C Hamano· Jul 30, 2013, 15:00 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 5 quoted lines
> 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.

Junio C Hamano· Jul 30, 2013, 15:27 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 18 quoted lines
> 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.
Ramkumar Ramachandra· Jul 30, 2013, 23:20 UTC · re: Junio C Hamano · lore

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

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.

← back to recent threads