| Message ID | 7c3b545ee5ea3a0e6686594afe582fa1a19929f6.1718899877.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sparse-index: improve clear_skip_worktree_from_present_files() | expand |
On Thu, Jun 20, 2024 at 10:11 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <stolee@gmail.com> > > In advance of changing the behavior of path_found(), take all of the > intermediate data values and group them into a single struct. This > simplifies the method prototype as well as the initialization. Future > changes can be made directly to the struct and method without changing > the callers with this approach. > > Note that the clear_path_found_data() method is currently empty, as > there is nothing to free. However, this will change in the future, so > place the method and its callers for now. I can't parse the second half of the final sentence. What was meant here? > Signed-off-by: Derrick Stolee <stolee@gmail.com> > --- > sparse-index.c | 45 +++++++++++++++++++++++++++++---------------- > 1 file changed, 29 insertions(+), 16 deletions(-) > > diff --git a/sparse-index.c b/sparse-index.c > index e0457c87fff..de6e727f5c1 100644 > --- a/sparse-index.c > +++ b/sparse-index.c > @@ -439,8 +439,22 @@ void ensure_correct_sparsity(struct index_state *istate) > ensure_full_index(istate); > } > > -static int path_found(const char *path, const char **dirname, size_t *dir_len, > - int *dir_found) > +struct path_found_data { > + const char *dirname; > + size_t dir_len; > + int dir_found; > +}; > + > +#define PATH_FOUND_DATA_INIT { \ > + .dir_found = 1 \ > +} > + > +static void clear_path_found_data(struct path_found_data *data) > +{ > + return; > +} > + > +static int path_found(const char *path, struct path_found_data *data) > { > struct stat st; > char *newdir; > @@ -450,7 +464,7 @@ static int path_found(const char *path, const char **dirname, size_t *dir_len, > * If dirname corresponds to a directory that doesn't exist, and this > * path starts with dirname, then path can't exist. > */ > - if (!*dir_found && !memcmp(path, *dirname, *dir_len)) > + if (!data->dir_found && !memcmp(path, data->dirname, data->dir_len)) > return 0; > > /* > @@ -472,15 +486,16 @@ static int path_found(const char *path, const char **dirname, size_t *dir_len, > * If path starts with directory (which we already lstat'ed and found), > * then no need to lstat parent directory again. > */ > - if (*dir_found && *dirname && memcmp(path, *dirname, *dir_len)) > + if (data->dir_found && data->dirname && > + memcmp(path, data->dirname, data->dir_len)) > return 0; > > /* Free previous dirname, and cache path's dirname */ > - *dirname = path; > - *dir_len = newdir - path + 1; > + data->dirname = path; > + data->dir_len = newdir - path + 1; > > - tmp = xstrndup(path, *dir_len); > - *dir_found = !lstat(tmp, &st); > + tmp = xstrndup(path, data->dir_len); > + data->dir_found = !lstat(tmp, &st); > free(tmp); > > return 0; > @@ -488,9 +503,7 @@ static int path_found(const char *path, const char **dirname, size_t *dir_len, > > static int clear_skip_worktree_from_present_files_sparse(struct index_state *istate) > { > - const char *last_dirname = NULL; > - size_t dir_len = 0; > - int dir_found = 1; > + struct path_found_data data = PATH_FOUND_DATA_INIT; > > int path_count = 0; > int to_restart = 0; > @@ -502,7 +515,7 @@ static int clear_skip_worktree_from_present_files_sparse(struct index_state *ist > > if (ce_skip_worktree(ce)) { > path_count++; > - if (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) { > + if (path_found(ce->name, &data)) { > if (S_ISSPARSEDIR(ce->ce_mode)) { > to_restart = 1; > break; > @@ -516,14 +529,13 @@ static int clear_skip_worktree_from_present_files_sparse(struct index_state *ist > "sparse_path_count", path_count); > trace2_region_leave("index", "clear_skip_worktree_from_present_files_sparse", > istate->repo); > + clear_path_found_data(&data); > return to_restart; > } > > static void clear_skip_worktree_from_present_files_full(struct index_state *istate) > { > - const char *last_dirname = NULL; > - size_t dir_len = 0; > - int dir_found = 1; > + struct path_found_data data = PATH_FOUND_DATA_INIT; > > int path_count = 0; > > @@ -537,7 +549,7 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista > > if (ce_skip_worktree(ce)) { > path_count++; > - if (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) > + if (path_found(ce->name, &data)) > ce->ce_flags &= ~CE_SKIP_WORKTREE; > } > } > @@ -546,6 +558,7 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista > "full_path_count", path_count); > trace2_region_leave("index", "clear_skip_worktree_from_present_files_full", > istate->repo); > + clear_path_found_data(&data); > } > > void clear_skip_worktree_from_present_files(struct index_state *istate) > -- > gitgitgadget It's surprising how much a simple, straightforward translation of the code can improve its readability; I should have done it this way all along.
On 6/24/24 6:13 PM, Elijah Newren wrote: > On Thu, Jun 20, 2024 at 10:11 AM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Derrick Stolee <stolee@gmail.com> >> >> In advance of changing the behavior of path_found(), take all of the >> intermediate data values and group them into a single struct. This >> simplifies the method prototype as well as the initialization. Future >> changes can be made directly to the struct and method without changing >> the callers with this approach. >> >> Note that the clear_path_found_data() method is currently empty, as >> there is nothing to free. However, this will change in the future, so >> place the method and its callers for now. > > I can't parse the second half of the final sentence. What was meant here? I was trying to point out how this method looks a bit silly: >> +static void clear_path_found_data(struct path_found_data *data) >> +{ >> + return; >> +} It will be filled in with a meaningful implementation later on, but I wanted to establish its callers here. I'll try to find a better way to communicate this. Thanks, -Stolee
diff --git a/sparse-index.c b/sparse-index.c index e0457c87fff..de6e727f5c1 100644 --- a/sparse-index.c +++ b/sparse-index.c @@ -439,8 +439,22 @@ void ensure_correct_sparsity(struct index_state *istate) ensure_full_index(istate); } -static int path_found(const char *path, const char **dirname, size_t *dir_len, - int *dir_found) +struct path_found_data { + const char *dirname; + size_t dir_len; + int dir_found; +}; + +#define PATH_FOUND_DATA_INIT { \ + .dir_found = 1 \ +} + +static void clear_path_found_data(struct path_found_data *data) +{ + return; +} + +static int path_found(const char *path, struct path_found_data *data) { struct stat st; char *newdir; @@ -450,7 +464,7 @@ static int path_found(const char *path, const char **dirname, size_t *dir_len, * If dirname corresponds to a directory that doesn't exist, and this * path starts with dirname, then path can't exist. */ - if (!*dir_found && !memcmp(path, *dirname, *dir_len)) + if (!data->dir_found && !memcmp(path, data->dirname, data->dir_len)) return 0; /* @@ -472,15 +486,16 @@ static int path_found(const char *path, const char **dirname, size_t *dir_len, * If path starts with directory (which we already lstat'ed and found), * then no need to lstat parent directory again. */ - if (*dir_found && *dirname && memcmp(path, *dirname, *dir_len)) + if (data->dir_found && data->dirname && + memcmp(path, data->dirname, data->dir_len)) return 0; /* Free previous dirname, and cache path's dirname */ - *dirname = path; - *dir_len = newdir - path + 1; + data->dirname = path; + data->dir_len = newdir - path + 1; - tmp = xstrndup(path, *dir_len); - *dir_found = !lstat(tmp, &st); + tmp = xstrndup(path, data->dir_len); + data->dir_found = !lstat(tmp, &st); free(tmp); return 0; @@ -488,9 +503,7 @@ static int path_found(const char *path, const char **dirname, size_t *dir_len, static int clear_skip_worktree_from_present_files_sparse(struct index_state *istate) { - const char *last_dirname = NULL; - size_t dir_len = 0; - int dir_found = 1; + struct path_found_data data = PATH_FOUND_DATA_INIT; int path_count = 0; int to_restart = 0; @@ -502,7 +515,7 @@ static int clear_skip_worktree_from_present_files_sparse(struct index_state *ist if (ce_skip_worktree(ce)) { path_count++; - if (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) { + if (path_found(ce->name, &data)) { if (S_ISSPARSEDIR(ce->ce_mode)) { to_restart = 1; break; @@ -516,14 +529,13 @@ static int clear_skip_worktree_from_present_files_sparse(struct index_state *ist "sparse_path_count", path_count); trace2_region_leave("index", "clear_skip_worktree_from_present_files_sparse", istate->repo); + clear_path_found_data(&data); return to_restart; } static void clear_skip_worktree_from_present_files_full(struct index_state *istate) { - const char *last_dirname = NULL; - size_t dir_len = 0; - int dir_found = 1; + struct path_found_data data = PATH_FOUND_DATA_INIT; int path_count = 0; @@ -537,7 +549,7 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista if (ce_skip_worktree(ce)) { path_count++; - if (path_found(ce->name, &last_dirname, &dir_len, &dir_found)) + if (path_found(ce->name, &data)) ce->ce_flags &= ~CE_SKIP_WORKTREE; } } @@ -546,6 +558,7 @@ static void clear_skip_worktree_from_present_files_full(struct index_state *ista "full_path_count", path_count); trace2_region_leave("index", "clear_skip_worktree_from_present_files_full", istate->repo); + clear_path_found_data(&data); } void clear_skip_worktree_from_present_files(struct index_state *istate)