Re: [PATCH] clone/fetch: anonymize URLs in the reflog

[Jeff King <peff@peff.net>]

On Mon, Jun 01, 2020 at 07:20:02PM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Even if we strongly discourage putting credentials into the URLs passed
> via the command-line, there _is_ support for that, and users _do_ do
> that.
> 
> Let's scrub them before writing them to the reflog.

Good idea.

>     This came up in an internal audit, but we do not consider this to be a
>     big deal: the reflog is local and not really shared with anybody.

Agreed.

>  builtin/clone.c            | 10 ++++++----
>  builtin/fetch.c            |  9 +++++++--
>  t/t5541-http-push-smart.sh | 15 +++++++++++++++

The patch itself looks very neatly done.

> @@ -993,11 +993,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  
>  	path = get_repo_path(repo_name, &is_bundle);
>  	if (path)
> -		repo = absolute_pathdup(repo_name);
> +		display_repo = repo = absolute_pathdup(repo_name);
>  	else if (!strchr(repo_name, ':'))
>  		die(_("repository '%s' does not exist"), repo_name);
> -	else
> +	else {
>  		repo = repo_name;
> +		display_repo = transport_anonymize_url(repo);
> +	}

Not introduced by your patch, but I had to read this a few times to make
sure we always end up with repo and display_repo set. IMHO it would be
easier to read as:

  if (this) {
     repo = ...;
     display_repo = ...;
  } else if (that) {
     repo = ...;
     display_repo = ...;
  } else {
     die(...);
  }

instead of sticking the die() in the middle.  Maybe just personal
preference, though. :)

> +	# should have been scrubbed down to vanilla URL
> +	git log -g master >reflog &&
> +	grep "$HTTPD_URL" reflog &&
> +	! grep "$HTTPD_URL_USER_PASS" reflog
> +'

And you make sure we retain the username. Nice.

-Peff