{"thread":{"id":"56611","subject":"Bug/regression report - 'git stash push -u' fatal errors when sub-repo present","startedAt":"2021-09-30T00:49:32Z","lastAt":"2021-10-08T09:04:20Z","messageCount":11,"participants":["Robert Leftwich","René Scharfe","Junio C Hamano","Derrick Stolee"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"437492","messageId":"CACr9BXmP1vQMK4b27Uc4R-3WWYHUYfCEEMN+hnth4yUg+UN7Zg@mail.gmail.com","threadId":"56611","inReplyTo":null,"subject":"Bug/regression report - 'git stash push -u' fatal errors when sub-repo present","fromName":"Robert Leftwich","fromEmail":"robert@gitpod.io","sentAt":"2021-09-30T00:49:20Z","receivedAt":"2021-09-30T00:49:32Z","isPatch":false,"sender":{"key":"robert@gitpod.io","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n  Initialised a repo and added a file or cloned a repo inside an existing\nrepo (think dependencies).\n  See https://github.com/gitpod-io/gitpod/issues/5675 for background.\n\n  In an existing repo:\n    $ mkdir sub_test && cd sub_test/ && git init . && touch test.txt && git\nadd test.txt\n    OR\n    $ git clone https://github.com/stencila/test.git sub_test\n    THEN\n    $ git stash push -u\n\nWhat did you expect to happen? (Expected behavior)\n  Command should complete without error but ignore the directory (this is\nthe existing behavior prior to v2.31)\n    $ git stash push -u\n    Ignoring path sub_test\n    Saved working directory and index state WIP on (no branch): 94f6e3e283\nGit 2.30.2\n\nWhat happened instead? (Actual behavior)\n  Command failed\n    $ git stash push -u\n    error: sub_test/: is a directory - add files inside instead\n    fatal: Unable to process path sub_test/\n    Cannot save the untracked files\n\nWhat's different between what you expected and what actually happened?\n  Command failed\n\nAnything else you want to add:\n  It happens on all versions from v2.31 to current master.\n  It is specifically related to this change:\n\nhttps://github.com/git/git/commit/6e773527b6b03976cefbb0f9571bd40dd5995e6c#diff-70525b6b89c7cac91e520085d954a68671038d218b77d22855e938ab075a68d8L1006\n\n  If this is the new expected behavior perhaps it can result in a better\nerror message and related documentation?\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.33.0.610.gcefe983a32\ncpu: x86_64\nbuilt from commit: cefe983a320c03d7843ac78e73bd513a27806845\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.4.0-1051-gke #54-Ubuntu SMP Thu Aug 5 18:52:13 UTC 2021\nx86_64\ncompiler info: gnuc: 9.3\nlibc info: glibc: 2.31\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\n"},{"id":"437513","messageId":"7b83c77e-dd87-f688-3da1-7826cf6b0d4e@web.de","threadId":"56611","inReplyTo":"CACr9BXmP1vQMK4b27Uc4R-3WWYHUYfCEEMN+hnth4yUg+UN7Zg@mail.gmail.com","subject":"Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-09-30T10:06:37Z","receivedAt":"2021-09-30T10:06:45Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.09.21 um 02:49 schrieb Robert Leftwich:\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n>\n> What did you do before the bug happened? (Steps to reproduce your issue)\n>   Initialised a repo and added a file or cloned a repo inside an existing\n> repo (think dependencies).\n>   See https://github.com/gitpod-io/gitpod/issues/5675 for background.\n>\n>   In an existing repo:\n>     $ mkdir sub_test && cd sub_test/ && git init . && touch test.txt && git\n> add test.txt\n>     OR\n>     $ git clone https://github.com/stencila/test.git sub_test\n>     THEN\n>     $ git stash push -u\n>\n> What did you expect to happen? (Expected behavior)\n>   Command should complete without error but ignore the directory (this is\n> the existing behavior prior to v2.31)\n>     $ git stash push -u\n>     Ignoring path sub_test\n>     Saved working directory and index state WIP on (no branch): 94f6e3e283\n> Git 2.30.2\n>\n> What happened instead? (Actual behavior)\n>   Command failed\n>     $ git stash push -u\n>     error: sub_test/: is a directory - add files inside instead\n>     fatal: Unable to process path sub_test/\n>     Cannot save the untracked files\n>\n> What's different between what you expected and what actually happened?\n>   Command failed\n>\n> Anything else you want to add:\n>   It happens on all versions from v2.31 to current master.\n>   It is specifically related to this change:\n>\n> https://github.com/git/git/commit/6e773527b6b03976cefbb0f9571bd40dd5995e6c#diff-70525b6b89c7cac91e520085d954a68671038d218b77d22855e938ab075a68d8L1006\n>\n>   If this is the new expected behavior perhaps it can result in a better\n> error message and related documentation?\n>\n> Please review the rest of the bug report below.\n> You can delete any lines you don't wish to share.\n\nLooping in Stolee as the author of 6e773527b6 (sparse-index: convert\nfrom full to sparse, 2021-03-30).\n\n>\n>\n> [System Info]\n> git version:\n> git version 2.33.0.610.gcefe983a32\n> cpu: x86_64\n> built from commit: cefe983a320c03d7843ac78e73bd513a27806845\n> sizeof-long: 8\n> sizeof-size_t: 8\n> shell-path: /bin/sh\n> uname: Linux 5.4.0-1051-gke #54-Ubuntu SMP Thu Aug 5 18:52:13 UTC 2021\n> x86_64\n> compiler info: gnuc: 9.3\n> libc info: glibc: 2.31\n> $SHELL (typically, interactive shell): /bin/bash\n>\n>\n> [Enabled Hooks]\n>\n\nHere's a raw patch for that.  Versions before 6e773527b6 pass the\nincluded test.\n\nThe magic return value of 2 is a bit ugly, but allows adding the\nadditional check only to the call-site relevant to the bug report.\n\nI don't know if other callers of verify_path() might also need that\ncheck, or if it is too narrow.\n\nRené\n\n\n---\n builtin/update-index.c             | 15 ++++++++++++++-\n read-cache.c                       |  2 +-\n t/t3905-stash-include-untracked.sh |  6 ++++++\n 3 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 187203e8bb..3d32db7304 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -445,10 +445,22 @@ static void chmod_path(char flip, const char *path)\n \tdie(\"git update-index: cannot chmod %cx '%s'\", flip, path);\n }\n\n+static int is_nonbare_repo_dir(const char *path)\n+{\n+\tint ret;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_addstr(&sb, path);\n+\tret = is_nonbare_repository_dir(&sb);\n+\tstrbuf_release(&sb);\n+\treturn ret;\n+}\n+\n static void update_one(const char *path)\n {\n \tint stat_errno = 0;\n \tstruct stat st;\n+\tint ret;\n\n \tif (mark_valid_only || mark_skip_worktree_only || force_remove ||\n \t    mark_fsmonitor_only)\n@@ -458,7 +470,8 @@ static void update_one(const char *path)\n \t\tstat_errno = errno;\n \t} /* else stat is valid */\n\n-\tif (!verify_path(path, st.st_mode)) {\n+\tret = verify_path(path, st.st_mode);\n+\tif (ret == 0 || (ret == 2 && is_nonbare_repo_dir(path))) {\n \t\tfprintf(stderr, \"Ignoring path %s\\n\", path);\n \t\treturn;\n \t}\ndiff --git a/read-cache.c b/read-cache.c\nindex f5d4385c40..0188b5b798 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1040,7 +1040,7 @@ int verify_path(const char *path, unsigned mode)\n \t\t\t * sparse directory entries.\n \t\t\t */\n \t\t\tif (c == '\\0')\n-\t\t\t\treturn S_ISDIR(mode);\n+\t\t\t\treturn S_ISDIR(mode) ? 2 : 0;\n \t\t} else if (c == '\\\\' && protect_ntfs) {\n \t\t\tif (is_ntfs_dotgit(path))\n \t\t\t\treturn 0;\ndiff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh\nindex dd2cdcc114..5390eec4e3 100755\n--- a/t/t3905-stash-include-untracked.sh\n+++ b/t/t3905-stash-include-untracked.sh\n@@ -422,4 +422,10 @@ test_expect_success 'stash show --{include,only}-untracked on stashes without un\n \ttest_must_be_empty actual\n '\n\n+test_expect_success 'stash -u ignores sub-repository' '\n+\ttest_when_finished \"rm -rf sub-repo\" &&\n+\tgit init sub-repo &&\n+\tgit stash -u\n+'\n+\n test_done\n--\n2.33.0\n"},{"id":"437553","messageId":"xmqqk0iydns7.fsf@gitster.g","threadId":"56611","inReplyTo":"7b83c77e-dd87-f688-3da1-7826cf6b0d4e@web.de","subject":"Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-30T16:35:36Z","receivedAt":"2021-09-30T16:35:41Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> Looping in Stolee as the author of 6e773527b6 (sparse-index: convert\n> from full to sparse, 2021-03-30).\n>\n> Here's a raw patch for that.  Versions before 6e773527b6 pass the\n> included test.\n>\n> The magic return value of 2 is a bit ugly, but allows adding the\n> additional check only to the call-site relevant to the bug report.\n>\n> I don't know if other callers of verify_path() might also need that\n> check, or if it is too narrow.\n\nThe callers inside read-cache.c, which I think were the original\nintended audiences of the function, always call verify_path() with\nthe pathname suitable to be stored as a cache entry.\n\nThe callee never has to assume an extra trailing slash might exist\nat the end.  And verify_path() said \"no\", if any caller passed an\nextra trailing slash before the commit in question.\n\nI _think_ we defined the new \"tree\" type entries the sparse index\nstuff added to be the _only_ cache entries whose path always ends\nwith a slash?  If so, perhaps we should audit the existing callers \nand move the \"paths that come from end-users to be entered to the\nindex must never end with a slash\" sanity check this function gave\nus, which was broken by 6e773527b6, perhaps?\n\n\"git update-index\" should never allow to create a \"tree\" kind of\ncache entry (making it sparse again should be the task of systems\ninternal, and should not be done by end-user feeding a pre-shrunk\n\"tree\" kind of entry to record a sparsely populated state, if I\nunderstand correctly), so its call to verify_path(), if it ends with\na directory separator, should say \"that's not a good path\".\n\nPrehaps we can rename verify_path() to verify_path_internal() that\nis _known_ to be called with names that are already vetted to be\nstored in ce_name and convert callers inside read-cache.c to call\nit, and write verify_path() as a thin wrapper of it to fail when the\nformer stops by returning S_ISDIR() at the place your fix touched,\nor something?\n\nThanks.\n\n>\n> René\n>\n>\n> ---\n>  builtin/update-index.c             | 15 ++++++++++++++-\n>  read-cache.c                       |  2 +-\n>  t/t3905-stash-include-untracked.sh |  6 ++++++\n>  3 files changed, 21 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/update-index.c b/builtin/update-index.c\n> index 187203e8bb..3d32db7304 100644\n> --- a/builtin/update-index.c\n> +++ b/builtin/update-index.c\n> @@ -445,10 +445,22 @@ static void chmod_path(char flip, const char *path)\n>  \tdie(\"git update-index: cannot chmod %cx '%s'\", flip, path);\n>  }\n>\n> +static int is_nonbare_repo_dir(const char *path)\n> +{\n> +\tint ret;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tstrbuf_addstr(&sb, path);\n> +\tret = is_nonbare_repository_dir(&sb);\n> +\tstrbuf_release(&sb);\n> +\treturn ret;\n> +}\n> +\n>  static void update_one(const char *path)\n>  {\n>  \tint stat_errno = 0;\n>  \tstruct stat st;\n> +\tint ret;\n>\n>  \tif (mark_valid_only || mark_skip_worktree_only || force_remove ||\n>  \t    mark_fsmonitor_only)\n> @@ -458,7 +470,8 @@ static void update_one(const char *path)\n>  \t\tstat_errno = errno;\n>  \t} /* else stat is valid */\n>\n> -\tif (!verify_path(path, st.st_mode)) {\n> +\tret = verify_path(path, st.st_mode);\n> +\tif (ret == 0 || (ret == 2 && is_nonbare_repo_dir(path))) {\n>  \t\tfprintf(stderr, \"Ignoring path %s\\n\", path);\n>  \t\treturn;\n>  \t}\n> diff --git a/read-cache.c b/read-cache.c\n> index f5d4385c40..0188b5b798 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1040,7 +1040,7 @@ int verify_path(const char *path, unsigned mode)\n>  \t\t\t * sparse directory entries.\n>  \t\t\t */\n>  \t\t\tif (c == '\\0')\n> -\t\t\t\treturn S_ISDIR(mode);\n> +\t\t\t\treturn S_ISDIR(mode) ? 2 : 0;\n>  \t\t} else if (c == '\\\\' && protect_ntfs) {\n>  \t\t\tif (is_ntfs_dotgit(path))\n>  \t\t\t\treturn 0;\n> diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh\n> index dd2cdcc114..5390eec4e3 100755\n> --- a/t/t3905-stash-include-untracked.sh\n> +++ b/t/t3905-stash-include-untracked.sh\n> @@ -422,4 +422,10 @@ test_expect_success 'stash show --{include,only}-untracked on stashes without un\n>  \ttest_must_be_empty actual\n>  '\n>\n> +test_expect_success 'stash -u ignores sub-repository' '\n> +\ttest_when_finished \"rm -rf sub-repo\" &&\n> +\tgit init sub-repo &&\n> +\tgit stash -u\n> +'\n> +\n>  test_done\n> --\n> 2.33.0\n"},{"id":"437687","messageId":"1d26a9f3-dcb5-408a-581e-40411e6a2179@web.de","threadId":"56611","inReplyTo":"xmqqk0iydns7.fsf@gitster.g","subject":"Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-01T14:25:47Z","receivedAt":"2021-10-01T14:25:58Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.09.21 um 18:35 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>> Looping in Stolee as the author of 6e773527b6 (sparse-index: convert\n>> from full to sparse, 2021-03-30).\n>>\n>> Here's a raw patch for that.  Versions before 6e773527b6 pass the\n>> included test.\n>>\n>> The magic return value of 2 is a bit ugly, but allows adding the\n>> additional check only to the call-site relevant to the bug report.\n>>\n>> I don't know if other callers of verify_path() might also need that\n>> check, or if it is too narrow.\n>\n> The callers inside read-cache.c, which I think were the original\n> intended audiences of the function, always call verify_path() with\n> the pathname suitable to be stored as a cache entry.\n>\n> The callee never has to assume an extra trailing slash might exist\n> at the end.  And verify_path() said \"no\", if any caller passed an\n> extra trailing slash before the commit in question.\n>\n> I _think_ we defined the new \"tree\" type entries the sparse index\n> stuff added to be the _only_ cache entries whose path always ends\n> with a slash?  If so, perhaps we should audit the existing callers\n> and move the \"paths that come from end-users to be entered to the\n> index must never end with a slash\" sanity check this function gave\n> us, which was broken by 6e773527b6, perhaps?\n>\n> \"git update-index\" should never allow to create a \"tree\" kind of\n> cache entry (making it sparse again should be the task of systems\n> internal, and should not be done by end-user feeding a pre-shrunk\n> \"tree\" kind of entry to record a sparsely populated state, if I\n> understand correctly), so its call to verify_path(), if it ends with\n> a directory separator, should say \"that's not a good path\".\n>\n> Prehaps we can rename verify_path() to verify_path_internal() that\n> is _known_ to be called with names that are already vetted to be\n> stored in ce_name and convert callers inside read-cache.c to call\n> it, and write verify_path() as a thin wrapper of it to fail when the\n> former stops by returning S_ISDIR() at the place your fix touched,\n> or something?\n\nRestoring the stricter check for the general case and providing a way\nout for the code handling sparse indexes sounds like a good idea.\n\nI don't know which callers are supposed to need that, but the following\npatch passes all tests, including the new one.\n\nRené\n\n\n---\n read-cache.c                       | 45 ++++++++++++++++++++----------\n t/t3905-stash-include-untracked.sh |  6 ++++\n 2 files changed, 36 insertions(+), 15 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex f5d4385c40..a1da4619c4 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -849,6 +849,19 @@ struct cache_entry *make_empty_transient_cache_entry(size_t len,\n \treturn xcalloc(1, cache_entry_size(len));\n }\n\n+enum verify_path_result {\n+\tPATH_OK,\n+\tPATH_INVALID,\n+\tPATH_DIR_WITH_SEP,\n+};\n+\n+static enum verify_path_result verify_path_internal(const char *, unsigned);\n+\n+int verify_path(const char *path, unsigned mode)\n+{\n+\treturn verify_path_internal(path, mode) == PATH_OK;\n+}\n+\n struct cache_entry *make_cache_entry(struct index_state *istate,\n \t\t\t\t     unsigned int mode,\n \t\t\t\t     const struct object_id *oid,\n@@ -859,7 +872,7 @@ struct cache_entry *make_cache_entry(struct index_state *istate,\n \tstruct cache_entry *ce, *ret;\n \tint len;\n\n-\tif (!verify_path(path, mode)) {\n+\tif (verify_path_internal(path, mode) == PATH_INVALID) {\n \t\terror(_(\"invalid path '%s'\"), path);\n \t\treturn NULL;\n \t}\n@@ -993,60 +1006,62 @@ static int verify_dotfile(const char *rest, unsigned mode)\n \treturn 1;\n }\n\n-int verify_path(const char *path, unsigned mode)\n+static enum verify_path_result verify_path_internal(const char *path,\n+\t\t\t\t\t\t    unsigned mode)\n {\n \tchar c = 0;\n\n \tif (has_dos_drive_prefix(path))\n-\t\treturn 0;\n+\t\treturn PATH_INVALID;\n\n \tif (!is_valid_path(path))\n-\t\treturn 0;\n+\t\treturn PATH_INVALID;\n\n \tgoto inside;\n \tfor (;;) {\n \t\tif (!c)\n-\t\t\treturn 1;\n+\t\t\treturn PATH_OK;\n \t\tif (is_dir_sep(c)) {\n inside:\n \t\t\tif (protect_hfs) {\n\n \t\t\t\tif (is_hfs_dotgit(path))\n-\t\t\t\t\treturn 0;\n+\t\t\t\t\treturn PATH_INVALID;\n \t\t\t\tif (S_ISLNK(mode)) {\n \t\t\t\t\tif (is_hfs_dotgitmodules(path))\n-\t\t\t\t\t\treturn 0;\n+\t\t\t\t\t\treturn PATH_INVALID;\n \t\t\t\t}\n \t\t\t}\n \t\t\tif (protect_ntfs) {\n #if defined GIT_WINDOWS_NATIVE || defined __CYGWIN__\n \t\t\t\tif (c == '\\\\')\n-\t\t\t\t\treturn 0;\n+\t\t\t\t\treturn PATH_INVALID;\n #endif\n \t\t\t\tif (is_ntfs_dotgit(path))\n-\t\t\t\t\treturn 0;\n+\t\t\t\t\treturn PATH_INVALID;\n \t\t\t\tif (S_ISLNK(mode)) {\n \t\t\t\t\tif (is_ntfs_dotgitmodules(path))\n-\t\t\t\t\t\treturn 0;\n+\t\t\t\t\t\treturn PATH_INVALID;\n \t\t\t\t}\n \t\t\t}\n\n \t\t\tc = *path++;\n \t\t\tif ((c == '.' && !verify_dotfile(path, mode)) ||\n \t\t\t    is_dir_sep(c))\n-\t\t\t\treturn 0;\n+\t\t\t\treturn PATH_INVALID;\n \t\t\t/*\n \t\t\t * allow terminating directory separators for\n \t\t\t * sparse directory entries.\n \t\t\t */\n \t\t\tif (c == '\\0')\n-\t\t\t\treturn S_ISDIR(mode);\n+\t\t\t\treturn S_ISDIR(mode) ? PATH_DIR_WITH_SEP :\n+\t\t\t\t\t\t       PATH_INVALID;\n \t\t} else if (c == '\\\\' && protect_ntfs) {\n \t\t\tif (is_ntfs_dotgit(path))\n-\t\t\t\treturn 0;\n+\t\t\t\treturn PATH_INVALID;\n \t\t\tif (S_ISLNK(mode)) {\n \t\t\t\tif (is_ntfs_dotgitmodules(path))\n-\t\t\t\t\treturn 0;\n+\t\t\t\t\treturn PATH_INVALID;\n \t\t\t}\n \t\t}\n\n@@ -1349,7 +1364,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n\n \tif (!ok_to_add)\n \t\treturn -1;\n-\tif (!verify_path(ce->name, ce->ce_mode))\n+\tif (verify_path_internal(ce->name, ce->ce_mode) == PATH_INVALID)\n \t\treturn error(_(\"invalid path '%s'\"), ce->name);\n\n \tif (!skip_df_check &&\ndiff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh\nindex dd2cdcc114..5390eec4e3 100755\n--- a/t/t3905-stash-include-untracked.sh\n+++ b/t/t3905-stash-include-untracked.sh\n@@ -422,4 +422,10 @@ test_expect_success 'stash show --{include,only}-untracked on stashes without un\n \ttest_must_be_empty actual\n '\n\n+test_expect_success 'stash -u ignores sub-repository' '\n+\ttest_when_finished \"rm -rf sub-repo\" &&\n+\tgit init sub-repo &&\n+\tgit stash -u\n+'\n+\n test_done\n--\n2.33.0\n"},{"id":"437941","messageId":"6df361a5-8e15-63a7-dbb8-77405c6edf0e@gmail.com","threadId":"56611","inReplyTo":"1d26a9f3-dcb5-408a-581e-40411e6a2179@web.de","subject":"Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-10-04T20:06:22Z","receivedAt":"2021-10-04T20:06:27Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/1/2021 10:25 AM, René Scharfe wrote:\n> Am 30.09.21 um 18:35 schrieb Junio C Hamano:\n>> René Scharfe <l.s.r@web.de> writes:\n>>\n>>> Looping in Stolee as the author of 6e773527b6 (sparse-index: convert\n>>> from full to sparse, 2021-03-30).\n\nThanks for looping me in. I'm still getting caught up from some\ntime off.\n\n>>> Here's a raw patch for that.  Versions before 6e773527b6 pass the\n>>> included test.\n>>>\n>>> The magic return value of 2 is a bit ugly, but allows adding the\n>>> additional check only to the call-site relevant to the bug report.\n>>>\n>>> I don't know if other callers of verify_path() might also need that\n>>> check, or if it is too narrow.\n>>\n>> The callers inside read-cache.c, which I think were the original\n>> intended audiences of the function, always call verify_path() with\n>> the pathname suitable to be stored as a cache entry.\n>>\n>> The callee never has to assume an extra trailing slash might exist\n>> at the end.  And verify_path() said \"no\", if any caller passed an\n>> extra trailing slash before the commit in question.\n>>\n>> I _think_ we defined the new \"tree\" type entries the sparse index\n>> stuff added to be the _only_ cache entries whose path always ends\n>> with a slash?  If so, perhaps we should audit the existing callers\n>> and move the \"paths that come from end-users to be entered to the\n>> index must never end with a slash\" sanity check this function gave\n>> us, which was broken by 6e773527b6, perhaps?\n>>\n>> \"git update-index\" should never allow to create a \"tree\" kind of\n>> cache entry (making it sparse again should be the task of systems\n>> internal, and should not be done by end-user feeding a pre-shrunk\n>> \"tree\" kind of entry to record a sparsely populated state, if I\n>> understand correctly), so its call to verify_path(), if it ends with\n>> a directory separator, should say \"that's not a good path\".\n>>\n>> Prehaps we can rename verify_path() to verify_path_internal() that\n>> is _known_ to be called with names that are already vetted to be\n>> stored in ce_name and convert callers inside read-cache.c to call\n>> it, and write verify_path() as a thin wrapper of it to fail when the\n>> former stops by returning S_ISDIR() at the place your fix touched,\n>> or something?\n> \n> Restoring the stricter check for the general case and providing a way\n> out for the code handling sparse indexes sounds like a good idea.\n> \n> I don't know which callers are supposed to need that, but the following\n> patch passes all tests, including the new one.\n\nI like this new approach.\n\n> +enum verify_path_result {\n> +\tPATH_OK,\n> +\tPATH_INVALID,\n> +\tPATH_DIR_WITH_SEP,\n> +};\n> +\n> +static enum verify_path_result verify_path_internal(const char *, unsigned);\n> +\n> +int verify_path(const char *path, unsigned mode)\n> +{\n> +\treturn verify_path_internal(path, mode) == PATH_OK;\n> +}\n> +\n\nI was confused at first as to why the verify_path() implementation is\nnot near the implementation of verify_path_internal(), but I see how\nyou use the _internal() version in make_cache_tree(), justifying this\nposition.\n\n>  struct cache_entry *make_cache_entry(struct index_state *istate,\n>  \t\t\t\t     unsigned int mode,\n>  \t\t\t\t     const struct object_id *oid,\n...\n>  \t\t\t/*\n>  \t\t\t * allow terminating directory separators for\n>  \t\t\t * sparse directory entries.\n>  \t\t\t */\n>  \t\t\tif (c == '\\0')\n> -\t\t\t\treturn S_ISDIR(mode);\n> +\t\t\t\treturn S_ISDIR(mode) ? PATH_DIR_WITH_SEP :\n> +\t\t\t\t\t\t       PATH_INVALID;\n\nThe rewrite of this method is mostly mechanical except for this one\nbit that is solving the issue at hand.\n\n> @@ -1349,7 +1364,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n> \n>  \tif (!ok_to_add)\n>  \t\treturn -1;\n> -\tif (!verify_path(ce->name, ce->ce_mode))\n> +\tif (verify_path_internal(ce->name, ce->ce_mode) == PATH_INVALID)\n>  \t\treturn error(_(\"invalid path '%s'\"), ce->name);\n\nAnd yes, I believe that make_cache_entry() and add_index_entry_with_check()\nare the only places that need this internal version. If we find others,\nthen we can add them as necessary. The tests in t1092 are getting rather\nrobust, although they don't do much for this test case:\n> +test_expect_success 'stash -u ignores sub-repository' '\n> +\ttest_when_finished \"rm -rf sub-repo\" &&\n> +\tgit init sub-repo &&\n> +\tgit stash -u\n> +'\n\nSeems like a good test to have, anyway.\n\nI look forward to seeing this as a full patch.\n\nThanks,\n-Stolee\n"},{"id":"437943","messageId":"xmqqzgrov7g2.fsf@gitster.g","threadId":"56611","inReplyTo":"6df361a5-8e15-63a7-dbb8-77405c6edf0e@gmail.com","subject":"Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-04T20:52:29Z","receivedAt":"2021-10-04T20:52:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> And yes, I believe that make_cache_entry() and add_index_entry_with_check()\n> are the only places that need this internal version. If we find others,\n> then we can add them as necessary. The tests in t1092 are getting rather\n> robust, although they don't do much for this test case:\n>> +test_expect_success 'stash -u ignores sub-repository' '\n>> +\ttest_when_finished \"rm -rf sub-repo\" &&\n>> +\tgit init sub-repo &&\n>> +\tgit stash -u\n>> +'\n>\n> Seems like a good test to have, anyway.\n>\n> I look forward to seeing this as a full patch.\n\nJust one thing I want to pick your brains for ;-)\n\nI said this earlier ...\n\n>>> \"git update-index\" should never allow to create a \"tree\" kind of\n>>> cache entry (making it sparse again should be the task of systems\n>>> internal, and should not be done by end-user feeding a pre-shrunk\n>>> \"tree\" kind of entry to record a sparsely populated state, if I\n>>> understand correctly), so its call to verify_path(), if it ends with\n>>> a directory separator, should say \"that's not a good path\".\n\n... without knowing what you had in mind when you did the \"tree kind\nof entry in the index\".  Are we on the same page, or do we think it\nmight be beneficial to give end-users a long-enough rope\nto hang themselves, aka get into the lower details of\nimplementation?\n\nOne _could_ imagine that allowing\n\n $ git update-index --cacheinfo 40000,609869396314577e5a,t/\n\ngiven by the end user to drop all entries under t/* and replace them\nwith a single sparse-dir-entry might be a good way to allow\nscripters the same power as the C-code to take advantage of the\nsparse checkout feature.  It needs to be paired with some mechanism\nto allow sparse-dir-entry observed by the end users with a plumbing,\ne.g. even though ls-files unconditionally calls ensure_full_index(),\n\n $ git ls-files --show-sparse\n\nmay show the sparse-dir-entry by bypassing the call.\n\nThanks.\n"},{"id":"437945","messageId":"a075091c-d0d4-db5d-fa21-c9d6c90c343e@gmail.com","threadId":"56611","inReplyTo":"xmqqzgrov7g2.fsf@gitster.g","subject":"Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-10-04T22:29:52Z","receivedAt":"2021-10-04T22:29:57Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/4/2021 4:52 PM, Junio C Hamano wrote:\n> Derrick Stolee <stolee@gmail.com> writes:\n> \n>> And yes, I believe that make_cache_entry() and add_index_entry_with_check()\n>> are the only places that need this internal version. If we find others,\n>> then we can add them as necessary. The tests in t1092 are getting rather\n>> robust, although they don't do much for this test case:\n>>> +test_expect_success 'stash -u ignores sub-repository' '\n>>> +\ttest_when_finished \"rm -rf sub-repo\" &&\n>>> +\tgit init sub-repo &&\n>>> +\tgit stash -u\n>>> +'\n>>\n>> Seems like a good test to have, anyway.\n>>\n>> I look forward to seeing this as a full patch.\n> \n> Just one thing I want to pick your brains for ;-)\n> \n> I said this earlier ...\n> \n>>>> \"git update-index\" should never allow to create a \"tree\" kind of\n>>>> cache entry (making it sparse again should be the task of systems\n>>>> internal, and should not be done by end-user feeding a pre-shrunk\n>>>> \"tree\" kind of entry to record a sparsely populated state, if I\n>>>> understand correctly), so its call to verify_path(), if it ends with\n>>>> a directory separator, should say \"that's not a good path\".\n> \n> ... without knowing what you had in mind when you did the \"tree kind\n> of entry in the index\".  Are we on the same page, or do we think it\n> might be beneficial to give end-users a long-enough rope\n> to hang themselves, aka get into the lower details of\n> implementation?\n> \n> One _could_ imagine that allowing\n> \n>  $ git update-index --cacheinfo 40000,609869396314577e5a,t/\n> \n> given by the end user to drop all entries under t/* and replace them\n> with a single sparse-dir-entry might be a good way to allow\n> scripters the same power as the C-code to take advantage of the\n> sparse checkout feature.  It needs to be paired with some mechanism\n> to allow sparse-dir-entry observed by the end users with a plumbing,\n> e.g. even though ls-files unconditionally calls ensure_full_index(),\n\nI think this is an interesting capability, but I'm not sure I see\na use case that is worth the footgun, especially with the accidents\nthat can happen when working with submodules.\n\nI think we can happily extend to include this functionality in the\nfuture. Having an error condition now that we relax in the future is\nthe good kind of behavior change.\n\n>  $ git ls-files --show-sparse\n> \n> may show the sparse-dir-entry by bypassing the call.\n\nI have something in the works for this, but I'm letting others send\ntheir sparse-index work first. I have not forgotten your request for\nsuch a feature!\n\nThanks,\n-Stolee\n"},{"id":"438220","messageId":"079598e4-47fa-671e-0d1d-cf82039d861f@web.de","threadId":"56611","inReplyTo":"CACr9BXmP1vQMK4b27Uc4R-3WWYHUYfCEEMN+hnth4yUg+UN7Zg@mail.gmail.com","subject":"[PATCH 1/3] t3905: show failure to ignore sub-repo","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-07T20:31:01Z","receivedAt":"2021-10-07T20:31:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"\"git stash\" used to ignore sub-repositories until 6e773527b6\n(sparse-index: convert from full to sparse, 2021-03-30).  Add a test\nthat demonstrates this regression.\n\nReported-by: Robert Leftwich <robert@gitpod.io>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/t3905-stash-include-untracked.sh | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh\nindex dd2cdcc114..19da46b7fb 100755\n--- a/t/t3905-stash-include-untracked.sh\n+++ b/t/t3905-stash-include-untracked.sh\n@@ -422,4 +422,10 @@ test_expect_success 'stash show --{include,only}-untracked on stashes without un\n \ttest_must_be_empty actual\n '\n\n+test_expect_failure 'stash -u ignores sub-repository' '\n+\ttest_when_finished \"rm -rf sub-repo\" &&\n+\tgit init sub-repo &&\n+\tgit stash -u\n+'\n+\n test_done\n--\n2.33.0\n"},{"id":"438221","messageId":"a3629513-3851-8474-aa07-d7fafc85c21d@web.de","threadId":"56611","inReplyTo":"CACr9BXmP1vQMK4b27Uc4R-3WWYHUYfCEEMN+hnth4yUg+UN7Zg@mail.gmail.com","subject":"[PATCH 2/3] read-cache: add verify_path_internal()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-07T20:31:44Z","receivedAt":"2021-10-07T20:31:54Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Turn verify_path() into an internal function that distinguishes between\nvalid paths and those with trailing directory separators and rename it\nto verify_path_internal().  Provide a wrapper with the old behavior\nunder the old name.  No functional change intended.  The new function\nwill be used in the next patch.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n read-cache.c | 41 ++++++++++++++++++++++++++++-------------\n 1 file changed, 28 insertions(+), 13 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex b0a06db5c5..5b6fc08e46 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -849,6 +849,19 @@ struct cache_entry *make_empty_transient_cache_entry(size_t len,\n \treturn xcalloc(1, cache_entry_size(len));\n }\n\n+enum verify_path_result {\n+\tPATH_OK,\n+\tPATH_INVALID,\n+\tPATH_DIR_WITH_SEP,\n+};\n+\n+static enum verify_path_result verify_path_internal(const char *, unsigned);\n+\n+int verify_path(const char *path, unsigned mode)\n+{\n+\treturn verify_path_internal(path, mode) != PATH_INVALID;\n+}\n+\n struct cache_entry *make_cache_entry(struct index_state *istate,\n \t\t\t\t     unsigned int mode,\n \t\t\t\t     const struct object_id *oid,\n@@ -993,60 +1006,62 @@ static int verify_dotfile(const char *rest, unsigned mode)\n \treturn 1;\n }\n\n-int verify_path(const char *path, unsigned mode)\n+static enum verify_path_result verify_path_internal(const char *path,\n+\t\t\t\t\t\t    unsigned mode)\n {\n \tchar c = 0;\n\n \tif (has_dos_drive_prefix(path))\n-\t\treturn 0;\n+\t\treturn PATH_INVALID;\n\n \tif (!is_valid_path(path))\n-\t\treturn 0;\n+\t\treturn PATH_INVALID;\n\n \tgoto inside;\n \tfor (;;) {\n \t\tif (!c)\n-\t\t\treturn 1;\n+\t\t\treturn PATH_OK;\n \t\tif (is_dir_sep(c)) {\n inside:\n \t\t\tif (protect_hfs) {\n\n \t\t\t\tif (is_hfs_dotgit(path))\n-\t\t\t\t\treturn 0;\n+\t\t\t\t\treturn PATH_INVALID;\n \t\t\t\tif (S_ISLNK(mode)) {\n \t\t\t\t\tif (is_hfs_dotgitmodules(path))\n-\t\t\t\t\t\treturn 0;\n+\t\t\t\t\t\treturn PATH_INVALID;\n \t\t\t\t}\n \t\t\t}\n \t\t\tif (protect_ntfs) {\n #if defined GIT_WINDOWS_NATIVE || defined __CYGWIN__\n \t\t\t\tif (c == '\\\\')\n-\t\t\t\t\treturn 0;\n+\t\t\t\t\treturn PATH_INVALID;\n #endif\n \t\t\t\tif (is_ntfs_dotgit(path))\n-\t\t\t\t\treturn 0;\n+\t\t\t\t\treturn PATH_INVALID;\n \t\t\t\tif (S_ISLNK(mode)) {\n \t\t\t\t\tif (is_ntfs_dotgitmodules(path))\n-\t\t\t\t\t\treturn 0;\n+\t\t\t\t\t\treturn PATH_INVALID;\n \t\t\t\t}\n \t\t\t}\n\n \t\t\tc = *path++;\n \t\t\tif ((c == '.' && !verify_dotfile(path, mode)) ||\n \t\t\t    is_dir_sep(c))\n-\t\t\t\treturn 0;\n+\t\t\t\treturn PATH_INVALID;\n \t\t\t/*\n \t\t\t * allow terminating directory separators for\n \t\t\t * sparse directory entries.\n \t\t\t */\n \t\t\tif (c == '\\0')\n-\t\t\t\treturn S_ISDIR(mode);\n+\t\t\t\treturn S_ISDIR(mode) ? PATH_DIR_WITH_SEP :\n+\t\t\t\t\t\t       PATH_INVALID;\n \t\t} else if (c == '\\\\' && protect_ntfs) {\n \t\t\tif (is_ntfs_dotgit(path))\n-\t\t\t\treturn 0;\n+\t\t\t\treturn PATH_INVALID;\n \t\t\tif (S_ISLNK(mode)) {\n \t\t\t\tif (is_ntfs_dotgitmodules(path))\n-\t\t\t\t\treturn 0;\n+\t\t\t\t\treturn PATH_INVALID;\n \t\t\t}\n \t\t}\n\n--\n2.33.0\n"},{"id":"438225","messageId":"5e6fafa9-f155-61af-6906-05fec27808e6@web.de","threadId":"56611","inReplyTo":"CACr9BXmP1vQMK4b27Uc4R-3WWYHUYfCEEMN+hnth4yUg+UN7Zg@mail.gmail.com","subject":"[PATCH 3/3] read-cache: let verify_path() reject trailing dir separators again","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-07T20:31:58Z","receivedAt":"2021-10-07T20:32:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"6e773527b6 (sparse-index: convert from full to sparse, 2021-03-30) made\nverify_path() accept trailing directory separators for directories,\nwhich is necessary for sparse directory entries.  This clemency causes\n\"git stash\" to stumble over sub-repositories, though, and there may be\nmore unintended side-effects.\n\nAvoid them by restoring the old verify_path() behavior and accepting\ntrailing directory separators only in places that are supposed to handle\nsparse directory entries.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n read-cache.c                       | 6 +++---\n t/t3905-stash-include-untracked.sh | 2 +-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 5b6fc08e46..f1aabc47b6 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -859,7 +859,7 @@ static enum verify_path_result verify_path_internal(const char *, unsigned);\n\n int verify_path(const char *path, unsigned mode)\n {\n-\treturn verify_path_internal(path, mode) != PATH_INVALID;\n+\treturn verify_path_internal(path, mode) == PATH_OK;\n }\n\n struct cache_entry *make_cache_entry(struct index_state *istate,\n@@ -872,7 +872,7 @@ struct cache_entry *make_cache_entry(struct index_state *istate,\n \tstruct cache_entry *ce, *ret;\n \tint len;\n\n-\tif (!verify_path(path, mode)) {\n+\tif (verify_path_internal(path, mode) == PATH_INVALID) {\n \t\terror(_(\"invalid path '%s'\"), path);\n \t\treturn NULL;\n \t}\n@@ -1364,7 +1364,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e\n\n \tif (!ok_to_add)\n \t\treturn -1;\n-\tif (!verify_path(ce->name, ce->ce_mode))\n+\tif (verify_path_internal(ce->name, ce->ce_mode) == PATH_INVALID)\n \t\treturn error(_(\"invalid path '%s'\"), ce->name);\n\n \tif (!skip_df_check &&\ndiff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh\nindex 19da46b7fb..5390eec4e3 100755\n--- a/t/t3905-stash-include-untracked.sh\n+++ b/t/t3905-stash-include-untracked.sh\n@@ -422,7 +422,7 @@ test_expect_success 'stash show --{include,only}-untracked on stashes without un\n \ttest_must_be_empty actual\n '\n\n-test_expect_failure 'stash -u ignores sub-repository' '\n+test_expect_success 'stash -u ignores sub-repository' '\n \ttest_when_finished \"rm -rf sub-repo\" &&\n \tgit init sub-repo &&\n \tgit stash -u\n--\n2.33.0\n"},{"id":"438282","messageId":"xmqqwnmnlwfl.fsf@gitster.g","threadId":"56611","inReplyTo":"5e6fafa9-f155-61af-6906-05fec27808e6@web.de","subject":"Re: [PATCH 3/3] read-cache: let verify_path() reject trailing dir separators again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-08T09:04:14Z","receivedAt":"2021-10-08T09:04:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> 6e773527b6 (sparse-index: convert from full to sparse, 2021-03-30) made\n> verify_path() accept trailing directory separators for directories,\n> which is necessary for sparse directory entries.  This clemency causes\n> \"git stash\" to stumble over sub-repositories, though, and there may be\n> more unintended side-effects.\n>\n> Avoid them by restoring the old verify_path() behavior and accepting\n> trailing directory separators only in places that are supposed to handle\n> sparse directory entries.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  read-cache.c                       | 6 +++---\n>  t/t3905-stash-include-untracked.sh | 2 +-\n>  2 files changed, 4 insertions(+), 4 deletions(-)\n\nMakes sense.  Thanks.\n"}]}