From: Jonathan Tan <jonathantanmy@google.com>
Date: Mon, 08 Nov 2021 23:06:12 GMT
Subject: Re: [PATCH v6 2/2] http-fetch: redact url on die() message
Message-ID: <20211108230612.1102476-1-jonathantanmy@google.com>
In-Reply-To: <38859ae7b7de0f6406180a0427b9ce07fe3b9aa3.1635532975.git.gitgitgadget@gmail.com>

> @@ -63,8 +64,17 @@ 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,
> -			    curl_errorstr);
> +			struct url_info url;
> +			char *nurl = url_normalize(preq->url, &url);
> +			if (!nurl || !git_env_bool("GIT_TRACE_REDACT", 1)) {
> +				die("unable to get pack file '%s'\n%s", preq->url,
> +				    curl_errorstr);
> +			} else {
> +				die("failed to get '%.*s' url from '%.*s' "
> +				    "(full URL redacted due to GIT_TRACE_REDACT setting)\n%s",
> +				    (int)url.scheme_len, url.url,
> +				    (int)url.host_len, &url.url[url.host_off], curl_errorstr);
> +			}

I was confused why nurl was set but never used in "else", but I see that
it's because url_normalize() also sets that value in the urlinfo struct.
This patch looks good (and patch 1 too, with my suggested changes).