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

2018-03-30 Thread 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(>list, _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 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

2018-03-29 Thread 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(>list, _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(_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(_setup_key, "setup: chdir to '%s'", new_cwd);
 
list_for_each(pos, _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(_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(_path);
+ *   chdir_notify_reparent("description", _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(_setup_key,
+"setup: move

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

2018-03-28 Thread Junio C Hamano 
Nguyễn Thái Ngọc Duy   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 0/8] Re: git complains packed-refs is not a directory when used with GIT_DIR and GIT_WORK_TREE envvars.

2018-03-28 Thread 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

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

2018-03-28 Thread Nguyễn Thái Ngọc Duy 
On Wed, Mar 28, 2018 at 7:36 PM, Jeff King  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

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

2018-03-28 Thread 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

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

2018-03-28 Thread Duy Nguyen 
On Wed, Mar 28, 2018 at 11:52 AM, Jeff King  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.

2018-03-28 Thread 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.

2018-03-28 Thread 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.

2018-03-27 Thread 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.

2018-03-27 Thread Duy Nguyen 
On Tue, Mar 27, 2018 at 6:47 PM, Jeff King  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.

2018-03-27 Thread 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, _be_files);
refs->store_flags = flags;
 
-   refs->gitdir = xstrdup(gitdir);
-   get_common_dir_noenv(, gitdir);
-   refs->gitcommondir = strbuf_detach(, NULL);
-   strbuf_addf(, "%s/packed-refs", refs->gitcommondir);
-   refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
-   strbuf_release();
+   if (gitdir) {
+   struct strbuf sb = STRBUF_INIT;
+   refs->gitdir = xstrdup(gitdir);
+   get_common_dir_noenv(, gitdir);
+   refs->gitcommondir = strbuf_detach(, NULL);
+   strbuf_addf(, "%s/packed-refs", refs->gitcommondir);
+   refs->packed_ref_store = packed_ref_store_create(sb.buf, flags);
+   strbuf_release();
+   } 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

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

2018-03-27 Thread Duy Nguyen 
On Tue, Mar 27, 2018 at 8:31 AM, Jeff King  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.

2018-03-27 Thread 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.

2018-03-26 Thread Æ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.