Skip to content

Commit 7b6ee0a

Browse files
gitstergitster
committed
Merge branch 'kn/fetch-push-bulk-ref-update-fixup' into jch
Additional fixes to the base topic. * kn/fetch-push-bulk-ref-update-fixup: receive-pack: handle reference deletions separately refs/files: skip updates with errors in batched updates
2 parents 3b93237 + 5916b99 commit 7b6ee0a

File tree

4 files changed

+131
-38
lines changed

4 files changed

+131
-38
lines changed

‎builtin/receive-pack.c

Lines changed: 66 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1868,47 +1868,79 @@ static void execute_commands_non_atomic(struct command *commands,
18681868
const char *reported_error = NULL;
18691869
struct strmap failed_refs = STRMAP_INIT;
18701870

1871-
transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
1872-
REF_TRANSACTION_ALLOW_FAILURE, &err);
1873-
if (!transaction) {
1874-
rp_error("%s", err.buf);
1875-
strbuf_reset(&err);
1876-
reported_error = "transaction failed to start";
1877-
goto failure;
1878-
}
1871+
/*
1872+
* Reference updates, where F/D conflicts shouldn't arise due to
1873+
* one reference being deleted, while the other being created
1874+
* are treated as conflicts in batched updates. This is because
1875+
* we don't do conflict resolution inside a transaction. To
1876+
* mitigate this, delete references in a separate batch.
1877+
*/
1878+
enum processing_phase {
1879+
PHASE_DELETIONS,
1880+
PHASE_OTHERS
1881+
};
18791882

1880-
for (cmd = commands; cmd; cmd = cmd->next) {
1881-
if (!should_process_cmd(cmd) || cmd->run_proc_receive)
1882-
continue;
1883+
for (enum processing_phase phase = PHASE_DELETIONS; phase <= PHASE_OTHERS; phase++) {
1884+
for (cmd = commands; cmd; cmd = cmd->next) {
1885+
if (!should_process_cmd(cmd) || cmd->run_proc_receive)
1886+
continue;
18831887

1884-
cmd->error_string = update(cmd, si);
1885-
}
1888+
if (phase == PHASE_DELETIONS && !is_null_oid(&cmd->new_oid))
1889+
continue;
1890+
else if (phase == PHASE_OTHERS && is_null_oid(&cmd->new_oid))
1891+
continue;
18861892

1887-
if (ref_transaction_commit(transaction, &err)) {
1888-
rp_error("%s", err.buf);
1889-
reported_error = "failed to update refs";
1890-
goto failure;
1891-
}
1893+
/*
1894+
* Lazily create a transaction only when we know there are
1895+
* updates to be added.
1896+
*/
1897+
if (!transaction) {
1898+
transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
1899+
REF_TRANSACTION_ALLOW_FAILURE, &err);
1900+
if (!transaction) {
1901+
rp_error("%s", err.buf);
1902+
strbuf_reset(&err);
1903+
reported_error = "transaction failed to s1tart";
1904+
goto failure;
1905+
}
1906+
}
18921907

1893-
ref_transaction_for_each_rejected_update(transaction,
1894-
ref_transaction_rejection_handler,
1895-
&failed_refs);
1908+
cmd->error_string = update(cmd, si);
1909+
}
18961910

1897-
if (strmap_empty(&failed_refs))
1898-
goto cleanup;
1911+
/*
1912+
* If no transaction was created, there is nothing to commit.
1913+
*/
1914+
if (!transaction)
1915+
goto cleanup;
18991916

1900-
failure:
1901-
for (cmd = commands; cmd; cmd = cmd->next) {
1902-
if (reported_error)
1903-
cmd->error_string = reported_error;
1904-
else if (strmap_contains(&failed_refs, cmd->ref_name))
1905-
cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
1906-
}
1917+
if (ref_transaction_commit(transaction, &err)) {
1918+
rp_error("%s", err.buf);
1919+
reported_error = "failed to update refs";
1920+
goto failure;
1921+
}
19071922

1908-
cleanup:
1909-
ref_transaction_free(transaction);
1910-
strmap_clear(&failed_refs, 0);
1911-
strbuf_release(&err);
1923+
ref_transaction_for_each_rejected_update(transaction,
1924+
ref_transaction_rejection_handler,
1925+
&failed_refs);
1926+
1927+
if (strmap_empty(&failed_refs))
1928+
goto cleanup;
1929+
1930+
failure:
1931+
for (cmd = commands; cmd; cmd = cmd->next) {
1932+
if (reported_error)
1933+
cmd->error_string = reported_error;
1934+
else if (strmap_contains(&failed_refs, cmd->ref_name))
1935+
cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
1936+
}
1937+
1938+
cleanup:
1939+
ref_transaction_free(transaction);
1940+
transaction = NULL;
1941+
strmap_clear(&failed_refs, 0);
1942+
strbuf_release(&err);
1943+
}
19121944
}
19131945

19141946
static void execute_commands_atomic(struct command *commands,

‎refs/files-backend.c

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3208,6 +3208,10 @@ static int files_transaction_finish(struct ref_store *ref_store,
32083208
*/
32093209
for (i = 0; i < transaction->nr; i++) {
32103210
struct ref_update *update = transaction->updates[i];
3211+
3212+
if (update->rejection_err)
3213+
continue;
3214+
32113215
if (update->flags & REF_DELETING &&
32123216
!(update->flags & REF_LOG_ONLY) &&
32133217
!(update->flags & REF_IS_PRUNING)) {
@@ -3239,6 +3243,9 @@ static int files_transaction_finish(struct ref_store *ref_store,
32393243
struct ref_update *update = transaction->updates[i];
32403244
struct ref_lock *lock = update->backend_data;
32413245

3246+
if (update->rejection_err)
3247+
continue;
3248+
32423249
if (update->flags & REF_DELETING &&
32433250
!(update->flags & REF_LOG_ONLY)) {
32443251
update->flags |= REF_DELETED_RMDIR;

‎t/t1400-update-ref.sh

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2299,6 +2299,51 @@ do
22992299
test_grep -q "refname conflict" stdout
23002300
)
23012301
'
2302+
2303+
test_expect_success "stdin $type batch-updates delete incorrect symbolic ref" '
2304+
git init repo &&
2305+
test_when_finished "rm -fr repo" &&
2306+
(
2307+
cd repo &&
2308+
test_commit c1 &&
2309+
head=$(git rev-parse HEAD) &&
2310+
git symbolic-ref refs/heads/symbolic refs/heads/non-existent &&
2311+
2312+
format_command $type "delete refs/heads/symbolic" "$head" >stdin &&
2313+
git update-ref $type --stdin --batch-updates <stdin >stdout &&
2314+
test_grep "reference does not exist" stdout
2315+
)
2316+
'
2317+
2318+
test_expect_success "stdin $type batch-updates delete with incorrect old_oid" '
2319+
git init repo &&
2320+
test_when_finished "rm -fr repo" &&
2321+
(
2322+
cd repo &&
2323+
test_commit c1 &&
2324+
git branch new-branch &&
2325+
test_commit c2 &&
2326+
head=$(git rev-parse HEAD) &&
2327+
2328+
format_command $type "delete refs/heads/new-branch" "$head" >stdin &&
2329+
git update-ref $type --stdin --batch-updates <stdin >stdout &&
2330+
test_grep "incorrect old value provided" stdout
2331+
)
2332+
'
2333+
2334+
test_expect_success "stdin $type batch-updates delete non-existent ref" '
2335+
git init repo &&
2336+
test_when_finished "rm -fr repo" &&
2337+
(
2338+
cd repo &&
2339+
test_commit commit &&
2340+
head=$(git rev-parse HEAD) &&
2341+
2342+
format_command $type "delete refs/heads/non-existent" "$head" >stdin &&
2343+
git update-ref $type --stdin --batch-updates <stdin >stdout &&
2344+
test_grep "reference does not exist" stdout
2345+
)
2346+
'
23022347
done
23032348

23042349
test_expect_success 'update-ref should also create reflog for HEAD' '

‎t/t5516-fetch-push.sh

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -744,8 +744,8 @@ test_expect_success 'pushing valid refs triggers post-receive and post-update ho
744744
EOF
745745
746746
cat >update.expect <<-EOF &&
747-
refs/heads/main $orgmain $newmain
748747
refs/heads/next $orgnext $newnext
748+
refs/heads/main $orgmain $newmain
749749
EOF
750750
751751
cat >post-receive.expect <<-EOF &&
@@ -808,8 +808,8 @@ test_expect_success 'deletion of a non-existent ref is not fed to post-receive a
808808
EOF
809809
810810
cat >update.expect <<-EOF &&
811-
refs/heads/main $orgmain $newmain
812811
refs/heads/nonexistent $ZERO_OID $ZERO_OID
812+
refs/heads/main $orgmain $newmain
813813
EOF
814814
815815
cat >post-receive.expect <<-EOF &&
@@ -868,10 +868,10 @@ test_expect_success 'mixed ref updates, deletes, invalid deletes trigger hooks w
868868
EOF
869869
870870
cat >update.expect <<-EOF &&
871-
refs/heads/main $orgmain $newmain
872871
refs/heads/next $orgnext $newnext
873-
refs/heads/seen $orgseen $newseen
874872
refs/heads/nonexistent $ZERO_OID $ZERO_OID
873+
refs/heads/main $orgmain $newmain
874+
refs/heads/seen $orgseen $newseen
875875
EOF
876876
877877
cat >post-receive.expect <<-EOF &&
@@ -1919,4 +1919,13 @@ test_expect_success 'push with config pack.usePathWalk=true' '
19191919
test_region pack-objects path-walk path-walk.txt
19201920
'
19211921

1922+
test_expect_success 'push with F/D conflict with deletion and creation' '
1923+
test_when_finished "git branch -D branch" &&
1924+
git branch branch/conflict &&
1925+
mk_test testrepo heads/branch/conflict &&
1926+
git branch -D branch/conflict &&
1927+
git branch branch &&
1928+
git push testrepo :refs/heads/branch/conflict refs/heads/branch
1929+
'
1930+
19221931
test_done

0 commit comments

Comments
 (0)