patch, 5 parts`--whitespace=fix` with `--no-ignore-whitespace`

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

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

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

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

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

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

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?

On Mon, Aug 26, 2024 at 05:35:14PM -0700, Junio C Hamano wrote:

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.

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

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

Thanks.

On Thu, Aug 29, 2024 at 04:13:14PM -0700, Junio C Hamano wrote:

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:

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

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().

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.

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

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

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

Rubén Justo <rjusto@gmail.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.

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

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.

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

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.

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

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

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").

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

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.

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

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

Makes sense.

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>
---
 t/t4124-apply-ws-rule.sh | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)