Subject: Re: [PATCH v2 02/11] refs/files: remove unused REF_DELETING in lock_ref_oid_basic()

From: Junio C Hamano <[email-masked]>
Date: Mon, 19 Jul 2021 09:16:49 -0700

Ævar Arnfjörð Bjarmason  <[email-masked]> writes:

> The lock_ref_oid_basic() function has gradually been replaced by use
> of the file transaction API, there are only 4 remaining callers of
> it.

Peff mentioned "file, not ref?", and I wonder about the same thing.

Inside the implementation of ref transaction (e.g. the codepath that
calls lock_ref_for_update() from files_transaction_prepare()), we do
end up calling the lockfile API and you could (but I'd prefer not to
see you do so) call it "file transaction API", but I think you meant
that most callers no longer perform low-level "acquire lock, update
and release" and instead use the ref transaction.

> None of those callers pass REF_DELETING, the last such caller went
> away in 8df4e511387 (struct ref_update: move "have_old" into "flags",
> 2015-02-17). This is the start of even more removal of unused code in
> and around this function.

While I agree that no existing calls to lock_ref_oid_basic() pass
REF_DELETING to it (hence this patch is a benign no-op), inside
ref_transaction_commit(), 8df4e511387 still used REF_DELETING, I
think, like so:

	/* Acquire all locks while verifying old values */
	for (i = 0; i < n; i++) {
		struct ref_update *update = updates[i];
		unsigned int flags = update->flags;

		if (is_null_sha1(update->new_sha1))
			flags |= REF_DELETING;
		update->lock = lock_ref_sha1_basic(
				update->refname,
				((update->flags & REF_HAVE_OLD) ?
				 update->old_sha1 : NULL),
				NULL,
				flags,
				&update->type);

> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
> ---
>  refs/files-backend.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 677b7e4cdd2..326f0224218 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -934,8 +934,6 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
>  
>  	if (mustexist)
>  		resolve_flags |= RESOLVE_REF_READING;
> -	if (flags & REF_DELETING)
> -		resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
>  
>  	files_ref_path(refs, &ref_file, refname);
>  	resolved = !!refs_resolve_ref_unsafe(&refs->base,