Re: [PATCH 2/2] remote.c: remove BUG in show_push_unqualified_ref_name_error()

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

Denton Liu <liu.denton@gmail.com> writes:

> In the case where a non-existent oid is given as the <src> for a
> refspec and the destination is unqualified, we end up hitting the BUG in
> show_push_unqualified_ref_name_error().
>
> This is because before hitting this advise message, the <src> is passed
> through repo_get_oid() which, upon receiving a fully qualified oid,
> doesn't actually check the existence of the object and just returns
> found.

The tail end of the above sentence does not quite parse for me.
Strike "and just returns found" out, perhaps?

> This means that it's actually possible for the
> odb_read_object_info() call to return not found under normal usage and
> thus, it's not actually a bug.

Again it is unclear what this "not found", used as noun, means.

Often saying "A" and having to follow it with "this means B" is a
sign that both needs to be rewritten to clarify.  The above does it
three times ("A", "this is because B", "this means C").  How about
flowing your thought in a slightly different order, perhaps like
this?

    When "git push <remote> <src>:<dst>" does not spell out the
    destination side of the ref fully, and when <src> is not given
    as a reference but an object name, the code tries to give advice
    messages based on the type of that object.

    The type is determined by calling odb_read_object_info() and
    signalled by its return value.  The code however reported a
    programming error with BUG() when this function said that there
    is no such object, which happens when the object name is given
    as a full hexadecimal (if the object name is given as a partial
    hexadecimal or an non-existing ref, the function would have died
    without returning, so this BUG() wouldn't have triggered).  This
    is wrong.  It is an ordinary end-user mistake to give an object
    name that does not exist and treated as such.

or something?

> Replace the BUG() with an advise() displaying a helpful message about
> the oid possibly not existing.

I briefly thought this may need to be an error(), but with a larger
context, this else clause is at the end of if/else if/... cascade
for different object types, each arm of which emits per object type
advice messages, so the new one being another call to advise() would
make sense.

>  	} else {
> -		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
> -		    matched_src_name, type);
> +		advise(_("The <src> part of the refspec is an oid that doesn't exist.\n"
> +			 "Please ensure that the oid '%s' is correct."),
> +		       matched_src_name);

Unlike the other existing messages, the second line after the
diagnosis in this new message states something that is too obvious
to anybody---even to somebody who may be helped by an advice message
that says "you seem to have a commit, perhaps you meant to create a
branch?"

Thanks.