{"thread":{"id":"58452","subject":"[PATCH] diagnose.c: refactor to safely use 'd_type'","startedAt":"2022-09-17T18:17:01Z","lastAt":"2022-09-19T01:13:45Z","messageCount":2,"participants":["Victoria Dye via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463143","messageId":"pull.1354.git.1663438615413.gitgitgadget@gmail.com","threadId":"58452","inReplyTo":null,"subject":"[PATCH] diagnose.c: refactor to safely use 'd_type'","fromName":"Victoria Dye via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-17T18:16:55Z","receivedAt":"2022-09-17T18:17:01Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"From: Victoria Dye <vdye@github.com>\n\nRefactor usage of the 'd_type' property of 'struct dirent' in 'diagnose.c'\nto instead utilize the compatibility macro 'DTYPE()'. On systems where\n'd_type' is not present in 'struct dirent', this macro will always return\n'DT_UNKNOWN'. In that case, instead fall back on using the 'stat.st_mode' to\ndetermine whether the dirent points to a dir, file, or link.\n\nAdditionally, add a test to 't0092-diagnose.sh' to verify that files (e.g.,\nloose objects) are counted properly.\n\nNote that the new function 'get_dtype()' is based on 'resolve_dtype()' in\n'dir.c' (which itself was refactored from a prior 'get_dtype()' in\nad6f2157f9 (dir: restructure in a way to avoid passing around a struct\ndirent, 2020-01-16)), but differs in that it is meant for use on arbitrary\nfiles, such as those inside the '.git' dir. Because of this, it does not\nsearch the index for a matching entry to derive the 'd_type'.\n\nReported-by: Randall S. Becker <rsbecker@nexbridge.com>\nSigned-off-by: Victoria Dye <vdye@github.com>\n---\n    diagnose.c: refactor to safely use 'd_type'\n    \n    This fixes the compilation error reported by Randall in [1], which\n    happens because 'd_type' doesn't exist in the 'dirent' struct on all\n    platforms.\n    \n    The implementation of new-'get_dtype()' shares some code with\n    'resolve_dtype()' (and its previous incarnations as old-'get_dtype()')\n    but, to keep this fix down to a single patch, I didn't refactor the bits\n    of shared logic into a common location. I'm happy to do that if it would\n    be a valuable addition to this fix, but I wanted to start small and\n    avoid the churn of a refactor if it isn't something people want here.\n    \n    Thanks!\n    \n     * Victoria\n    \n    [1]\n    https://lore.kernel.org/git/011001d8ca20$bc4d81f0$34e885d0$@nexbridge.com/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1354%2Fvdye%2Fbugfix%2Ffix-dtype-usage-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1354/vdye/bugfix/fix-dtype-usage-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1354\n\n diagnose.c          | 70 ++++++++++++++++++++++++++++++++++++---------\n t/t0092-diagnose.sh | 12 ++++++++\n 2 files changed, 68 insertions(+), 14 deletions(-)\n\ndiff --git a/diagnose.c b/diagnose.c\nindex beb0a8741ba..8f265698966 100644\n--- a/diagnose.c\n+++ b/diagnose.c\n@@ -66,17 +66,53 @@ static int dir_file_stats(struct object_directory *object_dir, void *data)\n \treturn 0;\n }\n \n-static int count_files(char *path)\n+/*\n+ * Get the d_type of a dirent. If the d_type is unknown, derive it from\n+ * stat.st_mode.\n+ *\n+ * Note that 'path' is assumed to have a trailing slash. It is also modified\n+ * in-place during the execution of the function, but is then reverted to its\n+ * original value before returning.\n+ */\n+static unsigned char get_dtype(struct dirent *e, struct strbuf *path)\n {\n-\tDIR *dir = opendir(path);\n+\tstruct stat st;\n+\tunsigned char dtype = DTYPE(e);\n+\tsize_t base_path_len;\n+\n+\tif (dtype != DT_UNKNOWN)\n+\t\treturn dtype;\n+\n+\t/* d_type unknown in dirent, try to fall back on lstat results */\n+\tbase_path_len = path->len;\n+\tstrbuf_addstr(path, e->d_name);\n+\tif (lstat(path->buf, &st))\n+\t\tgoto cleanup;\n+\n+\t/* determine d_type from st_mode */\n+\tif (S_ISREG(st.st_mode))\n+\t\tdtype = DT_REG;\n+\telse if (S_ISDIR(st.st_mode))\n+\t\tdtype = DT_DIR;\n+\telse if (S_ISLNK(st.st_mode))\n+\t\tdtype = DT_LNK;\n+\n+cleanup:\n+\tstrbuf_setlen(path, base_path_len);\n+\treturn dtype;\n+}\n+\n+static int count_files(struct strbuf *path)\n+{\n+\tDIR *dir = opendir(path->buf);\n \tstruct dirent *e;\n \tint count = 0;\n \n \tif (!dir)\n \t\treturn 0;\n \n-\twhile ((e = readdir(dir)) != NULL)\n-\t\tif (!is_dot_or_dotdot(e->d_name) && e->d_type == DT_REG)\n+\twhile ((e = readdir_skip_dot_and_dotdot(dir)) != NULL)\n+\t\tif (get_dtype(e, path) == DT_REG)\n \t\t\tcount++;\n \n \tclosedir(dir);\n@@ -104,13 +140,13 @@ static void loose_objs_stats(struct strbuf *buf, const char *path)\n \tstrbuf_addch(&count_path, '/');\n \tbase_path_len = count_path.len;\n \n-\twhile ((e = readdir(dir)) != NULL)\n-\t\tif (!is_dot_or_dotdot(e->d_name) &&\n-\t\t    e->d_type == DT_DIR && strlen(e->d_name) == 2 &&\n+\twhile ((e = readdir_skip_dot_and_dotdot(dir)) != NULL)\n+\t\tif (get_dtype(e, &count_path) == DT_DIR &&\n+\t\t    strlen(e->d_name) == 2 &&\n \t\t    !hex_to_bytes(&c, e->d_name, 1)) {\n \t\t\tstrbuf_setlen(&count_path, base_path_len);\n-\t\t\tstrbuf_addstr(&count_path, e->d_name);\n-\t\t\ttotal += (count = count_files(count_path.buf));\n+\t\t\tstrbuf_addf(&count_path, \"%s/\", e->d_name);\n+\t\t\ttotal += (count = count_files(&count_path));\n \t\t\tstrbuf_addf(buf, \"%s : %7d files\\n\", e->d_name, count);\n \t\t}\n \n@@ -144,22 +180,28 @@ static int add_directory_to_archiver(struct strvec *archiver_args,\n \tlen = buf.len;\n \tstrvec_pushf(archiver_args, \"--prefix=%s\", buf.buf);\n \n-\twhile (!res && (e = readdir(dir))) {\n-\t\tif (!strcmp(\".\", e->d_name) || !strcmp(\"..\", e->d_name))\n-\t\t\tcontinue;\n+\twhile (!res && (e = readdir_skip_dot_and_dotdot(dir))) {\n+\t\tstruct strbuf abspath = STRBUF_INIT;\n+\t\tunsigned char dtype;\n+\n+\t\tstrbuf_add_absolute_path(&abspath, at_root ? \".\" : path);\n+\t\tstrbuf_addch(&abspath, '/');\n+\t\tdtype = get_dtype(e, &abspath);\n \n \t\tstrbuf_setlen(&buf, len);\n \t\tstrbuf_addstr(&buf, e->d_name);\n \n-\t\tif (e->d_type == DT_REG)\n+\t\tif (dtype == DT_REG)\n \t\t\tstrvec_pushf(archiver_args, \"--add-file=%s\", buf.buf);\n-\t\telse if (e->d_type != DT_DIR)\n+\t\telse if (dtype != DT_DIR)\n \t\t\twarning(_(\"skipping '%s', which is neither file nor \"\n \t\t\t\t  \"directory\"), buf.buf);\n \t\telse if (recurse &&\n \t\t\t add_directory_to_archiver(archiver_args,\n \t\t\t\t\t\t   buf.buf, recurse) < 0)\n \t\t\tres = -1;\n+\n+\t\tstrbuf_release(&abspath);\n \t}\n \n \tclosedir(dir);\ndiff --git a/t/t0092-diagnose.sh b/t/t0092-diagnose.sh\nindex fca9b58489c..133e5747d61 100755\n--- a/t/t0092-diagnose.sh\n+++ b/t/t0092-diagnose.sh\n@@ -28,12 +28,23 @@ test_expect_success UNZIP 'creates diagnostics zip archive' '\n \t! \"$GIT_UNZIP\" -l \"$zip_path\" | grep \".git/\"\n '\n \n+test_expect_success UNZIP 'counts loose objects' '\n+\ttest_commit A &&\n+\n+\t# After committing, should have non-zero loose objects\n+\tgit diagnose -o test-count -s 1 >out &&\n+\tzip_path=test-count/git-diagnostics-1.zip &&\n+\t\"$GIT_UNZIP\" -p \"$zip_path\" objects-local.txt >out &&\n+\tgrep \"^Total: [1-9][0-9]* loose objects\" out\n+'\n+\n test_expect_success UNZIP '--mode=stats excludes .git dir contents' '\n \ttest_when_finished rm -rf report &&\n \n \tgit diagnose -o report -s test --mode=stats >out &&\n \n \t# Includes pack quantity/size info\n+\tzip_path=report/git-diagnostics-test.zip &&\n \t\"$GIT_UNZIP\" -p \"$zip_path\" packs-local.txt >out &&\n \tgrep \".git/objects\" out &&\n \n@@ -47,6 +58,7 @@ test_expect_success UNZIP '--mode=all includes .git dir contents' '\n \tgit diagnose -o report -s test --mode=all >out &&\n \n \t# Includes pack quantity/size info\n+\tzip_path=report/git-diagnostics-test.zip &&\n \t\"$GIT_UNZIP\" -p \"$zip_path\" packs-local.txt >out &&\n \tgrep \".git/objects\" out &&\n \n\nbase-commit: d3fa443f97e3a8d75b51341e2d5bac380b7422df\n-- \ngitgitgadget\n"},{"id":"463161","messageId":"xmqqczbsdv41.fsf@gitster.g","threadId":"58452","inReplyTo":"pull.1354.git.1663438615413.gitgitgadget@gmail.com","subject":"Re: [PATCH] diagnose.c: refactor to safely use 'd_type'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-19T01:13:34Z","receivedAt":"2022-09-19T01:13:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Victoria Dye via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Victoria Dye <vdye@github.com>\n>\n> Refactor usage of the 'd_type' property of 'struct dirent' in 'diagnose.c'\n> to instead utilize the compatibility macro 'DTYPE()'. On systems where\n> 'd_type' is not present in 'struct dirent', this macro will always return\n> 'DT_UNKNOWN'. In that case, instead fall back on using the 'stat.st_mode' to\n> determine whether the dirent points to a dir, file, or link.\n>\n> Additionally, add a test to 't0092-diagnose.sh' to verify that files (e.g.,\n> loose objects) are counted properly.\n>\n> Note that the new function 'get_dtype()' is based on 'resolve_dtype()' in\n> 'dir.c' (which itself was refactored from a prior 'get_dtype()' in\n> ad6f2157f9 (dir: restructure in a way to avoid passing around a struct\n> dirent, 2020-01-16)), but differs in that it is meant for use on arbitrary\n> files, such as those inside the '.git' dir. Because of this, it does not\n> search the index for a matching entry to derive the 'd_type'.\n>\n> Reported-by: Randall S. Becker <rsbecker@nexbridge.com>\n> Signed-off-by: Victoria Dye <vdye@github.com>\n> ---\n\nBeautiful.  Use of DTYPE() macro that already hides the platform\nspecifics, and get_dtype() implementation that calls lstat() lazily,\nare exactly the solution I was expecting to see.\n\nNicely done.  Will queue.  Thanks.\n"}]}