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 <pclouds@gmail.com>]

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