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