{"thread":{"id":"62479","subject":"[PATCH] diff: update conflict handling for whitespace to issue a warning","startedAt":"2024-11-11T17:49:59Z","lastAt":"2024-11-20T01:23:04Z","messageCount":11,"participants":["Usman Akinyemi via GitGitGadget","Junio C Hamano","Phillip Wood","Usman Akinyemi"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"507064","messageId":"pull.1828.git.git.1731347396097.gitgitgadget@gmail.com","threadId":"62479","inReplyTo":null,"subject":"[PATCH] diff: update conflict handling for whitespace to issue a warning","fromName":"Usman Akinyemi via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-11T17:49:55Z","receivedAt":"2024-11-11T17:49:59Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"From: Usman Akinyemi \n\nModify the conflict resolution between tab-in-indent and\nindent-with-non-tab to issue a warning instead of terminating\nthe operation with `die()`. Update the `git diff --check` test to\ncapture and verify the warning message output.\n\nSuggested-by: Phillip Wood \nSigned-off-by: Usman Akinyemi \n---\n diff: update conflict handling for whitespace to issue a warning\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1828%2FUnique-Usman%2Fmaster-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1828/Unique-Usman/master-v1\nPull-Request: https://github.com/git/git/pull/1828\n\n t/t4015-diff-whitespace.sh | 3 ++-\n ws.c | 7 +++++--\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 851cfe4f32c..ada3f90b288 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -808,7 +808,8 @@ test_expect_success 'ditto, but tabwidth=1 (must be irrelevant)' '\n test_expect_success 'check tab-in-indent and indent-with-non-tab conflict' '\n \tgit config core.whitespace \"tab-in-indent,indent-with-non-tab\" &&\n \techo \"foo ();\" >x &&\n-\ttest_must_fail git diff --check\n+\tgit diff --check 2>error &&\n+\ttest_grep \"warning: cannot enforce both tab-in-indent and indent-with-non-tab, removing tab-in-indent\" error\n '\n \n test_expect_success 'check tab-in-indent excluded from wildcard whitespace attribute' '\ndiff --git a/ws.c b/ws.c\nindex 9456e2fdbe3..2c11715177e 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -6,6 +6,7 @@\n #include \"git-compat-util.h\"\n #include \"attr.h\"\n #include \"strbuf.h\"\n+#include \"gettext.h\"\n #include \"ws.h\"\n \n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n@@ -70,8 +71,10 @@ unsigned parse_whitespace_rule(const char *string)\n \t\tstring = ep;\n \t}\n \n-\tif (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB)\n-\t\tdie(\"cannot enforce both tab-in-indent and indent-with-non-tab\");\n+\tif (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB) {\n+\t\twarning(_(\"cannot enforce both tab-in-indent and indent-with-non-tab, removing tab-in-indent\"));\n+\t\trule &= ~WS_TAB_IN_INDENT;\n+\t}\n \treturn rule;\n }\n \n\nbase-commit: facbe4f633e4ad31e641f64617bc88074c659959\n-- \ngitgitgadget\n"},{"id":"507091","messageId":"xmqqfrnx9v9l.fsf@gitster.g","threadId":"62479","inReplyTo":"pull.1828.git.git.1731347396097.gitgitgadget@gmail.com","subject":"Re: [PATCH] diff: update conflict handling for whitespace to issue a warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-11T23:59:50Z","receivedAt":"2024-11-11T23:59:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Usman Akinyemi via GitGitGadget\" writes:\n\n> From: Usman Akinyemi \n>\n> Modify the conflict resolution between tab-in-indent and\n> indent-with-non-tab to issue a warning instead of terminating\n> the operation with `die()`. Update the `git diff --check` test to\n> capture and verify the warning message output.\n\nHmph, giving a warning against these conflicting setting (instead of\ndying) and continuing _may_ make sense sometimes, but it is unclear\nwhich one should survive.\n\nI do not think of a scenario in which it makes much sense to let the\nprogram warn only on one but not the other one. Perhaps disabling\nboth, if we were to do the \"warn and keep going, instead of dying\",\nmay make some more sense than that. I dunno.\n\n\n"},{"id":"507230","messageId":"pull.1828.v2.git.git.1731524467045.gitgitgadget@gmail.com","threadId":"62479","inReplyTo":"pull.1828.git.git.1731347396097.gitgitgadget@gmail.com","subject":"[PATCH v2] diff: update conflict handling for whitespace to issue a warning","fromName":"Usman Akinyemi via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-13T19:01:06Z","receivedAt":"2024-11-13T19:01:10Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"From: Usman Akinyemi \n\nModify the conflict resolution between tab-in-indent and\nindent-with-non-tab to issue a warning instead of terminating\nthe operation with `die()`. Update the `git diff --check` test to\ncapture and verify the warning message output.\n\nSuggested-by: Phillip Wood \nSigned-off-by: Usman Akinyemi \n---\n diff: update conflict handling for whitespace to issue a warning\n \n Changes from V1\n \n * Disable both WS_TAB_IN_INDENT and WS_INDENT_WITH_NON_TAB when both\n are set.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1828%2FUnique-Usman%2Fmaster-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1828/Unique-Usman/master-v2\nPull-Request: https://github.com/git/git/pull/1828\n\nRange-diff vs v1:\n\n 1: dfb80a7ff2d ! 1: 8531e80811c diff: update conflict handling for whitespace to issue a warning\n @@ t/t4015-diff-whitespace.sh: test_expect_success 'ditto, but tabwidth=1 (must be\n \techo \"foo ();\" >x &&\n -\ttest_must_fail git diff --check\n +\tgit diff --check 2>error &&\n -+\ttest_grep \"warning: cannot enforce both tab-in-indent and indent-with-non-tab, removing tab-in-indent\" error\n ++\ttest_grep \"warning: cannot enforce both tab-in-indent and indent-with-non-tab, disabling both\" error\n '\n \n test_expect_success 'check tab-in-indent excluded from wildcard whitespace attribute' '\n @@ ws.c: unsigned parse_whitespace_rule(const char *string)\n -\tif (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB)\n -\t\tdie(\"cannot enforce both tab-in-indent and indent-with-non-tab\");\n +\tif (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB) {\n -+\t\twarning(_(\"cannot enforce both tab-in-indent and indent-with-non-tab, removing tab-in-indent\"));\n ++\t\twarning(_(\"cannot enforce both tab-in-indent and indent-with-non-tab, disabling both\"));\n +\t\trule &= ~WS_TAB_IN_INDENT;\n ++\t\trule &= ~WS_INDENT_WITH_NON_TAB;\n +\t}\n \treturn rule;\n }\n\n\n t/t4015-diff-whitespace.sh | 3 ++-\n ws.c | 8 ++++++--\n 2 files changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh\nindex 851cfe4f32c..849f1854fb9 100755\n--- a/t/t4015-diff-whitespace.sh\n+++ b/t/t4015-diff-whitespace.sh\n@@ -808,7 +808,8 @@ test_expect_success 'ditto, but tabwidth=1 (must be irrelevant)' '\n test_expect_success 'check tab-in-indent and indent-with-non-tab conflict' '\n \tgit config core.whitespace \"tab-in-indent,indent-with-non-tab\" &&\n \techo \"foo ();\" >x &&\n-\ttest_must_fail git diff --check\n+\tgit diff --check 2>error &&\n+\ttest_grep \"warning: cannot enforce both tab-in-indent and indent-with-non-tab, disabling both\" error\n '\n \n test_expect_success 'check tab-in-indent excluded from wildcard whitespace attribute' '\ndiff --git a/ws.c b/ws.c\nindex 9456e2fdbe3..3e9ce55d095 100644\n--- a/ws.c\n+++ b/ws.c\n@@ -6,6 +6,7 @@\n #include \"git-compat-util.h\"\n #include \"attr.h\"\n #include \"strbuf.h\"\n+#include \"gettext.h\"\n #include \"ws.h\"\n \n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n@@ -70,8 +71,11 @@ unsigned parse_whitespace_rule(const char *string)\n \t\tstring = ep;\n \t}\n \n-\tif (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB)\n-\t\tdie(\"cannot enforce both tab-in-indent and indent-with-non-tab\");\n+\tif (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB) {\n+\t\twarning(_(\"cannot enforce both tab-in-indent and indent-with-non-tab, disabling both\"));\n+\t\trule &= ~WS_TAB_IN_INDENT;\n+\t\trule &= ~WS_INDENT_WITH_NON_TAB;\n+\t}\n \treturn rule;\n }\n \n\nbase-commit: facbe4f633e4ad31e641f64617bc88074c659959\n-- \ngitgitgadget\n"},{"id":"507248","messageId":"xmqq4j4a8srw.fsf@gitster.g","threadId":"62479","inReplyTo":"pull.1828.v2.git.git.1731524467045.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] diff: update conflict handling for whitespace to issue a warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-14T02:15:47Z","receivedAt":"2024-11-14T02:15:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Usman Akinyemi via GitGitGadget\" writes:\n\n[jc: As Phillip is blamed for suggesting this addition, I added him\nto the recipient of this message.]\n\n> From: Usman Akinyemi \n>\n> Modify the conflict resolution between tab-in-indent and\n> indent-with-non-tab to issue a warning instead of terminating\n> the operation with `die()`. Update the `git diff --check` test to\n> capture and verify the warning message output.\n>\n> Suggested-by: Phillip Wood \n> Signed-off-by: Usman Akinyemi \n> ---\n\nIf the settings requires an impossible way to use whitespaces, the\nsettings is buggy, and it generally would be better to correct the\nsetting before moving on.\n\nI am curious to know in what situations this new behaviour can be\nseen as an improvement. It may allow you to go on _without_ fixing\nsuch a broken setting, but how would it help the end user? If the\nuser set both of these mutually-incompatible options A and B by\nmistake, but what the user really wanted to check for was A, picking\njust one of A or B arbitrarily and disabling it would not help, and\ndisabling both would not help, either. But wouldn't the real source\nof the problem be that we are trying to demote die() to force the\nuser to correct contradictiong setting into warning()?\n\nThanks.\n"},{"id":"507263","messageId":"29c81cbc-3678-4b70-9e0e-c500186d159f@gmail.com","threadId":"62479","inReplyTo":"xmqq4j4a8srw.fsf@gitster.g","subject":"Re: [PATCH v2] diff: update conflict handling for whitespace to issue a warning","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-11-14T10:06:12Z","receivedAt":"2024-11-14T10:06:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/11/2024 02:15, Junio C Hamano wrote:\n> \"Usman Akinyemi via GitGitGadget\" writes:\n> \n> [jc: As Phillip is blamed for suggesting this addition, I added him\n> to the recipient of this message.]\n\nThanks\n\n>> From: Usman Akinyemi \n>>\n>> Modify the conflict resolution between tab-in-indent and\n>> indent-with-non-tab to issue a warning instead of terminating\n>> the operation with `die()`. Update the `git diff --check` test to\n>> capture and verify the warning message output.\n\nUsman - when you're writing a commit message it is important to explain \nthe reason for making the changes contained in the patch so others can \nunderstand why it is a good idea. In this case the idea is to avoid \nbreaking \"git diff\" for everyone who clones a repository containing a \n.gitattributes file with bad whitespace attributes [1]. As I mentioned \nin [2] I think we only want to change the behavior when parsing \nwhitespace attributes - we still want the other callers of \nparse_whitespace_rule() to die() so the user can fix their config or \ncommandline. We can do that by adding a boolean parameter called \n\"gentle\" that determines whether we call warning() or die().\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://lore.kernel.org/git/e4a70501-af2d-450a-a232-4c7952196a74@gmail.com\n[2] \nhttps://lore.kernel.org/git/3c081d3c-3f6f-45ff-b254-09f1cd6b7de5@gmail.com\n\n>> Suggested-by: Phillip Wood \n>> Signed-off-by: Usman Akinyemi \n>> ---\n> \n> If the settings requires an impossible way to use whitespaces, the\n> settings is buggy, and it generally would be better to correct the\n> setting before moving on.\n> \n> I am curious to know in what situations this new behaviour can be\n> seen as an improvement. It may allow you to go on _without_ fixing\n> such a broken setting, but how would it help the end user? If the\n> user set both of these mutually-incompatible options A and B by\n> mistake, but what the user really wanted to check for was A, picking\n> just one of A or B arbitrarily and disabling it would not help, and\n> disabling both would not help, either. But wouldn't the real source\n> of the problem be that we are trying to demote die() to force the\n> user to correct contradictiong setting into warning()?\n> \n> Thanks.\n\n"},{"id":"507265","messageId":"CAPSxiM9ejv-ZHHu3UkB-ktokae9w7HiUxmByMRUVMSbG1u5Nxg@mail.gmail.com","threadId":"62479","inReplyTo":"29c81cbc-3678-4b70-9e0e-c500186d159f@gmail.com","subject":"Re: [PATCH v2] diff: update conflict handling for whitespace to issue a warning","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2024-11-14T11:29:55Z","receivedAt":"2024-11-14T11:30:07Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"On Thu, Nov 14, 2024 at 5:06 AM Phillip Wood wrote:\n>\n> On 14/11/2024 02:15, Junio C Hamano wrote:\n> > \"Usman Akinyemi via GitGitGadget\" writes:\n> >\n> > [jc: As Phillip is blamed for suggesting this addition, I added him\n> > to the recipient of this message.]\n>\n> Thanks\nHi Philip and Junio,\n\n>\n> >> From: Usman Akinyemi \n> >>\n> >> Modify the conflict resolution between tab-in-indent and\n> >> indent-with-non-tab to issue a warning instead of terminating\n> >> the operation with `die()`. Update the `git diff --check` test to\n> >> capture and verify the warning message output.\n>\n> Usman - when you're writing a commit message it is important to explain\n> the reason for making the changes contained in the patch so others can\n> understand why it is a good idea. In this case the idea is to avoid\n> breaking \"git diff\" for everyone who clones a repository containing a\n> .gitattributes file with bad whitespace attributes [1]. As I mentioned\n> in [2] I think we only want to change the behavior when parsing\n> whitespace attributes - we still want the other callers of\n> parse_whitespace_rule() to die() so the user can fix their config or\n> commandline. We can do that by adding a boolean parameter called\n> \"gentle\" that determines whether we call warning() or die().\n\nI am very sorry for the confusion. I will take this into consideration\nnext time and always give more explanation\nin commit messages.\n\nI will make the necessary changes.\n\nThank you very much.\nUsman.\n\n>\n> Best Wishes\n>\n> Phillip\n>\n> [1]\n> https://lore.kernel.org/git/e4a70501-af2d-450a-a232-4c7952196a74@gmail.com\n> [2]\n> https://lore.kernel.org/git/3c081d3c-3f6f-45ff-b254-09f1cd6b7de5@gmail.com\n>\n> >> Suggested-by: Phillip Wood \n> >> Signed-off-by: Usman Akinyemi \n> >> ---\n> >\n> > If the settings requires an impossible way to use whitespaces, the\n> > settings is buggy, and it generally would be better to correct the\n> > setting before moving on.\n> >\n> > I am curious to know in what situations this new behaviour can be\n> > seen as an improvement. It may allow you to go on _without_ fixing\n> > such a broken setting, but how would it help the end user? If the\n> > user set both of these mutually-incompatible options A and B by\n> > mistake, but what the user really wanted to check for was A, picking\n> > just one of A or B arbitrarily and disabling it would not help, and\n> > disabling both would not help, either. But wouldn't the real source\n> > of the problem be that we are trying to demote die() to force the\n> > user to correct contradictiong setting into warning()?\n> >\n> > Thanks.\n>\n"},{"id":"507302","messageId":"xmqqbjyh5pa5.fsf@gitster.g","threadId":"62479","inReplyTo":"29c81cbc-3678-4b70-9e0e-c500186d159f@gmail.com","subject":"Re: [PATCH v2] diff: update conflict handling for whitespace to issue a warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-15T00:11:46Z","receivedAt":"2024-11-15T00:11:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Usman - when you're writing a commit message it is important to\n> explain the reason for making the changes contained in the patch so\n> others can understand why it is a good idea. In this case the idea is\n> to avoid breaking \"git diff\" for everyone who clones a repository\n> containing a .gitattributes file with bad whitespace attributes\n> [1].\n\nHmph, it would certainly be a problem, but the right solution is not\nto butcher Git, but to make it easier for the participants of such a\nproject to know what is broken *and* what needs to be updated, to let\nthem move forward, no?\n\n> As I mentioned in [2] I think we only want to change the behavior\n> when parsing whitespace attributes - we still want the other callers\n> of parse_whitespace_rule() to die() so the user can fix their config\n> or commandline. We can do that by adding a boolean parameter called\n> \"gentle\" that determines whether we call warning() or die().\n\nI doubt that such a complexity is warranted.\n\nIt depends on the size of diff you are showing, but if it is large,\nthen giving a small warning that gets buried in the large diff is a\nconter-productive way to encourage users to correct such broken\nsetting. If it is small, then the damage may not be too bad, but\nstill, we are showing what the user did not really request.\n\nIf we were to fix anything, it is to make sure that we die() before\nproducing a single line of output. If you have a change to a path\nwhose \"type\" is without such a misconfigured attribute, that sorts\nlexicographically earlier than another path with a change, with a\nconflicting whitespace attribute, I suspect that with the way the\ncode is structured currently, we show the diff for the first path,\nbefore realizing that the second path has an issue and then die.\n\nIf we fix it, and then make sure that the die() message shows\nclearly what attribute setting we did not like, that would be\nsufficient to help users to locate the problem, fix it, and quickly\nmove on, no?\n\nThanks.\n"},{"id":"507535","messageId":"CAPSxiM-H378tKrnLqiTYaWbGb9fPitRzqVpBf+7+Tu03Th3UPg@mail.gmail.com","threadId":"62479","inReplyTo":"xmqqbjyh5pa5.fsf@gitster.g","subject":"Re: [PATCH v2] diff: update conflict handling for whitespace to issue a warning","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2024-11-18T21:03:52Z","receivedAt":"2024-11-18T21:04:03Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"On Fri, Nov 15, 2024 at 12:11 AM Junio C Hamano wrote:\n>\n> Phillip Wood writes:\n>\n> > Usman - when you're writing a commit message it is important to\n> > explain the reason for making the changes contained in the patch so\n> > others can understand why it is a good idea. In this case the idea is\n> > to avoid breaking \"git diff\" for everyone who clones a repository\n> > containing a .gitattributes file with bad whitespace attributes\n> > [1].\n>\n> Hmph, it would certainly be a problem, but the right solution is not\n> to butcher Git, but to make it easier for the participants of such a\n> project to know what is broken *and* what needs to be updated, to let\n> them move forward, no?\n>\n> > As I mentioned in [2] I think we only want to change the behavior\n> > when parsing whitespace attributes - we still want the other callers\n> > of parse_whitespace_rule() to die() so the user can fix their config\n> > or commandline. We can do that by adding a boolean parameter called\n> > \"gentle\" that determines whether we call warning() or die().\n>\n> I doubt that such a complexity is warranted.\n>\n> It depends on the size of diff you are showing, but if it is large,\n> then giving a small warning that gets buried in the large diff is a\n> conter-productive way to encourage users to correct such broken\n> setting. If it is small, then the damage may not be too bad, but\n> still, we are showing what the user did not really request.\n>\n> If we were to fix anything, it is to make sure that we die() before\n> producing a single line of output. If you have a change to a path\n> whose \"type\" is without such a misconfigured attribute, that sorts\n> lexicographically earlier than another path with a change, with a\n> conflicting whitespace attribute, I suspect that with the way the\n> code is structured currently, we show the diff for the first path,\n> before realizing that the second path has an issue and then die.\n>\n> If we fix it, and then make sure that the die() message shows\n> clearly what attribute setting we did not like, that would be\n> sufficient to help users to locate the problem, fix it, and quickly\n> move on, no?\nHi Junio,\n\nThanks for the review. From what I understand from your comment,\nwe should leave it the way it was which was die right ?\n\nThanks.\nUsman.\n>\n> Thanks.\n"},{"id":"507557","messageId":"xmqqcyishxf1.fsf@gitster.g","threadId":"62479","inReplyTo":"CAPSxiM-H378tKrnLqiTYaWbGb9fPitRzqVpBf+7+Tu03Th3UPg@mail.gmail.com","subject":"Re: [PATCH v2] diff: update conflict handling for whitespace to issue a warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-19T00:36:34Z","receivedAt":"2024-11-19T00:36:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Usman Akinyemi writes:\n\n> On Fri, Nov 15, 2024 at 12:11 AM Junio C Hamano wrote:\n>> ...\n>> If we were to fix anything, it is to make sure that we die() before\n>> producing a single line of output. If you have a change to a path\n>> whose \"type\" is without such a misconfigured attribute, that sorts\n>> lexicographically earlier than another path with a change, with a\n>> conflicting whitespace attribute, I suspect that with the way the\n>> code is structured currently, we show the diff for the first path,\n>> before realizing that the second path has an issue and then die.\n>>\n>> If we fix it, and then make sure that the die() message shows\n>> clearly what attribute setting we did not like, that would be\n>> sufficient to help users to locate the problem, fix it, and quickly\n>> move on, no?\n>\n> Thanks for the review. From what I understand from your comment,\n> we should leave it the way it was which was die right ?\n\nCorrect. I do not think replacing die() with warning() without\ndoing anything else makes sense. Making sure that we detect the\nbreakage before going half-way while producing a patch that touches\nmany paths may improve the end-user experience, though.\n\nThanks.\n"},{"id":"507631","messageId":"dc092d9e-d95c-4635-b4f9-85cf1802e571@gmail.com","threadId":"62479","inReplyTo":"xmqqbjyh5pa5.fsf@gitster.g","subject":"Re: [PATCH v2] diff: update conflict handling for whitespace to issue a warning","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-11-19T16:49:05Z","receivedAt":"2024-11-19T16:49:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 15/11/2024 00:11, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> Usman - when you're writing a commit message it is important to\n>> explain the reason for making the changes contained in the patch so\n>> others can understand why it is a good idea. In this case the idea is\n>> to avoid breaking \"git diff\" for everyone who clones a repository\n>> containing a .gitattributes file with bad whitespace attributes\n>> [1].\n> \n> Hmph, it would certainly be a problem, but the right solution is not\n> to butcher Git, but to make it easier for the participants of such a\n> project to know what is broken *and* what needs to be updated, to let\n> them move forward, no?\n\nArguably yes, but that's not the approach we take when the attributes \nfile is too large, a line in the file is is too long or the file \ncontains a negative filename pattern. For those cases we print a warning \nand continue. The recently merged e36f009e69b (merge: replace atoi() \nwith strtol_i() for marker size validation, 2024-10-24) followed suit \nand warns rather than dies for an invalid marker size. It would be nice \nto be consistent in the way we treat invalid attributes. Consistently \ndying and telling the user how to fix the problem would be a reasonable \napproach on the client side but I wonder if it could cause problems for \nforges running \"git diff\" and \"git merge-tree\" on a server though.\n\n> [...]\n> If we were to fix anything, it is to make sure that we die() before\n> producing a single line of output.\n\nThat would certainly be a good idea\n\nBest Wishes\n\nPhillip\n\n"},{"id":"507671","messageId":"xmqqmshuaebt.fsf@gitster.g","threadId":"62479","inReplyTo":"dc092d9e-d95c-4635-b4f9-85cf1802e571@gmail.com","subject":"Re: [PATCH v2] diff: update conflict handling for whitespace to issue a warning","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-20T01:23:02Z","receivedAt":"2024-11-20T01:23:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Arguably yes, but that's not the approach we take when the attributes\n> file is too large, a line in the file is is too long or the file\n> contains a negative filename pattern. For those cases we print a\n> warning and continue. The recently merged e36f009e69b (merge: replace\n> atoi() with strtol_i() for marker size validation, 2024-10-24)\n> followed suit and warns rather than dies for an invalid marker\n> size. It would be nice to be consistent in the way we treat invalid\n> attributes.\n\nArguably yes, but being careful when adding a new check and changing\nestablished behaviour, risking to break existing users, are different.\n\n> Consistently dying and telling the user how to fix the\n> problem would be a reasonable approach on the client side but I wonder\n> if it could cause problems for forges running \"git diff\" and \"git\n> merge-tree\" on a server though.\n\nThat's an interesting aspect. I wonder what happens when somebody\npushes a project with a .gitattributes with such a conflicting\nsetting to GitHub or GitLab.\n\nWould that bring the world to its end ;-)?\n\n"}]}