git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Elijah Newren <newren@gmail.com>
Cc: Glen Choo <chooglen@google.com>,
	Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>, Jeff King <peff@peff.net>
Subject: Re: [PATCH 8/8] dir: avoid removing the current working directory
Date: Wed, 24 Nov 2021 12:14:05 +0100	[thread overview]
Message-ID: <211124.86wnkxdbza.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <CABPp-BGZPheDqWD1pXbePOYe2BcT1-TqX-zGVqnNLGtFuXab1g@mail.gmail.com>


On Tue, Nov 23 2021, Elijah Newren wrote:

> On Tue, Nov 23, 2021 at 5:19 PM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:

>> Doesn't this series also change the behavior of e.g.:
>>
>>     cd contrib/subtree
>>     git rm -r ../subtree
>
> Yes, of course!
>
> Before:
>
>     $ cd contrib/subtree
>     $ git rm -r -q ../subtree/
>     $ ls -ld ../subtree/
>     ls: cannot access '../subtree/': No such file or directory
>     $ git status --porcelain
>     fatal: Unable to read current working directory: No such file or directory
>     $ git checkout HEAD ../subtree/
>     fatal: Unable to read current working directory: No such file or directory
>
> After:
>
>     $ cd contrib/subtree
>     $ git rm -r -q ../subtree/
>     $ ls -ld ../subtree/
>     drwxrwxr-x. 1 newren newren 0 Nov 23 19:18 ../subtree/
>     $ git status --porcelain
>     D  contrib/subtree/.gitignore
>     D  contrib/subtree/COPYING
>     D  contrib/subtree/INSTALL
>     D  contrib/subtree/Makefile
>     D  contrib/subtree/README
>     D  contrib/subtree/git-subtree.sh
>     D  contrib/subtree/git-subtree.txt
>     D  contrib/subtree/t/Makefile
>     D  contrib/subtree/t/t7900-subtree.sh
>     D  contrib/subtree/todo
>     $ git checkout HEAD ../subtree/
>     Updated 10 paths from c557be478e
>
> Very nice fix, don't you think?

I'd be more sympathetic to this for the "checkout" etc. commands, but
once I add a "-f" to that "rm" I'm *really* expecting it to rm the
directory, but it won't anymore because it's in the underlying library
function.

But if the goal is to get "git status" and the like working isn't a much
more pointed fix to have setup.c handle the case of getting ENOENT from
getcwd() more gracefully. I.e. currently (and even with your patches):

    $ (mkdir blah && cd blah && rmdir ../blah && git status)
    fatal: Unable to read current working directory: No such file or directory

Whereas if we do e.g.:
	
	diff --git a/strbuf.c b/strbuf.c
	index b22e9816559..3f9a957ff9d 100644
	--- a/strbuf.c
	+++ b/strbuf.c
	@@ -600,6 +600,16 @@ int strbuf_getcwd(struct strbuf *sb)
	                        return 0;
	                }
	
	+               if (errno == ENOENT){
	+                       const char *pwd = getenv("PWD");
	+
	+                       if (pwd) {
	+                               warning(_("unable to getcwd(), but can read PWD, limping along with that..."));
	+                               strbuf_addstr(sb, pwd);
	+                               return 0;
	+                       }
	+               }
	+
	                /*
	                 * If getcwd(3) is implemented as a syscall that falls
	                 * back to a regular lookup using readdir(3) etc. then

We'll get:
	
	$ (mkdir blah && cd blah && rmdir ../blah && GIT_DISCOVERY_ACROSS_FILESYSTEM=1 ~/g/git/git status)
	warning: unable to getcwd(), but can read PWD, limping along with that...
	On branch master
	Your branch is up to date with 'origin/master'.
	
	Changes not staged for commit:
	  (use "git add <file>..." to update what will be committed)
	  (use "git restore <file>..." to discard changes in working directory)
	        modified:   ../strbuf.c
	
	no changes added to commit (use "git add" and/or "git commit -a")

I think that getenv("PWD") trick is widely supported, and once we get
past that we seem OK. The relative path to strbuf.c is even correct.

Currently you'd need to set GIT_DISCOVERY_ACROSS_FILESYSTEM=1 because we
run into another case in setup.c where we're not carrying that ENOENT
forward, but we could just patch strbuf_getcwd() or that subsequent code
to handle this edge case.

>> If so then re the "spidey sense" comment I had earlier: There's no rm
>> codepaths or tests changed by this series,
>
> That's not correct; I explicitly added a new rm test in the first
> patch in my series.  Further, that same test was modified to mark it
> as passing by this particular patch you are commenting on.

Sorry about that, I didn't look carefully enough.

>> so the implementation of
>> doing it at this lower level might be casting too wide a net.
>
> I'm getting the vibe that you are assuming I'm changing these two
> functions without realizing what places might be calling them;
> basically, that I'm just flippantly changing them.  Ignoring the
> ramifications of such an assumption, if this vibe is correct[...]

Sorry no, I didn't mean to imply that. I snipped the rest, but hopefully
this answers the questions you had well enough (and in the time I have
for this reply):

I'm not concerned that you didn't research this change well enough, I
just find it a bit iffy to introduce semantics in git around FS
operations that don't conform with that of POSIX & the underlying OS. My
*nix system happily accepts an "rm -rf" or an "rmdir" of the directory
I'm in, I'd expect git to do the same.

But whatever research we do on in-tree users we're left with changing
behavior for users in the wild, e.g. a script that cd's to each subdir
in a repo, inspects something, and if it wants to remove that does an
"git rm -r" of the directory it's in, commits, and expects the repo to
be clean afterwards.

I did follow/read at least one of the the original discussions[1] a bit,
and some earlier discussion I'm vaguely recalling around bisect in a
subdir.

If the underlying goal is to address the UX problem in git of e.g. "git
status" and the like hard-dying I wonder if something in the direction
of the setup.c/strbuf.c change above might be more of a gentle change.

That approach of a more gentler setup.c also has the benefit of having
git work when it ends up in this situation without the git commands
having landed it there, as in the above "rmdir" example.

Anyway, I really didn't have time to look at this very carefully. I just
remember looking into this with bisect/status etc. in the past, and
thinking that these problems were solvable in those cases, i.e. they
were just being overly anal about ENOENT, and not falling back on "PWD"
etc.

1. https://lore.kernel.org/git/YS3Tv7UfNkF+adry@coredump.intra.peff.net/

  reply	other threads:[~2021-11-24 11:33 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 [this message]
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
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=211124.86wnkxdbza.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=chooglen@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=newren@gmail.com \
    --cc=peff@peff.net \
    /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).