| Message ID | b52e59cdac75e6c4530cb39f7dcb41bb327f50e2.1641556319.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: excessive hook execution with packed refs | expand |
Patrick Steinhardt <ps@pks.im> writes: > Add tests which demonstate which demonstrates that we're executing the You demonstrate too often, which may be the point of the test, but looks wrong. I actually think this should be done as part of the fix to the code itself, which presumably is a single-liner to tell the "skip when running delete in packed-refs backend". IOW, just fix the code and test how the externally observable behaviour of the code should be in new tests, in the same commit. > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh > index 6c941027a8..0567fbdf0b 100755 > --- a/t/t1416-ref-transaction-hooks.sh > +++ b/t/t1416-ref-transaction-hooks.sh > @@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' ' > test_cmp expect target-repo.git/actual > ' > > +test_expect_success 'hook does not get called on packing refs' ' > + # Pack references first such that we are in a known state. > + git pack-refs --all && > + > + write_script .git/hooks/reference-transaction <<-\EOF && > + echo "$@" >>actual > + cat >>actual > + EOF > + rm -f actual && > + > + git update-ref refs/heads/unpacked-ref $POST_OID && > + git pack-refs --all && > + > + # We only expect a single hook invocation, which is the call to > + # git-update-ref(1). But currently, packing refs will also trigger the > + # hook. > + cat >expect <<-EOF && > + prepared > + $ZERO_OID $POST_OID refs/heads/unpacked-ref > + committed > + $ZERO_OID $POST_OID refs/heads/unpacked-ref > + prepared > + $ZERO_OID $POST_OID refs/heads/unpacked-ref > + committed > + $ZERO_OID $POST_OID refs/heads/unpacked-ref > + prepared > + $POST_OID $ZERO_OID refs/heads/unpacked-ref > + committed > + $POST_OID $ZERO_OID refs/heads/unpacked-ref > + EOF > + > + test_cmp expect actual > +' > + > +test_expect_success 'deleting packed ref calls hook once' ' > + # Create a reference and pack it. > + git update-ref refs/heads/to-be-deleted $POST_OID && > + git pack-refs --all && > + > + write_script .git/hooks/reference-transaction <<-\EOF && > + echo "$@" >>actual > + cat >>actual > + EOF > + rm -f actual && > + > + git update-ref -d refs/heads/to-be-deleted $POST_OID && > + > + # We only expect a single hook invocation, which is the logical > + # deletion. But currently, we see two interleaving transactions, once > + # for deleting the loose refs and once for deleting the packed ref. > + cat >expect <<-EOF && > + prepared > + $ZERO_OID $ZERO_OID refs/heads/to-be-deleted > + prepared > + $POST_OID $ZERO_OID refs/heads/to-be-deleted > + committed > + $ZERO_OID $ZERO_OID refs/heads/to-be-deleted > + committed > + $POST_OID $ZERO_OID refs/heads/to-be-deleted > + EOF > + > + test_cmp expect actual > +' > + > test_done
On Fri, Jan 7, 2022 at 3:09 PM Patrick Steinhardt <ps@pks.im> wrote: > Add tests which demonstate which demonstrates that we're executing the > reference-transaction hook too often in some cases, which thus leaks > implementation details about the reference store's implementation > itself. Behaviour will be fixed in follow-up commits. s/which demonstate which demonstrates/which demonstrate/ > Signed-off-by: Patrick Steinhardt <ps@pks.im>
On Fri, Jan 07, 2022 at 05:31:04PM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > Add tests which demonstate which demonstrates that we're executing the > > You demonstrate too often, which may be the point of the test, but > looks wrong. > > I actually think this should be done as part of the fix to the code > itself, which presumably is a single-liner to tell the "skip when > running delete in packed-refs backend". IOW, just fix the code and > test how the externally observable behaviour of the code should be > in new tests, in the same commit. The reason why I chose to split this out into a separate commit is that it makes it easier to see what behaviour exactly is changing. If it was a single step, then a reader would only see the post-image behaviour but cannot reason about the pre-image behaviour without puzzling everything together. So personally I'd prefer to keep it as a separate step, but I'm not opposed to merging them if you still disagree with my reasoning above. Patrick > > diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh > > index 6c941027a8..0567fbdf0b 100755 > > --- a/t/t1416-ref-transaction-hooks.sh > > +++ b/t/t1416-ref-transaction-hooks.sh > > @@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' ' > > test_cmp expect target-repo.git/actual > > ' > > > > +test_expect_success 'hook does not get called on packing refs' ' > > + # Pack references first such that we are in a known state. > > + git pack-refs --all && > > + > > + write_script .git/hooks/reference-transaction <<-\EOF && > > + echo "$@" >>actual > > + cat >>actual > > + EOF > > + rm -f actual && > > + > > + git update-ref refs/heads/unpacked-ref $POST_OID && > > + git pack-refs --all && > > + > > + # We only expect a single hook invocation, which is the call to > > + # git-update-ref(1). But currently, packing refs will also trigger the > > + # hook. > > + cat >expect <<-EOF && > > + prepared > > + $ZERO_OID $POST_OID refs/heads/unpacked-ref > > + committed > > + $ZERO_OID $POST_OID refs/heads/unpacked-ref > > + prepared > > + $ZERO_OID $POST_OID refs/heads/unpacked-ref > > + committed > > + $ZERO_OID $POST_OID refs/heads/unpacked-ref > > + prepared > > + $POST_OID $ZERO_OID refs/heads/unpacked-ref > > + committed > > + $POST_OID $ZERO_OID refs/heads/unpacked-ref > > + EOF > > + > > + test_cmp expect actual > > +' > > + > > +test_expect_success 'deleting packed ref calls hook once' ' > > + # Create a reference and pack it. > > + git update-ref refs/heads/to-be-deleted $POST_OID && > > + git pack-refs --all && > > + > > + write_script .git/hooks/reference-transaction <<-\EOF && > > + echo "$@" >>actual > > + cat >>actual > > + EOF > > + rm -f actual && > > + > > + git update-ref -d refs/heads/to-be-deleted $POST_OID && > > + > > + # We only expect a single hook invocation, which is the logical > > + # deletion. But currently, we see two interleaving transactions, once > > + # for deleting the loose refs and once for deleting the packed ref. > > + cat >expect <<-EOF && > > + prepared > > + $ZERO_OID $ZERO_OID refs/heads/to-be-deleted > > + prepared > > + $POST_OID $ZERO_OID refs/heads/to-be-deleted > > + committed > > + $ZERO_OID $ZERO_OID refs/heads/to-be-deleted > > + committed > > + $POST_OID $ZERO_OID refs/heads/to-be-deleted > > + EOF > > + > > + test_cmp expect actual > > +' > > + > > test_done
diff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh index 6c941027a8..0567fbdf0b 100755 --- a/t/t1416-ref-transaction-hooks.sh +++ b/t/t1416-ref-transaction-hooks.sh @@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' ' test_cmp expect target-repo.git/actual ' +test_expect_success 'hook does not get called on packing refs' ' + # Pack references first such that we are in a known state. + git pack-refs --all && + + write_script .git/hooks/reference-transaction <<-\EOF && + echo "$@" >>actual + cat >>actual + EOF + rm -f actual && + + git update-ref refs/heads/unpacked-ref $POST_OID && + git pack-refs --all && + + # We only expect a single hook invocation, which is the call to + # git-update-ref(1). But currently, packing refs will also trigger the + # hook. + cat >expect <<-EOF && + prepared + $ZERO_OID $POST_OID refs/heads/unpacked-ref + committed + $ZERO_OID $POST_OID refs/heads/unpacked-ref + prepared + $ZERO_OID $POST_OID refs/heads/unpacked-ref + committed + $ZERO_OID $POST_OID refs/heads/unpacked-ref + prepared + $POST_OID $ZERO_OID refs/heads/unpacked-ref + committed + $POST_OID $ZERO_OID refs/heads/unpacked-ref + EOF + + test_cmp expect actual +' + +test_expect_success 'deleting packed ref calls hook once' ' + # Create a reference and pack it. + git update-ref refs/heads/to-be-deleted $POST_OID && + git pack-refs --all && + + write_script .git/hooks/reference-transaction <<-\EOF && + echo "$@" >>actual + cat >>actual + EOF + rm -f actual && + + git update-ref -d refs/heads/to-be-deleted $POST_OID && + + # We only expect a single hook invocation, which is the logical + # deletion. But currently, we see two interleaving transactions, once + # for deleting the loose refs and once for deleting the packed ref. + cat >expect <<-EOF && + prepared + $ZERO_OID $ZERO_OID refs/heads/to-be-deleted + prepared + $POST_OID $ZERO_OID refs/heads/to-be-deleted + committed + $ZERO_OID $ZERO_OID refs/heads/to-be-deleted + committed + $POST_OID $ZERO_OID refs/heads/to-be-deleted + EOF + + test_cmp expect actual +' + test_done
Add tests which demonstate which demonstrates that we're executing the reference-transaction hook too often in some cases, which thus leaks implementation details about the reference store's implementation itself. Behaviour will be fixed in follow-up commits. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t1416-ref-transaction-hooks.sh | 64 ++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+)