Re: [PATCH 3/3] submodule--helper: add intern-git-dir function
Stefan Beller writes: > On Mon, Nov 21, 2016 at 11:07 PM, Junio C Hamano wrote: >> Stefan Beller 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
On Mon, Nov 21, 2016 at 11:07 PM, Junio C Hamano wrote: > Stefan Beller 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
Stefan Beller 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
On Mon, Nov 21, 2016 at 1:14 PM, Junio C Hamano 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
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 00..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 3/3] submodule--helper: add intern-git-dir function
Stefan Beller 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?
[PATCH 3/3] submodule--helper: add intern-git-dir function
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
---
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] [--] [...]
'git submodule' [--quiet] foreach [--recursive]
'git submodule' [--quiet] sync [--recursive] [--] [...]
-
+'git submodule' [--quiet] interngitdirs [--] [...]
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 [...]"),
+ 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 c
