Re: [PATCH v2 5/5] refs/debug: trim trailing LF from reflog message

"Han-Wen Nienhuys via GitGitGadget" [off-list ref] writes:

The API promises to have only LF, not CRLF, at the end, so
strbuf_trim_trailing_newline() is a bit overkill (and if payload
happened to end with CR, we would lose it).

+	trace_printf_key(&trace_refs,
+			 "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
+			 dbg->refname, ret, o, n, committer,
+			 (long int)timestamp, trimmed.buf);
+	strbuf_release(&trimmed);
 	return ret;
 }

Can we use counted bytes in trace_printf()?  If we can, it would be
simpler to just scan "msg" for LF and then show only the span
between the beginning of the string and the found LF using "%.*s",
perhaps like this?

 refs/debug.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git c/refs/debug.c w/refs/debug.c
index 8667c64023..c97eeb5740 100644
--- c/refs/debug.c
+++ w/refs/debug.c

@@ -284,15 +284,20 @@ static int debug_print_reflog_ent(struct object_id *old_oid,
 	int ret;
 	char o[GIT_MAX_HEXSZ + 1] = "null";
 	char n[GIT_MAX_HEXSZ + 1] = "null";
+	int msglen;
+
 	if (old_oid)
 		oid_to_hex_r(o, old_oid);
 	if (new_oid)
 		oid_to_hex_r(n, new_oid);
 
+	msglen = strchrnul(msg, '\n') - msg;
 	ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
 		      dbg->cb_data);
-	trace_printf_key(&trace_refs, "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
-		dbg->refname, ret, o, n, committer, (long int)timestamp, msg);
+	trace_printf_key(&trace_refs,
+			 "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%.*s\"\n",
+			 dbg->refname, ret, o, n, committer, (long int)timestamp,
+			 (int)msglen, msg);
 	return ret;
 }
 

On Mon, Nov 29 2021, Han-Wen Nienhuys wrote:

On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano [off-list ref] wrote:

quoted

The API promises to have only LF, not CRLF, at the end, so
strbuf_trim_trailing_newline() is a bit overkill (and if payload
happened to end with CR, we would lose it).

it would be best if there was a way to escape characters (ie. "\n" =>
"\\n"). Do we have a function for that?

quoted

quoted

+     trace_printf_key(&trace_refs,
+                      "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
+                      dbg->refname, ret, o, n, committer,
+                      (long int)timestamp, trimmed.buf);
+     strbuf_release(&trimmed);
      return ret;
 }

Can we use counted bytes in trace_printf()?  If we can, it would be
simpler to just scan "msg" for LF and then show only the span
between the beginning of the string and the found LF using "%.*s",
perhaps like this?

I beg to differ - despite this being fewer lines of code, I think
pointer arithmetic is best avoided if possible.

We usually do this with pointer arithmetic, but the %.*s format doesn't
require that, just code like:

    const char *str = "foobar";
    size_t len = strlen(str);
    len -= 1; /* give me less! */
    printf("%.*s", (int)len, str);

So you can also feed it (len - 1) or whatever if you know it to end with
a character you don't want.

It's (more simply done as) pointer arithmetic if you're finding that end
marker with strstr() or whatever, but you can also bend over backwards
and get a "len" instead through other means, and in either case I think
it beats reallocating the whole thing (more for readability than any
optimization reasons).

On Fri, Nov 26, 2021 at 9:16 AM Junio C Hamano [off-list ref] wrote:

The API promises to have only LF, not CRLF, at the end, so
strbuf_trim_trailing_newline() is a bit overkill (and if payload
happened to end with CR, we would lose it).

it would be best if there was a way to escape characters (ie. "\n" =>
"\\n"). Do we have a function for that?

quoted

+     trace_printf_key(&trace_refs,
+                      "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%s\"\n",
+                      dbg->refname, ret, o, n, committer,
+                      (long int)timestamp, trimmed.buf);
+     strbuf_release(&trimmed);
      return ret;
 }

Can we use counted bytes in trace_printf()?  If we can, it would be
simpler to just scan "msg" for LF and then show only the span
between the beginning of the string and the found LF using "%.*s",
perhaps like this?

I beg to differ - despite this being fewer lines of code, I think
pointer arithmetic is best avoided if possible.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado