fixes for split index mode

repo_read_index calls read_index_from, which takes an path argument for the location of the index file. For the split index however it relies on the current working directory to construct the path using git_path.

repo_read_index calls read_index_from with the full path for the index file, however it doesn't change the cwd, so when split index mode is turned on, read_index_from can't find the file for the split index.

For example t3007-ls-files-recurse-submodules.sh was broken with GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also broken in a similar manner, probably by introducing struct repository there, although I didn't track down the exact commit for that.

Fix this by introducing a new read_index_for_repo function, which knows about the correct paths for the submodules.

The alternative would have been to make the callers pass in the base path for the split index, however that ended up being more complicated, and I think we want to converge towards using struct repository for things like these anyway.

Signed-off-by Thomas Gummerer <[email protected]>

---
 cache.h      |  1 +
 read-cache.c | 19 +++++++++++++++++--
 repository.c |  2 +-
 3 files changed, 19 insertions(+), 3 deletions(-)

cache.h +1 -0

index cb5db7bf83..d42bea1ef7 100644
@@ -614,6 +614,7 @@ extern int read_index_preload(struct index_state *, const struct pathspec *paths
 extern int do_read_index(struct index_state *istate, const char *path,
 			 int must_exist); /* for testting only! */
 extern int read_index_from(struct index_state *, const char *path);
+extern int read_index_for_repo(const struct repository *);
 extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);
 

read-cache.c +7 -2

index 2eb81a66b9..4d5c4ad79b 100644
@@ -20,6 +20,7 @@
 #include "split-index.h"
 #include "utf8.h"
 #include "fsmonitor.h"
+#include "repository.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 

@@ -1871,7 +1872,8 @@ static void freshen_shared_index(char *base_sha1_hex, int warn)
 	free(shared_index);
 }
 
-int read_index_from(struct index_state *istate, const char *path)
+static int do_read_index_from(struct index_state *istate, const char *path,
+			      const struct repository *repo)
 {
 	struct split_index *split_index;
 	int ret;

@@ -1896,7 +1898,10 @@ int read_index_from(struct index_state *istate, const char *path)
 		split_index->base = xcalloc(1, sizeof(*split_index->base));
 
 	base_sha1_hex = sha1_to_hex(split_index->base_sha1);
-	base_path = git_path("sharedindex.%s", base_sha1_hex);
+	if (repo)
+		base_path = repo_git_path(repo, "sharedindex.%s", base_sha1_hex);
+	else
+		base_path = git_path("sharedindex.%s", base_sha1_hex);
 	ret = do_read_index(split_index->base, base_path, 1);
 	if (hashcmp(split_index->base_sha1, split_index->base->sha1))
 		die("broken index, expect %s in %s, got %s",

@@ -1909,6 +1914,16 @@ int read_index_from(struct index_state *istate, const char *path)
 	return ret;
 }

read-cache.c +1 -0

+
 int is_index_unborn(struct index_state *istate)
 {
 	return (!istate->cache_nr && !istate->timestamp.sec);

repository.c +1 -1

index bb2fae5446..928b1f553d 100644
@@ -229,5 +229,5 @@ int repo_read_index(struct repository *repo)
 	if (!repo->index)
 		repo->index = xcalloc(1, sizeof(*repo->index));
 
-	return read_index_from(repo->index, repo->index_file);
+	return read_index_for_repo(repo);
 }

Signature · 1 line

2.15.1.504.g5279b80103

Quoted 17 lines · Thomas Gummerer

repo_read_index calls read_index_from, which takes an path argument for the location of the index file. For the split index however it relies on the current working directory to construct the path using git_path.

repo_read_index calls read_index_from with the full path for the index file, however it doesn't change the cwd, so when split index mode is turned on, read_index_from can't find the file for the split index.

For example t3007-ls-files-recurse-submodules.sh was broken with GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also broken in a similar manner, probably by introducing struct repository there, although I didn't track down the exact commit for that.

Fix this by introducing a new read_index_for_repo function, which knows about the correct paths for the submodules.

Quoted 4 lines

The alternative would have been to make the callers pass in the base path for the split index, however that ended up being more complicated, and I think we want to converge towards using struct repository for things like these anyway.

Thanks for catching this, I'm not a user of split index myself which is why I unfortunately overlooked this. Definitely a good change. I really only have one nit below.

Quoted 58 lines

Signed-off-by Thomas Gummerer <[email protected]>

---
 cache.h      |  1 +
 read-cache.c | 19 +++++++++++++++++--
 repository.c |  2 +-
 3 files changed, 19 insertions(+), 3 deletions(-)

cache.h +1 -0

index cb5db7bf83..d42bea1ef7 100644
@@ -614,6 +614,7 @@ extern int read_index_preload(struct index_state *, const struct pathspec *paths
 extern int do_read_index(struct index_state *istate, const char *path,
 			 int must_exist); /* for testting only! */
 extern int read_index_from(struct index_state *, const char *path);
+extern int read_index_for_repo(const struct repository *);
 extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);
 

read-cache.c +7 -2

index 2eb81a66b9..4d5c4ad79b 100644
@@ -20,6 +20,7 @@
 #include "split-index.h"
 #include "utf8.h"
 #include "fsmonitor.h"
+#include "repository.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 

@@ -1871,7 +1872,8 @@ static void freshen_shared_index(char *base_sha1_hex, int warn)
 	free(shared_index);
 }
 
-int read_index_from(struct index_state *istate, const char *path)
+static int do_read_index_from(struct index_state *istate, const char *path,
+			      const struct repository *repo)
 {
 	struct split_index *split_index;
 	int ret;

@@ -1896,7 +1898,10 @@ int read_index_from(struct index_state *istate, const char *path)
 		split_index->base = xcalloc(1, sizeof(*split_index->base));
 
 	base_sha1_hex = sha1_to_hex(split_index->base_sha1);
-	base_path = git_path("sharedindex.%s", base_sha1_hex);
+	if (repo)
+		base_path = repo_git_path(repo, "sharedindex.%s", base_sha1_hex);
+	else
+		base_path = git_path("sharedindex.%s", base_sha1_hex);
 	ret = do_read_index(split_index->base, base_path, 1);
 	if (hashcmp(split_index->base_sha1, split_index->base->sha1))
 		die("broken index, expect %s in %s, got %s",

@@ -1909,6 +1914,16 @@ int read_index_from(struct index_state *istate, const char *path)
 	return ret;
 }
 

read-cache.c int read_index_from(struct index_state *istate, const char *path) · line 1925

Quoted 9 lines

read-cache.c +9 -0

+int read_index_for_repo(const struct repository *repo)
+{
+	return do_read_index_from(repo->index, repo->index_file, repo);
+}
+
+int read_index_from(struct index_state *istate, const char *path)
+{
+	return do_read_index_from(istate, path, NULL);
+}

Instead of passing NULL and having to special case it in 'do_read_index_from()', how about we pass in 'the_repository'?

Quoted 18 lines

read-cache.c +1 -0

+
 int is_index_unborn(struct index_state *istate)
 {
 	return (!istate->cache_nr && !istate->timestamp.sec);

repository.c +1 -1

index bb2fae5446..928b1f553d 100644
@@ -229,5 +229,5 @@ int repo_read_index(struct repository *repo)
 	if (!repo->index)
 		repo->index = xcalloc(1, sizeof(*repo->index));
 
-	return read_index_from(repo->index, repo->index_file);
+	return read_index_for_repo(repo);
 }

Signature · 1 line

2.15.1.504.g5279b80103

Signature · 1 line

Brandon Williams

be489d02d2 ("revision.c: --indexed-objects add objects from all worktrees", 2017-08-23) made sure that pruning takes objects from all worktrees into account.

It did that by reading the index of every worktree and adding the necessary index objects to the set of pending objects. The index is read by read_index_from. As mentioned in the previous commit, read_index_from depends on the CWD for the location of the split index, and add_index_objects_to_pending doesn't set that before using read_index_from.

Instead of using read_index_from, use repo_read_index, which is aware of the proper paths for the worktree.

This fixes t5304-prune when ran with GIT_TEST_SPLIT_INDEX set.

Signed-off-by Thomas Gummerer <[email protected]>

---

This also fixes t7009 when ran with GIT_TEST_SPLIT_INDEX. I'm not quite sure why it is fixed by this. Either way I tracked the failure down to f767178a5a ("Merge branch 'jk/no-null-sha1-in-cache-tree'", 2017-05-16). Maybe Peff has an idea why this fixes that test?

 repository.c | 11 +++++++++++
 repository.h |  2 ++
 revision.c   | 13 ++++++++-----
 3 files changed, 21 insertions(+), 5 deletions(-)

repository.c +1 -0

index 928b1f553d..3c9bfbd1b8 100644
@@ -2,6 +2,7 @@
 #include "repository.h"
 #include "config.h"
 #include "submodule-config.h"
+#include "worktree.h"
 
 /* The main repository */
 static struct repository the_repo = {

@@ -146,6 +147,16 @@ int repo_init(struct repository *repo, const char *gitdir, const char *worktree)
 	return -1;
 }

repository.c +1 -0

+
 /*
  * Initialize 'submodule' as the submodule given by 'path' in parent repository
  * 'superproject'.

repository.h +2 -0

index 7f5e24a0a2..2adeb05bf4 100644
@@ -4,6 +4,7 @@
 struct config_set;
 struct index_state;
 struct submodule_cache;
+struct worktree;
 
 struct repository {
 	/* Environment */

@@ -87,6 +88,7 @@ extern struct repository *the_repository;
 extern void repo_set_gitdir(struct repository *repo, const char *path);
 extern void repo_set_worktree(struct repository *repo, const char *path);
 extern int repo_init(struct repository *repo, const char *gitdir, const char *worktree);
+extern int repo_worktree_init(struct repository *repo, struct worktree *worktree);
 extern int repo_submodule_init(struct repository *submodule,
 			       struct repository *superproject,
 			       const char *path);

revision.c +1 -0

index e2e691dd5a..9d8d9b96d1 100644
@@ -22,6 +22,7 @@
 #include "packfile.h"
 #include "worktree.h"
 #include "argv-array.h"
+#include "repository.h"
 
 volatile show_early_output_fn_t show_early_output;
 

revision.c +0 -0

 	}
 	free_worktrees(worktrees);
 }

Signature · 1 line

2.15.1.504.g5279b80103

Quoted 11 lines · Thomas Gummerer

be489d02d2 ("revision.c: --indexed-objects add objects from all worktrees", 2017-08-23) made sure that pruning takes objects from all worktrees into account.

It did that by reading the index of every worktree and adding the necessary index objects to the set of pending objects. The index is read by read_index_from. As mentioned in the previous commit, read_index_from depends on the CWD for the location of the split index, and add_index_objects_to_pending doesn't set that before using read_index_from.

Quoted 5 lines

Instead of using read_index_from, use repo_read_index, which is aware of the proper paths for the worktree.

This fixes t5304-prune when ran with GIT_TEST_SPLIT_INDEX set.

I'm on the fence about this change. I understand that this will ensure that the proper objects aren't pruned when using a split index in the presence of worktrees but I think the solution needs to be thought through a bit more.

My big concern right now is the interaction of 'struct worktree's and 'struct repository'. I'll try to highlight my concerns below.

Quoted 28 lines

Signed-off-by Thomas Gummerer <[email protected]>

---

This also fixes t7009 when ran with GIT_TEST_SPLIT_INDEX. I'm not quite sure why it is fixed by this. Either way I tracked the failure down to f767178a5a ("Merge branch 'jk/no-null-sha1-in-cache-tree'", 2017-05-16). Maybe Peff has an idea why this fixes that test?

 repository.c | 11 +++++++++++
 repository.h |  2 ++
 revision.c   | 13 ++++++++-----
 3 files changed, 21 insertions(+), 5 deletions(-)

repository.c +1 -0

index 928b1f553d..3c9bfbd1b8 100644
@@ -2,6 +2,7 @@
 #include "repository.h"
 #include "config.h"
 #include "submodule-config.h"
+#include "worktree.h"
 
 /* The main repository */
 static struct repository the_repo = {

@@ -146,6 +147,16 @@ int repo_init(struct repository *repo, const char *gitdir, const char *worktree)
 	return -1;
 }

repository.c int repo_init(struct repository *repo, const char *gitdir, const char *worktree) · line 158

Quoted 10 lines

repository.c +9 -0

 
+/*
+ * Initialize 'repo' based on the provided worktree
+ * Return 0 upon success and a non-zero value upon failure.
+ */
+int repo_worktree_init(struct repository *repo, struct worktree *worktree)
+{
+	return repo_init(repo, get_worktree_git_dir(worktree),
+			 worktree->path);
+}

My first concern is the use of 'get_worktree_git_dir()'. Under the hood it calls 'get_git_dir()', 'get_git_common_dir()', and 'git_common_path()' which rely on global state as stored in 'the_repository'. So how does one initialize a repository struct (using this initializer) using a worktree from a repository other than the global 'the_repository' struct? I'm not sure I have an answer right now, but its an issue that needs to be thought through before we head down this road.

Just thinking to myself, Does it make sense to have worktree's as a separate struct or to have them stored in 'struct repository' in some way?

Shouldn't a repository struct have a way to interact with all of its worktrees?

How would initializing a repository struct for every worktree work once we migrate the object store to be stored in 'struct repoisotry'? Shouldn't every worktree share the same object store in-memory like they do on-disk?

Quoted 43 lines

repository.c +1 -0

+
 /*
  * Initialize 'submodule' as the submodule given by 'path' in parent repository
  * 'superproject'.

repository.h +2 -0

index 7f5e24a0a2..2adeb05bf4 100644
@@ -4,6 +4,7 @@
 struct config_set;
 struct index_state;
 struct submodule_cache;
+struct worktree;
 
 struct repository {
 	/* Environment */

@@ -87,6 +88,7 @@ extern struct repository *the_repository;
 extern void repo_set_gitdir(struct repository *repo, const char *path);
 extern void repo_set_worktree(struct repository *repo, const char *path);
 extern int repo_init(struct repository *repo, const char *gitdir, const char *worktree);
+extern int repo_worktree_init(struct repository *repo, struct worktree *worktree);
 extern int repo_submodule_init(struct repository *submodule,
 			       struct repository *superproject,
 			       const char *path);

revision.c +2 -1

index e2e691dd5a..9d8d9b96d1 100644
@@ -22,6 +22,7 @@
 #include "packfile.h"
 #include "worktree.h"
 #include "argv-array.h"
+#include "repository.h"
 
 volatile show_early_output_fn_t show_early_output;
 

@@ -1346,15 +1347,17 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	worktrees = get_worktrees(0);
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
-		struct index_state istate = { NULL };
+		struct repository *repo;
 

revision.c void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags) · line 1352

Quoted 1 line

revision.c +1 -0

+		repo = xmalloc(sizeof(struct repository));

This was allocated but never freed, was that intentional?

Quoted 5 lines

revision.c +2 -0

 		if (wt->is_current)
 			continue; /* current index already taken care of */
+		if (repo_worktree_init(repo, wt))
+			BUG("couldn't initialize repository object from worktree");
 

revision.c void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags) · line 1360

Quoted 7 lines

revision.c +3 -4

-		if (read_index_from(&istate,
-				    worktree_git_path(wt, "index")) > 0)
-			do_add_index_objects_to_pending(revs, &istate);
-		discard_index(&istate);
+		if (repo_read_index(repo) > 0)
+			do_add_index_objects_to_pending(revs, repo->index);
+		discard_index(repo->index);

One we have separate object stores per-repository how would we handle this since this pruning should only work on a single repository's object store?

Quoted 6 lines

revision.c +0 -0

 	}
 	free_worktrees(worktrees);
 }

Signature · 1 line

2.15.1.504.g5279b80103

Signature · 1 line

Brandon Williams

Make sure that split index doesn't get broken, by running it on travis CI.

Run the test suite with split index enabled in linux 64 bit mode, and leave split index turned off in 32-bit mode. The laternative would be

to add an extra target in the matrix, enabling split index mode, but that would only use additional cycles on travis and would not bring that much benefit, as we are still running the test suite in the "vanilla" version in the 32-bit mode.

Signed-off-by Thomas Gummerer <[email protected]>

Make sure that split index doesn't get broken, by running it on travis CI.

Run the test suite with split index enabled in linux 64 bit mode, and leave split index turned off in 32-bit mode.

The laternative would be
to add an extra target in the matrix, enabling split index mode, but that would only use additional cycles on travis and would not bring that much benefit, as we are still running the test suite in the "vanilla" version in the 32-bit mode.

Signed-off-by Thomas Gummerer <[email protected]>

---
 .travis.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

.travis.yml +1 -1

index 281f101f31..c83c766dee 100644
@@ -39,7 +39,7 @@ env:
 
 matrix:
   include:
-    - env: GETTEXT_POISON=YesPlease
+    - env: GETTEXT_POISON=YesPlease GIT_TEST_SPLIT_INDEX=YesPlease
       os: linux
       compiler:
       addons:

Signature · 1 line

2.15.1.504.g5279b80103

Quoted 5 lines

Make sure that split index doesn't get broken, by running it on travis CI.

Run the test suite with split index enabled in linux 64 bit mode, and leave split index turned off in 32-bit mode.

This doesn't accurately describe what the patch does. Travis CI runs three 64 bit Linux build jobs for us: one compiled with Clang, one with GCC, and one with GETTEXT_POISON enabled. This patch enables split index only in the latter build job, but not in the Clang and GCC build jobs.

Quoted 26 lines

The laternative would be
to add an extra target in the matrix, enabling split index mode, but that would only use additional cycles on travis and would not bring that much benefit, as we are still running the test suite in the "vanilla" version in the 32-bit mode.

Signed-off-by Thomas Gummerer <[email protected]>

---
 .travis.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

.travis.yml +1 -1

index 281f101f31..c83c766dee 100644
@@ -39,7 +39,7 @@ env:
 
 matrix:
   include:
-    - env: GETTEXT_POISON=YesPlease
+    - env: GETTEXT_POISON=YesPlease GIT_TEST_SPLIT_INDEX=YesPlease
       os: linux
       compiler:
       addons:

Signature · 1 line

2.15.1.504.g5279b80103

Quoted 11 lines · SZEDER Gábor

Quoted 5 lines

Make sure that split index doesn't get broken, by running it on travis CI.

Run the test suite with split index enabled in linux 64 bit mode, and leave split index turned off in 32-bit mode.

This doesn't accurately describe what the patch does. Travis CI runs three 64 bit Linux build jobs for us: one compiled with Clang, one with GCC, and one with GETTEXT_POISON enabled. This patch enables split index only in the latter build job, but not in the Clang and GCC build jobs.

You're right, it's my first time using travis CI and I got confused about how the .travis.yml works, thanks for catching that. Will re-phrase the commit message.

Quoted 27 lines

Quoted 27 lines

The laternative would be
to add an extra target in the matrix, enabling split index mode, but that would only use additional cycles on travis and would not bring that much benefit, as we are still running the test suite in the "vanilla" version in the 32-bit mode.

Signed-off-by Thomas Gummerer <[email protected]>

---
.travis.yml | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

.travis.yml +0 -0

index 281f101f31..c83c766dee 100644
@@ -39,7 +39,7 @@ env:
 

matrix:
include:

.travis.yml +1 -1

-    - env: GETTEXT_POISON=YesPlease
+    - env: GETTEXT_POISON=YesPlease GIT_TEST_SPLIT_INDEX=YesPlease
      os: linux
      compiler:
      addons:

Signature · 1 line

2.15.1.504.g5279b80103

Quoted 3 lines

You're right, it's my first time using travis CI and I got confused about how the .travis.yml works, thanks for catching that. Will re-phrase the commit message.

Szeder is spot on. If you fix up the message, then this patch looks perfect! Pragmatic (= very good!) idea to combine GITTEXT_POISON with GIT_TEST_SPLIT_INDEX :-)

Quoted 7 lines · Lars Schneider

Quoted 3 lines

You're right, it's my first time using travis CI and I got confused about how the .travis.yml works, thanks for catching that. Will re-phrase the commit message.

Szeder is spot on. If you fix up the message, then this patch looks perfect! Pragmatic (= very good!) idea to combine GITTEXT_POISON with GIT_TEST_SPLIT_INDEX :-)

I am failing to guess the real intent of the smiley here.

If split-index code is so easy to break, I do not think it is a good idea to combine it into the poison build. In fact, the poison test is useless on a codebase where other/real breakages are expected to exist, because it is about seeing messages meant for non-humans are not passed to the _() mechanism by sloppy coding, and the way it does so is to corrupt all the messages that come through the _() mechanism. If we do not even produce a message when a correct code _should_ produce one, poison test would catch nothing useful.

I wonder if it makes more sense to update ci/run-tests.sh so that its final step is run twice with different settings, like so?

 ci/run-tests.sh | 1 +
 1 file changed, 1 insertion(+)

ci/run-tests.sh +1 -0

index f0c743de94..15a5f5a6cc 100755
@@ -8,3 +8,4 @@
 mkdir -p $HOME/travis-cache
 ln -s $HOME/travis-cache/.prove t/.prove
 make --quiet test
+GIT_TEST_SPLIT_INDEX=LetsTryIt make --quiet test

The breakages wen the split-index code fails tend to break things in much more obvious manners than a wrong message, usually git ends up dying if it gets broken. Both of the bugs that were fixed here would have been caught with the change in my patch.

But yeah I can see the argument that it doesn't give us a guarantee that it catches all things the test suite could catch.

Quoted 2 lines

I wonder if it makes more sense to update ci/run-tests.sh so that its final step is run twice with different settings, like so?

I kind of wanted to avoid that, because it ends up running the test suite twice on travis for every test, which seems a bit overkill. But I don't exactly how worried we are about cycles on travis (I don't check it very often personally, so I don't really know). If we aren't worried about cycles what you have below would certainly make sense.

Quoted 6 lines · Thomas Gummerer

The breakages wen the split-index code fails tend to break things in much more obvious manners than a wrong message, usually git ends up dying if it gets broken. Both of the bugs that were fixed here would have been caught with the change in my patch.

Quoted 2 lines

But yeah I can see the argument that it doesn't give us a guarantee that it catches all things the test suite could catch.

I think you misunderstood me. When split index is much easier to break than poison tests, combining them together would hurt the test coverage of poison tests. If you value poison tests much more than how well split index mode works, that is a worse outcome.

Quoted 8 lines

Quoted 2 lines

I wonder if it makes more sense to update ci/run-tests.sh so that its final step is run twice with different settings, like so?

I kind of wanted to avoid that, because it ends up running the test suite twice on travis for every test, which seems a bit overkill. But I don't exactly how worried we are about cycles on travis (I don't check it very often personally, so I don't really know). If we aren't worried about cycles what you have below would certainly make sense.

I think 64-bit gcc/clang builds tend to cost us about 10-20 minutes each, and 32-bit linux builds about 10 minutes or so. I wonder if it makes sense to do the "run twice" for only one of 64-bit builds (perhaps the clang one, as I suspect 32-bit linux one uses gcc) and the 32-bit linux build, and nowhere else.

Quoted 4 lines · Lars Schneider

Quoted 3 lines

You're right, it's my first time using travis CI and I got confused about how the .travis.yml works, thanks for catching that. Will re-phrase the commit message.

Quoted 3 lines

Szeder is spot on. If you fix up the message, then this patch looks perfect! Pragmatic (= very good!) idea to combine GITTEXT_POISON with GIT_TEST_SPLIT_INDEX :-)

I am failing to guess the real intent of the smiley here.

If split-index code is so easy to break, I do not think it is a good idea to combine it into the poison build. In fact, the poison test is useless on a codebase where other/real breakages are expected to exist, because it is about seeing messages meant for non-humans are not passed to the _() mechanism by sloppy coding, and the way it does so is to corrupt all the messages that come through the _() mechanism. If we do not even produce a message when a correct code _should_ produce one, poison test would catch nothing useful.

I wonder if it makes more sense to update ci/run-tests.sh so that its final step is run twice with different settings, like so?

ci/run-tests.sh | 1 +

1 file changed, 1 insertion(+)

ci/run-tests.sh +0 -0

index f0c743de94..15a5f5a6cc 100755
@@ -8,3 +8,4 @@

mkdir -p $HOME/travis-cache
ln -s $HOME/travis-cache/.prove t/.prove
make --quiet test
+GIT_TEST_SPLIT_INDEX=LetsTryIt make --quiet test

I think your solution points into the right direction. Right now we have the following test matrix:

1. Linux - clang
2. Linux - gcc
3. Mac - clang
4. Mac - gcc
5. Linux - gcc - GET_TEXT_POISION
6. Linux - gcc - 32bit
7. Windows

AFAIK your solution would run the split index test for 1, 2, 3, and 4. I think that is too much.

1 runs the fastest and I would like to keep it that way to get a quick "general" result. I think only 2 should be extended in the way you are suggesting. We could run the tests with different env variables there. What else do we have besides GIT_TEST_SPLIT_INDEX?

Would that work for everyone?

Quoted 11 lines · Lars Schneider

I think your solution points into the right direction. Right now we have the following test matrix:

1. Linux - clang
2. Linux - gcc
3. Mac - clang
4. Mac - gcc
5. Linux - gcc - GET_TEXT_POISION
6. Linux - gcc - 32bit
7. Windows

Quoted 2 lines

AFAIK your solution would run the split index test for 1, 2, 3, and 4. I think that is too much.

Not that it matters too much, but I meant to add it to 1. and
6. when I said "only one of 64-bit build plus 32-bit one".

Quoted 7 lines

1 runs the fastest and I would like to keep it that way to get a quick "general" result. I think only 2 should be extended in the way you are suggesting. We could run the tests with different env variables there. What else do we have besides GIT_TEST_SPLIT_INDEX?

Would that work for everyone?

I am OK to make 2. use split-index (which unfortunately would mean we lose tests without split-index under gcc), or add 2.5 that is a copy of 2. plus split-index.

Quoted 16 lines · Junio C Hamano

Quoted 11 lines · Lars Schneider

I think your solution points into the right direction. Right now we have the following test matrix:

1. Linux - clang
2. Linux - gcc
3. Mac - clang
4. Mac - gcc
5. Linux - gcc - GET_TEXT_POISION
6. Linux - gcc - 32bit
7. Windows

Quoted 5 lines

Quoted 2 lines

AFAIK your solution would run the split index test for 1, 2, 3, and 4. I think that is too much.

Not that it matters too much, but I meant to add it to 1. and
6. when I said "only one of 64-bit build plus 32-bit one".

Ah. Sorry, I didn't get that.

Quoted 8 lines

Quoted 7 lines

1 runs the fastest and I would like to keep it that way to get a quick "general" result. I think only 2 should be extended in the way you are suggesting. We could run the tests with different env variables there. What else do we have besides GIT_TEST_SPLIT_INDEX?

Would that work for everyone?

Quoted 3 lines

I am OK to make 2. use split-index (which unfortunately would mean we lose tests without split-index under gcc), or add 2.5 that is a copy of 2. plus split-index.

I think I experessed myself poorly. As far as I understand it, GIT_TEST_SPLIT_INDEX is an environment variable that only needs to be set at test time and not at compile time. Is this right?

If yes, my idea for 2. is as follows:
- build Git with gcc
- run tests with "make --quiet test"
- run tests with "GIT_TEST_SPLIT_INDEX=YesPlease make --quiet test"

This should be quicker than a new 2.5 target because we don't need to spin up the machine and build Git. Plus, we could run the tests a few more times with other test flags.

- Lars

repo_read_index calls read_index_from, which takes an path argument for the location of the index file. For the split index however it relies

on the current working directory to construct the path using git_path.

repo_read_index calls read_index_from with the full path for the index file, however it doesn't change the cwd, so when split index mode is turned on, read_index_from can't find the file for the split index.

For example t3007-ls-files-recurse-submodules.sh was broken with GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also broken in a similar manner, probably by introducing struct repository there, although I didn't track down the exact commit for that.

Fix this by introducing a new read_index_for_repo function, which knows about the correct paths for the submodules.

The alternative would have been to make the callers pass in the base path for the split index, however that ended up being more complicated, and I think we want to converge towards using struct repository for things like these anyway.

Signed-off-by Thomas Gummerer <[email protected]>

---
 cache.h      |  1 +
 read-cache.c | 16 ++++++++++++++--
 repository.c |  2 +-
 3 files changed, 16 insertions(+), 3 deletions(-)

cache.h +1 -0

index cb5db7bf83..d42bea1ef7 100644
@@ -614,6 +614,7 @@ extern int read_index_preload(struct index_state *, const struct pathspec *paths
 extern int do_read_index(struct index_state *istate, const char *path,
 			 int must_exist); /* for testting only! */
 extern int read_index_from(struct index_state *, const char *path);
+extern int read_index_for_repo(const struct repository *);
 extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);
 

read-cache.c +4 -2

index 2eb81a66b9..70357febdc 100644
@@ -20,6 +20,7 @@
 #include "split-index.h"
 #include "utf8.h"
 #include "fsmonitor.h"
+#include "repository.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 

@@ -1871,7 +1872,8 @@ static void freshen_shared_index(char *base_sha1_hex, int warn)
 	free(shared_index);
 }
 
-int read_index_from(struct index_state *istate, const char *path)
+static int do_read_index_from(struct index_state *istate, const char *path,
+			      const struct repository *repo)
 {
 	struct split_index *split_index;
 	int ret;

@@ -1896,7 +1898,7 @@ int read_index_from(struct index_state *istate, const char *path)
 		split_index->base = xcalloc(1, sizeof(*split_index->base));
 
 	base_sha1_hex = sha1_to_hex(split_index->base_sha1);
-	base_path = git_path("sharedindex.%s", base_sha1_hex);
+	base_path = repo_git_path(repo, "sharedindex.%s", base_sha1_hex);
 	ret = do_read_index(split_index->base, base_path, 1);
 	if (hashcmp(split_index->base_sha1, split_index->base->sha1))
 		die("broken index, expect %s in %s, got %s",

@@ -1909,6 +1911,16 @@ int read_index_from(struct index_state *istate, const char *path)
 	return ret;
 }
 

read-cache.c +1 -0

+
 int is_index_unborn(struct index_state *istate)
 {

read-cache.c +0 -0

 	return (!istate->cache_nr && !istate->timestamp.sec);

repository.c +1 -1

index bb2fae5446..928b1f553d 100644
@@ -229,5 +229,5 @@ int repo_read_index(struct repository *repo)
 	if (!repo->index)
 		repo->index = xcalloc(1, sizeof(*repo->index));
 
-	return read_index_from(repo->index, repo->index_file);
+	return read_index_for_repo(repo);
 }

Signature · 1 line

2.15.1.620.gb9897f4670

Quoted 4 lines · Thomas Gummerer

repo_read_index calls read_index_from, which takes an path argument for the location of the index file. For the split index however it relies

on the current working directory to construct the path using git_path.

This line isn't actually true and should probably be fixed. git_path doesn't rely on the CWD but rather it relies on the gitdir of the main repository (the_repository).

Quoted 77 lines

repo_read_index calls read_index_from with the full path for the index file, however it doesn't change the cwd, so when split index mode is turned on, read_index_from can't find the file for the split index.

For example t3007-ls-files-recurse-submodules.sh was broken with GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also broken in a similar manner, probably by introducing struct repository there, although I didn't track down the exact commit for that.

Fix this by introducing a new read_index_for_repo function, which knows about the correct paths for the submodules.

The alternative would have been to make the callers pass in the base path for the split index, however that ended up being more complicated, and I think we want to converge towards using struct repository for things like these anyway.

Signed-off-by Thomas Gummerer <[email protected]>

---
 cache.h      |  1 +
 read-cache.c | 16 ++++++++++++++--
 repository.c |  2 +-
 3 files changed, 16 insertions(+), 3 deletions(-)

cache.h +1 -0

index cb5db7bf83..d42bea1ef7 100644
@@ -614,6 +614,7 @@ extern int read_index_preload(struct index_state *, const struct pathspec *paths
 extern int do_read_index(struct index_state *istate, const char *path,
 			 int must_exist); /* for testting only! */
 extern int read_index_from(struct index_state *, const char *path);
+extern int read_index_for_repo(const struct repository *);
 extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);
 

read-cache.c +8 -2

index 2eb81a66b9..70357febdc 100644
@@ -20,6 +20,7 @@
 #include "split-index.h"
 #include "utf8.h"
 #include "fsmonitor.h"
+#include "repository.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 

@@ -1871,7 +1872,8 @@ static void freshen_shared_index(char *base_sha1_hex, int warn)
 	free(shared_index);
 }
 
-int read_index_from(struct index_state *istate, const char *path)
+static int do_read_index_from(struct index_state *istate, const char *path,
+			      const struct repository *repo)
 {
 	struct split_index *split_index;
 	int ret;

@@ -1896,7 +1898,7 @@ int read_index_from(struct index_state *istate, const char *path)
 		split_index->base = xcalloc(1, sizeof(*split_index->base));
 
 	base_sha1_hex = sha1_to_hex(split_index->base_sha1);
-	base_path = git_path("sharedindex.%s", base_sha1_hex);
+	base_path = repo_git_path(repo, "sharedindex.%s", base_sha1_hex);
 	ret = do_read_index(split_index->base, base_path, 1);
 	if (hashcmp(split_index->base_sha1, split_index->base->sha1))
 		die("broken index, expect %s in %s, got %s",

@@ -1909,6 +1911,16 @@ int read_index_from(struct index_state *istate, const char *path)
 	return ret;
 }
 
+int read_index_for_repo(const struct repository *repo)
+{
+	return do_read_index_from(repo->index, repo->index_file, repo);
+}

read-cache.c int read_index_from(struct index_state *istate, const char *path) · line 1922

Quoted 5 lines

read-cache.c +5 -0

+
+int read_index_from(struct index_state *istate, const char *path)
+{
+	return do_read_index_from(istate, path, the_repository);
+}

This looks fine, though I wonder what the point of passing in the index file even was since we end just ended up reading the 'sharedindex' file based on the git path. I'm just curious about how this function evolved.

Quoted 18 lines

read-cache.c +1 -0

+
 int is_index_unborn(struct index_state *istate)
 {
 	return (!istate->cache_nr && !istate->timestamp.sec);

repository.c +1 -1

index bb2fae5446..928b1f553d 100644
@@ -229,5 +229,5 @@ int repo_read_index(struct repository *repo)
 	if (!repo->index)
 		repo->index = xcalloc(1, sizeof(*repo->index));
 
-	return read_index_from(repo->index, repo->index_file);
+	return read_index_for_repo(repo);
 }

Signature · 1 line

2.15.1.620.gb9897f4670

Signature · 1 line

Brandon Williams

Quoted 9 lines · Brandon Williams

Quoted 4 lines · Thomas Gummerer

repo_read_index calls read_index_from, which takes an path argument for the location of the index file. For the split index however it relies

on the current working directory to construct the path using git_path.

This line isn't actually true and should probably be fixed. git_path doesn't rely on the CWD but rather it relies on the gitdir of the main repository (the_repository).

Right, let me fix that. Thanks!

Quoted 78 lines

Quoted 77 lines

repo_read_index calls read_index_from with the full path for the index file, however it doesn't change the cwd, so when split index mode is turned on, read_index_from can't find the file for the split index.

For example t3007-ls-files-recurse-submodules.sh was broken with GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also broken in a similar manner, probably by introducing struct repository there, although I didn't track down the exact commit for that.

Fix this by introducing a new read_index_for_repo function, which knows about the correct paths for the submodules.

The alternative would have been to make the callers pass in the base path for the split index, however that ended up being more complicated, and I think we want to converge towards using struct repository for things like these anyway.

Signed-off-by Thomas Gummerer <[email protected]>

---
 cache.h      |  1 +
 read-cache.c | 16 ++++++++++++++--
 repository.c |  2 +-
 3 files changed, 16 insertions(+), 3 deletions(-)

cache.h +1 -0

index cb5db7bf83..d42bea1ef7 100644
@@ -614,6 +614,7 @@ extern int read_index_preload(struct index_state *, const struct pathspec *paths
 extern int do_read_index(struct index_state *istate, const char *path,
 			 int must_exist); /* for testting only! */
 extern int read_index_from(struct index_state *, const char *path);
+extern int read_index_for_repo(const struct repository *);
 extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);
 

read-cache.c +8 -2

index 2eb81a66b9..70357febdc 100644
@@ -20,6 +20,7 @@
 #include "split-index.h"
 #include "utf8.h"
 #include "fsmonitor.h"
+#include "repository.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 

@@ -1871,7 +1872,8 @@ static void freshen_shared_index(char *base_sha1_hex, int warn)
 	free(shared_index);
 }
 
-int read_index_from(struct index_state *istate, const char *path)
+static int do_read_index_from(struct index_state *istate, const char *path,
+			      const struct repository *repo)
 {
 	struct split_index *split_index;
 	int ret;

@@ -1896,7 +1898,7 @@ int read_index_from(struct index_state *istate, const char *path)
 		split_index->base = xcalloc(1, sizeof(*split_index->base));
 
 	base_sha1_hex = sha1_to_hex(split_index->base_sha1);
-	base_path = git_path("sharedindex.%s", base_sha1_hex);
+	base_path = repo_git_path(repo, "sharedindex.%s", base_sha1_hex);
 	ret = do_read_index(split_index->base, base_path, 1);
 	if (hashcmp(split_index->base_sha1, split_index->base->sha1))
 		die("broken index, expect %s in %s, got %s",

@@ -1909,6 +1911,16 @@ int read_index_from(struct index_state *istate, const char *path)
 	return ret;
 }
 
+int read_index_for_repo(const struct repository *repo)
+{
+	return do_read_index_from(repo->index, repo->index_file, repo);
+}

Quoted 9 lines

read-cache.c int read_index_from(struct index_state *istate, const char *path) · line 1922

Quoted 5 lines

read-cache.c +5 -0

+
+int read_index_from(struct index_state *istate, const char *path)
+{
+	return do_read_index_from(istate, path, the_repository);
+}

This looks fine, though I wonder what the point of passing in the index file even was since we end just ended up reading the 'sharedindex' file based on the git path. I'm just curious about how this function evolved.

There are some callsites that are using an index different form $gitdir/index, or even GIT_INDEX_FILE. e.g. see builtin/am.c [*1*], which uses it's own 'patch-merge-index' in the am state directory for its internal operations.

The split index mode was later bolted on, and the sharedindex.xxxx would always go in $gitdir for the repository. Others probably know quite a bit more about this, while I'm always interested in index related things as that's how I got started with the git project, I couldn't follow all the conversations that were going on there.

*1*: https://github.com/gitster/git/blob/52015aaf9d19c97b52c47c7046058e6d029ff856/builtin/am.c#L1844

Quoted 22 lines

Quoted 19 lines

read-cache.c +1 -0

+
 int is_index_unborn(struct index_state *istate)
 {
 	return (!istate->cache_nr && !istate->timestamp.sec);

repository.c +1 -1

index bb2fae5446..928b1f553d 100644
@@ -229,5 +229,5 @@ int repo_read_index(struct repository *repo)
 	if (!repo->index)
 		repo->index = xcalloc(1, sizeof(*repo->index));
 
-	return read_index_from(repo->index, repo->index_file);
+	return read_index_for_repo(repo);
 }

Signature · 1 line

2.15.1.620.gb9897f4670

Signature · 1 line

Brandon Williams

be489d02d2 ("revision.c: --indexed-objects add objects from all worktrees", 2017-08-23) made sure that pruning takes objects from all worktrees into account.

It did that by reading the index of every worktree and adding the necessary index objects to the set of pending objects. The index is read by read_index_from. As mentioned in the previous commit, read_index_from depends on the CWD for the location of the split index,

and add_index_objects_to_pending doesn't set that before using read_index_from.

Instead of using read_index_from, use repo_read_index, which is aware of the proper paths for the worktree.

This fixes t5304-prune when ran with GIT_TEST_SPLIT_INDEX set.

Signed-off-by Thomas Gummerer <[email protected]>

---
 repository.c | 11 +++++++++++
 repository.h |  2 ++
 revision.c   | 14 +++++++++-----
 3 files changed, 22 insertions(+), 5 deletions(-)

repository.c +1 -0

index 928b1f553d..3c9bfbd1b8 100644
@@ -2,6 +2,7 @@
 #include "repository.h"
 #include "config.h"
 #include "submodule-config.h"
+#include "worktree.h"
 
 /* The main repository */
 static struct repository the_repo = {

@@ -146,6 +147,16 @@ int repo_init(struct repository *repo, const char *gitdir, const char *worktree)
 	return -1;

repository.c +2 -0

+}
+
 /*
  * Initialize 'submodule' as the submodule given by 'path' in parent repository
  * 'superproject'.

repository.h +2 -0

index 7f5e24a0a2..2adeb05bf4 100644
@@ -4,6 +4,7 @@
 struct config_set;
 struct index_state;
 struct submodule_cache;
+struct worktree;
 
 struct repository {
 	/* Environment */

@@ -87,6 +88,7 @@ extern struct repository *the_repository;
 extern void repo_set_gitdir(struct repository *repo, const char *path);
 extern void repo_set_worktree(struct repository *repo, const char *path);
 extern int repo_init(struct repository *repo, const char *gitdir, const char *worktree);
+extern int repo_worktree_init(struct repository *repo, struct worktree *worktree);
 extern int repo_submodule_init(struct repository *submodule,
 			       struct repository *superproject,
 			       const char *path);

revision.c +1 -1

index e2e691dd5a..34e1e4b799 100644
@@ -22,6 +22,7 @@
 #include "packfile.h"
 #include "worktree.h"
 #include "argv-array.h"
+#include "repository.h"
 
 volatile show_early_output_fn_t show_early_output;
 

@@ -1346,15 +1347,18 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	worktrees = get_worktrees(0);
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
-		struct index_state istate = { NULL };

Quoted 4 lines · Thomas Gummerer

be489d02d2 ("revision.c: --indexed-objects add objects from all worktrees", 2017-08-23) made sure that pruning takes objects from all worktrees into account.

Quoted 4 lines

It did that by reading the index of every worktree and adding the necessary index objects to the set of pending objects. The index is read by read_index_from. As mentioned in the previous commit, read_index_from depends on the CWD for the location of the split index,

As I mentioned before this doesn't actually depend on the CWD but rather the per-worktree gitdir.

Quoted 29 lines

and add_index_objects_to_pending doesn't set that before using read_index_from.

Instead of using read_index_from, use repo_read_index, which is aware of the proper paths for the worktree.

This fixes t5304-prune when ran with GIT_TEST_SPLIT_INDEX set.

Signed-off-by Thomas Gummerer <[email protected]>

---
 repository.c | 11 +++++++++++
 repository.h |  2 ++
 revision.c   | 14 +++++++++-----
 3 files changed, 22 insertions(+), 5 deletions(-)

repository.c +1 -0

index 928b1f553d..3c9bfbd1b8 100644
@@ -2,6 +2,7 @@
 #include "repository.h"
 #include "config.h"
 #include "submodule-config.h"
+#include "worktree.h"
 
 /* The main repository */
 static struct repository the_repo = {

@@ -146,6 +147,16 @@ int repo_init(struct repository *repo, const char *gitdir, const char *worktree)
 	return -1;

repository.c int repo_init(struct repository *repo, const char *gitdir, const char *worktree) · line 157

Quoted 10 lines

repository.c +8 -0

 }
 
+/*
+ * Initialize 'repo' based on the provided worktree
+ * Return 0 upon success and a non-zero value upon failure.
+ */
+int repo_worktree_init(struct repository *repo, struct worktree *worktree)
+{
+	return repo_init(repo, get_worktree_git_dir(worktree),
+			 worktree->path);

I still feel very unsettled about this and don't think its a good idea. get_worktree_git_dir depends implicitly on the global the_repository object and I would like to avoid relying on it for an initialization function like this.

Quoted 50 lines

repository.c +2 -0

+}
+
 /*
  * Initialize 'submodule' as the submodule given by 'path' in parent repository
  * 'superproject'.

repository.h +2 -0

index 7f5e24a0a2..2adeb05bf4 100644
@@ -4,6 +4,7 @@
 struct config_set;
 struct index_state;
 struct submodule_cache;
+struct worktree;
 
 struct repository {
 	/* Environment */

@@ -87,6 +88,7 @@ extern struct repository *the_repository;
 extern void repo_set_gitdir(struct repository *repo, const char *path);
 extern void repo_set_worktree(struct repository *repo, const char *path);
 extern int repo_init(struct repository *repo, const char *gitdir, const char *worktree);
+extern int repo_worktree_init(struct repository *repo, struct worktree *worktree);
 extern int repo_submodule_init(struct repository *submodule,
 			       struct repository *superproject,
 			       const char *path);

revision.c +5 -1

index e2e691dd5a..34e1e4b799 100644
@@ -22,6 +22,7 @@
 #include "packfile.h"
 #include "worktree.h"
 #include "argv-array.h"
+#include "repository.h"
 
 volatile show_early_output_fn_t show_early_output;
 

@@ -1346,15 +1347,18 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 	worktrees = get_worktrees(0);
 	for (p = worktrees; *p; p++) {
 		struct worktree *wt = *p;
-		struct index_state istate = { NULL };
+		struct repository *repo;
 
+		repo = xmalloc(sizeof(struct repository));
 		if (wt->is_current)
 			continue; /* current index already taken care of */
+		if (repo_worktree_init(repo, wt))
+			BUG("couldn't initialize repository object from worktree");
 

revision.c void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags) · line 1355

Quoted 2 lines

revision.c +0 -2

-		if (read_index_from(&istate,
-				    worktree_git_path(wt, "index")) > 0)

Ok, after thinking this through a bit more I think a better approach may be to restructure the call to read_index_from to take in both an index file as well as the explicit gitdir to use when constructing a path to the sharedindex file. That way you can fix this for worktrees and submodules without having to pass in a repository object to the logic which is reading an index file as well as avoiding needing to init a repository object for every worktree.

So you could rework the first patch to do something like:

read-cache.c +7 -7

index 2eb81a66b..3a04b5567 100644
@@ -1863,20 +1863,19 @@ int do_read_index(struct index_state *istate, const char *path, int must_exist)
  * This way, shared index can be removed if they have not been used
  * for some time.
  */
-static void freshen_shared_index(char *base_sha1_hex, int warn)
+static void freshen_shared_index(const char *shared_index, int warn)
 {
-	char *shared_index = git_pathdup("sharedindex.%s", base_sha1_hex);
 	if (!check_and_freshen_file(shared_index, 1) && warn)
 		warning("could not freshen shared index '%s'", shared_index);
-	free(shared_index);
 }
 
-int read_index_from(struct index_state *istate, const char *path)
+int read_index_from(struct index_state *istate, const char *path,
+		    const char *gitdir)
 {
 	struct split_index *split_index;
 	int ret;
 	char *base_sha1_hex;
-	const char *base_path;
+	char *base_path;
 
 	/* istate->initialized covers both .git/index and .git/sharedindex.xxx */
 	if (istate->initialized)

@@ -1896,14 +1895,14 @@ int read_index_from(struct index_state *istate, const char *path)
 		split_index->base = xcalloc(1, sizeof(*split_index->base));
 
 	base_sha1_hex = sha1_to_hex(split_index->base_sha1);
-	base_path = git_path("sharedindex.%s", base_sha1_hex);
+	base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_sha1_hex);
 	ret = do_read_index(split_index->base, base_path, 1);
 	if (hashcmp(split_index->base_sha1, split_index->base->sha1))
 		die("broken index, expect %s in %s, got %s",
 		    base_sha1_hex, base_path,
 		    sha1_to_hex(split_index->base->sha1));
 
-	freshen_shared_index(base_sha1_hex, 0);
+	freshen_shared_index(base_path, 0);
 	merge_base_index(istate);
 	post_read_index_from(istate);
+ free(base_path);
 	return ret;

Signature · 2 lines

-- 
Brandon Williams

[sorry for the late reply. I was on Christmas holidays until today and am still catching up on the mailing list. It will probably take me untill the weekend to send a re-roll]

Quoted 10 lines · Brandon Williams

Quoted 8 lines · Thomas Gummerer

be489d02d2 ("revision.c: --indexed-objects add objects from all worktrees", 2017-08-23) made sure that pruning takes objects from all worktrees into account.

It did that by reading the index of every worktree and adding the necessary index objects to the set of pending objects. The index is read by read_index_from. As mentioned in the previous commit, read_index_from depends on the CWD for the location of the split index,

Quoted 2 lines

As I mentioned before this doesn't actually depend on the CWD but rather the per-worktree gitdir.

Right, will fix.

Split index mode only has a few dedicated tests, but as the index is involved in nearly every git operation, this doesn't quite cover all the ways repositories with split index can break. To use split index mode throughout the test suite a GIT_TEST_SPLIT_INDEX environment variable can be set, which makes git split the index at random and thus excercises the functionality much more thoroughly.

As this is not turned on by default, it is not executed nearly as often as the test suite is run, so occationally breakages slip through. Try to counteract that by running the test suite with GIT_TEST_SPLIT_INDEX mode turned on on travis.

To avoid using too many cycles on travis only run split index mode in the linux-gcc and the linux 32-bit gcc targets.

The Linux builds were
chosen over the Mac OS builds because they tend to be much faster to complete.

The linux gcc build was chosen over the linux clang build because the linux clang build is the fastest build, so it can serve as an early indicator if something is broken and we want to avoid spending the extra cycles of running the test suite twice for that.

Helped-by Lars Schneider <[email protected]>

Helped-by Junio C Hamano <[email protected]>

Signed-off-by Thomas Gummerer <[email protected]>

---
ci/run-linux32-build.sh | 1 +
ci/run-tests.sh | 4 ++++

2 files changed, 5 insertions(+)

ci/run-linux32-build.sh +1 -0

index e30fb2cddc..f173c9cf2a 100755
@@ -27,4 +27,5 @@ linux32 --32bit i386 su -m -l $CI_USER -c '
    cd /usr/src/git &&
    make --jobs=2 &&
    make --quiet test
+    GIT_TEST_SPLIT_INDEX=YesPlease make --quiet test

'

ci/run-tests.sh +0 -0

index f0c743de94..c7aee5b9ff 100755
@@ -8,3 +8,7 @@

mkdir -p $HOME/travis-cache
ln -s $HOME/travis-cache/.prove t/.prove
make --quiet test

Quoted 15 lines · Thomas Gummerer

Split index mode only has a few dedicated tests, but as the index is involved in nearly every git operation, this doesn't quite cover all the ways repositories with split index can break. To use split index mode throughout the test suite a GIT_TEST_SPLIT_INDEX environment variable can be set, which makes git split the index at random and thus excercises the functionality much more thoroughly.

As this is not turned on by default, it is not executed nearly as often as the test suite is run, so occationally breakages slip through. Try to counteract that by running the test suite with GIT_TEST_SPLIT_INDEX mode turned on on travis.

Quoted 2 lines

To avoid using too many cycles on travis only run split index mode in the linux-gcc and the linux 32-bit gcc targets.

I am surprised to see the split index mode test for the linux 32-bit target. Is it likely that a split index bug appears only on 32-bit? Wouldn't the linux-gcc target be sufficient to save resources/time?

Quoted 36 lines

The Linux builds were
chosen over the Mac OS builds because they tend to be much faster to complete.

The linux gcc build was chosen over the linux clang build because the linux clang build is the fastest build, so it can serve as an early indicator if something is broken and we want to avoid spending the extra cycles of running the test suite twice for that.

Helped-by Lars Schneider <[email protected]>

Helped-by Junio C Hamano <[email protected]>

Signed-off-by Thomas Gummerer <[email protected]>

---
ci/run-linux32-build.sh | 1 +
ci/run-tests.sh | 4 ++++

2 files changed, 5 insertions(+)

ci/run-linux32-build.sh +1 -0

index e30fb2cddc..f173c9cf2a 100755
@@ -27,4 +27,5 @@ linux32 --32bit i386 su -m -l $CI_USER -c '
    cd /usr/src/git &&
    make --jobs=2 &&
    make --quiet test
+    GIT_TEST_SPLIT_INDEX=YesPlease make --quiet test

'

ci/run-tests.sh +0 -0

index f0c743de94..c7aee5b9ff 100755
@@ -8,3 +8,7 @@

mkdir -p $HOME/travis-cache
ln -s $HOME/travis-cache/.prove t/.prove
make --quiet test
+if test "$jobname" = "linux-gcc"

ci/run-tests.sh line 14

Quoted 3 lines

ci/run-tests.sh +3 -0

+then
+	GIT_TEST_SPLIT_INDEX=YesPlease make --quiet test
+fi

For now I think that looks good. Maybe we could define additional test configurations with an environment variable. That could be an array variable defined in the lib-travis.ci "case" statement:
https://github.com/git/git/blob/1229713f78cd2883798e95f33c19c81b523413fd/ci/lib-travisci.sh#L42-L65

- Lars

Quoted 18 lines · Lars Schneider

Quoted 17 lines · Thomas Gummerer

Split index mode only has a few dedicated tests, but as the index is involved in nearly every git operation, this doesn't quite cover all the ways repositories with split index can break. To use split index mode throughout the test suite a GIT_TEST_SPLIT_INDEX environment variable can be set, which makes git split the index at random and thus excercises the functionality much more thoroughly.

As this is not turned on by default, it is not executed nearly as often as the test suite is run, so occationally breakages slip through. Try to counteract that by running the test suite with GIT_TEST_SPLIT_INDEX mode turned on on travis.

To avoid using too many cycles on travis only run split index mode in the linux-gcc and the linux 32-bit gcc targets.

Quoted 3 lines

I am surprised to see the split index mode test for the linux 32-bit target. Is it likely that a split index bug appears only on 32-bit? Wouldn't the linux-gcc target be sufficient to save resources/time?

I'm not sure it's particularly likely for a bug to appear only on 32-bit builds. It also doesn't seem to take too long to run, so I thought I'd add it just in case, but I'm happy running the tests only in the 64-bit builds if that's preferred.

Quoted 5 lines · Lars Schneider

[snip]
For now I think that looks good. Maybe we could define additional test configurations with an environment variable. That could be an array variable defined in the lib-travis.ci "case" statement:
https://github.com/git/git/blob/1229713f78cd2883798e95f33c19c81b523413fd/ci/lib-travisci.sh#L42-L65

That sounds like a good idea. I'll try to see if I can come up with something.

Quoted 1 line

- Lars

~/w/git/temp $ git grep git_path read-cache.c

read-cache.c int read_index_from(struct index_state *istate, const char *path) · line 1898

Quoted 6 lines · Thomas Gummerer

read-cache.c +1 -1

@@ -1896,16 +1895,17 @@ int read_index_from(struct index_state *istate, const char *path)
                split_index->base = xcalloc(1, sizeof(*split_index->base));
 
        base_sha1_hex = sha1_to_hex(split_index->base_sha1);
-       base_path = git_path("sharedindex.%s", base_sha1_hex);
+       base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_sha1_hex);

Personally I prefer the repo_git_path() from v2 (sorry I was away and could not comment anything).

The thing is, git_path() and friends
could do some path translation underneath to support multiple worktrees. Think of the given path here as a "virtual path" that may be translated to something else, not exactly <git_dir> + "/" + "sharedindex.%s". But in practice, we're not breaking the relationship between $GIT_DIR/index and $GIT_DIR/sharedindex.* any time soon, doing manual path transformation here is fine.

What about the other git_path() in this file? With patch applied I still get

Quoted 1 line

~/w/git/temp $ git grep git_path read-cache.c

read-cache.c:           shared_index_path = git_path("%s", de->d_name);
read-cache.c:   temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
read-cache.c:                         git_path("sharedindex.%s",

sha1_to_hex(si->base->sha1)));
read-cache.c: const char *shared_index = git_path("sharedindex.%s",

I suppose submodule has not triggered any of these code paths yet. Not sure if we should deal with them now or wait until later.

Perhaps if we add a "struct repository *" pointer inside index_state, we could retrieve back the_repository (or others) and call repo_git_path() everywhere without changing index api too much. I don't know. I like the 'struct repository' concept but couldn't follow its development so I don't if this is what it should become.

Signature · 1 line

Duy

Quoted 10 lines · Duy Nguyen

read-cache.c int read_index_from(struct index_state *istate, const char *path) · line 1898

Quoted 6 lines · Thomas Gummerer

read-cache.c +1 -1

@@ -1896,16 +1895,17 @@ int read_index_from(struct index_state *istate, const char *path)
                split_index->base = xcalloc(1, sizeof(*split_index->base));
 
        base_sha1_hex = sha1_to_hex(split_index->base_sha1);
-       base_path = git_path("sharedindex.%s", base_sha1_hex);
+       base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_sha1_hex);

Personally I prefer the repo_git_path() from v2 (sorry I was away and could not comment anything).

It felt slightly nicer to me as well, but it threw up a bunch of questions about how worktrees will fit together with struct repository. As I don't feel very strongly about this either way I decided to go with what Brandon suggested as an alternative, which allows us to defer that decision. I'd be happy to revert this to the way I had it in v2, but I don't want to drag the discussion on too long either, as this series does fix some real regressions.

Quoted 8 lines

The thing is, git_path() and friends
could do some path translation underneath to support multiple worktrees. Think of the given path here as a "virtual path" that may be translated to something else, not exactly <git_dir> + "/" + "sharedindex.%s". But in practice, we're not breaking the relationship between $GIT_DIR/index and $GIT_DIR/sharedindex.* any time soon, doing manual path transformation here is fine.

Quoted 8 lines

What about the other git_path() in this file? With patch applied I still get

Quoted 1 line

~/w/git/temp $ git grep git_path read-cache.c

read-cache.c:           shared_index_path = git_path("%s", de->d_name);
read-cache.c:   temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
read-cache.c:                         git_path("sharedindex.%s",

sha1_to_hex(si->base->sha1)));
read-cache.c: const char *shared_index = git_path("sharedindex.%s",

Good point, I hadn't looked at these, I only looked at the current test failures. I'm going to be away for the rest of the week, but I'll have a look at them when I come back.

Quoted 2 lines

I suppose submodule has not triggered any of these code paths yet. Not sure if we should deal with them now or wait until later.

Quoted 5 lines

Perhaps if we add a "struct repository *" pointer inside index_state, we could retrieve back the_repository (or others) and call repo_git_path() everywhere without changing index api too much. I don't know. I like the 'struct repository' concept but couldn't follow its development so I don't if this is what it should become.

Interesting. I didn't follow the development of struct repository too closely either, so I'm not sure. Brandon might have more of an opinion on that? :)

Quoted 2 lines

Signature · 1 line

Duy

Quoted 3 lines · Thomas Gummerer

read-cache.c +0 -0

@@ -1896,16 +1895,17 @@ int read_index_from(struct index_state *istate, const char *path)
                split_index->base = xcalloc(1, sizeof(*split_index->base));
 

read-cache.c int read_index_from(struct index_state *istate, const char *path) · line 1898

Quoted 3 lines

read-cache.c +1 -1

        base_sha1_hex = sha1_to_hex(split_index->base_sha1);
-       base_path = git_path("sharedindex.%s", base_sha1_hex);
+       base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_sha1_hex);

Personally I prefer the repo_git_path() from v2 (sorry I was away and could not comment anything). The thing is, git_path() and friends could do some path translation underneath to support multiple worktrees. Think of the given path here as a "virtual path" that may be translated to something else, not exactly <git_dir> + "/" + "sharedindex.%s". But in practice, we're not breaking the relationship between $GIT_DIR/index and $GIT_DIR/sharedindex.* any time soon, doing manual path transformation here is fine.

What about the other git_path() in this file? With patch applied I still get

Quoted 1 line

~/w/git/temp $ git grep git_path read-cache.c

read-cache.c:           shared_index_path = git_path("%s", de->d_name);
read-cache.c:   temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
read-cache.c:                         git_path("sharedindex.%s",

sha1_to_hex(si->base->sha1)));
read-cache.c: const char *shared_index = git_path("sharedindex.%s",

I suppose submodule has not triggered any of these code paths yet. Not sure if we should deal with them now or wait until later.

Perhaps if we add a "struct repository *" pointer inside index_state, we could retrieve back the_repository (or others) and call repo_git_path() everywhere without changing index api too much. I don't know. I like the 'struct repository' concept but couldn't follow its development so I don't if this is what it should become.

Quoted 4 lines · Duy Nguyen

Quoted 3 lines · Thomas Gummerer

read-cache.c +0 -0

@@ -1896,16 +1895,17 @@ int read_index_from(struct index_state *istate, const char *path)
                split_index->base = xcalloc(1, sizeof(*split_index->base));
 

Quoted 12 lines

read-cache.c int read_index_from(struct index_state *istate, const char *path) · line 1898

Quoted 3 lines

read-cache.c +1 -1

        base_sha1_hex = sha1_to_hex(split_index->base_sha1);
-       base_path = git_path("sharedindex.%s", base_sha1_hex);
+       base_path = xstrfmt("%s/sharedindex.%s", gitdir, base_sha1_hex);

Personally I prefer the repo_git_path() from v2 (sorry I was away and could not comment anything). The thing is, git_path() and friends could do some path translation underneath to support multiple worktrees. Think of the given path here as a "virtual path" that may be translated to something else, not exactly <git_dir> + "/" + "sharedindex.%s". But in practice, we're not breaking the relationship between $GIT_DIR/index and $GIT_DIR/sharedindex.* any time soon, doing manual path transformation here is fine.

My biggest complaint about v2 is that we still don't quite know the best way to integrate worktrees and struct repository yet so I was very reluctant to start having them interact in the way v2 was using them together.

I'm very much in favor of this version (v3) as each worktree can explicitly provide their gitdir to be used to determine where to read the shared index file without having to replicate a struct repository for each.

Quoted 9 lines

What about the other git_path() in this file? With patch applied I still get

Quoted 1 line

~/w/git/temp $ git grep git_path read-cache.c

read-cache.c:           shared_index_path = git_path("%s", de->d_name);
read-cache.c:   temp = mks_tempfile(git_path("sharedindex_XXXXXX"));
read-cache.c:                         git_path("sharedindex.%s",

sha1_to_hex(si->base->sha1)));
read-cache.c: const char *shared_index = git_path("sharedindex.%s",

Quoted 8 lines

I suppose submodule has not triggered any of these code paths yet. Not sure if we should deal with them now or wait until later.

Perhaps if we add a "struct repository *" pointer inside index_state, we could retrieve back the_repository (or others) and call repo_git_path() everywhere without changing index api too much. I don't know. I like the 'struct repository' concept but couldn't follow its development so I don't if this is what it should become.

I'm not too keen on having an index_state struct contain a back pointer to a repository struct. I do think that we may want to have worktree structs contain a back pointer to the struct repository they correspond to. That way there is only one instance of the repository (and object-store once that gets integrated) yet multiple worktrees.

read_index_from() takes a path argument for the location of the index file. For reading the shared index in split index mode however it just ignores that path argument, and reads it from the gitdir of the current repository.

This works as long as an index in the_repository is read. Once that changes, such as when we read the index of a submodule, or of a different working tree than the current one, the gitdir of the_repository will no longer contain the appropriate shared index, and git will fail to read it.

For example t3007-ls-files-recurse-submodules.sh was broken with GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also broken in a similar manner, probably by introducing struct repository there, although I didn't track down the exact commit for that.

be489d02d2 ("revision.c: --indexed-objects add objects from all worktrees", 2017-08-23) breaks with split index mode in a similar manner, not erroring out when it can't read the index, but instead carrying on with pruning, without taking the index of the worktree into account.

Fix this by passing an additional gitdir parameter to read_index_from, to indicate where it should look for and read the shared index from.

read_cache_from() defaults to using the gitdir of the_repository. As it is mostly a convenience macro, having to pass get_git_dir() for every call seems overkill, and if necessary users can have more control by using read_index_from().

Quoted 23 lines · Thomas Gummerer

read_index_from() takes a path argument for the location of the index file. For reading the shared index in split index mode however it just ignores that path argument, and reads it from the gitdir of the current repository.

This works as long as an index in the_repository is read. Once that changes, such as when we read the index of a submodule, or of a different working tree than the current one, the gitdir of the_repository will no longer contain the appropriate shared index, and git will fail to read it.

For example t3007-ls-files-recurse-submodules.sh was broken with GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also broken in a similar manner, probably by introducing struct repository there, although I didn't track down the exact commit for that.

be489d02d2 ("revision.c: --indexed-objects add objects from all worktrees", 2017-08-23) breaks with split index mode in a similar manner, not erroring out when it can't read the index, but instead carrying on with pruning, without taking the index of the worktree into account.

Quoted 7 lines

Fix this by passing an additional gitdir parameter to read_index_from, to indicate where it should look for and read the shared index from.

read_cache_from() defaults to using the gitdir of the_repository. As it is mostly a convenience macro, having to pass get_git_dir() for every call seems overkill, and if necessary users can have more control by using read_index_from().

I'm not saying we need to change this again but I got to thinking about what the root cause for this bug is and I think it's a design flaw in how split index is implemented. IIUC Split index is an index extension that can be enabled to limit the size of the index file that is written when making changes to the index. It breaks the index into two pieces, index (which contains only changes) and sharedindex.XXXXX (which contains unchanged information) where 'XXXXX' is a value found in the index file. If we don't do anything fancy then these two files live next to one another in a repository's git directory at $GIT_DIR/index and $GIT_DIR/sharedindex.XXXXX. This seems to work all well and fine except that this isn't always the case and the read_index_from function takes this into account by enabling a caller to specify a path to where the index file is located. We can do this by specifying the index file we want to use by setting GIT_INDEX_FILE.

Being able to specify an index file via the environment is a feature that has existed for a very long time (one that I personally think makes things difficult because of things like additions like the sharedindex) and I don't think was taken into account when introducing the split index extension.

In this case if i were to specify a location of an
index file in my home directory '~/index' and be using the split index feature then the corresponding sharedindex file would live in my repository's git directory '~/project/.git/sharedindex.XXXXX'. So the sharedindex file is always located relative to the project's git directory and not the index file itself, which is kind of confusing. Maybe a better design would be to have the sharedindex file located relative to the index file.

Anyway, some food for thought.

--
Brandon Williams

Quoted 47 lines · Brandon Williams

Quoted 23 lines · Thomas Gummerer

read_index_from() takes a path argument for the location of the index file. For reading the shared index in split index mode however it just ignores that path argument, and reads it from the gitdir of the current repository.

This works as long as an index in the_repository is read. Once that changes, such as when we read the index of a submodule, or of a different working tree than the current one, the gitdir of the_repository will no longer contain the appropriate shared index, and git will fail to read it.

For example t3007-ls-files-recurse-submodules.sh was broken with GIT_TEST_SPLIT_INDEX set in 188dce131f ("ls-files: use repository object", 2017-06-22), and t7814-grep-recurse-submodules.sh was also broken in a similar manner, probably by introducing struct repository there, although I didn't track down the exact commit for that.

be489d02d2 ("revision.c: --indexed-objects add objects from all worktrees", 2017-08-23) breaks with split index mode in a similar manner, not erroring out when it can't read the index, but instead carrying on with pruning, without taking the index of the worktree into account.

Quoted 7 lines

Fix this by passing an additional gitdir parameter to read_index_from, to indicate where it should look for and read the shared index from.

read_cache_from() defaults to using the gitdir of the_repository. As it is mostly a convenience macro, having to pass get_git_dir() for every call seems overkill, and if necessary users can have more control by using read_index_from().

I'm not saying we need to change this again but I got to thinking about what the root cause for this bug is and I think it's a design flaw in how split index is implemented. IIUC Split index is an index extension that can be enabled to limit the size of the index file that is written when making changes to the index. It breaks the index into two pieces, index (which contains only changes) and sharedindex.XXXXX (which contains unchanged information) where 'XXXXX' is a value found in the index file. If we don't do anything fancy then these two files live next to one another in a repository's git directory at $GIT_DIR/index and $GIT_DIR/sharedindex.XXXXX. This seems to work all well and fine except that this isn't always the case and the read_index_from function takes this into account by enabling a caller to specify a path to where the index file is located. We can do this by specifying the index file we want to use by setting GIT_INDEX_FILE.

Quoted 5 lines

Being able to specify an index file via the environment is a feature that has existed for a very long time (one that I personally think makes things difficult because of things like additions like the sharedindex) and I don't think was taken into account when introducing the split index extension.

It was (partly because I did use GIT_INDEX_FILE on occasions).

Quoted 8 lines

In this case if i were to specify a location of an
index file in my home directory '~/index' and be using the split index feature then the corresponding sharedindex file would live in my repository's git directory '~/project/.git/sharedindex.XXXXX'. So the sharedindex file is always located relative to the project's git directory and not the index file itself, which is kind of confusing. Maybe a better design would be to have the sharedindex file located relative to the index file.

That adds more problems. Now when you move the index file around you have to move the shared index file too (think about atomic rename which we use in plenty of places, we can't achieve that by moving two files). A new dependency to $GIT_DIR is not that confusing to me, the index file is useless anyway if you don't have access to $GIT_DIR/objects. There was always the option to _not_ split the index when $GIT_INDEX_FILE is specified, I think I did consider that but I dropped it because we'd lose the performance gain by splitting.

Quoted 4 lines

Anyway, some food for thought.

--
Brandon Williams

Signature · 1 line

Duy

Quoted 12 lines · Brandon Williams

IIUC Split index is an index extension
that can be enabled to limit the size of the index file that is written when making changes to the index. It breaks the index into two pieces, index (which contains only changes) and sharedindex.XXXXX (which contains unchanged information) where 'XXXXX' is a value found in the index file. If we don't do anything fancy then these two files live next to one another in a repository's git directory at $GIT_DIR/index and $GIT_DIR/sharedindex.XXXXX. This seems to work all well and fine except that this isn't always the case and the read_index_from function takes this into account by enabling a caller to specify a path to where the index file is located. We can do this by specifying the index file we want to use by setting GIT_INDEX_FILE.

Quoted 8 lines

In this case if i were to specify a location of an
index file in my home directory '~/index' and be using the split index feature then the corresponding sharedindex file would live in my repository's git directory '~/project/.git/sharedindex.XXXXX'. So the sharedindex file is always located relative to the project's git directory and not the index file itself, which is kind of confusing. Maybe a better design would be to have the sharedindex file located relative to the index file.

That adds more problems. Now when you move the index file around you have to move the shared index file too (think about atomic rename which we use in plenty of places, we can't achieve that by moving two files). A new dependency to $GIT_DIR is not that confusing to me, the index file is useless anyway if you don't have access to $GIT_DIR/objects. There was always the option to _not_ split the index when $GIT_INDEX_FILE is specified, I think I did consider that but I dropped it because we'd lose the performance gain by splitting.

Hi,

Quoted 14 lines · Duy Nguyen

Quoted 12 lines · Brandon Williams

IIUC Split index is an index extension
that can be enabled to limit the size of the index file that is written when making changes to the index. It breaks the index into two pieces, index (which contains only changes) and sharedindex.XXXXX (which contains unchanged information) where 'XXXXX' is a value found in the index file. If we don't do anything fancy then these two files live next to one another in a repository's git directory at $GIT_DIR/index and $GIT_DIR/sharedindex.XXXXX. This seems to work all well and fine except that this isn't always the case and the read_index_from function takes this into account by enabling a caller to specify a path to where the index file is located. We can do this by specifying the index file we want to use by setting GIT_INDEX_FILE.

[...]

Quoted 9 lines

Quoted 8 lines

In this case if i were to specify a location of an
index file in my home directory '~/index' and be using the split index feature then the corresponding sharedindex file would live in my repository's git directory '~/project/.git/sharedindex.XXXXX'. So the sharedindex file is always located relative to the project's git directory and not the index file itself, which is kind of confusing. Maybe a better design would be to have the sharedindex file located relative to the index file.

Quoted 8 lines

That adds more problems. Now when you move the index file around you have to move the shared index file too (think about atomic rename which we use in plenty of places, we can't achieve that by moving two files). A new dependency to $GIT_DIR is not that confusing to me, the index file is useless anyway if you don't have access to $GIT_DIR/objects. There was always the option to _not_ split the index when $GIT_INDEX_FILE is specified, I think I did consider that but I dropped it because we'd lose the performance gain by splitting.

Can you elaborate a little more on this?

At first glance, it seems simpler to say "paths in index extensions named in the index file are relative to the location of the index file" and to make moving the index file also require moving the shared index file, exactly as you say. So at least from a "principle of least surprise" perspective I would be tempted to go that way.

It's true that we rely on atomic rename in plenty of places, but only within a directory. (Filesystem boundaries, NFS, etc mean that atomic renames across directories are a lost cause.)

Fortunately index files (including temp index files used by scripts) tend to only be in $GIT_DIR, for exactly that reason. So I am wondering if switching to index-file-relative semantics would be an invasive move and what the pros and cons of such a move are.

read_cache_from() defaults to using the gitdir of the_repository. As it is mostly a convenience macro, having to pass get_git_dir() for every call seems overkill, and if necessary users can have more control by using read_index_from().

Quoted 4 lines · Thomas Gummerer

read_cache_from() defaults to using the gitdir of the_repository. As it is mostly a convenience macro, having to pass get_git_dir() for every call seems overkill, and if necessary users can have more control by using read_index_from().

This was a bit painful change, given that some changes in flight do add new callsites to read_index_from() and they got the function changed under their feet.

Please double check if I made the right git-dir to be passed to them when I push out 'pu' in a few hours.

Thanks.

Quoted 7 lines · Junio C Hamano

Quoted 4 lines · Thomas Gummerer

read_cache_from() defaults to using the gitdir of the_repository. As it is mostly a convenience macro, having to pass get_git_dir() for every call seems overkill, and if necessary users can have more control by using read_index_from().

Quoted 3 lines

This was a bit painful change, given that some changes in flight do add new callsites to read_index_from() and they got the function changed under their feet.

Sorry about that. Is there any way to make such a change less painful in the future?

Quoted 12 lines · Thomas Gummerer

Quoted 10 lines · Junio C Hamano

Quoted 4 lines · Thomas Gummerer

read_cache_from() defaults to using the gitdir of the_repository. As it is mostly a convenience macro, having to pass get_git_dir() for every call seems overkill, and if necessary users can have more control by using read_index_from().

This was a bit painful change, given that some changes in flight do add new callsites to read_index_from() and they got the function changed under their feet.

Quoted 2 lines

Sorry about that. Is there any way to make such a change less painful in the future?

One way is to do for read_index_from() what you did for the existing users of read_cache_from(). Introduce a _new_ helper that will not be known for any existing topics in flight, and use that to make the existing API a thin wrapper around it.

I _think_ I got it right with evil merge, so unless this fix needs to be delayed for extended period of time for whatever reason while any more new callers of the function appears (which is unlikely), we should be OK ;-)

Thanks.

Quoted 6 lines

Quoted 3 lines

This was a bit painful change, given that some changes in flight do add new callsites to read_index_from() and they got the function changed under their feet.

Sorry about that. Is there any way to make such a change less painful in the future?

One way is to do for read_index_from() what you did for the existing users of read_cache_from(). Introduce a _new_ helper that will not be known for any existing topics in flight, and use that to make the existing API a thin wrapper around it.

In a0a967568e ("update-index --split-index: do not split if $GIT_DIR is read only", 2014-06-13), we tried to make sure we can still write an index, even if the shared index can not be written.

We did so by just calling 'do_write_locked_index()' from 'write_shared_index()'. 'do_write_locked_index()' always at least closes the tempfile nowadays, and used to close or commit the lockfile if COMMIT_LOCK or CLOSE_LOCK were given at the time this feature was introduced. COMMIT_LOCK or CLOSE_LOCK is passed in by most callers of 'write_locked_index()'.

After calling 'write_shared_index()', we call 'write_split_index()', which calls 'do_write_locked_index()' again, which then tries to use the closed lockfile again, but in fact fails to do so as it's already closed.

In the current version, git will in fact segfault if it can't create a new file in $gitdir, and this feature seems to never have worked in the first place.

Ever since introducing the split index feature, nobody has complained about this failing, and it really just papers over repositories that will sooner or later need fixing anyway.

Therefore just make being unable to write the split index a proper error, and have users fix their repositories instead of trying (but failing) to paper over the error.

Signed-off-by Thomas Gummerer <[email protected]>

---
 read-cache.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

read-cache.c +0 -2

index d13ce83794..a9c8facdfd 100644
@@ -2471,18 +2471,15 @@ static int clean_shared_index_files(const char *current_hex)
        return 0;
 }
 
-static int write_shared_index(struct index_state *istate,
-                             struct lock_file *lock, unsigned flags)

This series, queued as 'nd/shared-index-fix', makes the 32 bit Linux build job fail on Travis CI. Unfortunately, all it can tell us about the failure is this:

Test Summary Report
-------------------
t1700-split-index.sh (Wstat: 256 Tests: 23
Failed: 1)
Failed test: 23
Non-zero exit status: 1
Files=809, Tests=18491, 401 wallclock secs ( 7.22 usr 1.60 sys + 263.16 cusr 49.58 csys = 321.56 CPU)
Result: FAIL

because it can't access the test's verbose log due to lack of permissions.

https://travis-ci.org/git/git/jobs/329681826#L2074