Re: [PATCH 1/2] fetch-pack: redact packfile urls in traces

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Oct 08 2021, Ivan Frade via GitGitGadget wrote:

> diff --git a/http-fetch.c b/http-fetch.c
> index fa642462a9e..d35e33e4f65 100644
> --- a/http-fetch.c
> +++ b/http-fetch.c
> @@ -63,7 +63,9 @@ static void fetch_single_packfile(struct object_id *packfile_hash,
>  	if (start_active_slot(preq->slot)) {
>  		run_active_slot(preq->slot);
>  		if (results.curl_result != CURLE_OK) {
> -			die("Unable to get pack file %s\n%s", preq->url,
> +			int showUrl = git_env_bool("GIT_TRACE_REDACT", 1);
> +			die("Unable to get offloaded pack file %s\n%s",
> +			    showUrl ? preq->url : "<redacted>",
>  			    curl_errorstr);
>  		}
>  	} else {

Your CL and commit message just talk about traes, but this is a die()
message.

Perhaps it makes sense to redact it there too for some reason, but that
seems to be a thing to separately argue for.

This message is shown interactively to users, and I could see it be
annoying to not have the URL that failed in your terminal output, even
if it has some one-time token.

Which is presumably different from the use-cases you're thinking of, I'm
assuming some logging of detached processes, or central logging of user
actions.

> +test_expect_success 'packfile-uri redacted in trace' '
> +	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
> +	rm -rf "$P" http_child log &&
> +
> +	git init "$P" &&
> +	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
> +
> +	echo my-blob >"$P/my-blob" &&
> +	git -C "$P" add my-blob &&
> +	git -C "$P" commit -m x &&
> +
> +	configure_exclusion "$P" my-blob >h &&
> +
> +	GIT_TRACE=1 GIT_TRACE_PACKET="$(pwd)/log" GIT_TEST_SIDEBAND_ALL=1 \
> +	git -c protocol.version=2 \
> +		-c fetch.uriprotocols=http,https \
> +		clone "$HTTPD_URL/smart/http_parent" http_child &&
> +
> +	grep -A1 "clone<\ ..packfile-uris" log | grep "clone<\ <redacted>"

We don't rely on GNU options like those for the test suite, it'll break
on various supported platformrs.

In this case the whole LHS of the pipe looks like it could be dropped,
why not grep for "^clone< <redacted>"?

Also you don't need to quote the space character in regexes, it's not a
metacharacter.