Re: [PATCH 00/11] Clarify API for dir.[ch] and unpack-trees.[ch] -- mark relevant fields as internal

From: Elijah Newren <newren@gmail.com>
Date: Feb 23, 2023, 20:31 UTC
Message-ID: <CABPp-BF06RLazEjNfe0E=arM+q=dw9r478Q1ZLMeY-OxUf2CLg@mail.gmail.com>
In-Reply-To: <16ff5069-0408-21cd-995c-8b47afb9810d@github.com>

On Thu, Feb 23, 2023 at 7:18 AM Derrick Stolee <derrickstolee@github.com> wrote:

Show 69 quoted lines

>
> On 2/23/2023 4:14 AM, Elijah Newren via GitGitGadget wrote:
> > This patch is primarily about moving internal-only fields within these two
> > structs into an embedded internal struct. Patch breakdown:
> >
> >  * Patches 1-3: Restructuring dir_struct
> >    * Patch 1: Splitting off internal-use-only fields
> >    * Patch 2: Add important usage note to avoid accidentally using
> >      deprecated API
> >    * Patch 3: Mark output-only fields as such
> >  * Patches 4-11: Restructuring unpack_trees_options
> >    * Patches 4-6: Preparatory cleanup
> >    * Patches 7-10: Splitting off internal-use-only fields
> >    * Patch 11: Mark output-only field as such
>
> > And after the changes:
> >
> > struct dir_struct {
> >     enum [...] flags;
> >     int nr; /* output only */
> >     int ignored_nr; /* output only */
> >     struct dir_entry **entries; /* output only */
> >     struct dir_entry **ignored; /* output only */
> >     struct untracked_cache *untracked;
> >     const char *exclude_per_dir; /* deprecated */
> >     struct dir_struct_internal {
> >         int alloc;
> >         int ignored_alloc;
> > #define EXC_CMDL 0
> > #define EXC_DIRS 1
> > #define EXC_FILE 2
> >         struct exclude_list_group exclude_list_group[3];
> >         struct exclude_stack *exclude_stack;
> >         struct path_pattern *pattern;
> >         struct strbuf basebuf;
> >         struct oid_stat ss_info_exclude;
> >         struct oid_stat ss_excludes_file;
> >         unsigned unmanaged_exclude_files;
> >         unsigned visited_paths;
> >         unsigned visited_directories;
> >     } internal;
> > };
>
> This does present a very clear structure to avoid callers being
> confused when writing these changes. It doesn't, however, present
> any way to guarantee that callers can't mutate this state.
>
> ...here I go on a side track thinking of an alternative...
>
> One way to track this would be to anonymously declare 'struct
> dir_struct_internal' in the header file and let 'struct dir_struct'
> contain a _pointer_ to the internal struct. The dir_struct_internal
> can then be defined inside the .c file, limiting its scope. (It
> must be a pointer in dir_struct or else callers would not be able
> to create a dir_struct without using a pointer and an initializer
> method.
>
> The major downside to this pointer approach is that the internal
> struct needs to be initialized within API calls and somehow cleared
> by all callers. The internal data could be initialized by the common
> initializers read_directory() or fill_directory(). There is a
> dir_clear() that _should_ be called by all callers (but I notice we
> are leaking the struct in at least one place in add-interactive.c,
> and likely others).
>
> This alternative adds some complexity to the structure, but
> provides compiler-level guarantees that these internals are not used
> outside of dir.c. I thought it worth exploring, even if we decide
> that the complexity is not worth those guarantees.

In addition to the guarantees that others won't muck with those fields, such an approach would also buy us the following:

  * The implementation can change and internal data structures be
modified/extended/appended-to, without requiring recompiling all files
that depend upon our header.
  * Related to the last point, the ABI doesn't change when the
implementation and internal data structures change.  Might be helpful
for libification efforts.

For all three of these reasons, I pursued this alternate strategy you bring up in merge-recursive and merge-ort (they both use a "struct merge_options_internal *priv" and then define _very_ different "struct merge_options_iternal" in their respective C files). I thought about using this strategy you suggest here, but was worried at the time I created this patch that it might create more friction for Ævar who was pushing his struct initialization work and memory leak cleanups in the same area heavily at the time (and back then we had a few long threads back and forth because our work was overlapping and clashing slightly). But I figured that doing at least this much was good, because of what you point out:

Show 5 quoted lines

> The best news is that your existing series makes it easier to flip
> to the internal pointer method in the future, since we can shift
> the 'd->internal.member" uses into "d->internal->member" in a
> mechanical way. Thus, the change you are proposing does not lock us
> into this approach if we change our minds later.