On Thu, Nov 10, 2022 at 02:14:11PM -0800, Glen Choo wrote:

quoted

quoted

@@ -560,8 +560,10 @@ static void set_curl_keepalive(CURL *c)
 }
 #endif

-static void redact_sensitive_header(struct strbuf *header)
+/* Return 0 if redactions been made, 1 otherwise. */

Does it make sense to reverse the retval here?

`if (!redact_sensitive_header())` sounds like "if not redacted, ..." -
but here it means the opposite, right?

I struggled with this for a bit since I wasn't sure what the convention
is here. Enumerating some off the top of my head, we have:

For what it's worth, the "return zero if redactions were made" is what I
would have expected. I think of it as returning zero if we didn't
encounter an error (and returning a negative, non-zero value if we did).

- For functions that don't fail we have "0" for "nothing was done" and
  "1" for something was done (e.g. skip_prefix()).

(Tangent: from a readability perspective, this is pretty poor. I need to
know beforehand whether or not the function may fail with error before I
know what the return value means?)

This probably falls into the last category, so for consistency, I think
this should return "1" for "redactions have happened" (as you
suggested).

...But I don't really care that much ;-). As long as you choose
consistently, and document your choice where it is unclear, it is fine.

quoted

quoted

+/* Redact headers in info */
+static void redact_sensitive_info_header(struct strbuf *header)
+{
+	const char *sensitive_header;
+
+	if (trace_curl_redact &&
+	    skip_iprefix(header->buf, "h2h3 [", &sensitive_header)) {
+		struct strbuf inner = STRBUF_INIT;
+
+		/* Drop the trailing "]" */
+		strbuf_add(&inner, sensitive_header, strlen(sensitive_header) - 1);
+		if (!redact_sensitive_header(&inner)) {
+			strbuf_setlen(header, strlen("h2h3 ["));
+			strbuf_addbuf(header, &inner);
+			strbuf_addch(header, ']');

I'd really like some more comments in this function - even just one
describing the string we're trying to redact, or showing a sample line.
Navigating string parsing is always a bit difficult.

Ah yes, I should include a description of the string.

Eh. To be honest, I probably wouldn't have documented it any more than
you did. At most, I would add an example "before" and "after" string
that shows what we're trying to generate.

I agree that string manipulation can end up with some fairly convoluted
code. But I think what is written here is straightforward, and that
any attempt to comment it more than suggested would end up just
repeating what the code does in English.

Thanks,
Taylor