Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present

["René Scharfe" <l.s.r@web.de>]

Am 30.09.21 um 18:35 schrieb Junio C Hamano:
> 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