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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 4, 2025, 14:19 UTC
Message-ID: <xmqq5xf39nky.fsf@gitster.g>
In-Reply-To: <3eb95731ea07c5f25ed7a47cc639f53b4b18e113.1754300389.git.liu.denton@gmail.com>

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

Show 8 quoted lines

> 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.

Show 6 quoted lines

>  	} 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.