Re: [PATCH 1/5] apply: introduce `ignore_ws_default`

[Junio C Hamano <gitster@pobox.com>]

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