Clarify API for dir.[ch] and unpack-trees.[ch] -- mark relevant fields as internal #1149
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
newren
force-pushed
the
clarify-api
branch
2 times, most recently
from
76dcaf0
to
5926c98
Compare
|
/submit |
|
Submitted as pull.1149.git.1677143700.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): 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 |
|
User |
|
On the Git mailing list, Derrick Stolee wrote (reply to this): On 2/23/2023 10:18 AM, Derrick Stolee wrote:
> 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
...
> 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.
And now that I've read the series in its entirety, I think it is
well organized and does not need any updates. It creates a better
situation than what we already have, and any changes to split the
internal structs to be anonymous to callers can be done as a
follow-up.
Thanks,
-Stolee |
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Thu, Feb 23, 2023 at 7:18 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> 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:
> 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. |
|
User |
|
On the Git mailing list, Elijah Newren wrote (reply to this): On Thu, Feb 23, 2023 at 7:26 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> On 2/23/2023 10:18 AM, Derrick Stolee wrote:
> > 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
> ...
> > 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.
>
> And now that I've read the series in its entirety, I think it is
> well organized and does not need any updates. It creates a better
> situation than what we already have, and any changes to split the
> internal structs to be anonymous to callers can be done as a
> follow-up.
Wow, I was a bit worried pushing a couple dozen patches last night
that it'd be weeks before anyone took a look, and perhaps even that
I'd again get comments that I was pushing too many to the list. You
read and reviewed all of them across both series, including some
comments showing you read pretty carefully, all before I had even
woken up. Very cool; thanks. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@48face1. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Derrick Stolee <derrickstolee@github.com> writes:
> 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.
I actually think the current structure may be a good place to stop
at. Or we could use the original flat structure, but with members
that are supposed to be private prefixed with a longer prefix that
is very specific to the dir.c file, say "private_to_dir_c_".
Then have a block of #define
#define alloc private_to_dir_c_alloc
#define ignored_alloc private_to_dir_c_ignored_alloc
...
#define visited_directories private_to_dir_c_visited_directories
at the beginning dir.c to hide the cruft out of the implementation.
"git grep private_to_dir_c ':!dir.c'" would catch any outsider
peeking into the part of the struct that they shouldn't be touching.
|
|
On the Git mailing list, Jacob Keller wrote (reply to this): On Thu, Feb 23, 2023 at 7:29 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> 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.
>
Another approach, if you don't mind structure pointer math is to
create two structures:
a) the external public one in the public header file
struct dir_entry {
<public stuff>
};
b) a private structure in the source file:
struct dir_entry_private {
struct dir_entry entry;
<private stuff>
};
In the source file you also define a macro/function that can take a
pointer to dir_entry and get a pointer to dir_entry_private:
struct dir_entry_private *dir_entry_to_private(struct dir_entry *entry)
{
return (struct dir_entry_private *)(<calculate the offset that entry
inside dir_entry_private is, then subtract that from entry pointer>))
}
In Linux kernel this is container_of, not sure if git has this already
defined and its a common pattern.
Then you can set the code up such that the only way to allocate a
dir_entry is to call some function in the dir code. If a new entry
needs to be allocated, implement an alloc function that has the full
private structure definition.
This way you don't need an extra field in the dir_entry struct at all,
but at the cost of requiring special allocations. It works great for
code where the only way to get a dir_entry is already some other
functions that would ensure the private version is setup correctly.
> 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.
>
I think either approach is good. I like container_of because I'm quite
used to it in low level kernel code and its a good way to provide
private/public split abstractions there. The private pointer variation
is also a common approach to this problem and I think it sounds like
we already use it in a few places. Its perhaps better for those
reasons.
> Thanks,
> -Stolee |
|
User |
|
This patch series was integrated into seen via git@1ff69ae. |
|
This patch series was integrated into seen via git@f556130. |
|
There was a status update in the "New Topics" section about the branch source: <pull.1149.git.1677143700.gitgitgadget@gmail.com> |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
|
This patch series was integrated into seen via git@2dfd436. |
|
On the Git mailing list, Jonathan Tan wrote (reply to this): "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> I wrote this patch series about a year and a half ago, but never submitted
> it. I rebased and updated it due to [0].
[snip]
> [0] Search for "Extremely yes" in
> https://lore.kernel.org/git/CAJoAoZm+TkCL0Jpg_qFgKottxbtiG2QOiY0qGrz3-uQy+=waPg@mail.gmail.com/
I left some minor comments, but otherwise, this looks good. I do share
the desire to avoid unnecessary refactoring churn, but demarcation
of private and public fields does make code much clearer and can
potentially avoid a whole class of bugs, so I would be happy if this
patch set was merged in. |
|
/submit |
|
Submitted as pull.1149.v2.git.1677291960.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
/submit |
|
Submitted as pull.1149.v3.git.1677511700.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@31d8166. |
|
This patch series was integrated into seen via git@94f4b91. |
|
This patch series was integrated into next via git@434ac6b. |
| @@ -50,10 +50,13 @@ test_expect_success 'checkout commit with dir must not remove untracked a/b' ' | |||
|
|
|||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 2/27/2023 10:28 AM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> test_expect_success SYMLINKS 'the symlink remained' '
>
> - test_when_finished "rm a/b" &&
> test -h a/b
> '
>
> +test_expect_success 'cleanup after previous symlink tests' '
> + rm a/b
> +'
I was confused why this worked without "rm -f a/b" and it seems
the path exists in all cases, it's just a symlink on the filesystem
in the case of the SYMLINKS prerequisite.
Thanks,
-Stolee
|
This patch series was integrated into seen via git@60fe546. |
| @@ -66,4 +69,15 @@ test_expect_success SYMLINKS 'checkout -f must not follow symlinks when removing | |||
| test_path_is_file untracked/f | |||
There was a problem hiding this comment.
On the Git mailing list, Jonathan Tan wrote (reply to this):
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/unpack-trees.c b/unpack-trees.c
> index 3d05e45a279..4518d33ed99 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -2337,7 +2337,7 @@ static int verify_clean_subdirectory(const struct cache_entry *ce,
>
> memset(&d, 0, sizeof(d));
> if (o->dir)
> - d.exclude_per_dir = o->dir->exclude_per_dir;
> + setup_standard_excludes(&d);
> i = read_directory(&d, o->src_index, pathbuf, namelen+1, NULL);
> dir_clear(&d);
> free(pathbuf);
Thanks to the later patches in this patch set, I only needed to look at
unpack-trees.c to see how o->dir (later, o->internal.dir) is set. The
only place it is set is in unpack_trees(), in which a flag is set and
setup_standard_excludes() is called. So the RHS of the diff here does
effectively the same thing as the LHS. (As for the flag, it is not set
in the RHS, but it was not set in the LHS in the first place, so that's
fine.)
Thanks - all 13 patches in this patch set look good.
|
This patch series was integrated into seen via git@a29d3ec. |
|
This patch series was integrated into seen via git@0cc9f30. |
|
There was a status update in the "Cooking" section about the branch Code clean-up to clarify directory traversal API. Will cook in 'next'. source: <pull.1149.v3.git.1677511700.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@20a70af. |
|
This patch series was integrated into seen via git@e00dd1b. |
|
This patch series was integrated into seen via git@835fa4a. |
|
This patch series was integrated into seen via git@68aab2b. |
|
There was a status update in the "Cooking" section about the branch Code clean-up to clarify directory traversal API. Will cook in 'next'. source: <pull.1149.v3.git.1677511700.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@81e6a2d. |
|
This patch series was integrated into seen via git@43274b1. |
|
This patch series was integrated into seen via git@bbe5e3d. |
|
This patch series was integrated into seen via git@d332181. |
|
There was a status update in the "Cooking" section about the branch Code clean-up to clarify directory traversal API. Will cook in 'next'. source: <pull.1149.v3.git.1677511700.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@a792258. |
|
This patch series was integrated into seen via git@f17d232. |
|
This patch series was integrated into master via git@f17d232. |
|
This patch series was integrated into next via git@f17d232. |
|
Closed via f17d232. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes since v2:
Changes since v1 (thanks to Jonathan Tan for the careful reviews!)
Some time ago, I noticed that struct dir_struct and struct unpack_trees_options both have numerous fields meant for internal use only, most of which are not marked as such. This has resulted in callers accidentally trying to initialize some of these fields, and in at least one case required a fair amount of review to verify other changes were okay -- review that would have been simplified with the apriori knowledge that a combination of multiple fields were internal-only[1]. Looking closer, I found that only 6 out of 18 fields in dir_struct were actually meant to be public[2], and noted that unpack_trees_options also had 11 internal-only fields (out of 36).
This patch is primarily about moving internal-only fields within these two structs into an embedded internal struct. Patch breakdown:
To make the benefit more clear, here are compressed versions of dir_struct both before and after the changes. First, before:
And after the changes:
The former version has 18 fields (and 3 magic constants) which API users will have to figure out. The latter makes it clear there are only at most 2 fields you should be setting upon input, and at most 4 which you read at output, and the rest (including all the magic constants) you can ignore.
[0] Search for "Extremely yes" in https://lore.kernel.org/git/CAJoAoZm+TkCL0Jpg_qFgKottxbtiG2QOiY0qGrz3-uQy+=waPg@mail.gmail.com/
[1] https://lore.kernel.org/git/CABPp-BFSFN3WM6q7KzkD5mhrwsz--St_-ej5LbaY8Yr2sZzj=w@mail.gmail.com/
[2] https://lore.kernel.org/git/CABPp-BHgot=CPNyK_xNfog_SqsNPNoCGfiSb-gZoS2sn_741dQ@mail.gmail.com/
cc: Derrick Stolee derrickstolee@github.com
cc: Elijah Newren newren@gmail.com
cc: Jacob Keller jacob.keller@gmail.com
cc: Jonathan Tan jonathantanmy@google.com