kernel
/
git
mirror of https://git.kernel.org/pub/scm/git/git.git
1
0
Fork 0
Code Releases Activity

Merge branch 'ps/avoid-unnecessary-hook-invocation-with-packed-refs' 

Because a deletion of ref would need to remove it from both the
loose ref store and the packed ref store, a delete-ref operation
that logically removes one ref may end up invoking ref-transaction
hook twice, which has been corrected.

* ps/avoid-unnecessary-hook-invocation-with-packed-refs:
  refs: skip hooks when deleting uncovered packed refs
  refs: do not execute reference-transaction hook on packing refs
  refs: demonstrate excessive execution of the reference-transaction hook
  refs: allow skipping the reference-transaction hook
  refs: allow passing flags when beginning transactions
  refs: extract packed_refs_delete_refs() to allow control of transaction
maint
Junio C Hamano 2022-02-18 13:53:27 -08:00
parent bcd020f88e 2ed1b64ebd
commit 991b4d47f0
8 changed files with 114 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
refs.c
View File

@ -793,7 +793,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;

transaction = ref_store_transaction_begin(refs, &err);
transaction = ref_store_transaction_begin(refs, 0, &err);
if (!transaction ||
ref_transaction_delete(transaction, refname, old_oid,
flags, msg, &err) ||
@ -998,6 +998,7 @@ int read_ref_at(struct ref_store *refs, const char *refname,
}

struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
unsigned int flags,
struct strbuf *err)
{
struct ref_transaction *tr;
@ -1005,12 +1006,13 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,

CALLOC_ARRAY(tr, 1);
tr->ref_store = refs;
tr->flags = flags;
return tr;
}

struct ref_transaction *ref_transaction_begin(struct strbuf *err)
{
return ref_store_transaction_begin(get_main_ref_store(the_repository), err);
return ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err);
}

void ref_transaction_free(struct ref_transaction *transaction)
@ -1149,7 +1151,7 @@ int refs_update_ref(struct ref_store *refs, const char *msg,
struct strbuf err = STRBUF_INIT;
int ret = 0;

t = ref_store_transaction_begin(refs, &err);
t = ref_store_transaction_begin(refs, 0, &err);
if (!t ||
ref_transaction_update(t, refname, new_oid, old_oid, flags, msg,
&err) ||
@ -2065,6 +2067,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,
const char *hook;
int ret = 0, i;

if (transaction->flags & REF_TRANSACTION_SKIP_HOOK)
return 0;

hook = find_hook("reference-transaction");
if (!hook)
return ret;

8
refs.h
View File

@ -226,7 +226,7 @@ char *repo_default_branch_name(struct repository *r, int quiet);
* struct strbuf err = STRBUF_INIT;
* int ret = 0;
*
* transaction = ref_store_transaction_begin(refs, &err);
* transaction = ref_store_transaction_begin(refs, 0, &err);
* if (!transaction ||
* ref_transaction_update(...) ||
* ref_transaction_create(...) ||
@ -563,11 +563,17 @@ 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().
*/
struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
unsigned int flags,
struct strbuf *err);
struct ref_transaction *ref_transaction_begin(struct strbuf *err);


26
refs/files-backend.c
View File

@ -1114,7 +1114,8 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
if (check_refname_format(r->name, 0))
return;

transaction = ref_store_transaction_begin(&refs->base, &err);
transaction = ref_store_transaction_begin(&refs->base,
REF_TRANSACTION_SKIP_HOOK, &err);
if (!transaction)
goto cleanup;
ref_transaction_add_update(
@ -1185,7 +1186,8 @@ static int files_pack_refs(struct ref_store *ref_store, unsigned int flags)
struct strbuf err = STRBUF_INIT;
struct ref_transaction *transaction;

transaction = ref_store_transaction_begin(refs->packed_ref_store, &err);
transaction = ref_store_transaction_begin(refs->packed_ref_store,
REF_TRANSACTION_SKIP_HOOK, &err);
if (!transaction)
return -1;

@ -1242,6 +1244,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
{
struct files_ref_store *refs =
files_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
struct ref_transaction *transaction = NULL;
struct strbuf err = STRBUF_INIT;
int i, result = 0;

@ -1251,10 +1254,15 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
if (packed_refs_lock(refs->packed_ref_store, 0, &err))
goto error;

if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {
packed_refs_unlock(refs->packed_ref_store);
transaction = ref_store_transaction_begin(refs->packed_ref_store,
REF_TRANSACTION_SKIP_HOOK, &err);
if (!transaction)
goto error;

result = packed_refs_delete_refs(refs->packed_ref_store,
transaction, msg, refnames, flags);
if (result)
goto error;
}

packed_refs_unlock(refs->packed_ref_store);

@ -1265,6 +1273,7 @@ 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;

@ -1281,6 +1290,7 @@ error:
else
error(_("could not delete references: %s"), err.buf);

ref_transaction_free(transaction);
strbuf_release(&err);
return -1;
}
@ -2751,7 +2761,8 @@ 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,
REF_TRANSACTION_SKIP_HOOK, err);
if (!packed_transaction) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
@ -3022,7 +3033,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, err);
packed_transaction = ref_store_transaction_begin(refs->packed_ref_store,
REF_TRANSACTION_SKIP_HOOK, err);
if (!packed_transaction) {
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;

28
refs/packed-backend.c
View File

@ -1521,15 +1521,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,
static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
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 ref_transaction *transaction;
struct string_list_item *item;
int ret;

(void)refs; /* We need the check above, but don't use the variable */

if (!refnames->nr)
return 0;

@ -1539,10 +1534,30 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
* updates into a single transaction.
*/

transaction = ref_store_transaction_begin(ref_store, &err);
transaction = ref_store_transaction_begin(ref_store, 0, &err);
if (!transaction)
return -1;

ret = packed_refs_delete_refs(ref_store, transaction,
msg, refnames, flags);

ref_transaction_free(transaction);
return ret;
}

int packed_refs_delete_refs(struct ref_store *ref_store,
struct ref_transaction *transaction,
const char *msg,
struct string_list *refnames,
unsigned int flags)
{
struct strbuf err = STRBUF_INIT;
struct string_list_item *item;
int ret;

/* 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)) {
@ -1562,7 +1577,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);
return ret;
}

7
refs/packed-backend.h
View File

@ -3,6 +3,7 @@

struct repository;
struct ref_transaction;
struct string_list;

/*
* Support for storing references in a `packed-refs` file.
@ -27,6 +28,12 @@ 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);

int packed_refs_delete_refs(struct ref_store *ref_store,
struct ref_transaction *transaction,
const char *msg,
struct string_list *refnames,
unsigned int flags);

/*
* Return true if `transaction` really needs to be carried out against
* the specified packed_ref_store, or false if it can be skipped

1
refs/refs-internal.h
View File

@ -213,6 +213,7 @@ struct ref_transaction {
size_t nr;
enum ref_transaction_state state;
void *backend_data;
unsigned int flags;
};

/*

2
sequencer.c
View File

@ -3584,7 +3584,7 @@ static int do_label(struct repository *r, const char *name, int len)
strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
strbuf_addf(&msg, "rebase (label) '%.*s'", len, name);

transaction = ref_store_transaction_begin(refs, &err);
transaction = ref_store_transaction_begin(refs, 0, &err);
if (!transaction) {
error("%s", err.buf);
ret = -1;

50
t/t1416-ref-transaction-hooks.sh
View File

@ -136,4 +136,54 @@ 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).
cat >expect <<-EOF &&
prepared
$ZERO_OID $POST_OID refs/heads/unpacked-ref
committed
$ZERO_OID $POST_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.
cat >expect <<-EOF &&
prepared
$POST_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