Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present
flat view
From: Junio C Hamano <hidden>
Date: 2021-09-30 16:35:41
Possibly related (same subject, not in this thread)
- 2021-10-04 · Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present · Derrick Stolee <hidden>
- 2021-10-04 · Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present · Junio C Hamano <hidden>
- 2021-09-30 · Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present · René Scharfe <hidden>
- 2021-09-30 · Bug/regression report - 'git stash push -u' fatal errors when sub-repo present · Robert Leftwich <hidden>
René Scharfe [off-list ref] writes:
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.
quoted hunk
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