| Message ID | c4d09e3e5dbd11221cc4d229b815434d441cdb4d.1697607222.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | t: reduce direct disk access to data structures | expand |
On Wed, Oct 18, 2023 at 7:35 AM Patrick Steinhardt <ps@pks.im> 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. There is a test helper that can directly access the ref database, t/helper/test-ref-store.c. If you use that manipulate refs for testing purposes, you make the test independent of behavior git-for-each-ref/git-update-ref, which is what you want for testing replace-objects?
On Wed, Oct 18, 2023 at 03:27:11PM +0200, Han-Wen Nienhuys wrote: > On Wed, Oct 18, 2023 at 7:35 AM Patrick Steinhardt <ps@pks.im> 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. > > There is a test helper that can directly access the ref database, > t/helper/test-ref-store.c. > > If you use that manipulate refs for testing purposes, you make the > test independent of behavior git-for-each-ref/git-update-ref, which is > what you want for testing replace-objects? Is there any specific reason why we shouldn't be using git-for-each-ref(1) or git-update-ref(1) here? Neither of those commands are part of the system under test, as we rather care about git-log(1) here. So as those commands are already being verified in other tests I think it should be fine to assume that they work as intended here. Happy to hear differing viewpoints though. Patrick
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
On Mon, Oct 23, 2023 at 12:42:08PM -0400, Taylor Blau wrote: > 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 > 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
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" && test_commit C && test_commit D && @@ -99,7 +99,7 @@ EOF ' test_expect_success 'test coloring with grafted commit' ' - 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" && git replace --graft HEAD HEAD~2 &&
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(-)