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

["Rubén Justo" <rjusto@gmail.com>]

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.