git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Derrick Stolee <derrickstolee@github.com>
To: Goss Geppert <gg.oss.dev@gmail.com>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>,
	christian w <usebees@gmail.com>, Elijah Newren <newren@gmail.com>
Subject: Re: [PATCH v2 1/2] dir: consider worktree config in path recursion
Date: Mon, 23 May 2022 15:23:29 -0400	[thread overview]
Message-ID: <900d91cb-1982-b892-17e3-ad678e711dc9@github.com> (raw)
In-Reply-To: <20220510171527.25778-2-ggossdev@gmail.com>

On 5/10/22 1:15 PM, Goss Geppert wrote:
> Since 8d92fb2927 (dir: replace exponential algorithm with a linear one,
> 2020-04-01) the following no longer works:
> 
>     1) Initialize a repository.
>     2) Set the `core.worktree` location to a parent directory of the
>        default worktree.
>     3) Add a file located in the default worktree location to the index
>        (i.e. anywhere in the immediate parent directory of the gitdir).
> 
> This commit adds a check to determine whether a nested repository that
> is encountered in recursing a path is actually `the_repository`.  If so,
> simply treat the directory as if it doesn't contain a nested repository.
> 
> Prior to this commit, the `add` operation was silently ignored.
> 
> Signed-off-by: Goss Geppert <ggossdev@gmail.com>
> ---
>  dir.c                          | 22 +++++++++
>  t/t2205-add-worktree-config.sh | 89 ++++++++++++++++++++++++++++++++++
>  2 files changed, 111 insertions(+)
>  create mode 100755 t/t2205-add-worktree-config.sh
> 
> diff --git a/dir.c b/dir.c
> index f2b0f24210..a1886e61a3 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1893,9 +1893,31 @@ static enum path_treatment treat_directory(struct dir_struct *dir,
>  
>  	if ((dir->flags & DIR_SKIP_NESTED_GIT) ||
>  		!(dir->flags & DIR_NO_GITLINKS)) {
> +		/*
> +		 * Determine if `dirname` is a nested repo by confirming that:
> +		 * 1) we are in a nonbare repository, and
> +		 * 2) `dirname` is not an immediate parent of `the_repository->gitdir`,
> +		 *    which could occur if the `worktree` location was manually
> +		 *    configured by the user; see t2205 testcases 1a-1d for examples
> +		 *    where this matters
> +		 */
>  		struct strbuf sb = STRBUF_INIT;
>  		strbuf_addstr(&sb, dirname);
>  		nested_repo = is_nonbare_repository_dir(&sb);
> +
> +		if (nested_repo) {
> +			char *real_dirname, *real_gitdir;
> +			strbuf_reset(&sb);
> +			strbuf_addstr(&sb, dirname);
> +			strbuf_complete(&sb, '/');
> +			strbuf_addstr(&sb, ".git");

This could be strbuf_add(&sb, ".git", 4); to avoid a (minor)
strlen() computation.

> +			real_dirname = real_pathdup(sb.buf, 0);
> +			real_gitdir = real_pathdup(the_repository->gitdir, 0);

With regards to Junio's comment about repeating real_pathdup()
on the_repository->gitdir, we might be able to short-circuit
this by making real_gitdir static and only calling
real_pathdup() if real_gitdir is NULL. It would cut the cost
of these checks by half for big traversals.

> +
> +			nested_repo = !!strcmp(real_dirname, real_gitdir);
> +			free(real_gitdir);
> +			free(real_dirname);
> +		}
...
> +
> +test_description='directory traversal respects worktree config
> +
> +This test configures the repository`s worktree to be two levels above the
> +`.git` directory and checks whether we are able to add to the index those files
> +that are in either (1) the manually configured worktree directory or (2) the
> +standard worktree location with respect to the `.git` directory (i.e. ensuring
> +that the encountered `.git` directory is not treated as belonging to a foreign
> +nested repository)'

The test description should be a single line. The longer paragraph
could be a comment before your "setup" test case.

> +. ./test-lib.sh
> +
> +test_expect_success '1a: setup' '

Also, there is no need to number your tests in their names, as the
testing infrastructure will apply numbers based on their order in
the test file. These labels will grow stale if tests are removed
or inserted into the order.

> +	test_create_repo test1 &&
> +	git --git-dir="test1/.git" config core.worktree "$(pwd)" &&
> +
> +	mkdir -p outside-tracked outside-untracked &&
> +	mkdir -p test1/inside-tracked test1/inside-untracked &&
> +	>file-tracked &&
> +	>file-untracked &&
> +	>outside-tracked/file &&
> +	>outside-untracked/file &&
> +	>test1/file-tracked &&
> +	>test1/file-untracked &&
> +	>test1/inside-tracked/file &&
> +	>test1/inside-untracked/file &&
> +
> +	cat >expect-tracked-unsorted <<-EOF &&
> +	../file-tracked
> +	../outside-tracked/file
> +	file-tracked
> +	inside-tracked/file
> +	EOF
> +
> +	cat >expect-untracked-unsorted <<-EOF &&
> +	../file-untracked
> +	../outside-untracked/file
> +	file-untracked
> +	inside-untracked/file
> +	EOF
> +
> +	cat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&
> +
> +	cat >.gitignore <<-EOF
> +	.gitignore
> +	actual-*
> +	expect-*
> +	EOF

This use of .gitignore to ignore the helper files being used
during the test is interesting. I was thinking it would be better
to create isolated directories where the only activity was that
which you were testing:

	mkdir -p worktree &&
	test_create_repo worktree/contains-git &&

...or something like that. The names are more descriptive, and
we don't need the .gitignore trick.

> +'
> +
> +test_expect_success '1b: pre-add all' '
> +	local parent_dir="$(pwd)" &&

I was surprised by this "local". It isn't needed at this
point in the test script. We use it for helper methods to
be sure that we aren't stomping variables from test scripts,
but since the test is being executed inside an eval(), this
local isn't important.

> +	(
> +		cd test1 &&
> +		git ls-files -o --exclude-standard "$parent_dir" >../actual-all-unsorted

You can avoid the sub-shell using "git -C test1 ls-files ..."
right?

> +	) &&
> +	sort actual-all-unsorted >actual-all &&
> +	sort expect-all-unsorted >expect-all &&
> +	test_cmp expect-all actual-all
> +'
> +
> +test_expect_success '1c: post-add tracked' '
> +	local parent_dir="$(pwd)" &&
> +	(
> +		cd test1 &&
> +		git add file-tracked &&
> +		git add inside-tracked &&
> +		git add ../outside-tracked &&
> +		git add "$parent_dir/file-tracked" &&
> +		git ls-files "$parent_dir" >../actual-tracked-unsorted
> +	) &&

This sub-shell is important because of the relative paths
involved. OK.

This also checks to see if _any_ of these "git add"
commands fail, as opposed to failing immediately after
the first one fails. I think your approach is simpler and
should be relatively simple to identify which command did
the wrong thing by looking at the test_cmp output.

> +	sort actual-tracked-unsorted >actual-tracked &&
> +	sort expect-tracked-unsorted >expect-tracked &&
> +	test_cmp expect-tracked actual-tracked
> +'
> +
> +test_expect_success '1d: post-add untracked' '
> +	local parent_dir="$(pwd)" &&
> +	(
> +		cd test1 &&
> +		git ls-files -o --exclude-standard "$parent_dir" >../actual-untracked-unsorted
> +	) &&

Again, this one is not needed.

> +	sort actual-untracked-unsorted >actual-untracked &&
> +	sort expect-untracked-unsorted >expect-untracked &&
> +	test_cmp expect-untracked actual-untracked
> +'
> +

Thanks,
-Stolee

  parent reply	other threads:[~2022-05-23 19:36 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-05 20:32 [RFC PATCH 0/1] " Goss Geppert
2022-05-05 20:32 ` [RFC PATCH 1/1] " Goss Geppert
2022-05-07  3:26   ` Elijah Newren
2022-05-07 17:59     ` oss dev
2022-05-06 17:02 ` [RFC PATCH 0/1] " Junio C Hamano
2022-05-06 20:00   ` oss dev
2022-05-10 17:15 ` [PATCH v2 0/2] " Goss Geppert
2022-05-10 17:15   ` [PATCH v2 1/2] " Goss Geppert
2022-05-11 16:37     ` Junio C Hamano
2022-05-20 19:45       ` oss dev
2022-05-24 14:29       ` Elijah Newren
2022-05-24 19:45         ` Junio C Hamano
2022-05-25  3:46           ` Elijah Newren
2022-05-11 23:07     ` Junio C Hamano
2022-05-20 20:01       ` oss dev
2022-05-23 19:23     ` Derrick Stolee [this message]
2022-05-30 18:48       ` oss dev
2022-05-10 17:15   ` [PATCH v2 2/2] dir: minor refactoring / clean-up Goss Geppert
2022-05-11 16:51     ` Junio C Hamano
2022-05-20 20:03       ` oss dev
2022-05-20 19:28 ` [PATCH v3 0/3] dir: traverse into repository Goss Geppert
2022-05-20 19:28   ` [PATCH v3 1/3] " Goss Geppert
2022-05-20 19:28   ` [PATCH v3 2/3] dir: cache git_dir's realpath Goss Geppert
2022-05-24 14:32     ` Elijah Newren
2022-05-20 19:28   ` [PATCH v3 3/3] dir: minor refactoring / clean-up Goss Geppert
2022-06-16 23:19 ` [PATCH v4 0/2] dir: traverse into repository Goss Geppert
2022-06-22  4:57   ` Elijah Newren
     [not found] ` <20220616231956.154-1-gg.oss@outlook.com>
2022-06-16 23:19   ` [PATCH v4 1/2] " Goss Geppert
2022-06-16 23:44 ` [PATCH v4 0/2] dir: traverse into repository (resending) Goss Geppert
     [not found] ` <20220616234433.225-1-gg.oss@outlook.com>
2022-06-16 23:44   ` [PATCH v4 1/2] dir: traverse into repository Goss Geppert
2022-06-16 23:44   ` [PATCH v4 2/2] dir: minor refactoring / clean-up Goss Geppert

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=900d91cb-1982-b892-17e3-ad678e711dc9@github.com \
    --to=derrickstolee@github.com \
    --cc=gg.oss.dev@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=newren@gmail.com \
    --cc=usebees@gmail.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).