On Thu, Feb 06, 2025 at 10:21:24PM +0800, shejialuo wrote:
> 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.
> > 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
