* git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. @ 2018-03-26 21:27 Rafael Ascensao 2018-03-26 21:44 ` Ævar Arnfjörð Bjarmason 2018-03-27 6:31 ` Jeff King 0 siblings, 2 replies; 54+ messages in thread From: Rafael Ascensao @ 2018-03-26 21:27 UTC (permalink / raw) To: Git Mailing List One of the tools that manages PKGBUILDS for Arch Linux stores PKGBUILD git repos inside a cache directory for reuse. One of the repos is triggering some unexpected behaviour that can be reproduced in the CLI with: $ GIT_DIR=spotify/.git GIT_WORK_TREE=spotify git reset HEAD fatal: couldn't read spotify/.git/packed-refs: Not a directory Moving up one directory works as expected. $ cd .. $ GIT_DIR=sync/spotify/.git GIT_WORK_TREE=sync/spotify git reset HEAD Using -C seems to work fine too. $ git -C spotify reset HEAD $ GIT_TRACE_SETUP=2 GIT_TRACE=2 GIT_DIR="spotify/.git" GIT_WORK_TREE="spotify" git reset HEAD 22:10:53.020525 trace.c:315 setup: git_dir: spotify/.git 22:10:53.020605 trace.c:316 setup: git_common_dir: spotify/.git 22:10:53.020616 trace.c:317 setup: worktree: /home/rafasc/.cache/aurutils/sync/spotify 22:10:53.020625 trace.c:318 setup: cwd: /home/rafasc/.cache/aurutils/sync 22:10:53.020635 trace.c:319 setup: prefix: (null) 22:10:53.020644 git.c:344 trace: built-in: git 'reset' 'HEAD' fatal: couldn't read spotify/.git/packed-refs: Not a directory The issue seems to bisect to f57f37e2e1bf11ab4cdfd221ad47e961ba9353a0 I can't pinpoint why this particular repo is behaving differently. Cumprimentos, Rafael Ascensão ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-26 21:27 git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars Rafael Ascensao @ 2018-03-26 21:44 ` Ævar Arnfjörð Bjarmason 2018-03-27 6:31 ` Jeff King 1 sibling, 0 replies; 54+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-03-26 21:44 UTC (permalink / raw) To: Rafael Ascensao; +Cc: Git Mailing List, Nguyễn Thái Ngọc Duy On Mon, Mar 26 2018, Rafael Ascensao wrote: > One of the tools that manages PKGBUILDS for Arch Linux stores PKGBUILD > git repos inside a cache directory for reuse. > > One of the repos is triggering some unexpected behaviour that can be > reproduced in the CLI with: > > $ GIT_DIR=spotify/.git GIT_WORK_TREE=spotify git reset HEAD > fatal: couldn't read spotify/.git/packed-refs: Not a directory > > Moving up one directory works as expected. > $ cd .. > $ GIT_DIR=sync/spotify/.git GIT_WORK_TREE=sync/spotify git reset HEAD > > Using -C seems to work fine too. > $ git -C spotify reset HEAD > > $ GIT_TRACE_SETUP=2 GIT_TRACE=2 GIT_DIR="spotify/.git" > GIT_WORK_TREE="spotify" git reset HEAD > 22:10:53.020525 trace.c:315 setup: git_dir: spotify/.git > 22:10:53.020605 trace.c:316 setup: git_common_dir: spotify/.git > 22:10:53.020616 trace.c:317 setup: worktree: > /home/rafasc/.cache/aurutils/sync/spotify > 22:10:53.020625 trace.c:318 setup: cwd: > /home/rafasc/.cache/aurutils/sync > 22:10:53.020635 trace.c:319 setup: prefix: (null) > 22:10:53.020644 git.c:344 trace: built-in: git 'reset' 'HEAD' > fatal: couldn't read spotify/.git/packed-refs: Not a directory > > The issue seems to bisect to f57f37e2e1bf11ab4cdfd221ad47e961ba9353a0 > I can't pinpoint why this particular repo is behaving differently. CC-ing Duy, the author of that commit. ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-26 21:27 git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars Rafael Ascensao 2018-03-26 21:44 ` Ævar Arnfjörð Bjarmason @ 2018-03-27 6:31 ` Jeff King 2018-03-27 14:56 ` Duy Nguyen 1 sibling, 1 reply; 54+ messages in thread From: Jeff King @ 2018-03-27 6:31 UTC (permalink / raw) To: Rafael Ascensao; +Cc: Nguyễn Thái Ngọc Duy, Git Mailing List On Mon, Mar 26, 2018 at 10:27:09PM +0100, Rafael Ascensao wrote: > One of the tools that manages PKGBUILDS for Arch Linux stores PKGBUILD > git repos inside a cache directory for reuse. > > One of the repos is triggering some unexpected behaviour that can be > reproduced in the CLI with: > > $ GIT_DIR=spotify/.git GIT_WORK_TREE=spotify git reset HEAD > fatal: couldn't read spotify/.git/packed-refs: Not a directory > [...] > The issue seems to bisect to f57f37e2e1bf11ab4cdfd221ad47e961ba9353a0 > I can't pinpoint why this particular repo is behaving differently. I think we're getting confused by the relative paths. Here's a related reproduction: $ git init repo $ git -C repo commit --allow-empty -m foo $ GIT_DIR=repo/.git GIT_WORK_TREE=repo git reset HEAD $ find repo/repo repo/repo repo/repo/.git repo/repo/.git/logs repo/repo/.git/logs/HEAD repo/repo/.git/HEAD Er, what? It looks like we kept looking at "repo/.git" as our git directory, even though we should have normalized it into an absolute path after moving into the root of the work-tree. I can also reproduce your exact error by inserting: git -C repo pack-refs echo whatever >repo/repo before the call to "git reset" (and then we get ENOTDIR trying to read the packed-refs file, because the file "repo" is in the way). Looking at f57f37e2, I think the problem is that files_ref_store_create() saves the value of get_git_dir() at that point. But later after we chdir for the working tree, presumably it's updated, but we continue to use the out-dated relative path. So one "fix" is something like this: diff --git a/refs.c b/refs.c index 20ba82b434..449bdf2437 100644 --- a/refs.c +++ b/refs.c @@ -1643,11 +1643,14 @@ static struct ref_store *ref_store_init(const char *gitdir, const char *be_name = "files"; struct ref_storage_be *be = find_ref_storage_backend(be_name); struct ref_store *refs; + char *abs_gitdir; if (!be) die("BUG: reference backend %s is unknown", be_name); - refs = be->init(gitdir, flags); + abs_gitdir = absolute_pathdup(gitdir); + refs = be->init(abs_gitdir, flags); + free(abs_gitdir); return refs; } But that really feels like we're papering over the problem. It's not clear to me exactly what f57f37e2 is trying to accomplish, and whether it would work for it to look call get_git_dir() whenever it needed the path. -Peff ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-27 6:31 ` Jeff King @ 2018-03-27 14:56 ` Duy Nguyen 2018-03-27 16:47 ` Jeff King 0 siblings, 1 reply; 54+ messages in thread From: Duy Nguyen @ 2018-03-27 14:56 UTC (permalink / raw) To: Jeff King; +Cc: Rafael Ascensao, Git Mailing List On Tue, Mar 27, 2018 at 8:31 AM, Jeff King <peff@peff.net> wrote: >... > > But that really feels like we're papering over the problem. It is papering over the problem in my opinion. setup_work_tree() is involved here and when it moves cwd around, it re-init git-dir (and all other paths). Before my patch we call git_path() only when we need it and git-dir is likely already updated by setup_work_tree(). After this patch, the path is set in stone before setup_work_tree() kicks in. Once it moves cwd, relative paths all become invalid. The way setup_work_tree() does it is just bad to me. When it calls set_git_dir() again, the assumption is the new path is exactly the same as the old one. The only difference is relative vs absolute. But this is super hard to see from set_git_dir implementation. The new struct repository design also inherits this (i.e. allowing to call set_git_dir multiple times, which really does not make sense), but this won't fly long term. When cwd moves, all cached relative paths must be adjusted, we need a better mechanism to tell everybody that, not just do it for $GIT_DIR and related paths. I am planning to fix this. This is part of the "setup cleanup" effort to keep repository.c design less messy and hopefully unify how the main repo and submodule repo are created/set up. But frankly that may take a long while before I could submit anything substantial (I also have the "report multiple worktree's HEADs correctly and make fsck count all HEADs" task, which is not small and to me have higher priority). So I would not mind papering over it right now (with an understanding that absolute path pays some more overhead in path walking, which was th reason we tried to avoid it in setup code). A slightly better patch is trigger this path absolutization from setup_work_tree(), near set_git_dir(). But then you face some ugliness there: how to find out all ref stores to update, or just update the main ref store alone. > It's not > clear to me exactly what f57f37e2 is trying to accomplish, and whether > it would work for it to look call get_git_dir() whenever it needed the > path. > > -Peff -- Duy ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-27 14:56 ` Duy Nguyen @ 2018-03-27 16:47 ` Jeff King 2018-03-27 17:09 ` Duy Nguyen 0 siblings, 1 reply; 54+ messages in thread From: Jeff King @ 2018-03-27 16:47 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List On Tue, Mar 27, 2018 at 04:56:00PM +0200, Duy Nguyen wrote: > The way setup_work_tree() does it is just bad to me. When it calls > set_git_dir() again, the assumption is the new path is exactly the > same as the old one. The only difference is relative vs absolute. But > this is super hard to see from set_git_dir implementation. The new > struct repository design also inherits this (i.e. allowing to call > set_git_dir multiple times, which really does not make sense), but > this won't fly long term. When cwd moves, all cached relative paths > must be adjusted, we need a better mechanism to tell everybody that, > not just do it for $GIT_DIR and related paths. Yeah, I agree it's questionable. > I am planning to fix this. This is part of the "setup cleanup" effort > to keep repository.c design less messy and hopefully unify how the > main repo and submodule repo are created/set up. But frankly that may > take a long while before I could submit anything substantial (I also > have the "report multiple worktree's HEADs correctly and make fsck > count all HEADs" task, which is not small and to me have higher > priority). > > So I would not mind papering over it right now (with an understanding > that absolute path pays some more overhead in path walking, which was > th reason we tried to avoid it in setup code). A slightly better patch > is trigger this path absolutization from setup_work_tree(), near > set_git_dir(). But then you face some ugliness there: how to find out > all ref stores to update, or just update the main ref store alone. I don't quite get why f57f37e2 doesn't want to call git_path(). Is it to avoid the way the path is munged? Or is it to avoid some lazy-setup that is triggered by calling get_git_dir() at all (which doesn't make much sense to me, because we'd already have called get_git_dir() much earlier). Or is it just because we may sometimes fill in refs->git_dir with something besides get_git_dir() (for a submodule or worktree or something)? I.e., can we do one of (depending on which of those answers is "yes"): 1. Stop caching the value of get_git_dir(), and instead call it on-demand instead of looking at refs->git_dir? (If we just want to avoid git_path()). 2. If we need to avoid even calling get_git_dir(), can we add a "light" version of it that avoids whatever side effects we're trying to skip? 3. If the problem is just that sometimes we need get_git_dir() and sometimes not, could we perhaps store NULL as a sentinel to mean "look up get_git_dir() when you need it"? That would let submodules and worktrees fill in their paths as necessary (assuming they never change after init), but handle the case of get_git_dir() changing. Hmm. Typing that out, it seems like (3) is probably the right path. Something like the patch below seems to fix it and passes the tests. diff --git a/refs.c b/refs.c index 20ba82b434..4094f0e7d4 100644 --- a/refs.c +++ b/refs.c @@ -1656,7 +1656,7 @@ struct ref_store *get_main_ref_store(void) if (main_ref_store) return main_ref_store; - main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS); + main_ref_store = ref_store_init(NULL, REF_STORE_ALL_CAPS); return main_ref_store; } diff --git a/refs/files-backend.c b/refs/files-backend.c index bec8e30e9e..22118b5764 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -69,6 +69,7 @@ struct files_ref_store { struct ref_store base; unsigned int store_flags; + /* may be NULL to force lookup of get_git_dir() */ char *gitdir; char *gitcommondir; @@ -94,17 +95,23 @@ static struct ref_store *files_ref_store_create(const char *gitdir, { struct files_ref_store *refs = xcalloc(1, sizeof(*refs)); struct ref_store *ref_store = (struct ref_store *)refs; - struct strbuf sb = STRBUF_INIT; base_ref_store_init(ref_store, &refs_be_files); refs->store_flags = flags; - refs->gitdir = xstrdup(gitdir); - get_common_dir_noenv(&sb, gitdir); - refs->gitcommondir = strbuf_detach(&sb, NULL); - strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir); - refs->packed_ref_store = packed_ref_store_create(sb.buf, flags); - strbuf_release(&sb); + if (gitdir) { + struct strbuf sb = STRBUF_INIT; + refs->gitdir = xstrdup(gitdir); + get_common_dir_noenv(&sb, gitdir); + refs->gitcommondir = strbuf_detach(&sb, NULL); + strbuf_addf(&sb, "%s/packed-refs", refs->gitcommondir); + refs->packed_ref_store = packed_ref_store_create(sb.buf, flags); + strbuf_release(&sb); + } else { + refs->gitdir = NULL; + refs->gitcommondir = NULL; + refs->packed_ref_store = packed_ref_store_create(NULL, flags); + } return ref_store; } @@ -147,6 +154,16 @@ static struct files_ref_store *files_downcast(struct ref_store *ref_store, return refs; } +static const char *files_gitdir(const struct files_ref_store *refs) +{ + return refs->gitdir ? refs->gitdir : get_git_dir(); +} + +static const char *files_gitcommondir(const struct files_ref_store *refs) +{ + return refs->gitcommondir ? refs->gitcommondir : get_git_common_dir(); +} + static void files_reflog_path(struct files_ref_store *refs, struct strbuf *sb, const char *refname) @@ -154,10 +171,10 @@ static void files_reflog_path(struct files_ref_store *refs, switch (ref_type(refname)) { case REF_TYPE_PER_WORKTREE: case REF_TYPE_PSEUDOREF: - strbuf_addf(sb, "%s/logs/%s", refs->gitdir, refname); + strbuf_addf(sb, "%s/logs/%s", files_gitdir(refs), refname); break; case REF_TYPE_NORMAL: - strbuf_addf(sb, "%s/logs/%s", refs->gitcommondir, refname); + strbuf_addf(sb, "%s/logs/%s", files_gitcommondir(refs), refname); break; default: die("BUG: unknown ref type %d of ref %s", @@ -172,10 +189,10 @@ static void files_ref_path(struct files_ref_store *refs, switch (ref_type(refname)) { case REF_TYPE_PER_WORKTREE: case REF_TYPE_PSEUDOREF: - strbuf_addf(sb, "%s/%s", refs->gitdir, refname); + strbuf_addf(sb, "%s/%s", files_gitdir(refs), refname); break; case REF_TYPE_NORMAL: - strbuf_addf(sb, "%s/%s", refs->gitcommondir, refname); + strbuf_addf(sb, "%s/%s", files_gitcommondir(refs), refname); break; default: die("BUG: unknown ref type %d of ref %s", @@ -2151,13 +2168,13 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st files_downcast(ref_store, REF_STORE_READ, "reflog_iterator_begin"); - if (!strcmp(refs->gitdir, refs->gitcommondir)) { - return reflog_iterator_begin(ref_store, refs->gitcommondir); + if (!strcmp(files_gitdir(refs), files_gitcommondir(refs))) { + return reflog_iterator_begin(ref_store, files_gitcommondir(refs)); } else { return merge_ref_iterator_begin( 0, - reflog_iterator_begin(ref_store, refs->gitdir), - reflog_iterator_begin(ref_store, refs->gitcommondir), + reflog_iterator_begin(ref_store, files_gitdir(refs)), + reflog_iterator_begin(ref_store, files_gitcommondir(refs)), reflog_iterator_select, refs); } } diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 65288c6472..e5a5af1285 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -158,6 +158,16 @@ static void acquire_snapshot(struct snapshot *snapshot) snapshot->referrers++; } +static const char *packed_refs_path(const struct packed_ref_store *refs) +{ + return refs->path ? refs->path : git_path("packed-refs"); +} + +static const char *snapshot_path(const struct snapshot *snapshot) +{ + return packed_refs_path(snapshot->refs); +} + /* * If the buffer in `snapshot` is active, then either munmap the * memory and close the file, or free the memory. Then set the buffer @@ -168,7 +178,7 @@ static void clear_snapshot_buffer(struct snapshot *snapshot) if (snapshot->mmapped) { if (munmap(snapshot->buf, snapshot->eof - snapshot->buf)) die_errno("error ummapping packed-refs file %s", - snapshot->refs->path); + snapshot_path(snapshot)); snapshot->mmapped = 0; } else { free(snapshot->buf); @@ -201,7 +211,7 @@ struct ref_store *packed_ref_store_create(const char *path, base_ref_store_init(ref_store, &refs_be_packed); refs->store_flags = store_flags; - refs->path = xstrdup(path); + refs->path = xstrdup_or_null(path); return ref_store; } @@ -342,7 +352,7 @@ static void sort_snapshot(struct snapshot *snapshot) /* The safety check should prevent this. */ BUG("unterminated line found in packed-refs"); if (eol - pos < GIT_SHA1_HEXSZ + 2) - die_invalid_line(snapshot->refs->path, + die_invalid_line(snapshot_path(snapshot), pos, eof - pos); eol++; if (eol < eof && *eol == '^') { @@ -454,7 +464,7 @@ static void verify_buffer_safe(struct snapshot *snapshot) last_line = find_start_of_record(start, eof - 1); if (*(eof - 1) != '\n' || eof - last_line < GIT_SHA1_HEXSZ + 2) - die_invalid_line(snapshot->refs->path, + die_invalid_line(snapshot_path(snapshot), last_line, eof - last_line); } @@ -473,7 +483,7 @@ static int load_contents(struct snapshot *snapshot) size_t size; ssize_t bytes_read; - fd = open(snapshot->refs->path, O_RDONLY); + fd = open(snapshot_path(snapshot), O_RDONLY); if (fd < 0) { if (errno == ENOENT) { /* @@ -485,14 +495,14 @@ static int load_contents(struct snapshot *snapshot) */ return 0; } else { - die_errno("couldn't read %s", snapshot->refs->path); + die_errno("couldn't read %s", snapshot_path(snapshot)); } } stat_validity_update(&snapshot->validity, fd); if (fstat(fd, &st) < 0) - die_errno("couldn't stat %s", snapshot->refs->path); + die_errno("couldn't stat %s", snapshot_path(snapshot)); size = xsize_t(st.st_size); if (!size) { @@ -501,7 +511,7 @@ static int load_contents(struct snapshot *snapshot) snapshot->buf = xmalloc(size); bytes_read = read_in_full(fd, snapshot->buf, size); if (bytes_read < 0 || bytes_read != size) - die_errno("couldn't read %s", snapshot->refs->path); + die_errno("couldn't read %s", snapshot_path(snapshot)); snapshot->mmapped = 0; } else { snapshot->buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0); @@ -629,14 +639,14 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs) eol = memchr(snapshot->buf, '\n', snapshot->eof - snapshot->buf); if (!eol) - die_unterminated_line(refs->path, + die_unterminated_line(packed_refs_path(refs), snapshot->buf, snapshot->eof - snapshot->buf); tmp = xmemdupz(snapshot->buf, eol - snapshot->buf); if (!skip_prefix(tmp, "# pack-refs with:", (const char **)&p)) - die_invalid_line(refs->path, + die_invalid_line(packed_refs_path(refs), snapshot->buf, snapshot->eof - snapshot->buf); @@ -695,7 +705,8 @@ static struct snapshot *create_snapshot(struct packed_ref_store *refs) static void validate_snapshot(struct packed_ref_store *refs) { if (refs->snapshot && - !stat_validity_check(&refs->snapshot->validity, refs->path)) + !stat_validity_check(&refs->snapshot->validity, + packed_refs_path(refs))) clear_snapshot(refs); } @@ -739,7 +750,7 @@ static int packed_read_raw_ref(struct ref_store *ref_store, } if (get_oid_hex(rec, oid)) - die_invalid_line(refs->path, rec, snapshot->eof - rec); + die_invalid_line(packed_refs_path(refs), rec, snapshot->eof - rec); *type = REF_ISPACKED; return 0; @@ -795,12 +806,12 @@ static int next_record(struct packed_ref_iterator *iter) if (iter->eof - p < GIT_SHA1_HEXSZ + 2 || parse_oid_hex(p, &iter->oid, &p) || !isspace(*p++)) - die_invalid_line(iter->snapshot->refs->path, + die_invalid_line(snapshot_path(iter->snapshot), iter->pos, iter->eof - iter->pos); eol = memchr(p, '\n', iter->eof - p); if (!eol) - die_unterminated_line(iter->snapshot->refs->path, + die_unterminated_line(snapshot_path(iter->snapshot), iter->pos, iter->eof - iter->pos); strbuf_add(&iter->refname_buf, p, eol - p); @@ -825,7 +836,7 @@ static int next_record(struct packed_ref_iterator *iter) if (iter->eof - p < GIT_SHA1_HEXSZ + 1 || parse_oid_hex(p, &iter->peeled, &p) || *p++ != '\n') - die_invalid_line(iter->snapshot->refs->path, + die_invalid_line(snapshot_path(iter->snapshot), iter->pos, iter->eof - iter->pos); iter->pos = p; @@ -995,14 +1006,14 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err) */ if (hold_lock_file_for_update_timeout( &refs->lock, - refs->path, + packed_refs_path(refs), flags, timeout_value) < 0) { - unable_to_lock_message(refs->path, errno, err); + unable_to_lock_message(packed_refs_path(refs), errno, err); return -1; } if (close_lock_file_gently(&refs->lock)) { - strbuf_addf(err, "unable to close %s: %s", refs->path, strerror(errno)); + strbuf_addf(err, "unable to close %s: %s", packed_refs_path(refs), strerror(errno)); rollback_lock_file(&refs->lock); return -1; } @@ -1471,21 +1482,21 @@ static int packed_transaction_finish(struct ref_store *ref_store, REF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB, "ref_transaction_finish"); int ret = TRANSACTION_GENERIC_ERROR; - char *packed_refs_path; + char *path; clear_snapshot(refs); - packed_refs_path = get_locked_file_path(&refs->lock); - if (rename_tempfile(&refs->tempfile, packed_refs_path)) { + path = get_locked_file_path(&refs->lock); + if (rename_tempfile(&refs->tempfile, path)) { strbuf_addf(err, "error replacing %s: %s", - refs->path, strerror(errno)); + path, strerror(errno)); goto cleanup; } ret = 0; cleanup: - free(packed_refs_path); + free(path); packed_transaction_cleanup(refs, transaction); return ret; } ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-27 16:47 ` Jeff King @ 2018-03-27 17:09 ` Duy Nguyen 2018-03-27 17:30 ` Duy Nguyen 2018-03-28 9:47 ` git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars Jeff King 0 siblings, 2 replies; 54+ messages in thread From: Duy Nguyen @ 2018-03-27 17:09 UTC (permalink / raw) To: Jeff King; +Cc: Rafael Ascensao, Git Mailing List On Tue, Mar 27, 2018 at 6:47 PM, Jeff King <peff@peff.net> wrote: >> So I would not mind papering over it right now (with an understanding >> that absolute path pays some more overhead in path walking, which was >> th reason we tried to avoid it in setup code). A slightly better patch >> is trigger this path absolutization from setup_work_tree(), near >> set_git_dir(). But then you face some ugliness there: how to find out >> all ref stores to update, or just update the main ref store alone. > > I don't quite get why f57f37e2 doesn't want to call git_path(). Is it to > avoid the way the path is munged? Or is it to avoid some lazy-setup that > is triggered by calling get_git_dir() at all (which doesn't make much > sense to me, because we'd already have called get_git_dir() much > earlier). Or is it just because we may sometimes fill in refs->git_dir > with something besides get_git_dir() (for a submodule or worktree or > something)? None of those, I think. git_path() does some magic to translate paths so that refs/... ends up with $GIT_COMMON_DIR/refs/... while "index" ends up with $GIT_DIR/index. Michael wanted to avoid that magic and keep the control within refs code (i.e. this code knows refs/ and packed-refs are shared, and pseudo refs are not, what git_path() decides does not matter). > I.e., can we do one of (depending on which of those answers is "yes"): > > 1. Stop caching the value of get_git_dir(), and instead call it > on-demand instead of looking at refs->git_dir? (If we just want to > avoid git_path()). This probably works, but I count it as papering over the problem too. > > 2. If we need to avoid even calling get_git_dir(), can we add a > "light" version of it that avoids whatever side effects we're > trying to skip? > > 3. If the problem is just that sometimes we need get_git_dir() and > sometimes not, could we perhaps store NULL as a sentinel to mean > "look up get_git_dir() when you need it"? > > That would let submodules and worktrees fill in their paths as > necessary (assuming they never change after init), but handle the > case of get_git_dir() changing. > > Hmm. Typing that out, it seems like (3) is probably the right path. > Something like the patch below seems to fix it and passes the tests. Honestly I think this is just another way to work around the problem (with even more changes than your abspath approach). The problem is with setup_work_tree(). We create a ref store at a specific location and it should stay working without lazily calling get_git_dir(), which has nothing to do (anymore) with the path we have given a ref store. If somebody changes a global setting like $CWD, it should be well communicated to everybody involved. I would rather have something like ref_store_reinit() in the same spirit as the second call of set_git_dir() in setup_work_tree. It is hacky, but it works and keeps changes to minimal (so that it could be easily replaced later). -- Duy ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-27 17:09 ` Duy Nguyen @ 2018-03-27 17:30 ` Duy Nguyen 2018-03-28 9:52 ` Jeff King 2018-03-28 9:47 ` git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars Jeff King 1 sibling, 1 reply; 54+ messages in thread From: Duy Nguyen @ 2018-03-27 17:30 UTC (permalink / raw) To: Jeff King; +Cc: Rafael Ascensao, Git Mailing List On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote: > I would rather have something like ref_store_reinit() in the same > spirit as the second call of set_git_dir() in setup_work_tree. It is > hacky, but it works and keeps changes to minimal (so that it could be > easily replaced later). So in the name of hacky and dirty things, it would look something like this. This passed your test case. The test suite is still running (slow laptop) but I don't expect breakages there. -- 8< -- diff --git a/refs.c b/refs.c index 20ba82b434..c6116c4f7a 100644 --- a/refs.c +++ b/refs.c @@ -1660,6 +1660,16 @@ struct ref_store *get_main_ref_store(void) return main_ref_store; } +void make_main_ref_store_use_absolute_paths(void) +{ + files_force_absolute_paths(get_main_ref_store()); +} + +void make_main_ref_store_use_relative_paths(const char *cwd) +{ + files_make_relative_paths(get_main_ref_store(), cwd); +} + /* * Associate a ref store with a name. It is a fatal error to call this * function twice for the same name. diff --git a/refs.h b/refs.h index 01be5ae32f..532a4ad09d 100644 --- a/refs.h +++ b/refs.h @@ -759,6 +759,9 @@ int reflog_expire(const char *refname, const struct object_id *oid, int ref_storage_backend_exists(const char *name); struct ref_store *get_main_ref_store(void); +void make_main_ref_store_use_absolute_paths(void); +void make_main_ref_store_use_relative_paths(const char *cwd); + /* * Return the ref_store instance for the specified submodule. For the * main repository, use submodule==NULL; such a call cannot fail. For diff --git a/refs/files-backend.c b/refs/files-backend.c index bec8e30e9e..629198826f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3092,6 +3092,32 @@ static int files_reflog_expire(struct ref_store *ref_store, return -1; } +void files_force_absolute_paths(struct ref_store *ref_store) +{ + struct files_ref_store *refs = + files_downcast(ref_store, REF_STORE_WRITE, "don't ask"); + + char *path = refs->gitdir; + refs->gitdir = absolute_pathdup(path); + free(path); + + path = refs->gitcommondir; + refs->gitcommondir = absolute_pathdup(path); + free(path); +} + +void files_make_relative_paths(struct ref_store *ref_store, const char *cwd) +{ + struct files_ref_store *refs = + files_downcast(ref_store, REF_STORE_WRITE, "don't ask"); + + const char *path = remove_leading_path(refs->gitdir, cwd); + refs->gitdir = absolute_pathdup(path); + + path = remove_leading_path(refs->gitcommondir, cwd); + refs->gitcommondir = absolute_pathdup(path); +} + static int files_init_db(struct ref_store *ref_store, struct strbuf *err) { struct files_ref_store *refs = diff --git a/refs/refs-internal.h b/refs/refs-internal.h index dd834314bd..827e97bcca 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -669,4 +669,7 @@ struct ref_store { void base_ref_store_init(struct ref_store *refs, const struct ref_storage_be *be); +void files_force_absolute_paths(struct ref_store *refs); +void files_make_relative_paths(struct ref_store *refs, const char *cwd); + #endif /* REFS_REFS_INTERNAL_H */ diff --git a/setup.c b/setup.c index 7287779642..a5f4396b4e 100644 --- a/setup.c +++ b/setup.c @@ -3,6 +3,7 @@ #include "config.h" #include "dir.h" #include "string-list.h" +#include "refs.h" static int inside_git_dir = -1; static int inside_work_tree = -1; @@ -389,8 +390,10 @@ void setup_work_tree(void) work_tree = get_git_work_tree(); git_dir = get_git_dir(); - if (!is_absolute_path(git_dir)) + if (!is_absolute_path(git_dir)) { git_dir = real_path(get_git_dir()); + make_main_ref_store_use_absolute_paths(); + } if (!work_tree || chdir(work_tree)) die(_("this operation must be run in a work tree")); @@ -402,6 +405,7 @@ void setup_work_tree(void) setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1); set_git_dir(remove_leading_path(git_dir, work_tree)); + make_main_ref_store_use_relative_paths(work_tree); initialized = 1; } -- 8< -- ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-27 17:30 ` Duy Nguyen @ 2018-03-28 9:52 ` Jeff King 2018-03-28 10:10 ` Duy Nguyen 0 siblings, 1 reply; 54+ messages in thread From: Jeff King @ 2018-03-28 9:52 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List On Tue, Mar 27, 2018 at 07:30:24PM +0200, Duy Nguyen wrote: > On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote: > > I would rather have something like ref_store_reinit() in the same > > spirit as the second call of set_git_dir() in setup_work_tree. It is > > hacky, but it works and keeps changes to minimal (so that it could be > > easily replaced later). > > So in the name of hacky and dirty things, it would look something like > this. This passed your test case. The test suite is still running > (slow laptop) but I don't expect breakages there. I think this is the right direction. I mentioned in my last reply that it would be nice for this to be a bit more generic, in case we need to use it again (and also just to keep the module boundaries sane). This part confused me at first: > +void make_main_ref_store_use_absolute_paths(void) > +{ > + files_force_absolute_paths(get_main_ref_store()); > +} > + > +void make_main_ref_store_use_relative_paths(const char *cwd) > +{ > + files_make_relative_paths(get_main_ref_store(), cwd); > +} since I thought you were actually turning things into absolute paths. But your procedure is basically "turn absolute, then after chdir, turn them back relative". I think it might be clearer if a single call is given both the old and new paths. That requires the caller of chdir() storing getcwd() before it moves, but I don't think that should be a big deal. -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-28 9:52 ` Jeff King @ 2018-03-28 10:10 ` Duy Nguyen 2018-03-28 17:36 ` Jeff King 0 siblings, 1 reply; 54+ messages in thread From: Duy Nguyen @ 2018-03-28 10:10 UTC (permalink / raw) To: Jeff King; +Cc: Rafael Ascensao, Git Mailing List On Wed, Mar 28, 2018 at 11:52 AM, Jeff King <peff@peff.net> wrote: > On Tue, Mar 27, 2018 at 07:30:24PM +0200, Duy Nguyen wrote: > >> On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote: >> > I would rather have something like ref_store_reinit() in the same >> > spirit as the second call of set_git_dir() in setup_work_tree. It is >> > hacky, but it works and keeps changes to minimal (so that it could be >> > easily replaced later). >> >> So in the name of hacky and dirty things, it would look something like >> this. This passed your test case. The test suite is still running >> (slow laptop) but I don't expect breakages there. > > I think this is the right direction. I mentioned in my last reply that > it would be nice for this to be a bit more generic, in case we need to > use it again (and also just to keep the module boundaries sane). Yes, that's why I called it hacky and dirty :) I keep thinking about this, so I will probably fix it in a nicer way. > This part confused me at first: > >> +void make_main_ref_store_use_absolute_paths(void) >> +{ >> + files_force_absolute_paths(get_main_ref_store()); >> +} >> + >> +void make_main_ref_store_use_relative_paths(const char *cwd) >> +{ >> + files_make_relative_paths(get_main_ref_store(), cwd); >> +} > > since I thought you were actually turning things into absolute paths. > But your procedure is basically "turn absolute, then after chdir, turn > them back relative". > > I think it might be clearer if a single call is given both the old and > new paths. That requires the caller of chdir() storing getcwd() before > it moves, but I don't think that should be a big deal. The problem is switching relative paths relies on the old $CWD if I'm not mistaken and we need getcwd() for this. I'd love to have one callback that says "$CWD has been switched from this path to that path, do whatever you need to" that can be called any time, before or after chdir(). I'll look more into it. -- Duy ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-28 10:10 ` Duy Nguyen @ 2018-03-28 17:36 ` Jeff King 2018-03-28 17:38 ` [PATCH 1/4] set_git_dir: die when setenv() fails Jeff King ` (4 more replies) 0 siblings, 5 replies; 54+ messages in thread From: Jeff King @ 2018-03-28 17:36 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List On Wed, Mar 28, 2018 at 12:10:21PM +0200, Duy Nguyen wrote: > > I think it might be clearer if a single call is given both the old and > > new paths. That requires the caller of chdir() storing getcwd() before > > it moves, but I don't think that should be a big deal. > > The problem is switching relative paths relies on the old $CWD if I'm > not mistaken and we need getcwd() for this. I'd love to have one > callback that says "$CWD has been switched from this path to that > path, do whatever you need to" that can be called any time, before or > after chdir(). I'll look more into it. I think it should be OK to save getcwd() and just construct the original path after the fact. Here's some patches which do that in a nice way. For those just joining us, this fixes a regression that was in v2.13 (so nothing we need to worry about as part of the 2.17-rc track). [1/4]: set_git_dir: die when setenv() fails [2/4]: add chdir-notify API [3/4]: set_work_tree: use chdir_notify [4/4]: refs: use chdir_notify to update cached relative paths Makefile | 1 + cache.h | 2 +- chdir-notify.c | 71 +++++++++++++++++++++++++++++++++++++++++++ chdir-notify.h | 64 ++++++++++++++++++++++++++++++++++++++ environment.c | 22 ++++++++++++-- refs/files-backend.c | 4 +++ refs/packed-backend.c | 3 ++ setup.c | 9 ++---- t/t1501-work-tree.sh | 12 ++++++++ 9 files changed, 178 insertions(+), 10 deletions(-) create mode 100644 chdir-notify.c create mode 100644 chdir-notify.h -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 1/4] set_git_dir: die when setenv() fails 2018-03-28 17:36 ` Jeff King @ 2018-03-28 17:38 ` Jeff King 2018-03-28 17:40 ` [PATCH 2/4] add chdir-notify API Jeff King ` (3 subsequent siblings) 4 siblings, 0 replies; 54+ messages in thread From: Jeff King @ 2018-03-28 17:38 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List 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 <peff@peff.net> --- cache.h | 2 +- environment.c | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/cache.h b/cache.h index a61b2d3f0d..5c24394d84 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 d6dd64662c..e01acf8b11 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) -- 2.17.0.rc1.515.g3cc84c0ca4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 2/4] add chdir-notify API 2018-03-28 17:36 ` Jeff King 2018-03-28 17:38 ` [PATCH 1/4] set_git_dir: die when setenv() fails Jeff King @ 2018-03-28 17:40 ` Jeff King 2018-03-28 17:58 ` Eric Sunshine 2018-03-29 14:53 ` Duy Nguyen 2018-03-28 17:42 ` [PATCH 3/4] set_work_tree: use chdir_notify Jeff King ` (2 subsequent siblings) 4 siblings, 2 replies; 54+ messages in thread From: Jeff King @ 2018-03-28 17:40 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List 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 <peff@peff.net> --- Makefile | 1 + chdir-notify.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++ chdir-notify.h | 64 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 136 insertions(+) create mode 100644 chdir-notify.c create mode 100644 chdir-notify.h diff --git a/Makefile b/Makefile index a1d8775adb..0da98b9274 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 0000000000..0b88bf0e5b --- /dev/null +++ b/chdir-notify.c @@ -0,0 +1,71 @@ +#include "cache.h" +#include "chdir-notify.h" +#include "list.h" +#include "strbuf.h" + +struct chdir_notify_entry { + chdir_notify_callback cb; + void *data; + struct list_head list; +}; +static LIST_HEAD(chdir_notify_entries); + +void chdir_notify_register(chdir_notify_callback cb, void *data) +{ + struct chdir_notify_entry *e = xmalloc(sizeof(*e)); + e->cb = cb; + e->data = data; + list_add_tail(&e->list, &chdir_notify_entries); +} + +static void reparent_cb(const char *old_cwd, + const char *new_cwd, + void *data) +{ + char **path = data; + char *tmp = *path; + *path = reparent_relative_path(old_cwd, new_cwd, tmp); + free(tmp); +} + +void chdir_notify_reparent(char **path) +{ + if (!is_absolute_path(*path)) + chdir_notify_register(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) + return -1; + + list_for_each(pos, &chdir_notify_entries) { + struct chdir_notify_entry *e = + list_entry(pos, struct chdir_notify_entry, list); + e->cb(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 0000000000..c3bd818a85 --- /dev/null +++ b/chdir-notify.h @@ -0,0 +1,64 @@ +#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(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(&my_path); + * + * 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. + */ +typedef void (*chdir_notify_callback)(const char *old_cwd, + const char *new_cwd, + void *data); +void chdir_notify_register(chdir_notify_callback cb, void *data); +void chdir_notify_reparent(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 */ -- 2.17.0.rc1.515.g3cc84c0ca4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 2/4] add chdir-notify API 2018-03-28 17:40 ` [PATCH 2/4] add chdir-notify API Jeff King @ 2018-03-28 17:58 ` Eric Sunshine 2018-03-28 18:02 ` Jeff King 2018-03-29 14:53 ` Duy Nguyen 1 sibling, 1 reply; 54+ messages in thread From: Eric Sunshine @ 2018-03-28 17:58 UTC (permalink / raw) To: Jeff King; +Cc: Duy Nguyen, Rafael Ascensao, Git Mailing List On Wed, Mar 28, 2018 at 1:40 PM, Jeff King <peff@peff.net> wrote: > [...] > 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. > [...] > Signed-off-by: Jeff King <peff@peff.net> > --- > diff --git a/chdir-notify.c b/chdir-notify.c > @@ -0,0 +1,71 @@ > +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) > + return -1; This 'return' is leaking 'old_cwd', isn't it? > + list_for_each(pos, &chdir_notify_entries) { > + struct chdir_notify_entry *e = > + list_entry(pos, struct chdir_notify_entry, list); > + e->cb(old_cwd.buf, new_cwd, e->data); > + } > + > + strbuf_release(&old_cwd); > + return 0; > +} > diff --git a/chdir-notify.h b/chdir-notify.h > @@ -0,0 +1,64 @@ > + * 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(&my_path); > + */ > +typedef void (*chdir_notify_callback)(const char *old_cwd, > + const char *new_cwd, > + void *data); > +void chdir_notify_register(chdir_notify_callback cb, void *data); > +void chdir_notify_reparent(char **path); Can we have some documentation here (or in the chdir_notify_reparent() example above) explaining that *path is a heap-allocated value? I had to consult the implementation to understand ownership. ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 2/4] add chdir-notify API 2018-03-28 17:58 ` Eric Sunshine @ 2018-03-28 18:02 ` Jeff King 0 siblings, 0 replies; 54+ messages in thread From: Jeff King @ 2018-03-28 18:02 UTC (permalink / raw) To: Eric Sunshine; +Cc: Duy Nguyen, Rafael Ascensao, Git Mailing List On Wed, Mar 28, 2018 at 01:58:18PM -0400, Eric Sunshine wrote: > On Wed, Mar 28, 2018 at 1:40 PM, Jeff King <peff@peff.net> wrote: > > [...] > > 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. > > [...] > > Signed-off-by: Jeff King <peff@peff.net> > > --- > > diff --git a/chdir-notify.c b/chdir-notify.c > > @@ -0,0 +1,71 @@ > > +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) > > + return -1; > > This 'return' is leaking 'old_cwd', isn't it? Whoops, yes. I wrote it the other way around first to avoid the leak, but of course that has other problems. ;) > > diff --git a/chdir-notify.h b/chdir-notify.h > > @@ -0,0 +1,64 @@ > > + * 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(&my_path); > > + */ > > +typedef void (*chdir_notify_callback)(const char *old_cwd, > > + const char *new_cwd, > > + void *data); > > +void chdir_notify_register(chdir_notify_callback cb, void *data); > > +void chdir_notify_reparent(char **path); > > Can we have some documentation here (or in the chdir_notify_reparent() > example above) explaining that *path is a heap-allocated value? I had > to consult the implementation to understand ownership. Sure. I originally had reparent_relative_path() handle the freeing and allocation, and it explained the ownership in its docstring. But as I revised that to simply return its value, that got lost. I'll add it back in here. -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 2/4] add chdir-notify API 2018-03-28 17:40 ` [PATCH 2/4] add chdir-notify API Jeff King 2018-03-28 17:58 ` Eric Sunshine @ 2018-03-29 14:53 ` Duy Nguyen 2018-03-29 17:48 ` Jeff King 1 sibling, 1 reply; 54+ messages in thread From: Duy Nguyen @ 2018-03-29 14:53 UTC (permalink / raw) To: Jeff King; +Cc: Rafael Ascensao, Git Mailing List On Wed, Mar 28, 2018 at 7:40 PM, Jeff King <peff@peff.net> wrote: > +static void reparent_cb(const char *old_cwd, > + const char *new_cwd, > + void *data) > +{ > + char **path = data; Maybe check data == NULL and return early. This is just for convenience, e.g. instead of doing path = getenv("foo"); if (path) chdir_notify_reparent(&path); I could do path = getenv("foo"); chdir_notify_reparent(&path); > + char *tmp = *path; > + *path = reparent_relative_path(old_cwd, new_cwd, tmp); > + free(tmp); > +} > + > +void chdir_notify_reparent(char **path) > +{ > + if (!is_absolute_path(*path)) I think this check is a bit early. There could be a big gap between chdir_notify_reparent() and reparent_cb(). During that time, *path may be updated and become a relative path. We can check for absolute path inside reparent_cb() instead. > + chdir_notify_register(reparent_cb, path); > +} Overall, I like this API very much! I will add another one similar to chdir_notify_reparent() but it takes an env name instead and the callback will setenv() appropriately. The result looks super good. -- Duy ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 2/4] add chdir-notify API 2018-03-29 14:53 ` Duy Nguyen @ 2018-03-29 17:48 ` Jeff King 2018-03-29 18:12 ` Duy Nguyen 0 siblings, 1 reply; 54+ messages in thread From: Jeff King @ 2018-03-29 17:48 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List On Thu, Mar 29, 2018 at 04:53:42PM +0200, Duy Nguyen wrote: > On Wed, Mar 28, 2018 at 7:40 PM, Jeff King <peff@peff.net> wrote: > > +static void reparent_cb(const char *old_cwd, > > + const char *new_cwd, > > + void *data) > > +{ > > + char **path = data; > > Maybe check data == NULL and return early. This is just for > convenience, e.g. instead of doing > > path = getenv("foo"); > if (path) > chdir_notify_reparent(&path); > > I could do > > path = getenv("foo"); > chdir_notify_reparent(&path); You'd need to xstrdup(path) there anyway. I guess you could use xstrdup_or_null(), but it seems somewhat unlikely (unless your work on top really does add such a callsite?). I guess it's not that much code to be careful, though. > > + char *tmp = *path; > > + *path = reparent_relative_path(old_cwd, new_cwd, tmp); > > + free(tmp); > > +} > > + > > +void chdir_notify_reparent(char **path) > > +{ > > + if (!is_absolute_path(*path)) > > I think this check is a bit early. There could be a big gap between > chdir_notify_reparent() and reparent_cb(). During that time, *path may > be updated and become a relative path. We can check for absolute path > inside reparent_cb() instead. My thinking was that we'd be effectively zero-cost for an absolute path, though really adding an element to the notification list is probably not a big deal. I also assumed that whoever changed it to a relative path would then install the notification handler. One thing that my series kind of glosses over is whether somebody might call any of these functions twice, which would involve setting up the handler twice. So e.g. if you did: set_git_dir("foo"); set_git_dir("bar"); we'd have two handlers, which would do the wrong thing when the second one triggered. I hoped we could eventually add a BUG() if that happens, but maybe we should simply do a: static int registered_chdir; if (!registered_chdir) { chdir_notify_reparent(&path); registered_chdir = 1; } at each call-site. Another alternative would be to make it a noop to feed the same path twice. That requires a linear walk through the list, but it's a pretty small list. > > + chdir_notify_register(reparent_cb, path); > > +} > > Overall, I like this API very much! I will add another one similar to > chdir_notify_reparent() but it takes an env name instead and the > callback will setenv() appropriately. The result looks super good. Ooh, that's clever. I had also thought of extending it to other notifications besides chdir(), like one string depends on another. But then we'd basically re-inventing a cascading data-flow system, which is probably getting a bit magical. :) -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 2/4] add chdir-notify API 2018-03-29 17:48 ` Jeff King @ 2018-03-29 18:12 ` Duy Nguyen 0 siblings, 0 replies; 54+ messages in thread From: Duy Nguyen @ 2018-03-29 18:12 UTC (permalink / raw) To: Jeff King; +Cc: Rafael Ascensao, Git Mailing List On Thu, Mar 29, 2018 at 7:48 PM, Jeff King <peff@peff.net> wrote: > On Thu, Mar 29, 2018 at 04:53:42PM +0200, Duy Nguyen wrote: > >> On Wed, Mar 28, 2018 at 7:40 PM, Jeff King <peff@peff.net> wrote: >> > +static void reparent_cb(const char *old_cwd, >> > + const char *new_cwd, >> > + void *data) >> > +{ >> > + char **path = data; >> >> Maybe check data == NULL and return early. This is just for >> convenience, e.g. instead of doing >> >> path = getenv("foo"); >> if (path) >> chdir_notify_reparent(&path); >> >> I could do >> >> path = getenv("foo"); >> chdir_notify_reparent(&path); > > You'd need to xstrdup(path) there anyway. I guess you could use > xstrdup_or_null(), but it seems somewhat unlikely (unless your work on > top really does add such a callsite?). It actually exists in 'next', repository.c where we have this line repo->alternate_db = xstrdup_or_null(o->alternate_db); > I guess it's not that much code to be careful, though. >> > +void chdir_notify_reparent(char **path) >> > +{ >> > + if (!is_absolute_path(*path)) >> >> I think this check is a bit early. There could be a big gap between >> chdir_notify_reparent() and reparent_cb(). During that time, *path may >> be updated and become a relative path. We can check for absolute path >> inside reparent_cb() instead. > > My thinking was that we'd be effectively zero-cost for an absolute path, > though really adding an element to the notification list is probably not > a big deal. I think we could leave such optimization to the caller. I'm ok with keeping this "if" too, but you may need to clarify it in the big comment block in chdir-notify.h because this behavior to me is surprising. > I also assumed that whoever changed it to a relative path would then > install the notification handler. One thing that my series kind of > glosses over is whether somebody might call any of these functions > twice, which would involve setting up the handler twice. So e.g. if you > did: > > set_git_dir("foo"); > set_git_dir("bar"); > > we'd have two handlers, which would do the wrong thing when the second > one triggered. I hoped we could eventually add a BUG() if that happens, > but maybe we should simply do a: > > static int registered_chdir; > > if (!registered_chdir) { > chdir_notify_reparent(&path); > registered_chdir = 1; > } > > at each call-site. Another alternative would be to make it a noop to > feed the same path twice. That requires a linear walk through the list, > but it's a pretty small list. Well, given the number of call sites is rather small at the moment, I think chdir-notify can just stay dumb and let the caller deal with duplicate registration. One thing I like about your linked list design though, is it makes it quite easy to remove (or even update) a callback. You can simply return the (opaque) pointer to the entire chdir_notify_entry as a handle and we can free/unlink the entry based on it. It's kinda hard to me to do it with array-based design. -- Duy ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 3/4] set_work_tree: use chdir_notify 2018-03-28 17:36 ` Jeff King 2018-03-28 17:38 ` [PATCH 1/4] set_git_dir: die when setenv() fails Jeff King 2018-03-28 17:40 ` [PATCH 2/4] add chdir-notify API Jeff King @ 2018-03-28 17:42 ` Jeff King 2018-03-29 17:02 ` Duy Nguyen 2018-03-28 17:43 ` [PATCH 4/4] refs: use chdir_notify to update cached relative paths Jeff King 2018-03-30 18:34 ` [PATCH v2 0/5] re-parenting relative directories after chdir Jeff King 4 siblings, 1 reply; 54+ messages in thread From: Jeff King @ 2018-03-28 17:42 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List 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 <peff@peff.net> --- environment.c | 19 ++++++++++++++++++- setup.c | 9 +++------ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/environment.c b/environment.c index e01acf8b11..e1f535ae08 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,22 @@ void set_git_dir(const char *path) setup_git_env(); } +static void update_relative_gitdir(const char *old_cwd, + const char *new_cwd, + void *data) +{ + char *path = reparent_relative_path(old_cwd, new_cwd, get_git_dir()); + 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(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 7287779642..9eb2e808e1 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; } -- 2.17.0.rc1.515.g3cc84c0ca4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 3/4] set_work_tree: use chdir_notify 2018-03-28 17:42 ` [PATCH 3/4] set_work_tree: use chdir_notify Jeff King @ 2018-03-29 17:02 ` Duy Nguyen 2018-03-29 17:23 ` Duy Nguyen 2018-03-29 17:50 ` Jeff King 0 siblings, 2 replies; 54+ messages in thread From: Duy Nguyen @ 2018-03-29 17:02 UTC (permalink / raw) To: Jeff King; +Cc: Rafael Ascensao, Git Mailing List On Wed, Mar 28, 2018 at 7:42 PM, Jeff King <peff@peff.net> wrote: > 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. Another way to approach this problem is not delaying chdir() at all. We have to delay calling setup_work_tree() and not do it in setup_git_directory() because it can die() when chdir() fails. But in many cases, the command does not actually need the worktree and does not deserve to die. But what if we make setup_work_tree be gentle? If it successfully chdir() at the end of setup_git_directory() (and perhaps before the first set_git_dir call), great! The problem we're dealing here vanishes. If it fails, don't die, just set a flag. Later on when a command requests a worktree, we can check this flag and now can die(). It's less code this way, but it uses up more of your (or my) time because even though the first set_git_dir() call actually happens at 8 places. Is it worth trying ? -- Duy ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 3/4] set_work_tree: use chdir_notify 2018-03-29 17:02 ` Duy Nguyen @ 2018-03-29 17:23 ` Duy Nguyen 2018-03-29 17:50 ` Jeff King 2018-03-29 17:50 ` Jeff King 1 sibling, 1 reply; 54+ messages in thread From: Duy Nguyen @ 2018-03-29 17:23 UTC (permalink / raw) To: Jeff King; +Cc: Rafael Ascensao, Git Mailing List On Thu, Mar 29, 2018 at 7:02 PM, Duy Nguyen <pclouds@gmail.com> wrote: > ... > > It's less code this way, but it uses up more of your (or my) time > because even though the first set_git_dir() call actually happens at 8 > places. Is it worth trying ? Never mind. I took a stab anyway. The setup code should have much less side effect before we can do stuff like this. -- Duy ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 3/4] set_work_tree: use chdir_notify 2018-03-29 17:23 ` Duy Nguyen @ 2018-03-29 17:50 ` Jeff King 0 siblings, 0 replies; 54+ messages in thread From: Jeff King @ 2018-03-29 17:50 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List On Thu, Mar 29, 2018 at 07:23:17PM +0200, Duy Nguyen wrote: > On Thu, Mar 29, 2018 at 7:02 PM, Duy Nguyen <pclouds@gmail.com> wrote: > > ... > > > > It's less code this way, but it uses up more of your (or my) time > > because even though the first set_git_dir() call actually happens at 8 > > places. Is it worth trying ? > > Never mind. I took a stab anyway. The setup code should have much less > side effect before we can do stuff like this. Oh, I should have read this before responding. Oh well. :) -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 3/4] set_work_tree: use chdir_notify 2018-03-29 17:02 ` Duy Nguyen 2018-03-29 17:23 ` Duy Nguyen @ 2018-03-29 17:50 ` Jeff King 2018-03-29 18:01 ` Duy Nguyen 1 sibling, 1 reply; 54+ messages in thread From: Jeff King @ 2018-03-29 17:50 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List On Thu, Mar 29, 2018 at 07:02:21PM +0200, Duy Nguyen wrote: > On Wed, Mar 28, 2018 at 7:42 PM, Jeff King <peff@peff.net> wrote: > > 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. > > Another way to approach this problem is not delaying chdir() at all. > We have to delay calling setup_work_tree() and not do it in > setup_git_directory() because it can die() when chdir() fails. But in > many cases, the command does not actually need the worktree and does > not deserve to die. But what if we make setup_work_tree be gentle? > > If it successfully chdir() at the end of setup_git_directory() (and > perhaps before the first set_git_dir call), great! The problem we're > dealing here vanishes. If it fails, don't die, just set a flag. Later > on when a command requests a worktree, we can check this flag and now > can die(). > > It's less code this way, but it uses up more of your (or my) time > because even though the first set_git_dir() call actually happens at 8 > places. Is it worth trying ? I do kind of like that. I'm reasonably happy with the chdir_notify() interface, but it would be nicer still if we could get rid of it in the first place. It's true that we _could_ chdir from other places, but realistically speaking, we do our one big chdir as part of the setup process. -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 3/4] set_work_tree: use chdir_notify 2018-03-29 17:50 ` Jeff King @ 2018-03-29 18:01 ` Duy Nguyen 2018-03-30 17:23 ` Jeff King 0 siblings, 1 reply; 54+ messages in thread From: Duy Nguyen @ 2018-03-29 18:01 UTC (permalink / raw) To: Jeff King; +Cc: Rafael Ascensao, Git Mailing List On Thu, Mar 29, 2018 at 7:50 PM, Jeff King <peff@peff.net> wrote: > On Thu, Mar 29, 2018 at 07:02:21PM +0200, Duy Nguyen wrote: > >> On Wed, Mar 28, 2018 at 7:42 PM, Jeff King <peff@peff.net> wrote: >> > 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. >> >> Another way to approach this problem is not delaying chdir() at all. >> We have to delay calling setup_work_tree() and not do it in >> setup_git_directory() because it can die() when chdir() fails. But in >> many cases, the command does not actually need the worktree and does >> not deserve to die. But what if we make setup_work_tree be gentle? >> >> If it successfully chdir() at the end of setup_git_directory() (and >> perhaps before the first set_git_dir call), great! The problem we're >> dealing here vanishes. If it fails, don't die, just set a flag. Later >> on when a command requests a worktree, we can check this flag and now >> can die(). >> >> It's less code this way, but it uses up more of your (or my) time >> because even though the first set_git_dir() call actually happens at 8 >> places. Is it worth trying ? > > I do kind of like that. I'm reasonably happy with the chdir_notify() > interface, but it would be nicer still if we could get rid of it in the > first place. It's true that we _could_ chdir from other places, but There's another place we do, that I should mention and keep forgetting. Our run-command.c code allows to switch cwd, and if $GIT_DIR and stuff is relative then we should reparent them too just like we do with setup_work_tree(). Your chdir-notify makes it super easy to support that, we just need to move the prep_childenv() down below chdir(). But since nobody has complaint, I suppose that feature is not really popular (or at least not used to launch another git process anyway) > realistically speaking, we do our one big chdir as part of the setup > process. -- Duy ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 3/4] set_work_tree: use chdir_notify 2018-03-29 18:01 ` Duy Nguyen @ 2018-03-30 17:23 ` Jeff King 0 siblings, 0 replies; 54+ messages in thread From: Jeff King @ 2018-03-30 17:23 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List On Thu, Mar 29, 2018 at 08:01:33PM +0200, Duy Nguyen wrote: > > I do kind of like that. I'm reasonably happy with the chdir_notify() > > interface, but it would be nicer still if we could get rid of it in the > > first place. It's true that we _could_ chdir from other places, but > > There's another place we do, that I should mention and keep > forgetting. Our run-command.c code allows to switch cwd, and if > $GIT_DIR and stuff is relative then we should reparent them too just > like we do with setup_work_tree(). Your chdir-notify makes it super > easy to support that, we just need to move the prep_childenv() down > below chdir(). But since nobody has complaint, I suppose that feature > is not really popular (or at least not used to launch another git > process anyway) Yeah, I hadn't really considered that, since it would only matter for environment variables, not for internal strings (which are all about to get thrown out due to execve anyway). And my patch was just focusing on that. But I also wonder if rewriting the environment variables would matter here. If we're going to chdir for a child, we'd generaly be clearing repo-related env anyway. -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 4/4] refs: use chdir_notify to update cached relative paths 2018-03-28 17:36 ` Jeff King ` (2 preceding siblings ...) 2018-03-28 17:42 ` [PATCH 3/4] set_work_tree: use chdir_notify Jeff King @ 2018-03-28 17:43 ` Jeff King 2018-03-30 18:34 ` [PATCH v2 0/5] re-parenting relative directories after chdir Jeff King 4 siblings, 0 replies; 54+ messages in thread From: Jeff King @ 2018-03-28 17:43 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List 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 <rafa.almas@gmail.com> Helped-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- refs/files-backend.c | 4 ++++ refs/packed-backend.c | 3 +++ t/t1501-work-tree.sh | 12 ++++++++++++ 3 files changed, 19 insertions(+) diff --git a/refs/files-backend.c b/refs/files-backend.c index bec8e30e9e..ab3e00ffa0 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,9 @@ 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(&refs->gitdir); + chdir_notify_reparent(&refs->gitcommondir); + return ref_store; } diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 65288c6472..6725742f00 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(&refs->path); + return ref_store; } diff --git a/t/t1501-work-tree.sh b/t/t1501-work-tree.sh index b06210ec5e..a5db53337b 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 -- 2.17.0.rc1.515.g3cc84c0ca4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH v2 0/5] re-parenting relative directories after chdir 2018-03-28 17:36 ` Jeff King ` (3 preceding siblings ...) 2018-03-28 17:43 ` [PATCH 4/4] refs: use chdir_notify to update cached relative paths Jeff King @ 2018-03-30 18:34 ` Jeff King 2018-03-30 18:34 ` [PATCH v2 1/5] set_git_dir: die when setenv() fails Jeff King ` (5 more replies) 4 siblings, 6 replies; 54+ messages in thread From: Jeff King @ 2018-03-30 18:34 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List On Wed, Mar 28, 2018 at 01:36:56PM -0400, Jeff King wrote: > For those just joining us, this fixes a regression that was in v2.13 (so > nothing we need to worry about as part of the 2.17-rc track). > > [1/4]: set_git_dir: die when setenv() fails > [2/4]: add chdir-notify API > [3/4]: set_work_tree: use chdir_notify > [4/4]: refs: use chdir_notify to update cached relative paths Here's a re-roll based on the feedback I got, including: - fixes the memory leak and vague comment pointed out by Eric - adds in tracing code from Duy - I took Duy's suggestions regarding "least" surprise in some of the functions (reparenting NULL is a noop, and registering a reparent handler does so even for an absolute path). I punted on the "registering the same path twice" thing. That is a potential way to misuse this API, but I don't think there's a good solution. The "reparent" helper could figure this out for you, but in the general case we actually install an arbitrary callback. So the caller really has to handle it there. I think in the long run we'd want to outlaw calling set_git_dir() twice anyway. But possibly the handler-installers should be more careful. I'm dropping poor Rafael from the cc here, since it is turning into quite a lot of messages. :) I think at this point his bug has been duly reported and we are working on the fix. [1/5]: set_git_dir: die when setenv() fails [2/5]: trace.c: export trace_setup_key [3/5]: add chdir-notify API [4/5]: set_work_tree: use chdir_notify [5/5]: refs: use chdir_notify to update cached relative paths Makefile | 1 + cache.h | 2 +- chdir-notify.c | 93 +++++++++++++++++++++++++++++++++++++++++++ chdir-notify.h | 73 +++++++++++++++++++++++++++++++++ environment.c | 26 ++++++++++-- refs/files-backend.c | 6 +++ refs/packed-backend.c | 3 ++ setup.c | 9 ++--- t/t1501-work-tree.sh | 12 ++++++ trace.c | 14 +++---- trace.h | 1 + 11 files changed, 223 insertions(+), 17 deletions(-) create mode 100644 chdir-notify.c create mode 100644 chdir-notify.h -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v2 1/5] set_git_dir: die when setenv() fails 2018-03-30 18:34 ` [PATCH v2 0/5] re-parenting relative directories after chdir Jeff King @ 2018-03-30 18:34 ` Jeff King 2018-03-30 18:34 ` [PATCH v2 2/5] trace.c: export trace_setup_key Jeff King ` (4 subsequent siblings) 5 siblings, 0 replies; 54+ messages in thread From: Jeff King @ 2018-03-30 18:34 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List 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 <peff@peff.net> --- cache.h | 2 +- environment.c | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/cache.h b/cache.h index a61b2d3f0d..5c24394d84 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 d6dd64662c..e01acf8b11 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) -- 2.17.0.rc2.594.gdb94a0ce02 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH v2 2/5] trace.c: export trace_setup_key 2018-03-30 18:34 ` [PATCH v2 0/5] re-parenting relative directories after chdir Jeff King 2018-03-30 18:34 ` [PATCH v2 1/5] set_git_dir: die when setenv() fails Jeff King @ 2018-03-30 18:34 ` Jeff King 2018-03-30 19:46 ` Junio C Hamano 2018-03-30 18:35 ` [PATCH v2 3/5] add chdir-notify API Jeff King ` (3 subsequent siblings) 5 siblings, 1 reply; 54+ messages in thread From: Jeff King @ 2018-03-30 18:34 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> 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 <pclouds@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- trace.c | 14 +++++++------- trace.h | 1 + 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/trace.c b/trace.c index 7f3b08e148..fc623e91fd 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 88055abef7..2b6a1bc17c 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); -- 2.17.0.rc2.594.gdb94a0ce02 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH v2 2/5] trace.c: export trace_setup_key 2018-03-30 18:34 ` [PATCH v2 2/5] trace.c: export trace_setup_key Jeff King @ 2018-03-30 19:46 ` Junio C Hamano 2018-03-30 19:47 ` Jeff King 0 siblings, 1 reply; 54+ messages in thread From: Junio C Hamano @ 2018-03-30 19:46 UTC (permalink / raw) To: Jeff King; +Cc: Duy Nguyen, Git Mailing List Jeff King <peff@peff.net> writes: > From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > > This is so that we can print traces based on this key outside trace.c. "this key" meaning...? GIT_TRACE_SETUP? > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > Signed-off-by: Jeff King <peff@peff.net> > --- > trace.c | 14 +++++++------- > trace.h | 1 + > 2 files changed, 8 insertions(+), 7 deletions(-) > > diff --git a/trace.c b/trace.c > index 7f3b08e148..fc623e91fd 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 88055abef7..2b6a1bc17c 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); ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 2/5] trace.c: export trace_setup_key 2018-03-30 19:46 ` Junio C Hamano @ 2018-03-30 19:47 ` Jeff King 2018-03-30 19:50 ` Junio C Hamano 0 siblings, 1 reply; 54+ messages in thread From: Jeff King @ 2018-03-30 19:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List On Fri, Mar 30, 2018 at 12:46:26PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > > > > This is so that we can print traces based on this key outside trace.c. > > "this key" meaning...? GIT_TRACE_SETUP? I think "based on trace_setup_key". -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 2/5] trace.c: export trace_setup_key 2018-03-30 19:47 ` Jeff King @ 2018-03-30 19:50 ` Junio C Hamano 2018-03-30 19:54 ` Jeff King 0 siblings, 1 reply; 54+ messages in thread From: Junio C Hamano @ 2018-03-30 19:50 UTC (permalink / raw) To: Jeff King; +Cc: Duy Nguyen, Git Mailing List Jeff King <peff@peff.net> writes: > On Fri, Mar 30, 2018 at 12:46:26PM -0700, Junio C Hamano wrote: > >> Jeff King <peff@peff.net> writes: >> >> > From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> >> > >> > This is so that we can print traces based on this key outside trace.c. >> >> "this key" meaning...? GIT_TRACE_SETUP? > > I think "based on trace_setup_key". > > -Peff Yeah, I read, but did not pay enough attention to, the subject X-<. ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH v2 2/5] trace.c: export trace_setup_key 2018-03-30 19:50 ` Junio C Hamano @ 2018-03-30 19:54 ` Jeff King 0 siblings, 0 replies; 54+ messages in thread From: Jeff King @ 2018-03-30 19:54 UTC (permalink / raw) To: Junio C Hamano; +Cc: Duy Nguyen, Git Mailing List On Fri, Mar 30, 2018 at 12:50:50PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Fri, Mar 30, 2018 at 12:46:26PM -0700, Junio C Hamano wrote: > > > >> Jeff King <peff@peff.net> writes: > >> > >> > From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> > >> > > >> > This is so that we can print traces based on this key outside trace.c. > >> > >> "this key" meaning...? GIT_TRACE_SETUP? > > > > I think "based on trace_setup_key". > > > > -Peff > > Yeah, I read, but did not pay enough attention to, the subject X-<. To be fair, one of our guidelines is that the commit message should not overly rely on the subject line. Though I am certainly guilty of starting many a message with "This". Perhaps: The setup-tracing code is static-local to trace.c. In preparation for new GIT_TRACE_SETUP code outside of trace.c, let's make the trace_key globally available. would be a better commit message. -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH v2 3/5] add chdir-notify API 2018-03-30 18:34 ` [PATCH v2 0/5] re-parenting relative directories after chdir Jeff King 2018-03-30 18:34 ` [PATCH v2 1/5] set_git_dir: die when setenv() fails Jeff King 2018-03-30 18:34 ` [PATCH v2 2/5] trace.c: export trace_setup_key Jeff King @ 2018-03-30 18:35 ` Jeff King 2018-03-30 18:35 ` [PATCH v2 4/5] set_work_tree: use chdir_notify Jeff King ` (2 subsequent siblings) 5 siblings, 0 replies; 54+ messages in thread From: Jeff King @ 2018-03-30 18:35 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List 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 <peff@peff.net> --- Makefile | 1 + chdir-notify.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++ chdir-notify.h | 73 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+) create mode 100644 chdir-notify.c create mode 100644 chdir-notify.h diff --git a/Makefile b/Makefile index a1d8775adb..0da98b9274 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 0000000000..5f7f2c2ac2 --- /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 0000000000..366e4c1ee9 --- /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 */ -- 2.17.0.rc2.594.gdb94a0ce02 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH v2 4/5] set_work_tree: use chdir_notify 2018-03-30 18:34 ` [PATCH v2 0/5] re-parenting relative directories after chdir Jeff King ` (2 preceding siblings ...) 2018-03-30 18:35 ` [PATCH v2 3/5] add chdir-notify API Jeff King @ 2018-03-30 18:35 ` Jeff King 2018-03-30 18:35 ` [PATCH v2 5/5] refs: use chdir_notify to update cached relative paths Jeff King 2018-03-30 19:36 ` [PATCH v2 0/5] re-parenting relative directories after chdir Duy Nguyen 5 siblings, 0 replies; 54+ messages in thread From: Jeff King @ 2018-03-30 18:35 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List 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 <peff@peff.net> --- environment.c | 23 ++++++++++++++++++++++- setup.c | 9 +++------ 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/environment.c b/environment.c index e01acf8b11..903a6c9df7 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 7287779642..9eb2e808e1 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; } -- 2.17.0.rc2.594.gdb94a0ce02 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH v2 5/5] refs: use chdir_notify to update cached relative paths 2018-03-30 18:34 ` [PATCH v2 0/5] re-parenting relative directories after chdir Jeff King ` (3 preceding siblings ...) 2018-03-30 18:35 ` [PATCH v2 4/5] set_work_tree: use chdir_notify Jeff King @ 2018-03-30 18:35 ` Jeff King 2018-03-30 19:36 ` [PATCH v2 0/5] re-parenting relative directories after chdir Duy Nguyen 5 siblings, 0 replies; 54+ messages in thread From: Jeff King @ 2018-03-30 18:35 UTC (permalink / raw) To: Duy Nguyen; +Cc: Git Mailing List 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 <rafa.almas@gmail.com> Helped-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- refs/files-backend.c | 6 ++++++ refs/packed-backend.c | 3 +++ t/t1501-work-tree.sh | 12 ++++++++++++ 3 files changed, 21 insertions(+) diff --git a/refs/files-backend.c b/refs/files-backend.c index bec8e30e9e..a92a2aa821 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 65288c6472..369c34f886 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 b06210ec5e..a5db53337b 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 -- 2.17.0.rc2.594.gdb94a0ce02 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH v2 0/5] re-parenting relative directories after chdir 2018-03-30 18:34 ` [PATCH v2 0/5] re-parenting relative directories after chdir Jeff King ` (4 preceding siblings ...) 2018-03-30 18:35 ` [PATCH v2 5/5] refs: use chdir_notify to update cached relative paths Jeff King @ 2018-03-30 19:36 ` Duy Nguyen 5 siblings, 0 replies; 54+ messages in thread From: Duy Nguyen @ 2018-03-30 19:36 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List On Fri, Mar 30, 2018 at 02:34:25PM -0400, Jeff King wrote: > On Wed, Mar 28, 2018 at 01:36:56PM -0400, Jeff King wrote: > > > For those just joining us, this fixes a regression that was in v2.13 (so > > nothing we need to worry about as part of the 2.17-rc track). > > > > [1/4]: set_git_dir: die when setenv() fails > > [2/4]: add chdir-notify API > > [3/4]: set_work_tree: use chdir_notify > > [4/4]: refs: use chdir_notify to update cached relative paths > > Here's a re-roll based on the feedback I got, including: > > - fixes the memory leak and vague comment pointed out by Eric > > - adds in tracing code from Duy > > - I took Duy's suggestions regarding "least" surprise in some of the > functions (reparenting NULL is a noop, and registering a reparent > handler does so even for an absolute path). > > I punted on the "registering the same path twice" thing. That is a > potential way to misuse this API, but I don't think there's a good > solution. The "reparent" helper could figure this out for you, but in > the general case we actually install an arbitrary callback. So the > caller really has to handle it there. The series looks good to me. > > I think in the long run we'd want to outlaw calling set_git_dir() twice > anyway. Oh yeah. With my latest WIP changes, the bottom of setup_git_directory_gently() looks like this. Nowhere else in setup code calls these functions anymore (except the current setup_work_tree) -- 8< -- if (result.worktree) set_git_work_tree(result.worktree); if (result.gitdir) set_git_dir(result.gitdir); if (startup_info->have_repository) repo_set_hash_algo(the_repository, result.repo_fmt.hash_algo); ... return result.prefix; -- 8< -- From here on, it's not hard to see how to turn set_git_work_tree() into setup_work_tree_gently() (without doing any set_git_dir) and the last two calls into "repo_init_gitdir(gitdir, hash_algo)", which should be where we allocate a new repository object and initialize related object store, ref store... But I still have a couple setup corner cases to deal with first :( -- Duy ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-27 17:09 ` Duy Nguyen 2018-03-27 17:30 ` Duy Nguyen @ 2018-03-28 9:47 ` Jeff King 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy 1 sibling, 1 reply; 54+ messages in thread From: Jeff King @ 2018-03-28 9:47 UTC (permalink / raw) To: Duy Nguyen; +Cc: Rafael Ascensao, Git Mailing List On Tue, Mar 27, 2018 at 07:09:36PM +0200, Duy Nguyen wrote: > > I don't quite get why f57f37e2 doesn't want to call git_path(). Is it to > > avoid the way the path is munged? Or is it to avoid some lazy-setup that > > is triggered by calling get_git_dir() at all (which doesn't make much > > sense to me, because we'd already have called get_git_dir() much > > earlier). Or is it just because we may sometimes fill in refs->git_dir > > with something besides get_git_dir() (for a submodule or worktree or > > something)? > > None of those, I think. git_path() does some magic to translate paths > so that refs/... ends up with $GIT_COMMON_DIR/refs/... while "index" > ends up with $GIT_DIR/index. Michael wanted to avoid that magic and > keep the control within refs code (i.e. this code knows refs/ and > packed-refs are shared, and pseudo refs are not, what git_path() > decides does not matter). Ah, OK (that is my first one, "avoid the way the path is munged", but obviously I didn't spell it out very clearly). > > Hmm. Typing that out, it seems like (3) is probably the right path. > > Something like the patch below seems to fix it and passes the tests. > > Honestly I think this is just another way to work around the problem > (with even more changes than your abspath approach). The problem is > with setup_work_tree(). We create a ref store at a specific location > and it should stay working without lazily calling get_git_dir(), which > has nothing to do (anymore) with the path we have given a ref store. > If somebody changes a global setting like $CWD, it should be well > communicated to everybody involved. Yeah, I agree that the root of the problem is not the caching of get_git_dir(), but that chdir() may invalidate assumptions made by other parts of the program. > I would rather have something like ref_store_reinit() in the same > spirit as the second call of set_git_dir() in setup_work_tree. It is > hacky, but it works and keeps changes to minimal (so that it could be > easily replaced later). So the non-hacky solution is to inform all callers that we've changed directories, and they may need to recompute any relative paths. It does seem backwards for setup_work_tree() to need to know about the refs code. Should we have a system by which interested code can register to learn about changes to global state? E.g., something like: typedef void (*chdir_notify_cb)(const char *old_cwd, const char *new_cwd, void *data); /* Register interest in hearing about chdir */ void chdir_notify_register(chdir_notify_cb cb, void *data); /* Do a chdir and then tell everybody about it */ void chdir_notify(const char *path); Then the ref code (or anybody else) should be able to write a function to normalize a relative path from the old_cwd into a relative one from the new_cwd. -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-28 9:47 ` git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars Jeff King @ 2018-03-28 17:55 ` Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 1/8] strbuf.c: add strbuf_ensure_trailing_dr_sep() Nguyễn Thái Ngọc Duy ` (9 more replies) 0 siblings, 10 replies; 54+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 UTC (permalink / raw) To: git; +Cc: Jeff King, Rafael Ascensao, Nguyễn Thái Ngọc Duy On Wed, Mar 28, 2018 at 7:36 PM, Jeff King <peff@peff.net> wrote: > On Wed, Mar 28, 2018 at 12:10:21PM +0200, Duy Nguyen wrote: > >> > I think it might be clearer if a single call is given both the old and >> > new paths. That requires the caller of chdir() storing getcwd() before >> > it moves, but I don't think that should be a big deal. >> >> The problem is switching relative paths relies on the old $CWD if I'm >> not mistaken and we need getcwd() for this. I'd love to have one >> callback that says "$CWD has been switched from this path to that >> path, do whatever you need to" that can be called any time, before or >> after chdir(). I'll look more into it. > > I think it should be OK to save getcwd() and just construct the original > path after the fact. Here's some patches which do that in a nice way. Heh.. I should have checked mails more often while coding ;-) This is what I got, which is slightly different from your series because I want to call set_git_dir() just one time (by setup_git_directory) and never again. I think the API looks close enough. I will probably rework on top of your chdir-notify instead (and let yours to be merged earlier) Note, this one is built on a strange base, which is a merge of 'next' and 'sb/object-store' (I needed 'next' and Junio would have another evil merge if 'sb/object-store' was not in the base). Nguyễn Thái Ngọc Duy (8): strbuf.c: add strbuf_ensure_trailing_dr_sep() strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix) trace.c: export trace_setup_key setup.c: introduce setup_adjust_path() setup.c: allow other code to be notified when $CWD moves environment.c: adjust env containing relpath when $CWD is moved repository: adjust repo paths when $CWD moves refs: adjust main repo paths when $CWD moves abspath.c | 4 +-- builtin/difftool.c | 6 ++--- cache.h | 8 ++++++ dir-iterator.c | 3 +-- environment.c | 46 +++++++++++++++++++++++++++++++++ object-store.h | 3 +++ object.c | 15 +++++++++++ path.c | 9 +++---- refs.c | 10 ++++++++ refs/files-backend.c | 15 +++++++++++ refs/packed-backend.c | 12 +++++++++ refs/refs-internal.h | 4 +++ repository.c | 28 ++++++++++++++++++++ setup.c | 59 ++++++++++++++++++++++++++++++++++--------- strbuf.c | 43 ++++++++++++++++++++----------- strbuf.h | 8 ++++++ trace.c | 14 +++++----- trace.h | 1 + 18 files changed, 239 insertions(+), 49 deletions(-) -- 2.17.0.rc1.439.gca064e2955 ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 1/8] strbuf.c: add strbuf_ensure_trailing_dr_sep() 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 ` Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix) Nguyễn Thái Ngọc Duy ` (8 subsequent siblings) 9 siblings, 0 replies; 54+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 UTC (permalink / raw) To: git; +Cc: Jeff King, Rafael Ascensao, Duy Nguyen From: Duy Nguyen <pclouds@gmail.com> This is just good cleanup and the logic will also be needed in new patches. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- abspath.c | 4 +--- builtin/difftool.c | 6 ++---- dir-iterator.c | 3 +-- path.c | 9 +++------ strbuf.c | 6 ++++++ strbuf.h | 2 ++ 6 files changed, 15 insertions(+), 15 deletions(-) diff --git a/abspath.c b/abspath.c index 9857985329..994075b5c8 100644 --- a/abspath.c +++ b/abspath.c @@ -122,9 +122,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path, continue; } - /* append the next component and resolve resultant path */ - if (!is_dir_sep(resolved->buf[resolved->len - 1])) - strbuf_addch(resolved, '/'); + strbuf_ensure_trailing_dir_sep(resolved); strbuf_addbuf(resolved, &next); if (lstat(resolved->buf, &st)) { diff --git a/builtin/difftool.c b/builtin/difftool.c index ee8dce019e..8d125c7968 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -88,8 +88,7 @@ static int parse_index_info(char *p, int *mode1, int *mode2, static void add_path(struct strbuf *buf, size_t base_len, const char *path) { strbuf_setlen(buf, base_len); - if (buf->len && buf->buf[buf->len - 1] != '/') - strbuf_addch(buf, '/'); + strbuf_ensure_trailing_dir_sep(buf); strbuf_addstr(buf, path); } @@ -362,8 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, strbuf_addf(&ldir, "%s/left/", tmpdir); strbuf_addf(&rdir, "%s/right/", tmpdir); strbuf_addstr(&wtdir, workdir); - if (!wtdir.len || !is_dir_sep(wtdir.buf[wtdir.len - 1])) - strbuf_addch(&wtdir, '/'); + strbuf_ensure_trailing_dir_sep(&wtdir); mkdir(ldir.buf, 0700); mkdir(rdir.buf, 0700); diff --git a/dir-iterator.c b/dir-iterator.c index 34182a9a1c..249b5325cf 100644 --- a/dir-iterator.c +++ b/dir-iterator.c @@ -65,8 +65,7 @@ int dir_iterator_advance(struct dir_iterator *dir_iterator) * Note: dir_iterator_begin() ensures that * path is not the empty string. */ - if (!is_dir_sep(iter->base.path.buf[iter->base.path.len - 1])) - strbuf_addch(&iter->base.path, '/'); + strbuf_ensure_trailing_dir_sep(&iter->base.path); level->prefix_len = iter->base.path.len; level->dir = opendir(iter->base.path.buf); diff --git a/path.c b/path.c index 3308b7b958..cd0ad89868 100644 --- a/path.c +++ b/path.c @@ -408,8 +408,7 @@ static void do_git_path(const struct repository *repo, { int gitdir_len; strbuf_worktree_gitdir(buf, repo, wt); - if (buf->len && !is_dir_sep(buf->buf[buf->len - 1])) - strbuf_addch(buf, '/'); + strbuf_ensure_trailing_dir_sep(buf); gitdir_len = buf->len; strbuf_vaddf(buf, fmt, args); if (!wt) @@ -512,8 +511,7 @@ static void do_worktree_path(const struct repository *repo, const char *fmt, va_list args) { strbuf_addstr(buf, repo->worktree); - if(buf->len && !is_dir_sep(buf->buf[buf->len - 1])) - strbuf_addch(buf, '/'); + strbuf_ensure_trailing_dir_sep(buf); strbuf_vaddf(buf, fmt, args); strbuf_cleanup_path(buf); @@ -608,8 +606,7 @@ static void do_git_common_path(const struct repository *repo, va_list args) { strbuf_addstr(buf, repo->commondir); - if (buf->len && !is_dir_sep(buf->buf[buf->len - 1])) - strbuf_addch(buf, '/'); + strbuf_ensure_trailing_dir_sep(buf); strbuf_vaddf(buf, fmt, args); strbuf_cleanup_path(buf); } diff --git a/strbuf.c b/strbuf.c index 83d05024e6..d5b7cda61e 100644 --- a/strbuf.c +++ b/strbuf.c @@ -122,6 +122,12 @@ void strbuf_ltrim(struct strbuf *sb) sb->buf[sb->len] = '\0'; } +void strbuf_ensure_trailing_dir_sep(struct strbuf *sb) +{ + if (sb->len && !is_dir_sep(sb->buf[sb->len - 1])) + strbuf_addch(sb, '/'); +} + int strbuf_reencode(struct strbuf *sb, const char *from, const char *to) { char *out; diff --git a/strbuf.h b/strbuf.h index c4de5e4588..62dc7f16fa 100644 --- a/strbuf.h +++ b/strbuf.h @@ -189,6 +189,8 @@ extern void strbuf_ltrim(struct strbuf *); /* Strip trailing directory separators */ extern void strbuf_trim_trailing_dir_sep(struct strbuf *); +/* Append trailing directory separator if necessary */ +extern void strbuf_ensure_trailing_dir_sep(struct strbuf *sb); /** * Replace the contents of the strbuf with a reencoded form. Returns -1 -- 2.17.0.rc1.439.gca064e2955 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix) 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 1/8] strbuf.c: add strbuf_ensure_trailing_dr_sep() Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 ` Nguyễn Thái Ngọc Duy 2018-03-28 18:02 ` Stefan Beller 2018-03-28 17:55 ` [PATCH 3/8] trace.c: export trace_setup_key Nguyễn Thái Ngọc Duy ` (7 subsequent siblings) 9 siblings, 1 reply; 54+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 UTC (permalink / raw) To: git; +Cc: Jeff King, Rafael Ascensao, Duy Nguyen From: Duy Nguyen <pclouds@gmail.com> This function was added in 10c4c881c4 (Allow add_path() to add non-existent directories to the path - 2008-07-21) because getcwd() may fail on non-existing cwd and we want to construct non-existing absolute paths sometimes. The function was merged back in strbuf_add_absolute_path() some time later. Move it out again because it will have another caller shortly. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- strbuf.c | 37 ++++++++++++++++++++++--------------- strbuf.h | 6 ++++++ 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/strbuf.c b/strbuf.c index d5b7cda61e..aed4bec856 100644 --- a/strbuf.c +++ b/strbuf.c @@ -746,27 +746,34 @@ void strbuf_humanise_bytes(struct strbuf *buf, off_t bytes) } } +void strbuf_get_pwd_cwd(struct strbuf *sb) +{ + struct stat cwd_stat, pwd_stat; + char *cwd = xgetcwd(); + char *pwd = getenv("PWD"); + + if (pwd && strcmp(pwd, cwd) && + !stat(cwd, &cwd_stat) && + (cwd_stat.st_dev || cwd_stat.st_ino) && + !stat(pwd, &pwd_stat) && + pwd_stat.st_dev == cwd_stat.st_dev && + pwd_stat.st_ino == cwd_stat.st_ino) + strbuf_addstr(sb, pwd); + else + strbuf_addstr(sb, cwd); + free(cwd); +} + void strbuf_add_absolute_path(struct strbuf *sb, const char *path) { if (!*path) die("The empty string is not a valid path"); if (!is_absolute_path(path)) { - struct stat cwd_stat, pwd_stat; size_t orig_len = sb->len; - char *cwd = xgetcwd(); - char *pwd = getenv("PWD"); - if (pwd && strcmp(pwd, cwd) && - !stat(cwd, &cwd_stat) && - (cwd_stat.st_dev || cwd_stat.st_ino) && - !stat(pwd, &pwd_stat) && - pwd_stat.st_dev == cwd_stat.st_dev && - pwd_stat.st_ino == cwd_stat.st_ino) - strbuf_addstr(sb, pwd); - else - strbuf_addstr(sb, cwd); - if (sb->len > orig_len && !is_dir_sep(sb->buf[sb->len - 1])) - strbuf_addch(sb, '/'); - free(cwd); + + strbuf_get_pwd_cwd(sb); + if (sb->len > orig_len) + strbuf_ensure_trailing_dir_sep(sb); } strbuf_addstr(sb, path); } diff --git a/strbuf.h b/strbuf.h index 62dc7f16fa..f712c4ff92 100644 --- a/strbuf.h +++ b/strbuf.h @@ -458,6 +458,12 @@ extern int strbuf_getwholeline_fd(struct strbuf *, int, int); */ extern int strbuf_getcwd(struct strbuf *sb); +/** + * Return the current directory, fall back to $PWD if the + * current directory does not exist. + */ +extern void strbuf_get_pwd_cwd(struct strbuf *sb); + /** * Add a path to a buffer, converting a relative path to an * absolute one in the process. Symbolic links are not -- 2.17.0.rc1.439.gca064e2955 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix) 2018-03-28 17:55 ` [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix) Nguyễn Thái Ngọc Duy @ 2018-03-28 18:02 ` Stefan Beller 2018-03-28 18:05 ` Duy Nguyen 0 siblings, 1 reply; 54+ messages in thread From: Stefan Beller @ 2018-03-28 18:02 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, Rafael Ascensao On Wed, Mar 28, 2018 at 10:55 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote: > +/** > + * Return the current directory, fall back to $PWD if the > + * current directory does not exist. > + */ > +extern void strbuf_get_pwd_cwd(struct strbuf *sb); Please see 89a9f2c862 (CodingGuidelines: mention "static" and "extern", 2018-02-08) and drop the extern if you need to reroll. Thanks, Stefan ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix) 2018-03-28 18:02 ` Stefan Beller @ 2018-03-28 18:05 ` Duy Nguyen 0 siblings, 0 replies; 54+ messages in thread From: Duy Nguyen @ 2018-03-28 18:05 UTC (permalink / raw) To: Stefan Beller; +Cc: git, Jeff King, Rafael Ascensao On Wed, Mar 28, 2018 at 8:02 PM, Stefan Beller <sbeller@google.com> wrote: > On Wed, Mar 28, 2018 at 10:55 AM, Nguyễn Thái Ngọc Duy > <pclouds@gmail.com> wrote: >> +/** >> + * Return the current directory, fall back to $PWD if the >> + * current directory does not exist. >> + */ >> +extern void strbuf_get_pwd_cwd(struct strbuf *sb); > > Please see 89a9f2c862 (CodingGuidelines: mention "static" and "extern", > 2018-02-08) and drop the extern if you need to reroll. I'm aware (and in favor) of that actually. But this whole strbuf.h uses extern. Either I had to delete all extern first, or go with old style and maybe do the whole deletion later. I chose the former. Maybe be I'll delete all "extern" as a prep patch. -- Duy ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 3/8] trace.c: export trace_setup_key 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 1/8] strbuf.c: add strbuf_ensure_trailing_dr_sep() Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix) Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 ` Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 4/8] setup.c: introduce setup_adjust_path() Nguyễn Thái Ngọc Duy ` (6 subsequent siblings) 9 siblings, 0 replies; 54+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 UTC (permalink / raw) To: git; +Cc: Jeff King, Rafael Ascensao, Nguyễn Thái Ngọc Duy 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 <pclouds@gmail.com> --- trace.c | 14 +++++++------- trace.h | 1 + 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/trace.c b/trace.c index 7f3b08e148..fc623e91fd 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 88055abef7..2b6a1bc17c 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); -- 2.17.0.rc1.439.gca064e2955 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 4/8] setup.c: introduce setup_adjust_path() 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy ` (2 preceding siblings ...) 2018-03-28 17:55 ` [PATCH 3/8] trace.c: export trace_setup_key Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 ` Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 5/8] setup.c: allow other code to be notified when $CWD moves Nguyễn Thái Ngọc Duy ` (5 subsequent siblings) 9 siblings, 0 replies; 54+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 UTC (permalink / raw) To: git; +Cc: Jeff King, Rafael Ascensao, Duy Nguyen From: Duy Nguyen <pclouds@gmail.com> When $CWD is moved, relative path must be updated to be relative to the new $CWD. This function helps do that. The _in_place version is just for convenient (and we will use it quite often in subsequent patches). Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- cache.h | 3 +++ setup.c | 20 ++++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/cache.h b/cache.h index bbaf5c349a..05f32c9659 100644 --- a/cache.h +++ b/cache.h @@ -522,6 +522,9 @@ extern void set_git_work_tree(const char *tree); #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES" +extern void setup_adjust_path(const char *name, char **path, + const char *old_cwd, + const char *new_cwd); extern void setup_work_tree(void); /* * Find the commondir and gitdir of the repository that contains the current diff --git a/setup.c b/setup.c index 664453fcef..e26f44185e 100644 --- a/setup.c +++ b/setup.c @@ -376,6 +376,26 @@ int is_inside_work_tree(void) return inside_work_tree; } +void setup_adjust_path(const char *name, char **path, + const char *old_cwd, + const char *new_cwd) +{ + char *old_path = *path; + struct strbuf sb = STRBUF_INIT; + + if (!old_path || is_absolute_path(old_path)) + return; + + strbuf_addstr(&sb, old_cwd); + strbuf_ensure_trailing_dir_sep(&sb); + strbuf_addstr(&sb, old_path); + *path = xstrdup(remove_leading_path(sb.buf, new_cwd)); + trace_printf_key(&trace_setup_key, "setup: adjust '%s' to %s", + name, *path); + strbuf_release(&sb); + free(old_path); +} + void setup_work_tree(void) { const char *work_tree, *git_dir; -- 2.17.0.rc1.439.gca064e2955 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 5/8] setup.c: allow other code to be notified when $CWD moves 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy ` (3 preceding siblings ...) 2018-03-28 17:55 ` [PATCH 4/8] setup.c: introduce setup_adjust_path() Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 ` Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved Nguyễn Thái Ngọc Duy ` (4 subsequent siblings) 9 siblings, 0 replies; 54+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 UTC (permalink / raw) To: git; +Cc: Jeff King, Rafael Ascensao, Duy Nguyen From: Duy Nguyen <pclouds@gmail.com> When the current directory is moved, all relative paths may become invalid because they are still relative to the old current directory. At this point in setup_work_tree() many objects have been initialized and some keep relative paths in their data structure. $GIT_DIR and $GIT_WORK_TREE for example are the two examples which are dealt with explicitly in this code: $GIT_WORK_TREE is reset to "." and set_git_dir() is called the second time with a new relative path. Introduce a more generic mechanism to let all code get notified and do the path adjustment themselves instead of pulling all path adjustment logic in here. The $GIT_DIR and $GIT_WORK_TREE adjustments will removed from this code in later patches. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- cache.h | 5 +++++ setup.c | 27 ++++++++++++++++++++++++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/cache.h b/cache.h index 05f32c9659..895abe7e7e 100644 --- a/cache.h +++ b/cache.h @@ -522,6 +522,11 @@ extern void set_git_work_tree(const char *tree); #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES" +typedef void (*cwd_updated_fn)(const char *old_cwd, + const char *new_cwd, + void *cb_data); +void add_cwd_update_callback(cwd_updated_fn fn, void *cb_data); +void removet_cwd_update_callback(cwd_updated_fn fn, void *cb_data); extern void setup_adjust_path(const char *name, char **path, const char *old_cwd, const char *new_cwd); diff --git a/setup.c b/setup.c index e26f44185e..e340ee2130 100644 --- a/setup.c +++ b/setup.c @@ -376,6 +376,24 @@ int is_inside_work_tree(void) return inside_work_tree; } +struct cwd_update_callback { + cwd_updated_fn fn; + void *cb_data; +}; + +static struct cwd_update_callback *cwd_callbacks; +static int nr_cwd_callbacks; + +void add_cwd_update_callback(cwd_updated_fn fn, void *cb_data) +{ + struct cwd_update_callback *cb; + + REALLOC_ARRAY(cwd_callbacks, nr_cwd_callbacks + 1); + cb = cwd_callbacks + nr_cwd_callbacks++; + cb->fn = fn; + cb->cb_data = cb_data; +} + void setup_adjust_path(const char *name, char **path, const char *old_cwd, const char *new_cwd) @@ -398,8 +416,10 @@ void setup_adjust_path(const char *name, char **path, void setup_work_tree(void) { - const char *work_tree, *git_dir; static int initialized = 0; + const char *work_tree, *git_dir; + struct strbuf old_cwd = STRBUF_INIT; + int i; if (initialized) return; @@ -411,6 +431,7 @@ void setup_work_tree(void) git_dir = get_git_dir(); if (!is_absolute_path(git_dir)) git_dir = real_path(get_git_dir()); + strbuf_get_pwd_cwd(&old_cwd); if (!work_tree || chdir(work_tree)) die(_("this operation must be run in a work tree")); @@ -421,6 +442,10 @@ void setup_work_tree(void) if (getenv(GIT_WORK_TREE_ENVIRONMENT)) setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1); + for (i = 0; i < nr_cwd_callbacks; i++) + cwd_callbacks[i].fn(old_cwd.buf, work_tree, + cwd_callbacks[i].cb_data); + strbuf_release(&old_cwd); set_git_dir(remove_leading_path(git_dir, work_tree)); initialized = 1; } -- 2.17.0.rc1.439.gca064e2955 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy ` (4 preceding siblings ...) 2018-03-28 17:55 ` [PATCH 5/8] setup.c: allow other code to be notified when $CWD moves Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 ` Nguyễn Thái Ngọc Duy 2018-03-28 18:30 ` Jeff King 2018-03-28 17:55 ` [PATCH 7/8] repository: adjust repo paths when $CWD moves Nguyễn Thái Ngọc Duy ` (3 subsequent siblings) 9 siblings, 1 reply; 54+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 UTC (permalink / raw) To: git; +Cc: Jeff King, Rafael Ascensao, Duy Nguyen From: Duy Nguyen <pclouds@gmail.com> As noted in the previous patch, when $CWD is moved, we recognize the problem with relative paths and update $GIT_WORK_TREE and $GIT_DIR with new ones. We have plenty more environment variables that can contain paths though. If they are read and cached before setup_work_tree() is called, nobody will update them and they become bad paths. Hook into setup_work_tree() and update all those env variables. The code to update $GIT_WORK_TREE is no longer needed and removed. Technically we should remove the setenv() in set_git_dir() as well, but that is also called _not_ by setup_work_tree() and we should keep the behavior the same in that case for safety. set_git_dir() will be removed from setup_work_tree() soon, which achieves the same goal. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- environment.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ setup.c | 7 ------- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/environment.c b/environment.c index 39b3d906c8..f9dcc1b99e 100644 --- a/environment.c +++ b/environment.c @@ -128,6 +128,20 @@ const char * const local_repo_env[] = { NULL }; +/* A subset of local_repo_env[] that contains path */ +const char * const local_repo_path_env[] = { + ALTERNATE_DB_ENVIRONMENT, + CONFIG_ENVIRONMENT, + DB_ENVIRONMENT, + GIT_COMMON_DIR_ENVIRONMENT, + GIT_DIR_ENVIRONMENT, + GIT_SHALLOW_FILE_ENVIRONMENT, + GIT_WORK_TREE_ENVIRONMENT, + GRAFT_ENVIRONMENT, + INDEX_ENVIRONMENT, + NULL +}; + static char *expand_namespace(const char *raw_namespace) { struct strbuf buf = STRBUF_INIT; @@ -149,6 +163,32 @@ static char *expand_namespace(const char *raw_namespace) return strbuf_detach(&buf, NULL); } +static void update_path_envs(const char *old_cwd, const char *new_cwd, + void *cb) +{ + int i; + + /* + * FIXME: special treatment needed for + * GIT_ALTERNATE_OBJECT_DIRECTORIES because it can contain + * multiple paths. + */ + for (i = 0; local_repo_path_env[i]; i++) { + const char *name = local_repo_path_env[i]; + const char *value = getenv(name); + char *new_value; + + if (!value) + continue; + if (is_absolute_path(value)) + continue; + new_value = xstrdup(value); + setup_adjust_path(name, &new_value, old_cwd, new_cwd); + if (setenv(name, new_value, 10)) + error(_("could not set %s to '%s'"), name, value); + free(new_value); + } +} /* * Wrapper of getenv() that returns a strdup value. This value is kept * in argv to be freed later. @@ -170,6 +210,12 @@ void setup_git_env(const char *git_dir) const char *replace_ref_base; struct set_gitdir_args args = { NULL }; struct argv_array to_free = ARGV_ARRAY_INIT; + static int added_cwd_callback; + + if (!added_cwd_callback) { + add_cwd_update_callback(update_path_envs, NULL); + added_cwd_callback = 1; + } args.commondir = getenv_safe(&to_free, GIT_COMMON_DIR_ENVIRONMENT); args.object_dir = getenv_safe(&to_free, DB_ENVIRONMENT); diff --git a/setup.c b/setup.c index e340ee2130..23b8f89ce2 100644 --- a/setup.c +++ b/setup.c @@ -435,13 +435,6 @@ void setup_work_tree(void) if (!work_tree || chdir(work_tree)) die(_("this operation must be run in a work tree")); - /* - * Make sure subsequent git processes find correct worktree - * if $GIT_WORK_TREE is set relative - */ - if (getenv(GIT_WORK_TREE_ENVIRONMENT)) - setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1); - for (i = 0; i < nr_cwd_callbacks; i++) cwd_callbacks[i].fn(old_cwd.buf, work_tree, cwd_callbacks[i].cb_data); -- 2.17.0.rc1.439.gca064e2955 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved 2018-03-28 17:55 ` [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved Nguyễn Thái Ngọc Duy @ 2018-03-28 18:30 ` Jeff King 2018-03-28 18:45 ` Duy Nguyen 0 siblings, 1 reply; 54+ messages in thread From: Jeff King @ 2018-03-28 18:30 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Rafael Ascensao On Wed, Mar 28, 2018 at 07:55:35PM +0200, Nguyễn Thái Ngọc Duy wrote: > From: Duy Nguyen <pclouds@gmail.com> > > As noted in the previous patch, when $CWD is moved, we recognize the > problem with relative paths and update $GIT_WORK_TREE and $GIT_DIR > with new ones. > > We have plenty more environment variables that can contain paths > though. If they are read and cached before setup_work_tree() is > called, nobody will update them and they become bad paths. Hmm, yeah, I missed these. It would be interesting to know if there are easy-to-run test cases that show off these bugs, or if they're hypothetical. (Even if they are hypothetical, I'm not opposed to fixing them in the name of maintainability). > Hook into setup_work_tree() and update all those env variables. The > code to update $GIT_WORK_TREE is no longer needed and removed. That's a nice cleanup. > Technically we should remove the setenv() in set_git_dir() as well, > but that is also called _not_ by setup_work_tree() and we should keep > the behavior the same in that case for safety. set_git_dir() will be > removed from setup_work_tree() soon, which achieves the same goal. Makes sense. > diff --git a/environment.c b/environment.c > index 39b3d906c8..f9dcc1b99e 100644 > --- a/environment.c > +++ b/environment.c > @@ -128,6 +128,20 @@ const char * const local_repo_env[] = { > NULL > }; > > +/* A subset of local_repo_env[] that contains path */ > +const char * const local_repo_path_env[] = { > + ALTERNATE_DB_ENVIRONMENT, > + CONFIG_ENVIRONMENT, > + DB_ENVIRONMENT, > + GIT_COMMON_DIR_ENVIRONMENT, > + GIT_DIR_ENVIRONMENT, > + GIT_SHALLOW_FILE_ENVIRONMENT, > + GIT_WORK_TREE_ENVIRONMENT, > + GRAFT_ENVIRONMENT, > + INDEX_ENVIRONMENT, > + NULL > +}; It might be nice to fold this list into local_repo_env automatically. I think you'd have to do it with a macro. OTOH, it's possible that there could be a path-related environment variable that _isn't_ actually part of local_repo_env. E.g., I think GIT_CONFIG might classify there (though I don't know if it's worth worrying about). > +static void update_path_envs(const char *old_cwd, const char *new_cwd, > + void *cb) > +{ > + int i; > + > + /* > + * FIXME: special treatment needed for > + * GIT_ALTERNATE_OBJECT_DIRECTORIES because it can contain > + * multiple paths. > + */ Yuck. It just keeps getting more complicated. :( I do wonder if relative paths in variables like this are worth worrying about. AFAIK, it's always been a "well, it kind of works" situation, but not something we've tried to actively support. I think with the current code you'd potentially get inconsistent results between a command which sets up the work tree and one which doesn't. So this would be fixing that, but at the same time, I'm not sure how much we want to promise here. -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved 2018-03-28 18:30 ` Jeff King @ 2018-03-28 18:45 ` Duy Nguyen 0 siblings, 0 replies; 54+ messages in thread From: Duy Nguyen @ 2018-03-28 18:45 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Rafael Ascensao On Wed, Mar 28, 2018 at 8:30 PM, Jeff King <peff@peff.net> wrote: > On Wed, Mar 28, 2018 at 07:55:35PM +0200, Nguyễn Thái Ngọc Duy wrote: > >> From: Duy Nguyen <pclouds@gmail.com> >> >> As noted in the previous patch, when $CWD is moved, we recognize the >> problem with relative paths and update $GIT_WORK_TREE and $GIT_DIR >> with new ones. >> >> We have plenty more environment variables that can contain paths >> though. If they are read and cached before setup_work_tree() is >> called, nobody will update them and they become bad paths. > > Hmm, yeah, I missed these. It would be interesting to know if there are > easy-to-run test cases that show off these bugs, or if they're > hypothetical. (Even if they are hypothetical, I'm not opposed to fixing > them in the name of maintainability). It's kinda hard to show off these. But the GIT_ALTERNATE_OBJ.. variable could be one example in favor of this fix. Before, we lazily read the env var which is most likely after setup_work_tree() has run. After a bunch of code reorganization and stuff, GIT_ALTERNATE_OBJ is now read very early, which should be safe (why not?) but it actually breaks. Alternate db is only queried as the last resort if I'm not mistaken, so 90% of time you just hit an object in odb and never find out that your GIT_ALTERNATE_OBJ... points to nowhere. >> diff --git a/environment.c b/environment.c >> index 39b3d906c8..f9dcc1b99e 100644 >> --- a/environment.c >> +++ b/environment.c >> @@ -128,6 +128,20 @@ const char * const local_repo_env[] = { >> NULL >> }; >> >> +/* A subset of local_repo_env[] that contains path */ >> +const char * const local_repo_path_env[] = { >> + ALTERNATE_DB_ENVIRONMENT, >> + CONFIG_ENVIRONMENT, >> + DB_ENVIRONMENT, >> + GIT_COMMON_DIR_ENVIRONMENT, >> + GIT_DIR_ENVIRONMENT, >> + GIT_SHALLOW_FILE_ENVIRONMENT, >> + GIT_WORK_TREE_ENVIRONMENT, >> + GRAFT_ENVIRONMENT, >> + INDEX_ENVIRONMENT, >> + NULL >> +}; > > It might be nice to fold this list into local_repo_env automatically. I > think you'd have to do it with a macro. Aha! I did not like the split either and wanted to turn local_repo_env[] to an array of struct so we can add attributes to each variable, but the way local_repo_env[] is being used, that's impossible without more surgery. > OTOH, it's possible that there could be a path-related environment > variable that _isn't_ actually part of local_repo_env. E.g., I think > GIT_CONFIG might classify there (though I don't know if it's worth > worrying about). I'd rather fix it now and forget about it than trying to troubleshoot a problem related to bad relative $GIT_CONFIG (even if the chance of that happening is probably 1%) >> +static void update_path_envs(const char *old_cwd, const char *new_cwd, >> + void *cb) >> +{ >> + int i; >> + >> + /* >> + * FIXME: special treatment needed for >> + * GIT_ALTERNATE_OBJECT_DIRECTORIES because it can contain >> + * multiple paths. >> + */ > > Yuck. It just keeps getting more complicated. :( > > I do wonder if relative paths in variables like this are worth worrying > about. AFAIK, it's always been a "well, it kind of works" situation, but Yeah. 99% of time $GIT_DIR is $GIT_WORK_TREE/.git and no path adjustment is needed. > not something we've tried to actively support. I think with the current > code you'd potentially get inconsistent results between a command which > sets up the work tree and one which doesn't. So this would be fixing > that, but at the same time, I'm not sure how much we want to promise > here. I would be just as happy to die() when we find out we have relative paths that takes too much work to reparent. It keeps the amount of work down and will not bite us later (and will let us know if any user needs it because they would have to report back after hitting the said die()). -- Duy ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 7/8] repository: adjust repo paths when $CWD moves 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy ` (5 preceding siblings ...) 2018-03-28 17:55 ` [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 ` Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 8/8] refs: adjust main " Nguyễn Thái Ngọc Duy ` (2 subsequent siblings) 9 siblings, 0 replies; 54+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 UTC (permalink / raw) To: git; +Cc: Jeff King, Rafael Ascensao, Duy Nguyen From: Duy Nguyen <pclouds@gmail.com> As noted in previous and previous patches, when setup_work_tree() moves $CWD, it calls set_git_dir() with a new path to make sure relative $GIT_DIR is still correct. The same problem from the previous patch applies here: if $GIT_ALTERNATE_OBJECT_DIRECTORIES or $GIT_OBJECT_DIRECTORY are also relative, the user is screwed. Fortuntely the previous patch indirectly fixes this as well: update_path_envs() in environment.c is actually called before setup_git_env() is indirectly called by setup_work_tree() via set_git_dir(). At that point, it correctly gets the updated paths from env and pass them down. This patch is here for another reason. From the design perspective, calling set_git_dir() the second time [1] is just bad because it could potentially be used to point repo code to _another_ repository. The promise that "the new path actually points to the same place as the old one" is not guaranteed and also hard to see unless you know setup code well. This patch avoids this. The second set_git_dir() call is just a workaround to adjust paths. We can now do it directly from repo code by subscribing to the "$CWD updated" event and do everything needed by the repo. After this we could die() on subsequent repo_set_gitdir() calls because we do not support it (we never did). [1] The first time is done by setup_git_directory() code, which is correct: we have just found a repo and we want to initialize our code to read from that repo. Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- object-store.h | 3 +++ object.c | 15 +++++++++++++++ repository.c | 28 ++++++++++++++++++++++++++++ setup.c | 6 +----- 4 files changed, 47 insertions(+), 5 deletions(-) diff --git a/object-store.h b/object-store.h index fef33f345f..4af2c53bca 100644 --- a/object-store.h +++ b/object-store.h @@ -119,6 +119,9 @@ struct raw_object_store { }; struct raw_object_store *raw_object_store_new(void); +void raw_object_store_adjust_paths(struct raw_object_store *o, + const char *old_cwd, + const char *new_cwd); void raw_object_store_clear(struct raw_object_store *o); /* diff --git a/object.c b/object.c index a0a756f24f..5318922efd 100644 --- a/object.c +++ b/object.c @@ -457,6 +457,21 @@ struct raw_object_store *raw_object_store_new(void) return o; } +void raw_object_store_adjust_paths(struct raw_object_store *o, + const char *old_cwd, + const char *new_cwd) +{ + /* + * "main repo" is technically wrong because we don't know that + * from here. But it's the only repo that will execute this + * function for now. + */ + setup_adjust_path("main repo's object dir", + &o->objectdir, old_cwd, new_cwd); + setup_adjust_path("main repo's alt dir", + &o->alternate_db, old_cwd, new_cwd); +} + static void free_alt_odb(struct alternate_object_database *alt) { strbuf_release(&alt->scratch); diff --git a/repository.c b/repository.c index a4848c1bd0..c2edf227fe 100644 --- a/repository.c +++ b/repository.c @@ -8,6 +8,10 @@ static struct repository the_repo; struct repository *the_repository; +static void repo_adjust_paths(const char *old_cwd, + const char *new_cwd, + void *cb_data); + void initialize_the_repository(void) { the_repository = &the_repo; @@ -15,6 +19,11 @@ void initialize_the_repository(void) the_repo.index = &the_index; the_repo.objects = raw_object_store_new(); repo_set_hash_algo(&the_repo, GIT_HASH_SHA1); + /* + * non-main repos probably does not need this since $CWD should + * never change again by the time they are created. + */ + add_cwd_update_callback(repo_adjust_paths, the_repository); } static void expand_base_dir(char **out, const char *in, @@ -70,6 +79,25 @@ void repo_set_gitdir(struct repository *repo, repo->gitdir, "index"); } +static void repo_adjust_paths(const char *old_cwd, const char *new_cwd, + void *cb_data) +{ + struct repository *repo = cb_data; + + if (repo != the_repository) + die("BUG: this is supposed to happen to main repo only"); + + setup_adjust_path("main repo's gitdir", + &repo->gitdir, old_cwd, new_cwd); + setup_adjust_path("main repo's commondir", + &repo->commondir, old_cwd, new_cwd); + setup_adjust_path("main repo's graft file", + &repo->graft_file, old_cwd, new_cwd); + setup_adjust_path("main repo's index file", + &repo->index_file, old_cwd, new_cwd); + raw_object_store_adjust_paths(repo->objects, old_cwd, new_cwd); +} + void repo_set_hash_algo(struct repository *repo, int hash_algo) { repo->hash_algo = &hash_algos[hash_algo]; diff --git a/setup.c b/setup.c index 23b8f89ce2..e364aea7e5 100644 --- a/setup.c +++ b/setup.c @@ -417,7 +417,7 @@ void setup_adjust_path(const char *name, char **path, void setup_work_tree(void) { static int initialized = 0; - const char *work_tree, *git_dir; + const char *work_tree; struct strbuf old_cwd = STRBUF_INIT; int i; @@ -428,9 +428,6 @@ 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()); strbuf_get_pwd_cwd(&old_cwd); if (!work_tree || chdir(work_tree)) die(_("this operation must be run in a work tree")); @@ -439,7 +436,6 @@ void setup_work_tree(void) cwd_callbacks[i].fn(old_cwd.buf, work_tree, cwd_callbacks[i].cb_data); strbuf_release(&old_cwd); - set_git_dir(remove_leading_path(git_dir, work_tree)); initialized = 1; } -- 2.17.0.rc1.439.gca064e2955 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 8/8] refs: adjust main repo paths when $CWD moves 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy ` (6 preceding siblings ...) 2018-03-28 17:55 ` [PATCH 7/8] repository: adjust repo paths when $CWD moves Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 ` Nguyễn Thái Ngọc Duy 2018-03-28 18:19 ` [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars Jeff King 2018-03-28 22:24 ` Junio C Hamano 9 siblings, 0 replies; 54+ messages in thread From: Nguyễn Thái Ngọc Duy @ 2018-03-28 17:55 UTC (permalink / raw) To: git; +Cc: Jeff King, Rafael Ascensao, Duy Nguyen From: Duy Nguyen <pclouds@gmail.com> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> --- refs.c | 10 ++++++++++ refs/files-backend.c | 15 +++++++++++++++ refs/packed-backend.c | 12 ++++++++++++ refs/refs-internal.h | 4 ++++ setup.c | 1 + 5 files changed, 42 insertions(+) diff --git a/refs.c b/refs.c index 8b7a77fe5e..2457a31d4d 100644 --- a/refs.c +++ b/refs.c @@ -1651,12 +1651,22 @@ static struct ref_store *ref_store_init(const char *gitdir, return refs; } +static void ref_store_adjust_paths(const char *old_cwd, + const char *new_cwd, + void *cb) +{ + struct ref_store *refs = cb; + + refs->be->adjust_paths(refs, old_cwd, new_cwd); +} + struct ref_store *get_main_ref_store(void) { if (main_ref_store) return main_ref_store; main_ref_store = ref_store_init(get_git_dir(), REF_STORE_ALL_CAPS); + add_cwd_update_callback(ref_store_adjust_paths, main_ref_store); return main_ref_store; } diff --git a/refs/files-backend.c b/refs/files-backend.c index bec8e30e9e..d35a8db844 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -3092,6 +3092,20 @@ static int files_reflog_expire(struct ref_store *ref_store, return -1; } +static void files_adjust_paths(struct ref_store *ref_store, + const char *old_cwd, + const char *new_cwd) +{ + struct files_ref_store *refs = + files_downcast(ref_store, REF_STORE_WRITE, + "files_adjust_paths"); + + setup_adjust_path("ref store's gitdir", + &refs->gitdir, old_cwd, new_cwd); + setup_adjust_path("ref store's commondir", + &refs->gitcommondir, old_cwd, new_cwd); +} + static int files_init_db(struct ref_store *ref_store, struct strbuf *err) { struct files_ref_store *refs = @@ -3117,6 +3131,7 @@ struct ref_storage_be refs_be_files = { "files", files_ref_store_create, files_init_db, + files_adjust_paths, files_transaction_prepare, files_transaction_finish, files_transaction_abort, diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 65288c6472..764d1250a5 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1047,6 +1047,17 @@ int packed_refs_is_locked(struct ref_store *ref_store) return is_lock_file_locked(&refs->lock); } +static void packed_adjust_paths(struct ref_store *ref_store, + const char *old_cwd, + const char *new_cwd) +{ + struct packed_ref_store *refs = + packed_downcast(ref_store, REF_STORE_WRITE, + "packed_adjust_paths"); + + setup_adjust_path("packed-refs", &refs->path, old_cwd, new_cwd); +} + /* * The packed-refs header line that we write out. Perhaps other traits * will be added later. @@ -1632,6 +1643,7 @@ struct ref_storage_be refs_be_packed = { "packed", packed_ref_store_create, packed_init_db, + packed_adjust_paths, packed_transaction_prepare, packed_transaction_finish, packed_transaction_abort, diff --git a/refs/refs-internal.h b/refs/refs-internal.h index dd834314bd..73480543c0 100644 --- a/refs/refs-internal.h +++ b/refs/refs-internal.h @@ -505,6 +505,9 @@ struct ref_store; */ typedef struct ref_store *ref_store_init_fn(const char *gitdir, unsigned int flags); +typedef void ref_store_adjust_paths_fn(struct ref_store *refs, + const char *old_cwd, + const char *new_cwd); typedef int ref_init_db_fn(struct ref_store *refs, struct strbuf *err); @@ -625,6 +628,7 @@ struct ref_storage_be { const char *name; ref_store_init_fn *init; ref_init_db_fn *init_db; + ref_store_adjust_paths_fn *adjust_paths; ref_transaction_prepare_fn *transaction_prepare; ref_transaction_finish_fn *transaction_finish; diff --git a/setup.c b/setup.c index e364aea7e5..35e89a03e5 100644 --- a/setup.c +++ b/setup.c @@ -3,6 +3,7 @@ #include "config.h" #include "dir.h" #include "string-list.h" +#include "refs.h" static int inside_git_dir = -1; static int inside_work_tree = -1; -- 2.17.0.rc1.439.gca064e2955 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy ` (7 preceding siblings ...) 2018-03-28 17:55 ` [PATCH 8/8] refs: adjust main " Nguyễn Thái Ngọc Duy @ 2018-03-28 18:19 ` Jeff King 2018-03-29 14:57 ` Duy Nguyen 2018-03-28 22:24 ` Junio C Hamano 9 siblings, 1 reply; 54+ messages in thread From: Jeff King @ 2018-03-28 18:19 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Rafael Ascensao On Wed, Mar 28, 2018 at 07:55:29PM +0200, Nguyễn Thái Ngọc Duy wrote: > >> The problem is switching relative paths relies on the old $CWD if I'm > >> not mistaken and we need getcwd() for this. I'd love to have one > >> callback that says "$CWD has been switched from this path to that > >> path, do whatever you need to" that can be called any time, before or > >> after chdir(). I'll look more into it. > > > > I think it should be OK to save getcwd() and just construct the original > > path after the fact. Here's some patches which do that in a nice way. > > Heh.. I should have checked mails more often while coding ;-) I was worried about that. I started this earlier as a "well, I could probably just knock this out quickly" task. Many hours later, I found there were quite a few subtleties. :) > This is what I got, which is slightly different from your series > because I want to call set_git_dir() just one time (by > setup_git_directory) and never again. I think the API looks close > enough. Yeah, I actually think after this series we could enforce that set_git_dir() is never called twice. I think we could even make sure that repo_set_gitdir() is never called twice, too, but it would involve a little more surgery (we'd have to teach it to set up a child_notify handler, and the way it interacts with the set_git_dir() one is subtle. I did try it and it worked, but I went with what you saw in patch 3 because it was simpler). > I will probably rework on top of your chdir-notify instead (and let > yours to be merged earlier) Thanks. I like some of the related changes you made, like including this in the tracing output. That should be easy to do on top of mine, I think. I didn't look for other spots that might need similar treatment (like the object-store bits). -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-28 18:19 ` [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars Jeff King @ 2018-03-29 14:57 ` Duy Nguyen 2018-03-30 17:21 ` Jeff King 0 siblings, 1 reply; 54+ messages in thread From: Duy Nguyen @ 2018-03-29 14:57 UTC (permalink / raw) To: Jeff King; +Cc: git, Rafael Ascensao On Wed, Mar 28, 2018 at 02:19:32PM -0400, Jeff King wrote: > > > I will probably rework on top of your chdir-notify instead (and let > > yours to be merged earlier) > > Thanks. I like some of the related changes you made, like including this > in the tracing output. That should be easy to do on top of mine, I > think. Yeah. But is it possible to sneak something like this in your series (I assume you will reroll anyway)? I could do it separately, but it looks nicer if it's split out and merged in individual patches that add new chdir-notify call site. diff --git a/chdir-notify.c b/chdir-notify.c index 7034e98d71..a2a33443f8 100644 --- a/chdir-notify.c +++ b/chdir-notify.c @@ -4,21 +4,26 @@ #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(chdir_notify_callback cb, void *data) +void chdir_notify_register(const char *name, + chdir_notify_callback cb, + void *data) { struct chdir_notify_entry *e = xmalloc(sizeof(*e)); e->cb = cb; e->data = data; + e->name = name; list_add_tail(&e->list, &chdir_notify_entries); } -static void reparent_cb(const char *old_cwd, +static void reparent_cb(const char *name, + const char *old_cwd, const char *new_cwd, void *data) { @@ -28,12 +33,16 @@ static void reparent_cb(const char *old_cwd, if (!tmp || !is_absolute_path(tmp)) return; *path = reparent_relative_path(old_cwd, new_cwd, tmp); + if (name) + trace_printf_key(&trace_setup_key, + "setup: reparent %s to '%s'", + name, *path); free(tmp); } -void chdir_notify_reparent(char **path) +void chdir_notify_reparent(const char *name, char **path) { - chdir_notify_register(reparent_cb, path); + chdir_notify_register(name, reparent_cb, path); } int chdir_notify(const char *new_cwd) @@ -45,11 +54,12 @@ int chdir_notify(const char *new_cwd) return -1; if (chdir(new_cwd) < 0) return -1; + trace_printf_key(&trace_setup_key, "setup: chdir to '%s'", new_cwd); list_for_each(pos, &chdir_notify_entries) { struct chdir_notify_entry *e = list_entry(pos, struct chdir_notify_entry, list); - e->cb(old_cwd.buf, new_cwd, e->data); + e->cb(e->name, old_cwd.buf, new_cwd, e->data); } strbuf_release(&old_cwd); diff --git a/chdir-notify.h b/chdir-notify.h index c3bd818a85..b9be1b54ac 100644 --- a/chdir-notify.h +++ b/chdir-notify.h @@ -16,23 +16,29 @@ * warning("switched from %s to %s!", old_path, new_path); * } * ... - * chdir_notify_register(foo, data); + * 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(&my_path); + * chdir_notify_reparent("description", &my_path); * * 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 first argument is used for tracing purpose only. when my_path is updated + * by chdir_notify_reparent() it will print a message if $GIT_TRACE_SETUP is set. + * This argument could be NULL. */ -typedef void (*chdir_notify_callback)(const char *old_cwd, +typedef void (*chdir_notify_callback)(const char *name, + const char *old_cwd, const char *new_cwd, void *data); -void chdir_notify_register(chdir_notify_callback cb, void *data); -void chdir_notify_reparent(char **path); +void chdir_notify_register(const char *name, chdir_notify_callback cb, + void *data); +void chdir_notify_reparent(const char *name, char **path); /* * diff --git a/environment.c b/environment.c index 794a75a717..92701cbc0c 100644 --- a/environment.c +++ b/environment.c @@ -330,11 +330,15 @@ static void set_git_dir_1(const char *path) setup_git_env(path); } -static void update_relative_gitdir(const char *old_cwd, +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 $GIR_DIR and others to '%s'", + path); set_git_dir_1(path); free(path); } @@ -343,7 +347,7 @@ void set_git_dir(const char *path) { set_git_dir_1(path); if (!is_absolute_path(path)) - chdir_notify_register(update_relative_gitdir, NULL); + chdir_notify_register(NULL, update_relative_gitdir, NULL); } const char *get_log_output_encoding(void) diff --git a/refs/files-backend.c b/refs/files-backend.c index ab3e00ffa0..861072c0dc 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -107,8 +107,8 @@ 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(&refs->gitdir); - chdir_notify_reparent(&refs->gitcommondir); + chdir_notify_reparent("files-backend $GIT_DIR", &refs->gitdir); + chdir_notify_reparent("files-backedn $GIT_COMMONDIR", &refs->gitcommondir); return ref_store; } diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 6725742f00..369c34f886 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -203,7 +203,7 @@ struct ref_store *packed_ref_store_create(const char *path, refs->store_flags = store_flags; refs->path = xstrdup(path); - chdir_notify_reparent(&refs->path); + chdir_notify_reparent("packed-refs", &refs->path); return ref_store; } diff --git a/trace.c b/trace.c index 7f3b08e148..fc623e91fd 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 88055abef7..2b6a1bc17c 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); ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-29 14:57 ` Duy Nguyen @ 2018-03-30 17:21 ` Jeff King 0 siblings, 0 replies; 54+ messages in thread From: Jeff King @ 2018-03-30 17:21 UTC (permalink / raw) To: Duy Nguyen; +Cc: git, Rafael Ascensao On Thu, Mar 29, 2018 at 04:57:26PM +0200, Duy Nguyen wrote: > On Wed, Mar 28, 2018 at 02:19:32PM -0400, Jeff King wrote: > > > > > I will probably rework on top of your chdir-notify instead (and let > > > yours to be merged earlier) > > > > Thanks. I like some of the related changes you made, like including this > > in the tracing output. That should be easy to do on top of mine, I > > think. > > Yeah. But is it possible to sneak something like this in your series > (I assume you will reroll anyway)? I could do it separately, but it > looks nicer if it's split out and merged in individual patches that > add new chdir-notify call site. Sure. > -void chdir_notify_register(chdir_notify_callback cb, void *data) > +void chdir_notify_register(const char *name, > + chdir_notify_callback cb, > + void *data) > { > struct chdir_notify_entry *e = xmalloc(sizeof(*e)); > e->cb = cb; > e->data = data; > + e->name = name; > list_add_tail(&e->list, &chdir_notify_entries); > } I'm tempted to make a copy of the name here (or at least document that it must remain valid forever). -Peff ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars. 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy ` (8 preceding siblings ...) 2018-03-28 18:19 ` [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars Jeff King @ 2018-03-28 22:24 ` Junio C Hamano 9 siblings, 0 replies; 54+ messages in thread From: Junio C Hamano @ 2018-03-28 22:24 UTC (permalink / raw) To: Nguyễn Thái Ngọc Duy; +Cc: git, Jeff King, Rafael Ascensao Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes: > This is what I got, which is slightly different from your series > because I want to call set_git_dir() just one time (by > setup_git_directory) and never again. I think the API looks close > enough. > > I will probably rework on top of your chdir-notify instead (and let > yours to be merged earlier) I was occupied with the current cycle and did not have a chance to read this one over before seeing this exchange. After reading Peff's chdir-notify thing, I agree with its general direction and the idea to refine on top of it, which you two seem to have agreed. Thanks for working well together. ^ permalink raw reply [flat|nested] 54+ messages in thread
end of thread, other threads:[~2018-03-30 19:54 UTC | newest] Thread overview: 54+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-03-26 21:27 git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars Rafael Ascensao 2018-03-26 21:44 ` Ævar Arnfjörð Bjarmason 2018-03-27 6:31 ` Jeff King 2018-03-27 14:56 ` Duy Nguyen 2018-03-27 16:47 ` Jeff King 2018-03-27 17:09 ` Duy Nguyen 2018-03-27 17:30 ` Duy Nguyen 2018-03-28 9:52 ` Jeff King 2018-03-28 10:10 ` Duy Nguyen 2018-03-28 17:36 ` Jeff King 2018-03-28 17:38 ` [PATCH 1/4] set_git_dir: die when setenv() fails Jeff King 2018-03-28 17:40 ` [PATCH 2/4] add chdir-notify API Jeff King 2018-03-28 17:58 ` Eric Sunshine 2018-03-28 18:02 ` Jeff King 2018-03-29 14:53 ` Duy Nguyen 2018-03-29 17:48 ` Jeff King 2018-03-29 18:12 ` Duy Nguyen 2018-03-28 17:42 ` [PATCH 3/4] set_work_tree: use chdir_notify Jeff King 2018-03-29 17:02 ` Duy Nguyen 2018-03-29 17:23 ` Duy Nguyen 2018-03-29 17:50 ` Jeff King 2018-03-29 17:50 ` Jeff King 2018-03-29 18:01 ` Duy Nguyen 2018-03-30 17:23 ` Jeff King 2018-03-28 17:43 ` [PATCH 4/4] refs: use chdir_notify to update cached relative paths Jeff King 2018-03-30 18:34 ` [PATCH v2 0/5] re-parenting relative directories after chdir Jeff King 2018-03-30 18:34 ` [PATCH v2 1/5] set_git_dir: die when setenv() fails Jeff King 2018-03-30 18:34 ` [PATCH v2 2/5] trace.c: export trace_setup_key Jeff King 2018-03-30 19:46 ` Junio C Hamano 2018-03-30 19:47 ` Jeff King 2018-03-30 19:50 ` Junio C Hamano 2018-03-30 19:54 ` Jeff King 2018-03-30 18:35 ` [PATCH v2 3/5] add chdir-notify API Jeff King 2018-03-30 18:35 ` [PATCH v2 4/5] set_work_tree: use chdir_notify Jeff King 2018-03-30 18:35 ` [PATCH v2 5/5] refs: use chdir_notify to update cached relative paths Jeff King 2018-03-30 19:36 ` [PATCH v2 0/5] re-parenting relative directories after chdir Duy Nguyen 2018-03-28 9:47 ` git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars Jeff King 2018-03-28 17:55 ` [PATCH 0/8] " Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 1/8] strbuf.c: add strbuf_ensure_trailing_dr_sep() Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix) Nguyễn Thái Ngọc Duy 2018-03-28 18:02 ` Stefan Beller 2018-03-28 18:05 ` Duy Nguyen 2018-03-28 17:55 ` [PATCH 3/8] trace.c: export trace_setup_key Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 4/8] setup.c: introduce setup_adjust_path() Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 5/8] setup.c: allow other code to be notified when $CWD moves Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved Nguyễn Thái Ngọc Duy 2018-03-28 18:30 ` Jeff King 2018-03-28 18:45 ` Duy Nguyen 2018-03-28 17:55 ` [PATCH 7/8] repository: adjust repo paths when $CWD moves Nguyễn Thái Ngọc Duy 2018-03-28 17:55 ` [PATCH 8/8] refs: adjust main " Nguyễn Thái Ngọc Duy 2018-03-28 18:19 ` [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars Jeff King 2018-03-29 14:57 ` Duy Nguyen 2018-03-30 17:21 ` Jeff King 2018-03-28 22:24 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).