Re: [PATCH v6 3/9] ref: initialize target name outside of check functions

[Patrick Steinhardt <ps@pks.im>]

On Mon, Oct 21, 2024 at 09:34:31PM +0800, shejialuo wrote:
> We passes "refs_check_dir" to the "files_fsck_refs_name" function which
> allows it to create the checked ref name later. However, when we
> introduce a new check function, we have to re-calculate the target name.
> It's bad for us to do repeat calculation. Instead, we should calculate
> it only once and pass the target name to the check functions.

It would be nice to clarify what exactly is bad about it. Does it create
extra memory churn? Or is this about not duplicating logic?

> In order not to do repeat calculation, rename "refs_check_dir" to
> "target_name". And in "files_fsck_refs_dir", create a new strbuf
> "target_name", thus whenever we handle a new target, calculate the
> name and call the check functions one by one.

"target_name" is somewhat of a weird name. I'd expect that this is
either the path to the reference, in which case I'd call this "path", or
the name of the reference that is to be checked, in which case I'd call
this "refname".

> @@ -3539,6 +3538,7 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
>  			       const char *refs_check_dir,
>  			       files_fsck_refs_fn *fsck_refs_fn)
>  {
> +	struct strbuf target_name = STRBUF_INIT;
>  	struct strbuf sb = STRBUF_INIT;
>  	struct dir_iterator *iter;
>  	int iter_status;
> @@ -3557,11 +3557,15 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
>  			continue;
>  		} else if (S_ISREG(iter->st.st_mode) ||
>  			   S_ISLNK(iter->st.st_mode)) {
> +			strbuf_reset(&target_name);
> +			strbuf_addf(&target_name, "%s/%s", refs_check_dir,
> +				    iter->relative_path);
> +
>  			if (o->verbose)
> -				fprintf_ln(stderr, "Checking %s/%s",
> -					   refs_check_dir, iter->relative_path);
> +				fprintf_ln(stderr, "Checking %s", target_name.buf);
> +
>  			for (size_t i = 0; fsck_refs_fn[i]; i++) {
> -				if (fsck_refs_fn[i](ref_store, o, refs_check_dir, iter))
> +				if (fsck_refs_fn[i](ref_store, o, target_name.buf, iter))
>  					ret = -1;
>  			}
>  		} else {

The change itself does make sense though. We indeed avoid reallocating
the array for every single ref, which is a worthwhile change.

I was wondering whether we could reuse `sb` here, but we do use it at
the end of the function to potentially print an error message.

Patrick