[PATCH 0/5] `--whitespace=fix` with `--no-ignore-whitespace`

[PATCH 0/5] `--whitespace=fix` with `--no-ignore-whitespace`

[Rubén Justo]

When we see `--whitespace=fix` we don't consider a possible option:
`--no-ignore-whitespace`.  The following example produces unexpected,
at least for me, results:

    $ printf "a \nb\nc\n" >file
    $ git add file
    $ cat >patch <<END
    --- a/file
    +++ b/file
    @@ -1,3 +1,2 @@
     a
    -b
     c
    END
    $ git apply --no-ignore-whitespace --whitespace=fix patch
    $ xxd file
    00000000: 610a 630a                                a.c.

`git apply` should fail because the context line with 'a' doesn't
match the line with 'a ' in the file.

This series aims to make `--whitespace=fix` strictly match context
lines even if they have whitespace errors.

Adding a new `ignore_ws_default` is intended to reduce the blast
radius of changing the behavior of `--whitespace=fix`.  Perhaps there
are better ways to do this.  I'm open to suggestions.

The last two patches [4-5/5] contain minor code improvements I made
while reading the code working on this series.  They can be discarded
if anyone has concerns.

Thanks.

Rubén Justo (4):
  apply: introduce `ignore_ws_default`
  apply: honor `ignore_ws_none` with `correct_ws_error`
  apply: whitespace errors in context lines if we have `ignore_ws_none`
  apply: error message in `record_ws_error()`
  t4124: move test preparation into the test context

 apply.c                  | 17 ++++++++--------
 apply.h                  |  1 +
 t/t4124-apply-ws-rule.sh | 44 +++++++++++++++++++++++++++-------------
 3 files changed, 40 insertions(+), 22 deletions(-)

-- 
2.46.0.353.g385c909849

[PATCH 1/5] apply: introduce `ignore_ws_default`

[Rubén Justo]

When we see `--whitespace=fix` we don't consider a possible
option: `--no-ignore-whitespace`.

The expected result in the following example is a failure when
applying the patch, however:

    $ printf "a \nb\nc\n" >file
    $ git add file
    $ cat >patch <<END
    --- a/file
    +++ b/file
    @@ -1,3 +1,2 @@
     a
    -b
     c
    END
    $ git apply --no-ignore-whitespace --whitespace=fix patch
    $ xxd file
    00000000: 610a 630a                                a.c.

This unexpected result will be addressed in an upcoming commit.

As a preparation, we need to detect when the user has explicitly
said `--no-ignore-whitespace`.

Let's add a new value: `ignore_ws_default`, and use it to initialize
`ws_ignore_action` in `init_apply_state()`.  This will allow us to
distinguish whether the user has explicitly set any value for
`ws_ignore_action` via `--[no-]ignore-whitespace` or via
`apply.ignoreWhitespace`.

Currently, we only have one explicit consideration for
`ignore_ws_change`, and no, implicit or explicit, considerations for
`ignore_ws_none`.  Therefore, no modification to the existing logic
is required in this step.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 apply.c | 2 +-
 apply.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index 6e1060a952..63e58086f1 100644
--- a/apply.c
+++ b/apply.c
@@ -115,7 +115,7 @@ int init_apply_state(struct apply_state *state,
 	state->p_context = UINT_MAX;
 	state->squelch_whitespace_errors = 5;
 	state->ws_error_action = warn_on_ws_error;
-	state->ws_ignore_action = ignore_ws_none;
+	state->ws_ignore_action = ignore_ws_default;
 	state->linenr = 1;
 	string_list_init_nodup(&state->fn_table);
 	string_list_init_nodup(&state->limit_by_name);
diff --git a/apply.h b/apply.h
index cd25d24cc4..201f953a64 100644
--- a/apply.h
+++ b/apply.h
@@ -16,6 +16,7 @@ enum apply_ws_error_action {
 };
 
 enum apply_ws_ignore {
+	ignore_ws_default,
 	ignore_ws_none,
 	ignore_ws_change
 };
-- 
2.46.0.353.g385c909849

[PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`

[Rubén Justo]

Ensure strict matching of context lines when applying with
`--whitespace=fix` combined with `--no-ignore-whitespace`.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 apply.c                  |  3 ++-
 t/t4124-apply-ws-rule.sh | 27 +++++++++++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/apply.c b/apply.c
index 63e58086f1..0cb9d38e5a 100644
--- a/apply.c
+++ b/apply.c
@@ -2596,7 +2596,8 @@ static int match_fragment(struct apply_state *state,
 		goto out;
 	}
 
-	if (state->ws_error_action != correct_ws_error) {
+	if (state->ws_error_action != correct_ws_error ||
+	    state->ws_ignore_action == ignore_ws_none) {
 		ret = 0;
 		goto out;
 	}
diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index 485c7d2d12..573200da67 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -545,6 +545,33 @@ test_expect_success 'whitespace=fix to expand' '
 	git -c core.whitespace=tab-in-indent apply --whitespace=fix patch
 '
 
+test_expect_success 'whitespace=fix honors no-ignore-whitespace' '
+	qz_to_tab_space >preimage <<-\EOF &&
+	AZ
+	BZZ
+	EOF
+	qz_to_tab_space >patch <<-\EOF &&
+	diff --git a/preimage b/preimage
+	--- a/preimage
+	+++ b/preimage
+	@@ -1,2 +1,2 @@
+	-AZ
+	+A
+	 BZZZ
+	EOF
+	test_must_fail git apply --no-ignore-whitespace --whitespace=fix patch &&
+	qz_to_tab_space >patch <<-\EOF &&
+	diff --git a/preimage b/preimage
+	--- a/preimage
+	+++ b/preimage
+	@@ -1,2 +1,2 @@
+	-AZ
+	+A
+	 BZZ
+	EOF
+	git apply --no-ignore-whitespace --whitespace=fix patch
+'
+
 test_expect_success 'whitespace check skipped for excluded paths' '
 	git config core.whitespace blank-at-eol &&
 	>used &&
-- 
2.46.0.353.g385c909849

[PATCH 3/5] apply: whitespace errors in context lines if we have

[Rubén Justo]

If the user says `--no-ignore-space-change`, there's no need to
check for whitespace errors in the context lines.

Don't do it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 apply.c                  | 3 ++-
 t/t4124-apply-ws-rule.sh | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/apply.c b/apply.c
index 0cb9d38e5a..e1b4d14dba 100644
--- a/apply.c
+++ b/apply.c
@@ -1734,7 +1734,8 @@ static int parse_fragment(struct apply_state *state,
 			trailing++;
 			check_old_for_crlf(patch, line, len);
 			if (!state->apply_in_reverse &&
-			    state->ws_error_action == correct_ws_error)
+			    state->ws_error_action == correct_ws_error &&
+			    state->ws_ignore_action != ignore_ws_none)
 				check_whitespace(state, line, len, patch->ws_rule);
 			break;
 		case '-':
diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index 573200da67..e12b8333c3 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -569,7 +569,8 @@ test_expect_success 'whitespace=fix honors no-ignore-whitespace' '
 	+A
 	 BZZ
 	EOF
-	git apply --no-ignore-whitespace --whitespace=fix patch
+	git apply --no-ignore-whitespace --whitespace=fix patch 2>error &&
+	test_must_be_empty error
 '
 
 test_expect_success 'whitespace check skipped for excluded paths' '
-- 
2.46.0.353.g385c909849

[PATCH 4/5] apply: error message in `record_ws_error()`

[Rubén Justo]

It does not make sense to construct an error message if we're not
going to use it, especially when the process involves memory
allocations that need to be freed immediately.

If we know in advance that we won't use the message, not getting it
slightly reduces the workload and simplifies the code a bit.

Do it.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 apply.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/apply.c b/apply.c
index e1b4d14dba..e6df8b6ab4 100644
--- a/apply.c
+++ b/apply.c
@@ -1642,8 +1642,6 @@ static void record_ws_error(struct apply_state *state,
 			    int len,
 			    int linenr)
 {
-	char *err;
-
 	if (!result)
 		return;
 
@@ -1652,11 +1650,12 @@ static void record_ws_error(struct apply_state *state,
 	    state->squelch_whitespace_errors < state->whitespace_error)
 		return;
 
-	err = whitespace_error_string(result);
-	if (state->apply_verbosity > verbosity_silent)
+	if (state->apply_verbosity > verbosity_silent) {
+		char *err = whitespace_error_string(result);
 		fprintf(stderr, "%s:%d: %s.\n%.*s\n",
 			state->patch_input_file, linenr, err, len, line);
-	free(err);
+		free(err);
+	}
 }
 
 static void check_whitespace(struct apply_state *state,
-- 
2.46.0.353.g385c909849

[PATCH 5/5] t4124: move test preparation into the test context

[Rubén Justo]

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 t/t4124-apply-ws-rule.sh | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index e12b8333c3..56f15dc3ef 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -423,14 +423,8 @@ test_expect_success 'missing blanks at EOF must only match blank lines' '
 	test_must_fail git apply --ignore-space-change --whitespace=fix patch
 '
 
-sed -e's/Z//' >one <<EOF
-a
-b
-c
-		      Z
-EOF
-
 test_expect_success 'missing blank line should match context line with spaces' '
+	test_write_lines a b c "		      " >one &&
 	git add one &&
 	echo d >>one &&
 	git diff -- one >patch &&
@@ -443,14 +437,8 @@ test_expect_success 'missing blank line should match context line with spaces' '
 	test_cmp expect one
 '
 
-sed -e's/Z//' >one <<EOF
-a
-b
-c
-		      Z
-EOF
-
 test_expect_success 'same, but with the --ignore-space-option' '
+	test_write_lines a b c "		      " >one &&
 	git add one &&
 	echo d >>one &&
 	cp one expect &&
-- 
2.46.0.353.g385c909849

Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> Ensure strict matching of context lines when applying with
> `--whitespace=fix` combined with `--no-ignore-whitespace`.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  apply.c                  |  3 ++-
>  t/t4124-apply-ws-rule.sh | 27 +++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/apply.c b/apply.c
> index 63e58086f1..0cb9d38e5a 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -2596,7 +2596,8 @@ static int match_fragment(struct apply_state *state,
>  		goto out;
>  	}
>  
> -	if (state->ws_error_action != correct_ws_error) {
> +	if (state->ws_error_action != correct_ws_error ||
> +	    state->ws_ignore_action == ignore_ws_none) {
>  		ret = 0;
>  		goto out;
>  	}

Hmph, if we are correcting for whitespace violations, even if
whitespace fuzz is not allowed externally, wouldn't the issue that
c1beba5b (git-apply --whitespace=fix: fix whitespace fuzz introduced
by previous run, 2008-01-30) corrected still apply?  IOW, isn't this
change introducing a regression when an input touches a file with a
change with broken whitespaces, and then touches the same file to
replace the broken whitespace lines with something else?

Re: [PATCH 3/5] apply: whitespace errors in context lines if we have

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> If the user says `--no-ignore-space-change`, there's no need to
> check for whitespace errors in the context lines.
>
> Don't do it.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  apply.c                  | 3 ++-
>  t/t4124-apply-ws-rule.sh | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index 0cb9d38e5a..e1b4d14dba 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1734,7 +1734,8 @@ static int parse_fragment(struct apply_state *state,
>  			trailing++;
>  			check_old_for_crlf(patch, line, len);
>  			if (!state->apply_in_reverse &&
> -			    state->ws_error_action == correct_ws_error)
> +			    state->ws_error_action == correct_ws_error &&
> +			    state->ws_ignore_action != ignore_ws_none)
>  				check_whitespace(state, line, len, patch->ws_rule);
>  			break;

Hmph.  0a80bc9f (apply: detect and mark whitespace errors in context
lines when fixing, 2015-01-16) deliberately added this check because
we will correct the whitespace breakages on these lines after
parsing the hunk with this function while applying.

It is iffy that this case arm for " " kicks in ONLY when applying in
the forward direction (which is not what you are changing).  When
applying a patch in reverse, " " is still an "unchanged" context
line, so we should be treating it the same way regardless of the
direction.

But at least the call to check_whitespace() from this place when we
are correcting whitespace rule violations is not iffy, as far as I
can tell.



> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> index 573200da67..e12b8333c3 100755
> --- a/t/t4124-apply-ws-rule.sh
> +++ b/t/t4124-apply-ws-rule.sh
> @@ -569,7 +569,8 @@ test_expect_success 'whitespace=fix honors no-ignore-whitespace' '
>  	+A
>  	 BZZ
>  	EOF
> -	git apply --no-ignore-whitespace --whitespace=fix patch
> +	git apply --no-ignore-whitespace --whitespace=fix patch 2>error &&
> +	test_must_be_empty error
>  '
>  
>  test_expect_success 'whitespace check skipped for excluded paths' '

Re: [PATCH 4/5] apply: error message in `record_ws_error()`

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> It does not make sense to construct an error message if we're not
> going to use it, especially when the process involves memory
> allocations that need to be freed immediately.
>
> If we know in advance that we won't use the message, not getting it
> slightly reduces the workload and simplifies the code a bit.

Makes sense.

>
> Do it.

No need to say this when the above two paragraphs are clear enough,
like in this patch.

>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  apply.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/apply.c b/apply.c
> index e1b4d14dba..e6df8b6ab4 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1642,8 +1642,6 @@ static void record_ws_error(struct apply_state *state,
>  			    int len,
>  			    int linenr)
>  {
> -	char *err;
> -
>  	if (!result)
>  		return;
>  
> @@ -1652,11 +1650,12 @@ static void record_ws_error(struct apply_state *state,
>  	    state->squelch_whitespace_errors < state->whitespace_error)
>  		return;
>  
> -	err = whitespace_error_string(result);
> -	if (state->apply_verbosity > verbosity_silent)
> +	if (state->apply_verbosity > verbosity_silent) {
> +		char *err = whitespace_error_string(result);
>  		fprintf(stderr, "%s:%d: %s.\n%.*s\n",
>  			state->patch_input_file, linenr, err, len, line);
> -	free(err);
> +		free(err);
> +	}
>  }
>  
>  static void check_whitespace(struct apply_state *state,

Re: [PATCH 3/5] apply: whitespace errors in context lines if we have

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> If the user says `--no-ignore-space-change`, there's no need to
> check for whitespace errors in the context lines.

Because the default is *not* to ignore space change, the command
should behave exactly the same way between two cases: (1) the user
uses the default and does not give the "--ignore-space-change"
option, and (2) the user gives the "--no-ignore-space-change" option
explicitly.  So I am very much convinced that [1/5] is unneeded, and
"If the user says `--no-*`" in the above proposed log message is
insufficient (at least it also needs to say "or uses the default and
does not say "--ignore-space-change").

> Don't do it.

No need to say this, when the paragraphs above clearly and
unambiguously leads to this conclusion.

> diff --git a/apply.c b/apply.c
> index 0cb9d38e5a..e1b4d14dba 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1734,7 +1734,8 @@ static int parse_fragment(struct apply_state *state,
>  			trailing++;
>  			check_old_for_crlf(patch, line, len);
>  			if (!state->apply_in_reverse &&
> -			    state->ws_error_action == correct_ws_error)
> +			    state->ws_error_action == correct_ws_error &&
> +			    state->ws_ignore_action != ignore_ws_none)
>  				check_whitespace(state, line, len, patch->ws_rule);

I am not 100% convinced that this change is _wrong_, but it does
smell like reverting a necessary change as I pointed out in another
reply.

Thanks.

Re: [PATCH 1/5] apply: introduce `ignore_ws_default`

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> When we see `--whitespace=fix` we don't consider a possible
> option: `--no-ignore-whitespace`.
>
> The expected result in the following example is a failure when
> applying the patch, however:
>
>     $ printf "a \nb\nc\n" >file
>     $ git add file
>     $ cat >patch <<END
>     --- a/file
>     +++ b/file
>     @@ -1,3 +1,2 @@
>      a
>     -b
>      c
>     END
>     $ git apply --no-ignore-whitespace --whitespace=fix patch
>     $ xxd file
>     00000000: 610a 630a                                a.c.
>
> This unexpected result will be addressed in an upcoming commit.
>
> As a preparation, we need to detect when the user has explicitly
> said `--no-ignore-whitespace`.

If you said, before all of the above, what _other_ case you are
trying to differenciate from the case where the user explicitly gave
the "--no-ignore-whitespace" option, it would clarify why a
differenciator is needed.  IOW, perhaps start

    By default, "git apply" does not ignore whitespace changes
    (i.e. state.ws_ignore_action is initialized to ignore_ws_none).
    However we want to treat this default case and the case where
    the user explicitly gave the "--no-ignore-whitespace" option FOR
    SUCH AND SUCH REASONS.

    ... elaborate SUCH AND SUCH REASONS as needed here ...

    Initialize state.ws_ignore_action to ignore_ws_default, and
    later after the parse_options() returns, if the state is still
    _default, we can tell there wasn't such an explicit option.

or something?

The rest of the code paths are not told what to do when they see
ws_ignore_action is set to this new value, so I somehow find it iffy
that this step is complete.  Shouldn't it at least flip some other bit
after apply_parse_options() makes parse_options() call and notices that
the default value is still there, and then replace the _default value
with ws_none, or something, along the lines of ...

 apply.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git i/apply.c w/apply.c
index 6e1060a952..acc0f64d37 100644
--- i/apply.c
+++ w/apply.c
@@ -5190,5 +5190,13 @@ int apply_parse_options(int argc, const char **argv,
 		OPT_END()
 	};
 
-	return parse_options(argc, argv, state->prefix, builtin_apply_options, apply_usage, 0);
+	ret = parse_options(argc, argv, state->prefix,
+			    builtin_apply_options, apply_usage, 0);
+	if (!ret) {
+		if (state->ws_ignore_action == ignore_ws_default) {
+			... note that --no-ignore-whitespace was *NOT* used ...
+			state->ws_ignore_action = ignore_ws_none;
+		}
+	}
+	return ret;
 }

... without that anywhere state.ws_ignore_action gets inspected, the
all must treat _none and _default pretty much the same way, no?

> Currently, we only have one explicit consideration for
> `ignore_ws_change`, and no, implicit or explicit, considerations for
> `ignore_ws_none`.  Therefore, no modification to the existing logic
> is required in this step.

Yes, that is a plausible excuse, but it feels somehat brittle.

More importantly, the proposed log message does not explain why
"--no-ignore-whitespace", which is the default, needs to be special
cased when it is given explicitly.  You had symptoms you want to fix
described, but it is probably a few steps disconnected from the
reason why the default vs explicit setting of ws_ignore_action need
to make the code behave differently.

Thanks.

Re: [PATCH 3/5] apply: whitespace errors in context lines if we have

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Hmph.  0a80bc9f (apply: detect and mark whitespace errors in context
> lines when fixing, 2015-01-16) deliberately added this check because
> we will correct the whitespace breakages on these lines after
> parsing the hunk with this function while applying.
>
> It is iffy that this case arm for " " kicks in ONLY when applying in
> the forward direction (which is not what you are changing).  When
> applying a patch in reverse, " " is still an "unchanged" context
> line, so we should be treating it the same way regardless of the
> direction.
>
> But at least the call to check_whitespace() from this place when we
> are correcting whitespace rule violations is not iffy, as far as I
> can tell.

Having said all that, I do have to wonder how much value we are
getting by supporting that odd "feature" that makes apply take input
in a single session a patch that touches the same path TWICE.

If we can get rid of that feature (which I consider a misfeature),
we can lose quote a lot of code (anything that touches fn_table can
go) and recover the code quality that got visibly worse with the
addition of that feature back.

And without the "input may touch the same path TWICE", we do not
have to worry about this "context lines after applying a single
patch with whitespace=fix will have to be matched loosely with
respect to the whitespace when another patch modifies the same file
around the same lines", making your changes in [3/5] trivially the
right thing to do.

So, I am inclined to say that

 * we propose to get rid of that "a single input may touch the same
   path TWICE" feature at Git 3.0 boundary.

 * we at the same time apply [3/5] (and possibly others, but I do
   not think we want [1/5]).

But until we can shed our pretense that the "single input may touch
the same path TWICE" is seriously supported, I do not think applying
this series as-is makes sense, as it directly contradicts with that
(mis)feature.

Re: [PATCH 3/5] apply: whitespace errors in context lines if we have

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Hmph.  0a80bc9f (apply: detect and mark whitespace errors in context
>> lines when fixing, 2015-01-16) deliberately added this check because
>> we will correct the whitespace breakages on these lines after
>> parsing the hunk with this function while applying.
> ...
> So, I am inclined to say that
>
>  * we propose to get rid of that "a single input may touch the same
>    path TWICE" feature at Git 3.0 boundary.
>
>  * we at the same time apply [3/5] (and possibly others, but I do
>    not think we want [1/5]).
>
> But until we can shed our pretense that the "single input may touch
> the same path TWICE" is seriously supported, I do not think applying
> this series as-is makes sense, as it directly contradicts with that
> (mis)feature.

So, here is another thought.  Can we notice that we are dealing with
such an irregular patch that we would never produce ourselves, but
still have to support as a historical wart?  And deal with context
lines with whitespace breakages differently if that is the case.

I think that is doable.  I won't address the entire set of fixes in
your series, but a touched up version of your [3/5] may look like
the attached at the end.  This is on top of your whole series, not
as a replacement for [3/5], made just for illustration purposes.

>> It is iffy that this case arm for " " kicks in ONLY when applying in
>> the forward direction (which is not what you are changing).  When
>> applying a patch in reverse, " " is still an "unchanged" context
>> line, so we should be treating it the same way regardless of the
>> direction.

I didn't address this "why only in the forward direction?" iffyness
in the illustration patch, by the way.

 apply.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git c/apply.c w/apply.c
index e6df8b6ab4..04bb094e57 100644
--- c/apply.c
+++ w/apply.c
@@ -38,6 +38,8 @@
 #include "wildmatch.h"
 #include "ws.h"
 
+static struct patch *in_fn_table(struct apply_state *state, const char *name);
+
 struct gitdiff_data {
 	struct strbuf *root;
 	int linenr;
@@ -1697,10 +1699,13 @@ static int parse_fragment(struct apply_state *state,
 	int len = linelen(line, size), offset;
 	unsigned long oldlines, newlines;
 	unsigned long leading, trailing;
+	int touching_same_path; 
 
 	offset = parse_fragment_header(line, len, fragment);
 	if (offset < 0)
 		return -1;
+
+	touching_same_path = !!in_fn_table(state, patch->old_name);
 	if (offset > 0 && patch->recount)
 		recount_diff(line + offset, size - offset, fragment);
 	oldlines = fragment->oldlines;
@@ -1734,7 +1739,8 @@ static int parse_fragment(struct apply_state *state,
 			check_old_for_crlf(patch, line, len);
 			if (!state->apply_in_reverse &&
 			    state->ws_error_action == correct_ws_error &&
-			    state->ws_ignore_action != ignore_ws_none)
+			    (touching_same_path ||
+			     state->ws_ignore_action != ignore_ws_none))
 				check_whitespace(state, line, len, patch->ws_rule);
 			break;
 		case '-':

Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`

[Rubén Justo]

On Mon, Aug 26, 2024 at 05:35:14PM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > Ensure strict matching of context lines when applying with
> > `--whitespace=fix` combined with `--no-ignore-whitespace`.
> >
> > Signed-off-by: Rubén Justo <rjusto@gmail.com>
> > ---
> >  apply.c                  |  3 ++-
> >  t/t4124-apply-ws-rule.sh | 27 +++++++++++++++++++++++++++
> >  2 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/apply.c b/apply.c
> > index 63e58086f1..0cb9d38e5a 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -2596,7 +2596,8 @@ static int match_fragment(struct apply_state *state,
> >  		goto out;
> >  	}
> >  
> > -	if (state->ws_error_action != correct_ws_error) {
> > +	if (state->ws_error_action != correct_ws_error ||
> > +	    state->ws_ignore_action == ignore_ws_none) {
> >  		ret = 0;
> >  		goto out;
> >  	}
> 
> Hmph, if we are correcting for whitespace violations, even if
> whitespace fuzz is not allowed externally, wouldn't the issue that
> c1beba5b (git-apply --whitespace=fix: fix whitespace fuzz introduced
> by previous run, 2008-01-30) corrected still apply?  IOW, isn't this
> change introducing a regression when an input touches a file with a
> change with broken whitespaces, and then touches the same file to
> replace the broken whitespace lines with something else?

Yes, that is the center of the blast this series is producing;
affecting to what `--whitespace=fix` does (just a reminder for other
readers):

  - stop fixing whitespace errors in context lines and

  - no longer warning about them.

Clearly, as you point out, the change poses a problem when applying
changes with whitespace errors in lines involved multiple times,
either during the same apply session or across multiple sessions
executed in sequence.

The new `ignore_ws_default` enum option is intended to mitigate the
blast.  It would be unexpected IMHO for someone who wants the behavior
described in c1beba5b to be indicating `--no-ignore-whitespace`.  And
with it we allow the possibility for someone who finds that behavior
undesirable to avoid it.

I'm not very happy with the new enum, but I haven't come up with a
better idea.  There are other alternatives:

  --whitespace=fix-strict

  --do-not-fix-ws-errors-in-context-lines

None of them are better, I think.

Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> I'm not very happy with the new enum, but I haven't come up with a
> better idea.
> ...
> None of them are better, I think.

Not adding a new enum is probably much better.  See the "additional
thought" in my review on [3/5], for example.

Thanks.

Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`

[Rubén Justo]

On Thu, Aug 29, 2024 at 04:13:14PM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > I'm not very happy with the new enum, but I haven't come up with a
> > better idea.
> > ...
> > None of them are better, I think.
> 
> Not adding a new enum is probably much better.  See the "additional
> thought" in my review on [3/5], for example.

If I understand correctly the example you mentioned, using
`in_fn_table()` cannot help us in `parse_fragment()`.  But I could be
completely wrong and misunderstanding your intention.

I still don't see a better option than introducing a new value
`default`.  Perhaps described like this:

diff --git a/Documentation/config/apply.txt b/Documentation/config/apply.txt
index f9908e210a..7b642d2f3a 100644
--- a/Documentation/config/apply.txt
+++ b/Documentation/config/apply.txt
@@ -4,6 +4,10 @@ apply.ignoreWhitespace::
        option.
        When set to one of: no, none, never, false, it tells 'git apply' to
        respect all whitespace differences.
+       When not set or set to `default`, it tells `git apply` to
+       behave like the previous setting: `no`.  However, when
+       combined with 'whitespace=fix', some whitespace errors
+       will still be ignored because they are being fixed.
        See linkgit:git-apply[1].

 apply.whitespace:

Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`

[Junio C Hamano]

Rubén Justo <rjusto@gmail.com> writes:

> If I understand correctly the example you mentioned, using
> `in_fn_table()` cannot help us in `parse_fragment()`.  But I could be
> completely wrong and misunderstanding your intention.

Hmph.  It's unfortunate that the control flow goes like this:

 apply_all_patches()
 -> apply_patch()
    -> parse_chunk()
       -> parse_single_patch()
          -> parse_fragment()
    -> use_patch()
    -> check_patch_list()
       -> check_patch()
          -> apply_data()
             -> add_to_fn_table()

So deciding if a context line (or a new line for that matter)
contains a whitespace error is done too early in the current code,
and if you want to do this correctly, you'd need to move the check
down so that it happens in apply_one_fragment() that is called in
apply_data().  The rest of the whitespace checks are done there, and
regardless of the "patch that touches the same path twice" issue,
it feels like the right thing to do anyway.

Such a "right" fix might be involved.  If we want to punt, I think
you can still inspect the *patch inside parse_single_patch(), and
figure out if the target path of the current fragment you are
looking at has already been touched in the current session (we parse
everything into the patch struct whose fragments member has a
chained list of fragments).  Normally that should not be the case.
If we know we are not being fed such a patch with duplicated paths,
we do not have to inspect whitespace issues while parsing a context
' ' line in parse_fragments().

> I still don't see a better option than introducing a new value
> `default`.

As long as we can tell that our input is not a patch that touches
the same path twice, we shouldn't need a new knob or a command line
option.  When we are dealing with Git generated patch that does not
mention the same path twice, we can unconditionally say "unless
--ignore-space-change and other options that allow loose matching of
context lines are given, it is an error if context lines do not
exactly match, even when we are correcting for whitespace rule
violations".  Otherwise, we may need to keep the current workaround
logic in case a later change has a line as a ' ' context for a file
that an earlier change modified (and fixed with --whitespace=fix).
 
It is a different story if all you want to change is to add to
--whitespace family of options a new "silent-fix" that makes
corrections in the same way as "fix" (or "strip"), but wihtout
giving any warnings.  I think it makes sense to have such a mode,
but that is largely orthogonal to the discussion we are having, I
think, even though it might result in a similar effect visible to
the end-users.

Thanks.

Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`

[Rubén Justo]

On Tue, Sep 03, 2024 at 09:41:31PM -0700, Junio C Hamano wrote:

> ... a new "silent-fix" that makes
> corrections in the same way as "fix" (or "strip"), but wihtout
> giving any warnings.

My main intention is not to make "fix" quieter, although it will
probably be a pleasant consequence.

Let's consider a sequence of patches where some whitespace errors in
one patch are carried over to the next:
    
    $ # underscore (_) is whitespace for readability
    $ cat >file <<END
    a
    END
    $ cat >patch1 <<END # this adds a whitespace error
    --- v/file
    +++ m/file
    @@ -1 +1,2 @@
     a
    +b_
    END
    $ cat >patch2 <<END # this adds another one
    --- v/file
    +++ m/file
    @@ -1,2 +1,3 @@
     a
     b_
    +c_
    END

When applying "patch1" with `--whitespace=fix`, we'll fix the
whitespace error in "b ".  Consequently, when applying "patch2" we'll
need to fix that line in the patch before applying it, so that it
matches the context line, now with "b".  Makes sense.

This applies not only to:

    $ git apply --whitespace=fix patch1 && \
      git apply --whitespace=fix patch2
    
But to:

    $ git apply --whitespace=fix patch1 patch2
    
Even to:

    $ git apply --whitespace=fix <(cat patch1 patch2)

So far, so good.

However, a legit question is: Why "a " is being modified here?:

    $ cat >foo <<END
    a_
    b_
    END
    $ cat >patch <<END
    --- v/foo
    +++ m/foo
    @@ -1,2 +1,2 @@
     a
    -b_
    +b
    END
    $ git apply -v --whitespace=fix patch
    Checking patch foo...
    Applied patch foo cleanly.
    $ xxd foo
    00000000: 610a                                     a.

Is this a bug?  I don't think so.  But the result, IMHO, is
questionable, and "git apply" rejecting the patch could also be an
expected outcome.

We are assuming an implicit, and perhaps unwanted, step:

    --- v/foo
    +++ m/foo
    @@ -1,2 +1,2 @@
    -a_
    +a
     b_
    END

We cannot change the default behaviour (I won't dig into this), but I
think we can give a knob to allow changing it.  This is the main
intention of the, ugly, new "_default" enum value.

And... as a nice side effect, we'll know when to stop showing warnings
when the user touches lines next to other lines with whitespace errors
that they don't want, or can, change ;-)