From: Justin Tobler <jltobler@gmail.com>
Date: Wed, 17 Sep 2025 15:34:41 GMT
Subject: Re: [PATCH v3 1/4] refs/files: catch conflicts on case-insensitive file-systems
Message-ID: <eiu2y4r3nncfnmsg3ar3jhzya6ge4dnmwqyrkhsinhcjaxd4qn@ehu6l7xngtrg>
In-Reply-To: <CAOLa=ZR1LNs0i5TDPFG_QvUYS3=CMx3ov-68ph6QKNM273JUHQ@mail.gmail.com>

On 25/09/17 12:33AM, Karthik Nayak wrote:
> Justin Tobler <jltobler@gmail.com> writes:
> > On 25/09/13 10:54PM, Karthik Nayak wrote:
> >> +
> >>  /*
> >>   * Lock refname, without following symrefs, and set *lock_p to point
> >>   * at a newly-allocated lock object. Fill in lock->old_oid, referent,
> >> @@ -677,16 +697,17 @@ static void unlock_ref(struct ref_lock *lock)
> >>   * - Generate informative error messages in the case of failure
> >>   */
> >>  static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
> >> -					       struct ref_update *update,
> >> +					       struct ref_transaction *transaction,
> >>  					       size_t update_idx,
> >>  					       int mustexist,
> >>  					       struct string_list *refnames_to_check,
> >> -					       const struct string_list *extras,
> >>  					       struct ref_lock **lock_p,
> >>  					       struct strbuf *referent,
> >>  					       struct strbuf *err)
> >>  {
> >>  	enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
> >> +	struct ref_update *update = transaction->updates[update_idx];
> >> +	const struct string_list *extras = &transaction->refnames;
> >>  	const char *refname = update->refname;
> >>  	unsigned int *type = &update->type;
> >>  	struct ref_lock *lock;
> >> @@ -776,6 +797,9 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
> >>  			goto retry;
> >>  		} else {
> >>  			unable_to_lock_message(ref_file.buf, myerr, err);
> >
> > huh, so if if we have a lockfile error due to a case-insensitve
> > filesystem, does this mean we print the error message from
> > `unable_to_lock_message()` and the new message?
> >
> > If so, I wonder if we would be better off skipping the former since it
> > could be a bit misleading.
> >
> 
> I would say both are necessary. The errors added here are more technical
> and really talk about why we faced an issue. The error in
> 'builtin/fetch.c' is more about guidance to how to overcome that issue.
> 
> Also this error is client agnostic, so we'd add the error here for users
> of both regular transactions and batched updates. The error in
> 'builtin/fetch.c' is very specific to users of 'git-fetch(1)'. So I
> think both hold value.

Ah ok, I was orginally concerned that both error messages would be
printed when a reference update is rejected due to the case conflict
error. I see now that the "Unable to create '%s.lock': %s." message only
gets printed if the transaction actually fails. With this change, a case
conflict error on an individual references in batched updates doesn't
result in the entire transaction failing. Thus, only the new message is
printed. Makes sense.

Thanks,
-Justin