Re: [PATCH 2/2] remote.c: remove BUG in show_push_unqualified_ref_name_error()
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.
Denton Liu
