Patrick Steinhardt <ps@xxxxxx> writes:
> On Wed, Jun 05, 2024 at 12:29:53PM +0200, Karthik Nayak wrote:
>> From: Karthik Nayak <karthik.188@xxxxxxxxx>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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;
Attachment:
signature.asc
Description: PGP signature
