From 48988c4d0c35b5c569a2645b61c9346f6062021d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 30 Mar 2018 14:34:46 -0400 Subject: set_git_dir: die when setenv() fails The set_git_dir() function returns an error if setenv() fails, but there are zero callers who pay attention to this return value. If this ever were to happen, it could cause confusing results, as sub-processes would see a potentially stale GIT_DIR (e.g., if it is relative and we chdir()-ed to the root of the working tree). We _could_ try to fix each caller, but there's really nothing useful to do after this failure except die. Let's just lump setenv() failure into the same category as malloc failure: things that should never happen and cause us to abort catastrophically. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/cache.h b/cache.h index a61b2d3..5c24394 100644 --- a/cache.h +++ b/cache.h @@ -477,7 +477,7 @@ extern const char *get_git_common_dir(void); extern char *get_object_directory(void); extern char *get_index_file(void); extern char *get_graft_file(void); -extern int set_git_dir(const char *path); +extern void set_git_dir(const char *path); extern int get_common_dir_noenv(struct strbuf *sb, const char *gitdir); extern int get_common_dir(struct strbuf *sb, const char *gitdir); extern const char *get_git_namespace(void); diff --git a/environment.c b/environment.c index d6dd646..e01acf8 100644 --- a/environment.c +++ b/environment.c @@ -296,13 +296,12 @@ char *get_graft_file(void) return the_repository->graft_file; } -int set_git_dir(const char *path) +void set_git_dir(const char *path) { if (setenv(GIT_DIR_ENVIRONMENT, path, 1)) - return error("Could not set GIT_DIR to '%s'", path); + die("could not set GIT_DIR to '%s'", path); repo_set_gitdir(the_repository, path); setup_git_env(); - return 0; } const char *get_log_output_encoding(void) -- cgit v0.10.2-6-g49f6 From cb50761959c0c30aa9c44098db3a72db06570238 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Fri, 30 Mar 2018 14:34:59 -0400 Subject: trace.c: export trace_setup_key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is so that we can print traces based on this key outside trace.c. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/trace.c b/trace.c index 7f3b08e..fc623e9 100644 --- a/trace.c +++ b/trace.c @@ -26,6 +26,7 @@ struct trace_key trace_default_key = { "GIT_TRACE", 0, 0, 0 }; struct trace_key trace_perf_key = TRACE_KEY_INIT(PERFORMANCE); +struct trace_key trace_setup_key = TRACE_KEY_INIT(SETUP); /* Get a trace file descriptor from "key" env variable. */ static int get_trace_fd(struct trace_key *key) @@ -300,11 +301,10 @@ static const char *quote_crnl(const char *path) /* FIXME: move prefix to startup_info struct and get rid of this arg */ void trace_repo_setup(const char *prefix) { - static struct trace_key key = TRACE_KEY_INIT(SETUP); const char *git_work_tree; char *cwd; - if (!trace_want(&key)) + if (!trace_want(&trace_setup_key)) return; cwd = xgetcwd(); @@ -315,11 +315,11 @@ void trace_repo_setup(const char *prefix) if (!prefix) prefix = "(null)"; - trace_printf_key(&key, "setup: git_dir: %s\n", quote_crnl(get_git_dir())); - trace_printf_key(&key, "setup: git_common_dir: %s\n", quote_crnl(get_git_common_dir())); - trace_printf_key(&key, "setup: worktree: %s\n", quote_crnl(git_work_tree)); - trace_printf_key(&key, "setup: cwd: %s\n", quote_crnl(cwd)); - trace_printf_key(&key, "setup: prefix: %s\n", quote_crnl(prefix)); + trace_printf_key(&trace_setup_key, "setup: git_dir: %s\n", quote_crnl(get_git_dir())); + trace_printf_key(&trace_setup_key, "setup: git_common_dir: %s\n", quote_crnl(get_git_common_dir())); + trace_printf_key(&trace_setup_key, "setup: worktree: %s\n", quote_crnl(git_work_tree)); + trace_printf_key(&trace_setup_key, "setup: cwd: %s\n", quote_crnl(cwd)); + trace_printf_key(&trace_setup_key, "setup: prefix: %s\n", quote_crnl(prefix)); free(cwd); } diff --git a/trace.h b/trace.h index 88055ab..2b6a1bc 100644 --- a/trace.h +++ b/trace.h @@ -15,6 +15,7 @@ extern struct trace_key trace_default_key; #define TRACE_KEY_INIT(name) { "GIT_TRACE_" #name, 0, 0, 0 } extern struct trace_key trace_perf_key; +extern struct trace_key trace_setup_key; extern void trace_repo_setup(const char *prefix); extern int trace_want(struct trace_key *key); -- cgit v0.10.2-6-g49f6 From 2b5ed373656c225d2baea09da130531fe7275fa2 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 30 Mar 2018 14:35:04 -0400 Subject: add chdir-notify API If one part of the code does a permanent chdir(), then this invalidates any relative paths that may be held by other parts of the code. For example, setup_work_tree() moves us to the top of the working tree, which may invalidate a previously stored relative gitdir. We've hacked around this case by teaching setup_work_tree() to re-run set_git_dir() with an adjusted path, but this stomps all over the idea of module boundaries. setup_work_tree() shouldn't have to know all of the places that need to be fed an adjusted path. And indeed, there's at least one other place (the refs code) which needs adjusting. Let's provide an API to let code that stores relative paths "subscribe" to updates to the current working directory. This means that callers of chdir() don't need to know about all subscribers ahead of time; they can simply consult a dynamically built list. Note that our helper function to reparent relative paths uses the simple remove_leading_path(). We could in theory use the much smarter relative_path(), but that led to some problems as described in 41894ae3a3 (Use simpler relative_path when set_git_dir, 2013-10-14). Since we're aiming to replace the setup_work_tree() code here, let's follow its lead. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/Makefile b/Makefile index a1d8775..0da98b9 100644 --- a/Makefile +++ b/Makefile @@ -772,6 +772,7 @@ LIB_OBJS += branch.o LIB_OBJS += bulk-checkin.o LIB_OBJS += bundle.o LIB_OBJS += cache-tree.o +LIB_OBJS += chdir-notify.o LIB_OBJS += checkout.o LIB_OBJS += color.o LIB_OBJS += column.o diff --git a/chdir-notify.c b/chdir-notify.c new file mode 100644 index 0000000..5f7f2c2 --- /dev/null +++ b/chdir-notify.c @@ -0,0 +1,93 @@ +#include "cache.h" +#include "chdir-notify.h" +#include "list.h" +#include "strbuf.h" + +struct chdir_notify_entry { + const char *name; + chdir_notify_callback cb; + void *data; + struct list_head list; +}; +static LIST_HEAD(chdir_notify_entries); + +void chdir_notify_register(const char *name, + chdir_notify_callback cb, + void *data) +{ + struct chdir_notify_entry *e = xmalloc(sizeof(*e)); + e->name = name; + e->cb = cb; + e->data = data; + list_add_tail(&e->list, &chdir_notify_entries); +} + +static void reparent_cb(const char *name, + const char *old_cwd, + const char *new_cwd, + void *data) +{ + char **path = data; + char *tmp = *path; + + if (!tmp) + return; + + *path = reparent_relative_path(old_cwd, new_cwd, tmp); + free(tmp); + + if (name) { + trace_printf_key(&trace_setup_key, + "setup: reparent %s to '%s'", + name, *path); + } +} + +void chdir_notify_reparent(const char *name, char **path) +{ + chdir_notify_register(name, reparent_cb, path); +} + +int chdir_notify(const char *new_cwd) +{ + struct strbuf old_cwd = STRBUF_INIT; + struct list_head *pos; + + if (strbuf_getcwd(&old_cwd) < 0) + return -1; + if (chdir(new_cwd) < 0) { + int saved_errno = errno; + strbuf_release(&old_cwd); + errno = saved_errno; + return -1; + } + + trace_printf_key(&trace_setup_key, + "setup: chdir from '%s' to '%s'", + old_cwd.buf, new_cwd); + + list_for_each(pos, &chdir_notify_entries) { + struct chdir_notify_entry *e = + list_entry(pos, struct chdir_notify_entry, list); + e->cb(e->name, old_cwd.buf, new_cwd, e->data); + } + + strbuf_release(&old_cwd); + return 0; +} + +char *reparent_relative_path(const char *old_cwd, + const char *new_cwd, + const char *path) +{ + char *ret, *full; + + if (is_absolute_path(path)) + return xstrdup(path); + + full = xstrfmt("%s/%s", old_cwd, path); + ret = xstrdup(remove_leading_path(full, new_cwd)); + free(full); + + return ret; +} diff --git a/chdir-notify.h b/chdir-notify.h new file mode 100644 index 0000000..366e4c1 --- /dev/null +++ b/chdir-notify.h @@ -0,0 +1,73 @@ +#ifndef CHDIR_NOTIFY_H +#define CHDIR_NOTIFY_H + +/* + * An API to let code "subscribe" to changes to the current working directory. + * The general idea is that some code asks to be notified when the working + * directory changes, and other code that calls chdir uses a special wrapper + * that notifies everyone. + */ + +/* + * Callers who need to know about changes can do: + * + * void foo(const char *old_path, const char *new_path, void *data) + * { + * warning("switched from %s to %s!", old_path, new_path); + * } + * ... + * chdir_notify_register("description", foo, data); + * + * In practice most callers will want to move a relative path to the new root; + * they can use the reparent_relative_path() helper for that. If that's all + * you're doing, you can also use the convenience function: + * + * chdir_notify_reparent("description", &my_path); + * + * Whenever a chdir event occurs, that will update my_path (if it's relative) + * to adjust for the new cwd by freeing any existing string and allocating a + * new one. + * + * Registered functions are called in the order in which they were added. Note + * that there's currently no way to remove a function, so make sure that the + * data parameter remains valid for the rest of the program. + * + * The "name" argument is used only for printing trace output from + * $GIT_TRACE_SETUP. It may be NULL, but if non-NULL should point to + * storage which lasts as long as the registration is active. + */ +typedef void (*chdir_notify_callback)(const char *name, + const char *old_cwd, + const char *new_cwd, + void *data); +void chdir_notify_register(const char *name, chdir_notify_callback cb, void *data); +void chdir_notify_reparent(const char *name, char **path); + +/* + * + * Callers that want to chdir: + * + * chdir_notify(new_path); + * + * to switch to the new path and notify any callbacks. + * + * Note that you don't need to chdir_notify() if you're just temporarily moving + * to a directory and back, as long as you don't call any subscribed code in + * between (but it should be safe to do so if you're unsure). + */ +int chdir_notify(const char *new_cwd); + +/* + * Reparent a relative path from old_root to new_root. For example: + * + * reparent_relative_path("/a", "/a/b", "b/rel"); + * + * would return the (newly allocated) string "rel". Note that we may return an + * absolute path in some cases (e.g., if the resulting path is not inside + * new_cwd). + */ +char *reparent_relative_path(const char *old_cwd, + const char *new_cwd, + const char *path); + +#endif /* CHDIR_NOTIFY_H */ -- cgit v0.10.2-6-g49f6 From 8500e0de3fe2e33ba503d432a4a5301ce2fb60fa Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 30 Mar 2018 14:35:08 -0400 Subject: set_work_tree: use chdir_notify When we change to the top of the working tree, we manually re-adjust $GIT_DIR and call set_git_dir() again, in order to update any relative git-dir we'd compute earlier. Instead of the work-tree code having to know to call the git-dir code, let's use the new chdir_notify interface. There are two spots that need updating, with a few subtleties in each: 1. the set_git_dir() code needs to chdir_notify_register() so it can be told when to update its path. Technically we could push this down into repo_set_gitdir(), so that even repository structs besides the_repository could benefit from this. But that opens up a lot of complications: - we'd still need to touch set_git_dir(), because it does some other setup (like setting $GIT_DIR in the environment) - submodules using other repository structs get cleaned up, which means we'd need to remove them from the chdir_notify list - it's unlikely to fix any bugs, since we shouldn't generally chdir() in the middle of working on a submodule 2. setup_work_tree now needs to call chdir_notify(), and can lose its manual set_git_dir() call. Note that at first glance it looks like this undoes the absolute-to-relative optimization added by 044bbbcb63 (Make git_dir a path relative to work_tree in setup_work_tree(), 2008-06-19). But for the most part that optimization was just _undoing_ the relative-to-absolute conversion which the function was doing earlier (and which is now gone). It is true that if you already have an absolute git_dir that the setup_work_tree() function will no longer make it relative as a side effect. But: - we generally do have relative git-dir's due to the way the discovery code works - if we really care about making git-dir's relative when possible, then we should be relativizing them earlier (e.g., when we see an absolute $GIT_DIR we could turn it relative, whether we are going to chdir into a worktree or not). That would cover all cases, including ones that 044bbbcb63 did not. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/environment.c b/environment.c index e01acf8..903a6c9 100644 --- a/environment.c +++ b/environment.c @@ -13,6 +13,7 @@ #include "refs.h" #include "fmt-merge-msg.h" #include "commit.h" +#include "chdir-notify.h" int trust_executable_bit = 1; int trust_ctime = 1; @@ -296,7 +297,7 @@ char *get_graft_file(void) return the_repository->graft_file; } -void set_git_dir(const char *path) +static void set_git_dir_1(const char *path) { if (setenv(GIT_DIR_ENVIRONMENT, path, 1)) die("could not set GIT_DIR to '%s'", path); @@ -304,6 +305,26 @@ void set_git_dir(const char *path) setup_git_env(); } +static void update_relative_gitdir(const char *name, + const char *old_cwd, + const char *new_cwd, + void *data) +{ + char *path = reparent_relative_path(old_cwd, new_cwd, get_git_dir()); + trace_printf_key(&trace_setup_key, + "setup: move $GIT_DIR to '%s'", + path); + set_git_dir_1(path); + free(path); +} + +void set_git_dir(const char *path) +{ + set_git_dir_1(path); + if (!is_absolute_path(path)) + chdir_notify_register(NULL, update_relative_gitdir, NULL); +} + const char *get_log_output_encoding(void) { return git_log_output_encoding ? git_log_output_encoding diff --git a/setup.c b/setup.c index 7287779..9eb2e80 100644 --- a/setup.c +++ b/setup.c @@ -3,6 +3,7 @@ #include "config.h" #include "dir.h" #include "string-list.h" +#include "chdir-notify.h" static int inside_git_dir = -1; static int inside_work_tree = -1; @@ -378,7 +379,7 @@ int is_inside_work_tree(void) void setup_work_tree(void) { - const char *work_tree, *git_dir; + const char *work_tree; static int initialized = 0; if (initialized) @@ -388,10 +389,7 @@ void setup_work_tree(void) die(_("unable to set up work tree using invalid config")); work_tree = get_git_work_tree(); - git_dir = get_git_dir(); - if (!is_absolute_path(git_dir)) - git_dir = real_path(get_git_dir()); - if (!work_tree || chdir(work_tree)) + if (!work_tree || chdir_notify(work_tree)) die(_("this operation must be run in a work tree")); /* @@ -401,7 +399,6 @@ void setup_work_tree(void) if (getenv(GIT_WORK_TREE_ENVIRONMENT)) setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1); - set_git_dir(remove_leading_path(git_dir, work_tree)); initialized = 1; } -- cgit v0.10.2-6-g49f6 From fb9c2d27039a23457cb9710d86e00c51dfb910dc Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 30 Mar 2018 14:35:12 -0400 Subject: refs: use chdir_notify to update cached relative paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit f57f37e2e1 (files-backend: remove the use of git_path(), 2017-03-26) introduced a regression when a relative $GIT_DIR is used in a working tree: - when we initialize the ref backend, we make a copy of get_git_dir(), which may be relative - later, we may call setup_work_tree() and chdir to the root of the working tree - further calls to the ref code will use the stored git directory, but relative paths will now point to the wrong place The new test in t1501 demonstrates one such instance (the bug causes us to write the ref update to the nonsense "relative/relative/.git"). Since setup_work_tree() now uses chdir_notify, we can just ask it update our relative paths when necessary. Reported-by: Rafael Ascensao Helped-by: Nguyễn Thái Ngọc Duy Signed-off-by: Jeff King Signed-off-by: Junio C Hamano diff --git a/refs/files-backend.c b/refs/files-backend.c index bec8e30..a92a2aa 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -9,6 +9,7 @@ #include "../lockfile.h" #include "../object.h" #include "../dir.h" +#include "../chdir-notify.h" /* * This backend uses the following flags in `ref_update::flags` for @@ -106,6 +107,11 @@ static struct ref_store *files_ref_store_create(const char *gitdir, refs->packed_ref_store = packed_ref_store_create(sb.buf, flags); strbuf_release(&sb); + chdir_notify_reparent("files-backend $GIT_DIR", + &refs->gitdir); + chdir_notify_reparent("files-backend $GIT_COMMONDIR", + &refs->gitcommondir); + return ref_store; } diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 65288c6..369c34f 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -5,6 +5,7 @@ #include "packed-backend.h" #include "../iterator.h" #include "../lockfile.h" +#include "../chdir-notify.h" enum mmap_strategy { /* @@ -202,6 +203,8 @@ struct ref_store *packed_ref_store_create(const char *path, refs->store_flags = store_flags; refs->path = xstrdup(path); + chdir_notify_reparent("packed-refs", &refs->path); + return ref_store; } diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh index b06210e..a5db533 100755 --- a/t/t1501-work-tree.sh +++ b/t/t1501-work-tree.sh @@ -431,4 +431,16 @@ test_expect_success 'error out gracefully on invalid $GIT_WORK_TREE' ' ) ' +test_expect_success 'refs work with relative gitdir and work tree' ' + git init relative && + git -C relative commit --allow-empty -m one && + git -C relative commit --allow-empty -m two && + + GIT_DIR=relative/.git GIT_WORK_TREE=relative git reset HEAD^ && + + git -C relative log -1 --format=%s >actual && + echo one >expect && + test_cmp expect actual +' + test_done -- cgit v0.10.2-6-g49f6