{"thread":{"id":"59257","subject":"[PATCH] dir-iterator: drop unused `DIR_ITERATOR_FOLLOW_SYMLINKS`","startedAt":"2023-02-16T18:27:51Z","lastAt":"2023-02-17T00:40:34Z","messageCount":6,"participants":["Taylor Blau","Jeff King","Matheus Tavares Bernardino"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"472214","messageId":"9bb10b607e46f867a3f8f5c71abf13c990d1ecfe.1676572031.git.me@ttaylorr.com","threadId":"59257","inReplyTo":null,"subject":"[PATCH] dir-iterator: drop unused `DIR_ITERATOR_FOLLOW_SYMLINKS`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-02-16T18:27:14Z","receivedAt":"2023-02-16T18:27:51Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"The `FOLLOW_SYMLINKS` flag was added to the dir-iterator API in\nfa1da7d2ee (dir-iterator: add flags parameter to dir_iterator_begin,\n2019-07-10) in order to follow symbolic links while traversing through a\ndirectory.\n\n`FOLLOW_SYMLINKS` gained its first caller in ff7ccc8c9a (clone: use\ndir-iterator to avoid explicit dir traversal, 2019-07-10), but it was\nsubsequently removed in 6f054f9fb3 (builtin/clone.c: disallow `--local`\nclones with symlinks, 2022-07-28).\n\nSince then, we've held on to the code for `DIR_ITERATOR_FOLLOW_SYMLINKS`\nin the name of making minimally invasive changes during a security\nembargo.\n\nIn fact, we even changed the dir-iterator API in bffc762f87\n(dir-iterator: prevent top-level symlinks without FOLLOW_SYMLINKS,\n2023-01-24) without having any non-test callers of that flag.\n\nNow that we're past those security embargo(s), let's finalize our\ncleanup of the `DIR_ITERATOR_FOLLOW_SYMLINKS` code and remove its\nimplementation since there are no remaining callers.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n dir-iterator.c               | 12 +++---------\n dir-iterator.h               | 20 +-------------------\n t/helper/test-dir-iterator.c |  6 ++----\n t/t0066-dir-iterator.sh      | 34 ----------------------------------\n 4 files changed, 6 insertions(+), 66 deletions(-)\n\ndiff --git a/dir-iterator.c b/dir-iterator.c\nindex 3764dd81a1..cedd304759 100644\n--- a/dir-iterator.c\n+++ b/dir-iterator.c\n@@ -112,10 +112,7 @@ static int prepare_next_entry_data(struct dir_iterator_int *iter,\n \titer->base.basename = iter->base.path.buf +\n \t\t\t      iter->levels[iter->levels_nr - 1].prefix_len;\n \n-\tif (iter->flags & DIR_ITERATOR_FOLLOW_SYMLINKS)\n-\t\terr = stat(iter->base.path.buf, &iter->base.st);\n-\telse\n-\t\terr = lstat(iter->base.path.buf, &iter->base.st);\n+\terr = lstat(iter->base.path.buf, &iter->base.st);\n \n \tsaved_errno = errno;\n \tif (err && errno != ENOENT)\n@@ -213,13 +210,10 @@ struct dir_iterator *dir_iterator_begin(const char *path, unsigned int flags)\n \titer->flags = flags;\n \n \t/*\n-\t * Note: stat/lstat already checks for NULL or empty strings and\n+\t * Note: lstat already checks for NULL or empty strings and\n \t * nonexistent paths.\n \t */\n-\tif (iter->flags & DIR_ITERATOR_FOLLOW_SYMLINKS)\n-\t\terr = stat(iter->base.path.buf, &iter->base.st);\n-\telse\n-\t\terr = lstat(iter->base.path.buf, &iter->base.st);\n+\terr = lstat(iter->base.path.buf, &iter->base.st);\n \n \tif (err < 0) {\n \t\tsaved_errno = errno;\ndiff --git a/dir-iterator.h b/dir-iterator.h\nindex e3b6ff2800..479e1ec784 100644\n--- a/dir-iterator.h\n+++ b/dir-iterator.h\n@@ -54,24 +54,8 @@\n  *   and ITER_ERROR is returned immediately. In both cases, a meaningful\n  *   warning is emitted. Note: ENOENT errors are always ignored so that\n  *   the API users may remove files during iteration.\n- *\n- * - DIR_ITERATOR_FOLLOW_SYMLINKS: make dir-iterator follow symlinks.\n- *   i.e., linked directories' contents will be iterated over and\n- *   iter->base.st will contain information on the referred files,\n- *   not the symlinks themselves, which is the default behavior. Broken\n- *   symlinks are ignored.\n- *\n- *   Note: setting DIR_ITERATOR_FOLLOW_SYMLINKS affects resolving the\n- *   starting path as well (e.g., attempting to iterate starting at a\n- *   symbolic link pointing to a directory without FOLLOW_SYMLINKS will\n- *   result in an error).\n- *\n- * Warning: circular symlinks are also followed when\n- * DIR_ITERATOR_FOLLOW_SYMLINKS is set. The iteration may end up with\n- * an ELOOP if they happen and DIR_ITERATOR_PEDANTIC is set.\n  */\n #define DIR_ITERATOR_PEDANTIC (1 << 0)\n-#define DIR_ITERATOR_FOLLOW_SYMLINKS (1 << 1)\n \n struct dir_iterator {\n \t/* The current path: */\n@@ -88,9 +72,7 @@ struct dir_iterator {\n \tconst char *basename;\n \n \t/*\n-\t * The result of calling lstat() on path; or stat(), if the\n-\t * DIR_ITERATOR_FOLLOW_SYMLINKS flag was set at\n-\t * dir_iterator's initialization.\n+\t * The result of calling lstat() on path.\n \t */\n \tstruct stat st;\n };\ndiff --git a/t/helper/test-dir-iterator.c b/t/helper/test-dir-iterator.c\nindex 659b6bfa81..6b297bd753 100644\n--- a/t/helper/test-dir-iterator.c\n+++ b/t/helper/test-dir-iterator.c\n@@ -15,7 +15,7 @@ static const char *error_name(int error_number)\n \n /*\n  * usage:\n- * tool-test dir-iterator [--follow-symlinks] [--pedantic] directory_path\n+ * tool-test dir-iterator [--pedantic] directory_path\n  */\n int cmd__dir_iterator(int argc, const char **argv)\n {\n@@ -24,9 +24,7 @@ int cmd__dir_iterator(int argc, const char **argv)\n \tint iter_status;\n \n \tfor (++argv, --argc; *argv && starts_with(*argv, \"--\"); ++argv, --argc) {\n-\t\tif (strcmp(*argv, \"--follow-symlinks\") == 0)\n-\t\t\tflags |= DIR_ITERATOR_FOLLOW_SYMLINKS;\n-\t\telse if (strcmp(*argv, \"--pedantic\") == 0)\n+\t\tif (strcmp(*argv, \"--pedantic\") == 0)\n \t\t\tflags |= DIR_ITERATOR_PEDANTIC;\n \t\telse\n \t\t\tdie(\"invalid option '%s'\", *argv);\ndiff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh\nindex 04b811622b..4ed3136b00 100755\n--- a/t/t0066-dir-iterator.sh\n+++ b/t/t0066-dir-iterator.sh\n@@ -131,44 +131,10 @@ test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default\n \ttest_cmp expected-no-follow-sorted-output actual-no-follow-sorted-output\n '\n \n-test_expect_success SYMLINKS 'dir-iterator should follow symlinks w/ follow flag' '\n-\tcat >expected-follow-sorted-output <<-EOF &&\n-\t[d] (a) [a] ./dir4/a\n-\t[d] (a/f) [f] ./dir4/a/f\n-\t[d] (a/f/c) [c] ./dir4/a/f/c\n-\t[d] (b) [b] ./dir4/b\n-\t[d] (b/c) [c] ./dir4/b/c\n-\t[f] (a/d) [d] ./dir4/a/d\n-\t[f] (a/e) [e] ./dir4/a/e\n-\tEOF\n-\n-\ttest-tool dir-iterator --follow-symlinks ./dir4 >out &&\n-\tsort out >actual-follow-sorted-output &&\n-\n-\ttest_cmp expected-follow-sorted-output actual-follow-sorted-output\n-'\n-\n test_expect_success SYMLINKS 'dir-iterator does not resolve top-level symlinks' '\n \ttest_must_fail test-tool dir-iterator ./dir6 >out &&\n \n \tgrep \"ENOTDIR\" out\n '\n \n-test_expect_success SYMLINKS 'dir-iterator resolves top-level symlinks w/ follow flag' '\n-\tcat >expected-follow-sorted-output <<-EOF &&\n-\t[d] (a) [a] ./dir6/a\n-\t[d] (a/f) [f] ./dir6/a/f\n-\t[d] (a/f/c) [c] ./dir6/a/f/c\n-\t[d] (b) [b] ./dir6/b\n-\t[d] (b/c) [c] ./dir6/b/c\n-\t[f] (a/d) [d] ./dir6/a/d\n-\t[f] (a/e) [e] ./dir6/a/e\n-\tEOF\n-\n-\ttest-tool dir-iterator --follow-symlinks ./dir6 >out &&\n-\tsort out >actual-follow-sorted-output &&\n-\n-\ttest_cmp expected-follow-sorted-output actual-follow-sorted-output\n-'\n-\n test_done\n-- \n2.38.0.16.g393fd4c6db\n"},{"id":"472217","messageId":"Y+560+/G4pelldXF@coredump.intra.peff.net","threadId":"59257","inReplyTo":"9bb10b607e46f867a3f8f5c71abf13c990d1ecfe.1676572031.git.me@ttaylorr.com","subject":"Re: [PATCH] dir-iterator: drop unused `DIR_ITERATOR_FOLLOW_SYMLINKS`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-16T18:49:55Z","receivedAt":"2023-02-16T18:49:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 16, 2023 at 01:27:14PM -0500, Taylor Blau wrote:\n\n> The `FOLLOW_SYMLINKS` flag was added to the dir-iterator API in\n> fa1da7d2ee (dir-iterator: add flags parameter to dir_iterator_begin,\n> 2019-07-10) in order to follow symbolic links while traversing through a\n> directory.\n> \n> `FOLLOW_SYMLINKS` gained its first caller in ff7ccc8c9a (clone: use\n> dir-iterator to avoid explicit dir traversal, 2019-07-10), but it was\n> subsequently removed in 6f054f9fb3 (builtin/clone.c: disallow `--local`\n> clones with symlinks, 2022-07-28).\n> \n> Since then, we've held on to the code for `DIR_ITERATOR_FOLLOW_SYMLINKS`\n> in the name of making minimally invasive changes during a security\n> embargo.\n> \n> In fact, we even changed the dir-iterator API in bffc762f87\n> (dir-iterator: prevent top-level symlinks without FOLLOW_SYMLINKS,\n> 2023-01-24) without having any non-test callers of that flag.\n> \n> Now that we're past those security embargo(s), let's finalize our\n> cleanup of the `DIR_ITERATOR_FOLLOW_SYMLINKS` code and remove its\n> implementation since there are no remaining callers.\n\nThanks for following up on this. I think it's an obviously good\ndirection, and the patch looks sensible. It's hard to grep for\n--follow-symlinks or FOLLOW_SYMLINKS to make sure you got everything,\njust because there are other unrelated features that use that name. ;)\n\nBut I think you got all the relevant spots. The only thing I wondered is\nwhether we could clean up any of the test setup. Specifically, \"dir5\"\ndoes not seem to be used in the tests, and everything passes with:\n\ndiff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh\nindex 4ed3136b00..1f3e070ec2 100755\n--- a/t/t0066-dir-iterator.sh\n+++ b/t/t0066-dir-iterator.sh\n@@ -106,12 +106,6 @@ test_expect_success SYMLINKS 'setup dirs with symlinks' '\n \tln -s d dir4/a/e &&\n \tln -s ../b dir4/a/f &&\n \n-\tmkdir -p dir5/a/b &&\n-\tmkdir -p dir5/a/c &&\n-\tln -s ../c dir5/a/b/d &&\n-\tln -s ../ dir5/a/b/e &&\n-\tln -s ../../ dir5/a/b/f &&\n-\n \tln -s dir4 dir6\n '\n \n\nBut...that is true even before your patch. dir5 is not mentioned in any\nof the expected output, even in fa1da7d2ee (dir-iterator: add flags\nparameter to dir_iterator_begin, 2019-07-10) where it was added. Was it\njust vestigial? Or is it somehow important that it is _not_ in the\noutput?\n\nI didn't dig, and even if it can be removed, it would probably make\nsense to do it separately from your patch anyway.\n\n-Peff\n"},{"id":"472221","messageId":"Y+6L9R9HkkUc7PkX@nand.local","threadId":"59257","inReplyTo":"Y+560+/G4pelldXF@coredump.intra.peff.net","subject":"Re: [PATCH] dir-iterator: drop unused `DIR_ITERATOR_FOLLOW_SYMLINKS`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-02-16T20:03:01Z","receivedAt":"2023-02-16T20:03:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Feb 16, 2023 at 01:49:55PM -0500, Jeff King wrote:\n> Thanks for following up on this. I think it's an obviously good\n> direction, and the patch looks sensible. It's hard to grep for\n> --follow-symlinks or FOLLOW_SYMLINKS to make sure you got everything,\n> just because there are other unrelated features that use that name. ;)\n\nNo problem.\n\n> But...that is true even before your patch. dir5 is not mentioned in any\n> of the expected output, even in fa1da7d2ee (dir-iterator: add flags\n> parameter to dir_iterator_begin, 2019-07-10) where it was added. Was it\n> just vestigial? Or is it somehow important that it is _not_ in the\n> output?\n>\n> I didn't dig, and even if it can be removed, it would probably make\n> sense to do it separately from your patch anyway.\n\nI have no idea either. From a cursory scan, I think I'd err on the side\nof it being vestigial. But Matheus (cc'd) should be able to tell us for\nsure.\n\nThanks,\nTaylor\n"},{"id":"472223","messageId":"Y+6Mcnng0u6sz+C6@nand.local","threadId":"59257","inReplyTo":"Y+6L9R9HkkUc7PkX@nand.local","subject":"Re: [PATCH] dir-iterator: drop unused `DIR_ITERATOR_FOLLOW_SYMLINKS`","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-02-16T20:05:06Z","receivedAt":"2023-02-16T20:05:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Feb 16, 2023 at 03:03:01PM -0500, Taylor Blau wrote:\n> I have no idea either. From a cursory scan, I think I'd err on the side\n> of it being vestigial. But Matheus (cc'd) should be able to tell us for\n> sure.\n\nOops. That email (Matheus Tavares <matheus.bernardino@usp.br>) is\ndeactivated, which I should have known from 38645f8cb19 (mailmap: update\nemail address of Matheus Tavares, 2022-12-09).\n\nI CC'd the right address instead.\n\nThanks,\nTaylor\n"},{"id":"472239","messageId":"CAGdrTFhHBU2BNYdYr7LbOS7i1LOHGjWLw_d5ZJAXxvTKyLiFCA@mail.gmail.com","threadId":"59257","inReplyTo":"Y+6Mcnng0u6sz+C6@nand.local","subject":"Re: [PATCH] dir-iterator: drop unused `DIR_ITERATOR_FOLLOW_SYMLINKS`","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.tavb@gmail.com","sentAt":"2023-02-17T00:16:50Z","receivedAt":"2023-02-17T00:17:06Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi, Taylor and Jeff\n\nOn Thu, Feb 16, 2023 at 5:05 PM Taylor Blau <me@ttaylorr.com> wrote:\n>\n> On Thu, Feb 16, 2023 at 03:03:01PM -0500, Taylor Blau wrote:\n> > I have no idea either. From a cursory scan, I think I'd err on the side\n> > of it being vestigial. But Matheus (cc'd) should be able to tell us for\n> > sure.\n\nOh, it's been some time. But yes, dir5 is vestigial and should indeed\nbe removed. It was created back when I wanted to make dir-iterator\ndetect and avoid recursive symlinks (see this [1] earlier version of\nthe patch, where there was a test grepping for dir5 at the output).\nBut this idea ended up being discarded and I must have forgotten to\nremove the dir5.\n\n[1]: https://lore.kernel.org/git/20190502144829.4394-7-matheus.bernardino@usp.br/\n"},{"id":"472241","messageId":"Y+7M/VLcDRDCXX27@coredump.intra.peff.net","threadId":"59257","inReplyTo":"CAGdrTFhHBU2BNYdYr7LbOS7i1LOHGjWLw_d5ZJAXxvTKyLiFCA@mail.gmail.com","subject":"[PATCH 2/1] t0066: drop setup of \"dir5\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2023-02-17T00:40:29Z","receivedAt":"2023-02-17T00:40:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 16, 2023 at 09:16:50PM -0300, Matheus Tavares Bernardino wrote:\n\n> Oh, it's been some time. But yes, dir5 is vestigial and should indeed\n> be removed. It was created back when I wanted to make dir-iterator\n> detect and avoid recursive symlinks (see this [1] earlier version of\n> the patch, where there was a test grepping for dir5 at the output).\n> But this idea ended up being discarded and I must have forgotten to\n> remove the dir5.\n\nThanks! It's not super-important, but while we're thinking about it, it\nprobably makes sense to do this cleanup (I prepared it on top of\nTaylor's patch, since it would otherwise conflict textually):\n\n-- >8 --\nSubject: t0066: drop setup of \"dir5\"\n\nThe symlink setup in t0066 makes several directories with links, dir4\nthrough dir6. But ever since dir5 was introduced in fa1da7d2ee\n(dir-iterator: add flags parameter to dir_iterator_begin, 2019-07-10),\nit has never actually been used. It was left over from an earlier\niteration of the patch which tried to handle recursive symlinks\nspecially, as seen in:\n\n  https://lore.kernel.org/git/20190502144829.4394-7-matheus.bernardino@usp.br/\n\nIt's not hurting any of the existing tests to be there, but the extra\nsetup is confusing to anybody trying to read and understand the tests.\nLet's drop the extra directory, and we'll rename \"dir6\" to \"dir5\" so\nnobody wonders whether the gap in naming is important.\n\nHelped-by: Matheus Tavares Bernardino <matheus.tavb@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t0066-dir-iterator.sh | 10 ++--------\n 1 file changed, 2 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t0066-dir-iterator.sh b/t/t0066-dir-iterator.sh\nindex 4ed3136b00..7d0a0da8c0 100755\n--- a/t/t0066-dir-iterator.sh\n+++ b/t/t0066-dir-iterator.sh\n@@ -106,13 +106,7 @@ test_expect_success SYMLINKS 'setup dirs with symlinks' '\n \tln -s d dir4/a/e &&\n \tln -s ../b dir4/a/f &&\n \n-\tmkdir -p dir5/a/b &&\n-\tmkdir -p dir5/a/c &&\n-\tln -s ../c dir5/a/b/d &&\n-\tln -s ../ dir5/a/b/e &&\n-\tln -s ../../ dir5/a/b/f &&\n-\n-\tln -s dir4 dir6\n+\tln -s dir4 dir5\n '\n \n test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default' '\n@@ -132,7 +126,7 @@ test_expect_success SYMLINKS 'dir-iterator should not follow symlinks by default\n '\n \n test_expect_success SYMLINKS 'dir-iterator does not resolve top-level symlinks' '\n-\ttest_must_fail test-tool dir-iterator ./dir6 >out &&\n+\ttest_must_fail test-tool dir-iterator ./dir5 >out &&\n \n \tgrep \"ENOTDIR\" out\n '\n-- \n2.39.2.920.gf3beb43ccf\n\n"}]}