git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
shejialuo <shejialuo@gmail.com>
Date
Feb 6, 2025, 15:54 UTC
Message-ID
<Z6TbMGcPftyhUyC3@ArchLinux>
In-Reply-To
<20250206-b4-pks-path-drop-the-repository-v1-10-4e77f0313206@pks.im>
On Thu, Feb 06, 2025 at 08:58:06AM +0100, Patrick Steinhardt wrote:
Show 28 quoted lines
> 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.

Show 22 quoted lines
> +	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.
Show 38 quoted lines
> +	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.
Show 15 quoted lines
> @@ -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.

Show 25 quoted lines
>  	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.

Show 26 quoted lines
>  	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.
Show 24 quoted lines
>  	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.
Show 20 quoted lines
>  	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

Previous: Patrick SteinhardtNext: Patrick Steinhardt
Message 24 of 36 in “path: remove dependency on `the_repository`”
  1. 00/16 path: remove dependency on `the_repository`Patrick Steinhardt, Feb 6, 2025
  2. 01/16 path: refactor `repo_common_path()` family of functionsPatrick Steinhardt, Feb 6, 2025
  3. Karthik NayakFeb 6, 2025
  4. Patrick SteinhardtFeb 7, 2025
  5. shejialuoFeb 6, 2025
  6. Patrick SteinhardtFeb 7, 2025
  7. 02/16 path: refactor `repo_git_path()` family of functionsPatrick Steinhardt, Feb 6, 2025
  8. Karthik NayakFeb 6, 2025
  9. Patrick SteinhardtFeb 7, 2025
  10. 03/16 path: refactor `repo_worktree_path()` family of functionsPatrick Steinhardt, Feb 6, 2025
  11. 04/16 submodule: refactor `submodule_to_gitdir()` to accept a repoPatrick Steinhardt, Feb 6, 2025
  12. 05/16 path: refactor `repo_submodule_path()` family of functionsPatrick Steinhardt, Feb 6, 2025
  13. Karthik NayakFeb 6, 2025
  14. Patrick SteinhardtFeb 7, 2025
  15. Karthik NayakFeb 7, 2025
  16. shejialuoFeb 6, 2025
  17. 06/16 path: drop unused `strbuf_git_path()` functionPatrick Steinhardt, Feb 6, 2025
  18. 07/16 path: drop `git_pathdup()` in favor of `repo_git_path()`Patrick Steinhardt, Feb 6, 2025
  19. 08/16 path: drop `git_path_buf()` in favor of `repo_git_path_replace()`Patrick Steinhardt, Feb 6, 2025
  20. 09/16 worktree: return allocated string from `get_worktree_git_dir()`Patrick Steinhardt, Feb 6, 2025
  21. Karthik NayakFeb 7, 2025
  22. Patrick SteinhardtFeb 7, 2025
  23. 10/16 path: drop `git_common_path()` in favor of `repo_common_path()`Patrick Steinhardt, Feb 6, 2025
  24. shejialuoFeb 6, 2025
  25. Patrick SteinhardtFeb 7, 2025
  26. 11/16 rerere: let `rerere_path()` write paths into a caller-provided bufferPatrick Steinhardt, Feb 6, 2025
  27. 12/16 path: drop `git_path()` in favor of `repo_git_path()`Patrick Steinhardt, Feb 6, 2025
  28. shejialuoFeb 6, 2025
  29. Patrick SteinhardtFeb 7, 2025
  30. 13/16 repo-settings: introduce function to clear structPatrick Steinhardt, Feb 6, 2025
  31. 14/16 environment: move access to "core.hooksPath" into repo settingsPatrick Steinhardt, Feb 6, 2025
  32. 15/16 environment: move access to "core.sharedRepository" into repo settingsPatrick Steinhardt, Feb 6, 2025
  33. 16/16 path: adjust last remaining users of `the_repository`Patrick Steinhardt, Feb 6, 2025
  34. shejialuoFeb 6, 2025
  35. Patrick SteinhardtFeb 7, 2025
  36. Karthik NayakFeb 7, 2025

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.