Re: [PATCH 09/16] worktree: return allocated string from `get_worktree_git_dir()`

[Patrick Steinhardt <ps@pks.im>]

On Thu, Feb 06, 2025 at 11:15:58PM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> 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