From: Taylor Blau <me@ttaylorr.com>
Date: Mon, 23 Oct 2023 16:42:08 GMT
Subject: Re: [PATCH 08/11] t4207: delete replace references via git-update-ref(1)
Message-ID: <ZTaiYEyhKT/yZwHZ@nand.local>
In-Reply-To: <c4d09e3e5dbd11221cc4d229b815434d441cdb4d.1697607222.git.ps@pks.im>

On Wed, Oct 18, 2023 at 07:35:37AM +0200, Patrick Steinhardt wrote:
> In t4207 we set up a set of replace objects via git-replace(1). Because
> these references should not be impacting subsequent tests we also set up
> some cleanup logic that deletes the replacement references via a call to
> `rm -rf`. This reaches into the internal implementation details of the
> reference backend and will thus break when we grow an alternative refdb
> implementation.
>
> Refactor the tests to delete the replacement refs via Git commands so
> that we become independent of the actual refdb that's in use. As we
> don't have a nice way to delete all replacements or all references in a
> certain namespace, we opt for a combination of git-for-each-ref(1) and
> git-update-ref(1)'s `--stdin` mode.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  t/t4207-log-decoration-colors.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
> index 21986a866df..d138e513a04 100755
> --- a/t/t4207-log-decoration-colors.sh
> +++ b/t/t4207-log-decoration-colors.sh
> @@ -71,7 +71,7 @@ ${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
>  '
>
>  test_expect_success 'test coloring with replace-objects' '
> -	test_when_finished rm -rf .git/refs/replace* &&
> +	test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} | git update-ref --stdin" &&

Here and below, should we avoid the for-each-ref showing up on the
left-hand side of the pipe? I'd think we want something closer to:

    test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} >in && git update-ref --stdin <in" &&

But having to quote the --format argument with "${SQ}"s makes the whole
thing a little awkward to read and parse.

Do you think that something like the below would be a readability
improvement?

diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh
index d138e513a0..de8f6638cb 100755
--- a/t/t4207-log-decoration-colors.sh
+++ b/t/t4207-log-decoration-colors.sh
@@ -70,8 +70,13 @@ ${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A
 	cmp_filtered_decorations
 '

--- >8 ---
+remove_replace_refs () {
+	git for-each-ref 'refs/replace*/**' --format='delete %(refname)' >in &&
+	git update-ref --stdin <in
+}
+
 test_expect_success 'test coloring with replace-objects' '
-	test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} | git update-ref --stdin" &&
+	test_when_finished remove_replace_refs &&
 	test_commit C &&
 	test_commit D &&

@@ -99,7 +104,7 @@ EOF
 '

 test_expect_success 'test coloring with grafted commit' '
-	test_when_finished "git for-each-ref refs/replace*/** --format=${SQ}delete %(refname)${SQ} | git update-ref --stdin" &&
+	test_when_finished remove_replace_refs &&

 	git replace --graft HEAD HEAD~2 &&
--- 8< ---

Thanks,
Taylor