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

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 6, 2025, 06:14 UTC
Message-ID: <aJLywm9xWQQUADH1@pks.im>
In-Reply-To: <938dfb8d4e37ef962c811d6e0f32122a2522deb5.1754455931.git.liu.denton@gmail.com>

On Tue, Aug 05, 2025 at 09:53:42PM -0700, Denton Liu wrote:

Show 14 quoted lines

> 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