From: Phillip Wood Date: Mon, 10 Nov 2025 14:53:40 GMT Subject: Re: [PATCH v2 00/12] Incomplete lines Message-ID: In-Reply-To: <20251105213052.1499224-1-gitster@pobox.com> Hi Junio On 05/11/2025 21:30, Junio C Hamano wrote: > One of the common kind of whitespace errors is to lack the final > newline at the end of a file, but so far, neither "git diff" or "git > apply" did anything about them. > > This series introduces "incomplete-line" whitespace error class, > that you can add to either the core.whitespace configuration > variable, or the whitespace attribute in your .gitattributes files. I've read through all the diff related changes but not the changes to apply. I've left a few comments but the cleanups and the new implementation of the new feature look good to me. Thanks Phillip > The class is disabled by default, so the final step enables it for > our project by defining it in the .gitattributes file. > > The incomplete line marker that is given for a context line is not > considered an error. The reasoning is that your preimage did have > incomplete line, but you did not touch the contents on that > incomplete line in your patch, so you left the line intact. It is > not a new breakage you are responsible for. > > If the incomplete line marker follows a postimage line, on the other > hand, it means that you added a new line at the end of the file that > is incomplete *and* that line did not exist in the preimage. The > last line of the preimage may have been incomplete already, but then > you updated the contents on that line, so you could have easily > fixed the incompleteness of the line while at it. Either way, you > are responsible for the incompleteness of the last ine in the > resulting file. > > The organization of the series is as follows. > > * The first patch [01/12] is a clean-up we have seen earlier on the > list already (https://lore.kernel.org/git/xmqqfrb4hyjl.fsf@gitster.g/). > > * The patches [02/12] - [08/12] are preliminary clean-up made to > both "git diff" and "git apply" machinery. > > * The patch [09/12] shifts the bit assignment (cleaned-up in > [01/12] without changing any values) to make room for new > whitespace error class (which was last updated in 2007 IIRC, so > the set of whitespace errors surprisingly haven't changed for > quite some time), and defines the new "incomplete-line" class. > > * The patch [10/12] teaches "git apply --whitespace=" and > "git apply --check" about the incomplete-line error class. > > * The patch [11/12] teaches "git diff [--check]" about the > incomplete-line error class. > > * The final patch [12/12] enables the incomplete-line error class > for our project for C source files and shell scripts. I didn't > touch the cover-all * entry. > > Changes in v2: > > - rolled the definition (but not implementation) of the new > "incomplete-line" class into step [09/12] that shifts the bit > assignment. The documentation of core.whitespace has also be > updated in this step. > > - "git apply --check" miscounted line number reported for the > incomplete line error, which has been corrected in step [10/12]. > > - t4124-apply-ws-rule.sh has been extended to cover "git apply > --check" and the diagnostic output from it in step [10/12]. > > Junio C Hamano (12): > whitespace: correct bit assignment comments > diff: emit_line_ws_markup() if/else style fix > diff: correct suppress_blank_empty hack > diff: fix incorrect counting of line numbers > diff: refactor output of incomplete line > diff: call emit_callback ecbdata everywhere > diff: update the way rewrite diff handles incomplete lines > apply: revamp the parsing of incomplete lines > whitespace: allocate a few more bits and define WS_INCOMPLETE_LINE > apply: check and fix incomplete lines > diff: highlight and error out on incomplete lines > attr: enable incomplete-line whitespace error for this project > > .gitattributes | 4 +- > Documentation/config/core.adoc | 2 + > apply.c | 79 ++++++++++---- > diff.c | 148 +++++++++++++++++--------- > diff.h | 6 +- > t/t4015-diff-whitespace.sh | 63 ++++++++++- > t/t4124-apply-ws-rule.sh | 187 +++++++++++++++++++++++++++++++++ > ws.c | 20 ++++ > ws.h | 26 +++-- > 9 files changed, 448 insertions(+), 87 deletions(-) > > Range-diff against v1: > 1: 4168f28fe7 = 1: 8a493cdea5 whitespace: correct bit assignment comments > 2: 53b7a010e7 = 2: a01d99a055 diff: emit_line_ws_markup() if/else style fix > 3: d93dd05543 = 3: e3ea40af19 diff: correct suppress_blank_empty hack > 4: 5f58400bd7 = 4: e15e89d3e2 diff: fix incorrect counting of line numbers > 5: 84c4ca147f = 5: c007b3d7a7 diff: refactor output of incomplete line > 6: 55b42a1944 = 6: 0cea57091b diff: call emit_callback ecbdata everywhere > 7: 6947838d13 = 7: 523196b440 diff: update the way rewrite diff handles incomplete lines > 8: 63c36c6f70 = 8: e098932784 apply: revamp the parsing of incomplete lines > 9: 00b645bb4e ! 9: 28538f149f whitespace: allocate a few more bits > @@ Metadata > Author: Junio C Hamano > > ## Commit message ## > - whitespace: allocate a few more bits > + whitespace: allocate a few more bits and define WS_INCOMPLETE_LINE > > Reserve a few more bits in the diff flags word to be used for future > - whitespace rules. No behaviour changes intended. > + whitespace rules. Add WS_INCOMPLETE_LINE without implementing the > + behaviour (yet). > > Signed-off-by: Junio C Hamano > > + ## Documentation/config/core.adoc ## > +@@ Documentation/config/core.adoc: core.whitespace:: > + part of the line terminator, i.e. with it, `trailing-space` > + does not trigger if the character before such a carriage-return > + is not a whitespace (not enabled by default). > ++* `incomplete-line` treats the last line of a file that is missing the > ++ newline at the end as an error (not enabled by default). > + * `tabwidth=` tells how many character positions a tab occupies; this > + is relevant for `indent-with-non-tab` and when Git fixes `tab-in-indent` > + errors. The default tab width is 8. Allowed values are 1 to 63. > + > ## diff.c ## > @@ diff.c: enum diff_symbol { > > @@ diff.h: struct diff_options { > const char *prefix; > int prefix_length; > > + ## ws.c ## > +@@ ws.c: static struct whitespace_rule { > + { "blank-at-eol", WS_BLANK_AT_EOL, 0 }, > + { "blank-at-eof", WS_BLANK_AT_EOF, 0 }, > + { "tab-in-indent", WS_TAB_IN_INDENT, 0, 1 }, > ++ { "incomplete-line", WS_INCOMPLETE_LINE, 0, 0 }, > + }; > + > + unsigned parse_whitespace_rule(const char *string) > +@@ ws.c: char *whitespace_error_string(unsigned ws) > + strbuf_addstr(&err, ", "); > + strbuf_addstr(&err, "tab in indent"); > + } > ++ if (ws & WS_INCOMPLETE_LINE) { > ++ if (err.len) > ++ strbuf_addstr(&err, ", "); > ++ strbuf_addstr(&err, "no newline at the end of file"); > ++ } > + return strbuf_detach(&err, NULL); > + } > + > + > ## ws.h ## > @@ ws.h: struct strbuf; > + #define WS_CR_AT_EOL (1<<9) > + #define WS_BLANK_AT_EOF (1<<10) > + #define WS_TAB_IN_INDENT (1<<11) > ++#define WS_INCOMPLETE_LINE (1<<12) > + > + #define WS_TRAILING_SPACE (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF) > + #define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|8) > #define WS_TAB_WIDTH_MASK ((1<<6)-1) > > /* All WS_* -- when extended, adapt constants defined after diff.c:diff_symbol */ > 10: 662f15d0b4 ! 10: 7369e77309 apply: check and fix incomplete lines > @@ Commit message > what happens when A ends in an incomplete line, for example), and > text-oriented tools often mishandle such a line. > > - Introduce a new whitespace rule "incomplete-line", which is off by > - default for backward compatibility's sake, so that "git apply > + Implement checks in "git apply" for incomplete lines, which is off > + by default for backward compatibility's sake, so that "git apply > --whitespace={fix,warn,error}" can notice, warn against, and fix > them. > > @@ Commit message > > Signed-off-by: Junio C Hamano > > + ## apply.c ## > +@@ apply.c: static void record_ws_error(struct apply_state *state, > + state->squelch_whitespace_errors < state->whitespace_error) > + return; > + > ++ /* > ++ * line[len] for an incomplete line points at the "\n" at the end > ++ * of patch input line, so "%.*s" would drop the last letter on line; > ++ * compensate for it. > ++ */ > ++ if (result & WS_INCOMPLETE_LINE) > ++ len++; > ++ > + err = whitespace_error_string(result); > + if (state->apply_verbosity > verbosity_silent) > + fprintf(stderr, "%s:%d: %s.\n%.*s\n", > +@@ apply.c: static int parse_fragment(struct apply_state *state, > + } > + > + /* eat the "\\ No newline..." as well, if exists */ > +- len += skip_len; > ++ if (skip_len) { > ++ len += skip_len; > ++ state->linenr++; > ++ } > + } > + if (oldlines || newlines) > + return -1; > + > ## t/t4124-apply-ws-rule.sh ## > @@ t/t4124-apply-ws-rule.sh: test_expect_success 'whitespace check skipped for excluded paths' ' > git apply --include=used --stat --whitespace=error @@ t/t4124-apply-ws-rule.sh: test_expect_success 'whitespace check skipped for excl > + git apply --whitespace=error + test_cmp sample2-i target && > + > ++ cat sample-i >target && > ++ git apply --whitespace=error --check error && > ++ test_cmp sample-i target && > ++ test_must_be_empty error && > ++ > + cat sample2-i >target && > + git apply --whitespace=error -R -+ test_cmp sample-i target > ++ test_cmp sample-i target && > ++ > ++ cat sample2-i >target && > ++ git apply -R --whitespace=error --check error && > ++ test_cmp sample2-i target && > ++ test_must_be_empty error > +' > + > +test_expect_success 'last line made incomplete (error)' ' > @@ t/t4124-apply-ws-rule.sh: test_expect_success 'whitespace check skipped for excl > + test_must_fail git apply --whitespace=error error && > + test_grep "no newline" error && > + > ++ cat sample >target && > ++ test_must_fail git apply --whitespace=error --check actual && > ++ test_cmp sample target && > ++ cat >expect <<-\EOF && > ++ :10: no newline at the end of file. > ++ 6 > ++ error: 1 line adds whitespace errors. > ++ EOF > ++ test_cmp expect actual && > ++ > + cat sample-i >target && > + git apply --whitespace=error -R + test_cmp sample target && > + > ++ cat sample-i >target && > ++ git apply --whitespace=error --check -R error && > ++ test_cmp sample-i target && > ++ test_must_be_empty error && > ++ > + cat sample >target && > + git apply --whitespace=fix + test_cmp sample target > @@ t/t4124-apply-ws-rule.sh: test_expect_success 'whitespace check skipped for excl > + git apply --whitespace=error + test_cmp sample target && > + > ++ cat sample-i >target && > ++ git apply --whitespace=error --check error && > ++ test_cmp sample-i target && > ++ test_must_be_empty error && > ++ > + cat sample >target && > + test_must_fail git apply --whitespace=error -R error && > + test_grep "no newline" error && > + > + cat sample >target && > ++ test_must_fail git apply --whitespace=error --check -R actual && > ++ test_cmp sample target && > ++ cat >expect <<-\EOF && > ++ :9: no newline at the end of file. > ++ 6 > ++ error: 1 line adds whitespace errors. > ++ EOF > ++ test_cmp expect actual && > ++ > ++ cat sample >target && > + git apply --whitespace=fix -R + test_cmp sample target > +' > @@ t/t4124-apply-ws-rule.sh: test_expect_success 'whitespace check skipped for excl > + git apply --whitespace=error + test_cmp sample3 target && > + > ++ cat sample-i >target && > ++ git apply --whitespace=error --check error && > ++ test_cmp sample-i target && > ++ test_must_be_empty error && > ++ > + cat sample3 >target && > + test_must_fail git apply --whitespace=error -R error && > + test_grep "no newline" error && > + > + cat sample3 >target && > ++ test_must_fail git apply --whitespace=error -R --check actual && > ++ test_cmp sample3 target && > ++ cat >expect <<-\EOF && > ++ :9: no newline at the end of file. > ++ 6 > ++ error: 1 line adds whitespace errors. > ++ EOF > ++ test_cmp expect actual && > ++ > ++ cat sample3 >target && > + git apply --whitespace=fix -R + test_cmp sample target > +' > @@ t/t4124-apply-ws-rule.sh: test_expect_success 'whitespace check skipped for excl > + test_must_fail git apply --whitespace=error error && > + test_grep "no newline" error && > + > ++ cat sample-i >target && > ++ test_must_fail git apply --whitespace=error --check actual && > ++ test_cmp sample-i target && > ++ cat >expect <<-\EOF && > ++ :11: no newline at the end of file. > ++ 7 > ++ error: 1 line adds whitespace errors. > ++ EOF > ++ test_cmp expect actual && > ++ > + cat sample3-i >target && > + test_must_fail git apply --whitespace=error -R error && > + test_grep "no newline" error && > + > ++ cat sample3-i >target && > ++ test_must_fail git apply --whitespace=error --check -R actual && > ++ test_cmp sample3-i target && > ++ cat >expect <<-\EOF && > ++ :9: no newline at the end of file. > ++ 6 > ++ error: 1 line adds whitespace errors. > ++ EOF > ++ test_cmp expect actual && > ++ > + cat sample-i >target && > + git apply --whitespace=fix + test_cmp sample3 target && > @@ t/t4124-apply-ws-rule.sh: test_expect_success 'whitespace check skipped for excl > test_done > > ## ws.c ## > -@@ ws.c: static struct whitespace_rule { > - { "blank-at-eol", WS_BLANK_AT_EOL, 0 }, > - { "blank-at-eof", WS_BLANK_AT_EOF, 0 }, > - { "tab-in-indent", WS_TAB_IN_INDENT, 0, 1 }, > -+ { "incomplete-line", WS_INCOMPLETE_LINE, 0, 0 }, > - }; > - > - unsigned parse_whitespace_rule(const char *string) > -@@ ws.c: char *whitespace_error_string(unsigned ws) > - strbuf_addstr(&err, ", "); > - strbuf_addstr(&err, "tab in indent"); > - } > -+ if (ws & WS_INCOMPLETE_LINE) { > -+ if (err.len) > -+ strbuf_addstr(&err, ", "); > -+ strbuf_addstr(&err, "no newline at the end of file"); > -+ } > - return strbuf_detach(&err, NULL); > - } > - > @@ ws.c: static unsigned ws_check_emit_1(const char *line, int len, unsigned ws_rule, > if (trailing_whitespace == -1) > trailing_whitespace = len; > @@ ws.c: void ws_fix_copy(struct strbuf *dst, const char *src, int len, unsigned ws > /* > * Strip trailing whitespace > */ > - > - ## ws.h ## > -@@ ws.h: struct strbuf; > - #define WS_CR_AT_EOL (1<<9) > - #define WS_BLANK_AT_EOF (1<<10) > - #define WS_TAB_IN_INDENT (1<<11) > -+#define WS_INCOMPLETE_LINE (1<<12) > - > - #define WS_TRAILING_SPACE (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF) > - #define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|8) > 11: 36de2ac901 = 11: 17c2fa50a7 diff: highlight and error out on incomplete lines > 12: e82056bf55 = 12: 73af29fba7 attr: enable incomplete-line whitespace error for this project