* [PATCH 4/5] Make sequencer abort safer
From: Stephan Beyer @ 2016-12-07 21:51 UTC (permalink / raw)
To: git; +Cc: Stephan Beyer, Christian Couder, Junio C Hamano,
SZEDER Gábor
In-Reply-To: <xmqqlgvs28bh.fsf@gitster.mtv.corp.google.com>
In contrast to "git am --abort", a sequencer abort did not check
whether the current HEAD is the one that is expected. This can
lead to loss of work (when not spotted and resolved using reflog
before the garbage collector chimes in).
This behavior is now changed by mimicking "git am --abort":
the abortion is done but HEAD is not changed when the current HEAD
is not the expected HEAD.
A new file "sequencer/current" is added to save the expected HEAD.
The new behavior is only active when --abort is invoked on multiple
picks. The problem does not occur for the single-pick case because
it is handled differently.
Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
sequencer.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/sequencer.c b/sequencer.c
index 30b10ba14..c9b560ac1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
+static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
/*
* A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts)
return -1;
}
+static void update_curr_file()
+{
+ struct object_id head;
+
+ /* Do nothing on a single-pick */
+ if (!file_exists(git_path_seq_dir()))
+ return;
+
+ if (!get_oid("HEAD", &head))
+ write_file(git_path_curr_file(), "%s", oid_to_hex(&head));
+ else
+ write_file(git_path_curr_file(), "%s", "");
+}
+
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
{
@@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
strbuf_release(&sb);
strbuf_release(&err);
ref_transaction_free(transaction);
+ update_curr_file();
return 0;
}
@@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
leave:
free_message(commit, &msg);
+ update_curr_file();
return res;
}
@@ -1132,9 +1149,34 @@ static int save_head(const char *head)
return 0;
}
+static int rollback_is_safe()
+{
+ struct strbuf sb = STRBUF_INIT;
+ struct object_id expected_head, actual_head;
+
+ if (strbuf_read_file(&sb, git_path_curr_file(), 0) >= 0) {
+ strbuf_trim(&sb);
+ if (get_oid_hex(sb.buf, &expected_head)) {
+ strbuf_release(&sb);
+ die(_("could not parse %s"), git_path_curr_file());
+ }
+ strbuf_release(&sb);
+ }
+ else if (errno == ENOENT)
+ oidclr(&expected_head);
+ else
+ die_errno(_("could not read '%s'"), git_path_curr_file());
+
+ if (get_oid("HEAD", &actual_head))
+ oidclr(&actual_head);
+
+ return !oidcmp(&actual_head, &expected_head);
+}
+
static int reset_for_rollback(const unsigned char *sha1)
{
const char *argv[4]; /* reset --merge <arg> + NULL */
+
argv[0] = "reset";
argv[1] = "--merge";
argv[2] = sha1_to_hex(sha1);
@@ -1189,6 +1231,12 @@ int sequencer_rollback(struct replay_opts *opts)
error(_("cannot abort from a branch yet to be born"));
goto fail;
}
+
+ if (!rollback_is_safe()) {
+ /* Do not error, just do not rollback */
+ warning(_("You seem to have moved HEAD. "
+ "Not rewinding, check your HEAD!"));
+ } else
if (reset_for_rollback(sha1))
goto fail;
strbuf_release(&buf);
@@ -1393,6 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (save_opts(opts))
return -1;
+ update_curr_file();
res = pick_commits(&todo_list, opts);
todo_list_release(&todo_list);
return res;
--
2.11.0.27.g4eed97c
^ permalink raw reply related
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: vi0oss @ 2016-12-07 21:24 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Stefan Beller
In-Reply-To: <CAGZ79kY3LR2KA69b4iDJb164EhJLb3JuVSRRcN0-4-kp-eryog@mail.gmail.com>
On 12/07/2016 11:09 PM, Stefan Beller wrote:
>> 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".
> I wonder if this check is too weak and we actually have to check for
> either .git/objects or modules/<name/possibly/having/slashes>/objects.
> When writing the referenced commit I assumed we'd need a stronger check
> to be safer and not add some random location as a possible alternate.
>
1. Do we really need to check that it is named ".git"? Although
"git clone --mirror --recursive" is not (directly) supported
now, user may create one bare repository with [remnants of]
submodules by converting reqular repository into bare one.
Why not take advantage of additional information available locally
in this case?
2. Is the check need to be strict because of we need to traverse
one directory level up? Normally this "/objects" part is added by
Git, so just one "../" seems to be OK. User can't specify "--reference
somerepo/.git/objects", a strange reference can appear only if user
manually creates alternates. Maybe better to document this case
instead of restricting the feature?
3. If nonetheless check for ".git/*/objects", then
a. What functions should be used in Git codebase for such checks?
b. Should we handle tricks like "smth/.git/../../objects" and so on?
4. Should we print (or print in verbose mode) each used alternate,
to inform operator what his or her new clone will depend on?
P.S. Actually I discovered the --recursive --reference feature when tried to
put reference to a mega-repo with all possible submodules added as remotes.
I expected --reference to just get though across all submodules, but it
complained
to missing "/modules/..." instead (the check went though becase the
repository
was named like "megarepo.git", so it did ended in ".git/objects").
^ permalink raw reply
* Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly
From: Junio C Hamano @ 2016-12-07 21:08 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Paul Tan
In-Reply-To: <CAGZ79kZHGqU2y19_uKhtVuE6vhspzPNpw-nVDnm8gLQ8u528kQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> So my first question I had to answer was if we do the right thing here,
> i.e. if we could just fail instead. But we want to continue and just
> not write back the index, which is fine.
>
> So we do not have to guard refresh_cache, but just call
> update_index_if_able conditionally.
An explanation with stepping back a little bit may help.
You may be asked to visit a repository of a friend, to which you do
not have write access to but you can still read. You may want to do
"diff", "status" or "describe" there.
In order to avoid getting fooled into thinking some paths are dirty
only because the cached stat information does not match, these need
to refresh the in-core index before doing their "comparison" to
report which paths are different (in "diff"), what are the modified
but not staged paths (in "status"), and if there is a need to add
the "-dirty" suffix (in "describe").
Since we are doing the expensive "bunch of lstat()" anyway, if we
could write it back to the index, it would help future operations in
the same repository--that is the reasoning behind the opportunistic
updates. It is perfectly OK if we do not have write access to the
repository and cannot write update the index.
^ permalink raw reply
* [PATCHv5 1/5] submodule: use absolute path for computing relative path connecting
From: Stefan Beller @ 2016-12-07 21:01 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161207210157.18932-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.28.g2af45f1.dirty
^ permalink raw reply related
* [PATCHv5 5/5] submodule: add embed-git-dir function
From: Stefan Beller @ 2016-12-07 21:01 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161207210157.18932-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 embedded
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 `submodule_embed_git_dir_for_path`, 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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/git-submodule.txt | 14 +++++
builtin/submodule--helper.c | 39 ++++++++++++-
dir.c | 27 +++++++++
dir.h | 3 +
git-submodule.sh | 7 ++-
submodule.c | 115 ++++++++++++++++++++++++++++++++++++++
submodule.h | 7 +++
t/t7412-submodule-embedgitdirs.sh | 101 +++++++++++++++++++++++++++++++++
8 files changed, 311 insertions(+), 2 deletions(-)
create mode 100755 t/t7412-submodule-embedgitdirs.sh
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..34791cfc65 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] embedgitdirs [--] [<path>...]
DESCRIPTION
@@ -245,6 +246,19 @@ sync::
If `--recursive` is specified, this command will recurse into the
registered submodules, and sync any nested submodules within.
+embedgitdirs::
+ Move the git directory of submodules into its superprojects
+ `$GIT_DIR/modules` path and then connect the git directory and
+ its working directory by setting the `core.worktree` and adding
+ a .git file pointing to the git directory interned into the
+ superproject.
++
+A repository that was cloned independently and later added as a submodule or
+old setups have the submodules git directory inside the submodule instead of
+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 806e29ce4e..321c9e250a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,42 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,
return 0;
}
+static int embed_git_dir(int argc, const char **argv, const char *prefix)
+{
+ int i;
+ struct pathspec pathspec;
+ struct module_list list = MODULE_LIST_INIT;
+ unsigned flags = RELOCATE_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"),
+ RELOCATE_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++)
+ submodule_embed_git_dir(prefix, list.entries[i]->name, flags);
+
+ return 0;
+}
+
#define SUPPORT_SUPER_PREFIX (1<<0)
struct cmd_struct {
@@ -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}
};
int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
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);
+
+ if (rename(old_git_dir, new_git_dir) < 0) {
+ ret = errno;
+ strbuf_addf(err,
+ _("could not migrate git directory from '%s' to '%s'"),
+ old_git_dir, new_git_dir);
+ return ret;
+ }
+
+ connect_work_tree_and_git_dir(path, new_git_dir);
+
+ return ret;
+}
diff --git a/dir.h b/dir.h
index 97c83bb383..bf06729a86 100644
--- a/dir.h
+++ b/dir.h
@@ -335,4 +335,7 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);
void add_untracked_cache(struct index_state *istate);
void remove_untracked_cache(struct index_state *istate);
+extern int relocate_gitdir(const char *path, const char *old_git_dir,
+ const char *new_git_dir, const char *displaypath,
+ struct strbuf *err);
#endif
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a135d6..b7e124f340 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1131,6 +1131,11 @@ cmd_sync()
done
}
+cmd_embedgitdirs()
+{
+ git submodule--helper embed-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 | embedgitdirs)
command=$1
;;
-q|--quiet)
diff --git a/submodule.c b/submodule.c
index 66c5ce5a24..67a91275b8 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;
@@ -1263,3 +1264,117 @@ void prepare_submodule_repo_env(struct argv_array *out)
}
argv_array_push(out, "GIT_DIR=.git");
}
+
+/* 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);
+ }
+
+ 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();
+ strbuf_addf(&pathbuf, "%s%s", prefix ? prefix : "", path);
+
+ code = relocate_gitdir(path, real_old_git_dir, real_new_git_dir,
+ pathbuf.buf, &errbuf);
+ if (code) {
+ errno = code;
+ die_errno("%s\n", errbuf.buf);
+ }
+
+ free(old_git_dir);
+ free(real_old_git_dir);
+ free(real_new_git_dir);
+ strbuf_release(&pathbuf);
+}
+
+/*
+ * 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/.
+ */
+int submodule_embed_git_dir(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 embedded? */
+ 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))
+ 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);
+ }
+
+out:
+ strbuf_release(&gitdir);
+ free(real_sub_git_dir);
+ free(real_common_git_dir);
+ return 0;
+}
diff --git a/submodule.h b/submodule.h
index d9e197a948..922cfd258f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -75,4 +75,11 @@ int parallel_submodules(void);
*/
void prepare_submodule_repo_env(struct argv_array *out);
+/*
+ * Embed a git dir of the submodule given by path.
+ */
+#define RELOCATE_GITDIR_RECURSE_SUBMODULES (1<<0)
+extern int submodule_embed_git_dir(const char *prefix,
+ const char *path,
+ unsigned flags);
#endif
diff --git a/t/t7412-submodule-embedgitdirs.sh b/t/t7412-submodule-embedgitdirs.sh
new file mode 100755
index 0000000000..a02e7c447a
--- /dev/null
+++ b/t/t7412-submodule-embedgitdirs.sh
@@ -0,0 +1,101 @@
+#!/bin/sh
+
+test_description='Test submodule embedgitdirs
+
+This test verifies that `git submodue embedgitdirs` 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 'embed 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 embedgitdirs &&
+ 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 '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 &&
+ test -d sub1
+'
+
+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 'embed the git dir in a nested submodule' '
+ git status >expect.1 &&
+ git -C sub1/nested rev-parse HEAD >expect.2 &&
+ git submodule embedgitdirs &&
+ 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 'embedding the git dir fails for incomplete submodules' '
+ git status >expect.1 &&
+ git -C sub2 rev-parse HEAD >expect.2 &&
+ test_must_fail git submodule embedgitdirs &&
+ 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 'embed a submodule with multiple worktrees' '
+ test_must_fail git submodule embedgitdirs sub3 2>error &&
+ test_i18ngrep "not supported" error
+'
+
+test_done
--
2.11.0.rc2.28.g2af45f1.dirty
^ permalink raw reply related
* [PATCHv5 4/5] worktree: get worktrees from submodules
From: Stefan Beller @ 2016-12-07 21:01 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161207210157.18932-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 | 47 +++++++++++++++++++++++++++++++++++++----------
worktree.h | 6 ++++++
2 files changed, 43 insertions(+), 10 deletions(-)
diff --git a/worktree.c b/worktree.c
index eb6121263b..fa2b6dfa9a 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,31 @@ 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;
+ const char *submodule_common_dir;
+ 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);
+ submodule_common_dir = strbuf_detach(&sb, NULL);
+ ret = get_worktrees_internal(submodule_common_dir, flags);
+
+ 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.28.g2af45f1.dirty
^ permalink raw reply related
* [PATCHv5 3/5] test-lib-functions.sh: teach test_commit -C <dir>
From: Stefan Beller @ 2016-12-07 21:01 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161207210157.18932-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.28.g2af45f1.dirty
^ permalink raw reply related
* [PATCHv5 2/5] submodule helper: support super prefix
From: Stefan Beller @ 2016-12-07 21:01 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
In-Reply-To: <20161207210157.18932-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..806e29ce4e 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.28.g2af45f1.dirty
^ permalink raw reply related
* [PATCHv5 0/5] submodule embedgitdirs
From: Stefan Beller @ 2016-12-07 21:01 UTC (permalink / raw)
To: bmwill; +Cc: git, pclouds, gitster, Stefan Beller
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.
Thanks,
Stefan
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 (5):
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
submodule: add embed-git-dir function
Documentation/git-submodule.txt | 14 +++++
builtin/submodule--helper.c | 68 ++++++++++++++++----
dir.c | 27 ++++++++
dir.h | 3 +
git-submodule.sh | 7 ++-
git.c | 2 +-
submodule.c | 127 ++++++++++++++++++++++++++++++++++++--
submodule.h | 7 +++
t/t7412-submodule-embedgitdirs.sh | 101 ++++++++++++++++++++++++++++++
t/test-lib-functions.sh | 20 ++++--
worktree.c | 47 +++++++++++---
worktree.h | 6 ++
12 files changed, 396 insertions(+), 33 deletions(-)
create mode 100755 t/t7412-submodule-embedgitdirs.sh
diff to v4:
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 10df69c86a..321c9e250a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1106,31 +1106,8 @@ static int embed_git_dir(int argc, const char **argv, const char *prefix)
if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
return 1;
- for (i = 0; i < list.nr; i++) {
- const char *path = list.entries[i]->name, *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 free_and_continue;
-
- /* Is it already embedded? */
- 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), NULL)
- goto free_and_continue;
-
- relocate_gitdir(prefix, path, flags);
-
-free_and_continue:
- strbuf_release(&gitdir);
- free(real_sub_git_dir);
- free(real_common_git_dir);
- }
+ for (i = 0; i < list.nr; i++)
+ submodule_embed_git_dir(prefix, list.entries[i]->name, flags);
return 0;
}
diff --git a/dir.c b/dir.c
index d2f60b5abf..e023b04407 100644
--- a/dir.c
+++ b/dir.c
@@ -15,9 +15,6 @@
#include "utf8.h"
#include "varint.h"
#include "ewah/ewok.h"
-#include "submodule-config.h"
-#include "run-command.h"
-#include "worktree.h"
struct path_simplify {
int len;
@@ -2753,79 +2750,28 @@ void untracked_cache_add_to_index(struct index_state *istate,
}
/*
- * Migrate the given submodule (and all its submodules recursively) from
- * having its git directory within the working tree to the git dir nested
- * in its superprojects git dir under modules/.
+ * 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.
*/
-void relocate_gitdir(const char *prefix, const char *path, unsigned flags)
+int relocate_gitdir(const char *path, const char *old_git_dir,
+ const char *new_git_dir, const char *displaypath,
+ struct strbuf *err)
{
- char *old_git_dir;
- const char *new_git_dir;
- const struct submodule *sub;
- struct worktree **worktrees;
- int i;
-
- worktrees = get_submodule_worktrees(path, 0);
- if (worktrees) {
- for (i = 0; worktrees[i]; i++)
- ;
- free_worktrees(worktrees);
- if (i > 1)
- die(_("relocate_gitdir for submodule with more than one worktree not supported"));
- }
-
- old_git_dir = xstrfmt("%s/.git", path);
- if (read_gitfile(old_git_dir))
- /* If it is an actual gitfile, it doesn't need migration. */
- goto out;
-
- sub = submodule_from_path(null_sha1, path);
- if (!sub)
- die(_("Could not lookup name for submodule '%s'"),
- path);
+ int ret = 0;
- 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);
+ printf("Migrating git directory of '%s' from\n'%s' to\n'%s'\n",
+ displaypath, old_git_dir, new_git_dir);
- if (!prefix)
- prefix = get_super_prefix();
- printf("Migrating git directory of %s%s from\n'%s' to\n'%s'\n",
- prefix ? prefix : "", path,
- real_path(old_git_dir), new_git_dir);
-
- if (rename(old_git_dir, new_git_dir) < 0)
- die_errno(_("Could not migrate git directory from '%s' to '%s'"),
+ if (rename(old_git_dir, new_git_dir) < 0) {
+ ret = errno;
+ strbuf_addf(err,
+ _("could not migrate git directory from '%s' to '%s'"),
old_git_dir, new_git_dir);
+ return ret;
+ }
connect_work_tree_and_git_dir(path, new_git_dir);
-out:
- 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 migrate git directory in submodule '%s'"),
- path);
-
- strbuf_release(&sb);
- }
-
- free(old_git_dir);
+ return ret;
}
diff --git a/dir.h b/dir.h
index 0b5e99b21d..bf06729a86 100644
--- a/dir.h
+++ b/dir.h
@@ -335,8 +335,7 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long
void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);
void add_untracked_cache(struct index_state *istate);
void remove_untracked_cache(struct index_state *istate);
-
-#define RELOCATE_GITDIR_RECURSE_SUBMODULES (1<<0)
-extern void relocate_gitdir(const char *prefix, const char *path, unsigned flags);
-
+extern int relocate_gitdir(const char *path, const char *old_git_dir,
+ const char *new_git_dir, const char *displaypath,
+ struct strbuf *err);
#endif
diff --git a/submodule.c b/submodule.c
index 66c5ce5a24..67a91275b8 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;
@@ -1263,3 +1264,117 @@ void prepare_submodule_repo_env(struct argv_array *out)
}
argv_array_push(out, "GIT_DIR=.git");
}
+
+/* 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);
+ }
+
+ 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();
+ strbuf_addf(&pathbuf, "%s%s", prefix ? prefix : "", path);
+
+ code = relocate_gitdir(path, real_old_git_dir, real_new_git_dir,
+ pathbuf.buf, &errbuf);
+ if (code) {
+ errno = code;
+ die_errno("%s\n", errbuf.buf);
+ }
+
+ free(old_git_dir);
+ free(real_old_git_dir);
+ free(real_new_git_dir);
+ strbuf_release(&pathbuf);
+}
+
+/*
+ * 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/.
+ */
+int submodule_embed_git_dir(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 embedded? */
+ 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))
+ 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);
+ }
+
+out:
+ strbuf_release(&gitdir);
+ free(real_sub_git_dir);
+ free(real_common_git_dir);
+ return 0;
+}
diff --git a/submodule.h b/submodule.h
index d9e197a948..922cfd258f 100644
--- a/submodule.h
+++ b/submodule.h
@@ -75,4 +75,11 @@ int parallel_submodules(void);
*/
void prepare_submodule_repo_env(struct argv_array *out);
+/*
+ * Embed a git dir of the submodule given by path.
+ */
+#define RELOCATE_GITDIR_RECURSE_SUBMODULES (1<<0)
+extern int submodule_embed_git_dir(const char *prefix,
+ const char *path,
+ unsigned flags);
#endif
--
2.11.0.rc2.28.g2af45f1.dirty
^ permalink raw reply related
* Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly
From: Stefan Beller @ 2016-12-07 20:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Paul Tan
In-Reply-To: <20161207194105.25780-2-gitster@pobox.com>
On Wed, Dec 7, 2016 at 11:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> The require_clean_work_tree() function calls hold_locked_index()
> with die_on_error=0 to signal that it is OK if it fails to obtain
> the lock, but unconditionally calls update_index_if_able(), which
> will try to write into fd=-1.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
So my first question I had to answer was if we do the right thing here,
i.e. if we could just fail instead. But we want to continue and just
not write back the index, which is fine.
So we do not have to guard refresh_cache, but just call
update_index_if_able conditionally.
However I think the promise of that function is
to take care of the fd == -1?
/*
* Opportunistically update the index but do not complain if we can't
*/
void update_index_if_able(struct index_state *istate, struct
lock_file *lockfile)
{
if ((istate->cache_changed || has_racy_timestamp(istate)) &&
verify_index(istate) &&
write_locked_index(istate, lockfile, COMMIT_LOCK))
rollback_lock_file(lockfile);
}
So I would expect that we'd rather fix the update_index_if_able instead by
checking for the lockfile to be in the correct state?
> wt-status.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index a2e9d332d8..a715e71906 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -2258,11 +2258,12 @@ int has_uncommitted_changes(int ignore_submodules)
> int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)
> {
> struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
> - int err = 0;
> + int err = 0, fd;
>
> - hold_locked_index(lock_file, 0);
> + fd = hold_locked_index(lock_file, 0);
> refresh_cache(REFRESH_QUIET);
> - update_index_if_able(&the_index, lock_file);
> + if (0 <= fd)
> + update_index_if_able(&the_index, lock_file);
> rollback_lock_file(lock_file);
>
> if (has_unstaged_changes(ignore_submodules)) {
> --
> 2.11.0-274-g0631465056
>
^ permalink raw reply
* Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly
From: Stefan Beller @ 2016-12-07 20:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org, Paul Tan
In-Reply-To: <CAPc5daX2CZ0=UBMuz70KwFPBDTUgAdi8WoVUJ7gNTq+QEXKxbg@mail.gmail.com>
On Wed, Dec 7, 2016 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> On Wed, Dec 7, 2016 at 12:48 PM, Stefan Beller <sbeller@google.com> wrote:
>>
>> So I would expect that we'd rather fix the update_index_if_able instead by
>> checking for the lockfile to be in the correct state?
>
> I actually don't expect that, after looking at other call sites of
> that function.
Yes I checked the other callers as well right now and you seem to be correct.
My initial response was based on the name of the function,
specifically the _if_able
part as that hinted to me that I can call the function with no
precondition and the
_if_able will figure out when to do the actual update_index.
The first part of the condition of the function
(istate->cache_changed || has_racy_timestamp(istate)
reads rather as a _if_needed instead of an _if_able to me.
^ permalink raw reply
* Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly
From: Junio C Hamano @ 2016-12-07 20:53 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org, Paul Tan
In-Reply-To: <CAGZ79kZHGqU2y19_uKhtVuE6vhspzPNpw-nVDnm8gLQ8u528kQ@mail.gmail.com>
On Wed, Dec 7, 2016 at 12:48 PM, Stefan Beller <sbeller@google.com> wrote:
>
> So I would expect that we'd rather fix the update_index_if_able instead by
> checking for the lockfile to be in the correct state?
I actually don't expect that, after looking at other call sites of
that function.
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Johannes Sixt @ 2016-12-07 20:43 UTC (permalink / raw)
To: Brandon Williams; +Cc: Junio C Hamano, git, sbeller, peff, jacob.keller
In-Reply-To: <20161207001018.GD103573@google.com>
Am 07.12.2016 um 01:10 schrieb Brandon Williams:
> This function should accept both absolute and relative paths, which
> means it should probably accept "C:\My Files". I wasn't thinking about
> windows 100% of the time while writing this so I'm hoping that a windows
> expert will point things like this out to me :).
;)
With this patch, the test suite fails at the very first git init call:
D:\Src\mingw-git\t>sh t0000-basic.sh -v -i -x
fatal: Invalid path '/:': No such file or directory
error: cannot run git init -- have you built things yet?
FATAL: Unexpected exit with code 1
I haven't dug further, yet.
-- Hannes
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Junio C Hamano @ 2016-12-07 20:32 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Ramsay Jones, Brandon Williams, git, sbeller, peff, jacob.keller
In-Reply-To: <20161207201409.GA19743@tb-raspi>
Torsten Bögershausen <tboegi@web.de> writes:
> But in any case it seems that e.g.
> //SEFVER/SHARE/DIR1/DIR2/..
> must be converted into
> //SEFVER/SHARE/DIR1
>
> and
> \\SEFVER\SHARE\DIR1\DIR2\..
> must be converted into
> \\SEFVER\SHARE\DIR1
Additional questions that may be interesting are:
//A/B/../C is it //A/C? is it an error?
//A/B/../../C/D is it //C/D? is it an error?
^ permalink raw reply
* Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Stephan Beyer @ 2016-12-07 20:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder, SZEDER Gábor
In-Reply-To: <xmqq8trry08k.fsf@gitster.mtv.corp.google.com>
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.
Oh. ;) I am not sure. I personally think that --forget is a better name
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:
> Having said that, I have a feeling that these options do not have to
> exist; isn't their presence just a symptom that the "--abort" for
> the command misbehaves? Isn't the reason why there is no need for
> "am --quit" because its "--abort" behaves more sensibly?
You're probably right. I have no other use-case in mind than "oh I
forgot that I was rebasing... now just abort that and don't bother me
further (i.e. please don't bring me back)"
~Stephan
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Junio C Hamano @ 2016-12-07 20:28 UTC (permalink / raw)
To: Stefan Beller; +Cc: vi0oss, git@vger.kernel.org, Stefan Beller
In-Reply-To: <CAGZ79ka=XgO-VyS+Jqq2Fy28kPMy6HnXBAHb79Dt3NegZFd6kw@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Wed, Dec 7, 2016 at 12:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>>> 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.
>>>
>>> Sounds great!
>>
>> Is it safe to assume that all the submodules used recursively by
>> submodules share the same structure upstream? Does the alternate
>> location mechanism degrades sensibly if this assumption turns out to
>> be false (i.e. "possible alternates" above turns out to be mere
>> possibility and not there)?
>
> According to the last test in the patch, this seems to be doing the
> sensible thing.
OK, that sounds great. Thanks.
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Stefan Beller @ 2016-12-07 20:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: vi0oss, git@vger.kernel.org, Stefan Beller
In-Reply-To: <xmqq4m2fxzl2.fsf@gitster.mtv.corp.google.com>
On Wed, Dec 7, 2016 at 12:18 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>> 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.
>>
>> Sounds great!
>
> Is it safe to assume that all the submodules used recursively by
> submodules share the same structure upstream? Does the alternate
> location mechanism degrades sensibly if this assumption turns out to
> be false (i.e. "possible alternates" above turns out to be mere
> possibility and not there)?
According to the last test in the patch, this seems to be doing the
sensible thing.
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Junio C Hamano @ 2016-12-07 20:18 UTC (permalink / raw)
To: Stefan Beller; +Cc: vi0oss, git@vger.kernel.org, Stefan Beller
In-Reply-To: <CAGZ79kY3LR2KA69b4iDJb164EhJLb3JuVSRRcN0-4-kp-eryog@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
>> 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.
>
> Sounds great!
Is it safe to assume that all the submodules used recursively by
submodules share the same structure upstream? Does the alternate
location mechanism degrades sensibly if this assumption turns out to
be false (i.e. "possible alternates" above turns out to be mere
possibility and not there)?
^ permalink raw reply
* Re: [PATCH] real_path: make real_path thread-safe
From: Torsten Bögershausen @ 2016-12-07 20:14 UTC (permalink / raw)
To: Ramsay Jones
Cc: Brandon Williams, Junio C Hamano, git, sbeller, peff,
jacob.keller
In-Reply-To: <b73e61f8-0cff-b33e-118a-e530d367c94c@ramsayjones.plus.com>
On Wed, Dec 07, 2016 at 01:12:25AM +0000, Ramsay Jones wrote:
>
>
> On 07/12/16 00:10, Brandon Williams wrote:
> > On 12/06, Junio C Hamano wrote:
> >> POSIX cares about treating "//" at the very beginning of the path
> >> specially. Is that supposed to be handled here, or by a lot higher
> >> level up in the callchain?
> >
> > What exactly does "//" mean in this context? (I'm just naive in this
> > area)
>
> This refers to a UNC path (ie Universal Naming Convention) which
> is used to refer to servers, printers and other 'network resources'.
> Although this started (many moons ago) in unix, it isn't used too
> much outside of windows networks! (where it is usually denoted by
> \\servername\path).
>
> You can see the relics of unix UNC paths if you look at the wording
> for basename() in the POSIX standard. Note the special treatment of
> the path which 'is exactly "//"', see http://pubs.opengroup.org/onlinepubs/009695399/functions/basename.html
>
> ATB,
> Ramsay Jones
Please allow one more comment about UNC:
They are used under Windows, and typically wotk together with Git.
One breakage between 2.10 and 2.11 has been observed, saying that
pushing to \\SERVER\SHARE\DIRECTORY does not work any more.
It has been reported under
"git 2.11.0 error when pushing to remote located on a windows share"
both here and
here:
https://github.com/git-for-windows/git/issues/979#issuecomment-264816175
I don't have a Windows box at the moment, and I don't know if the
breakage was introduced by changes in real_patyh().
But in any case it seems that e.g.
//SEFVER/SHARE/DIR1/DIR2/..
must be converted into
//SEFVER/SHARE/DIR1
and
\\SEFVER\SHARE\DIR1\DIR2\..
must be converted into
\\SEFVER\SHARE\DIR1
^ permalink raw reply
* Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules
From: Stefan Beller @ 2016-12-07 20:09 UTC (permalink / raw)
To: vi0oss; +Cc: git@vger.kernel.org, Stefan Beller
In-Reply-To: <20161207184248.6130-1-vi0oss@gmail.com>
On Wed, Dec 7, 2016 at 10:42 AM, <vi0oss@gmail.com> wrote:
> From: Vitaly _Vi Shukela <vi0oss@gmail.com>
Thanks for contributing to Git!
(/me looks up if you have sent patches already as you
seem to know how to do that. :) unrelated side note: Maybe you want
to send a patch for the .mailmap file mapping your two email addresses
together, c.f. "git log -- .mailmap")
>
> Git v2.11 introduced "git clone --recursive --referece ...",
> but it didn't put the alternates for _nested_ submodules.
This message is targeted at people familiar with gits code base,
so we can be more specific. e.g.
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.
Sounds great!
>
> 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".
I wonder if this check is too weak and we actually have to check for
either .git/objects or modules/<name/possibly/having/slashes>/objects.
When writing the referenced commit I assumed we'd need a stronger check
to be safer and not add some random location as a possible alternate.
>
> New tests have been added to t7408-submodule-reference.
Thanks!
>
> Signed-off-by: Vitaly _Vi Shukela <vi0oss@gmail.com>
> ---
> builtin/submodule--helper.c | 24 ++++++++++++--
> t/t7408-submodule-reference.sh | 73 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 95 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 4beeda5..93dae62 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;
> @@ -672,6 +672,26 @@ 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 */
> + {
Usually we do not use braces to further nest code, please remove this nesting.
> + char *sm_alternate = NULL, *error_strategy = NULL;
> +
> + 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..7b64725 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -125,4 +125,77 @@ 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 &&
Usually we quote strings containing white space, e.g. echo "I am ..." >actual
> + 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
> + ) &&
> + echo not cleaning supersuper
This echo is left in for debugging purposes?
^ permalink raw reply
* Re: BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Junio C Hamano @ 2016-12-07 20:04 UTC (permalink / raw)
To: Stephan Beyer; +Cc: git, Christian Couder, SZEDER Gábor
In-Reply-To: <6facca6e-622a-ea8f-89d8-a18b7faee3cc@gmx.net>
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.
Having said that, I have a feeling that these options do not have to
exist; isn't their presence just a symptom that the "--abort" for
the command misbehaves? Isn't the reason why there is no need for
"am --quit" because its "--abort" behaves more sensibly?
^ permalink raw reply
* [PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR
From: Junio C Hamano @ 2016-12-07 19:41 UTC (permalink / raw)
To: git; +Cc: Robbie Iannucci, Johannes Schindelin
In-Reply-To: <20161207194105.25780-1-gitster@pobox.com>
The "libify sequencer" topic stopped passing the die_on_error option
to hold_locked_index(), and this lost an error message from "git
merge --ff-only $commit" when there are competing updates in
progress.
The command still exits with a non-zero status, but that is not of
much help for an interactive user. The last thing the command says
is "Updating $from..$to". We used to follow it with a big error
message that makes it clear that "merge --ff-only" did not succeed.
What is sad is that we should have noticed this regression while
reviewing the change. It was clear that the update to the
checkout_fast_forward() function made a failing hold_locked_index()
silent, but the only caller of the checkout_fast_forward() function
had this comment:
if (checkout_fast_forward(from, to, 1))
- exit(128); /* the callee should have complained already */
+ return -1; /* the callee should have complained already */
which clearly contradicted the assumption X-<.
Add a new option LOCK_REPORT_ON_ERROR that can be passed instead of
LOCK_DIE_ON_ERROR to the hold_lock*() family of functions and teach
checkout_fast_forward() to use it to fix this regression.
After going thourgh all calls to hold_lock*() family of functions
that used to pass LOCK_DIE_ON_ERROR but were modified to pass 0 in
the "libify sequencer" topic "git show --first-parent 2a4062a4a8",
it appears that this is the only one that has become silent. Many
others used to give detailed report that talked about "there may be
competing Git process running" but with the series merged they now
only give a single liner "Unable to lock ...", some of which may
have to be tweaked further, but at least they say something, unlike
the one this patch fixes.
Reported-by: Robbie Iannucci <iannucci@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
lockfile.c | 12 ++++++++++--
lockfile.h | 8 +++++++-
merge.c | 2 +-
3 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/lockfile.c b/lockfile.c
index 9268cdf325..aa69210d8b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -174,8 +174,16 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
int flags, long timeout_ms)
{
int fd = lock_file_timeout(lk, path, flags, timeout_ms);
- if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
- unable_to_lock_die(path, errno);
+ if (fd < 0) {
+ if (flags & LOCK_DIE_ON_ERROR)
+ unable_to_lock_die(path, errno);
+ if (flags & LOCK_REPORT_ON_ERROR) {
+ struct strbuf buf = STRBUF_INIT;
+ unable_to_lock_message(path, errno, &buf);
+ error("%s", buf.buf);
+ strbuf_release(&buf);
+ }
+ }
return fd;
}
diff --git a/lockfile.h b/lockfile.h
index d26ad27b2b..16775a7d79 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -129,11 +129,17 @@ struct lock_file {
/*
* If a lock is already taken for the file, `die()` with an error
* message. If this flag is not specified, trying to lock a file that
- * is already locked returns -1 to the caller.
+ * is already locked silently returns -1 to the caller, or ...
*/
#define LOCK_DIE_ON_ERROR 1
/*
+ * ... this flag can be passed instead to return -1 and give the usual
+ * error message upon an error.
+ */
+#define LOCK_REPORT_ON_ERROR 2
+
+/*
* Usually symbolic links in the destination path are resolved. This
* means that (1) the lockfile is created by adding ".lock" to the
* resolved path, and (2) upon commit, the resolved path is
diff --git a/merge.c b/merge.c
index 23866c9165..04ee5fc911 100644
--- a/merge.c
+++ b/merge.c
@@ -57,7 +57,7 @@ int checkout_fast_forward(const unsigned char *head,
refresh_cache(REFRESH_QUIET);
- if (hold_locked_index(lock_file, 0) < 0)
+ if (hold_locked_index(lock_file, LOCK_REPORT_ON_ERROR) < 0)
return -1;
memset(&trees, 0, sizeof(trees));
--
2.11.0-274-g0631465056
^ permalink raw reply related
* [PATCH 2/3] hold_locked_index(): align error handling with hold_lockfile_for_update()
From: Junio C Hamano @ 2016-12-07 19:41 UTC (permalink / raw)
To: git; +Cc: Robbie Iannucci, Johannes Schindelin
In-Reply-To: <20161207194105.25780-1-gitster@pobox.com>
Callers of the hold_locked_index() function pass 0 when they want to
prepare to write a new version of the index file without wishing to
die or emit an error message when the request fails (e.g. somebody
else already held the lock), and pass 1 when they want the call to
die upon failure.
This option is called LOCK_DIE_ON_ERROR by the underlying lockfile
API, and the hold_locked_index() function translates the paramter to
LOCK_DIE_ON_ERROR when calling the hold_lock_file_for_update().
Replace these hardcoded '1' with LOCK_DIE_ON_ERROR and stop
translating. Callers other than the ones that are replaced with
this change pass '0' to the function; no behaviour change is
intended with this patch.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Among the callers of hold_locked_index() that passes 0:
- diff.c::refresh_index_quietly() at the end of "git diff" is an
opportunistic update; it leaks the lockfile structure but it is
just before the program exits and nobody should care.
- builtin/describe.c::cmd_describe(),
builtin/commit.c::cmd_status(),
sequencer.c::read_and_refresh_cache() are all opportunistic
updates and they are OK.
- builtin/update-index.c::cmd_update_index() takes a lock upfront
but we may end up not needing to update the index (i.e. the
entries may be fully up-to-date), in which case we do not need to
issue an error upon failure to acquire the lock. We do diagnose
and die if we indeed need to update, so it is OK.
- wt-status.c::require_clean_work_tree() IS BUGGY. It asks
silence, does not check the returned value. Compare with
callsites like cmd_describe() and cmd_status() to notice that it
is wrong to call update_index_if_able() unconditionally.
---
apply.c | 2 +-
builtin/add.c | 2 +-
builtin/am.c | 6 +++---
builtin/checkout-index.c | 2 +-
builtin/checkout.c | 4 ++--
builtin/clone.c | 2 +-
builtin/commit.c | 8 ++++----
builtin/merge.c | 6 +++---
builtin/mv.c | 2 +-
builtin/read-tree.c | 2 +-
builtin/reset.c | 2 +-
builtin/rm.c | 2 +-
builtin/update-index.c | 1 +
merge-recursive.c | 2 +-
read-cache.c | 7 ++-----
rerere.c | 2 +-
sequencer.c | 2 +-
t/helper/test-scrap-cache-tree.c | 2 +-
18 files changed, 27 insertions(+), 29 deletions(-)
diff --git a/apply.c b/apply.c
index 705cf562f0..2ed808d429 100644
--- a/apply.c
+++ b/apply.c
@@ -4688,7 +4688,7 @@ static int apply_patch(struct apply_state *state,
state->index_file,
LOCK_DIE_ON_ERROR);
else
- state->newfd = hold_locked_index(state->lock_file, 1);
+ state->newfd = hold_locked_index(state->lock_file, LOCK_DIE_ON_ERROR);
}
if (state->check_index && read_apply_cache(state) < 0) {
diff --git a/builtin/add.c b/builtin/add.c
index e8fb80b36e..9f53f020d0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -361,7 +361,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
add_new_files = !take_worktree_changes && !refresh_only;
require_pathspec = !(take_worktree_changes || (0 < addremove_explicit));
- hold_locked_index(&lock_file, 1);
+ hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
(show_only ? ADD_CACHE_PRETEND : 0) |
diff --git a/builtin/am.c b/builtin/am.c
index 6981f42ce9..bb5da422fc 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1119,7 +1119,7 @@ static void refresh_and_write_cache(void)
{
struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
- hold_locked_index(lock_file, 1);
+ hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
refresh_cache(REFRESH_QUIET);
if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
die(_("unable to write index file"));
@@ -1976,7 +1976,7 @@ static int fast_forward_to(struct tree *head, struct tree *remote, int reset)
return -1;
lock_file = xcalloc(1, sizeof(struct lock_file));
- hold_locked_index(lock_file, 1);
+ hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
refresh_cache(REFRESH_QUIET);
@@ -2016,7 +2016,7 @@ static int merge_tree(struct tree *tree)
return -1;
lock_file = xcalloc(1, sizeof(struct lock_file));
- hold_locked_index(lock_file, 1);
+ hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
memset(&opts, 0, sizeof(opts));
opts.head_idx = 1;
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 30a49d9f42..07631d0c9c 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -205,7 +205,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
if (index_opt && !state.base_dir_len && !to_tempfile) {
state.refresh_cache = 1;
state.istate = &the_index;
- newfd = hold_locked_index(&lock_file, 1);
+ newfd = hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
}
/* Check out named files first */
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 512492aad9..bfe685c198 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -274,7 +274,7 @@ static int checkout_paths(const struct checkout_opts *opts,
lock_file = xcalloc(1, sizeof(struct lock_file));
- hold_locked_index(lock_file, 1);
+ hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
if (read_cache_preload(&opts->pathspec) < 0)
return error(_("index file corrupt"));
@@ -467,7 +467,7 @@ static int merge_working_tree(const struct checkout_opts *opts,
int ret;
struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
- hold_locked_index(lock_file, 1);
+ hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
if (read_cache_preload(NULL) < 0)
return error(_("index file corrupt"));
diff --git a/builtin/clone.c b/builtin/clone.c
index 6c76a6ed66..892bdbfe3f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -711,7 +711,7 @@ static int checkout(int submodule_progress)
setup_work_tree();
lock_file = xcalloc(1, sizeof(struct lock_file));
- hold_locked_index(lock_file, 1);
+ hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
memset(&opts, 0, sizeof opts);
opts.update = 1;
diff --git a/builtin/commit.c b/builtin/commit.c
index 8976c3d29b..b910e76017 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -351,7 +351,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
if (interactive) {
char *old_index_env = NULL;
- hold_locked_index(&index_lock, 1);
+ hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
refresh_cache_or_die(refresh_flags);
@@ -396,7 +396,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
* (B) on failure, rollback the real index.
*/
if (all || (also && pathspec.nr)) {
- hold_locked_index(&index_lock, 1);
+ hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
add_files_to_cache(also ? prefix : NULL, &pathspec, 0);
refresh_cache_or_die(refresh_flags);
update_main_cache_tree(WRITE_TREE_SILENT);
@@ -416,7 +416,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
* We still need to refresh the index here.
*/
if (!only && !pathspec.nr) {
- hold_locked_index(&index_lock, 1);
+ hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
refresh_cache_or_die(refresh_flags);
if (active_cache_changed
|| !cache_tree_fully_valid(active_cache_tree))
@@ -468,7 +468,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
if (read_cache() < 0)
die(_("cannot read the index"));
- hold_locked_index(&index_lock, 1);
+ hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
add_remove_files(&partial);
refresh_cache(REFRESH_QUIET);
update_main_cache_tree(WRITE_TREE_SILENT);
diff --git a/builtin/merge.c b/builtin/merge.c
index b65eeaa87d..0070bf2556 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -634,7 +634,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
{
static struct lock_file lock;
- hold_locked_index(&lock, 1);
+ hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
refresh_cache(REFRESH_QUIET);
if (active_cache_changed &&
write_locked_index(&the_index, &lock, COMMIT_LOCK))
@@ -671,7 +671,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
for (j = common; j; j = j->next)
commit_list_insert(j->item, &reversed);
- hold_locked_index(&lock, 1);
+ hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
clean = merge_recursive(&o, head,
remoteheads->item, reversed, &result);
if (clean < 0)
@@ -781,7 +781,7 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
struct commit_list *parents, **pptr = &parents;
static struct lock_file lock;
- hold_locked_index(&lock, 1);
+ hold_locked_index(&lock, LOCK_DIE_ON_ERROR);
refresh_cache(REFRESH_QUIET);
if (active_cache_changed &&
write_locked_index(&the_index, &lock, COMMIT_LOCK))
diff --git a/builtin/mv.c b/builtin/mv.c
index 2f43877bc9..43adf92ba6 100644
--- a/builtin/mv.c
+++ b/builtin/mv.c
@@ -126,7 +126,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
if (--argc < 1)
usage_with_options(builtin_mv_usage, builtin_mv_options);
- hold_locked_index(&lock_file, 1);
+ hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
if (read_cache() < 0)
die(_("index file corrupt"));
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 9bd1fd755e..fa6edb35b2 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -150,7 +150,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
argc = parse_options(argc, argv, unused_prefix, read_tree_options,
read_tree_usage, 0);
- hold_locked_index(&lock_file, 1);
+ hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
prefix_set = opts.prefix ? 1 : 0;
if (1 < opts.merge + opts.reset + prefix_set)
diff --git a/builtin/reset.c b/builtin/reset.c
index c04ac076dc..8ab915bfcb 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -354,7 +354,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (reset_type != SOFT) {
struct lock_file *lock = xcalloc(1, sizeof(*lock));
- hold_locked_index(lock, 1);
+ hold_locked_index(lock, LOCK_DIE_ON_ERROR);
if (reset_type == MIXED) {
int flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;
if (read_from_tree(&pathspec, &oid, intent_to_add))
diff --git a/builtin/rm.c b/builtin/rm.c
index 3f3e24eb36..7f15a3d7f8 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -292,7 +292,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
if (!index_only)
setup_work_tree();
- hold_locked_index(&lock_file, 1);
+ hold_locked_index(&lock_file, LOCK_DIE_ON_ERROR);
if (read_cache() < 0)
die(_("index file corrupt"));
diff --git a/builtin/update-index.c b/builtin/update-index.c
index f3f07e7f1c..d530e89368 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1012,6 +1012,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
/* We can't free this memory, it becomes part of a linked list parsed atexit() */
lock_file = xcalloc(1, sizeof(struct lock_file));
+ /* we will diagnose later if it turns out that we need to update it */
newfd = hold_locked_index(lock_file, 0);
if (newfd < 0)
lock_error = errno;
diff --git a/merge-recursive.c b/merge-recursive.c
index 9041c2f149..8442068716 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2124,7 +2124,7 @@ int merge_recursive_generic(struct merge_options *o,
}
}
- hold_locked_index(lock, 1);
+ hold_locked_index(lock, LOCK_DIE_ON_ERROR);
clean = merge_recursive(o, head_commit, next_commit, ca,
result);
if (clean < 0)
diff --git a/read-cache.c b/read-cache.c
index db5d910642..f92a912dcb 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1425,12 +1425,9 @@ static int read_index_extension(struct index_state *istate,
return 0;
}
-int hold_locked_index(struct lock_file *lk, int die_on_error)
+int hold_locked_index(struct lock_file *lk, int lock_flags)
{
- return hold_lock_file_for_update(lk, get_index_file(),
- die_on_error
- ? LOCK_DIE_ON_ERROR
- : 0);
+ return hold_lock_file_for_update(lk, get_index_file(), lock_flags);
}
int read_index(struct index_state *istate)
diff --git a/rerere.c b/rerere.c
index 5d083ca572..3bd55caf3b 100644
--- a/rerere.c
+++ b/rerere.c
@@ -708,7 +708,7 @@ static void update_paths(struct string_list *update)
{
int i;
- hold_locked_index(&index_lock, 1);
+ hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
for (i = 0; i < update->nr; i++) {
struct string_list_item *item = &update->items[i];
diff --git a/sequencer.c b/sequencer.c
index 30b10ba143..7fc1e2a5df 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -370,7 +370,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
char **xopt;
static struct lock_file index_lock;
- hold_locked_index(&index_lock, 1);
+ hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
read_cache();
diff --git a/t/helper/test-scrap-cache-tree.c b/t/helper/test-scrap-cache-tree.c
index 27fe0405b8..d2a63bea43 100644
--- a/t/helper/test-scrap-cache-tree.c
+++ b/t/helper/test-scrap-cache-tree.c
@@ -8,7 +8,7 @@ static struct lock_file index_lock;
int cmd_main(int ac, const char **av)
{
setup_git_directory();
- hold_locked_index(&index_lock, 1);
+ hold_locked_index(&index_lock, LOCK_DIE_ON_ERROR);
if (read_cache() < 0)
die("unable to read index file");
active_cache_tree = NULL;
--
2.11.0-274-g0631465056
^ permalink raw reply related
* [PATCH 1/3] wt-status: implement opportunisitc index update correctly
From: Junio C Hamano @ 2016-12-07 19:41 UTC (permalink / raw)
To: git; +Cc: Paul Tan
In-Reply-To: <20161207194105.25780-1-gitster@pobox.com>
The require_clean_work_tree() function calls hold_locked_index()
with die_on_error=0 to signal that it is OK if it fails to obtain
the lock, but unconditionally calls update_index_if_able(), which
will try to write into fd=-1.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
wt-status.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/wt-status.c b/wt-status.c
index a2e9d332d8..a715e71906 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2258,11 +2258,12 @@ int has_uncommitted_changes(int ignore_submodules)
int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)
{
struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
- int err = 0;
+ int err = 0, fd;
- hold_locked_index(lock_file, 0);
+ fd = hold_locked_index(lock_file, 0);
refresh_cache(REFRESH_QUIET);
- update_index_if_able(&the_index, lock_file);
+ if (0 <= fd)
+ update_index_if_able(&the_index, lock_file);
rollback_lock_file(lock_file);
if (has_unstaged_changes(ignore_submodules)) {
--
2.11.0-274-g0631465056
^ permalink raw reply related
* [PATCH 0/3] Do not be totally silent upon lock error
From: Junio C Hamano @ 2016-12-07 19:41 UTC (permalink / raw)
To: git; +Cc: Robbie Iannucci, Johannes Schindelin
In-Reply-To: <xmqqd1h3y506.fsf@gitster.mtv.corp.google.com>
Robbie Iannucci reported that "git merge" that fast-forwards fails
silently when a competing or stale index.lock is present in recent
Git:
$ git merge --ff-only master; echo $?
Updating 454cb6bd52..8d7a455ed5
1
$ exit
Did the update happen? We used to give "fatal: Unable to create
..." followed by "Another git process seems to be running..."
advice, and that would have helped the user from the confusion.
This was because there were only two choices available to the
callers of the lockfile API--they can either ask it to die with
message when the lock cannot be acquired, or ask it to silently
return -1 to signal an error. The recent "libify sequencer" topic
turned many existing "please die" calls to "just silently return
-1", and while it added new "unable to lock" error messages to most
of them, one spot didn't get any and now is allowed to just die
silently.
This series should fix it:
- 1/3 is something I noticed while reading the existing callers of
the lockfile API, and is not a new issue with "libify sequencer"
topic.
- 2/3 gives a new third option to callers of the lockfile API; they
can now say "please emit the usual error message upon failing to
acquire the lock, but give control back to me".
- 3/3 uses the new option to resurrect the message.
Junio C Hamano (3):
wt-status: implement opportunisitc index update correctly
hold_locked_index(): align error handling with hold_lockfile_for_update()
lockfile: LOCK_REPORT_ON_ERROR
apply.c | 2 +-
builtin/add.c | 2 +-
builtin/am.c | 6 +++---
builtin/checkout-index.c | 2 +-
builtin/checkout.c | 4 ++--
builtin/clone.c | 2 +-
builtin/commit.c | 8 ++++----
builtin/merge.c | 6 +++---
builtin/mv.c | 2 +-
builtin/read-tree.c | 2 +-
builtin/reset.c | 2 +-
builtin/rm.c | 2 +-
builtin/update-index.c | 1 +
lockfile.c | 12 ++++++++++--
lockfile.h | 8 +++++++-
merge-recursive.c | 2 +-
merge.c | 2 +-
read-cache.c | 7 ++-----
rerere.c | 2 +-
sequencer.c | 2 +-
t/helper/test-scrap-cache-tree.c | 2 +-
wt-status.c | 7 ++++---
22 files changed, 49 insertions(+), 36 deletions(-)
--
2.11.0-274-g0631465056
^ 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