Bug with git worktrees and git init

Hi,

I've been using multiple worktrees for months without issue (it's a great feature, thanks), until recently when I wanted to add hooks to them. So, when I added a template for the hooks, everything was fine until I did a git reset --hard in the original repo which both applied those changes to the other worktrees' working tree (the files on disk), and made my master branch kinda lose it's connection to the remote/think it was a kinda bare repo.

To reproduce this:

```
mkdir source-repo
cd source-repo
git init
touch foo
git add foo
git commit -m 'Add foo'
git worktree add ../worktree # which also creates a new branch 'worktree'
touch bar
git add bar
git commit -m 'Add bar'
cd ../worktree
git init
cd ../source-repo
git reset --hard master
cd ../worktree
git status # Suddenly `bar` has appear the working tree and is not tracked
```

I don't really now what is up with this, but I did notice that it is the last worktree in which git init has been run that is affected. I only ran git init to copy the hooks from the template, but if that is not something you should do in a worktree then a check would have been nice.

Thanks for this awesome tool, and I hope this helps Max Nordlund

Max Nordlund venit, vidit, dixit 23.08.2016 14:35:

This is where the problem is created already: git init does not quite notice that it is in a linked worktree. It treats

source-repo/.git/worktrees/worktree

as a proper gitdir and creates the full gitdir structure in there (branches, hooks, info) which is usually shared among worktrees; also, it adds the config setting

core.worktree = <abspath>/worktree

to the main config (source-repo/.git/config) which is why "git status" thinks that bar is missing - it indeed is in that worktree.

I've cc'ed the master of worktrees.

Thanks for the analysis. I'll provide some patch as soon as possible. This git-init may also be a good place to repair broken worktrees too (e.g. because you moved a worktree manually)..

PS. Master? Wheeee I'm a master!! :D

Duy

My ASAP is not so ASAP. Sorry about that but I think I have fixed it.

Side note about 2/3. I've known this problem (in general) for years (accidentally reading .git config file before .git is searched) and could not do anything about it. And because test_expect_failure should only be there if someone will eventually fix it, and I don't see myself doing it, and I don't see it super important that other people would bother, so I decided to simply sweep it under the rug. I could make a test_expect_failure if people think otherwise.

Side note about 3/3. Yes there's a FIXME in there. It will take a lot more time to remove that FIXME (because setup_git_directory is not as simple). For now it should be ok to leave it there. When we find a use for get_first_git_dir outside git-init, we can fix it then.

Nguyễn Thái Ngọc Duy (3):
init: correct re-initialization from a linked worktree
t0001: work around the bug that reads config file before repo setup
init: do not set core.worktree more often than necessary

 builtin/init-db.c |  4 ++--
 cache.h           |  1 +
 environment.c     | 16 +++++++++++++++-
 t/t0001-init.sh   | 19 +++++++++++++++++++
 4 files changed, 37 insertions(+), 3 deletions(-)

2.8.2.524.g6ff3d78

When 'git init' is called from a linked worktree, '.git' dir as the main '.git' (i.e. $GIT_COMMON_DIR) and populate the whole repository skeleton in there. It does not harm anything (*) but it is still wrong.

Since 'git init' calls set_git_dir() at preparation time, which indirectly calls get_common_dir() and correctly detects multiple worktree setup, all git_path_buf() calls in create_default_files() will return correct paths in both single and multiple worktree setups. The only thing left is copy_templates(), which targets $GIT_DIR, not $GIT_COMMON_DIR.

Fix that with get_git_common_dir(). This function will return $GIT_DIR in single-worktree setup, so we don't have to make a special case for multiple-worktree here.

(*) It does in fact, thanks to another bug. More on that later.

---
 builtin/init-db.c |  2 +-
 t/t0001-init.sh   | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

index 3a45f0b..6d9552e 100644
@@ -138,7 +138,7 @@ static void copy_templates(const char *template_dir)
 		goto close_free_return;
 	}
 
-	strbuf_addstr(&path, get_git_dir());
+	strbuf_addstr(&path, get_git_common_dir());
 	strbuf_complete(&path, '/');
 	copy_templates_1(&path, &template_path, dir);
 close_free_return:

index a6fdd5e..d64e5e3 100755
@@ -384,4 +384,19 @@ test_expect_success MINGW 'bare git dir not hidden' '
 	! is_hidden newdir
 '
 
+test_expect_success 're-init from a linked worktree' '
+	git init main-worktree &&
+	(
+		cd main-worktree &&
+		test_commit first &&
+		git worktree add ../linked-worktree &&
+		mv .git/info/exclude expected-exclude &&
+		find .git/worktrees -print | sort >expected &&
+		git -C ../linked-worktree init &&
+		test_cmp expected-exclude .git/info/exclude &&
+		find .git/worktrees -print | sort >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_done

2.8.2.524.g6ff3d78

-ECANNOTPARSE. Did you mean "... worktree, we treat '.git' dir as if it is the main '.git' ..." or something entirely different?

Yep I accidentally a couple words there. Will add them back.

Duy

git-init somehow reads '.git/config' at current directory and sets log_all_ref_updates based on this file. Because log_all_ref_updates is not unspecified (-1) any more. It will not be written to the new repo's config file (see create_default_files() function).

This will affect our tests in the next patch as we will compare the config file and expect that core.logallrefupdates is already set to true by "git init main-worktree".

---
 t/t0001-init.sh | 2 ++
 1 file changed, 2 insertions(+)

index d64e5e3..393c940 100755
@@ -385,7 +385,9 @@ test_expect_success MINGW 'bare git dir not hidden' '
 '
 
 test_expect_success 're-init from a linked worktree' '
+	mv .git/config work-around-init-reading-wrong-file &&
 	git init main-worktree &&
+	mv work-around-init-reading-wrong-file .git/config &&
 	(
 		cd main-worktree &&
 		test_commit first &&

2.8.2.524.g6ff3d78

I vaguely recall this was discussed recently somewhere. Is this related to <[email protected]>?

Ugh. The "mv" workaround is fine to demonstrate that you have fixed one of two problems, but as you said, it would be nice to have another that expects failure until both of them gets fixed.

This is a bug for more than worktrees, and is something I'm working on fixing (what I'd like to do is kill off the lazy fallback to ".git/" as the repo name, which is almost always the wrong thing to do).

I'm not opposed to your workaround, but just FYI.

-Peff

Great! test_expect_failure it is. But I'll make a separate patch, independent from this series though.

Duy

If you're curious what the fix looks like, it's in:

https://github.com/peff/git jk/config-repo-setup

The actual fix is in the final patch, but it needed a lot of preparatory work to avoid breaking various programs that made bad assumptions (and in the process, I uncovered a ton of other minor bugs).

This is just a preview in case you're interested, for two reasons:

1. I literally _just_ put the finishing touches on it, and it's extensive and tricky enough that I really should give it one more proofread.

2. There may be other related fallouts from the bug related to running "git init /path/to/foo" when "/path/to/foo" already exists (and in that case we _do_ want to read its config, but not the config from an existing repository). This may all just work fine, but I need to think about some tests.

-Peff

I looked over the series and it all seems like solid improvements to me, and I agree with the reasoning and logic. It's nice to also read very descriptive commit messages that clearly explain each improvement.

Regards,
Jake

When "git init" is called with GIT_WORK_TREE environment set, we want to keep this worktree's location in core.worktree so the user does not have to set the environment again and again. See ef6f0af (git-init: set core.worktree if GIT_WORK_TREE is specified - 2007-07-04)

We detect that by this logic (in needs_work_tree_config): normally worktree's top dir would contains ".git" directory, if this is not true, worktree is probably set to elsewhere by the user.

Unfortunately when it calls get_git_dir() it does not take ".git" files into account. When we find a .git file, we immediately follow the file until we find the real ".git" directory. The location of this first ".git" file is lost.

The .git file would satisfy the logic above and not create core.worktree (correct). But because the final .git's location is used, needs_work_tree_config() is misled and creates core.worktree anyway.

This would not be a huge deal normally. But if this happens in a multiple worktree setup it becomes a real problem because up until now, core.worktree will be applied to the main worktree only. If you accidentally do "git init" from a linked worktree, you set core.worktree (for the main repo) pointing to the _linked_ worktree. After that point, may you live in interesting times.

Record the .git file location and use it here.

---
 builtin/init-db.c |  2 +-
 cache.h           |  1 +
 environment.c     | 16 +++++++++++++++-
 t/t0001-init.sh   |  2 ++
 4 files changed, 19 insertions(+), 2 deletions(-)

index 6d9552e..36255f2 100644
@@ -254,7 +254,7 @@ static int create_default_files(const char *template_path)
 		/* allow template config file to override the default */
 		if (log_all_ref_updates == -1)
 			git_config_set("core.logallrefupdates", "true");
-		if (needs_work_tree_config(get_git_dir(), work_tree))
+		if (needs_work_tree_config(get_first_git_dir(), work_tree))
 			git_config_set("core.worktree", work_tree);
 	}
 

index b780a91..e6c05f8 100644
@@ -460,6 +460,7 @@ extern char *git_work_tree_cfg;
 extern int is_inside_work_tree(void);
 extern const char *get_git_dir(void);
 extern const char *get_git_common_dir(void);
+extern const char *get_first_git_dir(void);
 extern char *get_object_directory(void);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);

index ca72464..8cfb8f3 100644
@@ -100,7 +100,7 @@ static char *work_tree;
 static const char *namespace;
 static size_t namespace_len;
 
-static const char *git_dir, *git_common_dir;
+static const char *git_dir, *git_common_dir, *first_git_dir;
 static char *git_object_dir, *git_index_file, *git_graft_file;
 int git_db_env, git_index_env, git_graft_env, git_common_dir_env;
 

@@ -168,6 +168,8 @@ static void setup_git_env(void)
 	if (!git_dir)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
 	gitfile = read_gitfile(git_dir);
+	if (gitfile && !first_git_dir)
+		first_git_dir = xstrdup(git_dir);
 	git_dir = xstrdup(gitfile ? gitfile : git_dir);
 	if (get_common_dir(&sb, git_dir))
 		git_common_dir_env = 1;

@@ -203,6 +205,18 @@ const char *get_git_dir(void)
 	return git_dir;
 }
 
+/*
+ * Return the first ".git" that we have encountered.
+ * FIXME this function for not entirely correct because
+ * setup_git_directory() and enter_repo() do not update first_git_dir
+ * when they follow .git files. The function in its current state is
+ * only suitable for "git init".
+ */
+const char *get_first_git_dir(void)
+{
+	return first_git_dir ? first_git_dir : git_dir;
+}
+
 const char *get_git_common_dir(void)
 {
 	return git_common_dir;

index 393c940..d59669a 100755
@@ -393,9 +393,11 @@ test_expect_success 're-init from a linked worktree' '
 		test_commit first &&
 		git worktree add ../linked-worktree &&
 		mv .git/info/exclude expected-exclude &&
+		cp .git/config expected-config &&
 		find .git/worktrees -print | sort >expected &&
 		git -C ../linked-worktree init &&
 		test_cmp expected-exclude .git/info/exclude &&
+		test_cmp expected-config .git/config &&
 		find .git/worktrees -print | sort >actual &&
 		test_cmp expected actual
 	)

2.8.2.524.g6ff3d78

Would it be possible to move this to "init-db.c" then?

The very first thing cmd_init_db() does to what is in the environment.c is to call set_git_dir() via set_git_dir_init() to tell it where the ".git" thing is, no? Can't that code remember the location itself, instead of adding code that is known not to be usable by other callers? That would help avoiding the future confusion.

Good idea. I was fixated on read_gitfile()m it didn't occur to me.

Duy

v2 requires jk/setup-sequence-update so I could kill my workaround patch and avoid conflicts in t0001. And:

- 1/3 has a few missing words back in its commit message
- 2/3, which was 3/3 in v1, no longer has the ugly hacky get_first_git_dir()
- 3/3 is a new tiny code improvement after the new 2/3

Nguyễn Thái Ngọc Duy (3):
init: correct re-initialization from a linked worktree
init: do not set core.worktree more often than necessary
init: reuse original_git_dir in set_git_dir_init()

 builtin/init-db.c | 11 +++++++----
 t/t0001-init.sh   | 17 +++++++++++++++++
 2 files changed, 24 insertions(+), 4 deletions(-)

2.8.2.524.g6ff3d78

When 'git init' is called from a linked worktree, we treat '.git' dir (which is $GIT_COMMON_DIR/worktrees/something) as the main '.git' (i.e. $GIT_COMMON_DIR) and populate the whole repository skeleton in there. It does not harm anything (*) but it is still wrong.

Since 'git init' calls set_git_dir() at preparation time, which indirectly calls get_common_dir() and correctly detects multiple worktree setup, all git_path_buf() calls in create_default_files() will return correct paths in both single and multiple worktree setups. The only thing left is copy_templates(), which targets $GIT_DIR, not $GIT_COMMON_DIR.

Fix that with get_git_common_dir(). This function will return $GIT_DIR in single-worktree setup, so we don't have to make a special case for multiple-worktree here.

(*) It does in fact, thanks to another bug. More on that later.

---
 builtin/init-db.c |  2 +-
 t/t0001-init.sh   | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

index cc09fca..d5d7558 100644
@@ -138,7 +138,7 @@ static void copy_templates(const char *template_dir)
 		goto close_free_return;
 	}
 
-	strbuf_addstr(&path, get_git_dir());
+	strbuf_addstr(&path, get_git_common_dir());
 	strbuf_complete(&path, '/');
 	copy_templates_1(&path, &template_path, dir);
 close_free_return:

index 8ffbbea..488564b 100755
@@ -393,4 +393,19 @@ test_expect_success 'remote init from does not use config from cwd' '
 	test_cmp expect actual
 '
 
+test_expect_success 're-init from a linked worktree' '
+	git init main-worktree &&
+	(
+		cd main-worktree &&
+		test_commit first &&
+		git worktree add ../linked-worktree &&
+		mv .git/info/exclude expected-exclude &&
+		find .git/worktrees -print | sort >expected &&
+		git -C ../linked-worktree init &&
+		test_cmp expected-exclude .git/info/exclude &&
+		find .git/worktrees -print | sort >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_done

2.8.2.524.g6ff3d78

When "git init" is called with GIT_WORK_TREE environment set, we want to keep this worktree's location in core.worktree so the user does not have to set the environment again and again. See ef6f0af (git-init: set core.worktree if GIT_WORK_TREE is specified - 2007-07-04)

We detect that by this logic (in needs_work_tree_config): normally worktree's top dir would contains ".git" directory, if this is not true, worktree is probably set to elsewhere by the user.

Unfortunately when it calls get_git_dir() it does not take ".git" files into account. When we find a .git file, we immediately follow the file until we find the real ".git" directory. The location of this first ".git" file is lost.

The .git file would satisfy the logic above and not create core.worktree (correct). But because the final .git's location is used, needs_work_tree_config() is misled and creates core.worktree anyway.

This would not be a huge deal normally. But if this happens in a multiple worktree setup it becomes a real problem because up until now, core.worktree will be applied to the main worktree only. If you accidentally do "git init" from a linked worktree, you set core.worktree (for the main repo) pointing to the _linked_ worktree. After that point, may you live in interesting times.

Record the .git file location and use it here.

PS. real_path() resolves symlinks So original_git_dir is not truly original if '.git' is a symlink. Hopefully it's not longer used in favor of .git files.

---
 builtin/init-db.c | 5 ++++-
 t/t0001-init.sh   | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

index d5d7558..0d5cc76 100644
@@ -23,6 +23,7 @@ static int init_is_bare_repository = 0;
 static int init_shared_repository = -1;
 static const char *init_db_template_dir;
 static const char *git_link;
+static const char *original_git_dir;
 
 static void copy_templates_1(struct strbuf *path, struct strbuf *template,
 			     DIR *dir)

@@ -263,7 +264,7 @@ static int create_default_files(const char *template_path)
 		/* allow template config file to override the default */
 		if (log_all_ref_updates == -1)
 			git_config_set("core.logallrefupdates", "true");
-		if (needs_work_tree_config(get_git_dir(), work_tree))
+		if (needs_work_tree_config(original_git_dir, work_tree))
 			git_config_set("core.worktree", work_tree);
 	}
 

@@ -314,6 +315,8 @@ static void create_object_directory(void)
 int set_git_dir_init(const char *git_dir, const char *real_git_dir,
 		     int exist_ok)
 {
+	original_git_dir = xstrdup(real_path(git_dir));
+
 	if (real_git_dir) {
 		struct stat st;
 

index 488564b..b8fc588 100755
@@ -400,9 +400,11 @@ test_expect_success 're-init from a linked worktree' '
 		test_commit first &&
 		git worktree add ../linked-worktree &&
 		mv .git/info/exclude expected-exclude &&
+		cp .git/config expected-config &&
 		find .git/worktrees -print | sort >expected &&
 		git -C ../linked-worktree init &&
 		test_cmp expected-exclude .git/info/exclude &&
+		test_cmp expected-config .git/config &&
 		find .git/worktrees -print | sort >actual &&
 		test_cmp expected actual
 	)

2.8.2.524.g6ff3d78

s/contains/contain/;

The above explanation makes it sound like the correct fix belongs to needs_work_tree_config(), though.

I am starting to wonder if what ef6f0af did was misguided and we are better off without setting core.worktree ourselves, but that is a different issue.

Good thinking. We could go a step further, baking it as assert() to catch new/incorrect call sequences automatically.

Or we could combine the two functions init_db() and set_git_dir_init() into one. I prefer this one, but having problem with finding a good name for it because the new function would prepare $GIT_DIR for the entire process and init the repository. Maybe enter_and_init_db(), enter_and_init_repo()? If no good name is found, I'll go back to either adding comment or assert().

Duy

It probably is a reasonable way forward to name the combined function that takes the parameters that are taken by both functions in their current incarnation init_db(). After all, initializing the Git repository database involves telling the function where the .git directory is, among other things like where to take the template for a newly created repository is.

Commit "init: do not set core.worktree more often than necessary" adds a subtle dependency between set_git_dir_init() and init_db(). The former _must_ be called before init_db() so that original_git_dir can be set properly. If something else, like enter_repo() or setup_git_directory(), is used instead, the trick in that commit breaks down.

To eliminate the possibility that init_db() in future may be called without set_git_dir_init(), init_db() now calls that function internally (and does not allow anybody else to use it).

---
I think a separate commit for this is better than combining back to 2/3 so we can explain the problem properly (without making 2/3 commit message even longer)

Not sure if you want to s/contains/contain/ in 2/3 by yourself or I should resend the whole series. Let me know.

 builtin/clone.c   | 15 +++++++--------
 builtin/init-db.c | 18 +++++++++++-------
 cache.h           |  5 +++--
 3 files changed, 21 insertions(+), 17 deletions(-)

index 6616392..29b1832 100644
@@ -928,23 +928,22 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		set_git_work_tree(work_tree);
 	}
 
-	junk_git_dir = git_dir;
+	junk_git_dir = real_git_dir ? real_git_dir : git_dir;
 	if (safe_create_leading_directories_const(git_dir) < 0)
 		die(_("could not create leading directories of '%s'"), git_dir);
 
-	set_git_dir_init(git_dir, real_git_dir, 0);
-	if (real_git_dir) {
-		git_dir = real_git_dir;
-		junk_git_dir = real_git_dir;
-	}
-
 	if (0 <= option_verbosity) {
 		if (option_bare)
 			fprintf(stderr, _("Cloning into bare repository '%s'...\n"), dir);
 		else
 			fprintf(stderr, _("Cloning into '%s'...\n"), dir);
 	}
-	init_db(option_template, INIT_DB_QUIET);
+
+	init_db(git_dir, real_git_dir, option_template, INIT_DB_QUIET);
+
+	if (real_git_dir)
+		git_dir = real_git_dir;
+
 	write_config(&option_config);
 
 	git_config(git_default_config, NULL);

index d70fc45..ee7942f 100644
@@ -312,8 +312,9 @@ static void create_object_directory(void)
 	strbuf_release(&path);
 }
 
-int set_git_dir_init(const char *git_dir, const char *real_git_dir,
-		     int exist_ok)
+static int set_git_dir_init(const char *git_dir,
+			    const char *real_git_dir,
+			    int exist_ok)
 {
 	original_git_dir = xstrdup(real_path(git_dir));
 

@@ -362,10 +363,14 @@ static void separate_git_dir(const char *git_dir)
 	write_file(git_link, "gitdir: %s", git_dir);
 }
 
-int init_db(const char *template_dir, unsigned int flags)
+int init_db(const char *git_dir, const char *real_git_dir,
+	    const char *template_dir, unsigned int flags)
 {
 	int reinit;
-	const char *git_dir = get_git_dir();
+
+	set_git_dir_init(git_dir, real_git_dir, flags & INIT_DB_EXIST_OK);
+
+	git_dir = get_git_dir();
 
 	if (git_link)
 		separate_git_dir(git_dir);

@@ -585,7 +590,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			set_git_work_tree(work_tree);
 	}
 
-	set_git_dir_init(git_dir, real_git_dir, 1);
-
-	return init_db(template_dir, flags);
+	flags |= INIT_DB_EXIST_OK;
+	return init_db(git_dir, real_git_dir, template_dir, flags);
 }

index b2d77f3..7fc875f 100644
@@ -525,9 +525,10 @@ extern void verify_non_filename(const char *prefix, const char *name);
 extern int path_inside_repo(const char *prefix, const char *path);
 
 #define INIT_DB_QUIET 0x0001
+#define INIT_DB_EXIST_OK 0x0002
 
-extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);
-extern int init_db(const char *template_dir, unsigned int flags);
+extern int init_db(const char *git_dir, const char *real_git_dir,
+		   const char *template_dir, unsigned int flags);
 
 extern void sanitize_stdfds(void);
 extern int daemonize(void);

2.8.2.524.g6ff3d78

OK, I just amended it before applying this on top.

I think this 4/3 is not quite enough to fix the damage to the code caused by 2/3.

Given that

- set_git_dir_init() is is the only one that sets original_git_dir,

- create_default_files() is the only one that uses original_git_dir, and

- init_db() is the only one that calls set_git_dir_init() and create_default_files()

after 4/3 is applied, we should be able to remove the global variable 2/3 introduced, make init_db() receive that information as the return value of set_git_dir_init(), and pass that as a parameter to create_default_files().

That would look something like this squashed into 4/3, I think. I am not sure if a commit that squashes 2/3, 3/3, 4/3 and this update together is harder to understand than keeping 2/3, 3/3 and a fixed 4/3 separate, though. The end result looks much better structured than before 2/3 is applied to my quick scan-through of the code.

In any case, the log message of 2/3 needs to be updated to retitle it, I think. "do not ... more often than necessary" makes it sound as if we were doing things that did not make any difference in the end result, wasting cycles. But what you actually wanted to achieve was not to "avoid unnecessary work"--doing so gave a broken behaviour and that was what you were fixing, "do not record broken core.worktree", perhaps?

The solution (if we squash 2-4 and the fixup below) is to stop feeding get_git_dir() to needs_work_tree_config(), because the parameter to the latter is the path to ".git" that presumably is at the top of the working tree, and get_git_dir() is not that when "gitdir" file is involved. So a rewritten log message may say something like...

init: do not set unnecessary core.worktree

The function needs_work_tree_config() that is called from create_default_files() is supposed to be fed the path to ".git" that looks as if it is at the top of the working tree, and decide if that location matches the actual worktree being used. This comparison allows "git init" to decide if core.worktree needs to be recorded in the working tree.

In the current code, however, we feed the return value from get_git_dir(), which can be totally different from what the function expects when "gitdir" file is involved. Instead of giving the path to the ".git" at the top of the working tree, we end up feeding the actual path that the file points at.

This original location of ".git" however is only known to a helper function set_git_dir_init() that must be called before init_db() is called (they both have only two callsites, one in "git init" and the other in "git clone"), and in the current code, this original location is not visible to its callers.

By doing the following two things:

* Move call to set_git_dir_init() to init_db(), as the two must always be called in this order, and adjust its current callers.

* Make set_git_dir_init() return the original location of ".git" to the caller, which is init_db(), and have it passed to create_default_files() as a new parameter.

pass the correct location down to needs_work_tree_config() to fix this.

This suggests that 2/3, 3/3 and fixed 4/3 could be done in two logical steps. The first bullet point can be done as a separate preparatory step, and on top of that, the second bullet point can be done as a separate "fix".

 builtin/init-db.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

index ee7942f..527722c 100644
@@ -23,7 +23,6 @@ static int init_is_bare_repository = 0;
 static int init_shared_repository = -1;
 static const char *init_db_template_dir;
 static const char *git_link;
-static const char *original_git_dir;
 
 static void copy_templates_1(struct strbuf *path, struct strbuf *template,
 			     DIR *dir)

@@ -172,7 +171,8 @@ static int needs_work_tree_config(const char *git_dir, const char *work_tree)
 	return 1;
 }
 
-static int create_default_files(const char *template_path)
+static int create_default_files(const char *template_path,
+				const char *original_git_dir)
 {
 	struct stat st1;
 	struct strbuf buf = STRBUF_INIT;

@@ -312,11 +312,11 @@ static void create_object_directory(void)
 	strbuf_release(&path);
 }
 
-static int set_git_dir_init(const char *git_dir,
-			    const char *real_git_dir,
-			    int exist_ok)
+static char *set_git_dir_init(const char *git_dir,
+			      const char *real_git_dir,
+			      int exist_ok)
 {
-	original_git_dir = xstrdup(real_path(git_dir));
+	char *original_git_dir = xstrdup(real_path(git_dir));
 
 	if (real_git_dir) {
 		struct stat st;

@@ -339,7 +339,7 @@ static int set_git_dir_init(const char *git_dir,
 		git_link = NULL;
 	}
 	startup_info->have_repository = 1;
-	return 0;
+	return original_git_dir;
 }
 
 static void separate_git_dir(const char *git_dir)

@@ -367,9 +367,10 @@ int init_db(const char *git_dir, const char *real_git_dir,
 	    const char *template_dir, unsigned int flags)
 {
 	int reinit;
+	char *original_git_dir;
 
-	set_git_dir_init(git_dir, real_git_dir, flags & INIT_DB_EXIST_OK);
-
+	flags |= INIT_DB_EXIST_OK;
+	original_git_dir = set_git_dir_init(git_dir, real_git_dir, flags);
 	git_dir = get_git_dir();
 
 	if (git_link)

@@ -386,7 +387,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
 	 */
 	check_repository_format();
 
-	reinit = create_default_files(template_dir);
+	reinit = create_default_files(template_dir, original_git_dir);
 
 	create_object_directory();
 

How about this?

[1/5] init: correct re-initialization from a linked worktree [2/5] init: call set_git_dir_init() from within init_db() [3/5] init: kill set_git_dir_init()
[4/5] init: do not set unnecessary core.worktree [5/5] init: kill git_link variable

I went a bit further, merging set_git_dir_init() back to init_db() so I can kill "git_link" variable cleanly in 5/5. 3/5 does make init_db() a bit longer, but not to the alarming level yet. By 4/5, set_git_dir_init() is gone, so init_db() just needs to save and pass original_git_dir down to needs_work_tree_config().
--
Duy

When 'git init' is called from a linked worktree, we treat '.git' dir (which is $GIT_COMMON_DIR/worktrees/something) as the main '.git' (i.e. $GIT_COMMON_DIR) and populate the whole repository skeleton in there. It does not harm anything (*) but it is still wrong.

Since 'git init' calls set_git_dir() at preparation time, which indirectly calls get_common_dir() and correctly detects multiple worktree setup, all git_path_buf() calls in create_default_files() will return correct paths in both single and multiple worktree setups. The only thing left is copy_templates(), which targets $GIT_DIR, not $GIT_COMMON_DIR.

Fix that with get_git_common_dir(). This function will return $GIT_DIR in single-worktree setup, so we don't have to make a special case for multiple-worktree here.

(*) It does in fact, thanks to another bug. More on that later.

---
 builtin/init-db.c |  2 +-
 t/t0001-init.sh   | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

index cc09fca..d5d7558 100644
@@ -138,7 +138,7 @@ static void copy_templates(const char *template_dir)
 		goto close_free_return;
 	}
 
-	strbuf_addstr(&path, get_git_dir());
+	strbuf_addstr(&path, get_git_common_dir());
 	strbuf_complete(&path, '/');
 	copy_templates_1(&path, &template_path, dir);
 close_free_return:

index 8ffbbea..488564b 100755
@@ -393,4 +393,19 @@ test_expect_success 'remote init from does not use config from cwd' '
 	test_cmp expect actual
 '
 
+test_expect_success 're-init from a linked worktree' '
+	git init main-worktree &&
+	(
+		cd main-worktree &&
+		test_commit first &&
+		git worktree add ../linked-worktree &&
+		mv .git/info/exclude expected-exclude &&
+		find .git/worktrees -print | sort >expected &&
+		git -C ../linked-worktree init &&
+		test_cmp expected-exclude .git/info/exclude &&
+		find .git/worktrees -print | sort >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_done

2.8.2.524.g6ff3d78

The next commit requires that set_git_dir_init() must be called before init_db(). Let's make sure nobody can do otherwise.

---
 builtin/clone.c   | 15 +++++++--------
 builtin/init-db.c | 18 +++++++++++-------
 cache.h           |  5 +++--
 3 files changed, 21 insertions(+), 17 deletions(-)

index 6616392..29b1832 100644
@@ -928,23 +928,22 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		set_git_work_tree(work_tree);
 	}
 
-	junk_git_dir = git_dir;
+	junk_git_dir = real_git_dir ? real_git_dir : git_dir;
 	if (safe_create_leading_directories_const(git_dir) < 0)
 		die(_("could not create leading directories of '%s'"), git_dir);
 
-	set_git_dir_init(git_dir, real_git_dir, 0);
-	if (real_git_dir) {
-		git_dir = real_git_dir;
-		junk_git_dir = real_git_dir;
-	}
-
 	if (0 <= option_verbosity) {
 		if (option_bare)
 			fprintf(stderr, _("Cloning into bare repository '%s'...\n"), dir);
 		else
 			fprintf(stderr, _("Cloning into '%s'...\n"), dir);
 	}
-	init_db(option_template, INIT_DB_QUIET);
+
+	init_db(git_dir, real_git_dir, option_template, INIT_DB_QUIET);
+
+	if (real_git_dir)
+		git_dir = real_git_dir;
+
 	write_config(&option_config);
 
 	git_config(git_default_config, NULL);

index d5d7558..5cb05d9 100644
@@ -311,8 +311,9 @@ static void create_object_directory(void)
 	strbuf_release(&path);
 }
 
-int set_git_dir_init(const char *git_dir, const char *real_git_dir,
-		     int exist_ok)
+static int set_git_dir_init(const char *git_dir,
+			    const char *real_git_dir,
+			    int exist_ok)
 {
 	if (real_git_dir) {
 		struct stat st;

@@ -359,10 +360,14 @@ static void separate_git_dir(const char *git_dir)
 	write_file(git_link, "gitdir: %s", git_dir);
 }
 
-int init_db(const char *template_dir, unsigned int flags)
+int init_db(const char *git_dir, const char *real_git_dir,
+	    const char *template_dir, unsigned int flags)
 {
 	int reinit;
-	const char *git_dir = get_git_dir();
+
+	set_git_dir_init(git_dir, real_git_dir, flags & INIT_DB_EXIST_OK);
+
+	git_dir = get_git_dir();
 
 	if (git_link)
 		separate_git_dir(git_dir);

@@ -582,7 +587,6 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
 			set_git_work_tree(work_tree);
 	}
 
-	set_git_dir_init(git_dir, real_git_dir, 1);
-
-	return init_db(template_dir, flags);
+	flags |= INIT_DB_EXIST_OK;
+	return init_db(git_dir, real_git_dir, template_dir, flags);
 }

index b2d77f3..7fc875f 100644
@@ -525,9 +525,10 @@ extern void verify_non_filename(const char *prefix, const char *name);
 extern int path_inside_repo(const char *prefix, const char *path);
 
 #define INIT_DB_QUIET 0x0001
+#define INIT_DB_EXIST_OK 0x0002
 
-extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, int);
-extern int init_db(const char *template_dir, unsigned int flags);
+extern int init_db(const char *git_dir, const char *real_git_dir,
+		   const char *template_dir, unsigned int flags);
 
 extern void sanitize_stdfds(void);
 extern int daemonize(void);

2.8.2.524.g6ff3d78

This is a pure code move, necessary to kill the global variable git_link later (and also helps a bit in the next patch).

---
 builtin/init-db.c | 50 +++++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

index 5cb05d9..68c1ae3 100644
@@ -311,34 +311,6 @@ static void create_object_directory(void)
 	strbuf_release(&path);
 }
 
-static int set_git_dir_init(const char *git_dir,
-			    const char *real_git_dir,
-			    int exist_ok)
-{
-	if (real_git_dir) {
-		struct stat st;
-
-		if (!exist_ok && !stat(git_dir, &st))
-			die(_("%s already exists"), git_dir);
-
-		if (!exist_ok && !stat(real_git_dir, &st))
-			die(_("%s already exists"), real_git_dir);
-
-		/*
-		 * make sure symlinks are resolved because we'll be
-		 * moving the target repo later on in separate_git_dir()
-		 */
-		git_link = xstrdup(real_path(git_dir));
-		set_git_dir(real_path(real_git_dir));
-	}
-	else {
-		set_git_dir(real_path(git_dir));
-		git_link = NULL;
-	}
-	startup_info->have_repository = 1;
-	return 0;
-}
-
 static void separate_git_dir(const char *git_dir)
 {
 	struct stat st;

@@ -364,9 +336,29 @@ int init_db(const char *git_dir, const char *real_git_dir,
 	    const char *template_dir, unsigned int flags)
 {
 	int reinit;
+	int exist_ok = flags & INIT_DB_EXIST_OK;
 
-	set_git_dir_init(git_dir, real_git_dir, flags & INIT_DB_EXIST_OK);
+	if (real_git_dir) {
+		struct stat st;
 
+		if (!exist_ok && !stat(git_dir, &st))
+			die(_("%s already exists"), git_dir);
+
+		if (!exist_ok && !stat(real_git_dir, &st))
+			die(_("%s already exists"), real_git_dir);
+
+		/*
+		 * make sure symlinks are resolved because we'll be
+		 * moving the target repo later on in separate_git_dir()
+		 */
+		git_link = xstrdup(real_path(git_dir));
+		set_git_dir(real_path(real_git_dir));
+	}
+	else {
+		set_git_dir(real_path(git_dir));
+		git_link = NULL;
+	}
+	startup_info->have_repository = 1;
 	git_dir = get_git_dir();
 
 	if (git_link)

2.8.2.524.g6ff3d78

The function needs_work_tree_config() that is called from create_default_files() is supposed to be fed the path to ".git" that looks as if it is at the top of the working tree, and decide if that location matches the actual worktree being used. This comparison allows "git init" to decide if core.worktree needs to be recorded in the working tree.

In the current code, however, we feed the return value from get_git_dir(), which can be totally different from what the function expects when "gitdir" file is involved. Instead of giving the path to the ".git" at the top of the working tree, we end up feeding the actual path that the file points at.

This original location of ".git" however is only known to init_db(). Make init_db() save it and have it passed to create_default_files() as a new parameter, which passes the correct location down to needs_work_tree_config() to fix this.

---
 builtin/init-db.c | 9 ++++++---
 t/t0001-init.sh   | 2 ++
 2 files changed, 8 insertions(+), 3 deletions(-)

index 68c1ae3..8069cd2 100644
@@ -171,7 +171,8 @@ static int needs_work_tree_config(const char *git_dir, const char *work_tree)
 	return 1;
 }
 
-static int create_default_files(const char *template_path)
+static int create_default_files(const char *template_path,
+				const char *original_git_dir)
 {
 	struct stat st1;
 	struct strbuf buf = STRBUF_INIT;

@@ -263,7 +264,7 @@ static int create_default_files(const char *template_path)
 		/* allow template config file to override the default */
 		if (log_all_ref_updates == -1)
 			git_config_set("core.logallrefupdates", "true");
-		if (needs_work_tree_config(get_git_dir(), work_tree))
+		if (needs_work_tree_config(original_git_dir, work_tree))
 			git_config_set("core.worktree", work_tree);
 	}
 

@@ -337,6 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
 {
 	int reinit;
 	int exist_ok = flags & INIT_DB_EXIST_OK;
+	char *original_git_dir = xstrdup(real_path(git_dir));
 
 	if (real_git_dir) {
 		struct stat st;

@@ -375,7 +377,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
 	 */
 	check_repository_format();
 
-	reinit = create_default_files(template_dir);
+	reinit = create_default_files(template_dir, original_git_dir);
 
 	create_object_directory();
 

@@ -412,6 +414,7 @@ int init_db(const char *git_dir, const char *real_git_dir,
 		       git_dir, len && git_dir[len-1] != '/' ? "/" : "");
 	}
 
+	free(original_git_dir);
 	return 0;
 }
 

index 488564b..b8fc588 100755
@@ -400,9 +400,11 @@ test_expect_success 're-init from a linked worktree' '
 		test_commit first &&
 		git worktree add ../linked-worktree &&
 		mv .git/info/exclude expected-exclude &&
+		cp .git/config expected-config &&
 		find .git/worktrees -print | sort >expected &&
 		git -C ../linked-worktree init &&
 		test_cmp expected-exclude .git/info/exclude &&
+		test_cmp expected-config .git/config &&
 		find .git/worktrees -print | sort >actual &&
 		test_cmp expected actual
 	)

2.8.2.524.g6ff3d78

---
 builtin/init-db.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

index 8069cd2..37e318b 100644
@@ -22,7 +22,6 @@
 static int init_is_bare_repository = 0;
 static int init_shared_repository = -1;
 static const char *init_db_template_dir;
-static const char *git_link;
 
 static void copy_templates_1(struct strbuf *path, struct strbuf *template,
 			     DIR *dir)

@@ -312,7 +311,7 @@ static void create_object_directory(void)
 	strbuf_release(&path);
 }
 
-static void separate_git_dir(const char *git_dir)
+static void separate_git_dir(const char *git_dir, const char *git_link)
 {
 	struct stat st;
 

@@ -349,22 +348,15 @@ int init_db(const char *git_dir, const char *real_git_dir,
 		if (!exist_ok && !stat(real_git_dir, &st))
 			die(_("%s already exists"), real_git_dir);
 
-		/*
-		 * make sure symlinks are resolved because we'll be
-		 * moving the target repo later on in separate_git_dir()
-		 */
-		git_link = xstrdup(real_path(git_dir));
 		set_git_dir(real_path(real_git_dir));
+		git_dir = get_git_dir();
+		separate_git_dir(git_dir, original_git_dir);
 	}
 	else {
 		set_git_dir(real_path(git_dir));
-		git_link = NULL;
+		git_dir = get_git_dir();
 	}
 	startup_info->have_repository = 1;
-	git_dir = get_git_dir();
-
-	if (git_link)
-		separate_git_dir(git_dir);
 
 	safe_create_dir(git_dir, 0);
 

2.8.2.524.g6ff3d78

Since original_git_dir is a copy of real_path(git_dir), let's reuse it and avoid calling real_path() more than necessary.

The xstrdup() is removed too because original_git_dir is already a copy, and we're not going to free git_link in this code probably forever.

---
 builtin/init-db.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 0d5cc76..d70fc45 100644
@@ -330,11 +330,11 @@ int set_git_dir_init(const char *git_dir, const char *real_git_dir,
 		 * make sure symlinks are resolved because we'll be
 		 * moving the target repo later on in separate_git_dir()
 		 */
-		git_link = xstrdup(real_path(git_dir));
+		git_link = original_git_dir;
 		set_git_dir(real_path(real_git_dir));
 	}
 	else {
-		set_git_dir(real_path(git_dir));
+		set_git_dir(original_git_dir);
 		git_link = NULL;
 	}
 	startup_info->have_repository = 1;

2.8.2.524.g6ff3d78

Thanks. Will take a look.