* [PATCH 1/2] builtin-apply: rename "whitespace" variables and fix styles
@ 2007-11-24 4:24 Junio C Hamano
2007-11-24 4:25 ` [PATCH 2/2] builtin-apply: teach whitespace_rules Junio C Hamano
0 siblings, 1 reply; 43+ messages in thread
From: Junio C Hamano @ 2007-11-24 4:24 UTC (permalink / raw)
To: git
The variables were somewhat misnamed.
* "What to do when whitespace errors are detected" is now called
"ws_error_action" (used to be called "new_whitespace");
* The constants to denote the possible actions are "nowarn_ws_error",
"warn_on_ws_error", "die_on_ws_error", and "correct_ws_error". The
last one used to be "strip_whitespace", but we correct whitespace
error in indent (SP followed by HT) and "strip" is not quite an
accurate name for it.
Other than the renaming of variables and constants, there is no
functional change in this patch. While we are at it, it also fixes
overly long lines and multi-line comment styles (which of course do
not affect the generated code at all).
Ah, by the way, this introduces a synonym "fix" that is equivalent to
"strip" that you can give to --whitespace=<what-to-do>, so in that sense
it is not exactly "no functional change whatsoever".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This is on top of jc/spht topic that is in 'next'. The next one is
the real thing.
builtin-apply.c | 148 ++++++++++++++++++++++++++++++++++---------------------
1 files changed, 91 insertions(+), 57 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 8411b38..eb09bfe 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -47,12 +47,12 @@ static unsigned long p_context = ULONG_MAX;
static const char apply_usage[] =
"git-apply [--stat] [--numstat] [--summary] [--check] [--index] [--cached] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [--reverse] [--reject] [--verbose] [-z] [-pNUM] [-CNUM] [--whitespace=<nowarn|warn|error|error-all|strip>] <patch>...";
-static enum whitespace_eol {
- nowarn_whitespace,
- warn_on_whitespace,
- error_on_whitespace,
- strip_whitespace,
-} new_whitespace = warn_on_whitespace;
+static enum ws_error_action {
+ nowarn_ws_error,
+ warn_on_ws_error,
+ die_on_ws_error,
+ correct_ws_error,
+} ws_error_action = warn_on_ws_error;
static int whitespace_error;
static int squelch_whitespace_errors = 5;
static int applied_after_fixing_ws;
@@ -61,28 +61,28 @@ static const char *patch_input_file;
static void parse_whitespace_option(const char *option)
{
if (!option) {
- new_whitespace = warn_on_whitespace;
+ ws_error_action = warn_on_ws_error;
return;
}
if (!strcmp(option, "warn")) {
- new_whitespace = warn_on_whitespace;
+ ws_error_action = warn_on_ws_error;
return;
}
if (!strcmp(option, "nowarn")) {
- new_whitespace = nowarn_whitespace;
+ ws_error_action = nowarn_ws_error;
return;
}
if (!strcmp(option, "error")) {
- new_whitespace = error_on_whitespace;
+ ws_error_action = die_on_ws_error;
return;
}
if (!strcmp(option, "error-all")) {
- new_whitespace = error_on_whitespace;
+ ws_error_action = die_on_ws_error;
squelch_whitespace_errors = 0;
return;
}
- if (!strcmp(option, "strip")) {
- new_whitespace = strip_whitespace;
+ if (!strcmp(option, "strip") || !strcmp(option, "fix")) {
+ ws_error_action = correct_ws_error;
return;
}
die("unrecognized whitespace option '%s'", option);
@@ -90,11 +90,8 @@ static void parse_whitespace_option(const char *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);
- }
+ if (!whitespace_option && !apply_default_whitespace)
+ ws_error_action = (apply ? warn_on_ws_error : nowarn_ws_error);
}
/*
@@ -137,6 +134,11 @@ struct fragment {
#define BINARY_DELTA_DEFLATED 1
#define BINARY_LITERAL_DEFLATED 2
+/*
+ * This represents a "patch" to a file, both metainfo changes
+ * such as creation/deletion, filemode and content changes represented
+ * as a series of fragments.
+ */
struct patch {
char *new_name, *old_name, *def_name;
unsigned int old_mode, new_mode;
@@ -158,7 +160,8 @@ struct patch {
struct patch *next;
};
-static void say_patch_name(FILE *output, const char *pre, struct patch *patch, const char *post)
+static void say_patch_name(FILE *output, const char *pre,
+ struct patch *patch, const char *post)
{
fputs(pre, output);
if (patch->old_name && patch->new_name &&
@@ -229,7 +232,8 @@ static char *find_name(const char *line, char *def, int p_value, int terminate)
if (*line == '"') {
struct strbuf name;
- /* Proposed "new-style" GNU patch/diff format; see
+ /*
+ * Proposed "new-style" GNU patch/diff format; see
* http://marc.theaimsgroup.com/?l=git&m=112927316408690&w=2
*/
strbuf_init(&name, 0);
@@ -499,7 +503,8 @@ static int gitdiff_dissimilarity(const char *line, struct patch *patch)
static int gitdiff_index(const char *line, struct patch *patch)
{
- /* index line is N hexadecimal, "..", N hexadecimal,
+ /*
+ * index line is N hexadecimal, "..", N hexadecimal,
* and optional space with octal mode.
*/
const char *ptr, *eol;
@@ -550,7 +555,8 @@ static const char *stop_at_slash(const char *line, int llen)
return NULL;
}
-/* This is to extract the same name that appears on "diff --git"
+/*
+ * This is to extract the same name that appears on "diff --git"
* line. We do not find and return anything if it is a rename
* patch, and it is OK because we will find the name elsewhere.
* We need to reliably find name only when it is mode-change only,
@@ -584,7 +590,8 @@ static char *git_header_name(char *line, int llen)
goto free_and_fail1;
strbuf_remove(&first, 0, cp + 1 - first.buf);
- /* second points at one past closing dq of name.
+ /*
+ * second points at one past closing dq of name.
* find the second name.
*/
while ((second < line + llen) && isspace(*second))
@@ -627,7 +634,8 @@ static char *git_header_name(char *line, int llen)
return NULL;
name++;
- /* since the first name is unquoted, a dq if exists must be
+ /*
+ * since the first name is unquoted, a dq if exists must be
* the beginning of the second name.
*/
for (second = name; second < line + llen; second++) {
@@ -759,7 +767,7 @@ static int parse_num(const char *line, unsigned long *p)
}
static int parse_range(const char *line, int len, int offset, const char *expect,
- unsigned long *p1, unsigned long *p2)
+ unsigned long *p1, unsigned long *p2)
{
int digits, ex;
@@ -868,14 +876,14 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
return offset;
}
- /** --- followed by +++ ? */
+ /* --- followed by +++ ? */
if (memcmp("--- ", line, 4) || memcmp("+++ ", line + len, 4))
continue;
/*
* We only accept unified patches, so we want it to
* at least have "@@ -a,b +c,d @@\n", which is 14 chars
- * minimum
+ * minimum ("@@ -0,0 +1 @@\n" is the shortest).
*/
nextlen = linelen(line + len, size - len);
if (size < nextlen + 14 || memcmp("@@ -", line + len + nextlen, 4))
@@ -932,14 +940,14 @@ static void check_whitespace(const char *line, int len)
err, patch_input_file, linenr, len-2, line+1);
}
-
/*
* Parse a unified diff. Note that this really needs to parse each
* fragment separately, since the only way to know the difference
* between a "---" that is part of a patch, and a "---" that starts
* the next patch is to look at the line counts..
*/
-static int parse_fragment(char *line, unsigned long size, struct patch *patch, struct fragment *fragment)
+static int parse_fragment(char *line, unsigned long size,
+ struct patch *patch, struct fragment *fragment)
{
int added, deleted;
int len = linelen(line, size), offset;
@@ -980,7 +988,7 @@ static int parse_fragment(char *line, unsigned long size, struct patch *patch, s
break;
case '-':
if (apply_in_reverse &&
- new_whitespace != nowarn_whitespace)
+ ws_error_action != nowarn_ws_error)
check_whitespace(line, len);
deleted++;
oldlines--;
@@ -988,14 +996,15 @@ static int parse_fragment(char *line, unsigned long size, struct patch *patch, s
break;
case '+':
if (!apply_in_reverse &&
- new_whitespace != nowarn_whitespace)
+ ws_error_action != nowarn_ws_error)
check_whitespace(line, len);
added++;
newlines--;
trailing = 0;
break;
- /* We allow "\ No newline at end of file". Depending
+ /*
+ * We allow "\ No newline at end of file". Depending
* on locale settings when the patch was produced we
* don't know what this line looks like. The only
* thing we do know is that it begins with "\ ".
@@ -1013,7 +1022,8 @@ static int parse_fragment(char *line, unsigned long size, struct patch *patch, s
fragment->leading = leading;
fragment->trailing = trailing;
- /* If a fragment ends with an incomplete line, we failed to include
+ /*
+ * If a fragment ends with an incomplete line, we failed to include
* it in the above loop because we hit oldlines == newlines == 0
* before seeing it.
*/
@@ -1141,7 +1151,8 @@ static struct fragment *parse_binary_hunk(char **buf_p,
int *status_p,
int *used_p)
{
- /* Expect a line that begins with binary patch method ("literal"
+ /*
+ * Expect a line that begins with binary patch method ("literal"
* or "delta"), followed by the length of data before deflating.
* a sequence of 'length-byte' followed by base-85 encoded data
* should follow, terminated by a newline.
@@ -1190,7 +1201,8 @@ static struct fragment *parse_binary_hunk(char **buf_p,
size--;
break;
}
- /* Minimum line is "A00000\n" which is 7-byte long,
+ /*
+ * Minimum line is "A00000\n" which is 7-byte long,
* and the line length must be multiple of 5 plus 2.
*/
if ((llen < 7) || (llen-2) % 5)
@@ -1241,7 +1253,8 @@ static struct fragment *parse_binary_hunk(char **buf_p,
static int parse_binary(char *buffer, unsigned long size, struct patch *patch)
{
- /* We have read "GIT binary patch\n"; what follows is a line
+ /*
+ * We have read "GIT binary patch\n"; what follows is a line
* that says the patch method (currently, either "literal" or
* "delta") and the length of data before deflating; a
* sequence of 'length-byte' followed by base-85 encoded data
@@ -1271,7 +1284,8 @@ static int parse_binary(char *buffer, unsigned long size, struct patch *patch)
if (reverse)
used += used_1;
else if (status) {
- /* not having reverse hunk is not an error, but having
+ /*
+ * Not having reverse hunk is not an error, but having
* a corrupt reverse hunk is.
*/
free((void*) forward->patch);
@@ -1292,7 +1306,8 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)
if (offset < 0)
return offset;
- patchsize = parse_single_patch(buffer + offset + hdrsize, size - offset - hdrsize, patch);
+ patchsize = parse_single_patch(buffer + offset + hdrsize,
+ size - offset - hdrsize, patch);
if (!patchsize) {
static const char *binhdr[] = {
@@ -1368,8 +1383,10 @@ static void reverse_patches(struct patch *p)
}
}
-static const char pluses[] = "++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++";
-static const char minuses[]= "----------------------------------------------------------------------";
+static const char pluses[] =
+"++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++";
+static const char minuses[]=
+"----------------------------------------------------------------------";
static void show_stats(struct patch *patch)
{
@@ -1438,7 +1455,9 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)
}
}
-static int find_offset(const char *buf, unsigned long size, const char *fragment, unsigned long fragsize, int line, int *lines)
+static int find_offset(const char *buf, unsigned long size,
+ const char *fragment, unsigned long fragsize,
+ int line, int *lines)
{
int i;
unsigned long start, backwards, forwards;
@@ -1539,7 +1558,8 @@ static void remove_last_line(const char **rbuf, int *rsize)
static int apply_line(char *output, const char *patch, int plen)
{
- /* plen is number of bytes to be copied from patch,
+ /*
+ * plen is number of bytes to be copied from patch,
* starting at patch+1 (patch[0] is '+'). Typically
* patch[plen] is '\n', unless this is the incomplete
* last line.
@@ -1552,12 +1572,15 @@ static int apply_line(char *output, const char *patch, int plen)
int need_fix_leading_space = 0;
char *buf;
- if ((new_whitespace != strip_whitespace) || !whitespace_error ||
+ if ((ws_error_action != correct_ws_error) || !whitespace_error ||
*patch != '+') {
memcpy(output, patch + 1, plen);
return plen;
}
+ /*
+ * Strip trailing whitespace
+ */
if (1 < plen && isspace(patch[plen-1])) {
if (patch[plen] == '\n')
add_nl_to_tail = 1;
@@ -1567,6 +1590,9 @@ static int apply_line(char *output, const char *patch, int plen)
fixed = 1;
}
+ /*
+ * Check leading whitespaces (indent)
+ */
for (i = 1; i < plen; i++) {
char ch = patch[i];
if (ch == '\t') {
@@ -1583,7 +1609,8 @@ static int apply_line(char *output, const char *patch, int plen)
buf = output;
if (need_fix_leading_space) {
int consecutive_spaces = 0;
- /* between patch[1..last_tab_in_indent] strip the
+ /*
+ * between patch[1..last_tab_in_indent] strip the
* funny spaces, updating them to tab as needed.
*/
for (i = 1; i < last_tab_in_indent; i++, plen--) {
@@ -1613,7 +1640,8 @@ static int apply_line(char *output, const char *patch, int plen)
return output + plen - buf;
}
-static int apply_one_fragment(struct strbuf *buf, struct fragment *frag, int inaccurate_eof)
+static int apply_one_fragment(struct strbuf *buf, struct fragment *frag,
+ int inaccurate_eof)
{
int match_beginning, match_end;
const char *patch = frag->patch;
@@ -1695,8 +1723,9 @@ static int apply_one_fragment(struct strbuf *buf, struct fragment *frag, int ina
size -= len;
}
- if (inaccurate_eof && oldsize > 0 && old[oldsize - 1] == '\n' &&
- newsize > 0 && new[newsize - 1] == '\n') {
+ if (inaccurate_eof &&
+ oldsize > 0 && old[oldsize - 1] == '\n' &&
+ newsize > 0 && new[newsize - 1] == '\n') {
oldsize--;
newsize--;
}
@@ -1733,7 +1762,7 @@ static int apply_one_fragment(struct strbuf *buf, struct fragment *frag, int ina
if (match_beginning && offset)
offset = -1;
if (offset >= 0) {
- if (new_whitespace == strip_whitespace &&
+ if (ws_error_action == correct_ws_error &&
(buf->len - oldsize - offset == 0)) /* end of file? */
newsize -= new_blank_lines_at_end;
@@ -1758,9 +1787,10 @@ static int apply_one_fragment(struct strbuf *buf, struct fragment *frag, int ina
match_beginning = match_end = 0;
continue;
}
- /* Reduce the number of context lines
- * Reduce both leading and trailing if they are equal
- * otherwise just reduce the larger context.
+ /*
+ * Reduce the number of context lines; reduce both
+ * leading and trailing if they are equal otherwise
+ * just reduce the larger context.
*/
if (leading >= trailing) {
remove_first_line(&oldlines, &oldsize);
@@ -1820,7 +1850,8 @@ static int apply_binary(struct strbuf *buf, struct patch *patch)
const char *name = patch->old_name ? patch->old_name : patch->new_name;
unsigned char sha1[20];
- /* For safety, we require patch index line to contain
+ /*
+ * For safety, we require patch index line to contain
* full 40-byte textual SHA1 for old and new, at least for now.
*/
if (strlen(patch->old_sha1_prefix) != 40 ||
@@ -1831,7 +1862,8 @@ static int apply_binary(struct strbuf *buf, struct patch *patch)
"without full index line", name);
if (patch->old_name) {
- /* See if the old one matches what the patch
+ /*
+ * See if the old one matches what the patch
* applies to.
*/
hash_sha1_file(buf->buf, buf->len, blob_type, sha1);
@@ -1868,7 +1900,8 @@ static int apply_binary(struct strbuf *buf, struct patch *patch)
/* XXX read_sha1_file NUL-terminates */
strbuf_attach(buf, result, size, size + 1);
} else {
- /* We have verified buf matches the preimage;
+ /*
+ * We have verified buf matches the preimage;
* apply the patch data to it, which is stored
* in the patch->fragments->{patch,size}.
*/
@@ -2067,7 +2100,8 @@ static int check_patch(struct patch *patch, struct patch *prev_patch)
if (new_name && prev_patch && 0 < prev_patch->is_delete &&
!strcmp(prev_patch->old_name, new_name))
- /* A type-change diff is always split into a patch to
+ /*
+ * A type-change diff is always split into a patch to
* delete old, immediately followed by a patch to
* create new (see diff.c::run_diff()); in such a case
* it is Ok that the entry to be deleted by the
@@ -2671,7 +2705,7 @@ static int apply_patch(int fd, const char *filename, int inaccurate_eof)
offset += nr;
}
- if (whitespace_error && (new_whitespace == error_on_whitespace))
+ if (whitespace_error && (ws_error_action == die_on_ws_error))
apply = 0;
update_index = check_index && apply;
@@ -2866,7 +2900,7 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
squelched,
squelched == 1 ? "" : "s");
}
- if (new_whitespace == error_on_whitespace)
+ if (ws_error_action == die_on_ws_error)
die("%d line%s add%s whitespace errors.",
whitespace_error,
whitespace_error == 1 ? "" : "s",
--
1.5.3.6.1991.ge56ac
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 2/2] builtin-apply: teach whitespace_rules
2007-11-24 4:24 [PATCH 1/2] builtin-apply: rename "whitespace" variables and fix styles Junio C Hamano
@ 2007-11-24 4:25 ` Junio C Hamano
2007-11-24 20:09 ` [PATCH 3/2] core.whitespace: documentation updates Junio C Hamano
0 siblings, 1 reply; 43+ messages in thread
From: Junio C Hamano @ 2007-11-24 4:25 UTC (permalink / raw)
To: git
We earlier introduced core.whitespace to allow users to tweak the
definition of what the "whitespace errors" are, for the purpose of diff
output highlighting. This teaches the same to git-apply, so that the
command can both detect (when --whitespace=warn option is given) and fix
(when --whitespace=fix option is given) as configured.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-apply.c | 68 +++++++++++++++++-------
t/t4124-apply-ws-rule.sh | 133 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 182 insertions(+), 19 deletions(-)
create mode 100755 t/t4124-apply-ws-rule.sh
diff --git a/builtin-apply.c b/builtin-apply.c
index eb09bfe..e04b493 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -910,23 +910,35 @@ static void check_whitespace(const char *line, int len)
* this function. That is, an addition of an empty line would
* check the '+' here. Sneaky...
*/
- if (isspace(line[len-2]))
+ if ((whitespace_rule & WS_TRAILING_SPACE) && isspace(line[len-2]))
goto error;
/*
* Make sure that there is no space followed by a tab in
* indentation.
*/
- err = "Space in indent is followed by a tab";
- for (i = 1; i < len; i++) {
- if (line[i] == '\t') {
- if (seen_space)
- goto error;
- }
- else if (line[i] == ' ')
- seen_space = 1;
- else
- break;
+ if (whitespace_rule & WS_SPACE_BEFORE_TAB) {
+ err = "Space in indent is followed by a tab";
+ for (i = 1; i < len; i++) {
+ if (line[i] == '\t') {
+ if (seen_space)
+ goto error;
+ }
+ else if (line[i] == ' ')
+ seen_space = 1;
+ else
+ break;
+ }
+ }
+
+ /*
+ * Make sure that the indentation does not contain more than
+ * 8 spaces.
+ */
+ if ((whitespace_rule & WS_INDENT_WITH_NON_TAB) &&
+ (8 < len) && !strncmp("+ ", line, 9)) {
+ err = "Indent more than 8 places with spaces";
+ goto error;
}
return;
@@ -1581,7 +1593,8 @@ static int apply_line(char *output, const char *patch, int plen)
/*
* Strip trailing whitespace
*/
- if (1 < plen && isspace(patch[plen-1])) {
+ if ((whitespace_rule & WS_TRAILING_SPACE) &&
+ (1 < plen && isspace(patch[plen-1]))) {
if (patch[plen] == '\n')
add_nl_to_tail = 1;
plen--;
@@ -1597,11 +1610,16 @@ static int apply_line(char *output, const char *patch, int plen)
char ch = patch[i];
if (ch == '\t') {
last_tab_in_indent = i;
- if (0 <= last_space_in_indent)
+ if ((whitespace_rule & WS_SPACE_BEFORE_TAB) &&
+ 0 <= last_space_in_indent)
+ need_fix_leading_space = 1;
+ } else if (ch == ' ') {
+ last_space_in_indent = i;
+ if ((whitespace_rule & WS_INDENT_WITH_NON_TAB) &&
+ last_tab_in_indent < 0 &&
+ 8 <= i)
need_fix_leading_space = 1;
}
- else if (ch == ' ')
- last_space_in_indent = i;
else
break;
}
@@ -1609,11 +1627,21 @@ static int apply_line(char *output, const char *patch, int plen)
buf = output;
if (need_fix_leading_space) {
int consecutive_spaces = 0;
+ int last = last_tab_in_indent + 1;
+
+ if (whitespace_rule & WS_INDENT_WITH_NON_TAB) {
+ /* have "last" point at one past the indent */
+ if (last_tab_in_indent < last_space_in_indent)
+ last = last_space_in_indent + 1;
+ else
+ last = last_tab_in_indent + 1;
+ }
+
/*
- * between patch[1..last_tab_in_indent] strip the
- * funny spaces, updating them to tab as needed.
+ * between patch[1..last], strip the funny spaces,
+ * updating them to tab as needed.
*/
- for (i = 1; i < last_tab_in_indent; i++, plen--) {
+ for (i = 1; i < last; i++, plen--) {
char ch = patch[i];
if (ch != ' ') {
consecutive_spaces = 0;
@@ -1626,8 +1654,10 @@ static int apply_line(char *output, const char *patch, int plen)
}
}
}
+ while (0 < consecutive_spaces--)
+ *output++ = ' ';
fixed = 1;
- i = last_tab_in_indent;
+ i = last;
}
else
i = 1;
diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
new file mode 100755
index 0000000..f53ac46
--- /dev/null
+++ b/t/t4124-apply-ws-rule.sh
@@ -0,0 +1,133 @@
+#!/bin/sh
+
+test_description='core.whitespace rules and git-apply'
+
+. ./test-lib.sh
+
+prepare_test_file () {
+
+ # A line that has character X is touched iff RULE is in effect:
+ # X RULE
+ # ! trailing-space
+ # @ space-before-tab
+ # # indent-with-non-tab
+ sed -e "s/_/ /g" -e "s/>/ /" <<-\EOF
+ An_SP in an ordinary line>and a HT.
+ >A HT.
+ _>A SP and a HT (@).
+ _>_A SP, a HT and a SP (@).
+ _______Seven SP.
+ ________Eight SP (#).
+ _______>Seven SP and a HT (@).
+ ________>Eight SP and a HT (@#).
+ _______>_Seven SP, a HT and a SP (@).
+ ________>_Eight SP, a HT and a SP (@#).
+ _______________Fifteen SP (#).
+ _______________>Fifteen SP and a HT (@#).
+ ________________Sixteen SP (#).
+ ________________>Sixteen SP and a HT (@#).
+ _____a__Five SP, a non WS, two SP.
+ A line with a (!) trailing SP_
+ A line with a (!) trailing HT>
+ EOF
+}
+
+apply_patch () {
+ >target &&
+ sed -e "s|\([ab]\)/file|\1/target|" <patch |
+ git apply "$@"
+}
+
+test_fix () {
+
+ # fix should not barf
+ apply_patch --whitespace=fix || return 1
+
+ # find touched lines
+ diff file target | sed -n -e "s/^> //p" >fixed
+
+ # the changed lines are all expeced to change
+ fixed_cnt=$(wc -l <fixed)
+ case "$1" in
+ '') expect_cnt=$fixed_cnt ;;
+ ?*) expect_cnt=$(grep "[$1]" <fixed | wc -l) ;;
+ esac
+ test $fixed_cnt -eq $expect_cnt || return 1
+
+ # and we are not missing anything
+ case "$1" in
+ '') expect_cnt=0 ;;
+ ?*) expect_cnt=$(grep "[$1]" <file | wc -l) ;;
+ esac
+ test $fixed_cnt -eq $expect_cnt || return 1
+
+ # Get the patch actually applied
+ git diff-files -p target >fixed-patch
+ test -s fixed-patch && return 0
+
+ # Make sure it is complaint-free
+ >target
+ git apply --whitespace=error-all <fixed-patch
+
+}
+
+test_expect_success setup '
+
+ >file &&
+ git add file &&
+ prepare_test_file >file &&
+ git diff-files -p >patch &&
+ >target &&
+ git add target
+
+'
+
+test_expect_success 'whitespace=nowarn, default rule' '
+
+ apply_patch --whitespace=nowarn &&
+ diff file target
+
+'
+
+test_expect_success 'whitespace=warn, default rule' '
+
+ apply_patch --whitespace=warn &&
+ diff file target
+
+'
+
+test_expect_success 'whitespace=error-all, default rule' '
+
+ apply_patch --whitespace=error-all && return 1
+ test -s target && return 1
+ : happy
+
+'
+
+test_expect_success 'whitespace=error-all, no rule' '
+
+ git config core.whitespace -trailing,-space-before,-indent &&
+ apply_patch --whitespace=error-all &&
+ diff file target
+
+'
+
+for t in - ''
+do
+ case "$t" in '') tt='!' ;; *) tt= ;; esac
+ for s in - ''
+ do
+ case "$s" in '') ts='@' ;; *) ts= ;; esac
+ for i in - ''
+ do
+ case "$i" in '') ti='#' ;; *) ti= ;; esac
+ rule=${t}trailing,${s}space,${i}indent &&
+ test_expect_success "rule=$rule" '
+ git config core.whitespace "$rule" &&
+ test_fix "$tt$ts$ti"
+ '
+ done
+ done
+done
+
+test_done
--
1.5.3.6.1991.ge56ac
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 3/2] core.whitespace: documentation updates.
2007-11-24 4:25 ` [PATCH 2/2] builtin-apply: teach whitespace_rules Junio C Hamano
@ 2007-11-24 20:09 ` Junio C Hamano
2007-11-24 20:22 ` J. Bruce Fields
0 siblings, 1 reply; 43+ messages in thread
From: Junio C Hamano @ 2007-11-24 20:09 UTC (permalink / raw)
To: git
This adds description of core.whitespace to the manual page of git-config,
and updates the stale description of whitespace handling in the manual
page of git-apply.
Also demote "strip" to a synonym status for "fix" as the value of --whitespace
option given to git-apply.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This is meant to conclude the "War on more than 8-SP indent" Bruce
started some time ago. It is now configurable, and turned off by
default, so hopefully people outside the kernel circle would not mind.
A possible addition to the repertoire of core.whitespace is to add
"cr-at-end", which would consider a line that ends with CR an error.
We redefine "trailing-space" not to complain to a line ending with
CRLF but otherwise does not have trailing whitespaces. To be
compatible with the current behaviour, cr-at-end needs to be added to
the default set of errors to be detected, but it might be an
improvement if we stopped treating 'cr-at-end' as an error by
default.
Documentation/config.txt | 18 ++++++++++++++++--
Documentation/git-apply.txt | 35 +++++++++++++++++++++--------------
builtin-apply.c | 2 +-
3 files changed, 38 insertions(+), 17 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index edf50cd..0e71137 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -293,6 +293,20 @@ core.pager::
The command that git will use to paginate output. Can be overridden
with the `GIT_PAGER` environment variable.
+core.whitespace::
+ A comma separated list of common whitespace problems to
+ notice. `git diff` will use `color.diff.whitespace` to
+ highlight them, and `git apply --whitespace=error` will
+ consider them as errors:
++
+* `trailing-space` treats trailing whitespaces at the end of the line
+ as an error (enabled by default).
+* `space-before-tab` treats a space character that appears immediately
+ before a tab character in the initial indent part of the line as an
+ error (enabled by default).
+* `indent-with-non-tab` treats a line that is indented with 8 or more
+ space characters that can be replaced with tab characters.
+
alias.*::
Command aliases for the gitlink:git[1] command wrapper - e.g.
after defining "alias.last = cat-file commit HEAD", the invocation
@@ -378,8 +392,8 @@ color.diff.<slot>::
which part of the patch to use the specified color, and is one
of `plain` (context text), `meta` (metainformation), `frag`
(hunk header), `old` (removed lines), `new` (added lines),
- `commit` (commit headers), or `whitespace` (highlighting dubious
- whitespace). The values of these variables may be specified as
+ `commit` (commit headers), or `whitespace` (highlighting
+ whitespace errors). The values of these variables may be specified as
in color.branch.<slot>.
color.pager::
diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
index c1c54bf..bae3e7b 100644
--- a/Documentation/git-apply.txt
+++ b/Documentation/git-apply.txt
@@ -13,7 +13,7 @@ SYNOPSIS
[--apply] [--no-add] [--build-fake-ancestor <file>] [-R | --reverse]
[--allow-binary-replacement | --binary] [--reject] [-z]
[-pNUM] [-CNUM] [--inaccurate-eof] [--cached]
- [--whitespace=<nowarn|warn|error|error-all|strip>]
+ [--whitespace=<nowarn|warn|fix|error|error-all>]
[--exclude=PATH] [--verbose] [<patch>...]
DESCRIPTION
@@ -135,25 +135,32 @@ discouraged.
be useful when importing patchsets, where you want to exclude certain
files or directories.
---whitespace=<option>::
- When applying a patch, detect a new or modified line
- that ends with trailing whitespaces (this includes a
- line that solely consists of whitespaces). By default,
- the command outputs warning messages and applies the
- patch.
- When gitlink:git-apply[1] is used for statistics and not applying a
- patch, it defaults to `nowarn`.
- You can use different `<option>` to control this
- behavior:
+--whitespace=<action>::
+ When applying a patch, detect a new or modified line that has
+ whitespace errors. What are considered whitespace errors is
+ controlled by `core.whitespace` configuration. By default,
+ trailing whitespaces (including lines that solely consist of
+ whitespaces) and a space character that is immediately followed
+ by a tab character inside the initial indent of the line are
+ considered whitespace errors.
++
+By default, the command outputs warning messages but applies the patch.
+When gitlink:git-apply[1] is used for statistics and not applying a
+patch, it defaults to `nowarn`.
++
+You can use different `<action>` to control this
+behavior:
+
* `nowarn` turns off the trailing whitespace warning.
* `warn` outputs warnings for a few such errors, but applies the
- patch (default).
+ patch as-is (default).
+* `fix` outputs warnings for a few such errors, and applies the
+ patch after fixing them (`strip` is a synonym --- the tool
+ used to consider only trailing whitespaces as errors, and the
+ fix involved 'stripping' them, but modern gits do more).
* `error` outputs warnings for a few such errors, and refuses
to apply the patch.
* `error-all` is similar to `error` but shows all errors.
-* `strip` outputs warnings for a few such errors, strips out the
- trailing whitespaces and applies the patch.
--inaccurate-eof::
Under certain circumstances, some versions of diff do not correctly
diff --git a/builtin-apply.c b/builtin-apply.c
index e04b493..57efcd5 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -45,7 +45,7 @@ static const char *fake_ancestor;
static int line_termination = '\n';
static unsigned long p_context = ULONG_MAX;
static const char apply_usage[] =
-"git-apply [--stat] [--numstat] [--summary] [--check] [--index] [--cached] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [--reverse] [--reject] [--verbose] [-z] [-pNUM] [-CNUM] [--whitespace=<nowarn|warn|error|error-all|strip>] <patch>...";
+"git-apply [--stat] [--numstat] [--summary] [--check] [--index] [--cached] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [--reverse] [--reject] [--verbose] [-z] [-pNUM] [-CNUM] [--whitespace=<nowarn|warn|fix|error|error-all>] <patch>...";
static enum ws_error_action {
nowarn_ws_error,
--
1.5.3.6.1991.ge56ac
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 3/2] core.whitespace: documentation updates.
2007-11-24 20:09 ` [PATCH 3/2] core.whitespace: documentation updates Junio C Hamano
@ 2007-11-24 20:22 ` J. Bruce Fields
2007-11-24 21:42 ` Junio C Hamano
0 siblings, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-11-24 20:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Sat, Nov 24, 2007 at 12:09:13PM -0800, Junio C Hamano wrote:
> This adds description of core.whitespace to the manual page of git-config,
> and updates the stale description of whitespace handling in the manual
> page of git-apply.
>
> Also demote "strip" to a synonym status for "fix" as the value of --whitespace
> option given to git-apply.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
> * This is meant to conclude the "War on more than 8-SP indent" Bruce
> started some time ago. It is now configurable, and turned off by
> default, so hopefully people outside the kernel circle would not mind.
>
> A possible addition to the repertoire of core.whitespace is to add
> "cr-at-end", which would consider a line that ends with CR an error.
> We redefine "trailing-space" not to complain to a line ending with
> CRLF but otherwise does not have trailing whitespaces. To be
> compatible with the current behaviour, cr-at-end needs to be added to
> the default set of errors to be detected, but it might be an
> improvement if we stopped treating 'cr-at-end' as an error by
> default.
I'd still prefer this to be a gitattributes thing rather than a config
variable[1]. Last time I raised this you said something to the effect
of "I think you're right, let's fix that before it's merged." Would you
like me to work on that?
--b.
[1] A rehash of the argument: config variables vary depending on the
system (/etc/gitconfig), the user (~/.config), or the particular
repository ($GIT_DIR/config). But you don't want the whitespace policy
to vary that way: all users and repositories should see the same
whitespace policy for a given project. It's entirely possible, however,
that you might like the policy to vary depending on the file (Makefile
vs. main.py?). And of course getting the policy versioned and
distributed to other repositories automatically is nice too.
>
> Documentation/config.txt | 18 ++++++++++++++++--
> Documentation/git-apply.txt | 35 +++++++++++++++++++++--------------
> builtin-apply.c | 2 +-
> 3 files changed, 38 insertions(+), 17 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index edf50cd..0e71137 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -293,6 +293,20 @@ core.pager::
> The command that git will use to paginate output. Can be overridden
> with the `GIT_PAGER` environment variable.
>
> +core.whitespace::
> + A comma separated list of common whitespace problems to
> + notice. `git diff` will use `color.diff.whitespace` to
> + highlight them, and `git apply --whitespace=error` will
> + consider them as errors:
> ++
> +* `trailing-space` treats trailing whitespaces at the end of the line
> + as an error (enabled by default).
> +* `space-before-tab` treats a space character that appears immediately
> + before a tab character in the initial indent part of the line as an
> + error (enabled by default).
> +* `indent-with-non-tab` treats a line that is indented with 8 or more
> + space characters that can be replaced with tab characters.
> +
> alias.*::
> Command aliases for the gitlink:git[1] command wrapper - e.g.
> after defining "alias.last = cat-file commit HEAD", the invocation
> @@ -378,8 +392,8 @@ color.diff.<slot>::
> which part of the patch to use the specified color, and is one
> of `plain` (context text), `meta` (metainformation), `frag`
> (hunk header), `old` (removed lines), `new` (added lines),
> - `commit` (commit headers), or `whitespace` (highlighting dubious
> - whitespace). The values of these variables may be specified as
> + `commit` (commit headers), or `whitespace` (highlighting
> + whitespace errors). The values of these variables may be specified as
> in color.branch.<slot>.
>
> color.pager::
> diff --git a/Documentation/git-apply.txt b/Documentation/git-apply.txt
> index c1c54bf..bae3e7b 100644
> --- a/Documentation/git-apply.txt
> +++ b/Documentation/git-apply.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
> [--apply] [--no-add] [--build-fake-ancestor <file>] [-R | --reverse]
> [--allow-binary-replacement | --binary] [--reject] [-z]
> [-pNUM] [-CNUM] [--inaccurate-eof] [--cached]
> - [--whitespace=<nowarn|warn|error|error-all|strip>]
> + [--whitespace=<nowarn|warn|fix|error|error-all>]
> [--exclude=PATH] [--verbose] [<patch>...]
>
> DESCRIPTION
> @@ -135,25 +135,32 @@ discouraged.
> be useful when importing patchsets, where you want to exclude certain
> files or directories.
>
> ---whitespace=<option>::
> - When applying a patch, detect a new or modified line
> - that ends with trailing whitespaces (this includes a
> - line that solely consists of whitespaces). By default,
> - the command outputs warning messages and applies the
> - patch.
> - When gitlink:git-apply[1] is used for statistics and not applying a
> - patch, it defaults to `nowarn`.
> - You can use different `<option>` to control this
> - behavior:
> +--whitespace=<action>::
> + When applying a patch, detect a new or modified line that has
> + whitespace errors. What are considered whitespace errors is
> + controlled by `core.whitespace` configuration. By default,
> + trailing whitespaces (including lines that solely consist of
> + whitespaces) and a space character that is immediately followed
> + by a tab character inside the initial indent of the line are
> + considered whitespace errors.
> ++
> +By default, the command outputs warning messages but applies the patch.
> +When gitlink:git-apply[1] is used for statistics and not applying a
> +patch, it defaults to `nowarn`.
> ++
> +You can use different `<action>` to control this
> +behavior:
> +
> * `nowarn` turns off the trailing whitespace warning.
> * `warn` outputs warnings for a few such errors, but applies the
> - patch (default).
> + patch as-is (default).
> +* `fix` outputs warnings for a few such errors, and applies the
> + patch after fixing them (`strip` is a synonym --- the tool
> + used to consider only trailing whitespaces as errors, and the
> + fix involved 'stripping' them, but modern gits do more).
> * `error` outputs warnings for a few such errors, and refuses
> to apply the patch.
> * `error-all` is similar to `error` but shows all errors.
> -* `strip` outputs warnings for a few such errors, strips out the
> - trailing whitespaces and applies the patch.
>
> --inaccurate-eof::
> Under certain circumstances, some versions of diff do not correctly
> diff --git a/builtin-apply.c b/builtin-apply.c
> index e04b493..57efcd5 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -45,7 +45,7 @@ static const char *fake_ancestor;
> static int line_termination = '\n';
> static unsigned long p_context = ULONG_MAX;
> static const char apply_usage[] =
> -"git-apply [--stat] [--numstat] [--summary] [--check] [--index] [--cached] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [--reverse] [--reject] [--verbose] [-z] [-pNUM] [-CNUM] [--whitespace=<nowarn|warn|error|error-all|strip>] <patch>...";
> +"git-apply [--stat] [--numstat] [--summary] [--check] [--index] [--cached] [--apply] [--no-add] [--index-info] [--allow-binary-replacement] [--reverse] [--reject] [--verbose] [-z] [-pNUM] [-CNUM] [--whitespace=<nowarn|warn|fix|error|error-all>] <patch>...";
>
> static enum ws_error_action {
> nowarn_ws_error,
> --
> 1.5.3.6.1991.ge56ac
>
> -
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/2] core.whitespace: documentation updates.
2007-11-24 20:22 ` J. Bruce Fields
@ 2007-11-24 21:42 ` Junio C Hamano
2007-11-25 21:58 ` J. Bruce Fields
0 siblings, 1 reply; 43+ messages in thread
From: Junio C Hamano @ 2007-11-24 21:42 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: git
"J. Bruce Fields" <bfields@fieldses.org> writes:
> I'd still prefer this to be a gitattributes thing rather than a config
> variable[1]. Last time I raised this you said something to the effect
> of "I think you're right, let's fix that before it's merged." Would you
> like me to work on that?
Ah, I forgot about that, and you are right. Go wild.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/2] core.whitespace: documentation updates.
2007-11-24 21:42 ` Junio C Hamano
@ 2007-11-25 21:58 ` J. Bruce Fields
2007-12-06 9:04 ` Junio C Hamano
0 siblings, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-11-25 21:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Sat, Nov 24, 2007 at 01:42:46PM -0800, Junio C Hamano wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
>
> > I'd still prefer this to be a gitattributes thing rather than a config
> > variable[1]. Last time I raised this you said something to the effect
> > of "I think you're right, let's fix that before it's merged." Would you
> > like me to work on that?
>
> Ah, I forgot about that, and you are right. Go wild.
OK, I will go wild, but... very slowly.
--b.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/2] core.whitespace: documentation updates.
2007-11-25 21:58 ` J. Bruce Fields
@ 2007-12-06 9:04 ` Junio C Hamano
2007-12-06 19:18 ` J. Bruce Fields
2007-12-16 3:48 ` J. Bruce Fields
0 siblings, 2 replies; 43+ messages in thread
From: Junio C Hamano @ 2007-12-06 9:04 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: git
"J. Bruce Fields" <bfields@fieldses.org> writes:
> On Sat, Nov 24, 2007 at 01:42:46PM -0800, Junio C Hamano wrote:
>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>>
>> > I'd still prefer this to be a gitattributes thing rather than a config
>> > variable[1]. Last time I raised this you said something to the effect
>> > of "I think you're right, let's fix that before it's merged." Would you
>> > like me to work on that?
>>
>> Ah, I forgot about that, and you are right. Go wild.
>
> OK, I will go wild, but... very slowly.
How wild are you these days ;-)? I know December is a busy time for
everybody, and I ended up doing this myself while I was writing up the
API documentation for gitattributes.
-- >8 --
[PATCH] Use gitattributes to define per-path whitespace rule
The `core.whitespace` configuration variable allows you to define what
`diff` and `apply` should consider whitespace errors for all paths in
the project (See gitlink:git-config[1]). This attribute gives you finer
control per path.
For example, if you have these in the .gitattributes:
frotz whitespace
nitfol -whitespace
xyzzy whitespace=-trailing
all types of whitespace problems known to git are noticed in path 'frotz'
(i.e. diff shows them in diff.whitespace color, and apply warns about
them), no whitespace problem is noticed in path 'nitfol', and the
default types of whitespace problems except "trailing whitespace" are
noticed for path 'xyzzy'. A project with mixed Python and C might want
to have:
*.c whitespace
*.py whitespace=-indent-with-non-tab
in its toplevel .gitattributes file.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/gitattributes.txt | 31 +++++++++++++
Makefile | 2 +-
builtin-apply.c | 36 +++++++++------
cache.h | 4 +-
config.c | 50 +--------------------
diff.c | 19 +++++---
environment.c | 2 +-
t/t4019-diff-wserror.sh | 47 +++++++++++++++++++
t/t4124-apply-ws-rule.sh | 20 ++++++++-
ws.c | 96 +++++++++++++++++++++++++++++++++++++++
10 files changed, 233 insertions(+), 74 deletions(-)
create mode 100644 ws.c
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 20cf8ff..c4bcbb9 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -360,6 +360,37 @@ When left unspecified, the driver itself is used for both
internal merge and the final merge.
+Checking whitespace errors
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+`whitespace`
+^^^^^^^^^^^^
+
+The `core.whitespace` configuration variable allows you to define what
+`diff` and `apply` should consider whitespace errors for all paths in
+the project (See gitlink:git-config[1]). This attribute gives you finer
+control per path.
+
+Set::
+
+ Notice all types of potential whitespace errors known to git.
+
+Unset::
+
+ Do not notice anything as error.
+
+Unspecified::
+
+ Use the value of `core.whitespace` configuration variable to
+ decide what to notice as error.
+
+String::
+
+ Specify a comma separate list of common whitespace problems to
+ notice in the same format as `core.whitespace` configuration
+ variable.
+
+
EXAMPLE
-------
diff --git a/Makefile b/Makefile
index 042f79e..ac6b079 100644
--- a/Makefile
+++ b/Makefile
@@ -312,7 +312,7 @@ LIB_OBJS = \
alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
- transport.o bundle.o walker.o parse-options.o
+ transport.o bundle.o walker.o parse-options.o ws.o
BUILTIN_OBJS = \
builtin-add.o \
diff --git a/builtin-apply.c b/builtin-apply.c
index 57efcd5..ee3ef60 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -144,6 +144,7 @@ struct patch {
unsigned int old_mode, new_mode;
int is_new, is_delete; /* -1 = unknown, 0 = false, 1 = true */
int rejected;
+ unsigned ws_rule;
unsigned long deflate_origlen;
int lines_added, lines_deleted;
int score;
@@ -898,7 +899,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
return -1;
}
-static void check_whitespace(const char *line, int len)
+static void check_whitespace(const char *line, int len, unsigned ws_rule)
{
const char *err = "Adds trailing whitespace";
int seen_space = 0;
@@ -910,14 +911,14 @@ static void check_whitespace(const char *line, int len)
* this function. That is, an addition of an empty line would
* check the '+' here. Sneaky...
*/
- if ((whitespace_rule & WS_TRAILING_SPACE) && isspace(line[len-2]))
+ if ((ws_rule & WS_TRAILING_SPACE) && isspace(line[len-2]))
goto error;
/*
* Make sure that there is no space followed by a tab in
* indentation.
*/
- if (whitespace_rule & WS_SPACE_BEFORE_TAB) {
+ if (ws_rule & WS_SPACE_BEFORE_TAB) {
err = "Space in indent is followed by a tab";
for (i = 1; i < len; i++) {
if (line[i] == '\t') {
@@ -935,7 +936,7 @@ static void check_whitespace(const char *line, int len)
* Make sure that the indentation does not contain more than
* 8 spaces.
*/
- if ((whitespace_rule & WS_INDENT_WITH_NON_TAB) &&
+ if ((ws_rule & WS_INDENT_WITH_NON_TAB) &&
(8 < len) && !strncmp("+ ", line, 9)) {
err = "Indent more than 8 places with spaces";
goto error;
@@ -1001,7 +1002,7 @@ static int parse_fragment(char *line, unsigned long size,
case '-':
if (apply_in_reverse &&
ws_error_action != nowarn_ws_error)
- check_whitespace(line, len);
+ check_whitespace(line, len, patch->ws_rule);
deleted++;
oldlines--;
trailing = 0;
@@ -1009,7 +1010,7 @@ static int parse_fragment(char *line, unsigned long size,
case '+':
if (!apply_in_reverse &&
ws_error_action != nowarn_ws_error)
- check_whitespace(line, len);
+ check_whitespace(line, len, patch->ws_rule);
added++;
newlines--;
trailing = 0;
@@ -1318,6 +1319,10 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)
if (offset < 0)
return offset;
+ patch->ws_rule = whitespace_rule(patch->new_name
+ ? patch->new_name
+ : patch->old_name);
+
patchsize = parse_single_patch(buffer + offset + hdrsize,
size - offset - hdrsize, patch);
@@ -1568,7 +1573,8 @@ static void remove_last_line(const char **rbuf, int *rsize)
*rsize = offset + 1;
}
-static int apply_line(char *output, const char *patch, int plen)
+static int apply_line(char *output, const char *patch, int plen,
+ unsigned ws_rule)
{
/*
* plen is number of bytes to be copied from patch,
@@ -1593,7 +1599,7 @@ static int apply_line(char *output, const char *patch, int plen)
/*
* Strip trailing whitespace
*/
- if ((whitespace_rule & WS_TRAILING_SPACE) &&
+ if ((ws_rule & WS_TRAILING_SPACE) &&
(1 < plen && isspace(patch[plen-1]))) {
if (patch[plen] == '\n')
add_nl_to_tail = 1;
@@ -1610,12 +1616,12 @@ static int apply_line(char *output, const char *patch, int plen)
char ch = patch[i];
if (ch == '\t') {
last_tab_in_indent = i;
- if ((whitespace_rule & WS_SPACE_BEFORE_TAB) &&
+ if ((ws_rule & WS_SPACE_BEFORE_TAB) &&
0 <= last_space_in_indent)
need_fix_leading_space = 1;
} else if (ch == ' ') {
last_space_in_indent = i;
- if ((whitespace_rule & WS_INDENT_WITH_NON_TAB) &&
+ if ((ws_rule & WS_INDENT_WITH_NON_TAB) &&
last_tab_in_indent < 0 &&
8 <= i)
need_fix_leading_space = 1;
@@ -1629,7 +1635,7 @@ static int apply_line(char *output, const char *patch, int plen)
int consecutive_spaces = 0;
int last = last_tab_in_indent + 1;
- if (whitespace_rule & WS_INDENT_WITH_NON_TAB) {
+ if (ws_rule & WS_INDENT_WITH_NON_TAB) {
/* have "last" point at one past the indent */
if (last_tab_in_indent < last_space_in_indent)
last = last_space_in_indent + 1;
@@ -1671,7 +1677,7 @@ static int apply_line(char *output, const char *patch, int plen)
}
static int apply_one_fragment(struct strbuf *buf, struct fragment *frag,
- int inaccurate_eof)
+ int inaccurate_eof, unsigned ws_rule)
{
int match_beginning, match_end;
const char *patch = frag->patch;
@@ -1730,7 +1736,7 @@ static int apply_one_fragment(struct strbuf *buf, struct fragment *frag,
case '+':
if (first != '+' || !no_add) {
int added = apply_line(new + newsize, patch,
- plen);
+ plen, ws_rule);
newsize += added;
if (first == '+' &&
added == 1 && new[newsize-1] == '\n')
@@ -1953,12 +1959,14 @@ static int apply_fragments(struct strbuf *buf, struct patch *patch)
{
struct fragment *frag = patch->fragments;
const char *name = patch->old_name ? patch->old_name : patch->new_name;
+ unsigned ws_rule = patch->ws_rule;
+ unsigned inaccurate_eof = patch->inaccurate_eof;
if (patch->is_binary)
return apply_binary(buf, patch);
while (frag) {
- if (apply_one_fragment(buf, frag, patch->inaccurate_eof)) {
+ if (apply_one_fragment(buf, frag, inaccurate_eof, ws_rule)) {
error("patch failed: %s:%ld", name, frag->oldpos);
if (!apply_with_reject)
return -1;
diff --git a/cache.h b/cache.h
index 3f42827..9cc6268 100644
--- a/cache.h
+++ b/cache.h
@@ -610,6 +610,8 @@ void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, i
#define WS_SPACE_BEFORE_TAB 02
#define WS_INDENT_WITH_NON_TAB 04
#define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB)
-extern unsigned whitespace_rule;
+extern unsigned whitespace_rule_cfg;
+extern unsigned whitespace_rule(const char *);
+extern unsigned parse_whitespace_rule(const char *);
#endif /* CACHE_H */
diff --git a/config.c b/config.c
index d5b9766..2500e0d 100644
--- a/config.c
+++ b/config.c
@@ -246,54 +246,6 @@ static unsigned long get_unit_factor(const char *end)
die("unknown unit: '%s'", end);
}
-static struct whitespace_rule {
- const char *rule_name;
- unsigned rule_bits;
-} whitespace_rule_names[] = {
- { "trailing-space", WS_TRAILING_SPACE },
- { "space-before-tab", WS_SPACE_BEFORE_TAB },
- { "indent-with-non-tab", WS_INDENT_WITH_NON_TAB },
-};
-
-static unsigned parse_whitespace_rule(const char *string)
-{
- unsigned rule = WS_DEFAULT_RULE;
-
- while (string) {
- int i;
- size_t len;
- const char *ep;
- int negated = 0;
-
- string = string + strspn(string, ", \t\n\r");
- ep = strchr(string, ',');
- if (!ep)
- len = strlen(string);
- else
- len = ep - string;
-
- if (*string == '-') {
- negated = 1;
- string++;
- len--;
- }
- if (!len)
- break;
- for (i = 0; i < ARRAY_SIZE(whitespace_rule_names); i++) {
- if (strncmp(whitespace_rule_names[i].rule_name,
- string, len))
- continue;
- if (negated)
- rule &= ~whitespace_rule_names[i].rule_bits;
- else
- rule |= whitespace_rule_names[i].rule_bits;
- break;
- }
- string = ep;
- }
- return rule;
-}
-
int git_parse_long(const char *value, long *ret)
{
if (value && *value) {
@@ -480,7 +432,7 @@ int git_default_config(const char *var, const char *value)
}
if (!strcmp(var, "core.whitespace")) {
- whitespace_rule = parse_whitespace_rule(value);
+ whitespace_rule_cfg = parse_whitespace_rule(value);
return 0;
}
diff --git a/diff.c b/diff.c
index 6bb902f..c3a1942 100644
--- a/diff.c
+++ b/diff.c
@@ -454,6 +454,7 @@ static void diff_words_show(struct diff_words_data *diff_words)
struct emit_callback {
struct xdiff_emit_state xm;
int nparents, color_diff;
+ unsigned ws_rule;
const char **label_path;
struct diff_words_data *diff_words;
int *found_changesp;
@@ -493,8 +494,8 @@ static void emit_line(const char *set, const char *reset, const char *line, int
}
static void emit_line_with_ws(int nparents,
- const char *set, const char *reset, const char *ws,
- const char *line, int len)
+ const char *set, const char *reset, const char *ws,
+ const char *line, int len, unsigned ws_rule)
{
int col0 = nparents;
int last_tab_in_indent = -1;
@@ -511,7 +512,7 @@ static void emit_line_with_ws(int nparents,
for (i = col0; i < len; i++) {
if (line[i] == '\t') {
last_tab_in_indent = i;
- if ((whitespace_rule & WS_SPACE_BEFORE_TAB) &&
+ if ((ws_rule & WS_SPACE_BEFORE_TAB) &&
0 <= last_space_in_indent)
need_highlight_leading_space = 1;
}
@@ -520,7 +521,7 @@ static void emit_line_with_ws(int nparents,
else
break;
}
- if ((whitespace_rule & WS_INDENT_WITH_NON_TAB) &&
+ if ((ws_rule & WS_INDENT_WITH_NON_TAB) &&
0 <= last_space_in_indent &&
last_tab_in_indent < 0 &&
8 <= (i - col0)) {
@@ -551,7 +552,7 @@ static void emit_line_with_ws(int nparents,
tail = len - 1;
if (line[tail] == '\n' && i < tail)
tail--;
- if (whitespace_rule & WS_TRAILING_SPACE) {
+ if (ws_rule & WS_TRAILING_SPACE) {
while (i < tail) {
if (!isspace(line[tail]))
break;
@@ -578,7 +579,7 @@ static void emit_add_line(const char *reset, struct emit_callback *ecbdata, cons
emit_line(set, reset, line, len);
else
emit_line_with_ws(ecbdata->nparents, set, reset, ws,
- line, len);
+ line, len, ecbdata->ws_rule);
}
static void fn_out_consume(void *priv, char *line, unsigned long len)
@@ -994,6 +995,7 @@ struct checkdiff_t {
struct xdiff_emit_state xm;
const char *filename;
int lineno, color_diff;
+ unsigned ws_rule;
};
static void checkdiff_consume(void *priv, char *line, unsigned long len)
@@ -1029,7 +1031,8 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
if (white_space_at_end)
printf("white space at end");
printf(":%s ", reset);
- emit_line_with_ws(1, set, reset, ws, line, len);
+ emit_line_with_ws(1, set, reset, ws, line, len,
+ data->ws_rule);
}
data->lineno++;
@@ -1330,6 +1333,7 @@ static void builtin_diff(const char *name_a,
ecbdata.label_path = lbl;
ecbdata.color_diff = o->color_diff;
ecbdata.found_changesp = &o->found_changes;
+ ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
xecfg.ctxlen = o->context;
xecfg.flags = XDL_EMIT_FUNCNAMES;
@@ -1423,6 +1427,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
data.filename = name_b ? name_b : name_a;
data.lineno = 0;
data.color_diff = o->color_diff;
+ data.ws_rule = whitespace_rule(data.filename);
if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
die("unable to read files to diff");
diff --git a/environment.c b/environment.c
index 624dd96..2fbbc8e 100644
--- a/environment.c
+++ b/environment.c
@@ -35,7 +35,7 @@ int pager_in_use;
int pager_use_color = 1;
char *editor_program;
int auto_crlf = 0; /* 1: both ways, -1: only when adding git objects */
-unsigned whitespace_rule = WS_DEFAULT_RULE;
+unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
/* This is set by setup_git_dir_gently() and/or git_default_config() */
char *git_work_tree_cfg;
diff --git a/t/t4019-diff-wserror.sh b/t/t4019-diff-wserror.sh
index dbc895b..67e080b 100755
--- a/t/t4019-diff-wserror.sh
+++ b/t/t4019-diff-wserror.sh
@@ -45,8 +45,24 @@ test_expect_success 'without -trail' '
'
+test_expect_success 'without -trail (attribute)' '
+
+ git config --unset core.whitespace
+ echo "F whitespace=-trail" >.gitattributes
+ git diff --color >output
+ grep "$blue_grep" output >error
+ grep -v "$blue_grep" output >normal
+
+ grep Eight normal >/dev/null &&
+ grep HT error >/dev/null &&
+ grep With normal >/dev/null &&
+ grep No normal >/dev/null
+
+'
+
test_expect_success 'without -space' '
+ rm -f .gitattributes
git config core.whitespace -space
git diff --color >output
grep "$blue_grep" output >error
@@ -59,8 +75,24 @@ test_expect_success 'without -space' '
'
+test_expect_success 'without -space (attribute)' '
+
+ git config --unset core.whitespace
+ echo "F whitespace=-space" >.gitattributes
+ git diff --color >output
+ grep "$blue_grep" output >error
+ grep -v "$blue_grep" output >normal
+
+ grep Eight normal >/dev/null &&
+ grep HT normal >/dev/null &&
+ grep With error >/dev/null &&
+ grep No normal >/dev/null
+
+'
+
test_expect_success 'with indent-non-tab only' '
+ rm -f .gitattributes
git config core.whitespace indent,-trailing,-space
git diff --color >output
grep "$blue_grep" output >error
@@ -73,4 +105,19 @@ test_expect_success 'with indent-non-tab only' '
'
+test_expect_success 'with indent-non-tab only (attribute)' '
+
+ git config --unset core.whitespace
+ echo "F whitespace=indent,-trailing,-space" >.gitattributes
+ git diff --color >output
+ grep "$blue_grep" output >error
+ grep -v "$blue_grep" output >normal
+
+ grep Eight error >/dev/null &&
+ grep HT normal >/dev/null &&
+ grep With normal >/dev/null &&
+ grep No normal >/dev/null
+
+'
+
test_done
diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
index f53ac46..85f3da2 100755
--- a/t/t4124-apply-ws-rule.sh
+++ b/t/t4124-apply-ws-rule.sh
@@ -112,6 +112,15 @@ test_expect_success 'whitespace=error-all, no rule' '
'
+test_expect_success 'whitespace=error-all, no rule (attribute)' '
+
+ git config --unset core.whitespace &&
+ echo "target -whitespace" >.gitattributes &&
+ apply_patch --whitespace=error-all &&
+ diff file target
+
+'
+
for t in - ''
do
case "$t" in '') tt='!' ;; *) tt= ;; esac
@@ -121,11 +130,20 @@ do
for i in - ''
do
case "$i" in '') ti='#' ;; *) ti= ;; esac
- rule=${t}trailing,${s}space,${i}indent &&
+ rule=${t}trailing,${s}space,${i}indent
+
+ rm -f .gitattributes
test_expect_success "rule=$rule" '
git config core.whitespace "$rule" &&
test_fix "$tt$ts$ti"
'
+
+ test_expect_success "rule=$rule (attributes)" '
+ git config --unset core.whitespace &&
+ echo "target whitespace=$rule" >.gitattributes &&
+ test_fix "$tt$ts$ti"
+ '
+
done
done
done
diff --git a/ws.c b/ws.c
new file mode 100644
index 0000000..52c10ca
--- /dev/null
+++ b/ws.c
@@ -0,0 +1,96 @@
+/*
+ * Whitespace rules
+ *
+ * Copyright (c) 2007 Junio C Hamano
+ */
+
+#include "cache.h"
+#include "attr.h"
+
+static struct whitespace_rule {
+ const char *rule_name;
+ unsigned rule_bits;
+} whitespace_rule_names[] = {
+ { "trailing-space", WS_TRAILING_SPACE },
+ { "space-before-tab", WS_SPACE_BEFORE_TAB },
+ { "indent-with-non-tab", WS_INDENT_WITH_NON_TAB },
+};
+
+unsigned parse_whitespace_rule(const char *string)
+{
+ unsigned rule = WS_DEFAULT_RULE;
+
+ while (string) {
+ int i;
+ size_t len;
+ const char *ep;
+ int negated = 0;
+
+ string = string + strspn(string, ", \t\n\r");
+ ep = strchr(string, ',');
+ if (!ep)
+ len = strlen(string);
+ else
+ len = ep - string;
+
+ if (*string == '-') {
+ negated = 1;
+ string++;
+ len--;
+ }
+ if (!len)
+ break;
+ for (i = 0; i < ARRAY_SIZE(whitespace_rule_names); i++) {
+ if (strncmp(whitespace_rule_names[i].rule_name,
+ string, len))
+ continue;
+ if (negated)
+ rule &= ~whitespace_rule_names[i].rule_bits;
+ else
+ rule |= whitespace_rule_names[i].rule_bits;
+ break;
+ }
+ string = ep;
+ }
+ return rule;
+}
+
+static void setup_whitespace_attr_check(struct git_attr_check *check)
+{
+ static struct git_attr *attr_whitespace;
+
+ if (!attr_whitespace)
+ attr_whitespace = git_attr("whitespace", 10);
+ check[0].attr = attr_whitespace;
+}
+
+unsigned whitespace_rule(const char *pathname)
+{
+ struct git_attr_check attr_whitespace_rule;
+
+ setup_whitespace_attr_check(&attr_whitespace_rule);
+ if (!git_checkattr(pathname, 1, &attr_whitespace_rule)) {
+ const char *value;
+
+ value = attr_whitespace_rule.value;
+ if (ATTR_TRUE(value)) {
+ /* true (whitespace) */
+ unsigned all_rule = 0;
+ int i;
+ for (i = 0; i < ARRAY_SIZE(whitespace_rule_names); i++)
+ all_rule |= whitespace_rule_names[i].rule_bits;
+ return all_rule;
+ } else if (ATTR_FALSE(value)) {
+ /* false (-whitespace) */
+ return 0;
+ } else if (ATTR_UNSET(value)) {
+ /* reset to default (!whitespace) */
+ return whitespace_rule_cfg;
+ } else {
+ /* string */
+ return parse_whitespace_rule(value);
+ }
+ } else {
+ return whitespace_rule_cfg;
+ }
+}
--
1.5.3.7-2155-g4c25
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 3/2] core.whitespace: documentation updates.
2007-12-06 9:04 ` Junio C Hamano
@ 2007-12-06 19:18 ` J. Bruce Fields
2007-12-16 3:48 ` J. Bruce Fields
1 sibling, 0 replies; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-06 19:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, Dec 06, 2007 at 01:04:56AM -0800, Junio C Hamano wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
>
> > On Sat, Nov 24, 2007 at 01:42:46PM -0800, Junio C Hamano wrote:
> >> "J. Bruce Fields" <bfields@fieldses.org> writes:
> >>
> >> > I'd still prefer this to be a gitattributes thing rather than a config
> >> > variable[1]. Last time I raised this you said something to the effect
> >> > of "I think you're right, let's fix that before it's merged." Would you
> >> > like me to work on that?
> >>
> >> Ah, I forgot about that, and you are right. Go wild.
> >
> > OK, I will go wild, but... very slowly.
>
> How wild are you these days ;-)? I know December is a busy time for
> everybody, and I ended up doing this myself while I was writing up the
> API documentation for gitattributes.
Wow, thanks! Yes, I haven't done a thing on this.
> -- >8 --
> [PATCH] Use gitattributes to define per-path whitespace rule
>
> The `core.whitespace` configuration variable allows you to define what
> `diff` and `apply` should consider whitespace errors for all paths in
> the project (See gitlink:git-config[1]). This attribute gives you finer
> control per path.
That looks like what I'd hoped for.
I'll set aside some time this weekend to play around with it. (Is there
any other piece left that needs to be done?)
--b.
>
> For example, if you have these in the .gitattributes:
>
> frotz whitespace
> nitfol -whitespace
> xyzzy whitespace=-trailing
>
> all types of whitespace problems known to git are noticed in path 'frotz'
> (i.e. diff shows them in diff.whitespace color, and apply warns about
> them), no whitespace problem is noticed in path 'nitfol', and the
> default types of whitespace problems except "trailing whitespace" are
> noticed for path 'xyzzy'. A project with mixed Python and C might want
> to have:
>
> *.c whitespace
> *.py whitespace=-indent-with-non-tab
>
> in its toplevel .gitattributes file.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> Documentation/gitattributes.txt | 31 +++++++++++++
> Makefile | 2 +-
> builtin-apply.c | 36 +++++++++------
> cache.h | 4 +-
> config.c | 50 +--------------------
> diff.c | 19 +++++---
> environment.c | 2 +-
> t/t4019-diff-wserror.sh | 47 +++++++++++++++++++
> t/t4124-apply-ws-rule.sh | 20 ++++++++-
> ws.c | 96 +++++++++++++++++++++++++++++++++++++++
> 10 files changed, 233 insertions(+), 74 deletions(-)
> create mode 100644 ws.c
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 20cf8ff..c4bcbb9 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -360,6 +360,37 @@ When left unspecified, the driver itself is used for both
> internal merge and the final merge.
>
>
> +Checking whitespace errors
> +~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +`whitespace`
> +^^^^^^^^^^^^
> +
> +The `core.whitespace` configuration variable allows you to define what
> +`diff` and `apply` should consider whitespace errors for all paths in
> +the project (See gitlink:git-config[1]). This attribute gives you finer
> +control per path.
> +
> +Set::
> +
> + Notice all types of potential whitespace errors known to git.
> +
> +Unset::
> +
> + Do not notice anything as error.
> +
> +Unspecified::
> +
> + Use the value of `core.whitespace` configuration variable to
> + decide what to notice as error.
> +
> +String::
> +
> + Specify a comma separate list of common whitespace problems to
> + notice in the same format as `core.whitespace` configuration
> + variable.
> +
> +
> EXAMPLE
> -------
>
> diff --git a/Makefile b/Makefile
> index 042f79e..ac6b079 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -312,7 +312,7 @@ LIB_OBJS = \
> alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
> color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \
> convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \
> - transport.o bundle.o walker.o parse-options.o
> + transport.o bundle.o walker.o parse-options.o ws.o
>
> BUILTIN_OBJS = \
> builtin-add.o \
> diff --git a/builtin-apply.c b/builtin-apply.c
> index 57efcd5..ee3ef60 100644
> --- a/builtin-apply.c
> +++ b/builtin-apply.c
> @@ -144,6 +144,7 @@ struct patch {
> unsigned int old_mode, new_mode;
> int is_new, is_delete; /* -1 = unknown, 0 = false, 1 = true */
> int rejected;
> + unsigned ws_rule;
> unsigned long deflate_origlen;
> int lines_added, lines_deleted;
> int score;
> @@ -898,7 +899,7 @@ static int find_header(char *line, unsigned long size, int *hdrsize, struct patc
> return -1;
> }
>
> -static void check_whitespace(const char *line, int len)
> +static void check_whitespace(const char *line, int len, unsigned ws_rule)
> {
> const char *err = "Adds trailing whitespace";
> int seen_space = 0;
> @@ -910,14 +911,14 @@ static void check_whitespace(const char *line, int len)
> * this function. That is, an addition of an empty line would
> * check the '+' here. Sneaky...
> */
> - if ((whitespace_rule & WS_TRAILING_SPACE) && isspace(line[len-2]))
> + if ((ws_rule & WS_TRAILING_SPACE) && isspace(line[len-2]))
> goto error;
>
> /*
> * Make sure that there is no space followed by a tab in
> * indentation.
> */
> - if (whitespace_rule & WS_SPACE_BEFORE_TAB) {
> + if (ws_rule & WS_SPACE_BEFORE_TAB) {
> err = "Space in indent is followed by a tab";
> for (i = 1; i < len; i++) {
> if (line[i] == '\t') {
> @@ -935,7 +936,7 @@ static void check_whitespace(const char *line, int len)
> * Make sure that the indentation does not contain more than
> * 8 spaces.
> */
> - if ((whitespace_rule & WS_INDENT_WITH_NON_TAB) &&
> + if ((ws_rule & WS_INDENT_WITH_NON_TAB) &&
> (8 < len) && !strncmp("+ ", line, 9)) {
> err = "Indent more than 8 places with spaces";
> goto error;
> @@ -1001,7 +1002,7 @@ static int parse_fragment(char *line, unsigned long size,
> case '-':
> if (apply_in_reverse &&
> ws_error_action != nowarn_ws_error)
> - check_whitespace(line, len);
> + check_whitespace(line, len, patch->ws_rule);
> deleted++;
> oldlines--;
> trailing = 0;
> @@ -1009,7 +1010,7 @@ static int parse_fragment(char *line, unsigned long size,
> case '+':
> if (!apply_in_reverse &&
> ws_error_action != nowarn_ws_error)
> - check_whitespace(line, len);
> + check_whitespace(line, len, patch->ws_rule);
> added++;
> newlines--;
> trailing = 0;
> @@ -1318,6 +1319,10 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)
> if (offset < 0)
> return offset;
>
> + patch->ws_rule = whitespace_rule(patch->new_name
> + ? patch->new_name
> + : patch->old_name);
> +
> patchsize = parse_single_patch(buffer + offset + hdrsize,
> size - offset - hdrsize, patch);
>
> @@ -1568,7 +1573,8 @@ static void remove_last_line(const char **rbuf, int *rsize)
> *rsize = offset + 1;
> }
>
> -static int apply_line(char *output, const char *patch, int plen)
> +static int apply_line(char *output, const char *patch, int plen,
> + unsigned ws_rule)
> {
> /*
> * plen is number of bytes to be copied from patch,
> @@ -1593,7 +1599,7 @@ static int apply_line(char *output, const char *patch, int plen)
> /*
> * Strip trailing whitespace
> */
> - if ((whitespace_rule & WS_TRAILING_SPACE) &&
> + if ((ws_rule & WS_TRAILING_SPACE) &&
> (1 < plen && isspace(patch[plen-1]))) {
> if (patch[plen] == '\n')
> add_nl_to_tail = 1;
> @@ -1610,12 +1616,12 @@ static int apply_line(char *output, const char *patch, int plen)
> char ch = patch[i];
> if (ch == '\t') {
> last_tab_in_indent = i;
> - if ((whitespace_rule & WS_SPACE_BEFORE_TAB) &&
> + if ((ws_rule & WS_SPACE_BEFORE_TAB) &&
> 0 <= last_space_in_indent)
> need_fix_leading_space = 1;
> } else if (ch == ' ') {
> last_space_in_indent = i;
> - if ((whitespace_rule & WS_INDENT_WITH_NON_TAB) &&
> + if ((ws_rule & WS_INDENT_WITH_NON_TAB) &&
> last_tab_in_indent < 0 &&
> 8 <= i)
> need_fix_leading_space = 1;
> @@ -1629,7 +1635,7 @@ static int apply_line(char *output, const char *patch, int plen)
> int consecutive_spaces = 0;
> int last = last_tab_in_indent + 1;
>
> - if (whitespace_rule & WS_INDENT_WITH_NON_TAB) {
> + if (ws_rule & WS_INDENT_WITH_NON_TAB) {
> /* have "last" point at one past the indent */
> if (last_tab_in_indent < last_space_in_indent)
> last = last_space_in_indent + 1;
> @@ -1671,7 +1677,7 @@ static int apply_line(char *output, const char *patch, int plen)
> }
>
> static int apply_one_fragment(struct strbuf *buf, struct fragment *frag,
> - int inaccurate_eof)
> + int inaccurate_eof, unsigned ws_rule)
> {
> int match_beginning, match_end;
> const char *patch = frag->patch;
> @@ -1730,7 +1736,7 @@ static int apply_one_fragment(struct strbuf *buf, struct fragment *frag,
> case '+':
> if (first != '+' || !no_add) {
> int added = apply_line(new + newsize, patch,
> - plen);
> + plen, ws_rule);
> newsize += added;
> if (first == '+' &&
> added == 1 && new[newsize-1] == '\n')
> @@ -1953,12 +1959,14 @@ static int apply_fragments(struct strbuf *buf, struct patch *patch)
> {
> struct fragment *frag = patch->fragments;
> const char *name = patch->old_name ? patch->old_name : patch->new_name;
> + unsigned ws_rule = patch->ws_rule;
> + unsigned inaccurate_eof = patch->inaccurate_eof;
>
> if (patch->is_binary)
> return apply_binary(buf, patch);
>
> while (frag) {
> - if (apply_one_fragment(buf, frag, patch->inaccurate_eof)) {
> + if (apply_one_fragment(buf, frag, inaccurate_eof, ws_rule)) {
> error("patch failed: %s:%ld", name, frag->oldpos);
> if (!apply_with_reject)
> return -1;
> diff --git a/cache.h b/cache.h
> index 3f42827..9cc6268 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -610,6 +610,8 @@ void shift_tree(const unsigned char *, const unsigned char *, unsigned char *, i
> #define WS_SPACE_BEFORE_TAB 02
> #define WS_INDENT_WITH_NON_TAB 04
> #define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB)
> -extern unsigned whitespace_rule;
> +extern unsigned whitespace_rule_cfg;
> +extern unsigned whitespace_rule(const char *);
> +extern unsigned parse_whitespace_rule(const char *);
>
> #endif /* CACHE_H */
> diff --git a/config.c b/config.c
> index d5b9766..2500e0d 100644
> --- a/config.c
> +++ b/config.c
> @@ -246,54 +246,6 @@ static unsigned long get_unit_factor(const char *end)
> die("unknown unit: '%s'", end);
> }
>
> -static struct whitespace_rule {
> - const char *rule_name;
> - unsigned rule_bits;
> -} whitespace_rule_names[] = {
> - { "trailing-space", WS_TRAILING_SPACE },
> - { "space-before-tab", WS_SPACE_BEFORE_TAB },
> - { "indent-with-non-tab", WS_INDENT_WITH_NON_TAB },
> -};
> -
> -static unsigned parse_whitespace_rule(const char *string)
> -{
> - unsigned rule = WS_DEFAULT_RULE;
> -
> - while (string) {
> - int i;
> - size_t len;
> - const char *ep;
> - int negated = 0;
> -
> - string = string + strspn(string, ", \t\n\r");
> - ep = strchr(string, ',');
> - if (!ep)
> - len = strlen(string);
> - else
> - len = ep - string;
> -
> - if (*string == '-') {
> - negated = 1;
> - string++;
> - len--;
> - }
> - if (!len)
> - break;
> - for (i = 0; i < ARRAY_SIZE(whitespace_rule_names); i++) {
> - if (strncmp(whitespace_rule_names[i].rule_name,
> - string, len))
> - continue;
> - if (negated)
> - rule &= ~whitespace_rule_names[i].rule_bits;
> - else
> - rule |= whitespace_rule_names[i].rule_bits;
> - break;
> - }
> - string = ep;
> - }
> - return rule;
> -}
> -
> int git_parse_long(const char *value, long *ret)
> {
> if (value && *value) {
> @@ -480,7 +432,7 @@ int git_default_config(const char *var, const char *value)
> }
>
> if (!strcmp(var, "core.whitespace")) {
> - whitespace_rule = parse_whitespace_rule(value);
> + whitespace_rule_cfg = parse_whitespace_rule(value);
> return 0;
> }
>
> diff --git a/diff.c b/diff.c
> index 6bb902f..c3a1942 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -454,6 +454,7 @@ static void diff_words_show(struct diff_words_data *diff_words)
> struct emit_callback {
> struct xdiff_emit_state xm;
> int nparents, color_diff;
> + unsigned ws_rule;
> const char **label_path;
> struct diff_words_data *diff_words;
> int *found_changesp;
> @@ -493,8 +494,8 @@ static void emit_line(const char *set, const char *reset, const char *line, int
> }
>
> static void emit_line_with_ws(int nparents,
> - const char *set, const char *reset, const char *ws,
> - const char *line, int len)
> + const char *set, const char *reset, const char *ws,
> + const char *line, int len, unsigned ws_rule)
> {
> int col0 = nparents;
> int last_tab_in_indent = -1;
> @@ -511,7 +512,7 @@ static void emit_line_with_ws(int nparents,
> for (i = col0; i < len; i++) {
> if (line[i] == '\t') {
> last_tab_in_indent = i;
> - if ((whitespace_rule & WS_SPACE_BEFORE_TAB) &&
> + if ((ws_rule & WS_SPACE_BEFORE_TAB) &&
> 0 <= last_space_in_indent)
> need_highlight_leading_space = 1;
> }
> @@ -520,7 +521,7 @@ static void emit_line_with_ws(int nparents,
> else
> break;
> }
> - if ((whitespace_rule & WS_INDENT_WITH_NON_TAB) &&
> + if ((ws_rule & WS_INDENT_WITH_NON_TAB) &&
> 0 <= last_space_in_indent &&
> last_tab_in_indent < 0 &&
> 8 <= (i - col0)) {
> @@ -551,7 +552,7 @@ static void emit_line_with_ws(int nparents,
> tail = len - 1;
> if (line[tail] == '\n' && i < tail)
> tail--;
> - if (whitespace_rule & WS_TRAILING_SPACE) {
> + if (ws_rule & WS_TRAILING_SPACE) {
> while (i < tail) {
> if (!isspace(line[tail]))
> break;
> @@ -578,7 +579,7 @@ static void emit_add_line(const char *reset, struct emit_callback *ecbdata, cons
> emit_line(set, reset, line, len);
> else
> emit_line_with_ws(ecbdata->nparents, set, reset, ws,
> - line, len);
> + line, len, ecbdata->ws_rule);
> }
>
> static void fn_out_consume(void *priv, char *line, unsigned long len)
> @@ -994,6 +995,7 @@ struct checkdiff_t {
> struct xdiff_emit_state xm;
> const char *filename;
> int lineno, color_diff;
> + unsigned ws_rule;
> };
>
> static void checkdiff_consume(void *priv, char *line, unsigned long len)
> @@ -1029,7 +1031,8 @@ static void checkdiff_consume(void *priv, char *line, unsigned long len)
> if (white_space_at_end)
> printf("white space at end");
> printf(":%s ", reset);
> - emit_line_with_ws(1, set, reset, ws, line, len);
> + emit_line_with_ws(1, set, reset, ws, line, len,
> + data->ws_rule);
> }
>
> data->lineno++;
> @@ -1330,6 +1333,7 @@ static void builtin_diff(const char *name_a,
> ecbdata.label_path = lbl;
> ecbdata.color_diff = o->color_diff;
> ecbdata.found_changesp = &o->found_changes;
> + ecbdata.ws_rule = whitespace_rule(name_b ? name_b : name_a);
> xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
> xecfg.ctxlen = o->context;
> xecfg.flags = XDL_EMIT_FUNCNAMES;
> @@ -1423,6 +1427,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
> data.filename = name_b ? name_b : name_a;
> data.lineno = 0;
> data.color_diff = o->color_diff;
> + data.ws_rule = whitespace_rule(data.filename);
>
> if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
> die("unable to read files to diff");
> diff --git a/environment.c b/environment.c
> index 624dd96..2fbbc8e 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -35,7 +35,7 @@ int pager_in_use;
> int pager_use_color = 1;
> char *editor_program;
> int auto_crlf = 0; /* 1: both ways, -1: only when adding git objects */
> -unsigned whitespace_rule = WS_DEFAULT_RULE;
> +unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
>
> /* This is set by setup_git_dir_gently() and/or git_default_config() */
> char *git_work_tree_cfg;
> diff --git a/t/t4019-diff-wserror.sh b/t/t4019-diff-wserror.sh
> index dbc895b..67e080b 100755
> --- a/t/t4019-diff-wserror.sh
> +++ b/t/t4019-diff-wserror.sh
> @@ -45,8 +45,24 @@ test_expect_success 'without -trail' '
>
> '
>
> +test_expect_success 'without -trail (attribute)' '
> +
> + git config --unset core.whitespace
> + echo "F whitespace=-trail" >.gitattributes
> + git diff --color >output
> + grep "$blue_grep" output >error
> + grep -v "$blue_grep" output >normal
> +
> + grep Eight normal >/dev/null &&
> + grep HT error >/dev/null &&
> + grep With normal >/dev/null &&
> + grep No normal >/dev/null
> +
> +'
> +
> test_expect_success 'without -space' '
>
> + rm -f .gitattributes
> git config core.whitespace -space
> git diff --color >output
> grep "$blue_grep" output >error
> @@ -59,8 +75,24 @@ test_expect_success 'without -space' '
>
> '
>
> +test_expect_success 'without -space (attribute)' '
> +
> + git config --unset core.whitespace
> + echo "F whitespace=-space" >.gitattributes
> + git diff --color >output
> + grep "$blue_grep" output >error
> + grep -v "$blue_grep" output >normal
> +
> + grep Eight normal >/dev/null &&
> + grep HT normal >/dev/null &&
> + grep With error >/dev/null &&
> + grep No normal >/dev/null
> +
> +'
> +
> test_expect_success 'with indent-non-tab only' '
>
> + rm -f .gitattributes
> git config core.whitespace indent,-trailing,-space
> git diff --color >output
> grep "$blue_grep" output >error
> @@ -73,4 +105,19 @@ test_expect_success 'with indent-non-tab only' '
>
> '
>
> +test_expect_success 'with indent-non-tab only (attribute)' '
> +
> + git config --unset core.whitespace
> + echo "F whitespace=indent,-trailing,-space" >.gitattributes
> + git diff --color >output
> + grep "$blue_grep" output >error
> + grep -v "$blue_grep" output >normal
> +
> + grep Eight error >/dev/null &&
> + grep HT normal >/dev/null &&
> + grep With normal >/dev/null &&
> + grep No normal >/dev/null
> +
> +'
> +
> test_done
> diff --git a/t/t4124-apply-ws-rule.sh b/t/t4124-apply-ws-rule.sh
> index f53ac46..85f3da2 100755
> --- a/t/t4124-apply-ws-rule.sh
> +++ b/t/t4124-apply-ws-rule.sh
> @@ -112,6 +112,15 @@ test_expect_success 'whitespace=error-all, no rule' '
>
> '
>
> +test_expect_success 'whitespace=error-all, no rule (attribute)' '
> +
> + git config --unset core.whitespace &&
> + echo "target -whitespace" >.gitattributes &&
> + apply_patch --whitespace=error-all &&
> + diff file target
> +
> +'
> +
> for t in - ''
> do
> case "$t" in '') tt='!' ;; *) tt= ;; esac
> @@ -121,11 +130,20 @@ do
> for i in - ''
> do
> case "$i" in '') ti='#' ;; *) ti= ;; esac
> - rule=${t}trailing,${s}space,${i}indent &&
> + rule=${t}trailing,${s}space,${i}indent
> +
> + rm -f .gitattributes
> test_expect_success "rule=$rule" '
> git config core.whitespace "$rule" &&
> test_fix "$tt$ts$ti"
> '
> +
> + test_expect_success "rule=$rule (attributes)" '
> + git config --unset core.whitespace &&
> + echo "target whitespace=$rule" >.gitattributes &&
> + test_fix "$tt$ts$ti"
> + '
> +
> done
> done
> done
> diff --git a/ws.c b/ws.c
> new file mode 100644
> index 0000000..52c10ca
> --- /dev/null
> +++ b/ws.c
> @@ -0,0 +1,96 @@
> +/*
> + * Whitespace rules
> + *
> + * Copyright (c) 2007 Junio C Hamano
> + */
> +
> +#include "cache.h"
> +#include "attr.h"
> +
> +static struct whitespace_rule {
> + const char *rule_name;
> + unsigned rule_bits;
> +} whitespace_rule_names[] = {
> + { "trailing-space", WS_TRAILING_SPACE },
> + { "space-before-tab", WS_SPACE_BEFORE_TAB },
> + { "indent-with-non-tab", WS_INDENT_WITH_NON_TAB },
> +};
> +
> +unsigned parse_whitespace_rule(const char *string)
> +{
> + unsigned rule = WS_DEFAULT_RULE;
> +
> + while (string) {
> + int i;
> + size_t len;
> + const char *ep;
> + int negated = 0;
> +
> + string = string + strspn(string, ", \t\n\r");
> + ep = strchr(string, ',');
> + if (!ep)
> + len = strlen(string);
> + else
> + len = ep - string;
> +
> + if (*string == '-') {
> + negated = 1;
> + string++;
> + len--;
> + }
> + if (!len)
> + break;
> + for (i = 0; i < ARRAY_SIZE(whitespace_rule_names); i++) {
> + if (strncmp(whitespace_rule_names[i].rule_name,
> + string, len))
> + continue;
> + if (negated)
> + rule &= ~whitespace_rule_names[i].rule_bits;
> + else
> + rule |= whitespace_rule_names[i].rule_bits;
> + break;
> + }
> + string = ep;
> + }
> + return rule;
> +}
> +
> +static void setup_whitespace_attr_check(struct git_attr_check *check)
> +{
> + static struct git_attr *attr_whitespace;
> +
> + if (!attr_whitespace)
> + attr_whitespace = git_attr("whitespace", 10);
> + check[0].attr = attr_whitespace;
> +}
> +
> +unsigned whitespace_rule(const char *pathname)
> +{
> + struct git_attr_check attr_whitespace_rule;
> +
> + setup_whitespace_attr_check(&attr_whitespace_rule);
> + if (!git_checkattr(pathname, 1, &attr_whitespace_rule)) {
> + const char *value;
> +
> + value = attr_whitespace_rule.value;
> + if (ATTR_TRUE(value)) {
> + /* true (whitespace) */
> + unsigned all_rule = 0;
> + int i;
> + for (i = 0; i < ARRAY_SIZE(whitespace_rule_names); i++)
> + all_rule |= whitespace_rule_names[i].rule_bits;
> + return all_rule;
> + } else if (ATTR_FALSE(value)) {
> + /* false (-whitespace) */
> + return 0;
> + } else if (ATTR_UNSET(value)) {
> + /* reset to default (!whitespace) */
> + return whitespace_rule_cfg;
> + } else {
> + /* string */
> + return parse_whitespace_rule(value);
> + }
> + } else {
> + return whitespace_rule_cfg;
> + }
> +}
> --
> 1.5.3.7-2155-g4c25
>
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 3/2] core.whitespace: documentation updates.
2007-12-06 9:04 ` Junio C Hamano
2007-12-06 19:18 ` J. Bruce Fields
@ 2007-12-16 3:48 ` J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: fix off-by-one error in non-space-in-indent checking J. Bruce Fields
1 sibling, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 3:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
I wrote before:
> On Thu, Dec 06, 2007 at 01:04:56AM -0800, Junio C Hamano wrote:
> > "J. Bruce Fields" <bfields@fieldses.org> writes:
> > > OK, I will go wild, but... very slowly.
> >
> > How wild are you these days ;-)? I know December is a busy time for
> > everybody, and I ended up doing this myself while I was writing up the
> > API documentation for gitattributes.
>
> Wow, thanks! Yes, I haven't done a thing on this.
>
> > -- >8 --
> > [PATCH] Use gitattributes to define per-path whitespace rule
> >
> > The `core.whitespace` configuration variable allows you to define what
> > `diff` and `apply` should consider whitespace errors for all paths in
> > the project (See gitlink:git-config[1]). This attribute gives you
> > finer
> > control per path.
>
> That looks like what I'd hoped for.
>
> I'll set aside some time this weekend to play around with it.
Erm, well, some weekend anyway. You can also pull the following from
git://linux-nfs.org/~bfields/git.git master
if you'd like.
--b.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH] whitespace: fix off-by-one error in non-space-in-indent checking
2007-12-16 3:48 ` J. Bruce Fields
@ 2007-12-16 3:48 ` J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: reorganize initial-indent check J. Bruce Fields
0 siblings, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 3:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, J. Bruce Fields
If there were no tabs, and the last space was at position 7, then
positions 0..7 had spaces, so there were 8 spaces.
Update test to check exactly this case.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
t/t4015-diff-whitespace.sh | 4 ++--
ws.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 9bff8f5..0f16bca 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -298,7 +298,7 @@ test_expect_success 'check space before tab in indent (space-before-tab: on)' '
test_expect_success 'check spaces as indentation (indent-with-non-tab: off)' '
git config core.whitespace "-indent-with-non-tab"
- echo " foo ();" > x &&
+ echo " foo ();" > x &&
git diff --check
'
@@ -306,7 +306,7 @@ test_expect_success 'check spaces as indentation (indent-with-non-tab: off)' '
test_expect_success 'check spaces as indentation (indent-with-non-tab: on)' '
git config core.whitespace "indent-with-non-tab" &&
- echo " foo ();" > x &&
+ echo " foo ();" > x &&
! git diff --check
'
diff --git a/ws.c b/ws.c
index 46cbdd6..5ebd109 100644
--- a/ws.c
+++ b/ws.c
@@ -159,5 +159,5 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
}
/* Check for indent using non-tab. */
- if ((ws_rule & WS_INDENT_WITH_NON_TAB) && leading_space >= 8)
+ if ((ws_rule & WS_INDENT_WITH_NON_TAB) && leading_space >= 7)
result |= WS_INDENT_WITH_NON_TAB;
\ No newline at end of file
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH] whitespace: reorganize initial-indent check
2007-12-16 3:48 ` [PATCH] whitespace: fix off-by-one error in non-space-in-indent checking J. Bruce Fields
@ 2007-12-16 3:48 ` J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: minor cleanup J. Bruce Fields
2007-12-16 10:02 ` [PATCH] whitespace: reorganize initial-indent check Wincent Colaiuta
0 siblings, 2 replies; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 3:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, J. Bruce Fields
Reorganize to emphasize the most complicated part of the code (the tab
case).
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
ws.c | 15 +++++++--------
1 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/ws.c b/ws.c
index 5ebd109..7165874 100644
--- a/ws.c
+++ b/ws.c
@@ -146,16 +146,15 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
/* Check for space before tab in initial indent. */
for (i = 0; i < len; i++) {
- if (line[i] == '\t') {
- if ((ws_rule & WS_SPACE_BEFORE_TAB) &&
- (leading_space != -1))
- result |= WS_SPACE_BEFORE_TAB;
- break;
- }
- else if (line[i] == ' ')
+ if (line[i] == ' ') {
leading_space = i;
- else
+ continue;
+ }
+ if (line[i] != '\t')
break;
+ if ((ws_rule & WS_SPACE_BEFORE_TAB) && (leading_space != -1))
+ result |= WS_SPACE_BEFORE_TAB;
+ break;
}
/* Check for indent using non-tab. */
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH] whitespace: minor cleanup
2007-12-16 3:48 ` [PATCH] whitespace: reorganize initial-indent check J. Bruce Fields
@ 2007-12-16 3:48 ` J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: fix initial-indent checking J. Bruce Fields
2007-12-16 10:02 ` [PATCH] whitespace: reorganize initial-indent check Wincent Colaiuta
1 sibling, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 3:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, J. Bruce Fields
The variable leading_space is initially used to represent the index of
the last space seen before a non-space. Then later it represents the
index of the first non-indent character.
It will prove simpler to replace it by a variable representing a number
of bytes. Eventually it will represent the number of bytes written so
far (in the stream != NULL case).
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
ws.c | 21 +++++++++------------
1 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/ws.c b/ws.c
index 7165874..1b32e45 100644
--- a/ws.c
+++ b/ws.c
@@ -121,7 +121,7 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
const char *reset, const char *ws)
{
unsigned result = 0;
- int leading_space = -1;
+ int written = 0;
int trailing_whitespace = -1;
int trailing_newline = 0;
int i;
@@ -147,18 +147,18 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
/* Check for space before tab in initial indent. */
for (i = 0; i < len; i++) {
if (line[i] == ' ') {
- leading_space = i;
+ written = i + 1;
continue;
}
if (line[i] != '\t')
break;
- if ((ws_rule & WS_SPACE_BEFORE_TAB) && (leading_space != -1))
+ if ((ws_rule & WS_SPACE_BEFORE_TAB) && (written != 0))
result |= WS_SPACE_BEFORE_TAB;
break;
}
/* Check for indent using non-tab. */
- if ((ws_rule & WS_INDENT_WITH_NON_TAB) && leading_space >= 7)
+ if ((ws_rule & WS_INDENT_WITH_NON_TAB) && written >= 8)
result |= WS_INDENT_WITH_NON_TAB;
if (stream) {
@@ -166,23 +166,20 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
if ((result & WS_SPACE_BEFORE_TAB) ||
(result & WS_INDENT_WITH_NON_TAB)) {
fputs(ws, stream);
- fwrite(line, leading_space + 1, 1, stream);
+ fwrite(line, written, 1, stream);
fputs(reset, stream);
- leading_space++;
}
- else
- leading_space = 0;
- /* Now the rest of the line starts at leading_space.
+ /* Now the rest of the line starts at written.
* The non-highlighted part ends at trailing_whitespace. */
if (trailing_whitespace == -1)
trailing_whitespace = len;
/* Emit non-highlighted (middle) segment. */
- if (trailing_whitespace - leading_space > 0) {
+ if (trailing_whitespace - written > 0) {
fputs(set, stream);
- fwrite(line + leading_space,
- trailing_whitespace - leading_space, 1, stream);
+ fwrite(line + written,
+ trailing_whitespace - written, 1, stream);
fputs(reset, stream);
}
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH] whitespace: fix initial-indent checking
2007-12-16 3:48 ` [PATCH] whitespace: minor cleanup J. Bruce Fields
@ 2007-12-16 3:48 ` J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: more accurate initial-indent highlighting J. Bruce Fields
` (2 more replies)
0 siblings, 3 replies; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 3:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, J. Bruce Fields
After this patch, "written" counts the number of bytes up to and
including the most recently seen tab. This allows us to detect (and
count) spaces by comparing to "i".
This allows catching initial indents like '\t ' (a tab followed
by 8 spaces), while previously indent-with-non-tab caught only indents
that consisted entirely of spaces.
This also allows fixing an indent-with-non-tab regression, so we can
again detect indents like '\t \t'.
Also update tests to catch these cases.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
t/t4015-diff-whitespace.sh | 15 +++++++++++++++
ws.c | 10 ++++------
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 0f16bca..d30169f 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -125,6 +125,14 @@ test_expect_success 'check mixed spaces and tabs in indent' '
'
+test_expect_success 'check mixed tabs and spaces in indent' '
+
+ # This is indented with HT SP HT.
+ echo " foo();" > x &&
+ git diff --check | grep "space before tab in indent"
+
+'
+
test_expect_success 'check with no whitespace errors' '
git commit -m "snapshot" &&
@@ -311,4 +319,11 @@ test_expect_success 'check spaces as indentation (indent-with-non-tab: on)' '
'
+test_expect_success 'check tabs and spaces as indentation (indent-with-non-tab: on)' '
+
+ git config core.whitespace "indent-with-non-tab" &&
+ echo " foo ();" > x &&
+ ! git diff --check
+
+'
test_done
diff --git a/ws.c b/ws.c
index 1b32e45..aabd509 100644
--- a/ws.c
+++ b/ws.c
@@ -146,19 +146,17 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
/* Check for space before tab in initial indent. */
for (i = 0; i < len; i++) {
- if (line[i] == ' ') {
- written = i + 1;
+ if (line[i] == ' ')
continue;
- }
if (line[i] != '\t')
break;
- if ((ws_rule & WS_SPACE_BEFORE_TAB) && (written != 0))
+ if ((ws_rule & WS_SPACE_BEFORE_TAB) && written < i)
result |= WS_SPACE_BEFORE_TAB;
- break;
+ written = i + 1;
}
/* Check for indent using non-tab. */
- if ((ws_rule & WS_INDENT_WITH_NON_TAB) && written >= 8)
+ if ((ws_rule & WS_INDENT_WITH_NON_TAB) && i - written >= 8)
result |= WS_INDENT_WITH_NON_TAB;
if (stream) {
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH] whitespace: more accurate initial-indent highlighting
2007-12-16 3:48 ` [PATCH] whitespace: fix initial-indent checking J. Bruce Fields
@ 2007-12-16 3:48 ` J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: fix config.txt description of indent-with-non-tab J. Bruce Fields
2007-12-16 3:54 ` [PATCH] whitespace: fix initial-indent checking J. Bruce Fields
2007-12-16 9:08 ` Jakub Narebski
2 siblings, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 3:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, J. Bruce Fields
Instead of highlighting the entire initial indent, highlight only the
problematic spaces.
In the case of an indent like ' \t \t' there may be multiple problematic
ranges, so it's easiest to emit the highlighting as we go instead of
trying rember disjoint ranges and do it all at the end.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
ws.c | 24 ++++++++++++++++--------
1 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/ws.c b/ws.c
index aabd509..d09b9df 100644
--- a/ws.c
+++ b/ws.c
@@ -150,24 +150,32 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
continue;
if (line[i] != '\t')
break;
- if ((ws_rule & WS_SPACE_BEFORE_TAB) && written < i)
+ if ((ws_rule & WS_SPACE_BEFORE_TAB) && written < i) {
result |= WS_SPACE_BEFORE_TAB;
+ if (stream) {
+ fputs(ws, stream);
+ fwrite(line + written, i - written, 1, stream);
+ fputs(reset, stream);
+ }
+ } else if (stream)
+ fwrite(line + written, i - written, 1, stream);
+ if (stream)
+ fwrite(line + i, 1, 1, stream);
written = i + 1;
}
/* Check for indent using non-tab. */
- if ((ws_rule & WS_INDENT_WITH_NON_TAB) && i - written >= 8)
+ if ((ws_rule & WS_INDENT_WITH_NON_TAB) && i - written >= 8) {
result |= WS_INDENT_WITH_NON_TAB;
-
- if (stream) {
- /* Highlight errors in leading whitespace. */
- if ((result & WS_SPACE_BEFORE_TAB) ||
- (result & WS_INDENT_WITH_NON_TAB)) {
+ if (stream) {
fputs(ws, stream);
- fwrite(line, written, 1, stream);
+ fwrite(line + written, i - written, 1, stream);
fputs(reset, stream);
}
+ written = i;
+ }
+ if (stream) {
/* Now the rest of the line starts at written.
* The non-highlighted part ends at trailing_whitespace. */
if (trailing_whitespace == -1)
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH] whitespace: fix config.txt description of indent-with-non-tab
2007-12-16 3:48 ` [PATCH] whitespace: more accurate initial-indent highlighting J. Bruce Fields
@ 2007-12-16 3:48 ` J. Bruce Fields
0 siblings, 0 replies; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 3:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, J. Bruce Fields
Fix garbled description.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
Documentation/config.txt | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index fabe7f8..ce16fc7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -307,7 +307,7 @@ core.whitespace::
before a tab character in the initial indent part of the line as an
error (enabled by default).
* `indent-with-non-tab` treats a line that is indented with 8 or more
- space characters that can be replaced with tab characters.
+ space characters as an error (not enabled by default).
alias.*::
Command aliases for the gitlink:git[1] command wrapper - e.g.
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 3:48 ` [PATCH] whitespace: fix initial-indent checking J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: more accurate initial-indent highlighting J. Bruce Fields
@ 2007-12-16 3:54 ` J. Bruce Fields
2007-12-16 20:40 ` Junio C Hamano
2007-12-16 9:08 ` Jakub Narebski
2 siblings, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 3:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Sat, Dec 15, 2007 at 10:48:37PM -0500, J. Bruce Fields wrote:
> After this patch, "written" counts the number of bytes up to and
> including the most recently seen tab. This allows us to detect (and
> count) spaces by comparing to "i".
>
> This allows catching initial indents like '\t ' (a tab followed
> by 8 spaces), while previously indent-with-non-tab caught only indents
> that consisted entirely of spaces.
>
> This also allows fixing an indent-with-non-tab regression, so we can
> again detect indents like '\t \t'.
>
> Also update tests to catch these cases.
One slightly weird thing about this: I'd expect indent-with-non-tab to
catch any sequence of 8 or more contiguous spaces, not just such
sequences at the end of the indent. This doesn't quite do that.
You could make it do that with a few more lines of code. But really I
don't think the combination of indent-with-non-tab without
space-before-tab makes any sense.
The only reason I didn't just modify it to turn on the latter whenever
the former is turned on is because I couldn't figure out how to modify
t/t4124-apply-ws-rule.sh to pass.....
But, whatever, that's an extremely minor point. If people try that
combination who knows what they expect.
--b.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 3:54 ` [PATCH] whitespace: fix initial-indent checking J. Bruce Fields
@ 2007-12-16 20:40 ` Junio C Hamano
2007-12-16 21:19 ` J. Bruce Fields
0 siblings, 1 reply; 43+ messages in thread
From: Junio C Hamano @ 2007-12-16 20:40 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: git
"J. Bruce Fields" <bfields@fieldses.org> writes:
> One slightly weird thing about this: I'd expect indent-with-non-tab to
> catch any sequence of 8 or more contiguous spaces, not just such
> sequences at the end of the indent. This doesn't quite do that.
+-------+-------+-------+-------+-------+-------+-------+-------+-------+
Personally, I would hate that. That would muck with two spaces I
deliberately typed after the full stop before this sentence). Please
don't.
Emacs "M-x tabify" tends to do this and I found it unsuitable especially
for code (I am not complaining, it probably was invented for other
purposes and not reformatting code):
If you have original (the run of '>>..>>' is a single tab, '.' is a SP)
dcba....123
fedcba..123
gfedcba.123
and "tabify" the region, you would get:
dcba>>>>123
fedcba>>123
gfedcba.123
That is fine if you are shooting for minimum number of bytes, but often
it is not what you want in your code, especially when the part that
conains the whitespace "..cba 123.." is inside a string constant.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 20:40 ` Junio C Hamano
@ 2007-12-16 21:19 ` J. Bruce Fields
0 siblings, 0 replies; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 21:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Sun, Dec 16, 2007 at 12:40:59PM -0800, Junio C Hamano wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
>
> > One slightly weird thing about this: I'd expect indent-with-non-tab to
> > catch any sequence of 8 or more contiguous spaces, not just such
> > sequences at the end of the indent. This doesn't quite do that.
>
> +-------+-------+-------+-------+-------+-------+-------+-------+-------+
> Personally, I would hate that. That would muck with two spaces I
> deliberately typed after the full stop before this sentence). Please
> don't.
Right, I was only thinking of literal sequences of 8 contiguous spaces,
and only in the initial indent. So it's the failure to flag
"^\t \t"
that I find a little unexpected. Whatever--I don't think it's
particularly important. Like I say, I think there's really only two
cases anyone really cares about:
1. The kernel or git style, where all initial whitespace is
tabbed as much as possible.
2. Various other styles which may be harder to check completely,
but which are likely to share the "no spaces before tabs in
initial indent" rule as a least common denominator.
> Emacs "M-x tabify" tends to do this and I found it unsuitable especially
> for code (I am not complaining, it probably was invented for other
> purposes and not reformatting code):
>
> If you have original (the run of '>>..>>' is a single tab, '.' is a SP)
>
> dcba....123
> fedcba..123
> gfedcba.123
>
> and "tabify" the region, you would get:
>
> dcba>>>>123
> fedcba>>123
> gfedcba.123
>
> That is fine if you are shooting for minimum number of bytes, but often
> it is not what you want in your code, especially when the part that
> conains the whitespace "..cba 123.." is inside a string constant.
Yeah, that sounds pretty irritating.
--b.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 3:48 ` [PATCH] whitespace: fix initial-indent checking J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: more accurate initial-indent highlighting J. Bruce Fields
2007-12-16 3:54 ` [PATCH] whitespace: fix initial-indent checking J. Bruce Fields
@ 2007-12-16 9:08 ` Jakub Narebski
2007-12-16 10:00 ` Wincent Colaiuta
2 siblings, 1 reply; 43+ messages in thread
From: Jakub Narebski @ 2007-12-16 9:08 UTC (permalink / raw)
To: git
J. Bruce Fields wrote:
> This allows catching initial indents like '\t ' (a tab followed
> by 8 spaces), while previously indent-with-non-tab caught only indents
> that consisted entirely of spaces.
I prefer to use tabs for indent, but _spaces_ for align. While previous,
less strict version of check catches indent using spaces, this one also
catches _align_ using spaces.
--
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 9:08 ` Jakub Narebski
@ 2007-12-16 10:00 ` Wincent Colaiuta
2007-12-16 16:26 ` J. Bruce Fields
0 siblings, 1 reply; 43+ messages in thread
From: Wincent Colaiuta @ 2007-12-16 10:00 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Git Mailing List, J. Bruce Fields, Junio Hamano
El 16/12/2007, a las 10:08, Jakub Narebski escribió:
> J. Bruce Fields wrote:
>
>> This allows catching initial indents like '\t ' (a tab
>> followed
>> by 8 spaces), while previously indent-with-non-tab caught only
>> indents
>> that consisted entirely of spaces.
>
> I prefer to use tabs for indent, but _spaces_ for align. While
> previous,
> less strict version of check catches indent using spaces, this one
> also
> catches _align_ using spaces.
I'd say that Jakub's is a fairly common use case (it's used in many
places in the Git codebase too, I think) so it would be a bad thing to
change the behaviour of "indent-with-non-tab".
If you also want to check for "align-with-non-tab" then it really
should be a separate, optional class of whitespace error.
Cheers,
Wincent
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 10:00 ` Wincent Colaiuta
@ 2007-12-16 16:26 ` J. Bruce Fields
2007-12-16 18:16 ` Jakub Narebski
2007-12-16 19:43 ` Junio C Hamano
0 siblings, 2 replies; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 16:26 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: Jakub Narebski, Git Mailing List, Junio Hamano
On Sun, Dec 16, 2007 at 11:00:55AM +0100, Wincent Colaiuta wrote:
> El 16/12/2007, a las 10:08, Jakub Narebski escribió:
>
>> J. Bruce Fields wrote:
>>
>>> This allows catching initial indents like '\t ' (a tab followed
>>> by 8 spaces), while previously indent-with-non-tab caught only indents
>>> that consisted entirely of spaces.
>>
>> I prefer to use tabs for indent, but _spaces_ for align. While previous,
>> less strict version of check catches indent using spaces, this one also
>> catches _align_ using spaces.
No, the previous version didn't work for the align-with-spaces case
either. Consider, for example,
struct widget *find_widget_by_color(struct color *color,
int nth_match, unsigned long flags)
If following a "indent-with-tabs, align-with-spaces" policy, then the
initial whitespaace on the second line should be purely spaces
(otherwise adjusting the tab stops would ruin the alignment). But
indent-with-non-tab would flag this as incorrect even before my fix.
> I'd say that Jakub's is a fairly common use case (it's used in many places
> in the Git codebase too, I think) so it would be a bad thing to change the
> behaviour of "indent-with-non-tab".
>
> If you also want to check for "align-with-non-tab" then it really should be
> a separate, optional class of whitespace error.
I would agree with you if it were not for the fact that if you're using
an "indent-with-tabs, align-with-spaces" policy then the only indent
whitespace problems that you can flag automatically are space-before-tab
problems; anything else requires knowledge of the language syntax.
So indent-with-non-tab has only ever been useful for projects that
insist on tabs for all sequences of 8 spaces in the initial whitespace.
--b.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 16:26 ` J. Bruce Fields
@ 2007-12-16 18:16 ` Jakub Narebski
2007-12-16 18:24 ` Jakub Narebski
2007-12-16 19:59 ` J. Bruce Fields
2007-12-16 19:43 ` Junio C Hamano
1 sibling, 2 replies; 43+ messages in thread
From: Jakub Narebski @ 2007-12-16 18:16 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Wincent Colaiuta, Git Mailing List, Junio Hamano
J. Bruce Fields wrote:
> On Sun, Dec 16, 2007 at 11:00:55AM +0100, Wincent Colaiuta wrote:
>> El 16/12/2007, a las 10:08, Jakub Narebski escribió:
>>
>>> J. Bruce Fields wrote:
>>>
>>>> This allows catching initial indents like '\t ' (a tab followed
>>>> by 8 spaces), while previously indent-with-non-tab caught only indents
>>>> that consisted entirely of spaces.
>>>
>>> I prefer to use tabs for indent, but _spaces_ for align. While previous,
>>> less strict version of check catches indent using spaces, this one also
>>> catches _align_ using spaces.
>
> No, the previous version didn't work for the align-with-spaces case
> either. Consider, for example,
>
> struct widget *find_widget_by_color(struct color *color,
> int nth_match, unsigned long flags)
>
> If following a "indent-with-tabs, align-with-spaces" policy, then the
> initial whitespaace on the second line should be purely spaces
> (otherwise adjusting the tab stops would ruin the alignment). But
> indent-with-non-tab would flag this as incorrect even before my fix.
Yes, this is (if we want "indent with tab, align with spaces") false
positive even with current version of indent-with-non-tab policy, but
it is _rare_ false positive.
It is useful because it catches quite common "indent with spaces only",
for example if MTA or editor replaces tabs with spaces, or if editor
preserves whitespace but it uses spaces for indent.
So for me this version is a good compromise between false positives
and catching real indent whitespace errors. The version proposed has
IMHO too many false positive, while I guess not catching much more
errors in practice.
>> I'd say that Jakub's is a fairly common use case (it's used in many places
>> in the Git codebase too, I think) so it would be a bad thing to change the
>> behaviour of "indent-with-non-tab".
>>
>> If you also want to check for "align-with-non-tab" then it really should be
>> a separate, optional class of whitespace error.
>
> I would agree with you if it were not for the fact that if you're using
> an "indent-with-tabs, align-with-spaces" policy then the only indent
> whitespace problems that you can flag automatically are space-before-tab
> problems; anything else requires knowledge of the language syntax.
Unfortunately quite true (by the way, doesn't new version of
"align-with-non-tab" do not work for Python sources?)
Perhaps it should be called "no-8spaces" os something like that: is the
width (in columns) of a tab character configurable, by the way?
> So indent-with-non-tab has only ever been useful for projects that
> insist on tabs for all sequences of 8 spaces in the initial whitespace.
IMVVHO the new version of "indent-with-non-tab" (aka "no-8-spaces") is
useful _only_ for such project, while old version not only (see comment
above).
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 18:16 ` Jakub Narebski
@ 2007-12-16 18:24 ` Jakub Narebski
2007-12-16 19:59 ` J. Bruce Fields
1 sibling, 0 replies; 43+ messages in thread
From: Jakub Narebski @ 2007-12-16 18:24 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Wincent Colaiuta, Git Mailing List, Junio Hamano
By the way, I have just [a beginning of] an idea: what if we look at
neighbour lines to try to detect whitespace errors? For example when
using tabs for indent, spaces for align, I think it is an error if
spaces sequence after tab begins earlier than tab sequence in neighbour
line ends.
What do you think about it?
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 18:16 ` Jakub Narebski
2007-12-16 18:24 ` Jakub Narebski
@ 2007-12-16 19:59 ` J. Bruce Fields
2007-12-16 20:31 ` Jakub Narebski
2007-12-16 21:00 ` Junio C Hamano
1 sibling, 2 replies; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 19:59 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Wincent Colaiuta, Git Mailing List, Junio Hamano
On Sun, Dec 16, 2007 at 07:16:44PM +0100, Jakub Narebski wrote:
> J. Bruce Fields wrote:
> > No, the previous version didn't work for the align-with-spaces case
> > either. Consider, for example,
> >
> > struct widget *find_widget_by_color(struct color *color,
> > int nth_match, unsigned long flags)
> >
> > If following a "indent-with-tabs, align-with-spaces" policy, then the
> > initial whitespaace on the second line should be purely spaces
> > (otherwise adjusting the tab stops would ruin the alignment). But
> > indent-with-non-tab would flag this as incorrect even before my fix.
>
> Yes, this is (if we want "indent with tab, align with spaces") false
> positive even with current version of indent-with-non-tab policy, but
> it is _rare_ false positive.
You can find examples like the above all over the git source, even in C
where the top-level code is indented in main().
> It is useful because it catches quite common "indent with spaces only",
> for example if MTA or editor replaces tabs with spaces, or if editor
> preserves whitespace but it uses spaces for indent.
>
> So for me this version is a good compromise between false positives
> and catching real indent whitespace errors. The version proposed has
> IMHO too many false positive, while I guess not catching much more
> errors in practice.
Unfortunately, this compromise wouldn't solve my problem.
Which is: I do get the occasional kernel patch with whitespace problems
uncaught by git's existing checks. It's annoying to have to fix them up
manually (but wastes Andrew Morton's time if I don't). This shouldn't
be necessary, because the kernel has a simple policy for initial
whitespace that is completely automatable.
If we've got to define a fourth whitespace policy for this, well, OK,
I'll live--tell me what I need to do. I haven't seen a convincing
argument for that yet, though.
--b.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 19:59 ` J. Bruce Fields
@ 2007-12-16 20:31 ` Jakub Narebski
2007-12-16 21:00 ` Junio C Hamano
1 sibling, 0 replies; 43+ messages in thread
From: Jakub Narebski @ 2007-12-16 20:31 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Wincent Colaiuta, Git Mailing List, Junio Hamano
On Sun, 16 Dec 2007, J. Bruce Fields wrote:
> If we've got to define a fourth whitespace policy for this, well, OK,
> I'll live--tell me what I need to do. I haven't seen a convincing
> argument for that yet, though.
Perhaps the new version of policy should be called indent-with-non-tab,
and we keep old version under name indent-with-spaces, hmmm...?
By the way, what about check for diff3/rcsmerge conflict markers?
This is not "whitespace" error per se, but...
--
Jakub Narebski
Poland
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 19:59 ` J. Bruce Fields
2007-12-16 20:31 ` Jakub Narebski
@ 2007-12-16 21:00 ` Junio C Hamano
1 sibling, 0 replies; 43+ messages in thread
From: Junio C Hamano @ 2007-12-16 21:00 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Jakub Narebski, Wincent Colaiuta, Git Mailing List
"J. Bruce Fields" <bfields@fieldses.org> writes:
> Which is: I do get the occasional kernel patch with whitespace problems
> uncaught by git's existing checks. It's annoying to have to fix them up
> manually (but wastes Andrew Morton's time if I don't). This shouldn't
> be necessary, because the kernel has a simple policy for initial
> whitespace that is completely automatable.
>
> If we've got to define a fourth whitespace policy for this, well, OK,
> I'll live--tell me what I need to do. I haven't seen a convincing
> argument for that yet, though.
There is one "fix" you earlier implemented but there is no way to check
in the current infrastructure of checking one line at a time. A run of
blank lines at the end of file.
I personallyy find it much more interesting and useful than the "align
with space" discussion.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: fix initial-indent checking
2007-12-16 16:26 ` J. Bruce Fields
2007-12-16 18:16 ` Jakub Narebski
@ 2007-12-16 19:43 ` Junio C Hamano
1 sibling, 0 replies; 43+ messages in thread
From: Junio C Hamano @ 2007-12-16 19:43 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Wincent Colaiuta, Jakub Narebski, Git Mailing List
"J. Bruce Fields" <bfields@fieldses.org> writes:
>>> I prefer to use tabs for indent, but _spaces_ for align. While previous,
>>> less strict version of check catches indent using spaces, this one also
>>> catches _align_ using spaces.
>
> No, the previous version didn't work for the align-with-spaces case
> either.
When indent-with-non-tab is in effect, it should tabify spaces in indent
as much as possible. The error definition is deliberately incompatible
with "align with spaces" policy.
The reason I made indent-with-non-tab configurable was to cater to these
people. They can turn it off if they do not want it.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: reorganize initial-indent check
2007-12-16 3:48 ` [PATCH] whitespace: reorganize initial-indent check J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: minor cleanup J. Bruce Fields
@ 2007-12-16 10:02 ` Wincent Colaiuta
2007-12-16 16:31 ` J. Bruce Fields
2007-12-16 21:06 ` [PATCH] whitespace: reorganize initial-indent check Junio C Hamano
1 sibling, 2 replies; 43+ messages in thread
From: Wincent Colaiuta @ 2007-12-16 10:02 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Git Mailing List
El 16/12/2007, a las 4:48, J. Bruce Fields escribió:
> Reorganize to emphasize the most complicated part of the code (the tab
> case).
Any chance of either squashing this series into one patch seeing as
its all churning over the same part of the code, or resending it with
numbering? The patches seemed to arrive out of order in my mailbox and
I don't really know what order they're supposed to be applied in and
it's a bit hard to review.
Cheers,
Wincent
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: reorganize initial-indent check
2007-12-16 10:02 ` [PATCH] whitespace: reorganize initial-indent check Wincent Colaiuta
@ 2007-12-16 16:31 ` J. Bruce Fields
2007-12-16 16:31 ` [PATCH 1/6] whitespace: fix off-by-one error in non-space-in-indent checking J. Bruce Fields
2007-12-16 21:06 ` [PATCH] whitespace: reorganize initial-indent check Junio C Hamano
1 sibling, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 16:31 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git
Wincent Colaiuta wrote:
> Any chance of either squashing this series into one patch seeing as
> its all churning over the same part of the code, or resending it with
> numbering? The patches seemed to arrive out of order in my mailbox
> and I don't really know what order they're supposed to be applied in
> and it's a bit hard to review.
Apologies--I forgot the -n on format-patch.... Here you go. Thanks for
the review.
--b.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 1/6] whitespace: fix off-by-one error in non-space-in-indent checking
2007-12-16 16:31 ` J. Bruce Fields
@ 2007-12-16 16:31 ` J. Bruce Fields
2007-12-16 16:31 ` [PATCH 2/6] whitespace: reorganize initial-indent check J. Bruce Fields
0 siblings, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 16:31 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git, J. Bruce Fields
If there were no tabs, and the last space was at position 7, then
positions 0..7 had spaces, so there were 8 spaces.
Update test to check exactly this case.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
t/t4015-diff-whitespace.sh | 4 ++--
ws.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 9bff8f5..0f16bca 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -298,7 +298,7 @@ test_expect_success 'check space before tab in indent (space-before-tab: on)' '
test_expect_success 'check spaces as indentation (indent-with-non-tab: off)' '
git config core.whitespace "-indent-with-non-tab"
- echo " foo ();" > x &&
+ echo " foo ();" > x &&
git diff --check
'
@@ -306,7 +306,7 @@ test_expect_success 'check spaces as indentation (indent-with-non-tab: off)' '
test_expect_success 'check spaces as indentation (indent-with-non-tab: on)' '
git config core.whitespace "indent-with-non-tab" &&
- echo " foo ();" > x &&
+ echo " foo ();" > x &&
! git diff --check
'
diff --git a/ws.c b/ws.c
index 46cbdd6..5ebd109 100644
--- a/ws.c
+++ b/ws.c
@@ -159,5 +159,5 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
}
/* Check for indent using non-tab. */
- if ((ws_rule & WS_INDENT_WITH_NON_TAB) && leading_space >= 8)
+ if ((ws_rule & WS_INDENT_WITH_NON_TAB) && leading_space >= 7)
result |= WS_INDENT_WITH_NON_TAB;
\ No newline at end of file
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 2/6] whitespace: reorganize initial-indent check
2007-12-16 16:31 ` [PATCH 1/6] whitespace: fix off-by-one error in non-space-in-indent checking J. Bruce Fields
@ 2007-12-16 16:31 ` J. Bruce Fields
2007-12-16 16:31 ` [PATCH 3/6] whitespace: minor cleanup J. Bruce Fields
0 siblings, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 16:31 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git, J. Bruce Fields
Reorganize to emphasize the most complicated part of the code (the tab
case).
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
ws.c | 15 +++++++--------
1 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/ws.c b/ws.c
index 5ebd109..7165874 100644
--- a/ws.c
+++ b/ws.c
@@ -146,16 +146,15 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
/* Check for space before tab in initial indent. */
for (i = 0; i < len; i++) {
- if (line[i] == '\t') {
- if ((ws_rule & WS_SPACE_BEFORE_TAB) &&
- (leading_space != -1))
- result |= WS_SPACE_BEFORE_TAB;
- break;
- }
- else if (line[i] == ' ')
+ if (line[i] == ' ') {
leading_space = i;
- else
+ continue;
+ }
+ if (line[i] != '\t')
break;
+ if ((ws_rule & WS_SPACE_BEFORE_TAB) && (leading_space != -1))
+ result |= WS_SPACE_BEFORE_TAB;
+ break;
}
/* Check for indent using non-tab. */
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 3/6] whitespace: minor cleanup
2007-12-16 16:31 ` [PATCH 2/6] whitespace: reorganize initial-indent check J. Bruce Fields
@ 2007-12-16 16:31 ` J. Bruce Fields
2007-12-16 16:31 ` [PATCH 4/6] whitespace: fix initial-indent checking J. Bruce Fields
0 siblings, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 16:31 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git, J. Bruce Fields
The variable leading_space is initially used to represent the index of
the last space seen before a non-space. Then later it represents the
index of the first non-indent character.
It will prove simpler to replace it by a variable representing a number
of bytes. Eventually it will represent the number of bytes written so
far (in the stream != NULL case).
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
ws.c | 21 +++++++++------------
1 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/ws.c b/ws.c
index 7165874..1b32e45 100644
--- a/ws.c
+++ b/ws.c
@@ -121,7 +121,7 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
const char *reset, const char *ws)
{
unsigned result = 0;
- int leading_space = -1;
+ int written = 0;
int trailing_whitespace = -1;
int trailing_newline = 0;
int i;
@@ -147,18 +147,18 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
/* Check for space before tab in initial indent. */
for (i = 0; i < len; i++) {
if (line[i] == ' ') {
- leading_space = i;
+ written = i + 1;
continue;
}
if (line[i] != '\t')
break;
- if ((ws_rule & WS_SPACE_BEFORE_TAB) && (leading_space != -1))
+ if ((ws_rule & WS_SPACE_BEFORE_TAB) && (written != 0))
result |= WS_SPACE_BEFORE_TAB;
break;
}
/* Check for indent using non-tab. */
- if ((ws_rule & WS_INDENT_WITH_NON_TAB) && leading_space >= 7)
+ if ((ws_rule & WS_INDENT_WITH_NON_TAB) && written >= 8)
result |= WS_INDENT_WITH_NON_TAB;
if (stream) {
@@ -166,23 +166,20 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
if ((result & WS_SPACE_BEFORE_TAB) ||
(result & WS_INDENT_WITH_NON_TAB)) {
fputs(ws, stream);
- fwrite(line, leading_space + 1, 1, stream);
+ fwrite(line, written, 1, stream);
fputs(reset, stream);
- leading_space++;
}
- else
- leading_space = 0;
- /* Now the rest of the line starts at leading_space.
+ /* Now the rest of the line starts at written.
* The non-highlighted part ends at trailing_whitespace. */
if (trailing_whitespace == -1)
trailing_whitespace = len;
/* Emit non-highlighted (middle) segment. */
- if (trailing_whitespace - leading_space > 0) {
+ if (trailing_whitespace - written > 0) {
fputs(set, stream);
- fwrite(line + leading_space,
- trailing_whitespace - leading_space, 1, stream);
+ fwrite(line + written,
+ trailing_whitespace - written, 1, stream);
fputs(reset, stream);
}
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 4/6] whitespace: fix initial-indent checking
2007-12-16 16:31 ` [PATCH 3/6] whitespace: minor cleanup J. Bruce Fields
@ 2007-12-16 16:31 ` J. Bruce Fields
2007-12-16 16:31 ` [PATCH 5/6] whitespace: more accurate initial-indent highlighting J. Bruce Fields
0 siblings, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 16:31 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git, J. Bruce Fields
After this patch, "written" counts the number of bytes up to and
including the most recently seen tab. This allows us to detect (and
count) spaces by comparing to "i".
This allows catching initial indents like '\t ' (a tab followed
by 8 spaces), while previously indent-with-non-tab caught only indents
that consisted entirely of spaces.
This also allows fixing an indent-with-non-tab regression, so we can
again detect indents like '\t \t'.
Also update tests to catch these cases.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
t/t4015-diff-whitespace.sh | 15 +++++++++++++++
ws.c | 10 ++++------
2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 0f16bca..d30169f 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -125,6 +125,14 @@ test_expect_success 'check mixed spaces and tabs in indent' '
'
+test_expect_success 'check mixed tabs and spaces in indent' '
+
+ # This is indented with HT SP HT.
+ echo " foo();" > x &&
+ git diff --check | grep "space before tab in indent"
+
+'
+
test_expect_success 'check with no whitespace errors' '
git commit -m "snapshot" &&
@@ -311,4 +319,11 @@ test_expect_success 'check spaces as indentation (indent-with-non-tab: on)' '
'
+test_expect_success 'check tabs and spaces as indentation (indent-with-non-tab: on)' '
+
+ git config core.whitespace "indent-with-non-tab" &&
+ echo " foo ();" > x &&
+ ! git diff --check
+
+'
test_done
diff --git a/ws.c b/ws.c
index 1b32e45..aabd509 100644
--- a/ws.c
+++ b/ws.c
@@ -146,19 +146,17 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
/* Check for space before tab in initial indent. */
for (i = 0; i < len; i++) {
- if (line[i] == ' ') {
- written = i + 1;
+ if (line[i] == ' ')
continue;
- }
if (line[i] != '\t')
break;
- if ((ws_rule & WS_SPACE_BEFORE_TAB) && (written != 0))
+ if ((ws_rule & WS_SPACE_BEFORE_TAB) && written < i)
result |= WS_SPACE_BEFORE_TAB;
- break;
+ written = i + 1;
}
/* Check for indent using non-tab. */
- if ((ws_rule & WS_INDENT_WITH_NON_TAB) && written >= 8)
+ if ((ws_rule & WS_INDENT_WITH_NON_TAB) && i - written >= 8)
result |= WS_INDENT_WITH_NON_TAB;
if (stream) {
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 5/6] whitespace: more accurate initial-indent highlighting
2007-12-16 16:31 ` [PATCH 4/6] whitespace: fix initial-indent checking J. Bruce Fields
@ 2007-12-16 16:31 ` J. Bruce Fields
2007-12-16 16:31 ` [PATCH 6/6] whitespace: fix config.txt description of indent-with-non-tab J. Bruce Fields
2007-12-17 8:00 ` [PATCH 5/6] whitespace: more accurate initial-indent highlighting Wincent Colaiuta
0 siblings, 2 replies; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 16:31 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git, J. Bruce Fields
Instead of highlighting the entire initial indent, highlight only the
problematic spaces.
In the case of an indent like ' \t \t' there may be multiple problematic
ranges, so it's easiest to emit the highlighting as we go instead of
trying rember disjoint ranges and do it all at the end.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
ws.c | 24 ++++++++++++++++--------
1 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/ws.c b/ws.c
index aabd509..d09b9df 100644
--- a/ws.c
+++ b/ws.c
@@ -150,24 +150,32 @@ unsigned check_and_emit_line(const char *line, int len, unsigned ws_rule,
continue;
if (line[i] != '\t')
break;
- if ((ws_rule & WS_SPACE_BEFORE_TAB) && written < i)
+ if ((ws_rule & WS_SPACE_BEFORE_TAB) && written < i) {
result |= WS_SPACE_BEFORE_TAB;
+ if (stream) {
+ fputs(ws, stream);
+ fwrite(line + written, i - written, 1, stream);
+ fputs(reset, stream);
+ }
+ } else if (stream)
+ fwrite(line + written, i - written, 1, stream);
+ if (stream)
+ fwrite(line + i, 1, 1, stream);
written = i + 1;
}
/* Check for indent using non-tab. */
- if ((ws_rule & WS_INDENT_WITH_NON_TAB) && i - written >= 8)
+ if ((ws_rule & WS_INDENT_WITH_NON_TAB) && i - written >= 8) {
result |= WS_INDENT_WITH_NON_TAB;
-
- if (stream) {
- /* Highlight errors in leading whitespace. */
- if ((result & WS_SPACE_BEFORE_TAB) ||
- (result & WS_INDENT_WITH_NON_TAB)) {
+ if (stream) {
fputs(ws, stream);
- fwrite(line, written, 1, stream);
+ fwrite(line + written, i - written, 1, stream);
fputs(reset, stream);
}
+ written = i;
+ }
+ if (stream) {
/* Now the rest of the line starts at written.
* The non-highlighted part ends at trailing_whitespace. */
if (trailing_whitespace == -1)
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 6/6] whitespace: fix config.txt description of indent-with-non-tab
2007-12-16 16:31 ` [PATCH 5/6] whitespace: more accurate initial-indent highlighting J. Bruce Fields
@ 2007-12-16 16:31 ` J. Bruce Fields
2007-12-16 17:58 ` builtin-apply whitespace J. Bruce Fields
2007-12-17 8:00 ` [PATCH 5/6] whitespace: more accurate initial-indent highlighting Wincent Colaiuta
1 sibling, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 16:31 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git, J. Bruce Fields
Fix garbled description.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
Documentation/config.txt | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index fabe7f8..ce16fc7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -307,7 +307,7 @@ core.whitespace::
before a tab character in the initial indent part of the line as an
error (enabled by default).
* `indent-with-non-tab` treats a line that is indented with 8 or more
- space characters that can be replaced with tab characters.
+ space characters as an error (not enabled by default).
alias.*::
Command aliases for the gitlink:git[1] command wrapper - e.g.
--
1.5.4.rc0.41.gf723
^ permalink raw reply related [flat|nested] 43+ messages in thread
* builtin-apply whitespace
2007-12-16 16:31 ` [PATCH 6/6] whitespace: fix config.txt description of indent-with-non-tab J. Bruce Fields
@ 2007-12-16 17:58 ` J. Bruce Fields
2007-12-16 17:58 ` [PATCH 1/2] builtin-apply: minor cleanup of whitespace detection J. Bruce Fields
0 siblings, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 17:58 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git, Junio C Hamano
Apologies, I just realized I also forgot to do the corresponding fix on
the apply side. Here they are. These are also available at
git://linux-nfs.org/~bfields/git.git master
--b.
^ permalink raw reply [flat|nested] 43+ messages in thread
* [PATCH 1/2] builtin-apply: minor cleanup of whitespace detection
2007-12-16 17:58 ` builtin-apply whitespace J. Bruce Fields
@ 2007-12-16 17:58 ` J. Bruce Fields
2007-12-16 17:58 ` [PATCH 2/2] builtin-apply: stronger indent-with-on-tab fixing J. Bruce Fields
0 siblings, 1 reply; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 17:58 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git, Junio C Hamano, J. Bruce Fields
Use 0 instead of -1 for the case where not tabs or spaces are found; it
will make some later math slightly simpler.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
builtin-apply.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index 2edd83b..bd94a4b 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1550,8 +1550,8 @@ static int apply_line(char *output, const char *patch, int plen,
int i;
int add_nl_to_tail = 0;
int fixed = 0;
- int last_tab_in_indent = -1;
- int last_space_in_indent = -1;
+ int last_tab_in_indent = 0;
+ int last_space_in_indent = 0;
int need_fix_leading_space = 0;
char *buf;
@@ -1582,12 +1582,12 @@ static int apply_line(char *output, const char *patch, int plen,
if (ch == '\t') {
last_tab_in_indent = i;
if ((ws_rule & WS_SPACE_BEFORE_TAB) &&
- 0 <= last_space_in_indent)
+ 0 < last_space_in_indent)
need_fix_leading_space = 1;
} else if (ch == ' ') {
last_space_in_indent = i;
if ((ws_rule & WS_INDENT_WITH_NON_TAB) &&
- last_tab_in_indent < 0 &&
+ last_tab_in_indent <= 0 &&
8 <= i)
need_fix_leading_space = 1;
}
--
1.5.4.rc0.44.g21147
^ permalink raw reply related [flat|nested] 43+ messages in thread
* [PATCH 2/2] builtin-apply: stronger indent-with-on-tab fixing
2007-12-16 17:58 ` [PATCH 1/2] builtin-apply: minor cleanup of whitespace detection J. Bruce Fields
@ 2007-12-16 17:58 ` J. Bruce Fields
0 siblings, 0 replies; 43+ messages in thread
From: J. Bruce Fields @ 2007-12-16 17:58 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git, Junio C Hamano, J. Bruce Fields
Fix any sequence of 8 spaces in initial indent, not just the case where
the 8 spaces are the first thing on the line.
Signed-off-by: J. Bruce Fields <bfields@citi.umich.edu>
---
builtin-apply.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/builtin-apply.c b/builtin-apply.c
index bd94a4b..5e3b4a1 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1587,8 +1587,7 @@ static int apply_line(char *output, const char *patch, int plen,
} else if (ch == ' ') {
last_space_in_indent = i;
if ((ws_rule & WS_INDENT_WITH_NON_TAB) &&
- last_tab_in_indent <= 0 &&
- 8 <= i)
+ 8 <= i - last_tab_in_indent)
need_fix_leading_space = 1;
}
else
--
1.5.4.rc0.44.g21147
^ permalink raw reply related [flat|nested] 43+ messages in thread
* Re: [PATCH 5/6] whitespace: more accurate initial-indent highlighting
2007-12-16 16:31 ` [PATCH 5/6] whitespace: more accurate initial-indent highlighting J. Bruce Fields
2007-12-16 16:31 ` [PATCH 6/6] whitespace: fix config.txt description of indent-with-non-tab J. Bruce Fields
@ 2007-12-17 8:00 ` Wincent Colaiuta
2007-12-17 8:04 ` Junio C Hamano
1 sibling, 1 reply; 43+ messages in thread
From: Wincent Colaiuta @ 2007-12-17 8:00 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Git Mailing List, Junio Hamano
El 16/12/2007, a las 17:31, J. Bruce Fields escribió:
> Instead of highlighting the entire initial indent, highlight only the
> problematic spaces.
>
> In the case of an indent like ' \t \t' there may be multiple
> problematic
> ranges, so it's easiest to emit the highlighting as we go instead of
> trying rember disjoint ranges and do it all at the end.
I'm relatively opposed to mixing the "check" and the "emit" phases
here because it will make further refactoring harder.
In the initial version of your series you forgot that there was
another place in the codebase ("git apply --whitespace=fix") where
whitespace errors are detected, and so you introduced an inconsistency
which you later fixed up. To me this is indicative of the fact that we
need to refactor further so that there really is only *one* place
where the whitespace checking logic is implemented.
Basically I would have proposed extracting out each type of whitespace
error into an inline function in ws.c, where it could be used by both
check_and_emit_line() in ws.c and apply_one_fragment() in builtin-
apply.c.
Unfortunately, mixing checking and emission phases makes this proposed
refactoring a little bit ugly.
But I see it's already gone into master as ffe56885.
Wincent
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/6] whitespace: more accurate initial-indent highlighting
2007-12-17 8:00 ` [PATCH 5/6] whitespace: more accurate initial-indent highlighting Wincent Colaiuta
@ 2007-12-17 8:04 ` Junio C Hamano
2007-12-18 0:32 ` Jakub Narebski
0 siblings, 1 reply; 43+ messages in thread
From: Junio C Hamano @ 2007-12-17 8:04 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: J. Bruce Fields, Git Mailing List
Wincent Colaiuta <win@wincent.com> writes:
> Basically I would have proposed extracting out each type of whitespace
> error into an inline function in ws.c, where it could be used by both
> check_and_emit_line() in ws.c and apply_one_fragment() in builtin-
> apply.c.
>
> Unfortunately, mixing checking and emission phases makes this proposed
> refactoring a little bit ugly.
The right refactoring would be what JBF hinted in his message, to record
and return a list of suspicious ranges from the checker function and
have the highlighter and the fixer make use of that list.
Such a refactoring is still possible but I think it is beyond the scope
of pre 1.5.4 clean-up.
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH 5/6] whitespace: more accurate initial-indent highlighting
2007-12-17 8:04 ` Junio C Hamano
@ 2007-12-18 0:32 ` Jakub Narebski
2007-12-18 0:51 ` Junio C Hamano
0 siblings, 1 reply; 43+ messages in thread
From: Jakub Narebski @ 2007-12-18 0:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Wincent Colaiuta, J. Bruce Fields, Git Mailing List
Junio C Hamano <gitster@pobox.com> writes:
> Wincent Colaiuta <win@wincent.com> writes:
>
> > Basically I would have proposed extracting out each type of whitespace
> > error into an inline function in ws.c, where it could be used by both
> > check_and_emit_line() in ws.c and apply_one_fragment() in builtin-
> > apply.c.
> >
> > Unfortunately, mixing checking and emission phases makes this proposed
> > refactoring a little bit ugly.
>
> The right refactoring would be what JBF hinted in his message, to record
> and return a list of suspicious ranges from the checker function and
> have the highlighter and the fixer make use of that list.
>
> Such a refactoring is still possible but I think it is beyond the scope
> of pre 1.5.4 clean-up.
By the way, does "trailing empty lines at the end of file" whitespace
error get detected and hightlighted with refactored whitespace checking?
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] whitespace: reorganize initial-indent check
2007-12-16 10:02 ` [PATCH] whitespace: reorganize initial-indent check Wincent Colaiuta
2007-12-16 16:31 ` J. Bruce Fields
@ 2007-12-16 21:06 ` Junio C Hamano
1 sibling, 0 replies; 43+ messages in thread
From: Junio C Hamano @ 2007-12-16 21:06 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: J. Bruce Fields, Git Mailing List
Wincent Colaiuta <win@wincent.com> writes:
> El 16/12/2007, a las 4:48, J. Bruce Fields escribi> Reorganize to emphasize the most complicated part of the code (the tab
>> case).
>
> Any chance of either squashing this series into one patch seeing as
> its all churning over the same part of the code, or resending it with
> numbering? The patches seemed to arrive out of order in my mailbox and
> I don't really know what order they're supposed to be applied in and
> it's a bit hard to review.
I do not think squashing is necessary or a good idea in this case. As
far as I can tell, the series does not have "oops, this fixes the
earlier problem I introduced in the series", but it is purely a logical
progression. The first one fixes a bug introduced by the 0-base
conversion which can and should stand on its own.
The mails were properly threaded with in-reply-to so numbering is not
strictly necessary either, but would have helped readers with MUA that
do not pay attention to that header.
I already see JBF resent the series, which is very nice of him.
^ permalink raw reply [flat|nested] 43+ messages in thread
end of thread, other threads:[~2007-12-18 0:51 UTC | newest]
Thread overview: 43+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-11-24 4:24 [PATCH 1/2] builtin-apply: rename "whitespace" variables and fix styles Junio C Hamano
2007-11-24 4:25 ` [PATCH 2/2] builtin-apply: teach whitespace_rules Junio C Hamano
2007-11-24 20:09 ` [PATCH 3/2] core.whitespace: documentation updates Junio C Hamano
2007-11-24 20:22 ` J. Bruce Fields
2007-11-24 21:42 ` Junio C Hamano
2007-11-25 21:58 ` J. Bruce Fields
2007-12-06 9:04 ` Junio C Hamano
2007-12-06 19:18 ` J. Bruce Fields
2007-12-16 3:48 ` J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: fix off-by-one error in non-space-in-indent checking J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: reorganize initial-indent check J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: minor cleanup J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: fix initial-indent checking J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: more accurate initial-indent highlighting J. Bruce Fields
2007-12-16 3:48 ` [PATCH] whitespace: fix config.txt description of indent-with-non-tab J. Bruce Fields
2007-12-16 3:54 ` [PATCH] whitespace: fix initial-indent checking J. Bruce Fields
2007-12-16 20:40 ` Junio C Hamano
2007-12-16 21:19 ` J. Bruce Fields
2007-12-16 9:08 ` Jakub Narebski
2007-12-16 10:00 ` Wincent Colaiuta
2007-12-16 16:26 ` J. Bruce Fields
2007-12-16 18:16 ` Jakub Narebski
2007-12-16 18:24 ` Jakub Narebski
2007-12-16 19:59 ` J. Bruce Fields
2007-12-16 20:31 ` Jakub Narebski
2007-12-16 21:00 ` Junio C Hamano
2007-12-16 19:43 ` Junio C Hamano
2007-12-16 10:02 ` [PATCH] whitespace: reorganize initial-indent check Wincent Colaiuta
2007-12-16 16:31 ` J. Bruce Fields
2007-12-16 16:31 ` [PATCH 1/6] whitespace: fix off-by-one error in non-space-in-indent checking J. Bruce Fields
2007-12-16 16:31 ` [PATCH 2/6] whitespace: reorganize initial-indent check J. Bruce Fields
2007-12-16 16:31 ` [PATCH 3/6] whitespace: minor cleanup J. Bruce Fields
2007-12-16 16:31 ` [PATCH 4/6] whitespace: fix initial-indent checking J. Bruce Fields
2007-12-16 16:31 ` [PATCH 5/6] whitespace: more accurate initial-indent highlighting J. Bruce Fields
2007-12-16 16:31 ` [PATCH 6/6] whitespace: fix config.txt description of indent-with-non-tab J. Bruce Fields
2007-12-16 17:58 ` builtin-apply whitespace J. Bruce Fields
2007-12-16 17:58 ` [PATCH 1/2] builtin-apply: minor cleanup of whitespace detection J. Bruce Fields
2007-12-16 17:58 ` [PATCH 2/2] builtin-apply: stronger indent-with-on-tab fixing J. Bruce Fields
2007-12-17 8:00 ` [PATCH 5/6] whitespace: more accurate initial-indent highlighting Wincent Colaiuta
2007-12-17 8:04 ` Junio C Hamano
2007-12-18 0:32 ` Jakub Narebski
2007-12-18 0:51 ` Junio C Hamano
2007-12-16 21:06 ` [PATCH] whitespace: reorganize initial-indent check Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).