Re: [PATCH 2/2] builtin/mv: convert assert(3p) into `BUG()`

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 30, 2025, 18:45 UTC
Message-ID: <xmqqr0191oaz.fsf@gitster.g>
In-Reply-To: <20250430-pks-mv-parent-child-conflict-v1-2-11a87c55ffb9@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 5 quoted lines

> The use of asserts is discouraged in our codebase because they lead to
> different behaviour depending on how Git is built. When being unsure
> enough whether a condition always holds so that one adds the assert,
> then the assert should probably trigger regardless of how Git is being
> built.

Nicely put. Yes, this is another reason why we frown on the use of assert(), in addition to the reason why why Elijah's series that ends with 5633aa3a (treewide: replace assert() with ASSERT() in special cases, 2025-03-19) was written.

> Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`.

Being explicit about what we are unsure about is always good. It would hopefully entice those who want to get their hands dirty to see if they can "prove" that BUG() would never happen, which would be a great outcome ;-).

Thanks.

Show 20 quoted lines

>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/mv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index edb854677d9..07548fe96ae 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -562,7 +562,8 @@ int cmd_mv(int argc,
>  			continue;
>  
>  		pos = index_name_pos(the_repository->index, src, strlen(src));
> -		assert(pos >= 0);
> +		if (pos < 0)
> +			BUG("could not find source in index: '%s'", src);
>  		if (!(mode & SPARSE) && !lstat(src, &st))
>  			sparse_and_dirty = ie_modified(the_repository->index,
>  						       the_repository->index->cache[pos],