git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Patrick Steinhardt <ps@pks.im>
Cc: git@vger.kernel.org, peff@peff.net
Subject: Re: [PATCH v2 1/4] t1400: Avoid touching refs on filesystem
Date: Mon, 09 Nov 2020 11:48:20 -0800	[thread overview]
Message-ID: <xmqqpn4mb9bv.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <9b49e849eaf6786c63016d767d2ad56112d08d51.1604908834.git.ps@pks.im> (Patrick Steinhardt's message of "Mon, 9 Nov 2020 11:06:47 +0100")

Patrick Steinhardt <ps@pks.im> writes:

> The testcase t1400 exercises the git-update-ref(1) utility. To do so,
> many tests directly read and write references via the filesystem,
> assuming that we always use loose and/or packed references. While this
> is true now, it'll change with the introduction of the reftable backend.
>
> Convert those tests to use git-update-ref(1) and git-show-ref(1) where
> possible. As some tests exercise behaviour with broken references and
> neither of those tools actually allows writing or reading broken
> references, this commit doesn't adjust all tests.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t1400-update-ref.sh | 63 ++++++++++++++++++++++++-------------------
>  1 file changed, 35 insertions(+), 28 deletions(-)
>
> 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=$n_dir/fixes
>  outside=refs/foo
>  bare=bare-repo
>  
> +# Some of the tests delete HEAD, which causes us to not treat the current
> +# working directory as a Git repository anymore. To avoid using any potential
> +# parent repository to be discovered, we need to set up the ceiling directories.
> +GIT_CEILING_DIRECTORIES="$PWD/.."
> +export GIT_CEILING_DIRECTORIES
> +

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?

>  create_test_commits ()
>  {
>  	prfx="$1"
> @@ -48,17 +54,17 @@ test_expect_success "fail to delete $m with stale ref" '
>  	test $B = "$(git show-ref -s --verify $m)"
>  '
>  test_expect_success "delete $m" '
> -	test_when_finished "rm -f .git/$m" &&
> +	test_when_finished "git update-ref -d $m" &&
>  	git update-ref -d $m $B &&
> -	test_path_is_missing .git/$m
> +	test_must_fail git show-ref --verify -q $m
>  '
>  
>  test_expect_success "delete $m without oldvalue verification" '
> -	test_when_finished "rm -f .git/$m" &&
> +	test_when_finished "git update-ref -d $m" &&
>  	git update-ref $m $A &&
>  	test $A = $(git show-ref -s --verify $m) &&
>  	git update-ref -d $m &&
> -	test_path_is_missing .git/$m
> +	test_must_fail git show-ref --verify -q $m
>  '
>  
>  test_expect_success "fail to create $n" '
> @@ -80,26 +86,26 @@ test_expect_success "fail to delete $m (by HEAD) with stale ref" '
>  	test $B = $(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
>  '

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.

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.

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

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?

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

	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"

to ensure that other than removing HEAD and corrupting the
repository, it did not cause permanent damage by removing the
underlying ref, perhaps.

We may want to add some NEEDSWORK comment around such tests.

>  test_expect_success 'delete symref without dereference when the referred ref is packed' '
> @@ -208,7 +214,7 @@ test_expect_success 'delete symref without dereference 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
>  '

Does this share the same issue as the above?

Thanks.

  reply	other threads:[~2020-11-09 19:48 UTC|newest]

Thread overview: 41+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-04 14:57 [PATCH 0/2] update-ref: Allow creation of multiple transactions Patrick Steinhardt
2020-11-04 14:57 ` [PATCH 1/2] " Patrick Steinhardt
2020-11-05 19:29   ` Jeff King
2020-11-05 21:34     ` Junio C Hamano
2020-11-06 17:52       ` Jeff King
2020-11-06 19:30         ` Junio C Hamano
2020-11-06  6:36     ` Patrick Steinhardt
2020-11-04 14:57 ` [PATCH 2/2] p1400: Use `git-update-ref --stdin` to test " Patrick Steinhardt
2020-11-05 19:34   ` Jeff King
2020-11-09 10:06 ` [PATCH v2 0/4] update-ref: Allow creation of " Patrick Steinhardt
2020-11-09 10:06   ` [PATCH v2 1/4] t1400: Avoid touching refs on filesystem Patrick Steinhardt
2020-11-09 19:48     ` Junio C Hamano [this message]
2020-11-09 22:28       ` Jeff King
2020-11-10 12:34       ` Patrick Steinhardt
2020-11-10 17:04         ` Junio C Hamano
2020-11-09 10:06   ` [PATCH v2 2/4] update-ref: Allow creation of multiple transactions Patrick Steinhardt
2020-11-09 10:06   ` [PATCH v2 3/4] p1400: Use `git-update-ref --stdin` to test " Patrick Steinhardt
2020-11-09 10:07   ` [PATCH v2 4/4] update-ref: Disallow restart of ongoing transactions Patrick Steinhardt
2020-11-09 19:53     ` Junio C Hamano
2020-11-09 22:33   ` [PATCH v2 0/4] update-ref: Allow creation of multiple transactions Jeff King
2020-11-09 22:38     ` Junio C Hamano
2020-11-11  6:58 ` [PATCH v3 0/4] update-ref: allow " Patrick Steinhardt
2020-11-11  6:58   ` [PATCH v3 1/4] t1400: avoid touching refs on filesystem Patrick Steinhardt
2020-11-11  9:04     ` SZEDER Gábor
2020-11-11 10:00       ` Patrick Steinhardt
2020-11-11 10:24         ` SZEDER Gábor
2020-11-11 23:06     ` Jeff King
2020-11-13  8:08       ` Patrick Steinhardt
2020-11-11  6:58   ` [PATCH v3 2/4] update-ref: allow creation of multiple transactions Patrick Steinhardt
2020-11-11  6:58   ` [PATCH v3 3/4] p1400: use `git-update-ref --stdin` to test " Patrick Steinhardt
2020-11-11  6:58   ` [PATCH v3 4/4] update-ref: disallow "start" for ongoing transactions Patrick Steinhardt
2020-11-13  8:12 ` [PATCH v4 0/4] update-ref: allow creation of multiple transactions Patrick Steinhardt
2020-11-13  8:12   ` [PATCH v4 1/4] t1400: avoid touching refs on filesystem Patrick Steinhardt
2020-11-13 20:40     ` Jeff King
2020-11-18  6:48       ` Patrick Steinhardt
2020-11-18  7:02         ` Junio C Hamano
2020-11-13  8:12   ` [PATCH v4 2/4] update-ref: allow creation of multiple transactions Patrick Steinhardt
2020-11-13  8:12   ` [PATCH v4 3/4] p1400: use `git-update-ref --stdin` to test " Patrick Steinhardt
2020-11-13  8:12   ` [PATCH v4 4/4] update-ref: disallow "start" for ongoing transactions Patrick Steinhardt
2020-11-25 22:37   ` [PATCH v4 0/4] update-ref: allow creation of multiple transactions Junio C Hamano
2020-11-26  0:42     ` Jeff King

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=xmqqpn4mb9bv.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    --cc=ps@pks.im \
    /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).