Jeff King <peff@xxxxxxxx> writes:
> On Mon, Jun 01, 2020 at 07:20:02PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@xxxxxx>
>>
>> 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.
Nice.
>> 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. :)
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.
>> + # 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.
