From 1187df57c2ee1119b7ef357cacb63d10581256bc Mon Sep 17 00:00:00 2001 From: Linus Torvalds Date: Sun, 26 Feb 2006 09:29:00 -0800 Subject: The war on trailing whitespace On Sat, 25 Feb 2006, Andrew Morton wrote: > > I'd suggest a) git will simply refuse to apply such a patch unless given a > special `forcing' flag, b) even when thus forced, it will still warn and c) > with a different flag, it will strip-then-apply, without generating a > warning. This doesn't do the "strip-then-apply" thing, but it allows you to make git-apply generate a warning or error on extraneous whitespace. Use --whitespace=warn to warn, and (surprise, surprise) --whitespace=error to make it a fatal error to have whitespace at the end. Totally untested, of course. But it compiles, so it must be fine. HOWEVER! Note that this literally will check every single patch-line with "+" at the beginning. Which means that if you fix a simple typo, and the line had a space at the end before, and you didn't remove it, that's still considered a "new line with whitespace at the end", even though obviously the line wasn't really new. I assume this is what you wanted, and there isn't really any sane alternatives (you could make the warning activate only for _pure_ additions with no deletions at all in that hunk, but that sounds a bit insane). Linus diff --git a/apply.c b/apply.c index 2ad47fb..69229f8 100644 --- a/apply.c +++ b/apply.c @@ -34,6 +34,12 @@ static int line_termination = '\n'; static const char apply_usage[] = "git-apply [--stat] [--numstat] [--summary] [--check] [--index] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [-z] [-pNUM] ..."; +static enum whitespace_eol { + nowarn, + warn_on_whitespace, + error_on_whitespace +} new_whitespace = nowarn; + /* * For "diff-stat" like behaviour, we keep track of the biggest change * we've seen, and the longest filename. That allows us to do simple @@ -815,6 +821,22 @@ static int parse_fragment(char *line, unsigned long size, struct patch *patch, s oldlines--; break; case '+': + /* + * We know len is at least two, since we have a '+' and + * we checked that the last character was a '\n' above + */ + if (isspace(line[len-2])) { + switch (new_whitespace) { + case nowarn: + break; + case warn_on_whitespace: + new_whitespace = nowarn; /* Just once */ + error("Added whitespace at end of line at line %d", linenr); + break; + case error_on_whitespace: + die("Added whitespace at end of line at line %d", linenr); + } + } added++; newlines--; break; @@ -1831,6 +1853,17 @@ int main(int argc, char **argv) line_termination = 0; continue; } + if (!strncmp(arg, "--whitespace=", 13)) { + if (strcmp(arg+13, "warn")) { + new_whitespace = warn_on_whitespace; + continue; + } + if (strcmp(arg+13, "error")) { + new_whitespace = error_on_whitespace; + continue; + } + die("unrecognixed whitespace option '%s'", arg+13); + } if (check_index && prefix_length < 0) { prefix = setup_git_directory(); -- cgit v0.10.2-6-g49f6 From 5c7b580c94b4217803a116e24a3ad924bfb35f86 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 26 Feb 2006 18:13:25 -0800 Subject: apply --whitespace fixes and enhancements. In addition to fixing obvious command line parsing bugs in the previous round, this changes the following: * Adds "--whitespace=strip". This applies after stripping the new trailing whitespaces introduced to the patch. * The output error message format is changed to say "patch-filename:linenumber:contents of the line". This makes it similar to typical compiler error message format, and helps C-x ` (next-error) in Emacs compilation buffer. * --whitespace=error and --whitespace=warn do not stop at the first error. We might want to limit the output to say first 20 such lines to prevent cluttering, but on the other hand if you are willing to hand-fix after inspecting them, getting everything with a single run might be easier to work with. After all, somebody has to do the clean-up work somewhere. Signed-off-by: Junio C Hamano diff --git a/apply.c b/apply.c index 69229f8..a2f39b8 100644 --- a/apply.c +++ b/apply.c @@ -37,8 +37,11 @@ static const char apply_usage[] = static enum whitespace_eol { nowarn, warn_on_whitespace, - error_on_whitespace + error_on_whitespace, + strip_and_apply, } new_whitespace = nowarn; +static int whitespace_error = 0; +static const char *patch_input_file = NULL; /* * For "diff-stat" like behaviour, we keep track of the biggest change @@ -823,19 +826,17 @@ static int parse_fragment(char *line, unsigned long size, struct patch *patch, s case '+': /* * We know len is at least two, since we have a '+' and - * we checked that the last character was a '\n' above + * we checked that the last character was a '\n' above. + * That is, an addition of an empty line would check + * the '+' here. Sneaky... */ - if (isspace(line[len-2])) { - switch (new_whitespace) { - case nowarn: - break; - case warn_on_whitespace: - new_whitespace = nowarn; /* Just once */ - error("Added whitespace at end of line at line %d", linenr); - break; - case error_on_whitespace: - die("Added whitespace at end of line at line %d", linenr); - } + if ((new_whitespace != nowarn) && + isspace(line[len-2])) { + fprintf(stderr, "Added whitespace\n"); + fprintf(stderr, "%s:%d:%.*s\n", + patch_input_file, + linenr, len-2, line+1); + whitespace_error = 1; } added++; newlines--; @@ -1114,6 +1115,27 @@ struct buffer_desc { unsigned long alloc; }; +static int apply_line(char *output, const char *patch, int plen) +{ + /* plen is number of bytes to be copied from patch, + * starting at patch+1 (patch[0] is '+'). Typically + * patch[plen] is '\n'. + */ + int add_nl_to_tail = 0; + if ((new_whitespace == strip_and_apply) && + 1 < plen && isspace(patch[plen-1])) { + if (patch[plen] == '\n') + add_nl_to_tail = 1; + plen--; + while (0 < plen && isspace(patch[plen])) + plen--; + } + memcpy(output, patch + 1, plen); + if (add_nl_to_tail) + output[plen++] = '\n'; + return plen; +} + static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag) { char *buf = desc->buffer; @@ -1149,10 +1171,9 @@ static int apply_one_fragment(struct buffer_desc *desc, struct fragment *frag) break; /* Fall-through for ' ' */ case '+': - if (*patch != '+' || !no_add) { - memcpy(new + newsize, patch + 1, plen); - newsize += plen; - } + if (*patch != '+' || !no_add) + newsize += apply_line(new + newsize, patch, + plen); break; case '@': case '\\': /* Ignore it, we already handled it */ @@ -1713,7 +1734,7 @@ static int use_patch(struct patch *p) return 1; } -static int apply_patch(int fd) +static int apply_patch(int fd, const char *filename) { int newfd; unsigned long offset, size; @@ -1721,6 +1742,7 @@ static int apply_patch(int fd) struct patch *list = NULL, **listp = &list; int skipped_patch = 0; + patch_input_file = filename; if (!buffer) return -1; offset = 0; @@ -1747,6 +1769,9 @@ static int apply_patch(int fd) } newfd = -1; + if (whitespace_error && (new_whitespace == error_on_whitespace)) + apply = 0; + write_index = check_index && apply; if (write_index) newfd = hold_index_file_for_update(&cache_file, get_index_file()); @@ -1793,7 +1818,7 @@ int main(int argc, char **argv) int fd; if (!strcmp(arg, "-")) { - apply_patch(0); + apply_patch(0, ""); read_stdin = 0; continue; } @@ -1854,14 +1879,18 @@ int main(int argc, char **argv) continue; } if (!strncmp(arg, "--whitespace=", 13)) { - if (strcmp(arg+13, "warn")) { + if (!strcmp(arg+13, "warn")) { new_whitespace = warn_on_whitespace; continue; } - if (strcmp(arg+13, "error")) { + if (!strcmp(arg+13, "error")) { new_whitespace = error_on_whitespace; continue; } + if (!strcmp(arg+13, "strip")) { + new_whitespace = strip_and_apply; + continue; + } die("unrecognixed whitespace option '%s'", arg+13); } @@ -1877,10 +1906,12 @@ int main(int argc, char **argv) if (fd < 0) usage(apply_usage); read_stdin = 0; - apply_patch(fd); + apply_patch(fd, arg); close(fd); } if (read_stdin) - apply_patch(0); + apply_patch(0, ""); + if (whitespace_error && new_whitespace == error_on_whitespace) + return 1; return 0; } -- cgit v0.10.2-6-g49f6 From 59aa256204f74e02579d7586884c73f0db6eb0d6 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 27 Feb 2006 14:16:30 -0800 Subject: apply: squelch excessive errors and --whitespace=error-all This by default makes --whitespace=warn, error, and strip to warn only the first 5 additions of trailing whitespaces. A new option --whitespace=error-all can be used to view all of them before applying. Signed-off-by: Junio C Hamano diff --git a/apply.c b/apply.c index a2f39b8..f01e3ca 100644 --- a/apply.c +++ b/apply.c @@ -41,6 +41,8 @@ static enum whitespace_eol { strip_and_apply, } new_whitespace = nowarn; static int whitespace_error = 0; +static int squelch_whitespace_errors = 5; +static int applied_after_stripping = 0; static const char *patch_input_file = NULL; /* @@ -832,11 +834,16 @@ static int parse_fragment(char *line, unsigned long size, struct patch *patch, s */ if ((new_whitespace != nowarn) && isspace(line[len-2])) { - fprintf(stderr, "Added whitespace\n"); - fprintf(stderr, "%s:%d:%.*s\n", - patch_input_file, - linenr, len-2, line+1); - whitespace_error = 1; + whitespace_error++; + if (squelch_whitespace_errors && + squelch_whitespace_errors < + whitespace_error) + ; + else { + fprintf(stderr, "Adds trailing whitespace.\n%s:%d:%.*s\n", + patch_input_file, + linenr, len-2, line+1); + } } added++; newlines--; @@ -1129,6 +1136,7 @@ static int apply_line(char *output, const char *patch, int plen) plen--; while (0 < plen && isspace(patch[plen])) plen--; + applied_after_stripping++; } memcpy(output, patch + 1, plen); if (add_nl_to_tail) @@ -1887,11 +1895,16 @@ int main(int argc, char **argv) new_whitespace = error_on_whitespace; continue; } + if (!strcmp(arg+13, "error-all")) { + new_whitespace = error_on_whitespace; + squelch_whitespace_errors = 0; + continue; + } if (!strcmp(arg+13, "strip")) { new_whitespace = strip_and_apply; continue; } - die("unrecognixed whitespace option '%s'", arg+13); + die("unrecognized whitespace option '%s'", arg+13); } if (check_index && prefix_length < 0) { @@ -1911,7 +1924,31 @@ int main(int argc, char **argv) } if (read_stdin) apply_patch(0, ""); - if (whitespace_error && new_whitespace == error_on_whitespace) - return 1; + if (whitespace_error) { + if (squelch_whitespace_errors && + squelch_whitespace_errors < whitespace_error) { + int squelched = + whitespace_error - squelch_whitespace_errors; + fprintf(stderr, "warning: squelched %d whitespace error%s\n", + squelched, + squelched == 1 ? "" : "s"); + } + if (new_whitespace == error_on_whitespace) + die("%d line%s add%s trailing whitespaces.", + whitespace_error, + whitespace_error == 1 ? "" : "s", + whitespace_error == 1 ? "s" : ""); + if (applied_after_stripping) + fprintf(stderr, "warning: %d line%s applied after" + " stripping trailing whitespaces.\n", + applied_after_stripping, + applied_after_stripping == 1 ? "" : "s"); + else if (whitespace_error) + fprintf(stderr, "warning: %d line%s add%s trailing" + " whitespaces.\n", + whitespace_error, + whitespace_error == 1 ? "" : "s", + whitespace_error == 1 ? "s" : ""); + } return 0; } -- cgit v0.10.2-6-g49f6 From 383e20b6144d01286c90c0c82d88f92173a038f6 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 27 Feb 2006 14:47:45 -0800 Subject: apply --whitespace: configuration option. The new configuration option apply.whitespace can take one of "warn", "error", "error-all", or "strip". When git-apply is run to apply the patch to the index, they are used as the default value if there is no command line --whitespace option. Andrew can now tell people who feed him git trees to update to this version and say: git repo-config apply.whitespace error Signed-off-by: Junio C Hamano diff --git a/apply.c b/apply.c index f01e3ca..4748d7f 100644 --- a/apply.c +++ b/apply.c @@ -35,16 +35,42 @@ static const char apply_usage[] = "git-apply [--stat] [--numstat] [--summary] [--check] [--index] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [-z] [-pNUM] ..."; static enum whitespace_eol { - nowarn, + nowarn_whitespace, warn_on_whitespace, error_on_whitespace, - strip_and_apply, -} new_whitespace = nowarn; + strip_whitespace, +} new_whitespace = nowarn_whitespace; static int whitespace_error = 0; static int squelch_whitespace_errors = 5; static int applied_after_stripping = 0; static const char *patch_input_file = NULL; +static void parse_whitespace_option(const char *option) +{ + if (!option) { + new_whitespace = nowarn_whitespace; + return; + } + if (!strcmp(option, "warn")) { + new_whitespace = warn_on_whitespace; + return; + } + if (!strcmp(option, "error")) { + new_whitespace = error_on_whitespace; + return; + } + if (!strcmp(option, "error-all")) { + new_whitespace = error_on_whitespace; + squelch_whitespace_errors = 0; + return; + } + if (!strcmp(option, "strip")) { + new_whitespace = strip_whitespace; + return; + } + die("unrecognized whitespace option '%s'", option); +} + /* * For "diff-stat" like behaviour, we keep track of the biggest change * we've seen, and the longest filename. That allows us to do simple @@ -832,7 +858,7 @@ static int parse_fragment(char *line, unsigned long size, struct patch *patch, s * That is, an addition of an empty line would check * the '+' here. Sneaky... */ - if ((new_whitespace != nowarn) && + if ((new_whitespace != nowarn_whitespace) && isspace(line[len-2])) { whitespace_error++; if (squelch_whitespace_errors && @@ -1129,7 +1155,7 @@ static int apply_line(char *output, const char *patch, int plen) * patch[plen] is '\n'. */ int add_nl_to_tail = 0; - if ((new_whitespace == strip_and_apply) && + if ((new_whitespace == strip_whitespace) && 1 < plen && isspace(patch[plen-1])) { if (patch[plen] == '\n') add_nl_to_tail = 1; @@ -1816,10 +1842,21 @@ static int apply_patch(int fd, const char *filename) return 0; } +static int git_apply_config(const char *var, const char *value) +{ + if (!strcmp(var, "apply.whitespace")) { + apply_default_whitespace = strdup(value); + return 0; + } + return git_default_config(var, value); +} + + int main(int argc, char **argv) { int i; int read_stdin = 1; + const char *whitespace_option = NULL; for (i = 1; i < argc; i++) { const char *arg = argv[i]; @@ -1887,30 +1924,17 @@ int main(int argc, char **argv) continue; } if (!strncmp(arg, "--whitespace=", 13)) { - if (!strcmp(arg+13, "warn")) { - new_whitespace = warn_on_whitespace; - continue; - } - if (!strcmp(arg+13, "error")) { - new_whitespace = error_on_whitespace; - continue; - } - if (!strcmp(arg+13, "error-all")) { - new_whitespace = error_on_whitespace; - squelch_whitespace_errors = 0; - continue; - } - if (!strcmp(arg+13, "strip")) { - new_whitespace = strip_and_apply; - continue; - } - die("unrecognized whitespace option '%s'", arg+13); + whitespace_option = arg + 13; + parse_whitespace_option(arg + 13); + continue; } if (check_index && prefix_length < 0) { prefix = setup_git_directory(); prefix_length = prefix ? strlen(prefix) : 0; - git_config(git_default_config); + git_config(git_apply_config); + if (!whitespace_option && apply_default_whitespace) + parse_whitespace_option(apply_default_whitespace); } if (0 < prefix_length) arg = prefix_filename(prefix, prefix_length, arg); diff --git a/cache.h b/cache.h index 9f4adf5..f686e72 100644 --- a/cache.h +++ b/cache.h @@ -160,10 +160,12 @@ extern int hold_index_file_for_update(struct cache_file *, const char *path); extern int commit_index_file(struct cache_file *); extern void rollback_index_file(struct cache_file *); +/* Environment bits from configuration mechanism */ extern int trust_executable_bit; extern int only_use_symrefs; extern int diff_rename_limit_default; extern int shared_repository; +extern const char *apply_default_whitespace; #define GIT_REPO_VERSION 0 extern int repository_format_version; diff --git a/environment.c b/environment.c index 0596fc6..73e4b1c 100644 --- a/environment.c +++ b/environment.c @@ -16,6 +16,7 @@ int only_use_symrefs = 0; int repository_format_version = 0; char git_commit_encoding[MAX_ENCODING_LENGTH] = "utf-8"; int shared_repository = 0; +const char *apply_default_whitespace = NULL; static char *git_dir, *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file; -- cgit v0.10.2-6-g49f6 From 5c0d46eb3d5ab9182a2c6d942189671720f80f74 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 27 Feb 2006 17:07:16 -0800 Subject: git-apply --whitespace=nowarn Andrew insists --whitespace=warn should be the default, and I tend to agree. This introduces --whitespace=warn, so if your project policy is more lenient, you can squelch them by having apply.whitespace=nowarn in your configuration file. Signed-off-by: Junio C Hamano diff --git a/apply.c b/apply.c index 4748d7f..af9900f 100644 --- a/apply.c +++ b/apply.c @@ -39,7 +39,7 @@ static enum whitespace_eol { warn_on_whitespace, error_on_whitespace, strip_whitespace, -} new_whitespace = nowarn_whitespace; +} new_whitespace = warn_on_whitespace; static int whitespace_error = 0; static int squelch_whitespace_errors = 5; static int applied_after_stripping = 0; @@ -48,13 +48,17 @@ static const char *patch_input_file = NULL; static void parse_whitespace_option(const char *option) { if (!option) { - new_whitespace = nowarn_whitespace; + new_whitespace = warn_on_whitespace; return; } if (!strcmp(option, "warn")) { new_whitespace = warn_on_whitespace; return; } + if (!strcmp(option, "nowarn")) { + new_whitespace = nowarn_whitespace; + return; + } if (!strcmp(option, "error")) { new_whitespace = error_on_whitespace; return; -- cgit v0.10.2-6-g49f6 From 56248c5a5c77f65fe591dfec3ac413147d227ec4 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 28 Feb 2006 01:12:52 -0800 Subject: git-apply: war on whitespace -- finishing touches. This changes the default --whitespace policy to nowarn when we are only getting --stat, --summary etc. IOW when not applying the patch. When applying the patch, the default is warn (spit out warning message but apply the patch). Signed-off-by: Junio C Hamano diff --git a/apply.c b/apply.c index af9900f..453482a 100644 --- a/apply.c +++ b/apply.c @@ -75,6 +75,15 @@ static void parse_whitespace_option(const char *option) die("unrecognized whitespace option '%s'", option); } +static void set_default_whitespace_mode(const char *whitespace_option) +{ + if (!whitespace_option && !apply_default_whitespace) { + new_whitespace = (apply + ? warn_on_whitespace + : nowarn_whitespace); + } +} + /* * For "diff-stat" like behaviour, we keep track of the biggest change * we've seen, and the longest filename. That allows us to do simple @@ -1947,9 +1956,11 @@ int main(int argc, char **argv) if (fd < 0) usage(apply_usage); read_stdin = 0; + set_default_whitespace_mode(whitespace_option); apply_patch(fd, arg); close(fd); } + set_default_whitespace_mode(whitespace_option); if (read_stdin) apply_patch(0, ""); if (whitespace_error) { -- cgit v0.10.2-6-g49f6 From 12cbbdc40b50458b29b76af740ddc0ab1979b322 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Tue, 28 Feb 2006 20:26:25 -0800 Subject: git-am: --whitespace=x option. This is passed down to git-apply to override the built-in default and per-repository configuration at runtime. Signed-off-by: Junio C Hamano diff --git a/git-am.sh b/git-am.sh index 7cc4ae5..ab133fb 100755 --- a/git-am.sh +++ b/git-am.sh @@ -100,7 +100,7 @@ fall_back_3way () { } prec=4 -dotest=.dotest sign= utf8= keep= skip= interactive= resolved= binary= +dotest=.dotest sign= utf8= keep= skip= interactive= resolved= binary= ws= while case "$#" in 0) break;; esac do @@ -133,6 +133,9 @@ do --sk|--ski|--skip) skip=t; shift ;; + --whitespace=*) + ws=$1; shift ;; + --) shift; break ;; -*) @@ -171,10 +174,11 @@ else exit 1 } - # -b, -s, -u and -k flags are kept for the resuming session after - # a patch failure. + # -b, -s, -u, -k and --whitespace flags are kept for the + # resuming session after a patch failure. # -3 and -i can and must be given when resuming. echo "$binary" >"$dotest/binary" + echo " $ws" >"$dotest/whitespace" echo "$sign" >"$dotest/sign" echo "$utf8" >"$dotest/utf8" echo "$keep" >"$dotest/keep" @@ -202,6 +206,7 @@ if test "$(cat "$dotest/keep")" = t then keep=-k fi +ws=`cat "$dotest/whitespace"` if test "$(cat "$dotest/sign")" = t then SIGNOFF=`git-var GIT_COMMITTER_IDENT | sed -e ' @@ -355,7 +360,7 @@ do case "$resolved" in '') - git-apply $binary --index "$dotest/patch" + git-apply $binary --index $ws "$dotest/patch" apply_status=$? ;; t) -- cgit v0.10.2-6-g49f6