[PATCH 1/2] worktree setup: return to original cwd if prefix is set NULL

[PATCH 1/2] worktree setup: return to original cwd if prefix is set NULL

[Nguyễn Thái Ngọc Duy]

Commit a672ea6 (rehabilitate 'git index-pack' inside the object
store - 2008-10-20) worked around a fault in
setup_git_directory_gently(). When walking up from inside a git
repository, we will return NULL as prefix.

Prefix and cwd should be consistent. That is if cwd is moved, prefix
reflects that. If prefix is NULL, cwd is unmoved. In this case, to
retain current behavior, we move cwd back, with an eye on gitdir
because gitdir may be relative to cwd.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 setup.c             |    7 +++++++
 t/t1501-worktree.sh |   21 +++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/setup.c b/setup.c
index 87c21f0..33eb253 100644
--- a/setup.c
+++ b/setup.c
@@ -413,6 +413,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			if (!work_tree_env)
 				inside_work_tree = 0;
 			if (offset != len) {
+				/*
+				 * The returned prefix in this code path is NULL
+				 * chdir() back to match NULL prefix, i.e. unmoved cwd
+				 */
+				if (chdir(cwd))
+					die_errno("Cannot come back to cwd");
+
 				root_len = offset_1st_component(cwd);
 				cwd[offset > root_len ? offset : root_len] = '\0';
 				set_git_dir(cwd);
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index bd8b607..edd81ce 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -33,6 +33,27 @@ mkdir -p work/sub/dir || exit 1
 mkdir -p work2 || exit 1
 mv .git repo.git || exit 1
 
+cat >repo.git/objects/patch <<\EOF
+diff one one
+--- one
++++ one
+@@ -1 +1,2 @@
+ 1
++2
+EOF
+
+test_expect_success 'cwd is unchanged when prefix is NULL (from within a repo)' '
+	(
+		cd repo.git/objects &&
+		echo 1 > one &&
+		cp one expected &&
+		echo 2 >> expected &&
+		git apply patch &&
+		test_cmp expected one &&
+		rm one expected patch
+	)
+'
+
 say "core.worktree = relative path"
 GIT_DIR=repo.git
 GIT_CONFIG="$(pwd)"/$GIT_DIR/config
-- 
1.7.1.rc1.69.g24c2f7

[PATCH 2/2] Revert "rehabilitate 'git index-pack' inside the object store"

[Nguyễn Thái Ngọc Duy]

This reverts code change in commit a672ea6ac5a1b876bc7adfe6534b16fa2a32c94b.

The cwd behavior now fits index-pack well, i.e. NULL prefix means
no cwd changes.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/index-pack.c |   24 +++++-------------------
 1 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a89ae83..89a1f12 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -880,29 +880,15 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	char *index_name_buf = NULL, *keep_name_buf = NULL;
 	struct pack_idx_entry **idx_objects;
 	unsigned char pack_sha1[20];
+	int nongit;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(index_pack_usage);
 
-	/*
-	 * We wish to read the repository's config file if any, and
-	 * for that it is necessary to call setup_git_directory_gently().
-	 * However if the cwd was inside .git/objects/pack/ then we need
-	 * to go back there or all the pack name arguments will be wrong.
-	 * And in that case we cannot rely on any prefix returned by
-	 * setup_git_directory_gently() either.
-	 */
-	{
-		char cwd[PATH_MAX+1];
-		int nongit;
-
-		if (!getcwd(cwd, sizeof(cwd)-1))
-			die("Unable to get current working directory");
-		setup_git_directory_gently(&nongit);
-		git_config(git_index_pack_config, NULL);
-		if (chdir(cwd))
-			die("Cannot come back to cwd");
-	}
+	prefix = setup_git_directory_gently(&nongit);
+	git_config(git_index_pack_config, NULL);
+	if (prefix && chdir(prefix))
+		die("Cannot come back to cwd");
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
-- 
1.7.1.rc1.69.g24c2f7

Re: [PATCH 1/2] worktree setup: return to original cwd if prefix is set NULL

[Ævar Arnfjörð Bjarmason]

2010/7/23 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:

> +test_expect_success 'cwd is unchanged when prefix is NULL (from within a repo)' '
> +       (
> +               cd repo.git/objects &&
> +               echo 1 > one &&
> +               cp one expected &&
> +               echo 2 >> expected &&
> +               git apply patch &&
> +               test_cmp expected one &&
> +               rm one expected patch
> +       )
> +'

Is the rm at the end needed here?

This is a minor nit, but it's generally helpful for debugging tests
that things aren't removed if the --debug option is specified.

Re: [PATCH 1/2] worktree setup: return to original cwd if prefix is set NULL

[Nguyen Thai Ngoc Duy]

2010/7/24 Ævar Arnfjörð Bjarmason <avarab@gmail.com>:
> 2010/7/23 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
>
>> +test_expect_success 'cwd is unchanged when prefix is NULL (from within a repo)' '
>> +       (
>> +               cd repo.git/objects &&
>> +               echo 1 > one &&
>> +               cp one expected &&
>> +               echo 2 >> expected &&
>> +               git apply patch &&
>> +               test_cmp expected one &&
>> +               rm one expected patch
>> +       )
>> +'
>
> Is the rm at the end needed here?
>
> This is a minor nit, but it's generally helpful for debugging tests
> that things aren't removed if the --debug option is specified.
>

I don't want to pollute the repo early. If the test fails, rm would not be run.
-- 
Duy

Re: [PATCH 1/2] worktree setup: return to original cwd if prefix is set NULL

[Ævar Arnfjörð Bjarmason]

On Sat, Jul 24, 2010 at 00:50, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:

> I don't want to pollute the repo early. If the test fails, rm would not be run.

Sure, I'm just asking if there's a reason to run rm in this
case. Because usually we defer the whole rm to be done inside
test-lib.sh once the entire test completes.

[PATCH 1/2] worktree setup: return to original cwd if prefix is set NULL

[Nguyễn Thái Ngọc Duy]

Commit a672ea6 (rehabilitate 'git index-pack' inside the object
store - 2008-10-20) worked around a fault in
setup_git_directory_gently(). When walking up from inside a git
repository, we will return NULL as prefix.

Prefix and cwd should be consistent. That is if cwd is moved, prefix
reflects that. If prefix is NULL, cwd is unmoved. In this case, to
retain current behavior, we move cwd back, with an eye on gitdir
because gitdir may be relative to cwd.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
  On Sat, Jul 24, 2010 at 01:16:00AM +0000, Ævar Arnfjörð Bjarmason wrote:
  > On Sat, Jul 24, 2010 at 00:50, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
  > 
  > > I don't want to pollute the repo early. If the test fails, rm would not be run.
  > 
  > Sure, I'm just asking if there's a reason to run rm in this
  > case. Because usually we defer the whole rm to be done inside
  > test-lib.sh once the entire test completes.
  
  "rm" removed. The rest of the tests do not seem to care whatelse in objects..

 setup.c             |    7 +++++++
 t/t1501-worktree.sh |   20 ++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/setup.c b/setup.c
index 87c21f0..33eb253 100644
--- a/setup.c
+++ b/setup.c
@@ -413,6 +413,13 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			if (!work_tree_env)
 				inside_work_tree = 0;
 			if (offset != len) {
+				/*
+				 * The returned prefix in this code path is NULL
+				 * chdir() back to match NULL prefix, i.e. unmoved cwd
+				 */
+				if (chdir(cwd))
+					die_errno("Cannot come back to cwd");
+
 				root_len = offset_1st_component(cwd);
 				cwd[offset > root_len ? offset : root_len] = '\0';
 				set_git_dir(cwd);
diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index bd8b607..a024056 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -33,6 +33,26 @@ mkdir -p work/sub/dir || exit 1
 mkdir -p work2 || exit 1
 mv .git repo.git || exit 1
 
+cat >repo.git/objects/patch <<\EOF
+diff one one
+--- one
++++ one
+@@ -1 +1,2 @@
+ 1
++2
+EOF
+
+test_expect_success 'cwd is unchanged when prefix is NULL (from within a repo)' '
+	(
+		cd repo.git/objects &&
+		echo 1 > one &&
+		cp one expected &&
+		echo 2 >> expected &&
+		git apply patch &&
+		test_cmp expected one
+	)
+'
+
 say "core.worktree = relative path"
 GIT_DIR=repo.git
 GIT_CONFIG="$(pwd)"/$GIT_DIR/config
-- 
1.7.1.rc1.69.g24c2f7

[PATCH 0/9] setup_git_directory(): return to original cwd upon reaching .git

[Jonathan Nieder]

Hi Duy,

Nguyễn Thái Ngọc Duy wrote:

> Prefix and cwd should be consistent.

I agree with this, but it took me a while to figure out why your fix
is safe.  Here’s how I figured it out.

Patch 1 cleans up the test script you added a test to.  After cleaning
it up, it is clearer the test does not belong there.

Patch 2 creates a proper home for the new test.

Patches 3-7 split up setup_git_directory_gently() into small enough
pieces that a person with a short attention span can read it now.
No functional change intended.

Patch 8 is your fix.  I removed the comment (which was just confusing
me) and clarified the commit message to compensate

Patch 9 is your patch to revert the other, now redundant fix, also
with commit message tweaks.

After this exercise, your patches still look good. :)  Maybe these
by-products could be useful somehow.

Thoughts (especially improvements) welcome.

Jonathan Nieder (7):
  t1501 (rev-parse): clarify
  tests: try git apply from subdir of toplevel
  setup: split off $GIT_DIR-set case from setup_git_directory_gently
  setup: split off a function to checks working dir for .git file
  setup: split off code to handle stumbling upon a repository
  setup: split off a function to handle hitting ceiling in repo search
  setup: split off get_device_or_die function

Nguyễn Thái Ngọc Duy (2):
  setup: do not forget working dir from subdir of gitdir
  Revert "rehabilitate 'git index-pack' inside the object store"

 builtin/index-pack.c    |   24 +--
 setup.c                 |  171 ++++++++++--------
 t/t1501-worktree.sh     |  467 ++++++++++++++++++++++++++++++-----------------
 t/t4111-apply-subdir.sh |  141 ++++++++++++++
 4 files changed, 547 insertions(+), 256 deletions(-)
 create mode 100755 t/t4111-apply-subdir.sh

-- 
1.7.2.rc3

[PATCH 1/9] t1501 (rev-parse): clarify

[Jonathan Nieder]

Tweak the style of these tests to make them easier to read.

 - Replace test_rev_parse() which produced multiple mini-tests with a
   simple function that can be used with the test body.

 - Combine multiple mini-tests into larger chunks that are easier
   to read.

 - Do not hard-code object IDs.  We may use a different hash some day.

 - Use test_cmp in preference to the test builtin.  The former
   produces useful output when tests are run with the "-v" option.

 - Guard all test code with test_expect_success.  This makes it much
   easier to visually scan through the test and find code of interest.

 - Use subshells to make the current directory easier to track.
   Outside of any subshell, the current directory is always
   $TEST_DIRECTORY now.

Also add a new test demonstrating a possible bug noticed in the
process of cleaning up:  “git rev-parse --show-prefix” leaves out
the trailing newline after an empty prefix when cwd is at the
toplevel of the work tree.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t1501-worktree.sh |  467 +++++++++++++++++++++++++++++++++------------------
 1 files changed, 302 insertions(+), 165 deletions(-)

diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
index bd8b607..2c8f01f 100755
--- a/t/t1501-worktree.sh
+++ b/t/t1501-worktree.sh
@@ -3,183 +3,320 @@
 test_description='test separate work tree'
 . ./test-lib.sh
 
-test_rev_parse() {
-	name=$1
-	shift
-
-	test_expect_success "$name: is-bare-repository" \
-	"test '$1' = \"\$(git rev-parse --is-bare-repository)\""
-	shift
-	[ $# -eq 0 ] && return
-
-	test_expect_success "$name: is-inside-git-dir" \
-	"test '$1' = \"\$(git rev-parse --is-inside-git-dir)\""
-	shift
-	[ $# -eq 0 ] && return
-
-	test_expect_success "$name: is-inside-work-tree" \
-	"test '$1' = \"\$(git rev-parse --is-inside-work-tree)\""
-	shift
-	[ $# -eq 0 ] && return
-
-	test_expect_success "$name: prefix" \
-	"test '$1' = \"\$(git rev-parse --show-prefix)\""
-	shift
-	[ $# -eq 0 ] && return
-}
-
-EMPTY_TREE=$(git write-tree)
-mkdir -p work/sub/dir || exit 1
-mkdir -p work2 || exit 1
-mv .git repo.git || exit 1
-
-say "core.worktree = relative path"
-GIT_DIR=repo.git
-GIT_CONFIG="$(pwd)"/$GIT_DIR/config
-export GIT_DIR GIT_CONFIG
-unset GIT_WORK_TREE
-git config core.worktree ../work
-test_rev_parse 'outside'      false false false
-cd work || exit 1
-GIT_DIR=../repo.git
-GIT_CONFIG="$(pwd)"/$GIT_DIR/config
-test_rev_parse 'inside'       false false true ''
-cd sub/dir || exit 1
-GIT_DIR=../../../repo.git
-GIT_CONFIG="$(pwd)"/$GIT_DIR/config
-test_rev_parse 'subdirectory' false false true sub/dir/
-cd ../../.. || exit 1
-
-say "core.worktree = absolute path"
-GIT_DIR=$(pwd)/repo.git
-GIT_CONFIG=$GIT_DIR/config
-git config core.worktree "$(pwd)/work"
-test_rev_parse 'outside'      false false false
-cd work2
-test_rev_parse 'outside2'     false false false
-cd ../work || exit 1
-test_rev_parse 'inside'       false false true ''
-cd sub/dir || exit 1
-test_rev_parse 'subdirectory' false false true sub/dir/
-cd ../../.. || exit 1
-
-say "GIT_WORK_TREE=relative path (override core.worktree)"
-GIT_DIR=$(pwd)/repo.git
-GIT_CONFIG=$GIT_DIR/config
-git config core.worktree non-existent
-GIT_WORK_TREE=work
-export GIT_WORK_TREE
-test_rev_parse 'outside'      false false false
-cd work2
-test_rev_parse 'outside'      false false false
-cd ../work || exit 1
-GIT_WORK_TREE=.
-test_rev_parse 'inside'       false false true ''
-cd sub/dir || exit 1
-GIT_WORK_TREE=../..
-test_rev_parse 'subdirectory' false false true sub/dir/
-cd ../../.. || exit 1
-
-mv work repo.git/work
-mv work2 repo.git/work2
-
-say "GIT_WORK_TREE=absolute path, work tree below git dir"
-GIT_DIR=$(pwd)/repo.git
-GIT_CONFIG=$GIT_DIR/config
-GIT_WORK_TREE=$(pwd)/repo.git/work
-test_rev_parse 'outside'              false false false
-cd repo.git || exit 1
-test_rev_parse 'in repo.git'              false true  false
-cd objects || exit 1
-test_rev_parse 'in repo.git/objects'      false true  false
-cd ../work2 || exit 1
-test_rev_parse 'in repo.git/work2'      false true  false
-cd ../work || exit 1
-test_rev_parse 'in repo.git/work'         false true true ''
-cd sub/dir || exit 1
-test_rev_parse 'in repo.git/sub/dir' false true true sub/dir/
-cd ../../../.. || exit 1
-
-test_expect_success 'repo finds its work tree' '
-	(cd repo.git &&
-	 : > work/sub/dir/untracked &&
-	 test sub/dir/untracked = "$(git ls-files --others)")
-'
-
-test_expect_success 'repo finds its work tree from work tree, too' '
-	(cd repo.git/work/sub/dir &&
-	 : > tracked &&
-	 git --git-dir=../../.. add tracked &&
-	 cd ../../.. &&
-	 test sub/dir/tracked = "$(git ls-files)")
+test_expect_success 'setup' '
+	EMPTY_TREE=$(git write-tree) &&
+	EMPTY_BLOB=$(git hash-object -t blob --stdin </dev/null) &&
+	CHANGED_BLOB=$(echo changed | git hash-object -t blob --stdin) &&
+	ZEROES=0000000000000000000000000000000000000000 &&
+	EMPTY_BLOB7=$(echo $EMPTY_BLOB | sed "s/\(.......\).*/\1/") &&
+	CHANGED_BLOB7=$(echo $CHANGED_BLOB | sed "s/\(.......\).*/\1/") &&
+
+	mkdir -p work/sub/dir &&
+	mkdir -p work2 &&
+	mv .git repo.git
+'
+
+test_expect_success 'setup: helper for testing rev-parse' '
+	test_rev_parse() {
+		echo $1 >expected.bare &&
+		echo $2 >expected.inside-git &&
+		echo $3 >expected.inside-worktree &&
+		if test $# -ge 4
+		then
+			echo $4 >expected.prefix
+		fi &&
+
+		git rev-parse --is-bare-repository >actual.bare &&
+		git rev-parse --is-inside-git-dir >actual.inside-git &&
+		git rev-parse --is-inside-work-tree >actual.inside-worktree &&
+		if test $# -ge 4
+		then
+			git rev-parse --show-prefix >actual.prefix
+		fi &&
+
+		test_cmp expected.bare actual.bare &&
+		test_cmp expected.inside-git actual.inside-git &&
+		test_cmp expected.inside-worktree actual.inside-worktree &&
+		if test $# -ge 4
+		then
+			# rev-parse --show-prefix should output
+			# a single newline when at the top of the work tree,
+			# but we test for that separately.
+			test -z "$4" && ! test -s actual.prefix ||
+			test_cmp expected.prefix actual.prefix
+		fi
+	}
+'
+
+test_expect_success 'setup: core.worktree = relative path' '
+	unset GIT_WORK_TREE;
+	GIT_DIR=repo.git &&
+	GIT_CONFIG="$(pwd)"/$GIT_DIR/config &&
+	export GIT_DIR GIT_CONFIG &&
+	git config core.worktree ../work
+'
+
+test_expect_success 'outside' '
+	test_rev_parse false false false
+'
+
+test_expect_success 'inside work tree' '
+	(
+		cd work &&
+		GIT_DIR=../repo.git &&
+		GIT_CONFIG="$(pwd)"/$GIT_DIR/config &&
+		test_rev_parse false false true ""
+	)
+'
+
+test_expect_failure 'empty prefix is actually written out' '
+	echo >expected &&
+	(
+		cd work &&
+		GIT_DIR=../repo.git &&
+		GIT_CONFIG="$(pwd)"/$GIT_DIR/config &&
+		git rev-parse --show-prefix >../actual
+	) &&
+	test_cmp expected actual
+'
+
+test_expect_success 'subdir of work tree' '
+	(
+		cd work/sub/dir &&
+		GIT_DIR=../../../repo.git &&
+		GIT_CONFIG="$(pwd)"/$GIT_DIR/config &&
+		test_rev_parse false false true sub/dir/
+	)
+'
+
+test_expect_success 'setup: core.worktree = absolute path' '
+	unset GIT_WORK_TREE;
+	GIT_DIR=$(pwd)/repo.git &&
+	GIT_CONFIG=$GIT_DIR/config &&
+	export GIT_DIR GIT_CONFIG &&
+	git config core.worktree "$(pwd)/work"
+'
+
+test_expect_success 'outside' '
+	test_rev_parse false false false &&
+	(
+		cd work2 &&
+		test_rev_parse false false false
+	)
+'
+
+test_expect_success 'inside work tree' '
+	(
+		cd work &&
+		test_rev_parse false false true ""
+	)
+'
+
+test_expect_success 'subdir of work tree' '
+	(
+		cd work/sub/dir &&
+		test_rev_parse false false true sub/dir/
+	)
+'
+
+test_expect_success 'setup: GIT_WORK_TREE=relative (override core.worktree)' '
+	GIT_DIR=$(pwd)/repo.git &&
+	GIT_CONFIG=$GIT_DIR/config &&
+	git config core.worktree non-existent &&
+	GIT_WORK_TREE=work &&
+	export GIT_DIR GIT_CONFIG GIT_WORK_TREE
+'
+
+test_expect_success 'outside' '
+	test_rev_parse false false false &&
+	(
+		cd work2 &&
+		test_rev_parse false false false
+	)
+'
+
+test_expect_success 'inside work tree' '
+	(
+		cd work &&
+		GIT_WORK_TREE=. &&
+		test_rev_parse false false true ""
+	)
+'
+
+test_expect_success 'subdir of work tree' '
+	(
+		cd work/sub/dir &&
+		GIT_WORK_TREE=../.. &&
+		test_rev_parse false false true sub/dir/
+	)
+'
+
+test_expect_success 'setup: GIT_WORK_TREE=absolute, below git dir' '
+	mv work repo.git/work &&
+	mv work2 repo.git/work2 &&
+	GIT_DIR=$(pwd)/repo.git &&
+	GIT_CONFIG=$GIT_DIR/config &&
+	GIT_WORK_TREE=$(pwd)/repo.git/work &&
+	export GIT_DIR GIT_CONFIG GIT_WORK_TREE
+'
+
+test_expect_success 'outside' '
+	echo outside &&
+	test_rev_parse false false false
+'
+
+test_expect_success 'in repo.git' '
+	(
+		cd repo.git &&
+		test_rev_parse false true false
+	) &&
+	(
+		cd repo.git/objects &&
+		test_rev_parse false true false
+	) &&
+	(
+		cd repo.git/work2 &&
+		test_rev_parse false true false
+	)
+'
+
+test_expect_success 'inside work tree' '
+	(
+		cd repo.git/work &&
+		test_rev_parse false true true ""
+	)
+'
+
+test_expect_success 'subdir of work tree' '
+	(
+		cd repo.git/work/sub/dir &&
+		test_rev_parse false true true sub/dir/
+	)
+'
+
+test_expect_success 'find work tree from repo' '
+	echo sub/dir/untracked >expected &&
+	cat <<-\EOF >repo.git/work/.gitignore &&
+	expected.*
+	actual.*
+	.gitignore
+	EOF
+	>repo.git/work/sub/dir/untracked &&
+	(
+		cd repo.git &&
+		git ls-files --others --exclude-standard >../actual
+	) &&
+	test_cmp expected actual
+'
+
+test_expect_success 'find work tree from work tree' '
+	echo sub/dir/tracked >expected &&
+	>repo.git/work/sub/dir/tracked &&
+	(
+		cd repo.git/work/sub/dir &&
+		git --git-dir=../../.. add tracked
+	) &&
+	(
+		cd repo.git &&
+		git ls-files >../actual
+	) &&
+	test_cmp expected actual
 '
 
 test_expect_success '_gently() groks relative GIT_DIR & GIT_WORK_TREE' '
-	(cd repo.git/work/sub/dir &&
-	GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \
+	(
+		cd repo.git/work/sub/dir &&
+		GIT_DIR=../../.. &&
+		GIT_WORK_TREE=../.. &&
+		GIT_PAGER= &&
+		export GIT_DIR GIT_WORK_TREE GIT_PAGER &&
+
 		git diff --exit-code tracked &&
-	echo changed > tracked &&
-	! GIT_DIR=../../.. GIT_WORK_TREE=../.. GIT_PAGER= \
-		git diff --exit-code tracked)
+		echo changed >tracked &&
+		test_must_fail git diff --exit-code tracked
+	)
 '
-cat > diff-index-cached.expected <<\EOF
-:000000 100644 0000000000000000000000000000000000000000 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 A	sub/dir/tracked
-EOF
-cat > diff-index.expected <<\EOF
-:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A	sub/dir/tracked
-EOF
 
+test_expect_success 'diff-index respects work tree under .git dir' '
+	cat >diff-index-cached.expected <<-EOF &&
+	:000000 100644 $ZEROES $EMPTY_BLOB A	sub/dir/tracked
+	EOF
+	cat >diff-index.expected <<-EOF &&
+	:000000 100644 $ZEROES $ZEROES A	sub/dir/tracked
+	EOF
 
-test_expect_success 'git diff-index' '
-	GIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff-index $EMPTY_TREE > result &&
-	test_cmp diff-index.expected result &&
-	GIT_DIR=repo.git git diff-index --cached $EMPTY_TREE > result &&
-	test_cmp diff-index-cached.expected result
+	(
+		GIT_DIR=repo.git &&
+		GIT_WORK_TREE=repo.git/work &&
+		export GIT_DIR GIT_WORK_TREE &&
+		git diff-index $EMPTY_TREE >diff-index.actual &&
+		git diff-index --cached $EMPTY_TREE >diff-index-cached.actual
+	) &&
+	test_cmp diff-index.expected diff-index.actual &&
+	test_cmp diff-index-cached.expected diff-index-cached.actual
 '
-cat >diff-files.expected <<\EOF
-:100644 100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0000000000000000000000000000000000000000 M	sub/dir/tracked
-EOF
 
-test_expect_success 'git diff-files' '
-	GIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff-files > result &&
-	test_cmp diff-files.expected result
+test_expect_success 'diff-files respects work tree under .git dir' '
+	cat >diff-files.expected <<-EOF &&
+	:100644 100644 $EMPTY_BLOB $ZEROES M	sub/dir/tracked
+	EOF
+
+	(
+		GIT_DIR=repo.git &&
+		GIT_WORK_TREE=repo.git/work &&
+		export GIT_DIR GIT_WORK_TREE &&
+		git diff-files >diff-files.actual
+	) &&
+	test_cmp diff-files.expected diff-files.actual
 '
 
-cat >diff-TREE.expected <<\EOF
-diff --git a/sub/dir/tracked b/sub/dir/tracked
-new file mode 100644
-index 0000000..5ea2ed4
---- /dev/null
-+++ b/sub/dir/tracked
-@@ -0,0 +1 @@
-+changed
-EOF
-cat >diff-TREE-cached.expected <<\EOF
-diff --git a/sub/dir/tracked b/sub/dir/tracked
-new file mode 100644
-index 0000000..e69de29
-EOF
-cat >diff-FILES.expected <<\EOF
-diff --git a/sub/dir/tracked b/sub/dir/tracked
-index e69de29..5ea2ed4 100644
---- a/sub/dir/tracked
-+++ b/sub/dir/tracked
-@@ -0,0 +1 @@
-+changed
-EOF
+test_expect_success 'git diff respects work tree under .git dir' '
+	cat >diff-TREE.expected <<-EOF &&
+	diff --git a/sub/dir/tracked b/sub/dir/tracked
+	new file mode 100644
+	index 0000000..$CHANGED_BLOB7
+	--- /dev/null
+	+++ b/sub/dir/tracked
+	@@ -0,0 +1 @@
+	+changed
+	EOF
+	cat >diff-TREE-cached.expected <<-EOF &&
+	diff --git a/sub/dir/tracked b/sub/dir/tracked
+	new file mode 100644
+	index 0000000..$EMPTY_BLOB7
+	EOF
+	cat >diff-FILES.expected <<-EOF &&
+	diff --git a/sub/dir/tracked b/sub/dir/tracked
+	index $EMPTY_BLOB7..$CHANGED_BLOB7 100644
+	--- a/sub/dir/tracked
+	+++ b/sub/dir/tracked
+	@@ -0,0 +1 @@
+	+changed
+	EOF
 
-test_expect_success 'git diff' '
-	GIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff $EMPTY_TREE > result &&
-	test_cmp diff-TREE.expected result &&
-	GIT_DIR=repo.git git diff --cached $EMPTY_TREE > result &&
-	test_cmp diff-TREE-cached.expected result &&
-	GIT_DIR=repo.git GIT_WORK_TREE=repo.git/work git diff > result &&
-	test_cmp diff-FILES.expected result
+	(
+		GIT_DIR=repo.git &&
+		GIT_WORK_TREE=repo.git/work &&
+		export GIT_DIR GIT_WORK_TREE &&
+		git diff $EMPTY_TREE >diff-TREE.actual &&
+		git diff --cached $EMPTY_TREE >diff-TREE-cached.actual &&
+		git diff >diff-FILES.actual
+	) &&
+	test_cmp diff-TREE.expected diff-TREE.actual &&
+	test_cmp diff-TREE-cached.expected diff-TREE-cached.actual &&
+	test_cmp diff-FILES.expected diff-FILES.actual
 '
 
 test_expect_success 'git grep' '
-	(cd repo.git/work/sub &&
-	GIT_DIR=../.. GIT_WORK_TREE=.. git grep -l changed | grep dir/tracked)
+	echo dir/tracked >expected.grep &&
+	(
+		cd repo.git/work/sub &&
+		GIT_DIR=../.. &&
+		GIT_WORK_TREE=.. &&
+		export GIT_DIR GIT_WORK_TREE &&
+		git grep -l changed >../../../actual.grep
+	) &&
+	test_cmp expected.grep actual.grep
 '
 
 test_expect_success 'git commit' '
@@ -191,14 +328,14 @@ test_expect_success 'git commit' '
 
 test_expect_success 'absolute pathspec should fail gracefully' '
 	(
-		cd repo.git || exit 1
-		git config --unset core.worktree
+		cd repo.git &&
+		test_might_fail git config --unset core.worktree &&
 		test_must_fail git log HEAD -- /home
 	)
 '
 
 test_expect_success 'make_relative_path handles double slashes in GIT_DIR' '
-	: > dummy_file
+	>dummy_file
 	echo git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file &&
 	git --git-dir="$(pwd)//repo.git" --work-tree="$(pwd)" add dummy_file
 '
-- 
1.7.2.rc3

[PATCH 2/9] tests: try git apply from subdir of toplevel

[Jonathan Nieder]

Make sure git apply can apply patches with paths relative to the
toplevel of a work tree, a subdirectory, or within the repository
metadata directory.

Relative paths are broken for most commands when run from a
subdirectory of $GIT_DIR, "git apply" being no exception.  The other
tests are meant to keep the demonstration of that company.

Based on a test by Duy.

Cc: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t4111-apply-subdir.sh |  141 +++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 141 insertions(+), 0 deletions(-)
 create mode 100755 t/t4111-apply-subdir.sh

diff --git a/t/t4111-apply-subdir.sh b/t/t4111-apply-subdir.sh
new file mode 100755
index 0000000..60c2237
--- /dev/null
+++ b/t/t4111-apply-subdir.sh
@@ -0,0 +1,141 @@
+#!/bin/sh
+
+test_description='patching from inconvenient places'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	cat >patch <<-\EOF &&
+	diff file.orig file
+	--- a/file.orig
+	+++ b/file
+	@@ -1 +1,2 @@
+	 1
+	+2
+	EOF
+	patch="$(pwd)/patch" &&
+
+	echo 1 >preimage &&
+	printf "%s\n" 1 2 >postimage &&
+	echo 3 >other &&
+
+	test_tick &&
+	git commit --allow-empty -m basis
+'
+
+test_expect_success 'setup: subdir' '
+	reset_subdir() {
+		git reset &&
+		mkdir -p sub/dir/b &&
+		mkdir -p objects &&
+		cp "$1" file &&
+		cp "$1" objects/file &&
+		cp "$1" sub/dir/file &&
+		cp "$1" sub/dir/b/file &&
+		git add file sub/dir/file sub/dir/b/file objects/file &&
+		cp "$2" file &&
+		cp "$2" sub/dir/file &&
+		cp "$2" sub/dir/b/file &&
+		cp "$2" objects/file
+	}
+'
+
+test_expect_success 'apply from subdir of toplevel' '
+	cp postimage expected &&
+	reset_subdir other preimage &&
+	(
+		cd sub/dir &&
+		git apply "$patch"
+	) &&
+	test_cmp expected sub/dir/file
+'
+
+test_expect_success 'apply --cached from subdir of toplevel' '
+	cp postimage expected &&
+	cp other expected.working &&
+	reset_subdir preimage other &&
+	(
+		cd sub/dir &&
+		git apply --cached "$patch"
+	) &&
+	git show :sub/dir/file >actual &&
+	test_cmp expected actual &&
+	test_cmp expected.working sub/dir/file
+'
+
+test_expect_success 'apply --index from subdir of toplevel' '
+	cp postimage expected &&
+	reset_subdir preimage other &&
+	(
+		cd sub/dir &&
+		test_must_fail git apply --index "$patch"
+	) &&
+	reset_subdir other preimage &&
+	(
+		cd sub/dir &&
+		test_must_fail git apply --index "$patch"
+	) &&
+	reset_subdir preimage preimage &&
+	(
+		cd sub/dir &&
+		git apply --index "$patch"
+	) &&
+	git show :sub/dir/file >actual &&
+	test_cmp expected actual &&
+	test_cmp expected sub/dir/file
+'
+
+test_expect_success 'apply from .git dir' '
+	cp postimage expected &&
+	cp preimage .git/file &&
+	cp preimage .git/objects/file
+	(
+		cd .git &&
+		git apply "$patch"
+	) &&
+	test_cmp expected .git/file
+'
+
+test_expect_failure 'apply from subdir of .git dir' '
+	cp postimage expected &&
+	cp preimage .git/file &&
+	cp preimage .git/objects/file
+	(
+		cd .git/objects &&
+		git apply "$patch"
+	) &&
+	test_cmp expected .git/objects/file
+'
+
+test_expect_success 'apply --cached from .git dir' '
+	cp postimage expected &&
+	cp other expected.working &&
+	cp other .git/file &&
+	reset_subdir preimage other &&
+	(
+		cd .git &&
+		git apply --cached "$patch"
+	) &&
+	git show :file >actual &&
+	test_cmp expected actual &&
+	test_cmp expected.working file &&
+	test_cmp expected.working .git/file
+'
+
+test_expect_success 'apply --cached from subdir of .git dir' '
+	cp postimage expected &&
+	cp preimage expected.subdir &&
+	cp other .git/file &&
+	cp other .git/objects/file &&
+	reset_subdir preimage other &&
+	(
+		cd .git/objects &&
+		git apply --cached "$patch"
+	) &&
+	git show :file >actual &&
+	git show :objects/file >actual.subdir &&
+	test_cmp expected actual &&
+	test_cmp expected.subdir actual.subdir
+'
+
+test_done
-- 
1.7.2.rc3

[PATCH 3/9] setup: split off $GIT_DIR-set case from setup_git_directory_gently

[Jonathan Nieder]

If $GIT_DIR is set, setup_git_directory_gently does not have
to do any repository discovery at all.  Split off a function
for the validation it still does do, in the hope that this will
make setup_git_directory_gently proper less daunting to read.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 setup.c |   69 +++++++++++++++++++++++++++++++++-----------------------------
 1 files changed, 37 insertions(+), 32 deletions(-)

diff --git a/setup.c b/setup.c
index 2769160..16bee6d 100644
--- a/setup.c
+++ b/setup.c
@@ -313,6 +313,41 @@ const char *read_gitfile_gently(const char *path)
 	return path;
 }
 
+static const char *setup_explicit_git_dir(const char *gitdirenv,
+				const char *work_tree_env, int *nongit_ok)
+{
+	static char buffer[1024 + 1];
+	const char *retval;
+
+	if (PATH_MAX - 40 < strlen(gitdirenv))
+		die("'$%s' too big", GIT_DIR_ENVIRONMENT);
+	if (!is_git_directory(gitdirenv)) {
+		if (nongit_ok) {
+			*nongit_ok = 1;
+			return NULL;
+		}
+		die("Not a git repository: '%s'", gitdirenv);
+	}
+	if (!work_tree_env) {
+		retval = set_work_tree(gitdirenv);
+		/* config may override worktree */
+		if (check_repository_format_gently(nongit_ok))
+			return NULL;
+		return retval;
+	}
+	if (check_repository_format_gently(nongit_ok))
+		return NULL;
+	retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
+			get_git_work_tree());
+	if (!retval || !*retval)
+		return NULL;
+	set_git_dir(make_absolute_path(gitdirenv));
+	if (chdir(work_tree_env) < 0)
+		die_errno ("Could not chdir to '%s'", work_tree_env);
+	strcat(buffer, "/");
+	return retval;
+}
+
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
@@ -343,38 +378,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	 * validation.
 	 */
 	gitdirenv = getenv(GIT_DIR_ENVIRONMENT);
-	if (gitdirenv) {
-		if (PATH_MAX - 40 < strlen(gitdirenv))
-			die("'$%s' too big", GIT_DIR_ENVIRONMENT);
-		if (is_git_directory(gitdirenv)) {
-			static char buffer[1024 + 1];
-			const char *retval;
-
-			if (!work_tree_env) {
-				retval = set_work_tree(gitdirenv);
-				/* config may override worktree */
-				if (check_repository_format_gently(nongit_ok))
-					return NULL;
-				return retval;
-			}
-			if (check_repository_format_gently(nongit_ok))
-				return NULL;
-			retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
-					get_git_work_tree());
-			if (!retval || !*retval)
-				return NULL;
-			set_git_dir(make_absolute_path(gitdirenv));
-			if (chdir(work_tree_env) < 0)
-				die_errno ("Could not chdir to '%s'", work_tree_env);
-			strcat(buffer, "/");
-			return retval;
-		}
-		if (nongit_ok) {
-			*nongit_ok = 1;
-			return NULL;
-		}
-		die("Not a git repository: '%s'", gitdirenv);
-	}
+	if (gitdirenv)
+		return setup_explicit_git_dir(gitdirenv, work_tree_env, nongit_ok);
 
 	if (!getcwd(cwd, sizeof(cwd)-1))
 		die_errno("Unable to read current working directory");
-- 
1.7.2.rc3.593.g19611.dirty

Re: [PATCH 3/9] setup: split off $GIT_DIR-set case from setup_git_directory_gently

[Nguyen Thai Ngoc Duy]

2010/7/24 Jonathan Nieder <jrnieder@gmail.com>:
> +static const char *setup_explicit_git_dir(const char *gitdirenv,
> +                               const char *work_tree_env, int *nongit_ok)
> +{
> +       static char buffer[1024 + 1];
> +       const char *retval;
> +
> +       if (PATH_MAX - 40 < strlen(gitdirenv))
> +               die("'$%s' too big", GIT_DIR_ENVIRONMENT);
> +       if (!is_git_directory(gitdirenv)) {
> +               if (nongit_ok) {
> +                       *nongit_ok = 1;
> +                       return NULL;
> +               }
> +               die("Not a git repository: '%s'", gitdirenv);
> +       }
> +       if (!work_tree_env) {
> +               retval = set_work_tree(gitdirenv);
> +               /* config may override worktree */
> +               if (check_repository_format_gently(nongit_ok))
> +                       return NULL;
> +               return retval;
> +       }
> +       if (check_repository_format_gently(nongit_ok))
> +               return NULL;
> +       retval = get_relative_cwd(buffer, sizeof(buffer) - 1,
> +                       get_git_work_tree());

Note to self, cwd should be passed to setup_explicit_git_dir, then
drop get_relative_cwd() in favor of prefixcmp(). One less getcwd call,
buffer will not be not needed. And probably easier to understand.
-- 
Duy

[PATCH 4/9] setup: split off a function to checks working dir for .git file

[Jonathan Nieder]

The repository discovery procedure looks something like this:

	while (same filesystem) {
		check .git in working dir
		check .
		chdir(..)
	}

Add a function for the first step to make the actual code look a bit
closer to that pseudocode.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 setup.c |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/setup.c b/setup.c
index 16bee6d..3d25d0f 100644
--- a/setup.c
+++ b/setup.c
@@ -348,6 +348,18 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
 	return retval;
 }
 
+static int cwd_contains_git_dir(const char **gitfile_dirp)
+{
+	const char *gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
+	*gitfile_dirp = gitfile_dir;
+	if (gitfile_dir) {
+		if (set_git_dir(gitfile_dir))
+			die("Repository setup failed");
+		return 1;
+	}
+	return is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT);
+}
+
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
@@ -407,13 +419,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		current_device = buf.st_dev;
 	}
 	for (;;) {
-		gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
-		if (gitfile_dir) {
-			if (set_git_dir(gitfile_dir))
-				die("Repository setup failed");
-			break;
-		}
-		if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
+		if (cwd_contains_git_dir(&gitfile_dir))
 			break;
 		if (is_git_directory(".")) {
 			inside_git_dir = 1;
-- 
1.7.2.rc3.593.g19611.dirty

Re: [PATCH 4/9] setup: split off a function to checks working dir for .git file

[Nguyen Thai Ngoc Duy]

2010/7/24 Jonathan Nieder <jrnieder@gmail.com>:
> The repository discovery procedure looks something like this:
>
>        while (same filesystem) {
>                check .git in working dir
>                check .
>                chdir(..)
>        }
>
> Add a function for the first step to make the actual code look a bit
> closer to that pseudocode.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  setup.c |   20 +++++++++++++-------
>  1 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 16bee6d..3d25d0f 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -348,6 +348,18 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
>        return retval;
>  }
>
> +static int cwd_contains_git_dir(const char **gitfile_dirp)
> +{
> +       const char *gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> +       *gitfile_dirp = gitfile_dir;
> +       if (gitfile_dir) {
> +               if (set_git_dir(gitfile_dir))
> +                       die("Repository setup failed");
> +               return 1;
> +       }
> +       return is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT);
> +}
> +
>  /*
>  * We cannot decide in this function whether we are in the work tree or
>  * not, since the config can only be read _after_ this function was called.
> @@ -407,13 +419,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
>                current_device = buf.st_dev;
>        }
>        for (;;) {
> -               gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
> -               if (gitfile_dir) {
> -                       if (set_git_dir(gitfile_dir))
> -                               die("Repository setup failed");
> -                       break;
> -               }
> -               if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
> +               if (cwd_contains_git_dir(&gitfile_dir))
>                        break;

Notice the two "break;" here, which is the only way to reach out the
code below the loop. Perhaps you should move that code into
cwd_contains_git_dir() too and change its name appropriately.
setup_git_directory_gently() would become "discovery only" as real
setup would be done by three new functions.

Something like this on top (won't even compile):

diff --git a/setup.c b/setup.c
index 8901149..45b9860 100644
--- a/setup.c
+++ b/setup.c
@@ -346,16 +346,36 @@ static const char *setup_explicit_git_dir(const
char *gitdirenv,
 	return retval;
 }

-static int cwd_contains_git_dir(const char **gitfile_dirp)
+static int cwd_contains_git_dir(const char **gitfile_dirp, char *cwd)
 {
 	const char *gitfile_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
 	*gitfile_dirp = gitfile_dir;
 	if (gitfile_dir) {
 		if (set_git_dir(gitfile_dir))
 			die("Repository setup failed");
-		return 1;
+		goto found;
 	}
-	return is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT);
+	if (is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT))
+		goto found;
+
+	return NULL;
+
+found:
+	inside_git_dir = 0;
+	if (!work_tree_env)
+		inside_work_tree = 1;
+	root_len = offset_1st_component(cwd);
+	git_work_tree_cfg = xstrndup(cwd, offset > root_len ? offset : root_len);
+	if (check_repository_format_gently(nongit_ok))
+		return NULL;
+	if (offset == len)
+		return NULL;
+
+	/* Make "offset" point to past the '/', and add a '/' at the end */
+	offset++;
+	cwd[len++] = '/';
+	cwd[len] = 0;
+	return cwd + offset;
 }

 /*
@@ -369,6 +389,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	static char cwd[PATH_MAX+1];
 	const char *gitdirenv;
 	const char *gitfile_dir;
+	const char *prefix;
 	int len, offset, ceil_offset, root_len;
 	dev_t current_device = 0;
 	int one_filesystem = 1;
@@ -417,8 +438,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		current_device = buf.st_dev;
 	}
 	for (;;) {
-		if (cwd_contains_git_dir(&gitfile_dir))
-			break;
+		if ((prefix = cwd_contains_git_dir(&gitfile_dir, cwd)))
+			return prefix;
+
 		if (is_git_directory(".")) {
 			inside_git_dir = 1;
 			if (!work_tree_env)
@@ -464,22 +486,6 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			die_errno("Cannot change to '%s/..'", cwd);
 		}
 	}
-
-	inside_git_dir = 0;
-	if (!work_tree_env)
-		inside_work_tree = 1;
-	root_len = offset_1st_component(cwd);
-	git_work_tree_cfg = xstrndup(cwd, offset > root_len ? offset : root_len);
-	if (check_repository_format_gently(nongit_ok))
-		return NULL;
-	if (offset == len)
-		return NULL;
-
-	/* Make "offset" point to past the '/', and add a '/' at the end */
-	offset++;
-	cwd[len++] = '/';
-	cwd[len] = 0;
-	return cwd + offset;
 }

 int git_config_perm(const char *var, const char *value)

-- 
Duy

Re: [PATCH 4/9] setup: split off a function to checks working dir for .git file

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:

> Notice the two "break;" here, which is the only way to reach out the
> code below the loop. Perhaps you should move that code into
> cwd_contains_git_dir() too and change its name appropriately.
> setup_git_directory_gently() would become "discovery only" as real
> setup would be done by three new functions.

Neat.

-- %< --
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Subject: setup: split off a function to handle ordinary .git directories

Finish the clean-up of setup_git_directory_gently() by splitting the
last case of validation+setup (global variables, prefix, check_format,
set_git_dir) into its own function.  Now setup_git_git_directory_gently
itself takes care of discovery only and the functions that pick up
from there are nearby in the source file so they can be easily
compared.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
diff --git i/setup.c w/setup.c
index 5734a1f..d19aa7d 100644
--- i/setup.c
+++ w/setup.c
@@ -360,6 +360,28 @@ static int cwd_contains_git_dir(const char **gitfile_dirp)
 	return is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT);
 }
 
+static const char *setup_discovered_git_dir(const char *work_tree_env,
+		int offset, int len, char *cwd, int *nongit_ok)
+{
+	int root_len;
+
+	inside_git_dir = 0;
+	if (!work_tree_env)
+		inside_work_tree = 1;
+	root_len = offset_1st_component(cwd);
+	git_work_tree_cfg = xstrndup(cwd, offset > root_len ? offset : root_len);
+	if (check_repository_format_gently(nongit_ok))
+		return NULL;
+	if (offset == len)
+		return NULL;
+
+	/* Make "offset" point to past the '/', and add a '/' at the end */
+	offset++;
+	cwd[len++] = '/';
+	cwd[len] = 0;
+	return cwd + offset;
+}
+
 static const char *setup_bare_git_dir(const char *work_tree_env,
 		int offset, int len, char *cwd, int *nongit_ok)
 {
@@ -411,7 +433,7 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	static char cwd[PATH_MAX+1];
 	const char *gitdirenv;
 	const char *gitfile_dir;
-	int len, offset, ceil_offset, root_len;
+	int len, offset, ceil_offset;
 	dev_t current_device = 0;
 	int one_filesystem = 1;
 
@@ -456,7 +478,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		current_device = get_device_or_die(".", NULL);
 	for (;;) {
 		if (cwd_contains_git_dir(&gitfile_dir))
-			break;
+			return setup_discovered_git_dir(work_tree_env, offset,
+							len, cwd, nongit_ok);
 		if (is_git_directory("."))
 			return setup_bare_git_dir(work_tree_env, offset,
 							len, cwd, nongit_ok);
@@ -482,22 +505,6 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			die_errno("Cannot change to '%s/..'", cwd);
 		}
 	}
-
-	inside_git_dir = 0;
-	if (!work_tree_env)
-		inside_work_tree = 1;
-	root_len = offset_1st_component(cwd);
-	git_work_tree_cfg = xstrndup(cwd, offset > root_len ? offset : root_len);
-	if (check_repository_format_gently(nongit_ok))
-		return NULL;
-	if (offset == len)
-		return NULL;
-
-	/* Make "offset" point to past the '/', and add a '/' at the end */
-	offset++;
-	cwd[len++] = '/';
-	cwd[len] = 0;
-	return cwd + offset;
 }
 
 int git_config_perm(const char *var, const char *value)
-- 

[PATCH 5/9] setup: split off code to handle stumbling upon a repository

[Jonathan Nieder]

If a repository is found as an ancestor of the original
working directory, it is assumed by default to be bare.
Handle this case with its own function.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 setup.c |   34 +++++++++++++++++++++-------------
 1 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/setup.c b/setup.c
index 3d25d0f..ec11c46 100644
--- a/setup.c
+++ b/setup.c
@@ -360,6 +360,24 @@ static int cwd_contains_git_dir(const char **gitfile_dirp)
 	return is_git_directory(DEFAULT_GIT_DIR_ENVIRONMENT);
 }
 
+static const char *setup_bare_git_dir(const char *work_tree_env,
+		int offset, int len, char *cwd, int *nongit_ok)
+{
+	int root_len;
+
+	inside_git_dir = 1;
+	if (!work_tree_env)
+		inside_work_tree = 0;
+	if (offset != len) {
+		root_len = offset_1st_component(cwd);
+		cwd[offset > root_len ? offset : root_len] = '\0';
+		set_git_dir(cwd);
+	} else
+		set_git_dir(".");
+	check_repository_format_gently(nongit_ok);
+	return NULL;
+}
+
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
@@ -421,19 +439,9 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	for (;;) {
 		if (cwd_contains_git_dir(&gitfile_dir))
 			break;
-		if (is_git_directory(".")) {
-			inside_git_dir = 1;
-			if (!work_tree_env)
-				inside_work_tree = 0;
-			if (offset != len) {
-				root_len = offset_1st_component(cwd);
-				cwd[offset > root_len ? offset : root_len] = '\0';
-				set_git_dir(cwd);
-			} else
-				set_git_dir(".");
-			check_repository_format_gently(nongit_ok);
-			return NULL;
-		}
+		if (is_git_directory("."))
+			return setup_bare_git_dir(work_tree_env, offset,
+							len, cwd, nongit_ok);
 		while (--offset > ceil_offset && cwd[offset] != '/');
 		if (offset <= ceil_offset) {
 			if (nongit_ok) {
-- 
1.7.2.rc3.593.g19611.dirty

[PATCH 6/9] setup: split off a function to handle hitting ceiling in repo search

[Jonathan Nieder]

Perhaps some day, other similar conditions (hitting the mount point,
hitting the root of the file system) will share this code.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 setup.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/setup.c b/setup.c
index ec11c46..9fc05e2 100644
--- a/setup.c
+++ b/setup.c
@@ -378,6 +378,16 @@ static const char *setup_bare_git_dir(const char *work_tree_env,
 	return NULL;
 }
 
+static const char *setup_nongit(const char *cwd, int *nongit_ok)
+{
+	if (!nongit_ok)
+		die("Not a git repository (or any of the parent directories): %s", DEFAULT_GIT_DIR_ENVIRONMENT);
+	if (chdir(cwd))
+		die_errno("Cannot come back to cwd");
+	*nongit_ok = 1;
+	return NULL;
+}
+
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
@@ -443,15 +453,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 			return setup_bare_git_dir(work_tree_env, offset,
 							len, cwd, nongit_ok);
 		while (--offset > ceil_offset && cwd[offset] != '/');
-		if (offset <= ceil_offset) {
-			if (nongit_ok) {
-				if (chdir(cwd))
-					die_errno("Cannot come back to cwd");
-				*nongit_ok = 1;
-				return NULL;
-			}
-			die("Not a git repository (or any of the parent directories): %s", DEFAULT_GIT_DIR_ENVIRONMENT);
-		}
+		if (offset <= ceil_offset)
+			return setup_nongit(cwd, nongit_ok);
 		if (one_filesystem) {
 			if (stat("..", &buf)) {
 				cwd[offset] = '\0';
-- 
1.7.2.rc3.593.g19611.dirty

[PATCH 7/9] setup: split off get_device_or_die helper

[Jonathan Nieder]

This does not eliminate any code, but it skims some off of
the main loop of setup_git_directory_gently so that can be
understood more easily.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 setup.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/setup.c b/setup.c
index 9fc05e2..5fb9c54 100644
--- a/setup.c
+++ b/setup.c
@@ -388,6 +388,16 @@ static const char *setup_nongit(const char *cwd, int *nongit_ok)
 	return NULL;
 }
 
+static dev_t get_device_or_die(const char *path, const char *prefix)
+{
+	struct stat buf;
+	if (stat(path, &buf))
+		die_errno("failed to stat '%s%s%s'",
+				prefix ? prefix : "",
+				prefix ? "/" : "", path);
+	return buf.st_dev;
+}
+
 /*
  * We cannot decide in this function whether we are in the work tree or
  * not, since the config can only be read _after_ this function was called.
@@ -402,7 +412,6 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	int len, offset, ceil_offset, root_len;
 	dev_t current_device = 0;
 	int one_filesystem = 1;
-	struct stat buf;
 
 	/*
 	 * Let's assume that we are in a git repository.
@@ -441,11 +450,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	 */
 	offset = len = strlen(cwd);
 	one_filesystem = !git_env_bool("GIT_DISCOVERY_ACROSS_FILESYSTEM", 0);
-	if (one_filesystem) {
-		if (stat(".", &buf))
-			die_errno("failed to stat '.'");
-		current_device = buf.st_dev;
-	}
+	if (one_filesystem)
+		current_device = get_device_or_die(".", NULL);
 	for (;;) {
 		if (cwd_contains_git_dir(&gitfile_dir))
 			break;
@@ -456,11 +462,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 		if (offset <= ceil_offset)
 			return setup_nongit(cwd, nongit_ok);
 		if (one_filesystem) {
-			if (stat("..", &buf)) {
-				cwd[offset] = '\0';
-				die_errno("failed to stat '%s/..'", cwd);
-			}
-			if (buf.st_dev != current_device) {
+			dev_t parent_device = get_device_or_die("..", cwd);
+			if (parent_device != current_device) {
 				if (nongit_ok) {
 					if (chdir(cwd))
 						die_errno("Cannot come back to cwd");
-- 
1.7.2.rc3.593.g19611.dirty

[PATCH 8/9] setup: do not forget working dir from subdir of gitdir

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

v1.6.1.3~4^2 (Fix gitdir detection when in subdir of gitdir,
2009-01-16) did not go far enough: when a git directory is
an ancestor of the original working directory, not only
should GIT_DIR be set to point to the .git directory, but
the original working directory should be restored before
carrying out the relevant command.

This way, the effect of running a git command from a subdir
of .git will be the same whether or not GIT_DIR is explicitly
set.

Noticed while investigating v1.6.0.3~1 (rehabilitate 'git
index-pack' inside the object store, 2008-10-20).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 setup.c                 |    2 ++
 t/t4111-apply-subdir.sh |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/setup.c b/setup.c
index 5fb9c54..5734a1f 100644
--- a/setup.c
+++ b/setup.c
@@ -369,6 +369,8 @@ static const char *setup_bare_git_dir(const char *work_tree_env,
 	if (!work_tree_env)
 		inside_work_tree = 0;
 	if (offset != len) {
+		if (chdir(cwd))
+			die_errno("Cannot come back to cwd");
 		root_len = offset_1st_component(cwd);
 		cwd[offset > root_len ? offset : root_len] = '\0';
 		set_git_dir(cwd);
diff --git a/t/t4111-apply-subdir.sh b/t/t4111-apply-subdir.sh
index 60c2237..57cae50 100755
--- a/t/t4111-apply-subdir.sh
+++ b/t/t4111-apply-subdir.sh
@@ -96,7 +96,7 @@ test_expect_success 'apply from .git dir' '
 	test_cmp expected .git/file
 '
 
-test_expect_failure 'apply from subdir of .git dir' '
+test_expect_success 'apply from subdir of .git dir' '
 	cp postimage expected &&
 	cp preimage .git/file &&
 	cp preimage .git/objects/file
-- 
1.7.2.rc3

[PATCH 9/9] Revert "rehabilitate 'git index-pack' inside the object store"

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Now setup_git_directory_gently behaves sanely even from subdirs of
.git, so simplify index-pack by no longer protecting against that.

This reverts commit a672ea6ac5a1b876bc7adfe6534b16fa2a32c94b
(excluding tests).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That’s the end of the series.  Thanks for reading.

Good night,
Jonathan

 builtin/index-pack.c |   24 +++++-------------------
 1 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index a89ae83..89a1f12 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -880,29 +880,15 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	char *index_name_buf = NULL, *keep_name_buf = NULL;
 	struct pack_idx_entry **idx_objects;
 	unsigned char pack_sha1[20];
+	int nongit;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage(index_pack_usage);
 
-	/*
-	 * We wish to read the repository's config file if any, and
-	 * for that it is necessary to call setup_git_directory_gently().
-	 * However if the cwd was inside .git/objects/pack/ then we need
-	 * to go back there or all the pack name arguments will be wrong.
-	 * And in that case we cannot rely on any prefix returned by
-	 * setup_git_directory_gently() either.
-	 */
-	{
-		char cwd[PATH_MAX+1];
-		int nongit;
-
-		if (!getcwd(cwd, sizeof(cwd)-1))
-			die("Unable to get current working directory");
-		setup_git_directory_gently(&nongit);
-		git_config(git_index_pack_config, NULL);
-		if (chdir(cwd))
-			die("Cannot come back to cwd");
-	}
+	prefix = setup_git_directory_gently(&nongit);
+	git_config(git_index_pack_config, NULL);
+	if (prefix && chdir(prefix))
+		die("Cannot come back to cwd");
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
-- 
1.7.2.rc3.593.g19611.dirty

Re: [PATCH 0/9] setup_git_directory(): return to original cwd upon reaching .git

[Nguyen Thai Ngoc Duy]

2010/7/24 Jonathan Nieder <jrnieder@gmail.com>:
> Hi Duy,
>
> Nguyễn Thái Ngọc Duy wrote:
>
>> Prefix and cwd should be consistent.
>
> I agree with this, but it took me a while to figure out why your fix
> is safe.  Here’s how I figured it out.
>
> Patch 1 cleans up the test script you added a test to.  After cleaning
> it up, it is clearer the test does not belong there.
>
> Patch 2 creates a proper home for the new test.

The test was originally to demostrate the breakage and apply was
chosen because it's the first command I found applicable. I doubt if
only "apply" (and index-pack) were affected.

But anyway, more tests can't harm git.

>
> Patches 3-7 split up setup_git_directory_gently() into small enough
> pieces that a person with a short attention span can read it now.
> No functional change intended.

Yeah. I tempted to clean up setup_*_gently too, by splitting the
discovery path ($GIT_DIR check, chdir up...) and the real setup (bunch
of global variables, prefix, check_format, set_git_dir) apart. The was
painful and did not improve readability as much. I gave up.

> Patch 8 is your fix.  I removed the comment (which was just confusing
> me) and clarified the commit message to compensate

If that makes it easier to understand, I'm OK.

> Patch 9 is your patch to revert the other, now redundant fix, also
> with commit message tweaks.
>
> After this exercise, your patches still look good. :)  Maybe these
> by-products could be useful somehow.
>
> Thoughts (especially improvements) welcome.
-- 
Duy