Re: [PATCH 8/8] dir: avoid removing the current working directory

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Nov 24 2021, Ævar Arnfjörð Bjarmason wrote:

> 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/

I fleshened this out a bit in this WIP change:
https://github.com/avar/git/tree/avar/setup-handle-gone-directory +
commit:
https://github.com/avar/git/commit/97968518909eef88edba44973b7885d154b7a273

As noted there there's some caveats, but so far nothing I spotted that
can't be overcome. It's particularly painful to test it because of an
implementation detail of our test suite, the bin-wrappers are
shellscripts, and the very first thing they do is reset $PWD (none of
which happens if you run the real "git" binary).

That's b.t.w. the "shell-init" error you noted in
https://lore.kernel.org/git/CABPp-BEp3OL7F2J_LzqtC-x-8pBUPO8ZR1fTx_6XbqZeOH1kRw@mail.gmail.com/,
it's from the bin-wrapper.

I really wish we didn't have the bin-wrappers...