Subject: Re: [PATCH v7 3/9] packed-backend: check whether the "packed-refs" is regular file

From: shejialuo <[email-masked]>
Date: Thu, 27 Feb 2025 08:57:01 +0800

On Wed, Feb 26, 2025 at 10:36:29AM -0800, Junio C Hamano wrote:
> shejialuo <[email-masked]> writes:
> 
> > +static int packed_fsck(struct ref_store *ref_store,
> > +		       struct fsck_options *o,
> >  		       struct worktree *wt)
> >  {
> > +	struct packed_ref_store *refs = packed_downcast(ref_store,
> > +							REF_STORE_READ, "fsck");
> > +	struct stat st;
> > +	int ret = 0;
> > +	int fd;
> >  
> >  	if (!is_main_worktree(wt))
> >  		return 0;
> 
> I do not think it is worth a reroll only to improve this one, but
> for future reference, initializing "fd = -1" and jumping to cleanup
> here instead of "return 0" would future-proof the code better.  This
> is especially so, given that in a few patches later, we would add a
> strbuf that is initialized before this "we do not do anything
> outside the primary worktree" short-cut, and many "goto cleanup"s we
> see in this patch below would jump to cleanup to strbuf_release() on
> that initialized but unused strbuf.  Jumping there with negative fd
> to cleanup that already avoids close(fd) for negative fd would be
> like jumping there with initialized but unused strbuf.  Having a
> single exit point ("cleanup:" label) would help future evolution of
> the code, by making it easier to add more resource-acquriing code to
> this function in the future.
> 

You are right. Actually, I just want to avoid assigning the `fd` to -1.
However, I didn't realize that I would initialize the strbuf later.
After waking up, I have suddenly realized this problem.

If other reviewers don't have any comments for this new version, I will
send out a reroll. We have already iterated many times, if we could make
it better, why not?

Thanks,
Jialuo