[1/5] apply: introduce `ignore_ws_default`

Commit Message

Rubén Justo Aug. 25, 2024, 10:17 a.m. UTC

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 <[e-mail]>
---
 apply.c | 2 +-
 apply.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

Comments

Junio C Hamano Aug. 27, 2024, 1:11 a.m. UTC | #1

Rubén Justo <[e-mail]> writes:

> 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`.

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

diff --git i/apply.c w/apply.c
index 6e1060a952..acc0f64d37 100644
--- i/apply.c
+++ w/apply.c
@@ -5190,5 +5190,13 @@ int apply_parse_options(int argc, const char **argv,
 		OPT_END()
 	};
 
-	return parse_options(argc, argv, state->prefix, builtin_apply_options, apply_usage, 0);
+	ret = parse_options(argc, argv, state->prefix,
+			    builtin_apply_options, apply_usage, 0);
+	if (!ret) {
+		if (state->ws_ignore_action == ignore_ws_default) {
+			... note that --no-ignore-whitespace was *NOT* used ...
+			state->ws_ignore_action = ignore_ws_none;
+		}
+	}
+	return ret;
 }

... without that anywhere state.ws_ignore_action gets inspected, the
all must treat _none and _default pretty much the same way, no?

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

Yes, that is a plausible excuse, but it feels somehat brittle.

More importantly, the proposed log message does not explain why
"--no-ignore-whitespace", which is the default, needs to be special
cased when it is given explicitly.  You had symptoms you want to fix
described, but it is probably a few steps disconnected from the
reason why the default vs explicit setting of ws_ignore_action need
to make the code behave differently.

Thanks.

Patch

diff --git a/apply.c b/apply.c
index 6e1060a952..63e58086f1 100644
--- a/apply.c
+++ b/apply.c
@@ -115,7 +115,7 @@  int init_apply_state(struct apply_state *state,
 	state->p_context = UINT_MAX;
 	state->squelch_whitespace_errors = 5;
 	state->ws_error_action = warn_on_ws_error;
-	state->ws_ignore_action = ignore_ws_none;
+	state->ws_ignore_action = ignore_ws_default;
 	state->linenr = 1;
 	string_list_init_nodup(&state->fn_table);
 	string_list_init_nodup(&state->limit_by_name);
diff --git a/apply.h b/apply.h
index cd25d24cc4..201f953a64 100644
--- a/apply.h
+++ b/apply.h
@@ -16,6 +16,7 @@  enum apply_ws_error_action {
 };
 
 enum apply_ws_ignore {
+	ignore_ws_default,
 	ignore_ws_none,
 	ignore_ws_change
 };