git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "René Scharfe" <l.s.r@web.de>
To: Robert Leftwich <robert@gitpod.io>,
	git@vger.kernel.org, Derrick Stolee <dstolee@microsoft.com>
Subject: Re: Bug/regression report - 'git stash push -u' fatal errors when sub-repo present
Date: Thu, 30 Sep 2021 12:06:37 +0200	[thread overview]
Message-ID: <7b83c77e-dd87-f688-3da1-7826cf6b0d4e@web.de> (raw)
In-Reply-To: <CACr9BXmP1vQMK4b27Uc4R-3WWYHUYfCEEMN+hnth4yUg+UN7Zg@mail.gmail.com>

Am 30.09.21 um 02:49 schrieb Robert Leftwich:
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
>   Initialised a repo and added a file or cloned a repo inside an existing
> repo (think dependencies).
>   See https://github.com/gitpod-io/gitpod/issues/5675 for background.
>
>   In an existing repo:
>     $ mkdir sub_test && cd sub_test/ && git init . && touch test.txt && git
> add test.txt
>     OR
>     $ git clone https://github.com/stencila/test.git sub_test
>     THEN
>     $ git stash push -u
>
> What did you expect to happen? (Expected behavior)
>   Command should complete without error but ignore the directory (this is
> the existing behavior prior to v2.31)
>     $ git stash push -u
>     Ignoring path sub_test
>     Saved working directory and index state WIP on (no branch): 94f6e3e283
> Git 2.30.2
>
> What happened instead? (Actual behavior)
>   Command failed
>     $ git stash push -u
>     error: sub_test/: is a directory - add files inside instead
>     fatal: Unable to process path sub_test/
>     Cannot save the untracked files
>
> What's different between what you expected and what actually happened?
>   Command failed
>
> Anything else you want to add:
>   It happens on all versions from v2.31 to current master.
>   It is specifically related to this change:
>
> https://github.com/git/git/commit/6e773527b6b03976cefbb0f9571bd40dd5995e6c#diff-70525b6b89c7cac91e520085d954a68671038d218b77d22855e938ab075a68d8L1006
>
>   If this is the new expected behavior perhaps it can result in a better
> error message and related documentation?
>
> Please review the rest of the bug report below.
> You can delete any lines you don't wish to share.

Looping in Stolee as the author of 6e773527b6 (sparse-index: convert
from full to sparse, 2021-03-30).

>
>
> [System Info]
> git version:
> git version 2.33.0.610.gcefe983a32
> cpu: x86_64
> built from commit: cefe983a320c03d7843ac78e73bd513a27806845
> sizeof-long: 8
> sizeof-size_t: 8
> shell-path: /bin/sh
> uname: Linux 5.4.0-1051-gke #54-Ubuntu SMP Thu Aug 5 18:52:13 UTC 2021
> x86_64
> compiler info: gnuc: 9.3
> libc info: glibc: 2.31
> $SHELL (typically, interactive shell): /bin/bash
>
>
> [Enabled Hooks]
>

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.

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

  reply	other threads:[~2021-09-30 10:06 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-30  0:49 Robert Leftwich
2021-09-30 10:06 ` René Scharfe [this message]
2021-09-30 16:35   ` Junio C Hamano
2021-10-01 14:25     ` René Scharfe
2021-10-04 20:06       ` Derrick Stolee
2021-10-04 20:52         ` Junio C Hamano
2021-10-04 22:29           ` Derrick Stolee
2021-10-07 20:31 ` [PATCH 1/3] t3905: show failure to ignore sub-repo René Scharfe
2021-10-07 20:31 ` [PATCH 2/3] read-cache: add verify_path_internal() René Scharfe
2021-10-07 20:31 ` [PATCH 3/3] read-cache: let verify_path() reject trailing dir separators again René Scharfe
2021-10-08  9:04   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7b83c77e-dd87-f688-3da1-7826cf6b0d4e@web.de \
    --to=l.s.r@web.de \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=robert@gitpod.io \
    --subject='Re: Bug/regression report - '\''git stash push -u'\'' fatal errors when sub-repo present' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).