Re: [PATCH 02/15] for-each-ref: don't print out elements directly

Ramkumar Ramachandra [off-list ref] writes:

The change in the first step made some sense; if asked not to quote,
it just emits the atom with fputs() in the switch (quote_style), so
there is nothing more to do after the switch.

But this change cannot be correct.  It prints literally to sb, and
leaves the function without emitting anything nor freeing the
resource in sb.

On Wed, Jun 5, 2013 at 6:13 AM, Junio C Hamano [off-list ref] wrote:

Ramkumar Ramachandra [off-list ref] writes:

quoted

From: Nguyễn Thái Ngọc Duy <redacted>

Currently, the entire callchain starting from show_ref() parses and
prints immediately.  This inflexibility limits our ability to extend the
parser.  So, convert the entire callchain to accept a strbuf argument to
write to.  Also introduce a show_refs() helper that calls show_ref() in
a loop to avoid cluttering up cmd_for_each_ref() with the task of
initializing/freeing the strbuf.

[rr: commit message]

Signed-off-by: Ramkumar Ramachandra <redacted>
---
 builtin/for-each-ref.c | 55 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 20 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 1d4083c..e2d6c5a 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -864,31 +864,31 @@ static void sort_refs(struct ref_sort *sort, struct refinfo **refs, int num_refs
      qsort(refs, num_refs, sizeof(struct refinfo *), compare_refs);
 }

-static void print_value(struct refinfo *ref, int atom, int quote_style)
+static void print_value(struct strbuf *sb, 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);
+             strbuf_addstr(sb, v->s);
              break;
      case QUOTE_SHELL:
-             sq_quote_buf(&sb, v->s);
+             sq_quote_buf(sb, v->s);
              break;
      case QUOTE_PERL:
-             perl_quote_buf(&sb, v->s);
+             perl_quote_buf(sb, v->s);
              break;
      case QUOTE_PYTHON:
-             python_quote_buf(&sb, v->s);
+             python_quote_buf(sb, v->s);
              break;
      case QUOTE_TCL:
-             tcl_quote_buf(&sb, v->s);
+             tcl_quote_buf(sb, v->s);
              break;
      }
      if (quote_style != QUOTE_NONE) {
-             fputs(sb.buf, stdout);
-             strbuf_release(&sb);
+             fputs(sb->buf, stdout);
+             strbuf_release(sb);
      }
 }

The change in the first step made some sense; if asked not to quote,
it just emits the atom with fputs() in the switch (quote_style), so
there is nothing more to do after the switch.

But this change cannot be correct.  It prints literally to sb, and
leaves the function without emitting anything nor freeing the
resource in sb.

Everything will/should be emitted in show_refs(), so the "if
(quote_style !=..)" block should be removed. The end result still
looks correct due to the order of fputs (in print_value and
show_refs). It only breaks down when %<(*) is used together because
then "sb" content matters. Will fix.
--
Duy