git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Rafael Ascensao]

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

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Ævar Arnfjörð Bjarmason]

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.

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

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

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Duy Nguyen]

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

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

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;
 }

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Duy Nguyen]

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

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Duy Nguyen]

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< --

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

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

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

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

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Duy Nguyen]

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

Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

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

[PATCH 1/4] set_git_dir: die when setenv() fails

[Jeff King]

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

[PATCH 2/4] add chdir-notify API

[Jeff King]

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

[PATCH 3/4] set_work_tree: use chdir_notify

[Jeff King]

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

[PATCH 4/4] refs: use chdir_notify to update cached relative paths

[Jeff King]

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

[PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[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

[PATCH 1/8] strbuf.c: add strbuf_ensure_trailing_dr_sep()

[Nguyễn Thái Ngọc Duy]

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

[PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix)

[Nguyễn Thái Ngọc Duy]

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

[PATCH 3/8] trace.c: export trace_setup_key

[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

[PATCH 4/8] setup.c: introduce setup_adjust_path()

[Nguyễn Thái Ngọc Duy]

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

[PATCH 5/8] setup.c: allow other code to be notified when $CWD moves

[Nguyễn Thái Ngọc Duy]

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

[PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved

[Nguyễn Thái Ngọc Duy]

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

[PATCH 7/8] repository: adjust repo paths when $CWD moves

[Nguyễn Thái Ngọc Duy]

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

[PATCH 8/8] refs: adjust main repo paths when $CWD moves

[Nguyễn Thái Ngọc Duy]

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

Re: [PATCH 2/4] add chdir-notify API

[Eric Sunshine]

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.

Re: [PATCH 2/4] add chdir-notify API

[Jeff King]

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

Re: [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix)

[Stefan Beller]

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

Re: [PATCH 2/8] strbuf.c: reintroduce get_pwd_cwd() (with strbuf_ prefix)

[Duy Nguyen]

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

Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

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

Re: [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved

[Jeff King]

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

Re: [PATCH 6/8] environment.c: adjust env containing relpath when $CWD is moved

[Duy Nguyen]

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

Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Junio C Hamano]

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.

Re: [PATCH 2/4] add chdir-notify API

[Duy Nguyen]

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

Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Duy Nguyen]

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);

Re: [PATCH 3/4] set_work_tree: use chdir_notify

[Duy Nguyen]

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

Re: [PATCH 3/4] set_work_tree: use chdir_notify

[Duy Nguyen]

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

Re: [PATCH 2/4] add chdir-notify API

[Jeff King]

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

Re: [PATCH 3/4] set_work_tree: use chdir_notify

[Jeff King]

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

Re: [PATCH 3/4] set_work_tree: use chdir_notify

[Jeff King]

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

Re: [PATCH 3/4] set_work_tree: use chdir_notify

[Duy Nguyen]

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

Re: [PATCH 2/4] add chdir-notify API

[Duy Nguyen]

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

Re: [PATCH 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

[Jeff King]

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

Re: [PATCH 3/4] set_work_tree: use chdir_notify

[Jeff King]

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

[PATCH v2 0/5] re-parenting relative directories after chdir

[Jeff King]

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

[PATCH v2 1/5] set_git_dir: die when setenv() fails

[Jeff King]

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

[PATCH v2 2/5] trace.c: export trace_setup_key

[Jeff King]

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

[PATCH v2 3/5] add chdir-notify API

[Jeff King]

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

[PATCH v2 4/5] set_work_tree: use chdir_notify

[Jeff King]

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

[PATCH v2 5/5] refs: use chdir_notify to update cached relative paths

[Jeff King]

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

Re: [PATCH v2 0/5] re-parenting relative directories after chdir

[Duy Nguyen]

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

Re: [PATCH v2 2/5] trace.c: export trace_setup_key

[Junio C Hamano]

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);

Re: [PATCH v2 2/5] trace.c: export trace_setup_key

[Jeff King]

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

Re: [PATCH v2 2/5] trace.c: export trace_setup_key

[Junio C Hamano]

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-<.

Re: [PATCH v2 2/5] trace.c: export trace_setup_key

[Jeff King]

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