git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "René Scharfe" <l.s.r@web.de>
Cc: 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 09:35:36 -0700	[thread overview]
Message-ID: <xmqqk0iydns7.fsf@gitster.g> (raw)
In-Reply-To: <7b83c77e-dd87-f688-3da1-7826cf6b0d4e@web.de> (=?utf-8?Q?=22R?= =?utf-8?Q?en=C3=A9?= Scharfe"'s message of "Thu, 30 Sep 2021 12:06:37 +0200")

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?

Thanks.

>
> 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 16:35 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
2021-09-30 16:35   ` Junio C Hamano [this message]
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=xmqqk0iydns7.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=l.s.r@web.de \
    --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).