| Message ID | 20240605102958.716432-3-knayak@gitlab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | update-ref: add symref support for --stdin | expand |
On Wed, Jun 05, 2024 at 12:29:53PM +0200, Karthik Nayak wrote: > From: Karthik Nayak <karthik.188@gmail.com> > > When a regular reference update contains `old_target` set, we call the s/contains/has its/ > `ref_update_check_old_target` function to check the referent value. But > for regular refs we know that the referent value is not set and this This is fairly technical and focussed on the implementation. Can we maybe talk more about intent ("expected a symref, but is a direct ref") rather than the exact implementation to make the commit message a bit easier to understand for folks? > simply raises a generic error which says nothing about this being a > regular ref. Instead let's raise a more specific error when a regular > ref update contains `old_target`. It might be helpful to include before/after in this commit message to show the change. Even better would be a test of course, but I think we cannot add one at this point in time yet. > Signed-off-by: Karthik Nayak <karthik.188@gmail.com> > --- > refs/files-backend.c | 13 +++++++------ > refs/reftable-backend.c | 9 +++++++++ > 2 files changed, 16 insertions(+), 6 deletions(-) > > diff --git a/refs/files-backend.c b/refs/files-backend.c > index 194e74eb4d..f516d4d82f 100644 > --- a/refs/files-backend.c > +++ b/refs/files-backend.c > @@ -2491,14 +2491,15 @@ static int lock_ref_for_update(struct files_ref_store *refs, > > /* > * Even if the ref is a regular ref, if `old_target` is set, we > - * check the referent value. Ideally `old_target` should only > - * be set for symrefs, but we're strict about its usage. > + * fail with an error. > */ > if (update->old_target) { > - if (ref_update_check_old_target(referent.buf, update, err)) { > - ret = TRANSACTION_GENERIC_ERROR; > - goto out; > - } > + strbuf_addf(err, _("cannot update regular ref: '%s': " > + "symref target '%s' set"), > + ref_update_original_update_refname(update), > + update->old_target); > + ret = TRANSACTION_GENERIC_ERROR; > + goto out; I feel like these error messages are somewhat technical. If I were reading it as a user, I don't think I'd understand what it is trying to tell me. How about: strbuf_addf(err, _("cannot lock ref '%s': %s", "expected symref but is a direct ref")); This also matches the other error messages we have in this function more closely. The same is true for the equivalent in the reftable backend. Patrick
Patrick Steinhardt <ps@pks.im> writes: > On Wed, Jun 05, 2024 at 12:29:53PM +0200, Karthik Nayak wrote: >> From: Karthik Nayak <karthik.188@gmail.com> >> >> When a regular reference update contains `old_target` set, we call the > > s/contains/has its/ > >> `ref_update_check_old_target` function to check the referent value. But >> for regular refs we know that the referent value is not set and this > > This is fairly technical and focussed on the implementation. Can we > maybe talk more about intent ("expected a symref, but is a direct ref") > rather than the exact implementation to make the commit message a bit > easier to understand for folks? > Fair enough, will change this. >> simply raises a generic error which says nothing about this being a >> regular ref. Instead let's raise a more specific error when a regular >> ref update contains `old_target`. > > It might be helpful to include before/after in this commit message to > show the change. Even better would be a test of course, but I think we > cannot add one at this point in time yet. > Yeah will do. Yeah, adding a test is only possible in the commit where we introduce `symref-delete` and we do test it there. >> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> >> --- >> refs/files-backend.c | 13 +++++++------ >> refs/reftable-backend.c | 9 +++++++++ >> 2 files changed, 16 insertions(+), 6 deletions(-) >> >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index 194e74eb4d..f516d4d82f 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> @@ -2491,14 +2491,15 @@ static int lock_ref_for_update(struct files_ref_store *refs, >> >> /* >> * Even if the ref is a regular ref, if `old_target` is set, we >> - * check the referent value. Ideally `old_target` should only >> - * be set for symrefs, but we're strict about its usage. >> + * fail with an error. >> */ >> if (update->old_target) { >> - if (ref_update_check_old_target(referent.buf, update, err)) { >> - ret = TRANSACTION_GENERIC_ERROR; >> - goto out; >> - } >> + strbuf_addf(err, _("cannot update regular ref: '%s': " >> + "symref target '%s' set"), >> + ref_update_original_update_refname(update), >> + update->old_target); >> + ret = TRANSACTION_GENERIC_ERROR; >> + goto out; > > I feel like these error messages are somewhat technical. If I were > reading it as a user, I don't think I'd understand what it is trying to > tell me. How about: > > strbuf_addf(err, _("cannot lock ref '%s': %s", > "expected symref but is a direct ref")); > > This also matches the other error messages we have in this function more > closely. The same is true for the equivalent in the reftable backend. > > Patrick > I do want to also note that there is a old_target set so perhaps modified refs/files-backend.c @@ -2494,8 +2494,9 @@ static int lock_ref_for_update(struct files_ref_store *refs, * fail with an error. */ if (update->old_target) { - strbuf_addf(err, _("cannot update regular ref: '%s': " - "symref target '%s' set"), + strbuf_addf(err, _("cannot lock ref '%s': " + "expected symref with target '%s': " + "but is a regular ref"), ref_update_original_update_refname(update), update->old_target); ret = TRANSACTION_GENERIC_ERROR;
diff --git a/refs/files-backend.c b/refs/files-backend.c index 194e74eb4d..f516d4d82f 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2491,14 +2491,15 @@ static int lock_ref_for_update(struct files_ref_store *refs, /* * Even if the ref is a regular ref, if `old_target` is set, we - * check the referent value. Ideally `old_target` should only - * be set for symrefs, but we're strict about its usage. + * fail with an error. */ if (update->old_target) { - if (ref_update_check_old_target(referent.buf, update, err)) { - ret = TRANSACTION_GENERIC_ERROR; - goto out; - } + strbuf_addf(err, _("cannot update regular ref: '%s': " + "symref target '%s' set"), + ref_update_original_update_refname(update), + 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; diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index b838cf8f00..bd89ce8d76 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -928,6 +928,15 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store, * backend returns, which keeps our tests happy. */ if (u->old_target) { + if (!(u->type & REF_ISSYMREF)) { + strbuf_addf(err, _("cannot update regular ref: '%s': " + "symref target '%s' set"), + ref_update_original_update_refname(u), + u->old_target); + ret = -1; + goto done; + } + if (ref_update_check_old_target(referent.buf, u, err)) { ret = -1; goto done;