diff: ensure consistent diff behavior with ignore options
In git-diff, options like `-w` and `-I<regex>`, two files are considered equivalent under the specified "ignore" rules, even when they are not bit-for-bit identical. For options like `--raw`, `--name-status`, and `--name-only`, git-diff deliberately compares only the SHA values to determine whether two files are equivalent, for performance reasons. As a result, a file shown in `git diff --name-status` may not appear in `git diff --patch`. To quickly determine whether two files are equivalent, add a helper function diff_flush_patch_quietly() in diff.c. Add `.dry_run` field in `struct diff_options`. When `.dry_run` is true, builtin_diff() returns immediately upon finding any change. Call diff_flush_patch_quietly() to determine if we should flush `--raw`, `--name-only` or `--name-status` output. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Lidong Yan <yldhome2d2@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>maint
							parent
							
								
									866e6a391f
								
							
						
					
					
						commit
						b55e6d36eb
					
				
							
								
								
									
										64
									
								
								diff.c
								
								
								
								
							
							
						
						
									
										64
									
								
								diff.c
								
								
								
								
							|  | @ -2444,6 +2444,15 @@ static int fn_out_consume(void *priv, char *line, unsigned long len) | ||||||
| 	return 0; | 	return 0; | ||||||
| } | } | ||||||
|  |  | ||||||
|  | static int quick_consume(void *priv, char *line UNUSED, unsigned long len UNUSED) | ||||||
|  | { | ||||||
|  | 	struct emit_callback *ecbdata = priv; | ||||||
|  | 	struct diff_options *o = ecbdata->opt; | ||||||
|  |  | ||||||
|  | 	o->found_changes = 1; | ||||||
|  | 	return 1; | ||||||
|  | } | ||||||
|  |  | ||||||
| static void pprint_rename(struct strbuf *name, const char *a, const char *b) | static void pprint_rename(struct strbuf *name, const char *a, const char *b) | ||||||
| { | { | ||||||
| 	const char *old_name = a; | 	const char *old_name = a; | ||||||
|  | @ -3759,8 +3768,21 @@ static void builtin_diff(const char *name_a, | ||||||
|  |  | ||||||
| 		if (o->word_diff) | 		if (o->word_diff) | ||||||
| 			init_diff_words_data(&ecbdata, o, one, two); | 			init_diff_words_data(&ecbdata, o, one, two); | ||||||
| 		if (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume, | 		if (o->dry_run) { | ||||||
| 				  &ecbdata, &xpp, &xecfg)) | 			/* | ||||||
|  | 			 * Unlike the !dry_run case, we need to ignore the | ||||||
|  | 			 * return value from xdi_diff_outf() here, because | ||||||
|  | 			 * xdi_diff_outf() takes non-zero return from its | ||||||
|  | 			 * callback function as a sign of error and returns | ||||||
|  | 			 * early (which is why we return non-zero from our | ||||||
|  | 			 * callback, quick_consume()).  Unfortunately, | ||||||
|  | 			 * xdi_diff_outf() signals an error by returning | ||||||
|  | 			 * non-zero. | ||||||
|  | 			 */ | ||||||
|  | 			xdi_diff_outf(&mf1, &mf2, NULL, quick_consume, | ||||||
|  | 				      &ecbdata, &xpp, &xecfg); | ||||||
|  | 		} else if (xdi_diff_outf(&mf1, &mf2, NULL, fn_out_consume, | ||||||
|  | 					 &ecbdata, &xpp, &xecfg)) | ||||||
| 			die("unable to generate diff for %s", one->path); | 			die("unable to generate diff for %s", one->path); | ||||||
| 		if (o->word_diff) | 		if (o->word_diff) | ||||||
| 			free_diff_words_data(&ecbdata); | 			free_diff_words_data(&ecbdata); | ||||||
|  | @ -6150,6 +6172,22 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o) | ||||||
| 	run_diff(p, o); | 	run_diff(p, o); | ||||||
| } | } | ||||||
|  |  | ||||||
|  | /* return 1 if any change is found; otherwise, return 0 */ | ||||||
|  | static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o) | ||||||
|  | { | ||||||
|  | 	int saved_dry_run = o->dry_run; | ||||||
|  | 	int saved_found_changes = o->found_changes; | ||||||
|  | 	int ret; | ||||||
|  |  | ||||||
|  | 	o->dry_run = 1; | ||||||
|  | 	o->found_changes = 0; | ||||||
|  | 	diff_flush_patch(p, o); | ||||||
|  | 	ret = o->found_changes; | ||||||
|  | 	o->dry_run = saved_dry_run; | ||||||
|  | 	o->found_changes |= saved_found_changes; | ||||||
|  | 	return ret; | ||||||
|  | } | ||||||
|  |  | ||||||
| static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o, | static void diff_flush_stat(struct diff_filepair *p, struct diff_options *o, | ||||||
| 			    struct diffstat_t *diffstat) | 			    struct diffstat_t *diffstat) | ||||||
| { | { | ||||||
|  | @ -6778,8 +6816,15 @@ void diff_flush(struct diff_options *options) | ||||||
| 			     DIFF_FORMAT_CHECKDIFF)) { | 			     DIFF_FORMAT_CHECKDIFF)) { | ||||||
| 		for (i = 0; i < q->nr; i++) { | 		for (i = 0; i < q->nr; i++) { | ||||||
| 			struct diff_filepair *p = q->queue[i]; | 			struct diff_filepair *p = q->queue[i]; | ||||||
| 			if (check_pair_status(p)) |  | ||||||
| 				flush_one_pair(p, options); | 			if (!check_pair_status(p)) | ||||||
|  | 				continue; | ||||||
|  |  | ||||||
|  | 			if (options->flags.diff_from_contents && | ||||||
|  | 			    !diff_flush_patch_quietly(p, options)) | ||||||
|  | 				continue; | ||||||
|  |  | ||||||
|  | 			flush_one_pair(p, options); | ||||||
| 		} | 		} | ||||||
| 		separator++; | 		separator++; | ||||||
| 	} | 	} | ||||||
|  | @ -6831,19 +6876,10 @@ void diff_flush(struct diff_options *options) | ||||||
| 	if (output_format & DIFF_FORMAT_NO_OUTPUT && | 	if (output_format & DIFF_FORMAT_NO_OUTPUT && | ||||||
| 	    options->flags.exit_with_status && | 	    options->flags.exit_with_status && | ||||||
| 	    options->flags.diff_from_contents) { | 	    options->flags.diff_from_contents) { | ||||||
| 		/* |  | ||||||
| 		 * run diff_flush_patch for the exit status. setting |  | ||||||
| 		 * options->file to /dev/null should be safe, because we |  | ||||||
| 		 * aren't supposed to produce any output anyway. |  | ||||||
| 		 */ |  | ||||||
| 		diff_free_file(options); |  | ||||||
| 		options->file = xfopen("/dev/null", "w"); |  | ||||||
| 		options->close_file = 1; |  | ||||||
| 		options->color_moved = 0; |  | ||||||
| 		for (i = 0; i < q->nr; i++) { | 		for (i = 0; i < q->nr; i++) { | ||||||
| 			struct diff_filepair *p = q->queue[i]; | 			struct diff_filepair *p = q->queue[i]; | ||||||
| 			if (check_pair_status(p)) | 			if (check_pair_status(p)) | ||||||
| 				diff_flush_patch(p, options); | 				diff_flush_patch_quietly(p, options); | ||||||
| 			if (options->found_changes) | 			if (options->found_changes) | ||||||
| 				break; | 				break; | ||||||
| 		} | 		} | ||||||
|  |  | ||||||
							
								
								
									
										2
									
								
								diff.h
								
								
								
								
							
							
						
						
									
										2
									
								
								diff.h
								
								
								
								
							|  | @ -400,6 +400,8 @@ struct diff_options { | ||||||
| 	#define COLOR_MOVED_WS_ERROR (1<<0) | 	#define COLOR_MOVED_WS_ERROR (1<<0) | ||||||
| 	unsigned color_moved_ws_handling; | 	unsigned color_moved_ws_handling; | ||||||
|  |  | ||||||
|  | 	bool dry_run; | ||||||
|  |  | ||||||
| 	struct repository *repo; | 	struct repository *repo; | ||||||
| 	struct strmap *additional_path_headers; | 	struct strmap *additional_path_headers; | ||||||
|  |  | ||||||
|  |  | ||||||
|  | @ -648,6 +648,19 @@ test_expect_success 'diff -I<regex>: detect malformed regex' ' | ||||||
| 	test_grep "invalid regex given to -I: " error | 	test_grep "invalid regex given to -I: " error | ||||||
| ' | ' | ||||||
|  |  | ||||||
|  | test_expect_success 'diff -I<regex>: ignore matching file' ' | ||||||
|  | 	test_when_finished "git rm -f file1" && | ||||||
|  | 	test_seq 50 >file1 && | ||||||
|  | 	git add file1 && | ||||||
|  | 	test_seq 50 | sed -e "s/13/ten and three/" -e "s/^[124-9].*/& /" >file1 && | ||||||
|  |  | ||||||
|  | 	: >actual && | ||||||
|  | 	git diff --raw --ignore-blank-lines -I"ten.*e" -I"^[124-9]" >>actual && | ||||||
|  | 	git diff --name-only --ignore-blank-lines -I"ten.*e" -I"^[124-9]" >>actual && | ||||||
|  | 	git diff --name-status --ignore-blank-lines -I"ten.*e" -I"^[124-9]" >>actual && | ||||||
|  | 	test_grep ! "file1" actual | ||||||
|  | ' | ||||||
|  |  | ||||||
| # check_prefix <patch> <src> <dst> | # check_prefix <patch> <src> <dst> | ||||||
| # check only lines with paths to avoid dependency on exact oid/contents | # check only lines with paths to avoid dependency on exact oid/contents | ||||||
| check_prefix () { | check_prefix () { | ||||||
|  |  | ||||||
|  | @ -11,12 +11,8 @@ test_description='Test special whitespace in diff engine. | ||||||
| . "$TEST_DIRECTORY"/lib-diff.sh | . "$TEST_DIRECTORY"/lib-diff.sh | ||||||
|  |  | ||||||
| for opt_res in --patch --quiet -s --stat --shortstat --dirstat=lines \ | for opt_res in --patch --quiet -s --stat --shortstat --dirstat=lines \ | ||||||
| 	       --raw! --name-only! --name-status! | 	       --raw --name-only --name-status | ||||||
| do | do | ||||||
| 	opts=${opt_res%!} expect_failure= |  | ||||||
| 	test "$opts" = "$opt_res" || |  | ||||||
| 		expect_failure="test_expect_code 1" |  | ||||||
|  |  | ||||||
| 	test_expect_success "status with $opts (different)" ' | 	test_expect_success "status with $opts (different)" ' | ||||||
| 		echo foo >x && | 		echo foo >x && | ||||||
| 		git add x && | 		git add x && | ||||||
|  | @ -43,7 +39,7 @@ do | ||||||
| 		echo foo >x && | 		echo foo >x && | ||||||
| 		git add x && | 		git add x && | ||||||
| 		echo " foo" >x && | 		echo " foo" >x && | ||||||
| 		$expect_failure git diff -w $opts --exit-code x | 		git diff -w $opts --exit-code x | ||||||
| 	' | 	' | ||||||
| done | done | ||||||
|  |  | ||||||
|  |  | ||||||
|  | @ -28,9 +28,9 @@ | ||||||
|  * from an error internal to xdiff, xdiff itself will see that |  * from an error internal to xdiff, xdiff itself will see that | ||||||
|  * non-zero return and translate it to -1. |  * non-zero return and translate it to -1. | ||||||
|  * |  * | ||||||
|  * See "diff_grep" in diffcore-pickaxe.c for a trick to work around |  * See "diff_grep" in diffcore-pickaxe.c and "quick_consume" in diff.c | ||||||
|  * this, i.e. using the "consume_callback_data" to note the desired |  * for a trick to work around this, i.e. using the "consume_callback_data" | ||||||
|  * early return. |  * to note the desired early return. | ||||||
|  */ |  */ | ||||||
| typedef int (*xdiff_emit_line_fn)(void *, char *, unsigned long); | typedef int (*xdiff_emit_line_fn)(void *, char *, unsigned long); | ||||||
| typedef void (*xdiff_emit_hunk_fn)(void *data, | typedef void (*xdiff_emit_hunk_fn)(void *data, | ||||||
|  |  | ||||||
		Loading…
	
		Reference in New Issue
	
	 Lidong Yan
						Lidong Yan