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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 19, 2021, 16:16 UTC
Message-ID: <xmqqk0lmnury.fsf@gitster.g>
In-Reply-To: <patch-02.11-033c0cec33d-20210716T140631Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> 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);

Show 18 quoted lines

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  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,