[v2] diff: update conflict handling for whitespace to issue a warning

Commit Message

Usman Akinyemi Nov. 13, 2024, 7:01 p.m. UTC

From: Usman Akinyemi <[e-mail]>

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 <[e-mail]>
Signed-off-by: Usman Akinyemi <[e-mail]>
---
    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(-)


base-commit: facbe4f633e4ad31e641f64617bc88074c659959

Comments

Junio C Hamano Nov. 14, 2024, 2:15 a.m. UTC | #1

"Usman Akinyemi via GitGitGadget" <[e-mail]> writes:

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

> From: Usman Akinyemi <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Usman Akinyemi <[e-mail]>
> ---

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 Nov. 14, 2024, 10:06 a.m. UTC | #2

On 14/11/2024 02:15, Junio C Hamano wrote:
> "Usman Akinyemi via GitGitGadget" <[e-mail]> writes:
> 
> [jc: As Phillip is blamed for suggesting this addition, I added him
> to the recipient of this message.]

Thanks

>> From: Usman Akinyemi <[e-mail]>
>>
>> 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/[e-mail]
[2] 
https://lore.kernel.org/git/[e-mail]

>> Suggested-by: Phillip Wood <[e-mail]>
>> Signed-off-by: Usman Akinyemi <[e-mail]>
>> ---
> 
> 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 Nov. 14, 2024, 11:29 a.m. UTC | #3

On Thu, Nov 14, 2024 at 5:06 AM Phillip Wood <[e-mail]> wrote:
>
> On 14/11/2024 02:15, Junio C Hamano wrote:
> > "Usman Akinyemi via GitGitGadget" <[e-mail]> writes:
> >
> > [jc: As Phillip is blamed for suggesting this addition, I added him
> > to the recipient of this message.]
>
> Thanks
Hi Philip and Junio,

>
> >> From: Usman Akinyemi <[e-mail]>
> >>
> >> 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().

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.

>
> Best Wishes
>
> Phillip
>
> [1]
> https://lore.kernel.org/git/[e-mail]
> [2]
> https://lore.kernel.org/git/[e-mail]
>
> >> Suggested-by: Phillip Wood <[e-mail]>
> >> Signed-off-by: Usman Akinyemi <[e-mail]>
> >> ---
> >
> > 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.
>

Patch

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 851cfe4f32c..849f1854fb9 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -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' '
diff --git a/ws.c b/ws.c
index 9456e2fdbe3..3e9ce55d095 100644
--- a/ws.c
+++ b/ws.c
@@ -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;
 }