Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`
- From
Rubén Justo <rjusto@gmail.com>
- Date
- Aug 29, 2024, 05:07 UTC
- Message-ID
- <afade304-51e3-441d-9ae6-e0a422d00bc4@gmail.com>
- In-Reply-To
- <xmqqseuqerb1.fsf@gitster.g>
On Mon, Aug 26, 2024 at 05:35:14PM -0700, Junio C Hamano wrote:
Show 33 quoted lines
> 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.