[PATCH v3 0/3] dir: traverse into repository

[Goss Geppert <gg.oss.dev@gmail.com>]

Thanks for your feedback so far. This revision includes a number of changes:

* I realized that using option `--git-dir` (or environment variable `GIT_DIR`)
  when the working directory is outside the repository can also cause the
  directory traversal to fail to enter `the_repository`.  I've added test cases
  and updated comments / messages to reflect this.
* I noticed that `real_pathdup` can fail even for valid paths (and return NULL)
  if e.g. the path contains too many nested symlinks (currently 32).  We'll now
  `die_on_error`.
* I no longer reset the string buffer holding the `dirname` and just reuse it.
  While the prior call to `is_nonbare_repository_dir` does not treat its
  argument as const, the function does reset the argument to its effective
  input state.  The `strbuf_addstr` and `strbuf_complete` have also been
  removed.
* I added a separate commit to cache the `real_gitdir` value.  On my mystery
  machine, the call to `real_pathdup`, instrumented with trace2, takes an
  anecdotal 40 usecs for a somewhat lengthy path [1].  This compares to around
  10 usecs for an empty trace2 region.  I can remove the commit if you don't
  think the performance savings are worth the clutter.

Let me know if you have further comments or suggestions.

[1]: path with one symlink: './repo/link/a/b/././././..//////c/d/./xyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxyxy/././../e/../f/../g/../h/../././///../../..'

Goss Geppert (3):
  dir: traverse into repository
  dir: cache git_dir's realpath
  dir: minor refactoring / clean-up

 dir.c                          |  90 ++++++++---
 t/t2205-add-worktree-config.sh | 274 +++++++++++++++++++++++++++++++++
 2 files changed, 341 insertions(+), 23 deletions(-)
 create mode 100755 t/t2205-add-worktree-config.sh

Range-diff against v2:
1:  26e6a878cd ! 1:  391fd4d8cf dir: consider worktree config in path recursion
    @@ Metadata
     Author: Goss Geppert <ggossdev@gmail.com>
     
      ## Commit message ##
    -    dir: consider worktree config in path recursion
    +    dir: traverse into repository
     
         Since 8d92fb2927 (dir: replace exponential algorithm with a linear one,
    -    2020-04-01) the following no longer works:
    +    2020-04-01) traversing into a repository's directory tree when the
    +    traversal began outside the repository's standard location has failed
    +    because the encountered repository was identified as a nested foreign
    +    repository.
     
    -        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).
    +    Prior to this commit, the failure to traverse into a repository's
    +    default worktree location was observable from a user's perspective under
    +    either of the following conditions (there may be others):
    +
    +        1) Set the `core.worktree` location to a parent directory of the
    +           default worktree; or
    +        2) Use the `--git_dir` option while the working directory is outside
    +           the repository's default worktree location
    +
    +    Under either of these conditions, symptoms of the failure to traverse
    +    into the repository's default worktree location include the inability to
    +    add files to the index or get a list of untracked files via ls-files.
     
         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.
    +    we simply treat the directory as if it doesn't contain a nested
    +    repository.
     
    -    Prior to this commit, the `add` operation was silently ignored.
    +    The commit includes test-cases to reduce the likelihood of future
    +    regressions.
     
         Signed-off-by: Goss Geppert <ggossdev@gmail.com>
     
    @@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,
     +		 * 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
    ++		 *    which could occur if the git_dir or worktree location was
    ++		 *    manually configured by the user; see t2205 testcases 1-3 for
    ++		 *    examples where this matters
     +		 */
      		struct strbuf sb = STRBUF_INIT;
      		strbuf_addstr(&sb, dirname);
    @@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,
     +
     +		if (nested_repo) {
     +			char *real_dirname, *real_gitdir;
    -+			strbuf_reset(&sb);
    -+			strbuf_addstr(&sb, dirname);
    -+			strbuf_complete(&sb, '/');
     +			strbuf_addstr(&sb, ".git");
    -+			real_dirname = real_pathdup(sb.buf, 0);
    -+			real_gitdir = real_pathdup(the_repository->gitdir, 0);
    ++			real_dirname = real_pathdup(sb.buf, 1);
    ++			real_gitdir = real_pathdup(the_repository->gitdir, 1);
     +
     +			nested_repo = !!strcmp(real_dirname, real_gitdir);
     +			free(real_gitdir);
    @@ t/t2205-add-worktree-config.sh (new)
     @@
     +#!/bin/sh
     +
    -+test_description='directory traversal respects worktree config
    ++test_description='directory traversal respects user config
    ++
    ++This test verifies the traversal of the directory tree when the traversal begins
    ++outside the repository.  Two instances for which this can occur are tested:
     +
    -+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)'
    ++	1) The user manually sets the worktree.  For this instance, the test sets
    ++	   the worktree 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).
    ++	2) The user manually sets the `git_dir` while the working directory is
    ++	   outside the repository.  The test checks that files inside the
    ++	   repository can be added to the index.
    ++	'
     +
     +. ./test-lib.sh
     +
    -+test_expect_success '1a: setup' '
    -+	test_create_repo test1 &&
    -+	git --git-dir="test1/.git" config core.worktree "$(pwd)" &&
    ++test_expect_success '1a: setup--config worktree' '
    ++	mkdir test1 &&
    ++	(
    ++	cd test1 &&
    ++	test_create_repo repo &&
    ++	git --git-dir="repo/.git" config core.worktree "$(pwd)" &&
     +
     +	mkdir -p outside-tracked outside-untracked &&
    -+	mkdir -p test1/inside-tracked test1/inside-untracked &&
    ++	mkdir -p repo/inside-tracked repo/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 &&
    ++	>repo/file-tracked &&
    ++	>repo/file-untracked &&
    ++	>repo/inside-tracked/file &&
    ++	>repo/inside-untracked/file &&
     +
     +	cat >expect-tracked-unsorted <<-EOF &&
     +	../file-tracked
    @@ t/t2205-add-worktree-config.sh (new)
     +	inside-untracked/file
     +	EOF
     +
    ++	cat >expect-all-dir-unsorted <<-EOF &&
    ++	../file-untracked
    ++	../file-tracked
    ++	../outside-untracked/
    ++	../outside-tracked/
    ++	./
    ++	EOF
    ++
     +	cat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&
     +
     +	cat >.gitignore <<-EOF
    @@ t/t2205-add-worktree-config.sh (new)
     +	actual-*
     +	expect-*
     +	EOF
    ++	)
     +'
     +
     +test_expect_success '1b: pre-add all' '
    ++	(
    ++	cd test1 &&
     +	local parent_dir="$(pwd)" &&
     +	(
    -+		cd test1 &&
    ++		cd repo &&
     +		git ls-files -o --exclude-standard "$parent_dir" >../actual-all-unsorted
     +	) &&
     +	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' '
    ++test_expect_success '1c: pre-add dir all' '
    ++	(
    ++	cd test1 &&
     +	local parent_dir="$(pwd)" &&
     +	(
    -+		cd test1 &&
    ++		cd repo &&
    ++		git ls-files -o --directory --exclude-standard "$parent_dir" >../actual-all-dir-unsorted
    ++	) &&
    ++	sort actual-all-dir-unsorted >actual-all &&
    ++	sort expect-all-dir-unsorted >expect-all &&
    ++	test_cmp expect-all actual-all
    ++	)
    ++'
    ++
    ++test_expect_success '1d: post-add tracked' '
    ++	(
    ++	cd test1 &&
    ++	local parent_dir="$(pwd)" &&
    ++	(
    ++		cd repo &&
     +		git add file-tracked &&
     +		git add inside-tracked &&
     +		git add ../outside-tracked &&
    @@ t/t2205-add-worktree-config.sh (new)
     +	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' '
    ++test_expect_success '1e: post-add untracked' '
    ++	(
    ++	cd test1 &&
     +	local parent_dir="$(pwd)" &&
     +	(
    -+		cd test1 &&
    ++		cd repo &&
     +		git ls-files -o --exclude-standard "$parent_dir" >../actual-untracked-unsorted
     +	) &&
     +	sort actual-untracked-unsorted >actual-untracked &&
     +	sort expect-untracked-unsorted >expect-untracked &&
     +	test_cmp expect-untracked actual-untracked
    ++	)
    ++'
    ++
    ++test_expect_success '2a: setup--set git-dir' '
    ++	mkdir test2 &&
    ++	(
    ++	cd test2 &&
    ++	test_create_repo repo &&
    ++	# create two foreign repositories that should remain untracked
    ++	test_create_repo repo-outside &&
    ++	test_create_repo repo/repo-inside &&
    ++
    ++	mkdir -p repo/inside-tracked repo/inside-untracked &&
    ++	>repo/file-tracked &&
    ++	>repo/file-untracked &&
    ++	>repo/inside-tracked/file &&
    ++	>repo/inside-untracked/file &&
    ++	>repo-outside/file &&
    ++	>repo/repo-inside/file &&
    ++
    ++	cat >expect-tracked-unsorted <<-EOF &&
    ++	repo/file-tracked
    ++	repo/inside-tracked/file
    ++	EOF
    ++
    ++	cat >expect-untracked-unsorted <<-EOF &&
    ++	repo/file-untracked
    ++	repo/inside-untracked/file
    ++	repo/repo-inside/
    ++	repo-outside/
    ++	EOF
    ++
    ++	cat >expect-all-dir-unsorted <<-EOF &&
    ++	repo/
    ++	repo-outside/
    ++	EOF
    ++
    ++	cat expect-tracked-unsorted expect-untracked-unsorted >expect-all-unsorted &&
    ++
    ++	cat >.gitignore <<-EOF
    ++	.gitignore
    ++	actual-*
    ++	expect-*
    ++	EOF
    ++	)
    ++'
    ++
    ++test_expect_success '2b: pre-add all' '
    ++	(
    ++	cd test2 &&
    ++	git --git-dir=repo/.git ls-files -o --exclude-standard >actual-all-unsorted &&
    ++	sort actual-all-unsorted >actual-all &&
    ++	sort expect-all-unsorted >expect-all &&
    ++	test_cmp expect-all actual-all
    ++	)
    ++'
    ++
    ++test_expect_success '2c: pre-add dir all' '
    ++	(
    ++	cd test2 &&
    ++	git --git-dir=repo/.git ls-files -o --directory --exclude-standard >actual-all-dir-unsorted &&
    ++	sort actual-all-dir-unsorted >actual-all &&
    ++	sort expect-all-dir-unsorted >expect-all &&
    ++	test_cmp expect-all actual-all
    ++	)
    ++'
    ++
    ++test_expect_success '2d: post-add tracked' '
    ++	(
    ++	cd test2 &&
    ++	git --git-dir=repo/.git add repo/file-tracked &&
    ++	git --git-dir=repo/.git add repo/inside-tracked &&
    ++	git --git-dir=repo/.git ls-files >actual-tracked-unsorted &&
    ++	sort actual-tracked-unsorted >actual-tracked &&
    ++	sort expect-tracked-unsorted >expect-tracked &&
    ++	test_cmp expect-tracked actual-tracked
    ++	)
    ++'
    ++
    ++test_expect_success '2e: post-add untracked' '
    ++	(
    ++	cd test2 &&
    ++	git --git-dir=repo/.git ls-files -o --exclude-standard >actual-untracked-unsorted &&
    ++	sort actual-untracked-unsorted >actual-untracked &&
    ++	sort expect-untracked-unsorted >expect-untracked &&
    ++	test_cmp expect-untracked actual-untracked
    ++	)
    ++'
    ++
    ++test_expect_success '3a: setup--add repo dir' '
    ++	mkdir test3 &&
    ++	(
    ++	cd test3 &&
    ++	test_create_repo repo &&
    ++
    ++	mkdir -p repo/inside-tracked repo/inside-ignored &&
    ++	>repo/file-tracked &&
    ++	>repo/file-ignored &&
    ++	>repo/inside-tracked/file &&
    ++	>repo/inside-ignored/file &&
    ++
    ++	cat >.gitignore <<-EOF &&
    ++	.gitignore
    ++	actual-*
    ++	expect-*
    ++	*ignored
    ++	EOF
    ++
    ++	cat >expect-tracked-unsorted <<-EOF &&
    ++	repo/file-tracked
    ++	repo/inside-tracked/file
    ++	EOF
    ++
    ++	cat >expect-ignored-unsorted <<-EOF
    ++	repo/file-ignored
    ++	repo/inside-ignored/
    ++	.gitignore
    ++	actual-ignored-unsorted
    ++	expect-ignored-unsorted
    ++	expect-tracked-unsorted
    ++	EOF
    ++	)
    ++'
    ++
    ++test_expect_success '3b: ignored' '
    ++	(
    ++	cd test3 &&
    ++	git --git-dir=repo/.git ls-files -io --directory --exclude-standard >actual-ignored-unsorted &&
    ++	sort actual-ignored-unsorted >actual-ignored &&
    ++	sort expect-ignored-unsorted >expect-ignored &&
    ++	test_cmp expect-ignored actual-ignored
    ++	)
    ++'
    ++
    ++test_expect_success '3c: add repo' '
    ++	(
    ++	cd test3 &&
    ++	git --git-dir=repo/.git add repo &&
    ++	git --git-dir=repo/.git ls-files >actual-tracked-unsorted &&
    ++	sort actual-tracked-unsorted >actual-tracked &&
    ++	sort expect-tracked-unsorted >expect-tracked &&
    ++	test_cmp expect-tracked actual-tracked
    ++	)
     +'
     +
     +test_done
-:  ---------- > 2:  16e87a0345 dir: cache git_dir's realpath
2:  2e0a178c78 ! 3:  6de7932731 dir: minor refactoring / clean-up
    @@ Metadata
      ## Commit message ##
         dir: minor refactoring / clean-up
     
    -    Improve readability.
    +    Narrow the scope of the `nested_repo` variable and conditional return
    +    statement to the block where the variable is set.
     
         Signed-off-by: Goss Geppert <ggossdev@gmail.com>
     
    @@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,
      	/* The "len-1" is to strip the final '/' */
      	enum exist_status status = directory_exists_in_index(istate, dirname, len-1);
     @@ dir.c: static enum path_treatment treat_directory(struct dir_struct *dir,
    - 		 *    configured by the user; see t2205 testcases 1a-1d for examples
    - 		 *    where this matters
    + 		 *    manually configured by the user; see t2205 testcases 1-3 for
    + 		 *    examples where this matters
      		 */
     +		int nested_repo;
      		struct strbuf sb = STRBUF_INIT;
-- 
2.36.0