{"thread":{"id":"62001","subject":"[PATCH 0/5] `--whitespace=fix` with `--no-ignore-whitespace`","startedAt":"2024-08-25T10:09:39Z","lastAt":"2024-09-04T18:20:40Z","messageCount":18,"participants":["Rubén Justo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"501630","messageId":"6dd964c2-9dee-4257-8f1a-5bc31a73722e@gmail.com","threadId":"62001","inReplyTo":null,"subject":"[PATCH 0/5] `--whitespace=fix` with `--no-ignore-whitespace`","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-08-25T10:09:36Z","receivedAt":"2024-08-25T10:09:39Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"When we see `--whitespace=fix` we don't consider a possible option:\n`--no-ignore-whitespace`. The following example produces unexpected,\nat least for me, results:\n\n $ printf \"a \\nb\\nc\\n\" >file\n $ git add file\n $ cat >patch <file\n $ git add file\n $ cat >patch <\n---\n apply.c | 2 +-\n apply.h | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 6e1060a952..63e58086f1 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -115,7 +115,7 @@ int init_apply_state(struct apply_state *state,\n \tstate->p_context = UINT_MAX;\n \tstate->squelch_whitespace_errors = 5;\n \tstate->ws_error_action = warn_on_ws_error;\n-\tstate->ws_ignore_action = ignore_ws_none;\n+\tstate->ws_ignore_action = ignore_ws_default;\n \tstate->linenr = 1;\n \tstring_list_init_nodup(&state->fn_table);\n \tstring_list_init_nodup(&state->limit_by_name);\ndiff --git a/apply.h b/apply.h\nindex cd25d24cc4..201f953a64 100644\n--- a/apply.h\n+++ b/apply.h\n@@ -16,6 +16,7 @@ enum apply_ws_error_action {\n };\n \n enum apply_ws_ignore {\n+\tignore_ws_default,\n \tignore_ws_none,\n \tignore_ws_change\n };\n-- \n2.46.0.353.g385c909849\n"},{"id":"501632","messageId":"1eb33969-1739-4a27-a77b-3f4268f5519d@gmail.com","threadId":"62001","inReplyTo":"6dd964c2-9dee-4257-8f1a-5bc31a73722e@gmail.com","subject":"[PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-08-25T10:18:28Z","receivedAt":"2024-08-25T10:18:30Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Ensure strict matching of context lines when applying with\n`--whitespace=fix` combined with `--no-ignore-whitespace`.\n\nSigned-off-by: Rubén Justo \n---\n apply.c | 3 ++-\n t/t4124-apply-ws-rule.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 1 deletion(-)\n\ndiff --git a/apply.c b/apply.c\nindex 63e58086f1..0cb9d38e5a 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -2596,7 +2596,8 @@ static int match_fragment(struct apply_state *state,\n \t\tgoto out;\n \t}\n \n-\tif (state->ws_error_action != correct_ws_error) {\n+\tif (state->ws_error_action != correct_ws_error ||\n+\t state->ws_ignore_action == ignore_ws_none) {\n \t\tret = 0;\n \t\tgoto out;\n \t}\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex 485c7d2d12..573200da67 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -545,6 +545,33 @@ test_expect_success 'whitespace=fix to expand' '\n \tgit -c core.whitespace=tab-in-indent apply --whitespace=fix patch\n '\n \n+test_expect_success 'whitespace=fix honors no-ignore-whitespace' '\n+\tqz_to_tab_space >preimage <<-\\EOF &&\n+\tAZ\n+\tBZZ\n+\tEOF\n+\tqz_to_tab_space >patch <<-\\EOF &&\n+\tdiff --git a/preimage b/preimage\n+\t--- a/preimage\n+\t+++ b/preimage\n+\t@@ -1,2 +1,2 @@\n+\t-AZ\n+\t+A\n+\t BZZZ\n+\tEOF\n+\ttest_must_fail git apply --no-ignore-whitespace --whitespace=fix patch &&\n+\tqz_to_tab_space >patch <<-\\EOF &&\n+\tdiff --git a/preimage b/preimage\n+\t--- a/preimage\n+\t+++ b/preimage\n+\t@@ -1,2 +1,2 @@\n+\t-AZ\n+\t+A\n+\t BZZ\n+\tEOF\n+\tgit apply --no-ignore-whitespace --whitespace=fix patch\n+'\n+\n test_expect_success 'whitespace check skipped for excluded paths' '\n \tgit config core.whitespace blank-at-eol &&\n \t>used &&\n-- \n2.46.0.353.g385c909849\n"},{"id":"501633","messageId":"5da09529-e95b-407b-9e66-34ebac4b4128@gmail.com","threadId":"62001","inReplyTo":"6dd964c2-9dee-4257-8f1a-5bc31a73722e@gmail.com","subject":"[PATCH 3/5] apply: whitespace errors in context lines if we have","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-08-25T10:18:44Z","receivedAt":"2024-08-25T10:18:47Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"If the user says `--no-ignore-space-change`, there's no need to\ncheck for whitespace errors in the context lines.\n\nDon't do it.\n\nSigned-off-by: Rubén Justo \n---\n apply.c | 3 ++-\n t/t4124-apply-ws-rule.sh | 3 ++-\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex 0cb9d38e5a..e1b4d14dba 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1734,7 +1734,8 @@ static int parse_fragment(struct apply_state *state,\n \t\t\ttrailing++;\n \t\t\tcheck_old_for_crlf(patch, line, len);\n \t\t\tif (!state->apply_in_reverse &&\n-\t\t\t state->ws_error_action == correct_ws_error)\n+\t\t\t state->ws_error_action == correct_ws_error &&\n+\t\t\t state->ws_ignore_action != ignore_ws_none)\n \t\t\t\tcheck_whitespace(state, line, len, patch->ws_rule);\n \t\t\tbreak;\n \t\tcase '-':\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex 573200da67..e12b8333c3 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -569,7 +569,8 @@ test_expect_success 'whitespace=fix honors no-ignore-whitespace' '\n \t+A\n \t BZZ\n \tEOF\n-\tgit apply --no-ignore-whitespace --whitespace=fix patch\n+\tgit apply --no-ignore-whitespace --whitespace=fix patch 2>error &&\n+\ttest_must_be_empty error\n '\n \n test_expect_success 'whitespace check skipped for excluded paths' '\n-- \n2.46.0.353.g385c909849\n"},{"id":"501634","messageId":"a8ce6f0d-f0f2-4467-bf16-e7ce78c6ce2d@gmail.com","threadId":"62001","inReplyTo":"6dd964c2-9dee-4257-8f1a-5bc31a73722e@gmail.com","subject":"[PATCH 4/5] apply: error message in `record_ws_error()`","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-08-25T10:19:27Z","receivedAt":"2024-08-25T10:19:30Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"It does not make sense to construct an error message if we're not\ngoing to use it, especially when the process involves memory\nallocations that need to be freed immediately.\n\nIf we know in advance that we won't use the message, not getting it\nslightly reduces the workload and simplifies the code a bit.\n\nDo it.\n\nSigned-off-by: Rubén Justo \n---\n apply.c | 9 ++++-----\n 1 file changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/apply.c b/apply.c\nindex e1b4d14dba..e6df8b6ab4 100644\n--- a/apply.c\n+++ b/apply.c\n@@ -1642,8 +1642,6 @@ static void record_ws_error(struct apply_state *state,\n \t\t\t int len,\n \t\t\t int linenr)\n {\n-\tchar *err;\n-\n \tif (!result)\n \t\treturn;\n \n@@ -1652,11 +1650,12 @@ static void record_ws_error(struct apply_state *state,\n \t state->squelch_whitespace_errors < state->whitespace_error)\n \t\treturn;\n \n-\terr = whitespace_error_string(result);\n-\tif (state->apply_verbosity > verbosity_silent)\n+\tif (state->apply_verbosity > verbosity_silent) {\n+\t\tchar *err = whitespace_error_string(result);\n \t\tfprintf(stderr, \"%s:%d: %s.\\n%.*s\\n\",\n \t\t\tstate->patch_input_file, linenr, err, len, line);\n-\tfree(err);\n+\t\tfree(err);\n+\t}\n }\n \n static void check_whitespace(struct apply_state *state,\n-- \n2.46.0.353.g385c909849\n"},{"id":"501635","messageId":"4c0c05a2-c143-43b4-bc1b-d70b3645c702@gmail.com","threadId":"62001","inReplyTo":"6dd964c2-9dee-4257-8f1a-5bc31a73722e@gmail.com","subject":"[PATCH 5/5] t4124: move test preparation into the test context","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-08-25T10:19:48Z","receivedAt":"2024-08-25T10:19:50Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Signed-off-by: Rubén Justo \n---\n t/t4124-apply-ws-rule.sh | 16 ++--------------\n 1 file changed, 2 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\nindex e12b8333c3..56f15dc3ef 100755\n--- a/t/t4124-apply-ws-rule.sh\n+++ b/t/t4124-apply-ws-rule.sh\n@@ -423,14 +423,8 @@ test_expect_success 'missing blanks at EOF must only match blank lines' '\n \ttest_must_fail git apply --ignore-space-change --whitespace=fix patch\n '\n \n-sed -e's/Z//' >one <one &&\n \tgit add one &&\n \techo d >>one &&\n \tgit diff -- one >patch &&\n@@ -443,14 +437,8 @@ test_expect_success 'missing blank line should match context line with spaces' '\n \ttest_cmp expect one\n '\n \n-sed -e's/Z//' >one <one &&\n \tgit add one &&\n \techo d >>one &&\n \tcp one expect &&\n-- \n2.46.0.353.g385c909849\n"},{"id":"501706","messageId":"xmqqseuqerb1.fsf@gitster.g","threadId":"62001","inReplyTo":"1eb33969-1739-4a27-a77b-3f4268f5519d@gmail.com","subject":"Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-27T00:35:14Z","receivedAt":"2024-08-27T00:35:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> Ensure strict matching of context lines when applying with\n> `--whitespace=fix` combined with `--no-ignore-whitespace`.\n>\n> Signed-off-by: Rubén Justo \n> ---\n> apply.c | 3 ++-\n> t/t4124-apply-ws-rule.sh | 27 +++++++++++++++++++++++++++\n> 2 files changed, 29 insertions(+), 1 deletion(-)\n>\n> diff --git a/apply.c b/apply.c\n> index 63e58086f1..0cb9d38e5a 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -2596,7 +2596,8 @@ static int match_fragment(struct apply_state *state,\n> \t\tgoto out;\n> \t}\n> \n> -\tif (state->ws_error_action != correct_ws_error) {\n> +\tif (state->ws_error_action != correct_ws_error ||\n> +\t state->ws_ignore_action == ignore_ws_none) {\n> \t\tret = 0;\n> \t\tgoto out;\n> \t}\n\nHmph, if we are correcting for whitespace violations, even if\nwhitespace fuzz is not allowed externally, wouldn't the issue that\nc1beba5b (git-apply --whitespace=fix: fix whitespace fuzz introduced\nby previous run, 2008-01-30) corrected still apply? IOW, isn't this\nchange introducing a regression when an input touches a file with a\nchange with broken whitespaces, and then touches the same file to\nreplace the broken whitespace lines with something else?\n\n"},{"id":"501707","messageId":"xmqqo75eeqx0.fsf@gitster.g","threadId":"62001","inReplyTo":"5da09529-e95b-407b-9e66-34ebac4b4128@gmail.com","subject":"Re: [PATCH 3/5] apply: whitespace errors in context lines if we have","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-27T00:43:39Z","receivedAt":"2024-08-27T00:43:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> If the user says `--no-ignore-space-change`, there's no need to\n> check for whitespace errors in the context lines.\n>\n> Don't do it.\n>\n> Signed-off-by: Rubén Justo \n> ---\n> apply.c | 3 ++-\n> t/t4124-apply-ws-rule.sh | 3 ++-\n> 2 files changed, 4 insertions(+), 2 deletions(-)\n>\n> diff --git a/apply.c b/apply.c\n> index 0cb9d38e5a..e1b4d14dba 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -1734,7 +1734,8 @@ static int parse_fragment(struct apply_state *state,\n> \t\t\ttrailing++;\n> \t\t\tcheck_old_for_crlf(patch, line, len);\n> \t\t\tif (!state->apply_in_reverse &&\n> -\t\t\t state->ws_error_action == correct_ws_error)\n> +\t\t\t state->ws_error_action == correct_ws_error &&\n> +\t\t\t state->ws_ignore_action != ignore_ws_none)\n> \t\t\t\tcheck_whitespace(state, line, len, patch->ws_rule);\n> \t\t\tbreak;\n\nHmph. 0a80bc9f (apply: detect and mark whitespace errors in context\nlines when fixing, 2015-01-16) deliberately added this check because\nwe will correct the whitespace breakages on these lines after\nparsing the hunk with this function while applying.\n\nIt is iffy that this case arm for \" \" kicks in ONLY when applying in\nthe forward direction (which is not what you are changing). When\napplying a patch in reverse, \" \" is still an \"unchanged\" context\nline, so we should be treating it the same way regardless of the\ndirection.\n\nBut at least the call to check_whitespace() from this place when we\nare correcting whitespace rule violations is not iffy, as far as I\ncan tell.\n\n\n\n> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh\n> index 573200da67..e12b8333c3 100755\n> --- a/t/t4124-apply-ws-rule.sh\n> +++ b/t/t4124-apply-ws-rule.sh\n> @@ -569,7 +569,8 @@ test_expect_success 'whitespace=fix honors no-ignore-whitespace' '\n> \t+A\n> \t BZZ\n> \tEOF\n> -\tgit apply --no-ignore-whitespace --whitespace=fix patch\n> +\tgit apply --no-ignore-whitespace --whitespace=fix patch 2>error &&\n> +\ttest_must_be_empty error\n> '\n> \n> test_expect_success 'whitespace check skipped for excluded paths' '\n"},{"id":"501708","messageId":"xmqqjzg2eqvc.fsf@gitster.g","threadId":"62001","inReplyTo":"a8ce6f0d-f0f2-4467-bf16-e7ce78c6ce2d@gmail.com","subject":"Re: [PATCH 4/5] apply: error message in `record_ws_error()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-27T00:44:39Z","receivedAt":"2024-08-27T00:44:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> It does not make sense to construct an error message if we're not\n> going to use it, especially when the process involves memory\n> allocations that need to be freed immediately.\n>\n> If we know in advance that we won't use the message, not getting it\n> slightly reduces the workload and simplifies the code a bit.\n\nMakes sense.\n\n>\n> Do it.\n\nNo need to say this when the above two paragraphs are clear enough,\nlike in this patch.\n\n>\n> Signed-off-by: Rubén Justo \n> ---\n> apply.c | 9 ++++-----\n> 1 file changed, 4 insertions(+), 5 deletions(-)\n>\n> diff --git a/apply.c b/apply.c\n> index e1b4d14dba..e6df8b6ab4 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -1642,8 +1642,6 @@ static void record_ws_error(struct apply_state *state,\n> \t\t\t int len,\n> \t\t\t int linenr)\n> {\n> -\tchar *err;\n> -\n> \tif (!result)\n> \t\treturn;\n> \n> @@ -1652,11 +1650,12 @@ static void record_ws_error(struct apply_state *state,\n> \t state->squelch_whitespace_errors < state->whitespace_error)\n> \t\treturn;\n> \n> -\terr = whitespace_error_string(result);\n> -\tif (state->apply_verbosity > verbosity_silent)\n> +\tif (state->apply_verbosity > verbosity_silent) {\n> +\t\tchar *err = whitespace_error_string(result);\n> \t\tfprintf(stderr, \"%s:%d: %s.\\n%.*s\\n\",\n> \t\t\tstate->patch_input_file, linenr, err, len, line);\n> -\tfree(err);\n> +\t\tfree(err);\n> +\t}\n> }\n> \n> static void check_whitespace(struct apply_state *state,\n"},{"id":"501709","messageId":"xmqqbk1eeqk0.fsf@gitster.g","threadId":"62001","inReplyTo":"5da09529-e95b-407b-9e66-34ebac4b4128@gmail.com","subject":"Re: [PATCH 3/5] apply: whitespace errors in context lines if we have","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-27T00:51:27Z","receivedAt":"2024-08-27T00:51:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> If the user says `--no-ignore-space-change`, there's no need to\n> check for whitespace errors in the context lines.\n\nBecause the default is *not* to ignore space change, the command\nshould behave exactly the same way between two cases: (1) the user\nuses the default and does not give the \"--ignore-space-change\"\noption, and (2) the user gives the \"--no-ignore-space-change\" option\nexplicitly. So I am very much convinced that [1/5] is unneeded, and\n\"If the user says `--no-*`\" in the above proposed log message is\ninsufficient (at least it also needs to say \"or uses the default and\ndoes not say \"--ignore-space-change\").\n\n> Don't do it.\n\nNo need to say this, when the paragraphs above clearly and\nunambiguously leads to this conclusion.\n\n> diff --git a/apply.c b/apply.c\n> index 0cb9d38e5a..e1b4d14dba 100644\n> --- a/apply.c\n> +++ b/apply.c\n> @@ -1734,7 +1734,8 @@ static int parse_fragment(struct apply_state *state,\n> \t\t\ttrailing++;\n> \t\t\tcheck_old_for_crlf(patch, line, len);\n> \t\t\tif (!state->apply_in_reverse &&\n> -\t\t\t state->ws_error_action == correct_ws_error)\n> +\t\t\t state->ws_error_action == correct_ws_error &&\n> +\t\t\t state->ws_ignore_action != ignore_ws_none)\n> \t\t\t\tcheck_whitespace(state, line, len, patch->ws_rule);\n\nI am not 100% convinced that this change is _wrong_, but it does\nsmell like reverting a necessary change as I pointed out in another\nreply.\n\nThanks.\n\n"},{"id":"501710","messageId":"xmqq4j76epmk.fsf@gitster.g","threadId":"62001","inReplyTo":"5e35f260-056c-4af3-95d9-70d6f117bff9@gmail.com","subject":"Re: [PATCH 1/5] apply: introduce `ignore_ws_default`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-27T01:11:31Z","receivedAt":"2024-08-27T01:11:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> When we see `--whitespace=fix` we don't consider a possible\n> option: `--no-ignore-whitespace`.\n>\n> The expected result in the following example is a failure when\n> applying the patch, however:\n>\n> $ printf \"a \\nb\\nc\\n\" >file\n> $ git add file\n> $ cat >patch < --- a/file\n> +++ b/file\n> @@ -1,3 +1,2 @@\n> a\n> -b\n> c\n> END\n> $ git apply --no-ignore-whitespace --whitespace=fix patch\n> $ xxd file\n> 00000000: 610a 630a a.c.\n>\n> This unexpected result will be addressed in an upcoming commit.\n>\n> As a preparation, we need to detect when the user has explicitly\n> said `--no-ignore-whitespace`.\n\nIf you said, before all of the above, what _other_ case you are\ntrying to differenciate from the case where the user explicitly gave\nthe \"--no-ignore-whitespace\" option, it would clarify why a\ndifferenciator is needed. IOW, perhaps start\n\n By default, \"git apply\" does not ignore whitespace changes\n (i.e. state.ws_ignore_action is initialized to ignore_ws_none).\n However we want to treat this default case and the case where\n the user explicitly gave the \"--no-ignore-whitespace\" option FOR\n SUCH AND SUCH REASONS.\n\n ... elaborate SUCH AND SUCH REASONS as needed here ...\n\n Initialize state.ws_ignore_action to ignore_ws_default, and\n later after the parse_options() returns, if the state is still\n _default, we can tell there wasn't such an explicit option.\n\nor something?\n\nThe rest of the code paths are not told what to do when they see\nws_ignore_action is set to this new value, so I somehow find it iffy\nthat this step is complete. Shouldn't it at least flip some other bit\nafter apply_parse_options() makes parse_options() call and notices that\nthe default value is still there, and then replace the _default value\nwith ws_none, or something, along the lines of ...\n\n apply.c | 10 +++++++++-\n 1 file changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git i/apply.c w/apply.c\nindex 6e1060a952..acc0f64d37 100644\n--- i/apply.c\n+++ w/apply.c\n@@ -5190,5 +5190,13 @@ int apply_parse_options(int argc, const char **argv,\n \t\tOPT_END()\n \t};\n \n-\treturn parse_options(argc, argv, state->prefix, builtin_apply_options, apply_usage, 0);\n+\tret = parse_options(argc, argv, state->prefix,\n+\t\t\t builtin_apply_options, apply_usage, 0);\n+\tif (!ret) {\n+\t\tif (state->ws_ignore_action == ignore_ws_default) {\n+\t\t\t... note that --no-ignore-whitespace was *NOT* used ...\n+\t\t\tstate->ws_ignore_action = ignore_ws_none;\n+\t\t}\n+\t}\n+\treturn ret;\n }\n\n... without that anywhere state.ws_ignore_action gets inspected, the\nall must treat _none and _default pretty much the same way, no?\n\n> Currently, we only have one explicit consideration for\n> `ignore_ws_change`, and no, implicit or explicit, considerations for\n> `ignore_ws_none`. Therefore, no modification to the existing logic\n> is required in this step.\n\nYes, that is a plausible excuse, but it feels somehat brittle.\n\nMore importantly, the proposed log message does not explain why\n\"--no-ignore-whitespace\", which is the default, needs to be special\ncased when it is given explicitly. You had symptoms you want to fix\ndescribed, but it is probably a few steps disconnected from the\nreason why the default vs explicit setting of ws_ignore_action need\nto make the code behave differently.\n\nThanks.\n"},{"id":"501711","messageId":"xmqqv7zmd9a5.fsf@gitster.g","threadId":"62001","inReplyTo":"xmqqo75eeqx0.fsf@gitster.g","subject":"Re: [PATCH 3/5] apply: whitespace errors in context lines if we have","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-27T01:49:54Z","receivedAt":"2024-08-27T01:50:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Hmph. 0a80bc9f (apply: detect and mark whitespace errors in context\n> lines when fixing, 2015-01-16) deliberately added this check because\n> we will correct the whitespace breakages on these lines after\n> parsing the hunk with this function while applying.\n>\n> It is iffy that this case arm for \" \" kicks in ONLY when applying in\n> the forward direction (which is not what you are changing). When\n> applying a patch in reverse, \" \" is still an \"unchanged\" context\n> line, so we should be treating it the same way regardless of the\n> direction.\n>\n> But at least the call to check_whitespace() from this place when we\n> are correcting whitespace rule violations is not iffy, as far as I\n> can tell.\n\nHaving said all that, I do have to wonder how much value we are\ngetting by supporting that odd \"feature\" that makes apply take input\nin a single session a patch that touches the same path TWICE.\n\nIf we can get rid of that feature (which I consider a misfeature),\nwe can lose quote a lot of code (anything that touches fn_table can\ngo) and recover the code quality that got visibly worse with the\naddition of that feature back.\n\nAnd without the \"input may touch the same path TWICE\", we do not\nhave to worry about this \"context lines after applying a single\npatch with whitespace=fix will have to be matched loosely with\nrespect to the whitespace when another patch modifies the same file\naround the same lines\", making your changes in [3/5] trivially the\nright thing to do.\n\nSo, I am inclined to say that\n\n * we propose to get rid of that \"a single input may touch the same\n path TWICE\" feature at Git 3.0 boundary.\n\n * we at the same time apply [3/5] (and possibly others, but I do\n not think we want [1/5]).\n\nBut until we can shed our pretense that the \"single input may touch\nthe same path TWICE\" is seriously supported, I do not think applying\nthis series as-is makes sense, as it directly contradicts with that\n(mis)feature.\n\n\n"},{"id":"501724","messageId":"xmqqplpuaqsr.fsf@gitster.g","threadId":"62001","inReplyTo":"xmqqv7zmd9a5.fsf@gitster.g","subject":"Re: [PATCH 3/5] apply: whitespace errors in context lines if we have","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-27T16:12:04Z","receivedAt":"2024-08-27T16:12:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Junio C Hamano writes:\n>\n>> Hmph. 0a80bc9f (apply: detect and mark whitespace errors in context\n>> lines when fixing, 2015-01-16) deliberately added this check because\n>> we will correct the whitespace breakages on these lines after\n>> parsing the hunk with this function while applying.\n> ...\n> So, I am inclined to say that\n>\n> * we propose to get rid of that \"a single input may touch the same\n> path TWICE\" feature at Git 3.0 boundary.\n>\n> * we at the same time apply [3/5] (and possibly others, but I do\n> not think we want [1/5]).\n>\n> But until we can shed our pretense that the \"single input may touch\n> the same path TWICE\" is seriously supported, I do not think applying\n> this series as-is makes sense, as it directly contradicts with that\n> (mis)feature.\n\nSo, here is another thought. Can we notice that we are dealing with\nsuch an irregular patch that we would never produce ourselves, but\nstill have to support as a historical wart? And deal with context\nlines with whitespace breakages differently if that is the case.\n\nI think that is doable. I won't address the entire set of fixes in\nyour series, but a touched up version of your [3/5] may look like\nthe attached at the end. This is on top of your whole series, not\nas a replacement for [3/5], made just for illustration purposes.\n\n>> It is iffy that this case arm for \" \" kicks in ONLY when applying in\n>> the forward direction (which is not what you are changing). When\n>> applying a patch in reverse, \" \" is still an \"unchanged\" context\n>> line, so we should be treating it the same way regardless of the\n>> direction.\n\nI didn't address this \"why only in the forward direction?\" iffyness\nin the illustration patch, by the way.\n\n apply.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git c/apply.c w/apply.c\nindex e6df8b6ab4..04bb094e57 100644\n--- c/apply.c\n+++ w/apply.c\n@@ -38,6 +38,8 @@\n #include \"wildmatch.h\"\n #include \"ws.h\"\n \n+static struct patch *in_fn_table(struct apply_state *state, const char *name);\n+\n struct gitdiff_data {\n \tstruct strbuf *root;\n \tint linenr;\n@@ -1697,10 +1699,13 @@ static int parse_fragment(struct apply_state *state,\n \tint len = linelen(line, size), offset;\n \tunsigned long oldlines, newlines;\n \tunsigned long leading, trailing;\n+\tint touching_same_path; \n \n \toffset = parse_fragment_header(line, len, fragment);\n \tif (offset < 0)\n \t\treturn -1;\n+\n+\ttouching_same_path = !!in_fn_table(state, patch->old_name);\n \tif (offset > 0 && patch->recount)\n \t\trecount_diff(line + offset, size - offset, fragment);\n \toldlines = fragment->oldlines;\n@@ -1734,7 +1739,8 @@ static int parse_fragment(struct apply_state *state,\n \t\t\tcheck_old_for_crlf(patch, line, len);\n \t\t\tif (!state->apply_in_reverse &&\n \t\t\t state->ws_error_action == correct_ws_error &&\n-\t\t\t state->ws_ignore_action != ignore_ws_none)\n+\t\t\t (touching_same_path ||\n+\t\t\t state->ws_ignore_action != ignore_ws_none))\n \t\t\t\tcheck_whitespace(state, line, len, patch->ws_rule);\n \t\t\tbreak;\n \t\tcase '-':\n"},{"id":"501800","messageId":"afade304-51e3-441d-9ae6-e0a422d00bc4@gmail.com","threadId":"62001","inReplyTo":"xmqqseuqerb1.fsf@gitster.g","subject":"Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-08-29T05:07:53Z","receivedAt":"2024-08-29T05:08:00Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Mon, Aug 26, 2024 at 05:35:14PM -0700, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> > Ensure strict matching of context lines when applying with\n> > `--whitespace=fix` combined with `--no-ignore-whitespace`.\n> >\n> > Signed-off-by: Rubén Justo \n> > ---\n> > apply.c | 3 ++-\n> > t/t4124-apply-ws-rule.sh | 27 +++++++++++++++++++++++++++\n> > 2 files changed, 29 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/apply.c b/apply.c\n> > index 63e58086f1..0cb9d38e5a 100644\n> > --- a/apply.c\n> > +++ b/apply.c\n> > @@ -2596,7 +2596,8 @@ static int match_fragment(struct apply_state *state,\n> > \t\tgoto out;\n> > \t}\n> > \n> > -\tif (state->ws_error_action != correct_ws_error) {\n> > +\tif (state->ws_error_action != correct_ws_error ||\n> > +\t state->ws_ignore_action == ignore_ws_none) {\n> > \t\tret = 0;\n> > \t\tgoto out;\n> > \t}\n> \n> Hmph, if we are correcting for whitespace violations, even if\n> whitespace fuzz is not allowed externally, wouldn't the issue that\n> c1beba5b (git-apply --whitespace=fix: fix whitespace fuzz introduced\n> by previous run, 2008-01-30) corrected still apply? IOW, isn't this\n> change introducing a regression when an input touches a file with a\n> change with broken whitespaces, and then touches the same file to\n> replace the broken whitespace lines with something else?\n\nYes, that is the center of the blast this series is producing;\naffecting to what `--whitespace=fix` does (just a reminder for other\nreaders):\n\n - stop fixing whitespace errors in context lines and\n\n - no longer warning about them.\n\nClearly, as you point out, the change poses a problem when applying\nchanges with whitespace errors in lines involved multiple times,\neither during the same apply session or across multiple sessions\nexecuted in sequence.\n\nThe new `ignore_ws_default` enum option is intended to mitigate the\nblast. It would be unexpected IMHO for someone who wants the behavior\ndescribed in c1beba5b to be indicating `--no-ignore-whitespace`. And\nwith it we allow the possibility for someone who finds that behavior\nundesirable to avoid it.\n\nI'm not very happy with the new enum, but I haven't come up with a\nbetter idea. There are other alternatives:\n\n --whitespace=fix-strict\n\n --do-not-fix-ws-errors-in-context-lines\n\nNone of them are better, I think.\n"},{"id":"501883","messageId":"xmqqed66udmd.fsf@gitster.g","threadId":"62001","inReplyTo":"afade304-51e3-441d-9ae6-e0a422d00bc4@gmail.com","subject":"Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-08-29T23:13:14Z","receivedAt":"2024-08-29T23:13:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> I'm not very happy with the new enum, but I haven't come up with a\n> better idea.\n> ...\n> None of them are better, I think.\n\nNot adding a new enum is probably much better. See the \"additional\nthought\" in my review on [3/5], for example.\n\nThanks.\n\n"},{"id":"502071","messageId":"50d85a93-6711-4b42-87a5-f26b58b8c5c7@gmail.com","threadId":"62001","inReplyTo":"xmqqed66udmd.fsf@gitster.g","subject":"Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-09-03T22:06:10Z","receivedAt":"2024-09-03T22:06:13Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Thu, Aug 29, 2024 at 04:13:14PM -0700, Junio C Hamano wrote:\n> Rubén Justo writes:\n> \n> > I'm not very happy with the new enum, but I haven't come up with a\n> > better idea.\n> > ...\n> > None of them are better, I think.\n> \n> Not adding a new enum is probably much better. See the \"additional\n> thought\" in my review on [3/5], for example.\n\nIf I understand correctly the example you mentioned, using\n`in_fn_table()` cannot help us in `parse_fragment()`. But I could be\ncompletely wrong and misunderstanding your intention.\n\nI still don't see a better option than introducing a new value\n`default`. Perhaps described like this:\n\ndiff --git a/Documentation/config/apply.txt b/Documentation/config/apply.txt\nindex f9908e210a..7b642d2f3a 100644\n--- a/Documentation/config/apply.txt\n+++ b/Documentation/config/apply.txt\n@@ -4,6 +4,10 @@ apply.ignoreWhitespace::\n option.\n When set to one of: no, none, never, false, it tells 'git apply' to\n respect all whitespace differences.\n+ When not set or set to `default`, it tells `git apply` to\n+ behave like the previous setting: `no`. However, when\n+ combined with 'whitespace=fix', some whitespace errors\n+ will still be ignored because they are being fixed.\n See linkgit:git-apply[1].\n\n apply.whitespace:\n"},{"id":"502087","messageId":"xmqqseugdo90.fsf@gitster.g","threadId":"62001","inReplyTo":"50d85a93-6711-4b42-87a5-f26b58b8c5c7@gmail.com","subject":"Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-04T04:41:31Z","receivedAt":"2024-09-04T04:41:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo writes:\n\n> If I understand correctly the example you mentioned, using\n> `in_fn_table()` cannot help us in `parse_fragment()`. But I could be\n> completely wrong and misunderstanding your intention.\n\nHmph. It's unfortunate that the control flow goes like this:\n\n apply_all_patches()\n -> apply_patch()\n -> parse_chunk()\n -> parse_single_patch()\n -> parse_fragment()\n -> use_patch()\n -> check_patch_list()\n -> check_patch()\n -> apply_data()\n -> add_to_fn_table()\n\nSo deciding if a context line (or a new line for that matter)\ncontains a whitespace error is done too early in the current code,\nand if you want to do this correctly, you'd need to move the check\ndown so that it happens in apply_one_fragment() that is called in\napply_data(). The rest of the whitespace checks are done there, and\nregardless of the \"patch that touches the same path twice\" issue,\nit feels like the right thing to do anyway.\n\nSuch a \"right\" fix might be involved. If we want to punt, I think\nyou can still inspect the *patch inside parse_single_patch(), and\nfigure out if the target path of the current fragment you are\nlooking at has already been touched in the current session (we parse\neverything into the patch struct whose fragments member has a\nchained list of fragments). Normally that should not be the case.\nIf we know we are not being fed such a patch with duplicated paths,\nwe do not have to inspect whitespace issues while parsing a context\n' ' line in parse_fragments().\n\n> I still don't see a better option than introducing a new value\n> `default`.\n\nAs long as we can tell that our input is not a patch that touches\nthe same path twice, we shouldn't need a new knob or a command line\noption. When we are dealing with Git generated patch that does not\nmention the same path twice, we can unconditionally say \"unless\n--ignore-space-change and other options that allow loose matching of\ncontext lines are given, it is an error if context lines do not\nexactly match, even when we are correcting for whitespace rule\nviolations\". Otherwise, we may need to keep the current workaround\nlogic in case a later change has a line as a ' ' context for a file\nthat an earlier change modified (and fixed with --whitespace=fix).\n \nIt is a different story if all you want to change is to add to\n--whitespace family of options a new \"silent-fix\" that makes\ncorrections in the same way as \"fix\" (or \"strip\"), but wihtout\ngiving any warnings. I think it makes sense to have such a mode,\nbut that is largely orthogonal to the discussion we are having, I\nthink, even though it might result in a similar effect visible to\nthe end-users.\n\nThanks.\n"},{"id":"502162","messageId":"f3f863da-50bd-41e3-981c-df93ae771d24@gmail.com","threadId":"62001","inReplyTo":"xmqqseugdo90.fsf@gitster.g","subject":"Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-09-04T18:20:37Z","receivedAt":"2024-09-04T18:20:40Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Tue, Sep 03, 2024 at 09:41:31PM -0700, Junio C Hamano wrote:\n\n> ... a new \"silent-fix\" that makes\n> corrections in the same way as \"fix\" (or \"strip\"), but wihtout\n> giving any warnings.\n\nMy main intention is not to make \"fix\" quieter, although it will\nprobably be a pleasant consequence.\n\nLet's consider a sequence of patches where some whitespace errors in\none patch are carried over to the next:\n \n $ # underscore (_) is whitespace for readability\n $ cat >file <patch1 <patch2 <foo <patch <