| Message ID | 20241010133022.1733542-4-bence@ferdinandy.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/6] refs_update_symref: atomically record overwritten ref | expand |
Bence Ferdinandy <bence@ferdinandy.com> writes: > transaction: add TRANSACTION_CREATE_EXISTS error Nit: but it would be nice to have s/transaction/refs here and similarly in other patches. Phrasing 'Documentation/SubmittingPatches': It is also conventional in most cases to prefix the first line with "area: " where the area is a filename or identifier for the general area of the code being modified, e.g. * doc: clarify distinction between sign-off and pgp-signing * githooks.txt: improve the intro section So in this sense, it would be the refs area, no? > Currently there is only one special error for transaction, for when > there is a naming conflict, all other errors are dumped under a generic > error. Add a new special error case for when the caller requests the > reference to be updated only when it does not yet exist and the > reference actually does exist. > > Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> > --- > > Notes: > v4: new patch > v5: no change > v6: no change > > refs.h | 4 +++- > refs/files-backend.c | 28 ++++++++++++++++++++-------- > refs/reftable-backend.c | 6 ++++-- > 3 files changed, 27 insertions(+), 11 deletions(-) > > diff --git a/refs.h b/refs.h > index f38616db84..166affbc89 100644 > --- a/refs.h > +++ b/refs.h > @@ -759,8 +759,10 @@ int ref_transaction_verify(struct ref_transaction *transaction, > > /* Naming conflict (for example, the ref names A and A/B conflict). */ > #define TRANSACTION_NAME_CONFLICT -1 > +/* When only creation was requested, but the ref already exists. */ > +#define TRANSACTION_CREATE_EXISTS -2 > /* All other errors. */ > -#define TRANSACTION_GENERIC_ERROR -2 > +#define TRANSACTION_GENERIC_ERROR -3 > > /* > * Perform the preparatory stages of committing `transaction`. Acquire > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 8415f2d020..272ad81315 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -2502,14 +2502,18 @@ static int split_symref_update(struct ref_update *update, > static int check_old_oid(struct ref_update *update, struct object_id *oid, > struct strbuf *err) > { > + int ret = TRANSACTION_GENERIC_ERROR; > + > if (!(update->flags & REF_HAVE_OLD) || > oideq(oid, &update->old_oid)) > return 0; > > - if (is_null_oid(&update->old_oid)) > + if (is_null_oid(&update->old_oid)) { > strbuf_addf(err, "cannot lock ref '%s': " > "reference already exists", > ref_update_original_update_refname(update)); > + ret = TRANSACTION_CREATE_EXISTS; > + } > else if (is_null_oid(oid)) > strbuf_addf(err, "cannot lock ref '%s': " > "reference is missing but expected %s", > @@ -2522,7 +2526,7 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid, > oid_to_hex(oid), > oid_to_hex(&update->old_oid)); > > - return -1; > + return ret; > } > > /* > @@ -2603,9 +2607,13 @@ static int lock_ref_for_update(struct files_ref_store *refs, > ret = TRANSACTION_GENERIC_ERROR; > goto out; > } > - } else if (check_old_oid(update, &lock->old_oid, err)) { > - ret = TRANSACTION_GENERIC_ERROR; > - goto out; > + } else { > + int checkret; > + checkret = check_old_oid(update, &lock->old_oid, err); > + if (checkret) { > + ret = checkret; > + goto out; > + } Can't we simply do: ret = check_old_oid(update, &lock->old_oid, err); if (ret) { goto out } if ret is '0', it shouldn't matter no? [snip]
karthik nayak <karthik.188@gmail.com> writes: >> + } else { >> + int checkret; >> + checkret = check_old_oid(update, &lock->old_oid, err); >> + if (checkret) { >> + ret = checkret; >> + goto out; >> + } > > Can't we simply do: > > ret = check_old_oid(update, &lock->old_oid, err); > if (ret) { > goto out > } > > if ret is '0', it shouldn't matter no? That's nice. Yes, as long as "ret" has no useful information when we enter this "else" block, reusing it like you did is just fine. This is one of these moments I tell myself "oh, why didn't *I* think of that" ;-). Thanks.
diff --git a/refs.h b/refs.h index f38616db84..166affbc89 100644 --- a/refs.h +++ b/refs.h @@ -759,8 +759,10 @@ int ref_transaction_verify(struct ref_transaction *transaction, /* Naming conflict (for example, the ref names A and A/B conflict). */ #define TRANSACTION_NAME_CONFLICT -1 +/* When only creation was requested, but the ref already exists. */ +#define TRANSACTION_CREATE_EXISTS -2 /* All other errors. */ -#define TRANSACTION_GENERIC_ERROR -2 +#define TRANSACTION_GENERIC_ERROR -3 /* * Perform the preparatory stages of committing `transaction`. Acquire diff --git a/refs/files-backend.c b/refs/files-backend.c index 8415f2d020..272ad81315 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2502,14 +2502,18 @@ static int split_symref_update(struct ref_update *update, static int check_old_oid(struct ref_update *update, struct object_id *oid, struct strbuf *err) { + int ret = TRANSACTION_GENERIC_ERROR; + if (!(update->flags & REF_HAVE_OLD) || oideq(oid, &update->old_oid)) return 0; - if (is_null_oid(&update->old_oid)) + if (is_null_oid(&update->old_oid)) { strbuf_addf(err, "cannot lock ref '%s': " "reference already exists", ref_update_original_update_refname(update)); + ret = TRANSACTION_CREATE_EXISTS; + } else if (is_null_oid(oid)) strbuf_addf(err, "cannot lock ref '%s': " "reference is missing but expected %s", @@ -2522,7 +2526,7 @@ static int check_old_oid(struct ref_update *update, struct object_id *oid, oid_to_hex(oid), oid_to_hex(&update->old_oid)); - return -1; + return ret; } /* @@ -2603,9 +2607,13 @@ static int lock_ref_for_update(struct files_ref_store *refs, ret = TRANSACTION_GENERIC_ERROR; goto out; } - } else if (check_old_oid(update, &lock->old_oid, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto out; + } else { + int checkret; + checkret = check_old_oid(update, &lock->old_oid, err); + if (checkret) { + ret = checkret; + goto out; + } } } else { /* @@ -2636,9 +2644,13 @@ static int lock_ref_for_update(struct files_ref_store *refs, update->old_target); ret = TRANSACTION_GENERIC_ERROR; goto out; - } else if (check_old_oid(update, &lock->old_oid, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto out; + } else { + int checkret; + checkret = check_old_oid(update, &lock->old_oid, err); + if (checkret) { + ret = checkret; + goto out; + } } /* diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 32330b6bc6..c6b25ebac4 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1206,10 +1206,13 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, goto done; } } else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) { - if (is_null_oid(&u->old_oid)) + ret = TRANSACTION_NAME_CONFLICT; + if (is_null_oid(&u->old_oid)) { strbuf_addf(err, _("cannot lock ref '%s': " "reference already exists"), ref_update_original_update_refname(u)); + ret = TRANSACTION_CREATE_EXISTS; + } else if (is_null_oid(¤t_oid)) strbuf_addf(err, _("cannot lock ref '%s': " "reference is missing but expected %s"), @@ -1221,7 +1224,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, ref_update_original_update_refname(u), oid_to_hex(¤t_oid), oid_to_hex(&u->old_oid)); - ret = -1; goto done; }
Currently there is only one special error for transaction, for when there is a naming conflict, all other errors are dumped under a generic error. Add a new special error case for when the caller requests the reference to be updated only when it does not yet exist and the reference actually does exist. Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com> --- Notes: v4: new patch v5: no change v6: no change refs.h | 4 +++- refs/files-backend.c | 28 ++++++++++++++++++++-------- refs/reftable-backend.c | 6 ++++-- 3 files changed, 27 insertions(+), 11 deletions(-)