Re: [PATCH 01/16] path: refactor `repo_common_path()` family of functions

From: Patrick Steinhardt <ps@pks.im>
Date: Feb 7, 2025, 06:16 UTC
Message-ID: <Z6WlMvasi64N3dQQ@pks.im>
In-Reply-To: <Z6TFZGNjs3Xfkh9x@ArchLinux>

On Thu, Feb 06, 2025 at 10:21:24PM +0800, shejialuo wrote:

Show 26 quoted lines

> On Thu, Feb 06, 2025 at 08:57:57AM +0100, Patrick Steinhardt wrote:
> > The functions provided by the "path" subsystem to derive repository
> > paths for the commondir, gitdir, worktrees and submodules are quite
> > inconsistent. Some functions have a `strbuf_` prefix, others have
> > different return values, some don't provide a variant working on top of
> > `strbuf`s.
> > 
> > We're thus about to refactor all of these family of functions so that
> > they follow a common pattern:
> > 
> >   - `repo_*_path()` returns an allocated string.
> > 
> >   - `repo_*_path_append()` appends the path to the caller-provided
> >     buffer while returning a constant pointer to the buffer. This
> >     clarifies whether the buffer is being appended to or rewritten,
> >     which otherwise wasn't immediately obvious.
> > 
> >   - `repo_*_path_replace()` replaces contents of the buffer with the
> >     computed path, again returning a pointer to the buffer contents.
> > 
> 
> I want to ask a design question about this. Why do we need to return the
> raw pointer to the `struct strbuf` for the last two cases? I somehow
> understand why you want to do this. You want to follow a common pattern
> for those three functions. But I wonder should we let the caller to
> decide whether they want to use the raw pointer?

It allows patterns like this:

    if (stat(&st, repo_common_path_replace(...)) ||
        unlink(&st, repo_common_path_replace(...)))
            ...

So the reason is not that I want to follow a common pattern, the reason is that it's useful to some callers.

> And in this patch, the return value of the last two cases has never been
> used. Until I read the next patch, I have seen the usage of the return
> value thus I could understand your motivation.

Yeah, that's fair. I'll adapt the commit message to explain this better.

Show 18 quoted lines

> > diff --git a/path.h b/path.h
> > index 5f6c85e5f8..3c75495e1a 100644
> > --- a/path.h
> > +++ b/path.h
> > @@ -243,6 +241,12 @@ struct strbuf *get_pathname(void);
> >  #  include "strbuf.h"
> >  #  include "repository.h"
> >  
> > +/* Internal implementation detail that should not be used. */
> > +void repo_common_pathv(const struct repository *repo,
> > +		       struct strbuf *buf,
> > +		       const char *fmt,
> > +		       va_list args);
> > +
> 
> If we decide to make this as internal implementation, why we don't just
> delete this declaration in the header file? Do I miss out something
> here?

We can't, it's still used to implement `git_common_path()` in the header. We'll remove it in a subsequent commit.

Patrick