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

[Derrick Stolee <derrickstolee@github.com>]

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.

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.

Thanks,
-Stolee