From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 1 Jul 2008 23:22:35 +0000 (-0700)
Subject: Merge branch 'jc/checkdiff'
X-Git-Tag: v1.6.0-rc0~175
X-Git-Url: https://git.lorimer.id.au/gitweb.git/diff_plain/d4b76e15ea733002e364298a81889452b8bb2dfd?ds=inline;hp=-c

Merge branch 'jc/checkdiff'

* jc/checkdiff:
Fix t4017-diff-retval for white-space from wc
Update sample pre-commit hook to use "diff --check"
diff --check: detect leftover conflict markers
Teach "diff --check" about new blank lines at end
checkdiff: pass diff_options to the callback
check_and_emit_line(): rename and refactor
diff --check: explain why we do not care whether old side is binary
---

d4b76e15ea733002e364298a81889452b8bb2dfd
diff --combined builtin-apply.c
index 318504a037,92f00471bb..985ca3be5a
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@@ -12,7 -12,6 +12,7 @@@
  #include "blob.h"
  #include "delta.h"
  #include "builtin.h"
 +#include "path-list.h"
  
  /*
   *  --check turns on checking that the working tree matches the
@@@ -186,13 -185,6 +186,13 @@@ struct image 
  	struct line *line;
  };
  
 +/*
 + * Records filenames that have been touched, in order to handle
 + * the case where more than one patches touch the same file.
 + */
 +
 +static struct path_list fn_table;
 +
  static uint32_t hash_line(const char *cp, size_t len)
  {
  	size_t i;
@@@ -987,8 -979,7 +987,7 @@@ static int find_header(char *line, unsi
  static void check_whitespace(const char *line, int len, unsigned ws_rule)
  {
  	char *err;
- 	unsigned result = check_and_emit_line(line + 1, len - 1, ws_rule,
- 	    NULL, NULL, NULL, NULL);
+ 	unsigned result = ws_check(line + 1, len - 1, ws_rule);
  	if (!result)
  		return;
  
@@@ -999,7 -990,7 +998,7 @@@
  	else {
  		err = whitespace_error_string(result);
  		fprintf(stderr, "%s:%d: %s.\n%.*s\n",
- 		     patch_input_file, linenr, err, len - 2, line + 1);
+ 			patch_input_file, linenr, err, len - 2, line + 1);
  		free(err);
  	}
  }
@@@ -2184,62 -2175,15 +2183,62 @@@ static int read_file_or_gitlink(struct 
  	return 0;
  }
  
 +static struct patch *in_fn_table(const char *name)
 +{
 +	struct path_list_item *item;
 +
 +	if (name == NULL)
 +		return NULL;
 +
 +	item = path_list_lookup(name, &fn_table);
 +	if (item != NULL)
 +		return (struct patch *)item->util;
 +
 +	return NULL;
 +}
 +
 +static void add_to_fn_table(struct patch *patch)
 +{
 +	struct path_list_item *item;
 +
 +	/*
 +	 * Always add new_name unless patch is a deletion
 +	 * This should cover the cases for normal diffs,
 +	 * file creations and copies
 +	 */
 +	if (patch->new_name != NULL) {
 +		item = path_list_insert(patch->new_name, &fn_table);
 +		item->util = patch;
 +	}
 +
 +	/*
 +	 * store a failure on rename/deletion cases because
 +	 * later chunks shouldn't patch old names
 +	 */
 +	if ((patch->new_name == NULL) || (patch->is_rename)) {
 +		item = path_list_insert(patch->old_name, &fn_table);
 +		item->util = (struct patch *) -1;
 +	}
 +}
 +
  static int apply_data(struct patch *patch, struct stat *st, struct cache_entry *ce)
  {
  	struct strbuf buf;
  	struct image image;
  	size_t len;
  	char *img;
 +	struct patch *tpatch;
  
  	strbuf_init(&buf, 0);
 -	if (cached) {
 +
 +	if ((tpatch = in_fn_table(patch->old_name)) != NULL) {
 +		if (tpatch == (struct patch *) -1) {
 +			return error("patch %s has been renamed/deleted",
 +				patch->old_name);
 +		}
 +		/* We have a patched copy in memory use that */
 +		strbuf_add(&buf, tpatch->result, tpatch->resultsize);
 +	} else if (cached) {
  		if (read_file_or_gitlink(ce, &buf))
  			return error("read of %s failed", patch->old_name);
  	} else if (patch->old_name) {
@@@ -2266,7 -2210,6 +2265,7 @@@
  		return -1; /* note with --reject this succeeds. */
  	patch->result = image.buf;
  	patch->resultsize = image.len;
 +	add_to_fn_table(patch);
  	free(image.line_allocated);
  
  	if (0 < patch->is_delete && patch->resultsize)
@@@ -2311,7 -2254,6 +2310,7 @@@ static int verify_index_match(struct ca
  static int check_preimage(struct patch *patch, struct cache_entry **ce, struct stat *st)
  {
  	const char *old_name = patch->old_name;
 +	struct patch *tpatch;
  	int stat_ret = 0;
  	unsigned st_mode = 0;
  
@@@ -2325,17 -2267,12 +2324,17 @@@
  		return 0;
  
  	assert(patch->is_new <= 0);
 -	if (!cached) {
 +	if ((tpatch = in_fn_table(old_name)) != NULL) {
 +		if (tpatch == (struct patch *) -1) {
 +			return error("%s: has been deleted/renamed", old_name);
 +		}
 +		st_mode = tpatch->new_mode;
 +	} else if (!cached) {
  		stat_ret = lstat(old_name, st);
  		if (stat_ret && errno != ENOENT)
  			return error("%s: %s", old_name, strerror(errno));
  	}
 -	if (check_index) {
 +	if (check_index && !tpatch) {
  		int pos = cache_name_pos(old_name, strlen(old_name));
  		if (pos < 0) {
  			if (patch->is_new < 0)
@@@ -2387,7 -2324,7 +2386,7 @@@
  	return 0;
  }
  
 -static int check_patch(struct patch *patch, struct patch *prev_patch)
 +static int check_patch(struct patch *patch)
  {
  	struct stat st;
  	const char *old_name = patch->old_name;
@@@ -2404,7 -2341,8 +2403,7 @@@
  		return status;
  	old_name = patch->old_name;
  
 -	if (new_name && prev_patch && 0 < prev_patch->is_delete &&
 -	    !strcmp(prev_patch->old_name, new_name))
 +	if (in_fn_table(new_name) == (struct patch *) -1)
  		/*
  		 * A type-change diff is always split into a patch to
  		 * delete old, immediately followed by a patch to
@@@ -2454,14 -2392,15 +2453,14 @@@
  
  static int check_patch_list(struct patch *patch)
  {
 -	struct patch *prev_patch = NULL;
  	int err = 0;
  
 -	for (prev_patch = NULL; patch ; patch = patch->next) {
 +	while (patch) {
  		if (apply_verbosely)
  			say_patch_name(stderr,
  				       "Checking patch ", patch, "...\n");
 -		err |= check_patch(patch, prev_patch);
 -		prev_patch = patch;
 +		err |= check_patch(patch);
 +		patch = patch->next;
  	}
  	return err;
  }
@@@ -2979,8 -2918,6 +2978,8 @@@ static int apply_patch(int fd, const ch
  	struct patch *list = NULL, **listp = &list;
  	int skipped_patch = 0;
  
 +	/* FIXME - memory leak when using multiple patch files as inputs */
 +	memset(&fn_table, 0, sizeof(struct path_list));
  	strbuf_init(&buf, 0);
  	patch_input_file = filename;
  	read_patch_file(&buf, fd);
diff --combined diff.c
index 66851b5647,d515b06ea3..803fbba451
--- a/diff.c
+++ b/diff.c
@@@ -535,9 -535,9 +535,9 @@@ static void emit_add_line(const char *r
  	else {
  		/* Emit just the prefix, then the rest. */
  		emit_line(ecbdata->file, set, reset, line, ecbdata->nparents);
- 		(void)check_and_emit_line(line + ecbdata->nparents,
- 		    len - ecbdata->nparents, ecbdata->ws_rule,
- 		    ecbdata->file, set, reset, ws);
+ 		ws_check_emit(line + ecbdata->nparents,
+ 			      len - ecbdata->nparents, ecbdata->ws_rule,
+ 			      ecbdata->file, set, reset, ws);
  	}
  }
  
@@@ -830,12 -830,12 +830,12 @@@ static void show_stats(struct diffstat_
  	/* Sanity: give at least 5 columns to the graph,
  	 * but leave at least 10 columns for the name.
  	 */
 -	if (width < name_width + 15) {
 -		if (name_width <= 25)
 -			width = name_width + 15;
 -		else
 -			name_width = width - 15;
 -	}
 +	if (width < 25)
 +		width = 25;
 +	if (name_width < 10)
 +		name_width = 10;
 +	else if (width < name_width + 15)
 +		name_width = width - 15;
  
  	/* Find the longest filename and max number of changes */
  	reset = diff_get_color_opt(options, DIFF_RESET);
@@@ -1136,42 -1136,85 +1136,85 @@@ static void free_diffstat_info(struct d
  struct checkdiff_t {
  	struct xdiff_emit_state xm;
  	const char *filename;
- 	int lineno, color_diff;
+ 	int lineno;
+ 	struct diff_options *o;
  	unsigned ws_rule;
  	unsigned status;
- 	FILE *file;
+ 	int trailing_blanks_start;
  };
  
+ static int is_conflict_marker(const char *line, unsigned long len)
+ {
+ 	char firstchar;
+ 	int cnt;
+ 
+ 	if (len < 8)
+ 		return 0;
+ 	firstchar = line[0];
+ 	switch (firstchar) {
+ 	case '=': case '>': case '<':
+ 		break;
+ 	default:
+ 		return 0;
+ 	}
+ 	for (cnt = 1; cnt < 7; cnt++)
+ 		if (line[cnt] != firstchar)
+ 			return 0;
+ 	/* line[0] thru line[6] are same as firstchar */
+ 	if (firstchar == '=') {
+ 		/* divider between ours and theirs? */
+ 		if (len != 8 || line[7] != '\n')
+ 			return 0;
+ 	} else if (len < 8 || !isspace(line[7])) {
+ 		/* not divider before ours nor after theirs */
+ 		return 0;
+ 	}
+ 	return 1;
+ }
+ 
  static void checkdiff_consume(void *priv, char *line, unsigned long len)
  {
  	struct checkdiff_t *data = priv;
- 	const char *ws = diff_get_color(data->color_diff, DIFF_WHITESPACE);
- 	const char *reset = diff_get_color(data->color_diff, DIFF_RESET);
- 	const char *set = diff_get_color(data->color_diff, DIFF_FILE_NEW);
+ 	int color_diff = DIFF_OPT_TST(data->o, COLOR_DIFF);
+ 	const char *ws = diff_get_color(color_diff, DIFF_WHITESPACE);
+ 	const char *reset = diff_get_color(color_diff, DIFF_RESET);
+ 	const char *set = diff_get_color(color_diff, DIFF_FILE_NEW);
  	char *err;
  
  	if (line[0] == '+') {
  		unsigned bad;
  		data->lineno++;
- 		bad = check_and_emit_line(line + 1, len - 1,
- 		    data->ws_rule, NULL, NULL, NULL, NULL);
+ 		if (!ws_blank_line(line + 1, len - 1, data->ws_rule))
+ 			data->trailing_blanks_start = 0;
+ 		else if (!data->trailing_blanks_start)
+ 			data->trailing_blanks_start = data->lineno;
+ 		if (is_conflict_marker(line + 1, len - 1)) {
+ 			data->status |= 1;
+ 			fprintf(data->o->file,
+ 				"%s:%d: leftover conflict marker\n",
+ 				data->filename, data->lineno);
+ 		}
+ 		bad = ws_check(line + 1, len - 1, data->ws_rule);
  		if (!bad)
  			return;
  		data->status |= bad;
  		err = whitespace_error_string(bad);
- 		fprintf(data->file, "%s:%d: %s.\n", data->filename, data->lineno, err);
+ 		fprintf(data->o->file, "%s:%d: %s.\n",
+ 			data->filename, data->lineno, err);
  		free(err);
- 		emit_line(data->file, set, reset, line, 1);
- 		(void)check_and_emit_line(line + 1, len - 1, data->ws_rule,
- 		    data->file, set, reset, ws);
- 	} else if (line[0] == ' ')
+ 		emit_line(data->o->file, set, reset, line, 1);
+ 		ws_check_emit(line + 1, len - 1, data->ws_rule,
+ 			      data->o->file, set, reset, ws);
+ 	} else if (line[0] == ' ') {
  		data->lineno++;
- 	else if (line[0] == '@') {
+ 		data->trailing_blanks_start = 0;
+ 	} else if (line[0] == '@') {
  		char *plus = strchr(line, '+');
  		if (plus)
  			data->lineno = strtol(plus, NULL, 10) - 1;
  		else
  			die("invalid diff");
+ 		data->trailing_blanks_start = 0;
  	}
  }
  
@@@ -1544,8 -1587,9 +1587,9 @@@ static void builtin_diffstat(const cha
  
  static void builtin_checkdiff(const char *name_a, const char *name_b,
  			      const char *attr_path,
- 			     struct diff_filespec *one,
- 			     struct diff_filespec *two, struct diff_options *o)
+ 			      struct diff_filespec *one,
+ 			      struct diff_filespec *two,
+ 			      struct diff_options *o)
  {
  	mmfile_t mf1, mf2;
  	struct checkdiff_t data;
@@@ -1557,13 -1601,18 +1601,18 @@@
  	data.xm.consume = checkdiff_consume;
  	data.filename = name_b ? name_b : name_a;
  	data.lineno = 0;
- 	data.color_diff = DIFF_OPT_TST(o, COLOR_DIFF);
+ 	data.o = o;
  	data.ws_rule = whitespace_rule(attr_path);
- 	data.file = o->file;
  
  	if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
  		die("unable to read files to diff");
  
+ 	/*
+ 	 * All the other codepaths check both sides, but not checking
+ 	 * the "old" side here is deliberate.  We are checking the newly
+ 	 * introduced changes, and as long as the "new" side is text, we
+ 	 * can and should check what it introduces.
+ 	 */
  	if (diff_filespec_is_binary(two))
  		goto free_and_return;
  	else {
@@@ -1577,6 -1626,12 +1626,12 @@@
  		ecb.outf = xdiff_outf;
  		ecb.priv = &data;
  		xdi_diff(&mf1, &mf2, &xpp, &xecfg, &ecb);
+ 
+ 		if (data.trailing_blanks_start) {
+ 			fprintf(o->file, "%s:%d: ends with blank lines.\n",
+ 				data.filename, data.trailing_blanks_start);
+ 			data.status = 1; /* report errors */
+ 		}
  	}
   free_and_return:
  	diff_free_filespec_data(one);