Re: [GSoC][PATCH v13 06/10] git refs: add verify subcommand

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jul 30, 2024, 17:56 UTC
Message-ID: <CAPig+cRD3F-QHneC+0bEUcWt4ep8hXgjg4OT_MvfLjQKNE54iA@mail.gmail.com>
In-Reply-To: <ZqkN4wNK0PIRyNky@ArchLinux>

On Tue, Jul 30, 2024 at 11:59 AM shejialuo <shejialuo@gmail.com> wrote:

Show 29 quoted lines

> 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@mail.gmail.com/