Hi Junio,
Junio C Hamano <gitster@xxxxxxxxx> writes:
> This may allow you to compile and build, but does the resulting
> binary do what you want it to?
> [...]
> On a platform without d_type member, DTYPE() macro gives DT_UNKNOWN
> that is not DT_DIR, so essentially you are always passing 0 even
> when you are looking at a directory (in which case you must pass 1)
> to match_leading_pathspec().
>
> So I somehow doubt this is a correct fix.
Good points. I guess I had reviewed the original patch too late to
realize myself...
> I do not know if get_dtype() helper function is easily applicable to
> this codepath, so I wrote this in a longhand...
>
>
> diff-no-index.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git c/diff-no-index.c w/diff-no-index.c
> index 7c95222ba6..677df91fc5 100644
> --- c/diff-no-index.c
> +++ w/diff-no-index.c
> @@ -41,12 +41,28 @@ static int read_directory_contents(const char *path, struct string_list *list,
>
> while ((e = readdir_skip_dot_and_dotdot(dir))) {
> if (pathspec) {
> + int is_dir = 0;
> +
> strbuf_setlen(&match, len);
> strbuf_addstr(&match, e->d_name);
> + if (dtype != DT_UNKNOWN) {
> + is_dir = dtype == DT_DIR;
> + } else {
> + struct stat st;
> + struct strbuf pathbuf = STRBUF_INIT;
> + strbuf_addstr(&pathbuf, path);
> + strbuf_complete(&pathbuf, '/');
> + strbuf_addstr(&pathbuf, e->d_name);
> + if (!lstat(&st, pathbuf.buf))
> + is_dir = S_ISDIR(st.st_mode);
> + else
> + ; /* punt */
> + strbuf_release(&pathbuf);
> + }
>
> if (!match_leading_pathspec(NULL, pathspec,
> match.buf, match.len,
> - 0, NULL, DTYPE(e) == DT_DIR ? 1 : 0))
> + 0, NULL, is_dir))
> continue;
> }
>
Two very minor issues with with this patch. The arguments to 'lstat' are
reversed and the 'dtype' variable is not declared. Here is a diff that I
applied after your diff:
diff --git a/diff-no-index.c b/diff-no-index.c
index 677df91fc5..a768b46dcd 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -42,6 +42,7 @@ static int read_directory_contents(const char *path, struct string_list *list,
while ((e = readdir_skip_dot_and_dotdot(dir))) {
if (pathspec) {
int is_dir = 0;
+ int dtype = DTYPE(e);
strbuf_setlen(&match, len);
strbuf_addstr(&match, e->d_name);
@@ -53,7 +54,7 @@ static int read_directory_contents(const char *path, struct string_list *list,
strbuf_addstr(&pathbuf, path);
strbuf_complete(&pathbuf, '/');
strbuf_addstr(&pathbuf, e->d_name);
- if (!lstat(&st, pathbuf.buf))
+ if (!lstat(pathbuf.buf, &st))
is_dir = S_ISDIR(st.st_mode);
else
; /* punt */
With Carlo's patch the following tests fail:
$ sh t4053-diff-no-index.sh
[...]
not ok 35 - diff --no-index with pathspec nested pathspec
#
# test_expect_code 1 git diff --name-status --no-index c d 1/2 >actual &&
# cat >expect <<-EOF &&
# D c/1/2/a
# D c/1/2/b
# EOF
# test_cmp expect actual
#
not ok 36 - diff --no-index with pathspec glob
#
# test_expect_code 1 git diff --name-status --no-index c d ":(glob)**/a" >actual &&
# cat >expect <<-EOF &&
# D c/1/2/a
# EOF
# test_cmp expect actual
#
ok 37 - diff --no-index with pathspec glob and exclude
# failed 2 among 37 test(s)
But with your patch (+ the minor corrections) the tests pass as
expected:
$ sh t4053-diff-no-index.sh
[...]
ok 35 - diff --no-index with pathspec nested pathspec
ok 36 - diff --no-index with pathspec glob
ok 37 - diff --no-index with pathspec glob and exclude
# passed all 37 test(s)
Therefore, I think your fix is good to go with a commit message and my
changes. Feel free to add me to Reviewed-by/Tested-By.
Collin
