Re: [PATCH v2 11/11] refs/files: remove unused "errno == EISDIR" code

From: Jeff King <peff@peff.net>
Date: Jul 17, 2021, 02:30 UTC
Message-ID: <YPJAx1AIkMhN/dgV@coredump.intra.peff.net>
In-Reply-To: <patch-11.11-96c3e5e9f79-20210716T140631Z-avarab@gmail.com>

On Fri, Jul 16, 2021 at 04:13:07PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 7 quoted lines

> Further historical commentary:
> 
> Before the two preceding commits the caller in files_reflog_expire()
> was the only one out of our 4 callers that would pass non-NULL as an
> oid. We would then set a (now gone) "resolve_flags" to
> "RESOLVE_REF_READING" and just before that "errno != EISDIR" check do:
> [...]

Right, thanks for digging into this situation in the commit message.

Show 31 quoted lines

> @@ -891,30 +890,9 @@ static struct ref_lock *lock_ref_oid_basic(struct files_ref_store *refs,
>  	CALLOC_ARRAY(lock, 1);
>  
>  	files_ref_path(refs, &ref_file, refname);
> -	resolved = !!refs_resolve_ref_unsafe(&refs->base, refname,
> -					     RESOLVE_REF_NO_RECURSE,
> -					     &lock->old_oid, type);
> -	if (!resolved && errno == EISDIR) {
> -		/*
> -		 * we are trying to lock foo but we used to
> -		 * have foo/bar which now does not exist;
> -		 * it is normal for the empty directory 'foo'
> -		 * to remain.
> -		 */
> -		if (remove_empty_directories(&ref_file)) {
> -			last_errno = errno;
> -			if (!refs_verify_refname_available(
> -					    &refs->base,
> -					    refname, NULL, NULL, err))
> -				strbuf_addf(err, "there are still refs under '%s'",
> -					    refname);
> -			goto error_return;
> -		}
> -		resolved = !!refs_resolve_ref_unsafe(&refs->base, refname,
> -						     RESOLVE_REF_NO_RECURSE,
> -						     &lock->old_oid, type);
> -	}
> -	if (!resolved) {
> +	if (!refs_resolve_ref_unsafe(&refs->base, refname,
> +				     RESOLVE_REF_NO_RECURSE,
> +				     &lock->old_oid, type)) {

I agree that this is all trivially dead code now, and can be removed.

>  		last_errno = errno;
>  		if (last_errno != ENOTDIR ||
>  		    !refs_verify_refname_available(&refs->base, refname,

Not necessary, but I think "last_errno != ENOTDIR" will always be true now, too. We treat it the same as EISDIR in refs_resolve_ref_unsafe(). It's not that big a cleanup, but it could easily go on top.

-Peff