diff: update conflict handling for whitespace to issue a warning

PATCH Superseded Usman Akinyemi via GitGitGadget 4 participants 11 messages Nov 11, 2024 – Nov 20, 2024

lore

Share & tools

lore

Usman Akinyemi via GitGitGadget

author

2024-11-11 17:49 UTC #0 lore raw

PATCH 1/1 diff: update conflict handling for whitespace to issue a warning

From: Usman Akinyemi <[email protected]>

Modify the conflict resolution between tab-in-indent and indent-with-non-tab to issue a warning instead of terminating the operation with die(). Update the git diff --check test to capture and verify the warning message output.

Signed-off-by Usman Akinyemi <[email protected]>

---
diff: update conflict handling for whitespace to issue a warning

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1828%2FUnique-Usman%2Fmaster-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1828/Unique-Usman/master-v1
Pull-Request: https://github.com/git/git/pull/1828

 t/t4015-diff-whitespace.sh | 3 ++-
 ws.c                       | 7 +++++--
 2 files changed, 7 insertions(+), 3 deletions(-)

t/t4015-diff-whitespace.sh +2 -1

index 851cfe4f32c..ada3f90b288 100755
@@ -808,7 +808,8 @@ test_expect_success 'ditto, but tabwidth=1 (must be irrelevant)' '
 test_expect_success 'check tab-in-indent and indent-with-non-tab conflict' '
 	git config core.whitespace "tab-in-indent,indent-with-non-tab" &&
 	echo "foo ();" >x &&
-	test_must_fail git diff --check
+	git diff --check 2>error &&
+	test_grep "warning: cannot enforce both tab-in-indent and indent-with-non-tab, removing tab-in-indent" error
 '
 
 test_expect_success 'check tab-in-indent excluded from wildcard whitespace attribute' '

ws.c +5 -2

index 9456e2fdbe3..2c11715177e 100644
@@ -6,6 +6,7 @@
 #include "git-compat-util.h"
 #include "attr.h"
 #include "strbuf.h"
+#include "gettext.h"
 #include "ws.h"
 
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;

@@ -70,8 +71,10 @@ unsigned parse_whitespace_rule(const char *string)
 		string = ep;
 	}
 
-	if (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB)
-		die("cannot enforce both tab-in-indent and indent-with-non-tab");
+	if (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB) {
+		warning(_("cannot enforce both tab-in-indent and indent-with-non-tab, removing tab-in-indent"));
+		rule &= ~WS_TAB_IN_INDENT;
+	}
 	return rule;
 }

base-commit: facbe4f633e4ad31e641f64617bc88074c659959

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-11-11 23:59 UTC #1 lore raw

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>
Modify the conflict resolution between tab-in-indent and indent-with-non-tab to issue a warning instead of terminating the operation with die(). Update the git diff --check test to capture and verify the warning message output.

Hmph, giving a warning against these conflicting setting (instead of dying) and continuing _may_ make sense sometimes, but it is unclear which one should survive.

I do not think of a scenario in which it makes much sense to let the program warn only on one but not the other one. Perhaps disabling both, if we were to do the "warn and keep going, instead of dying", may make some more sense than that. I dunno.

Usman Akinyemi via GitGitGadget

author

2024-11-13 19:01 UTC #2 lore raw

PATCH v2 1/1 diff: update conflict handling for whitespace to issue a warning

In reply to Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

Signed-off-by Usman Akinyemi <[email protected]>

---
diff: update conflict handling for whitespace to issue a warning

Changes from V1

* Disable both WS_TAB_IN_INDENT and WS_INDENT_WITH_NON_TAB when both are set.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1828%2FUnique-Usman%2Fmaster-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1828/Unique-Usman/master-v2
Pull-Request: https://github.com/git/git/pull/1828

Range-diff vs v1:

1: dfb80a7ff2d ! 1: 8531e80811c diff: update conflict handling for whitespace to issue a warning

     @@ t/t4015-diff-whitespace.sh: test_expect_success 'ditto, but tabwidth=1 (must be
       	echo "foo ();" >x &&
      -	test_must_fail git diff --check
      +	git diff --check 2>error &&
     -+	test_grep "warning: cannot enforce both tab-in-indent and indent-with-non-tab, removing tab-in-indent" error
     ++	test_grep "warning: cannot enforce both tab-in-indent and indent-with-non-tab, disabling both" error
       '
       
       test_expect_success 'check tab-in-indent excluded from wildcard whitespace attribute' '
     @@ ws.c: unsigned parse_whitespace_rule(const char *string)
      -	if (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB)
      -		die("cannot enforce both tab-in-indent and indent-with-non-tab");
      +	if (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB) {
     -+		warning(_("cannot enforce both tab-in-indent and indent-with-non-tab, removing tab-in-indent"));
     ++		warning(_("cannot enforce both tab-in-indent and indent-with-non-tab, disabling both"));
      +		rule &= ~WS_TAB_IN_INDENT;
     ++		rule &= ~WS_INDENT_WITH_NON_TAB;
      +	}
       	return rule;
       }

 t/t4015-diff-whitespace.sh | 3 ++-
 ws.c                       | 8 ++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

t/t4015-diff-whitespace.sh +2 -1

index 851cfe4f32c..849f1854fb9 100755
@@ -808,7 +808,8 @@ test_expect_success 'ditto, but tabwidth=1 (must be irrelevant)' '
 test_expect_success 'check tab-in-indent and indent-with-non-tab conflict' '
 	git config core.whitespace "tab-in-indent,indent-with-non-tab" &&
 	echo "foo ();" >x &&
-	test_must_fail git diff --check
+	git diff --check 2>error &&
+	test_grep "warning: cannot enforce both tab-in-indent and indent-with-non-tab, disabling both" error
 '
 
 test_expect_success 'check tab-in-indent excluded from wildcard whitespace attribute' '

ws.c +6 -2

index 9456e2fdbe3..3e9ce55d095 100644
@@ -6,6 +6,7 @@
 #include "git-compat-util.h"
 #include "attr.h"
 #include "strbuf.h"
+#include "gettext.h"
 #include "ws.h"
 
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;

@@ -70,8 +71,11 @@ unsigned parse_whitespace_rule(const char *string)
 		string = ep;
 	}
 
-	if (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB)
-		die("cannot enforce both tab-in-indent and indent-with-non-tab");
+	if (rule & WS_TAB_IN_INDENT && rule & WS_INDENT_WITH_NON_TAB) {
+		warning(_("cannot enforce both tab-in-indent and indent-with-non-tab, disabling both"));
+		rule &= ~WS_TAB_IN_INDENT;
+		rule &= ~WS_INDENT_WITH_NON_TAB;
+	}
 	return rule;
 }

base-commit: facbe4f633e4ad31e641f64617bc88074c659959

Signature · 1 line

gitgitgadget

Junio C Hamano

2024-11-14 02:15 UTC #3 lore raw

"Usman Akinyemi via GitGitGadget" <[email protected]> writes:

[jc: As Phillip is blamed for suggesting this addition, I added him to the recipient of this message.]

Quoted 10 lines

From: Usman Akinyemi <[email protected]>
Modify the conflict resolution between tab-in-indent and indent-with-non-tab to issue a warning instead of terminating the operation with die(). Update the git diff --check test to capture and verify the warning message output.
Suggested-by Phillip Wood <[email protected]>
Signed-off-by Usman Akinyemi <[email protected]>
---

If the settings requires an impossible way to use whitespaces, the settings is buggy, and it generally would be better to correct the setting before moving on.

I am curious to know in what situations this new behaviour can be seen as an improvement. It may allow you to go on _without_ fixing such a broken setting, but how would it help the end user? If the user set both of these mutually-incompatible options A and B by mistake, but what the user really wanted to check for was A, picking just one of A or B arbitrarily and disabling it would not help, and disabling both would not help, either. But wouldn't the real source of the problem be that we are trying to demote die() to force the user to correct contradictiong setting into warning()?

Thanks.

Phillip Wood

2024-11-14 10:06 UTC #4 lore raw

Quoted 4 lines · Junio C Hamano

"Usman Akinyemi via GitGitGadget" <[email protected]> writes:
[jc: As Phillip is blamed for suggesting this addition, I added him to the recipient of this message.]

Thanks

Quoted 6 lines

Quoted 6 lines
From: Usman Akinyemi <[email protected]>
Modify the conflict resolution between tab-in-indent and indent-with-non-tab to issue a warning instead of terminating the operation with die(). Update the git diff --check test to capture and verify the warning message output.

Usman - when you're writing a commit message it is important to explain the reason for making the changes contained in the patch so others can understand why it is a good idea. In this case the idea is to avoid breaking "git diff" for everyone who clones a repository containing a .gitattributes file with bad whitespace attributes [1]. As I mentioned in [2] I think we only want to change the behavior when parsing whitespace attributes - we still want the other callers of parse_whitespace_rule() to die() so the user can fix their config or commandline. We can do that by adding a boolean parameter called "gentle" that determines whether we call warning() or die().

Best Wishes

Phillip

[1] https://lore.kernel.org/git/[email protected]
[2] https://lore.kernel.org/git/[email protected]

Quoted 19 lines

Quoted 3 lines
Suggested-by Phillip Wood <[email protected]>
Signed-off-by Usman Akinyemi <[email protected]>
---
If the settings requires an impossible way to use whitespaces, the settings is buggy, and it generally would be better to correct the setting before moving on.
I am curious to know in what situations this new behaviour can be seen as an improvement. It may allow you to go on _without_ fixing such a broken setting, but how would it help the end user? If the user set both of these mutually-incompatible options A and B by mistake, but what the user really wanted to check for was A, picking just one of A or B arbitrarily and disabling it would not help, and disabling both would not help, either. But wouldn't the real source of the problem be that we are trying to demote die() to force the user to correct contradictiong setting into warning()?
Thanks.

Usman Akinyemi

2024-11-14 11:30 UTC #5 lore raw

Quoted 8 lines · Phillip Wood

Quoted 4 lines · Junio C Hamano
"Usman Akinyemi via GitGitGadget" <[email protected]> writes:
[jc: As Phillip is blamed for suggesting this addition, I added him to the recipient of this message.]
Thanks

Hi Philip and Junio,

Quoted 8 lines

Quoted 7 lines
Quoted 7 lines
From: Usman Akinyemi <[email protected]>
Modify the conflict resolution between tab-in-indent and indent-with-non-tab to issue a warning instead of terminating the operation with die(). Update the git diff --check test to capture and verify the warning message output.

Quoted 10 lines

Usman - when you're writing a commit message it is important to explain the reason for making the changes contained in the patch so others can understand why it is a good idea. In this case the idea is to avoid breaking "git diff" for everyone who clones a repository containing a .gitattributes file with bad whitespace attributes [1]. As I mentioned in [2] I think we only want to change the behavior when parsing whitespace attributes - we still want the other callers of parse_whitespace_rule() to die() so the user can fix their config or commandline. We can do that by adding a boolean parameter called "gentle" that determines whether we call warning() or die().

I am very sorry for the confusion. I will take this into consideration next time and always give more explanation
in commit messages.

I will make the necessary changes.

Thank you very much.
Usman.

Quoted 30 lines

Best Wishes
Phillip
[1]
https://lore.kernel.org/git/[email protected]
[2]
https://lore.kernel.org/git/[email protected]
Quoted 19 lines
Quoted 3 lines
Suggested-by Phillip Wood <[email protected]>
Signed-off-by Usman Akinyemi <[email protected]>
---
If the settings requires an impossible way to use whitespaces, the settings is buggy, and it generally would be better to correct the setting before moving on.
I am curious to know in what situations this new behaviour can be seen as an improvement. It may allow you to go on _without_ fixing such a broken setting, but how would it help the end user? If the user set both of these mutually-incompatible options A and B by mistake, but what the user really wanted to check for was A, picking just one of A or B arbitrarily and disabling it would not help, and disabling both would not help, either. But wouldn't the real source of the problem be that we are trying to demote die() to force the user to correct contradictiong setting into warning()?
Thanks.

Junio C Hamano

2024-11-15 00:11 UTC #6 lore raw

In reply to Phillip Wood

Quoted 6 lines · Phillip Wood

Usman - when you're writing a commit message it is important to explain the reason for making the changes contained in the patch so others can understand why it is a good idea. In this case the idea is to avoid breaking "git diff" for everyone who clones a repository containing a .gitattributes file with bad whitespace attributes [1].

Hmph, it would certainly be a problem, but the right solution is not to butcher Git, but to make it easier for the participants of such a project to know what is broken *and* what needs to be updated, to let them move forward, no?

Quoted 5 lines

As I mentioned in [2] I think we only want to change the behavior when parsing whitespace attributes - we still want the other callers of parse_whitespace_rule() to die() so the user can fix their config or commandline. We can do that by adding a boolean parameter called "gentle" that determines whether we call warning() or die().

I doubt that such a complexity is warranted.

It depends on the size of diff you are showing, but if it is large, then giving a small warning that gets buried in the large diff is a conter-productive way to encourage users to correct such broken setting. If it is small, then the damage may not be too bad, but still, we are showing what the user did not really request.

If we were to fix anything, it is to make sure that we die() before producing a single line of output. If you have a change to a path whose "type" is without such a misconfigured attribute, that sorts lexicographically earlier than another path with a change, with a conflicting whitespace attribute, I suspect that with the way the code is structured currently, we show the diff for the first path, before realizing that the second path has an issue and then die.

If we fix it, and then make sure that the die() message shows clearly what attribute setting we did not like, that would be sufficient to help users to locate the problem, fix it, and quickly move on, no?

Thanks.

Usman Akinyemi

2024-11-18 21:04 UTC #7 lore raw

Quoted 37 lines · Junio C Hamano

Quoted 6 lines · Phillip Wood
Usman - when you're writing a commit message it is important to explain the reason for making the changes contained in the patch so others can understand why it is a good idea. In this case the idea is to avoid breaking "git diff" for everyone who clones a repository containing a .gitattributes file with bad whitespace attributes [1].
Hmph, it would certainly be a problem, but the right solution is not to butcher Git, but to make it easier for the participants of such a project to know what is broken *and* what needs to be updated, to let them move forward, no?
Quoted 5 lines
As I mentioned in [2] I think we only want to change the behavior when parsing whitespace attributes - we still want the other callers of parse_whitespace_rule() to die() so the user can fix their config or commandline. We can do that by adding a boolean parameter called "gentle" that determines whether we call warning() or die().
I doubt that such a complexity is warranted.
It depends on the size of diff you are showing, but if it is large, then giving a small warning that gets buried in the large diff is a conter-productive way to encourage users to correct such broken setting. If it is small, then the damage may not be too bad, but still, we are showing what the user did not really request.
If we were to fix anything, it is to make sure that we die() before producing a single line of output. If you have a change to a path whose "type" is without such a misconfigured attribute, that sorts lexicographically earlier than another path with a change, with a conflicting whitespace attribute, I suspect that with the way the code is structured currently, we show the diff for the first path, before realizing that the second path has an issue and then die.

Quoted 4 lines

If we fix it, and then make sure that the die() message shows clearly what attribute setting we did not like, that would be sufficient to help users to locate the problem, fix it, and quickly move on, no?

Hi Junio,

Thanks for the review. From what I understand from your comment, we should leave it the way it was which was die right ?

Thanks.
Usman.

Quoted 2 lines

Thanks.

Junio C Hamano

2024-11-19 00:36 UTC #8 lore raw

Quoted 10 lines · Usman Akinyemi

Quoted 9 lines · Junio C Hamano
...
If we were to fix anything, it is to make sure that we die() before producing a single line of output. If you have a change to a path whose "type" is without such a misconfigured attribute, that sorts lexicographically earlier than another path with a change, with a conflicting whitespace attribute, I suspect that with the way the code is structured currently, we show the diff for the first path, before realizing that the second path has an issue and then die.

Quoted 7 lines

Quoted 4 lines
If we fix it, and then make sure that the die() message shows clearly what attribute setting we did not like, that would be sufficient to help users to locate the problem, fix it, and quickly move on, no?
Thanks for the review. From what I understand from your comment, we should leave it the way it was which was die right ?

Correct. I do not think replacing die() with warning() without doing anything else makes sense. Making sure that we detect the breakage before going half-way while producing a patch that touches many paths may improve the end-user experience, though.

Thanks.

Phillip Wood

2024-11-19 16:49 UTC #9 lore raw

In reply to Junio C Hamano

Quoted 9 lines · Junio C Hamano

Quoted 6 lines · Phillip Wood
Usman - when you're writing a commit message it is important to explain the reason for making the changes contained in the patch so others can understand why it is a good idea. In this case the idea is to avoid breaking "git diff" for everyone who clones a repository containing a .gitattributes file with bad whitespace attributes [1].

Quoted 4 lines

Hmph, it would certainly be a problem, but the right solution is not to butcher Git, but to make it easier for the participants of such a project to know what is broken *and* what needs to be updated, to let them move forward, no?

Arguably yes, but that's not the approach we take when the attributes file is too large, a line in the file is is too long or the file contains a negative filename pattern. For those cases we print a warning and continue. The recently merged e36f009e69b (merge: replace atoi() with strtol_i() for marker size validation, 2024-10-24) followed suit and warns rather than dies for an invalid marker size. It would be nice to be consistent in the way we treat invalid attributes. Consistently dying and telling the user how to fix the problem would be a reasonable approach on the client side but I wonder if it could cause problems for forges running "git diff" and "git merge-tree" on a server though.

Quoted 3 lines

[...]
If we were to fix anything, it is to make sure that we die() before producing a single line of output.

That would certainly be a good idea

Best Wishes

Phillip

Junio C Hamano

2024-11-20 01:23 UTC #10 lore raw

Quoted 8 lines · Phillip Wood

Arguably yes, but that's not the approach we take when the attributes file is too large, a line in the file is is too long or the file contains a negative filename pattern. For those cases we print a warning and continue. The recently merged e36f009e69b (merge: replace atoi() with strtol_i() for marker size validation, 2024-10-24) followed suit and warns rather than dies for an invalid marker size. It would be nice to be consistent in the way we treat invalid attributes.

Arguably yes, but being careful when adding a new check and changing established behaviour, risking to break existing users, are different.

Quoted 4 lines

Consistently dying and telling the user how to fix the problem would be a reasonable approach on the client side but I wonder if it could cause problems for forges running "git diff" and "git merge-tree" on a server though.

That's an interesting aspect. I wonder what happens when somebody pushes a project with a .gitattributes with such a conflicting setting to GitHub or GitLab.

Would that bring the world to its end ;-)?