Re: [WIP PATCH 1/4] Prepare checkout_entry() for recursive checkout of submodules

Jens Lehmann [off-list ref] writes:

+int checkout_submodule(const char *path, const unsigned char sha1[20], int force)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct child_process cp;
+	const char *hex_sha1 = sha1_to_hex(sha1);
+	const char *argv[] = {
+		"checkout",
+                force ? "-qf" : "-q",
+		hex_sha1,
+		NULL,
+	};

Why force -q?

+	strbuf_addf(&buf, "%s/.git/", path);
+	if (!is_directory(buf.buf)) {
+		strbuf_release(&buf);
+		/* The submodule is not populated, so we can't check it out */
+		return 0;
+	}

This would give you an incorrect result if .git is a file that records
"gitdir: overthere" (see read_gitfile_gently() in setup.c); I would expect
it would become a fairly important ingredient if we ever enhance the
submodule support to add submodule that disappears/reappears in the
history.

Am 09.04.2010 23:59, schrieb Junio C Hamano:

Jens Lehmann [off-list ref] writes:

quoted

+int checkout_submodule(const char *path, const unsigned char sha1[20], int force)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct child_process cp;
+	const char *hex_sha1 = sha1_to_hex(sha1);
+	const char *argv[] = {
+		"checkout",
+                force ? "-qf" : "-q",
+		hex_sha1,
+		NULL,
+	};

Why force -q?

Good question. I seem to have tried to silence the output of checkout
from run_command(), which AFIACS doesn't reach the console anyway
unless i want it to ... (while at the same time managing to mess up
the tabs in that line ... :-/ )

quoted

+	strbuf_addf(&buf, "%s/.git/", path);
+	if (!is_directory(buf.buf)) {
+		strbuf_release(&buf);
+		/* The submodule is not populated, so we can't check it out */
+		return 0;
+	}

This would give you an incorrect result if .git is a file that records
"gitdir: overthere" (see read_gitfile_gently() in setup.c); I would expect
it would become a fairly important ingredient if we ever enhance the
submodule support to add submodule that disappears/reappears in the
history.

Right. This assumption is also present in add_submodule_odb() (used by
show_submodule_summary()) and is_submodule_modified(), so i just reused
it. This should be addressed in another patch.

Am 10.04.2010 01:11, schrieb Jens Lehmann:

Am 09.04.2010 23:59, schrieb Junio C Hamano:

quoted

Jens Lehmann [off-list ref] writes:

quoted

+	strbuf_addf(&buf, "%s/.git/", path);
+	if (!is_directory(buf.buf)) {
+		strbuf_release(&buf);
+		/* The submodule is not populated, so we can't check it out */
+		return 0;
+	}

This would give you an incorrect result if .git is a file that records
"gitdir: overthere" (see read_gitfile_gently() in setup.c); I would expect
it would become a fairly important ingredient if we ever enhance the
submodule support to add submodule that disappears/reappears in the
history.

Right. This assumption is also present in add_submodule_odb() (used by
show_submodule_summary()) and is_submodule_modified(), so i just reused
it. This should be addressed in another patch.

What about this one:

-----------------8<---------------
Subject: [PATCH] Teach diff --submodule and status to handle .git files in submodules

The simple test for an existing .git directory gives an incorrect result
if .git is a file that records "gitdir: overthere". So for submodules that
use a .git file, "git status" and the diff family - when the "--submodule"
option is given - did assume the submodule was not populated at all when
a .git file was used, thus generating wrong output or no output at all.

This is fixed by using read_gitfile_gently() to get the correct location
of the .git directory. While at it, is_submodule_modified() was cleaned up
to use the "dir" member of "struct child_process" instead of setting the
GIT_WORK_TREE and GIT_DIR environment variables.

Signed-off-by: Jens Lehmann <redacted>
---
 submodule.c                 |   33 ++++++++++++++++-----------------
 t/t4041-diff-submodule.sh   |   15 +++++++++++++++
 t/t7506-status-submodule.sh |   16 ++++++++++++++++
 3 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/submodule.c b/submodule.c
index b3b8bc1..676d48f 100644
--- a/submodule.c
+++ b/submodule.c

@@ -12,8 +12,15 @@ 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/objects/", path);
+	strbuf_addf(&objects_directory, "%s/.git", path);
+	git_dir = read_gitfile_gently(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)) {
 		ret = -1;
 		goto done;

@@ -132,7 +139,6 @@ void show_submodule_summary(FILE *f, const char *path,

 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-	int i;
 	ssize_t len;
 	struct child_process cp;
 	const char *argv[] = {

@@ -141,16 +147,16 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 		NULL,
 		NULL,
 	};
-	const char *env[LOCAL_REPO_ENV_SIZE + 3];
 	struct strbuf buf = STRBUF_INIT;
 	unsigned dirty_submodule = 0;
 	const char *line, *next_line;
+	const char *git_dir;

-	for (i = 0; i < LOCAL_REPO_ENV_SIZE; i++)
-		env[i] = local_repo_env[i];
-
-	strbuf_addf(&buf, "%s/.git/", path);
-	if (!is_directory(buf.buf)) {
+	strbuf_addf(&buf, "%s/.git", path);
+	git_dir = read_gitfile_gently(buf.buf);
+	if (!git_dir)
+		git_dir = buf.buf;
+	if (!is_directory(git_dir)) {
 		strbuf_release(&buf);
 		/* The submodule is not checked out, so it is not modified */
 		return 0;

@@ -158,21 +164,16 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	}
 	strbuf_reset(&buf);

-	strbuf_addf(&buf, "GIT_WORK_TREE=%s", path);
-	env[i++] = strbuf_detach(&buf, NULL);
-	strbuf_addf(&buf, "GIT_DIR=%s/.git", path);
-	env[i++] = strbuf_detach(&buf, NULL);
-	env[i] = NULL;
-
 	if (ignore_untracked)
 		argv[2] = "-uno";

 	memset(&cp, 0, sizeof(cp));
 	cp.argv = argv;
-	cp.env = env;
+	cp.env = local_repo_env;
 	cp.git_cmd = 1;
 	cp.no_stdin = 1;
 	cp.out = -1;
+	cp.dir = path;
 	if (start_command(&cp))
 		die("Could not run git status --porcelain");

@@ -201,8 +202,6 @@ unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	if (finish_command(&cp))
 		die("git status --porcelain failed");

-	for (i = LOCAL_REPO_ENV_SIZE; env[i]; i++)
-		free((char *)env[i]);
 	strbuf_release(&buf);
 	return dirty_submodule;
 }

diff --git a/t/t4041-diff-submodule.sh b/t/t4041-diff-submodule.sh
index 11b1997..019acb9 100755
--- a/t/t4041-diff-submodule.sh
+++ b/t/t4041-diff-submodule.sh

@@ -329,4 +329,19 @@ index 0000000..$head7
 EOF
 "

+test_expect_success 'setup .git file for sm2' '
+	(cd sm2 &&
+	 REAL="$(pwd)/../.real" &&
+	 mv .git "$REAL"
+	 echo "gitdir: $REAL" >.git)
+'
+
+test_expect_success 'diff --submodule with .git file' '
+	git diff --submodule HEAD^ >actual &&
+	diff actual - <<-EOF
+Submodule sm1 $head6...0000000 (submodule deleted)
+Submodule sm2 0000000...$head7 (new submodule)
+EOF
+'
+
 test_done

diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index aeec1f6..3d4f85d 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh

@@ -157,6 +157,22 @@ test_expect_success 'status with added and untracked file in modified submodule
 	EOF
 '

+test_expect_success 'setup .git file for sub' '
+	(cd sub &&
+	 rm -f new-file
+	 REAL="$(pwd)/../.real" &&
+	 mv .git "$REAL"
+	 echo "gitdir: $REAL" >.git) &&
+	 echo .real >>.gitignore &&
+	 git commit -m "added .real to .gitignore" .gitignore
+'
+
+test_expect_success 'status with added file in modified submodule with .git file' '
+	(cd sub && git reset --hard && echo >foo && git add foo) &&
+	git status >output &&
+	grep "modified:   sub (new commits, modified content)" output
+'
+
 test_expect_success 'rm submodule contents' '
 	rm -rf sub/* sub/.git
 '

-- 
1.7.1.rc0.265.g16922.dirty