@matheusbsbs / matheus-bagliano / commits / b32c23be3b

refs: move object parsing to the generic layer

Regular reference updates made via reference transactions validate that the provided object ID exists in the object database, which is done by calling 'parse_object()'. This check is done independently by the backends which leads to duplicated logic. Let's move this to the generic layer, ensuring the backends only have to care about reference storage and not about validation of the object IDs. With this also remove the 'REF_TRANSACTION_ERROR_INVALID_NEW_VALUE' error type as its no longer used. Since we don't iterate over individual references in `ref_transaction_prepare()`, we add this check to `ref_transaction_update()`. This means that the validation is done as soon as an update is queued, without needing to prepare the transaction. It can be argued that this is more ideal, since this validation has no dependency on the reference transaction being prepared. It must be noted that the change in behavior means that this error cannot be ignored even with usage of batched updates, since this happens when the update is being added to the transaction. But since the caller gets specific error codes, they can either abort the transaction or continue adding other updates to the transaction. Modify 'builtin/receive-pack.c' to now capture the error type so that the error propagated to the client stays the same. Also remove two of the tests which validates batch-updates with invalid new_oid. Signed-off-by: Karthik Nayak <karthik.188@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Karthik Nayak committed May 4, 2026 at 19:44 UTC b32c23be3bf444ad8d56e8daee4a704ff8cae0ea

5 files changed +47 -54

builtin/receive-pack.c

+13 -9

@@ -1641,8 +1641,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)

1641 ret = NULL; /* good */

1642 }

1643 strbuf_release(&err);

1644 - }

1645 - else {

1644 + } else {

1645 + enum ref_transaction_error tx_err;

1646 struct strbuf err = STRBUF_INIT;

1647 if (shallow_update && si->shallow_ref[cmd->index] &&

1648 update_shallow_ref(cmd, si)) {

@@ -1650,14 +1650,18 @@ static const char *update(struct command *cmd, struct shallow_info *si)

1650 goto out;

1651 }

1652

1653 - if (ref_transaction_update(transaction,

1654 - namespaced_name,

1655 - new_oid, old_oid,

1656 - NULL, NULL,

1657 - 0, "push",

1658 - &err)) {

1653 + tx_err = ref_transaction_update(transaction,

1654 + namespaced_name,

1655 + new_oid, old_oid,

1656 + NULL, NULL,

1657 + 0, "push",

1658 + &err);

1659 + if (tx_err) {

1660 rp_error("%s", err.buf);

1660 - ret = "failed to update ref";

1661 + if (tx_err == REF_TRANSACTION_ERROR_GENERIC)

1662 + ret = "failed to update ref";

1663 + else

1664 + ret = ref_transaction_error_msg(tx_err);

1665 } else {

1666 ret = NULL; /* good */

1667 }

refs.c

+18

@@ -1416,6 +1416,24 @@ enum ref_transaction_error ref_transaction_update(struct ref_transaction *transa

1416 flags |= (new_oid ? REF_HAVE_NEW : 0) | (old_oid ? REF_HAVE_OLD : 0);

1417 flags |= (new_target ? REF_HAVE_NEW : 0) | (old_target ? REF_HAVE_OLD : 0);

1418

1419 + if ((flags & REF_HAVE_NEW) && !new_target && !is_null_oid(new_oid) &&

1420 + !(flags & REF_SKIP_OID_VERIFICATION) && !(flags & REF_LOG_ONLY)) {

1421 + struct object *o = parse_object(transaction->ref_store->repo, new_oid);

1422 +

1423 + if (!o) {

1424 + strbuf_addf(err,

1425 + _("trying to write ref '%s' with nonexistent object %s"),

1426 + refname, oid_to_hex(new_oid));

1427 + return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;

1428 + }

1429 +

1430 + if (o->type != OBJ_COMMIT && is_branch(refname)) {

1431 + strbuf_addf(err, _("trying to write non-commit object %s to branch '%s'"),

1432 + oid_to_hex(new_oid), refname);

1433 + return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;

1434 + }

1435 + }

1436 +

1437 ref_transaction_add_update(transaction, refname, flags,

1438 new_oid, old_oid, new_target,

1439 old_target, NULL, msg);

refs/files-backend.c

+2 -26

@@ -19,7 +19,6 @@

19 #include "../iterator.h"

20 #include "../dir-iterator.h"

21 #include "../lockfile.h"

22 -#include "../object.h"

22 #include "../path.h"

23 #include "../dir.h"

24 #include "../chdir-notify.h"

@@ -1589,7 +1588,6 @@ static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)

1588 static enum ref_transaction_error write_ref_to_lockfile(struct files_ref_store *refs,

1589 struct ref_lock *lock,

1590 const struct object_id *oid,

1592 - int skip_oid_verification,

1591 struct strbuf *err);

1592 static int commit_ref_update(struct files_ref_store *refs,

1593 struct ref_lock *lock,

@@ -1737,7 +1735,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,

1735 }

1736 oidcpy(&lock->old_oid, &orig_oid);

1737

1740 - if (write_ref_to_lockfile(refs, lock, &orig_oid, 0, &err) ||

1738 + if (write_ref_to_lockfile(refs, lock, &orig_oid, &err) ||

1739 commit_ref_update(refs, lock, &orig_oid, logmsg, 0, &err)) {

1740 error("unable to write current sha1 into %s: %s", newrefname, err.buf);

1741 strbuf_release(&err);

@@ -1755,7 +1753,7 @@ static int files_copy_or_rename_ref(struct ref_store *ref_store,

1753 goto rollbacklog;

1754 }

1755

1758 - if (write_ref_to_lockfile(refs, lock, &orig_oid, 0, &err) ||

1756 + if (write_ref_to_lockfile(refs, lock, &orig_oid, &err) ||

1757 commit_ref_update(refs, lock, &orig_oid, NULL, REF_SKIP_CREATE_REFLOG, &err)) {

1758 error("unable to write current sha1 into %s: %s", oldrefname, err.buf);

1759 strbuf_release(&err);

@@ -1999,32 +1997,11 @@ static int files_log_ref_write(struct files_ref_store *refs,

1997 static enum ref_transaction_error write_ref_to_lockfile(struct files_ref_store *refs,

1998 struct ref_lock *lock,

1999 const struct object_id *oid,

2002 - int skip_oid_verification,

2000 struct strbuf *err)

2001 {

2002 static char term = '\n';

2006 - struct object *o;

2003 int fd;

2004

2009 - if (!skip_oid_verification) {

2010 - o = parse_object(refs->base.repo, oid);

2011 - if (!o) {

2012 - strbuf_addf(

2013 - err,

2014 - "trying to write ref '%s' with nonexistent object %s",

2015 - lock->ref_name, oid_to_hex(oid));

2016 - unlock_ref(lock);

2017 - return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;

2018 - }

2019 - if (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {

2020 - strbuf_addf(

2021 - err,

2022 - "trying to write non-commit object %s to branch '%s'",

2023 - oid_to_hex(oid), lock->ref_name);

2024 - unlock_ref(lock);

2025 - return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;

2026 - }

2027 - }

2005 fd = get_lock_file_fd(&lock->lk);

2006 if (write_in_full(fd, oid_to_hex(oid), refs->base.repo->hash_algo->hexsz) < 0 ||

2007 write_in_full(fd, &term, 1) < 0 ||

@@ -2828,7 +2805,6 @@ static enum ref_transaction_error lock_ref_for_update(struct files_ref_store *re

2805 } else {

2806 ret = write_ref_to_lockfile(

2807 refs, lock, &update->new_oid,

2831 - update->flags & REF_SKIP_OID_VERIFICATION,

2808 err);

2809 if (ret) {

2810 char *write_err = strbuf_detach(err, NULL);

refs/reftable-backend.c

-19

@@ -1081,25 +1081,6 @@ static enum ref_transaction_error prepare_single_update(struct reftable_ref_stor

1081 return 0;

1082 }

1083

1084 - /* Verify that the new object ID is valid. */

1085 - if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) &&

1086 - !(u->flags & REF_SKIP_OID_VERIFICATION) &&

1087 - !(u->flags & REF_LOG_ONLY)) {

1088 - struct object *o = parse_object(refs->base.repo, &u->new_oid);

1089 - if (!o) {

1090 - strbuf_addf(err,

1091 - _("trying to write ref '%s' with nonexistent object %s"),

1092 - u->refname, oid_to_hex(&u->new_oid));

1093 - return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;

1094 - }

1095 -

1096 - if (o->type != OBJ_COMMIT && is_branch(u->refname)) {

1097 - strbuf_addf(err, _("trying to write non-commit object %s to branch '%s'"),

1098 - oid_to_hex(&u->new_oid), u->refname);

1099 - return REF_TRANSACTION_ERROR_INVALID_NEW_VALUE;

1100 - }

1101 - }

1102 -

1084 /*

1085 * When we update the reference that HEAD points to we enqueue

1086 * a second log-only update for HEAD so that its reflog is

t/t1400-update-ref.sh

+14

@@ -1196,6 +1196,20 @@ test_expect_success 'stdin -z create ref fails with empty new value' '

1196 test_must_fail git rev-parse --verify -q $c

1197 '

1198

1199 +test_expect_success 'stdin -z create ref fails with non commit object' '

1200 + printf $F "create $c" "$(test_oid 001)" >stdin &&

1201 + test_must_fail git update-ref -z --stdin <stdin 2>err &&

1202 + grep "fatal: trying to write ref ${SQ}$c${SQ} with nonexistent object" err &&

1203 + test_must_fail git rev-parse --verify -q $c

1204 +'

1205 +

1206 +test_expect_success 'stdin -z update ref fails with non commit object' '

1207 + printf $F "update $b" "$(test_oid 001)" "" >stdin &&

1208 + test_must_fail git update-ref -z --stdin <stdin 2>err &&

1209 + grep "fatal: trying to write ref ${SQ}$b${SQ} with nonexistent object" err &&

1210 + test_must_fail git rev-parse --verify -q $c

1211 +'

1212 +

1213 test_expect_success 'stdin -z update ref works with right old value' '

1214 printf $F "update $b" "$m~1" "$m" >stdin &&

1215 git update-ref -z --stdin <stdin &&