[PATCH v4 0/2] path: implement common_dir handling in git_path_submodule()

[PATCH v4 0/2] path: implement common_dir handling in git_path_submodule()

[Max Kirillov]

After http://thread.gmane.org/gmane.comp.version-control.git/261990
the only thing which did not enter the serie is these 2 pathes.

Should be applied over the patches by link, or
ecf2ff6ace6a1cc3d55b6f917be5452b5fb0c21b in current pu.

Max Kirillov (2):
  submodule refactor: use git_path_submodule() in add_submodule_odb()
  path: implement common_dir handling in git_path_submodule()

 cache.h                          |  1 +
 path.c                           | 24 ++++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 submodule.c                      | 28 ++++++++++------------------
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 5 files changed, 53 insertions(+), 27 deletions(-)

-- 
2.1.1.391.g7a54a76

[PATCH v4 1/2] submodule refactor: use git_path_submodule() in add_submodule_odb()

[Max Kirillov]

Signed-off-by: Max Kirillov <max@max630.net>
---
 submodule.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/submodule.c b/submodule.c
index 34094f5..4aad3d4 100644
--- a/submodule.c
+++ b/submodule.c
@@ -122,43 +122,35 @@ void stage_updated_gitmodules(void)
 
 static int add_submodule_odb(const char *path)
 {
-	struct strbuf objects_directory = STRBUF_INIT;
 	struct alternate_object_database *alt_odb;
+	const char* objects_directory;
 	int ret = 0;
-	const char *git_dir;
 
-	strbuf_addf(&objects_directory, "%s/.git", path);
-	git_dir = read_gitfile(objects_directory.buf);
-	if (git_dir) {
-		strbuf_reset(&objects_directory);
-		strbuf_addstr(&objects_directory, git_dir);
-	}
-	strbuf_addstr(&objects_directory, "/objects/");
-	if (!is_directory(objects_directory.buf)) {
+	objects_directory = git_path_submodule(path, "objects/");
+	if (!is_directory(objects_directory)) {
 		ret = -1;
 		goto done;
 	}
+
 	/* avoid adding it twice */
 	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
-		if (alt_odb->name - alt_odb->base == objects_directory.len &&
-				!strncmp(alt_odb->base, objects_directory.buf,
-					objects_directory.len))
+		if (alt_odb->name - alt_odb->base == strlen(objects_directory) &&
+				!strcmp(alt_odb->base, objects_directory))
 			goto done;
 
-	alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
+	alt_odb = xmalloc(strlen(objects_directory) + 42 + sizeof(*alt_odb));
 	alt_odb->next = alt_odb_list;
-	strcpy(alt_odb->base, objects_directory.buf);
-	alt_odb->name = alt_odb->base + objects_directory.len;
+	strcpy(alt_odb->base, objects_directory);
+	alt_odb->name = alt_odb->base + strlen(objects_directory);
 	alt_odb->name[2] = '/';
 	alt_odb->name[40] = '\0';
 	alt_odb->name[41] = '\0';
 	alt_odb_list = alt_odb;
 
 	/* add possible alternates from the submodule */
-	read_info_alternates(objects_directory.buf, 0);
+	read_info_alternates(objects_directory, 0);
 	prepare_alt_odb();
 done:
-	strbuf_release(&objects_directory);
 	return ret;
 }
 
-- 
2.1.1.391.g7a54a76

[PATCH v4 2/2] path: implement common_dir handling in git_path_submodule()

[Max Kirillov]

This allows making submodules a linked workdirs.

Same as for .git, but ignores the GIT_COMMON_DIR environment variable,
because it would mean common directory for the parent repository and
does not make sense for submodule.

Also add test for functionality which uses this call.

Signed-off-by: Max Kirillov <max@max630.net>
---
 cache.h                          |  1 +
 path.c                           | 24 ++++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 4 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/cache.h b/cache.h
index 670e861..4cd03f0 100644
--- a/cache.h
+++ b/cache.h
@@ -437,6 +437,7 @@ extern char *get_object_directory(void);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
+extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
 extern const char *get_git_namespace(void);
 extern const char *strip_namespace(const char *namespaced_ref);
diff --git a/path.c b/path.c
index a5c51a3..78f718f 100644
--- a/path.c
+++ b/path.c
@@ -98,7 +98,7 @@ static const char *common_list[] = {
 	NULL
 };
 
-static void update_common_dir(struct strbuf *buf, int git_dir_len)
+static void update_common_dir(struct strbuf *buf, int git_dir_len, const char* common_dir)
 {
 	char *base = buf->buf + git_dir_len;
 	const char **p;
@@ -115,12 +115,17 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len)
 			path++;
 			is_dir = 1;
 		}
+
+		if (!common_dir) {
+			common_dir = get_git_common_dir();
+		}
+
 		if (is_dir && dir_prefix(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 		if (!is_dir && !strcmp(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 	}
@@ -160,7 +165,7 @@ static void adjust_git_path(struct strbuf *buf, int git_dir_len)
 	else if (git_db_env && dir_prefix(base, "objects"))
 		replace_dir(buf, git_dir_len + 7, get_object_directory());
 	else if (git_common_dir_env)
-		update_common_dir(buf, git_dir_len);
+		update_common_dir(buf, git_dir_len, NULL);
 }
 
 static void do_git_path(struct strbuf *buf, const char *fmt, va_list args)
@@ -256,6 +261,8 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)
 {
 	struct strbuf *buf = get_pathname();
 	const char *git_dir;
+	struct strbuf git_submodule_common_dir = STRBUF_INIT;
+	struct strbuf git_submodule_dir = STRBUF_INIT;
 	va_list args;
 
 	strbuf_addstr(buf, path);
@@ -269,11 +276,20 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)
 		strbuf_addstr(buf, git_dir);
 	}
 	strbuf_addch(buf, '/');
+	strbuf_addstr(&git_submodule_dir, buf->buf);
 
 	va_start(args, fmt);
 	strbuf_vaddf(buf, fmt, args);
 	va_end(args);
+
+	if (get_common_dir_noenv(&git_submodule_common_dir, git_submodule_dir.buf)) {
+		update_common_dir(buf, git_submodule_dir.len, git_submodule_common_dir.buf);
+	}
+
 	strbuf_cleanup_path(buf);
+
+	strbuf_release(&git_submodule_dir);
+	strbuf_release(&git_submodule_common_dir);
 	return buf->buf;
 }
 
diff --git a/setup.c b/setup.c
index fb61860..ffda622 100644
--- a/setup.c
+++ b/setup.c
@@ -226,14 +226,21 @@ void verify_non_filename(const char *prefix, const char *arg)
 
 int get_common_dir(struct strbuf *sb, const char *gitdir)
 {
+	const char *git_env_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
+	if (git_env_common_dir) {
+		strbuf_addstr(sb, git_env_common_dir);
+		return 1;
+	} else {
+		return get_common_dir_noenv(sb, gitdir);
+	}
+}
+
+int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
+{
 	struct strbuf data = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
-	const char *git_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
 	int ret = 0;
-	if (git_common_dir) {
-		strbuf_addstr(sb, git_common_dir);
-		return 1;
-	}
+
 	strbuf_addf(&path, "%s/commondir", gitdir);
 	if (file_exists(path.buf)) {
 		if (strbuf_read_file(&data, path.buf, 0) <= 0)
diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh
index 8f30aed..b43391a 100755
--- a/t/t7410-submodule-checkout-to.sh
+++ b/t/t7410-submodule-checkout-to.sh
@@ -47,4 +47,14 @@ test_expect_success 'checkout main and initialize independed clones' \
 test_expect_success 'can see submodule diffs after independed cloning' \
     '(cd fully_cloned_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
 
+test_expect_success 'checkout sub manually' \
+    'mkdir linked_submodule &&
+    (cd clone/main &&
+	git checkout --to "$base_path/linked_submodule/main" "$rev1_hash_main") &&
+    (cd clone/main/sub &&
+	git checkout --to "$base_path/linked_submodule/main/sub" "$rev1_hash_sub")'
+
+test_expect_success 'can see submodule diffs after manual checkout of linked submodule' \
+    '(cd linked_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
+
 test_done
-- 
2.1.1.391.g7a54a76

[PATCH v5 0/2] path: implement common_dir handling in git_path_submodule()

[Max Kirillov]

Rebased to latest master, also merges cleanly to pu. Otherwise no changes.

Max Kirillov (2):
  submodule refactor: use git_path_submodule() in add_submodule_odb()
  path: implement common_dir handling in git_path_submodule()

 cache.h                          |  1 +
 path.c                           | 24 ++++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 submodule.c                      | 28 ++++++++++------------------
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 5 files changed, 53 insertions(+), 27 deletions(-)

-- 
2.3.4.2801.g3d0809b

[PATCH v5 1/2] submodule refactor: use git_path_submodule() in add_submodule_odb()

[Max Kirillov]

Signed-off-by: Max Kirillov <max@max630.net>
---
 submodule.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/submodule.c b/submodule.c
index 15e90d1..f6afe0a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -122,43 +122,35 @@ void stage_updated_gitmodules(void)
 
 static int add_submodule_odb(const char *path)
 {
-	struct strbuf objects_directory = STRBUF_INIT;
 	struct alternate_object_database *alt_odb;
+	const char* objects_directory;
 	int ret = 0;
-	const char *git_dir;
 
-	strbuf_addf(&objects_directory, "%s/.git", path);
-	git_dir = read_gitfile(objects_directory.buf);
-	if (git_dir) {
-		strbuf_reset(&objects_directory);
-		strbuf_addstr(&objects_directory, git_dir);
-	}
-	strbuf_addstr(&objects_directory, "/objects/");
-	if (!is_directory(objects_directory.buf)) {
+	objects_directory = git_path_submodule(path, "objects/");
+	if (!is_directory(objects_directory)) {
 		ret = -1;
 		goto done;
 	}
+
 	/* avoid adding it twice */
 	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
-		if (alt_odb->name - alt_odb->base == objects_directory.len &&
-				!strncmp(alt_odb->base, objects_directory.buf,
-					objects_directory.len))
+		if (alt_odb->name - alt_odb->base == strlen(objects_directory) &&
+				!strcmp(alt_odb->base, objects_directory))
 			goto done;
 
-	alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
+	alt_odb = xmalloc(strlen(objects_directory) + 42 + sizeof(*alt_odb));
 	alt_odb->next = alt_odb_list;
-	strcpy(alt_odb->base, objects_directory.buf);
-	alt_odb->name = alt_odb->base + objects_directory.len;
+	strcpy(alt_odb->base, objects_directory);
+	alt_odb->name = alt_odb->base + strlen(objects_directory);
 	alt_odb->name[2] = '/';
 	alt_odb->name[40] = '\0';
 	alt_odb->name[41] = '\0';
 	alt_odb_list = alt_odb;
 
 	/* add possible alternates from the submodule */
-	read_info_alternates(objects_directory.buf, 0);
+	read_info_alternates(objects_directory, 0);
 	prepare_alt_odb();
 done:
-	strbuf_release(&objects_directory);
 	return ret;
 }
 
-- 
2.3.4.2801.g3d0809b

[PATCH v5 2/2] path: implement common_dir handling in git_path_submodule()

[Max Kirillov]

This allows making submodules a linked workdirs.

Same as for .git, but ignores the GIT_COMMON_DIR environment variable,
because it would mean common directory for the parent repository and
does not make sense for submodule.

Also add test for functionality which uses this call.

Signed-off-by: Max Kirillov <max@max630.net>
---
 cache.h                          |  1 +
 path.c                           | 24 ++++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 4 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/cache.h b/cache.h
index 4f55466..b87ec75 100644
--- a/cache.h
+++ b/cache.h
@@ -442,6 +442,7 @@ extern char *get_object_directory(void);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
+extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
 extern const char *get_git_namespace(void);
 extern const char *strip_namespace(const char *namespaced_ref);
diff --git a/path.c b/path.c
index 10f4cbf..f292d02 100644
--- a/path.c
+++ b/path.c
@@ -98,7 +98,7 @@ static const char *common_list[] = {
 	NULL
 };
 
-static void update_common_dir(struct strbuf *buf, int git_dir_len)
+static void update_common_dir(struct strbuf *buf, int git_dir_len, const char* common_dir)
 {
 	char *base = buf->buf + git_dir_len;
 	const char **p;
@@ -115,12 +115,17 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len)
 			path++;
 			is_dir = 1;
 		}
+
+		if (!common_dir) {
+			common_dir = get_git_common_dir();
+		}
+
 		if (is_dir && dir_prefix(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 		if (!is_dir && !strcmp(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 	}
@@ -160,7 +165,7 @@ static void adjust_git_path(struct strbuf *buf, int git_dir_len)
 	else if (git_db_env && dir_prefix(base, "objects"))
 		replace_dir(buf, git_dir_len + 7, get_object_directory());
 	else if (git_common_dir_env)
-		update_common_dir(buf, git_dir_len);
+		update_common_dir(buf, git_dir_len, NULL);
 }
 
 static void do_git_path(struct strbuf *buf, const char *fmt, va_list args)
@@ -228,6 +233,8 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)
 {
 	struct strbuf *buf = get_pathname();
 	const char *git_dir;
+	struct strbuf git_submodule_common_dir = STRBUF_INIT;
+	struct strbuf git_submodule_dir = STRBUF_INIT;
 	va_list args;
 
 	strbuf_addstr(buf, path);
@@ -241,11 +248,20 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)
 		strbuf_addstr(buf, git_dir);
 	}
 	strbuf_addch(buf, '/');
+	strbuf_addstr(&git_submodule_dir, buf->buf);
 
 	va_start(args, fmt);
 	strbuf_vaddf(buf, fmt, args);
 	va_end(args);
+
+	if (get_common_dir_noenv(&git_submodule_common_dir, git_submodule_dir.buf)) {
+		update_common_dir(buf, git_submodule_dir.len, git_submodule_common_dir.buf);
+	}
+
 	strbuf_cleanup_path(buf);
+
+	strbuf_release(&git_submodule_dir);
+	strbuf_release(&git_submodule_common_dir);
 	return buf->buf;
 }
 
diff --git a/setup.c b/setup.c
index 82c0cc2..39ea06b 100644
--- a/setup.c
+++ b/setup.c
@@ -229,14 +229,21 @@ void verify_non_filename(const char *prefix, const char *arg)
 
 int get_common_dir(struct strbuf *sb, const char *gitdir)
 {
+	const char *git_env_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
+	if (git_env_common_dir) {
+		strbuf_addstr(sb, git_env_common_dir);
+		return 1;
+	} else {
+		return get_common_dir_noenv(sb, gitdir);
+	}
+}
+
+int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
+{
 	struct strbuf data = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
-	const char *git_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
 	int ret = 0;
-	if (git_common_dir) {
-		strbuf_addstr(sb, git_common_dir);
-		return 1;
-	}
+
 	strbuf_addf(&path, "%s/commondir", gitdir);
 	if (file_exists(path.buf)) {
 		if (strbuf_read_file(&data, path.buf, 0) <= 0)
diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh
index 3f609e8..1acef32 100755
--- a/t/t7410-submodule-checkout-to.sh
+++ b/t/t7410-submodule-checkout-to.sh
@@ -47,4 +47,14 @@ test_expect_success 'checkout main and initialize independed clones' \
 test_expect_success 'can see submodule diffs after independed cloning' \
     '(cd fully_cloned_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
 
+test_expect_success 'checkout sub manually' \
+    'mkdir linked_submodule &&
+    (cd clone/main &&
+	git worktree add "$base_path/linked_submodule/main" "$rev1_hash_main") &&
+    (cd clone/main/sub &&
+	git worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub")'
+
+test_expect_success 'can see submodule diffs after manual checkout of linked submodule' \
+    '(cd linked_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
+
 test_done
-- 
2.3.4.2801.g3d0809b

Re: [PATCH v5 1/2] submodule refactor: use git_path_submodule() in add_submodule_odb()

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

Here is a space to describe why this change is a good thing.  Is
this a fix to change the behaviour, and if so how is the behaviour
different with and without the patch?  Or is this just to drop the
block of code from here and replace it with a call to an existing
helper that does exactly the same thing?  I _suspect_ that it is the
latter, but please do not force reviewers to guess.

> Signed-off-by: Max Kirillov <max@max630.net>
> ---
>  submodule.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 15e90d1..f6afe0a 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -122,43 +122,35 @@ void stage_updated_gitmodules(void)
>  
>  static int add_submodule_odb(const char *path)
>  {
> -	struct strbuf objects_directory = STRBUF_INIT;
>  	struct alternate_object_database *alt_odb;
> +	const char* objects_directory;

Style; asterisk sticks to the variable, not the type.  I think you
need to fix this in multiple places (not just 1/2 but also in 2/2).

Other than that I think it is a sensible "replace bulk of code with
identical helper that already exists" rewrite.

Thanks.

>  	int ret = 0;
> -	const char *git_dir;
>  
> -	strbuf_addf(&objects_directory, "%s/.git", path);
> -	git_dir = read_gitfile(objects_directory.buf);
> -	if (git_dir) {
> -		strbuf_reset(&objects_directory);
> -		strbuf_addstr(&objects_directory, git_dir);
> -	}
> -	strbuf_addstr(&objects_directory, "/objects/");
> -	if (!is_directory(objects_directory.buf)) {
> +	objects_directory = git_path_submodule(path, "objects/");
> +	if (!is_directory(objects_directory)) {
>  		ret = -1;
>  		goto done;
>  	}
> +
>  	/* avoid adding it twice */
>  	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
> -		if (alt_odb->name - alt_odb->base == objects_directory.len &&
> -				!strncmp(alt_odb->base, objects_directory.buf,
> -					objects_directory.len))
> +		if (alt_odb->name - alt_odb->base == strlen(objects_directory) &&
> +				!strcmp(alt_odb->base, objects_directory))
>  			goto done;
>  
> -	alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
> +	alt_odb = xmalloc(strlen(objects_directory) + 42 + sizeof(*alt_odb));
>  	alt_odb->next = alt_odb_list;
> -	strcpy(alt_odb->base, objects_directory.buf);
> -	alt_odb->name = alt_odb->base + objects_directory.len;
> +	strcpy(alt_odb->base, objects_directory);
> +	alt_odb->name = alt_odb->base + strlen(objects_directory);
>  	alt_odb->name[2] = '/';
>  	alt_odb->name[40] = '\0';
>  	alt_odb->name[41] = '\0';
>  	alt_odb_list = alt_odb;
>  
>  	/* add possible alternates from the submodule */
> -	read_info_alternates(objects_directory.buf, 0);
> +	read_info_alternates(objects_directory, 0);
>  	prepare_alt_odb();
>  done:
> -	strbuf_release(&objects_directory);
>  	return ret;
>  }

[PATCH v6 0/2] path: implement common_dir handling in git_path_submodule()

[Max Kirillov]

Fixed the pointer declaration spacing issue and updated commit messages.

The 1/2 refactoring still might leave some cases behind. I could grep only function
connect_work_tree_and_git_dir() which, as far as I can see, involved in moving repositories.
Have not touched it because correct moving parts of multiple-worktree
repositories probably is a bigger task than just fix config file location.

Max Kirillov (2):
  submodule refactor: use git_path_submodule() in add_submodule_odb()
  path: implement common_dir handling in git_path_submodule()

 cache.h                          |  1 +
 path.c                           | 24 ++++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 submodule.c                      | 28 ++++++++++------------------
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 5 files changed, 53 insertions(+), 27 deletions(-)

-- 
2.3.4.2801.g3d0809b

[PATCH v6 1/2] submodule refactor: use git_path_submodule() in add_submodule_odb()

[Max Kirillov]

Functions which directly operate submodule's object database do not handle the
case when the submodule is linked worktree (which are introduced in
c7b3a3d2fe). Instead of fixing the path calculation use already existing
git_path_submodule() function, with intention to modify only that function
whenever we need to change real location of submodule's repository content.

Signed-off-by: Max Kirillov <max@max630.net>
---
 submodule.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/submodule.c b/submodule.c
index 15e90d1..70d18ec 100644
--- a/submodule.c
+++ b/submodule.c
@@ -122,43 +122,35 @@ void stage_updated_gitmodules(void)
 
 static int add_submodule_odb(const char *path)
 {
-	struct strbuf objects_directory = STRBUF_INIT;
 	struct alternate_object_database *alt_odb;
+	const char *objects_directory;
 	int ret = 0;
-	const char *git_dir;
 
-	strbuf_addf(&objects_directory, "%s/.git", path);
-	git_dir = read_gitfile(objects_directory.buf);
-	if (git_dir) {
-		strbuf_reset(&objects_directory);
-		strbuf_addstr(&objects_directory, git_dir);
-	}
-	strbuf_addstr(&objects_directory, "/objects/");
-	if (!is_directory(objects_directory.buf)) {
+	objects_directory = git_path_submodule(path, "objects/");
+	if (!is_directory(objects_directory)) {
 		ret = -1;
 		goto done;
 	}
+
 	/* avoid adding it twice */
 	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
-		if (alt_odb->name - alt_odb->base == objects_directory.len &&
-				!strncmp(alt_odb->base, objects_directory.buf,
-					objects_directory.len))
+		if (alt_odb->name - alt_odb->base == strlen(objects_directory) &&
+				!strcmp(alt_odb->base, objects_directory))
 			goto done;
 
-	alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
+	alt_odb = xmalloc(strlen(objects_directory) + 42 + sizeof(*alt_odb));
 	alt_odb->next = alt_odb_list;
-	strcpy(alt_odb->base, objects_directory.buf);
-	alt_odb->name = alt_odb->base + objects_directory.len;
+	strcpy(alt_odb->base, objects_directory);
+	alt_odb->name = alt_odb->base + strlen(objects_directory);
 	alt_odb->name[2] = '/';
 	alt_odb->name[40] = '\0';
 	alt_odb->name[41] = '\0';
 	alt_odb_list = alt_odb;
 
 	/* add possible alternates from the submodule */
-	read_info_alternates(objects_directory.buf, 0);
+	read_info_alternates(objects_directory, 0);
 	prepare_alt_odb();
 done:
-	strbuf_release(&objects_directory);
 	return ret;
 }
 
-- 
2.3.4.2801.g3d0809b

[PATCH v6 2/2] path: implement common_dir handling in git_path_submodule()

[Max Kirillov]

When submodule is a linked worktree, "git diff --submodule" and other
calls which directly access the submodule's object database do not correctly
calculate its path. Fix it by changing the git_path_submodule() behavior,
to use either common or per-worktree directory.

Do it similarly as for parent repository, but ignore the GIT_COMMON_DIR
environment variable, because it would mean common directory for the parent
repository and does not make sense for submodule.

Also add test for functionality which uses this call.

Signed-off-by: Max Kirillov <max@max630.net>
---
 cache.h                          |  1 +
 path.c                           | 24 ++++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 4 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/cache.h b/cache.h
index 4f55466..b87ec75 100644
--- a/cache.h
+++ b/cache.h
@@ -442,6 +442,7 @@ extern char *get_object_directory(void);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
+extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
 extern const char *get_git_namespace(void);
 extern const char *strip_namespace(const char *namespaced_ref);
diff --git a/path.c b/path.c
index 10f4cbf..b0cf444 100644
--- a/path.c
+++ b/path.c
@@ -98,7 +98,7 @@ static const char *common_list[] = {
 	NULL
 };
 
-static void update_common_dir(struct strbuf *buf, int git_dir_len)
+static void update_common_dir(struct strbuf *buf, int git_dir_len, const char *common_dir)
 {
 	char *base = buf->buf + git_dir_len;
 	const char **p;
@@ -115,12 +115,17 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len)
 			path++;
 			is_dir = 1;
 		}
+
+		if (!common_dir) {
+			common_dir = get_git_common_dir();
+		}
+
 		if (is_dir && dir_prefix(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 		if (!is_dir && !strcmp(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 	}
@@ -160,7 +165,7 @@ static void adjust_git_path(struct strbuf *buf, int git_dir_len)
 	else if (git_db_env && dir_prefix(base, "objects"))
 		replace_dir(buf, git_dir_len + 7, get_object_directory());
 	else if (git_common_dir_env)
-		update_common_dir(buf, git_dir_len);
+		update_common_dir(buf, git_dir_len, NULL);
 }
 
 static void do_git_path(struct strbuf *buf, const char *fmt, va_list args)
@@ -228,6 +233,8 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)
 {
 	struct strbuf *buf = get_pathname();
 	const char *git_dir;
+	struct strbuf git_submodule_common_dir = STRBUF_INIT;
+	struct strbuf git_submodule_dir = STRBUF_INIT;
 	va_list args;
 
 	strbuf_addstr(buf, path);
@@ -241,11 +248,20 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)
 		strbuf_addstr(buf, git_dir);
 	}
 	strbuf_addch(buf, '/');
+	strbuf_addstr(&git_submodule_dir, buf->buf);
 
 	va_start(args, fmt);
 	strbuf_vaddf(buf, fmt, args);
 	va_end(args);
+
+	if (get_common_dir_noenv(&git_submodule_common_dir, git_submodule_dir.buf)) {
+		update_common_dir(buf, git_submodule_dir.len, git_submodule_common_dir.buf);
+	}
+
 	strbuf_cleanup_path(buf);
+
+	strbuf_release(&git_submodule_dir);
+	strbuf_release(&git_submodule_common_dir);
 	return buf->buf;
 }
 
diff --git a/setup.c b/setup.c
index 82c0cc2..39ea06b 100644
--- a/setup.c
+++ b/setup.c
@@ -229,14 +229,21 @@ void verify_non_filename(const char *prefix, const char *arg)
 
 int get_common_dir(struct strbuf *sb, const char *gitdir)
 {
+	const char *git_env_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
+	if (git_env_common_dir) {
+		strbuf_addstr(sb, git_env_common_dir);
+		return 1;
+	} else {
+		return get_common_dir_noenv(sb, gitdir);
+	}
+}
+
+int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
+{
 	struct strbuf data = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
-	const char *git_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
 	int ret = 0;
-	if (git_common_dir) {
-		strbuf_addstr(sb, git_common_dir);
-		return 1;
-	}
+
 	strbuf_addf(&path, "%s/commondir", gitdir);
 	if (file_exists(path.buf)) {
 		if (strbuf_read_file(&data, path.buf, 0) <= 0)
diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh
index 3f609e8..1acef32 100755
--- a/t/t7410-submodule-checkout-to.sh
+++ b/t/t7410-submodule-checkout-to.sh
@@ -47,4 +47,14 @@ test_expect_success 'checkout main and initialize independed clones' \
 test_expect_success 'can see submodule diffs after independed cloning' \
     '(cd fully_cloned_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
 
+test_expect_success 'checkout sub manually' \
+    'mkdir linked_submodule &&
+    (cd clone/main &&
+	git worktree add "$base_path/linked_submodule/main" "$rev1_hash_main") &&
+    (cd clone/main/sub &&
+	git worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub")'
+
+test_expect_success 'can see submodule diffs after manual checkout of linked submodule' \
+    '(cd linked_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
+
 test_done
-- 
2.3.4.2801.g3d0809b

[PATCH v7 0/2] path: implement common_dir handling in git_path_submodule()

[Max Kirillov]

Emphasized that 1/2 does not change behavior.

Max Kirillov (2):
  submodule refactor: use git_path_submodule() in add_submodule_odb()
  path: implement common_dir handling in git_path_submodule()

 cache.h                          |  1 +
 path.c                           | 24 ++++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 submodule.c                      | 28 ++++++++++------------------
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 5 files changed, 53 insertions(+), 27 deletions(-)

-- 
2.3.4.2801.g3d0809b

[PATCH v7 1/2] submodule refactor: use git_path_submodule() in add_submodule_odb()

[Max Kirillov]

Functions which directly operate submodule's object database do not handle the
case when the submodule is linked worktree (which are introduced in
c7b3a3d2fe). Instead of fixing the path calculation use already existing
git_path_submodule() function without changing overall behavior. Then it will
be possible to modify only that function whenever we need to change real
location of submodule's repository content.

Signed-off-by: Max Kirillov <max@max630.net>
---
 submodule.c | 28 ++++++++++------------------
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/submodule.c b/submodule.c
index 15e90d1..70d18ec 100644
--- a/submodule.c
+++ b/submodule.c
@@ -122,43 +122,35 @@ void stage_updated_gitmodules(void)
 
 static int add_submodule_odb(const char *path)
 {
-	struct strbuf objects_directory = STRBUF_INIT;
 	struct alternate_object_database *alt_odb;
+	const char *objects_directory;
 	int ret = 0;
-	const char *git_dir;
 
-	strbuf_addf(&objects_directory, "%s/.git", path);
-	git_dir = read_gitfile(objects_directory.buf);
-	if (git_dir) {
-		strbuf_reset(&objects_directory);
-		strbuf_addstr(&objects_directory, git_dir);
-	}
-	strbuf_addstr(&objects_directory, "/objects/");
-	if (!is_directory(objects_directory.buf)) {
+	objects_directory = git_path_submodule(path, "objects/");
+	if (!is_directory(objects_directory)) {
 		ret = -1;
 		goto done;
 	}
+
 	/* avoid adding it twice */
 	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
-		if (alt_odb->name - alt_odb->base == objects_directory.len &&
-				!strncmp(alt_odb->base, objects_directory.buf,
-					objects_directory.len))
+		if (alt_odb->name - alt_odb->base == strlen(objects_directory) &&
+				!strcmp(alt_odb->base, objects_directory))
 			goto done;
 
-	alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
+	alt_odb = xmalloc(strlen(objects_directory) + 42 + sizeof(*alt_odb));
 	alt_odb->next = alt_odb_list;
-	strcpy(alt_odb->base, objects_directory.buf);
-	alt_odb->name = alt_odb->base + objects_directory.len;
+	strcpy(alt_odb->base, objects_directory);
+	alt_odb->name = alt_odb->base + strlen(objects_directory);
 	alt_odb->name[2] = '/';
 	alt_odb->name[40] = '\0';
 	alt_odb->name[41] = '\0';
 	alt_odb_list = alt_odb;
 
 	/* add possible alternates from the submodule */
-	read_info_alternates(objects_directory.buf, 0);
+	read_info_alternates(objects_directory, 0);
 	prepare_alt_odb();
 done:
-	strbuf_release(&objects_directory);
 	return ret;
 }
 
-- 
2.3.4.2801.g3d0809b

[PATCH v7 2/2] path: implement common_dir handling in git_path_submodule()

[Max Kirillov]

When submodule is a linked worktree, "git diff --submodule" and other
calls which directly access the submodule's object database do not correctly
calculate its path. Fix it by changing the git_path_submodule() behavior,
to use either common or per-worktree directory.

Do it similarly as for parent repository, but ignore the GIT_COMMON_DIR
environment variable, because it would mean common directory for the parent
repository and does not make sense for submodule.

Also add test for functionality which uses this call.

Signed-off-by: Max Kirillov <max@max630.net>
---
 cache.h                          |  1 +
 path.c                           | 24 ++++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 4 files changed, 43 insertions(+), 9 deletions(-)

diff --git a/cache.h b/cache.h
index 4f55466..b87ec75 100644
--- a/cache.h
+++ b/cache.h
@@ -442,6 +442,7 @@ extern char *get_object_directory(void);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
+extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
 extern const char *get_git_namespace(void);
 extern const char *strip_namespace(const char *namespaced_ref);
diff --git a/path.c b/path.c
index 10f4cbf..b0cf444 100644
--- a/path.c
+++ b/path.c
@@ -98,7 +98,7 @@ static const char *common_list[] = {
 	NULL
 };
 
-static void update_common_dir(struct strbuf *buf, int git_dir_len)
+static void update_common_dir(struct strbuf *buf, int git_dir_len, const char *common_dir)
 {
 	char *base = buf->buf + git_dir_len;
 	const char **p;
@@ -115,12 +115,17 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len)
 			path++;
 			is_dir = 1;
 		}
+
+		if (!common_dir) {
+			common_dir = get_git_common_dir();
+		}
+
 		if (is_dir && dir_prefix(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 		if (!is_dir && !strcmp(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 	}
@@ -160,7 +165,7 @@ static void adjust_git_path(struct strbuf *buf, int git_dir_len)
 	else if (git_db_env && dir_prefix(base, "objects"))
 		replace_dir(buf, git_dir_len + 7, get_object_directory());
 	else if (git_common_dir_env)
-		update_common_dir(buf, git_dir_len);
+		update_common_dir(buf, git_dir_len, NULL);
 }
 
 static void do_git_path(struct strbuf *buf, const char *fmt, va_list args)
@@ -228,6 +233,8 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)
 {
 	struct strbuf *buf = get_pathname();
 	const char *git_dir;
+	struct strbuf git_submodule_common_dir = STRBUF_INIT;
+	struct strbuf git_submodule_dir = STRBUF_INIT;
 	va_list args;
 
 	strbuf_addstr(buf, path);
@@ -241,11 +248,20 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)
 		strbuf_addstr(buf, git_dir);
 	}
 	strbuf_addch(buf, '/');
+	strbuf_addstr(&git_submodule_dir, buf->buf);
 
 	va_start(args, fmt);
 	strbuf_vaddf(buf, fmt, args);
 	va_end(args);
+
+	if (get_common_dir_noenv(&git_submodule_common_dir, git_submodule_dir.buf)) {
+		update_common_dir(buf, git_submodule_dir.len, git_submodule_common_dir.buf);
+	}
+
 	strbuf_cleanup_path(buf);
+
+	strbuf_release(&git_submodule_dir);
+	strbuf_release(&git_submodule_common_dir);
 	return buf->buf;
 }
 
diff --git a/setup.c b/setup.c
index 82c0cc2..39ea06b 100644
--- a/setup.c
+++ b/setup.c
@@ -229,14 +229,21 @@ void verify_non_filename(const char *prefix, const char *arg)
 
 int get_common_dir(struct strbuf *sb, const char *gitdir)
 {
+	const char *git_env_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
+	if (git_env_common_dir) {
+		strbuf_addstr(sb, git_env_common_dir);
+		return 1;
+	} else {
+		return get_common_dir_noenv(sb, gitdir);
+	}
+}
+
+int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
+{
 	struct strbuf data = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
-	const char *git_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
 	int ret = 0;
-	if (git_common_dir) {
-		strbuf_addstr(sb, git_common_dir);
-		return 1;
-	}
+
 	strbuf_addf(&path, "%s/commondir", gitdir);
 	if (file_exists(path.buf)) {
 		if (strbuf_read_file(&data, path.buf, 0) <= 0)
diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh
index 3f609e8..1acef32 100755
--- a/t/t7410-submodule-checkout-to.sh
+++ b/t/t7410-submodule-checkout-to.sh
@@ -47,4 +47,14 @@ test_expect_success 'checkout main and initialize independed clones' \
 test_expect_success 'can see submodule diffs after independed cloning' \
     '(cd fully_cloned_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
 
+test_expect_success 'checkout sub manually' \
+    'mkdir linked_submodule &&
+    (cd clone/main &&
+	git worktree add "$base_path/linked_submodule/main" "$rev1_hash_main") &&
+    (cd clone/main/sub &&
+	git worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub")'
+
+test_expect_success 'can see submodule diffs after manual checkout of linked submodule' \
+    '(cd linked_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
+
 test_done
-- 
2.3.4.2801.g3d0809b

Re: [PATCH v7 2/2] path: implement common_dir handling in git_path_submodule()

[Stefan Beller]

On Tue, Aug 4, 2015 at 3:05 PM, Max Kirillov <max@max630.net> wrote:
> When submodule is a linked worktree, "git diff --submodule" and other
> calls which directly access the submodule's object database do not correctly
> calculate its path. Fix it by changing the git_path_submodule() behavior,
> to use either common or per-worktree directory.
>
> Do it similarly as for parent repository, but ignore the GIT_COMMON_DIR
> environment variable, because it would mean common directory for the parent
> repository and does not make sense for submodule.
>
> Also add test for functionality which uses this call.
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
>  cache.h                          |  1 +
>  path.c                           | 24 ++++++++++++++++++++----
>  setup.c                          | 17 ++++++++++++-----
>  t/t7410-submodule-checkout-to.sh | 10 ++++++++++
>  4 files changed, 43 insertions(+), 9 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 4f55466..b87ec75 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -442,6 +442,7 @@ extern char *get_object_directory(void);
>  extern char *get_index_file(void);
>  extern char *get_graft_file(void);
>  extern int set_git_dir(const char *path);
> +extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
>  extern int get_common_dir(struct strbuf *sb, const char *gitdir);
>  extern const char *get_git_namespace(void);
>  extern const char *strip_namespace(const char *namespaced_ref);
> diff --git a/path.c b/path.c
> index 10f4cbf..b0cf444 100644
> --- a/path.c
> +++ b/path.c
> @@ -98,7 +98,7 @@ static const char *common_list[] = {
>         NULL
>  };
>
> -static void update_common_dir(struct strbuf *buf, int git_dir_len)
> +static void update_common_dir(struct strbuf *buf, int git_dir_len, const char *common_dir)
>  {
>         char *base = buf->buf + git_dir_len;
>         const char **p;
> @@ -115,12 +115,17 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len)
>                         path++;
>                         is_dir = 1;
>                 }
> +
> +               if (!common_dir) {
> +                       common_dir = get_git_common_dir();
> +               }

Usually we omit the braces for blocks with just one command.

> +
>                 if (is_dir && dir_prefix(base, path)) {
> -                       replace_dir(buf, git_dir_len, get_git_common_dir());
> +                       replace_dir(buf, git_dir_len, common_dir);
>                         return;
>                 }
>                 if (!is_dir && !strcmp(base, path)) {
> -                       replace_dir(buf, git_dir_len, get_git_common_dir());
> +                       replace_dir(buf, git_dir_len, common_dir);
>                         return;
>                 }
>         }
> @@ -160,7 +165,7 @@ static void adjust_git_path(struct strbuf *buf, int git_dir_len)
>         else if (git_db_env && dir_prefix(base, "objects"))
>                 replace_dir(buf, git_dir_len + 7, get_object_directory());
>         else if (git_common_dir_env)
> -               update_common_dir(buf, git_dir_len);
> +               update_common_dir(buf, git_dir_len, NULL);
>  }
>
>  static void do_git_path(struct strbuf *buf, const char *fmt, va_list args)
> @@ -228,6 +233,8 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)
>  {
>         struct strbuf *buf = get_pathname();
>         const char *git_dir;
> +       struct strbuf git_submodule_common_dir = STRBUF_INIT;
> +       struct strbuf git_submodule_dir = STRBUF_INIT;
>         va_list args;
>
>         strbuf_addstr(buf, path);
> @@ -241,11 +248,20 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)
>                 strbuf_addstr(buf, git_dir);
>         }
>         strbuf_addch(buf, '/');
> +       strbuf_addstr(&git_submodule_dir, buf->buf);
>
>         va_start(args, fmt);
>         strbuf_vaddf(buf, fmt, args);
>         va_end(args);
> +
> +       if (get_common_dir_noenv(&git_submodule_common_dir, git_submodule_dir.buf)) {
> +               update_common_dir(buf, git_submodule_dir.len, git_submodule_common_dir.buf);
> +       }
> +
>         strbuf_cleanup_path(buf);
> +
> +       strbuf_release(&git_submodule_dir);
> +       strbuf_release(&git_submodule_common_dir);
>         return buf->buf;
>  }
>
> diff --git a/setup.c b/setup.c
> index 82c0cc2..39ea06b 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -229,14 +229,21 @@ void verify_non_filename(const char *prefix, const char *arg)
>
>  int get_common_dir(struct strbuf *sb, const char *gitdir)
>  {
> +       const char *git_env_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
> +       if (git_env_common_dir) {
> +               strbuf_addstr(sb, git_env_common_dir);
> +               return 1;
> +       } else {
> +               return get_common_dir_noenv(sb, gitdir);
> +       }
> +}
> +
> +int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> +{
>         struct strbuf data = STRBUF_INIT;
>         struct strbuf path = STRBUF_INIT;
> -       const char *git_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
>         int ret = 0;
> -       if (git_common_dir) {
> -               strbuf_addstr(sb, git_common_dir);
> -               return 1;
> -       }
> +
>         strbuf_addf(&path, "%s/commondir", gitdir);
>         if (file_exists(path.buf)) {
>                 if (strbuf_read_file(&data, path.buf, 0) <= 0)
> diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh
> index 3f609e8..1acef32 100755
> --- a/t/t7410-submodule-checkout-to.sh
> +++ b/t/t7410-submodule-checkout-to.sh
> @@ -47,4 +47,14 @@ test_expect_success 'checkout main and initialize independed clones' \
>  test_expect_success 'can see submodule diffs after independed cloning' \
>      '(cd fully_cloned_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
>
> +test_expect_success 'checkout sub manually' \
> +    'mkdir linked_submodule &&
> +    (cd clone/main &&
> +       git worktree add "$base_path/linked_submodule/main" "$rev1_hash_main") &&
> +    (cd clone/main/sub &&
> +       git worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub")'
> +
> +test_expect_success 'can see submodule diffs after manual checkout of linked submodule' \
> +    '(cd linked_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
> +
>  test_done
> --
> 2.3.4.2801.g3d0809b

Looks good to me otherwise.

>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/2] submodule refactor: use git_path_submodule() in add_submodule_odb()

[Stefan Beller]

On Tue, Aug 4, 2015 at 3:05 PM, Max Kirillov <max@max630.net> wrote:
> Functions which directly operate submodule's object database do not handle the
> case when the submodule is linked worktree (which are introduced in
> c7b3a3d2fe). Instead of fixing the path calculation use already existing
> git_path_submodule() function without changing overall behavior. Then it will
> be possible to modify only that function whenever we need to change real
> location of submodule's repository content.
>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
>  submodule.c | 28 ++++++++++------------------
>  1 file changed, 10 insertions(+), 18 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 15e90d1..70d18ec 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -122,43 +122,35 @@ void stage_updated_gitmodules(void)
>
>  static int add_submodule_odb(const char *path)
>  {
> -       struct strbuf objects_directory = STRBUF_INIT;
>         struct alternate_object_database *alt_odb;
> +       const char *objects_directory;
>         int ret = 0;
> -       const char *git_dir;
>
> -       strbuf_addf(&objects_directory, "%s/.git", path);
> -       git_dir = read_gitfile(objects_directory.buf);
> -       if (git_dir) {
> -               strbuf_reset(&objects_directory);
> -               strbuf_addstr(&objects_directory, git_dir);
> -       }
> -       strbuf_addstr(&objects_directory, "/objects/");
> -       if (!is_directory(objects_directory.buf)) {
> +       objects_directory = git_path_submodule(path, "objects/");

git_path_submodule is going away with  jk/git-path (2015-08-10)
So you may want to use git_pathdup_submodule and free the
result of that function before returning.

> +       if (!is_directory(objects_directory)) {
>                 ret = -1;
>                 goto done;
>         }
> +
>         /* avoid adding it twice */
>         for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
> -               if (alt_odb->name - alt_odb->base == objects_directory.len &&
> -                               !strncmp(alt_odb->base, objects_directory.buf,
> -                                       objects_directory.len))
> +               if (alt_odb->name - alt_odb->base == strlen(objects_directory) &&
> +                               !strcmp(alt_odb->base, objects_directory))
>                         goto done;
>
> -       alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
> +       alt_odb = xmalloc(strlen(objects_directory) + 42 + sizeof(*alt_odb));
>         alt_odb->next = alt_odb_list;
> -       strcpy(alt_odb->base, objects_directory.buf);
> -       alt_odb->name = alt_odb->base + objects_directory.len;
> +       strcpy(alt_odb->base, objects_directory);
> +       alt_odb->name = alt_odb->base + strlen(objects_directory);
>         alt_odb->name[2] = '/';
>         alt_odb->name[40] = '\0';
>         alt_odb->name[41] = '\0';
>         alt_odb_list = alt_odb;
>
>         /* add possible alternates from the submodule */
> -       read_info_alternates(objects_directory.buf, 0);
> +       read_info_alternates(objects_directory, 0);
>         prepare_alt_odb();
>  done:
> -       strbuf_release(&objects_directory);
>         return ret;
>  }
>

This looks good, though I am diving into the submodule code base
myself just now,
so it's not a strong review. :)

> --
> 2.3.4.2801.g3d0809b
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v8 0/2] Submodule object path

[Max Kirillov]

* Rebased to recent master (v2.6.0-rc0).
* Use git_pathdup_submodule() instead of git_path_submodule()
* There are more conflicts in pu with [1], not sure
  what should I do about it.
* Style fixes as Stefan suggested

[1] http://thread.gmane.org/gmane.comp.version-control.git/276628

Max Kirillov (2):
  submodule refactor: use git_pathdup_submodule() in add_submodule_odb()
  path: implement common_dir handling in git_pathdup_submodule()

 cache.h                          |  1 +
 path.c                           | 22 ++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 submodule.c                      | 30 ++++++++++++------------------
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 5 files changed, 53 insertions(+), 27 deletions(-)

-- 
2.3.4.2801.g3d0809b

[PATCH v8 1/2] submodule refactor: use git_pathdup_submodule() in add_submodule_odb()

[Max Kirillov]

Functions which directly operate submodule's object database do not handle the
case when the submodule is linked worktree (which are introduced in
c7b3a3d2fe). Instead of fixing the path calculation use already existing
git_pathdup_submodule() function without changing overall behavior. Then it
will be possible to modify only that function whenever we need to change real
location of submodule's repository content.

Signed-off-by: Max Kirillov <max@max630.net>
---
 submodule.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/submodule.c b/submodule.c
index 245ed4d..7340069 100644
--- a/submodule.c
+++ b/submodule.c
@@ -119,43 +119,37 @@ void stage_updated_gitmodules(void)
 
 static int add_submodule_odb(const char *path)
 {
-	struct strbuf objects_directory = STRBUF_INIT;
 	struct alternate_object_database *alt_odb;
+	char *objects_directory;
 	int ret = 0;
-	const char *git_dir;
 
-	strbuf_addf(&objects_directory, "%s/.git", path);
-	git_dir = read_gitfile(objects_directory.buf);
-	if (git_dir) {
-		strbuf_reset(&objects_directory);
-		strbuf_addstr(&objects_directory, git_dir);
-	}
-	strbuf_addstr(&objects_directory, "/objects/");
-	if (!is_directory(objects_directory.buf)) {
+	objects_directory = git_pathdup_submodule(path, "objects/");
+	if (!is_directory(objects_directory)) {
 		ret = -1;
 		goto done;
 	}
+
 	/* avoid adding it twice */
 	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
-		if (alt_odb->name - alt_odb->base == objects_directory.len &&
-				!strncmp(alt_odb->base, objects_directory.buf,
-					objects_directory.len))
+		if (alt_odb->name - alt_odb->base == strlen(objects_directory) &&
+				!strcmp(alt_odb->base, objects_directory))
 			goto done;
 
-	alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
+	alt_odb = xmalloc(strlen(objects_directory) + 42 + sizeof(*alt_odb));
 	alt_odb->next = alt_odb_list;
-	strcpy(alt_odb->base, objects_directory.buf);
-	alt_odb->name = alt_odb->base + objects_directory.len;
+	strcpy(alt_odb->base, objects_directory);
+	alt_odb->name = alt_odb->base + strlen(objects_directory);
 	alt_odb->name[2] = '/';
 	alt_odb->name[40] = '\0';
 	alt_odb->name[41] = '\0';
 	alt_odb_list = alt_odb;
 
 	/* add possible alternates from the submodule */
-	read_info_alternates(objects_directory.buf, 0);
+	read_info_alternates(objects_directory, 0);
 	prepare_alt_odb();
 done:
-	strbuf_release(&objects_directory);
+	free(objects_directory);
+
 	return ret;
 }
 
-- 
2.3.4.2801.g3d0809b

[PATCH v8 2/2] path: implement common_dir handling in git_pathdup_submodule()

[Max Kirillov]

When submodule is a linked worktree, "git diff --submodule" and other
calls which directly access the submodule's object database do not correctly
calculate its path. Fix it by changing the git_pathdup_submodule() behavior,
to use either common or per-worktree directory.

Do it similarly as for parent repository, but ignore the GIT_COMMON_DIR
environment variable, because it would mean common directory for the parent
repository and does not make sense for submodule.

Also add test for functionality which uses this call.

Signed-off-by: Max Kirillov <max@max630.net>
---
 cache.h                          |  1 +
 path.c                           | 22 ++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 4 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/cache.h b/cache.h
index 79066e5..5eb36b4 100644
--- a/cache.h
+++ b/cache.h
@@ -443,6 +443,7 @@ extern char *get_object_directory(void);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
+extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
 extern const char *get_git_namespace(void);
 extern const char *strip_namespace(const char *namespaced_ref);
diff --git a/path.c b/path.c
index 95acbaf..22b9e0b 100644
--- a/path.c
+++ b/path.c
@@ -98,7 +98,7 @@ static const char *common_list[] = {
 	NULL
 };
 
-static void update_common_dir(struct strbuf *buf, int git_dir_len)
+static void update_common_dir(struct strbuf *buf, int git_dir_len, const char *common_dir)
 {
 	char *base = buf->buf + git_dir_len;
 	const char **p;
@@ -115,12 +115,16 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len)
 			path++;
 			is_dir = 1;
 		}
+
+		if (!common_dir)
+			common_dir = get_git_common_dir();
+
 		if (is_dir && dir_prefix(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 		if (!is_dir && !strcmp(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 	}
@@ -160,7 +164,7 @@ static void adjust_git_path(struct strbuf *buf, int git_dir_len)
 	else if (git_db_env && dir_prefix(base, "objects"))
 		replace_dir(buf, git_dir_len + 7, get_object_directory());
 	else if (git_common_dir_env)
-		update_common_dir(buf, git_dir_len);
+		update_common_dir(buf, git_dir_len, NULL);
 }
 
 static void do_git_path(struct strbuf *buf, const char *fmt, va_list args)
@@ -228,6 +232,8 @@ static void do_submodule_path(struct strbuf *buf, const char *path,
 			      const char *fmt, va_list args)
 {
 	const char *git_dir;
+	struct strbuf git_submodule_common_dir = STRBUF_INIT;
+	struct strbuf git_submodule_dir = STRBUF_INIT;
 
 	strbuf_addstr(buf, path);
 	if (buf->len && buf->buf[buf->len - 1] != '/')
@@ -240,9 +246,17 @@ static void do_submodule_path(struct strbuf *buf, const char *path,
 		strbuf_addstr(buf, git_dir);
 	}
 	strbuf_addch(buf, '/');
+	strbuf_addstr(&git_submodule_dir, buf->buf);
 
 	strbuf_vaddf(buf, fmt, args);
+
+	if (get_common_dir_noenv(&git_submodule_common_dir, git_submodule_dir.buf))
+		update_common_dir(buf, git_submodule_dir.len, git_submodule_common_dir.buf);
+
 	strbuf_cleanup_path(buf);
+
+	strbuf_release(&git_submodule_dir);
+	strbuf_release(&git_submodule_common_dir);
 }
 
 char *git_pathdup_submodule(const char *path, const char *fmt, ...)
diff --git a/setup.c b/setup.c
index a17c51e..e41e5e1 100644
--- a/setup.c
+++ b/setup.c
@@ -229,14 +229,21 @@ void verify_non_filename(const char *prefix, const char *arg)
 
 int get_common_dir(struct strbuf *sb, const char *gitdir)
 {
+	const char *git_env_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
+	if (git_env_common_dir) {
+		strbuf_addstr(sb, git_env_common_dir);
+		return 1;
+	} else {
+		return get_common_dir_noenv(sb, gitdir);
+	}
+}
+
+int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
+{
 	struct strbuf data = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
-	const char *git_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
 	int ret = 0;
-	if (git_common_dir) {
-		strbuf_addstr(sb, git_common_dir);
-		return 1;
-	}
+
 	strbuf_addf(&path, "%s/commondir", gitdir);
 	if (file_exists(path.buf)) {
 		if (strbuf_read_file(&data, path.buf, 0) <= 0)
diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh
index 3f609e8..1acef32 100755
--- a/t/t7410-submodule-checkout-to.sh
+++ b/t/t7410-submodule-checkout-to.sh
@@ -47,4 +47,14 @@ test_expect_success 'checkout main and initialize independed clones' \
 test_expect_success 'can see submodule diffs after independed cloning' \
     '(cd fully_cloned_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
 
+test_expect_success 'checkout sub manually' \
+    'mkdir linked_submodule &&
+    (cd clone/main &&
+	git worktree add "$base_path/linked_submodule/main" "$rev1_hash_main") &&
+    (cd clone/main/sub &&
+	git worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub")'
+
+test_expect_success 'can see submodule diffs after manual checkout of linked submodule' \
+    '(cd linked_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
+
 test_done
-- 
2.3.4.2801.g3d0809b

Re: [PATCH v8 0/2] Submodule object path

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> * There are more conflicts in pu with [1], not sure
>   what should I do about it.
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/276628

I do not think conflicts got any worse than before.  Between the
result of merging this round and the previous one to 'pu', the only
difference in path.c that conflicts with [1] I see is essentially a
no-op (the diff attached at the end of this essage).

When I push the updated 'pu' out, could you please check

    $ git log --oneline --first-parent origin/master..origin/pu

to find the place your topic is merged (I am planning to make it
"3af7cd6 Merge branch 'mk/submodule-gitdir-path' into pu", but
please do not rely on the exact commit object name; I may have to
redo the integration if I find some breakages caused by this or any
other topic), and then eyeballing (call that merge $m) output of

    $ git diff $m^ $m

to see if there is anything I screwed up?

Thanks.


diff --git a/path.c b/path.c
index 776d8f0..2725bc3 100644
--- a/path.c
+++ b/path.c
@@ -466,11 +466,11 @@ static void do_submodule_path(struct strbuf *buf, const char *path,
 
 	strbuf_vaddf(buf, fmt, args);
 
-	if (get_common_dir_noenv(&git_submodule_common_dir, git_submodule_dir.buf)) {
+	if (get_common_dir_noenv(&git_submodule_common_dir, git_submodule_dir.buf))
 		update_common_dir(buf, git_submodule_dir.len, git_submodule_common_dir.buf);
-	}
 
 	strbuf_cleanup_path(buf);
+
 	strbuf_release(&git_submodule_dir);
 	strbuf_release(&git_submodule_common_dir);
 }

Re: [PATCH v8 1/2] submodule refactor: use git_pathdup_submodule() in add_submodule_odb()

[Jeff King]

On Fri, Sep 11, 2015 at 12:57:10AM +0300, Max Kirillov wrote:

> diff --git a/submodule.c b/submodule.c
> index 245ed4d..7340069 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -119,43 +119,37 @@ void stage_updated_gitmodules(void)
>  
>  static int add_submodule_odb(const char *path)
>  {
> -	struct strbuf objects_directory = STRBUF_INIT;
>  	struct alternate_object_database *alt_odb;
> +	char *objects_directory;

Now that we have strbuf_git_path_submodule(), is there any reason to
switch this away from a strbuf?

That saves us a bunch of strlen calls, and it makes the diff way
shorter. My ulterior motive is that the result also conflicts a lot less
with some patches I'm about to post to harden the malloc and strcpy
calls below.

That would make your patch look something like:

diff --git a/submodule.c b/submodule.c
index 245ed4d..5e5a46f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -122,15 +122,8 @@ static int add_submodule_odb(const char *path)
 	struct strbuf objects_directory = STRBUF_INIT;
 	struct alternate_object_database *alt_odb;
 	int ret = 0;
-	const char *git_dir;
 
-	strbuf_addf(&objects_directory, "%s/.git", path);
-	git_dir = read_gitfile(objects_directory.buf);
-	if (git_dir) {
-		strbuf_reset(&objects_directory);
-		strbuf_addstr(&objects_directory, git_dir);
-	}
-	strbuf_addstr(&objects_directory, "/objects/");
+	strbuf_git_path_submodule(&objects_directory, path, "objects/");
 	if (!is_directory(objects_directory.buf)) {
 		ret = -1;
 		goto done;

-Peff

[PATCH v9 0/2] Submodule object path

[Max Kirillov]

Since v8:
Use strbuf_git_path_submodule() as Jeff suggested

Max Kirillov (2):
  submodule refactor: use strbuf_git_path_submodule() in
    add_submodule_odb()
  path: implement common_dir handling in git_pathdup_submodule()

 cache.h                          |  1 +
 path.c                           | 22 ++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 submodule.c                      |  8 +-------
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 5 files changed, 42 insertions(+), 16 deletions(-)

-- 
2.3.4.2801.g3d0809b

[PATCH v9 1/2] submodule refactor: use strbuf_git_path_submodule() in add_submodule_odb()

[Max Kirillov]

Functions which directly operate submodule's object database do not
handle the case when the submodule is linked worktree (which are
introduced in c7b3a3d2fe). Instead of fixing the path calculation use
already existing strbuf_git_path_submodule() function without changing
overall behaviour. Then it will be possible to modify only that function
whenever we need to change real location of submodule's repository
content.

Edited-by: Jeff King <peff@peff.net>
Signed-off-by: Max Kirillov <max@max630.net>
---
 submodule.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/submodule.c b/submodule.c
index 245ed4d..16b7c35 100644
--- a/submodule.c
+++ b/submodule.c
@@ -124,13 +124,7 @@ static int add_submodule_odb(const char *path)
 	int ret = 0;
 	const char *git_dir;
 
-	strbuf_addf(&objects_directory, "%s/.git", path);
-	git_dir = read_gitfile(objects_directory.buf);
-	if (git_dir) {
-		strbuf_reset(&objects_directory);
-		strbuf_addstr(&objects_directory, git_dir);
-	}
-	strbuf_addstr(&objects_directory, "/objects/");
+	strbuf_git_path_submodule(&objects_directory, path, "objects/");
 	if (!is_directory(objects_directory.buf)) {
 		ret = -1;
 		goto done;
-- 
2.3.4.2801.g3d0809b

[PATCH v9 2/2] path: implement common_dir handling in git_pathdup_submodule()

[Max Kirillov]

When submodule is a linked worktree, "git diff --submodule" and other
calls which directly access the submodule's object database do not correctly
calculate its path. Fix it by changing the git_pathdup_submodule() behavior,
to use either common or per-worktree directory.

Do it similarly as for parent repository, but ignore the GIT_COMMON_DIR
environment variable, because it would mean common directory for the parent
repository and does not make sense for submodule.

Also add test for functionality which uses this call.

Signed-off-by: Max Kirillov <max@max630.net>
---
 cache.h                          |  1 +
 path.c                           | 22 ++++++++++++++++++----
 setup.c                          | 17 ++++++++++++-----
 t/t7410-submodule-checkout-to.sh | 10 ++++++++++
 4 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/cache.h b/cache.h
index 79066e5..5eb36b4 100644
--- a/cache.h
+++ b/cache.h
@@ -443,6 +443,7 @@ extern char *get_object_directory(void);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
+extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir);
 extern int get_common_dir(struct strbuf *sb, const char *gitdir);
 extern const char *get_git_namespace(void);
 extern const char *strip_namespace(const char *namespaced_ref);
diff --git a/path.c b/path.c
index 95acbaf..22b9e0b 100644
--- a/path.c
+++ b/path.c
@@ -98,7 +98,7 @@ static const char *common_list[] = {
 	NULL
 };
 
-static void update_common_dir(struct strbuf *buf, int git_dir_len)
+static void update_common_dir(struct strbuf *buf, int git_dir_len, const char *common_dir)
 {
 	char *base = buf->buf + git_dir_len;
 	const char **p;
@@ -115,12 +115,16 @@ static void update_common_dir(struct strbuf *buf, int git_dir_len)
 			path++;
 			is_dir = 1;
 		}
+
+		if (!common_dir)
+			common_dir = get_git_common_dir();
+
 		if (is_dir && dir_prefix(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 		if (!is_dir && !strcmp(base, path)) {
-			replace_dir(buf, git_dir_len, get_git_common_dir());
+			replace_dir(buf, git_dir_len, common_dir);
 			return;
 		}
 	}
@@ -160,7 +164,7 @@ static void adjust_git_path(struct strbuf *buf, int git_dir_len)
 	else if (git_db_env && dir_prefix(base, "objects"))
 		replace_dir(buf, git_dir_len + 7, get_object_directory());
 	else if (git_common_dir_env)
-		update_common_dir(buf, git_dir_len);
+		update_common_dir(buf, git_dir_len, NULL);
 }
 
 static void do_git_path(struct strbuf *buf, const char *fmt, va_list args)
@@ -228,6 +232,8 @@ static void do_submodule_path(struct strbuf *buf, const char *path,
 			      const char *fmt, va_list args)
 {
 	const char *git_dir;
+	struct strbuf git_submodule_common_dir = STRBUF_INIT;
+	struct strbuf git_submodule_dir = STRBUF_INIT;
 
 	strbuf_addstr(buf, path);
 	if (buf->len && buf->buf[buf->len - 1] != '/')
@@ -240,9 +246,17 @@ static void do_submodule_path(struct strbuf *buf, const char *path,
 		strbuf_addstr(buf, git_dir);
 	}
 	strbuf_addch(buf, '/');
+	strbuf_addstr(&git_submodule_dir, buf->buf);
 
 	strbuf_vaddf(buf, fmt, args);
+
+	if (get_common_dir_noenv(&git_submodule_common_dir, git_submodule_dir.buf))
+		update_common_dir(buf, git_submodule_dir.len, git_submodule_common_dir.buf);
+
 	strbuf_cleanup_path(buf);
+
+	strbuf_release(&git_submodule_dir);
+	strbuf_release(&git_submodule_common_dir);
 }
 
 char *git_pathdup_submodule(const char *path, const char *fmt, ...)
diff --git a/setup.c b/setup.c
index a17c51e..e41e5e1 100644
--- a/setup.c
+++ b/setup.c
@@ -229,14 +229,21 @@ void verify_non_filename(const char *prefix, const char *arg)
 
 int get_common_dir(struct strbuf *sb, const char *gitdir)
 {
+	const char *git_env_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
+	if (git_env_common_dir) {
+		strbuf_addstr(sb, git_env_common_dir);
+		return 1;
+	} else {
+		return get_common_dir_noenv(sb, gitdir);
+	}
+}
+
+int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
+{
 	struct strbuf data = STRBUF_INIT;
 	struct strbuf path = STRBUF_INIT;
-	const char *git_common_dir = getenv(GIT_COMMON_DIR_ENVIRONMENT);
 	int ret = 0;
-	if (git_common_dir) {
-		strbuf_addstr(sb, git_common_dir);
-		return 1;
-	}
+
 	strbuf_addf(&path, "%s/commondir", gitdir);
 	if (file_exists(path.buf)) {
 		if (strbuf_read_file(&data, path.buf, 0) <= 0)
diff --git a/t/t7410-submodule-checkout-to.sh b/t/t7410-submodule-checkout-to.sh
index 3f609e8..1acef32 100755
--- a/t/t7410-submodule-checkout-to.sh
+++ b/t/t7410-submodule-checkout-to.sh
@@ -47,4 +47,14 @@ test_expect_success 'checkout main and initialize independed clones' \
 test_expect_success 'can see submodule diffs after independed cloning' \
     '(cd fully_cloned_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
 
+test_expect_success 'checkout sub manually' \
+    'mkdir linked_submodule &&
+    (cd clone/main &&
+	git worktree add "$base_path/linked_submodule/main" "$rev1_hash_main") &&
+    (cd clone/main/sub &&
+	git worktree add "$base_path/linked_submodule/main/sub" "$rev1_hash_sub")'
+
+test_expect_success 'can see submodule diffs after manual checkout of linked submodule' \
+    '(cd linked_submodule/main && git diff --submodule master"^!" | grep "file1 updated")'
+
 test_done
-- 
2.3.4.2801.g3d0809b

Re: [PATCH v8 1/2] submodule refactor: use git_pathdup_submodule() in add_submodule_odb()

[Max Kirillov]

On Fri, Sep 11, 2015 at 03:53:40AM -0400, Jeff King wrote:
> 
> Now that we have strbuf_git_path_submodule(), is there any reason to
> switch this away from a strbuf?
> 
> That saves us a bunch of strlen calls, and it makes the diff way
> shorter. My ulterior motive is that the result also conflicts a lot less
> with some patches I'm about to post to harden the malloc and strcpy
> calls below.
> 
> That would make your patch look something like:
> 
> diff --git a/submodule.c b/submodule.c
> index 245ed4d..5e5a46f 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -122,15 +122,8 @@ static int add_submodule_odb(const char *path)
>  	struct strbuf objects_directory = STRBUF_INIT;
>  	struct alternate_object_database *alt_odb;
>  	int ret = 0;
> -	const char *git_dir;
>  
> -	strbuf_addf(&objects_directory, "%s/.git", path);
> -	git_dir = read_gitfile(objects_directory.buf);
> -	if (git_dir) {
> -		strbuf_reset(&objects_directory);
> -		strbuf_addstr(&objects_directory, git_dir);
> -	}
> -	strbuf_addstr(&objects_directory, "/objects/");
> +	strbuf_git_path_submodule(&objects_directory, path, "objects/");
>  	if (!is_directory(objects_directory.buf)) {
>  		ret = -1;
>  		goto done;
> 
> -Peff

Thank you. This is how it looks now.

-- 
Max

Re: [PATCH v8 0/2] Submodule object path

[Max Kirillov]

On Thu, Sep 10, 2015 at 06:10:13PM -0700, Junio C Hamano wrote:
> When I push the updated 'pu' out, could you please check

I follow the pu merges. So far resolutions seem correct and
all tests pass. If you don't mind re-resolving it as I send
newer versions I will base them on master.

Thank you.

-- 
Max

Re: [PATCH v9 1/2] submodule refactor: use strbuf_git_path_submodule() in add_submodule_odb()

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> Functions which directly operate submodule's object database do not
> handle the case when the submodule is linked worktree (which are
> introduced in c7b3a3d2fe). Instead of fixing the path calculation use
> already existing strbuf_git_path_submodule() function without changing
> overall behaviour. Then it will be possible to modify only that function
> whenever we need to change real location of submodule's repository
> content.
>
> Edited-by: Jeff King <peff@peff.net>
> Signed-off-by: Max Kirillov <max@max630.net>
> ---
>  submodule.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)

This certainly is a lot nicer ;-)

Thanks.

>
> diff --git a/submodule.c b/submodule.c
> index 245ed4d..16b7c35 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -124,13 +124,7 @@ static int add_submodule_odb(const char *path)
>  	int ret = 0;
>  	const char *git_dir;
>  
> -	strbuf_addf(&objects_directory, "%s/.git", path);
> -	git_dir = read_gitfile(objects_directory.buf);
> -	if (git_dir) {
> -		strbuf_reset(&objects_directory);
> -		strbuf_addstr(&objects_directory, git_dir);
> -	}
> -	strbuf_addstr(&objects_directory, "/objects/");
> +	strbuf_git_path_submodule(&objects_directory, path, "objects/");
>  	if (!is_directory(objects_directory.buf)) {
>  		ret = -1;
>  		goto done;

Re: [PATCH v8 0/2] Submodule object path

[Junio C Hamano]

Max Kirillov <max@max630.net> writes:

> On Thu, Sep 10, 2015 at 06:10:13PM -0700, Junio C Hamano wrote:
>> When I push the updated 'pu' out, could you please check
>
> I follow the pu merges. So far resolutions seem correct and
> all tests pass.

The primary thing I ask from "eyeballing by the original author" is
to see if "git diff M^ M" for the merge M of the topic is something
the original author would have done if he was given a starting point
of M^ to build his or her topic (which is what your "resolutions
seem correct" really is), so we are good.

> If you don't mind re-resolving it as I send newer versions I will
> base them on master.

So far rerere seems to be happy, so basing on the same commit is
fine, but I suspect this round should already be ready for 'next'.

I had to remove the now-unused variable "const char *git_dir;" in
[PATCH 1/2], but other than that it looked OK from a cursory re-read.

Thanks.