[PATCH 0/3] Introduce `submodule interngitdirs`

[PATCH 0/3] Introduce `submodule interngitdirs`

[Stefan Beller]

The discussion of the submodule checkout series revealed to me that a command
is needed to move the git directory from the submodules working tree to be
embedded into the superprojects git directory. 

So I wrote the code to intern the submodules git dir into the superproject,
but whilst writing the code I realized this could be valueable for our use
in testing too. So I exposed it via the submodule--helper. But as the
submodule helper ought to be just an internal API, we could also
offer it via the proper submodule command.

The command as it is has little value to the end user for now, but
breaking it out of the submodule checkout series hopefully makes review easier.

Thanks,
Stefan

Stefan Beller (3):
  submodule: use absolute path for computing relative path connecting
  test-lib-functions.sh: teach test_commit -C <dir>
  submodule--helper: add intern-git-dir function

 Documentation/git-submodule.txt    | 15 ++++++++++-
 builtin/submodule--helper.c        | 33 ++++++++++++++++++++++-
 git-submodule.sh                   |  7 ++++-
 submodule.c                        | 55 ++++++++++++++++++++++++++++++++++----
 submodule.h                        |  1 +
 t/t7412-submodule-interngitdirs.sh | 41 ++++++++++++++++++++++++++++
 t/test-lib-functions.sh            | 20 ++++++++++----
 7 files changed, 159 insertions(+), 13 deletions(-)
 create mode 100755 t/t7412-submodule-interngitdirs.sh

-- 
2.11.0.rc2.18.g0126045.dirty

[PATCH 1/3] submodule: use absolute path for computing relative path connecting

[Stefan Beller]

This addresses a similar concern as in f8eaa0ba98b (submodule--helper,
module_clone: always operate on absolute paths, 2016-03-31)

When computing the relative path from one to another location, we
need to provide both locations as absolute paths to make sure the
computation of the relative path is correct.

While at it, change `real_work_tree` to be non const as we own
the memory.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 submodule.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/submodule.c b/submodule.c
index 6f7d883de9..66c5ce5a24 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
 {
 	struct strbuf file_name = STRBUF_INIT;
 	struct strbuf rel_path = STRBUF_INIT;
-	const char *real_work_tree = xstrdup(real_path(work_tree));
+	char *real_git_dir = xstrdup(real_path(git_dir));
+	char *real_work_tree = xstrdup(real_path(work_tree));
 
 	/* Update gitfile */
 	strbuf_addf(&file_name, "%s/.git", work_tree);
 	write_file(file_name.buf, "gitdir: %s",
-		   relative_path(git_dir, real_work_tree, &rel_path));
+		   relative_path(real_git_dir, real_work_tree, &rel_path));
 
 	/* Update core.worktree setting */
 	strbuf_reset(&file_name);
-	strbuf_addf(&file_name, "%s/config", git_dir);
+	strbuf_addf(&file_name, "%s/config", real_git_dir);
 	git_config_set_in_file(file_name.buf, "core.worktree",
-			       relative_path(real_work_tree, git_dir,
+			       relative_path(real_work_tree, real_git_dir,
 					     &rel_path));
 
 	strbuf_release(&file_name);
 	strbuf_release(&rel_path);
-	free((void *)real_work_tree);
+	free(real_work_tree);
+	free(real_git_dir);
 }
 
 int parallel_submodules(void)
-- 
2.11.0.rc2.18.g0126045.dirty

Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Subject: Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting

connecting what?  IOW, has the subject line somehow truncated?

> This addresses a similar concern as in f8eaa0ba98b (submodule--helper,
> module_clone: always operate on absolute paths, 2016-03-31)
>
> When computing the relative path from one to another location, we
> need to provide both locations as absolute paths to make sure the
> computation of the relative path is correct.

Can the effect of this change demonstrated in a new test?  There
must be a scenario where the current behaviour is broken and this
change fixes an incorrect computation of relative path, no?

>
> While at it, change `real_work_tree` to be non const as we own
> the memory.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

>  submodule.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 6f7d883de9..66c5ce5a24 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
>  {
>  	struct strbuf file_name = STRBUF_INIT;
>  	struct strbuf rel_path = STRBUF_INIT;
> -	const char *real_work_tree = xstrdup(real_path(work_tree));
> +	char *real_git_dir = xstrdup(real_path(git_dir));
> +	char *real_work_tree = xstrdup(real_path(work_tree));
>  
>  	/* Update gitfile */
>  	strbuf_addf(&file_name, "%s/.git", work_tree);
>  	write_file(file_name.buf, "gitdir: %s",
> -		   relative_path(git_dir, real_work_tree, &rel_path));
> +		   relative_path(real_git_dir, real_work_tree, &rel_path));
>  
>  	/* Update core.worktree setting */
>  	strbuf_reset(&file_name);
> -	strbuf_addf(&file_name, "%s/config", git_dir);
> +	strbuf_addf(&file_name, "%s/config", real_git_dir);
>  	git_config_set_in_file(file_name.buf, "core.worktree",
> -			       relative_path(real_work_tree, git_dir,
> +			       relative_path(real_work_tree, real_git_dir,
>  					     &rel_path));
>  
>  	strbuf_release(&file_name);
>  	strbuf_release(&rel_path);
> -	free((void *)real_work_tree);
> +	free(real_work_tree);
> +	free(real_git_dir);
>  }
>  
>  int parallel_submodules(void)

Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting

[Stefan Beller]

On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Subject: Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting
>
> connecting what?  IOW, has the subject line somehow truncated?
>
>> This addresses a similar concern as in f8eaa0ba98b (submodule--helper,
>> module_clone: always operate on absolute paths, 2016-03-31)
>>
>> When computing the relative path from one to another location, we
>> need to provide both locations as absolute paths to make sure the
>> computation of the relative path is correct.
>
> Can the effect of this change demonstrated in a new test?  There
> must be a scenario where the current behaviour is broken and this
> change fixes an incorrect computation of relative path, no?
>

I found the latest patch of this series broken without this patch.
I'll try to find existing broken code, though.

Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting

[Stefan Beller]

On Mon, Nov 21, 2016 at 1:03 PM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Can the effect of this change demonstrated in a new test?  There
>> must be a scenario where the current behaviour is broken and this
>> change fixes an incorrect computation of relative path, no?

I do not think the current usage exposes this bug in
connect_work_tree_and_git_dir. It is only used in builtin/mv.c,
which fills the second parameter `git_dir` via a call to read_gitfile,
which itself produces an absolute path.

The latest patch of this series however passes in relative path for the
respective git directories.

So the commit message of this patch is misleading at least.
Maybe:

  The current caller of connect_work_tree_and_git_dir passes
  an absolute path for the `git_dir` parameter. In the future patch
  we will also pass in relative path for `git_dir`. Extend the functionality
  of connect_work_tree_and_git_dir to take relative paths for parameters.

  We could work around this in the future patch by computing the absolute
  path for the git_dir in the calling site, however accepting relative
  paths for either parameter makes the API for this function easier
  to use.

  While at it, change `real_work_tree` to be non const as we own
  the memory.


>
> I found the latest patch of this series broken without this patch.
> I'll try to find existing broken code, though.

Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Mon, Nov 21, 2016 at 1:03 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>
>>> Can the effect of this change demonstrated in a new test?  There
>>> must be a scenario where the current behaviour is broken and this
>>> change fixes an incorrect computation of relative path, no?
>
> I do not think the current usage exposes this bug in
> connect_work_tree_and_git_dir. It is only used in builtin/mv.c,
> which fills the second parameter `git_dir` via a call to read_gitfile,
> which itself produces an absolute path.

OK.  Fixing a potential bug as a preparatory step is good.

>   The current caller of connect_work_tree_and_git_dir passes
>   an absolute path for the `git_dir` parameter. In the future patch
>   we will also pass in relative path for `git_dir`. Extend the functionality
>   of connect_work_tree_and_git_dir to take relative paths for parameters.
>
>   We could work around this in the future patch by computing the absolute
>   path for the git_dir in the calling site, however accepting relative
>   paths for either parameter makes the API for this function easier
>   to use.

Yup, sounds sensible.  Thanks.

[PATCH 2/3] test-lib-functions.sh: teach test_commit -C <dir>

[Stefan Beller]

Specifically when setting up submodule tests, it comes in handy if
we can create commits in repositories that are not at the root of
the tested trash dir. Add "-C <dir>" similar to gits -C parameter
that will perform the operation in the given directory.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 t/test-lib-functions.sh | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..579e812506 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -157,16 +157,21 @@ debug () {
 	 GIT_TEST_GDB=1 "$@"
 }
 
-# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
+# Call test_commit with the arguments
+# [-C <directory>] <message> [<file> [<contents> [<tag>]]]"
 #
 # This will commit a file with the given contents and the given commit
 # message, and tag the resulting commit with the given tag name.
 #
 # <file>, <contents>, and <tag> all default to <message>.
+#
+# If the first argument is "-C", the second argument is used as a path for
+# the git invocations.
 
 test_commit () {
 	notick= &&
 	signoff= &&
+	indir= &&
 	while test $# != 0
 	do
 		case "$1" in
@@ -176,21 +181,26 @@ test_commit () {
 		--signoff)
 			signoff="$1"
 			;;
+		-C)
+			indir="$2"
+			shift
+			;;
 		*)
 			break
 			;;
 		esac
 		shift
 	done &&
+	indir=${indir:+"$indir"/} &&
 	file=${2:-"$1.t"} &&
-	echo "${3-$1}" > "$file" &&
-	git add "$file" &&
+	echo "${3-$1}" > "$indir$file" &&
+	git ${indir:+ -C "$indir"} add "$file" &&
 	if test -z "$notick"
 	then
 		test_tick
 	fi &&
-	git commit $signoff -m "$1" &&
-	git tag "${4:-$1}"
+	git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
+	git ${indir:+ -C "$indir"} tag "${4:-$1}"
 }
 
 # Call test_merge with the arguments "<message> <commit>", where <commit>
-- 
2.11.0.rc2.18.g0126045.dirty

Re: [PATCH 2/3] test-lib-functions.sh: teach test_commit -C <dir>

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> Specifically when setting up submodule tests, it comes in handy if
> we can create commits in repositories that are not at the root of
> the tested trash dir. Add "-C <dir>" similar to gits -C parameter
> that will perform the operation in the given directory.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Looks useful.

>  t/test-lib-functions.sh | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index fdaeb3a96b..579e812506 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -157,16 +157,21 @@ debug () {
>  	 GIT_TEST_GDB=1 "$@"
>  }
>  
> -# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
> +# Call test_commit with the arguments
> +# [-C <directory>] <message> [<file> [<contents> [<tag>]]]"
>  #
>  # This will commit a file with the given contents and the given commit
>  # message, and tag the resulting commit with the given tag name.
>  #
>  # <file>, <contents>, and <tag> all default to <message>.
> +#
> +# If the first argument is "-C", the second argument is used as a path for
> +# the git invocations.
>  
>  test_commit () {
>  	notick= &&
>  	signoff= &&
> +	indir= &&
>  	while test $# != 0
>  	do
>  		case "$1" in
> @@ -176,21 +181,26 @@ test_commit () {
>  		--signoff)
>  			signoff="$1"
>  			;;
> +		-C)
> +			indir="$2"
> +			shift
> +			;;
>  		*)
>  			break
>  			;;
>  		esac
>  		shift
>  	done &&
> +	indir=${indir:+"$indir"/} &&
>  	file=${2:-"$1.t"} &&
> -	echo "${3-$1}" > "$file" &&
> -	git add "$file" &&
> +	echo "${3-$1}" > "$indir$file" &&
> +	git ${indir:+ -C "$indir"} add "$file" &&
>  	if test -z "$notick"
>  	then
>  		test_tick
>  	fi &&
> -	git commit $signoff -m "$1" &&
> -	git tag "${4:-$1}"
> +	git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
> +	git ${indir:+ -C "$indir"} tag "${4:-$1}"
>  }
>  
>  # Call test_merge with the arguments "<message> <commit>", where <commit>

[PATCH 3/3] submodule--helper: add intern-git-dir function

[Stefan Beller]

When a submodule has its git dir inside the working dir, the submodule
support for checkout that we plan to add in a later patch will fail.

Add functionality to migrate the git directory to be embedded
into the superprojects git directory.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/git-submodule.txt    | 15 ++++++++++++-
 builtin/submodule--helper.c        | 33 ++++++++++++++++++++++++++++-
 git-submodule.sh                   |  7 ++++++-
 submodule.c                        | 43 ++++++++++++++++++++++++++++++++++++++
 submodule.h                        |  1 +
 t/t7412-submodule-interngitdirs.sh | 41 ++++++++++++++++++++++++++++++++++++
 6 files changed, 137 insertions(+), 3 deletions(-)
 create mode 100755 t/t7412-submodule-interngitdirs.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..80d55350eb 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -22,7 +22,7 @@ SYNOPSIS
 	      [commit] [--] [<path>...]
 'git submodule' [--quiet] foreach [--recursive] <command>
 'git submodule' [--quiet] sync [--recursive] [--] [<path>...]
-
+'git submodule' [--quiet] interngitdirs [--] [<path>...]
 
 DESCRIPTION
 -----------
@@ -245,6 +245,19 @@ sync::
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and sync any nested submodules within.
 
+interngitdirs::
+	Move the git directory of submodules into its superprojects
+	`$GIT_DIR/modules` path and then connect the git directory and
+	its working directory by setting the `core.worktree` and adding
+	a .git file pointing to the git directory interned into the
+	superproject.
++
+	A repository that was cloned independently and later added
+	as a submodule or old setups have the submodules git directory
+	inside the submodule instead of the
++
+	This command is recursive by default.
+
 OPTIONS
 -------
 -q::
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5f9f..256f8e9439 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,36 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
 	return 0;
 }
 
+static int intern_git_dir(int argc, const char **argv, const char *prefix)
+{
+	int i;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+
+	struct option intern_gitdir_options[] = {
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule--helper intern-git-dir [<path>...]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, intern_gitdir_options,
+			     git_submodule_helper_usage, 0);
+
+	gitmodules_config();
+	git_config(submodule_config, NULL);
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		return 1;
+
+	for (i = 0; i < list.nr; i++)
+		migrate_submodule_gitdir(list.entries[i]->name);
+
+	return 0;
+}
+
 struct cmd_struct {
 	const char *cmd;
 	int (*fn)(int, const char **, const char *);
@@ -1090,7 +1120,8 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url", resolve_relative_url},
 	{"resolve-relative-url-test", resolve_relative_url_test},
 	{"init", module_init},
-	{"remote-branch", resolve_remote_submodule_branch}
+	{"remote-branch", resolve_remote_submodule_branch},
+	{"intern-git-dir", intern_git_dir}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a135d6..747e934df2 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1131,6 +1131,11 @@ cmd_sync()
 	done
 }
 
+cmd_interngitdirs()
+{
+	git submodule--helper intern-git-dir "$@"
+}
+
 # This loop parses the command line arguments to find the
 # subcommand name to dispatch.  Parsing of the subcommand specific
 # options are primarily done by the subcommand implementations.
@@ -1140,7 +1145,7 @@ cmd_sync()
 while test $# != 0 && test -z "$command"
 do
 	case "$1" in
-	add | foreach | init | deinit | update | status | summary | sync)
+	add | foreach | init | deinit | update | status | summary | sync | interngitdirs)
 		command=$1
 		;;
 	-q|--quiet)
diff --git a/submodule.c b/submodule.c
index 66c5ce5a24..99befdba85 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1263,3 +1263,46 @@ void prepare_submodule_repo_env(struct argv_array *out)
 	}
 	argv_array_push(out, "GIT_DIR=.git");
 }
+
+/*
+ * Migrate the given submodule (and all its submodules recursively) from
+ * having its git directory within the working tree to the git dir nested
+ * in its superprojects git dir under modules/.
+ */
+void migrate_submodule_gitdir(const char *path)
+{
+	char *old_git_dir;
+	const char *new_git_dir;
+	const struct submodule *sub;
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	cp.git_cmd = 1;
+	cp.no_stdin = 1;
+	cp.dir = path;
+	argv_array_pushl(&cp.args, "submodule", "foreach", "--recursive",
+			"git", "submodule--helper" "intern-git-dir", NULL);
+
+	if (run_command(&cp))
+		die(_("Could not migrate git directory in submodule '%s'"),
+		    path);
+
+	old_git_dir = xstrfmt("%s/.git", path);
+	if (read_gitfile(old_git_dir))
+		/* If it is an actual gitfile, it doesn't need migration. */
+		goto out;
+
+	sub = submodule_from_path(null_sha1, path);
+	if (!sub)
+		die(_("Could not lookup name for submodule '%s'"),
+		      path);
+	new_git_dir = git_common_path("modules/%s", sub->name);
+	mkdir_in_gitdir(".git/modules");
+
+	if (rename(old_git_dir, new_git_dir) < 0)
+		die_errno(_("Could not migrate git directory from '%s' to '%s'"),
+			old_git_dir, new_git_dir);
+
+	connect_work_tree_and_git_dir(path, new_git_dir);
+out:
+	free(old_git_dir);
+}
diff --git a/submodule.h b/submodule.h
index d9e197a948..859026ecfa 100644
--- a/submodule.h
+++ b/submodule.h
@@ -75,4 +75,5 @@ int parallel_submodules(void);
  */
 void prepare_submodule_repo_env(struct argv_array *out);
 
+extern void migrate_submodule_gitdir(const char *path);
 #endif
diff --git a/t/t7412-submodule-interngitdirs.sh b/t/t7412-submodule-interngitdirs.sh
new file mode 100755
index 0000000000..8938a4c8b7
--- /dev/null
+++ b/t/t7412-submodule-interngitdirs.sh
@@ -0,0 +1,41 @@
+#!/bin/sh
+
+test_description='Test submodule interngitdirs
+
+This test verifies that `git submodue interngitdirs` moves a submodules git
+directory into the superproject.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup a real submodule' '
+	git init sub1 &&
+	test_commit -C sub1 first &&
+	git submodule add ./sub1 &&
+	test_tick &&
+	git commit -m superproject
+'
+
+test_expect_success 'intern the git dir' '
+	git submodule interngitdirs &&
+	test -f sub1/.git &&
+	test -d .git/modules/sub1 &&
+	# check that we did not break the repository:
+	git status
+'
+
+test_expect_success 'setup a gitlink with missing .gitmodules entry' '
+	git init sub2 &&
+	test_commit -C sub2 first &&
+	git add sub2 &&
+	git commit -m superproject
+'
+
+test_expect_success 'intern the git dir fails for incomplete submodules' '
+	test_must_fail git submodule interngitdirs &&
+	# check that we did not break the repository:
+	git status
+'
+
+test_done
+
-- 
2.11.0.rc2.18.g0126045.dirty

Re: [PATCH 3/3] submodule--helper: add intern-git-dir function

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +interngitdirs::
> +	Move the git directory of submodules into its superprojects
> +	`$GIT_DIR/modules` path and then connect the git directory and
> +	its working directory by setting the `core.worktree` and adding
> +	a .git file pointing to the git directory interned into the
> +	superproject.
> ++
> +	A repository that was cloned independently and later added
> +	as a submodule or old setups have the submodules git directory
> +	inside the submodule instead of the
> ++
> +	This command is recursive by default.

Does this format correctly?

I somehow thought that second and subsequent paragraphs continued
with "+" want no indentation before them.  See for example the
Values section in config.txt and see how entries for boolean:: and
color:: use multiple '+' paragraphs.

If we do not have to refrain from indenting the second and
subsequent paragraphs, that would be great for readability, but I
take the existing practice as telling me that we cannot do that X-<.

> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '
> +	git init sub2 &&
> +	test_commit -C sub2 first &&
> +	git add sub2 &&
> +	git commit -m superproject
> +'
> +
> +test_expect_success 'intern the git dir fails for incomplete submodules' '
> +	test_must_fail git submodule interngitdirs &&
> +	# check that we did not break the repository:
> +	git status
> +'

It is not clear what the last "git status" wants to test.  In the
extreme, if the failed "git submodule" command did

	rm -fr .git ?* && git init

wouldn't "git status" still succeed?  

What are the minimum things that we expect from "did not break" to
see?  sub2/.git is still a directory and is a valid repository?  The
contents of the .git/modules/* before and after the "git submodule"
does not change?  Some other things?

Re: [PATCH 3/3] submodule--helper: add intern-git-dir function

[Stefan Beller]

On Mon, Nov 21, 2016 at 1:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Does this format correctly?
>
> I somehow thought that second and subsequent paragraphs continued
> with "+" want no indentation before them.  See for example the
> Values section in config.txt and see how entries for boolean:: and
> color:: use multiple '+' paragraphs.
>
> If we do not have to refrain from indenting the second and
> subsequent paragraphs, that would be great for readability, but I
> take the existing practice as telling me that we cannot do that X-<.

Will fix and test in a resend.


>
>> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '
>> +     git init sub2 &&
>> +     test_commit -C sub2 first &&
>> +     git add sub2 &&
>> +     git commit -m superproject
>> +'
>> +
>> +test_expect_success 'intern the git dir fails for incomplete submodules' '
>> +     test_must_fail git submodule interngitdirs &&
>> +     # check that we did not break the repository:
>> +     git status
>> +'
>
> It is not clear what the last "git status" wants to test.

Any errors that I ran into when manually truing to embed a submodules
git dir, were fatal with `git status` already, e.g. missing or wrong
call of connect_work_tree_and_git_dir were my main failure points.

So I guess we should test a bit more extensively, maybe

    git status >expect
    git submodule embedgitdirs
    git status >actual
    test_cmp expect actual
    # further testing via
    test -f ..
    test -d ..

>  In the
> extreme, if the failed "git submodule" command did
>
>         rm -fr .git ?* && git init
>
> wouldn't "git status" still succeed?

    In that particular case you'd get
    $ git status
    fatal: Not a git repository (or any parent up to mount point ....)
    Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).
    $ echo $?
    128

but I get the idea, which is why I propose the double check via status.
That would detect any logical change for the repository, e.g. a
change to the .gitmodules file.

>
> What are the minimum things that we expect from "did not break" to
> see?  sub2/.git is still a directory and is a valid repository?  The
> contents of the .git/modules/* before and after the "git submodule"
> does not change?  Some other things?

I thought about making up a name for such a repo and creating that engry
in .gitmodules. But I refrained from doing so, because it seems too much
for this command.

I dunno, but I would suspect the double status is fine here, too?

Re: [PATCH 3/3] submodule--helper: add intern-git-dir function

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> So I guess we should test a bit more extensively, maybe
>
>     git status >expect
>     git submodule embedgitdirs
>     git status >actual
>     test_cmp expect actual
>     # further testing via
>     test -f ..
>     test -d ..

Something along that line.  "status should succeed" does not tell
the readers what kind of breakage the test is expecting to protect
us from.  If we are expecting a breakage in embed-git-dirs would
somehow corrupt an existing submodule, which would lead to "status"
that is run in the superproject report the submodule differently,
then comparing output before and after the operation may be a
reasonable test.  Going there to the submodule working tree and
checking the health of the repository (of the submodule) may be
another sensible test.

>>  In the
>> extreme, if the failed "git submodule" command did
>>
>>         rm -fr .git ?* && git init
>>
>> wouldn't "git status" still succeed?
>
>     In that particular case you'd get
>     $ git status
>     fatal: Not a git repository (or any parent up to mount point ....)

Even with "&& git init"?  Or you forgot that part?

Re: [PATCH 3/3] submodule--helper: add intern-git-dir function

[Stefan Beller]

On Mon, Nov 21, 2016 at 11:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> So I guess we should test a bit more extensively, maybe
>>
>>     git status >expect
>>     git submodule embedgitdirs
>>     git status >actual
>>     test_cmp expect actual
>>     # further testing via
>>     test -f ..
>>     test -d ..
>
> Something along that line.  "status should succeed" does not tell
> the readers what kind of breakage the test is expecting to protect
> us from.  If we are expecting a breakage in embed-git-dirs would
> somehow corrupt an existing submodule, which would lead to "status"
> that is run in the superproject report the submodule differently,
> then comparing output before and after the operation may be a
> reasonable test.  Going there to the submodule working tree and
> checking the health of the repository (of the submodule) may be
> another sensible test.

and by checking the health you mean just a status in there, or rather a
more nuanced thing like `git rev-parse HEAD` ? I'll go with that for now.

>
>>>  In the
>>> extreme, if the failed "git submodule" command did
>>>
>>>         rm -fr .git ?* && git init
>>>
>>> wouldn't "git status" still succeed?
>>
>>     In that particular case you'd get
>>     $ git status
>>     fatal: Not a git repository (or any parent up to mount point ....)
>
> Even with "&& git init"?  Or you forgot that part?

yes I did, because why would you run init after an accidental rm -rf ... ?

Re: [PATCH 3/3] submodule--helper: add intern-git-dir function

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Mon, Nov 21, 2016 at 11:07 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> So I guess we should test a bit more extensively, maybe
>>>
>>>     git status >expect
>>>     git submodule embedgitdirs
>>>     git status >actual
>>>     test_cmp expect actual
>>>     # further testing via
>>>     test -f ..
>>>     test -d ..
>>
>> Something along that line.  "status should succeed" does not tell
>> the readers what kind of breakage the test is expecting to protect
>> us from.  If we are expecting a breakage in embed-git-dirs would
>> somehow corrupt an existing submodule, which would lead to "status"
>> that is run in the superproject report the submodule differently,
>> then comparing output before and after the operation may be a
>> reasonable test.  Going there to the submodule working tree and
>> checking the health of the repository (of the submodule) may be
>> another sensible test.
>
> and by checking the health you mean just a status in there, or rather a
> more nuanced thing like `git rev-parse HEAD` ? I'll go with that for now.

Would fsck have caught the breakages you caused while developing it,
for example?

As the core of the "embed" operation is an non-atomic "move the .git
directory to .git/modules/$name in the superproject and then make a
gitfile pointing at it", verifying that there is no change in the
contents of .git/modules before and after the failed operation, and
verifying that the submodule working tree has the embedded .git/
directory would be good enough, I would think.

Re: [PATCH 3/3] submodule--helper: add intern-git-dir function

[Brandon Williams]

On 11/21, Stefan Beller wrote:
> diff --git a/t/t7412-submodule-interngitdirs.sh b/t/t7412-submodule-interngitdirs.sh
> new file mode 100755
> index 0000000000..8938a4c8b7
> --- /dev/null
> +++ b/t/t7412-submodule-interngitdirs.sh
> @@ -0,0 +1,41 @@
> +#!/bin/sh
> +
> +test_description='Test submodule interngitdirs
> +
> +This test verifies that `git submodue interngitdirs` moves a submodules git
> +directory into the superproject.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup a real submodule' '
> +	git init sub1 &&
> +	test_commit -C sub1 first &&
> +	git submodule add ./sub1 &&
> +	test_tick &&
> +	git commit -m superproject
> +'
> +
> +test_expect_success 'intern the git dir' '
> +	git submodule interngitdirs &&
> +	test -f sub1/.git &&
> +	test -d .git/modules/sub1 &&
> +	# check that we did not break the repository:
> +	git status
> +'
> +
> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '
> +	git init sub2 &&
> +	test_commit -C sub2 first &&
> +	git add sub2 &&
> +	git commit -m superproject
> +'
> +
> +test_expect_success 'intern the git dir fails for incomplete submodules' '
> +	test_must_fail git submodule interngitdirs &&
> +	# check that we did not break the repository:
> +	git status
> +'
> +
> +test_done
> +

Could we add a test which has nested submodules that need to be
migrated?  Hopfully its just as easy as adding the test :)

-- 
Brandon Williams

Re: [PATCH 0/3] Introduce `submodule interngitdirs`

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> The discussion of the submodule checkout series revealed to me
> that a command is needed to move the git directory from the
> submodules working tree to be embedded into the superprojects git
> directory.

You used "move" here, and "migrate" in the function name in 3/3,
both of which make sense.  

But "intern" sounds funny.  Who is confined as a prisoner here?
North American English uses that verb as "serve as an intern", but
that does not apply here.  The verb also is used in Lisp-ish
languages to mean the act of turning a string into a symbol, but
that does not apply here, either.

Re: [PATCH 0/3] Introduce `submodule interngitdirs`

[Stefan Beller]

On Mon, Nov 21, 2016 at 12:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> The discussion of the submodule checkout series revealed to me
>> that a command is needed to move the git directory from the
>> submodules working tree to be embedded into the superprojects git
>> directory.
>
> You used "move" here, and "migrate" in the function name in 3/3,
> both of which make sense.
>
> But "intern" sounds funny.  Who is confined as a prisoner here?
> North American English uses that verb as "serve as an intern", but
> that does not apply here.  The verb also is used in Lisp-ish
> languages to mean the act of turning a string into a symbol, but
> that does not apply here, either.

I was inspired by the latter as we ask Git to make the submodule
"properly embedded" into the superproject, which is what I imagined
is similar to the lisp interning.

So I guess my imagination went to far and we rather want to invoke it via
"git submodule migrategitdirs" ?

But there you would ask "where are we migrating the git dirs to?", so
it would be reasonable to expect 2 parameters (just like the mv
command).

So maybe "git submodule embedgitdirs" ?