From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Jeff King" <peff@peff.net>,
"René Scharfe" <l.s.r@web.de>, "Glen Choo" <chooglen@google.com>,
"Philip Oakley" <philipoakley@iee.email>,
"Elijah Newren" <newren@gmail.com>
Subject: Re: [PATCH v2 2/9] setup: introduce startup_info->original_cwd
Date: Thu, 25 Nov 2021 11:44:29 +0100 [thread overview]
Message-ID: <211125.86bl28jyil.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <f6129a8ac9c3d052fb7fb508a62d4eedb8d9ed57.1637829556.git.gitgitgadget@gmail.com>
On Thu, Nov 25 2021, Elijah Newren via GitGitGadget wrote:
> Removing the current working directory causes all subsequent git
> commands run from that directory to get confused and fail with a message
> about being unable to read the current working directory:
>
> $ git status
> fatal: Unable to read current working directory: No such file or directory
>
> Non-git commands likely have similar warnings or even errors, e.g.
>
> $ bash -c 'echo hello'
> shell-init: error retrieving current directory: getcwd: cannot access parent directories: No such file or directory
> hello
Okey, but...
> diff --git a/git.c b/git.c
> index 5ff21be21f3..2c98ab48936 100644
> --- a/git.c
> +++ b/git.c
> @@ -866,6 +866,8 @@ int cmd_main(int argc, const char **argv)
>
> trace_command_performance(argv);
>
> + startup_info->original_cwd = xgetcwd();
> +
> /*
> * "git-xxxx" is the same as "git xxxx", but we obviously:
> *
> diff --git a/setup.c b/setup.c
> index 347d7181ae9..f30657723ea 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -432,6 +432,54 @@ void setup_work_tree(void)
> initialized = 1;
> }
>
> +static void setup_original_cwd(void)
> +{
> + struct strbuf tmp = STRBUF_INIT;
> + const char *worktree = NULL;
> + int offset = -1;
> +
> + /*
> + * startup_info->original_cwd wass set early on in cmd_main(), unless
> + * we're an auxiliary tool like git-remote-http or test-tool.
> + */
> + if (!startup_info->original_cwd)
> + return;
This really doesn't belong in those places, by calling xgetcwd() so
early you'll be making commands that don't care about RUN_SETUP at all
die. E.g. with this change:
mkdir x &&
cd x &&
rm -rf ../x &&
git version
Will die.
So as a follow-up to my comment on this v2's 01/09 I think what's also
missing here is something that does that, but instead of "git version"
does it for all of the "while read builtin" list in t0012-help.sh.
There's other cans of worms to be had here, the discussion downthread of
the not-integrated[1] points to some of that.
I.e. how should the various commands like "ls-remote" that can work
with/without a repo behave here? That one happens to die before/after,
but as noted in that thread that's also a bug.
So anything that adds more really early dying in this area should also
think about the greater goals of what we're doing with RUN_SETUP &
related flags. I.e. does the direction make sense?
If this is moved to some soft recording of the getcwd() (and maybe the
$PWD, as in my WIP change?) shouldn't this go into common-main.c? With
git.c's cmd_main() we're excluding the test helpers and things like
git-daemon. Is that intentional?
I'd also think we'd want to do this much earlier if e.g. thing like the
trace2 setup wanted to call the remove_directory() call.
Per what Glen & mentioned I'm still not sure if I'm on board with that
idea at all, just running with the ball you put in play :) I.e. if we're
modifying all callers, let's make sure we catch all callers.
Perhaps a better approach is to intercept chdir() instead? And anything
that calls chdir() sets some GIT_* variable so we'll pass "here's our
original cwd" down to sub-processes, like we do with "git -c" for
config.
That would presumably save you the effort of in-advance whitelisting
everything like "git version", we can just move to doing this lazily. If
you're a command that does a RUN_SETUP or otherwise needs chdir() we'll
record the original getcwd(), otherwise...
1. https://lore.kernel.org/git/patch-1.1-fc26c46d39-20210722T140648Z-avarab@gmail.com/
next prev parent reply other threads:[~2021-11-25 10:57 UTC|newest]
Thread overview: 163+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-21 0:46 [PATCH 0/8] Avoid removing the current working directory, even if it becomes empty Elijah Newren via GitGitGadget
2021-11-21 0:46 ` [PATCH 1/8] t2501: add various tests for removing the current working directory Elijah Newren via GitGitGadget
2021-11-21 17:57 ` Ævar Arnfjörð Bjarmason
2021-11-23 1:45 ` Elijah Newren
2021-11-23 2:19 ` Ævar Arnfjörð Bjarmason
2021-11-23 3:11 ` Elijah Newren
2021-11-25 10:04 ` Ævar Arnfjörð Bjarmason
2021-11-21 0:46 ` [PATCH 2/8] repository, setup: introduce the_cwd Elijah Newren via GitGitGadget
2021-11-21 8:00 ` Junio C Hamano
2021-11-22 22:38 ` Elijah Newren
2021-11-21 8:56 ` René Scharfe
2021-11-22 23:09 ` Elijah Newren
2021-11-21 0:46 ` [PATCH 3/8] unpack-trees: refuse to remove the current working directory Elijah Newren via GitGitGadget
2021-11-21 0:46 ` [PATCH 4/8] unpack-trees: add special cwd handling Elijah Newren via GitGitGadget
2021-11-21 0:46 ` [PATCH 5/8] symlinks: do not include current working directory in dir removal Elijah Newren via GitGitGadget
2021-11-21 8:56 ` René Scharfe
2021-11-23 0:35 ` Elijah Newren
2021-11-21 0:46 ` [PATCH 6/8] clean: do not attempt to remove current working directory Elijah Newren via GitGitGadget
2021-11-21 17:51 ` Ævar Arnfjörð Bjarmason
2021-11-23 1:28 ` Elijah Newren
2021-11-21 0:46 ` [PATCH 7/8] stash: " Elijah Newren via GitGitGadget
2021-11-21 0:47 ` [PATCH 8/8] dir: avoid removing the " Elijah Newren via GitGitGadget
2021-11-23 0:39 ` Glen Choo
2021-11-23 1:19 ` Elijah Newren
2021-11-23 18:19 ` Glen Choo
2021-11-23 19:56 ` Elijah Newren
2021-11-23 20:32 ` Glen Choo
2021-11-23 21:57 ` Junio C Hamano
2021-11-23 23:23 ` Elijah Newren
2021-11-24 5:46 ` Junio C Hamano
2021-11-23 23:13 ` Elijah Newren
2021-11-24 0:39 ` Glen Choo
2021-11-24 5:46 ` Junio C Hamano
2021-11-24 1:10 ` Ævar Arnfjörð Bjarmason
2021-11-24 4:35 ` Elijah Newren
2021-11-24 11:14 ` Ævar Arnfjörð Bjarmason
2021-11-24 14:11 ` Ævar Arnfjörð Bjarmason
2021-11-25 2:54 ` Elijah Newren
2021-11-25 11:12 ` Ævar Arnfjörð Bjarmason
2021-11-26 21:40 ` The overhead of bin-wrappers/ (was: [PATCH 8/8] dir: avoid removing the current working directory) Ævar Arnfjörð Bjarmason
2021-11-24 14:33 ` [PATCH 8/8] dir: avoid removing the current working directory Philip Oakley
2021-11-24 19:46 ` Junio C Hamano
2021-11-25 12:54 ` Philip Oakley
2021-11-25 13:51 ` Ævar Arnfjörð Bjarmason
2021-11-25 2:48 ` Elijah Newren
2021-11-24 19:43 ` Junio C Hamano
2021-11-21 8:11 ` [PATCH 0/8] Avoid removing the current working directory, even if it becomes empty Junio C Hamano
2021-11-25 8:39 ` [PATCH v2 0/9] " Elijah Newren via GitGitGadget
2021-11-25 8:39 ` [PATCH v2 1/9] t2501: add various tests for removing the current working directory Elijah Newren via GitGitGadget
2021-11-25 10:21 ` Ævar Arnfjörð Bjarmason
2021-11-25 8:39 ` [PATCH v2 2/9] setup: introduce startup_info->original_cwd Elijah Newren via GitGitGadget
2021-11-25 10:44 ` Ævar Arnfjörð Bjarmason [this message]
2021-11-26 17:55 ` Elijah Newren
2021-11-26 6:52 ` Junio C Hamano
2021-11-26 18:01 ` Elijah Newren
2021-11-29 14:05 ` Derrick Stolee
2021-11-29 17:18 ` Elijah Newren
2021-11-29 17:43 ` Derrick Stolee
2021-11-29 17:42 ` Junio C Hamano
2021-11-25 8:39 ` [PATCH v2 3/9] unpack-trees: refuse to remove startup_info->original_cwd Elijah Newren via GitGitGadget
2021-11-25 10:56 ` Ævar Arnfjörð Bjarmason
2021-11-26 18:06 ` Elijah Newren
2021-11-29 14:10 ` Derrick Stolee
2021-11-29 17:26 ` Elijah Newren
2021-11-25 8:39 ` [PATCH v2 4/9] unpack-trees: add special cwd handling Elijah Newren via GitGitGadget
2021-11-29 14:14 ` Derrick Stolee
2021-11-29 17:33 ` Elijah Newren
2021-11-25 8:39 ` [PATCH v2 5/9] symlinks: do not include startup_info->original_cwd in dir removal Elijah Newren via GitGitGadget
2021-11-25 8:39 ` [PATCH v2 6/9] clean: do not attempt to remove startup_info->original_cwd Elijah Newren via GitGitGadget
2021-11-25 8:39 ` [PATCH v2 7/9] stash: " Elijah Newren via GitGitGadget
2021-11-25 10:58 ` Ævar Arnfjörð Bjarmason
2021-11-26 18:04 ` Elijah Newren
2021-11-25 8:39 ` [PATCH v2 8/9] dir: avoid incidentally removing the original_cwd in remove_path() Elijah Newren via GitGitGadget
2021-11-25 8:39 ` [PATCH v2 9/9] dir: new flag to remove_dir_recurse() to spare the original_cwd Elijah Newren via GitGitGadget
2021-11-26 22:40 ` [PATCH v3 00/11] Avoid removing the current working directory, even if it becomes empty Elijah Newren via GitGitGadget
2021-11-26 22:40 ` [PATCH v3 01/11] t2501: add various tests for removing the current working directory Elijah Newren via GitGitGadget
2021-11-27 10:32 ` Ævar Arnfjörð Bjarmason
2021-11-27 19:16 ` Elijah Newren
2021-11-26 22:40 ` [PATCH v3 02/11] setup: introduce startup_info->original_cwd Elijah Newren via GitGitGadget
2021-11-27 10:35 ` Ævar Arnfjörð Bjarmason
2021-11-27 17:05 ` Elijah Newren
2021-11-27 10:40 ` Ævar Arnfjörð Bjarmason
2021-11-27 18:31 ` Elijah Newren
2021-11-28 18:04 ` Ævar Arnfjörð Bjarmason
2021-11-29 21:58 ` Elijah Newren
2021-11-26 22:40 ` [PATCH v3 03/11] unpack-trees: refuse to remove startup_info->original_cwd Elijah Newren via GitGitGadget
2021-11-26 22:40 ` [PATCH v3 04/11] unpack-trees: add special cwd handling Elijah Newren via GitGitGadget
2021-11-26 22:40 ` [PATCH v3 05/11] symlinks: do not include startup_info->original_cwd in dir removal Elijah Newren via GitGitGadget
2021-11-26 22:40 ` [PATCH v3 06/11] clean: do not attempt to remove startup_info->original_cwd Elijah Newren via GitGitGadget
2021-11-26 22:40 ` [PATCH v3 07/11] rebase: " Elijah Newren via GitGitGadget
2021-11-29 17:50 ` Derrick Stolee
2021-11-29 19:22 ` Elijah Newren
2021-11-29 19:42 ` Derrick Stolee
2021-11-26 22:40 ` [PATCH v3 08/11] stash: " Elijah Newren via GitGitGadget
2021-11-26 22:41 ` [PATCH v3 09/11] dir: avoid incidentally removing the original_cwd in remove_path() Elijah Newren via GitGitGadget
2021-11-26 22:41 ` [PATCH v3 10/11] dir: new flag to remove_dir_recurse() to spare the original_cwd Elijah Newren via GitGitGadget
2021-11-26 22:41 ` [PATCH v3 11/11] t2501: simplify the tests since we can now assume desired behavior Elijah Newren via GitGitGadget
2021-11-29 17:57 ` [PATCH v3 00/11] Avoid removing the current working directory, even if it becomes empty Derrick Stolee
2021-11-29 22:37 ` [PATCH v4 " Elijah Newren via GitGitGadget
2021-11-29 22:37 ` [PATCH v4 01/11] t2501: add various tests for removing the current working directory Elijah Newren via GitGitGadget
2021-11-30 6:47 ` Junio C Hamano
2021-11-30 6:53 ` Elijah Newren
2021-11-29 22:37 ` [PATCH v4 02/11] setup: introduce startup_info->original_cwd Elijah Newren via GitGitGadget
2021-11-29 22:37 ` [PATCH v4 03/11] unpack-trees: refuse to remove startup_info->original_cwd Elijah Newren via GitGitGadget
2021-11-29 22:37 ` [PATCH v4 04/11] unpack-trees: add special cwd handling Elijah Newren via GitGitGadget
2021-11-29 22:37 ` [PATCH v4 05/11] symlinks: do not include startup_info->original_cwd in dir removal Elijah Newren via GitGitGadget
2021-11-29 22:37 ` [PATCH v4 06/11] clean: do not attempt to remove startup_info->original_cwd Elijah Newren via GitGitGadget
2021-11-29 22:37 ` [PATCH v4 07/11] rebase: " Elijah Newren via GitGitGadget
2021-11-29 22:37 ` [PATCH v4 08/11] stash: " Elijah Newren via GitGitGadget
2021-11-29 22:37 ` [PATCH v4 09/11] dir: avoid incidentally removing the original_cwd in remove_path() Elijah Newren via GitGitGadget
2021-11-29 22:37 ` [PATCH v4 10/11] dir: new flag to remove_dir_recurse() to spare the original_cwd Elijah Newren via GitGitGadget
2021-11-29 22:37 ` [PATCH v4 11/11] t2501: simplify the tests since we can now assume desired behavior Elijah Newren via GitGitGadget
2021-11-29 23:38 ` [PATCH v4 00/11] Avoid removing the current working directory, even if it becomes empty Eric Sunshine
2021-11-30 0:16 ` Elijah Newren
2021-12-01 6:40 ` [PATCH v5 " Elijah Newren via GitGitGadget
2021-12-01 6:40 ` [PATCH v5 01/11] t2501: add various tests for removing the current working directory Elijah Newren via GitGitGadget
2021-12-01 6:40 ` [PATCH v5 02/11] setup: introduce startup_info->original_cwd Elijah Newren via GitGitGadget
2021-12-01 6:40 ` [PATCH v5 03/11] unpack-trees: refuse to remove startup_info->original_cwd Elijah Newren via GitGitGadget
2021-12-01 6:40 ` [PATCH v5 04/11] unpack-trees: add special cwd handling Elijah Newren via GitGitGadget
2021-12-01 6:40 ` [PATCH v5 05/11] symlinks: do not include startup_info->original_cwd in dir removal Elijah Newren via GitGitGadget
2021-12-01 6:40 ` [PATCH v5 06/11] clean: do not attempt to remove startup_info->original_cwd Elijah Newren via GitGitGadget
2021-12-01 6:40 ` [PATCH v5 07/11] rebase: " Elijah Newren via GitGitGadget
2022-01-25 20:26 ` [Bug] Rebase from worktree subdir is broken (was Re: [PATCH v5 07/11] rebase: do not attempt to remove startup_info->original_cwd) Glen Choo
2022-01-25 23:59 ` Elijah Newren
2022-01-26 0:30 ` Glen Choo
2022-01-26 19:04 ` Elijah Newren
2022-01-26 0:32 ` Eric Sunshine
2022-01-26 0:38 ` Eric Sunshine
2022-01-26 0:51 ` Elijah Newren
2022-01-26 1:15 ` Glen Choo
2022-01-26 1:38 ` Elijah Newren
2022-01-26 11:00 ` Phillip Wood
2022-01-26 17:53 ` Eric Sunshine
2022-01-27 11:01 ` Phillip Wood
2022-01-27 20:03 ` Elijah Newren
2022-02-05 11:23 ` Eric Sunshine
2022-02-05 11:42 ` Eric Sunshine
2022-02-05 22:35 ` Elijah Newren
2021-12-01 6:40 ` [PATCH v5 08/11] stash: do not attempt to remove startup_info->original_cwd Elijah Newren via GitGitGadget
2021-12-01 6:40 ` [PATCH v5 09/11] dir: avoid incidentally removing the original_cwd in remove_path() Elijah Newren via GitGitGadget
2021-12-01 6:40 ` [PATCH v5 10/11] dir: new flag to remove_dir_recurse() to spare the original_cwd Elijah Newren via GitGitGadget
2021-12-01 6:40 ` [PATCH v5 11/11] t2501: simplify the tests since we can now assume desired behavior Elijah Newren via GitGitGadget
2021-12-07 16:09 ` [PATCH v5 00/11] Avoid removing the current working directory, even if it becomes empty Derrick Stolee
2021-12-07 18:30 ` Ævar Arnfjörð Bjarmason
2021-12-07 20:57 ` Derrick Stolee
2021-12-08 10:23 ` Ævar Arnfjörð Bjarmason
2021-12-07 20:43 ` Elijah Newren
2021-12-07 21:00 ` Derrick Stolee
2021-12-09 5:08 ` [PATCH v6 " Elijah Newren via GitGitGadget
2021-12-09 5:08 ` [PATCH v6 01/11] t2501: add various tests for removing the current working directory Elijah Newren via GitGitGadget
2022-03-11 11:57 ` Ævar Arnfjörð Bjarmason
2021-12-09 5:08 ` [PATCH v6 02/11] setup: introduce startup_info->original_cwd Elijah Newren via GitGitGadget
2021-12-09 5:08 ` [PATCH v6 03/11] unpack-trees: refuse to remove startup_info->original_cwd Elijah Newren via GitGitGadget
2021-12-09 5:08 ` [PATCH v6 04/11] unpack-trees: add special cwd handling Elijah Newren via GitGitGadget
2021-12-09 5:08 ` [PATCH v6 05/11] symlinks: do not include startup_info->original_cwd in dir removal Elijah Newren via GitGitGadget
2021-12-09 5:08 ` [PATCH v6 06/11] clean: do not attempt to remove startup_info->original_cwd Elijah Newren via GitGitGadget
2021-12-09 5:08 ` [PATCH v6 07/11] rebase: " Elijah Newren via GitGitGadget
2021-12-09 5:08 ` [PATCH v6 08/11] stash: " Elijah Newren via GitGitGadget
2021-12-09 5:08 ` [PATCH v6 09/11] dir: avoid incidentally removing the original_cwd in remove_path() Elijah Newren via GitGitGadget
2021-12-09 5:08 ` [PATCH v6 10/11] dir: new flag to remove_dir_recurse() to spare the original_cwd Elijah Newren via GitGitGadget
2021-12-09 5:08 ` [PATCH v6 11/11] t2501: simplify the tests since we can now assume desired behavior Elijah Newren via GitGitGadget
2021-11-30 11:04 ` [PATCH v3 00/11] Avoid removing the current working directory, even if it becomes empty Phillip Wood
2021-12-01 0:03 ` Elijah Newren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=211125.86bl28jyil.gmgdl@evledraar.gmail.com \
--to=avarab@gmail.com \
--cc=chooglen@google.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=l.s.r@web.de \
--cc=newren@gmail.com \
--cc=peff@peff.net \
--cc=philipoakley@iee.email \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).