Hi Karthik
On 05/05/2024 17:09, Karthik Nayak wrote:
Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
On 03/05/2024 13:41, Karthik Nayak wrote:
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -938,7 +940,22 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
* individual refs. But the error messages match what the files
* backend returns, which keeps our tests happy.
*/
- if (u->flags & REF_HAVE_OLD && !oideq(¤t_oid, &u->old_oid)) {
+ if (u->old_target) {
+ if (strcmp(referent.buf, u->old_target)) {
+ if (!strcmp(referent.buf, ""))
+ strbuf_addf(err, "verifying symref target: '%s': "
+ "reference is missing but expected %s",
+ original_update_refname(u),
+ u->old_target);
+ else
+ strbuf_addf(err, "verifying symref target: '%s': "
+ "is at %s but expected %s",
+ original_update_refname(u),
+ referent.buf, u->old_target);
The messages in this function differ from the equivalent messages in
check_old_target() from the files backend above. This is potentially
confusing to users, creates more work for translators and makes it hard
to write tests that are independent of the backend. Can we export
check_old_target() so it can be reused here. If not we should reword
these messages to match the other messages all of which talk about not
being able to lock the ref.
This is very intentional, the way the backends work at this point are
quite different and while in the files backend, we talk about locking a
particular ref.
I agree that the existing messages could be improved - these messages
are returned when checking the old value of the ref so talking about
being unable to lock the ref is not helpful as the important information
is that the old value does not match the expected value. However that is
not dependent on the backend or on whether the expected value is a
symref or an oid so it feels a bit random to make these two messages
different.
In the reftable backend we do not lock single refs. We
lock tables. So keeping it consistent doesn't make sense here.
Where an update is prevented by another process holding a lock I think
that the granularity of the lock that prevents the ref from being
updated is not particularly relevant as far as the user is concerned. As
far as I can see the existing error messages in the reftable backend try
to be consistent with the messages in the files backend.
However, we could make the files backend similar to this one, I would be
okay doing that.
I would be very happy to see the messages improved for both backends
when the old value does not match the expected (oid or symref) value. I
do think we should have consistent error messages in this case that are
essentially independent of the backend and type of the expected value.
Best Wishes
Phillip
