* [PATCHv6 3/7] test-lib-functions.sh: teach test_commit -C <dir>
From: Stefan Beller @ 2016-12-08 1:46 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161208014623.7588-1-sbeller@google.com>
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>
Signed-off-by: Junio C Hamano <gitster@pobox.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.30.gc512cbd.dirty
^ permalink raw reply related
* [PATCHv6 5/7] worktree: add function to check if worktrees are in use
From: Stefan Beller @ 2016-12-08 1:46 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161208014623.7588-1-sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
worktree.c | 24 ++++++++++++++++++++++++
worktree.h | 7 +++++++
2 files changed, 31 insertions(+)
diff --git a/worktree.c b/worktree.c
index 75db689672..2559f33846 100644
--- a/worktree.c
+++ b/worktree.c
@@ -406,3 +406,27 @@ const struct worktree *find_shared_symref(const char *symref,
return existing;
}
+
+static int uses_worktree_internal(struct worktree **worktrees)
+{
+ int i;
+ for (i = 0; worktrees[i]; i++)
+ ; /* nothing */
+
+ free_worktrees(worktrees);
+ return i > 1;
+}
+
+int uses_worktrees(void)
+{
+ return uses_worktree_internal(get_worktrees(0));
+}
+
+int submodule_uses_worktrees(const char *path)
+{
+ struct worktree **worktrees = get_submodule_worktrees(path, 0);
+ if (!worktrees)
+ return 0;
+
+ return uses_worktree_internal(worktrees);
+}
diff --git a/worktree.h b/worktree.h
index 157fbc4a66..76027b1fd2 100644
--- a/worktree.h
+++ b/worktree.h
@@ -33,6 +33,13 @@ extern struct worktree **get_worktrees(unsigned flags);
extern struct worktree **get_submodule_worktrees(const char *path,
unsigned flags);
+/*
+ * Returns 1 if more than one worktree exists.
+ * Returns 0 if only the main worktree exists.
+ */
+extern int uses_worktrees(void);
+extern int submodule_uses_worktrees(const char *path);
+
/*
* Return git dir of the worktree. Note that the path may be relative.
* If wt is NULL, git dir of current worktree is returned.
--
2.11.0.rc2.30.gc512cbd.dirty
^ permalink raw reply related
* [PATCHv6 7/7] submodule: add absorb-git-dir function
From: Stefan Beller @ 2016-12-08 1:46 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161208014623.7588-1-sbeller@google.com>
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 absorbed
into the superprojects git directory.
The newly added code in this patch is structured such that other areas of
Git can also make use of it. The code in the submodule--helper is a mere
wrapper and option parser for the function
`absorb_git_dir_into_superproject`, that takes care of embedding the
submodules git directory into the superprojects git dir. That function
makes use of the more abstract function for this use case
`relocate_gitdir`, which can be used by e.g. the worktree code eventually
to move around a git directory.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/git-submodule.txt | 15 ++++++
builtin/submodule--helper.c | 38 ++++++++++++++
dir.c | 12 +++++
dir.h | 3 ++
git-submodule.sh | 7 ++-
submodule.c | 103 +++++++++++++++++++++++++++++++++++++
submodule.h | 4 ++
t/t7412-submodule-absorbgitdirs.sh | 101 ++++++++++++++++++++++++++++++++++++
8 files changed, 282 insertions(+), 1 deletion(-)
create mode 100755 t/t7412-submodule-absorbgitdirs.sh
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..918bd1d1bd 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -22,6 +22,7 @@ SYNOPSIS
[commit] [--] [<path>...]
'git submodule' [--quiet] foreach [--recursive] <command>
'git submodule' [--quiet] sync [--recursive] [--] [<path>...]
+'git submodule' [--quiet] absorbgitdirs [--] [<path>...]
DESCRIPTION
@@ -245,6 +246,20 @@ sync::
If `--recursive` is specified, this command will recurse into the
registered submodules, and sync any nested submodules within.
+absorbgitdirs::
+ If a git directory of a submodule is inside the submodule,
+ move the git directory of the submodule 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 embedded in the
+ superprojects git directory.
++
+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
+embedded into the superprojects git directory.
++
+This command is recursive by default.
+
OPTIONS
-------
-q::
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 33676a57cf..0108afac93 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,43 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
return 0;
}
+static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
+{
+ int i;
+ struct pathspec pathspec;
+ struct module_list list = MODULE_LIST_INIT;
+ unsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES;
+
+ struct option embed_gitdir_options[] = {
+ OPT_STRING(0, "prefix", &prefix,
+ N_("path"),
+ N_("path into the working tree")),
+ OPT_BIT(0, "--recursive", &flags, N_("recurse into submodules"),
+ ABSORB_GITDIR_RECURSE_SUBMODULES),
+ OPT_END()
+ };
+
+ const char *const git_submodule_helper_usage[] = {
+ N_("git submodule--helper embed-git-dir [<path>...]"),
+ NULL
+ };
+
+ argc = parse_options(argc, argv, prefix, embed_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++)
+ absorb_git_dir_into_superproject(prefix,
+ list.entries[i]->name, flags);
+
+ return 0;
+}
+
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@@ -1094,6 +1131,7 @@ static struct cmd_struct commands[] = {
{"resolve-relative-url-test", resolve_relative_url_test, 0},
{"init", module_init, 0},
{"remote-branch", resolve_remote_submodule_branch, 0},
+ {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
};
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/dir.c b/dir.c
index 8b74997c66..cc5729f733 100644
--- a/dir.c
+++ b/dir.c
@@ -2774,3 +2774,15 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
free(real_work_tree);
free(real_git_dir);
}
+
+/*
+ * Migrate the git directory of the given path from old_git_dir to new_git_dir.
+ */
+void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
+{
+ 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);
+}
diff --git a/dir.h b/dir.h
index 051674a431..bf23a470af 100644
--- a/dir.h
+++ b/dir.h
@@ -336,4 +336,7 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra
void add_untracked_cache(struct index_state *istate);
void remove_untracked_cache(struct index_state *istate);
extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
+extern void relocate_gitdir(const char *path,
+ const char *old_git_dir,
+ const char *new_git_dir);
#endif
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a135d6..9285b5c43d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1131,6 +1131,11 @@ cmd_sync()
done
}
+cmd_absorbgitdirs()
+{
+ git submodule--helper absorb-git-dirs --prefix "$wt_prefix" "$@"
+}
+
# 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 | absorbgitdirs)
command=$1
;;
-q|--quiet)
diff --git a/submodule.c b/submodule.c
index 0bb50b4b62..6477746ce4 100644
--- a/submodule.c
+++ b/submodule.c
@@ -14,6 +14,7 @@
#include "blob.h"
#include "thread-utils.h"
#include "quote.h"
+#include "worktree.h"
static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;
static int parallel_jobs = 1;
@@ -1237,3 +1238,105 @@ void prepare_submodule_repo_env(struct argv_array *out)
}
argv_array_push(out, "GIT_DIR=.git");
}
+
+/*
+ * Embeds a single submodules git directory into the superprojects git dir,
+ * non recursively.
+ */
+static void relocate_single_git_dir_into_superproject(const char *prefix,
+ const char *path)
+{
+ char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
+ const char *new_git_dir;
+ const struct submodule *sub;
+
+ if (submodule_uses_worktrees(path))
+ die(_("relocate_gitdir for submodule '%s' with "
+ "more than one worktree not supported"), 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. */
+ return;
+
+ real_old_git_dir = xstrdup(real_path(old_git_dir));
+
+ sub = submodule_from_path(null_sha1, path);
+ if (!sub)
+ die(_("could not lookup name for submodule '%s'"), path);
+
+ new_git_dir = git_path("modules/%s", sub->name);
+ if (safe_create_leading_directories_const(new_git_dir) < 0)
+ die(_("could not create directory '%s'"), new_git_dir);
+ real_new_git_dir = xstrdup(real_path(new_git_dir));
+
+ if (!prefix)
+ prefix = get_super_prefix();
+
+ fprintf(stderr, "Migrating git directory of '%s%s' from\n'%s' to\n'%s'\n",
+ prefix ? prefix : "", path,
+ real_old_git_dir, real_new_git_dir);
+
+ relocate_gitdir(path, real_old_git_dir, real_new_git_dir);
+
+ free(old_git_dir);
+ free(real_old_git_dir);
+ free(real_new_git_dir);
+}
+
+/*
+ * Migrate the git directory of the submodule given by path from
+ * having its git directory within the working tree to the git dir nested
+ * in its superprojects git dir under modules/.
+ */
+void absorb_git_dir_into_superproject(const char *prefix,
+ const char *path,
+ unsigned flags)
+{
+ const char *sub_git_dir, *v;
+ char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;
+ struct strbuf gitdir = STRBUF_INIT;
+
+ strbuf_addf(&gitdir, "%s/.git", path);
+ sub_git_dir = resolve_gitdir(gitdir.buf);
+
+ /* Not populated? */
+ if (!sub_git_dir)
+ goto out;
+
+ /* Is it already absorbed into the superprojects git dir? */
+ real_sub_git_dir = xstrdup(real_path(sub_git_dir));
+ real_common_git_dir = xstrdup(real_path(get_git_common_dir()));
+ if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))
+ relocate_single_git_dir_into_superproject(prefix, path);
+
+ if (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {
+ struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf sb = STRBUF_INIT;
+
+ if (flags & ~ABSORB_GITDIR_RECURSE_SUBMODULES)
+ die("BUG: we don't know how to pass the flags down?");
+
+ if (get_super_prefix())
+ strbuf_addstr(&sb, get_super_prefix());
+ strbuf_addstr(&sb, path);
+ strbuf_addch(&sb, '/');
+
+ cp.dir = path;
+ cp.git_cmd = 1;
+ cp.no_stdin = 1;
+ argv_array_pushl(&cp.args, "--super-prefix", sb.buf,
+ "submodule--helper",
+ "absorb-git-dirs", NULL);
+ prepare_submodule_repo_env(&cp.env_array);
+ if (run_command(&cp))
+ die(_("could not recurse into submodule '%s'"), path);
+
+ strbuf_release(&sb);
+ }
+
+out:
+ strbuf_release(&gitdir);
+ free(real_sub_git_dir);
+ free(real_common_git_dir);
+}
diff --git a/submodule.h b/submodule.h
index 4e3bf469b4..6229054b99 100644
--- a/submodule.h
+++ b/submodule.h
@@ -74,4 +74,8 @@ int parallel_submodules(void);
*/
void prepare_submodule_repo_env(struct argv_array *out);
+#define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)
+extern void absorb_git_dir_into_superproject(const char *prefix,
+ const char *path,
+ unsigned flags);
#endif
diff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh
new file mode 100755
index 0000000000..7c4e8b8d79
--- /dev/null
+++ b/t/t7412-submodule-absorbgitdirs.sh
@@ -0,0 +1,101 @@
+#!/bin/sh
+
+test_description='Test submodule absorbgitdirs
+
+This test verifies that `git submodue absorbgitdirs` 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 'absorb the git dir' '
+ >expect.1 &&
+ >expect.2 &&
+ >actual.1 &&
+ >actual.2 &&
+ git status >expect.1 &&
+ git -C sub1 rev-parse HEAD >expect.2 &&
+ git submodule absorbgitdirs &&
+ git fsck &&
+ test -f sub1/.git &&
+ test -d .git/modules/sub1 &&
+ git status >actual.1 &&
+ git -C sub1 rev-parse HEAD >actual.2 &&
+ test_cmp expect.1 actual.1 &&
+ test_cmp expect.2 actual.2
+'
+
+test_expect_success 'absorbing does not fail for deinitalized submodules' '
+ test_when_finished "git submodule update --init" &&
+ git submodule deinit --all &&
+ git submodule absorbgitdirs &&
+ test -d .git/modules/sub1 &&
+ test -d sub1 &&
+ ! test -e sub1/.git
+'
+
+test_expect_success 'setup nested submodule' '
+ git init sub1/nested &&
+ test_commit -C sub1/nested first_nested &&
+ git -C sub1 submodule add ./nested &&
+ test_tick &&
+ git -C sub1 commit -m "add nested" &&
+ git add sub1 &&
+ git commit -m "sub1 to include nested submodule"
+'
+
+test_expect_success 'absorb the git dir in a nested submodule' '
+ git status >expect.1 &&
+ git -C sub1/nested rev-parse HEAD >expect.2 &&
+ git submodule absorbgitdirs &&
+ test -f sub1/nested/.git &&
+ test -d .git/modules/sub1/modules/nested &&
+ git status >actual.1 &&
+ git -C sub1/nested rev-parse HEAD >actual.2 &&
+ test_cmp expect.1 actual.1 &&
+ test_cmp expect.2 actual.2
+'
+
+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 'absorbing the git dir fails for incomplete submodules' '
+ git status >expect.1 &&
+ git -C sub2 rev-parse HEAD >expect.2 &&
+ test_must_fail git submodule absorbgitdirs &&
+ git -C sub2 fsck &&
+ test -d sub2/.git &&
+ git status >actual &&
+ git -C sub2 rev-parse HEAD >actual.2 &&
+ test_cmp expect.1 actual.1 &&
+ test_cmp expect.2 actual.2
+'
+
+test_expect_success 'setup a submodule with multiple worktrees' '
+ # first create another unembedded git dir in a new submodule
+ git init sub3 &&
+ test_commit -C sub3 first &&
+ git submodule add ./sub3 &&
+ test_tick &&
+ git commit -m "add another submodule" &&
+ git -C sub3 worktree add ../sub3_second_work_tree
+'
+
+test_expect_success 'absorb a submodule with multiple worktrees' '
+ test_must_fail git submodule absorbgitdirs sub3 2>error &&
+ test_i18ngrep "not supported" error
+'
+
+test_done
--
2.11.0.rc2.30.gc512cbd.dirty
^ permalink raw reply related
* [PATCHv6 4/7] worktree: get worktrees from submodules
From: Stefan Beller @ 2016-12-08 1:46 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161208014623.7588-1-sbeller@google.com>
In a later patch we want to move around the the git directory of
a submodule. Both submodules as well as worktrees are involved in
placing git directories at unusual places, so their functionality
may collide. To react appropriately to situations where worktrees
in submodules are in use, offer a new function to query the
worktrees for submodules.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
worktree.c | 46 ++++++++++++++++++++++++++++++++++++----------
worktree.h | 6 ++++++
2 files changed, 42 insertions(+), 10 deletions(-)
diff --git a/worktree.c b/worktree.c
index eb6121263b..75db689672 100644
--- a/worktree.c
+++ b/worktree.c
@@ -72,7 +72,7 @@ static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)
/**
* get the main worktree
*/
-static struct worktree *get_main_worktree(void)
+static struct worktree *get_main_worktree(const char *git_common_dir)
{
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ -81,12 +81,12 @@ static struct worktree *get_main_worktree(void)
int is_bare = 0;
int is_detached = 0;
- strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
+ strbuf_add_absolute_path(&worktree_path, git_common_dir);
is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
if (is_bare)
strbuf_strip_suffix(&worktree_path, "/.");
- strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
+ strbuf_addf(&path, "%s/HEAD", git_common_dir);
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
@@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
return worktree;
}
-static struct worktree *get_linked_worktree(const char *id)
+static struct worktree *get_linked_worktree(const char *git_common_dir,
+ const char *id)
{
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ -112,7 +113,7 @@ static struct worktree *get_linked_worktree(const char *id)
if (!id)
die("Missing linked worktree name");
- strbuf_git_common_path(&path, "worktrees/%s/gitdir", id);
+ strbuf_addf(&path, "%s/worktrees/%s/gitdir", git_common_dir, id);
if (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)
/* invalid gitdir file */
goto done;
@@ -125,7 +126,7 @@ static struct worktree *get_linked_worktree(const char *id)
}
strbuf_reset(&path);
- strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+ strbuf_addf(&path, "%s/worktrees/%s/HEAD", git_common_dir, id);
if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
goto done;
@@ -167,7 +168,8 @@ static int compare_worktree(const void *a_, const void *b_)
return fspathcmp((*a)->path, (*b)->path);
}
-struct worktree **get_worktrees(unsigned flags)
+static struct worktree **get_worktrees_internal(const char *git_common_dir,
+ unsigned flags)
{
struct worktree **list = NULL;
struct strbuf path = STRBUF_INIT;
@@ -177,9 +179,9 @@ struct worktree **get_worktrees(unsigned flags)
list = xmalloc(alloc * sizeof(struct worktree *));
- list[counter++] = get_main_worktree();
+ list[counter++] = get_main_worktree(git_common_dir);
- strbuf_addf(&path, "%s/worktrees", get_git_common_dir());
+ strbuf_addf(&path, "%s/worktrees", git_common_dir);
dir = opendir(path.buf);
strbuf_release(&path);
if (dir) {
@@ -188,7 +190,7 @@ struct worktree **get_worktrees(unsigned flags)
if (is_dot_or_dotdot(d->d_name))
continue;
- if ((linked = get_linked_worktree(d->d_name))) {
+ if ((linked = get_linked_worktree(git_common_dir, d->d_name))) {
ALLOC_GROW(list, counter + 1, alloc);
list[counter++] = linked;
}
@@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)
return list;
}
+struct worktree **get_worktrees(unsigned flags)
+{
+ return get_worktrees_internal(get_git_common_dir(), flags);
+}
+
+struct worktree **get_submodule_worktrees(const char *path, unsigned flags)
+{
+ char *submodule_gitdir;
+ struct strbuf sb = STRBUF_INIT;
+ struct worktree **ret;
+
+ submodule_gitdir = git_pathdup_submodule(path, "%s", "");
+ if (!submodule_gitdir)
+ return NULL;
+
+ /* the env would be set for the superproject */
+ get_common_dir_noenv(&sb, submodule_gitdir);
+ ret = get_worktrees_internal(sb.buf, flags);
+
+ strbuf_release(&sb);
+ free(submodule_gitdir);
+ return ret;
+}
+
const char *get_worktree_git_dir(const struct worktree *wt)
{
if (!wt)
diff --git a/worktree.h b/worktree.h
index d59ce1fee8..157fbc4a66 100644
--- a/worktree.h
+++ b/worktree.h
@@ -27,6 +27,12 @@ struct worktree {
*/
extern struct worktree **get_worktrees(unsigned flags);
+/*
+ * Get the worktrees of a submodule named by `path`.
+ */
+extern struct worktree **get_submodule_worktrees(const char *path,
+ unsigned flags);
+
/*
* Return git dir of the worktree. Note that the path may be relative.
* If wt is NULL, git dir of current worktree is returned.
--
2.11.0.rc2.30.gc512cbd.dirty
^ permalink raw reply related
* [PATCHv6 2/7] submodule helper: support super prefix
From: Stefan Beller @ 2016-12-08 1:46 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161208014623.7588-1-sbeller@google.com>
Just like main commands in Git, the submodule helper needs
access to the superproject prefix. Enable this in the git.c
but have its own fuse in the helper code by having a flag to
turn on the super prefix.
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/submodule--helper.c | 31 ++++++++++++++++++++-----------
git.c | 2 +-
2 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5f9f..33676a57cf 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,21 +1076,24 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
return 0;
}
+#define SUPPORT_SUPER_PREFIX (1<<0)
+
struct cmd_struct {
const char *cmd;
int (*fn)(int, const char **, const char *);
+ int option;
};
static struct cmd_struct commands[] = {
- {"list", module_list},
- {"name", module_name},
- {"clone", module_clone},
- {"update-clone", update_clone},
- {"relative-path", resolve_relative_path},
- {"resolve-relative-url", resolve_relative_url},
- {"resolve-relative-url-test", resolve_relative_url_test},
- {"init", module_init},
- {"remote-branch", resolve_remote_submodule_branch}
+ {"list", module_list, 0},
+ {"name", module_name, 0},
+ {"clone", module_clone, 0},
+ {"update-clone", update_clone, 0},
+ {"relative-path", resolve_relative_path, 0},
+ {"resolve-relative-url", resolve_relative_url, 0},
+ {"resolve-relative-url-test", resolve_relative_url_test, 0},
+ {"init", module_init, 0},
+ {"remote-branch", resolve_remote_submodule_branch, 0},
};
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
@@ -1100,9 +1103,15 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
die(_("submodule--helper subcommand must be "
"called with a subcommand"));
- for (i = 0; i < ARRAY_SIZE(commands); i++)
- if (!strcmp(argv[1], commands[i].cmd))
+ for (i = 0; i < ARRAY_SIZE(commands); i++) {
+ if (!strcmp(argv[1], commands[i].cmd)) {
+ if (get_super_prefix() &&
+ !(commands[i].option & SUPPORT_SUPER_PREFIX))
+ die("%s doesn't support --super-prefix",
+ commands[i].cmd);
return commands[i].fn(argc - 1, argv + 1, prefix);
+ }
+ }
die(_("'%s' is not a valid submodule--helper "
"subcommand"), argv[1]);
diff --git a/git.c b/git.c
index efa1059fe0..98dcf6c518 100644
--- a/git.c
+++ b/git.c
@@ -493,7 +493,7 @@ static struct cmd_struct commands[] = {
{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
{ "stripspace", cmd_stripspace },
- { "submodule--helper", cmd_submodule__helper, RUN_SETUP },
+ { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},
{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
{ "tag", cmd_tag, RUN_SETUP },
{ "unpack-file", cmd_unpack_file, RUN_SETUP },
--
2.11.0.rc2.30.gc512cbd.dirty
^ permalink raw reply related
* [PATCHv6 1/7] submodule: use absolute path for computing relative path connecting
From: Stefan Beller @ 2016-12-08 1:46 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161208014623.7588-1-sbeller@google.com>
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 much harder
to misuse.
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.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.30.gc512cbd.dirty
^ permalink raw reply related
* [PATCHv6 0/7] submodule embedgitdirs
From: Stefan Beller @ 2016-12-08 1:46 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
v6:
* renamed embedgitdirs to absorbgitdirs embedding may be interpreted as
embedding the git dir into the working directory, whereas absorbing sounds
more like the submodule is absorbed by the superproject, making the
submodule less independent
* Worktrees API offer uses_worktrees(void) and submodule_uses_worktree(path).
* moved the printing to stderr and one layer up (out of the pure
relocate_git_dir function).
* connect_... is in dir.h now.
v5:
* Add another layer of abstraction, i.e. the relocate_git_dir is only about
moving a git dir of one repository. The submodule specific stuff (e.g.
recursion into nested submodules) is in submodule.{c,h}
This was motivated by reviews on the series of checkout aware of submodules
building on top of this series, as we want to directly call the embed-git-dirs
function without the overhead of spawning a child process.
v4:
* rebuilt on top of nd/worktree-list-fixup
* fix and test behavior for un-init submodules (don't crash, rather do nothing)
* incorporated a "static" as pointed out by Ramsay
* use internal functions instead of duplicating code in worktree.c
(use get_common_dir_noenv for the submodule to actually get the common dir)
* fixed a memory leak in relocate_gitdir
v3:
* have a slightly more generic function "relocate_gitdir".
The recursion is strictly related to submodules, though.
* bail out if a submodule is using worktrees.
This also lays the groundwork for later doing the proper thing,
as worktree.h offers a function `get_submodule_worktrees(path)`
* nit by duy: use git_path instead of git_common_dir
v2:
* fixed commit message for patch:
"submodule: use absolute path for computing relative path connecting"
* a new patch "submodule helper: support super prefix"
* redid the final patch with more tests and fixing bugs along the way
* "test-lib-functions.sh: teach test_commit -C <dir>" unchanged
v1:
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 (7):
submodule: use absolute path for computing relative path connecting
submodule helper: support super prefix
test-lib-functions.sh: teach test_commit -C <dir>
worktree: get worktrees from submodules
worktree: add function to check if worktrees are in use
move connect_work_tree_and_git_dir to dir.h
submodule: add absorb-git-dir function
Documentation/git-submodule.txt | 15 +++++
builtin/submodule--helper.c | 69 ++++++++++++++++----
dir.c | 38 +++++++++++
dir.h | 4 ++
git-submodule.sh | 7 +-
git.c | 2 +-
submodule.c | 127 ++++++++++++++++++++++++++++++-------
submodule.h | 5 +-
t/t7412-submodule-absorbgitdirs.sh | 101 +++++++++++++++++++++++++++++
t/test-lib-functions.sh | 20 ++++--
worktree.c | 70 +++++++++++++++++---
worktree.h | 13 ++++
12 files changed, 418 insertions(+), 53 deletions(-)
create mode 100755 t/t7412-submodule-absorbgitdirs.sh
--
2.11.0.rc2.30.gc512cbd.dirty
^ permalink raw reply
* [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: vi0oss @ 2016-12-08 1:38 UTC (permalink / raw)
To: git; +Cc: stefanbeller, Vitaly "_Vi" Shukela
From: "Vitaly \"_Vi\" Shukela" <vi0oss@gmail.com>
In 31224cbdc7 (clone: recursive and reference option triggers
submodule alternates, 2016-08-17) a mechanism was added to
have submodules referenced. It did not address _nested_
submodules, however.
This patch makes all not just the root repository, but also
all submodules (recursively) have submodule.alternateLocation
and submodule.alternateErrorStrategy configured, making Git
search for possible alternates for nested submodules as well.
As submodule's alternate target does not end in .git/objects
(rather .git/modules/qqqqqq/objects), this alternate target
path restriction for in add_possible_reference_from_superproject
relates from "*.git/objects" to just */objects".
New tests have been added to t7408-submodule-reference.
Signed-off-by: Vitaly _Vi Shukela <vi0oss@gmail.com>
---
Notes:
Third review: missing && in test fixed.
Mailmap change not included.
builtin/submodule--helper.c | 19 ++++++++++--
t/t7408-submodule-reference.sh | 66 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..92fd676 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(
/*
* If the alternate object store is another repository, try the
- * standard layout with .git/modules/<name>/objects
+ * standard layout with .git/(modules/<name>)+/objects
*/
- if (ends_with(alt->path, ".git/objects")) {
+ if (ends_with(alt->path, "/objects")) {
char *sm_alternate;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
@@ -583,6 +583,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
struct string_list reference = STRING_LIST_INIT_NODUP;
+ char *sm_alternate = NULL, *error_strategy = NULL;
struct option module_clone_options[] = {
OPT_STRING(0, "prefix", &prefix,
@@ -672,6 +673,20 @@ static int module_clone(int argc, const char **argv, const char *prefix)
die(_("could not get submodule directory for '%s'"), path);
git_config_set_in_file(p, "core.worktree",
relative_path(path, sm_gitdir, &rel_path));
+
+ /* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
+ git_config_get_string("submodule.alternateLocation", &sm_alternate);
+ if (sm_alternate)
+ git_config_set_in_file(p, "submodule.alternateLocation",
+ sm_alternate);
+ git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
+ if (error_strategy)
+ git_config_set_in_file(p, "submodule.alternateErrorStrategy",
+ error_strategy);
+
+ free(sm_alternate);
+ free(error_strategy);
+
strbuf_release(&sb);
strbuf_release(&rel_path);
free(sm_gitdir);
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 1c1e289..e159fc5 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -125,4 +125,70 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm
)
'
+test_expect_success 'preparing second superproject with a nested submodule plus partial clone' '
+ test_create_repo supersuper &&
+ (
+ cd supersuper &&
+ echo "I am super super." >file &&
+ git add file &&
+ git commit -m B-super-super-initial
+ git submodule add "file://$base_dir/super" subwithsub &&
+ git commit -m B-super-super-added &&
+ git submodule update --init --recursive &&
+ git repack -ad
+ ) &&
+ git clone supersuper supersuper2 &&
+ (
+ cd supersuper2 &&
+ git submodule update --init
+ )
+'
+
+# At this point there are three root-level positories: A, B, super and super2
+
+test_expect_success 'nested submodule alternate in works and is actually used' '
+ test_when_finished "rm -rf supersuper-clone" &&
+ git clone --recursive --reference supersuper supersuper supersuper-clone &&
+ (
+ cd supersuper-clone &&
+ # test superproject has alternates setup correctly
+ test_alternate_is_used .git/objects/info/alternates . &&
+ # immediate submodule has alternate:
+ test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub &&
+ # nested submodule also has alternate:
+ test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
+ )
+'
+
+check_that_two_of_three_alternates_are_used() {
+ test_alternate_is_used .git/objects/info/alternates . &&
+ # immediate submodule has alternate:
+ test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub &&
+ # but nested submodule has no alternate:
+ test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
+}
+
+
+test_expect_success 'missing nested submodule alternate fails clone and submodule update' '
+ test_when_finished "rm -rf supersuper-clone" &&
+ test_must_fail git clone --recursive --reference supersuper2 supersuper2 supersuper-clone &&
+ (
+ cd supersuper-clone &&
+ check_that_two_of_three_alternates_are_used &&
+ # update of the submodule fails
+ test_must_fail git submodule update --init --recursive
+ )
+'
+
+test_expect_success 'missing nested submodule alternate in --reference-if-able mode' '
+ test_when_finished "rm -rf supersuper-clone" &&
+ git clone --recursive --reference-if-able supersuper2 supersuper2 supersuper-clone &&
+ (
+ cd supersuper-clone &&
+ check_that_two_of_three_alternates_are_used &&
+ # update of the submodule succeeds
+ git submodule update --init --recursive
+ )
+'
+
test_done
--
2.10.2
^ permalink raw reply related
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Stefan Beller @ 2016-12-08 1:22 UTC (permalink / raw)
To: vi0oss, Jeff King; +Cc: git@vger.kernel.org, Stefan Beller
In-Reply-To: <20161208003940.28794-1-vi0oss@gmail.com>
On Wed, Dec 7, 2016 at 4:39 PM, <vi0oss@gmail.com> wrote:
>
> Previously test contained errorneous
> test_must_fail, which was masked by
> missing &&.
I wonder if we could make either
the test_must_fail intelligent to detect such a broken && call chain
or the test_expect_success macro to see for those broken chains.
Patch looks good to me except one very minor nit. :)
> +test_expect_success 'nested submodule alternate in works and is actually used' '
> + test_when_finished "rm -rf supersuper-clone" &&
> + git clone --recursive --reference supersuper supersuper supersuper-clone &&
> + (
> + cd supersuper-clone &&
> + # test superproject has alternates setup correctly
> + test_alternate_is_used .git/objects/info/alternates . &&
> + # immediate submodule has alternate:
> + test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
here is a && missing ;)
^ permalink raw reply
* Re: [PATCHv5 4/5] worktree: get worktrees from submodules
From: Brandon Williams @ 2016-12-08 1:14 UTC (permalink / raw)
To: Stefan Beller; +Cc: Junio C Hamano, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <CAGZ79kZiS9dx6QUOcFYh8sSNoVsrv2eNLXJd6X54UekzUiC8VQ@mail.gmail.com>
On 12/07, Stefan Beller wrote:
> On Wed, Dec 7, 2016 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Stefan Beller <sbeller@google.com> writes:
> >
> >> + submodule_common_dir = strbuf_detach(&sb, NULL);
> >> + ret = get_worktrees_internal(submodule_common_dir, flags);
> >> +
> >> + free(submodule_gitdir);
> >
> > This sequence felt somewhat unusual. I would have written this
> > without an extra variable, i.e.
> >
> > ret = get_worktrees_internal(sb.buf, flags);
> > strbuf_release(&sb);
>
> Yours is cleaner; I don't remember what I was thinking.
>
> Feel free to squash it in; in case a resend is needed I will do that.
Just make sure to leave that free in as it refers to another variable
(submodule_gitdir). It actually turns out there is a memory leak in the
original code because submodule_common_dir is never freed after being
detached from the strbuf.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 2/2] describe: add support for multiple match patterns
From: Jacob Keller @ 2016-12-08 1:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <xmqq60mvuv8v.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 7, 2016 at 4:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> Basically, this started as a script to try each pattern in sequence,
>> but this is slow, cumbersome and easy to mess up.
>>
>> You're suggesting just add a single second pattern that we will do
>> matches and discard any tag that matches that first?
>
> I am not suggesting anything. I was just trying to see how well what
> was designed and implemented supports the use case that motivated
> the feature. Think of it as a sanity check and review of the design.
>
Makes sense.
>> I think I can implement that pretty easily, and it should have simpler
>> semantics. We can discard first, and then match what remains easily.
>
> I actually think "multiple" and "negative" are orthogonal and both
> are good things. If we are enhancing the filtering by refname
> patterns to allow multiple patterns (i.e. your patch), that is good,
> and it would be ideal if we can also have support for negative ones.
I can add support for negative matches pretty easily. I personally
don't see the value of "logical and" filters, ie to match only tags
that match all the given filters, though that does allow some other
forms of expression.
I do like the idea of negative filters, and I'll go ahead and work on
adding that as another extension.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCHv5 5/5] submodule: add embed-git-dir function
From: Brandon Williams @ 2016-12-08 1:09 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, pclouds, gitster
In-Reply-To: <20161207210157.18932-6-sbeller@google.com>
On 12/07, Stefan Beller wrote:
> + argv_array_pushl(&cp.args, "--super-prefix", sb.buf,
> + "submodule--helper",
> + "embed-git-dirs", NULL);
check the spacing on these lines, looks like there is an extra space
before "submodule--helper"
--
Brandon Williams
^ permalink raw reply
* [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: vi0oss @ 2016-12-08 0:39 UTC (permalink / raw)
To: git; +Cc: stefanbeller, Vitaly "_Vi" Shukela
From: "Vitaly \"_Vi\" Shukela" <vi0oss@gmail.com>
In 31224cbdc7 (clone: recursive and reference option triggers
submodule alternates, 2016-08-17) a mechanism was added to
have submodules referenced. It did not address _nested_
submodules, however.
This patch makes all not just the root repository, but also
all submodules (recursively) have submodule.alternateLocation
and submodule.alternateErrorStrategy configured, making Git
search for possible alternates for nested submodules as well.
As submodule's alternate target does not end in .git/objects
(rather .git/modules/qqqqqq/objects), this alternate target
path restriction for in add_possible_reference_from_superproject
relates from "*.git/objects" to just */objects".
New tests have been added to t7408-submodule-reference.
Signed-off-by: Vitaly _Vi Shukela <vi0oss@gmail.com>
---
Notes:
Another round of fixups:
* Corrected code style
* Refactored and fixed the test
Previously test contained errorneous
test_must_fail, which was masked by
missing &&.
Mailmap patch omitted this time.
builtin/submodule--helper.c | 19 ++++++++++--
t/t7408-submodule-reference.sh | 66 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 83 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..92fd676 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(
/*
* If the alternate object store is another repository, try the
- * standard layout with .git/modules/<name>/objects
+ * standard layout with .git/(modules/<name>)+/objects
*/
- if (ends_with(alt->path, ".git/objects")) {
+ if (ends_with(alt->path, "/objects")) {
char *sm_alternate;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
@@ -583,6 +583,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
struct string_list reference = STRING_LIST_INIT_NODUP;
+ char *sm_alternate = NULL, *error_strategy = NULL;
struct option module_clone_options[] = {
OPT_STRING(0, "prefix", &prefix,
@@ -672,6 +673,20 @@ static int module_clone(int argc, const char **argv, const char *prefix)
die(_("could not get submodule directory for '%s'"), path);
git_config_set_in_file(p, "core.worktree",
relative_path(path, sm_gitdir, &rel_path));
+
+ /* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
+ git_config_get_string("submodule.alternateLocation", &sm_alternate);
+ if (sm_alternate)
+ git_config_set_in_file(p, "submodule.alternateLocation",
+ sm_alternate);
+ git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
+ if (error_strategy)
+ git_config_set_in_file(p, "submodule.alternateErrorStrategy",
+ error_strategy);
+
+ free(sm_alternate);
+ free(error_strategy);
+
strbuf_release(&sb);
strbuf_release(&rel_path);
free(sm_gitdir);
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 1c1e289..9325297 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -125,4 +125,70 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm
)
'
+test_expect_success 'preparing second superproject with a nested submodule plus partial clone' '
+ test_create_repo supersuper &&
+ (
+ cd supersuper &&
+ echo "I am super super." >file &&
+ git add file &&
+ git commit -m B-super-super-initial
+ git submodule add "file://$base_dir/super" subwithsub &&
+ git commit -m B-super-super-added &&
+ git submodule update --init --recursive &&
+ git repack -ad
+ ) &&
+ git clone supersuper supersuper2 &&
+ (
+ cd supersuper2 &&
+ git submodule update --init
+ )
+'
+
+# At this point there are three root-level positories: A, B, super and super2
+
+test_expect_success 'nested submodule alternate in works and is actually used' '
+ test_when_finished "rm -rf supersuper-clone" &&
+ git clone --recursive --reference supersuper supersuper supersuper-clone &&
+ (
+ cd supersuper-clone &&
+ # test superproject has alternates setup correctly
+ test_alternate_is_used .git/objects/info/alternates . &&
+ # immediate submodule has alternate:
+ test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
+ # nested submodule also has alternate:
+ test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
+ )
+'
+
+check_that_two_of_three_alternates_are_used() {
+ test_alternate_is_used .git/objects/info/alternates . &&
+ # immediate submodule has alternate:
+ test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub &&
+ # but nested submodule has no alternate:
+ test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
+}
+
+
+test_expect_success 'missing nested submodule alternate fails clone and submodule update' '
+ test_when_finished "rm -rf supersuper-clone" &&
+ test_must_fail git clone --recursive --reference supersuper2 supersuper2 supersuper-clone &&
+ (
+ cd supersuper-clone &&
+ check_that_two_of_three_alternates_are_used &&
+ # update of the submodule fails
+ test_must_fail git submodule update --init --recursive
+ )
+'
+
+test_expect_success 'missing nested submodule alternate in --reference-if-able mode' '
+ test_when_finished "rm -rf supersuper-clone" &&
+ git clone --recursive --reference-if-able supersuper2 supersuper2 supersuper-clone &&
+ (
+ cd supersuper-clone &&
+ check_that_two_of_three_alternates_are_used &&
+ # update of the submodule succeeds
+ git submodule update --init --recursive
+ )
+'
+
test_done
--
2.10.2
^ permalink raw reply related
* Re: [PATCH 01/17] mv: convert to using pathspec struct interface
From: Brandon Williams @ 2016-12-08 0:36 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8Bm+Nif_PL1=BzTYLKGrnz94rjKOB=_PXz6OB47Js6v2A@mail.gmail.com>
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> > Convert the 'internal_copy_pathspec()' function to use the pathspec
> > struct interface from using the deprecated 'get_pathspec()' interface.
> >
> > In addition to this, fix a memory leak caused by only duplicating some
> > of the pathspec elements. Instead always duplicate all of the the
> > pathspec elements as an intermediate step (with modificationed based on
> > the passed in flags). This way the intermediate strings can then be
> > freed prior to duplicating the result of parse_pathspec (which contains
> > each of the elements with the prefix prepended).
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> > builtin/mv.c | 45 ++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 32 insertions(+), 13 deletions(-)
>
> Stefan did something similar last year [1] but I couldn't find why it
> did not get merged. Not sure if the reasons are still relevant or
> not...
>
> [1] http://public-inbox.org/git/%3C1438885632-26470-1-git-send-email-sbeller@google.com%3E/
>
> > diff --git a/builtin/mv.c b/builtin/mv.c
> > index 2f43877..4df4a12 100644
> > --- a/builtin/mv.c
> > +++ b/builtin/mv.c
> > @@ -4,6 +4,7 @@
> > * Copyright (C) 2006 Johannes Schindelin
> > */
> > #include "builtin.h"
> > +#include "pathspec.h"
> > #include "lockfile.h"
> > #include "dir.h"
> > #include "cache-tree.h"
> > @@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char *prefix,
> > {
> > int i;
> > const char **result;
> > + struct pathspec ps;
> > ALLOC_ARRAY(result, count + 1);
> > - COPY_ARRAY(result, pathspec, count);
> > - result[count] = NULL;
> > +
> > + /* Create an intermediate copy of the pathspec based on the flags */
> > for (i = 0; i < count; i++) {
> > - int length = strlen(result[i]);
> > + int length = strlen(pathspec[i]);
> > int to_copy = length;
> > + char *it;
> > while (!(flags & KEEP_TRAILING_SLASH) &&
> > - to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
> > + to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
> > to_copy--;
> > - if (to_copy != length || flags & DUP_BASENAME) {
> > - char *it = xmemdupz(result[i], to_copy);
> > - if (flags & DUP_BASENAME) {
> > - result[i] = xstrdup(basename(it));
> > - free(it);
> > - } else
> > - result[i] = it;
> > - }
> > +
> > + it = xmemdupz(pathspec[i], to_copy);
> > + if (flags & DUP_BASENAME) {
> > + result[i] = xstrdup(basename(it));
> > + free(it);
> > + } else
> > + result[i] = it;
> > + }
> > + result[count] = NULL;
> > +
> > + parse_pathspec(&ps,
> > + PATHSPEC_ALL_MAGIC &
> > + ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
> > + PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
> > + prefix, result);
> > + assert(count == ps.nr);
> > +
> > + /* Copy the pathspec and free the old intermediate strings */
> > + for (i = 0; i < count; i++) {
> > + const char *match = xstrdup(ps.items[i].match);
> > + free((char *) result[i]);
> > + result[i] = match;
>
> Sigh.. it looks so weird that we do all the parsing (in a _copy_
> pathspec function) then remove struct pathspec and return the plain
> string. I guess we can't do anything more until we rework cmd_mv code
> to handle pathspec natively.
>
> At the least I think we should rename this function to something else.
> But if you have time I really wish we could kill this function. I
> haven't stared at cmd_mv() long and hard, but it looks to me that we
> combining two separate functionalities in the same function here.
>
> If "mv" takes n arguments, then the first <n-1> arguments may be
> pathspec, the last one is always a plain path. The "dest_path =
> internal_copy_pathspec..." could be as simple as "dest_path =
> prefix_path(argv[argc - 1])". the special treatment for this last
> argument [1] can live here. Then, we can do parse_pathspec for the
> <n-1> arguments in cmd_mv(). It's still far from perfect, because
> cmd_mv can't handle pathspec properly, but it reduces the messy mess
> in internal_copy_pathspec a bit, I hope.
>
> [1] c57f628 (mv: let 'git mv file no-such-dir/' error out - 2013-12-03)
>
> > }
> > - return get_pathspec(prefix, result);
> > +
> > + clear_pathspec(&ps);
> > + return result;
> > }
> >
> > static const char *add_slash(const char *path)
> > --
> > 2.8.0.rc3.226.g39d4020
> >
>
>
>
> --
> Duy
Actually, after looking at this a bit more it seems like we could
technically use prefix_path for both source and dest (based on how the
current code is structured) since the source's provied must all exist (as
in no wildcards are allowed). We could drop using the pathspec struct
completely in addition to renaming the function (to what I'm still
unsure). I agree that this code should probably be rewritten and made a
bit cleaner, I don't know if that fits in the scope of this series or
should be done as a followup patch. If you think it fits here then I
can try and find some time to do the rework.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 2/2] describe: add support for multiple match patterns
From: Junio C Hamano @ 2016-12-08 0:20 UTC (permalink / raw)
To: Jacob Keller; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <CA+P7+xrPivwMzGhzKxu30jns+YvSQGXBKUc4JDmfbenTy27tZg@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> Basically, this started as a script to try each pattern in sequence,
> but this is slow, cumbersome and easy to mess up.
>
> You're suggesting just add a single second pattern that we will do
> matches and discard any tag that matches that first?
I am not suggesting anything. I was just trying to see how well what
was designed and implemented supports the use case that motivated
the feature. Think of it as a sanity check and review of the design.
> I think I can implement that pretty easily, and it should have simpler
> semantics. We can discard first, and then match what remains easily.
I actually think "multiple" and "negative" are orthogonal and both
are good things. If we are enhancing the filtering by refname
patterns to allow multiple patterns (i.e. your patch), that is good,
and it would be ideal if we can also have support for negative ones.
^ permalink raw reply
* Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface
From: Brandon Williams @ 2016-12-08 0:03 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8AX09pxkyUkLU905v1MpXocLzV5bK0APuNmMUNb50Lavg@mail.gmail.com>
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> > Convert 'create_simplify()' to use the pathspec struct interface from
> > using the '_raw' entry in the pathspec.
>
> It would be even better to kill this create_simplify() and let
> simplify_away() handle struct pathspec directly.
>
> There is a bug in this code, that might have been found if we
> simpify_away() handled pathspec directly: the memcmp() in
> simplify_away() will not play well with :(icase) magic. My bad. If
> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> maybe we can teach simplify_away() to do strncasecmp instead. We could
> ignore exclude patterns there too (although not excluding is not a
> bug).
So are you implying that the simplify struct needs to be killed? That
way the pathspec struct itself is being passed around instead?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH 1/2] submodule--helper: set alternateLocation for cloned submodules
From: Stefan Beller @ 2016-12-08 0:02 UTC (permalink / raw)
To: vi0oss; +Cc: git@vger.kernel.org, Stefan Beller
In-Reply-To: <20161207224948.7957-1-vi0oss@gmail.com>
On Wed, Dec 7, 2016 at 2:49 PM, <vi0oss@gmail.com> wrote:
> Notes:
> Resolved issues pointed by Stefan Beller except of
> the one about loosened path check, which he aggreed
> to be relaxed for this case.
I am sorry to have given an incomplete review at the first time. :/
More below.
> + /* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
> + git_config_get_string("submodule.alternateLocation", &sm_alternate);
> + if (sm_alternate) {
> + git_config_set_in_file(p, "submodule.alternateLocation",
> + sm_alternate);
> + }
For a single command, we usually omit the braces for
an if clause, i.e.
if (foo)
bar(...);
/* code goes on */
> + git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
> + if (error_strategy) {
> + git_config_set_in_file(p, "submodule.alternateErrorStrategy",
> + error_strategy);
> + }
> +
> + free(sm_alternate);
> + free(error_strategy);
> +
The indentation seems a bit off for the free here?
(The main nit that motivated me writing the email)
> strbuf_release(&sb);
> strbuf_release(&rel_path);
> free(sm_gitdir);
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index 1c1e289..ef7771b 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -125,4 +125,76 @@ test_expect_success 'ignoring missing submodule alternates passes clone and subm
> )
> '
>
> +test_expect_success 'preparing second superproject with a nested submodule' '
> + test_create_repo supersuper &&
> + (
> + cd supersuper &&
> + echo "I am super super." >file &&
> + git add file &&
> + git commit -m B-super-super-initial
> + git submodule add "file://$base_dir/super" subwithsub &&
> + git commit -m B-super-super-added &&
> + git submodule update --init --recursive &&
> + git repack -ad
> + )
> +'
> +
> +# At this point there are three root-level positories: A, B, super and super2
> +
> +test_expect_success 'nested submodule alternate in works and is actually used' '
> + test_when_finished "rm -rf supersuper-clone" &&
> + git clone --recursive --reference supersuper supersuper supersuper-clone &&
> + (
> + cd supersuper-clone &&
> + # test superproject has alternates setup correctly
> + test_alternate_is_used .git/objects/info/alternates . &&
> + # immediate submodule has alternate:
> + test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
> + # nested submodule also has alternate:
> + test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
> + )
> +'
> +
> +test_expect_success 'missing nested submodule alternate fails clone and submodule update' '
> + test_when_finished "rm -rf supersuper-clone supersuper2" &&
> + git clone supersuper supersuper2 &&
> + (
> + cd supersuper2 &&
> + git submodule update --init
> + ) &&
> + test_must_fail git clone --recursive --reference supersuper2 supersuper2 supersuper-clone &&
> + (
> + cd supersuper-clone &&
> + # test superproject has alternates setup correctly
> + test_alternate_is_used .git/objects/info/alternates . &&
> + # update of the submodule fails
> + test_must_fail git submodule update --init --recursive &&
> + # immediate submodule has alternate:
> + test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
> + # but nested submodule has no alternate:
> + test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
> + )
> +'
> +
> +test_expect_success 'missing nested submodule alternate in --reference-if-able mode' '
> + test_when_finished "rm -rf supersuper-clone supersuper2" &&
> + git clone supersuper supersuper2 &&
> + (
> + cd supersuper2 &&
> + git submodule update --init
> + ) &&
> + git clone --recursive --reference-if-able supersuper2 supersuper2 supersuper-clone &&
> + (
> + cd supersuper-clone &&
> + # test superproject has alternates setup correctly
> + test_alternate_is_used .git/objects/info/alternates . &&
> + # update of the submodule fails
> + test_must_fail git submodule update --init --recursive &&
> + # immediate submodule has alternate:
> + test_alternate_is_used .git/modules/subwithsub/objects/info/alternates subwithsub
> + # but nested submodule has no alternate:
> + test_must_fail test_alternate_is_used .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
> + )
Both the first and the last part are the same in the last two tests,
the only difference is the line with git clone --reference ...
Maybe we can use a function somehow to make this a bit more
obvious?
Otherwise the tests look good to me.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options
From: Jacob Keller @ 2016-12-08 0:01 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list, Junio C Hamano, Jakub Narębski
In-Reply-To: <20161207153627.1468-1-Karthik.188@gmail.com>
On Wed, Dec 7, 2016 at 7:36 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> This is part of unification of the commands 'git tag -l, git branch -l
> and git for-each-ref'. This ports over branch.c to use ref-filter's
> printing options.
>
> Initially posted here: $(gmane/279226). It was decided that this series
> would follow up after refactoring ref-filter parsing mechanism, which
> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>
> v1 can be found here: $(gmane/288342)
> v2 can be found here: $(gmane/288863)
> v3 can be found here: $(gmane/290299)
> v4 can be found here: $(gmane/291106)
> v5b can be found here: $(gmane/292467)
> v6 can be found here: http://marc.info/?l=git&m=146330914118766&w=2
> v7 can be found here: http://marc.info/?l=git&m=147863593317362&w=2
>
> Changes in this version:
>
> 1. use an enum for holding the comparision type in
> %(if:[equals/notequals=...]) options.
> 2. rename the 'strip' option to 'lstrip' and introduce an 'rstrip'
> option. Also modify them to take negative values. This drops the
> ':dri' and ':base' options.
> 3. Drop unecessary code.
> 4. Cleanup code and fix spacing.
> 5. Add more comments wherever required.
> 6. Add quote_literal_for_format(const char *s) for safer string
> insertions in branch.c:build_format().
>
> Thanks to Jacob, Jackub, Junio and Matthieu for their inputs on the
> previous version.
>
> Interdiff below.
>
> Karthik Nayak (19):
> ref-filter: implement %(if), %(then), and %(else) atoms
> ref-filter: include reference to 'used_atom' within 'atom_value'
> ref-filter: implement %(if:equals=<string>) and
> %(if:notequals=<string>)
> ref-filter: modify "%(objectname:short)" to take length
> ref-filter: move get_head_description() from branch.c
> ref-filter: introduce format_ref_array_item()
> ref-filter: make %(upstream:track) prints "[gone]" for invalid
> upstreams
> ref-filter: add support for %(upstream:track,nobracket)
> ref-filter: make "%(symref)" atom work with the ':short' modifier
> ref-filter: introduce refname_atom_parser_internal()
> ref-filter: introduce refname_atom_parser()
> ref-filter: make remote_ref_atom_parser() use
> refname_atom_parser_internal()
> ref-filter: rename the 'strip' option to 'lstrip'
> ref-filter: modify the 'lstrip=<N>' option to work with negative '<N>'
> ref-filter: add an 'rstrip=<N>' option to atoms which deal with
> refnames
> ref-filter: allow porcelain to translate messages in the output
> branch, tag: use porcelain output
> branch: use ref-filter printing APIs
> branch: implement '--format' option
>
> Documentation/git-branch.txt | 7 +-
> Documentation/git-for-each-ref.txt | 86 +++++--
> builtin/branch.c | 290 +++++++---------------
> builtin/tag.c | 6 +-
> ref-filter.c | 488 +++++++++++++++++++++++++++++++------
> ref-filter.h | 7 +
> t/t3203-branch-output.sh | 16 +-
> t/t6040-tracking-info.sh | 2 +-
> t/t6300-for-each-ref.sh | 88 ++++++-
> t/t6302-for-each-ref-filter.sh | 94 +++++++
> 10 files changed, 784 insertions(+), 300 deletions(-)
>
> Interdiff:
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index f4ad297..c72baeb 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -92,13 +92,14 @@ refname::
> The name of the ref (the part after $GIT_DIR/).
> For a non-ambiguous short name of the ref append `:short`.
> The option core.warnAmbiguousRefs is used to select the strict
> - abbreviation mode. If `strip=<N>` is appended, strips `<N>`
> - slash-separated path components from the front of the refname
> - (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
> - `<N>` must be a positive integer. If a displayed ref has fewer
> - components than `<N>`, the command aborts with an error. For the base
> - directory of the ref (i.e. foo in refs/foo/bar/boz) append
> - `:base`. For the entire directory path append `:dir`.
> + abbreviation mode. If `lstrip=<N>` or `rstrip=<N>` option can
Grammar here, drop the If before `lstrip since you're referring to
multiples and you say "x can be appended to y" rather than "if x is
added, do y"
> + be appended to strip `<N>` slash-separated path components
> + from or end of the refname respectively (e.g.,
> + `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo` and
> + `%(refname:rstrip=2)` turns `refs/tags/foo` into `refs`). if
> + `<N>` is a negative number, then only `<N>` path components
> + are left behind. If a displayed ref has fewer components than
> + `<N>`, the command aborts with an error.
>
Would it make more sense to not die and instead just return the empty
string? On the one hand, if we die() it's obvious that you tried to
strip too many components. But on the other hand, it's also somewhat
annoying to have the whole command fail because we happen upon a
single ref that has fewer components?
So, for positive numbers, we simply strip what we can, which may
result in the empty string, and for negative numbers, we keep up to
what we said, while potentially keeping the entire string. I feel
that's a better alternative than a die() in the middle of a ref
filter..
What are other people's thoughts on this?
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH 2/2] describe: add support for multiple match patterns
From: Jacob Keller @ 2016-12-07 23:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <xmqqa8c7wfxu.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 7, 2016 at 2:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> ... Suppose that you version all
>> your official releases such as "v1.2", "v1.3", "v1.4", "v2.1" and so on.
>> Now, you also have other tags which represent -rc releases and other
>> such tags. If you want to find the first major release that contains
>> a given commit you might try
>>
>> git describe --contains --match="v?.?" <commit>
>>
>> This will work as long as you have only single digits. But if you start
>> adding multiple digits, the pattern becomes not enough to match all the
>> tags you wanted while excluding the ones you didn't.
>
> Isn't what you really want for the use case a negative pattern,
> i.e. "I want ones that match v* but not the ones that match *-rc*",
> though?
That's another way of implementing it. I just went with straight
forward patterns that I was already using in sequence.
Basically, this started as a script to try each pattern in sequence,
but this is slow, cumbersome and easy to mess up.
You're suggesting just add a single second pattern that we will do
matches and discard any tag that matches that first?
I think I can implement that pretty easily, and it should have simpler
semantics. We can discard first, and then match what remains easily.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCHv5 0/5] submodule embedgitdirs
From: Stefan Beller @ 2016-12-07 23:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <xmqqeg1juxd7.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 7, 2016 at 3:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> v5:
>>> * Add another layer of abstraction, i.e. the relocate_git_dir is only about
>>> moving a git dir of one repository. The submodule specific stuff (e.g.
>>> recursion into nested submodules) is in submodule.{c,h}
>>>
>>> This was motivated by reviews on the series of checkout aware of submodules
>>> building on top of this series, as we want to directly call the embed-git-dirs
>>> function without the overhead of spawning a child process.
>>
>> OK. Comparing the last steps between this round and the previous
>> one, I do think the separation of the responsibility among helpers
>> is much more reasonable in this version, where:
>>
>> - submodule_embed_git_dir() is given a single path and is
>> responsible for that submodule itself, which is done by calling
>> submodule_embed_git_dir_for_path() on itself, and its
>> sub-submodules, which is done by spawning the helper recursively
>> with appropriate super-prefix;
>>
>> - submodule_embed_git_dir_for_path() computes where the given path
>> needs to be moved to using the knowledge specific to the
>> submodule subsystem, and asks relocate_gitdir() to perform the
>> actual relocation;
>>
>> - relocate_gitdir() used to do quite a lot more, but now it is only
>> about moving an existing .git directory elsewhere and pointing to
>> the new location with .git file placed in the old location.
>>
>> I would have called the second helper submodule_embed_one_git_dir(),
>> but that is a minor detail.
>>
>> Very nicely done.
>
> One thing that is not so nice from the code organization point of
> view is that "dir.c" now needs to include "submodule.h" because it
> wants to call connect_work_tree_and_git_dir().
>
> I wonder if that function belongs to dir.c or worktree.c not
> submodule.c; I do not see anything that is submodule specific about
> what the function does.
Right, it just happened historically for that function to live
in submodule area. I'll move the connect_work_tree_and_git_dir
to dir.c as well.
^ permalink raw reply
* Re: [PATCH v2 0/6] shallow.c improvements
From: Junio C Hamano @ 2016-12-07 23:42 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jeff King, Git Mailing List, Rasmus Villemoes
In-Reply-To: <CACsJy8A=KeGsXAt6ZR-eOkTurSsnYPkt3yTfkYT9aZ86rV1rYg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Tue, Dec 6, 2016 at 8:42 PM, Jeff King <peff@peff.net> wrote:
>> The final one _seems_ reasonable after reading your explanation, but I
>> lack enough context to know whether or not there might be a corner case
>> that you're missing. I'm inclined to trust your assessment on it.
>
> Yeah I basically just wrote down my thoughts so somebody could maybe
> spot something wrong. I'm going to think about it some more in the
> next few days.
In the meantime let me queue them as-is.
Thanks.
^ permalink raw reply
* Re: [PATCHv5 5/5] submodule: add embed-git-dir function
From: Stefan Beller @ 2016-12-07 23:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org, Duy Nguyen
In-Reply-To: <xmqqlgvruyt2.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 7, 2016 at 3:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> @@ -1093,7 +1129,8 @@ static struct cmd_struct commands[] = {
>> {"resolve-relative-url", resolve_relative_url, 0},
>> {"resolve-relative-url-test", resolve_relative_url_test, 0},
>> {"init", module_init, 0},
>> - {"remote-branch", resolve_remote_submodule_branch, 0}
>> + {"remote-branch", resolve_remote_submodule_branch, 0},
>> + {"embed-git-dirs", embed_git_dir, SUPPORT_SUPER_PREFIX}
>> };
>
> If you want to avoid patch noise like this, your 2/5 can add a
> trailing comma after the entry for remote-branch. It is OK to end
> elements in an array literal with trailing comma, even though we
> avoid doing similar in enum definition (which is only allowed in
> newer C standards).
Ok, thanks for pointing out!
>
>> diff --git a/dir.c b/dir.c
>> index bfa8c8a9a5..e023b04407 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -2748,3 +2748,30 @@ void untracked_cache_add_to_index(struct index_state *istate,
>> {
>> untracked_cache_invalidate_path(istate, path);
>> }
>> +
>> +/*
>> + * Migrate the git directory of the given `path` from `old_git_dir` to
>> + * `new_git_dir`. If an error occurs, append it to `err` and return the
>> + * error code.
>> + */
>> +int relocate_gitdir(const char *path, const char *old_git_dir,
>> + const char *new_git_dir, const char *displaypath,
>> + struct strbuf *err)
>> +{
>> + int ret = 0;
>> +
>> + printf("Migrating git directory of '%s' from\n'%s' to\n'%s'\n",
>> + displaypath, old_git_dir, new_git_dir);
>
> By using "strbuf err", it looks like that the calling convention of
> this function wants to cater to callers who want to have tight
> control over their output and an unconditional printing to the
> standard output looks somewhat out of place.
Before sending it out to the list I had a version with another flag
that controlled whether we die on error or just print a warning.
That was not fully true however as the connect_work_tree_and_git_dir
would die nevertheless, we'd only warn for the failed rename().
It turns out we do not need a fully functional non-die()ing version
for the checkout series, so I ripped that out again and the version sent out
just dies in case of failure in relocate_gitdir.
That said, I think the printing of the migration message ought to go
into the caller and to stderr as you note.
>
> Besides, does it belong to the standard output? It looks more like
> a progress-bar eye candy that we send out to the standard error
> stream (and only when we are talking to a tty).
>
>> +/* Embeds a single submodule, non recursively. */
>> +static void submodule_embed_git_dir_for_path(const char *prefix, const char *path)
>> +{
>> + struct worktree **worktrees;
>> + struct strbuf pathbuf = STRBUF_INIT;
>> + struct strbuf errbuf = STRBUF_INIT;
>> + char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
>> + const char *new_git_dir;
>> + const struct submodule *sub;
>> + int code;
>> +
>> + worktrees = get_submodule_worktrees(path, 0);
>> + if (worktrees) {
>> + int i;
>> + for (i = 0; worktrees[i]; i++)
>> + ;
>> + free_worktrees(worktrees);
>> + if (i > 1)
>> + die(_("relocate_gitdir for submodule '%s' with "
>> + "more than one worktree not supported"), path);
>> + }
>
> We may benefit from "does this have secondary worktrees?" boolean
> helper function, perhaps?
ok, I'll add that helper as its own patch to the worktree code earlier
in the series.
>
>> + old_git_dir = xstrfmt("%s/.git", path);
>> + if (read_gitfile(old_git_dir))
>> + /* If it is an actual gitfile, it doesn't need migration. */
>> + return;
>
> Isn't this "no-op return" a valid thing to do, even when the
> submodule has secondary worktrees that borrow from it?
If we have 2 worktrees, all bets are off (in my non-understanding).
The git file here *could* point to git directory living inside the
other working tree, which then would need migration from that other
working tree to some embedded place.
I don't think that is how worktrees work (or have ever worked?)
but I am unfamiliar with worktrees, so I erred on the safe side.
> I am
> wondering if the "ah, we don't do a repository that has secondary
> worktrees" check should come after this one.
Maybe Duy can answer that? (Where are git directories
located in a worktree setup, even historically? Were they
ever inside one of the submodules? The other working tree may
not be a submodule, but a standalone thing as well?)
>
>> + real_old_git_dir = xstrdup(real_path(old_git_dir));
>> +...
>> +}
>
>> +/*
>> + * Migrate the git directory of the submodule given by path from
>> + * having its git directory within the working tree to the git dir nested
>> + * in its superprojects git dir under modules/.
>> + */
>
> I think that this operation removes the git directories that are
> embedded in the working tree of the superproject and storing them
> away to safer place, i.e. unembedding.
Oh right we had that discussion what the embedding actually means.
I asked around for English language help here:
What about "absorbGitDir" ? (absorb as in eat/consume) for the external
UI and internally I can call it relocate_submodule_git_dir_into_superproject
(or embed_git_dir_into_superproject)
>> +
>> +
>
> Lose the extra blank line here?
ok
>> + if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))
>
> Yeah, checking for NULL-ness with !skip_prefix() helps ;-)
I think you are referring to the interdiff to the previous patches..
Yes we should just use it as it is here.
>
>> + submodule_embed_git_dir_for_path(prefix, path);
>> +
>> + if (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + struct strbuf sb = STRBUF_INIT;
>> +
>> + if (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)
>> + die("BUG: we don't know how to pass the flags down?");
>> +
>> + if (get_super_prefix())
>> + strbuf_addstr(&sb, get_super_prefix());
>> + strbuf_addstr(&sb, path);
>> + strbuf_addch(&sb, '/');
>> +
>> + cp.dir = path;
>> + cp.git_cmd = 1;
>> + cp.no_stdin = 1;
>> + argv_array_pushl(&cp.args, "--super-prefix", sb.buf,
>> + "submodule--helper",
>> + "embed-git-dirs", NULL);
>> + prepare_submodule_repo_env(&cp.env_array);
>> + if (run_command(&cp))
>> + die(_("could not recurse into submodule '%s'"), path);
>> + strbuf_release(&sb);
>> + }
>
> Hmph. We cannot use run_processes_parallel() thing here? Is its
> API too hard to use to be worth it?
The problem here is that we do not know about the names of
the nested submodules. We could do a "git -C path submodule--helper list"
and then use the run_processes_parallel for
git -C <submodule> embed-git-dir <nested-sub1>
git -C <submodule> embed-git-dir <nested-sub2>
git -C <submodule> embed-git-dir <nested-sub3>
etc. As a first approach I considered
git -C <submodule> embed-git-dir <no-pathspec>
to be fast enough as the functionality is not implemented is only
a filesystem-local rename() (fast regardless of directory size).
So if we want to make that relocate git dir aware of
non-atomic-cross-filesystem moves, we want to revisit the decision
to run it in parallel?
>
>> +test_expect_success 'embedding does not fail for deinitalized submodules' '
>> + test_when_finished "git submodule update --init" &&
>> + git submodule deinit --all &&
>> + git submodule embedgitdirs &&
>> + test -d .git/modules/sub1 &&
>> + ! test -f sub1/.git &&
>
> Does this expect "sub1/.git is not a regular file (we want directory
> instead)"? Or "there is no filesystem entity at sub1/.git"?
This mainly ought to test that the new call doesn't crash or segfault
but accepts this as a valid state.
>
> If the former, write "test -d sub1/.git"; if the latter, you
> probably want "! test -e sub1/.git" instead.
However the -e is the correct thing to do.
^ permalink raw reply
* Re: [PATCHv5 0/5] submodule embedgitdirs
From: Junio C Hamano @ 2016-12-07 23:34 UTC (permalink / raw)
To: Stefan Beller; +Cc: bmwill, git, pclouds
In-Reply-To: <xmqqzik7v04b.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Stefan Beller <sbeller@google.com> writes:
>
>> v5:
>> * Add another layer of abstraction, i.e. the relocate_git_dir is only about
>> moving a git dir of one repository. The submodule specific stuff (e.g.
>> recursion into nested submodules) is in submodule.{c,h}
>>
>> This was motivated by reviews on the series of checkout aware of submodules
>> building on top of this series, as we want to directly call the embed-git-dirs
>> function without the overhead of spawning a child process.
>
> OK. Comparing the last steps between this round and the previous
> one, I do think the separation of the responsibility among helpers
> is much more reasonable in this version, where:
>
> - submodule_embed_git_dir() is given a single path and is
> responsible for that submodule itself, which is done by calling
> submodule_embed_git_dir_for_path() on itself, and its
> sub-submodules, which is done by spawning the helper recursively
> with appropriate super-prefix;
>
> - submodule_embed_git_dir_for_path() computes where the given path
> needs to be moved to using the knowledge specific to the
> submodule subsystem, and asks relocate_gitdir() to perform the
> actual relocation;
>
> - relocate_gitdir() used to do quite a lot more, but now it is only
> about moving an existing .git directory elsewhere and pointing to
> the new location with .git file placed in the old location.
>
> I would have called the second helper submodule_embed_one_git_dir(),
> but that is a minor detail.
>
> Very nicely done.
One thing that is not so nice from the code organization point of
view is that "dir.c" now needs to include "submodule.h" because it
wants to call connect_work_tree_and_git_dir().
I wonder if that function belongs to dir.c or worktree.c not
submodule.c; I do not see anything that is submodule specific about
what the function does.
^ permalink raw reply
* Re: [PATCH 16/17] pathspec: small readability changes
From: Brandon Williams @ 2016-12-07 23:27 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List, Stefan Beller, Junio C Hamano
In-Reply-To: <CACsJy8ChJ_H3gDOuKVYGAKYumG0u2WkBVpNr_3ePyAJ9NojvEg@mail.gmail.com>
On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams <bmwill@google.com> wrote:
> > A few small changes to improve readability. This is done by grouping related
> > assignments, adding blank lines, ensuring lines are <80 characters, etc.
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> > pathspec.c | 15 ++++++++++-----
> > 1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/pathspec.c b/pathspec.c
> > index 41aa213..8a07b02 100644
> > --- a/pathspec.c
> > +++ b/pathspec.c
> > @@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> > if ((magic & PATHSPEC_LITERAL) && (magic & PATHSPEC_GLOB))
> > die(_("%s: 'literal' and 'glob' are incompatible"), elt);
> >
> > + /* Create match string which will be used for pathspec matching */
> > if (pathspec_prefix >= 0) {
> > match = xstrdup(copyfrom);
> > prefixlen = pathspec_prefix;
> > @@ -341,11 +342,16 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> > match = xstrdup(copyfrom);
> > prefixlen = 0;
> > } else {
> > - match = prefix_path_gently(prefix, prefixlen, &prefixlen, copyfrom);
> > + match = prefix_path_gently(prefix, prefixlen,
> > + &prefixlen, copyfrom);
> > if (!match)
> > die(_("%s: '%s' is outside repository"), elt, copyfrom);
> > }
> > +
> > item->match = match;
> > + item->len = strlen(item->match);
> > + item->prefix = prefixlen;
> > +
> > /*
> > * Prefix the pathspec (keep all magic) and assign to
> > * original. Useful for passing to another command.
> > @@ -362,8 +368,6 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> > } else {
> > item->original = xstrdup(elt);
> > }
> > - item->len = strlen(item->match);
> > - item->prefix = prefixlen;
> >
> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
> > strip_submodule_slash_cheap(item);
> > @@ -371,13 +375,14 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
> > strip_submodule_slash_expensive(item);
> >
> > - if (magic & PATHSPEC_LITERAL)
> > + if (magic & PATHSPEC_LITERAL) {
> > item->nowildcard_len = item->len;
> > - else {
> > + } else {
> > item->nowildcard_len = simple_length(item->match);
> > if (item->nowildcard_len < prefixlen)
> > item->nowildcard_len = prefixlen;
> > }
> > +
> > item->flags = 0;
>
> You probably can move this line up with the others too.
I didn't move the item->flags assignment up since the code immediately
following this assignment deal with setting item->flags. I made more
sense to keep them grouped.
--
Brandon Williams
^ permalink raw reply
* Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Duy Nguyen @ 2016-12-07 23:21 UTC (permalink / raw)
To: Stephan Beyer
Cc: Junio C Hamano, Git Mailing List, Christian Couder,
SZEDER Gábor
In-Reply-To: <8d32c645-e850-5e7c-b01c-6e4d81e2d672@gmx.net>
On Thu, Dec 8, 2016 at 3:35 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
> Hi,
>
> On 12/07/2016 09:04 PM, Junio C Hamano wrote:
>> Stephan Beyer <s-beyer@gmx.net> writes:
>>
>>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>>> different wording for the same thing makes things unintuitive.
>>
>> It is not too late to STOP "--forget" from getting added to "rebase"
>> and give it a better name.
Sorry I didn't know about --quit (and it has been there since 2011, I
guess I'm just not big sequencer user).
> Oh. ;) I am not sure. I personally think that --forget is a better name
Yeah, I was stuck with the name --destroy for many months and was very
happy the day I found --forget, which does not imply any destructive
side effects and is distinct enough from --abort to not confuse
people.
> than --quit because when I hear --quit I tend to look into the manual
> page first to check if there are weird side effects (and then the manual
> page says that it "forgets" ;D).
> So I'd rather favor adding --forget to cherry-pick/revert instead... or
> this:
--
Duy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox