Re: [PATCH 2/2] push: add an advice on unqualified <dst> push

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 10, 2018, 21:54 UTC
Message-ID: <xmqqbm81lb7c.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181010205505.GB12949@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 24 quoted lines

>> Fix both of those, now the message will look like this instead:
>> 
>>     $ ./git-push avar v2.19.0^{commit}:newbranch -n
>>     error: unable to push to unqualified destination: newbranch
>>     hint: The destination refspec neither matches an existing
>>     hint: ref on the remote nor begins with refs/, and we are
>>     hint: unable to guess a prefix based on the source ref.
>>     hint:
>>     hint: The <src> part of the refspec is a commit object.
>>     hint: Did you mean to create a new branch by pushing to
>>     hint: 'v2.19.0^{commit}:refs/heads/newbranch'?
>>     error: failed to push some refs to 'git@github.com:avar/git.git'
>> 
>> When trying to push a tag, tree or a blob we suggest that perhaps the
>> user meant to push them to refs/tags/ instead.
>
> This is much better, and I love the customized behavior based on the
> object type.
>
> I wonder if we could reword the first paragraph to be a little less
> confusing, and spell out what we tried already. E.g., something like:
>
>   The destination you provided is not a full refname (i.e., starting
>   with "ref"). Git tried to guess what you meant by:

s|ref|refs/|; I fully agree that "unqualified destination" was a poor way to communicate the failure to those who would likely hit this error path, because somebody who can ell what's qualified and what's not would not be triggering the error in the first place.

Show 8 quoted lines

>     - looking for a matching branch or tag on the remote side
>
>     - looking at the refname of the local source
>
>   but neither worked.
>
>   The <src> part of the refspec is a commit object.
>   Did you mean...

Looks great.

> I'm not sure about saying "branch or tag" in the first bullet. It's
> friendlier to most users, but less technically correct (if you said
> "notes/foo", I believe we'd match an existing "refs/notes/foo", because
> it's really just using the normal lookup rules).

An alternative may be "looking for a ref that matches %s on the remote side". I am no longer a total newbie, so I cannot tell how well that message would help one to connect notes/foo one just typed with refs/notes/foo that potentially exists on the remote side.

> Also, as an aside, I wonder if we should allow "heads/foo" to work as
> "refs/heads/foo" (even when no such ref already exists). But that is
> totally orthogonal to changing the message.

I am neutral on this point but agree that it is better done outside this patch.