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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Aug 05, 2025 at 09:53:42PM -0700, Denton Liu wrote:
> diff --git a/remote.c b/remote.c
> index e965f022f1..465e0ea0eb 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1218,8 +1218,8 @@ static void show_push_unqualified_ref_name_error(const char *dst_value,
>  			 "'%s:refs/tags/%s'?"),
>  		       matched_src_name, dst_value);
>  	} else {
> -		BUG("'%s' should be commit/tag/tree/blob, is '%d'",
> -		    matched_src_name, type);
> +		advise(_("The <src> part of the refspec ('%s') is an object ID that doesn't exist.\n"),
> +		       matched_src_name);
>  	}
>  }

This reads a lot better, thanks. We could arguably convert the
if-else-chain into a switch to make all of this read a bit better, but
that is a subjective style change and definitely not something that you
have to do as part of this series.

Regarding the logic this looks sensible to me. We have already handled
all valid object types in the cases leading up to this final `else`, so
we can be sure that we weren't able to look up the object. And warning
about that case feels reasonable.

One thing I wondered is whether it's okay to not die anymore via
`BUG()`. The other error cases already don't die though, so this ought
to be fine. Going up the callchain shows that we do bubble up the error
as expected until we end up in `match_push_refs()`. There's multiple
callers of that function, and all except one perform error handling for
it.

The only exception is git-remote(1) in `get_push_ref_states()`, where it
gets executed via `git remote show $remote_name`. As far as I understand
we would end up not showing any references that are broken, and we would
print the above advise. Which I think is reasonable.

So all of this looks good to me, thanks!

Patrick