{"thread":{"id":"44523","subject":"[PATCH 1/3] submodule: use absolute path for computing relative path connecting","startedAt":"2016-11-21T20:43:03Z","lastAt":"2016-11-22T17:53:21Z","messageCount":17,"participants":["Stefan Beller","Junio C Hamano","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"306243","messageId":"20161121204146.13665-2-sbeller@google.com","threadId":"44523","inReplyTo":"20161121204146.13665-1-sbeller@google.com","subject":"[PATCH 1/3] submodule: use absolute path for computing relative path connecting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-21T20:41:44Z","receivedAt":"2016-11-21T20:43:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This addresses a similar concern as in f8eaa0ba98b (submodule--helper,\nmodule_clone: always operate on absolute paths, 2016-03-31)\n\nWhen computing the relative path from one to another location, we\nneed to provide both locations as absolute paths to make sure the\ncomputation of the relative path is correct.\n\nWhile at it, change `real_work_tree` to be non const as we own\nthe memory.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n submodule.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 6f7d883de9..66c5ce5a24 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n {\n \tstruct strbuf file_name = STRBUF_INIT;\n \tstruct strbuf rel_path = STRBUF_INIT;\n-\tconst char *real_work_tree = xstrdup(real_path(work_tree));\n+\tchar *real_git_dir = xstrdup(real_path(git_dir));\n+\tchar *real_work_tree = xstrdup(real_path(work_tree));\n \n \t/* Update gitfile */\n \tstrbuf_addf(&file_name, \"%s/.git\", work_tree);\n \twrite_file(file_name.buf, \"gitdir: %s\",\n-\t\t   relative_path(git_dir, real_work_tree, &rel_path));\n+\t\t   relative_path(real_git_dir, real_work_tree, &rel_path));\n \n \t/* Update core.worktree setting */\n \tstrbuf_reset(&file_name);\n-\tstrbuf_addf(&file_name, \"%s/config\", git_dir);\n+\tstrbuf_addf(&file_name, \"%s/config\", real_git_dir);\n \tgit_config_set_in_file(file_name.buf, \"core.worktree\",\n-\t\t\t       relative_path(real_work_tree, git_dir,\n+\t\t\t       relative_path(real_work_tree, real_git_dir,\n \t\t\t\t\t     &rel_path));\n \n \tstrbuf_release(&file_name);\n \tstrbuf_release(&rel_path);\n-\tfree((void *)real_work_tree);\n+\tfree(real_work_tree);\n+\tfree(real_git_dir);\n }\n \n int parallel_submodules(void)\n-- \n2.11.0.rc2.18.g0126045.dirty\n\n"},{"id":"306244","messageId":"20161121204146.13665-3-sbeller@google.com","threadId":"44523","inReplyTo":"20161121204146.13665-1-sbeller@google.com","subject":"[PATCH 2/3] test-lib-functions.sh: teach test_commit -C <dir>","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-21T20:41:45Z","receivedAt":"2016-11-21T20:43:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Specifically when setting up submodule tests, it comes in handy if\nwe can create commits in repositories that are not at the root of\nthe tested trash dir. Add \"-C <dir>\" similar to gits -C parameter\nthat will perform the operation in the given directory.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/test-lib-functions.sh | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex fdaeb3a96b..579e812506 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -157,16 +157,21 @@ debug () {\n \t GIT_TEST_GDB=1 \"$@\"\n }\n \n-# Call test_commit with the arguments \"<message> [<file> [<contents> [<tag>]]]\"\n+# Call test_commit with the arguments\n+# [-C <directory>] <message> [<file> [<contents> [<tag>]]]\"\n #\n # This will commit a file with the given contents and the given commit\n # message, and tag the resulting commit with the given tag name.\n #\n # <file>, <contents>, and <tag> all default to <message>.\n+#\n+# If the first argument is \"-C\", the second argument is used as a path for\n+# the git invocations.\n \n test_commit () {\n \tnotick= &&\n \tsignoff= &&\n+\tindir= &&\n \twhile test $# != 0\n \tdo\n \t\tcase \"$1\" in\n@@ -176,21 +181,26 @@ test_commit () {\n \t\t--signoff)\n \t\t\tsignoff=\"$1\"\n \t\t\t;;\n+\t\t-C)\n+\t\t\tindir=\"$2\"\n+\t\t\tshift\n+\t\t\t;;\n \t\t*)\n \t\t\tbreak\n \t\t\t;;\n \t\tesac\n \t\tshift\n \tdone &&\n+\tindir=${indir:+\"$indir\"/} &&\n \tfile=${2:-\"$1.t\"} &&\n-\techo \"${3-$1}\" > \"$file\" &&\n-\tgit add \"$file\" &&\n+\techo \"${3-$1}\" > \"$indir$file\" &&\n+\tgit ${indir:+ -C \"$indir\"} add \"$file\" &&\n \tif test -z \"$notick\"\n \tthen\n \t\ttest_tick\n \tfi &&\n-\tgit commit $signoff -m \"$1\" &&\n-\tgit tag \"${4:-$1}\"\n+\tgit ${indir:+ -C \"$indir\"} commit $signoff -m \"$1\" &&\n+\tgit ${indir:+ -C \"$indir\"} tag \"${4:-$1}\"\n }\n \n # Call test_merge with the arguments \"<message> <commit>\", where <commit>\n-- \n2.11.0.rc2.18.g0126045.dirty\n\n"},{"id":"306245","messageId":"20161121204146.13665-1-sbeller@google.com","threadId":"44523","inReplyTo":null,"subject":"[PATCH 0/3] Introduce `submodule interngitdirs`","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-21T20:41:43Z","receivedAt":"2016-11-21T20:43:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The discussion of the submodule checkout series revealed to me that a command\nis needed to move the git directory from the submodules working tree to be\nembedded into the superprojects git directory. \n\nSo I wrote the code to intern the submodules git dir into the superproject,\nbut whilst writing the code I realized this could be valueable for our use\nin testing too. So I exposed it via the submodule--helper. But as the\nsubmodule helper ought to be just an internal API, we could also\noffer it via the proper submodule command.\n\nThe command as it is has little value to the end user for now, but\nbreaking it out of the submodule checkout series hopefully makes review easier.\n\nThanks,\nStefan\n\nStefan Beller (3):\n  submodule: use absolute path for computing relative path connecting\n  test-lib-functions.sh: teach test_commit -C <dir>\n  submodule--helper: add intern-git-dir function\n\n Documentation/git-submodule.txt    | 15 ++++++++++-\n builtin/submodule--helper.c        | 33 ++++++++++++++++++++++-\n git-submodule.sh                   |  7 ++++-\n submodule.c                        | 55 ++++++++++++++++++++++++++++++++++----\n submodule.h                        |  1 +\n t/t7412-submodule-interngitdirs.sh | 41 ++++++++++++++++++++++++++++\n t/test-lib-functions.sh            | 20 ++++++++++----\n 7 files changed, 159 insertions(+), 13 deletions(-)\n create mode 100755 t/t7412-submodule-interngitdirs.sh\n\n-- \n2.11.0.rc2.18.g0126045.dirty\n\n"},{"id":"306246","messageId":"20161121204146.13665-4-sbeller@google.com","threadId":"44523","inReplyTo":"20161121204146.13665-1-sbeller@google.com","subject":"[PATCH 3/3] submodule--helper: add intern-git-dir function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-21T20:41:46Z","receivedAt":"2016-11-21T20:43:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When a submodule has its git dir inside the working dir, the submodule\nsupport for checkout that we plan to add in a later patch will fail.\n\nAdd functionality to migrate the git directory to be embedded\ninto the superprojects git directory.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n Documentation/git-submodule.txt    | 15 ++++++++++++-\n builtin/submodule--helper.c        | 33 ++++++++++++++++++++++++++++-\n git-submodule.sh                   |  7 ++++++-\n submodule.c                        | 43 ++++++++++++++++++++++++++++++++++++++\n submodule.h                        |  1 +\n t/t7412-submodule-interngitdirs.sh | 41 ++++++++++++++++++++++++++++++++++++\n 6 files changed, 137 insertions(+), 3 deletions(-)\n create mode 100755 t/t7412-submodule-interngitdirs.sh\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex d841573475..80d55350eb 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -22,7 +22,7 @@ SYNOPSIS\n \t      [commit] [--] [<path>...]\n 'git submodule' [--quiet] foreach [--recursive] <command>\n 'git submodule' [--quiet] sync [--recursive] [--] [<path>...]\n-\n+'git submodule' [--quiet] interngitdirs [--] [<path>...]\n \n DESCRIPTION\n -----------\n@@ -245,6 +245,19 @@ sync::\n If `--recursive` is specified, this command will recurse into the\n registered submodules, and sync any nested submodules within.\n \n+interngitdirs::\n+\tMove the git directory of submodules into its superprojects\n+\t`$GIT_DIR/modules` path and then connect the git directory and\n+\tits working directory by setting the `core.worktree` and adding\n+\ta .git file pointing to the git directory interned into the\n+\tsuperproject.\n++\n+\tA repository that was cloned independently and later added\n+\tas a submodule or old setups have the submodules git directory\n+\tinside the submodule instead of the\n++\n+\tThis command is recursive by default.\n+\n OPTIONS\n -------\n -q::\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4beeda5f9f..256f8e9439 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1076,6 +1076,36 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,\n \treturn 0;\n }\n \n+static int intern_git_dir(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\n+\tstruct option intern_gitdir_options[] = {\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper intern-git-dir [<path>...]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, intern_gitdir_options,\n+\t\t\t     git_submodule_helper_usage, 0);\n+\n+\tgitmodules_config();\n+\tgit_config(submodule_config, NULL);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n+\t\treturn 1;\n+\n+\tfor (i = 0; i < list.nr; i++)\n+\t\tmigrate_submodule_gitdir(list.entries[i]->name);\n+\n+\treturn 0;\n+}\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n@@ -1090,7 +1120,8 @@ static struct cmd_struct commands[] = {\n \t{\"resolve-relative-url\", resolve_relative_url},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test},\n \t{\"init\", module_init},\n-\t{\"remote-branch\", resolve_remote_submodule_branch}\n+\t{\"remote-branch\", resolve_remote_submodule_branch},\n+\t{\"intern-git-dir\", intern_git_dir}\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex a024a135d6..747e934df2 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1131,6 +1131,11 @@ cmd_sync()\n \tdone\n }\n \n+cmd_interngitdirs()\n+{\n+\tgit submodule--helper intern-git-dir \"$@\"\n+}\n+\n # This loop parses the command line arguments to find the\n # subcommand name to dispatch.  Parsing of the subcommand specific\n # options are primarily done by the subcommand implementations.\n@@ -1140,7 +1145,7 @@ cmd_sync()\n while test $# != 0 && test -z \"$command\"\n do\n \tcase \"$1\" in\n-\tadd | foreach | init | deinit | update | status | summary | sync)\n+\tadd | foreach | init | deinit | update | status | summary | sync | interngitdirs)\n \t\tcommand=$1\n \t\t;;\n \t-q|--quiet)\ndiff --git a/submodule.c b/submodule.c\nindex 66c5ce5a24..99befdba85 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1263,3 +1263,46 @@ void prepare_submodule_repo_env(struct argv_array *out)\n \t}\n \targv_array_push(out, \"GIT_DIR=.git\");\n }\n+\n+/*\n+ * Migrate the given submodule (and all its submodules recursively) from\n+ * having its git directory within the working tree to the git dir nested\n+ * in its superprojects git dir under modules/.\n+ */\n+void migrate_submodule_gitdir(const char *path)\n+{\n+\tchar *old_git_dir;\n+\tconst char *new_git_dir;\n+\tconst struct submodule *sub;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tcp.git_cmd = 1;\n+\tcp.no_stdin = 1;\n+\tcp.dir = path;\n+\targv_array_pushl(&cp.args, \"submodule\", \"foreach\", \"--recursive\",\n+\t\t\t\"git\", \"submodule--helper\" \"intern-git-dir\", NULL);\n+\n+\tif (run_command(&cp))\n+\t\tdie(_(\"Could not migrate git directory in submodule '%s'\"),\n+\t\t    path);\n+\n+\told_git_dir = xstrfmt(\"%s/.git\", path);\n+\tif (read_gitfile(old_git_dir))\n+\t\t/* If it is an actual gitfile, it doesn't need migration. */\n+\t\tgoto out;\n+\n+\tsub = submodule_from_path(null_sha1, path);\n+\tif (!sub)\n+\t\tdie(_(\"Could not lookup name for submodule '%s'\"),\n+\t\t      path);\n+\tnew_git_dir = git_common_path(\"modules/%s\", sub->name);\n+\tmkdir_in_gitdir(\".git/modules\");\n+\n+\tif (rename(old_git_dir, new_git_dir) < 0)\n+\t\tdie_errno(_(\"Could not migrate git directory from '%s' to '%s'\"),\n+\t\t\told_git_dir, new_git_dir);\n+\n+\tconnect_work_tree_and_git_dir(path, new_git_dir);\n+out:\n+\tfree(old_git_dir);\n+}\ndiff --git a/submodule.h b/submodule.h\nindex d9e197a948..859026ecfa 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -75,4 +75,5 @@ int parallel_submodules(void);\n  */\n void prepare_submodule_repo_env(struct argv_array *out);\n \n+extern void migrate_submodule_gitdir(const char *path);\n #endif\ndiff --git a/t/t7412-submodule-interngitdirs.sh b/t/t7412-submodule-interngitdirs.sh\nnew file mode 100755\nindex 0000000000..8938a4c8b7\n--- /dev/null\n+++ b/t/t7412-submodule-interngitdirs.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='Test submodule interngitdirs\n+\n+This test verifies that `git submodue interngitdirs` moves a submodules git\n+directory into the superproject.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a real submodule' '\n+\tgit init sub1 &&\n+\ttest_commit -C sub1 first &&\n+\tgit submodule add ./sub1 &&\n+\ttest_tick &&\n+\tgit commit -m superproject\n+'\n+\n+test_expect_success 'intern the git dir' '\n+\tgit submodule interngitdirs &&\n+\ttest -f sub1/.git &&\n+\ttest -d .git/modules/sub1 &&\n+\t# check that we did not break the repository:\n+\tgit status\n+'\n+\n+test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n+\tgit init sub2 &&\n+\ttest_commit -C sub2 first &&\n+\tgit add sub2 &&\n+\tgit commit -m superproject\n+'\n+\n+test_expect_success 'intern the git dir fails for incomplete submodules' '\n+\ttest_must_fail git submodule interngitdirs &&\n+\t# check that we did not break the repository:\n+\tgit status\n+'\n+\n+test_done\n+\n-- \n2.11.0.rc2.18.g0126045.dirty\n\n"},{"id":"306248","messageId":"xmqqd1hoimov.fsf@gitster.mtv.corp.google.com","threadId":"44523","inReplyTo":"20161121204146.13665-1-sbeller@google.com","subject":"Re: [PATCH 0/3] Introduce `submodule interngitdirs`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T20:48:32Z","receivedAt":"2016-11-21T20:48:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The discussion of the submodule checkout series revealed to me\n> that a command is needed to move the git directory from the\n> submodules working tree to be embedded into the superprojects git\n> directory.\n\nYou used \"move\" here, and \"migrate\" in the function name in 3/3,\nboth of which make sense.  \n\nBut \"intern\" sounds funny.  Who is confined as a prisoner here?\nNorth American English uses that verb as \"serve as an intern\", but\nthat does not apply here.  The verb also is used in Lisp-ish\nlanguages to mean the act of turning a string into a symbol, but\nthat does not apply here, either.\n"},{"id":"306249","messageId":"CAGZ79kZPZ5q_-XAb4FrzFDeZW8uYcy1byaD=Ron54Be=wTh5XQ@mail.gmail.com","threadId":"44523","inReplyTo":"xmqqd1hoimov.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/3] Introduce `submodule interngitdirs`","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-21T20:56:10Z","receivedAt":"2016-11-21T20:56:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 21, 2016 at 12:48 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> The discussion of the submodule checkout series revealed to me\n>> that a command is needed to move the git directory from the\n>> submodules working tree to be embedded into the superprojects git\n>> directory.\n>\n> You used \"move\" here, and \"migrate\" in the function name in 3/3,\n> both of which make sense.\n>\n> But \"intern\" sounds funny.  Who is confined as a prisoner here?\n> North American English uses that verb as \"serve as an intern\", but\n> that does not apply here.  The verb also is used in Lisp-ish\n> languages to mean the act of turning a string into a symbol, but\n> that does not apply here, either.\n\nI was inspired by the latter as we ask Git to make the submodule\n\"properly embedded\" into the superproject, which is what I imagined\nis similar to the lisp interning.\n\nSo I guess my imagination went to far and we rather want to invoke it via\n\"git submodule migrategitdirs\" ?\n\nBut there you would ask \"where are we migrating the git dirs to?\", so\nit would be reasonable to expect 2 parameters (just like the mv\ncommand).\n\nSo maybe \"git submodule embedgitdirs\" ?\n"},{"id":"306250","messageId":"xmqq8tscim31.fsf@gitster.mtv.corp.google.com","threadId":"44523","inReplyTo":"20161121204146.13665-2-sbeller@google.com","subject":"Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T21:01:38Z","receivedAt":"2016-11-21T21:01:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Subject: Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting\n\nconnecting what?  IOW, has the subject line somehow truncated?\n\n> This addresses a similar concern as in f8eaa0ba98b (submodule--helper,\n> module_clone: always operate on absolute paths, 2016-03-31)\n>\n> When computing the relative path from one to another location, we\n> need to provide both locations as absolute paths to make sure the\n> computation of the relative path is correct.\n\nCan the effect of this change demonstrated in a new test?  There\nmust be a scenario where the current behaviour is broken and this\nchange fixes an incorrect computation of relative path, no?\n\n>\n> While at it, change `real_work_tree` to be non const as we own\n> the memory.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\n>  submodule.c | 12 +++++++-----\n>  1 file changed, 7 insertions(+), 5 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 6f7d883de9..66c5ce5a24 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n>  {\n>  \tstruct strbuf file_name = STRBUF_INIT;\n>  \tstruct strbuf rel_path = STRBUF_INIT;\n> -\tconst char *real_work_tree = xstrdup(real_path(work_tree));\n> +\tchar *real_git_dir = xstrdup(real_path(git_dir));\n> +\tchar *real_work_tree = xstrdup(real_path(work_tree));\n>  \n>  \t/* Update gitfile */\n>  \tstrbuf_addf(&file_name, \"%s/.git\", work_tree);\n>  \twrite_file(file_name.buf, \"gitdir: %s\",\n> -\t\t   relative_path(git_dir, real_work_tree, &rel_path));\n> +\t\t   relative_path(real_git_dir, real_work_tree, &rel_path));\n>  \n>  \t/* Update core.worktree setting */\n>  \tstrbuf_reset(&file_name);\n> -\tstrbuf_addf(&file_name, \"%s/config\", git_dir);\n> +\tstrbuf_addf(&file_name, \"%s/config\", real_git_dir);\n>  \tgit_config_set_in_file(file_name.buf, \"core.worktree\",\n> -\t\t\t       relative_path(real_work_tree, git_dir,\n> +\t\t\t       relative_path(real_work_tree, real_git_dir,\n>  \t\t\t\t\t     &rel_path));\n>  \n>  \tstrbuf_release(&file_name);\n>  \tstrbuf_release(&rel_path);\n> -\tfree((void *)real_work_tree);\n> +\tfree(real_work_tree);\n> +\tfree(real_git_dir);\n>  }\n>  \n>  int parallel_submodules(void)\n"},{"id":"306251","messageId":"CAGZ79kY=tzrsn7rS4UsRfSku_pKNNWNDc2OiTO-4-vg5h8NwWQ@mail.gmail.com","threadId":"44523","inReplyTo":"xmqq8tscim31.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-21T21:03:42Z","receivedAt":"2016-11-21T21:03:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Subject: Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting\n>\n> connecting what?  IOW, has the subject line somehow truncated?\n>\n>> This addresses a similar concern as in f8eaa0ba98b (submodule--helper,\n>> module_clone: always operate on absolute paths, 2016-03-31)\n>>\n>> When computing the relative path from one to another location, we\n>> need to provide both locations as absolute paths to make sure the\n>> computation of the relative path is correct.\n>\n> Can the effect of this change demonstrated in a new test?  There\n> must be a scenario where the current behaviour is broken and this\n> change fixes an incorrect computation of relative path, no?\n>\n\nI found the latest patch of this series broken without this patch.\nI'll try to find existing broken code, though.\n"},{"id":"306252","messageId":"xmqq37ikilxk.fsf@gitster.mtv.corp.google.com","threadId":"44523","inReplyTo":"20161121204146.13665-3-sbeller@google.com","subject":"Re: [PATCH 2/3] test-lib-functions.sh: teach test_commit -C <dir>","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T21:04:55Z","receivedAt":"2016-11-21T21:05:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Specifically when setting up submodule tests, it comes in handy if\n> we can create commits in repositories that are not at the root of\n> the tested trash dir. Add \"-C <dir>\" similar to gits -C parameter\n> that will perform the operation in the given directory.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n\nLooks useful.\n\n>  t/test-lib-functions.sh | 20 +++++++++++++++-----\n>  1 file changed, 15 insertions(+), 5 deletions(-)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index fdaeb3a96b..579e812506 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -157,16 +157,21 @@ debug () {\n>  \t GIT_TEST_GDB=1 \"$@\"\n>  }\n>  \n> -# Call test_commit with the arguments \"<message> [<file> [<contents> [<tag>]]]\"\n> +# Call test_commit with the arguments\n> +# [-C <directory>] <message> [<file> [<contents> [<tag>]]]\"\n>  #\n>  # This will commit a file with the given contents and the given commit\n>  # message, and tag the resulting commit with the given tag name.\n>  #\n>  # <file>, <contents>, and <tag> all default to <message>.\n> +#\n> +# If the first argument is \"-C\", the second argument is used as a path for\n> +# the git invocations.\n>  \n>  test_commit () {\n>  \tnotick= &&\n>  \tsignoff= &&\n> +\tindir= &&\n>  \twhile test $# != 0\n>  \tdo\n>  \t\tcase \"$1\" in\n> @@ -176,21 +181,26 @@ test_commit () {\n>  \t\t--signoff)\n>  \t\t\tsignoff=\"$1\"\n>  \t\t\t;;\n> +\t\t-C)\n> +\t\t\tindir=\"$2\"\n> +\t\t\tshift\n> +\t\t\t;;\n>  \t\t*)\n>  \t\t\tbreak\n>  \t\t\t;;\n>  \t\tesac\n>  \t\tshift\n>  \tdone &&\n> +\tindir=${indir:+\"$indir\"/} &&\n>  \tfile=${2:-\"$1.t\"} &&\n> -\techo \"${3-$1}\" > \"$file\" &&\n> -\tgit add \"$file\" &&\n> +\techo \"${3-$1}\" > \"$indir$file\" &&\n> +\tgit ${indir:+ -C \"$indir\"} add \"$file\" &&\n>  \tif test -z \"$notick\"\n>  \tthen\n>  \t\ttest_tick\n>  \tfi &&\n> -\tgit commit $signoff -m \"$1\" &&\n> -\tgit tag \"${4:-$1}\"\n> +\tgit ${indir:+ -C \"$indir\"} commit $signoff -m \"$1\" &&\n> +\tgit ${indir:+ -C \"$indir\"} tag \"${4:-$1}\"\n>  }\n>  \n>  # Call test_merge with the arguments \"<message> <commit>\", where <commit>\n"},{"id":"306253","messageId":"xmqqy40ch6wp.fsf@gitster.mtv.corp.google.com","threadId":"44523","inReplyTo":"20161121204146.13665-4-sbeller@google.com","subject":"Re: [PATCH 3/3] submodule--helper: add intern-git-dir function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-21T21:14:46Z","receivedAt":"2016-11-21T21:22:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> +interngitdirs::\n> +\tMove the git directory of submodules into its superprojects\n> +\t`$GIT_DIR/modules` path and then connect the git directory and\n> +\tits working directory by setting the `core.worktree` and adding\n> +\ta .git file pointing to the git directory interned into the\n> +\tsuperproject.\n> ++\n> +\tA repository that was cloned independently and later added\n> +\tas a submodule or old setups have the submodules git directory\n> +\tinside the submodule instead of the\n> ++\n> +\tThis command is recursive by default.\n\nDoes this format correctly?\n\nI somehow thought that second and subsequent paragraphs continued\nwith \"+\" want no indentation before them.  See for example the\nValues section in config.txt and see how entries for boolean:: and\ncolor:: use multiple '+' paragraphs.\n\nIf we do not have to refrain from indenting the second and\nsubsequent paragraphs, that would be great for readability, but I\ntake the existing practice as telling me that we cannot do that X-<.\n\n> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n> +\tgit init sub2 &&\n> +\ttest_commit -C sub2 first &&\n> +\tgit add sub2 &&\n> +\tgit commit -m superproject\n> +'\n> +\n> +test_expect_success 'intern the git dir fails for incomplete submodules' '\n> +\ttest_must_fail git submodule interngitdirs &&\n> +\t# check that we did not break the repository:\n> +\tgit status\n> +'\n\nIt is not clear what the last \"git status\" wants to test.  In the\nextreme, if the failed \"git submodule\" command did\n\n\trm -fr .git ?* && git init\n\nwouldn't \"git status\" still succeed?  \n\nWhat are the minimum things that we expect from \"did not break\" to\nsee?  sub2/.git is still a directory and is a valid repository?  The\ncontents of the .git/modules/* before and after the \"git submodule\"\ndoes not change?  Some other things?\n"},{"id":"306257","messageId":"20161121215621.GB149321@google.com","threadId":"44523","inReplyTo":"20161121204146.13665-4-sbeller@google.com","subject":"Re: [PATCH 3/3] submodule--helper: add intern-git-dir function","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-11-21T21:56:21Z","receivedAt":"2016-11-21T21:56:29Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 11/21, Stefan Beller wrote:\n> diff --git a/t/t7412-submodule-interngitdirs.sh b/t/t7412-submodule-interngitdirs.sh\n> new file mode 100755\n> index 0000000000..8938a4c8b7\n> --- /dev/null\n> +++ b/t/t7412-submodule-interngitdirs.sh\n> @@ -0,0 +1,41 @@\n> +#!/bin/sh\n> +\n> +test_description='Test submodule interngitdirs\n> +\n> +This test verifies that `git submodue interngitdirs` moves a submodules git\n> +directory into the superproject.\n> +'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup a real submodule' '\n> +\tgit init sub1 &&\n> +\ttest_commit -C sub1 first &&\n> +\tgit submodule add ./sub1 &&\n> +\ttest_tick &&\n> +\tgit commit -m superproject\n> +'\n> +\n> +test_expect_success 'intern the git dir' '\n> +\tgit submodule interngitdirs &&\n> +\ttest -f sub1/.git &&\n> +\ttest -d .git/modules/sub1 &&\n> +\t# check that we did not break the repository:\n> +\tgit status\n> +'\n> +\n> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n> +\tgit init sub2 &&\n> +\ttest_commit -C sub2 first &&\n> +\tgit add sub2 &&\n> +\tgit commit -m superproject\n> +'\n> +\n> +test_expect_success 'intern the git dir fails for incomplete submodules' '\n> +\ttest_must_fail git submodule interngitdirs &&\n> +\t# check that we did not break the repository:\n> +\tgit status\n> +'\n> +\n> +test_done\n> +\n\nCould we add a test which has nested submodules that need to be\nmigrated?  Hopfully its just as easy as adding the test :)\n\n-- \nBrandon Williams\n"},{"id":"306268","messageId":"CAGZ79ka60_D8xfQyBegkXxkTGW5YDMuagB7kjhiCR6NriLR8+A@mail.gmail.com","threadId":"44523","inReplyTo":"CAGZ79kY=tzrsn7rS4UsRfSku_pKNNWNDc2OiTO-4-vg5h8NwWQ@mail.gmail.com","subject":"Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-22T00:04:47Z","receivedAt":"2016-11-22T00:04:54Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 21, 2016 at 1:03 PM, Stefan Beller <sbeller@google.com> wrote:\n> On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Can the effect of this change demonstrated in a new test?  There\n>> must be a scenario where the current behaviour is broken and this\n>> change fixes an incorrect computation of relative path, no?\n\nI do not think the current usage exposes this bug in\nconnect_work_tree_and_git_dir. It is only used in builtin/mv.c,\nwhich fills the second parameter `git_dir` via a call to read_gitfile,\nwhich itself produces an absolute path.\n\nThe latest patch of this series however passes in relative path for the\nrespective git directories.\n\nSo the commit message of this patch is misleading at least.\nMaybe:\n\n  The current caller of connect_work_tree_and_git_dir passes\n  an absolute path for the `git_dir` parameter. In the future patch\n  we will also pass in relative path for `git_dir`. Extend the functionality\n  of connect_work_tree_and_git_dir to take relative paths for parameters.\n\n  We could work around this in the future patch by computing the absolute\n  path for the git_dir in the calling site, however accepting relative\n  paths for either parameter makes the API for this function easier\n  to use.\n\n  While at it, change `real_work_tree` to be non const as we own\n  the memory.\n\n\n>\n> I found the latest patch of this series broken without this patch.\n> I'll try to find existing broken code, though.\n"},{"id":"306281","messageId":"CAGZ79kb_4wWs_90AfsT932iPWbCXf6yRq875JUxoRZjUcsBW5A@mail.gmail.com","threadId":"44523","inReplyTo":"xmqqy40ch6wp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] submodule--helper: add intern-git-dir function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-22T02:09:41Z","receivedAt":"2016-11-22T02:09:48Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 21, 2016 at 1:14 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Does this format correctly?\n>\n> I somehow thought that second and subsequent paragraphs continued\n> with \"+\" want no indentation before them.  See for example the\n> Values section in config.txt and see how entries for boolean:: and\n> color:: use multiple '+' paragraphs.\n>\n> If we do not have to refrain from indenting the second and\n> subsequent paragraphs, that would be great for readability, but I\n> take the existing practice as telling me that we cannot do that X-<.\n\nWill fix and test in a resend.\n\n\n>\n>> +test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n>> +     git init sub2 &&\n>> +     test_commit -C sub2 first &&\n>> +     git add sub2 &&\n>> +     git commit -m superproject\n>> +'\n>> +\n>> +test_expect_success 'intern the git dir fails for incomplete submodules' '\n>> +     test_must_fail git submodule interngitdirs &&\n>> +     # check that we did not break the repository:\n>> +     git status\n>> +'\n>\n> It is not clear what the last \"git status\" wants to test.\n\nAny errors that I ran into when manually truing to embed a submodules\ngit dir, were fatal with `git status` already, e.g. missing or wrong\ncall of connect_work_tree_and_git_dir were my main failure points.\n\nSo I guess we should test a bit more extensively, maybe\n\n    git status >expect\n    git submodule embedgitdirs\n    git status >actual\n    test_cmp expect actual\n    # further testing via\n    test -f ..\n    test -d ..\n\n>  In the\n> extreme, if the failed \"git submodule\" command did\n>\n>         rm -fr .git ?* && git init\n>\n> wouldn't \"git status\" still succeed?\n\n    In that particular case you'd get\n    $ git status\n    fatal: Not a git repository (or any parent up to mount point ....)\n    Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\n    $ echo $?\n    128\n\nbut I get the idea, which is why I propose the double check via status.\nThat would detect any logical change for the repository, e.g. a\nchange to the .gitmodules file.\n\n>\n> What are the minimum things that we expect from \"did not break\" to\n> see?  sub2/.git is still a directory and is a valid repository?  The\n> contents of the .git/modules/* before and after the \"git submodule\"\n> does not change?  Some other things?\n\nI thought about making up a name for such a repo and creating that engry\nin .gitmodules. But I refrained from doing so, because it seems too much\nfor this command.\n\nI dunno, but I would suspect the double status is fine here, too?\n"},{"id":"306287","messageId":"xmqqr364f152.fsf@gitster.mtv.corp.google.com","threadId":"44523","inReplyTo":"CAGZ79ka60_D8xfQyBegkXxkTGW5YDMuagB7kjhiCR6NriLR8+A@mail.gmail.com","subject":"Re: [PATCH 1/3] submodule: use absolute path for computing relative path connecting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T07:02:17Z","receivedAt":"2016-11-22T07:02:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Mon, Nov 21, 2016 at 1:03 PM, Stefan Beller <sbeller@google.com> wrote:\n>> On Mon, Nov 21, 2016 at 1:01 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>>\n>>> Can the effect of this change demonstrated in a new test?  There\n>>> must be a scenario where the current behaviour is broken and this\n>>> change fixes an incorrect computation of relative path, no?\n>\n> I do not think the current usage exposes this bug in\n> connect_work_tree_and_git_dir. It is only used in builtin/mv.c,\n> which fills the second parameter `git_dir` via a call to read_gitfile,\n> which itself produces an absolute path.\n\nOK.  Fixing a potential bug as a preparatory step is good.\n\n>   The current caller of connect_work_tree_and_git_dir passes\n>   an absolute path for the `git_dir` parameter. In the future patch\n>   we will also pass in relative path for `git_dir`. Extend the functionality\n>   of connect_work_tree_and_git_dir to take relative paths for parameters.\n>\n>   We could work around this in the future patch by computing the absolute\n>   path for the git_dir in the calling site, however accepting relative\n>   paths for either parameter makes the API for this function easier\n>   to use.\n\nYup, sounds sensible.  Thanks.\n"},{"id":"306288","messageId":"xmqqmvgsf0wo.fsf@gitster.mtv.corp.google.com","threadId":"44523","inReplyTo":"CAGZ79kb_4wWs_90AfsT932iPWbCXf6yRq875JUxoRZjUcsBW5A@mail.gmail.com","subject":"Re: [PATCH 3/3] submodule--helper: add intern-git-dir function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T07:07:19Z","receivedAt":"2016-11-22T07:07:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> So I guess we should test a bit more extensively, maybe\n>\n>     git status >expect\n>     git submodule embedgitdirs\n>     git status >actual\n>     test_cmp expect actual\n>     # further testing via\n>     test -f ..\n>     test -d ..\n\nSomething along that line.  \"status should succeed\" does not tell\nthe readers what kind of breakage the test is expecting to protect\nus from.  If we are expecting a breakage in embed-git-dirs would\nsomehow corrupt an existing submodule, which would lead to \"status\"\nthat is run in the superproject report the submodule differently,\nthen comparing output before and after the operation may be a\nreasonable test.  Going there to the submodule working tree and\nchecking the health of the repository (of the submodule) may be\nanother sensible test.\n\n>>  In the\n>> extreme, if the failed \"git submodule\" command did\n>>\n>>         rm -fr .git ?* && git init\n>>\n>> wouldn't \"git status\" still succeed?\n>\n>     In that particular case you'd get\n>     $ git status\n>     fatal: Not a git repository (or any parent up to mount point ....)\n\nEven with \"&& git init\"?  Or you forgot that part?\n"},{"id":"306314","messageId":"CAGZ79kb7062abd6Cbf-ey3u0L6PXUcRf7m-og5EyCRZencOR6g@mail.gmail.com","threadId":"44523","inReplyTo":"xmqqmvgsf0wo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/3] submodule--helper: add intern-git-dir function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-11-22T17:16:36Z","receivedAt":"2016-11-22T17:16:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 21, 2016 at 11:07 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> So I guess we should test a bit more extensively, maybe\n>>\n>>     git status >expect\n>>     git submodule embedgitdirs\n>>     git status >actual\n>>     test_cmp expect actual\n>>     # further testing via\n>>     test -f ..\n>>     test -d ..\n>\n> Something along that line.  \"status should succeed\" does not tell\n> the readers what kind of breakage the test is expecting to protect\n> us from.  If we are expecting a breakage in embed-git-dirs would\n> somehow corrupt an existing submodule, which would lead to \"status\"\n> that is run in the superproject report the submodule differently,\n> then comparing output before and after the operation may be a\n> reasonable test.  Going there to the submodule working tree and\n> checking the health of the repository (of the submodule) may be\n> another sensible test.\n\nand by checking the health you mean just a status in there, or rather a\nmore nuanced thing like `git rev-parse HEAD` ? I'll go with that for now.\n\n>\n>>>  In the\n>>> extreme, if the failed \"git submodule\" command did\n>>>\n>>>         rm -fr .git ?* && git init\n>>>\n>>> wouldn't \"git status\" still succeed?\n>>\n>>     In that particular case you'd get\n>>     $ git status\n>>     fatal: Not a git repository (or any parent up to mount point ....)\n>\n> Even with \"&& git init\"?  Or you forgot that part?\n\nyes I did, because why would you run init after an accidental rm -rf ... ?\n"},{"id":"306322","messageId":"xmqqfumjqu4c.fsf@gitster.mtv.corp.google.com","threadId":"44523","inReplyTo":"CAGZ79kb7062abd6Cbf-ey3u0L6PXUcRf7m-og5EyCRZencOR6g@mail.gmail.com","subject":"Re: [PATCH 3/3] submodule--helper: add intern-git-dir function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-22T17:53:07Z","receivedAt":"2016-11-22T17:53:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> On Mon, Nov 21, 2016 at 11:07 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> So I guess we should test a bit more extensively, maybe\n>>>\n>>>     git status >expect\n>>>     git submodule embedgitdirs\n>>>     git status >actual\n>>>     test_cmp expect actual\n>>>     # further testing via\n>>>     test -f ..\n>>>     test -d ..\n>>\n>> Something along that line.  \"status should succeed\" does not tell\n>> the readers what kind of breakage the test is expecting to protect\n>> us from.  If we are expecting a breakage in embed-git-dirs would\n>> somehow corrupt an existing submodule, which would lead to \"status\"\n>> that is run in the superproject report the submodule differently,\n>> then comparing output before and after the operation may be a\n>> reasonable test.  Going there to the submodule working tree and\n>> checking the health of the repository (of the submodule) may be\n>> another sensible test.\n>\n> and by checking the health you mean just a status in there, or rather a\n> more nuanced thing like `git rev-parse HEAD` ? I'll go with that for now.\n\nWould fsck have caught the breakages you caused while developing it,\nfor example?\n\nAs the core of the \"embed\" operation is an non-atomic \"move the .git\ndirectory to .git/modules/$name in the superproject and then make a\ngitfile pointing at it\", verifying that there is no change in the\ncontents of .git/modules before and after the failed operation, and\nverifying that the submodule working tree has the embedded .git/\ndirectory would be good enough, I would think.\n"}]}