diff mbox series

[07/20] path: hide functions using `the_repository` by default

Message ID b4e973a2804ba09149224a2e18a359717228607e.1723013714.git.ps@pks.im (mailing list archive)
State Superseded
Headers show
Series Stop using `the_repository` in "config.c" | expand

Commit Message

Patrick Steinhardt Aug. 7, 2024, 6:57 a.m. UTC
The path subsytem provides a bunch of legacy functions that compute
paths relative to the "gitdir" and "commondir" directories of the global
`the_repository` variable. Use of those functions is discouraged, and it
is easy to miss the implicit dependency on `the_repository` that calls
to those functions may cause.

With `USE_THE_REPOSITORY_VARIABLE`, we have recently introduced a tool
that allows us to get rid of such functions over time. With this define,
we can hide away functions that have such implicit dependency such that
other subsystems that want to be free of `the_repository` will not use
them by accident.

Move all path-related functions that use `the_repository` into a block
that gets only conditionally compiled depending on whether or not the
macro has been defined. This also removes all dependencies on that
variable in "path.c", allowing us to remove the definition of said
preprocessor macro.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 path.c |  52 +-------------------
 path.h | 147 ++++++++++++++++++++++++++++++++++++++-------------------
 2 files changed, 100 insertions(+), 99 deletions(-)

Comments

Justin Tobler Aug. 9, 2024, 7:43 p.m. UTC | #1
On 24/08/07 08:57AM, Patrick Steinhardt wrote:
> The path subsytem provides a bunch of legacy functions that compute

s/subsytem/subsystem/

> paths relative to the "gitdir" and "commondir" directories of the global
> `the_repository` variable. Use of those functions is discouraged, and it
> is easy to miss the implicit dependency on `the_repository` that calls
> to those functions may cause.
> 
> With `USE_THE_REPOSITORY_VARIABLE`, we have recently introduced a tool
> that allows us to get rid of such functions over time. With this define,

s/define/defined/

> we can hide away functions that have such implicit dependency such that
> other subsystems that want to be free of `the_repository` will not use
> them by accident.
> 
> Move all path-related functions that use `the_repository` into a block
> that gets only conditionally compiled depending on whether or not the
> macro has been defined. This also removes all dependencies on that
> variable in "path.c", allowing us to remove the definition of said
> preprocessor macro.

Nice! So now the "path.c" is `the_repository` free. Implicit use of
`the_repository` is guarded behind `USE_THE_REPOSITORY_VARIABLE`. This
is quite neat. :)

> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  path.c |  52 +-------------------
>  path.h | 147 ++++++++++++++++++++++++++++++++++++++-------------------
>  2 files changed, 100 insertions(+), 99 deletions(-)
> 
> diff --git a/path.c b/path.c
> index 567eff5253..d073ae6449 100644
> --- a/path.c
> +++ b/path.c
> @@ -2,8 +2,6 @@
>   * Utilities for paths and pathnames
>   */
>  
> -#define USE_THE_REPOSITORY_VARIABLE
> -
>  #include "git-compat-util.h"
>  #include "abspath.h"
>  #include "environment.h"
> @@ -30,7 +28,7 @@ static int get_st_mode_bits(const char *path, int *mode)
>  	return 0;
>  }
>  
> -static struct strbuf *get_pathname(void)
> +struct strbuf *get_pathname(void)

Now that functions with implicit `the_repository` usage are static
inlined we are required to expose `get_pathname()` for the same reason
as some of the other functions in previous commits.

[snip]
> +# ifdef USE_THE_REPOSITORY_VARIABLE
> +#  include "strbuf.h"
> +#  include "repository.h"

Naive question, what is the purpose of providing the include statements
here? Wouldn't they always already be included?

> +
> +/*
> + * Return a statically allocated path into the main repository's
> + * (the_repository) common git directory.
> + */
> +__attribute__((format (printf, 1, 2)))
> +static inline const char *git_common_path(const char *fmt, ...)
> +{
> +	struct strbuf *pathname = get_pathname();
> +	va_list args;
> +	va_start(args, fmt);
> +	strbuf_git_common_pathv(pathname, the_repository, fmt, args);
> +	va_end(args);
> +	return pathname->buf;
> +}
> +
> +/*
> + * Construct a path into the main repository's (the_repository) git directory
> + * and place it in the provided buffer `buf`, the contents of the buffer will
> + * be overridden.
> + */
> +__attribute__((format (printf, 2, 3)))
> +static inline char *git_path_buf(struct strbuf *buf, const char *fmt, ...)
> +{
> +	va_list args;
> +	strbuf_reset(buf);
> +	va_start(args, fmt);
> +	repo_git_pathv(the_repository, NULL, buf, fmt, args);
> +	va_end(args);
> +	return buf->buf;
> +}
> +
> +/*
> + * Construct a path into the main repository's (the_repository) git directory
> + * and append it to the provided buffer `sb`.
> + */
> +__attribute__((format (printf, 2, 3)))
> +static inline void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
> +{
> +	va_list args;
> +	va_start(args, fmt);
> +	repo_git_pathv(the_repository, NULL, sb, fmt, args);
> +	va_end(args);
> +}
> +
> +/*
> + * Return a statically allocated path into the main repository's
> + * (the_repository) git directory.
> + */
> +__attribute__((format (printf, 1, 2)))
> +static inline const char *git_path(const char *fmt, ...)
> +{
> +	struct strbuf *pathname = get_pathname();
> +	va_list args;
> +	va_start(args, fmt);
> +	repo_git_pathv(the_repository, NULL, pathname, fmt, args);
> +	va_end(args);
> +	return pathname->buf;
> +}
> +
> +#define GIT_PATH_FUNC(func, filename) \
> +	const char *func(void) \
> +	{ \
> +		static char *ret; \
> +		if (!ret) \
> +			ret = git_pathdup(filename); \
> +		return ret; \
> +	}
> +
> +/*
> + * Return a path into the main repository's (the_repository) git directory.
> + */
> +__attribute__((format (printf, 1, 2)))
> +static inline char *git_pathdup(const char *fmt, ...)
> +{
> +	struct strbuf path = STRBUF_INIT;
> +	va_list args;
> +	va_start(args, fmt);
> +	repo_git_pathv(the_repository, NULL, &path, fmt, args);
> +	va_end(args);
> +	return strbuf_detach(&path, NULL);
> +}
> +
> +# endif /* USE_THE_REPOSITORY_VARIABLE */

I like how it's all consolidated into this one block. Very nice! :)
Patrick Steinhardt Aug. 13, 2024, 9:25 a.m. UTC | #2
On Fri, Aug 09, 2024 at 02:43:59PM -0500, Justin Tobler wrote:
> On 24/08/07 08:57AM, Patrick Steinhardt wrote:
> > +# ifdef USE_THE_REPOSITORY_VARIABLE
> > +#  include "strbuf.h"
> > +#  include "repository.h"
> 
> Naive question, what is the purpose of providing the include statements
> here? Wouldn't they always already be included?

Not necessarily, no. Most headers only have a forward declaration of
`struct strbuf` and `struct repository`. For one this means that we
cannot access the strbuf contents like we do in the function definitions
in this header now. Second, it means that we cannot access `the_repository`.

Patrick
diff mbox series

Patch

diff --git a/path.c b/path.c
index 567eff5253..d073ae6449 100644
--- a/path.c
+++ b/path.c
@@ -2,8 +2,6 @@ 
  * Utilities for paths and pathnames
  */
 
-#define USE_THE_REPOSITORY_VARIABLE
-
 #include "git-compat-util.h"
 #include "abspath.h"
 #include "environment.h"
@@ -30,7 +28,7 @@  static int get_st_mode_bits(const char *path, int *mode)
 	return 0;
 }
 
-static struct strbuf *get_pathname(void)
+struct strbuf *get_pathname(void)
 {
 	static struct strbuf pathname_array[4] = {
 		STRBUF_INIT, STRBUF_INIT, STRBUF_INIT, STRBUF_INIT
@@ -453,44 +451,6 @@  void strbuf_repo_git_path(struct strbuf *sb,
 	va_end(args);
 }
 
-char *git_path_buf(struct strbuf *buf, const char *fmt, ...)
-{
-	va_list args;
-	strbuf_reset(buf);
-	va_start(args, fmt);
-	repo_git_pathv(the_repository, NULL, buf, fmt, args);
-	va_end(args);
-	return buf->buf;
-}
-
-void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
-{
-	va_list args;
-	va_start(args, fmt);
-	repo_git_pathv(the_repository, NULL, sb, fmt, args);
-	va_end(args);
-}
-
-const char *git_path(const char *fmt, ...)
-{
-	struct strbuf *pathname = get_pathname();
-	va_list args;
-	va_start(args, fmt);
-	repo_git_pathv(the_repository, NULL, pathname, fmt, args);
-	va_end(args);
-	return pathname->buf;
-}
-
-char *git_pathdup(const char *fmt, ...)
-{
-	struct strbuf path = STRBUF_INIT;
-	va_list args;
-	va_start(args, fmt);
-	repo_git_pathv(the_repository, NULL, &path, fmt, args);
-	va_end(args);
-	return strbuf_detach(&path, NULL);
-}
-
 char *mkpathdup(const char *fmt, ...)
 {
 	struct strbuf sb = STRBUF_INIT;
@@ -634,16 +594,6 @@  void strbuf_git_common_pathv(struct strbuf *sb,
 	strbuf_cleanup_path(sb);
 }
 
-const char *git_common_path(const char *fmt, ...)
-{
-	struct strbuf *pathname = get_pathname();
-	va_list args;
-	va_start(args, fmt);
-	strbuf_git_common_pathv(pathname, the_repository, fmt, args);
-	va_end(args);
-	return pathname->buf;
-}
-
 void strbuf_git_common_path(struct strbuf *sb,
 			    const struct repository *repo,
 			    const char *fmt, ...)
diff --git a/path.h b/path.h
index 6228ca03d7..22fdfc3d3a 100644
--- a/path.h
+++ b/path.h
@@ -25,7 +25,7 @@  char *mkpathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
 
 /*
- * The `git_common_path` family of functions will construct a path into a
+ * The `strbuf_git_common_path` family of functions will construct a path into a
  * repository's common git directory, which is shared by all worktrees.
  */
 
@@ -43,14 +43,7 @@  void strbuf_git_common_pathv(struct strbuf *sb,
 			     va_list args);
 
 /*
- * Return a statically allocated path into the main repository's
- * (the_repository) common git directory.
- */
-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
+ * The `repo_git_path` family of functions will construct a path into a repository's
  * git directory.
  *
  * These functions will perform adjustments to the resultant path to account
@@ -87,14 +80,7 @@  void strbuf_repo_git_path(struct strbuf *sb,
 	__attribute__((format (printf, 3, 4)));
 
 /*
- * Return a statically allocated path into the main repository's
- * (the_repository) git directory.
- */
-const char *git_path(const char *fmt, ...)
-	__attribute__((format (printf, 1, 2)));
-
-/*
- * Similar to git_path() but can produce paths for a specified
+ * Similar to repo_git_path() but can produce paths for a specified
  * worktree instead of current one
  */
 const char *worktree_git_path(struct repository *r,
@@ -102,27 +88,6 @@  const char *worktree_git_path(struct repository *r,
 			      const char *fmt, ...)
 	__attribute__((format (printf, 3, 4)));
 
-/*
- * Return a path into the main repository's (the_repository) git directory.
- */
-char *git_pathdup(const char *fmt, ...)
-	__attribute__((format (printf, 1, 2)));
-
-/*
- * Construct a path into the main repository's (the_repository) git directory
- * and place it in the provided buffer `buf`, the contents of the buffer will
- * be overridden.
- */
-char *git_path_buf(struct strbuf *buf, const char *fmt, ...)
-	__attribute__((format (printf, 2, 3)));
-
-/*
- * Construct a path into the main repository's (the_repository) git directory
- * and append it to the provided buffer `sb`.
- */
-void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
-	__attribute__((format (printf, 2, 3)));
-
 /*
  * Return a path into the worktree of repository `repo`.
  *
@@ -164,19 +129,10 @@  void report_linked_checkout_garbage(struct repository *r);
 /*
  * You can define a static memoized git path like:
  *
- *    static GIT_PATH_FUNC(git_path_foo, "FOO")
+ *    static REPO_GIT_PATH_FUNC(git_path_foo, "FOO")
  *
  * or use one of the global ones below.
  */
-#define GIT_PATH_FUNC(func, filename) \
-	const char *func(void) \
-	{ \
-		static char *ret; \
-		if (!ret) \
-			ret = git_pathdup(filename); \
-		return ret; \
-	}
-
 #define REPO_GIT_PATH_FUNC(var, filename) \
 	const char *git_path_##var(struct repository *r) \
 	{ \
@@ -260,4 +216,99 @@  char *xdg_cache_home(const char *filename);
  */
 void safe_create_dir(const char *dir, int share);
 
+/*
+ * Do not use this function. It is only exported to other subsystems until we
+ * can get rid of the below block of functions that implicitly rely on
+ * `the_repository`.
+ */
+struct strbuf *get_pathname(void);
+
+# ifdef USE_THE_REPOSITORY_VARIABLE
+#  include "strbuf.h"
+#  include "repository.h"
+
+/*
+ * Return a statically allocated path into the main repository's
+ * (the_repository) common git directory.
+ */
+__attribute__((format (printf, 1, 2)))
+static inline const char *git_common_path(const char *fmt, ...)
+{
+	struct strbuf *pathname = get_pathname();
+	va_list args;
+	va_start(args, fmt);
+	strbuf_git_common_pathv(pathname, the_repository, fmt, args);
+	va_end(args);
+	return pathname->buf;
+}
+
+/*
+ * Construct a path into the main repository's (the_repository) git directory
+ * and place it in the provided buffer `buf`, the contents of the buffer will
+ * be overridden.
+ */
+__attribute__((format (printf, 2, 3)))
+static inline char *git_path_buf(struct strbuf *buf, const char *fmt, ...)
+{
+	va_list args;
+	strbuf_reset(buf);
+	va_start(args, fmt);
+	repo_git_pathv(the_repository, NULL, buf, fmt, args);
+	va_end(args);
+	return buf->buf;
+}
+
+/*
+ * Construct a path into the main repository's (the_repository) git directory
+ * and append it to the provided buffer `sb`.
+ */
+__attribute__((format (printf, 2, 3)))
+static inline void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
+{
+	va_list args;
+	va_start(args, fmt);
+	repo_git_pathv(the_repository, NULL, sb, fmt, args);
+	va_end(args);
+}
+
+/*
+ * Return a statically allocated path into the main repository's
+ * (the_repository) git directory.
+ */
+__attribute__((format (printf, 1, 2)))
+static inline const char *git_path(const char *fmt, ...)
+{
+	struct strbuf *pathname = get_pathname();
+	va_list args;
+	va_start(args, fmt);
+	repo_git_pathv(the_repository, NULL, pathname, fmt, args);
+	va_end(args);
+	return pathname->buf;
+}
+
+#define GIT_PATH_FUNC(func, filename) \
+	const char *func(void) \
+	{ \
+		static char *ret; \
+		if (!ret) \
+			ret = git_pathdup(filename); \
+		return ret; \
+	}
+
+/*
+ * Return a path into the main repository's (the_repository) git directory.
+ */
+__attribute__((format (printf, 1, 2)))
+static inline char *git_pathdup(const char *fmt, ...)
+{
+	struct strbuf path = STRBUF_INIT;
+	va_list args;
+	va_start(args, fmt);
+	repo_git_pathv(the_repository, NULL, &path, fmt, args);
+	va_end(args);
+	return strbuf_detach(&path, NULL);
+}
+
+# endif /* USE_THE_REPOSITORY_VARIABLE */
+
 #endif /* PATH_H */