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-Status: No, score=-3.9 required=3.0 tests=AWL,BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,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 6D9911F66F for ; Tue, 10 Nov 2020 12:34:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731738AbgKJMe3 (ORCPT ); Tue, 10 Nov 2020 07:34:29 -0500 Received: from wout3-smtp.messagingengine.com ([64.147.123.19]:58651 "EHLO wout3-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1731275AbgKJMe2 (ORCPT ); Tue, 10 Nov 2020 07:34:28 -0500 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.west.internal (Postfix) with ESMTP id 41FE4880; Tue, 10 Nov 2020 07:34:27 -0500 (EST) Received: from mailfrontend2 ([10.202.2.163]) by compute1.internal (MEProxy); Tue, 10 Nov 2020 07:34:27 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=dPziJzAlThwGe9CevoksdUo8rlT KHOmRBWM5Pi2ywYM=; b=UCicLiKzWWy3B+7GiaHHSsh0tff0Bp2+XAmAmtHYal4 Zeh2fG810P/kXMU2ZUCZ+3flBNXviUww2mTpkN0v4BD6Z4apLhIYtptJ9GtDr/2J ec24upSvdX9B1f7j8V/c044o8EBz04NI56vDpSwZC9hJCBeHQrEJXrZkMXClrUoV 0t1iP4sU7jpCasuH6aD95DLzEp43L6WumB/RJB7mA++F0BrnamMBRz5jtaRazgVz 0aoHJwuje0j9rPx5Q9e/RuebZ7wmQkrRwdS0xeIEGnTocPAIdNp32RsvVb4rgZgY 8ou4pakgIu5ftKeIS51xMYXiI2WCy1rEdXO6OpJt7kQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=dPziJz AlThwGe9CevoksdUo8rlTKHOmRBWM5Pi2ywYM=; b=P7ILrptOzIDRCoCWhxR4Ra gVjT3hqLJldWuQA1p1iEhQFyMbtWACHCd0dZFPn8PpRf2yRZFsA22J4C7rIUFEoE lodGMXxN1bf0XxCHszC3+pIiLXUN9UphN4ztbW61a0liPqmT/5Ys9ANdfZUW+EZJ 6mkMowxLv9/9tttX1tAjnxtz4MrTHHjc+PLywdxuXI8v0NOoLGcw27VcWYlLIr8N Oaq3NzFux5ziABDPHryGz6GunJ0qdbC4PDH8TMw/iRA9yEUQHdzFo5EycSs7WL44 ik44AmgshrqDkO+hwTgOMwlUg6CKWoRJ9UXFk8j50A6w6cF/Si9Nar7NhuOYplZA == X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedujedruddujedggeduucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucenucfjughrpeffhffvuffkfhggtggujgesghdtre ertddtvdenucfhrhhomheprfgrthhrihgtkhcuufhtvghinhhhrghrughtuceophhssehp khhsrdhimheqnecuggftrfgrthhtvghrnhepheeghfdtfeeuffehkefgffduleffjedthf dvjeektdfhhedvlefgtefgvdettdfhnecukfhppeejjedrudeluddrvdehhedrudegkeen ucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehpshesph hkshdrihhm X-ME-Proxy: Received: from vm-mail.pks.im (x4dbfff94.dyn.telefonica.de [77.191.255.148]) by mail.messagingengine.com (Postfix) with ESMTPA id C92A73063082; Tue, 10 Nov 2020 07:34:25 -0500 (EST) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 815b60b0 (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Tue, 10 Nov 2020 12:34:24 +0000 (UTC) Date: Tue, 10 Nov 2020 13:34:22 +0100 From: Patrick Steinhardt To: Junio C Hamano Cc: git@vger.kernel.org, peff@peff.net Subject: Re: [PATCH v2 1/4] t1400: Avoid touching refs on filesystem Message-ID: References: <9b49e849eaf6786c63016d767d2ad56112d08d51.1604908834.git.ps@pks.im> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="ICly2YHBkf7mGUAp" Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org --ICly2YHBkf7mGUAp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 09, 2020 at 11:48:20AM -0800, Junio C Hamano wrote: > Patrick Steinhardt writes: > > diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh > > index 4c01e08551..957bef272d 100755 > > --- a/t/t1400-update-ref.sh > > +++ b/t/t1400-update-ref.sh > > @@ -14,6 +14,12 @@ n=3D$n_dir/fixes > > outside=3Drefs/foo > > bare=3Dbare-repo > > =20 > > +# Some of the tests delete HEAD, which causes us to not treat the curr= ent > > +# working directory as a Git repository anymore. To avoid using any po= tential > > +# parent repository to be discovered, we need to set up the ceiling di= rectories. > > +GIT_CEILING_DIRECTORIES=3D"$PWD/.." > > +export GIT_CEILING_DIRECTORIES > > + >=20 > Interesting. The current tests do not need to do this because the > HEAD-less broken state is transitory and is corrected without using > "git" at all (e.g. echoing a valid value into .git/HEAD), I presume? Correct. > > @@ -80,26 +86,26 @@ test_expect_success "fail to delete $m (by HEAD) wi= th stale ref" ' > > test $B =3D $(git show-ref -s --verify $m) > > ' > > test_expect_success "delete $m (by HEAD)" ' > > - test_when_finished "rm -f .git/$m" && > > + test_when_finished "git update-ref -d $m" && > > git update-ref -d HEAD $B && > > - test_path_is_missing .git/$m > > + test_must_fail git show-ref --verify -q $m > > ' >=20 > During the above test, we are on the branch ${m#refs/heads/}, so > "update-ref -d HEAD" is removing the underlying branch ref, making > it an unborn branch, without destroying the repository, so this is > perfectly sensible. >=20 > This is a tangent, but what makes this test doubly interesting is > that "git update-ref -d HEAD" would have allowed us to make it a > non-repository if HEAD were detached, and it seems that we do not > require "--force" to do so. We probably should forbid removing HEAD > that id detached without "--force", which is such a destructive > operation. That'd make sense to me, yes. It also took me quite some time to actually figure out why tests were misbehaving when I converted it. Until I finally realized: this is not a Git repo anymore, and refs have now been modified directly in the real git.git repository. Just this morning I still found some invalid refs created by the test in git.git. > > cp -f .git/HEAD .git/HEAD.orig > > test_expect_success 'delete symref without dereference' ' > > test_when_finished "cp -f .git/HEAD.orig .git/HEAD" && > > git update-ref --no-deref -d HEAD && > > - test_path_is_missing .git/HEAD > > + test_must_fail git show-ref --verify -q HEAD > > ' >=20 > This is an example of breaking the repository. I am not sure if the > test_must_fail is a good replacement--it would fail even if you say > "git show-ref --verify -q refs/heads/$branch" where $branch is a > name of a branch that exists, no? Right, it's not. We're not detecting HEAD deletion by means of checking that it doesn't exist anymore, but only detect it because the repo is not a repo anymore. Which would in fact cause most git commands to fail now. > For now, I think this is fine, but we'd probably clean it up so that > without --force update-ref would not corrupt the repository like > this. When used with --force, or before adding such a safety > measure, how we test if we successfully corrupted the repository is > an interesting matter. I'd say >=20 > git update-ref --force --no-deref -d HEAD && > test_must_fail git show-ref --verify -q HEAD && > cp -f .git/HEAD.orig .git/HEAD && > git show-ref --verify -q "$m" >=20 > to ensure that other than removing HEAD and corrupting the > repository, it did not cause permanent damage by removing the > underlying ref, perhaps. >=20 > We may want to add some NEEDSWORK comment around such tests. I think the proper way to do the test would be to create a non-mandatory symref and delete it. That'd also be a nice preparation for the `--force` parameter already. > > test_expect_success 'delete symref without dereference when the referr= ed ref is packed' ' > > @@ -208,7 +214,7 @@ test_expect_success 'delete symref without derefere= nce when the referred ref is > > git commit -m foo && > > git pack-refs --all && > > git update-ref --no-deref -d HEAD && > > - test_path_is_missing .git/HEAD > > + test_must_fail git show-ref --verify -q HEAD > > ' >=20 > Does this share the same issue as the above? Yup, it does. Patrick --ICly2YHBkf7mGUAp Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAl+qiM4ACgkQVbJhu7ck PpRw/w/+Jrp2WPEOdS3gPJsHiiwgCTSU3GqZoT8t1EjvqXwMyrxsqZQYNXzQ4IIP 1DxVonGMMoE0UfnF3hjq2w6wVLrGSNQLEOz+qYypb36Xi922wb9/QawtPNSMrYEN /oqEAsvo/aXoGn/CgiU2QOG46/1PArtEF7Nm/hJda84M5ZspdNl9LaZRTXRLISJf IwAdz33Vk/XkIjNGM1u0oyc1uPVjNxnL9MwFHFSWR4xqyK1GcB1m5OkOISyDd6Rr eoNN4gUA1BImLbcCHqqmxILxeo7WsUhL/Z22tmBRrnT95/YNdZlarxIAqxWKipFf 6G+yWPnpAlopN3+Q1+mlkF2izN2E+EjK6Ve8J6dbcASQHqolU7pnDHsJTgO9wyl8 36cq4g0xhu6mrGEz4OfiIxP4AyprWMtsmZOoEJpF16r3xcMlWBqtXpgJTVlmBTHU smPOtvn6vMdEcDc2UIOstRev2DSiNhQez/J2xq4BHDzEY309GQbb+OglNNGDDtg7 NRUqq1PgMEYGKSwaCRfYmljMrFif0errrYbic/ZnnD8gAPGOJGQrm4D6M/dnwYXc ScEcUgBUZI1HqqDOe30ymh4FgUxej0fLQ685BdvwKwh2+qZJHtB5TUmUAv4I9W2D Ip4Wmpk8nQ2H/+dsjbOiXvMlQdU4xB3r75Zhl2RMAlMPoZijNUo= =TksO -----END PGP SIGNATURE----- --ICly2YHBkf7mGUAp--