| Message ID | 5da09529-e95b-407b-9e66-34ebac4b4128@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | `--whitespace=fix` with `--no-ignore-whitespace` | expand |
Rubén Justo <rjusto@gmail.com> writes: > If the user says `--no-ignore-space-change`, there's no need to > check for whitespace errors in the context lines. > > Don't do it. > > Signed-off-by: Rubén Justo <rjusto@gmail.com> > --- > apply.c | 3 ++- > t/t4124-apply-ws-rule.sh | 3 ++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/apply.c b/apply.c > index 0cb9d38e5a..e1b4d14dba 100644 > --- a/apply.c > +++ b/apply.c > @@ -1734,7 +1734,8 @@ static int parse_fragment(struct apply_state *state, > trailing++; > check_old_for_crlf(patch, line, len); > if (!state->apply_in_reverse && > - state->ws_error_action == correct_ws_error) > + state->ws_error_action == correct_ws_error && > + state->ws_ignore_action != ignore_ws_none) > check_whitespace(state, line, len, patch->ws_rule); > break; Hmph. 0a80bc9f (apply: detect and mark whitespace errors in context lines when fixing, 2015-01-16) deliberately added this check because we will correct the whitespace breakages on these lines after parsing the hunk with this function while applying. It is iffy that this case arm for " " kicks in ONLY when applying in the forward direction (which is not what you are changing). When applying a patch in reverse, " " is still an "unchanged" context line, so we should be treating it the same way regardless of the direction. But at least the call to check_whitespace() from this place when we are correcting whitespace rule violations is not iffy, as far as I can tell. > diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh > index 573200da67..e12b8333c3 100755 > --- a/t/t4124-apply-ws-rule.sh > +++ b/t/t4124-apply-ws-rule.sh > @@ -569,7 +569,8 @@ test_expect_success 'whitespace=fix honors no-ignore-whitespace' ' > +A > BZZ > EOF > - git apply --no-ignore-whitespace --whitespace=fix patch > + git apply --no-ignore-whitespace --whitespace=fix patch 2>error && > + test_must_be_empty error > ' > > test_expect_success 'whitespace check skipped for excluded paths' '
Rubén Justo <rjusto@gmail.com> writes: > If the user says `--no-ignore-space-change`, there's no need to > check for whitespace errors in the context lines. Because the default is *not* to ignore space change, the command should behave exactly the same way between two cases: (1) the user uses the default and does not give the "--ignore-space-change" option, and (2) the user gives the "--no-ignore-space-change" option explicitly. So I am very much convinced that [1/5] is unneeded, and "If the user says `--no-*`" in the above proposed log message is insufficient (at least it also needs to say "or uses the default and does not say "--ignore-space-change"). > Don't do it. No need to say this, when the paragraphs above clearly and unambiguously leads to this conclusion. > diff --git a/apply.c b/apply.c > index 0cb9d38e5a..e1b4d14dba 100644 > --- a/apply.c > +++ b/apply.c > @@ -1734,7 +1734,8 @@ static int parse_fragment(struct apply_state *state, > trailing++; > check_old_for_crlf(patch, line, len); > if (!state->apply_in_reverse && > - state->ws_error_action == correct_ws_error) > + state->ws_error_action == correct_ws_error && > + state->ws_ignore_action != ignore_ws_none) > check_whitespace(state, line, len, patch->ws_rule); I am not 100% convinced that this change is _wrong_, but it does smell like reverting a necessary change as I pointed out in another reply. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Hmph. 0a80bc9f (apply: detect and mark whitespace errors in context > lines when fixing, 2015-01-16) deliberately added this check because > we will correct the whitespace breakages on these lines after > parsing the hunk with this function while applying. > > It is iffy that this case arm for " " kicks in ONLY when applying in > the forward direction (which is not what you are changing). When > applying a patch in reverse, " " is still an "unchanged" context > line, so we should be treating it the same way regardless of the > direction. > > But at least the call to check_whitespace() from this place when we > are correcting whitespace rule violations is not iffy, as far as I > can tell. Having said all that, I do have to wonder how much value we are getting by supporting that odd "feature" that makes apply take input in a single session a patch that touches the same path TWICE. If we can get rid of that feature (which I consider a misfeature), we can lose quote a lot of code (anything that touches fn_table can go) and recover the code quality that got visibly worse with the addition of that feature back. And without the "input may touch the same path TWICE", we do not have to worry about this "context lines after applying a single patch with whitespace=fix will have to be matched loosely with respect to the whitespace when another patch modifies the same file around the same lines", making your changes in [3/5] trivially the right thing to do. So, I am inclined to say that * we propose to get rid of that "a single input may touch the same path TWICE" feature at Git 3.0 boundary. * we at the same time apply [3/5] (and possibly others, but I do not think we want [1/5]). But until we can shed our pretense that the "single input may touch the same path TWICE" is seriously supported, I do not think applying this series as-is makes sense, as it directly contradicts with that (mis)feature.
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> Hmph. 0a80bc9f (apply: detect and mark whitespace errors in context >> lines when fixing, 2015-01-16) deliberately added this check because >> we will correct the whitespace breakages on these lines after >> parsing the hunk with this function while applying. > ... > So, I am inclined to say that > > * we propose to get rid of that "a single input may touch the same > path TWICE" feature at Git 3.0 boundary. > > * we at the same time apply [3/5] (and possibly others, but I do > not think we want [1/5]). > > But until we can shed our pretense that the "single input may touch > the same path TWICE" is seriously supported, I do not think applying > this series as-is makes sense, as it directly contradicts with that > (mis)feature. So, here is another thought. Can we notice that we are dealing with such an irregular patch that we would never produce ourselves, but still have to support as a historical wart? And deal with context lines with whitespace breakages differently if that is the case. I think that is doable. I won't address the entire set of fixes in your series, but a touched up version of your [3/5] may look like the attached at the end. This is on top of your whole series, not as a replacement for [3/5], made just for illustration purposes. >> It is iffy that this case arm for " " kicks in ONLY when applying in >> the forward direction (which is not what you are changing). When >> applying a patch in reverse, " " is still an "unchanged" context >> line, so we should be treating it the same way regardless of the >> direction. I didn't address this "why only in the forward direction?" iffyness in the illustration patch, by the way. apply.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git c/apply.c w/apply.c index e6df8b6ab4..04bb094e57 100644 --- c/apply.c +++ w/apply.c @@ -38,6 +38,8 @@ #include "wildmatch.h" #include "ws.h" +static struct patch *in_fn_table(struct apply_state *state, const char *name); + struct gitdiff_data { struct strbuf *root; int linenr; @@ -1697,10 +1699,13 @@ static int parse_fragment(struct apply_state *state, int len = linelen(line, size), offset; unsigned long oldlines, newlines; unsigned long leading, trailing; + int touching_same_path; offset = parse_fragment_header(line, len, fragment); if (offset < 0) return -1; + + touching_same_path = !!in_fn_table(state, patch->old_name); if (offset > 0 && patch->recount) recount_diff(line + offset, size - offset, fragment); oldlines = fragment->oldlines; @@ -1734,7 +1739,8 @@ static int parse_fragment(struct apply_state *state, check_old_for_crlf(patch, line, len); if (!state->apply_in_reverse && state->ws_error_action == correct_ws_error && - state->ws_ignore_action != ignore_ws_none) + (touching_same_path || + state->ws_ignore_action != ignore_ws_none)) check_whitespace(state, line, len, patch->ws_rule); break; case '-':
diff --git a/apply.c b/apply.c index 0cb9d38e5a..e1b4d14dba 100644 --- a/apply.c +++ b/apply.c @@ -1734,7 +1734,8 @@ static int parse_fragment(struct apply_state *state, trailing++; check_old_for_crlf(patch, line, len); if (!state->apply_in_reverse && - state->ws_error_action == correct_ws_error) + state->ws_error_action == correct_ws_error && + state->ws_ignore_action != ignore_ws_none) check_whitespace(state, line, len, patch->ws_rule); break; case '-': diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh index 573200da67..e12b8333c3 100755 --- a/t/t4124-apply-ws-rule.sh +++ b/t/t4124-apply-ws-rule.sh @@ -569,7 +569,8 @@ test_expect_success 'whitespace=fix honors no-ignore-whitespace' ' +A BZZ EOF - git apply --no-ignore-whitespace --whitespace=fix patch + git apply --no-ignore-whitespace --whitespace=fix patch 2>error && + test_must_be_empty error ' test_expect_success 'whitespace check skipped for excluded paths' '
If the user says `--no-ignore-space-change`, there's no need to check for whitespace errors in the context lines. Don't do it. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- apply.c | 3 ++- t/t4124-apply-ws-rule.sh | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)