Patrick Steinhardt <ps@xxxxxx> writes:
> The `get_worktree_git_dir()` function returns a string constant that
> does not need to be free'd by the caller. For `repo_get_git_dir()` and
> `repo_get_common_dir()` this is because we return strings owned by
> `the_repository`.
>
Not sure what the second sentence is signifying here. What relation does
`get_worktree_git_dir()` have with `repo_get_git_dir()` and
`repo_get_common_dir()`?
> But for `git_common_path()` it's a bit less obvious though, because that
> function does end up allocating memory. This doesn't result in a memory
> leak either because we write into a buffer returned by `get_pathname()`,
> which returns one out of four static buffers.
>
Now `git_common_path()`, what is binding all these functions together?
Sneaking down to the code, it looks like `get_worktree_git_dir()` calls
one of the other three functions. Maybe we should start with that?
>
> We're about to drop `git_common_path()` in favor of `repo_common_path()`,
> which doesn't use the same mechanism but instead returns an allocated
> string owned by the caller. While we could adapt `get_worktree_git_dir()`
> to also use `get_pathname()` and print the derived common path into that
> buffer, the whole schema feels a lot like premature optimization in this
> context. There are some callsites where we call `get_worktree_git_dir()`
> in a loop that iterates through all worktrees. But none of these loops
> seem to be even remotely in the hot path, so saving a single allocation
> there does not feel worth it.
>
> Refactor the function to instead consistently return an allocated path
> so that we can start using `repo_common_path()` in a subsequent commit.
>
This makes sense.
[snip]
> diff --git a/revision.c b/revision.c
> index 474fa1e767..be72f226f3 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1874,15 +1874,20 @@ void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
> for (p = worktrees; *p; p++) {
> struct worktree *wt = *p;
> struct index_state istate = INDEX_STATE_INIT(revs->repo);
> + char *gitdir;
>
Nit: should this be named `wt_gitdir` to stay consistent?
> if (wt->is_current)
> continue; /* current index already taken care of */
>
> + gitdir = get_worktree_git_dir(wt);
> +
> if (read_index_from(&istate,
> worktree_git_path(the_repository, wt, "index"),
> - get_worktree_git_dir(wt)) > 0)
> + gitdir) > 0)
> do_add_index_objects_to_pending(revs, &istate, flags);
> +
> discard_index(&istate);
> + free(gitdir);
> }
> free_worktrees(worktrees);
> }
[snip]
> diff --git a/worktree.c b/worktree.c
> index 8f4fc10c44..3b94535963 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -59,8 +59,9 @@ static void add_head_info(struct worktree *wt)
> static int is_current_worktree(struct worktree *wt)
> {
> char *git_dir = absolute_pathdup(repo_get_git_dir(the_repository));
> - const char *wt_git_dir = get_worktree_git_dir(wt);
> + char *wt_git_dir = get_worktree_git_dir(wt);
Nit: here too, even though it is a pre-existing name. Perhaps it is just
me. So feel free to ignore :)
> int is_current = !fspathcmp(git_dir, absolute_path(wt_git_dir));
> + free(wt_git_dir);
> free(git_dir);
> return is_current;
> }
> @@ -175,14 +176,14 @@ struct worktree **get_worktrees(void)
> return get_worktrees_internal(0);
> }
>
> -const char *get_worktree_git_dir(const struct worktree *wt)
> +char *get_worktree_git_dir(const struct worktree *wt)
> {
> if (!wt)
> - return repo_get_git_dir(the_repository);
> + return xstrdup(repo_get_git_dir(the_repository));
> else if (!wt->id)
> - return repo_get_common_dir(the_repository);
> + return xstrdup(repo_get_common_dir(the_repository));
> else
> - return git_common_path("worktrees/%s", wt->id);
> + return xstrdup(git_common_path("worktrees/%s", wt->id));
> }
>
So this is the crux of the patch, we allocate the string for the first
two function's return value to ensure that `get_worktree_git_dir()`
returns a string which needs to be free'd. Alright!
Attachment:
signature.asc
Description: PGP signature
