Re: [PATCH 10/16] path: drop `git_common_path()` in favor of `repo_common_path()`

[shejialuo <shejialuo@gmail.com>]

On Thu, Feb 06, 2025 at 08:58:06AM +0100, Patrick Steinhardt wrote:


> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 2cea9441a6..761e302a36 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -151,7 +151,7 @@ static int delete_git_dir(const char *id)
>  	struct strbuf sb = STRBUF_INIT;
>  	int ret;
>  
> -	strbuf_addstr(&sb, git_common_path("worktrees/%s", id));
> +	repo_common_path_append(the_repository, &sb, "worktrees/%s", id);
>  	ret = remove_dir_recursively(&sb, 0);
>  	if (ret < 0 && errno == ENOTDIR)
>  		ret = unlink(sb.buf);
> @@ -1102,6 +1102,7 @@ static int lock_worktree(int ac, const char **av, const char *prefix,
>  		OPT_END()
>  	};
>  	struct worktree **worktrees, *wt;
> +	char *path;
>  
>  	ac = parse_options(ac, av, prefix, options, git_worktree_lock_usage, 0);
>  	if (ac != 1)
> @@ -1122,9 +1123,11 @@ static int lock_worktree(int ac, const char **av, const char *prefix,
>  		die(_("'%s' is already locked"), av[0]);
>  	}
>  
> -	write_file(git_common_path("worktrees/%s/locked", wt->id),
> -		   "%s", reason);
> +	path = repo_common_path(the_repository, "worktrees/%s/locked", wt->id);

From my perspective, we may use `repo_common_path_replace` here to avoid
using the raw string pointer? This is because we return a changeable
pointer "char *". But we pass this pointer to a "const char *". This is
not critical, but we may make the semantics clearer.

> +	write_file(path, "%s", reason);
> +
>  	free_worktrees(worktrees);
> +	free(path);
>  	return 0;
>  }
>  
> @@ -1135,6 +1138,7 @@ static int unlock_worktree(int ac, const char **av, const char *prefix,
>  		OPT_END()
>  	};
>  	struct worktree **worktrees, *wt;
> +	char *path;
>  	int ret;
>  
>  	ac = parse_options(ac, av, prefix, options, git_worktree_unlock_usage, 0);
> @@ -1149,8 +1153,12 @@ static int unlock_worktree(int ac, const char **av, const char *prefix,
>  		die(_("The main working tree cannot be locked or unlocked"));
>  	if (!worktree_lock_reason(wt))
>  		die(_("'%s' is not locked"), av[0]);
> -	ret = unlink_or_warn(git_common_path("worktrees/%s/locked", wt->id));
> +
> +	path = repo_common_path(the_repository, "worktrees/%s/locked", wt->id);

This one as above.

> +	ret = unlink_or_warn(path);
> +
>  	free_worktrees(worktrees);
> +	free(path);
>  	return ret;
>  }
>  
> diff --git a/path.c b/path.c
> index d721507be8..f6b795d75f 100644
> --- a/path.c
> +++ b/path.c
> @@ -634,10 +634,10 @@ const char *repo_submodule_path_replace(struct repository *repo,
>  	return buf->buf;
>  }
>  
> -void repo_common_pathv(const struct repository *repo,
> -		       struct strbuf *sb,
> -		       const char *fmt,
> -		       va_list args)
> +static void repo_common_pathv(const struct repository *repo,
> +			      struct strbuf *sb,
> +			      const char *fmt,
> +			      va_list args)
>  {
>  	strbuf_addstr(sb, repo->commondir);
>  	if (sb->len && !is_dir_sep(sb->buf[sb->len - 1]))
> diff --git a/path.h b/path.h
> index 904eeac068..496f27fdfd 100644
> --- a/path.h
> +++ b/path.h
> @@ -233,29 +233,10 @@ struct strbuf *get_pathname(void);
>  #  include "repository.h"
>  
>  /* Internal implementation details that should not be used. */
> -void repo_common_pathv(const struct repository *repo,
> -		       struct strbuf *buf,
> -		       const char *fmt,
> -		       va_list args);

So, we finally mark this function "static" and delete the declaration in
this patch. We cannot do this in the earlier patch because
"git_common_path" is defined in the header file and it needs to use this
function. Make sense.

However, I somehow feel a little strange especially in [PATCH 01/16]
that you have added a comment:

    /* Internal implementation detail that should not be used. *

When I see this comment, my first intuitive thinking is that if we
should not use this function, why do we need to expose this in the first
place?

This really introduces confusion.

> @@ -343,7 +344,7 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
>  	if (!is_absolute_path(wt->path)) {
>  		strbuf_addf_gently(errmsg,
>  				   _("'%s' file does not contain absolute path to the working tree location"),
> -				   git_common_path("worktrees/%s/gitdir", wt->id));
> +				   repo_common_path_replace(the_repository, &buf, "worktrees/%s/gitdir", wt->id));
>  		goto done;
>  	}
>  
> @@ -365,14 +366,16 @@ int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
>  		goto done;
>  	}
>  
> -	strbuf_realpath(&realpath, git_common_path("worktrees/%s", wt->id), 1);
> +	strbuf_realpath(&realpath, repo_common_path_replace(the_repository, &buf, "worktrees/%s", wt->id), 1);

We rely on the return value of `repo_common_path_replace` to elegantly
do this. Make sense.

>  	ret = fspathcmp(path, realpath.buf);
>  
>  	if (ret)
>  		strbuf_addf_gently(errmsg, _("'%s' does not point back to '%s'"),
> -				   wt->path, git_common_path("worktrees/%s", wt->id));
> +				   wt->path, repo_common_path_replace(the_repository, &buf,
> +								      "worktrees/%s", wt->id));
>  done:
>  	free(path);
> +	strbuf_release(&buf);
>  	strbuf_release(&wt_path);
>  	strbuf_release(&realpath);
>  	return ret;
> @@ -384,11 +387,13 @@ void update_worktree_location(struct worktree *wt, const char *path_,
>  	struct strbuf path = STRBUF_INIT;
>  	struct strbuf dotgit = STRBUF_INIT;
>  	struct strbuf gitdir = STRBUF_INIT;
> +	char *wt_gitdir;
>  
>  	if (is_main_worktree(wt))
>  		BUG("can't relocate main worktree");
>  
> -	strbuf_realpath(&gitdir, git_common_path("worktrees/%s/gitdir", wt->id), 1);
> +	wt_gitdir = repo_common_path(the_repository, "worktrees/%s/gitdir", wt->id);
> +	strbuf_realpath(&gitdir, wt_gitdir, 1);

Why we don't use above pattern which means the following:

    strbuf_realpath(&gitdir, git_common_path_replace(...), ...);

I think we should be consistent. And we should not use "char *" type to
pass to a "const char *" type here although this won't be harmful to the
program. However, git_common_path_replace will return a "const char *"
to make sure the caller cannot change this pointer.

>  	strbuf_realpath(&path, path_, 1);
>  	strbuf_addf(&dotgit, "%s/.git", path.buf);
>  	if (fspathcmp(wt->path, path.buf)) {
> @@ -400,6 +405,7 @@ void update_worktree_location(struct worktree *wt, const char *path_,
>  	strbuf_release(&path);
>  	strbuf_release(&dotgit);
>  	strbuf_release(&gitdir);
> +	free(wt_gitdir);
>  }
>  
>  int is_worktree_being_rebased(const struct worktree *wt,
> @@ -585,6 +591,7 @@ static void repair_gitfile(struct worktree *wt,
>  	struct strbuf backlink = STRBUF_INIT;
>  	char *dotgit_contents = NULL;
>  	const char *repair = NULL;
> +	char *path = NULL;
>  	int err;
>  
>  	/* missing worktree can't be repaired */
> @@ -596,7 +603,8 @@ static void repair_gitfile(struct worktree *wt,
>  		goto done;
>  	}
>  
> -	strbuf_realpath(&repo, git_common_path("worktrees/%s", wt->id), 1);
> +	path = repo_common_path(the_repository, "worktrees/%s", wt->id);
> +	strbuf_realpath(&repo, path, 1);

This one as above.

>  	strbuf_addf(&dotgit, "%s/.git", wt->path);
>  	strbuf_addf(&gitdir, "%s/gitdir", repo.buf);
>  	dotgit_contents = xstrdup_or_null(read_gitfile_gently(dotgit.buf, &err));
> @@ -626,6 +634,7 @@ static void repair_gitfile(struct worktree *wt,
>  
>  done:
>  	free(dotgit_contents);
> +	free(path);
>  	strbuf_release(&repo);
>  	strbuf_release(&dotgit);
>  	strbuf_release(&gitdir);
> @@ -657,11 +666,13 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path
>  	struct strbuf gitdir = STRBUF_INIT;
>  	struct strbuf dotgit = STRBUF_INIT;
>  	int is_relative_path;
> +	char *path = NULL;
>  
>  	if (is_main_worktree(wt))
>  		goto done;
>  
> -	strbuf_realpath(&gitdir, git_common_path("worktrees/%s/gitdir", wt->id), 1);
> +	path = repo_common_path(the_repository, "worktrees/%s/gitdir", wt->id);
> +	strbuf_realpath(&gitdir, path, 1);
>  

Also this one.

>  	if (strbuf_read_file(&dotgit, gitdir.buf, 0) < 0)
>  		goto done;
> @@ -680,6 +691,7 @@ void repair_worktree_after_gitdir_move(struct worktree *wt, const char *old_path
>  done:
>  	strbuf_release(&gitdir);
>  	strbuf_release(&dotgit);
> +	free(path);
>  }
>  
>  void repair_worktrees_after_gitdir_move(const char *old_path)
> @@ -871,7 +883,11 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
>  	ssize_t read_result;
>  
>  	*wtpath = NULL;
> -	strbuf_realpath(&repo, git_common_path("worktrees/%s", id), 1);
> +
> +	path = repo_common_path(the_repository, "worktrees/%s", id);
> +	strbuf_realpath(&repo, path, 1);
> +	FREE_AND_NULL(path);
> +

I somehow agree that we could use `repo_common_path` in this way where
we want to reuse "path" variable.

Thanks,
Jialuo