Bug with git worktrees and git init

Bug with git worktrees and git init

[Max Nordlund]

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

Re: Bug with git worktrees and git init

[Michael J Gruber]

Max Nordlund venit, vidit, dixit 23.08.2016 14:35:
> 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

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.

> 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
> 

Re: Bug with git worktrees and git init

[Duy Nguyen]

On Tue, Aug 23, 2016 at 10:21 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> 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

[PATCH 0/3] Fix git-init in linked worktrees

[Nguyễn Thái Ngọc 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

[PATCH 1/3] init: correct re-initialization from a linked worktree

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

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.

Noticed-by: Max Nordlund <max.nordlund@sqore.com>
Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/init-db.c |  2 +-
 t/t0001-init.sh   | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 3a45f0b..6d9552e 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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:
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index a6fdd5e..d64e5e3 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -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

Re: [PATCH 1/3] init: correct re-initialization from a linked worktree

[Junio C Hamano]

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

> 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.

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

> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index 3a45f0b..6d9552e 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -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:
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index a6fdd5e..d64e5e3 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -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

Re: [PATCH 1/3] init: correct re-initialization from a linked worktree

[Duy Nguyen]

On Fri, Sep 9, 2016 at 2:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> 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.
>
> -ECANNOTPARSE.  Did you mean "... worktree, we treat '.git' dir as
> if it is the main '.git' ..."

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

> or something entirely different?
-- 
Duy

[PATCH 2/3] t0001: work around the bug that reads config file before repo setup

[Nguyễn Thái Ngọc 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".

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t0001-init.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index d64e5e3..393c940 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -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

Re: [PATCH 2/3] t0001: work around the bug that reads config file before repo setup

[Junio C Hamano]

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

> 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).

I vaguely recall this was discussed recently somewhere.  Is this
related to <20160902091130.jxckdeomw35eewky@sigill.intra.peff.net>?


> 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".

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.

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  t/t0001-init.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index d64e5e3..393c940 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -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 &&

Re: [PATCH 2/3] t0001: work around the bug that reads config file before repo setup

[Jeff King]

On Thu, Sep 08, 2016 at 08:47:18PM +0700, Nguyễn Thái Ngọc Duy wrote:

> 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".

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

Re: [PATCH 2/3] t0001: work around the bug that reads config file before repo setup

[Duy Nguyen]

On Fri, Sep 9, 2016 at 3:02 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Sep 08, 2016 at 08:47:18PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> 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".
>
> This is a bug for more than worktrees, and is something I'm working on
> fixing

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

Re: [PATCH 2/3] t0001: work around the bug that reads config file before repo setup

[Jeff King]

On Fri, Sep 09, 2016 at 05:32:21PM +0700, Duy Nguyen wrote:

> On Fri, Sep 9, 2016 at 3:02 AM, Jeff King <peff@peff.net> wrote:
> > On Thu, Sep 08, 2016 at 08:47:18PM +0700, Nguyễn Thái Ngọc Duy wrote:
> >
> >> 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".
> >
> > This is a bug for more than worktrees, and is something I'm working on
> > fixing
> 
> Great! test_expect_failure it is. But I'll make a separate patch,
> independent from this series though.

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

Re: [PATCH 2/3] t0001: work around the bug that reads config file before repo setup

[Jacob Keller]

On Fri, Sep 9, 2016 at 4:22 AM, Jeff King <peff@peff.net> wrote:
> 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

[PATCH 3/3] init: do not set core.worktree more often than necessary

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

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.

Noticed-by: Max Nordlund <max.nordlund@sqore.com>
Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/init-db.c |  2 +-
 cache.h           |  1 +
 environment.c     | 16 +++++++++++++++-
 t/t0001-init.sh   |  2 ++
 4 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 6d9552e..36255f2 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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);
 	}
 
diff --git a/cache.h b/cache.h
index b780a91..e6c05f8 100644
--- a/cache.h
+++ b/cache.h
@@ -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);
diff --git a/environment.c b/environment.c
index ca72464..8cfb8f3 100644
--- a/environment.c
+++ b/environment.c
@@ -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;
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 393c940..d59669a 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -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

Re: [PATCH 3/3] init: do not set core.worktree more often than necessary

[Junio C Hamano]

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

> +/*
> + * 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".
> + */

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.

> +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;
> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 393c940..d59669a 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -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
>  	)

Re: [PATCH 3/3] init: do not set core.worktree more often than necessary

[Duy Nguyen]

On Fri, Sep 9, 2016 at 2:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> +/*
>> + * 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".
>> + */
>
> 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

[PATCH v2 0/3] Fix git-init in linked worktrees

[Nguyễn Thái Ngọc 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

[PATCH v2 1/3] init: correct re-initialization from a linked worktree

[Nguyễn Thái Ngọc 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.

Noticed-by: Max Nordlund <max.nordlund@sqore.com>
Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/init-db.c |  2 +-
 t/t0001-init.sh   | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index cc09fca..d5d7558 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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:
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 8ffbbea..488564b 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -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

[PATCH v2 2/3] init: do not set core.worktree more often than necessary

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

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.

Noticed-by: Max Nordlund <max.nordlund@sqore.com>
Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/init-db.c | 5 ++++-
 t/t0001-init.sh   | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index d5d7558..0d5cc76 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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;
 
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 488564b..b8fc588 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -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

Re: [PATCH v2 2/3] init: do not set core.worktree more often than necessary

[Junio C Hamano]

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

> 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,

s/contains/contain/;

> 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.

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.

> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index d5d7558..0d5cc76 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -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;

The function being extern bothers me.  The create_default_files()
function, which is the only thing consumes this variable, is called
only from init_db(), and I'd prefer to see some way to guarantee
that everybody who calls init_db() calls set_git_dir_init()
beforehand.  Right now, cmd_init_db() and cmd_clone() are the only
ones that call init_db() and they both call set_dir_git_init(); if a
new caller starts calling init_db() and forgets to call the other
one, that caller will be buggy.

Perhaps a comment before init_db() to tell callers to always call
the other one is the least thing necessary?

> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
> index 488564b..b8fc588 100755
> --- a/t/t0001-init.sh
> +++ b/t/t0001-init.sh
> @@ -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
>  	)

Re: [PATCH v2 2/3] init: do not set core.worktree more often than necessary

[Duy Nguyen]

On Thu, Sep 22, 2016 at 1:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> @@ -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;
>
> The function being extern bothers me.  The create_default_files()
> function, which is the only thing consumes this variable, is called
> only from init_db(), and I'd prefer to see some way to guarantee
> that everybody who calls init_db() calls set_git_dir_init()
> beforehand.  Right now, cmd_init_db() and cmd_clone() are the only
> ones that call init_db() and they both call set_dir_git_init(); if a
> new caller starts calling init_db() and forgets to call the other
> one, that caller will be buggy.
>
> Perhaps a comment before init_db() to tell callers to always call
> the other one is the least thing necessary?

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

Re: [PATCH v2 2/3] init: do not set core.worktree more often than necessary

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

>> Perhaps a comment before init_db() to tell callers to always call
>> the other one is the least thing necessary?
>
> 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 ...

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.

[PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

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

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).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 6616392..29b1832 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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);
diff --git a/builtin/init-db.c b/builtin/init-db.c
index d70fc45..ee7942f 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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);
 }
diff --git a/cache.h b/cache.h
index b2d77f3..7fc875f 100644
--- a/cache.h
+++ b/cache.h
@@ -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

Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Junio C Hamano]

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

>  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.

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

> +	flags |= INIT_DB_EXIST_OK;
> +	return init_db(git_dir, real_git_dir, template_dir, flags);

I do not think of anything better, but EXIST_OK does not sound
grammatical.  "REINIT" is not quite it--we are merely allowing
the function to re-init if there already is a repository.  And
OK_TO_REINIT is a bit too long.  Let's take the patch as-is for
now.

Thanks.

Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Junio C Hamano]

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

>  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.

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().

Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I think this 4/3 is not quite enough to fix the damage to the code
> caused by 2/3.
> ...
> 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(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index ee7942f..527722c 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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();
 

Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Duy Nguyen]

On Sat, Sep 24, 2016 at 11:55:33AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I think this 4/3 is not quite enough to fix the damage to the code
> > caused by 2/3.
> > ...
> > 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.
> ...

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

[PATCH v3 1/5] init: correct re-initialization from a linked worktree

[Nguyễn Thái Ngọc 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.

Noticed-by: Max Nordlund <max.nordlund@sqore.com>
Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/init-db.c |  2 +-
 t/t0001-init.sh   | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index cc09fca..d5d7558 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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:
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 8ffbbea..488564b 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -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

[PATCH v3 2/5] init: call set_git_dir_init() from within init_db()

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

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/clone.c   | 15 +++++++--------
 builtin/init-db.c | 18 +++++++++++-------
 cache.h           |  5 +++--
 3 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 6616392..29b1832 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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);
diff --git a/builtin/init-db.c b/builtin/init-db.c
index d5d7558..5cb05d9 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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);
 }
diff --git a/cache.h b/cache.h
index b2d77f3..7fc875f 100644
--- a/cache.h
+++ b/cache.h
@@ -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

[PATCH v3 3/5] init: kill set_git_dir_init()

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

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/init-db.c | 50 +++++++++++++++++++++-----------------------------
 1 file changed, 21 insertions(+), 29 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 5cb05d9..68c1ae3 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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

[PATCH v3 4/5] init: do not set unnecessary core.worktree

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

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.

Noticed-by: Max Nordlund <max.nordlund@sqore.com>
Helped-by: Michael J Gruber <git@drmicha.warpmail.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/init-db.c | 9 ++++++---
 t/t0001-init.sh   | 2 ++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 68c1ae3..8069cd2 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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;
 }
 
diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 488564b..b8fc588 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -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

[PATCH v3 5/5] init: kill git_link variable

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/init-db.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 8069cd2..37e318b 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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

[PATCH v2 3/3] init: reuse original_git_dir in set_git_dir_init()

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

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/init-db.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/init-db.c b/builtin/init-db.c
index 0d5cc76..d70fc45 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -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

Re: [PATCH v2 0/3] Fix git-init in linked worktrees

[Junio C Hamano]

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

> 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()

Thanks. Will take a look.