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

From: Rubén Justo <rjusto@gmail.com>
Date: Sep 4, 2024, 18:20 UTC
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 ;-)