Re: [PATCH] diff --no-index: fix logic for paths ending in '/'

[Junio C Hamano <gitster@pobox.com>]

Jacob Keller <jacob.e.keller@intel.com> writes:

> diff --git a/diff-no-index.c b/diff-no-index.c
> index 88ae4cee56ba..c70f82b80559 100644
> --- a/diff-no-index.c
> +++ b/diff-no-index.c
> ...
> @@ -346,7 +355,8 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,
>  		  int implicit_no_index, int argc, const char **argv)
>  {
>  	struct pathspec pathspec, *ps = NULL;
> -	int i, no_index, skip1 = 0, skip2 = 0;
> +	struct strbuf ps_match1 = STRBUF_INIT, ps_match2 = STRBUF_INIT;
> +	int i, no_index;
>  	int ret = 1;
>  	const char *paths[2];
>  	char *to_free[ARRAY_SIZE(paths)] = { 0 };
> @@ -387,11 +397,6 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,
>  			       NULL, &argv[2]);
>  		if (pathspec.nr)
>  			ps = &pathspec;
> -
> -		skip1 = strlen(paths[0]);
> -		skip1 += paths[0][skip1] == '/' ? 0 : 1;
> -		skip2 = strlen(paths[1]);
> -		skip2 += paths[1][skip2] == '/' ? 0 : 1;
>  	} else if (argc > 2) {
>  		warning(_("Limiting comparison with pathspecs is only "
>  			  "supported if both paths are directories."));
> @@ -415,7 +420,7 @@ int diff_no_index(struct rev_info *revs, const struct git_hash_algo *algop,
>  	revs->diffopt.flags.exit_with_status = 1;
>  
>  	if (queue_diff(&revs->diffopt, algop, paths[0], paths[1], 0, ps,
> -		       skip1, skip2))
> +		       &ps_match1, &ps_match2))

Inside queue_diff() that makes recursive calls to itself, lenthens
these strbuf to hold longer paths while using setlen when it wants
to trim the tail end of the paths.

So it is likely that ps_match.buf would never become NUL even when
ps_match.len goes down to 0 after the recursion and queue_diff()
uses setlen to trim the string back to what was originally in there.

Hence, I think the clean-up code of this function this goto ...

>  		goto out;

... jumps to would need

	strbuf_release(&ps_match1);
	strbuf_release(&ps_match2);

added after that "out:" label?

If we run this test with leak sanitizer, wouldn't it find leak in
these (I haven't tried it myself---I just am speculating)?

> diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
> index 01db9243abfe..e0ea437685b0 100755
> --- a/t/t4053-diff-no-index.sh
> +++ b/t/t4053-diff-no-index.sh
> @@ -322,6 +322,22 @@ test_expect_success 'diff --no-index with pathspec' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'diff --no-index first path ending in slash with pathspec' '
> +	test_expect_code 1 git diff --name-status --no-index a/ b 1 >actual &&
> +	cat >expect <<-EOF &&
> +	D	a/1
> +	EOF
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'diff --no-index second path ending in slash with pathspec' '
> +	test_expect_code 1 git diff --name-status --no-index a b/ 1 >actual &&
> +	cat >expect <<-EOF &&
> +	D	a/1
> +	EOF
> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'diff --no-index with pathspec no matches' '
>  	test_expect_code 0 git diff --name-status --no-index a b missing
>  '