Skip to content

Commit 02abc6b

Browse files
gitstergitster
committed
Merge branch 'mh/avoid-rewriting-packed-refs' into maint
Recent update to the refs infrastructure implementation started rewriting packed-refs file more often than before; this has been optimized again for most trivial cases. * mh/avoid-rewriting-packed-refs: files-backend: don't rewrite the `packed-refs` file unnecessarily t1409: check that `packed-refs` is not rewritten unnecessarily
2 parents 9b185be + 7c6bd25 commit 02abc6b

File tree

4 files changed

+238
-1
lines changed

4 files changed

+238
-1
lines changed

‎refs/files-backend.c

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2605,7 +2605,23 @@ static int files_transaction_prepare(struct ref_store *ref_store,
26052605
goto cleanup;
26062606
}
26072607
backend_data->packed_refs_locked = 1;
2608-
ret = ref_transaction_prepare(packed_transaction, err);
2608+
2609+
if (is_packed_transaction_needed(refs->packed_ref_store,
2610+
packed_transaction)) {
2611+
ret = ref_transaction_prepare(packed_transaction, err);
2612+
} else {
2613+
/*
2614+
* We can skip rewriting the `packed-refs`
2615+
* file. But we do need to leave it locked, so
2616+
* that somebody else doesn't pack a reference
2617+
* that we are trying to delete.
2618+
*/
2619+
if (ref_transaction_abort(packed_transaction, err)) {
2620+
ret = TRANSACTION_GENERIC_ERROR;
2621+
goto cleanup;
2622+
}
2623+
backend_data->packed_transaction = NULL;
2624+
}
26092625
}
26102626

26112627
cleanup:

‎refs/packed-backend.c

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1261,6 +1261,100 @@ static int write_with_updates(struct packed_ref_store *refs,
12611261
return -1;
12621262
}
12631263

1264+
int is_packed_transaction_needed(struct ref_store *ref_store,
1265+
struct ref_transaction *transaction)
1266+
{
1267+
struct packed_ref_store *refs = packed_downcast(
1268+
ref_store,
1269+
REF_STORE_READ,
1270+
"is_packed_transaction_needed");
1271+
struct strbuf referent = STRBUF_INIT;
1272+
size_t i;
1273+
int ret;
1274+
1275+
if (!is_lock_file_locked(&refs->lock))
1276+
BUG("is_packed_transaction_needed() called while unlocked");
1277+
1278+
/*
1279+
* We're only going to bother returning false for the common,
1280+
* trivial case that references are only being deleted, their
1281+
* old values are not being checked, and the old `packed-refs`
1282+
* file doesn't contain any of those reference(s). This gives
1283+
* false positives for some other cases that could
1284+
* theoretically be optimized away:
1285+
*
1286+
* 1. It could be that the old value is being verified without
1287+
* setting a new value. In this case, we could verify the
1288+
* old value here and skip the update if it agrees. If it
1289+
* disagrees, we could either let the update go through
1290+
* (the actual commit would re-detect and report the
1291+
* problem), or come up with a way of reporting such an
1292+
* error to *our* caller.
1293+
*
1294+
* 2. It could be that a new value is being set, but that it
1295+
* is identical to the current packed value of the
1296+
* reference.
1297+
*
1298+
* Neither of these cases will come up in the current code,
1299+
* because the only caller of this function passes to it a
1300+
* transaction that only includes `delete` updates with no
1301+
* `old_id`. Even if that ever changes, false positives only
1302+
* cause an optimization to be missed; they do not affect
1303+
* correctness.
1304+
*/
1305+
1306+
/*
1307+
* Start with the cheap checks that don't require old
1308+
* reference values to be read:
1309+
*/
1310+
for (i = 0; i < transaction->nr; i++) {
1311+
struct ref_update *update = transaction->updates[i];
1312+
1313+
if (update->flags & REF_HAVE_OLD)
1314+
/* Have to check the old value -> needed. */
1315+
return 1;
1316+
1317+
if ((update->flags & REF_HAVE_NEW) && !is_null_oid(&update->new_oid))
1318+
/* Have to set a new value -> needed. */
1319+
return 1;
1320+
}
1321+
1322+
/*
1323+
* The transaction isn't checking any old values nor is it
1324+
* setting any nonzero new values, so it still might be able
1325+
* to be skipped. Now do the more expensive check: the update
1326+
* is needed if any of the updates is a delete, and the old
1327+
* `packed-refs` file contains a value for that reference.
1328+
*/
1329+
ret = 0;
1330+
for (i = 0; i < transaction->nr; i++) {
1331+
struct ref_update *update = transaction->updates[i];
1332+
unsigned int type;
1333+
struct object_id oid;
1334+
1335+
if (!(update->flags & REF_HAVE_NEW))
1336+
/*
1337+
* This reference isn't being deleted -> not
1338+
* needed.
1339+
*/
1340+
continue;
1341+
1342+
if (!refs_read_raw_ref(ref_store, update->refname,
1343+
oid.hash, &referent, &type) ||
1344+
errno != ENOENT) {
1345+
/*
1346+
* We have to actually delete that reference
1347+
* -> this transaction is needed.
1348+
*/
1349+
ret = 1;
1350+
break;
1351+
}
1352+
}
1353+
1354+
strbuf_release(&referent);
1355+
return ret;
1356+
}
1357+
12641358
struct packed_transaction_backend_data {
12651359
/* True iff the transaction owns the packed-refs lock. */
12661360
int own_lock;

‎refs/packed-backend.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,4 +23,13 @@ int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)
2323
void packed_refs_unlock(struct ref_store *ref_store);
2424
int packed_refs_is_locked(struct ref_store *ref_store);
2525

26+
/*
27+
* Return true if `transaction` really needs to be carried out against
28+
* the specified packed_ref_store, or false if it can be skipped
29+
* (i.e., because it is an obvious NOOP). `ref_store` must be locked
30+
* before calling this function.
31+
*/
32+
int is_packed_transaction_needed(struct ref_store *ref_store,
33+
struct ref_transaction *transaction);
34+
2635
#endif /* REFS_PACKED_BACKEND_H */

‎t/t1409-avoid-packing-refs.sh

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
#!/bin/sh
2+
3+
test_description='avoid rewriting packed-refs unnecessarily'
4+
5+
. ./test-lib.sh
6+
7+
# Add an identifying mark to the packed-refs file header line. This
8+
# shouldn't upset readers, and it should be omitted if the file is
9+
# ever rewritten.
10+
mark_packed_refs () {
11+
sed -e "s/^\(#.*\)/\1 t1409 /" <.git/packed-refs >.git/packed-refs.new &&
12+
mv .git/packed-refs.new .git/packed-refs
13+
}
14+
15+
# Verify that the packed-refs file is still marked.
16+
check_packed_refs_marked () {
17+
grep -q '^#.* t1409 ' .git/packed-refs
18+
}
19+
20+
test_expect_success 'setup' '
21+
git commit --allow-empty -m "Commit A" &&
22+
A=$(git rev-parse HEAD) &&
23+
git commit --allow-empty -m "Commit B" &&
24+
B=$(git rev-parse HEAD) &&
25+
git commit --allow-empty -m "Commit C" &&
26+
C=$(git rev-parse HEAD)
27+
'
28+
29+
test_expect_success 'do not create packed-refs file gratuitously' '
30+
test_must_fail test -f .git/packed-refs &&
31+
git update-ref refs/heads/foo $A &&
32+
test_must_fail test -f .git/packed-refs &&
33+
git update-ref refs/heads/foo $B &&
34+
test_must_fail test -f .git/packed-refs &&
35+
git update-ref refs/heads/foo $C $B &&
36+
test_must_fail test -f .git/packed-refs &&
37+
git update-ref -d refs/heads/foo &&
38+
test_must_fail test -f .git/packed-refs
39+
'
40+
41+
test_expect_success 'check that marking the packed-refs file works' '
42+
git for-each-ref >expected &&
43+
git pack-refs --all &&
44+
mark_packed_refs &&
45+
check_packed_refs_marked &&
46+
git for-each-ref >actual &&
47+
test_cmp expected actual &&
48+
git pack-refs --all &&
49+
test_must_fail check_packed_refs_marked &&
50+
git for-each-ref >actual2 &&
51+
test_cmp expected actual2
52+
'
53+
54+
test_expect_success 'leave packed-refs untouched on update of packed' '
55+
git update-ref refs/heads/packed-update $A &&
56+
git pack-refs --all &&
57+
mark_packed_refs &&
58+
git update-ref refs/heads/packed-update $B &&
59+
check_packed_refs_marked
60+
'
61+
62+
test_expect_success 'leave packed-refs untouched on checked update of packed' '
63+
git update-ref refs/heads/packed-checked-update $A &&
64+
git pack-refs --all &&
65+
mark_packed_refs &&
66+
git update-ref refs/heads/packed-checked-update $B $A &&
67+
check_packed_refs_marked
68+
'
69+
70+
test_expect_success 'leave packed-refs untouched on verify of packed' '
71+
git update-ref refs/heads/packed-verify $A &&
72+
git pack-refs --all &&
73+
mark_packed_refs &&
74+
echo "verify refs/heads/packed-verify $A" | git update-ref --stdin &&
75+
check_packed_refs_marked
76+
'
77+
78+
test_expect_success 'touch packed-refs on delete of packed' '
79+
git update-ref refs/heads/packed-delete $A &&
80+
git pack-refs --all &&
81+
mark_packed_refs &&
82+
git update-ref -d refs/heads/packed-delete &&
83+
test_must_fail check_packed_refs_marked
84+
'
85+
86+
test_expect_success 'leave packed-refs untouched on update of loose' '
87+
git pack-refs --all &&
88+
git update-ref refs/heads/loose-update $A &&
89+
mark_packed_refs &&
90+
git update-ref refs/heads/loose-update $B &&
91+
check_packed_refs_marked
92+
'
93+
94+
test_expect_success 'leave packed-refs untouched on checked update of loose' '
95+
git pack-refs --all &&
96+
git update-ref refs/heads/loose-checked-update $A &&
97+
mark_packed_refs &&
98+
git update-ref refs/heads/loose-checked-update $B $A &&
99+
check_packed_refs_marked
100+
'
101+
102+
test_expect_success 'leave packed-refs untouched on verify of loose' '
103+
git pack-refs --all &&
104+
git update-ref refs/heads/loose-verify $A &&
105+
mark_packed_refs &&
106+
echo "verify refs/heads/loose-verify $A" | git update-ref --stdin &&
107+
check_packed_refs_marked
108+
'
109+
110+
test_expect_success 'leave packed-refs untouched on delete of loose' '
111+
git pack-refs --all &&
112+
git update-ref refs/heads/loose-delete $A &&
113+
mark_packed_refs &&
114+
git update-ref -d refs/heads/loose-delete &&
115+
check_packed_refs_marked
116+
'
117+
118+
test_done

0 commit comments

Comments
 (0)