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

Denton Liu [off-list ref] 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.