Re: [PATCH 2/2] push: suggest <remote> <branch> for a slash slip

[Junio C Hamano <gitster@pobox.com>]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> "git push origin/main" is treated as a repository and dies with
> "'origin/main' does not appear to be a git repository", with no hint
> that a space was meant instead of a slash.

This is easier for me to guess than what the user may have wanted to
do in the decription of [1/2].  But it still will be easier on
readers to say

    When pusing out up update the "main" branch to the remote
    "origin", i.e.,

        $ git push origin main

    it is easy for some users to mistakenly say

        $ git push origin/main

    instead.  This however instructs to push to remote "origin/main"
    with configured refspecs, which means a completely different
    thing.  Lucikly, often origin/main does not exist as a remote
    and the command fails without doing any harm, but still may
    leave the user puzzled what happened.  Give hint to ...

or something like that.

> When the argument is not an existing path or configured remote but its
> part before the first slash names one, suggest the intended
> "git push <remote> <branch>" form. The suggestion is shown as advice so
> it can be silenced with advice.pushRepoLooksLikeRef.

Sounds sensible.

>  	if (repo) {
>  		if (!add_remote_or_group(repo, &remote_group)) {
> +			const char *slash = strchr(repo, '/');
> +			struct remote *r;
> +
> +			/*
> +			 * A "<remote>/<branch>" argument that does not name
> +			 * a path is likely a slip for the separate
> +			 * "<remote> <branch>" form, so suggest that instead.
> +			 */
> +			if (slash && slash[1] && !file_exists(repo)) {
> +				struct strbuf name = STRBUF_INIT;
> +
> +				strbuf_add(&name, repo, slash - repo);
> +				if (remote_is_configured(remote_get(name.buf), 0)) {
> +					int code = die_message(_("'%s' is not a valid push target"), repo);
> +					advise_if_enabled(ADVICE_PUSH_REPO_LOOKS_LIKE_REF,
> +							  _("Did you mean to use: git push %s %s?"),
> +							  name.buf, slash + 1);
> +					strbuf_release(&name);
> +					exit(code);
> +				}
> +				strbuf_release(&name);
> +			}

Hmph, if this class of hint is not enabled, do we still have to
spend cycles on these "is this a remote?  is the first token a
remote?" computation?  I would have expected that a change here
would be a two-liner:

    if (!add_remote_or_group(...)) {
+	if (advise_enabled(ADVICE_PUSH_REPO_LOOKS_LIKE_REF))
+		die_if_plausible_typo(...);
	... do the "try treating it as a direct URL or path" thing ...
    }


with the bulk of the "if it has slash, it is not a file, then advise
and die" logic inside the new helper function.

What I find especially troubling is that even when advise for this
class of hint is not enabled, the new code will hit the new exit(),
without falling back to the "try treating it as a direct URL or
path" thing.  Or am I missing something?

Thanks.