Re: [PATCH] portability: allow building in systems without d_type

[Collin Funk <collin.funk1@gmail.com>]

Hi Junio,

Junio C Hamano <gitster@pobox.com> 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