Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 30, 2021, 16:35 UTC
- Message-ID
- <xmqqk0iydns7.fsf@gitster.g>
- In-Reply-To
- <7b83c77e-dd87-f688-3da1-7826cf6b0d4e@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 11 quoted lines
> Looping in Stolee as the author of 6e773527b6 (sparse-index: convert > from full to sparse, 2021-03-30). > > Here's a raw patch for that. Versions before 6e773527b6 pass the > included test. > > The magic return value of 2 is a bit ugly, but allows adding the > additional check only to the call-site relevant to the bug report. > > I don't know if other callers of verify_path() might also need that > check, or if it is too narrow.
The callers inside read-cache.c, which I think were the original intended audiences of the function, always call verify_path() with the pathname suitable to be stored as a cache entry.
The callee never has to assume an extra trailing slash might exist at the end. And verify_path() said "no", if any caller passed an extra trailing slash before the commit in question.
I _think_ we defined the new "tree" type entries the sparse index stuff added to be the _only_ cache entries whose path always ends with a slash? If so, perhaps we should audit the existing callers and move the "paths that come from end-users to be entered to the index must never end with a slash" sanity check this function gave us, which was broken by 6e773527b6, perhaps?
"git update-index" should never allow to create a "tree" kind of cache entry (making it sparse again should be the task of systems internal, and should not be done by end-user feeding a pre-shrunk "tree" kind of entry to record a sparsely populated state, if I understand correctly), so its call to verify_path(), if it ends with a directory separator, should say "that's not a good path".
Prehaps we can rename verify_path() to verify_path_internal() that is _known_ to be called with names that are already vetted to be stored in ce_name and convert callers inside read-cache.c to call it, and write verify_path() as a thin wrapper of it to fail when the former stops by returning S_ISDIR() at the place your fix touched, or something?
Thanks.
Show 77 quoted lines
>
> René
>
>
> ---
> builtin/update-index.c | 15 ++++++++++++++-
> read-cache.c | 2 +-
> t/t3905-stash-include-untracked.sh | 6 ++++++
> 3 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 187203e8bb..3d32db7304 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -445,10 +445,22 @@ static void chmod_path(char flip, const char *path)
> die("git update-index: cannot chmod %cx '%s'", flip, path);
> }
>
> +static int is_nonbare_repo_dir(const char *path)
> +{
> + int ret;
> + struct strbuf sb = STRBUF_INIT;
> +
> + strbuf_addstr(&sb, path);
> + ret = is_nonbare_repository_dir(&sb);
> + strbuf_release(&sb);
> + return ret;
> +}
> +
> static void update_one(const char *path)
> {
> int stat_errno = 0;
> struct stat st;
> + int ret;
>
> if (mark_valid_only || mark_skip_worktree_only || force_remove ||
> mark_fsmonitor_only)
> @@ -458,7 +470,8 @@ static void update_one(const char *path)
> stat_errno = errno;
> } /* else stat is valid */
>
> - if (!verify_path(path, st.st_mode)) {
> + ret = verify_path(path, st.st_mode);
> + if (ret == 0 || (ret == 2 && is_nonbare_repo_dir(path))) {
> fprintf(stderr, "Ignoring path %s\n", path);
> return;
> }
> diff --git a/read-cache.c b/read-cache.c
> index f5d4385c40..0188b5b798 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1040,7 +1040,7 @@ int verify_path(const char *path, unsigned mode)
> * sparse directory entries.
> */
> if (c == '\0')
> - return S_ISDIR(mode);
> + return S_ISDIR(mode) ? 2 : 0;
> } else if (c == '\\' && protect_ntfs) {
> if (is_ntfs_dotgit(path))
> return 0;
> diff --git a/t/t3905-stash-include-untracked.sh b/t/t3905-stash-include-untracked.sh
> index dd2cdcc114..5390eec4e3 100755
> --- a/t/t3905-stash-include-untracked.sh
> +++ b/t/t3905-stash-include-untracked.sh
> @@ -422,4 +422,10 @@ test_expect_success 'stash show --{include,only}-untracked on stashes without un
> test_must_be_empty actual
> '
>
> +test_expect_success 'stash -u ignores sub-repository' '
> + test_when_finished "rm -rf sub-repo" &&
> + git init sub-repo &&
> + git stash -u
> +'
> +
> test_done
> --
> 2.33.0