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

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.