{"thread":{"id":"63070","subject":"[PATCH 1/2] t6423: add a testcase causing a failed assertion in process_renames","startedAt":"2025-03-06T15:30:31Z","lastAt":"2025-03-14T00:25:09Z","messageCount":11,"participants":["Dmitry Goncharov via GitGitGadget","Elijah Newren via GitGitGadget","Taylor Blau","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"513708","messageId":"69da363cab9127cbe4fe6843c49c1a41ba9d4521.1741275027.git.gitgitgadget@gmail.com","threadId":"63070","inReplyTo":"pull.1873.git.1741275027.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t6423: add a testcase causing a failed assertion in process_renames","fromName":"Dmitry Goncharov via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-06T15:30:26Z","receivedAt":"2025-03-06T15:30:31Z","isPatch":true,"sender":{"key":"name:Dmitry Goncharov","avatar":null},"body":"From: Dmitry Goncharov \n\nIf one side of history renames a directory A/ -> B/, and the other side\nof history adds new files to A/, then directory rename detection notices\nand moves or suggests moving those new files to B/. A similar thing is\ndone for paths renamed into A/, causing them to be transitively renamed\ninto B/. But, if the file originally came from B/, then this can end up\ncausing a file to be renamed back to itself. merge-ort crashes under\nthis special case, due to a slightly overzealous assertion:\n\n git: merge-ort.c:3051: process_renames: Assertion `source_deleted || oldinfo->filemask & old_sidemask' failed.\n Aborted (core dumped)\n\nAdd a testcase demonstrating this.\n\nSigned-off-by: Dmitry Goncharov \n[en: Instead of adding a new testsuite, place it near similar tests in\n t6423, adjusting to match the style of those tests. Tweak the commit\n message to not repeat the entire testcase, but just describe the bug.\n Also update the line number in the error message.]\nSigned-off-by: Elijah Newren \n---\n t/t6423-merge-rename-directories.sh | 41 +++++++++++++++++++++++++++++\n 1 file changed, 41 insertions(+)\n\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 88d1cf2cde9..259ee9628e4 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5360,6 +5360,47 @@ test_expect_merge_algorithm failure success '12m: Change parent of renamed-dir t\n \t)\n '\n \n+test_setup_12n () {\n+\tgit init 12n &&\n+\t(\n+\t\tcd 12n &&\n+\n+\t\tmkdir tools &&\n+\t\techo hello >tools/hello &&\n+\t\tgit add tools/hello &&\n+\t\tgit commit -m \"O\" &&\n+\n+\t\tgit branch O &&\n+\t\tgit branch A &&\n+\t\tgit branch B &&\n+\n+\t\tgit switch A &&\n+\t\techo world >world &&\n+\t\tgit add world &&\n+\t\tgit commit -q world -m 'Add world' &&\n+\n+\t\tgit mv world tools/world &&\n+\t\tgit commit -m \"Move world into tools/\" &&\n+\n+\t\tgit switch B &&\n+\t\tgit mv tools/hello hello &&\n+\t\tgit commit -m \"Move hello from tools/ to toplevel\"\n+\t)\n+}\n+\n+test_expect_failure '12n: Directory rename transitively makes rename back to self' '\n+\ttest_setup_12n &&\n+\t(\n+\t\tcd 12n &&\n+\n+\t\tgit checkout -q B^0 &&\n+\n+\t\ttest_must_fail git cherry-pick A^0 >out &&\n+\t\tgrep \"CONFLICT (file location).*should perhaps be moved\" out\n+\t)\n+'\n+\n+\n ###########################################################################\n # SECTION 13: Checking informational and conflict messages\n #\n-- \ngitgitgadget\n\n"},{"id":"513709","messageId":"f48b3310d4ae8d05780fd25e467083c4dc9852cc.1741275027.git.gitgitgadget@gmail.com","threadId":"63070","inReplyTo":"pull.1873.git.1741275027.gitgitgadget@gmail.com","subject":"[PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-06T15:30:27Z","receivedAt":"2025-03-06T15:30:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nmerge-ort has a number of sanity checks on the file it is processing in\nprocess_renames(). One of these sanity checks was slightly overzealous\nbecause it indirectly assumed that a renamed file always ended up at a\ndifferent path than where it started. That is normally an entirely fair\nassumption, but directory rename detection can make things interesting.\n\nAs a quick refresher, if one side of history renames directory A/ -> B/,\nand the other side of history adds new files to A/, then directory\nrename detection notices and suggests moving those new files to B/. A\nsimilar thing is done for paths renamed into A/, causing them to be\ntransitively renamed into B/. But, if the file originally came from B/,\nthen this can end up causing a file to be renamed back to itself.\n\nIt turns out the rest of the code following this assertion handled the\ncase fine; the assertion was just an extra sanity check, not a rigid\nprecondition. Therefore, simply adjust the assertion to pass under this\nspecial case as well.\n\nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 3 ++-\n t/t6423-merge-rename-directories.sh | 2 +-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 46e78c3ffa6..b0ff2236af0 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3048,7 +3048,8 @@ static int process_renames(struct merge_options *opt,\n \t\t\t}\n \t\t}\n \n-\t\tassert(source_deleted || oldinfo->filemask & old_sidemask);\n+\t\tassert(source_deleted || oldinfo->filemask & old_sidemask ||\n+\t\t !strcmp(pair->one->path, pair->two->path));\n \n \t\t/* Need to check for special types of rename conflicts... */\n \t\tif (collision && !source_deleted) {\ndiff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh\nindex 259ee9628e4..9cbd41d3c69 100755\n--- a/t/t6423-merge-rename-directories.sh\n+++ b/t/t6423-merge-rename-directories.sh\n@@ -5388,7 +5388,7 @@ test_setup_12n () {\n \t)\n }\n \n-test_expect_failure '12n: Directory rename transitively makes rename back to self' '\n+test_expect_success '12n: Directory rename transitively makes rename back to self' '\n \ttest_setup_12n &&\n \t(\n \t\tcd 12n &&\n-- \ngitgitgadget\n"},{"id":"513710","messageId":"pull.1873.git.1741275027.gitgitgadget@gmail.com","threadId":"63070","inReplyTo":null,"subject":"[PATCH 0/2] merge-ort: fix a crash in process_renames for a file transitively renamed to itself","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-06T15:30:25Z","receivedAt":"2025-03-06T15:30:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Maintainer note: this is not a recent regression; it need not be included in\nv2.49.0.\n\nThis is a follow-up to Dmitry's report of a case where both merge-ort and\nmerge-recursive fail to properly handle a very specific type of conflict.\nSee\nhttps://lore.kernel.org/git/20240921024533.15249-2-dgoncharov@users.sf.net/\nfor the earlier thread; the first patch is an adaptation of his\ndemonstrating the issue, and the second patch is my fix for it.\n\nDmitry Goncharov (1):\n t6423: add a testcase causing a failed assertion in process_renames\n\nElijah Newren (1):\n merge-ort: fix slightly overzealous assertion for rename-to-self\n\n merge-ort.c | 3 ++-\n t/t6423-merge-rename-directories.sh | 41 +++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n\n\nbase-commit: f93ff170b93a1782659637824b25923245ac9dd1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1873%2Fnewren%2Fendit-fix-funny-rename-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1873/newren/endit-fix-funny-rename-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1873\n-- \ngitgitgadget\n"},{"id":"514131","messageId":"Z9ID/2zx25qesuJs@nand.local","threadId":"63070","inReplyTo":"f48b3310d4ae8d05780fd25e467083c4dc9852cc.1741275027.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-12T22:00:31Z","receivedAt":"2025-03-12T22:00:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 06, 2025 at 03:30:27PM +0000, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n>\n> merge-ort has a number of sanity checks on the file it is processing in\n> process_renames(). One of these sanity checks was slightly overzealous\n> because it indirectly assumed that a renamed file always ended up at a\n> different path than where it started. That is normally an entirely fair\n> assumption, but directory rename detection can make things interesting.\n>\n> As a quick refresher, if one side of history renames directory A/ -> B/,\n> and the other side of history adds new files to A/, then directory\n> rename detection notices and suggests moving those new files to B/. A\n> similar thing is done for paths renamed into A/, causing them to be\n> transitively renamed into B/. But, if the file originally came from B/,\n> then this can end up causing a file to be renamed back to itself.\n>\n> It turns out the rest of the code following this assertion handled the\n> case fine; the assertion was just an extra sanity check, not a rigid\n> precondition. Therefore, simply adjust the assertion to pass under this\n> special case as well.\n\nWow, that is a very interesting edge case from Dmitry. The explanation\nand fix makes sense, and yeah, looks like just an over-zealous\nassertion.\n\nAs a meta-comment, I vaguely remember past discussions on the list about\nwhen to assert() versus when to BUG(). I recall that where we landed was\nthat:\n\n if (foo)\n BUG(\"something\");\n\nwas preferable to:\n\n assert(foo);\n\nI know that we don't usually define NDEBUG, but I think there are a\ncouple of cases where we do, like for nedmalloc stuff when building with\nUSE_NED_ALLOCATOR, and in Windows builds if DEBUG is undefined.\n\nSo I don't think it makes a huge practical difference, but it might be\nnice to put in CodingGuidelines or similar.\n\nThanks,\nTaylor\n"},{"id":"514134","messageId":"CABPp-BH+9JcP6FDP6RFQzZbwW=XOWnWAR8PmrCS6hG4iyd2pPQ@mail.gmail.com","threadId":"63070","inReplyTo":"Z9ID/2zx25qesuJs@nand.local","subject":"Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-12T22:36:07Z","receivedAt":"2025-03-12T22:36:19Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 12, 2025 at 3:00 PM Taylor Blau wrote:\n>\n> On Thu, Mar 06, 2025 at 03:30:27PM +0000, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > merge-ort has a number of sanity checks on the file it is processing in\n> > process_renames(). One of these sanity checks was slightly overzealous\n> > because it indirectly assumed that a renamed file always ended up at a\n> > different path than where it started. That is normally an entirely fair\n> > assumption, but directory rename detection can make things interesting.\n> >\n> > As a quick refresher, if one side of history renames directory A/ -> B/,\n> > and the other side of history adds new files to A/, then directory\n> > rename detection notices and suggests moving those new files to B/. A\n> > similar thing is done for paths renamed into A/, causing them to be\n> > transitively renamed into B/. But, if the file originally came from B/,\n> > then this can end up causing a file to be renamed back to itself.\n> >\n> > It turns out the rest of the code following this assertion handled the\n> > case fine; the assertion was just an extra sanity check, not a rigid\n> > precondition. Therefore, simply adjust the assertion to pass under this\n> > special case as well.\n>\n> Wow, that is a very interesting edge case from Dmitry. The explanation\n> and fix makes sense, and yeah, looks like just an over-zealous\n> assertion.\n>\n> As a meta-comment, I vaguely remember past discussions on the list about\n> when to assert() versus when to BUG(). I recall that where we landed was\n> that:\n>\n> if (foo)\n> BUG(\"something\");\n>\n> was preferable to:\n>\n> assert(foo);\n\nI disagree that we landed on that; at least the last time it was\nbrought up to me there was no ending statement of such --\nhttps://lore.kernel.org/git/CABPp-BFLyYTboiGkP=sc7S+stRZvozAqnJPnza+M0q2nakvD2Q@mail.gmail.com/,\nand then I proceeded to continue submitting and getting merged several\nmore assert statements.\n\nIf there was a project-wide directive to remove assert() statements,\nthen I'd quickly create something else (e.g. \"BUG_ON(foo)\") and\nconvert to that rather than \"if (foo) BUG(bar)\". And I'd probably\nmake it depend on NDEBUG too...\n\n> I know that we don't usually define NDEBUG, but I think there are a\n> couple of cases where we do, like for nedmalloc stuff when building with\n> USE_NED_ALLOCATOR, and in Windows builds if DEBUG is undefined.\n\nIf the statement is important to have in production builds, I have\nused \"if (foo) BUG(bar)\", including in merge-ort.c. But I think there\nare different classes of notes-for-other-developers. Code comments\nfor the lightest end, if (foo) BUG (bar) for the heavier end, and\nassertions for somewhere in the middle. I think I've used a higher\ndensity of checks in merge-ort than tend to be used elsewhere, in part\nbecause of the number of optimizations added to the code and the\ndifficulty of keeping track of all the preconditions and assumptions\nwith the extra data structures feeding those. Lots of those checks\nmight have just been code comments somewhere else; they aren't\nnecessarily worth a real if-check. They'd be fine to compile out\ngenerally. In some cases, they might go to the full \"if (foo)\nBUG(bar)\", but I think there's a reasonable middle ground. assert()\nstatements survive and are kept up-to-date better than a code comment,\nthey tend to be clearer than a code comment, and they catch more bugs\nwhile refactoring (or originally developing the code) than code\ncomments. And they are much more brief and ergonomic than the \"if\n(foo) BUG(bar)\" construct (why do I have to come up with a separate\nbar, when it's already embedded in foo?)\n\n> So I don't think it makes a huge practical difference, but it might be\n> nice to put in CodingGuidelines or similar.\n\nMaybe I should put something in there so I don't have to look up the\nold conversations and point out my disagreements with this suggestion\nyet again... ;-)\n\nBut it might be worth mentioning that having side-effects in\nassertions is a huge no-no, and I understand that when folks have to\ndebug one of those (I had to in a separate project years ago which was\nkind of nasty), that they sometimes jump to the conclusion that\nassertions are bad. I understand being bitten, but disagree with the\nconclusion.\n"},{"id":"514135","messageId":"xmqqr031x2gg.fsf@gitster.g","threadId":"63070","inReplyTo":"CABPp-BH+9JcP6FDP6RFQzZbwW=XOWnWAR8PmrCS6hG4iyd2pPQ@mail.gmail.com","subject":"Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-12T23:18:39Z","receivedAt":"2025-03-12T23:18:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> But it might be worth mentioning that having side-effects in\n> assertions is a huge no-no, and I understand that when folks have to\n> debug one of those (I had to in a separate project years ago which was\n> kind of nasty), that they sometimes jump to the conclusion that\n> assertions are bad.\n\nYes, assert() invites such mistakes. Why not avoid them when there\nare better alternatives like \"if (condition) BUG()\"?\n\n"},{"id":"514162","messageId":"CABPp-BF7iTvqKFvuOZ5wZ3vZADjCvB63iKR-4D5+WeTxnZGThg@mail.gmail.com","threadId":"63070","inReplyTo":"xmqqr031x2gg.fsf@gitster.g","subject":"Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-13T06:22:56Z","receivedAt":"2025-03-13T06:23:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Mar 12, 2025 at 4:18 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > But it might be worth mentioning that having side-effects in\n> > assertions is a huge no-no, and I understand that when folks have to\n> > debug one of those (I had to in a separate project years ago which was\n> > kind of nasty), that they sometimes jump to the conclusion that\n> > assertions are bad.\n>\n> Yes, assert() invites such mistakes. Why not avoid them when there\n> are better alternatives like \"if (condition) BUG()\"?\n\nI mean, I just gave my reasons above which you snipped out. But if\nyou feel it is important for folks to move away from assert(), would\nyou like to see someone create a better alternative to assert, such as\nBUG_ON(condition), so that they have reason to want to switch?\n"},{"id":"514209","messageId":"xmqqbju4q39j.fsf@gitster.g","threadId":"63070","inReplyTo":"CABPp-BF7iTvqKFvuOZ5wZ3vZADjCvB63iKR-4D5+WeTxnZGThg@mail.gmail.com","subject":"Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T16:55:20Z","receivedAt":"2025-03-13T16:55:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Wed, Mar 12, 2025 at 4:18 PM Junio C Hamano wrote:\n>>\n>> Elijah Newren writes:\n>>\n>> > But it might be worth mentioning that having side-effects in\n>> > assertions is a huge no-no, and I understand that when folks have to\n>> > debug one of those (I had to in a separate project years ago which was\n>> > kind of nasty), that they sometimes jump to the conclusion that\n>> > assertions are bad.\n>>\n>> Yes, assert() invites such mistakes. Why not avoid them when there\n>> are better alternatives like \"if (condition) BUG()\"?\n>\n> I mean, I just gave my reasons above which you snipped out. But if\n> you feel it is important for folks to move away from assert(), would\n> you like to see someone create a better alternative to assert, such as\n> BUG_ON(condition), so that they have reason to want to switch?\n\nYou said \"BUG_ON()\" is better than \"if (condition) BUG()\", but I do\nnot see a reason why. It also shares the same problem with assert()\nif we make it honor NDEBUG.\n\nThanks.\n"},{"id":"514211","messageId":"CABPp-BF01a8bEZ2mfp7H_=HKsv6mSE4+ee8bi9D9xKWbH=0b5g@mail.gmail.com","threadId":"63070","inReplyTo":"xmqqbju4q39j.fsf@gitster.g","subject":"Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-13T17:15:49Z","receivedAt":"2025-03-13T17:16:00Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Mar 13, 2025 at 9:55 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > On Wed, Mar 12, 2025 at 4:18 PM Junio C Hamano wrote:\n> >>\n> >> Elijah Newren writes:\n> >>\n> >> > But it might be worth mentioning that having side-effects in\n> >> > assertions is a huge no-no, and I understand that when folks have to\n> >> > debug one of those (I had to in a separate project years ago which was\n> >> > kind of nasty), that they sometimes jump to the conclusion that\n> >> > assertions are bad.\n> >>\n> >> Yes, assert() invites such mistakes. Why not avoid them when there\n> >> are better alternatives like \"if (condition) BUG()\"?\n> >\n> > I mean, I just gave my reasons above which you snipped out. But if\n> > you feel it is important for folks to move away from assert(), would\n> > you like to see someone create a better alternative to assert, such as\n> > BUG_ON(condition), so that they have reason to want to switch?\n>\n> You said \"BUG_ON()\" is better than \"if (condition) BUG()\", but I do\n> not see a reason why. It also shares the same problem with assert()\n> if we make it honor NDEBUG.\n\n\"if (condition) BUG()\" is invalid; it needs more arguments. \"if\n(condition) BUG(something)\" requires a separate \"something\", which\nrequires awkward additional wording and/or is needlessly duplicative.\n\nIf you don't want to see assert in the codebase because of NDEBUG,\nthen obviously we'd leave NDEBUG out of BUG_ON().\n\nSuch a BUG_ON() would be an improvement because it maintains the\nergonomics of assert() while avoiding the potential mistake of\naccidentally including something with side-effects. \"If (condition)\nBUG(something)\" doesn't preserve the ergonomics and is thus worse,\nIMO. (Personally, I still like having cheap checks that are okay to\ncompile out, but if we want a project directive against those, then I\nthink a BUG_ON() that doesn't depend on NDEBUG would be the way to\nmove in that direction.)\n"},{"id":"514223","messageId":"xmqqcyekok4g.fsf@gitster.g","threadId":"63070","inReplyTo":"CABPp-BF01a8bEZ2mfp7H_=HKsv6mSE4+ee8bi9D9xKWbH=0b5g@mail.gmail.com","subject":"Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-13T18:34:07Z","receivedAt":"2025-03-13T18:34:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> \"if (condition) BUG()\" is invalid; it needs more arguments. \"if\n> (condition) BUG(something)\" requires a separate \"something\", which\n> requires awkward additional wording and/or is needlessly duplicative.\n\nAh, obviously we differ on that point.\n\nI consider it an advantage that can be more descriptive\nin a developer friendly way than expression alone. If\nyou wrote\n\n\tif (istate->cache_nr < i)\n\t\tBUG(\"ran past the end of the istate->cache[] array?\");\n\nyou can spot a bug of the assertion, because the human-readable part\ntells us the intention that the condition is supposed to be the\nusual array bounds check, and should be checking with \"<=\". In\ncontrast\n\n\tBUG_ON(istate->cache_nr < i);\n\ndoes not give us the extra information we can use to double check\nwhat the developer who wrote it, who may not be with us anymore,\nwanted to really check. Is it a misspelt \"<=\", or does this code\npath use 'i' not just as a variable to iterate over the array but\nalso allows it to go one past its end, so \"<\" is the condition it\nreally wants to validate?\n\n> If you don't want to see assert in the codebase because of NDEBUG,\n> then obviously we'd leave NDEBUG out of BUG_ON().\n\nAbsolutely, because the largest problem with assert() is that the\ncondition can be compiled away while the compiler does not help\nensure that the condition part is free of side effects. If we drop\nNDEBUG, that problem goes away.\n\n> Such a BUG_ON() would be an improvement because it maintains the\n> ergonomics of assert() while avoiding the potential mistake of\n> accidentally including something with side-effects. \n\nYes, we are half on the same page (as I disagree that assert() that\ndoes not give a room to explain why we are checking the condition is\nergonomic at all) and agree that discarding NDEBUG gives us safer\nvariant than assert().\n\n> \"If (condition)\n> BUG(something)\" doesn't preserve the ergonomics and is thus worse,\n\nAgain, this is philosophical difference between us. It gives us the\nsame safety as NDEBUG-less BUG_ON() but allows us to be more\ndescriptive to help developers. While I understand sometimes the\ncondition may be self evident (especially while developing new code,\nwhere the developer thought the problem long and deep) and people\nmay find it tedious to have to explain the reasoning behind the\ncheck, I find it highly problematic not to give room to explain to\nthose who want to. And /* comments */ is not the same thing, as I\nthink it is important to explain your BUG() well, just like it is\ngood practice to explain your commit well in the log message.\n\nOne thing I view as a downside of \"if (condition) BUG(message)\" is\nthat it does not make it clear syntactically that (condition) is\nabout assertion, and you can write\n\n\tif (condition) {\n\t\tdo something;\n\t\tBUG(message);\n\t}\n\nwhich probably is not what you want. I am OK with\n\n\tBUG_ON(, );\n\nwhich would make such a construct impossible.\n\nThanks.\n"},{"id":"514247","messageId":"CABPp-BG-7RYskvG6K+gYYNQxqRkVSQYLPvcFaFaT3u-w3bBrYg@mail.gmail.com","threadId":"63070","inReplyTo":"xmqqcyekok4g.fsf@gitster.g","subject":"Re: [PATCH 2/2] merge-ort: fix slightly overzealous assertion for rename-to-self","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2025-03-14T00:24:56Z","receivedAt":"2025-03-14T00:25:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Mar 13, 2025 at 11:34 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > \"if (condition) BUG()\" is invalid; it needs more arguments. \"if\n> > (condition) BUG(something)\" requires a separate \"something\", which\n> > requires awkward additional wording and/or is needlessly duplicative.\n>\n> Ah, obviously we differ on that point.\n>\n> I consider it an advantage that can be more descriptive\n> in a developer friendly way than expression alone.\n\nI think there's been a communication disconnect. I too like \"if\n(condition) BUG (something)\" and often use it, as I pointed out\nearlier in this thread. The fact that it _can_ be more descriptive is\nan advantage in some situations (and you provided a great example.)\n\nThe problem is that some situations != all situations, and I find it\nworse when trying to force it for all the other cases. If we still\ndisagree on that point, that's fine, but I wanted to make it clear\nthat I am a fan of and use \"if (condition) BUG (something)\" where it\nfits.\n\n[...]\n> > If you don't want to see assert in the codebase because of NDEBUG,\n> > then obviously we'd leave NDEBUG out of BUG_ON().\n>\n> Absolutely, because the largest problem with assert() is that the\n> condition can be compiled away while the compiler does not help\n> ensure that the condition part is free of side effects. If we drop\n> NDEBUG, that problem goes away.\n\nOr we can just add a static analysis job that will error whenever the\ncompiler/linker can't prove that assertions have no side effects, and\nsuggest folks use an alternative macro for those instances. I just\nsubmitted a series to do that over here:\nhttps://lore.kernel.org/git/pull.1881.git.1741911652.gitgitgadget@gmail.com/\n\nThat should be far more thorough than any CodingGuideline at making\nsure we don't have asserts with side effects, be far less effort than\nattempting to change our several hundred existing assert() invocations\nin the code base, and should be enough to consider the problem solved\nand let us move on to something more interesting.\n"}]}