From: Rubén Justo <rjusto@gmail.com>
Date: Wed, 04 Sep 2024 18:20:37 GMT
Subject: Re: [PATCH 2/5] apply: honor `ignore_ws_none` with `correct_ws_error`
Message-ID: <f3f863da-50bd-41e3-981c-df93ae771d24@gmail.com>
In-Reply-To: <xmqqseugdo90.fsf@gitster.g>

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 ;-)