diff mbox series

[06/20] path: stop relying on `the_repository` in `worktree_git_path()`

Message ID 67405dcd0a121aee971f854dc35ba89bd4f808c4.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 
When not provided a worktree, then `worktree_git_path()` will fall back
to returning a path relative to the main repository. In this case, we
implicitly rely on `the_repository` to derive the path. Remove this
dependency by passing a `struct repository` as parameter.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fsck.c     |  2 +-
 builtin/worktree.c |  4 ++--
 path.c             |  9 +++++++--
 path.h             |  5 +++--
 revision.c         |  2 +-
 worktree.c         |  2 +-
 wt-status.c        | 14 +++++++-------
 7 files changed, 22 insertions(+), 16 deletions(-)

Comments

Justin Tobler Aug. 9, 2024, 7:02 p.m. UTC | #1 
On 24/08/07 08:57AM, Patrick Steinhardt wrote:
> When not provided a worktree, then `worktree_git_path()` will fall back
> to returning a path relative to the main repository. In this case, we
> implicitly rely on `the_repository` to derive the path. Remove this
> dependency by passing a `struct repository` as parameter.

Are there many situations where `worktree_git_path()` would expect to
not be provided a worktree? I wonder whether this implicit behavior is
really necessary to begin with.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
[snip] 
> -const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
> +const char *worktree_git_path(struct repository *r,
> +			      const struct worktree *wt, const char *fmt, ...)
>  {
>  	struct strbuf *pathname = get_pathname();
>  	va_list args;
> +
> +	if (wt && wt->repo != r)
> +		BUG("worktree not connected to expected repository");

So if we receive a worktree, we expect that it should always match the
main repostiory. Makes sense.

> +
>  	va_start(args, fmt);
> -	repo_git_pathv(the_repository, wt, pathname, fmt, args);
> +	repo_git_pathv(r, wt, pathname, fmt, args);
>  	va_end(args);
>  	return pathname->buf;
>  }
> diff --git a/path.h b/path.h
> index 3d21b9cd16..6228ca03d7 100644
> --- a/path.h
> +++ b/path.h
> @@ -97,9 +97,10 @@ const char *git_path(const char *fmt, ...)
>   * Similar to git_path() but can produce paths for a specified
>   * worktree instead of current one
>   */

Now that the previously implicit behavior is more explicit, it might be
update the comment to explain that the provided repository is used as a
fallback.

> -const char *worktree_git_path(const struct worktree *wt,
> +const char *worktree_git_path(struct repository *r,
> +			      const struct worktree *wt,
>  			      const char *fmt, ...)
> -	__attribute__((format (printf, 2, 3)));
> +	__attribute__((format (printf, 3, 4)));

Overall, I quite like how this series is bubbling up `the_repository`
usages from the bottom up. :)
Patrick Steinhardt Aug. 13, 2024, 9:25 a.m. UTC | #2 
On Fri, Aug 09, 2024 at 02:02:21PM -0500, Justin Tobler wrote:
> On 24/08/07 08:57AM, Patrick Steinhardt wrote:
> > When not provided a worktree, then `worktree_git_path()` will fall back
> > to returning a path relative to the main repository. In this case, we
> > implicitly rely on `the_repository` to derive the path. Remove this
> > dependency by passing a `struct repository` as parameter.
> 
> Are there many situations where `worktree_git_path()` would expect to
> not be provided a worktree? I wonder whether this implicit behavior is
> really necessary to begin with.

Yeah, there are cases. I found that to be somewhat weird at first, but I
does make the logic easier to handle because you don't need to special
case whether you do or don't have a worktree.

> > diff --git a/path.h b/path.h
> > index 3d21b9cd16..6228ca03d7 100644
> > --- a/path.h
> > +++ b/path.h
> > @@ -97,9 +97,10 @@ const char *git_path(const char *fmt, ...)
> >   * Similar to git_path() but can produce paths for a specified
> >   * worktree instead of current one
> >   */
> 
> Now that the previously implicit behavior is more explicit, it might be
> update the comment to explain that the provided repository is used as a
> fallback.

Good idea.

Patrick
diff mbox series

Patch

diff --git a/builtin/fsck.c b/builtin/fsck.c
index d13a226c2e..ad36df9628 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -1050,7 +1050,7 @@  int cmd_fsck(int argc, const char **argv, const char *prefix)
 			 * and may get overwritten by other calls
 			 * while we're examining the index.
 			 */
-			path = xstrdup(worktree_git_path(wt, "index"));
+			path = xstrdup(worktree_git_path(the_repository, wt, "index"));
 			read_index_from(&istate, path, get_worktree_git_dir(wt));
 			fsck_index(&istate, path, wt->is_current);
 			discard_index(&istate);
diff --git a/builtin/worktree.c b/builtin/worktree.c
index a4b7f24e1e..eb0a386992 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -1146,14 +1146,14 @@  static void validate_no_submodules(const struct worktree *wt)
 	struct strbuf path = STRBUF_INIT;
 	int i, found_submodules = 0;
 
-	if (is_directory(worktree_git_path(wt, "modules"))) {
+	if (is_directory(worktree_git_path(the_repository, wt, "modules"))) {
 		/*
 		 * There could be false positives, e.g. the "modules"
 		 * directory exists but is empty. But it's a rare case and
 		 * this simpler check is probably good enough for now.
 		 */
 		found_submodules = 1;
-	} else if (read_index_from(&istate, worktree_git_path(wt, "index"),
+	} else if (read_index_from(&istate, worktree_git_path(the_repository, wt, "index"),
 				   get_worktree_git_dir(wt)) > 0) {
 		for (i = 0; i < istate.cache_nr; i++) {
 			struct cache_entry *ce = istate.cache[i];
diff --git a/path.c b/path.c
index d6bdb992ba..567eff5253 100644
--- a/path.c
+++ b/path.c
@@ -512,12 +512,17 @@  const char *mkpath(const char *fmt, ...)
 	return cleanup_path(pathname->buf);
 }
 
-const char *worktree_git_path(const struct worktree *wt, const char *fmt, ...)
+const char *worktree_git_path(struct repository *r,
+			      const struct worktree *wt, const char *fmt, ...)
 {
 	struct strbuf *pathname = get_pathname();
 	va_list args;
+
+	if (wt && wt->repo != r)
+		BUG("worktree not connected to expected repository");
+
 	va_start(args, fmt);
-	repo_git_pathv(the_repository, wt, pathname, fmt, args);
+	repo_git_pathv(r, wt, pathname, fmt, args);
 	va_end(args);
 	return pathname->buf;
 }
diff --git a/path.h b/path.h
index 3d21b9cd16..6228ca03d7 100644
--- a/path.h
+++ b/path.h
@@ -97,9 +97,10 @@  const char *git_path(const char *fmt, ...)
  * Similar to git_path() but can produce paths for a specified
  * worktree instead of current one
  */
-const char *worktree_git_path(const struct worktree *wt,
+const char *worktree_git_path(struct repository *r,
+			      const struct worktree *wt,
 			      const char *fmt, ...)
-	__attribute__((format (printf, 2, 3)));
+	__attribute__((format (printf, 3, 4)));
 
 /*
  * Return a path into the main repository's (the_repository) git directory.
diff --git a/revision.c b/revision.c
index 1c0192f522..0b92a13af5 100644
--- a/revision.c
+++ b/revision.c
@@ -1872,7 +1872,7 @@  void add_index_objects_to_pending(struct rev_info *revs, unsigned int flags)
 			continue; /* current index already taken care of */
 
 		if (read_index_from(&istate,
-				    worktree_git_path(wt, "index"),
+				    worktree_git_path(the_repository, wt, "index"),
 				    get_worktree_git_dir(wt)) > 0)
 			do_add_index_objects_to_pending(revs, &istate, flags);
 		discard_index(&istate);
diff --git a/worktree.c b/worktree.c
index f3c4c8ec54..886c5db691 100644
--- a/worktree.c
+++ b/worktree.c
@@ -252,7 +252,7 @@  const char *worktree_lock_reason(struct worktree *wt)
 	if (!wt->lock_reason_valid) {
 		struct strbuf path = STRBUF_INIT;
 
-		strbuf_addstr(&path, worktree_git_path(wt, "locked"));
+		strbuf_addstr(&path, worktree_git_path(the_repository, wt, "locked"));
 		if (file_exists(path.buf)) {
 			struct strbuf lock_reason = STRBUF_INIT;
 			if (strbuf_read_file(&lock_reason, path.buf, 0) < 0)
diff --git a/wt-status.c b/wt-status.c
index b778eef989..b477239039 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1618,7 +1618,7 @@  static char *get_branch(const struct worktree *wt, const char *path)
 	struct object_id oid;
 	const char *branch_name;
 
-	if (strbuf_read_file(&sb, worktree_git_path(wt, "%s", path), 0) <= 0)
+	if (strbuf_read_file(&sb, worktree_git_path(the_repository, wt, "%s", path), 0) <= 0)
 		goto got_nothing;
 
 	while (sb.len && sb.buf[sb.len - 1] == '\n')
@@ -1716,18 +1716,18 @@  int wt_status_check_rebase(const struct worktree *wt,
 {
 	struct stat st;
 
-	if (!stat(worktree_git_path(wt, "rebase-apply"), &st)) {
-		if (!stat(worktree_git_path(wt, "rebase-apply/applying"), &st)) {
+	if (!stat(worktree_git_path(the_repository, wt, "rebase-apply"), &st)) {
+		if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/applying"), &st)) {
 			state->am_in_progress = 1;
-			if (!stat(worktree_git_path(wt, "rebase-apply/patch"), &st) && !st.st_size)
+			if (!stat(worktree_git_path(the_repository, wt, "rebase-apply/patch"), &st) && !st.st_size)
 				state->am_empty_patch = 1;
 		} else {
 			state->rebase_in_progress = 1;
 			state->branch = get_branch(wt, "rebase-apply/head-name");
 			state->onto = get_branch(wt, "rebase-apply/onto");
 		}
-	} else if (!stat(worktree_git_path(wt, "rebase-merge"), &st)) {
-		if (!stat(worktree_git_path(wt, "rebase-merge/interactive"), &st))
+	} else if (!stat(worktree_git_path(the_repository, wt, "rebase-merge"), &st)) {
+		if (!stat(worktree_git_path(the_repository, wt, "rebase-merge/interactive"), &st))
 			state->rebase_interactive_in_progress = 1;
 		else
 			state->rebase_in_progress = 1;
@@ -1743,7 +1743,7 @@  int wt_status_check_bisect(const struct worktree *wt,
 {
 	struct stat st;
 
-	if (!stat(worktree_git_path(wt, "BISECT_LOG"), &st)) {
+	if (!stat(worktree_git_path(the_repository, wt, "BISECT_LOG"), &st)) {
 		state->bisect_in_progress = 1;
 		state->bisecting_from = get_branch(wt, "BISECT_START");
 		return 1;