{"thread":{"id":"57851","subject":"[RFC PATCH 0/1] dir: consider worktree config in path recursion","startedAt":"2022-05-05T20:32:56Z","lastAt":"2022-06-22T04:58:45Z","messageCount":31,"participants":["Goss Geppert","Junio C Hamano","oss dev","Elijah Newren","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"454882","messageId":"20220505203234.21586-1-ggossdev@gmail.com","threadId":"57851","inReplyTo":null,"subject":"[RFC PATCH 0/1] dir: consider worktree config in path recursion","fromName":"Goss Geppert","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-05T20:32:33Z","receivedAt":"2022-05-05T20:32:56Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"Please see the commit message for a description. Effectively, since\n8d92fb2927 (dir: replace exponential algorithm with a linear one,\n2020-04-01) the following no longer does what it used to do:\n\n```\ntest_repo=\"$(mktemp -d /tmp/git-test-XXXXXXX)\"\ncd \"$test_repo\"\n>test-file\ngit init\ngit config core.worktree /tmp\ngit add test-file\n```\n\nIn terms of desired behavior, it seems that if it is possible to track\nfiles that are outside the default worktree, then it should also be\npossible to track files inside the default worktree location.\nPersonally, I was using this setup to track dotfiles and some other\nsystem related snippets and liked that design more than alternatives I'm\naware of. I ran into this problem because I was recently unable to add a\nfile to an existing repository with this configuration.\n\nThere are a couple open items but I wanted to get some feedback to\nconfirm that the behavior of this patch is desirable or if I'm\noverlooking something entirely.\n\nTODOs:\n[ ] add test\n[ ] add signoffs\n\nGoss Geppert (1):\n dir: consider worktree config in path recursion\n\n dir.c | 37 ++++++++++++++++++++++++++++++-------\n 1 file changed, 30 insertions(+), 7 deletions(-)\n\n\nbase-commit: 0f828332d5ac36fc63b7d8202652efa152809856\n-- \n2.36.0\n\n"},{"id":"454883","messageId":"20220505203234.21586-2-ggossdev@gmail.com","threadId":"57851","inReplyTo":"20220505203234.21586-1-ggossdev@gmail.com","subject":"[RFC PATCH 1/1] dir: consider worktree config in path recursion","fromName":"Goss Geppert","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-05T20:32:34Z","receivedAt":"2022-05-05T20:32:59Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"Since 8d92fb2927 (dir: replace exponential algorithm with a linear one,\n2020-04-01) the following no longer works:\n\n 1) Initialize a repository.\n 2) Set the `core.worktree` location to a parent directory of the\n default worktree.\n 3) Add a file located in the default worktree location to the index\n (i.e. anywhere in the immediate parent directory of the gitdir).\n\nThis commit adds a check to determine whether a nested repository that\nis encountered in recursing a path is actually `the_repository`. If so,\nsimply treat the directory as if it doesn't contain a nested repository.\n\nPrior to this commit, the `add` operation was silently ignored.\n---\n dir.c | 37 ++++++++++++++++++++++++++++++-------\n 1 file changed, 30 insertions(+), 7 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex f2b0f24210..cef39f43d8 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1861,7 +1861,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t */\n \tenum path_treatment state;\n \tint matches_how = 0;\n-\tint nested_repo = 0, check_only, stop_early;\n+\tint check_only, stop_early;\n \tint old_ignored_nr, old_untracked_nr;\n \t/* The \"len-1\" is to strip the final '/' */\n \tenum exist_status status = directory_exists_in_index(istate, dirname, len-1);\n@@ -1893,16 +1893,39 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \n \tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n \t\t!(dir->flags & DIR_NO_GITLINKS)) {\n+\t\t/*\n+\t\t * Determine if `dirname` is a nested repo by confirming that:\n+\t\t * 1) we are in a nonbare repository, and\n+\t\t * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,\n+\t\t * which could occur if the `worktree` location was manually\n+\t\t * configured by the user\n+\t\t */\n+\t\tint nested_repo;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, dirname);\n \t\tnested_repo = is_nonbare_repository_dir(&sb);\n+\n+\t\tif (nested_repo) {\n+\t\t\tchar *real_dirname, *real_gitdir;\n+\t\t\tstrbuf_reset(&sb);\n+\t\t\tstrbuf_addstr(&sb, dirname);\n+\t\t\tstrbuf_complete(&sb, '/');\n+\t\t\tstrbuf_addstr(&sb, \".git\");\n+\t\t\treal_dirname = real_pathdup(sb.buf, 0);\n+\t\t\treal_gitdir = real_pathdup(the_repository->gitdir, 0);\n+\n+\t\t\tnested_repo = !!strcmp(real_dirname, real_gitdir);\n+\t\t\tfree(real_gitdir);\n+\t\t\tfree(real_dirname);\n+\t\t}\n \t\tstrbuf_release(&sb);\n-\t}\n-\tif (nested_repo) {\n-\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n-\t\t (matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n-\t\t\treturn path_none;\n-\t\treturn excluded ? path_excluded : path_untracked;\n+\n+\t\tif (nested_repo) {\n+\t\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n+\t\t\t\t(matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n+\t\t\t\treturn path_none;\n+\t\t\treturn excluded ? path_excluded : path_untracked;\n+\t\t}\n \t}\n \n \tif (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES)) {\n-- \n2.36.0\n\n"},{"id":"454924","messageId":"xmqqilqimw6v.fsf@gitster.g","threadId":"57851","inReplyTo":"20220505203234.21586-1-ggossdev@gmail.com","subject":"Re: [RFC PATCH 0/1] dir: consider worktree config in path recursion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-06T17:02:00Z","receivedAt":"2022-05-06T17:02:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Goss Geppert writes:\n\n> Please see the commit message for a description. Effectively, since\n> 8d92fb2927 (dir: replace exponential algorithm with a linear one,\n> 2020-04-01) the following no longer does what it used to do:\n>\n> ```\n> test_repo=\"$(mktemp -d /tmp/git-test-XXXXXXX)\"\n> cd \"$test_repo\"\n> >test-file\n> git init\n> git config core.worktree /tmp\n> git add test-file\n\nAnd this is supposed to add a new entry to the index at\n\"git-test-123456/test-file\"?\n\nIf it added a new index entry 'test-file', that means \"git config\ncore.worktree\" is completely ignored, because we are not treating\n\"/tmp\" as the root of the working tree as it is instructed and that\nsounds like a bug. Is that what you mean by \"no longer does what it\nused to do\"?\n\nThanks for raising an issue (although it is not quite clear yet to\nme what the issue is, exactly ;-).\n"},{"id":"454938","messageId":"CAAA5oLk-HhJQ2571cHd7qAZM+VFiDA=W12zyh47XzkFJik+XAw@mail.gmail.com","threadId":"57851","inReplyTo":"xmqqilqimw6v.fsf@gitster.g","subject":"Re: [RFC PATCH 0/1] dir: consider worktree config in path recursion","fromName":"oss dev","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-06T20:00:13Z","receivedAt":"2022-05-06T20:00:30Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"On Fri, May 6, 2022 at 1:02 PM Junio C Hamano wrote:\n\n> And this is supposed to add a new entry to the index at\n> \"git-test-123456/test-file\"?\n\nCorrect, I expected an index entry at \"git-test-123456/test-file\". If\nI run the following:\n\n```\ntest_repo=\"$(mktemp -d /tmp/git-test-XXXXXXX)\"\ncd \"$test_repo\"\n>test-file\n>/tmp/test-file\ngit init\ngit config core.worktree /tmp\ngit add test-file\ngit add /tmp/test-file\n```\n\n.. then with current git I get an index entry for \"/tmp/test-file\" but\nnot \"/tmp/git-test-123456/test-file\". That is, I can add files\noutside the standard worktree location but not inside it.\n\nA playground example of the prior behavior (git 2.25.1) is here:\nhttps://www.onlinegdb.com/OXt2cvL8S\n"},{"id":"454962","messageId":"CABPp-BFwiLXMVaFs-Lc-zU6X=pBiF8+o9rOuHEAArD2zMQ1NNQ@mail.gmail.com","threadId":"57851","inReplyTo":"20220505203234.21586-2-ggossdev@gmail.com","subject":"Re: [RFC PATCH 1/1] dir: consider worktree config in path recursion","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-07T03:26:15Z","receivedAt":"2022-05-07T03:26:35Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, May 5, 2022 at 1:32 PM Goss Geppert wrote:\n>\n> Since 8d92fb2927 (dir: replace exponential algorithm with a linear one,\n> 2020-04-01) the following no longer works:\n>\n> 1) Initialize a repository.\n> 2) Set the `core.worktree` location to a parent directory of the\n> default worktree.\n> 3) Add a file located in the default worktree location to the index\n> (i.e. anywhere in the immediate parent directory of the gitdir).\n>\n> This commit adds a check to determine whether a nested repository that\n> is encountered in recursing a path is actually `the_repository`. If so,\n> simply treat the directory as if it doesn't contain a nested repository.\n>\n> Prior to this commit, the `add` operation was silently ignored.\n> ---\n> dir.c | 37 ++++++++++++++++++++++++++++++-------\n> 1 file changed, 30 insertions(+), 7 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index f2b0f24210..cef39f43d8 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1861,7 +1861,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n> */\n> enum path_treatment state;\n> int matches_how = 0;\n> - int nested_repo = 0, check_only, stop_early;\n> + int check_only, stop_early;\n\nThis part of the patch, along with two other parts below, is\northogonal to the actual fix being made. It probably makes the code\nclearer, but should be done in an independent cleanup commit.\n\n> int old_ignored_nr, old_untracked_nr;\n> /* The \"len-1\" is to strip the final '/' */\n> enum exist_status status = directory_exists_in_index(istate, dirname, len-1);\n> @@ -1893,16 +1893,39 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n>\n> if ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n> !(dir->flags & DIR_NO_GITLINKS)) {\n> + /*\n> + * Determine if `dirname` is a nested repo by confirming that:\n> + * 1) we are in a nonbare repository, and\n> + * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,\n> + * which could occur if the `worktree` location was manually\n> + * configured by the user\n> + */\n\nThis is a good comment, but it'd be really nice to be able to point a\nuser at a testcase in the testsuite for them to inspect further (e.g.\n\"see t####, 'TESTCASE DESCRIPTION' for an case where this matters\").\n\n> + int nested_repo;\n\nJust this line immediately above this comment is part of the orthogonal cleanup.\n\n> struct strbuf sb = STRBUF_INIT;\n> strbuf_addstr(&sb, dirname);\n> nested_repo = is_nonbare_repository_dir(&sb);\n> +\n> + if (nested_repo) {\n> + char *real_dirname, *real_gitdir;\n> + strbuf_reset(&sb);\n> + strbuf_addstr(&sb, dirname);\n> + strbuf_complete(&sb, '/');\n> + strbuf_addstr(&sb, \".git\");\n\n> + real_dirname = real_pathdup(sb.buf, 0);\n> + real_gitdir = real_pathdup(the_repository->gitdir, 0);\n\nI haven't thought it through, but is there a reason you can't just\ncompare the_repository->gitdir to sb.buf at this point? Why is\nreal_pathdup needed?\n\n> +\n> + nested_repo = !!strcmp(real_dirname, real_gitdir);\n> + free(real_gitdir);\n> + free(real_dirname);\n> + }\n\nEverything up to here other than the two parts I mentioned as being\northogonal (both relating to where \"nested_repo\" is defined), makes up\nthe actual fix.\n\n> strbuf_release(&sb);\n> - }\n> - if (nested_repo) {\n> - if ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n> - (matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n> - return path_none;\n> - return excluded ? path_excluded : path_untracked;\n> +\n> + if (nested_repo) {\n> + if ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n> + (matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n> + return path_none;\n> + return excluded ? path_excluded : path_untracked;\n> + }\n\nThis final block is part of the orthogonal code cleanup that belongs\nin a separate commit.\n\n> }\n>\n> if (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES)) {\n> --\n> 2.36.0\n\nThanks for finding, reporting, and sending in a patch. Could you\nsplit up the patch as indicated above, and add a testcase to the patch\nwith the fix, and include your Signed-off-by trailer on the commits?\n"},{"id":"454977","messageId":"CAAA5oLmAMu=Dj8pOgMLGCeYO2c4mrNP38bjdKEVuBkzdb1=R9g@mail.gmail.com","threadId":"57851","inReplyTo":"CABPp-BFwiLXMVaFs-Lc-zU6X=pBiF8+o9rOuHEAArD2zMQ1NNQ@mail.gmail.com","subject":"Re: [RFC PATCH 1/1] dir: consider worktree config in path recursion","fromName":"oss dev","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-07T17:59:14Z","receivedAt":"2022-05-07T17:59:32Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"On Fri, May 6, 2022 at 11:26 PM Elijah Newren wrote:\n> I haven't thought it through, but is there a reason you can't just\n> compare the_repository->gitdir to sb.buf at this point? Why is\n> real_pathdup needed?\n\nI used `real_pathdup` here because `the_repository->gitdir` is not necessarily\nnormalized as required for the `strcmp`. For example, when using option\n`--git-dir=./././xyz/.git` while outside the worktree, the path will remain as\nis (see [1] and [2]).\n\n[1]: https://github.com/git/git/blob/e8005e4871f130c4e402ddca2032c111252f070a/setup.c#L962\n[2]: https://github.com/git/git/blob/e8005e4871f130c4e402ddca2032c111252f070a/environment.c#L353\n\n> Thanks for finding, reporting, and sending in a patch. Could you\n> split up the patch as indicated above, and add a testcase to the patch\n> with the fix, and include your Signed-off-by trailer on the commits?\n\nWIll do, thx.\n"},{"id":"455088","messageId":"20220510171527.25778-1-ggossdev@gmail.com","threadId":"57851","inReplyTo":"20220505203234.21586-1-ggossdev@gmail.com","subject":"[PATCH v2 0/2] dir: consider worktree config in path recursion","fromName":"Goss Geppert","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-10T17:15:25Z","receivedAt":"2022-05-10T17:15:39Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"Updated for requested revisions:\n* Split patch between fix and clean-up\n* Added a test\n* Included my sign-off\n\nIt wasn't clear to me where the testcases should be placed--let me know\nif a different location is more appropriate.\n\n\nGoss Geppert (2):\n dir: consider worktree config in path recursion\n dir: minor refactoring / clean-up\n\n dir.c | 38 ++++++++++++---\n t/t2205-add-worktree-config.sh | 89 ++++++++++++++++++++++++++++++++++\n 2 files changed, 120 insertions(+), 7 deletions(-)\n create mode 100755 t/t2205-add-worktree-config.sh\n\nRange-diff against v1:\n1: b9ca67e61d ! 1: 83bf9f40ac dir: consider worktree config in path recursion\n @@ Commit message\n \n Prior to this commit, the `add` operation was silently ignored.\n \n + Signed-off-by: Goss Geppert \n +\n ## dir.c ##\n @@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,\n - \t */\n - \tenum path_treatment state;\n - \tint matches_how = 0;\n --\tint nested_repo = 0, check_only, stop_early;\n -+\tint check_only, stop_early;\n - \tint old_ignored_nr, old_untracked_nr;\n - \t/* The \"len-1\" is to strip the final '/' */\n - \tenum exist_status status = directory_exists_in_index(istate, dirname, len-1);\n -@@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,\n \n \tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n \t\t!(dir->flags & DIR_NO_GITLINKS)) {\n @@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,\n +\t\t * 1) we are in a nonbare repository, and\n +\t\t * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,\n +\t\t * which could occur if the `worktree` location was manually\n -+\t\t * configured by the user\n ++\t\t * configured by the user; see t2205 testcases 1a-1d for examples\n ++\t\t * where this matters\n +\t\t */\n -+\t\tint nested_repo;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, dirname);\n \t\tnested_repo = is_nonbare_repository_dir(&sb);\n @@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,\n +\t\t\tfree(real_dirname);\n +\t\t}\n \t\tstrbuf_release(&sb);\n --\t}\n --\tif (nested_repo) {\n --\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n --\t\t (matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n --\t\t\treturn path_none;\n --\t\treturn excluded ? path_excluded : path_untracked;\n -+\n -+\t\tif (nested_repo) {\n -+\t\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n -+\t\t\t\t(matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n -+\t\t\t\treturn path_none;\n -+\t\t\treturn excluded ? path_excluded : path_untracked;\n -+\t\t}\n \t}\n - \n - \tif (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES)) {\n + \tif (nested_repo) {\n +\n + ## t/t2205-add-worktree-config.sh (new) ##\n +@@\n ++#!/bin/sh\n ++\n ++test_description='directory traversal respects worktree config\n ++\n ++This test configures the repository`s worktree to be two levels above the\n ++`.git` directory and checks whether we are able to add to the index those files\n ++that are in either (1) the manually configured worktree directory or (2) the\n ++standard worktree location with respect to the `.git` directory (i.e. ensuring\n ++that the encountered `.git` directory is not treated as belonging to a foreign\n ++nested repository)'\n ++\n ++. ./test-lib.sh\n ++\n ++test_expect_success '1a: setup' '\n ++\ttest_create_repo test1 &&\n ++\tgit --git-dir=\"test1/.git\" config core.worktree \"$(pwd)\" &&\n ++\n ++\tmkdir -p outside-tracked outside-untracked &&\n ++\tmkdir -p test1/inside-tracked test1/inside-untracked &&\n ++\t>file-tracked &&\n ++\t>file-untracked &&\n ++\t>outside-tracked/file &&\n ++\t>outside-untracked/file &&\n ++\t>test1/file-tracked &&\n ++\t>test1/file-untracked &&\n ++\t>test1/inside-tracked/file &&\n ++\t>test1/inside-untracked/file &&\n ++\n ++\tcat >expect-tracked-unsorted <<-EOF &&\n ++\t../file-tracked\n ++\t../outside-tracked/file\n ++\tfile-tracked\n ++\tinside-tracked/file\n ++\tEOF\n ++\n ++\tcat >expect-untracked-unsorted <<-EOF &&\n ++\t../file-untracked\n ++\t../outside-untracked/file\n ++\tfile-untracked\n ++\tinside-untracked/file\n ++\tEOF\n ++\n ++\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n ++\n ++\tcat >.gitignore <<-EOF\n ++\t.gitignore\n ++\tactual-*\n ++\texpect-*\n ++\tEOF\n ++'\n ++\n ++test_expect_success '1b: pre-add all' '\n ++\tlocal parent_dir=\"$(pwd)\" &&\n ++\t(\n ++\t\tcd test1 &&\n ++\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-all-unsorted\n ++\t) &&\n ++\tsort actual-all-unsorted >actual-all &&\n ++\tsort expect-all-unsorted >expect-all &&\n ++\ttest_cmp expect-all actual-all\n ++'\n ++\n ++test_expect_success '1c: post-add tracked' '\n ++\tlocal parent_dir=\"$(pwd)\" &&\n ++\t(\n ++\t\tcd test1 &&\n ++\t\tgit add file-tracked &&\n ++\t\tgit add inside-tracked &&\n ++\t\tgit add ../outside-tracked &&\n ++\t\tgit add \"$parent_dir/file-tracked\" &&\n ++\t\tgit ls-files \"$parent_dir\" >../actual-tracked-unsorted\n ++\t) &&\n ++\tsort actual-tracked-unsorted >actual-tracked &&\n ++\tsort expect-tracked-unsorted >expect-tracked &&\n ++\ttest_cmp expect-tracked actual-tracked\n ++'\n ++\n ++test_expect_success '1d: post-add untracked' '\n ++\tlocal parent_dir=\"$(pwd)\" &&\n ++\t(\n ++\t\tcd test1 &&\n ++\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-untracked-unsorted\n ++\t) &&\n ++\tsort actual-untracked-unsorted >actual-untracked &&\n ++\tsort expect-untracked-unsorted >expect-untracked &&\n ++\ttest_cmp expect-untracked actual-untracked\n ++'\n ++\n ++test_done\n-: ---------- > 2: 0da9804776 dir: minor refactoring / clean-up\n-- \n2.36.0\n\n"},{"id":"455089","messageId":"20220510171527.25778-2-ggossdev@gmail.com","threadId":"57851","inReplyTo":"20220510171527.25778-1-ggossdev@gmail.com","subject":"[PATCH v2 1/2] dir: consider worktree config in path recursion","fromName":"Goss Geppert","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-10T17:15:26Z","receivedAt":"2022-05-10T17:15:43Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"Since 8d92fb2927 (dir: replace exponential algorithm with a linear one,\n2020-04-01) the following no longer works:\n\n 1) Initialize a repository.\n 2) Set the `core.worktree` location to a parent directory of the\n default worktree.\n 3) Add a file located in the default worktree location to the index\n (i.e. anywhere in the immediate parent directory of the gitdir).\n\nThis commit adds a check to determine whether a nested repository that\nis encountered in recursing a path is actually `the_repository`. If so,\nsimply treat the directory as if it doesn't contain a nested repository.\n\nPrior to this commit, the `add` operation was silently ignored.\n\nSigned-off-by: Goss Geppert \n---\n dir.c | 22 +++++++++\n t/t2205-add-worktree-config.sh | 89 ++++++++++++++++++++++++++++++++++\n 2 files changed, 111 insertions(+)\n create mode 100755 t/t2205-add-worktree-config.sh\n\ndiff --git a/dir.c b/dir.c\nindex f2b0f24210..a1886e61a3 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1893,9 +1893,31 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \n \tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n \t\t!(dir->flags & DIR_NO_GITLINKS)) {\n+\t\t/*\n+\t\t * Determine if `dirname` is a nested repo by confirming that:\n+\t\t * 1) we are in a nonbare repository, and\n+\t\t * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,\n+\t\t * which could occur if the `worktree` location was manually\n+\t\t * configured by the user; see t2205 testcases 1a-1d for examples\n+\t\t * where this matters\n+\t\t */\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, dirname);\n \t\tnested_repo = is_nonbare_repository_dir(&sb);\n+\n+\t\tif (nested_repo) {\n+\t\t\tchar *real_dirname, *real_gitdir;\n+\t\t\tstrbuf_reset(&sb);\n+\t\t\tstrbuf_addstr(&sb, dirname);\n+\t\t\tstrbuf_complete(&sb, '/');\n+\t\t\tstrbuf_addstr(&sb, \".git\");\n+\t\t\treal_dirname = real_pathdup(sb.buf, 0);\n+\t\t\treal_gitdir = real_pathdup(the_repository->gitdir, 0);\n+\n+\t\t\tnested_repo = !!strcmp(real_dirname, real_gitdir);\n+\t\t\tfree(real_gitdir);\n+\t\t\tfree(real_dirname);\n+\t\t}\n \t\tstrbuf_release(&sb);\n \t}\n \tif (nested_repo) {\ndiff --git a/t/t2205-add-worktree-config.sh b/t/t2205-add-worktree-config.sh\nnew file mode 100755\nindex 0000000000..ca70cf3fe2\n--- /dev/null\n+++ b/t/t2205-add-worktree-config.sh\n@@ -0,0 +1,89 @@\n+#!/bin/sh\n+\n+test_description='directory traversal respects worktree config\n+\n+This test configures the repository`s worktree to be two levels above the\n+`.git` directory and checks whether we are able to add to the index those files\n+that are in either (1) the manually configured worktree directory or (2) the\n+standard worktree location with respect to the `.git` directory (i.e. ensuring\n+that the encountered `.git` directory is not treated as belonging to a foreign\n+nested repository)'\n+\n+. ./test-lib.sh\n+\n+test_expect_success '1a: setup' '\n+\ttest_create_repo test1 &&\n+\tgit --git-dir=\"test1/.git\" config core.worktree \"$(pwd)\" &&\n+\n+\tmkdir -p outside-tracked outside-untracked &&\n+\tmkdir -p test1/inside-tracked test1/inside-untracked &&\n+\t>file-tracked &&\n+\t>file-untracked &&\n+\t>outside-tracked/file &&\n+\t>outside-untracked/file &&\n+\t>test1/file-tracked &&\n+\t>test1/file-untracked &&\n+\t>test1/inside-tracked/file &&\n+\t>test1/inside-untracked/file &&\n+\n+\tcat >expect-tracked-unsorted <<-EOF &&\n+\t../file-tracked\n+\t../outside-tracked/file\n+\tfile-tracked\n+\tinside-tracked/file\n+\tEOF\n+\n+\tcat >expect-untracked-unsorted <<-EOF &&\n+\t../file-untracked\n+\t../outside-untracked/file\n+\tfile-untracked\n+\tinside-untracked/file\n+\tEOF\n+\n+\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n+\n+\tcat >.gitignore <<-EOF\n+\t.gitignore\n+\tactual-*\n+\texpect-*\n+\tEOF\n+'\n+\n+test_expect_success '1b: pre-add all' '\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\t(\n+\t\tcd test1 &&\n+\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-all-unsorted\n+\t) &&\n+\tsort actual-all-unsorted >actual-all &&\n+\tsort expect-all-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+'\n+\n+test_expect_success '1c: post-add tracked' '\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\t(\n+\t\tcd test1 &&\n+\t\tgit add file-tracked &&\n+\t\tgit add inside-tracked &&\n+\t\tgit add ../outside-tracked &&\n+\t\tgit add \"$parent_dir/file-tracked\" &&\n+\t\tgit ls-files \"$parent_dir\" >../actual-tracked-unsorted\n+\t) &&\n+\tsort actual-tracked-unsorted >actual-tracked &&\n+\tsort expect-tracked-unsorted >expect-tracked &&\n+\ttest_cmp expect-tracked actual-tracked\n+'\n+\n+test_expect_success '1d: post-add untracked' '\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\t(\n+\t\tcd test1 &&\n+\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-untracked-unsorted\n+\t) &&\n+\tsort actual-untracked-unsorted >actual-untracked &&\n+\tsort expect-untracked-unsorted >expect-untracked &&\n+\ttest_cmp expect-untracked actual-untracked\n+'\n+\n+test_done\n-- \n2.36.0\n\n"},{"id":"455090","messageId":"20220510171527.25778-3-ggossdev@gmail.com","threadId":"57851","inReplyTo":"20220510171527.25778-1-ggossdev@gmail.com","subject":"[PATCH v2 2/2] dir: minor refactoring / clean-up","fromName":"Goss Geppert","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-10T17:15:27Z","receivedAt":"2022-05-10T17:15:46Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"Improve readability.\n\nSigned-off-by: Goss Geppert \n---\n dir.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex a1886e61a3..329db72999 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1861,7 +1861,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t */\n \tenum path_treatment state;\n \tint matches_how = 0;\n-\tint nested_repo = 0, check_only, stop_early;\n+\tint check_only, stop_early;\n \tint old_ignored_nr, old_untracked_nr;\n \t/* The \"len-1\" is to strip the final '/' */\n \tenum exist_status status = directory_exists_in_index(istate, dirname, len-1);\n@@ -1901,6 +1901,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\t * configured by the user; see t2205 testcases 1a-1d for examples\n \t\t * where this matters\n \t\t */\n+\t\tint nested_repo;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, dirname);\n \t\tnested_repo = is_nonbare_repository_dir(&sb);\n@@ -1919,12 +1920,13 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\t\tfree(real_dirname);\n \t\t}\n \t\tstrbuf_release(&sb);\n-\t}\n-\tif (nested_repo) {\n-\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n-\t\t (matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n-\t\t\treturn path_none;\n-\t\treturn excluded ? path_excluded : path_untracked;\n+\n+\t\tif (nested_repo) {\n+\t\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n+\t\t\t\t(matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n+\t\t\t\treturn path_none;\n+\t\t\treturn excluded ? path_excluded : path_untracked;\n+\t\t}\n \t}\n \n \tif (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES)) {\n-- \n2.36.0\n\n"},{"id":"455166","messageId":"xmqq7d6sm3e0.fsf@gitster.g","threadId":"57851","inReplyTo":"20220510171527.25778-2-ggossdev@gmail.com","subject":"Re: [PATCH v2 1/2] dir: consider worktree config in path recursion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-11T16:37:43Z","receivedAt":"2022-05-11T16:37:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Goss Geppert writes:\n\n> diff --git a/dir.c b/dir.c\n> index f2b0f24210..a1886e61a3 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1893,9 +1893,31 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n\n> \tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n> \t\t!(dir->flags & DIR_NO_GITLINKS)) {\n> +\t\t/*\n> +\t\t * Determine if `dirname` is a nested repo by confirming that:\n> +\t\t * 1) we are in a nonbare repository, and\n> +\t\t * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,\n> +\t\t * which could occur if the `worktree` location was manually\n> +\t\t * configured by the user; see t2205 testcases 1a-1d for examples\n> +\t\t * where this matters\n> +\t\t */\n> \t\tstruct strbuf sb = STRBUF_INIT;\n> \t\tstrbuf_addstr(&sb, dirname);\n> \t\tnested_repo = is_nonbare_repository_dir(&sb);\n> +\n> +\t\tif (nested_repo) {\n> +\t\t\tchar *real_dirname, *real_gitdir;\n> +\t\t\tstrbuf_reset(&sb);\n> +\t\t\tstrbuf_addstr(&sb, dirname);\n> +\t\t\tstrbuf_complete(&sb, '/');\n> +\t\t\tstrbuf_addstr(&sb, \".git\");\n> +\t\t\treal_dirname = real_pathdup(sb.buf, 0);\n\nIs this _complete() necessary?\n\nNear the top of this function there is\n\n\t/* The \"len-1\" is to strip the final '/' */\n\tenum exist_status status = directory_exists_in_index(istate, dirname, len-1);\n\nwhich indicates that we are safe to assume dirname ends with '/'.\n\n> +\t\t\treal_gitdir = real_pathdup(the_repository->gitdir, 0);\n\nThis function is repeatedly called during the traversal.\n\nHow expensive is it to keep calling real_pathdup() on the constant\nthe_repository->gitdir just in case it might be the same as our true\nGIT_DIR?\n\n> +\n> +\t\t\tnested_repo = !!strcmp(real_dirname, real_gitdir);\n> +\t\t\tfree(real_gitdir);\n> +\t\t\tfree(real_dirname);\n> +\t\t}\n> \t\tstrbuf_release(&sb);\n> \t}\n> \tif (nested_repo) {\n> diff --git a/t/t2205-add-worktree-config.sh b/t/t2205-add-worktree-config.sh\n> new file mode 100755\n> index 0000000000..ca70cf3fe2\n> --- /dev/null\n> +++ b/t/t2205-add-worktree-config.sh\n> @@ -0,0 +1,89 @@\n> +#!/bin/sh\n> +\n> +test_description='directory traversal respects worktree config\n> +\n> +This test configures the repository`s worktree to be two levels above the\n> +`.git` directory and checks whether we are able to add to the index those files\n> +that are in either (1) the manually configured worktree directory or (2) the\n> +standard worktree location with respect to the `.git` directory (i.e. ensuring\n> +that the encountered `.git` directory is not treated as belonging to a foreign\n> +nested repository)'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success '1a: setup' '\n> +\ttest_create_repo test1 &&\n> +\tgit --git-dir=\"test1/.git\" config core.worktree \"$(pwd)\" &&\n> +\n> +\tmkdir -p outside-tracked outside-untracked &&\n> +\tmkdir -p test1/inside-tracked test1/inside-untracked &&\n> +\t>file-tracked &&\n> +\t>file-untracked &&\n> +\t>outside-tracked/file &&\n> +\t>outside-untracked/file &&\n> +\t>test1/file-tracked &&\n> +\t>test1/file-untracked &&\n> +\t>test1/inside-tracked/file &&\n> +\t>test1/inside-untracked/file &&\n> +\n> +\tcat >expect-tracked-unsorted <<-EOF &&\n> +\t../file-tracked\n> +\t../outside-tracked/file\n> +\tfile-tracked\n> +\tinside-tracked/file\n> +\tEOF\n> +\n> +\tcat >expect-untracked-unsorted <<-EOF &&\n> +\t../file-untracked\n> +\t../outside-untracked/file\n> +\tfile-untracked\n> +\tinside-untracked/file\n> +\tEOF\n> +\n> +\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n> +\n> +\tcat >.gitignore <<-EOF\n> +\t.gitignore\n> +\tactual-*\n> +\texpect-*\n> +\tEOF\n> +'\n> +\n> +test_expect_success '1b: pre-add all' '\n> +\tlocal parent_dir=\"$(pwd)\" &&\n> +\t(\n> +\t\tcd test1 &&\n> +\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-all-unsorted\n> +\t) &&\n> +\tsort actual-all-unsorted >actual-all &&\n> +\tsort expect-all-unsorted >expect-all &&\n> +\ttest_cmp expect-all actual-all\n> +'\n> +\n> +test_expect_success '1c: post-add tracked' '\n> +\tlocal parent_dir=\"$(pwd)\" &&\n> +\t(\n> +\t\tcd test1 &&\n> +\t\tgit add file-tracked &&\n> +\t\tgit add inside-tracked &&\n> +\t\tgit add ../outside-tracked &&\n> +\t\tgit add \"$parent_dir/file-tracked\" &&\n> +\t\tgit ls-files \"$parent_dir\" >../actual-tracked-unsorted\n> +\t) &&\n> +\tsort actual-tracked-unsorted >actual-tracked &&\n> +\tsort expect-tracked-unsorted >expect-tracked &&\n> +\ttest_cmp expect-tracked actual-tracked\n> +'\n> +\n> +test_expect_success '1d: post-add untracked' '\n> +\tlocal parent_dir=\"$(pwd)\" &&\n> +\t(\n> +\t\tcd test1 &&\n> +\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-untracked-unsorted\n> +\t) &&\n> +\tsort actual-untracked-unsorted >actual-untracked &&\n> +\tsort expect-untracked-unsorted >expect-untracked &&\n> +\ttest_cmp expect-untracked actual-untracked\n> +'\n> +\n> +test_done\n"},{"id":"455167","messageId":"xmqqv8ucko6v.fsf@gitster.g","threadId":"57851","inReplyTo":"20220510171527.25778-3-ggossdev@gmail.com","subject":"Re: [PATCH v2 2/2] dir: minor refactoring / clean-up","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-11T16:51:20Z","receivedAt":"2022-05-11T16:52:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Goss Geppert writes:\n\n> Improve readability.\n\nIt reads somewhat a subjective opinion, without explaining how/why\nthe change makes it more readable. Perhaps\n\n\tNarrow the scope of the nested_repo variable to the block\n\tthat uses it.\n\nor something?\n\nI was hoping that we can/should apply these two patches near where\nthe breakage happened, but unfortunately this part has been updated\na bit since then, so merging upwards would involve a bit of conflict\nresolution. We should be able to manage.\n\nThanks.\n\n\n> Signed-off-by: Goss Geppert \n> ---\n> dir.c | 16 +++++++++-------\n> 1 file changed, 9 insertions(+), 7 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index a1886e61a3..329db72999 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1861,7 +1861,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n> \t */\n> \tenum path_treatment state;\n> \tint matches_how = 0;\n> -\tint nested_repo = 0, check_only, stop_early;\n> +\tint check_only, stop_early;\n> \tint old_ignored_nr, old_untracked_nr;\n> \t/* The \"len-1\" is to strip the final '/' */\n> \tenum exist_status status = directory_exists_in_index(istate, dirname, len-1);\n> @@ -1901,6 +1901,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n> \t\t * configured by the user; see t2205 testcases 1a-1d for examples\n> \t\t * where this matters\n> \t\t */\n> +\t\tint nested_repo;\n> \t\tstruct strbuf sb = STRBUF_INIT;\n> \t\tstrbuf_addstr(&sb, dirname);\n> \t\tnested_repo = is_nonbare_repository_dir(&sb);\n> @@ -1919,12 +1920,13 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n> \t\t\tfree(real_dirname);\n> \t\t}\n> \t\tstrbuf_release(&sb);\n> -\t}\n> -\tif (nested_repo) {\n> -\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n> -\t\t (matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n> -\t\t\treturn path_none;\n> -\t\treturn excluded ? path_excluded : path_untracked;\n> +\n> +\t\tif (nested_repo) {\n> +\t\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n> +\t\t\t\t(matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n> +\t\t\t\treturn path_none;\n> +\t\t\treturn excluded ? path_excluded : path_untracked;\n> +\t\t}\n> \t}\n> \n> \tif (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES)) {\n"},{"id":"455185","messageId":"xmqqsfpfhdnh.fsf@gitster.g","threadId":"57851","inReplyTo":"20220510171527.25778-2-ggossdev@gmail.com","subject":"Re: [PATCH v2 1/2] dir: consider worktree config in path recursion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-11T23:07:14Z","receivedAt":"2022-05-11T23:07:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Goss Geppert writes:\n\n> Since 8d92fb2927 (dir: replace exponential algorithm with a linear one,\n> 2020-04-01) the following no longer works:\n>\n> 1) Initialize a repository.\n> 2) Set the `core.worktree` location to a parent directory of the\n> default worktree.\n> 3) Add a file located in the default worktree location to the index\n> (i.e. anywhere in the immediate parent directory of the gitdir).\n\nI think I've mentioned this ealier, but the above only describes the\nscenario and does not say what behaviour is expected and what behaviour\nis observed. \"no longer works\" is OK, but not sufficient.\n\nEasily remedied by saying \"It used to do X, but now it does Y\" after\nthe above description of the scenario to describe what happens and\nwhat you want to happen.\n\n> +\tgit --git-dir=\"test1/.git\" config core.worktree \"$(pwd)\" &&\n\nI wonder if this lets funny paths to be added to the index, e.g.\nwould \"git add test1\" recursively add everything in that directory?\n\nThanks.\n\n"},{"id":"455620","messageId":"20220520192840.8942-1-ggossdev@gmail.com","threadId":"57851","inReplyTo":"20220505203234.21586-1-ggossdev@gmail.com","subject":"[PATCH v3 0/3] dir: traverse into repository","fromName":"Goss Geppert","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-20T19:28:37Z","receivedAt":"2022-05-20T19:29:19Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"Thanks for your feedback so far. This revision includes a number of changes:\n\n* I realized that using option `--git-dir` (or environment variable `GIT_DIR`)\n when the working directory is outside the repository can also cause the\n directory traversal to fail to enter `the_repository`. I've added test cases\n and updated comments / messages to reflect this.\n* I noticed that `real_pathdup` can fail even for valid paths (and return NULL)\n if e.g. the path contains too many nested symlinks (currently 32). We'll now\n `die_on_error`.\n* I no longer reset the string buffer holding the `dirname` and just reuse it.\n While the prior call to `is_nonbare_repository_dir` does not treat its\n argument as const, the function does reset the argument to its effective\n input state. The `strbuf_addstr` and `strbuf_complete` have also been\n removed.\n* I added a separate commit to cache the `real_gitdir` value. On my mystery\n machine, the call to `real_pathdup`, instrumented with trace2, takes an\n anecdotal 40 usecs for a somewhat lengthy path [1]. This compares to around\n 10 usecs for an empty trace2 region. I can remove the commit if you don't\n think the performance savings are worth the clutter.\n\nLet me know if you have further comments or suggestions.\n\n[1]: path with one symlink: './repo/link/a/b/././././..//////c/d/./xyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxy/././../e/../f/../g/../h/../././///../../..'\n\nGoss Geppert (3):\n dir: traverse into repository\n dir: cache git_dir's realpath\n dir: minor refactoring / clean-up\n\n dir.c | 90 ++++++++---\n t/t2205-add-worktree-config.sh | 274 +++++++++++++++++++++++++++++++++\n 2 files changed, 341 insertions(+), 23 deletions(-)\n create mode 100755 t/t2205-add-worktree-config.sh\n\nRange-diff against v2:\n1: 26e6a878cd ! 1: 391fd4d8cf dir: consider worktree config in path recursion\n @@ Metadata\n Author: Goss Geppert \n \n ## Commit message ##\n - dir: consider worktree config in path recursion\n + dir: traverse into repository\n \n Since 8d92fb2927 (dir: replace exponential algorithm with a linear one,\n - 2020-04-01) the following no longer works:\n + 2020-04-01) traversing into a repository's directory tree when the\n + traversal began outside the repository's standard location has failed\n + because the encountered repository was identified as a nested foreign\n + repository.\n \n - 1) Initialize a repository.\n - 2) Set the `core.worktree` location to a parent directory of the\n - default worktree.\n - 3) Add a file located in the default worktree location to the index\n - (i.e. anywhere in the immediate parent directory of the gitdir).\n + Prior to this commit, the failure to traverse into a repository's\n + default worktree location was observable from a user's perspective under\n + either of the following conditions (there may be others):\n +\n + 1) Set the `core.worktree` location to a parent directory of the\n + default worktree; or\n + 2) Use the `--git_dir` option while the working directory is outside\n + the repository's default worktree location\n +\n + Under either of these conditions, symptoms of the failure to traverse\n + into the repository's default worktree location include the inability to\n + add files to the index or get a list of untracked files via ls-files.\n \n This commit adds a check to determine whether a nested repository that\n is encountered in recursing a path is actually `the_repository`. If so,\n - simply treat the directory as if it doesn't contain a nested repository.\n + we simply treat the directory as if it doesn't contain a nested\n + repository.\n \n - Prior to this commit, the `add` operation was silently ignored.\n + The commit includes test-cases to reduce the likelihood of future\n + regressions.\n \n Signed-off-by: Goss Geppert \n \n @@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,\n +\t\t * Determine if `dirname` is a nested repo by confirming that:\n +\t\t * 1) we are in a nonbare repository, and\n +\t\t * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,\n -+\t\t * which could occur if the `worktree` location was manually\n -+\t\t * configured by the user; see t2205 testcases 1a-1d for examples\n -+\t\t * where this matters\n ++\t\t * which could occur if the git_dir or worktree location was\n ++\t\t * manually configured by the user; see t2205 testcases 1-3 for\n ++\t\t * examples where this matters\n +\t\t */\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, dirname);\n @@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,\n +\n +\t\tif (nested_repo) {\n +\t\t\tchar *real_dirname, *real_gitdir;\n -+\t\t\tstrbuf_reset(&sb);\n -+\t\t\tstrbuf_addstr(&sb, dirname);\n -+\t\t\tstrbuf_complete(&sb, '/');\n +\t\t\tstrbuf_addstr(&sb, \".git\");\n -+\t\t\treal_dirname = real_pathdup(sb.buf, 0);\n -+\t\t\treal_gitdir = real_pathdup(the_repository->gitdir, 0);\n ++\t\t\treal_dirname = real_pathdup(sb.buf, 1);\n ++\t\t\treal_gitdir = real_pathdup(the_repository->gitdir, 1);\n +\n +\t\t\tnested_repo = !!strcmp(real_dirname, real_gitdir);\n +\t\t\tfree(real_gitdir);\n @@ t/t2205-add-worktree-config.sh (new)\n @@\n +#!/bin/sh\n +\n -+test_description='directory traversal respects worktree config\n ++test_description='directory traversal respects user config\n ++\n ++This test verifies the traversal of the directory tree when the traversal begins\n ++outside the repository. Two instances for which this can occur are tested:\n +\n -+This test configures the repository`s worktree to be two levels above the\n -+`.git` directory and checks whether we are able to add to the index those files\n -+that are in either (1) the manually configured worktree directory or (2) the\n -+standard worktree location with respect to the `.git` directory (i.e. ensuring\n -+that the encountered `.git` directory is not treated as belonging to a foreign\n -+nested repository)'\n ++\t1) The user manually sets the worktree. For this instance, the test sets\n ++\t the worktree two levels above the `.git` directory and checks whether we\n ++\t are able to add to the index those files that are in either (1) the\n ++\t manually configured worktree directory or (2) the standard worktree\n ++\t location with respect to the `.git` directory (i.e. ensuring that the\n ++\t encountered `.git` directory is not treated as belonging to a foreign\n ++\t nested repository).\n ++\t2) The user manually sets the `git_dir` while the working directory is\n ++\t outside the repository. The test checks that files inside the\n ++\t repository can be added to the index.\n ++\t'\n +\n +. ./test-lib.sh\n +\n -+test_expect_success '1a: setup' '\n -+\ttest_create_repo test1 &&\n -+\tgit --git-dir=\"test1/.git\" config core.worktree \"$(pwd)\" &&\n ++test_expect_success '1a: setup--config worktree' '\n ++\tmkdir test1 &&\n ++\t(\n ++\tcd test1 &&\n ++\ttest_create_repo repo &&\n ++\tgit --git-dir=\"repo/.git\" config core.worktree \"$(pwd)\" &&\n +\n +\tmkdir -p outside-tracked outside-untracked &&\n -+\tmkdir -p test1/inside-tracked test1/inside-untracked &&\n ++\tmkdir -p repo/inside-tracked repo/inside-untracked &&\n +\t>file-tracked &&\n +\t>file-untracked &&\n +\t>outside-tracked/file &&\n +\t>outside-untracked/file &&\n -+\t>test1/file-tracked &&\n -+\t>test1/file-untracked &&\n -+\t>test1/inside-tracked/file &&\n -+\t>test1/inside-untracked/file &&\n ++\t>repo/file-tracked &&\n ++\t>repo/file-untracked &&\n ++\t>repo/inside-tracked/file &&\n ++\t>repo/inside-untracked/file &&\n +\n +\tcat >expect-tracked-unsorted <<-EOF &&\n +\t../file-tracked\n @@ t/t2205-add-worktree-config.sh (new)\n +\tinside-untracked/file\n +\tEOF\n +\n ++\tcat >expect-all-dir-unsorted <<-EOF &&\n ++\t../file-untracked\n ++\t../file-tracked\n ++\t../outside-untracked/\n ++\t../outside-tracked/\n ++\t./\n ++\tEOF\n ++\n +\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n +\n +\tcat >.gitignore <<-EOF\n @@ t/t2205-add-worktree-config.sh (new)\n +\tactual-*\n +\texpect-*\n +\tEOF\n ++\t)\n +'\n +\n +test_expect_success '1b: pre-add all' '\n ++\t(\n ++\tcd test1 &&\n +\tlocal parent_dir=\"$(pwd)\" &&\n +\t(\n -+\t\tcd test1 &&\n ++\t\tcd repo &&\n +\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-all-unsorted\n +\t) &&\n +\tsort actual-all-unsorted >actual-all &&\n +\tsort expect-all-unsorted >expect-all &&\n +\ttest_cmp expect-all actual-all\n ++\t)\n +'\n +\n -+test_expect_success '1c: post-add tracked' '\n ++test_expect_success '1c: pre-add dir all' '\n ++\t(\n ++\tcd test1 &&\n +\tlocal parent_dir=\"$(pwd)\" &&\n +\t(\n -+\t\tcd test1 &&\n ++\t\tcd repo &&\n ++\t\tgit ls-files -o --directory --exclude-standard \"$parent_dir\" >../actual-all-dir-unsorted\n ++\t) &&\n ++\tsort actual-all-dir-unsorted >actual-all &&\n ++\tsort expect-all-dir-unsorted >expect-all &&\n ++\ttest_cmp expect-all actual-all\n ++\t)\n ++'\n ++\n ++test_expect_success '1d: post-add tracked' '\n ++\t(\n ++\tcd test1 &&\n ++\tlocal parent_dir=\"$(pwd)\" &&\n ++\t(\n ++\t\tcd repo &&\n +\t\tgit add file-tracked &&\n +\t\tgit add inside-tracked &&\n +\t\tgit add ../outside-tracked &&\n @@ t/t2205-add-worktree-config.sh (new)\n +\tsort actual-tracked-unsorted >actual-tracked &&\n +\tsort expect-tracked-unsorted >expect-tracked &&\n +\ttest_cmp expect-tracked actual-tracked\n ++\t)\n +'\n +\n -+test_expect_success '1d: post-add untracked' '\n ++test_expect_success '1e: post-add untracked' '\n ++\t(\n ++\tcd test1 &&\n +\tlocal parent_dir=\"$(pwd)\" &&\n +\t(\n -+\t\tcd test1 &&\n ++\t\tcd repo &&\n +\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-untracked-unsorted\n +\t) &&\n +\tsort actual-untracked-unsorted >actual-untracked &&\n +\tsort expect-untracked-unsorted >expect-untracked &&\n +\ttest_cmp expect-untracked actual-untracked\n ++\t)\n ++'\n ++\n ++test_expect_success '2a: setup--set git-dir' '\n ++\tmkdir test2 &&\n ++\t(\n ++\tcd test2 &&\n ++\ttest_create_repo repo &&\n ++\t# create two foreign repositories that should remain untracked\n ++\ttest_create_repo repo-outside &&\n ++\ttest_create_repo repo/repo-inside &&\n ++\n ++\tmkdir -p repo/inside-tracked repo/inside-untracked &&\n ++\t>repo/file-tracked &&\n ++\t>repo/file-untracked &&\n ++\t>repo/inside-tracked/file &&\n ++\t>repo/inside-untracked/file &&\n ++\t>repo-outside/file &&\n ++\t>repo/repo-inside/file &&\n ++\n ++\tcat >expect-tracked-unsorted <<-EOF &&\n ++\trepo/file-tracked\n ++\trepo/inside-tracked/file\n ++\tEOF\n ++\n ++\tcat >expect-untracked-unsorted <<-EOF &&\n ++\trepo/file-untracked\n ++\trepo/inside-untracked/file\n ++\trepo/repo-inside/\n ++\trepo-outside/\n ++\tEOF\n ++\n ++\tcat >expect-all-dir-unsorted <<-EOF &&\n ++\trepo/\n ++\trepo-outside/\n ++\tEOF\n ++\n ++\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n ++\n ++\tcat >.gitignore <<-EOF\n ++\t.gitignore\n ++\tactual-*\n ++\texpect-*\n ++\tEOF\n ++\t)\n ++'\n ++\n ++test_expect_success '2b: pre-add all' '\n ++\t(\n ++\tcd test2 &&\n ++\tgit --git-dir=repo/.git ls-files -o --exclude-standard >actual-all-unsorted &&\n ++\tsort actual-all-unsorted >actual-all &&\n ++\tsort expect-all-unsorted >expect-all &&\n ++\ttest_cmp expect-all actual-all\n ++\t)\n ++'\n ++\n ++test_expect_success '2c: pre-add dir all' '\n ++\t(\n ++\tcd test2 &&\n ++\tgit --git-dir=repo/.git ls-files -o --directory --exclude-standard >actual-all-dir-unsorted &&\n ++\tsort actual-all-dir-unsorted >actual-all &&\n ++\tsort expect-all-dir-unsorted >expect-all &&\n ++\ttest_cmp expect-all actual-all\n ++\t)\n ++'\n ++\n ++test_expect_success '2d: post-add tracked' '\n ++\t(\n ++\tcd test2 &&\n ++\tgit --git-dir=repo/.git add repo/file-tracked &&\n ++\tgit --git-dir=repo/.git add repo/inside-tracked &&\n ++\tgit --git-dir=repo/.git ls-files >actual-tracked-unsorted &&\n ++\tsort actual-tracked-unsorted >actual-tracked &&\n ++\tsort expect-tracked-unsorted >expect-tracked &&\n ++\ttest_cmp expect-tracked actual-tracked\n ++\t)\n ++'\n ++\n ++test_expect_success '2e: post-add untracked' '\n ++\t(\n ++\tcd test2 &&\n ++\tgit --git-dir=repo/.git ls-files -o --exclude-standard >actual-untracked-unsorted &&\n ++\tsort actual-untracked-unsorted >actual-untracked &&\n ++\tsort expect-untracked-unsorted >expect-untracked &&\n ++\ttest_cmp expect-untracked actual-untracked\n ++\t)\n ++'\n ++\n ++test_expect_success '3a: setup--add repo dir' '\n ++\tmkdir test3 &&\n ++\t(\n ++\tcd test3 &&\n ++\ttest_create_repo repo &&\n ++\n ++\tmkdir -p repo/inside-tracked repo/inside-ignored &&\n ++\t>repo/file-tracked &&\n ++\t>repo/file-ignored &&\n ++\t>repo/inside-tracked/file &&\n ++\t>repo/inside-ignored/file &&\n ++\n ++\tcat >.gitignore <<-EOF &&\n ++\t.gitignore\n ++\tactual-*\n ++\texpect-*\n ++\t*ignored\n ++\tEOF\n ++\n ++\tcat >expect-tracked-unsorted <<-EOF &&\n ++\trepo/file-tracked\n ++\trepo/inside-tracked/file\n ++\tEOF\n ++\n ++\tcat >expect-ignored-unsorted <<-EOF\n ++\trepo/file-ignored\n ++\trepo/inside-ignored/\n ++\t.gitignore\n ++\tactual-ignored-unsorted\n ++\texpect-ignored-unsorted\n ++\texpect-tracked-unsorted\n ++\tEOF\n ++\t)\n ++'\n ++\n ++test_expect_success '3b: ignored' '\n ++\t(\n ++\tcd test3 &&\n ++\tgit --git-dir=repo/.git ls-files -io --directory --exclude-standard >actual-ignored-unsorted &&\n ++\tsort actual-ignored-unsorted >actual-ignored &&\n ++\tsort expect-ignored-unsorted >expect-ignored &&\n ++\ttest_cmp expect-ignored actual-ignored\n ++\t)\n ++'\n ++\n ++test_expect_success '3c: add repo' '\n ++\t(\n ++\tcd test3 &&\n ++\tgit --git-dir=repo/.git add repo &&\n ++\tgit --git-dir=repo/.git ls-files >actual-tracked-unsorted &&\n ++\tsort actual-tracked-unsorted >actual-tracked &&\n ++\tsort expect-tracked-unsorted >expect-tracked &&\n ++\ttest_cmp expect-tracked actual-tracked\n ++\t)\n +'\n +\n +test_done\n-: ---------- > 2: 16e87a0345 dir: cache git_dir's realpath\n2: 2e0a178c78 ! 3: 6de7932731 dir: minor refactoring / clean-up\n @@ Metadata\n ## Commit message ##\n dir: minor refactoring / clean-up\n \n - Improve readability.\n + Narrow the scope of the `nested_repo` variable and conditional return\n + statement to the block where the variable is set.\n \n Signed-off-by: Goss Geppert \n \n @@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,\n \t/* The \"len-1\" is to strip the final '/' */\n \tenum exist_status status = directory_exists_in_index(istate, dirname, len-1);\n @@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,\n - \t\t * configured by the user; see t2205 testcases 1a-1d for examples\n - \t\t * where this matters\n + \t\t * manually configured by the user; see t2205 testcases 1-3 for\n + \t\t * examples where this matters\n \t\t */\n +\t\tint nested_repo;\n \t\tstruct strbuf sb = STRBUF_INIT;\n-- \n2.36.0\n\n"},{"id":"455621","messageId":"20220520192840.8942-2-ggossdev@gmail.com","threadId":"57851","inReplyTo":"20220520192840.8942-1-ggossdev@gmail.com","subject":"[PATCH v3 1/3] dir: traverse into repository","fromName":"Goss Geppert","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-20T19:28:38Z","receivedAt":"2022-05-20T19:29:21Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"Since 8d92fb2927 (dir: replace exponential algorithm with a linear one,\n2020-04-01) traversing into a repository's directory tree when the\ntraversal began outside the repository's standard location has failed\nbecause the encountered repository was identified as a nested foreign\nrepository.\n\nPrior to this commit, the failure to traverse into a repository's\ndefault worktree location was observable from a user's perspective under\neither of the following conditions (there may be others):\n\n 1) Set the `core.worktree` location to a parent directory of the\n default worktree; or\n 2) Use the `--git_dir` option while the working directory is outside\n the repository's default worktree location\n\nUnder either of these conditions, symptoms of the failure to traverse\ninto the repository's default worktree location include the inability to\nadd files to the index or get a list of untracked files via ls-files.\n\nThis commit adds a check to determine whether a nested repository that\nis encountered in recursing a path is actually `the_repository`. If so,\nwe simply treat the directory as if it doesn't contain a nested\nrepository.\n\nThe commit includes test-cases to reduce the likelihood of future\nregressions.\n\nSigned-off-by: Goss Geppert \n---\n dir.c | 19 +++\n t/t2205-add-worktree-config.sh | 274 +++++++++++++++++++++++++++++++++\n 2 files changed, 293 insertions(+)\n create mode 100755 t/t2205-add-worktree-config.sh\n\ndiff --git a/dir.c b/dir.c\nindex 26c4d141ab..e45e117875 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1893,9 +1893,28 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \n \tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n \t\t!(dir->flags & DIR_NO_GITLINKS)) {\n+\t\t/*\n+\t\t * Determine if `dirname` is a nested repo by confirming that:\n+\t\t * 1) we are in a nonbare repository, and\n+\t\t * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,\n+\t\t * which could occur if the git_dir or worktree location was\n+\t\t * manually configured by the user; see t2205 testcases 1-3 for\n+\t\t * examples where this matters\n+\t\t */\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, dirname);\n \t\tnested_repo = is_nonbare_repository_dir(&sb);\n+\n+\t\tif (nested_repo) {\n+\t\t\tchar *real_dirname, *real_gitdir;\n+\t\t\tstrbuf_addstr(&sb, \".git\");\n+\t\t\treal_dirname = real_pathdup(sb.buf, 1);\n+\t\t\treal_gitdir = real_pathdup(the_repository->gitdir, 1);\n+\n+\t\t\tnested_repo = !!strcmp(real_dirname, real_gitdir);\n+\t\t\tfree(real_gitdir);\n+\t\t\tfree(real_dirname);\n+\t\t}\n \t\tstrbuf_release(&sb);\n \t}\n \tif (nested_repo) {\ndiff --git a/t/t2205-add-worktree-config.sh b/t/t2205-add-worktree-config.sh\nnew file mode 100755\nindex 0000000000..056894eb94\n--- /dev/null\n+++ b/t/t2205-add-worktree-config.sh\n@@ -0,0 +1,274 @@\n+#!/bin/sh\n+\n+test_description='directory traversal respects user config\n+\n+This test verifies the traversal of the directory tree when the traversal begins\n+outside the repository. Two instances for which this can occur are tested:\n+\n+\t1) The user manually sets the worktree. For this instance, the test sets\n+\t the worktree two levels above the `.git` directory and checks whether we\n+\t are able to add to the index those files that are in either (1) the\n+\t manually configured worktree directory or (2) the standard worktree\n+\t location with respect to the `.git` directory (i.e. ensuring that the\n+\t encountered `.git` directory is not treated as belonging to a foreign\n+\t nested repository).\n+\t2) The user manually sets the `git_dir` while the working directory is\n+\t outside the repository. The test checks that files inside the\n+\t repository can be added to the index.\n+\t'\n+\n+. ./test-lib.sh\n+\n+test_expect_success '1a: setup--config worktree' '\n+\tmkdir test1 &&\n+\t(\n+\tcd test1 &&\n+\ttest_create_repo repo &&\n+\tgit --git-dir=\"repo/.git\" config core.worktree \"$(pwd)\" &&\n+\n+\tmkdir -p outside-tracked outside-untracked &&\n+\tmkdir -p repo/inside-tracked repo/inside-untracked &&\n+\t>file-tracked &&\n+\t>file-untracked &&\n+\t>outside-tracked/file &&\n+\t>outside-untracked/file &&\n+\t>repo/file-tracked &&\n+\t>repo/file-untracked &&\n+\t>repo/inside-tracked/file &&\n+\t>repo/inside-untracked/file &&\n+\n+\tcat >expect-tracked-unsorted <<-EOF &&\n+\t../file-tracked\n+\t../outside-tracked/file\n+\tfile-tracked\n+\tinside-tracked/file\n+\tEOF\n+\n+\tcat >expect-untracked-unsorted <<-EOF &&\n+\t../file-untracked\n+\t../outside-untracked/file\n+\tfile-untracked\n+\tinside-untracked/file\n+\tEOF\n+\n+\tcat >expect-all-dir-unsorted <<-EOF &&\n+\t../file-untracked\n+\t../file-tracked\n+\t../outside-untracked/\n+\t../outside-tracked/\n+\t./\n+\tEOF\n+\n+\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n+\n+\tcat >.gitignore <<-EOF\n+\t.gitignore\n+\tactual-*\n+\texpect-*\n+\tEOF\n+\t)\n+'\n+\n+test_expect_success '1b: pre-add all' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-all-unsorted\n+\t) &&\n+\tsort actual-all-unsorted >actual-all &&\n+\tsort expect-all-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '1c: pre-add dir all' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit ls-files -o --directory --exclude-standard \"$parent_dir\" >../actual-all-dir-unsorted\n+\t) &&\n+\tsort actual-all-dir-unsorted >actual-all &&\n+\tsort expect-all-dir-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '1d: post-add tracked' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit add file-tracked &&\n+\t\tgit add inside-tracked &&\n+\t\tgit add ../outside-tracked &&\n+\t\tgit add \"$parent_dir/file-tracked\" &&\n+\t\tgit ls-files \"$parent_dir\" >../actual-tracked-unsorted\n+\t) &&\n+\tsort actual-tracked-unsorted >actual-tracked &&\n+\tsort expect-tracked-unsorted >expect-tracked &&\n+\ttest_cmp expect-tracked actual-tracked\n+\t)\n+'\n+\n+test_expect_success '1e: post-add untracked' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-untracked-unsorted\n+\t) &&\n+\tsort actual-untracked-unsorted >actual-untracked &&\n+\tsort expect-untracked-unsorted >expect-untracked &&\n+\ttest_cmp expect-untracked actual-untracked\n+\t)\n+'\n+\n+test_expect_success '2a: setup--set git-dir' '\n+\tmkdir test2 &&\n+\t(\n+\tcd test2 &&\n+\ttest_create_repo repo &&\n+\t# create two foreign repositories that should remain untracked\n+\ttest_create_repo repo-outside &&\n+\ttest_create_repo repo/repo-inside &&\n+\n+\tmkdir -p repo/inside-tracked repo/inside-untracked &&\n+\t>repo/file-tracked &&\n+\t>repo/file-untracked &&\n+\t>repo/inside-tracked/file &&\n+\t>repo/inside-untracked/file &&\n+\t>repo-outside/file &&\n+\t>repo/repo-inside/file &&\n+\n+\tcat >expect-tracked-unsorted <<-EOF &&\n+\trepo/file-tracked\n+\trepo/inside-tracked/file\n+\tEOF\n+\n+\tcat >expect-untracked-unsorted <<-EOF &&\n+\trepo/file-untracked\n+\trepo/inside-untracked/file\n+\trepo/repo-inside/\n+\trepo-outside/\n+\tEOF\n+\n+\tcat >expect-all-dir-unsorted <<-EOF &&\n+\trepo/\n+\trepo-outside/\n+\tEOF\n+\n+\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n+\n+\tcat >.gitignore <<-EOF\n+\t.gitignore\n+\tactual-*\n+\texpect-*\n+\tEOF\n+\t)\n+'\n+\n+test_expect_success '2b: pre-add all' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git ls-files -o --exclude-standard >actual-all-unsorted &&\n+\tsort actual-all-unsorted >actual-all &&\n+\tsort expect-all-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '2c: pre-add dir all' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git ls-files -o --directory --exclude-standard >actual-all-dir-unsorted &&\n+\tsort actual-all-dir-unsorted >actual-all &&\n+\tsort expect-all-dir-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '2d: post-add tracked' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git add repo/file-tracked &&\n+\tgit --git-dir=repo/.git add repo/inside-tracked &&\n+\tgit --git-dir=repo/.git ls-files >actual-tracked-unsorted &&\n+\tsort actual-tracked-unsorted >actual-tracked &&\n+\tsort expect-tracked-unsorted >expect-tracked &&\n+\ttest_cmp expect-tracked actual-tracked\n+\t)\n+'\n+\n+test_expect_success '2e: post-add untracked' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git ls-files -o --exclude-standard >actual-untracked-unsorted &&\n+\tsort actual-untracked-unsorted >actual-untracked &&\n+\tsort expect-untracked-unsorted >expect-untracked &&\n+\ttest_cmp expect-untracked actual-untracked\n+\t)\n+'\n+\n+test_expect_success '3a: setup--add repo dir' '\n+\tmkdir test3 &&\n+\t(\n+\tcd test3 &&\n+\ttest_create_repo repo &&\n+\n+\tmkdir -p repo/inside-tracked repo/inside-ignored &&\n+\t>repo/file-tracked &&\n+\t>repo/file-ignored &&\n+\t>repo/inside-tracked/file &&\n+\t>repo/inside-ignored/file &&\n+\n+\tcat >.gitignore <<-EOF &&\n+\t.gitignore\n+\tactual-*\n+\texpect-*\n+\t*ignored\n+\tEOF\n+\n+\tcat >expect-tracked-unsorted <<-EOF &&\n+\trepo/file-tracked\n+\trepo/inside-tracked/file\n+\tEOF\n+\n+\tcat >expect-ignored-unsorted <<-EOF\n+\trepo/file-ignored\n+\trepo/inside-ignored/\n+\t.gitignore\n+\tactual-ignored-unsorted\n+\texpect-ignored-unsorted\n+\texpect-tracked-unsorted\n+\tEOF\n+\t)\n+'\n+\n+test_expect_success '3b: ignored' '\n+\t(\n+\tcd test3 &&\n+\tgit --git-dir=repo/.git ls-files -io --directory --exclude-standard >actual-ignored-unsorted &&\n+\tsort actual-ignored-unsorted >actual-ignored &&\n+\tsort expect-ignored-unsorted >expect-ignored &&\n+\ttest_cmp expect-ignored actual-ignored\n+\t)\n+'\n+\n+test_expect_success '3c: add repo' '\n+\t(\n+\tcd test3 &&\n+\tgit --git-dir=repo/.git add repo &&\n+\tgit --git-dir=repo/.git ls-files >actual-tracked-unsorted &&\n+\tsort actual-tracked-unsorted >actual-tracked &&\n+\tsort expect-tracked-unsorted >expect-tracked &&\n+\ttest_cmp expect-tracked actual-tracked\n+\t)\n+'\n+\n+test_done\n-- \n2.36.0\n\n"},{"id":"455622","messageId":"20220520192840.8942-4-ggossdev@gmail.com","threadId":"57851","inReplyTo":"20220520192840.8942-1-ggossdev@gmail.com","subject":"[PATCH v3 3/3] dir: minor refactoring / clean-up","fromName":"Goss Geppert","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-20T19:28:40Z","receivedAt":"2022-05-20T19:29:24Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"Narrow the scope of the `nested_repo` variable and conditional return\nstatement to the block where the variable is set.\n\nSigned-off-by: Goss Geppert \n---\n dir.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 45b89560fc..92bdd4daf8 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1874,7 +1874,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t */\n \tenum path_treatment state;\n \tint matches_how = 0;\n-\tint nested_repo = 0, check_only, stop_early;\n+\tint check_only, stop_early;\n \tint old_ignored_nr, old_untracked_nr;\n \t/* The \"len-1\" is to strip the final '/' */\n \tenum exist_status status = directory_exists_in_index(istate, dirname, len-1);\n@@ -1914,6 +1914,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\t * manually configured by the user; see t2205 testcases 1-3 for\n \t\t * examples where this matters\n \t\t */\n+\t\tint nested_repo;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, dirname);\n \t\tnested_repo = is_nonbare_repository_dir(&sb);\n@@ -1929,12 +1930,13 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\t\tfree(real_dirname);\n \t\t}\n \t\tstrbuf_release(&sb);\n-\t}\n-\tif (nested_repo) {\n-\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n-\t\t (matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n-\t\t\treturn path_none;\n-\t\treturn excluded ? path_excluded : path_untracked;\n+\n+\t\tif (nested_repo) {\n+\t\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n+\t\t\t\t(matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n+\t\t\t\treturn path_none;\n+\t\t\treturn excluded ? path_excluded : path_untracked;\n+\t\t}\n \t}\n \n \tif (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES)) {\n-- \n2.36.0\n\n"},{"id":"455623","messageId":"20220520192840.8942-3-ggossdev@gmail.com","threadId":"57851","inReplyTo":"20220520192840.8942-1-ggossdev@gmail.com","subject":"[PATCH v3 2/3] dir: cache git_dir's realpath","fromName":"Goss Geppert","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-20T19:28:39Z","receivedAt":"2022-05-20T19:29:27Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"When traversing the directory tree, the realpath of the `git_dir` is\nrequired each time a nested repository is encountered to determine how\nthe nested repository should be treated. To prevent excessive resource\nusage in pathological cases (e.g. many nested repositories, a long path,\nsymlinks, etc.), cache the realpath after the first usage until the\ntraversal is complete.\n---\n dir.c | 63 ++++++++++++++++++++++++++++++++++++++++-------------------\n 1 file changed, 43 insertions(+), 20 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex e45e117875..45b89560fc 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -47,10 +47,22 @@ struct cached_dir {\n \tstruct untracked_cache_dir *ucd;\n };\n \n+/*\n+ * Support data structure for the recursing into the directory tree.\n+ */\n+struct traversal_cache {\n+\t/*\n+\t * The realpath for the `git_dir` may be required in the recursion and is\n+\t * cached for repeated use.\n+\t */\n+\tchar *real_gitdir;\n+};\n+\n static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \tstruct index_state *istate, const char *path, int len,\n \tstruct untracked_cache_dir *untracked,\n-\tint check_only, int stop_at_first_file, const struct pathspec *pathspec);\n+\tint check_only, int stop_at_first_file, const struct pathspec *pathspec,\n+\tstruct traversal_cache *cache);\n static int resolve_dtype(int dtype, struct index_state *istate,\n \t\t\t const char *path, int len);\n struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp)\n@@ -1852,7 +1864,8 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \tstruct index_state *istate,\n \tstruct untracked_cache_dir *untracked,\n \tconst char *dirname, int len, int baselen, int excluded,\n-\tconst struct pathspec *pathspec)\n+\tconst struct pathspec *pathspec,\n+\tstruct traversal_cache *cache)\n {\n \t/*\n \t * WARNING: From this function, you can return path_recurse or you\n@@ -1906,13 +1919,13 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\tnested_repo = is_nonbare_repository_dir(&sb);\n \n \t\tif (nested_repo) {\n-\t\t\tchar *real_dirname, *real_gitdir;\n+\t\t\tchar *real_dirname;\n \t\t\tstrbuf_addstr(&sb, \".git\");\n \t\t\treal_dirname = real_pathdup(sb.buf, 1);\n-\t\t\treal_gitdir = real_pathdup(the_repository->gitdir, 1);\n+\t\t\tif (!cache->real_gitdir)\n+\t\t\t\tcache->real_gitdir = real_pathdup(the_repository->gitdir, 1);\n \n-\t\t\tnested_repo = !!strcmp(real_dirname, real_gitdir);\n-\t\t\tfree(real_gitdir);\n+\t\t\tnested_repo = !!strcmp(real_dirname, cache->real_gitdir);\n \t\t\tfree(real_dirname);\n \t\t}\n \t\tstrbuf_release(&sb);\n@@ -1944,7 +1957,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\t\t\treturn path_excluded;\n \n \t\t\tif (read_directory_recursive(dir, istate, dirname, len,\n-\t\t\t\t\t\t untracked, 1, 1, pathspec) == path_excluded)\n+\t\t\t\t\t\t untracked, 1, 1, pathspec, cache) == path_excluded)\n \t\t\t\treturn path_excluded;\n \n \t\t\treturn path_none;\n@@ -2045,7 +2058,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \tuntracked = lookup_untracked(dir->untracked, untracked,\n \t\t\t\t dirname + baselen, len - baselen);\n \tstate = read_directory_recursive(dir, istate, dirname, len, untracked,\n-\t\t\t\t\t check_only, stop_early, pathspec);\n+\t\t\t\t\t check_only, stop_early, pathspec, cache);\n \n \t/* There are a variety of reasons we may need to fixup the state... */\n \tif (state == path_excluded) {\n@@ -2242,7 +2255,8 @@ static enum path_treatment treat_path_fast(struct dir_struct *dir,\n \t\t\t\t\t struct index_state *istate,\n \t\t\t\t\t struct strbuf *path,\n \t\t\t\t\t int baselen,\n-\t\t\t\t\t const struct pathspec *pathspec)\n+\t\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t\t struct traversal_cache *cache)\n {\n \t/*\n \t * WARNING: From this function, you can return path_recurse or you\n@@ -2264,7 +2278,7 @@ static enum path_treatment treat_path_fast(struct dir_struct *dir,\n \t\t * with check_only set.\n \t\t */\n \t\treturn read_directory_recursive(dir, istate, path->buf, path->len,\n-\t\t\t\t\t\tcdir->ucd, 1, 0, pathspec);\n+\t\t\t\t\t\tcdir->ucd, 1, 0, pathspec, cache);\n \t/*\n \t * We get path_recurse in the first run when\n \t * directory_exists_in_index() returns index_nonexistent. We\n@@ -2280,13 +2294,14 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \t\t\t\t struct index_state *istate,\n \t\t\t\t struct strbuf *path,\n \t\t\t\t int baselen,\n-\t\t\t\t const struct pathspec *pathspec)\n+\t\t\t\t const struct pathspec *pathspec,\n+\t\t\t\t struct traversal_cache *cache)\n {\n \tint has_path_in_index, dtype, excluded;\n \n \tif (!cdir->d_name)\n \t\treturn treat_path_fast(dir, cdir, istate, path,\n-\t\t\t\t baselen, pathspec);\n+\t\t\t\t baselen, pathspec, cache);\n \tif (is_dot_or_dotdot(cdir->d_name) || !fspathcmp(cdir->d_name, \".git\"))\n \t\treturn path_none;\n \tstrbuf_setlen(path, baselen);\n@@ -2348,7 +2363,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n \t\tstrbuf_addch(path, '/');\n \t\treturn treat_directory(dir, istate, untracked,\n \t\t\t\t path->buf, path->len,\n-\t\t\t\t baselen, excluded, pathspec);\n+\t\t\t\t baselen, excluded, pathspec, cache);\n \tcase DT_REG:\n \tcase DT_LNK:\n \t\tif (pathspec &&\n@@ -2549,7 +2564,8 @@ static void add_path_to_appropriate_result_list(struct dir_struct *dir,\n static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \tstruct index_state *istate, const char *base, int baselen,\n \tstruct untracked_cache_dir *untracked, int check_only,\n-\tint stop_at_first_file, const struct pathspec *pathspec)\n+\tint stop_at_first_file, const struct pathspec *pathspec,\n+\tstruct traversal_cache *cache)\n {\n \t/*\n \t * WARNING: Do NOT recurse unless path_recurse is returned from\n@@ -2572,7 +2588,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \twhile (!read_cached_dir(&cdir)) {\n \t\t/* check how the file or directory should be treated */\n \t\tstate = treat_path(dir, untracked, &cdir, istate, &path,\n-\t\t\t\t baselen, pathspec);\n+\t\t\t\t baselen, pathspec, cache);\n \t\tdir->visited_paths++;\n \n \t\tif (state > dir_state)\n@@ -2587,7 +2603,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n \t\t\tsubdir_state =\n \t\t\t\tread_directory_recursive(dir, istate, path.buf,\n \t\t\t\t\t\t\t path.len, ud,\n-\t\t\t\t\t\t\t check_only, stop_at_first_file, pathspec);\n+\t\t\t\t\t\t\t check_only, stop_at_first_file, pathspec, cache);\n \t\t\tif (subdir_state > dir_state)\n \t\t\t\tdir_state = subdir_state;\n \n@@ -2661,7 +2677,8 @@ int check_dir_entry_contains(const struct dir_entry *out, const struct dir_entry\n static int treat_leading_path(struct dir_struct *dir,\n \t\t\t struct index_state *istate,\n \t\t\t const char *path, int len,\n-\t\t\t const struct pathspec *pathspec)\n+\t\t\t const struct pathspec *pathspec,\n+\t\t\t struct traversal_cache *cache)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct strbuf subdir = STRBUF_INIT;\n@@ -2713,7 +2730,8 @@ static int treat_leading_path(struct dir_struct *dir,\n \t\tstrbuf_reset(&subdir);\n \t\tstrbuf_add(&subdir, path+prevlen, baselen-prevlen);\n \t\tcdir.d_name = subdir.buf;\n-\t\tstate = treat_path(dir, NULL, &cdir, istate, &sb, prevlen, pathspec);\n+\n+\t\tstate = treat_path(dir, NULL, &cdir, istate, &sb, prevlen, pathspec, cache);\n \n \t\tif (state != path_recurse)\n \t\t\tbreak; /* do not recurse into it */\n@@ -2986,6 +3004,9 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t\t const char *path, int len, const struct pathspec *pathspec)\n {\n \tstruct untracked_cache_dir *untracked;\n+\tstruct traversal_cache cache = {\n+\t\t.real_gitdir = NULL,\n+\t};\n \n \ttrace2_region_enter(\"dir\", \"read_directory\", istate->repo);\n \tdir->visited_paths = 0;\n@@ -3003,8 +3024,10 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n \t\t * e.g. prep_exclude()\n \t\t */\n \t\tdir->untracked = NULL;\n-\tif (!len || treat_leading_path(dir, istate, path, len, pathspec))\n-\t\tread_directory_recursive(dir, istate, path, len, untracked, 0, 0, pathspec);\n+\tif (!len || treat_leading_path(dir, istate, path, len, pathspec, &cache)) {\n+\t\tread_directory_recursive(dir, istate, path, len, untracked, 0, 0, pathspec, &cache);\n+\t}\n+\tFREE_AND_NULL(cache.real_gitdir);\n \tQSORT(dir->entries, dir->nr, cmp_dir_entry);\n \tQSORT(dir->ignored, dir->ignored_nr, cmp_dir_entry);\n \n-- \n2.36.0\n\n"},{"id":"455625","messageId":"CAAA5oLnJRTWzQ7AtX-7iDvOHFbGfZBEw+KiNoU0Y08rx16oSsQ@mail.gmail.com","threadId":"57851","inReplyTo":"xmqq7d6sm3e0.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] dir: consider worktree config in path recursion","fromName":"oss dev","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-20T19:45:39Z","receivedAt":"2022-05-20T19:45:58Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"> Is this _complete() necessary?\n\nIt is not. Though _complete won't actually change the buffer if the\ntrailing character matches, I've removed it. The '/' is added prior\nto each call here [1].\n\n[1]: https://github.com/git/git/blob/277cf0bc36094f6dc4297d8c9cef79df045b735d/dir.c#L2329\n\n> This function is repeatedly called during the traversal.\n>\n> How expensive is it to keep calling real_pathdup() on the constant\n> the_repository->gitdir just in case it might be the same as our true\n> GIT_DIR?\n\nUsing trace2, `real_pathdup` takes around 40 usecs on my machine\ncompared to 10 usecs for an empty region.\n\nPlease see the cover letter for v3 of the patch series for additional\ndetails on both of these items.\n"},{"id":"455627","messageId":"CAAA5oLkpATLtxgLDg3tZzSZLd_cUGkOeDrMN-5QgTqPOhRRAOg@mail.gmail.com","threadId":"57851","inReplyTo":"xmqqsfpfhdnh.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] dir: consider worktree config in path recursion","fromName":"oss dev","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-20T20:01:06Z","receivedAt":"2022-05-20T20:01:23Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"> I think I've mentioned this ealier, but the above only describes the\n> scenario and does not say what behaviour is expected and what behaviour\n> is observed. \"no longer works\" is OK, but not sufficient.\n\nI've updated the commit message for the fact that using option\n`--git-dir` can also cause the traversal to skip over the repository.\nI've therefore had to be a bit more generic in the description, so let\nme know if it's not specific enough.\n\n> > + git --git-dir=\"test1/.git\" config core.worktree \"$(pwd)\" &&\n>\n> I wonder if this lets funny paths to be added to the index, e.g.\n> would \"git add test1\" recursively add everything in that directory?\n\nYes, it would recursively add files inside test1. I've added testcase\n3c which I believe does what we want but let me know if that doesn't\naddress your question.\n"},{"id":"455628","messageId":"CAAA5oLndkcrRSXSmMbGk9JTQEE58e_Q=WYr3vaFx1C8i7ZSgZw@mail.gmail.com","threadId":"57851","inReplyTo":"xmqqv8ucko6v.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] dir: minor refactoring / clean-up","fromName":"oss dev","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-20T20:03:14Z","receivedAt":"2022-05-20T20:03:31Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"> > Improve readability.\n>\n> It reads somewhat a subjective opinion, without explaining how/why\n> the change makes it more readable. Perhaps\n>\n> Narrow the scope of the nested_repo variable to the block\n> that uses it.\n>\n> or something?\n\nI've updated this message. Thanks!\n"},{"id":"455810","messageId":"900d91cb-1982-b892-17e3-ad678e711dc9@github.com","threadId":"57851","inReplyTo":"20220510171527.25778-2-ggossdev@gmail.com","subject":"Re: [PATCH v2 1/2] dir: consider worktree config in path recursion","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-05-23T19:23:29Z","receivedAt":"2022-05-23T19:36:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/10/22 1:15 PM, Goss Geppert wrote:\n> Since 8d92fb2927 (dir: replace exponential algorithm with a linear one,\n> 2020-04-01) the following no longer works:\n> \n> 1) Initialize a repository.\n> 2) Set the `core.worktree` location to a parent directory of the\n> default worktree.\n> 3) Add a file located in the default worktree location to the index\n> (i.e. anywhere in the immediate parent directory of the gitdir).\n> \n> This commit adds a check to determine whether a nested repository that\n> is encountered in recursing a path is actually `the_repository`. If so,\n> simply treat the directory as if it doesn't contain a nested repository.\n> \n> Prior to this commit, the `add` operation was silently ignored.\n> \n> Signed-off-by: Goss Geppert \n> ---\n> dir.c | 22 +++++++++\n> t/t2205-add-worktree-config.sh | 89 ++++++++++++++++++++++++++++++++++\n> 2 files changed, 111 insertions(+)\n> create mode 100755 t/t2205-add-worktree-config.sh\n> \n> diff --git a/dir.c b/dir.c\n> index f2b0f24210..a1886e61a3 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -1893,9 +1893,31 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n> \n> \tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n> \t\t!(dir->flags & DIR_NO_GITLINKS)) {\n> +\t\t/*\n> +\t\t * Determine if `dirname` is a nested repo by confirming that:\n> +\t\t * 1) we are in a nonbare repository, and\n> +\t\t * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,\n> +\t\t * which could occur if the `worktree` location was manually\n> +\t\t * configured by the user; see t2205 testcases 1a-1d for examples\n> +\t\t * where this matters\n> +\t\t */\n> \t\tstruct strbuf sb = STRBUF_INIT;\n> \t\tstrbuf_addstr(&sb, dirname);\n> \t\tnested_repo = is_nonbare_repository_dir(&sb);\n> +\n> +\t\tif (nested_repo) {\n> +\t\t\tchar *real_dirname, *real_gitdir;\n> +\t\t\tstrbuf_reset(&sb);\n> +\t\t\tstrbuf_addstr(&sb, dirname);\n> +\t\t\tstrbuf_complete(&sb, '/');\n> +\t\t\tstrbuf_addstr(&sb, \".git\");\n\nThis could be strbuf_add(&sb, \".git\", 4); to avoid a (minor)\nstrlen() computation.\n\n> +\t\t\treal_dirname = real_pathdup(sb.buf, 0);\n> +\t\t\treal_gitdir = real_pathdup(the_repository->gitdir, 0);\n\nWith regards to Junio's comment about repeating real_pathdup()\non the_repository->gitdir, we might be able to short-circuit\nthis by making real_gitdir static and only calling\nreal_pathdup() if real_gitdir is NULL. It would cut the cost\nof these checks by half for big traversals.\n\n> +\n> +\t\t\tnested_repo = !!strcmp(real_dirname, real_gitdir);\n> +\t\t\tfree(real_gitdir);\n> +\t\t\tfree(real_dirname);\n> +\t\t}\n...\n> +\n> +test_description='directory traversal respects worktree config\n> +\n> +This test configures the repository`s worktree to be two levels above the\n> +`.git` directory and checks whether we are able to add to the index those files\n> +that are in either (1) the manually configured worktree directory or (2) the\n> +standard worktree location with respect to the `.git` directory (i.e. ensuring\n> +that the encountered `.git` directory is not treated as belonging to a foreign\n> +nested repository)'\n\nThe test description should be a single line. The longer paragraph\ncould be a comment before your \"setup\" test case.\n\n> +. ./test-lib.sh\n> +\n> +test_expect_success '1a: setup' '\n\nAlso, there is no need to number your tests in their names, as the\ntesting infrastructure will apply numbers based on their order in\nthe test file. These labels will grow stale if tests are removed\nor inserted into the order.\n\n> +\ttest_create_repo test1 &&\n> +\tgit --git-dir=\"test1/.git\" config core.worktree \"$(pwd)\" &&\n> +\n> +\tmkdir -p outside-tracked outside-untracked &&\n> +\tmkdir -p test1/inside-tracked test1/inside-untracked &&\n> +\t>file-tracked &&\n> +\t>file-untracked &&\n> +\t>outside-tracked/file &&\n> +\t>outside-untracked/file &&\n> +\t>test1/file-tracked &&\n> +\t>test1/file-untracked &&\n> +\t>test1/inside-tracked/file &&\n> +\t>test1/inside-untracked/file &&\n> +\n> +\tcat >expect-tracked-unsorted <<-EOF &&\n> +\t../file-tracked\n> +\t../outside-tracked/file\n> +\tfile-tracked\n> +\tinside-tracked/file\n> +\tEOF\n> +\n> +\tcat >expect-untracked-unsorted <<-EOF &&\n> +\t../file-untracked\n> +\t../outside-untracked/file\n> +\tfile-untracked\n> +\tinside-untracked/file\n> +\tEOF\n> +\n> +\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n> +\n> +\tcat >.gitignore <<-EOF\n> +\t.gitignore\n> +\tactual-*\n> +\texpect-*\n> +\tEOF\n\nThis use of .gitignore to ignore the helper files being used\nduring the test is interesting. I was thinking it would be better\nto create isolated directories where the only activity was that\nwhich you were testing:\n\n\tmkdir -p worktree &&\n\ttest_create_repo worktree/contains-git &&\n\n...or something like that. The names are more descriptive, and\nwe don't need the .gitignore trick.\n\n> +'\n> +\n> +test_expect_success '1b: pre-add all' '\n> +\tlocal parent_dir=\"$(pwd)\" &&\n\nI was surprised by this \"local\". It isn't needed at this\npoint in the test script. We use it for helper methods to\nbe sure that we aren't stomping variables from test scripts,\nbut since the test is being executed inside an eval(), this\nlocal isn't important.\n\n> +\t(\n> +\t\tcd test1 &&\n> +\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-all-unsorted\n\nYou can avoid the sub-shell using \"git -C test1 ls-files ...\"\nright?\n\n> +\t) &&\n> +\tsort actual-all-unsorted >actual-all &&\n> +\tsort expect-all-unsorted >expect-all &&\n> +\ttest_cmp expect-all actual-all\n> +'\n> +\n> +test_expect_success '1c: post-add tracked' '\n> +\tlocal parent_dir=\"$(pwd)\" &&\n> +\t(\n> +\t\tcd test1 &&\n> +\t\tgit add file-tracked &&\n> +\t\tgit add inside-tracked &&\n> +\t\tgit add ../outside-tracked &&\n> +\t\tgit add \"$parent_dir/file-tracked\" &&\n> +\t\tgit ls-files \"$parent_dir\" >../actual-tracked-unsorted\n> +\t) &&\n\nThis sub-shell is important because of the relative paths\ninvolved. OK.\n\nThis also checks to see if _any_ of these \"git add\"\ncommands fail, as opposed to failing immediately after\nthe first one fails. I think your approach is simpler and\nshould be relatively simple to identify which command did\nthe wrong thing by looking at the test_cmp output.\n\n> +\tsort actual-tracked-unsorted >actual-tracked &&\n> +\tsort expect-tracked-unsorted >expect-tracked &&\n> +\ttest_cmp expect-tracked actual-tracked\n> +'\n> +\n> +test_expect_success '1d: post-add untracked' '\n> +\tlocal parent_dir=\"$(pwd)\" &&\n> +\t(\n> +\t\tcd test1 &&\n> +\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-untracked-unsorted\n> +\t) &&\n\nAgain, this one is not needed.\n\n> +\tsort actual-untracked-unsorted >actual-untracked &&\n> +\tsort expect-untracked-unsorted >expect-untracked &&\n> +\ttest_cmp expect-untracked actual-untracked\n> +'\n> +\n\nThanks,\n-Stolee\n"},{"id":"455904","messageId":"CABPp-BGXRzYCvyM38dEUvQ125+VtRu++7L9UiRz98u+1=Lov7A@mail.gmail.com","threadId":"57851","inReplyTo":"xmqq7d6sm3e0.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] dir: consider worktree config in path recursion","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-24T14:29:09Z","receivedAt":"2022-05-24T14:29:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, May 11, 2022 at 9:37 AM Junio C Hamano wrote:\n>\n> Goss Geppert writes:\n>\n> > diff --git a/dir.c b/dir.c\n> > index f2b0f24210..a1886e61a3 100644\n> > --- a/dir.c\n> > +++ b/dir.c\n> > @@ -1893,9 +1893,31 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n>\n[...]\n>\n> > + real_gitdir = real_pathdup(the_repository->gitdir, 0);\n>\n> This function is repeatedly called during the traversal.\n>\n> How expensive is it to keep calling real_pathdup() on the constant\n> the_repository->gitdir just in case it might be the same as our true\n> GIT_DIR?\n\nI agree that treat_directory is called many times, but this\nreal_pathdup() call is inside the \"if (nested_repo)\" block, so this\nnew real_pathdup() invocation should occur very seldom. Or are you\nworried about cases where users have *very* large numbers of bare\nrepositories nested under the working directory? Even in that case,\nwhich seems pathological to me, I'd suspect the\nis_nonbare_repository_dir() ->\nread_gitfile_gently()/is_git_directory() codepath (used to determine\nthe value of nested_repo) would be much more expensive than this call\nto real_pathdup(), so would it be worth trying to optimize this\nreal_pathdup() call away even in that rare case?\n"},{"id":"455905","messageId":"CABPp-BGkGu8NZ6wOiMFEVQ+s=28CUMiD-+upWweAtZBjg9ciAA@mail.gmail.com","threadId":"57851","inReplyTo":"20220520192840.8942-3-ggossdev@gmail.com","subject":"Re: [PATCH v3 2/3] dir: cache git_dir's realpath","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-24T14:32:39Z","receivedAt":"2022-05-24T14:32:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, May 20, 2022 at 12:29 PM Goss Geppert wrote:\n>\n> When traversing the directory tree, the realpath of the `git_dir` is\n> required each time a nested repository is encountered to determine how\n> the nested repository should be treated. To prevent excessive resource\n> usage in pathological cases (e.g. many nested repositories, a long path,\n> symlinks, etc.), cache the realpath after the first usage until the\n> traversal is complete.\n> ---\n> dir.c | 63 ++++++++++++++++++++++++++++++++++++++++-------------------\n> 1 file changed, 43 insertions(+), 20 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index e45e117875..45b89560fc 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -47,10 +47,22 @@ struct cached_dir {\n> struct untracked_cache_dir *ucd;\n> };\n>\n> +/*\n> + * Support data structure for the recursing into the directory tree.\n> + */\n> +struct traversal_cache {\n> + /*\n> + * The realpath for the `git_dir` may be required in the recursion and is\n> + * cached for repeated use.\n> + */\n> + char *real_gitdir;\n> +};\n> +\n> static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> struct index_state *istate, const char *path, int len,\n> struct untracked_cache_dir *untracked,\n> - int check_only, int stop_at_first_file, const struct pathspec *pathspec);\n> + int check_only, int stop_at_first_file, const struct pathspec *pathspec,\n> + struct traversal_cache *cache);\n> static int resolve_dtype(int dtype, struct index_state *istate,\n> const char *path, int len);\n> struct dirent *readdir_skip_dot_and_dotdot(DIR *dirp)\n> @@ -1852,7 +1864,8 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n> struct index_state *istate,\n> struct untracked_cache_dir *untracked,\n> const char *dirname, int len, int baselen, int excluded,\n> - const struct pathspec *pathspec)\n> + const struct pathspec *pathspec,\n> + struct traversal_cache *cache)\n> {\n> /*\n> * WARNING: From this function, you can return path_recurse or you\n> @@ -1906,13 +1919,13 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n> nested_repo = is_nonbare_repository_dir(&sb);\n>\n> if (nested_repo) {\n> - char *real_dirname, *real_gitdir;\n> + char *real_dirname;\n> strbuf_addstr(&sb, \".git\");\n> real_dirname = real_pathdup(sb.buf, 1);\n> - real_gitdir = real_pathdup(the_repository->gitdir, 1);\n> + if (!cache->real_gitdir)\n> + cache->real_gitdir = real_pathdup(the_repository->gitdir, 1);\n>\n> - nested_repo = !!strcmp(real_dirname, real_gitdir);\n> - free(real_gitdir);\n> + nested_repo = !!strcmp(real_dirname, cache->real_gitdir);\n> free(real_dirname);\n> }\n> strbuf_release(&sb);\n> @@ -1944,7 +1957,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n> return path_excluded;\n>\n> if (read_directory_recursive(dir, istate, dirname, len,\n> - untracked, 1, 1, pathspec) == path_excluded)\n> + untracked, 1, 1, pathspec, cache) == path_excluded)\n> return path_excluded;\n>\n> return path_none;\n> @@ -2045,7 +2058,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n> untracked = lookup_untracked(dir->untracked, untracked,\n> dirname + baselen, len - baselen);\n> state = read_directory_recursive(dir, istate, dirname, len, untracked,\n> - check_only, stop_early, pathspec);\n> + check_only, stop_early, pathspec, cache);\n>\n> /* There are a variety of reasons we may need to fixup the state... */\n> if (state == path_excluded) {\n> @@ -2242,7 +2255,8 @@ static enum path_treatment treat_path_fast(struct dir_struct *dir,\n> struct index_state *istate,\n> struct strbuf *path,\n> int baselen,\n> - const struct pathspec *pathspec)\n> + const struct pathspec *pathspec,\n> + struct traversal_cache *cache)\n> {\n> /*\n> * WARNING: From this function, you can return path_recurse or you\n> @@ -2264,7 +2278,7 @@ static enum path_treatment treat_path_fast(struct dir_struct *dir,\n> * with check_only set.\n> */\n> return read_directory_recursive(dir, istate, path->buf, path->len,\n> - cdir->ucd, 1, 0, pathspec);\n> + cdir->ucd, 1, 0, pathspec, cache);\n> /*\n> * We get path_recurse in the first run when\n> * directory_exists_in_index() returns index_nonexistent. We\n> @@ -2280,13 +2294,14 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n> struct index_state *istate,\n> struct strbuf *path,\n> int baselen,\n> - const struct pathspec *pathspec)\n> + const struct pathspec *pathspec,\n> + struct traversal_cache *cache)\n> {\n> int has_path_in_index, dtype, excluded;\n>\n> if (!cdir->d_name)\n> return treat_path_fast(dir, cdir, istate, path,\n> - baselen, pathspec);\n> + baselen, pathspec, cache);\n> if (is_dot_or_dotdot(cdir->d_name) || !fspathcmp(cdir->d_name, \".git\"))\n> return path_none;\n> strbuf_setlen(path, baselen);\n> @@ -2348,7 +2363,7 @@ static enum path_treatment treat_path(struct dir_struct *dir,\n> strbuf_addch(path, '/');\n> return treat_directory(dir, istate, untracked,\n> path->buf, path->len,\n> - baselen, excluded, pathspec);\n> + baselen, excluded, pathspec, cache);\n> case DT_REG:\n> case DT_LNK:\n> if (pathspec &&\n> @@ -2549,7 +2564,8 @@ static void add_path_to_appropriate_result_list(struct dir_struct *dir,\n> static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> struct index_state *istate, const char *base, int baselen,\n> struct untracked_cache_dir *untracked, int check_only,\n> - int stop_at_first_file, const struct pathspec *pathspec)\n> + int stop_at_first_file, const struct pathspec *pathspec,\n> + struct traversal_cache *cache)\n> {\n> /*\n> * WARNING: Do NOT recurse unless path_recurse is returned from\n> @@ -2572,7 +2588,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> while (!read_cached_dir(&cdir)) {\n> /* check how the file or directory should be treated */\n> state = treat_path(dir, untracked, &cdir, istate, &path,\n> - baselen, pathspec);\n> + baselen, pathspec, cache);\n> dir->visited_paths++;\n>\n> if (state > dir_state)\n> @@ -2587,7 +2603,7 @@ static enum path_treatment read_directory_recursive(struct dir_struct *dir,\n> subdir_state =\n> read_directory_recursive(dir, istate, path.buf,\n> path.len, ud,\n> - check_only, stop_at_first_file, pathspec);\n> + check_only, stop_at_first_file, pathspec, cache);\n> if (subdir_state > dir_state)\n> dir_state = subdir_state;\n>\n> @@ -2661,7 +2677,8 @@ int check_dir_entry_contains(const struct dir_entry *out, const struct dir_entry\n> static int treat_leading_path(struct dir_struct *dir,\n> struct index_state *istate,\n> const char *path, int len,\n> - const struct pathspec *pathspec)\n> + const struct pathspec *pathspec,\n> + struct traversal_cache *cache)\n> {\n> struct strbuf sb = STRBUF_INIT;\n> struct strbuf subdir = STRBUF_INIT;\n> @@ -2713,7 +2730,8 @@ static int treat_leading_path(struct dir_struct *dir,\n> strbuf_reset(&subdir);\n> strbuf_add(&subdir, path+prevlen, baselen-prevlen);\n> cdir.d_name = subdir.buf;\n> - state = treat_path(dir, NULL, &cdir, istate, &sb, prevlen, pathspec);\n> +\n> + state = treat_path(dir, NULL, &cdir, istate, &sb, prevlen, pathspec, cache);\n>\n> if (state != path_recurse)\n> break; /* do not recurse into it */\n> @@ -2986,6 +3004,9 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n> const char *path, int len, const struct pathspec *pathspec)\n> {\n> struct untracked_cache_dir *untracked;\n> + struct traversal_cache cache = {\n> + .real_gitdir = NULL,\n> + };\n>\n> trace2_region_enter(\"dir\", \"read_directory\", istate->repo);\n> dir->visited_paths = 0;\n> @@ -3003,8 +3024,10 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,\n> * e.g. prep_exclude()\n> */\n> dir->untracked = NULL;\n> - if (!len || treat_leading_path(dir, istate, path, len, pathspec))\n> - read_directory_recursive(dir, istate, path, len, untracked, 0, 0, pathspec);\n> + if (!len || treat_leading_path(dir, istate, path, len, pathspec, &cache)) {\n> + read_directory_recursive(dir, istate, path, len, untracked, 0, 0, pathspec, &cache);\n> + }\n> + FREE_AND_NULL(cache.real_gitdir);\n> QSORT(dir->entries, dir->nr, cmp_dir_entry);\n> QSORT(dir->ignored, dir->ignored_nr, cmp_dir_entry);\n>\n> --\n> 2.36.0\n\nThis makes sense as a way to avoid the call to real_pathdup() if\nthat's what we want, but I feel it's a micro-optimization that isn't\nworth this code churn. I'd rather drop this patch from the series and\njust use the real_pathdup() you had before (see\nhttps://lore.kernel.org/git/CABPp-BGXRzYCvyM38dEUvQ125+VtRu++7L9UiRz98u+1=Lov7A@mail.gmail.com/).\nLet's see what Junio says, but I'm hoping he agrees this patch isn't\nneeded.\n"},{"id":"455942","messageId":"xmqqilpuhfz2.fsf@gitster.g","threadId":"57851","inReplyTo":"CABPp-BGXRzYCvyM38dEUvQ125+VtRu++7L9UiRz98u+1=Lov7A@mail.gmail.com","subject":"Re: [PATCH v2 1/2] dir: consider worktree config in path recursion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-24T19:45:37Z","receivedAt":"2022-05-24T19:45:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Wed, May 11, 2022 at 9:37 AM Junio C Hamano wrote:\n>>\n>> Goss Geppert writes:\n>>\n>> > diff --git a/dir.c b/dir.c\n>> > index f2b0f24210..a1886e61a3 100644\n>> > --- a/dir.c\n>> > +++ b/dir.c\n>> > @@ -1893,9 +1893,31 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n>>\n> [...]\n>>\n>> > + real_gitdir = real_pathdup(the_repository->gitdir, 0);\n>>\n>> This function is repeatedly called during the traversal.\n>>\n>> How expensive is it to keep calling real_pathdup() on the constant\n>> the_repository->gitdir just in case it might be the same as our true\n>> GIT_DIR?\n>\n> I agree that treat_directory is called many times, but this\n> real_pathdup() call is inside the \"if (nested_repo)\" block, so this\n> new real_pathdup() invocation should occur very seldom. Or are you\n> worried about cases where users have *very* large numbers of bare\n> repositories nested under the working directory?\n\nNo. I wasn't worried about anything in particular. I just wanted\nto get the feel of how deep a thought the patch was backed by by\nspot checking what was and what was not taken into account when\ndesigning the change.\n\nI do not care too much when there are very large numbers of things\nthat cause this codepath to be exercised. Strange situations can be\nleft for later optimization only when they turn up in the real world\nand prove to be a problem.\n\nBy the way, where is a bare repository involved? did you mean\nnon-bare aka worktree-full repository?\n"},{"id":"455994","messageId":"CABPp-BHHyouFfJ4OC=xShxFASTdBL=7qxnvwS7pDQ+xO6iTW2w@mail.gmail.com","threadId":"57851","inReplyTo":"xmqqilpuhfz2.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] dir: consider worktree config in path recursion","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-05-25T03:46:59Z","receivedAt":"2022-05-25T03:47:16Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, May 24, 2022 at 12:45 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > On Wed, May 11, 2022 at 9:37 AM Junio C Hamano wrote:\n> >>\n> >> Goss Geppert writes:\n> >>\n> >> > diff --git a/dir.c b/dir.c\n> >> > index f2b0f24210..a1886e61a3 100644\n> >> > --- a/dir.c\n> >> > +++ b/dir.c\n> >> > @@ -1893,9 +1893,31 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n> >>\n> > [...]\n> >>\n> >> > + real_gitdir = real_pathdup(the_repository->gitdir, 0);\n> >>\n> >> This function is repeatedly called during the traversal.\n> >>\n> >> How expensive is it to keep calling real_pathdup() on the constant\n> >> the_repository->gitdir just in case it might be the same as our true\n> >> GIT_DIR?\n> >\n> > I agree that treat_directory is called many times, but this\n> > real_pathdup() call is inside the \"if (nested_repo)\" block, so this\n> > new real_pathdup() invocation should occur very seldom. Or are you\n> > worried about cases where users have *very* large numbers of bare\n> > repositories nested under the working directory?\n>\n> No. I wasn't worried about anything in particular. I just wanted\n> to get the feel of how deep a thought the patch was backed by by\n> spot checking what was and what was not taken into account when\n> designing the change.\n>\n> I do not care too much when there are very large numbers of things\n> that cause this codepath to be exercised. Strange situations can be\n> left for later optimization only when they turn up in the real world\n> and prove to be a problem.\n\nAh, gotcha.\n\n> By the way, where is a bare repository involved? did you mean\n> non-bare aka worktree-full repository?\n\nYes, sorry, I meant non-bare repository.\n"},{"id":"456377","messageId":"CAAA5oLnAwcH+Kr9UT1+V-_bjNNJS+8=U1+uMZUETSmP+nfM7kA@mail.gmail.com","threadId":"57851","inReplyTo":"900d91cb-1982-b892-17e3-ad678e711dc9@github.com","subject":"Re: [PATCH v2 1/2] dir: consider worktree config in path recursion","fromName":"oss dev","fromEmail":"gg.oss.dev@gmail.com","sentAt":"2022-05-30T18:48:12Z","receivedAt":"2022-05-30T18:48:37Z","isPatch":true,"sender":{"key":"gg.oss.dev@gmail.com","avatar":null},"body":"> This could be strbuf_add(&sb, \".git\", 4); to avoid a (minor)\n> strlen() computation.\n\nI checked gcc's disassembly--the strbuf_addstr is inlined and the\nstrlen is evaluated at compile time. I believe gcc has evaluated\nstrlen at compile time for string literals for quite some time. Also\nseems common in the rest of the code base to use strbuf_addstr with\nstring literals.\n\n\n> With regards to Junio's comment about repeating real_pathdup()\n> on the_repository->gitdir, we might be able to short-circuit\n> this by making real_gitdir static and only calling\n> real_pathdup() if real_gitdir is NULL. It would cut the cost\n> of these checks by half for big traversals.\n\nYeah, I couldn't come up with a super clean way of caching\nreal_pathdup (assuming we don't want to leak the memory), but based on\nsubsequent comments it sounds like it was more a kicking-the-tires\nquestion than real concern.\n\n\n> The test description should be a single line. The longer paragraph\n> could be a comment before your \"setup\" test case.\n\nI'm not sure what the current best practice is, but I expected best\npractice to be reflected here [1] and many other tests use multi-line\ndescriptions (see e.g. t0000 [2] or t1512 [3] for a nice ascii graphic\nright in the description).\n\n[1]: https://git-scm.com/docs/MyFirstContribution#write-new-test\n[2]: https://github.com/git/git/blob/8ddf593a250e07d388059f7e3f471078e1d2ed5c/t/t0000-basic.sh\n[3]: https://github.com/git/git/blob/8ddf593a250e07d388059f7e3f471078e1d2ed5c/t/t1512-rev-parse-disambiguation.sh\n\n\n> Also, there is no need to number your tests in their names, as the\n> testing infrastructure will apply numbers based on their order in\n> the test file. These labels will grow stale if tests are removed\n> or inserted into the order.\n\nIn v1 of the patch, I was asked to include a comment in the code that\nrefers to a test for which the changes that are being made in this\npatch are meaningful. Grepping for other instances where this was\ndone, similar comments generally refer to numbered testcases (see e.g.\n[1]). I figured if I don't number it then someone might suggest that\nsimilar comments generally refer to numbered test cases. Though I'll\nconcede that referring to any specific tests will likely eventually\nresult in stale references. For example, t6043 referenced by [1]\n(currently in master) appears to have been moved elsewhere.\nPersonally, I'm indifferent.\n\n[1]: https://github.com/git/git/blob/8ddf593a250e07d388059f7e3f471078e1d2ed5c/merge-recursive.c#L1620\n\n\n> This use of .gitignore to ignore the helper files being used\n> during the test is interesting. I was thinking it would be better\n> to create isolated directories where the only activity was that\n> which you were testing:\n>\n> mkdir -p worktree &&\n> test_create_repo worktree/contains-git &&\n>\n> ...or something like that. The names are more descriptive, and\n> we don't need the .gitignore trick.\n\nWhile trying to figure out what section this test should go into, I\nlooked through several tests and was likely inspired by e.g. [1] or\n[2]. I also think there could be a benefit to ensuring that .gitignore\nworks correctly with the new traversal behavior in the nested repo,\ni.e. the .gitignore becomes part of the test. In principle, I don't\nmind testing .gitignore more explicitly, but I also think the tests\nare quite lengthy already for a small patch and doing more with less\nseems attractive.\n\n[1]: https://github.com/git/git/blob/8ddf593a250e07d388059f7e3f471078e1d2ed5c/t/t1501-work-tree.sh#L198-L202\n[2]: https://github.com/git/git/blob/8ddf593a250e07d388059f7e3f471078e1d2ed5c/t/t2202-add-addremove.sh#L13-L17\n\n\n> I was surprised by this \"local\". It isn't needed at this\n> point in the test script. We use it for helper methods to\n> be sure that we aren't stomping variables from test scripts,\n> but since the test is being executed inside an eval(), this\n> local isn't important.\n\nI tend to default to `local` or `declare` to limit unintended side\neffects after refactoring, etc., but I don't mind removing it if it's\nnot desired. I'm ok either way, pls confirm.\n\n> You can avoid the sub-shell using \"git -C test1 ls-files ...\"\n> right?\n\nOk, I can update this and similar instances.\n\n\n> This also checks to see if _any_ of these \"git add\"\n> commands fail, as opposed to failing immediately after\n> the first one fails. I think your approach is simpler and\n> should be relatively simple to identify which command did\n> the wrong thing by looking at the test_cmp output.\n\nAgreed, I'm combining several `git add`s into one test here -- I\ndidn't want to get too long-winded since it seems like a minor patch.\n\nThanks for the comments; v3 of the patch had already been sent prior\nto these, so none is incorporated there. I'll include confirmed\nchanges in the next version.\n"},{"id":"457432","messageId":"DM6PR13MB29876A53962557131120EF619DAC9@DM6PR13MB2987.namprd13.prod.outlook.com","threadId":"57851","inReplyTo":"20220505203234.21586-1-ggossdev@gmail.com","subject":"[PATCH v4 0/2] dir: traverse into repository","fromName":"Goss Geppert","fromEmail":"gg.oss@outlook.com","sentAt":"2022-06-16T23:19:54Z","receivedAt":"2022-06-16T23:20:47Z","isPatch":true,"sender":{"key":"gg.oss@outlook.com","avatar":null},"body":"From: Goss Geppert \n\nThis latest version of the patch series contains relatively minor\nmodifications relative to the previous version. Let me know if there\nis anything else I need to do.\n\nChanges compared to v3:\n* remove the commit that caches the gitdir's realpath as this was deemed\n a premature optimization given the amount of code churn\n* use git option `-C` in some of the testcases to avoid creating a\n subshell\n\nGoss Geppert (2):\n dir: traverse into repository\n dir: minor refactoring / clean-up\n\n dir.c | 35 ++++-\n t/t2205-add-worktree-config.sh | 265 +++++++++++++++++++++++++++++++++\n 2 files changed, 293 insertions(+), 7 deletions(-)\n create mode 100755 t/t2205-add-worktree-config.sh\n\nRange-diff against v3:\n1: 0fc8886f1e ! 1: f84cefe731 dir: traverse into repository\n @@ t/t2205-add-worktree-config.sh (new)\n +\t(\n +\tcd test1 &&\n +\tlocal parent_dir=\"$(pwd)\" &&\n -+\t(\n -+\t\tcd repo &&\n -+\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-all-unsorted\n -+\t) &&\n ++\tgit -C repo ls-files -o --exclude-standard \"$parent_dir\" >actual-all-unsorted &&\n +\tsort actual-all-unsorted >actual-all &&\n +\tsort expect-all-unsorted >expect-all &&\n +\ttest_cmp expect-all actual-all\n @@ t/t2205-add-worktree-config.sh (new)\n +\t(\n +\tcd test1 &&\n +\tlocal parent_dir=\"$(pwd)\" &&\n -+\t(\n -+\t\tcd repo &&\n -+\t\tgit ls-files -o --directory --exclude-standard \"$parent_dir\" >../actual-all-dir-unsorted\n -+\t) &&\n ++\tgit -C repo ls-files -o --directory --exclude-standard \"$parent_dir\" >actual-all-dir-unsorted &&\n +\tsort actual-all-dir-unsorted >actual-all &&\n +\tsort expect-all-dir-unsorted >expect-all &&\n +\ttest_cmp expect-all actual-all\n @@ t/t2205-add-worktree-config.sh (new)\n +\t(\n +\tcd test1 &&\n +\tlocal parent_dir=\"$(pwd)\" &&\n -+\t(\n -+\t\tcd repo &&\n -+\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-untracked-unsorted\n -+\t) &&\n ++\tgit -C repo ls-files -o --exclude-standard \"$parent_dir\" >actual-untracked-unsorted &&\n +\tsort actual-untracked-unsorted >actual-untracked &&\n +\tsort expect-untracked-unsorted >expect-untracked &&\n +\ttest_cmp expect-untracked actual-untracked\n2: a80cbd5517 < -: ---------- dir: cache git_dir's realpath\n3: 899c69300c = 2: d4ff1bd40a dir: minor refactoring / clean-up\n-- \n2.36.0\n\n"},{"id":"457433","messageId":"DM6PR13MB29877F2E161FD450CC2BD8339DAC9@DM6PR13MB2987.namprd13.prod.outlook.com","threadId":"57851","inReplyTo":"20220616231956.154-1-gg.oss@outlook.com","subject":"[PATCH v4 1/2] dir: traverse into repository","fromName":"Goss Geppert","fromEmail":"gg.oss@outlook.com","sentAt":"2022-06-16T23:19:55Z","receivedAt":"2022-06-16T23:20:49Z","isPatch":true,"sender":{"key":"gg.oss@outlook.com","avatar":null},"body":"From: Goss Geppert \n\nSince 8d92fb2927 (dir: replace exponential algorithm with a linear one,\n2020-04-01) traversing into a repository's directory tree when the\ntraversal began outside the repository's standard location has failed\nbecause the encountered repository was identified as a nested foreign\nrepository.\n\nPrior to this commit, the failure to traverse into a repository's\ndefault worktree location was observable from a user's perspective under\neither of the following conditions (there may be others):\n\n 1) Set the `core.worktree` location to a parent directory of the\n default worktree; or\n 2) Use the `--git_dir` option while the working directory is outside\n the repository's default worktree location\n\nUnder either of these conditions, symptoms of the failure to traverse\ninto the repository's default worktree location include the inability to\nadd files to the index or get a list of untracked files via ls-files.\n\nThis commit adds a check to determine whether a nested repository that\nis encountered in recursing a path is actually `the_repository`. If so,\nwe simply treat the directory as if it doesn't contain a nested\nrepository.\n\nThe commit includes test-cases to reduce the likelihood of future\nregressions.\n\nSigned-off-by: Goss Geppert \n---\n dir.c | 19 +++\n t/t2205-add-worktree-config.sh | 265 +++++++++++++++++++++++++++++++++\n 2 files changed, 284 insertions(+)\n create mode 100755 t/t2205-add-worktree-config.sh\n\ndiff --git a/dir.c b/dir.c\nindex 6ca2ef5f04..c4f41247e0 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1893,9 +1893,28 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \n \tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n \t\t!(dir->flags & DIR_NO_GITLINKS)) {\n+\t\t/*\n+\t\t * Determine if `dirname` is a nested repo by confirming that:\n+\t\t * 1) we are in a nonbare repository, and\n+\t\t * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,\n+\t\t * which could occur if the git_dir or worktree location was\n+\t\t * manually configured by the user; see t2205 testcases 1-3 for\n+\t\t * examples where this matters\n+\t\t */\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, dirname);\n \t\tnested_repo = is_nonbare_repository_dir(&sb);\n+\n+\t\tif (nested_repo) {\n+\t\t\tchar *real_dirname, *real_gitdir;\n+\t\t\tstrbuf_addstr(&sb, \".git\");\n+\t\t\treal_dirname = real_pathdup(sb.buf, 1);\n+\t\t\treal_gitdir = real_pathdup(the_repository->gitdir, 1);\n+\n+\t\t\tnested_repo = !!strcmp(real_dirname, real_gitdir);\n+\t\t\tfree(real_gitdir);\n+\t\t\tfree(real_dirname);\n+\t\t}\n \t\tstrbuf_release(&sb);\n \t}\n \tif (nested_repo) {\ndiff --git a/t/t2205-add-worktree-config.sh b/t/t2205-add-worktree-config.sh\nnew file mode 100755\nindex 0000000000..43d950de64\n--- /dev/null\n+++ b/t/t2205-add-worktree-config.sh\n@@ -0,0 +1,265 @@\n+#!/bin/sh\n+\n+test_description='directory traversal respects user config\n+\n+This test verifies the traversal of the directory tree when the traversal begins\n+outside the repository. Two instances for which this can occur are tested:\n+\n+\t1) The user manually sets the worktree. For this instance, the test sets\n+\t the worktree two levels above the `.git` directory and checks whether we\n+\t are able to add to the index those files that are in either (1) the\n+\t manually configured worktree directory or (2) the standard worktree\n+\t location with respect to the `.git` directory (i.e. ensuring that the\n+\t encountered `.git` directory is not treated as belonging to a foreign\n+\t nested repository).\n+\t2) The user manually sets the `git_dir` while the working directory is\n+\t outside the repository. The test checks that files inside the\n+\t repository can be added to the index.\n+\t'\n+\n+. ./test-lib.sh\n+\n+test_expect_success '1a: setup--config worktree' '\n+\tmkdir test1 &&\n+\t(\n+\tcd test1 &&\n+\ttest_create_repo repo &&\n+\tgit --git-dir=\"repo/.git\" config core.worktree \"$(pwd)\" &&\n+\n+\tmkdir -p outside-tracked outside-untracked &&\n+\tmkdir -p repo/inside-tracked repo/inside-untracked &&\n+\t>file-tracked &&\n+\t>file-untracked &&\n+\t>outside-tracked/file &&\n+\t>outside-untracked/file &&\n+\t>repo/file-tracked &&\n+\t>repo/file-untracked &&\n+\t>repo/inside-tracked/file &&\n+\t>repo/inside-untracked/file &&\n+\n+\tcat >expect-tracked-unsorted <<-EOF &&\n+\t../file-tracked\n+\t../outside-tracked/file\n+\tfile-tracked\n+\tinside-tracked/file\n+\tEOF\n+\n+\tcat >expect-untracked-unsorted <<-EOF &&\n+\t../file-untracked\n+\t../outside-untracked/file\n+\tfile-untracked\n+\tinside-untracked/file\n+\tEOF\n+\n+\tcat >expect-all-dir-unsorted <<-EOF &&\n+\t../file-untracked\n+\t../file-tracked\n+\t../outside-untracked/\n+\t../outside-tracked/\n+\t./\n+\tEOF\n+\n+\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n+\n+\tcat >.gitignore <<-EOF\n+\t.gitignore\n+\tactual-*\n+\texpect-*\n+\tEOF\n+\t)\n+'\n+\n+test_expect_success '1b: pre-add all' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\tgit -C repo ls-files -o --exclude-standard \"$parent_dir\" >actual-all-unsorted &&\n+\tsort actual-all-unsorted >actual-all &&\n+\tsort expect-all-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '1c: pre-add dir all' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\tgit -C repo ls-files -o --directory --exclude-standard \"$parent_dir\" >actual-all-dir-unsorted &&\n+\tsort actual-all-dir-unsorted >actual-all &&\n+\tsort expect-all-dir-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '1d: post-add tracked' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit add file-tracked &&\n+\t\tgit add inside-tracked &&\n+\t\tgit add ../outside-tracked &&\n+\t\tgit add \"$parent_dir/file-tracked\" &&\n+\t\tgit ls-files \"$parent_dir\" >../actual-tracked-unsorted\n+\t) &&\n+\tsort actual-tracked-unsorted >actual-tracked &&\n+\tsort expect-tracked-unsorted >expect-tracked &&\n+\ttest_cmp expect-tracked actual-tracked\n+\t)\n+'\n+\n+test_expect_success '1e: post-add untracked' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\tgit -C repo ls-files -o --exclude-standard \"$parent_dir\" >actual-untracked-unsorted &&\n+\tsort actual-untracked-unsorted >actual-untracked &&\n+\tsort expect-untracked-unsorted >expect-untracked &&\n+\ttest_cmp expect-untracked actual-untracked\n+\t)\n+'\n+\n+test_expect_success '2a: setup--set git-dir' '\n+\tmkdir test2 &&\n+\t(\n+\tcd test2 &&\n+\ttest_create_repo repo &&\n+\t# create two foreign repositories that should remain untracked\n+\ttest_create_repo repo-outside &&\n+\ttest_create_repo repo/repo-inside &&\n+\n+\tmkdir -p repo/inside-tracked repo/inside-untracked &&\n+\t>repo/file-tracked &&\n+\t>repo/file-untracked &&\n+\t>repo/inside-tracked/file &&\n+\t>repo/inside-untracked/file &&\n+\t>repo-outside/file &&\n+\t>repo/repo-inside/file &&\n+\n+\tcat >expect-tracked-unsorted <<-EOF &&\n+\trepo/file-tracked\n+\trepo/inside-tracked/file\n+\tEOF\n+\n+\tcat >expect-untracked-unsorted <<-EOF &&\n+\trepo/file-untracked\n+\trepo/inside-untracked/file\n+\trepo/repo-inside/\n+\trepo-outside/\n+\tEOF\n+\n+\tcat >expect-all-dir-unsorted <<-EOF &&\n+\trepo/\n+\trepo-outside/\n+\tEOF\n+\n+\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n+\n+\tcat >.gitignore <<-EOF\n+\t.gitignore\n+\tactual-*\n+\texpect-*\n+\tEOF\n+\t)\n+'\n+\n+test_expect_success '2b: pre-add all' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git ls-files -o --exclude-standard >actual-all-unsorted &&\n+\tsort actual-all-unsorted >actual-all &&\n+\tsort expect-all-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '2c: pre-add dir all' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git ls-files -o --directory --exclude-standard >actual-all-dir-unsorted &&\n+\tsort actual-all-dir-unsorted >actual-all &&\n+\tsort expect-all-dir-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '2d: post-add tracked' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git add repo/file-tracked &&\n+\tgit --git-dir=repo/.git add repo/inside-tracked &&\n+\tgit --git-dir=repo/.git ls-files >actual-tracked-unsorted &&\n+\tsort actual-tracked-unsorted >actual-tracked &&\n+\tsort expect-tracked-unsorted >expect-tracked &&\n+\ttest_cmp expect-tracked actual-tracked\n+\t)\n+'\n+\n+test_expect_success '2e: post-add untracked' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git ls-files -o --exclude-standard >actual-untracked-unsorted &&\n+\tsort actual-untracked-unsorted >actual-untracked &&\n+\tsort expect-untracked-unsorted >expect-untracked &&\n+\ttest_cmp expect-untracked actual-untracked\n+\t)\n+'\n+\n+test_expect_success '3a: setup--add repo dir' '\n+\tmkdir test3 &&\n+\t(\n+\tcd test3 &&\n+\ttest_create_repo repo &&\n+\n+\tmkdir -p repo/inside-tracked repo/inside-ignored &&\n+\t>repo/file-tracked &&\n+\t>repo/file-ignored &&\n+\t>repo/inside-tracked/file &&\n+\t>repo/inside-ignored/file &&\n+\n+\tcat >.gitignore <<-EOF &&\n+\t.gitignore\n+\tactual-*\n+\texpect-*\n+\t*ignored\n+\tEOF\n+\n+\tcat >expect-tracked-unsorted <<-EOF &&\n+\trepo/file-tracked\n+\trepo/inside-tracked/file\n+\tEOF\n+\n+\tcat >expect-ignored-unsorted <<-EOF\n+\trepo/file-ignored\n+\trepo/inside-ignored/\n+\t.gitignore\n+\tactual-ignored-unsorted\n+\texpect-ignored-unsorted\n+\texpect-tracked-unsorted\n+\tEOF\n+\t)\n+'\n+\n+test_expect_success '3b: ignored' '\n+\t(\n+\tcd test3 &&\n+\tgit --git-dir=repo/.git ls-files -io --directory --exclude-standard >actual-ignored-unsorted &&\n+\tsort actual-ignored-unsorted >actual-ignored &&\n+\tsort expect-ignored-unsorted >expect-ignored &&\n+\ttest_cmp expect-ignored actual-ignored\n+\t)\n+'\n+\n+test_expect_success '3c: add repo' '\n+\t(\n+\tcd test3 &&\n+\tgit --git-dir=repo/.git add repo &&\n+\tgit --git-dir=repo/.git ls-files >actual-tracked-unsorted &&\n+\tsort actual-tracked-unsorted >actual-tracked &&\n+\tsort expect-tracked-unsorted >expect-tracked &&\n+\ttest_cmp expect-tracked actual-tracked\n+\t)\n+'\n+\n+test_done\n-- \n2.36.0\n\n"},{"id":"457434","messageId":"DM6PR13MB29879E6133A7DCC9903C4B4A9DAC9@DM6PR13MB2987.namprd13.prod.outlook.com","threadId":"57851","inReplyTo":"20220505203234.21586-1-ggossdev@gmail.com","subject":"[PATCH v4 0/2] dir: traverse into repository (resending)","fromName":"Goss Geppert","fromEmail":"gg.oss@outlook.com","sentAt":"2022-06-16T23:44:31Z","receivedAt":"2022-06-16T23:44:55Z","isPatch":true,"sender":{"key":"gg.oss@outlook.com","avatar":null},"body":"From: Goss Geppert \n\nAttempt #2 to send this. It seems sending patch v4 2/2 resulted in an\nerror with the outgoing server.\n\nOriginal message below:\n\nThis latest version of the patch series contains relatively minor\nmodifications relative to the previous version. Let me know if there\nis anything else I need to do.\n\nChanges compared to v3:\n* remove the commit that caches the gitdir's realpath as this was deemed\n a premature optimization given the amount of code churn\n* use git option `-C` in some of the testcases to avoid creating a\n subshell\n\nGoss Geppert (2):\n dir: traverse into repository\n dir: minor refactoring / clean-up\n\n dir.c | 35 ++++-\n t/t2205-add-worktree-config.sh | 265 +++++++++++++++++++++++++++++++++\n 2 files changed, 293 insertions(+), 7 deletions(-)\n create mode 100755 t/t2205-add-worktree-config.sh\n\nRange-diff against v3:\n1: 0fc8886f1e ! 1: f84cefe731 dir: traverse into repository\n @@ t/t2205-add-worktree-config.sh (new)\n +\t(\n +\tcd test1 &&\n +\tlocal parent_dir=\"$(pwd)\" &&\n -+\t(\n -+\t\tcd repo &&\n -+\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-all-unsorted\n -+\t) &&\n ++\tgit -C repo ls-files -o --exclude-standard \"$parent_dir\" >actual-all-unsorted &&\n +\tsort actual-all-unsorted >actual-all &&\n +\tsort expect-all-unsorted >expect-all &&\n +\ttest_cmp expect-all actual-all\n @@ t/t2205-add-worktree-config.sh (new)\n +\t(\n +\tcd test1 &&\n +\tlocal parent_dir=\"$(pwd)\" &&\n -+\t(\n -+\t\tcd repo &&\n -+\t\tgit ls-files -o --directory --exclude-standard \"$parent_dir\" >../actual-all-dir-unsorted\n -+\t) &&\n ++\tgit -C repo ls-files -o --directory --exclude-standard \"$parent_dir\" >actual-all-dir-unsorted &&\n +\tsort actual-all-dir-unsorted >actual-all &&\n +\tsort expect-all-dir-unsorted >expect-all &&\n +\ttest_cmp expect-all actual-all\n @@ t/t2205-add-worktree-config.sh (new)\n +\t(\n +\tcd test1 &&\n +\tlocal parent_dir=\"$(pwd)\" &&\n -+\t(\n -+\t\tcd repo &&\n -+\t\tgit ls-files -o --exclude-standard \"$parent_dir\" >../actual-untracked-unsorted\n -+\t) &&\n ++\tgit -C repo ls-files -o --exclude-standard \"$parent_dir\" >actual-untracked-unsorted &&\n +\tsort actual-untracked-unsorted >actual-untracked &&\n +\tsort expect-untracked-unsorted >expect-untracked &&\n +\ttest_cmp expect-untracked actual-untracked\n2: a80cbd5517 < -: ---------- dir: cache git_dir's realpath\n3: 899c69300c = 2: d4ff1bd40a dir: minor refactoring / clean-up\n-- \n2.36.0\n\n"},{"id":"457435","messageId":"DM6PR13MB29875B5203E253D1AF983A169DAC9@DM6PR13MB2987.namprd13.prod.outlook.com","threadId":"57851","inReplyTo":"20220616234433.225-1-gg.oss@outlook.com","subject":"[PATCH v4 1/2] dir: traverse into repository","fromName":"Goss Geppert","fromEmail":"gg.oss@outlook.com","sentAt":"2022-06-16T23:44:32Z","receivedAt":"2022-06-16T23:44:56Z","isPatch":true,"sender":{"key":"gg.oss@outlook.com","avatar":null},"body":"From: Goss Geppert \n\nSince 8d92fb2927 (dir: replace exponential algorithm with a linear one,\n2020-04-01) traversing into a repository's directory tree when the\ntraversal began outside the repository's standard location has failed\nbecause the encountered repository was identified as a nested foreign\nrepository.\n\nPrior to this commit, the failure to traverse into a repository's\ndefault worktree location was observable from a user's perspective under\neither of the following conditions (there may be others):\n\n 1) Set the `core.worktree` location to a parent directory of the\n default worktree; or\n 2) Use the `--git_dir` option while the working directory is outside\n the repository's default worktree location\n\nUnder either of these conditions, symptoms of the failure to traverse\ninto the repository's default worktree location include the inability to\nadd files to the index or get a list of untracked files via ls-files.\n\nThis commit adds a check to determine whether a nested repository that\nis encountered in recursing a path is actually `the_repository`. If so,\nwe simply treat the directory as if it doesn't contain a nested\nrepository.\n\nThe commit includes test-cases to reduce the likelihood of future\nregressions.\n\nSigned-off-by: Goss Geppert \n---\n dir.c | 19 +++\n t/t2205-add-worktree-config.sh | 265 +++++++++++++++++++++++++++++++++\n 2 files changed, 284 insertions(+)\n create mode 100755 t/t2205-add-worktree-config.sh\n\ndiff --git a/dir.c b/dir.c\nindex 6ca2ef5f04..c4f41247e0 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1893,9 +1893,28 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \n \tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n \t\t!(dir->flags & DIR_NO_GITLINKS)) {\n+\t\t/*\n+\t\t * Determine if `dirname` is a nested repo by confirming that:\n+\t\t * 1) we are in a nonbare repository, and\n+\t\t * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,\n+\t\t * which could occur if the git_dir or worktree location was\n+\t\t * manually configured by the user; see t2205 testcases 1-3 for\n+\t\t * examples where this matters\n+\t\t */\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, dirname);\n \t\tnested_repo = is_nonbare_repository_dir(&sb);\n+\n+\t\tif (nested_repo) {\n+\t\t\tchar *real_dirname, *real_gitdir;\n+\t\t\tstrbuf_addstr(&sb, \".git\");\n+\t\t\treal_dirname = real_pathdup(sb.buf, 1);\n+\t\t\treal_gitdir = real_pathdup(the_repository->gitdir, 1);\n+\n+\t\t\tnested_repo = !!strcmp(real_dirname, real_gitdir);\n+\t\t\tfree(real_gitdir);\n+\t\t\tfree(real_dirname);\n+\t\t}\n \t\tstrbuf_release(&sb);\n \t}\n \tif (nested_repo) {\ndiff --git a/t/t2205-add-worktree-config.sh b/t/t2205-add-worktree-config.sh\nnew file mode 100755\nindex 0000000000..43d950de64\n--- /dev/null\n+++ b/t/t2205-add-worktree-config.sh\n@@ -0,0 +1,265 @@\n+#!/bin/sh\n+\n+test_description='directory traversal respects user config\n+\n+This test verifies the traversal of the directory tree when the traversal begins\n+outside the repository. Two instances for which this can occur are tested:\n+\n+\t1) The user manually sets the worktree. For this instance, the test sets\n+\t the worktree two levels above the `.git` directory and checks whether we\n+\t are able to add to the index those files that are in either (1) the\n+\t manually configured worktree directory or (2) the standard worktree\n+\t location with respect to the `.git` directory (i.e. ensuring that the\n+\t encountered `.git` directory is not treated as belonging to a foreign\n+\t nested repository).\n+\t2) The user manually sets the `git_dir` while the working directory is\n+\t outside the repository. The test checks that files inside the\n+\t repository can be added to the index.\n+\t'\n+\n+. ./test-lib.sh\n+\n+test_expect_success '1a: setup--config worktree' '\n+\tmkdir test1 &&\n+\t(\n+\tcd test1 &&\n+\ttest_create_repo repo &&\n+\tgit --git-dir=\"repo/.git\" config core.worktree \"$(pwd)\" &&\n+\n+\tmkdir -p outside-tracked outside-untracked &&\n+\tmkdir -p repo/inside-tracked repo/inside-untracked &&\n+\t>file-tracked &&\n+\t>file-untracked &&\n+\t>outside-tracked/file &&\n+\t>outside-untracked/file &&\n+\t>repo/file-tracked &&\n+\t>repo/file-untracked &&\n+\t>repo/inside-tracked/file &&\n+\t>repo/inside-untracked/file &&\n+\n+\tcat >expect-tracked-unsorted <<-EOF &&\n+\t../file-tracked\n+\t../outside-tracked/file\n+\tfile-tracked\n+\tinside-tracked/file\n+\tEOF\n+\n+\tcat >expect-untracked-unsorted <<-EOF &&\n+\t../file-untracked\n+\t../outside-untracked/file\n+\tfile-untracked\n+\tinside-untracked/file\n+\tEOF\n+\n+\tcat >expect-all-dir-unsorted <<-EOF &&\n+\t../file-untracked\n+\t../file-tracked\n+\t../outside-untracked/\n+\t../outside-tracked/\n+\t./\n+\tEOF\n+\n+\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n+\n+\tcat >.gitignore <<-EOF\n+\t.gitignore\n+\tactual-*\n+\texpect-*\n+\tEOF\n+\t)\n+'\n+\n+test_expect_success '1b: pre-add all' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\tgit -C repo ls-files -o --exclude-standard \"$parent_dir\" >actual-all-unsorted &&\n+\tsort actual-all-unsorted >actual-all &&\n+\tsort expect-all-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '1c: pre-add dir all' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\tgit -C repo ls-files -o --directory --exclude-standard \"$parent_dir\" >actual-all-dir-unsorted &&\n+\tsort actual-all-dir-unsorted >actual-all &&\n+\tsort expect-all-dir-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '1d: post-add tracked' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\t(\n+\t\tcd repo &&\n+\t\tgit add file-tracked &&\n+\t\tgit add inside-tracked &&\n+\t\tgit add ../outside-tracked &&\n+\t\tgit add \"$parent_dir/file-tracked\" &&\n+\t\tgit ls-files \"$parent_dir\" >../actual-tracked-unsorted\n+\t) &&\n+\tsort actual-tracked-unsorted >actual-tracked &&\n+\tsort expect-tracked-unsorted >expect-tracked &&\n+\ttest_cmp expect-tracked actual-tracked\n+\t)\n+'\n+\n+test_expect_success '1e: post-add untracked' '\n+\t(\n+\tcd test1 &&\n+\tlocal parent_dir=\"$(pwd)\" &&\n+\tgit -C repo ls-files -o --exclude-standard \"$parent_dir\" >actual-untracked-unsorted &&\n+\tsort actual-untracked-unsorted >actual-untracked &&\n+\tsort expect-untracked-unsorted >expect-untracked &&\n+\ttest_cmp expect-untracked actual-untracked\n+\t)\n+'\n+\n+test_expect_success '2a: setup--set git-dir' '\n+\tmkdir test2 &&\n+\t(\n+\tcd test2 &&\n+\ttest_create_repo repo &&\n+\t# create two foreign repositories that should remain untracked\n+\ttest_create_repo repo-outside &&\n+\ttest_create_repo repo/repo-inside &&\n+\n+\tmkdir -p repo/inside-tracked repo/inside-untracked &&\n+\t>repo/file-tracked &&\n+\t>repo/file-untracked &&\n+\t>repo/inside-tracked/file &&\n+\t>repo/inside-untracked/file &&\n+\t>repo-outside/file &&\n+\t>repo/repo-inside/file &&\n+\n+\tcat >expect-tracked-unsorted <<-EOF &&\n+\trepo/file-tracked\n+\trepo/inside-tracked/file\n+\tEOF\n+\n+\tcat >expect-untracked-unsorted <<-EOF &&\n+\trepo/file-untracked\n+\trepo/inside-untracked/file\n+\trepo/repo-inside/\n+\trepo-outside/\n+\tEOF\n+\n+\tcat >expect-all-dir-unsorted <<-EOF &&\n+\trepo/\n+\trepo-outside/\n+\tEOF\n+\n+\tcat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&\n+\n+\tcat >.gitignore <<-EOF\n+\t.gitignore\n+\tactual-*\n+\texpect-*\n+\tEOF\n+\t)\n+'\n+\n+test_expect_success '2b: pre-add all' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git ls-files -o --exclude-standard >actual-all-unsorted &&\n+\tsort actual-all-unsorted >actual-all &&\n+\tsort expect-all-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '2c: pre-add dir all' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git ls-files -o --directory --exclude-standard >actual-all-dir-unsorted &&\n+\tsort actual-all-dir-unsorted >actual-all &&\n+\tsort expect-all-dir-unsorted >expect-all &&\n+\ttest_cmp expect-all actual-all\n+\t)\n+'\n+\n+test_expect_success '2d: post-add tracked' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git add repo/file-tracked &&\n+\tgit --git-dir=repo/.git add repo/inside-tracked &&\n+\tgit --git-dir=repo/.git ls-files >actual-tracked-unsorted &&\n+\tsort actual-tracked-unsorted >actual-tracked &&\n+\tsort expect-tracked-unsorted >expect-tracked &&\n+\ttest_cmp expect-tracked actual-tracked\n+\t)\n+'\n+\n+test_expect_success '2e: post-add untracked' '\n+\t(\n+\tcd test2 &&\n+\tgit --git-dir=repo/.git ls-files -o --exclude-standard >actual-untracked-unsorted &&\n+\tsort actual-untracked-unsorted >actual-untracked &&\n+\tsort expect-untracked-unsorted >expect-untracked &&\n+\ttest_cmp expect-untracked actual-untracked\n+\t)\n+'\n+\n+test_expect_success '3a: setup--add repo dir' '\n+\tmkdir test3 &&\n+\t(\n+\tcd test3 &&\n+\ttest_create_repo repo &&\n+\n+\tmkdir -p repo/inside-tracked repo/inside-ignored &&\n+\t>repo/file-tracked &&\n+\t>repo/file-ignored &&\n+\t>repo/inside-tracked/file &&\n+\t>repo/inside-ignored/file &&\n+\n+\tcat >.gitignore <<-EOF &&\n+\t.gitignore\n+\tactual-*\n+\texpect-*\n+\t*ignored\n+\tEOF\n+\n+\tcat >expect-tracked-unsorted <<-EOF &&\n+\trepo/file-tracked\n+\trepo/inside-tracked/file\n+\tEOF\n+\n+\tcat >expect-ignored-unsorted <<-EOF\n+\trepo/file-ignored\n+\trepo/inside-ignored/\n+\t.gitignore\n+\tactual-ignored-unsorted\n+\texpect-ignored-unsorted\n+\texpect-tracked-unsorted\n+\tEOF\n+\t)\n+'\n+\n+test_expect_success '3b: ignored' '\n+\t(\n+\tcd test3 &&\n+\tgit --git-dir=repo/.git ls-files -io --directory --exclude-standard >actual-ignored-unsorted &&\n+\tsort actual-ignored-unsorted >actual-ignored &&\n+\tsort expect-ignored-unsorted >expect-ignored &&\n+\ttest_cmp expect-ignored actual-ignored\n+\t)\n+'\n+\n+test_expect_success '3c: add repo' '\n+\t(\n+\tcd test3 &&\n+\tgit --git-dir=repo/.git add repo &&\n+\tgit --git-dir=repo/.git ls-files >actual-tracked-unsorted &&\n+\tsort actual-tracked-unsorted >actual-tracked &&\n+\tsort expect-tracked-unsorted >expect-tracked &&\n+\ttest_cmp expect-tracked actual-tracked\n+\t)\n+'\n+\n+test_done\n-- \n2.36.0\n\n"},{"id":"457436","messageId":"DM6PR13MB29871C3B675679262F9960389DAC9@DM6PR13MB2987.namprd13.prod.outlook.com","threadId":"57851","inReplyTo":"20220616234433.225-1-gg.oss@outlook.com","subject":"[PATCH v4 2/2] dir: minor refactoring / clean-up","fromName":"Goss Geppert","fromEmail":"gg.oss@outlook.com","sentAt":"2022-06-16T23:44:33Z","receivedAt":"2022-06-16T23:45:05Z","isPatch":true,"sender":{"key":"gg.oss@outlook.com","avatar":null},"body":"From: Goss Geppert \n\nNarrow the scope of the `nested_repo` variable and conditional return\nstatement to the block where the variable is set.\n\nSigned-off-by: Goss Geppert \n---\n dir.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex c4f41247e0..d7cfb08e44 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1861,7 +1861,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t */\n \tenum path_treatment state;\n \tint matches_how = 0;\n-\tint nested_repo = 0, check_only, stop_early;\n+\tint check_only, stop_early;\n \tint old_ignored_nr, old_untracked_nr;\n \t/* The \"len-1\" is to strip the final '/' */\n \tenum exist_status status = directory_exists_in_index(istate, dirname, len-1);\n@@ -1901,6 +1901,7 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\t * manually configured by the user; see t2205 testcases 1-3 for\n \t\t * examples where this matters\n \t\t */\n+\t\tint nested_repo;\n \t\tstruct strbuf sb = STRBUF_INIT;\n \t\tstrbuf_addstr(&sb, dirname);\n \t\tnested_repo = is_nonbare_repository_dir(&sb);\n@@ -1916,12 +1917,13 @@ static enum path_treatment treat_directory(struct dir_struct *dir,\n \t\t\tfree(real_dirname);\n \t\t}\n \t\tstrbuf_release(&sb);\n-\t}\n-\tif (nested_repo) {\n-\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n-\t\t (matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n-\t\t\treturn path_none;\n-\t\treturn excluded ? path_excluded : path_untracked;\n+\n+\t\tif (nested_repo) {\n+\t\t\tif ((dir->flags & DIR_SKIP_NESTED_GIT) ||\n+\t\t\t\t(matches_how == MATCHED_RECURSIVELY_LEADING_PATHSPEC))\n+\t\t\t\treturn path_none;\n+\t\t\treturn excluded ? path_excluded : path_untracked;\n+\t\t}\n \t}\n \n \tif (!(dir->flags & DIR_SHOW_OTHER_DIRECTORIES)) {\n-- \n2.36.0\n\n"},{"id":"457679","messageId":"CABPp-BExinbGArOQQTkyPWz-KhJL7T6D0fsvP88quv-TM=zF3A@mail.gmail.com","threadId":"57851","inReplyTo":"DM6PR13MB29876A53962557131120EF619DAC9@DM6PR13MB2987.namprd13.prod.outlook.com","subject":"Re: [PATCH v4 0/2] dir: traverse into repository","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-06-22T04:57:38Z","receivedAt":"2022-06-22T04:58:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jun 16, 2022 at 4:20 PM Goss Geppert wrote:\n>\n> From: Goss Geppert \n>\n> This latest version of the patch series contains relatively minor\n> modifications relative to the previous version. Let me know if there\n> is anything else I need to do.\n>\n> Changes compared to v3:\n> * remove the commit that caches the gitdir's realpath as this was deemed\n> a premature optimization given the amount of code churn\n> * use git option `-C` in some of the testcases to avoid creating a\n> subshell\n>\n> Goss Geppert (2):\n> dir: traverse into repository\n> dir: minor refactoring / clean-up\n>\n> dir.c | 35 ++++-\n> t/t2205-add-worktree-config.sh | 265 +++++++++++++++++++++++++++++++++\n> 2 files changed, 293 insertions(+), 7 deletions(-)\n> create mode 100755 t/t2205-add-worktree-config.sh\n>\n> Range-diff against v3:\n> 1: 0fc8886f1e ! 1: f84cefe731 dir: traverse into repository\n> @@ t/t2205-add-worktree-config.sh (new)\n> + (\n> + cd test1 &&\n> + local parent_dir=\"$(pwd)\" &&\n> -+ (\n> -+ cd repo &&\n> -+ git ls-files -o --exclude-standard \"$parent_dir\" >../actual-all-unsorted\n> -+ ) &&\n> ++ git -C repo ls-files -o --exclude-standard \"$parent_dir\" >actual-all-unsorted &&\n> + sort actual-all-unsorted >actual-all &&\n> + sort expect-all-unsorted >expect-all &&\n> + test_cmp expect-all actual-all\n> @@ t/t2205-add-worktree-config.sh (new)\n> + (\n> + cd test1 &&\n> + local parent_dir=\"$(pwd)\" &&\n> -+ (\n> -+ cd repo &&\n> -+ git ls-files -o --directory --exclude-standard \"$parent_dir\" >../actual-all-dir-unsorted\n> -+ ) &&\n> ++ git -C repo ls-files -o --directory --exclude-standard \"$parent_dir\" >actual-all-dir-unsorted &&\n> + sort actual-all-dir-unsorted >actual-all &&\n> + sort expect-all-dir-unsorted >expect-all &&\n> + test_cmp expect-all actual-all\n> @@ t/t2205-add-worktree-config.sh (new)\n> + (\n> + cd test1 &&\n> + local parent_dir=\"$(pwd)\" &&\n> -+ (\n> -+ cd repo &&\n> -+ git ls-files -o --exclude-standard \"$parent_dir\" >../actual-untracked-unsorted\n> -+ ) &&\n> ++ git -C repo ls-files -o --exclude-standard \"$parent_dir\" >actual-untracked-unsorted &&\n> + sort actual-untracked-unsorted >actual-untracked &&\n> + sort expect-untracked-unsorted >expect-untracked &&\n> + test_cmp expect-untracked actual-untracked\n> 2: a80cbd5517 < -: ---------- dir: cache git_dir's realpath\n> 3: 899c69300c = 2: d4ff1bd40a dir: minor refactoring / clean-up\n\nThanks, this version looks good to me.\n\nReviewed-by: Elijah Newren \n"}]}