Re: [PATCH] fsck: ignore missing "refs" directory for linked worktrees

From: shejialuo <shejialuo@gmail.com>
Date: Jun 2, 2025, 12:16 UTC
Message-ID: <aD2WBkxVGilxH5vM@ArchLinux>
In-Reply-To: <b92b5d93-7f7f-4370-ac79-7d9767bb0db5@gmail.com>

On Mon, Jun 02, 2025 at 10:53:50AM +0100, Phillip Wood wrote:

Show 27 quoted lines

> Hi Shejialuo
> 
> On 31/05/2025 04:39, shejialuo wrote:
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index 4d1f65a57a..bf6f89b1d1 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -3762,6 +3762,9 @@ static int files_fsck_refs_dir(struct ref_store *ref_store,
> >   	iter = dir_iterator_begin(sb.buf, 0);
> >   	if (!iter) {
> > +		if (errno == ENOENT && !is_main_worktree(wt))
> > +			goto out;
> > +
> >   		ret = error_errno(_("cannot open directory %s"), sb.buf);
> >   		goto out;
> >   	}
> 
> I think it would be clearer to write this as
> 
> 	if (is_main_worktree(wt) || errno != ENOENT)
> 		ret = error_errno(_("cannot open directory %s"), sb.buf);
> 	goto out;
> 
> so that the condition that triggers the error message is explicit rather
> than having to mentally invert the condition to figure out when we return an
> error
>

I agree with you that by using this way, when reading above code, we could know explicitly in which situation, we would report the error.

Patrick has given his safety concern with reordering the condition check. If `is_main_worktree(wt)` were to modify error (although there is a minor possibility that it would), it could interfere with next errno check.

Besides this, I somehow prefer the short-circuit way. Although in the current code, we only have small code paths after the short-circuit way, this pattern follows a common defensive programming practice where we handle special cases early and exit quickly. This approach reduces nesting and makes the main logic flow cleaner by filtering out edge cases upfront.

So, let's keep this. Really thanks for your suggestion.

> Best Wishes
> 
> Phillip
>

Jialuo