Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present
Am 30.09.21 um 18:35 schrieb Junio C Hamano:
Show 42 quoted lines
> René Scharfe <l.s.r@web.de> 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?
Restoring the stricter check for the general case and providing a way out for the code handling sparse indexes sounds like a good idea.
I don't know which callers are supposed to need that, but the following patch passes all tests, including the new one.
René
---
read-cache.c | 45 ++++++++++++++++++++----------
t/t3905-stash-include-untracked.sh | 6 ++++
2 files changed, 36 insertions(+), 15 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index f5d4385c40..a1da4619c4 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -849,6 +849,19 @@ struct cache_entry *make_empty_transient_cache_entry(size_t len,
return xcalloc(1, cache_entry_size(len));
}
+enum verify_path_result {
+ PATH_OK,
+ PATH_INVALID,
+ PATH_DIR_WITH_SEP,
+};
+
+static enum verify_path_result verify_path_internal(const char *, unsigned);
+
+int verify_path(const char *path, unsigned mode)
+{
+ return verify_path_internal(path, mode) == PATH_OK;
+}
+
struct cache_entry *make_cache_entry(struct index_state *istate,
unsigned int mode,
const struct object_id *oid,
@@ -859,7 +872,7 @@ struct cache_entry *make_cache_entry(struct index_state *istate,
struct cache_entry *ce, *ret;
int len;
- if (!verify_path(path, mode)) {
+ if (verify_path_internal(path, mode) == PATH_INVALID) {
error(_("invalid path '%s'"), path);
return NULL;
}
@@ -993,60 +1006,62 @@ static int verify_dotfile(const char *rest, unsigned mode)
return 1;
}
-int verify_path(const char *path, unsigned mode)
+static enum verify_path_result verify_path_internal(const char *path,
+ unsigned mode)
{
char c = 0;
if (has_dos_drive_prefix(path))
- return 0;
+ return PATH_INVALID;
if (!is_valid_path(path))
- return 0;
+ return PATH_INVALID;
goto inside;
for (;;) {
if (!c)
- return 1;
+ return PATH_OK;
if (is_dir_sep(c)) {
inside:
if (protect_hfs) {
if (is_hfs_dotgit(path))
- return 0;
+ return PATH_INVALID;
if (S_ISLNK(mode)) {
if (is_hfs_dotgitmodules(path))
- return 0;
+ return PATH_INVALID;
}
}
if (protect_ntfs) {
#if defined GIT_WINDOWS_NATIVE || defined __CYGWIN__
if (c == '\\')
- return 0;
+ return PATH_INVALID;
#endif
if (is_ntfs_dotgit(path))
- return 0;
+ return PATH_INVALID;
if (S_ISLNK(mode)) {
if (is_ntfs_dotgitmodules(path))
- return 0;
+ return PATH_INVALID;
}
}
c = *path++;
if ((c == '.' && !verify_dotfile(path, mode)) ||
is_dir_sep(c))
- return 0;
+ return PATH_INVALID;
/*
* allow terminating directory separators for
* sparse directory entries.
*/
if (c == '\0')
- return S_ISDIR(mode);
+ return S_ISDIR(mode) ? PATH_DIR_WITH_SEP :
+ PATH_INVALID;
} else if (c == '\\' && protect_ntfs) {
if (is_ntfs_dotgit(path))
- return 0;
+ return PATH_INVALID;
if (S_ISLNK(mode)) {
if (is_ntfs_dotgitmodules(path))
- return 0;
+ return PATH_INVALID;
}
}
@@ -1349,7 +1364,7 @@ static int add_index_entry_with_check(struct index_state *istate, struct cache_e
if (!ok_to_add)
return -1;
- if (!verify_path(ce->name, ce->ce_mode))
+ if (verify_path_internal(ce->name, ce->ce_mode) == PATH_INVALID)
return error(_("invalid path '%s'"), ce->name);
if (!skip_df_check &&
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
