Subject: Re: [PATCH v5 04/16] refs: introduce function to batch refname availability checks

From: Patrick Steinhardt <[email-masked]>
Date: Wed, 12 Mar 2025 16:36:33 +0100

On Wed, Mar 12, 2025 at 08:36:47PM +0800, shejialuo wrote:
> On Thu, Mar 06, 2025 at 04:08:35PM +0100, Patrick Steinhardt wrote:
> > diff --git a/refs.c b/refs.c
> > index f4094a326a9..5a9b0f2fa1e 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -2489,79 +2485,91 @@ int refs_verify_refname_available(struct ref_store *refs,
> >  
> >  	assert(err);
> >  
> > -	strbuf_grow(&dirname, strlen(refname) + 1);
> > -	for (slash = strchr(refname, '/'); slash; slash = strchr(slash + 1, '/')) {
> > -		/*
> > -		 * Just saying "Is a directory" when we e.g. can't
> > -		 * lock some multi-level ref isn't very informative,
> > -		 * the user won't be told *what* is a directory, so
> > -		 * let's not use strerror() below.
> > -		 */
> > -		int ignore_errno;
> > -		/* Expand dirname to the new prefix, not including the trailing slash: */
> > -		strbuf_add(&dirname, refname + dirname.len, slash - refname - dirname.len);
> > +	for (size_t i = 0; i < refnames->nr; i++) {
> 
> Nit: we may just use `for_each_string_list_item` instead of use the raw
> "for" loop.

Fair, can do.

> > diff --git a/refs.h b/refs.h
> > index a0cdd99250e..185aed5a461 100644
> > --- a/refs.h
> > +++ b/refs.h
> > @@ -124,6 +124,18 @@ int refs_verify_refname_available(struct ref_store *refs,
> >  				  unsigned int initial_transaction,
> >  				  struct strbuf *err);
> >  
> > +/*
> > + * Same as `refs_verify_refname_available()`, but checking for a list of
> > + * refnames instead of only a single item. This is more efficient in the case
> > + * where one needs to check multiple refnames.
> > + */
> 
> Should we talk about more about why this is more efficient?

I don't think the caller needs to be aware of why specifically it is
faster. All they should care for is that it does the same than the other
function, but that it knows to optimize better.

Patrick