Re: [PATCH 08/11] t4207: delete replace references via git-update-ref(1)

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 24, 2023, 06:42 UTC
Message-ID: <ZTdnWoTaAMuVMc7v@tanuki>
In-Reply-To: <ZTaiYEyhKT/yZwHZ@nand.local>

On Mon, Oct 23, 2023 at 12:42:08PM -0400, Taylor Blau wrote:

Show 40 quoted lines

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

Yes, this certainly looks like a good improvement to me, thanks!

Patrick

Show 32 quoted lines

> 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