{"thread":{"id":"64282","subject":"[PATCH] refs: forbid clang to complain about unreachable code","startedAt":"2025-10-09T07:46:24Z","lastAt":"2025-10-11T10:49:38Z","messageCount":7,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Patrick Steinhardt","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"528370","messageId":"pull.1984.git.1759995982220.gitgitgadget@gmail.com","threadId":"64282","inReplyTo":null,"subject":"[PATCH] refs: forbid clang to complain about unreachable code","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-10-09T07:46:22Z","receivedAt":"2025-10-09T07:46:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen `NO_SYMLINK_HEAD` is defined, `create_ref_symlink()` is hard-coded\nas `(-1)`, and as a consequence the condition `!create_ref_symlink()`\nalways evaluates to false, rendering any code guarded by that condition\nunreachable.\n\nTherefore, clang is _technically_ correct when it complains about\nunreachable code. It does completely miss the fact that this is okay\nbecause on _other_ platforms, where `NO_SYMLINK_HEAD` is not defined,\nthe code isn't unreachable at all.\n\nLet's use the same trick as in 82e79c63642c (git-compat-util: add\nNOT_CONSTANT macro and use it in atfork_prepare(), 2025-03-17) to\nappease clang while at the same time keeping the `-Wunreachable` flag\nto potentially find _actually_ unreachable code.\n\nSigned-off-by: Johannes Schindelin \n---\n refs: forbid clang to complain about unreachable code\n \n Just upstreamin'\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1984%2Fdscho%2Frefs-clang-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1984/dscho/refs-clang-fix-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1984\n\n refs/files-backend.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 088b52c740..814decf323 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3186,7 +3186,13 @@ static int files_transaction_finish(struct ref_store *ref_store,\n \t\t * next update. If not, we try and create a regular symref.\n \t\t */\n \t\tif (update->new_target && refs->prefer_symlink_refs)\n-\t\t\tif (!create_ref_symlink(lock, update->new_target))\n+\t\t\t/*\n+\t\t\t * By using the `NOT_CONSTANT()` trick, we can avoid\n+\t\t\t * errors by `clang`'s `-Wunreachable` logic that would\n+\t\t\t * report that the `continue` statement is not reachable\n+\t\t\t * when `NO_SYMLINK_HEAD` is `#define`d.\n+\t\t\t */\n+\t\t\tif (NOT_CONSTANT(!create_ref_symlink(lock, update->new_target)))\n \t\t\t\tcontinue;\n \n \t\tif (update->flags & REF_NEEDS_COMMIT) {\n\nbase-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\n-- \ngitgitgadget\n"},{"id":"528399","messageId":"xmqqzf9zddia.fsf@gitster.g","threadId":"64282","inReplyTo":"pull.1984.git.1759995982220.gitgitgadget@gmail.com","subject":"Re: [PATCH] refs: forbid clang to complain about unreachable code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-09T20:30:21Z","receivedAt":"2025-10-09T20:30:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> When `NO_SYMLINK_HEAD` is defined, `create_ref_symlink()` is hard-coded\n> as `(-1)`, and as a consequence the condition `!create_ref_symlink()`\n> always evaluates to false, rendering any code guarded by that condition\n> unreachable.\n>\n> Therefore, clang is _technically_ correct when it complains about\n> unreachable code. It does completely miss the fact that this is okay\n> because on _other_ platforms, where `NO_SYMLINK_HEAD` is not defined,\n> the code isn't unreachable at all.\n>\n> Let's use the same trick as in 82e79c63642c (git-compat-util: add\n> NOT_CONSTANT macro and use it in atfork_prepare(), 2025-03-17) to\n> appease clang while at the same time keeping the `-Wunreachable` flag\n> to potentially find _actually_ unreachable code.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> refs: forbid clang to complain about unreachable code\n> \n> Just upstreamin'\n\nIt may not be a bad idea to deprecate core.preferSymlinkRefs now and\nremove it at Git 3.0 boundary. Some platforms may not be able to do\nsymbolic links and use it to represent HEAD, but everybody should be\nable to create a small text file with a single line.\n\nBut until then, this is a very reasonable thing to do.\n\nThanks.\n\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1984%2Fdscho%2Frefs-clang-fix-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1984/dscho/refs-clang-fix-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1984\n>\n> refs/files-backend.c | 8 +++++++-\n> 1 file changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 088b52c740..814decf323 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -3186,7 +3186,13 @@ static int files_transaction_finish(struct ref_store *ref_store,\n> \t\t * next update. If not, we try and create a regular symref.\n> \t\t */\n> \t\tif (update->new_target && refs->prefer_symlink_refs)\n> -\t\t\tif (!create_ref_symlink(lock, update->new_target))\n> +\t\t\t/*\n> +\t\t\t * By using the `NOT_CONSTANT()` trick, we can avoid\n> +\t\t\t * errors by `clang`'s `-Wunreachable` logic that would\n> +\t\t\t * report that the `continue` statement is not reachable\n> +\t\t\t * when `NO_SYMLINK_HEAD` is `#define`d.\n> +\t\t\t */\n> +\t\t\tif (NOT_CONSTANT(!create_ref_symlink(lock, update->new_target)))\n> \t\t\t\tcontinue;\n> \n> \t\tif (update->flags & REF_NEEDS_COMMIT) {\n>\n> base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0\n"},{"id":"528459","messageId":"aOia55_sjFQjw1UQ@pks.im","threadId":"64282","inReplyTo":"pull.1984.git.1759995982220.gitgitgadget@gmail.com","subject":"Re: [PATCH] refs: forbid clang to complain about unreachable code","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-10T05:34:31Z","receivedAt":"2025-10-10T05:34:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 09, 2025 at 07:46:22AM +0000, Johannes Schindelin via GitGitGadget wrote:\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 088b52c740..814decf323 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -3186,7 +3186,13 @@ static int files_transaction_finish(struct ref_store *ref_store,\n> \t\t * next update. If not, we try and create a regular symref.\n> \t\t */\n> \t\tif (update->new_target && refs->prefer_symlink_refs)\n> -\t\t\tif (!create_ref_symlink(lock, update->new_target))\n> +\t\t\t/*\n> +\t\t\t * By using the `NOT_CONSTANT()` trick, we can avoid\n> +\t\t\t * errors by `clang`'s `-Wunreachable` logic that would\n> +\t\t\t * report that the `continue` statement is not reachable\n> +\t\t\t * when `NO_SYMLINK_HEAD` is `#define`d.\n> +\t\t\t */\n> +\t\t\tif (NOT_CONSTANT(!create_ref_symlink(lock, update->new_target)))\n> \t\t\t\tcontinue;\n\nAn alternative could be to fix this at the source, e.g. like the below\n(untested) patch. But I don't mind this too much, especially given that\nthis here is the only callsite of that function anyway. So please feel\nfree to disregard.\n\nThanks!\n\nPatrick\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bb2bec3807..cb402a2a54 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n }\n \n #ifdef NO_SYMLINK_HEAD\n-#define create_ref_symlink(a, b) (-1)\n+#define create_ref_symlink(a, b) NOT_CONSTANT(-1)\n #else\n static int create_ref_symlink(struct ref_lock *lock, const char *target)\n {\n\n"},{"id":"528460","messageId":"aOibbUqe-gfal6sd@pks.im","threadId":"64282","inReplyTo":"xmqqzf9zddia.fsf@gitster.g","subject":"Re: [PATCH] refs: forbid clang to complain about unreachable code","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-10T05:36:45Z","receivedAt":"2025-10-10T05:36:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Oct 09, 2025 at 01:30:21PM -0700, Junio C Hamano wrote:\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n> \n> > From: Johannes Schindelin \n> >\n> > When `NO_SYMLINK_HEAD` is defined, `create_ref_symlink()` is hard-coded\n> > as `(-1)`, and as a consequence the condition `!create_ref_symlink()`\n> > always evaluates to false, rendering any code guarded by that condition\n> > unreachable.\n> >\n> > Therefore, clang is _technically_ correct when it complains about\n> > unreachable code. It does completely miss the fact that this is okay\n> > because on _other_ platforms, where `NO_SYMLINK_HEAD` is not defined,\n> > the code isn't unreachable at all.\n> >\n> > Let's use the same trick as in 82e79c63642c (git-compat-util: add\n> > NOT_CONSTANT macro and use it in atfork_prepare(), 2025-03-17) to\n> > appease clang while at the same time keeping the `-Wunreachable` flag\n> > to potentially find _actually_ unreachable code.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > refs: forbid clang to complain about unreachable code\n> > \n> > Just upstreamin'\n> \n> It may not be a bad idea to deprecate core.preferSymlinkRefs now and\n> remove it at Git 3.0 boundary. Some platforms may not be able to do\n> symbolic links and use it to represent HEAD, but everybody should be\n> able to create a small text file with a single line.\n> \n> But until then, this is a very reasonable thing to do.\n\nAgreed. I don't see any reason why anyone would like to use symbolic\nrefs for this. The reading side for such symrefs may continue to exist\nfor a while. But the writing side can go away.\n\nPatrick\n"},{"id":"528507","messageId":"2346bcc7-b60e-0ad6-14fe-c0b49e427921@gmx.de","threadId":"64282","inReplyTo":"aOia55_sjFQjw1UQ@pks.im","subject":"Re: [PATCH] refs: forbid clang to complain about unreachable code","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-10-10T13:49:37Z","receivedAt":"2025-10-10T13:49:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Fri, 10 Oct 2025, Patrick Steinhardt wrote:\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index bb2bec3807..cb402a2a54 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n> }\n> \n> #ifdef NO_SYMLINK_HEAD\n> -#define create_ref_symlink(a, b) (-1)\n> +#define create_ref_symlink(a, b) NOT_CONSTANT(-1)\n> #else\n> static int create_ref_symlink(struct ref_lock *lock, const char *target)\n> {\n\nWhile this is correct, and \"closer to the root\", in my experience it is\nbetter to have work-arounds closer to where the symptom appears. In this\ncase, it would be directly in the condition of the `if ()` construct.\nTherefore, I would prefer to keep the proposed patch as-is.\n\nCiao,\nJohannes\n"},{"id":"528516","messageId":"xmqqwm52ahav.fsf@gitster.g","threadId":"64282","inReplyTo":"aOia55_sjFQjw1UQ@pks.im","subject":"Re: [PATCH] refs: forbid clang to complain about unreachable code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-10T15:48:56Z","receivedAt":"2025-10-10T15:48:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> An alternative could be to fix this at the source, e.g. like the below\n> (untested) patch. But I don't mind this too much, especially given that\n> this here is the only callsite of that function anyway. So please feel\n> free to disregard.\n>\n> Thanks!\n\nIndeed that is much nicer as it targets only NO_SYMLINK_HEAD case\nwithout affecting the other side of #ifdef/#else/#endif but as you\nsay, I think this falls into \"once the code is written one way, it\nis not worth revisiting to change it with more iterations\" category.\n\nThanks for very sharp eyes, nevertheless.\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index bb2bec3807..cb402a2a54 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n> }\n> \n> #ifdef NO_SYMLINK_HEAD\n> -#define create_ref_symlink(a, b) (-1)\n> +#define create_ref_symlink(a, b) NOT_CONSTANT(-1)\n> #else\n> static int create_ref_symlink(struct ref_lock *lock, const char *target)\n> {\n"},{"id":"528567","messageId":"aOo2Os8Q0X3WaLM0@pks.im","threadId":"64282","inReplyTo":"2346bcc7-b60e-0ad6-14fe-c0b49e427921@gmx.de","subject":"Re: [PATCH] refs: forbid clang to complain about unreachable code","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-10-11T10:49:30Z","receivedAt":"2025-10-11T10:49:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Oct 10, 2025 at 03:49:37PM +0200, Johannes Schindelin wrote:\n> Hi Patrick,\n> \n> On Fri, 10 Oct 2025, Patrick Steinhardt wrote:\n> \n> > diff --git a/refs/files-backend.c b/refs/files-backend.c\n> > index bb2bec3807..cb402a2a54 100644\n> > --- a/refs/files-backend.c\n> > +++ b/refs/files-backend.c\n> > @@ -2115,7 +2115,7 @@ static int commit_ref_update(struct files_ref_store *refs,\n> > }\n> > \n> > #ifdef NO_SYMLINK_HEAD\n> > -#define create_ref_symlink(a, b) (-1)\n> > +#define create_ref_symlink(a, b) NOT_CONSTANT(-1)\n> > #else\n> > static int create_ref_symlink(struct ref_lock *lock, const char *target)\n> > {\n> \n> While this is correct, and \"closer to the root\", in my experience it is\n> better to have work-arounds closer to where the symptom appears. In this\n> case, it would be directly in the condition of the `if ()` construct.\n> Therefore, I would prefer to keep the proposed patch as-is.\n\nAs said, I don't feel strongly about it, so this is totally fine with\nme. Thanks!\n\nPatrick\n"}]}