On Thu, Feb 27, 2025 at 08:57:01AM +0800, shejialuo wrote:
> On Wed, Feb 26, 2025 at 10:36:29AM -0800, Junio C Hamano wrote:
> > shejialuo <shejialuo@xxxxxxxxx> 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?
I don't have anything else to add to this version, thanks!
Patrick
