On Thu, Feb 06, 2025 at 11:15:58PM -0800, Karthik Nayak wrote:
> 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()`?
These are getting called in `get_worktree_git_dir()`. I'll rephrase the
message to make this clearer.
> > 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?
Yeah, I've reworded the commit message accordingly.
> > 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?
Makes sense.
> > 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 :)
I'll leave that one as-is, even though I share your sentiment.
Patrick
