Rubén Justo <rjusto@xxxxxxxxx> writes:
> 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.
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().
> I still don't see a better option than introducing a new value
> `default`.
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.
