refs: forbid clang to complain about unreachable code #1984
+7
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 82e79c6 (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 <[email protected]>
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Johannes Schindelin via GitGitGadget" <[email protected]>
writes:
> From: Johannes Schindelin <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): 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)
{
|
|
User |
|
On the Git mailing list, Patrick Steinhardt wrote (reply to this): On Thu, Oct 09, 2025 at 01:30:21PM -0700, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <[email protected]>
> writes:
>
> > From: Johannes Schindelin <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): 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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Patrick Steinhardt <[email protected]> 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)
> { |
|
This patch series was integrated into seen via git@58dc249. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just upstreamin'
cc: Patrick Steinhardt [email protected]