On Tue, Jul 30, 2024 at 11:59 AM shejialuo <shejialuo@xxxxxxxxx> wrote:
> On Tue, Jul 30, 2024 at 10:31:37AM +0200, Patrick Steinhardt wrote:
> > On Mon, Jul 29, 2024 at 09:27:12PM +0800, shejialuo wrote:
> > > + /*
> > > + * Explicitly free the allocated array and "skip_oids" set
> > > + */
> > > + free(fsck_refs_options.msg_type);
> > > + oidset_clear(&fsck_refs_options.skip_oids);
> >
> > Should we provide a `fsck_options_clear()` function that does this for
> > us? Otherwise we'll have to adapt callsites of `refs_fsck` whenever
> > internal implementation details of the subsystem add newly allocated
> > members.
> [...]
> But how does "fsck.c" free "skip_oids", actually "fsck.c" never frees
> "skip_oids". This is because "git-fsck(1)" defines the following:
>
> static struct fsck_options fsck_walk_options = FSCK_OPTIONS_DEFAULT;
> static struct fsck_options fsck_obj_options = FSCK_OPTIONS_DEFAULT;
>
> Because these two options are "static", so there is no memory leak. We
> leave it to the operating system. So maybe a more simple way is just to
> add "static" identifier in "cmd_refs_verify" which means:
>
> - struct fsck_options fsck_refs_options = FSCK_REFS_OPTIONS_DEFAULT;
> + static struct fsck_options fsck_refs_options = FSCK_REFS_OPTIONS_DEFAULT;
>
> But I don't think we should use `static`, because Eric has told me that
> making a variable "static" will make the code harder to "libfy". So
> let's use "fsck_options_clear" function instead.
I haven't been following this topic closely and I'm not familiar with
this code (and don't have much time now to dig into it), but I suspect
the context here is rather different from the one[*] in which I was
highly skeptical of the use of `static`. The `static` in that earlier
case was suspicious/questionable for two reasons. First, it was a case
of premature optimization (which, by definition, is frowned upon).
Second, it was in a "library" function (namely, top-level
fsck.c:fsck_refs_error_function()) which may someday become a linkable
library which other programs (aside from `git` itself) may utilize.
Having a static strbuf in the library function makes the function
non-reentrant and takes memory management out of the hands of the
client.
In the case under discussion here (namely `builtin/fsck.c`), it is a
Git-specific command, not library code. As such, "libification" is
much less of an issue since Git-specific command code is less likely
to be reused by some other project. (However, that's not to say that
we shouldn't worry about unnecessary use of `static` even in builtin
commands; code from those commands does periodically migrate from
`builtin/*.c` to top-level library oriented `*.c`.)
So, considering that the variable under discussion:
struct fsck_options fsck_refs_options = FSCK_REFS_OPTIONS_DEFAULT;
is part of a builtin command rather than library code, we don't have
to worry about "libification" so much, thus making it `static` would
be a workable approach. However, doing so merely to avoid complaint by
the leak-checker does not seem like good justification. Hence, keeping
this variable non-static and freeing it explicitly seems a better idea
(which is what this code does presently).
I do agree with Patrick that adding fsck_options_clear() to top-level
`fsck.h` would be sensible since it frees callers from having to know
implementation details of `fsck_options`.
By the way, regarding the static `fsck_walk_options` and
`fsck_obj_options` those are probably global static for convenience
rather than out of necessity. It might very well be possible to make
those local variables in builtin/fsck.c:cmd_fsck() and then plumb them
through to called functions so that they don't have to be static, and
then they would be freed manually by cmd_fsck(), as well. However,
that sort of change is well outside the scope of this topic.
[*] https://lore.kernel.org/git/CAPig+cR=RgMeaAy1PRGgHu6_Ak+7=_-5tGvBZRekKRxi7GtdHw@xxxxxxxxxxxxxx/
