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

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

[Johannes Schindelin via GitGitGadget]

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'

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
-- 
gitgitgadget

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

[Junio C Hamano]

"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

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

[Patrick Steinhardt]

On Thu, Oct 09, 2025 at 01:30:21PM -0700, Junio C Hamano wrote:
> "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.

Agreed. I don't see any reason why anyone would like to use symbolic
refs for this. The reading side for such symrefs may continue to exist
for a while. But the writing side can go away.

Patrick

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

[Patrick Steinhardt]

On Thu, Oct 09, 2025 at 07:46:22AM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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;

An alternative could be to fix this at the source, e.g. like the below
(untested) patch. But I don't mind this too much, especially given that
this here is the only callsite of that function anyway. So please feel
free to disregard.

Thanks!

Patrick

diff --git a/refs/files-backend.c b/refs/files-backend.c
index bb2bec3807..cb402a2a54 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs,
 }
 
 #ifdef NO_SYMLINK_HEAD
-#define create_ref_symlink(a, b) (-1)
+#define create_ref_symlink(a, b) NOT_CONSTANT(-1)
 #else
 static int create_ref_symlink(struct ref_lock *lock, const char *target)
 {

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

[Johannes Schindelin]

Hi Patrick,

On Fri, 10 Oct 2025, Patrick Steinhardt wrote:

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index bb2bec3807..cb402a2a54 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs,
>  }
>  
>  #ifdef NO_SYMLINK_HEAD
> -#define create_ref_symlink(a, b) (-1)
> +#define create_ref_symlink(a, b) NOT_CONSTANT(-1)
>  #else
>  static int create_ref_symlink(struct ref_lock *lock, const char *target)
>  {

While this is correct, and "closer to the root", in my experience it is
better to have work-arounds closer to where the symptom appears. In this
case, it would be directly in the condition of the `if ()` construct.
Therefore, I would prefer to keep the proposed patch as-is.

Ciao,
Johannes

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

[Patrick Steinhardt]

On Fri, Oct 10, 2025 at 03:49:37PM +0200, Johannes Schindelin wrote:
> Hi Patrick,
> 
> On Fri, 10 Oct 2025, Patrick Steinhardt wrote:
> 
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index bb2bec3807..cb402a2a54 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs,
> >  }
> >  
> >  #ifdef NO_SYMLINK_HEAD
> > -#define create_ref_symlink(a, b) (-1)
> > +#define create_ref_symlink(a, b) NOT_CONSTANT(-1)
> >  #else
> >  static int create_ref_symlink(struct ref_lock *lock, const char *target)
> >  {
> 
> While this is correct, and "closer to the root", in my experience it is
> better to have work-arounds closer to where the symptom appears. In this
> case, it would be directly in the condition of the `if ()` construct.
> Therefore, I would prefer to keep the proposed patch as-is.

As said, I don't feel strongly about it, so this is totally fine with
me. Thanks!

Patrick

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> An alternative could be to fix this at the source, e.g. like the below
> (untested) patch. But I don't mind this too much, especially given that
> this here is the only callsite of that function anyway. So please feel
> free to disregard.
>
> Thanks!

Indeed that is much nicer as it targets only NO_SYMLINK_HEAD case
without affecting the other side of #ifdef/#else/#endif but as you
say, I think this falls into "once the code is written one way, it
is not worth revisiting to change it with more iterations" category.

Thanks for very sharp eyes, nevertheless.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index bb2bec3807..cb402a2a54 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs,
>  }
>  
>  #ifdef NO_SYMLINK_HEAD
> -#define create_ref_symlink(a, b) (-1)
> +#define create_ref_symlink(a, b) NOT_CONSTANT(-1)
>  #else
>  static int create_ref_symlink(struct ref_lock *lock, const char *target)
>  {