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

[PATCH v3] diff-no-index: 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>
---


v3 improves the test coverage to include variations of 'diff --quiet'
where one or both of the files are outside the repository. Tests for
'--ignore-space-at-eol' and '--ignore-all-space' are included as well.


 diff-no-index.c       |  3 +-
 t/t4035-diff-quiet.sh | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index f0b0010..b935d2a 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -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);
+	int result = diff_result_code(&revs->diffopt, 0);
+	exit(result);
 }
diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
index cdb9202..33d8980 100755
--- a/t/t4035-diff-quiet.sh
+++ b/t/t4035-diff-quiet.sh
@@ -10,7 +10,22 @@ test_expect_success 'setup' '
 	git commit -m first &&
 	echo 2 >b &&
 	git add . &&
-	git commit -a -m second
+	git commit -a -m second &&
+	mkdir -p test-outside/repo && (
+		cd test-outside/repo &&
+		git init &&
+		echo "1 1" > a &&
+		git add . &&
+		git commit -m 1
+	) &&
+	mkdir -p test-outside/no-repo && (
+		cd test-outside/no-repo &&
+		echo "1 1" >a &&
+		echo "1 1" >matching-file &&
+		echo "1 1 " >trailing-space &&
+		echo "1   1" >extra-space &&
+		echo "2" >never-match
+	)
 '
 
 test_expect_success 'git diff-tree HEAD^ HEAD' '
@@ -77,4 +92,66 @@ test_expect_success 'git diff-index --cached HEAD' '
 	}
 '
 
+test_expect_success 'git diff, one file outside repo' '
+	(
+		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd test-outside/repo &&
+		test_expect_code 0 git diff --quiet a "$TRASH_DIRECTORY/test-outside/no-repo/matching-file" &&
+		test_expect_code 1 git diff --quiet a "$TRASH_DIRECTORY/test-outside/no-repo/extra-space"
+	)
+'
+
+test_expect_success 'git diff, both files outside repo' '
+	(
+		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd test-outside/no-repo &&
+		test_expect_code 0 git diff --quiet a matching-file &&
+		test_expect_code 1 git diff --quiet a extra-space
+	)
+'
+
+test_expect_success 'git diff --ignore-space-at-eol, one file outside repo' '
+	(
+		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd test-outside/repo &&
+		test_expect_code 0 git diff --quiet --ignore-space-at-eol a "$TRASH_DIRECTORY/test-outside/no-repo/trailing-space" &&
+		test_expect_code 1 git diff --quiet --ignore-space-at-eol a "$TRASH_DIRECTORY/test-outside/no-repo/extra-space"
+	)
+'
+
+test_expect_success 'git diff --ignore-space-at-eol, both files outside repo' '
+	(
+		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd test-outside/no-repo &&
+		test_expect_code 0 git diff --quiet --ignore-space-at-eol a trailing-space &&
+		test_expect_code 1 git diff --quiet --ignore-space-at-eol a extra-space
+	)
+'
+
+test_expect_success 'git diff --ignore-all-space, one file outside repo' '
+	(
+		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd test-outside/repo &&
+		test_expect_code 0 git diff --quiet --ignore-all-space a "$TRASH_DIRECTORY/test-outside/no-repo/trailing-space" &&
+		test_expect_code 0 git diff --quiet --ignore-all-space a "$TRASH_DIRECTORY/test-outside/no-repo/extra-space" &&
+		test_expect_code 1 git diff --quiet --ignore-all-space a "$TRASH_DIRECTORY/test-outside/no-repo/never-match"
+	)
+'
+
+test_expect_success 'git diff --ignore-all-space, both files outside repo' '
+	(
+		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd test-outside/no-repo &&
+		test_expect_code 0 git diff --quiet --ignore-all-space a trailing-space &&
+		test_expect_code 0 git diff --quiet --ignore-all-space a extra-space &&
+		test_expect_code 1 git diff --quiet --ignore-all-space a never-match
+	)
+'
+
 test_done
-- 
1.7.11.rc3.6.g5532165

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

[Jeff King]

On Mon, Jun 18, 2012 at 03:28:24PM -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.

Can we explain in the commit message a bit about why the patch works? If
I hadn't just dug into this a few days ago, the patch would be somewhat
confusing. Maybe something like:

  The problem comes from checking diff_options's found_changes member to
  see whether any changes were found. This flag is set only when we
  actually run xdiff and it finds a change. However, the diff machinery
  will optimize out the actual xdiff call when it is not necessary
  (i.e., when we are doing a straight byte-for-byte comparison and do
  not care about the output). As a result, this flag was never set, and
  we must check the HAS_CHANGES flag instead, just like the regular
  index-aware diff code paths do.

I am also tempted to suggest that found_changes be renamed to
xdiff_found_changes or something similar, because it really is quite
misleading as-is.

> diff --git a/diff-no-index.c b/diff-no-index.c
> index f0b0010..b935d2a 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -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);
> +	int result = diff_result_code(&revs->diffopt, 0);
> +	exit(result);

This is a declaration-after-statement, no? We try to stick to C89, which
does not allow this.

I don't see any reason why the extra variable could not be removed
entirely:

  exit(diff_result_code(&revs->diffopt, 0));

-Peff

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

[Junio C Hamano]

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

> 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>
> ---
>
>
> v3 improves the test coverage to include variations of 'diff --quiet'
> where one or both of the files are outside the repository. Tests for
> '--ignore-space-at-eol' and '--ignore-all-space' are included as well.
>
>
>  diff-no-index.c       |  3 +-
>  t/t4035-diff-quiet.sh | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 80 insertions(+), 2 deletions(-)
>
> diff --git a/diff-no-index.c b/diff-no-index.c
> index f0b0010..b935d2a 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> @@ -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);
> +	int result = diff_result_code(&revs->diffopt, 0);
> +	exit(result);
>  }

Decl-after-stmt.

> diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
> index cdb9202..33d8980 100755
> --- a/t/t4035-diff-quiet.sh
> +++ b/t/t4035-diff-quiet.sh
> @@ -10,7 +10,22 @@ test_expect_success 'setup' '
>  	git commit -m first &&
>  	echo 2 >b &&
>  	git add . &&
> -	git commit -a -m second
> +	git commit -a -m second &&
> +	mkdir -p test-outside/repo && (
> +		cd test-outside/repo &&
> +		git init &&
> +		echo "1 1" > a &&

Please drop extra SP between ">" and "a".

> +		git add . &&
> +		git commit -m 1
> +	) &&
> +	mkdir -p test-outside/no-repo && (
> +		cd test-outside/no-repo &&
> +		echo "1 1" >a &&
> +		echo "1 1" >matching-file &&
> +		echo "1 1 " >trailing-space &&
> +		echo "1   1" >extra-space &&
> +		echo "2" >never-match
> +	)

The inspiration of using CEILING comes from the existing t7810-grep
test, and I would have preferred if you used the same non/git for a
non-git repository for easier greppability ("git grep CEIL t/" to
notice the use of the technique and then "git grep non/git t/" to
verify, for example).

>  '
>  
>  test_expect_success 'git diff-tree HEAD^ HEAD' '
> @@ -77,4 +92,66 @@ test_expect_success 'git diff-index --cached HEAD' '
>  	}
>  '
>  
> +test_expect_success 'git diff, one file outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&

Do you even need these two lines for this test?  your test runs
inside test-outside/repo that _is_ a git repository, and that
repository knows that ../no-repo is not part of it already.

> +		cd test-outside/repo &&
> +		test_expect_code 0 git diff --quiet a "$TRASH_DIRECTORY/test-outside/no-repo/matching-file" &&
> +		test_expect_code 1 git diff --quiet a "$TRASH_DIRECTORY/test-outside/no-repo/extra-space"
> +	)
> +'
> +
> +test_expect_success 'git diff, both files outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd test-outside/no-repo &&
> +		test_expect_code 0 git diff --quiet a matching-file &&
> +		test_expect_code 1 git diff --quiet a extra-space

This one does need the ceiling to prevent git from finding the trash
directory.

> +	)
> +'
> +
> +test_expect_success 'git diff --ignore-space-at-eol, one file outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&

This one does not, I think (please correct me; I am not being very careful).

> +		cd test-outside/repo &&
> +		test_expect_code 0 git diff --quiet --ignore-space-at-eol a "$TRASH_DIRECTORY/test-outside/no-repo/trailing-space" &&
> +		test_expect_code 1 git diff --quiet --ignore-space-at-eol a "$TRASH_DIRECTORY/test-outside/no-repo/extra-space"
> +	)
> +'
> +
> +test_expect_success 'git diff --ignore-space-at-eol, both files outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd test-outside/no-repo &&
> +		test_expect_code 0 git diff --quiet --ignore-space-at-eol a trailing-space &&
> +		test_expect_code 1 git diff --quiet --ignore-space-at-eol a extra-space
> +	)
> +'
> +
> +test_expect_success 'git diff --ignore-all-space, one file outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&

This one does not, I think (please correct me; I am not being very careful).

> +		cd test-outside/repo &&
> +		test_expect_code 0 git diff --quiet --ignore-all-space a "$TRASH_DIRECTORY/test-outside/no-repo/trailing-space" &&
> +		test_expect_code 0 git diff --quiet --ignore-all-space a "$TRASH_DIRECTORY/test-outside/no-repo/extra-space" &&
> +		test_expect_code 1 git diff --quiet --ignore-all-space a "$TRASH_DIRECTORY/test-outside/no-repo/never-match"
> +	)
> +'
> +
> +test_expect_success 'git diff --ignore-all-space, both files outside repo' '
> +	(
> +		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
> +		export GIT_CEILING_DIRECTORIES &&
> +		cd test-outside/no-repo &&
> +		test_expect_code 0 git diff --quiet --ignore-all-space a trailing-space &&
> +		test_expect_code 0 git diff --quiet --ignore-all-space a extra-space &&
> +		test_expect_code 1 git diff --quiet --ignore-all-space a never-match
> +	)
> +'
> +
>  test_done

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

[Tim Henigan]

On Mon, Jun 18, 2012 at 4:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> 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.
>> -     exit(revs->diffopt.found_changes);
>> +     int result = diff_result_code(&revs->diffopt, 0);
>> +     exit(result);
>>  }
>
> Decl-after-stmt.

Will eliminate intermediate variable in v4.  Thanks to both you and
Jeff for pointing this out.


>> +             echo "1 1" > a &&
>
> Please drop extra SP between ">" and "a".

Will fix in v4.


>> +             git add . &&
>> +             git commit -m 1
>> +     ) &&
>> +     mkdir -p test-outside/no-repo && (
>> +             cd test-outside/no-repo &&
>> +             echo "1 1" >a &&
>> +             echo "1 1" >matching-file &&
>> +             echo "1 1 " >trailing-space &&
>> +             echo "1   1" >extra-space &&
>> +             echo "2" >never-match
>> +     )
>
> The inspiration of using CEILING comes from the existing t7810-grep
> test, and I would have preferred if you used the same non/git for a
> non-git repository for easier greppability ("git grep CEIL t/" to
> notice the use of the technique and then "git grep non/git t/" to
> verify, for example).

Okay.  I still need the non/git directory to be outside the test
repo's path, so the new layout will be:

    $TRASH_DIRECTORY/
        test-outside/
            repo/
            non/git/

This adds an extra layer to the non git paths, but won't cause any problems.


>> +test_expect_success 'git diff, one file outside repo' '
>> +     (
>> +             GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY/test-outside" &&
>> +             export GIT_CEILING_DIRECTORIES &&
>
> Do you even need these two lines for this test?  your test runs
> inside test-outside/repo that _is_ a git repository, and that
> repository knows that ../no-repo is not part of it already.

You are correct, CEILING does not need to be set for tests where one
file is inside 'test-outside/repo'.

As a side note, I found that these tests fail if a relative path is
used for the file in 'non/git'.  In other words, this passes:

    test_expect_code 0 git diff --quiet a
"$TRASH_DIRECTORY/test-outside/non/git/matching-file"

but this fails:

    test_expect_code 0 git diff --quiet a ../non/git/matching-file

This surprised me, but I have not investigated any further.

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

[Jeff King]

On Tue, Jun 19, 2012 at 09:05:40AM -0400, Tim Henigan wrote:

> As a side note, I found that these tests fail if a relative path is
> used for the file in 'non/git'.  In other words, this passes:
> 
>     test_expect_code 0 git diff --quiet a
> "$TRASH_DIRECTORY/test-outside/non/git/matching-file"
> 
> but this fails:
> 
>     test_expect_code 0 git diff --quiet a ../non/git/matching-file
> 
> This surprised me, but I have not investigated any further.

The problem is that path_outside_repo in diff-no-index.c does not bother
handling relative paths at all, and just assumes they are inside the
repository. This is obviously not true if the path starts with "..", in
which case you would need to compare the number of ".." with the current
depth in the repository.

prefix_path already does this (and is what generates the later
"../non/git/matching-file is not in the repository" message). We could
perhaps get rid of path_outside_repo and just re-use prefix_path's
logic, something like (not tested):

diff --git a/cache.h b/cache.h
index 0b7ddee..0736bfb 100644
--- a/cache.h
+++ b/cache.h
@@ -411,6 +411,7 @@ extern const char *prefix_filename(const char *prefix, int len, const char *path
 extern int check_filename(const char *prefix, const char *name);
 extern void verify_filename(const char *prefix, const char *name);
 extern void verify_non_filename(const char *prefix, const char *name);
+extern int path_inside_repo(const char *prefix, const char *path);
 
 #define INIT_DB_QUIET 0x0001
 
diff --git a/diff-no-index.c b/diff-no-index.c
index 6911196..9a1b459 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -151,23 +151,6 @@ static int queue_diff(struct diff_options *o,
 	}
 }
 
-static int path_outside_repo(const char *path)
-{
-	const char *work_tree;
-	size_t len;
-
-	if (!is_absolute_path(path))
-		return 0;
-	work_tree = get_git_work_tree();
-	if (!work_tree)
-		return 1;
-	len = strlen(work_tree);
-	if (strncmp(path, work_tree, len) ||
-	    (path[len] != '\0' && path[len] != '/'))
-		return 1;
-	return 0;
-}
-
 void diff_no_index(struct rev_info *revs,
 		   int argc, const char **argv,
 		   int nongit, const char *prefix)
@@ -197,8 +180,8 @@ void diff_no_index(struct rev_info *revs,
 		 * a colourful "diff" replacement.
 		 */
 		if ((argc != i + 2) ||
-		    (!path_outside_repo(argv[i]) &&
-		     !path_outside_repo(argv[i+1])))
+		    (path_inside_repo(prefix, argv[i]) &&
+		     path_inside_repo(prefix, argv[i+1])))
 			return;
 	}
 	if (argc != i + 2)
diff --git a/setup.c b/setup.c
index 731851a..2cfa037 100644
--- a/setup.c
+++ b/setup.c
@@ -4,7 +4,7 @@
 static int inside_git_dir = -1;
 static int inside_work_tree = -1;
 
-char *prefix_path(const char *prefix, int len, const char *path)
+static char *prefix_path_gently(const char *prefix, int len, const char *path)
 {
 	const char *orig = path;
 	char *sanitized;
@@ -31,7 +31,8 @@ char *prefix_path(const char *prefix, int len, const char *path)
 		if (strncmp(sanitized, work_tree, len) ||
 		    (len > root_len && sanitized[len] != '\0' && sanitized[len] != '/')) {
 		error_out:
-			die("'%s' is outside repository", orig);
+			free(sanitized);
+			return NULL;
 		}
 		if (sanitized[len] == '/')
 			len++;
@@ -40,6 +41,25 @@ char *prefix_path(const char *prefix, int len, const char *path)
 	return sanitized;
 }
 
+char *prefix_path(const char *prefix, int len, const char *path)
+{
+	char *r = prefix_path_gently(prefix, len, path);
+	if (!r)
+		die("'%s' is outside repository", path);
+	return r;
+}
+
+int path_inside_repo(const char *prefix, const char *path)
+{
+	int len = prefix ? strlen(prefix) : 0;
+	char *r = prefix_path_gently(prefix, len, path);
+	if (r) {
+		free(r);
+		return 1;
+	}
+	return 0;
+}
+
 int check_filename(const char *prefix, const char *arg)
 {
 	const char *name;

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

[Tim Henigan]

On Tue, Jun 19, 2012 at 9:58 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Jun 19, 2012 at 09:05:40AM -0400, Tim Henigan wrote:
>
>> As a side note, I found that these tests fail if a relative path is
>> used for the file in 'non/git'.  In other words, this passes:
>>
>>     test_expect_code 0 git diff --quiet a
>> "$TRASH_DIRECTORY/test-outside/non/git/matching-file"
>>
>> but this fails:
>>
>>     test_expect_code 0 git diff --quiet a ../non/git/matching-file
>>
>> This surprised me, but I have not investigated any further.
>
> The problem is that path_outside_repo in diff-no-index.c does not bother
> handling relative paths at all, and just assumes they are inside the
> repository. This is obviously not true if the path starts with "..", in
> which case you would need to compare the number of ".." with the current
> depth in the repository.
>
> prefix_path already does this (and is what generates the later
> "../non/git/matching-file is not in the repository" message). We could
> perhaps get rid of path_outside_repo and just re-use prefix_path's
> logic, something like (not tested):

With your patch applied, I was able to use relative paths in my tests.
 I also confirmed that all the t4*.sh tests pass.

For what its worth, your patch looks correct to me.  Existing
consumers of 'prefix_path' should get the same results as before and
the one added xmalloc is paired with a free.

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

[Tim Henigan]

On Tue, Jun 19, 2012 at 12:47 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
> On Tue, Jun 19, 2012 at 9:58 AM, Jeff King <peff@peff.net> wrote:
>> On Tue, Jun 19, 2012 at 09:05:40AM -0400, Tim Henigan wrote:
>>
>>> As a side note, I found that these tests fail if a relative path is
>>> used for the file in 'non/git'.  In other words, this passes:
>>>
>>>     test_expect_code 0 git diff --quiet a
>>> "$TRASH_DIRECTORY/test-outside/non/git/matching-file"
>>>
>>> but this fails:
>>>
>>>     test_expect_code 0 git diff --quiet a ../non/git/matching-file
>>>
>>> This surprised me, but I have not investigated any further.
>>
>> The problem is that path_outside_repo in diff-no-index.c does not bother
>> handling relative paths at all, and just assumes they are inside the
>> repository. This is obviously not true if the path starts with "..", in
>> which case you would need to compare the number of ".." with the current
>> depth in the repository.
>>
>> prefix_path already does this (and is what generates the later
>> "../non/git/matching-file is not in the repository" message). We could
>> perhaps get rid of path_outside_repo and just re-use prefix_path's
>> logic, something like (not tested):
>
> With your patch applied, I was able to use relative paths in my tests.
>  I also confirmed that all the t4*.sh tests pass.
>
> For what its worth, your patch looks correct to me.  Existing
> consumers of 'prefix_path' should get the same results as before and
> the one added xmalloc is paired with a free.

Jeff,

Are you planning to send this patch to the list?  If not, can I
include it as 1 of 2 before my patch?  If we go that route, I'm not
sure how to properly show you as the author...

Also, in an earlier email [1] you mentioned that it may be a good idea
to rename 'found_changes' to something like 'xdiff_found_changes'.  I
like the idea...I could submit this change as another patch in the
series, if you have no objections.

Thanks again for your review and help.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/200160/focus=200163

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

[Jeff King]

On Wed, Jun 20, 2012 at 09:38:15AM -0400, Tim Henigan wrote:

> > With your patch applied, I was able to use relative paths in my tests.
> >  I also confirmed that all the t4*.sh tests pass.
> >
> > For what its worth, your patch looks correct to me.  Existing
> > consumers of 'prefix_path' should get the same results as before and
> > the one added xmalloc is paired with a free.
> 
> Jeff,
> 
> Are you planning to send this patch to the list?  If not, can I
> include it as 1 of 2 before my patch?  If we go that route, I'm not
> sure how to properly show you as the author...

I'd probably get to it eventually, but I haven't touched it since I sent
it. If you want to include some tests and package it with a commit
message, that would make me very happy.

You can override the author by including a "From: " header as the first
line in the body of the email (which git-am will use rather than the
identity in the email's From header). If you use git-send-email, it will
do this automatically when the patch author does not match your
identity.

I didn't sign-off the original, but please feel free to include my
sign-off, as well as add your own. And note your own contributions in
the commit message. So the resulting email would be something like:


   From: Tim Henigan <tim.henigan@gmail.com>
   Date: ...
   Subject: [PATCH 1/2] diff: handle relative paths in no-index

   From: Jeff King <peff@peff.net>

   ... some commit message body ...

   Tests and commit message by Tim Henigan.

   Signed-off-by: Jeff King <peff@peff.net>
   Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
   ---
   ... the actual patch ...

> Also, in an earlier email [1] you mentioned that it may be a good idea
> to rename 'found_changes' to something like 'xdiff_found_changes'.  I
> like the idea...I could submit this change as another patch in the
> series, if you have no objections.

Fine by me. I think "xdiff_found_changes" is not quite accurate; it is
really "did builtin_diff find any changes?" since we might never call
into xdiff (e.g., for binary files). I'm not sure what the best name is.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Fine by me. I think "xdiff_found_changes" is not quite accurate; it is
> really "did builtin_diff find any changes?" since we might never call
> into xdiff (e.g., for binary files). I'm not sure what the best name is.

"diffopt.found_changes" is clear enough for me.

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

[Jeff King]

On Wed, Jun 20, 2012 at 11:44:25AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Fine by me. I think "xdiff_found_changes" is not quite accurate; it is
> > really "did builtin_diff find any changes?" since we might never call
> > into xdiff (e.g., for binary files). I'm not sure what the best name is.
> 
> "diffopt.found_changes" is clear enough for me.

I thought this bug would be enough to show that diffopt.found_changes is
not clear enough. It is the source of the original bug (the code should
have been using HAS_CHANGES instead of found_changes), and it gave at
least one of the bug investigators (i.e., me) quite a bit of confusion
to understand why found_changes was not being set when diff_flush found
changes.

IOW, as a naive reader of the "struct diff_options", how do I understand
the difference between HAS_CHANGES and found_changes?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I thought this bug would be enough to show that diffopt.found_changes is
> not clear enough. It is the source of the original bug (the code should
> have been using HAS_CHANGES instead of found_changes), and it gave at
> least one of the bug investigators (i.e., me) quite a bit of confusion
> to understand why found_changes was not being set when diff_flush found
> changes.

I think when found_changes was introduced so that diff can indicate
more than what HAS_CHANGES (i.e. there is a blob-level difference
exists) can represent, the patch forgot to update the no-index
codepath.

> IOW, as a naive reader of the "struct diff_options", how do I understand
> the difference between HAS_CHANGES and found_changes?

HAS_CHANGES and found_changes should be implementation detail of
diff_result_code() and as long as we do not add outside users of it,
the names should not matter too much.  If we were to rename them,
HAS_CHANGES should also be made more descriptive to hint what it
means ("object level difference exists"), I would think.  Given the
recent discussion on "diff/log -L <bottom>,<top>", found_changes
would mean "content level change that the caller cares about
exists".

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

[Tim Henigan]

On Wed, Jun 20, 2012 at 12:06 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Jun 20, 2012 at 09:38:15AM -0400, Tim Henigan wrote:
>>
>> Are you planning to send this patch to the list?  If not, can I
>> include it as 1 of 2 before my patch?  If we go that route, I'm not
>> sure how to properly show you as the author...
>
> I'd probably get to it eventually, but I haven't touched it since I sent
> it. If you want to include some tests and package it with a commit
> message, that would make me very happy.

Thanks, I will do that.

It looks like the best place to add tests is t4010-diff-pathspec.sh.
The only cases not tested through other means appear to be:

    git diff <file in repo> <relative path outside repo>
    git diff <relative path outside repo> <relative path outside repo>

Other pathspec variations seem to be covered extensively by other
tests (mostly as a side effect).  Am I missing other variations that
should be checked?

In both cases shown above, we are simply verifying that giving a
relative path to diff does not cause it to abort.  So it may be
sufficient to only test one of the above.

The tests that I added to t4035-diff-quiet.sh already cover both of
the cases listed above.  Is it worthwhile to duplicate some of those
tests in t4010?

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

[Junio C Hamano]

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

> It looks like the best place to add tests is t4010-diff-pathspec.sh.

Well, pathspecs are all about limiting the changes in the repository
by patterns, while "diff --no-index" is about exact pathnames, so I
am not sure if that is a good place to put them.  Isn't there a test
script that is dedicated to the "diff --no-index" codepath (perhaps
4053) that is more appropriate for doing this?