From: Patrick Steinhardt <hidden> Date: 2021-12-07 10:56:38
Hi,
as reported by Waleed in [1], the reference-transaction hook is being
executed when packing refs. Given that the hook ideally ought to track
logical updates to refs instead of leaking low-level implementation
details of how the files backend works, this is understandably leading
to some confusion.
This patch series aims to fix that by improving how the tandom of loose
and packed refs backends interact such that we skip executing the hook
when the loose backend:
- repacks references.
- needs to delete packed refs when deleting a loose ref would
uncover that packed ref.
Patrick
[1]: [off-list ref]
Patrick Steinhardt (6):
refs: open-code deletion of packed refs
refs: allow passing flags when beginning transactions
refs: allow skipping the reference-transaction hook
refs: demonstrate excessive execution of the reference-transaction
hook
refs: do not execute reference-transaction hook on packing refs
refs: skip hooks when deleting uncovered packed refs
refs.c | 11 +++++--
refs.h | 8 ++++-
refs/files-backend.c | 25 +++++++++++-----
refs/packed-backend.c | 30 ++++++++++++++-----
refs/packed-backend.h | 6 ++++
refs/refs-internal.h | 1 +
sequencer.c | 2 +-
t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++
8 files changed, 113 insertions(+), 20 deletions(-)
--
2.34.1
From: Patrick Steinhardt <hidden> Date: 2021-12-07 10:56:41
When deleting loose refs, then we also have to delete the refs in the
packed backend. This is done by calling `refs_delete_refs()`, which
then uses the packed-backend's logic to delete refs. This doesn't allow
us to exercise any control over the reference transaction which is being
created in the packed backend, which is required in a subsequent commit.
Extract a new function `packed_refs_delete_refs()`, which hosts most of
the logic to delete refs except for creating the transaction itself.
Like this, we can easily create the transactionion in the files backend
and thus exert more control over it.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 12 +++++++++---
refs/packed-backend.c | 28 +++++++++++++++++++++-------
refs/packed-backend.h | 6 ++++++
3 files changed, 36 insertions(+), 10 deletions(-)
@@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,staticintpacked_delete_refs(structref_store*ref_store,constchar*msg,structstring_list*refnames,unsignedintflags){-structpacked_ref_store*refs=-packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");structstrbuferr=STRBUF_INIT;structref_transaction*transaction;-structstring_list_item*item;intret;-(void)refs;/* We need the check above, but don't use the variable */-if(!refnames->nr)return0;
@@ -1544,6 +1539,27 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,if(!transaction)return-1;+ret=packed_refs_delete_refs(ref_store,transaction,+msg,refnames,flags);++ref_transaction_free(transaction);+returnret;+}++intpacked_refs_delete_refs(structref_store*ref_store,+structref_transaction*transaction,+constchar*msg,+structstring_list*refnames,+unsignedintflags)+{+structpacked_ref_store*refs=+packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");+structstrbuferr=STRBUF_INIT;+structstring_list_item*item;+intret;++(void)(refs);/* We need the check above, but don't use the variable */+for_each_string_list_item(item,refnames){if(ref_transaction_delete(transaction,item->string,NULL,flags,msg,&err)){
@@ -1554,7 +1570,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,}ret=ref_transaction_commit(transaction,&err);-if(ret){if(refnames->nr==1)error(_("could not delete reference %s: %s"),
@@ -1563,7 +1578,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,error(_("could not delete references: %s"),err.buf);}-ref_transaction_free(transaction);strbuf_release(&err);returnret;}
From: Patrick Steinhardt <hidden> Date: 2021-12-07 10:56:44
We do not currently have any flags when creating reference transactions,
but we'll add one to disable execution of the reference transaction hook
in some cases.
Allow passing flags to `ref_store_transaction_begin()` to prepare for
this change.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs.c | 8 +++++---
refs.h | 3 ++-
refs/files-backend.c | 10 +++++-----
refs/packed-backend.c | 2 +-
refs/refs-internal.h | 1 +
sequencer.c | 2 +-
6 files changed, 15 insertions(+), 11 deletions(-)
@@ -2767,7 +2767,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,*/if(!packed_transaction){packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,err);+refs->packed_ref_store,0,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3038,7 +3038,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,err);+packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,0,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
From: Patrick Steinhardt <hidden> Date: 2021-12-07 10:56:48
The reference-transaction hook is executing whenever we prepare, commit
or abort a reference transaction. While this is mostly intentional, in
case of the files backend we're leaking the implementation detail that
the store is in fact a composite store with one loose and one packed
backend to the caller. So while we want to execute the hook for all
logical updates, executing it for such implementation details is
unexpected.
Prepare for a fix by adding a new flag which allows to skip execution of
the hook.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs.c | 3 +++
refs.h | 5 +++++
2 files changed, 8 insertions(+)
From: Patrick Steinhardt <hidden> Date: 2021-12-07 10:56:51
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 <redacted>
---
t/t1416-ref-transaction-hooks.sh | 64 ++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
@@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' 'test_cmpexpecttarget-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.+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-refrefs/heads/unpacked-ref$POST_OID&&+gitpack-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_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+committed+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+EOF++test_cmpexpectactual+'++test_expect_success'deleting packed ref calls hook once''+# Create a reference and pack it.+gitupdate-refrefs/heads/to-be-deleted$POST_OID&&+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-ref-drefs/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_OIDrefs/heads/to-be-deleted+prepared+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+EOF++test_cmpexpectactual+'+ test_done
From: Patrick Steinhardt <hidden> Date: 2021-12-07 10:56:55
The reference-transaction hook is supposed to track logical changes to
references, but it currently also gets executed when packing refs in a
repository. This is unexpected and ultimately not all that useful:
packing refs is not supposed to result in any user-visible change to the
refs' state, and it ultimately is an implementation detail of how refs
stores work.
Fix this excessive execution of the hook when packing refs.
Reported-by: Waleed Khan <redacted>
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 6 ++++--
t/t1416-ref-transaction-hooks.sh | 11 +----------
2 files changed, 5 insertions(+), 12 deletions(-)
@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' 'gitpack-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.+# git-update-ref(1).cat>expect<<-EOF&&prepared$ZERO_OID$POST_OIDrefs/heads/unpacked-refcommitted$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-committed-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$POST_OID$ZERO_OIDrefs/heads/unpacked-ref-committed-$POST_OID$ZERO_OIDrefs/heads/unpacked-refEOFtest_cmpexpectactual
From: Patrick Steinhardt <hidden> Date: 2021-12-07 10:57:00
When deleting refs from the loose-files refs backend, then we need to be
careful to also delete the same ref from the packed refs backend, if it
exists. If we don't, then deleting the loose ref would "uncover" the
packed ref. We thus always have to queue up deletions of refs for both
the loose and the packed refs backend. This is done in two separate
transactions, where the end result is that the reference-transaction
hook is executed twice for the deleted refs.
This behaviour is quite misleading: it's exposing implementation details
of how the files backend works to the user, in contrast to the logical
updates that we'd really want to expose via the hook. Worse yet, whether
the hook gets executed once or twice depends on how well-packed the
repository is: if the ref only exists as a loose ref, then we execute it
once, otherwise if it is also packed then we execute it twice.
Fix this behaviour and don't execute the reference-transaction hook at
all when refs in the packed-refs backend if it's driven by the files
backend.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 9 ++++++---
t/t1416-ref-transaction-hooks.sh | 7 +------
2 files changed, 7 insertions(+), 9 deletions(-)
@@ -2769,7 +2770,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,*/if(!packed_transaction){packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,0,err);+refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3040,7 +3042,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,0,err);+packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' 'gitupdate-ref-drefs/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.+# deletion.cat>expect<<-EOF&&-prepared-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deletedprepared$POST_OID$ZERO_OIDrefs/heads/to-be-deletedcommitted-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted-committed$POST_OID$ZERO_OIDrefs/heads/to-be-deletedEOF
From: Patrick Steinhardt <hidden> Date: 2022-01-07 11:55:38
Hi,
this is a resend of version 1 of this patch series to hopefully entice
some reviews. The only change is that v2 is rebased onto the current
main branch at commit e83ba647f7 (The seventh batch, 2022-01-05). The
following was from the orignial cover letter:
As reported by Waleed in [1], the reference-transaction hook is being
executed when packing refs. Given that the hook ideally ought to track
logical updates to refs instead of leaking low-level implementation
details of how the files backend works, this is understandably leading
to some confusion.
This patch series aims to fix that by improving how the tandom of loose
and packed refs backends interact such that we skip executing the hook
when the loose backend:
- repacks references.
- needs to delete packed refs when deleting a loose ref would
uncover that packed ref.
Patrick
[1]: [off-list ref]
Patrick Steinhardt (6):
refs: open-code deletion of packed refs
refs: allow passing flags when beginning transactions
refs: allow skipping the reference-transaction hook
refs: demonstrate excessive execution of the reference-transaction
hook
refs: do not execute reference-transaction hook on packing refs
refs: skip hooks when deleting uncovered packed refs
refs.c | 11 +++++--
refs.h | 8 ++++-
refs/files-backend.c | 25 +++++++++++-----
refs/packed-backend.c | 30 ++++++++++++++-----
refs/packed-backend.h | 6 ++++
refs/refs-internal.h | 1 +
sequencer.c | 2 +-
t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++
8 files changed, 113 insertions(+), 20 deletions(-)
--
2.34.1
From: Patrick Steinhardt <hidden> Date: 2022-01-07 11:55:40
When deleting loose refs, then we also have to delete the refs in the
packed backend. This is done by calling `refs_delete_refs()`, which
then uses the packed-backend's logic to delete refs. This doesn't allow
us to exercise any control over the reference transaction which is being
created in the packed backend, which is required in a subsequent commit.
Extract a new function `packed_refs_delete_refs()`, which hosts most of
the logic to delete refs except for creating the transaction itself.
Like this, we can easily create the transactionion in the files backend
and thus exert more control over it.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 12 +++++++++---
refs/packed-backend.c | 28 +++++++++++++++++++++-------
refs/packed-backend.h | 6 ++++++
3 files changed, 36 insertions(+), 10 deletions(-)
@@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,staticintpacked_delete_refs(structref_store*ref_store,constchar*msg,structstring_list*refnames,unsignedintflags){-structpacked_ref_store*refs=-packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");structstrbuferr=STRBUF_INIT;structref_transaction*transaction;-structstring_list_item*item;intret;-(void)refs;/* We need the check above, but don't use the variable */-if(!refnames->nr)return0;
@@ -1544,6 +1539,27 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,if(!transaction)return-1;+ret=packed_refs_delete_refs(ref_store,transaction,+msg,refnames,flags);++ref_transaction_free(transaction);+returnret;+}++intpacked_refs_delete_refs(structref_store*ref_store,+structref_transaction*transaction,+constchar*msg,+structstring_list*refnames,+unsignedintflags)+{+structpacked_ref_store*refs=+packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");+structstrbuferr=STRBUF_INIT;+structstring_list_item*item;+intret;++(void)(refs);/* We need the check above, but don't use the variable */+for_each_string_list_item(item,refnames){if(ref_transaction_delete(transaction,item->string,NULL,flags,msg,&err)){
@@ -1554,7 +1570,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,}ret=ref_transaction_commit(transaction,&err);-if(ret){if(refnames->nr==1)error(_("could not delete reference %s: %s"),
@@ -1563,7 +1578,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,error(_("could not delete references: %s"),err.buf);}-ref_transaction_free(transaction);strbuf_release(&err);returnret;}
From: Patrick Steinhardt <hidden> Date: 2022-01-07 11:55:44
We do not currently have any flags when creating reference transactions,
but we'll add one to disable execution of the reference transaction hook
in some cases.
Allow passing flags to `ref_store_transaction_begin()` to prepare for
this change.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs.c | 8 +++++---
refs.h | 3 ++-
refs/files-backend.c | 10 +++++-----
refs/packed-backend.c | 2 +-
refs/refs-internal.h | 1 +
sequencer.c | 2 +-
6 files changed, 15 insertions(+), 11 deletions(-)
@@ -2773,7 +2773,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,*/if(!packed_transaction){packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,err);+refs->packed_ref_store,0,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3044,7 +3044,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,err);+packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,0,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
From: Patrick Steinhardt <hidden> Date: 2022-01-07 11:55:52
The reference-transaction hook is executing whenever we prepare, commit
or abort a reference transaction. While this is mostly intentional, in
case of the files backend we're leaking the implementation detail that
the store is in fact a composite store with one loose and one packed
backend to the caller. So while we want to execute the hook for all
logical updates, executing it for such implementation details is
unexpected.
Prepare for a fix by adding a new flag which allows to skip execution of
the hook.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs.c | 3 +++
refs.h | 5 +++++
2 files changed, 8 insertions(+)
From: Patrick Steinhardt <hidden> Date: 2022-01-07 11:56:06
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 <redacted>
---
t/t1416-ref-transaction-hooks.sh | 64 ++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
@@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' 'test_cmpexpecttarget-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.+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-refrefs/heads/unpacked-ref$POST_OID&&+gitpack-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_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+committed+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+EOF++test_cmpexpectactual+'++test_expect_success'deleting packed ref calls hook once''+# Create a reference and pack it.+gitupdate-refrefs/heads/to-be-deleted$POST_OID&&+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-ref-drefs/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_OIDrefs/heads/to-be-deleted+prepared+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+EOF++test_cmpexpectactual+'+ test_done
From: Patrick Steinhardt <hidden> Date: 2022-01-07 11:56:08
When deleting refs from the loose-files refs backend, then we need to be
careful to also delete the same ref from the packed refs backend, if it
exists. If we don't, then deleting the loose ref would "uncover" the
packed ref. We thus always have to queue up deletions of refs for both
the loose and the packed refs backend. This is done in two separate
transactions, where the end result is that the reference-transaction
hook is executed twice for the deleted refs.
This behaviour is quite misleading: it's exposing implementation details
of how the files backend works to the user, in contrast to the logical
updates that we'd really want to expose via the hook. Worse yet, whether
the hook gets executed once or twice depends on how well-packed the
repository is: if the ref only exists as a loose ref, then we execute it
once, otherwise if it is also packed then we execute it twice.
Fix this behaviour and don't execute the reference-transaction hook at
all when refs in the packed-refs backend if it's driven by the files
backend.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 9 ++++++---
t/t1416-ref-transaction-hooks.sh | 7 +------
2 files changed, 7 insertions(+), 9 deletions(-)
@@ -2775,7 +2776,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,*/if(!packed_transaction){packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,0,err);+refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3046,7 +3048,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,0,err);+packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' 'gitupdate-ref-drefs/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.+# deletion.cat>expect<<-EOF&&-prepared-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deletedprepared$POST_OID$ZERO_OIDrefs/heads/to-be-deletedcommitted-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted-committed$POST_OID$ZERO_OIDrefs/heads/to-be-deletedEOF
From: Patrick Steinhardt <hidden> Date: 2022-01-07 11:56:27
The reference-transaction hook is supposed to track logical changes to
references, but it currently also gets executed when packing refs in a
repository. This is unexpected and ultimately not all that useful:
packing refs is not supposed to result in any user-visible change to the
refs' state, and it ultimately is an implementation detail of how refs
stores work.
Fix this excessive execution of the hook when packing refs.
Reported-by: Waleed Khan <redacted>
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 6 ++++--
t/t1416-ref-transaction-hooks.sh | 11 +----------
2 files changed, 5 insertions(+), 12 deletions(-)
@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' 'gitpack-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.+# git-update-ref(1).cat>expect<<-EOF&&prepared$ZERO_OID$POST_OIDrefs/heads/unpacked-refcommitted$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-committed-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$POST_OID$ZERO_OIDrefs/heads/unpacked-ref-committed-$POST_OID$ZERO_OIDrefs/heads/unpacked-refEOFtest_cmpexpectactual
From: Junio C Hamano <hidden> Date: 2022-01-08 01:31:09
Patrick Steinhardt [off-list ref] 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.
@@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' 'test_cmpexpecttarget-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.+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-refrefs/heads/unpacked-ref$POST_OID&&+gitpack-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_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+committed+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+EOF++test_cmpexpectactual+'++test_expect_success'deleting packed ref calls hook once''+# Create a reference and pack it.+gitupdate-refrefs/heads/to-be-deleted$POST_OID&&+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-ref-drefs/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_OIDrefs/heads/to-be-deleted+prepared+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+EOF++test_cmpexpectactual+'+ test_done
From: Junio C Hamano <hidden> Date: 2022-01-08 01:51:11
Patrick Steinhardt [off-list ref] writes:
When deleting loose refs, then we also have to delete the refs in the
packed backend. This is done by calling `refs_delete_refs()`, which
then uses the packed-backend's logic to delete refs. This doesn't allow
us to exercise any control over the reference transaction which is being
created in the packed backend, which is required in a subsequent commit.
Extract a new function `packed_refs_delete_refs()`, which hosts most of
the logic to delete refs except for creating the transaction itself.
Like this, we can easily create the transactionion in the files backend
ionion?
and thus exert more control over it.
Signed-off-by: Patrick Steinhardt <redacted>
---
The title might be technically correct, but I think the primary
point, the benefit this step brings to the system, is now we have a
helper function we can call in a transaction we create ourselves.
refs: extract packed_refs_delete_refs() to be used in our own transaction
or something along that line, perhaps? I dunno.
Without looking at the later patches, my guess is that we'd create a
single transaction for packed-refs backend and then issue multiple
delete requests in that single transaction?
Ah, no. We are already deleting multiple refs in one go, so that is
not what is happening here. Makes readers a bit curious. Hopefully
we will find it out in a later patch.
From: Junio C Hamano <hidden> Date: 2022-01-08 01:51:13
Patrick Steinhardt [off-list ref] writes:
quoted hunk
We do not currently have any flags when creating reference transactions,
but we'll add one to disable execution of the reference transaction hook
in some cases.
Allow passing flags to `ref_store_transaction_begin()` to prepare for
this change.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs.c | 8 +++++---
refs.h | 3 ++-
refs/files-backend.c | 10 +++++-----
refs/packed-backend.c | 2 +-
refs/refs-internal.h | 1 +
sequencer.c | 2 +-
6 files changed, 15 insertions(+), 11 deletions(-)
Ah, OK, the "later" came soon enough. It wasn't that we wanted to
cram multiple calls for "delete" in a single transaction. It was
that we wanted to tweak how each transaction works by passing a
flag word.
From: Junio C Hamano <hidden> Date: 2022-01-08 02:01:09
Patrick Steinhardt [off-list ref] writes:
When deleting refs from the loose-files refs backend, then we need to be
careful to also delete the same ref from the packed refs backend, if it
exists. If we don't, then deleting the loose ref would "uncover" the
packed ref. We thus always have to queue up deletions of refs for both
the loose and the packed refs backend. This is done in two separate
transactions, where the end result is that the reference-transaction
hook is executed twice for the deleted refs.
This behaviour is quite misleading: it's exposing implementation details
of how the files backend works to the user, in contrast to the logical
updates that we'd really want to expose via the hook. Worse yet, whether
the hook gets executed once or twice depends on how well-packed the
repository is: if the ref only exists as a loose ref, then we execute it
once, otherwise if it is also packed then we execute it twice.
If the ref only exists as a packed ref, what happens? ...
Fix this behaviour and don't execute the reference-transaction hook at
all when refs in the packed-refs backend if it's driven by the files
backend.
... We try to remove from the loose backend, which would say "nah,
it did not exist in my store". I am not sure if it should execute
the delete hook in such a case for the ref. But if it does not, not
running the hook in the ref transaction for packed backend driven by
the loose backend would mean nobody notifies the deletion of the
ref, no?
To me, it seems that the only case this strategy would work
correctly is when the files backend calls deletion hook for a
request to delete nonexistent ref, which by itself sounds like a
problem.
@@ -2775,7 +2776,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,*/if(!packed_transaction){packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,0,err);+refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3046,7 +3048,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,0,err);+packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' 'gitupdate-ref-drefs/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.+# deletion.cat>expect<<-EOF&&-prepared-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deletedprepared$POST_OID$ZERO_OIDrefs/heads/to-be-deletedcommitted-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted-committed$POST_OID$ZERO_OIDrefs/heads/to-be-deletedEOF
From: Eric Sunshine <hidden> Date: 2022-01-08 05:43:53
On Fri, Jan 7, 2022 at 3:09 PM Patrick Steinhardt [off-list ref] 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/
From: Patrick Steinhardt <hidden> Date: 2022-01-10 12:55:07
On Fri, Jan 07, 2022 at 05:31:04PM -0800, Junio C Hamano wrote:
Patrick Steinhardt [off-list ref] writes:
quoted
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
@@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' 'test_cmpexpecttarget-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.+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-refrefs/heads/unpacked-ref$POST_OID&&+gitpack-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_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+committed+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+EOF++test_cmpexpectactual+'++test_expect_success'deleting packed ref calls hook once''+# Create a reference and pack it.+gitupdate-refrefs/heads/to-be-deleted$POST_OID&&+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-ref-drefs/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_OIDrefs/heads/to-be-deleted+prepared+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+EOF++test_cmpexpectactual+'+ test_done
From: Patrick Steinhardt <hidden> Date: 2022-01-10 13:18:33
On Fri, Jan 07, 2022 at 06:01:04PM -0800, Junio C Hamano wrote:
Patrick Steinhardt [off-list ref] writes:
quoted
When deleting refs from the loose-files refs backend, then we need to be
careful to also delete the same ref from the packed refs backend, if it
exists. If we don't, then deleting the loose ref would "uncover" the
packed ref. We thus always have to queue up deletions of refs for both
the loose and the packed refs backend. This is done in two separate
transactions, where the end result is that the reference-transaction
hook is executed twice for the deleted refs.
This behaviour is quite misleading: it's exposing implementation details
of how the files backend works to the user, in contrast to the logical
updates that we'd really want to expose via the hook. Worse yet, whether
the hook gets executed once or twice depends on how well-packed the
repository is: if the ref only exists as a loose ref, then we execute it
once, otherwise if it is also packed then we execute it twice.
If the ref only exists as a packed ref, what happens? ...
quoted
Fix this behaviour and don't execute the reference-transaction hook at
all when refs in the packed-refs backend if it's driven by the files
backend.
... We try to remove from the loose backend, which would say "nah,
it did not exist in my store". I am not sure if it should execute
the delete hook in such a case for the ref. But if it does not, not
running the hook in the ref transaction for packed backend driven by
the loose backend would mean nobody notifies the deletion of the
ref, no?
This is shown in the test I've added, "deleting packed ref calls hook
once". We create a new reference and pack it such that it doesn't exist
as loose ref anymore, but only as a packed one. Updating that ref
would've caused us to execute the hook twice before, once via the
packed-backend and once via the loose-backend. With my fix we only
execute it once via the loose-backend, even if it doesn't currently know
it. This works because the loose-backend has to create a lock for the
nonexistent reference such that no concurrent call touches the same ref.
To me, it seems that the only case this strategy would work
correctly is when the files backend calls deletion hook for a
request to delete nonexistent ref, which by itself sounds like a
problem.
It does so only if the ref exists in either the loose or packed backend
though. If trying to update a ref which exists in neither of those, then
the reference transaction would fail with an "unable to resolve
reference" error in `lock_raw_ref()`.
So this should behave as expected: deleting a packed ref calls the hook
once, deleting a nonexistent ref fails and doesn't call the hook at all.
Patrick
@@ -2775,7 +2776,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,*/if(!packed_transaction){packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,0,err);+refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3046,7 +3048,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,0,err);+packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' 'gitupdate-ref-drefs/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.+# deletion.cat>expect<<-EOF&&-prepared-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deletedprepared$POST_OID$ZERO_OIDrefs/heads/to-be-deletedcommitted-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted-committed$POST_OID$ZERO_OIDrefs/heads/to-be-deletedEOF
From: Patrick Steinhardt <hidden> Date: 2022-01-13 06:11:32
Hi,
this is the third version of this patch series, which addresses an issue
where the reference-transaction hook is being invoked twice when
deleting refs both in the packed-refs and loose-refs file.
The following things changed in comparison to v2:
- Fixed some typos in commit messages.
- Improved the subject of the first patch to more clearly highlight
the purpose of it, not only say what the patch does.
- Fixed a missing declaration for `struct string_list`.
- Clarified why the last patch does the right thing even in case the
ref only exists in the packed-refs backend, and in case it doesn't
exist in either of the backends.
Thanks for your feedback!
Patrick
Patrick Steinhardt (6):
refs: extract packed_refs_delete_refs() to allow control of
transaction
refs: allow passing flags when beginning transactions
refs: allow skipping the reference-transaction hook
refs: demonstrate excessive execution of the reference-transaction
hook
refs: do not execute reference-transaction hook on packing refs
refs: skip hooks when deleting uncovered packed refs
refs.c | 11 +++++--
refs.h | 8 ++++-
refs/files-backend.c | 25 +++++++++++-----
refs/packed-backend.c | 30 ++++++++++++++-----
refs/packed-backend.h | 7 +++++
refs/refs-internal.h | 1 +
sequencer.c | 2 +-
t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++
8 files changed, 114 insertions(+), 20 deletions(-)
Range-diff against v2:
1: 0739f085b2 ! 1: abbc28822b refs: open-code deletion of packed refs
@@ Metadata
Author: Patrick Steinhardt [off-list ref]
## Commit message ##
- refs: open-code deletion of packed refs
+ refs: extract packed_refs_delete_refs() to allow control of transaction
When deleting loose refs, then we also have to delete the refs in the
packed backend. This is done by calling `refs_delete_refs()`, which
@@ Commit message
Extract a new function `packed_refs_delete_refs()`, which hosts most of
the logic to delete refs except for creating the transaction itself.
- Like this, we can easily create the transactionion in the files backend
+ Like this, we can easily create the transaction in the files backend
and thus exert more control over it.
Signed-off-by: Patrick Steinhardt [off-list ref]
@@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store
}
## refs/packed-backend.h ##
+@@
+
+ struct repository;
+ struct ref_transaction;
++struct string_list;
+
+ /*
+ * Support for storing references in a `packed-refs` file.
@@ refs/packed-backend.h: int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
void packed_refs_unlock(struct ref_store *ref_store);
int packed_refs_is_locked(struct ref_store *ref_store);
2: 629be01d50 = 2: 9dd172a757 refs: allow passing flags when beginning transactions
3: 550d89a323 = 3: be826bae3b refs: allow skipping the reference-transaction hook
4: b52e59cdac ! 4: 662a6e6244 refs: demonstrate excessive execution of the reference-transaction hook
@@ Metadata
## Commit message ##
refs: demonstrate excessive execution of the reference-transaction hook
- Add tests which demonstate which demonstrates that we're executing the
+ Add tests which demonstate 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.
5: 1539e9711f = 5: d83f309b9c refs: do not execute reference-transaction hook on packing refs
6: 0fbf68dbf4 ! 6: 279eadc41c refs: skip hooks when deleting uncovered packed refs
@@ Commit message
Fix this behaviour and don't execute the reference-transaction hook at
all when refs in the packed-refs backend if it's driven by the files
- backend.
+ backend. This works as expected even in case the refs to be deleted only
+ exist in the packed-refs backend because the loose-backend always queues
+ refs in its own transaction even if they don't exist such that they can
+ be locked for concurrent creation. And it also does the right thing in
+ case neither of the backends has the ref because that would cause the
+ transaction to fail completely.
Signed-off-by: Patrick Steinhardt [off-list ref]
--
2.34.1
From: Patrick Steinhardt <hidden> Date: 2022-01-13 06:11:33
When deleting loose refs, then we also have to delete the refs in the
packed backend. This is done by calling `refs_delete_refs()`, which
then uses the packed-backend's logic to delete refs. This doesn't allow
us to exercise any control over the reference transaction which is being
created in the packed backend, which is required in a subsequent commit.
Extract a new function `packed_refs_delete_refs()`, which hosts most of
the logic to delete refs except for creating the transaction itself.
Like this, we can easily create the transaction in the files backend
and thus exert more control over it.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 12 +++++++++---
refs/packed-backend.c | 28 +++++++++++++++++++++-------
refs/packed-backend.h | 7 +++++++
3 files changed, 37 insertions(+), 10 deletions(-)
@@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,staticintpacked_delete_refs(structref_store*ref_store,constchar*msg,structstring_list*refnames,unsignedintflags){-structpacked_ref_store*refs=-packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");structstrbuferr=STRBUF_INIT;structref_transaction*transaction;-structstring_list_item*item;intret;-(void)refs;/* We need the check above, but don't use the variable */-if(!refnames->nr)return0;
@@ -1544,6 +1539,27 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,if(!transaction)return-1;+ret=packed_refs_delete_refs(ref_store,transaction,+msg,refnames,flags);++ref_transaction_free(transaction);+returnret;+}++intpacked_refs_delete_refs(structref_store*ref_store,+structref_transaction*transaction,+constchar*msg,+structstring_list*refnames,+unsignedintflags)+{+structpacked_ref_store*refs=+packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");+structstrbuferr=STRBUF_INIT;+structstring_list_item*item;+intret;++(void)(refs);/* We need the check above, but don't use the variable */+for_each_string_list_item(item,refnames){if(ref_transaction_delete(transaction,item->string,NULL,flags,msg,&err)){
@@ -1554,7 +1570,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,}ret=ref_transaction_commit(transaction,&err);-if(ret){if(refnames->nr==1)error(_("could not delete reference %s: %s"),
@@ -1563,7 +1578,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,error(_("could not delete references: %s"),err.buf);}-ref_transaction_free(transaction);strbuf_release(&err);returnret;}
From: Patrick Steinhardt <hidden> Date: 2022-01-13 06:11:35
We do not currently have any flags when creating reference transactions,
but we'll add one to disable execution of the reference transaction hook
in some cases.
Allow passing flags to `ref_store_transaction_begin()` to prepare for
this change.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs.c | 8 +++++---
refs.h | 3 ++-
refs/files-backend.c | 10 +++++-----
refs/packed-backend.c | 2 +-
refs/refs-internal.h | 1 +
sequencer.c | 2 +-
6 files changed, 15 insertions(+), 11 deletions(-)
@@ -2773,7 +2773,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,*/if(!packed_transaction){packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,err);+refs->packed_ref_store,0,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3044,7 +3044,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,err);+packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,0,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
From: Patrick Steinhardt <hidden> Date: 2022-01-13 06:11:39
The reference-transaction hook is executing whenever we prepare, commit
or abort a reference transaction. While this is mostly intentional, in
case of the files backend we're leaking the implementation detail that
the store is in fact a composite store with one loose and one packed
backend to the caller. So while we want to execute the hook for all
logical updates, executing it for such implementation details is
unexpected.
Prepare for a fix by adding a new flag which allows to skip execution of
the hook.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs.c | 3 +++
refs.h | 5 +++++
2 files changed, 8 insertions(+)
From: Patrick Steinhardt <hidden> Date: 2022-01-13 06:11:43
Add tests which demonstate 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 <redacted>
---
t/t1416-ref-transaction-hooks.sh | 64 ++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
@@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' 'test_cmpexpecttarget-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.+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-refrefs/heads/unpacked-ref$POST_OID&&+gitpack-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_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+committed+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+EOF++test_cmpexpectactual+'++test_expect_success'deleting packed ref calls hook once''+# Create a reference and pack it.+gitupdate-refrefs/heads/to-be-deleted$POST_OID&&+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-ref-drefs/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_OIDrefs/heads/to-be-deleted+prepared+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+EOF++test_cmpexpectactual+'+ test_done
From: Patrick Steinhardt <hidden> Date: 2022-01-13 06:11:48
The reference-transaction hook is supposed to track logical changes to
references, but it currently also gets executed when packing refs in a
repository. This is unexpected and ultimately not all that useful:
packing refs is not supposed to result in any user-visible change to the
refs' state, and it ultimately is an implementation detail of how refs
stores work.
Fix this excessive execution of the hook when packing refs.
Reported-by: Waleed Khan <redacted>
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 6 ++++--
t/t1416-ref-transaction-hooks.sh | 11 +----------
2 files changed, 5 insertions(+), 12 deletions(-)
@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' 'gitpack-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.+# git-update-ref(1).cat>expect<<-EOF&&prepared$ZERO_OID$POST_OIDrefs/heads/unpacked-refcommitted$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-committed-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$POST_OID$ZERO_OIDrefs/heads/unpacked-ref-committed-$POST_OID$ZERO_OIDrefs/heads/unpacked-refEOFtest_cmpexpectactual
From: Patrick Steinhardt <hidden> Date: 2022-01-13 06:11:53
When deleting refs from the loose-files refs backend, then we need to be
careful to also delete the same ref from the packed refs backend, if it
exists. If we don't, then deleting the loose ref would "uncover" the
packed ref. We thus always have to queue up deletions of refs for both
the loose and the packed refs backend. This is done in two separate
transactions, where the end result is that the reference-transaction
hook is executed twice for the deleted refs.
This behaviour is quite misleading: it's exposing implementation details
of how the files backend works to the user, in contrast to the logical
updates that we'd really want to expose via the hook. Worse yet, whether
the hook gets executed once or twice depends on how well-packed the
repository is: if the ref only exists as a loose ref, then we execute it
once, otherwise if it is also packed then we execute it twice.
Fix this behaviour and don't execute the reference-transaction hook at
all when refs in the packed-refs backend if it's driven by the files
backend. This works as expected even in case the refs to be deleted only
exist in the packed-refs backend because the loose-backend always queues
refs in its own transaction even if they don't exist such that they can
be locked for concurrent creation. And it also does the right thing in
case neither of the backends has the ref because that would cause the
transaction to fail completely.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 9 ++++++---
t/t1416-ref-transaction-hooks.sh | 7 +------
2 files changed, 7 insertions(+), 9 deletions(-)
@@ -2775,7 +2776,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,*/if(!packed_transaction){packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,0,err);+refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3046,7 +3048,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,0,err);+packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' 'gitupdate-ref-drefs/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.+# deletion.cat>expect<<-EOF&&-prepared-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deletedprepared$POST_OID$ZERO_OIDrefs/heads/to-be-deletedcommitted-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted-committed$POST_OID$ZERO_OIDrefs/heads/to-be-deletedEOF
[[PGP Signed Part:Undecided]]
When deleting loose refs, then we also have to delete the refs in the
packed backend. This is done by calling `refs_delete_refs()`, which
then uses the packed-backend's logic to delete refs. This doesn't allow
us to exercise any control over the reference transaction which is being
created in the packed backend, which is required in a subsequent commit.
Extract a new function `packed_refs_delete_refs()`, which hosts most of
the logic to delete refs except for creating the transaction itself.
Like this, we can easily create the transaction in the files backend
and thus exert more control over it.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 12 +++++++++---
refs/packed-backend.c | 28 +++++++++++++++++++++-------
refs/packed-backend.h | 7 +++++++
3 files changed, 37 insertions(+), 10 deletions(-)
@@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,staticintpacked_delete_refs(structref_store*ref_store,constchar*msg,structstring_list*refnames,unsignedintflags){-structpacked_ref_store*refs=-packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");structstrbuferr=STRBUF_INIT;structref_transaction*transaction;-structstring_list_item*item;intret;-(void)refs;/* We need the check above, but don't use the variable */-if(!refnames->nr)return0;
@@ -1544,6 +1539,27 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,if(!transaction)return-1;+ret=packed_refs_delete_refs(ref_store,transaction,+msg,refnames,flags);++ref_transaction_free(transaction);+returnret;+}++intpacked_refs_delete_refs(structref_store*ref_store,+structref_transaction*transaction,+constchar*msg,+structstring_list*refnames,+unsignedintflags)+{+structpacked_ref_store*refs=+packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");+structstrbuferr=STRBUF_INIT;+structstring_list_item*item;+intret;++(void)(refs);/* We need the check above, but don't use the variable */+for_each_string_list_item(item,refnames){if(ref_transaction_delete(transaction,item->string,NULL,flags,msg,&err)){
I see you're just moving this code around, but FWIW we can just do this
(also in the pre-image):
int packed_refs_delete_refs(...)
{
[declare variables]
/* Assert ref store sanity */
packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs")
[...]
}
Not sure it's good to change it around just for this mostly-move, just a
note...
quoted hunk
@@ -1554,7 +1570,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg, } ret = ref_transaction_commit(transaction, &err);-
[[PGP Signed Part:Undecided]]
The reference-transaction hook is supposed to track logical changes to
references, but it currently also gets executed when packing refs in a
repository. This is unexpected and ultimately not all that useful:
packing refs is not supposed to result in any user-visible change to the
refs' state, and it ultimately is an implementation detail of how refs
stores work.
Fix this excessive execution of the hook when packing refs.
Reported-by: Waleed Khan <redacted>
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 6 ++++--
t/t1416-ref-transaction-hooks.sh | 11 +----------
2 files changed, 5 insertions(+), 12 deletions(-)
@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' 'gitpack-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.+# git-update-ref(1).cat>expect<<-EOF&&prepared$ZERO_OID$POST_OIDrefs/heads/unpacked-refcommitted$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-committed-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$POST_OID$ZERO_OIDrefs/heads/unpacked-ref-committed-$POST_OID$ZERO_OIDrefs/heads/unpacked-refEOFtest_cmpexpectactual
I wondered how we'd deal with cases where the loose ref != the
corresponding packed ref, but I can't think of ones where it won't be
invisible externally, i.e. we'll correctly update the packed-refs and
delete that loose ref as part of this transaction.
I do wonder if the docs also need updating, currently they say:
[The hook] executes whenever a reference transaction is prepared,
committed or aborted[...]
But now we'll explicitly exclude certain classes of
transactions. Perhaps we should expand:
"The hook does not cover symbolic references (but that may change in
the future)."
Into some list of types of changes we intentionally exclude, might
include in the future etc.
[[PGP Signed Part:Undecided]]
When deleting refs from the loose-files refs backend, then we need to be
careful to also delete the same ref from the packed refs backend, if it
exists. If we don't, then deleting the loose ref would "uncover" the
packed ref. We thus always have to queue up deletions of refs for both
the loose and the packed refs backend. This is done in two separate
transactions, where the end result is that the reference-transaction
hook is executed twice for the deleted refs.
But do we (which would be an issue before this series) delete the loose
and and then the packed one, thus racily exposing the stale ref to any
concurrent repository reader, or do we first update the packed ref to
the valu of the now-locked loose ref to avoid such a race?
[...]
Fix this behaviour and don't execute the reference-transaction hook at
all when refs in the packed-refs backend if it's driven by the files
backend. This works as expected even in case the refs to be deleted only
exist in the packed-refs backend because the loose-backend always queues
refs in its own transaction even if they don't exist such that they can
be locked for concurrent creation. And it also does the right thing in
case neither of the backends has the ref because that would cause the
transaction to fail completely.
I do wonder if the fundimental approach here is the right
one. I.e. changing the hook to only expose "real" updates, as opposed to
leaving it as a lower-level facility to listed in on any sort of ref
updates.
In such a scenario we could imagine adding a third parameter or
otherwise flag the update as "real" to the hook, so a dumber hook
consumer could ignore the more verbose inter-transactional chatter.
I say that because this change does the right thing for the use-case you
have in mind, but if you e.g. imagine a more gentle background-friendly
"gc" such a thing could be implemented by backing off as soon as it sees
an ongoing transaction being started.
With my ae35e16cd43 (reflog expire: don't lock reflogs using previously
seen OID, 2021-08-23) not getting that more chatty data should be be OK
for such a hypothetical hook.
But we might have more avoidable tripping over locks as the gc and ref
transaction race one another to lock various things in the repository.
Or maybe nobody cares in practice, just food for thought.
[[PGP Signed Part:Undecided]]
The reference-transaction hook is executing whenever we prepare, commit
or abort a reference transaction. While this is mostly intentional, in
case of the files backend we're leaking the implementation detail that
the store is in fact a composite store with one loose and one packed
backend to the caller. So while we want to execute the hook for all
logical updates, executing it for such implementation details is
unexpected.
Prepare for a fix by adding a new flag which allows to skip execution of
the hook.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs.c | 3 +++
refs.h | 5 +++++
2 files changed, 8 insertions(+)
This isn't needed in refs.h, so let's put it in refs-internal.h where
e.g. "enum ref_transaction_state" now lives:
diff --git a/refs.h b/refs.h
index d4056f9fe26..31f7bf96424 100644
--- a/refs.h
+++ b/refs.h
@@ -568,11 +568,6 @@ enum action_on_err {
UPDATE_REFS_QUIET_ON_ERR
};
-/*
- * Skip executing the reference-transaction hook.
- */
-#define REF_TRANSACTION_SKIP_HOOK (1 << 0)
-
/*
* Begin a reference transaction. The reference transaction must
* be freed by calling ref_transaction_free().
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index a0af63f162f..87da39243f7 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -201,6 +201,11 @@ enum ref_transaction_state {
REF_TRANSACTION_CLOSED = 2
};
+/*
+ * Skip executing the reference-transaction hook.
+ */
+#define REF_TRANSACTION_SKIP_HOOK (1 << 0)
+
/*
* Data structure for holding a reference transaction, which can
* consist of checks and updates to multiple references, carried out
A bit more odd is that this series ends up with a
ref_transaction_begin() that doesn't correspond to its ref_store_*()
parent, i.e. the others pass the ref store for you, but now we omit the
flags too.
I see why you did that, to avoid tweaking every existing
ref_transaction_begin() caller.
But isn't something like the below a better approach? We can introduce a
refs-internal.h-only flag enum, and then just have a new
ref_store_transaction_begin_no_hook() called from these new
files-backend.c users.
The diff on top is a bit verbose, but the exposed API is cleaner
(presumably no "public" user should be allowed to skip the hook), and
the overall diff if this is squashed in is smaller.
@@ -2775,9 +2772,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,*packed-refsifitexiststhere.*/if(!packed_transaction){-packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,-REF_TRANSACTION_SKIP_HOOK,err);+packed_transaction=ref_store_transaction_begin_no_hook(+refs->packed_ref_store,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3048,8 +3044,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,-REF_TRANSACTION_SKIP_HOOK,err);+packed_transaction=ref_store_transaction_begin_no_hook(refs->packed_ref_store,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
From: Patrick Steinhardt <hidden> Date: 2022-01-17 07:36:31
On Thu, Jan 13, 2022 at 01:43:18PM +0100, Ævar Arnfjörð Bjarmason wrote:
On Thu, Jan 13 2022, Patrick Steinhardt wrote:
quoted
[[PGP Signed Part:Undecided]]
When deleting loose refs, then we also have to delete the refs in the
packed backend. This is done by calling `refs_delete_refs()`, which
then uses the packed-backend's logic to delete refs. This doesn't allow
us to exercise any control over the reference transaction which is being
created in the packed backend, which is required in a subsequent commit.
Extract a new function `packed_refs_delete_refs()`, which hosts most of
the logic to delete refs except for creating the transaction itself.
Like this, we can easily create the transaction in the files backend
and thus exert more control over it.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 12 +++++++++---
refs/packed-backend.c | 28 +++++++++++++++++++++-------
refs/packed-backend.h | 7 +++++++
3 files changed, 37 insertions(+), 10 deletions(-)
@@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,staticintpacked_delete_refs(structref_store*ref_store,constchar*msg,structstring_list*refnames,unsignedintflags){-structpacked_ref_store*refs=-packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");structstrbuferr=STRBUF_INIT;structref_transaction*transaction;-structstring_list_item*item;intret;-(void)refs;/* We need the check above, but don't use the variable */-if(!refnames->nr)return0;
@@ -1544,6 +1539,27 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,if(!transaction)return-1;+ret=packed_refs_delete_refs(ref_store,transaction,+msg,refnames,flags);++ref_transaction_free(transaction);+returnret;+}++intpacked_refs_delete_refs(structref_store*ref_store,+structref_transaction*transaction,+constchar*msg,+structstring_list*refnames,+unsignedintflags)+{+structpacked_ref_store*refs=+packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");+structstrbuferr=STRBUF_INIT;+structstring_list_item*item;+intret;++(void)(refs);/* We need the check above, but don't use the variable */+for_each_string_list_item(item,refnames){if(ref_transaction_delete(transaction,item->string,NULL,flags,msg,&err)){
I see you're just moving this code around, but FWIW we can just do this
(also in the pre-image):
int packed_refs_delete_refs(...)
{
[declare variables]
/* Assert ref store sanity */
packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs")
[...]
}
Not sure it's good to change it around just for this mostly-move, just a
note...
I think this change is trivial enough to make while at it.
Patrick
quoted
@@ -1554,7 +1570,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg, } ret = ref_transaction_commit(transaction, &err);-
From: Patrick Steinhardt <hidden> Date: 2022-01-17 07:44:18
On Thu, Jan 13, 2022 at 02:00:10PM +0100, Ævar Arnfjörð Bjarmason wrote:
On Thu, Jan 13 2022, Patrick Steinhardt wrote:
quoted
[[PGP Signed Part:Undecided]]
The reference-transaction hook is supposed to track logical changes to
references, but it currently also gets executed when packing refs in a
repository. This is unexpected and ultimately not all that useful:
packing refs is not supposed to result in any user-visible change to the
refs' state, and it ultimately is an implementation detail of how refs
stores work.
Fix this excessive execution of the hook when packing refs.
Reported-by: Waleed Khan <redacted>
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 6 ++++--
t/t1416-ref-transaction-hooks.sh | 11 +----------
2 files changed, 5 insertions(+), 12 deletions(-)
@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' 'gitpack-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.+# git-update-ref(1).cat>expect<<-EOF&&prepared$ZERO_OID$POST_OIDrefs/heads/unpacked-refcommitted$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-committed-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$POST_OID$ZERO_OIDrefs/heads/unpacked-ref-committed-$POST_OID$ZERO_OIDrefs/heads/unpacked-refEOFtest_cmpexpectactual
I wondered how we'd deal with cases where the loose ref != the
corresponding packed ref, but I can't think of ones where it won't be
invisible externally, i.e. we'll correctly update the packed-refs and
delete that loose ref as part of this transaction.
With the previous code we'd see two hook executions with different old
OIDs. Given that we only care about logical updates though the user'd
only want to see the one which deletes the user-visible OID, which is
what's stored in the loose ref. And with the fixes in this series that's
the hook invocation we retain.
I do wonder if the docs also need updating, currently they say:
[The hook] executes whenever a reference transaction is prepared,
committed or aborted[...]
But now we'll explicitly exclude certain classes of
transactions. Perhaps we should expand:
"The hook does not cover symbolic references (but that may change in
the future)."
Into some list of types of changes we intentionally exclude, might
include in the future etc.
Well, from the user's perspective we do execute the hook whenever we
drive a reference transaction: all modifications to the files backend
are still visible to the hook after the changes in this series. The
issue is that with the files backend being a combination of two
backends, we essentially saw a subset of refs executing the hook twice,
which really is an implementation detail.
Patrick
From: Patrick Steinhardt <hidden> Date: 2022-01-17 07:56:38
On Thu, Jan 13, 2022 at 02:04:38PM +0100, Ævar Arnfjörð Bjarmason wrote:
On Thu, Jan 13 2022, Patrick Steinhardt wrote:
quoted
[[PGP Signed Part:Undecided]]
When deleting refs from the loose-files refs backend, then we need to be
careful to also delete the same ref from the packed refs backend, if it
exists. If we don't, then deleting the loose ref would "uncover" the
packed ref. We thus always have to queue up deletions of refs for both
the loose and the packed refs backend. This is done in two separate
transactions, where the end result is that the reference-transaction
hook is executed twice for the deleted refs.
But do we (which would be an issue before this series) delete the loose
and and then the packed one, thus racily exposing the stale ref to any
concurrent repository reader, or do we first update the packed ref to
the valu of the now-locked loose ref to avoid such a race?
We first commit the packed-refs file so that the stale ref is not
exposed.
quoted
[...]
Fix this behaviour and don't execute the reference-transaction hook at
all when refs in the packed-refs backend if it's driven by the files
backend. This works as expected even in case the refs to be deleted only
exist in the packed-refs backend because the loose-backend always queues
refs in its own transaction even if they don't exist such that they can
be locked for concurrent creation. And it also does the right thing in
case neither of the backends has the ref because that would cause the
transaction to fail completely.
I do wonder if the fundimental approach here is the right
one. I.e. changing the hook to only expose "real" updates, as opposed to
leaving it as a lower-level facility to listed in on any sort of ref
updates.
In such a scenario we could imagine adding a third parameter or
otherwise flag the update as "real" to the hook, so a dumber hook
consumer could ignore the more verbose inter-transactional chatter.
I say that because this change does the right thing for the use-case you
have in mind, but if you e.g. imagine a more gentle background-friendly
"gc" such a thing could be implemented by backing off as soon as it sees
an ongoing transaction being started.
I've mostly been acting on the original report by Waleed. And I tend to
agree with his report given that we also got a workaround at GitLab
which filters out reference transactions which only consist of force
deletions because they're likely to be pruning refs in the packed
backend which are about to be uncovered. The result is that execution of
the reftx hook is dependent on how well-packed a repository's refs are:
when refs are packed we execute the hook twice, whereas we execute it
once when it's not well-packed. This is surprising behaviour, even
though one can definitely argue that it's just working as intended.
I think ultimately the question boils down to whether we want to treat
the files backend as a single compound backend and whether the reftx
hook should treat it like that. If we treat it as a single backend, then
we shouldn't report a change in refs when pruning about-to-be-uncovered
refs given that it wouldn't have been visible, but it's only internal
cleanup. And neither should we report ref changes when repacking refs
into a single file given that from the backend's perspective nothing is
about to change.
Patrick
With my ae35e16cd43 (reflog expire: don't lock reflogs using previously
seen OID, 2021-08-23) not getting that more chatty data should be be OK
for such a hypothetical hook.
But we might have more avoidable tripping over locks as the gc and ref
transaction race one another to lock various things in the repository.
Or maybe nobody cares in practice, just food for thought.
From: Patrick Steinhardt <hidden> Date: 2022-01-17 08:04:12
On Thu, Jan 13, 2022 at 02:34:41PM +0100, Ævar Arnfjörð Bjarmason wrote:
On Thu, Jan 13 2022, Patrick Steinhardt wrote:
quoted
[[PGP Signed Part:Undecided]]
The reference-transaction hook is executing whenever we prepare, commit
or abort a reference transaction. While this is mostly intentional, in
case of the files backend we're leaking the implementation detail that
the store is in fact a composite store with one loose and one packed
backend to the caller. So while we want to execute the hook for all
logical updates, executing it for such implementation details is
unexpected.
Prepare for a fix by adding a new flag which allows to skip execution of
the hook.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs.c | 3 +++
refs.h | 5 +++++
2 files changed, 8 insertions(+)
This isn't needed in refs.h, so let's put it in refs-internal.h where
e.g. "enum ref_transaction_state" now lives:
On the other hand we got the flags parameter in the function just two
lines later. A reader would thus wonder "which flags?!" if we didn't
carry the definition of flags nearby and would have to go search the
codebase for the set of supported flags.
diff --git a/refs.h b/refs.h
index d4056f9fe26..31f7bf96424 100644
--- a/refs.h
+++ b/refs.h
@@ -568,11 +568,6 @@ enum action_on_err {
UPDATE_REFS_QUIET_ON_ERR
};
-/*
- * Skip executing the reference-transaction hook.
- */
-#define REF_TRANSACTION_SKIP_HOOK (1 << 0)
-
/*
* Begin a reference transaction. The reference transaction must
* be freed by calling ref_transaction_free().
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index a0af63f162f..87da39243f7 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -201,6 +201,11 @@ enum ref_transaction_state {
REF_TRANSACTION_CLOSED = 2
};
+/*
+ * Skip executing the reference-transaction hook.
+ */
+#define REF_TRANSACTION_SKIP_HOOK (1 << 0)
+
/*
* Data structure for holding a reference transaction, which can
* consist of checks and updates to multiple references, carried out
A bit more odd is that this series ends up with a
ref_transaction_begin() that doesn't correspond to its ref_store_*()
parent, i.e. the others pass the ref store for you, but now we omit the
flags too.
I see why you did that, to avoid tweaking every existing
ref_transaction_begin() caller.
But isn't something like the below a better approach? We can introduce a
refs-internal.h-only flag enum, and then just have a new
ref_store_transaction_begin_no_hook() called from these new
files-backend.c users.
The diff on top is a bit verbose, but the exposed API is cleaner
(presumably no "public" user should be allowed to skip the hook), and
the overall diff if this is squashed in is smaller.
While the diff does look sensible it makes me wonder how maintainable it
is in the future. If we ever were to accept another flag then we might
have to duplicate your `ref_store_transaction_begin_no_hook()` to also
carry flags, or add `ref_store_transaciton_begin_another_option()` to
stay consistent.
So for now I tend to prefer the current version given that it feels more
extensible going forward. But I'm happy to change it if others agree
with you.
Patrick
@@ -2775,9 +2772,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,*packed-refsifitexiststhere.*/if(!packed_transaction){-packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,-REF_TRANSACTION_SKIP_HOOK,err);+packed_transaction=ref_store_transaction_begin_no_hook(+refs->packed_ref_store,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3048,8 +3044,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,-REF_TRANSACTION_SKIP_HOOK,err);+packed_transaction=ref_store_transaction_begin_no_hook(refs->packed_ref_store,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
From: Patrick Steinhardt <hidden> Date: 2022-01-17 08:12:33
Hi,
this is the fourth version of this patch series, which addresses an
issue where the reference-transaction hook is being invoked twice when
deleting refs both in the packed-refs and loose-refs file.
The following things changed in comparison to v3:
- Fixed a memory leak in `files_delete_refs()`.
- Refactored the `packed_downcast()` invocation such that we don't
have to mark its unused returned variable as used.
- Removed a spurious whitespace change.
Patrick
Patrick Steinhardt (6):
refs: extract packed_refs_delete_refs() to allow control of
transaction
refs: allow passing flags when beginning transactions
refs: allow skipping the reference-transaction hook
refs: demonstrate excessive execution of the reference-transaction
hook
refs: do not execute reference-transaction hook on packing refs
refs: skip hooks when deleting uncovered packed refs
refs.c | 11 +++++--
refs.h | 8 ++++-
refs/files-backend.c | 26 ++++++++++++-----
refs/packed-backend.c | 28 +++++++++++++-----
refs/packed-backend.h | 7 +++++
refs/refs-internal.h | 1 +
sequencer.c | 2 +-
t/t1416-ref-transaction-hooks.sh | 50 ++++++++++++++++++++++++++++++++
8 files changed, 114 insertions(+), 19 deletions(-)
Range-diff against v3:
1: abbc28822b ! 1: 14775046e1 refs: extract packed_refs_delete_refs() to allow control of transaction
@@ refs/files-backend.c: static int files_delete_refs(struct ref_store *ref_store,
packed_refs_unlock(refs->packed_ref_store);
+@@ refs/files-backend.c: static int files_delete_refs(struct ref_store *ref_store, const char *msg,
+ result |= error(_("could not remove reference %s"), refname);
+ }
+
++ ref_transaction_free(transaction);
+ strbuf_release(&err);
+ return result;
+
@@ refs/files-backend.c: static int files_delete_refs(struct ref_store *ref_store, const char *msg,
else
error(_("could not delete references: %s"), err.buf);
@@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store
+ struct string_list *refnames,
+ unsigned int flags)
+{
-+ struct packed_ref_store *refs =
-+ packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
+ struct strbuf err = STRBUF_INIT;
+ struct string_list_item *item;
+ int ret;
+
-+ (void)(refs); /* We need the check above, but don't use the variable */
++ /* Assert that the ref store refers to a packed backend. */
++ packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
+
for_each_string_list_item(item, refnames) {
if (ref_transaction_delete(transaction, item->string, NULL,
flags, msg, &err)) {
-@@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
- }
-
- ret = ref_transaction_commit(transaction, &err);
--
- if (ret) {
- if (refnames->nr == 1)
- error(_("could not delete reference %s: %s"),
@@ refs/packed-backend.c: static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
error(_("could not delete references: %s"), err.buf);
}
2: 9dd172a757 = 2: d4ac24c8b8 refs: allow passing flags when beginning transactions
3: be826bae3b = 3: f4a07fe9a8 refs: allow skipping the reference-transaction hook
4: 662a6e6244 = 4: a8981baef7 refs: demonstrate excessive execution of the reference-transaction hook
5: d83f309b9c = 5: 23c344854e refs: do not execute reference-transaction hook on packing refs
6: 279eadc41c = 6: d6c7d765af refs: skip hooks when deleting uncovered packed refs
--
2.34.1
From: Patrick Steinhardt <hidden> Date: 2022-01-17 08:12:36
When deleting loose refs, then we also have to delete the refs in the
packed backend. This is done by calling `refs_delete_refs()`, which
then uses the packed-backend's logic to delete refs. This doesn't allow
us to exercise any control over the reference transaction which is being
created in the packed backend, which is required in a subsequent commit.
Extract a new function `packed_refs_delete_refs()`, which hosts most of
the logic to delete refs except for creating the transaction itself.
Like this, we can easily create the transaction in the files backend
and thus exert more control over it.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 13 ++++++++++---
refs/packed-backend.c | 26 ++++++++++++++++++++------
refs/packed-backend.h | 7 +++++++
3 files changed, 37 insertions(+), 9 deletions(-)
@@ -1522,15 +1522,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,staticintpacked_delete_refs(structref_store*ref_store,constchar*msg,structstring_list*refnames,unsignedintflags){-structpacked_ref_store*refs=-packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");structstrbuferr=STRBUF_INIT;structref_transaction*transaction;-structstring_list_item*item;intret;-(void)refs;/* We need the check above, but don't use the variable */-if(!refnames->nr)return0;
@@ -1544,6 +1539,26 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,if(!transaction)return-1;+ret=packed_refs_delete_refs(ref_store,transaction,+msg,refnames,flags);++ref_transaction_free(transaction);+returnret;+}++intpacked_refs_delete_refs(structref_store*ref_store,+structref_transaction*transaction,+constchar*msg,+structstring_list*refnames,+unsignedintflags)+{+structstrbuferr=STRBUF_INIT;+structstring_list_item*item;+intret;++/* Assert that the ref store refers to a packed backend. */+packed_downcast(ref_store,REF_STORE_WRITE,"delete_refs");+for_each_string_list_item(item,refnames){if(ref_transaction_delete(transaction,item->string,NULL,flags,msg,&err)){
@@ -1563,7 +1578,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,error(_("could not delete references: %s"),err.buf);}-ref_transaction_free(transaction);strbuf_release(&err);returnret;}
From: Patrick Steinhardt <hidden> Date: 2022-01-17 08:12:40
We do not currently have any flags when creating reference transactions,
but we'll add one to disable execution of the reference transaction hook
in some cases.
Allow passing flags to `ref_store_transaction_begin()` to prepare for
this change.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs.c | 8 +++++---
refs.h | 3 ++-
refs/files-backend.c | 10 +++++-----
refs/packed-backend.c | 2 +-
refs/refs-internal.h | 1 +
sequencer.c | 2 +-
6 files changed, 15 insertions(+), 11 deletions(-)
@@ -2774,7 +2774,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,*/if(!packed_transaction){packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,err);+refs->packed_ref_store,0,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3045,7 +3045,7 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,err);+packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,0,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
From: Patrick Steinhardt <hidden> Date: 2022-01-17 08:12:46
The reference-transaction hook is executing whenever we prepare, commit
or abort a reference transaction. While this is mostly intentional, in
case of the files backend we're leaking the implementation detail that
the store is in fact a composite store with one loose and one packed
backend to the caller. So while we want to execute the hook for all
logical updates, executing it for such implementation details is
unexpected.
Prepare for a fix by adding a new flag which allows to skip execution of
the hook.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs.c | 3 +++
refs.h | 5 +++++
2 files changed, 8 insertions(+)
From: Patrick Steinhardt <hidden> Date: 2022-01-17 08:12:49
Add tests which demonstate 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 <redacted>
---
t/t1416-ref-transaction-hooks.sh | 64 ++++++++++++++++++++++++++++++++
1 file changed, 64 insertions(+)
@@ -136,4 +136,68 @@ test_expect_success 'interleaving hook calls succeed' 'test_cmpexpecttarget-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.+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-refrefs/heads/unpacked-ref$POST_OID&&+gitpack-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_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+committed+$ZERO_OID$POST_OIDrefs/heads/unpacked-ref+prepared+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+committed+$POST_OID$ZERO_OIDrefs/heads/unpacked-ref+EOF++test_cmpexpectactual+'++test_expect_success'deleting packed ref calls hook once''+# Create a reference and pack it.+gitupdate-refrefs/heads/to-be-deleted$POST_OID&&+gitpack-refs--all&&++write_script.git/hooks/reference-transaction<<-\EOF&&+echo"$@">>actual+cat>>actual+EOF+rm-factual&&++gitupdate-ref-drefs/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_OIDrefs/heads/to-be-deleted+prepared+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted+committed+$POST_OID$ZERO_OIDrefs/heads/to-be-deleted+EOF++test_cmpexpectactual+'+ test_done
From: Patrick Steinhardt <hidden> Date: 2022-01-17 08:12:53
The reference-transaction hook is supposed to track logical changes to
references, but it currently also gets executed when packing refs in a
repository. This is unexpected and ultimately not all that useful:
packing refs is not supposed to result in any user-visible change to the
refs' state, and it ultimately is an implementation detail of how refs
stores work.
Fix this excessive execution of the hook when packing refs.
Reported-by: Waleed Khan <redacted>
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 6 ++++--
t/t1416-ref-transaction-hooks.sh | 11 +----------
2 files changed, 5 insertions(+), 12 deletions(-)
@@ -150,21 +150,12 @@ test_expect_success 'hook does not get called on packing refs' 'gitpack-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.+# git-update-ref(1).cat>expect<<-EOF&&prepared$ZERO_OID$POST_OIDrefs/heads/unpacked-refcommitted$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-committed-$ZERO_OID$POST_OIDrefs/heads/unpacked-ref-prepared-$POST_OID$ZERO_OIDrefs/heads/unpacked-ref-committed-$POST_OID$ZERO_OIDrefs/heads/unpacked-refEOFtest_cmpexpectactual
From: Patrick Steinhardt <hidden> Date: 2022-01-17 08:12:57
When deleting refs from the loose-files refs backend, then we need to be
careful to also delete the same ref from the packed refs backend, if it
exists. If we don't, then deleting the loose ref would "uncover" the
packed ref. We thus always have to queue up deletions of refs for both
the loose and the packed refs backend. This is done in two separate
transactions, where the end result is that the reference-transaction
hook is executed twice for the deleted refs.
This behaviour is quite misleading: it's exposing implementation details
of how the files backend works to the user, in contrast to the logical
updates that we'd really want to expose via the hook. Worse yet, whether
the hook gets executed once or twice depends on how well-packed the
repository is: if the ref only exists as a loose ref, then we execute it
once, otherwise if it is also packed then we execute it twice.
Fix this behaviour and don't execute the reference-transaction hook at
all when refs in the packed-refs backend if it's driven by the files
backend. This works as expected even in case the refs to be deleted only
exist in the packed-refs backend because the loose-backend always queues
refs in its own transaction even if they don't exist such that they can
be locked for concurrent creation. And it also does the right thing in
case neither of the backends has the ref because that would cause the
transaction to fail completely.
Signed-off-by: Patrick Steinhardt <redacted>
---
refs/files-backend.c | 9 ++++++---
t/t1416-ref-transaction-hooks.sh | 7 +------
2 files changed, 7 insertions(+), 9 deletions(-)
@@ -2776,7 +2777,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,*/if(!packed_transaction){packed_transaction=ref_store_transaction_begin(-refs->packed_ref_store,0,err);+refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -3047,7 +3049,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,&affected_refnames))BUG("initial ref transaction called with existing refs");-packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,0,err);+packed_transaction=ref_store_transaction_begin(refs->packed_ref_store,+REF_TRANSACTION_SKIP_HOOK,err);if(!packed_transaction){ret=TRANSACTION_GENERIC_ERROR;gotocleanup;
@@ -175,16 +175,11 @@ test_expect_success 'deleting packed ref calls hook once' 'gitupdate-ref-drefs/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.+# deletion.cat>expect<<-EOF&&-prepared-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deletedprepared$POST_OID$ZERO_OIDrefs/heads/to-be-deletedcommitted-$ZERO_OID$ZERO_OIDrefs/heads/to-be-deleted-committed$POST_OID$ZERO_OIDrefs/heads/to-be-deletedEOF