Re: [PATCH] portability: allow building in systems without d_type
- From
Collin Funk <collin.funk1@gmail.com>
- Date
- Jun 18, 2025, 19:32 UTC
- Message-ID
- <87frfwsv4r.fsf@gmail.com>
- In-Reply-To
- <xmqqwm98ewsd.fsf@gitster.g>
Hi Junio,
Junio C Hamano <gitster@pobox.com> writes:
Show 9 quoted lines
> 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...
Show 41 quoted lines
> 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