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

Jeff King [off-list ref] writes:

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

quoted

From: Johannes Schindelin <redacted>

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.

quoted

    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.

Nice.

quoted

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

The patch itself looks very neatly done.

quoted

@@ -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. :)

For a if/elseif cascade of few-liner blocks each, I do not think it
would matter, but if a block were larger, having the die() case at
the beginning or at the end would indeed make it easier to spot any
anomalies.

quoted

+	# 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.