Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs
On 08/10, Junio C Hamano wrote: > Brandon Williams writes: > > > Introduce a helper function "submodule_name_to_gitdir()" (and the > > submodule--helper subcommand "gitdir") which constructs a path to a > > submodule's gitdir, located in the provided repository's "modules" > > directory. > > > > This consolidates the logic needed to build up a path into a > > repository's "modules" directory, abstracting away the fact that > > submodule git directories are stored in a repository's common gitdir. > > This makes it easier to adjust how submodules gitdir are stored in the > > "modules" directory in a future patch. > > > > Signed-off-by: Brandon Williams > > --- > > ... > > @@ -2018,7 +2039,7 @@ static int connect_gitdir_workingtree(int argc, const > > char **argv, const char *p > > name = argv[1]; > > path = argv[2]; > > > > - strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); > > + submodule_name_to_gitdir(&sb, the_repository, name); > > sm_gitdir = absolute_pathdup(sb.buf); > > > > connect_work_tree_and_git_dir(path, sm_gitdir, 0); > > This function goes away with 1c866b98 ("submodule--helper: replace > connect-gitdir-workingtree by ensure-core-worktree", 2018-08-03) in > sb/submodule-update-in-c topic. git-submodule.sh has simlar > conflicts. > > I guess its replacement function does not care as deeply as its > predecessor used to about where the submodule's $GIT_DIR is, so the > correct resolution may be just to ignore the change made to this > caller to the new name-to-gitdir function. Well that patch still cares about where the gitdir is except it initializes a "struct repository" for the submodule and then builds a path to the config using: cfg_file = xstrfmt("%s/config", subrepo.gitdir); hmm...I didn't get a chance to look at that series but that line looks wrong. It probably should be more like: cfg_file = repo_git_path(&subrepo, "config"); I'll go comment on that series. > > It would have been nicer to see a bit better inter-developer > coordination, especially between two who sit practically next to > each other ;-) > > Thanks. -- Brandon Williams
Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs
Brandon Williams writes: > Introduce a helper function "submodule_name_to_gitdir()" (and the > submodule--helper subcommand "gitdir") which constructs a path to a > submodule's gitdir, located in the provided repository's "modules" > directory. > > This consolidates the logic needed to build up a path into a > repository's "modules" directory, abstracting away the fact that > submodule git directories are stored in a repository's common gitdir. > This makes it easier to adjust how submodules gitdir are stored in the > "modules" directory in a future patch. > > Signed-off-by: Brandon Williams > --- > ... > @@ -2018,7 +2039,7 @@ static int connect_gitdir_workingtree(int argc, const > char **argv, const char *p > name = argv[1]; > path = argv[2]; > > - strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); > + submodule_name_to_gitdir(&sb, the_repository, name); > sm_gitdir = absolute_pathdup(sb.buf); > > connect_work_tree_and_git_dir(path, sm_gitdir, 0); This function goes away with 1c866b98 ("submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree", 2018-08-03) in sb/submodule-update-in-c topic. git-submodule.sh has simlar conflicts. I guess its replacement function does not care as deeply as its predecessor used to about where the submodule's $GIT_DIR is, so the correct resolution may be just to ignore the change made to this caller to the new name-to-gitdir function. It would have been nicer to see a bit better inter-developer coordination, especially between two who sit practically next to each other ;-) Thanks.
Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs
On 08/08, Stefan Beller wrote: > On Wed, Aug 8, 2018 at 3:33 PM Brandon Williams wrote: > > > > Introduce a helper function "submodule_name_to_gitdir()" (and the > > submodule--helper subcommand "gitdir") which constructs a path to a > > submodule's gitdir, located in the provided repository's "modules" > > directory. > > Makes sense. > > > > > This consolidates the logic needed to build up a path into a > > repository's "modules" directory, abstracting away the fact that > > submodule git directories are stored in a repository's common gitdir. > > This makes it easier to adjust how submodules gitdir are stored in the > > "modules" directory in a future patch. > > and yet, all places that we touch were and still are broken for old-style > submodules that have their git directory inside the working tree? > Do we need to pay attention to those, too? This series only tries to address the issues with submodules stored in $GITDIR/modules/ and places in our codebase that explicitly reference submodules stored there. For those old-old-style submodules, wouldn't the absorb submodule functions handle that migration? > > > > diff --git a/git-submodule.sh b/git-submodule.sh > > index 8b5ad59bde..053747d290 100755 > > --- a/git-submodule.sh > > +++ b/git-submodule.sh > > > @@ -577,7 +578,7 @@ cmd_update() > > die "$(eval_gettext "Unable to find current > > \${remote_name}/\${branch} revision in submodule path '\$sm_path'")" > > fi > > > > - if ! $(git config -f "$(git rev-parse > > --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null > > + if ! $(git config -f "$(git submodule--helper gitdir > > "$name")/config" core.worktree) 2>/dev/null > > This will collide with origin/sb/submodule-update-in-c specifically > 1c866b9831d (submodule--helper: replace connect-gitdir-workingtree > by ensure-core-worktree, 2018-08-03), but as that removes these lines, > it should be easy to resolve the conflict. -- Brandon Williams
Re: [PATCH 1/2] submodule: create helper to build paths to submodule gitdirs
On Wed, Aug 8, 2018 at 3:33 PM Brandon Williams wrote: > > Introduce a helper function "submodule_name_to_gitdir()" (and the > submodule--helper subcommand "gitdir") which constructs a path to a > submodule's gitdir, located in the provided repository's "modules" > directory. Makes sense. > > This consolidates the logic needed to build up a path into a > repository's "modules" directory, abstracting away the fact that > submodule git directories are stored in a repository's common gitdir. > This makes it easier to adjust how submodules gitdir are stored in the > "modules" directory in a future patch. and yet, all places that we touch were and still are broken for old-style submodules that have their git directory inside the working tree? Do we need to pay attention to those, too? > diff --git a/git-submodule.sh b/git-submodule.sh > index 8b5ad59bde..053747d290 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -577,7 +578,7 @@ cmd_update() > die "$(eval_gettext "Unable to find current > \${remote_name}/\${branch} revision in submodule path '\$sm_path'")" > fi > > - if ! $(git config -f "$(git rev-parse > --git-common-dir)/modules/$name/config" core.worktree) 2>/dev/null > + if ! $(git config -f "$(git submodule--helper gitdir > "$name")/config" core.worktree) 2>/dev/null This will collide with origin/sb/submodule-update-in-c specifically 1c866b9831d (submodule--helper: replace connect-gitdir-workingtree by ensure-core-worktree, 2018-08-03), but as that removes these lines, it should be easy to resolve the conflict.
[PATCH 1/2] submodule: create helper to build paths to submodule gitdirs
Introduce a helper function "submodule_name_to_gitdir()" (and the submodule--helper subcommand "gitdir") which constructs a path to a submodule's gitdir, located in the provided repository's "modules" directory. This consolidates the logic needed to build up a path into a repository's "modules" directory, abstracting away the fact that submodule git directories are stored in a repository's common gitdir. This makes it easier to adjust how submodules gitdir are stored in the "modules" directory in a future patch. Signed-off-by: Brandon Williams --- builtin/submodule--helper.c | 28 +++-- dir.c| 2 +- git-submodule.sh | 7 +++-- repository.c | 3 +- submodule.c | 53 submodule.h | 7 + t/t7410-submodule-checkout-to.sh | 6 ++-- 7 files changed, 75 insertions(+), 31 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index a3c4564c6c..5bfd2d0be9 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -906,6 +906,21 @@ static int module_name(int argc, const char **argv, const char *prefix) return 0; } +static int module_gitdir(int argc, const char **argv, const char *prefix) +{ + struct strbuf gitdir = STRBUF_INIT; + + if (argc != 2) + usage(_("git submodule--helper gitdir ")); + + submodule_name_to_gitdir(&gitdir, the_repository, argv[1]); + + printf("%s\n", gitdir.buf); + + strbuf_release(&gitdir); + return 0; +} + struct sync_cb { const char *prefix; unsigned int flags; @@ -1268,18 +1283,24 @@ static int add_possible_reference_from_superproject( * standard layout with .git/(modules/)+/objects */ if (ends_with(alt->path, "/objects")) { + struct repository alternate; char *sm_alternate; struct strbuf sb = STRBUF_INIT; struct strbuf err = STRBUF_INIT; strbuf_add(&sb, alt->path, strlen(alt->path) - strlen("objects")); + repo_init(&alternate, sb.buf, NULL); + /* * We need to end the new path with '/' to mark it as a dir, * otherwise a submodule name containing '/' will be broken * as the last part of a missing submodule reference would * be taken as a file name. */ - strbuf_addf(&sb, "modules/%s/", sas->submodule_name); + strbuf_reset(&sb); + submodule_name_to_gitdir(&sb, &alternate, sas->submodule_name); + strbuf_addch(&sb, '/'); + repo_clear(&alternate); sm_alternate = compute_alternate_path(sb.buf, &err); if (sm_alternate) { @@ -1392,7 +1413,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) usage_with_options(git_submodule_helper_usage, module_clone_options); - strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); + submodule_name_to_gitdir(&sb, the_repository, name); sm_gitdir = absolute_pathdup(sb.buf); strbuf_reset(&sb); @@ -2018,7 +2039,7 @@ static int connect_gitdir_workingtree(int argc, const char **argv, const char *p name = argv[1]; path = argv[2]; - strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name); + submodule_name_to_gitdir(&sb, the_repository, name); sm_gitdir = absolute_pathdup(sb.buf); connect_work_tree_and_git_dir(path, sm_gitdir, 0); @@ -2040,6 +2061,7 @@ struct cmd_struct { static struct cmd_struct commands[] = { {"list", module_list, 0}, {"name", module_name, 0}, + {"gitdir", module_gitdir, 0}, {"clone", module_clone, 0}, {"update-clone", update_clone, 0}, {"connect-gitdir-workingtree", connect_gitdir_workingtree, 0}, diff --git a/dir.c b/dir.c index 21e6f2520a..7a9827ea4b 100644 --- a/dir.c +++ b/dir.c @@ -3053,7 +3053,7 @@ static void connect_wt_gitdir_in_nested(const char *sub_worktree, strbuf_reset(&sub_wt); strbuf_reset(&sub_gd); strbuf_addf(&sub_wt, "%s/%s", sub_worktree, sub->path); - strbuf_addf(&sub_gd, "%s/modules/%s", sub_gitdir, sub->name); + submodule_name_to_gitdir(&sub_gd, &subrepo, sub->name); connect_work_tree_and_git_dir(sub_wt.buf, sub_gd.buf, 1); } diff --git a/git-submodule.sh b/git-submodule.sh index 8b5ad59bde..053747d290 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -252,12 +252,13 @@ Use -f if you really want to add it." >&2 fi else - if test -d ".git/modules/$sm_name" + sm_gitdir="$(git submodule--helper gitdir "$sm_name")" + if test -d "$sm_gitdir"