Re: [PATCH 2/8] repository, setup: introduce the_cwd

"Elijah Newren via GitGitGadget" [off-list ref] writes:

From: Elijah Newren <redacted>

Removing the current working directory causes all subsequent git
commands (and likely a number of non-git commands) run from that
directory to get confused and fail with a message about being unable to
read the current working directory.  That confuses end users,
particularly since the command they get the error from is not the one
that caused the problem; the problem came from the side-effect of some
previous command.

We would like to avoid removing the current working directory; towards
this end, introduce a new the_cwd variable that tracks the current
working directory.  Subsequent commits will make use of this new
variable.

Maybe a stupid question, but how is this different from doing getcwd()
and storing it away to the_cwd, or adding a check to see if the
directory we are about to rmdir() is the cwd, next to the existing
check that we do to see if that directory has some untracked files?

I am wondering how we are going to make sure that the_cwd is always
set to, and maintained to be, the correct value, even in the future
when these code paths change.  I also wonder if it might be safer to
learn what the value of cwd is very near the place where it will
become needed (i.e. the callsites of such rmdir() of a directory
inside working tree), instead of caching.

On Sun, Nov 21, 2021 at 12:00 AM Junio C Hamano [off-list ref] wrote:

"Elijah Newren via GitGitGadget" [off-list ref] writes:

quoted

From: Elijah Newren <redacted>

Removing the current working directory causes all subsequent git
commands (and likely a number of non-git commands) run from that
directory to get confused and fail with a message about being unable to
read the current working directory.  That confuses end users,
particularly since the command they get the error from is not the one
that caused the problem; the problem came from the side-effect of some
previous command.

We would like to avoid removing the current working directory; towards
this end, introduce a new the_cwd variable that tracks the current
working directory.  Subsequent commits will make use of this new
variable.

Maybe a stupid question, but how is this different from doing getcwd()
and storing it away to the_cwd

The exact output of getcwd() is an absolute path, whereas it's easier
to perform later checks if we have a path that is relative to the
toplevel working directory.  Also, setup.c already calls getcwd() (via
strbuf_getcwd()) and then massages it, and I'd have to do the exact
same type of massaging.  So, instead of calling getcwd() again and
re-massaging its output into the format I want, I just reuse the
existing call of getcwd() and its massaged output.

or adding a check to see if the
directory we are about to rmdir() is the cwd, next to the existing
check that we do to see if that directory has some untracked files?

I am wondering how we are going to make sure that the_cwd is always
set to, and maintained to be, the correct value, even in the future
when these code paths change.  I also wonder if it might be safer to
learn what the value of cwd is very near the place where it will
become needed (i.e. the callsites of such rmdir() of a directory
inside working tree), instead of caching.

I need to clean up my wording a bit, to clarify this.  Doing as you
suggest here would protect the wrong thing.  git has long done an
automatic chdir() at startup to the toplevel working tree, whereas
what we want to protect is the original current working directory as
of the time git started.  The original current working directory is
likely also still the current working directory of the parent process
that spawned us, and if that parent process is a shell, any subsequent
commands executed from there can be somewhat confusing when its
current working directory is removed.

quoted

Signed-off-by: Elijah Newren <redacted>
---
 repository.c | 1 +
 repository.h | 1 +
 setup.c      | 2 ++
 3 files changed, 4 insertions(+)

diff --git a/repository.c b/repository.c
index c5b90ba93ea..69a106c553c 100644
--- a/repository.c
+++ b/repository.c

@@ -17,6 +17,7 @@
 static struct repository the_repo;
 struct repository *the_repository;
 struct index_state the_index;
+char *the_cwd;

 void initialize_the_repository(void)
 {

diff --git a/repository.h b/repository.h
index a057653981c..45de85d18ef 100644
--- a/repository.h
+++ b/repository.h

@@ -147,6 +147,7 @@ struct repository {
 };

 extern struct repository *the_repository;
+extern char *the_cwd;

 /*
  * Define a custom repository layout. Any field can be NULL, which

diff --git a/setup.c b/setup.c
index 347d7181ae9..4466fa55af3 100644
--- a/setup.c
+++ b/setup.c

@@ -887,6 +887,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
              set_git_dir(gitdirenv, 1);
              if (chdir(worktree))
                      die_errno(_("cannot chdir to '%s'"), worktree);
+             the_cwd = xstrdup(cwd->buf + offset);
              strbuf_addch(cwd, '/');
              free(gitfile);
              return cwd->buf + offset;

@@ -940,6 +941,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
      /* Make "offset" point past the '/' (already the case for root dirs) */
      if (offset != offset_1st_component(cwd->buf))
              offset++;
+     the_cwd = xstrdup(cwd->buf + offset);
      /* Add a '/' at the end */
      strbuf_addch(cwd, '/');
      return cwd->buf + offset;