diff mbox series

[05/16] path: refactor `repo_submodule_path()` family of functions

Message ID 20250206-b4-pks-path-drop-the-repository-v1-5-4e77f0313206@pks.im (mailing list archive)
State Superseded
Headers show
Series path: remove dependency on `the_repository` | expand

Commit Message

Patrick Steinhardt Feb. 6, 2025, 7:58 a.m. UTC 
As explained in an earlier commit, we're refactoring path-related
functions to provide a consistent interface for computing paths into the
commondir, gitdir and worktree. Refactor the "submodule" family of
functions accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/submodule--helper.c |  2 +-
 path.c                      | 37 +++++++++++++++++++++++++++++--------
 path.h                      | 30 ++++++++++++++++++------------
 t/helper/test-ref-store.c   |  7 +++----
 worktree.c                  |  3 ++-
 5 files changed, 53 insertions(+), 26 deletions(-)

Comments

Karthik Nayak Feb. 6, 2025, 12:05 p.m. UTC | #1 
Patrick Steinhardt <ps@pks.im> writes:

[snip]

> diff --git a/path.c b/path.c
> index d918d0409e..d721507be8 100644
> --- a/path.c
> +++ b/path.c
> @@ -560,14 +560,15 @@ const char *repo_worktree_path_replace(const struct repository *repo,
>  }
>
>  /* Returns 0 on success, negative on failure. */
> -static int do_submodule_path(struct strbuf *buf, const char *path,
> +static int do_submodule_path(struct repository *repo,
> +			     struct strbuf *buf, const char *path,
>  			     const char *fmt, va_list args)
>  {
>  	struct strbuf git_submodule_common_dir = STRBUF_INIT;
>  	struct strbuf git_submodule_dir = STRBUF_INIT;
>  	int ret;
>
> -	ret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);
> +	ret = submodule_to_gitdir(repo, &git_submodule_dir, path);
>  	if (ret)
>  		goto cleanup;
>
> @@ -586,13 +587,14 @@ static int do_submodule_path(struct strbuf *buf, const char *path,
>  	return ret;
>  }
>
> -char *git_pathdup_submodule(const char *path, const char *fmt, ...)
> +char *repo_submodule_path(struct repository *repo,

To stay consistent with the other repo_* functions, should we change
`struct repository *repo` to `const struct repository *repo`?

> +			  const char *path, const char *fmt, ...)
>  {
>  	int err;
>  	va_list args;
>  	struct strbuf buf = STRBUF_INIT;
>  	va_start(args, fmt);
> -	err = do_submodule_path(&buf, path, fmt, args);
> +	err = do_submodule_path(repo, &buf, path, fmt, args);
>  	va_end(args);
>  	if (err) {
>  		strbuf_release(&buf);
> @@ -601,16 +603,35 @@ char *git_pathdup_submodule(const char *path, const char *fmt, ...)
>  	return strbuf_detach(&buf, NULL);
>  }
>
> -int strbuf_git_path_submodule(struct strbuf *buf, const char *path,
> -			      const char *fmt, ...)
> +const char *repo_submodule_path_append(struct repository *repo,
> +				       struct strbuf *buf,
> +				       const char *path,
> +				       const char *fmt, ...)
>  {
>  	int err;
>  	va_list args;
>  	va_start(args, fmt);
> -	err = do_submodule_path(buf, path, fmt, args);
> +	err = do_submodule_path(repo, buf, path, fmt, args);
>  	va_end(args);
> +	if (err)
> +		return NULL;
> +	return buf->buf;
> +}
>
> -	return err;
> +const char *repo_submodule_path_replace(struct repository *repo,
> +					struct strbuf *buf,
> +					const char *path,
> +					const char *fmt, ...)
> +{
> +	int err;
> +	va_list args;
> +	strbuf_reset(buf);
> +	va_start(args, fmt);
> +	err = do_submodule_path(repo, buf, path, fmt, args);
> +	va_end(args);
> +	if (err)
> +		return NULL;
> +	return buf->buf;
>  }
>
>  void repo_common_pathv(const struct repository *repo,

[snip]
shejialuo Feb. 6, 2025, 3:03 p.m. UTC | #2 
On Thu, Feb 06, 2025 at 08:58:01AM +0100, Patrick Steinhardt wrote:
> As explained in an earlier commit, we're refactoring path-related
> functions to provide a consistent interface for computing paths into the
> commondir, gitdir and worktree. Refactor the "submodule" family of
> functions accordingly.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/submodule--helper.c |  2 +-
>  path.c                      | 37 +++++++++++++++++++++++++++++--------
>  path.h                      | 30 ++++++++++++++++++------------
>  t/helper/test-ref-store.c   |  7 +++----
>  worktree.c                  |  3 ++-
>  5 files changed, 53 insertions(+), 26 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 3a64f7e605..c1a8029714 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1826,7 +1826,7 @@ static int clone_submodule(const struct module_clone_data *clone_data,
>  
>  	connect_work_tree_and_git_dir(clone_data_path, sm_gitdir, 0);
>  
> -	p = git_pathdup_submodule(clone_data_path, "config");
> +	p = repo_submodule_path(the_repository, clone_data_path, "config");
>  	if (!p)
>  		die(_("could not get submodule directory for '%s'"), clone_data_path);
>  
> diff --git a/path.c b/path.c
> index d918d0409e..d721507be8 100644
> --- a/path.c
> +++ b/path.c
> @@ -560,14 +560,15 @@ const char *repo_worktree_path_replace(const struct repository *repo,
>  }
>  
>  /* Returns 0 on success, negative on failure. */
> -static int do_submodule_path(struct strbuf *buf, const char *path,
> +static int do_submodule_path(struct repository *repo,
> +			     struct strbuf *buf, const char *path,
>  			     const char *fmt, va_list args)
>  {
>  	struct strbuf git_submodule_common_dir = STRBUF_INIT;
>  	struct strbuf git_submodule_dir = STRBUF_INIT;
>  	int ret;
>  
> -	ret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);
> +	ret = submodule_to_gitdir(repo, &git_submodule_dir, path);
>  	if (ret)
>  		goto cleanup;
>  
> @@ -586,13 +587,14 @@ static int do_submodule_path(struct strbuf *buf, const char *path,
>  	return ret;
>  }
>  
> -char *git_pathdup_submodule(const char *path, const char *fmt, ...)
> +char *repo_submodule_path(struct repository *repo,
> +			  const char *path, const char *fmt, ...)
>  {
>  	int err;
>  	va_list args;
>  	struct strbuf buf = STRBUF_INIT;
>  	va_start(args, fmt);
> -	err = do_submodule_path(&buf, path, fmt, args);
> +	err = do_submodule_path(repo, &buf, path, fmt, args);
>  	va_end(args);
>  	if (err) {
>  		strbuf_release(&buf);
> @@ -601,16 +603,35 @@ char *git_pathdup_submodule(const char *path, const char *fmt, ...)
>  	return strbuf_detach(&buf, NULL);
>  }
>  
> -int strbuf_git_path_submodule(struct strbuf *buf, const char *path,
> -			      const char *fmt, ...)
> +const char *repo_submodule_path_append(struct repository *repo,
> +				       struct strbuf *buf,
> +				       const char *path,
> +				       const char *fmt, ...)
>  {
>  	int err;
>  	va_list args;
>  	va_start(args, fmt);
> -	err = do_submodule_path(buf, path, fmt, args);
> +	err = do_submodule_path(repo, buf, path, fmt, args);
>  	va_end(args);
> +	if (err)
> +		return NULL;
> +	return buf->buf;
> +}
>  
> -	return err;
> +const char *repo_submodule_path_replace(struct repository *repo,
> +					struct strbuf *buf,
> +					const char *path,
> +					const char *fmt, ...)
> +{
> +	int err;
> +	va_list args;
> +	strbuf_reset(buf);
> +	va_start(args, fmt);
> +	err = do_submodule_path(repo, buf, path, fmt, args);
> +	va_end(args);
> +	if (err)
> +		return NULL;
> +	return buf->buf;
>  }

By reading through the patches from 1 to this. I gradually understand
your design now. For every refactor, we will provide three kinds of
functions. All of these functions will return `const char *` and we
could elegantly use `NULL` to indicate the error.

Thanks,
Jialuo
Patrick Steinhardt Feb. 7, 2025, 6:16 a.m. UTC | #3 
On Thu, Feb 06, 2025 at 04:05:13AM -0800, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> [snip]
> 
> > diff --git a/path.c b/path.c
> > index d918d0409e..d721507be8 100644
> > --- a/path.c
> > +++ b/path.c
> > @@ -560,14 +560,15 @@ const char *repo_worktree_path_replace(const struct repository *repo,
> >  }
> >
> >  /* Returns 0 on success, negative on failure. */
> > -static int do_submodule_path(struct strbuf *buf, const char *path,
> > +static int do_submodule_path(struct repository *repo,
> > +			     struct strbuf *buf, const char *path,
> >  			     const char *fmt, va_list args)
> >  {
> >  	struct strbuf git_submodule_common_dir = STRBUF_INIT;
> >  	struct strbuf git_submodule_dir = STRBUF_INIT;
> >  	int ret;
> >
> > -	ret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);
> > +	ret = submodule_to_gitdir(repo, &git_submodule_dir, path);
> >  	if (ret)
> >  		goto cleanup;
> >
> > @@ -586,13 +587,14 @@ static int do_submodule_path(struct strbuf *buf, const char *path,
> >  	return ret;
> >  }
> >
> > -char *git_pathdup_submodule(const char *path, const char *fmt, ...)
> > +char *repo_submodule_path(struct repository *repo,
> 
> To stay consistent with the other repo_* functions, should we change
> `struct repository *repo` to `const struct repository *repo`?

Somebody noticed :) But no, we cannot, we need to internally pass the
repo to functions that expect a non-const pointer. This is because deep
down in the callstack we end up calling `repo_read_gitmodules()`, which
modifies the repository.

I'll add a comment to the commit message.

Patrick
Karthik Nayak Feb. 7, 2025, 7:04 a.m. UTC | #4 
Patrick Steinhardt <ps@pks.im> writes:

> On Thu, Feb 06, 2025 at 04:05:13AM -0800, Karthik Nayak wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> [snip]
>>
>> > diff --git a/path.c b/path.c
>> > index d918d0409e..d721507be8 100644
>> > --- a/path.c
>> > +++ b/path.c
>> > @@ -560,14 +560,15 @@ const char *repo_worktree_path_replace(const struct repository *repo,
>> >  }
>> >
>> >  /* Returns 0 on success, negative on failure. */
>> > -static int do_submodule_path(struct strbuf *buf, const char *path,
>> > +static int do_submodule_path(struct repository *repo,
>> > +			     struct strbuf *buf, const char *path,
>> >  			     const char *fmt, va_list args)
>> >  {
>> >  	struct strbuf git_submodule_common_dir = STRBUF_INIT;
>> >  	struct strbuf git_submodule_dir = STRBUF_INIT;
>> >  	int ret;
>> >
>> > -	ret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);
>> > +	ret = submodule_to_gitdir(repo, &git_submodule_dir, path);
>> >  	if (ret)
>> >  		goto cleanup;
>> >
>> > @@ -586,13 +587,14 @@ static int do_submodule_path(struct strbuf *buf, const char *path,
>> >  	return ret;
>> >  }
>> >
>> > -char *git_pathdup_submodule(const char *path, const char *fmt, ...)
>> > +char *repo_submodule_path(struct repository *repo,
>>
>> To stay consistent with the other repo_* functions, should we change
>> `struct repository *repo` to `const struct repository *repo`?
>
> Somebody noticed :) But no, we cannot, we need to internally pass the
> repo to functions that expect a non-const pointer. This is because deep
> down in the callstack we end up calling `repo_read_gitmodules()`, which
> modifies the repository.
>

Okay that makes sense.

> I'll add a comment to the commit message.
>
> Patrick
>

Okay perfect, thanks!
diff mbox series

Patch

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3a64f7e605..c1a8029714 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1826,7 +1826,7 @@  static int clone_submodule(const struct module_clone_data *clone_data,
 
 	connect_work_tree_and_git_dir(clone_data_path, sm_gitdir, 0);
 
-	p = git_pathdup_submodule(clone_data_path, "config");
+	p = repo_submodule_path(the_repository, clone_data_path, "config");
 	if (!p)
 		die(_("could not get submodule directory for '%s'"), clone_data_path);
 
diff --git a/path.c b/path.c
index d918d0409e..d721507be8 100644
--- a/path.c
+++ b/path.c
@@ -560,14 +560,15 @@  const char *repo_worktree_path_replace(const struct repository *repo,
 }
 
 /* Returns 0 on success, negative on failure. */
-static int do_submodule_path(struct strbuf *buf, const char *path,
+static int do_submodule_path(struct repository *repo,
+			     struct strbuf *buf, const char *path,
 			     const char *fmt, va_list args)
 {
 	struct strbuf git_submodule_common_dir = STRBUF_INIT;
 	struct strbuf git_submodule_dir = STRBUF_INIT;
 	int ret;
 
-	ret = submodule_to_gitdir(the_repository, &git_submodule_dir, path);
+	ret = submodule_to_gitdir(repo, &git_submodule_dir, path);
 	if (ret)
 		goto cleanup;
 
@@ -586,13 +587,14 @@  static int do_submodule_path(struct strbuf *buf, const char *path,
 	return ret;
 }
 
-char *git_pathdup_submodule(const char *path, const char *fmt, ...)
+char *repo_submodule_path(struct repository *repo,
+			  const char *path, const char *fmt, ...)
 {
 	int err;
 	va_list args;
 	struct strbuf buf = STRBUF_INIT;
 	va_start(args, fmt);
-	err = do_submodule_path(&buf, path, fmt, args);
+	err = do_submodule_path(repo, &buf, path, fmt, args);
 	va_end(args);
 	if (err) {
 		strbuf_release(&buf);
@@ -601,16 +603,35 @@  char *git_pathdup_submodule(const char *path, const char *fmt, ...)
 	return strbuf_detach(&buf, NULL);
 }
 
-int strbuf_git_path_submodule(struct strbuf *buf, const char *path,
-			      const char *fmt, ...)
+const char *repo_submodule_path_append(struct repository *repo,
+				       struct strbuf *buf,
+				       const char *path,
+				       const char *fmt, ...)
 {
 	int err;
 	va_list args;
 	va_start(args, fmt);
-	err = do_submodule_path(buf, path, fmt, args);
+	err = do_submodule_path(repo, buf, path, fmt, args);
 	va_end(args);
+	if (err)
+		return NULL;
+	return buf->buf;
+}
 
-	return err;
+const char *repo_submodule_path_replace(struct repository *repo,
+					struct strbuf *buf,
+					const char *path,
+					const char *fmt, ...)
+{
+	int err;
+	va_list args;
+	strbuf_reset(buf);
+	va_start(args, fmt);
+	err = do_submodule_path(repo, buf, path, fmt, args);
+	va_end(args);
+	if (err)
+		return NULL;
+	return buf->buf;
 }
 
 void repo_common_pathv(const struct repository *repo,
diff --git a/path.h b/path.h
index 8761c4c660..63a8f91947 100644
--- a/path.h
+++ b/path.h
@@ -93,20 +93,26 @@  const char *repo_worktree_path_replace(const struct repository *repo,
 	__attribute__((format (printf, 3, 4)));
 
 /*
- * Return a path into a submodule's git directory located at `path`.  `path`
- * must only reference a submodule of the main repository (the_repository).
- */
-char *git_pathdup_submodule(const char *path, const char *fmt, ...)
-	__attribute__((format (printf, 2, 3)));
-
-/*
- * Construct a path into a submodule's git directory located at `path` and
- * append it to the provided buffer `sb`.  `path` must only reference a
- * submodule of the main repository (the_repository).
+ * The `repo_submodule_path` family of functions will construct a path into a
+ * submodule's git directory located at `path`. `path` must be a submodule path
+ * as found in the index and must be part of the given repository.
+ *
+ * Returns a `NULL` pointer in case the submodule cannot be found.
  */
-int strbuf_git_path_submodule(struct strbuf *sb, const char *path,
-				     const char *fmt, ...)
+char *repo_submodule_path(struct repository *repo,
+			  const char *path,
+			  const char *fmt, ...)
 	__attribute__((format (printf, 3, 4)));
+const char *repo_submodule_path_append(struct repository *repo,
+				       struct strbuf *sb,
+				       const char *path,
+				       const char *fmt, ...)
+	__attribute__((format (printf, 4, 5)));
+const char *repo_submodule_path_replace(struct repository *repo,
+					struct strbuf *sb,
+					const char *path,
+					const char *fmt, ...)
+	__attribute__((format (printf, 4, 5)));
 
 void report_linked_checkout_garbage(struct repository *r);
 
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 1cc05f043a..e00fce592b 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -75,11 +75,10 @@  static const char **get_store(const char **argv, struct ref_store **refs)
 		*refs = get_main_ref_store(the_repository);
 	} else if (skip_prefix(argv[0], "submodule:", &gitdir)) {
 		struct strbuf sb = STRBUF_INIT;
-		int ret;
 
-		ret = strbuf_git_path_submodule(&sb, gitdir, "objects/");
-		if (ret)
-			die("strbuf_git_path_submodule failed: %d", ret);
+		if (!repo_submodule_path_append(the_repository,
+						&sb, gitdir, "objects/"))
+			die("computing submodule path failed");
 		add_to_alternates_memory(sb.buf);
 		strbuf_release(&sb);
 
diff --git a/worktree.c b/worktree.c
index f8d6e7127f..8f4fc10c44 100644
--- a/worktree.c
+++ b/worktree.c
@@ -487,7 +487,8 @@  int submodule_uses_worktrees(const char *path)
 	int ret = 0;
 	struct repository_format format = REPOSITORY_FORMAT_INIT;
 
-	submodule_gitdir = git_pathdup_submodule(path, "%s", "");
+	submodule_gitdir = repo_submodule_path(the_repository,
+					       path, "%s", "");
 	if (!submodule_gitdir)
 		return 0;