Re: [PATCH] refs: forbid clang to complain about unreachable code

[Junio C Hamano <gitster@pobox.com>]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When `NO_SYMLINK_HEAD` is defined, `create_ref_symlink()` is hard-coded
> as `(-1)`, and as a consequence the condition `!create_ref_symlink()`
> always evaluates to false, rendering any code guarded by that condition
> unreachable.
>
> Therefore, clang is _technically_ correct when it complains about
> unreachable code. It does completely miss the fact that this is okay
> because on _other_ platforms, where `NO_SYMLINK_HEAD` is not defined,
> the code isn't unreachable at all.
>
> Let's use the same trick as in 82e79c63642c (git-compat-util: add
> NOT_CONSTANT macro and use it in atfork_prepare(), 2025-03-17) to
> appease clang while at the same time keeping the `-Wunreachable` flag
> to potentially find _actually_ unreachable code.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     refs: forbid clang to complain about unreachable code
>     
>     Just upstreamin'

It may not be a bad idea to deprecate core.preferSymlinkRefs now and
remove it at Git 3.0 boundary.  Some platforms may not be able to do
symbolic links and use it to represent HEAD, but everybody should be
able to create a small text file with a single line.

But until then, this is a very reasonable thing to do.

Thanks.

>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1984%2Fdscho%2Frefs-clang-fix-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1984/dscho/refs-clang-fix-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1984
>
>  refs/files-backend.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 088b52c740..814decf323 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3186,7 +3186,13 @@ static int files_transaction_finish(struct ref_store *ref_store,
>  		 * next update. If not, we try and create a regular symref.
>  		 */
>  		if (update->new_target && refs->prefer_symlink_refs)
> -			if (!create_ref_symlink(lock, update->new_target))
> +			/*
> +			 * By using the `NOT_CONSTANT()` trick, we can avoid
> +			 * errors by `clang`'s `-Wunreachable` logic that would
> +			 * report that the `continue` statement is not reachable
> +			 * when `NO_SYMLINK_HEAD` is `#define`d.
> +			 */
> +			if (NOT_CONSTANT(!create_ref_symlink(lock, update->new_target)))
>  				continue;
>  
>  		if (update->flags & REF_NEEDS_COMMIT) {
>
> base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0