[PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Tim Henigan]

When running 'git diff --quiet <file1> <file2>', if file1 or file2
is outside the repository, it will exit(0) even if the files differ.
It should exit(1) when they differ.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

This was the least invasive fix that I found.  I considered adding
the following when the '--quiet' option is parsed instead:

+   DIFF_OPT_SET(options, EXIT_WITH_STATUS)
+   DIFF_OPT_SET(options, DIFF_FROM_CONTENTS)


 diff.c                | 5 +++--
 t/t4035-diff-quiet.sh | 5 +++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 77edd50..b1d74fe 100644
--- a/diff.c
+++ b/diff.c
@@ -4432,9 +4432,10 @@ void diff_flush(struct diff_options *options)
 		separator++;
 	}
 
-	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
+	if ((output_format & DIFF_FORMAT_NO_OUTPUT &&
 	    DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&
-	    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
+	    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) ||
+		DIFF_OPT_TST(options, QUICK)) {
 		/*
 		 * run diff_flush_patch for the exit status. setting
 		 * options->file to /dev/null should be safe, becaue we
diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
index cdb9202..e14e676 100755
--- a/t/t4035-diff-quiet.sh
+++ b/t/t4035-diff-quiet.sh
@@ -77,4 +77,9 @@ test_expect_success 'git diff-index --cached HEAD' '
 	}
 '
 
+test_expect_success 'git diff <tracked file> <file outside repo>' '
+	git diff --quiet c /dev/null
+	test $? = 1
+'
+
 test_done
-- 
1.7.11.rc3.6.g501bf14

Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Jeff King]

On Fri, Jun 15, 2012 at 01:31:03PM -0400, Tim Henigan wrote:

> When running 'git diff --quiet <file1> <file2>', if file1 or file2
> is outside the repository, it will exit(0) even if the files differ.
> It should exit(1) when they differ.

>From your description, I would expect the fix to be in builtin/diff.c,
or in diff-no-index.c, since that is where the code paths diverge.

> This was the least invasive fix that I found.  I considered adding
> the following when the '--quiet' option is parsed instead:
> 
> +   DIFF_OPT_SET(options, EXIT_WITH_STATUS)
> +   DIFF_OPT_SET(options, DIFF_FROM_CONTENTS)

We already set EXIT_WITH_STATUS when we see --quiet (we just do it a
little later, during diff_setup_done). We would not want to set
DIFF_FROM_CONTENTS all the time with --quiet. The point of that flag is
"we cannot know just from seeing the path sha1s whether they are
different or not, because we are doing content-level munging" (for
example, things like ignoring whitespace changes).

So we would not want to always set it whenever --quiet is given, because
it means we must do a lot of extra work comparing file content.

> diff --git a/diff.c b/diff.c
> index 77edd50..b1d74fe 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4432,9 +4432,10 @@ void diff_flush(struct diff_options *options)
>  		separator++;
>  	}
>  
> -	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
> +	if ((output_format & DIFF_FORMAT_NO_OUTPUT &&
>  	    DIFF_OPT_TST(options, EXIT_WITH_STATUS) &&
> -	    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) {
> +	    DIFF_OPT_TST(options, DIFF_FROM_CONTENTS)) ||
> +		DIFF_OPT_TST(options, QUICK)) {
>  		/*
>  		 * run diff_flush_patch for the exit status. setting
>  		 * options->file to /dev/null should be safe, becaue we

And this is equally bad, because it means that --quiet gets much slower
for _all_ cases, not just the no-index case.

I suspect what you actually want is to set DIFF_FROM_CONTENTS in the
no-index case, since we by definition do not have a pair of sha1s to
compare. But it may also be that diff.c could detect this case
automatically. I'd have to look closer.

-Peff

Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Jeff King]

On Fri, Jun 15, 2012 at 02:40:30PM -0400, Jeff King wrote:

> I suspect what you actually want is to set DIFF_FROM_CONTENTS in the
> no-index case, since we by definition do not have a pair of sha1s to
> compare. But it may also be that diff.c could detect this case
> automatically. I'd have to look closer.

This works for me:

diff --git a/diff.c b/diff.c
index 77edd50..27231d2 100644
--- a/diff.c
+++ b/diff.c
@@ -3228,7 +3228,8 @@ int diff_setup_done(struct diff_options *options)
 
 	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, NO_INDEX))
 		DIFF_OPT_SET(options, DIFF_FROM_CONTENTS);
 	else
 		DIFF_OPT_CLR(options, DIFF_FROM_CONTENTS);

One could also set DIFF_FROM_CONTENTS manually in diff_on_index, but
note that diff_setup_done unsets it (which seems odd; who would have set
it in the first place if they did not want to override it? I think that
else clause can simply be dropped).

-Peff

PS Your test should probably be spelled with test_expect_code, like:

diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
index cdb9202..0b83235 100755
--- a/t/t4035-diff-quiet.sh
+++ b/t/t4035-diff-quiet.sh
@@ -77,4 +77,8 @@ test_expect_success 'git diff-index --cached HEAD' '
 	}
 '
 
+test_expect_success 'git diff <tracked file> <file outside repo>' '
+	test_expect_code 1 git diff --quiet c /dev/null
+'
+
 test_done

Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> This was the least invasive fix that I found.

I think this breaks a normal case of comparing revisions and tracked
contents in a big way.

The "quick" thing is meant to notice any difference in the paths
involved (e.g. "git diff maint master -- t/") at the blob object
name level without having to look at the contents when there is no
DIFF_FROM_CONTENTS processing is needed.  We call into the more
expensive diff_flush_patch() codepath only when things like "ignore
whitespace change" is given, in which case we would need to compare
the contents.

Your patch seems to be making us go through diff_flush_patch()
codepath unconditionally, and it looks like it is only to sweep some
other breakage in diff-no-index codepath under the rug.

Have you looked at how "git diff --quiet HEAD^" (without any other
option) for tracked files notices that there is a difference and
exit with non-zero status?  Is it doing something wrong?  Otherwise
why can't the no-index codepath do the same thing?

I think the following may be a lot closer to the correct fix; I
didn't test many combinations of options with it, though.

 diff-no-index.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index f0b0010..ed74e27 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -172,7 +172,7 @@ void diff_no_index(struct rev_info *revs,
 		   int argc, const char **argv,
 		   int nongit, const char *prefix)
 {
-	int i;
+	int i, result;
 	int no_index = 0;
 	unsigned options = 0;
 
@@ -273,5 +273,6 @@ void diff_no_index(struct rev_info *revs,
 	 * The return code for --no-index imitates diff(1):
 	 * 0 = no changes, 1 = changes, else error
 	 */
-	exit(revs->diffopt.found_changes);
+	result = !!diff_result_code(&revs->diffopt, 0);
+	exit(result);
 }

Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Jeff King]

On Fri, Jun 15, 2012 at 11:45:47AM -0700, Junio C Hamano wrote:

> I think the following may be a lot closer to the correct fix; I
> didn't test many combinations of options with it, though.
> 
>  diff-no-index.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/diff-no-index.c b/diff-no-index.c
> index f0b0010..ed74e27 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -172,7 +172,7 @@ void diff_no_index(struct rev_info *revs,
>  		   int argc, const char **argv,
>  		   int nongit, const char *prefix)
>  {
> -	int i;
> +	int i, result;
>  	int no_index = 0;
>  	unsigned options = 0;
>  
> @@ -273,5 +273,6 @@ void diff_no_index(struct rev_info *revs,
>  	 * The return code for --no-index imitates diff(1):
>  	 * 0 = no changes, 1 = changes, else error
>  	 */
> -	exit(revs->diffopt.found_changes);
> +	result = !!diff_result_code(&revs->diffopt, 0);
> +	exit(result);

Hmm. This works because is checks HAS_CHANGES instead of found_changes.
I was somewhat surprised that the former works at all, but it is because
diffcore_std sets it if there is anything in diff_queued_diff. Which
makes sense; found_changes seems to be only about communicating changes
up from builtin_diff to the rest of the diff code; nobody should be
reading it outside of there.

Which left me to wonder how we tell when two files are actually the
same. The answer is that diffcore_skip_stat_unmatch actually loads and
memcmps each pair, removing ones that are identical. And diff_no_index
always sets skip_stat_unmatch, whether diff.autorefreshindex is on or
not.

So the patch I posted elsewhere in the thread is not right; we do not
need to do diff_flush_patch to actually compare, because the
stat_unmatch code will have done everything we want (unless
DIFF_FROM_CONTENTS really is set). And the bug is purely one of looking
at the wrong output flag.

-Peff

Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Tim Henigan]

On Fri, Jun 15, 2012 at 3:37 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jun 15, 2012 at 11:45:47AM -0700, Junio C Hamano wrote:
>
>> I think this breaks a normal case of comparing revisions and tracked
>> contents in a big way.

I didn't understand the ramifications of making the change where I
did.  I appreciate you and Jeff taking the time to point out the
problem (and suggest better solutions).


>> I think the following may be a lot closer to the correct fix; I
>> didn't test many combinations of options with it, though.
>>
>>  diff-no-index.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/diff-no-index.c b/diff-no-index.c
>> index f0b0010..ed74e27 100644
>> --- a/diff-no-index.c
>> +++ b/diff-no-index.c
>> @@ -172,7 +172,7 @@ void diff_no_index(struct rev_info *revs,
>>                  int argc, const char **argv,
>>                  int nongit, const char *prefix)
>>  {
>> -     int i;
>> +     int i, result;
>>       int no_index = 0;
>>       unsigned options = 0;
>>
>> @@ -273,5 +273,6 @@ void diff_no_index(struct rev_info *revs,
>>        * The return code for --no-index imitates diff(1):
>>        * 0 = no changes, 1 = changes, else error
>>        */
>> -     exit(revs->diffopt.found_changes);
>> +     result = !!diff_result_code(&revs->diffopt, 0);
>> +     exit(result);

I assume the '!!' before 'diff_result_code' is a typo.  Reverting my
changes and putting this in solves the problem.

...

> So the patch I posted elsewhere in the thread is not right; we do not
> need to do diff_flush_patch to actually compare, because the
> stat_unmatch code will have done everything we want (unless
> DIFF_FROM_CONTENTS really is set). And the bug is purely one of looking
> at the wrong output flag.

I will send v2 with the change to 'diff-no-index.c' suggested by
Junio.  I will also include the 'test_expect_code' improvement
suggested by Jeff.

Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

>>> @@ -273,5 +273,6 @@ void diff_no_index(struct rev_info *revs,
>>>        * The return code for --no-index imitates diff(1):
>>>        * 0 = no changes, 1 = changes, else error
>>>        */
>>> -     exit(revs->diffopt.found_changes);
>>> +     result = !!diff_result_code(&revs->diffopt, 0);
>>> +     exit(result);
>
> I assume the '!!' before 'diff_result_code' is a typo.

Not a typo.  I meant to use that idiom to turn 0 or not into
boolean, as diff_result_code() can return values other than 0 or 1.

Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Jeff King]

On Fri, Jun 15, 2012 at 01:08:32PM -0700, Junio C Hamano wrote:

> Tim Henigan <tim.henigan@gmail.com> writes:
> 
> >>> @@ -273,5 +273,6 @@ void diff_no_index(struct rev_info *revs,
> >>>        * The return code for --no-index imitates diff(1):
> >>>        * 0 = no changes, 1 = changes, else error
> >>>        */
> >>> -     exit(revs->diffopt.found_changes);
> >>> +     result = !!diff_result_code(&revs->diffopt, 0);
> >>> +     exit(result);
> >
> > I assume the '!!' before 'diff_result_code' is a typo.
> 
> Not a typo.  I meant to use that idiom to turn 0 or not into
> boolean, as diff_result_code() can return values other than 0 or 1.

I wonder if that is a good idea, though. AFAICT, diff_result_code will
only return a different exit code if "--check" is used. If we pass along
the exit code fully, then:

  1. If --check is not used, we will be diff(1)-compatible.

  2. If --check is used, then we will not be compatible with diff(1) in
     our exit code. But diff(1) does not have --check in the first
     place, so there is no point in us trying to be a drop-in
     replacement.

-Peff

Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   2. If --check is used, then we will not be compatible with diff(1) in
>      our exit code. But diff(1) does not have --check in the first
>      place, so there is no point in us trying to be a drop-in
>      replacement.

Sounds sensible. Thanks.

Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> I will send v2 with the change to 'diff-no-index.c' suggested by
> Junio.  I will also include the 'test_expect_code' improvement
> suggested by Jeff.

Also the test script shouldn't be just testing a simplest case, I
would think.

For example, comparing two files with "a b c" and "a  b c" in them
with "--quiet" should yield "They are different!" while running the
same comparison with "--quiet -w" should say "They are the same!",
no?

Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Tim Henigan]

On Fri, Jun 15, 2012 at 4:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> I will send v2 with the change to 'diff-no-index.c' suggested by
>> Junio.  I will also include the 'test_expect_code' improvement
>> suggested by Jeff.
>
> Also the test script shouldn't be just testing a simplest case, I
> would think.
>
> For example, comparing two files with "a b c" and "a  b c" in them
> with "--quiet" should yield "They are different!" while running the
> same comparison with "--quiet -w" should say "They are the same!",
> no?

To obtain full coverage, it seems we need to test:
    1) git diff --quiet <file in repo> <file outside repo>
    2) git diff --quiet <file outside repo> <file outside repo>

for each of the following options:
    a) no additional options
    b) --ignore-space-at-eol
    c) --ignore-all-space

These seem like the only diff options that should be tested...am I
missing anything?

Because some of the tests require one directory that is a repo and one
directory that is not, it seems best to create a new test file with
$TEST_NO_CREATE_REPO set.  Then the setup function can create both the
repo and the plain directory.

So, I am thinking of adding 't/t4054-diff-outside-repo.sh' that does
the following:

  1) setup (includes a cd into the repo directory)
  2) git diff --quiet, one outside repo, matching files
  3) git diff --quiet, one outside repo, different files
  4) git diff --quiet, one outside repo, ignore trailing whitespace
  5) git diff --quiet, one outside repo, ignore all whitespace
  6) git diff --quiet, both outside repo, matching files
  7) git diff --quiet, both outside repo, different files
  8) git diff --quiet, both outside repo, ignore trailing whitespace
  9) git diff --quiet, both outside repo, ignore all whitespace
  10) cleanup (cd back to $HOME test directory)

Does this sound reasonable?

Re: [PATCH] diff: exit(1) if 'diff --quiet <repo file> <external file>' finds changes

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> To obtain full coverage, it seems we need to test:
>     1) git diff --quiet <file in repo> <file outside repo>
>     2) git diff --quiet <file outside repo> <file outside repo>
>
> for each of the following options:
>     a) no additional options
>     b) --ignore-space-at-eol
>     c) --ignore-all-space
>
> These seem like the only diff options that should be tested...am I
> missing anything?
>
> Because some of the tests require one directory that is a repo and one
> directory that is not, it seems best to create a new test file with
> $TEST_NO_CREATE_REPO set.  Then the setup function can create both the
> repo and the plain directory.

It is probably overkill to add a new test that only to add 3 (the
"two paths outside repo" among the 6 = 2 * 3 combinations above).
Inside t4035, you could dig new directory two levels deep, chdir to
it and set GIT_CEILING_DIRECTORY to its first level and run test,
no?  Something along these lines (the diff invoked would obviously
be not between foo and bar)...

 t/t4035-diff-quiet.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
index cdb9202..e7065cc 100755
--- a/t/t4035-diff-quiet.sh
+++ b/t/t4035-diff-quiet.sh
@@ -77,4 +77,16 @@ test_expect_success 'git diff-index --cached HEAD' '
 	}
 '
 
+test_expect_success 'git diff-no-index outside repository' '
+	mkdir -p not/here &&
+	(
+		cd not/here &&
+		GIT_CEILING_DIRECTORIES=$TRASH_DIRECTORY/not &&
+		export GIT_CEILING_DIRECTORIES &&
+		echo foo >foo &&
+		echo bar >bar &&
+		git diff foo bar
+	)
+'
+
 test_done