Skip to content

Commit 991b4d4

Browse files
gitstergitster
committed
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
2 parents bcd020f + 2ed1b64 commit 991b4d4

File tree

8 files changed

+114
-19
lines changed

8 files changed

+114
-19
lines changed

‎refs.c‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -793,7 +793,7 @@ int refs_delete_ref(struct ref_store *refs, const char *msg,
793793
struct ref_transaction *transaction;
794794
struct strbuf err = STRBUF_INIT;
795795

796-
transaction = ref_store_transaction_begin(refs, &err);
796+
transaction = ref_store_transaction_begin(refs, 0, &err);
797797
if (!transaction ||
798798
ref_transaction_delete(transaction, refname, old_oid,
799799
flags, msg, &err) ||
@@ -998,19 +998,21 @@ int read_ref_at(struct ref_store *refs, const char *refname,
998998
}
999999

10001000
struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
1001+
unsigned int flags,
10011002
struct strbuf *err)
10021003
{
10031004
struct ref_transaction *tr;
10041005
assert(err);
10051006

10061007
CALLOC_ARRAY(tr, 1);
10071008
tr->ref_store = refs;
1009+
tr->flags = flags;
10081010
return tr;
10091011
}
10101012

10111013
struct ref_transaction *ref_transaction_begin(struct strbuf *err)
10121014
{
1013-
return ref_store_transaction_begin(get_main_ref_store(the_repository), err);
1015+
return ref_store_transaction_begin(get_main_ref_store(the_repository), 0, err);
10141016
}
10151017

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

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

2070+
if (transaction->flags & REF_TRANSACTION_SKIP_HOOK)
2071+
return 0;
2072+
20682073
hook = find_hook("reference-transaction");
20692074
if (!hook)
20702075
return ret;

‎refs.h‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ char *repo_default_branch_name(struct repository *r, int quiet);
226226
* struct strbuf err = STRBUF_INIT;
227227
* int ret = 0;
228228
*
229-
* transaction = ref_store_transaction_begin(refs, &err);
229+
* transaction = ref_store_transaction_begin(refs, 0, &err);
230230
* if (!transaction ||
231231
* ref_transaction_update(...) ||
232232
* ref_transaction_create(...) ||
@@ -563,11 +563,17 @@ enum action_on_err {
563563
UPDATE_REFS_QUIET_ON_ERR
564564
};
565565

566+
/*
567+
* Skip executing the reference-transaction hook.
568+
*/
569+
#define REF_TRANSACTION_SKIP_HOOK (1 << 0)
570+
566571
/*
567572
* Begin a reference transaction. The reference transaction must
568573
* be freed by calling ref_transaction_free().
569574
*/
570575
struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
576+
unsigned int flags,
571577
struct strbuf *err);
572578
struct ref_transaction *ref_transaction_begin(struct strbuf *err);
573579

‎refs/files-backend.c‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1114,7 +1114,8 @@ static void prune_ref(struct files_ref_store *refs, struct ref_to_prune *r)
11141114
if (check_refname_format(r->name, 0))
11151115
return;
11161116

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

1188-
transaction = ref_store_transaction_begin(refs->packed_ref_store, &err);
1189+
transaction = ref_store_transaction_begin(refs->packed_ref_store,
1190+
REF_TRANSACTION_SKIP_HOOK, &err);
11891191
if (!transaction)
11901192
return -1;
11911193

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

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

1254-
if (refs_delete_refs(refs->packed_ref_store, msg, refnames, flags)) {
1255-
packed_refs_unlock(refs->packed_ref_store);
1257+
transaction = ref_store_transaction_begin(refs->packed_ref_store,
1258+
REF_TRANSACTION_SKIP_HOOK, &err);
1259+
if (!transaction)
1260+
goto error;
1261+
1262+
result = packed_refs_delete_refs(refs->packed_ref_store,
1263+
transaction, msg, refnames, flags);
1264+
if (result)
12561265
goto error;
1257-
}
12581266

12591267
packed_refs_unlock(refs->packed_ref_store);
12601268

@@ -1265,6 +1273,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
12651273
result |= error(_("could not remove reference %s"), refname);
12661274
}
12671275

1276+
ref_transaction_free(transaction);
12681277
strbuf_release(&err);
12691278
return result;
12701279

@@ -1281,6 +1290,7 @@ static int files_delete_refs(struct ref_store *ref_store, const char *msg,
12811290
else
12821291
error(_("could not delete references: %s"), err.buf);
12831292

1293+
ref_transaction_free(transaction);
12841294
strbuf_release(&err);
12851295
return -1;
12861296
}
@@ -2751,7 +2761,8 @@ static int files_transaction_prepare(struct ref_store *ref_store,
27512761
*/
27522762
if (!packed_transaction) {
27532763
packed_transaction = ref_store_transaction_begin(
2754-
refs->packed_ref_store, err);
2764+
refs->packed_ref_store,
2765+
REF_TRANSACTION_SKIP_HOOK, err);
27552766
if (!packed_transaction) {
27562767
ret = TRANSACTION_GENERIC_ERROR;
27572768
goto cleanup;
@@ -3022,7 +3033,8 @@ static int files_initial_transaction_commit(struct ref_store *ref_store,
30223033
&affected_refnames))
30233034
BUG("initial ref transaction called with existing refs");
30243035

3025-
packed_transaction = ref_store_transaction_begin(refs->packed_ref_store, err);
3036+
packed_transaction = ref_store_transaction_begin(refs->packed_ref_store,
3037+
REF_TRANSACTION_SKIP_HOOK, err);
30263038
if (!packed_transaction) {
30273039
ret = TRANSACTION_GENERIC_ERROR;
30283040
goto cleanup;

‎refs/packed-backend.c‎

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1521,15 +1521,10 @@ static int packed_initial_transaction_commit(struct ref_store *ref_store,
15211521
static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
15221522
struct string_list *refnames, unsigned int flags)
15231523
{
1524-
struct packed_ref_store *refs =
1525-
packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
15261524
struct strbuf err = STRBUF_INIT;
15271525
struct ref_transaction *transaction;
1528-
struct string_list_item *item;
15291526
int ret;
15301527

1531-
(void)refs; /* We need the check above, but don't use the variable */
1532-
15331528
if (!refnames->nr)
15341529
return 0;
15351530

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

1542-
transaction = ref_store_transaction_begin(ref_store, &err);
1537+
transaction = ref_store_transaction_begin(ref_store, 0, &err);
15431538
if (!transaction)
15441539
return -1;
15451540

1541+
ret = packed_refs_delete_refs(ref_store, transaction,
1542+
msg, refnames, flags);
1543+
1544+
ref_transaction_free(transaction);
1545+
return ret;
1546+
}
1547+
1548+
int packed_refs_delete_refs(struct ref_store *ref_store,
1549+
struct ref_transaction *transaction,
1550+
const char *msg,
1551+
struct string_list *refnames,
1552+
unsigned int flags)
1553+
{
1554+
struct strbuf err = STRBUF_INIT;
1555+
struct string_list_item *item;
1556+
int ret;
1557+
1558+
/* Assert that the ref store refers to a packed backend. */
1559+
packed_downcast(ref_store, REF_STORE_WRITE, "delete_refs");
1560+
15461561
for_each_string_list_item(item, refnames) {
15471562
if (ref_transaction_delete(transaction, item->string, NULL,
15481563
flags, msg, &err)) {
@@ -1562,7 +1577,6 @@ static int packed_delete_refs(struct ref_store *ref_store, const char *msg,
15621577
error(_("could not delete references: %s"), err.buf);
15631578
}
15641579

1565-
ref_transaction_free(transaction);
15661580
strbuf_release(&err);
15671581
return ret;
15681582
}

‎refs/packed-backend.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
struct repository;
55
struct ref_transaction;
6+
struct string_list;
67

78
/*
89
* 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)
2728
void packed_refs_unlock(struct ref_store *ref_store);
2829
int packed_refs_is_locked(struct ref_store *ref_store);
2930

31+
int packed_refs_delete_refs(struct ref_store *ref_store,
32+
struct ref_transaction *transaction,
33+
const char *msg,
34+
struct string_list *refnames,
35+
unsigned int flags);
36+
3037
/*
3138
* Return true if `transaction` really needs to be carried out against
3239
* the specified packed_ref_store, or false if it can be skipped

‎refs/refs-internal.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,7 @@ struct ref_transaction {
213213
size_t nr;
214214
enum ref_transaction_state state;
215215
void *backend_data;
216+
unsigned int flags;
216217
};
217218

218219
/*

‎sequencer.c‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3584,7 +3584,7 @@ static int do_label(struct repository *r, const char *name, int len)
35843584
strbuf_addf(&ref_name, "refs/rewritten/%.*s", len, name);
35853585
strbuf_addf(&msg, "rebase (label) '%.*s'", len, name);
35863586

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

‎t/t1416-ref-transaction-hooks.sh‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,4 +136,54 @@ test_expect_success 'interleaving hook calls succeed' '
136136
test_cmp expect target-repo.git/actual
137137
'
138138

139+
test_expect_success 'hook does not get called on packing refs' '
140+
# Pack references first such that we are in a known state.
141+
git pack-refs --all &&
142+
143+
write_script .git/hooks/reference-transaction <<-\EOF &&
144+
echo "$@" >>actual
145+
cat >>actual
146+
EOF
147+
rm -f actual &&
148+
149+
git update-ref refs/heads/unpacked-ref $POST_OID &&
150+
git pack-refs --all &&
151+
152+
# We only expect a single hook invocation, which is the call to
153+
# git-update-ref(1).
154+
cat >expect <<-EOF &&
155+
prepared
156+
$ZERO_OID $POST_OID refs/heads/unpacked-ref
157+
committed
158+
$ZERO_OID $POST_OID refs/heads/unpacked-ref
159+
EOF
160+
161+
test_cmp expect actual
162+
'
163+
164+
test_expect_success 'deleting packed ref calls hook once' '
165+
# Create a reference and pack it.
166+
git update-ref refs/heads/to-be-deleted $POST_OID &&
167+
git pack-refs --all &&
168+
169+
write_script .git/hooks/reference-transaction <<-\EOF &&
170+
echo "$@" >>actual
171+
cat >>actual
172+
EOF
173+
rm -f actual &&
174+
175+
git update-ref -d refs/heads/to-be-deleted $POST_OID &&
176+
177+
# We only expect a single hook invocation, which is the logical
178+
# deletion.
179+
cat >expect <<-EOF &&
180+
prepared
181+
$POST_OID $ZERO_OID refs/heads/to-be-deleted
182+
committed
183+
$POST_OID $ZERO_OID refs/heads/to-be-deleted
184+
EOF
185+
186+
test_cmp expect actual
187+
'
188+
139189
test_done

0 commit comments

Comments
 (0)