| Message ID | e7a143c30dd95d8192eacc35c876e7926cb7c6a4.1723013714.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Stop using `the_repository` in "config.c" | expand |
On 24/08/07 08:56AM, Patrick Steinhardt wrote: > With the same reasoning as the preceding commit, expose the function > `do_git_common_path()` as `strbuf_git_common_pathv()`. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > path.c | 22 +++++++++++----------- > path.h | 5 ++++- > 2 files changed, 15 insertions(+), 12 deletions(-) > > diff --git a/path.c b/path.c > index 71f1cb4dfb..bff98b255e 100644 > --- a/path.c > +++ b/path.c > @@ -617,16 +617,16 @@ int strbuf_git_path_submodule(struct strbuf *buf, const char *path, > return err; > } > > -static void do_git_common_path(const struct repository *repo, > - struct strbuf *buf, > - const char *fmt, > - va_list args) > +void strbuf_git_common_pathv(struct strbuf *sb, > + const struct repository *repo, > + const char *fmt, > + va_list args) Here we reorder the arguments to make `strbuf` first. I assume we are do this to align with the preexisting `strbuf_git_common_path()` and use the "strbuf_" prefix in the function name. In the previous commit we used the "repo_" prefix for `repo_git_pathv()`. Would it make sense to be consistent here? All these functions are operating on the provided buffer, but for a given repository. Not sure what would be most appropriate here. > { > - strbuf_addstr(buf, repo->commondir); > - if (buf->len && !is_dir_sep(buf->buf[buf->len - 1])) > - strbuf_addch(buf, '/'); > - strbuf_vaddf(buf, fmt, args); > - strbuf_cleanup_path(buf); > + strbuf_addstr(sb, repo->commondir); > + if (sb->len && !is_dir_sep(sb->buf[sb->len - 1])) > + strbuf_addch(sb, '/'); > + strbuf_vaddf(sb, fmt, args); > + strbuf_cleanup_path(sb); > }
Justin Tobler <jltobler@gmail.com> writes: >> -static void do_git_common_path(const struct repository *repo, >> - struct strbuf *buf, >> - const char *fmt, >> - va_list args) >> +void strbuf_git_common_pathv(struct strbuf *sb, >> + const struct repository *repo, >> + const char *fmt, >> + va_list args) > > Here we reorder the arguments to make `strbuf` first. I assume we are do > this to align with the preexisting `strbuf_git_common_path()` and use > the "strbuf_" prefix in the function name. I thought that we already established as a general guideline that "strbuf_" should be cleaned up so that functions that happen to use strbuf merely as a way to carry parameters into or results out of them but are not primarily about string manipulation are renamed out of the "strbuf_" namespace. https://lore.kernel.org/git/ZqiLA0bGYZfH1OWD@tanuki/ And this is about getting a path, which is communicated via a "struct strbuf", and not the standard "char *". That is a prime example of a function that we do *not* want to stress strbuf-ness of the function. > In the previous commit we used the "repo_" prefix for > `repo_git_pathv()`. Would it make sense to be consistent here? All these > functions are operating on the provided buffer, but for a given > repository. Not sure what would be most appropriate here. Yes, if the function is about obtaining the path for a file in a given repository's metadata directory, and its association with "strbuf" is that it merely happens to use it instead of "char *", it should not be named as if "strbuf_" ness is the primary characteristics of the function. strbuf_cleanup_path() should also be renamed for the same reason. >> { >> - strbuf_addstr(buf, repo->commondir); >> - if (buf->len && !is_dir_sep(buf->buf[buf->len - 1])) >> - strbuf_addch(buf, '/'); >> - strbuf_vaddf(buf, fmt, args); >> - strbuf_cleanup_path(buf); >> + strbuf_addstr(sb, repo->commondir); >> + if (sb->len && !is_dir_sep(sb->buf[sb->len - 1])) >> + strbuf_addch(sb, '/'); >> + strbuf_vaddf(sb, fmt, args); >> + strbuf_cleanup_path(sb); >> }
On Fri, Aug 09, 2024 at 10:32:54AM -0700, Junio C Hamano wrote: > Justin Tobler <jltobler@gmail.com> writes: > > >> -static void do_git_common_path(const struct repository *repo, > >> - struct strbuf *buf, > >> - const char *fmt, > >> - va_list args) > >> +void strbuf_git_common_pathv(struct strbuf *sb, > >> + const struct repository *repo, > >> + const char *fmt, > >> + va_list args) > > > > Here we reorder the arguments to make `strbuf` first. I assume we are do > > this to align with the preexisting `strbuf_git_common_path()` and use > > the "strbuf_" prefix in the function name. > > I thought that we already established as a general guideline that > "strbuf_" should be cleaned up so that functions that happen to use > strbuf merely as a way to carry parameters into or results out of > them but are not primarily about string manipulation are renamed out > of the "strbuf_" namespace. > > https://lore.kernel.org/git/ZqiLA0bGYZfH1OWD@tanuki/ > > And this is about getting a path, which is communicated via a > "struct strbuf", and not the standard "char *". That is a prime > example of a function that we do *not* want to stress strbuf-ness > of the function. > > > In the previous commit we used the "repo_" prefix for > > `repo_git_pathv()`. Would it make sense to be consistent here? All these > > functions are operating on the provided buffer, but for a given > > repository. Not sure what would be most appropriate here. > > Yes, if the function is about obtaining the path for a file in a > given repository's metadata directory, and its association with > "strbuf" is that it merely happens to use it instead of "char *", > it should not be named as if "strbuf_" ness is the primary > characteristics of the function. > > strbuf_cleanup_path() should also be renamed for the same reason. Agreed. I was doing it for consistency's sake in this case, but let's rather not make the overall interface any weirder than it already is. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> Yes, if the function is about obtaining the path for a file in a >> given repository's metadata directory, and its association with >> "strbuf" is that it merely happens to use it instead of "char *", >> it should not be named as if "strbuf_" ness is the primary >> characteristics of the function. >> >> strbuf_cleanup_path() should also be renamed for the same reason. > > Agreed. I was doing it for consistency's sake in this case, but let's > rather not make the overall interface any weirder than it already is. Thanks, then we are in agreement.
diff --git a/path.c b/path.c index 71f1cb4dfb..bff98b255e 100644 --- a/path.c +++ b/path.c @@ -617,16 +617,16 @@ int strbuf_git_path_submodule(struct strbuf *buf, const char *path, return err; } -static void do_git_common_path(const struct repository *repo, - struct strbuf *buf, - const char *fmt, - va_list args) +void strbuf_git_common_pathv(struct strbuf *sb, + const struct repository *repo, + const char *fmt, + va_list args) { - strbuf_addstr(buf, repo->commondir); - if (buf->len && !is_dir_sep(buf->buf[buf->len - 1])) - strbuf_addch(buf, '/'); - strbuf_vaddf(buf, fmt, args); - strbuf_cleanup_path(buf); + strbuf_addstr(sb, repo->commondir); + if (sb->len && !is_dir_sep(sb->buf[sb->len - 1])) + strbuf_addch(sb, '/'); + strbuf_vaddf(sb, fmt, args); + strbuf_cleanup_path(sb); } const char *git_common_path(const char *fmt, ...) @@ -634,7 +634,7 @@ const char *git_common_path(const char *fmt, ...) struct strbuf *pathname = get_pathname(); va_list args; va_start(args, fmt); - do_git_common_path(the_repository, pathname, fmt, args); + strbuf_git_common_pathv(pathname, the_repository, fmt, args); va_end(args); return pathname->buf; } @@ -645,7 +645,7 @@ void strbuf_git_common_path(struct strbuf *sb, { va_list args; va_start(args, fmt); - do_git_common_path(repo, sb, fmt, args); + strbuf_git_common_pathv(sb, repo, fmt, args); va_end(args); } diff --git a/path.h b/path.h index 94e7030f0b..77eb0e6543 100644 --- a/path.h +++ b/path.h @@ -37,6 +37,10 @@ void strbuf_git_common_path(struct strbuf *sb, const struct repository *repo, const char *fmt, ...) __attribute__((format (printf, 3, 4))); +void strbuf_git_common_pathv(struct strbuf *sb, + const struct repository *repo, + const char *fmt, + va_list args); /* * Return a statically allocated path into the main repository's @@ -45,7 +49,6 @@ void strbuf_git_common_path(struct strbuf *sb, const char *git_common_path(const char *fmt, ...) __attribute__((format (printf, 1, 2))); - /* * The `git_path` family of functions will construct a path into a repository's * git directory.
With the same reasoning as the preceding commit, expose the function `do_git_common_path()` as `strbuf_git_common_pathv()`. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- path.c | 22 +++++++++++----------- path.h | 5 ++++- 2 files changed, 15 insertions(+), 12 deletions(-)