Re: [PATCH 03/12] refs: refactor logic to look up storage backends

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2044 bytes --]

On Fri, Dec 22, 2023 at 04:38:30AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > +
> > +const char *ref_storage_format_to_name(int ref_storage_format)
> > +{
> > +	const struct ref_storage_be *be = find_ref_storage_backend(ref_storage_format);
> > +	if (!be)
> > +		return "unknown";
> > +	return be->name;
> > +}
> >
> 
> Would it make more sense to return NULL here?

The only purpose of this function will be to print error messages, so
"unknown" seems like the better choice.

> >  /*
> >   * How to handle various characters in refnames:
> >   * 0: An acceptable character for refs
> > @@ -2029,12 +2045,12 @@ static struct ref_store *ref_store_init(struct repository *repo,
> >  					const char *gitdir,
> >  					unsigned int flags)
> >  {
> > -	const char *be_name = "files";
> > -	struct ref_storage_be *be = find_ref_storage_backend(be_name);
> > +	int format = REF_STORAGE_FORMAT_FILES;
> > +	const struct ref_storage_be *be = find_ref_storage_backend(format);
> >  	struct ref_store *refs;
> >
> >  	if (!be)
> > -		BUG("reference backend %s is unknown", be_name);
> > +		BUG("reference backend %s is unknown", ref_storage_format_to_name(format));
> >
> 
> This doesn't really get us more information, since be == NULL, calling
> `ref_storage_format_to_name` will again call `find_ref_storage_backend`,
> which will be NULL. I wonder if its better to:
> 
> @@ -2029,12 +2045,12 @@ static struct ref_store *ref_store_init(struct
> repository *repo,
>                      const char *gitdir,
>                      unsigned int flags)
>   {
>  -	const char *be_name = "files";
>  -	struct ref_storage_be *be = find_ref_storage_backend(be_name);
>  +	int format = REF_STORAGE_FORMAT_FILES;
>  +	const struct ref_storage_be *be = find_ref_storage_backend(format);
>      struct ref_store *refs;
> 
>      if (!be)
>  -		BUG("reference backend %s is unknown", be_name);
>  +		BUG("reference backend is unknown");

Good point, will change.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]