threads / patch / 40345

v9, 2 partssubmodule refactor: use strbuf_git_path_submodule() in add_submodule_odb()

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

## tl;dr

4 messages between Sep 13, 2015 and Sep 14, 2015. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Max Kirillov· Sep 13, 2015, 22:17 UTC · lore

[PATCH v9 0/2] Submodule object path

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
Max Kirillov· Sep 13, 2015, 22:17 UTC · re: Max Kirillov · lore

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(-)
Show changes to submodule.c +1 −7
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
Junio C Hamano· Sep 14, 2015, 18:00 UTC · re: Max Kirillov · lore

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

Max Kirillov <max@max630.net> writes:
Show 13 quoted lines
> 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.
Show 20 quoted lines
>
> 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;
Max Kirillov· Sep 13, 2015, 22:17 UTC · re: Max Kirillov · lore

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

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(-)
Show changes to 4 files +41 −9

cache.h, path.c, setup.c, t/t7410-submodule-checkout-to.sh

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

← back to recent threads