Re: git diff --quiet exits with 1 on clean tree with CRLF conversions
- From
Mike Crowe <mac@mcrowe.com>
- Date
- Feb 20, 2017, 15:33 UTC
- Message-ID
- <20170220153322.GA8352@mcrowe.com>
- In-Reply-To
- <20170217221958.GA12163@mcrowe.com>
On Friday 17 February 2017 at 22:19:58 +0000, Mike Crowe wrote:
Show 37 quoted lines
> On Friday 17 February 2017 at 14:05:17 -0800, Junio C Hamano wrote: > > Mike Crowe <mac@mcrowe.com> writes: > > > > > If "git diff --quiet" finds it necessary to compare actual file contents, > > > and a file requires CRLF conversion, then it incorrectly exits with an exit > > > code of 1 even if there have been no changes. > > > > > > The patch below adds a test file that shows the problem. > > > > If "git diff" does not show any output and "git diff --exit-code" or > > "git diff --quiet" says there are differences, then it is a bug. > > > > I would however have expected that any culprit would involve a code > > that says "under QUICK option, we do not have to bother doing > > this". The part you quoted: > > > > > if (!DIFF_FILE_VALID(p->one) || /* (1) */ > > > !DIFF_FILE_VALID(p->two) || > > > (p->one->oid_valid && p->two->oid_valid) || > > > (p->one->mode != p->two->mode) || > > > diff_populate_filespec(p->one, CHECK_SIZE_ONLY) || > > > diff_populate_filespec(p->two, CHECK_SIZE_ONLY) || > > > (p->one->size != p->two->size) || > > > !diff_filespec_is_identical(p->one, p->two)) /* (2) */ > > > p->skip_stat_unmatch_result = 1; > > > > is used by "git diff" with and without "--quiet", afacr, so I > > suspect that the bug lies somewhere else. > > I can't say that I really understand the code fully, but it appears that > the first pass generates a queue of files that contain differences. The > result of the quiet diff comes from the size of that queue, > diff_queued_diff.nr, being non-zero in diffcore_std. I'm assuming that the > result of the noisy diff comes from actually comparing the files. > > But, I've only spent a short while looking so I may have got the wrong end > of the stick.
Tricking Git into checking the actual file contents (by passing --ignore-space-change for example) is sufficient to ensure that the exit status is never 1 when it should be zero. (Of course that option has other unwanted effects in this case.)
I think that if there's a risk that file contents will undergo conversion then this should force the diff to check the file contents. Something like:
diff --git a/diff.c b/diff.c index 051761b..bee1662 100644 --- a/diff.c +++ b/diff.c @@ -3413,13 +3413,14 @@ void diff_setup_done(struct diff_options *options) /* * Most of the time we can say "there are changes" * only by checking if there are changed paths, but - * --ignore-whitespace* options force us to look - * inside contents. + * --ignore-whitespace* options or text conversion + * force us to look inside contents. */ if (DIFF_XDL_TST(options, IGNORE_WHITESPACE) || DIFF_XDL_TST(options, IGNORE_WHITESPACE_CHANGE) || - DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL)) + DIFF_XDL_TST(options, IGNORE_WHITESPACE_AT_EOL) || + DIFF_OPT_TST(options, ALLOW_TEXTCONV)) DIFF_OPT_SET(options, DIFF_FROM_CONTENTS); else DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS); This solves the problem for me and my test case now passes. Unfortunately it breaks the 'removing and adding subproject' test case in t3040-subprojects-basic at the line: test_expect_code 1 git diff -M --name-status --exit-code HEAD^ HEAD presumably because after the rename has been detected the file contents are identical. :( A rename of a single file appears to still be handled correctly. Mike.