From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS53758 23.128.96.0/24 X-Spam-Status: No, score=-3.7 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 6C7901F953 for ; Wed, 24 Nov 2021 14:43:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1352604AbhKXOqS (ORCPT ); Wed, 24 Nov 2021 09:46:18 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50822 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1352250AbhKXOqN (ORCPT ); Wed, 24 Nov 2021 09:46:13 -0500 Received: from mail-ed1-x531.google.com (mail-ed1-x531.google.com [IPv6:2a00:1450:4864:20::531]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C8AA4C061191 for ; Wed, 24 Nov 2021 06:18:36 -0800 (PST) Received: by mail-ed1-x531.google.com with SMTP id e3so11213194edu.4 for ; Wed, 24 Nov 2021 06:18:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=from:to:cc:subject:date:references:user-agent:in-reply-to :message-id:mime-version:content-transfer-encoding; bh=FwVh/oTciJq0zCOcl6qm/Xl1F5/fNMgbzuoS8dijwPM=; b=l9so2+eHuGlfLIX5BxnfIX0nDvA/2pdbTrd6jvz8h+X0J8RYCQD2BTcJ/QFtsIwsR1 U8BIKf5V5vJDNtbJ37KSt0IpYzXqTR8ALnU1yKveHZEGzTeAPneTelgqlL7XmNi7YeaM CIDhB3OLrM/U1cD59ak+IIqNoeACFIlhC2qnXo8/2zNMP6/Wjez2egaSLdDU5MlxX4/I d0/l1JyLuRPIea/zwshj43wZnTSo1bWDom/Y5DxY7mXKDP7yNuf6rwsQXSSlr6Q4ZK2Y CLLBa08bM3m5uChlbNyLpPZoeJChBs2N9wmvNeoUgp9VbByu9CkMh7CmdC5dRUpG3EwG AgyQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:references:user-agent :in-reply-to:message-id:mime-version:content-transfer-encoding; bh=FwVh/oTciJq0zCOcl6qm/Xl1F5/fNMgbzuoS8dijwPM=; b=LMbzVJkRS79b6JJq7KBaWIMSFviRCoNESLgGM9XD1yNYXLdO++V92B6vjF79DDz3CW V3LfTbzGGGnMK1GVgPZJo640jNzDC+1cXExGPHYz87NBoJ/tK65hb0nbYAbzcjekYm7M tf4vjPwVW4rtYrQWWDho3N09YmyLNcOPxM0ccOvc/NPy6PAcj7M6PdXjyvEo6rtJZN5i 4lGJ/Vhot69JtgV3bDiQ9PvAheWwNP5WHw9NhHqdTLvZeDlkSTTwgwHTTMy9T1vaR0O+ QluJUbfB2cApjMSkm73MNVdz5cNDelbjSmpL6U+zZBGyE6TlrapONutbXceUemWKY1Rs zQ0Q== X-Gm-Message-State: AOAM533Zj6ysiN7WUaRlK110ppWqxuctxs4yv7l6Mq/imLiIx0aGxe5H tVtZqs+p3U+LfLpqIHF7cuk= X-Google-Smtp-Source: ABdhPJz/D0FK7+vWZcveSmtJe7eFfGBEGFyh9H/RrwwSXcrCMD+Rj+9QHHQrSV2DyTd+p86CSB2W6w== X-Received: by 2002:a17:907:6e16:: with SMTP id sd22mr19676490ejc.542.1637763515133; Wed, 24 Nov 2021 06:18:35 -0800 (PST) Received: from gmgdl (j120189.upc-j.chello.nl. [24.132.120.189]) by smtp.gmail.com with ESMTPSA id j17sm18883edj.0.2021.11.24.06.18.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 24 Nov 2021 06:18:34 -0800 (PST) Received: from avar by gmgdl with local (Exim 4.95) (envelope-from ) id 1mpt6Y-000CTd-3i; Wed, 24 Nov 2021 15:18:34 +0100 From: =?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason To: Elijah Newren Cc: Glen Choo , Johannes Schindelin via GitGitGadget , Git Mailing List , Jeff King Subject: Re: [PATCH 8/8] dir: avoid removing the current working directory Date: Wed, 24 Nov 2021 15:11:22 +0100 References: <20211123003958.3978-1-chooglen@google.com> <211124.86ee76e4fl.gmgdl@evledraar.gmail.com> <211124.86wnkxdbza.gmgdl@evledraar.gmail.com> User-agent: Debian GNU/Linux bookworm/sid; Emacs 27.1; mu4e 1.6.9 In-reply-to: <211124.86wnkxdbza.gmgdl@evledraar.gmail.com> Message-ID: <211124.86sfvld4cl.gmgdl@evledraar.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Wed, Nov 24 2021, =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason wrote: > On Tue, Nov 23 2021, Elijah Newren wrote: > >> On Tue, Nov 23, 2021 at 5:19 PM =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason >> 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 dir= ectory >> $ git checkout HEAD ../subtree/ >> fatal: Unable to read current working directory: No such file or dir= ectory >> >> 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 dire= ctory > > Whereas if we do e.g.: >=20=09 > 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; > } >=20=09 > + if (errno =3D=3D ENOENT){ > + const char *pwd =3D getenv("PWD"); > + > + if (pwd) { > + warning(_("unable to getcwd(), but can r= ead 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: >=20=09 > $ (mkdir blah && cd blah && rmdir ../blah && GIT_DISCOVERY_ACROSS_FILESY= STEM=3D1 ~/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'. >=20=09 > Changes not staged for commit: > (use "git add ..." to update what will be committed) > (use "git restore ..." to discard changes in working directory) > modified: ../strbuf.c >=20=09 > 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=3D1 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_6XbqZeOH= 1kRw@mail.gmail.com/, it's from the bin-wrapper. I really wish we didn't have the bin-wrappers...