Carlos MartÃn Nieto venit, vidit, dixit 16.03.2011 17:51:
> Rename the make_*_path functions so it's clearer what they do, in
> particlar make clear what the differnce between make_absolute_path and
> make_nonrelative_path is by renaming them real_path and absolute_path
> respectively. make_relative_path has an understandable name and is
> renamed to relative_path to maintain the name convention.
>
> The function calls have been replaced 1-to-1 in their usage.
>
> Signed-off-by: Carlos MartÃn Nieto <cmn@xxxxxxxx>
> ---
>
> This supercedes the 2/3 and 3/3 patches.
>
> abspath.c | 22 +++++++++++++++++++---
> builtin/clone.c | 12 ++++++------
> builtin/init-db.c | 8 ++++----
> builtin/receive-pack.c | 2 +-
> cache.h | 6 +++---
> dir.c | 4 ++--
> environment.c | 4 ++--
> exec_cmd.c | 2 +-
> lockfile.c | 4 ++--
> path.c | 2 +-
> setup.c | 14 ++++++--------
> t/t0000-basic.sh | 10 +++++-----
> test-path-utils.c | 4 ++--
> 13 files changed, 54 insertions(+), 40 deletions(-)
>
> diff --git a/abspath.c b/abspath.c
> index ff14068..47bc73e 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -14,7 +14,14 @@ int is_directory(const char *path)
> /* We allow "recursive" symbolic links. Only within reason, though. */
> #define MAXDEPTH 5
>
> -const char *make_absolute_path(const char *path)
> +/*
> + * Use this to get the real path, i.e. resolve links. If you want an
> + * absolute path but don't mind links, use absolute_path.
> + *
> + * If path is our buffer, then return path, as it's already what the
> + * user wants.
> + */
> +const char *real_path(const char *path)
> {
> static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf = bufs[1];
> char cwd[1024] = "";
> @@ -104,13 +111,22 @@ static const char *get_pwd_cwd(void)
> return cwd;
> }
>
> -const char *make_nonrelative_path(const char *path)
> +/*
> + * Use this to get an absolute path from a relative one. If you want
> + * to resolve links, you should use real_path.
> + *
> + * If the path is already absolute, then return path. As the user is
> + * never meant to free the return value, we're safe.
> + */
> +const char *absolute_path(const char *path)
> {
> static char buf[PATH_MAX + 1];
>
> if (is_absolute_path(path)) {
> - if (strlcpy(buf, path, PATH_MAX) >= PATH_MAX)
> + if (strlen(path) >= PATH_MAX)
> die("Too long path: %.*s", 60, path);
> + else
> + return path;
> } else {
> size_t len;
> const char *fmt;
This is not simply a renaming change that you're sneaking in here. What
is it about?
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 60d9a64..780809d 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -100,7 +100,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
> path = mkpath("%s%s", repo, suffix[i]);
> if (is_directory(path)) {
> *is_bundle = 0;
> - return xstrdup(make_nonrelative_path(path));
> + return xstrdup(absolute_path(path));
> }
> }
>
> @@ -109,7 +109,7 @@ static char *get_repo_path(const char *repo, int *is_bundle)
> path = mkpath("%s%s", repo, bundle_suffix[i]);
> if (!stat(path, &st) && S_ISREG(st.st_mode)) {
> *is_bundle = 1;
> - return xstrdup(make_nonrelative_path(path));
> + return xstrdup(absolute_path(path));
> }
> }
>
> @@ -203,7 +203,7 @@ static void setup_reference(const char *repo)
> struct transport *transport;
> const struct ref *extra;
>
> - ref_git = make_absolute_path(option_reference);
> + ref_git = real_path(option_reference);
>
> if (is_directory(mkpath("%s/.git/objects", ref_git)))
> ref_git = mkpath("%s/.git", ref_git);
> @@ -411,9 +411,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>
> path = get_repo_path(repo_name, &is_bundle);
> if (path)
> - repo = xstrdup(make_nonrelative_path(repo_name));
> + repo = xstrdup(absolute_path(repo_name));
> else if (!strchr(repo_name, ':'))
> - repo = xstrdup(make_absolute_path(repo_name));
> + repo = xstrdup(real_path(repo_name));
> else
> repo = repo_name;
> is_local = path && !is_bundle;
> @@ -466,7 +466,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>
> if (safe_create_leading_directories_const(git_dir) < 0)
> die("could not create leading directories of '%s'", git_dir);
> - set_git_dir(make_absolute_path(git_dir));
> + set_git_dir(real_path(git_dir));
>
> if (0 <= option_verbosity)
> printf("Cloning into %s%s...\n",
> diff --git a/builtin/init-db.c b/builtin/init-db.c
> index fbeb380..63cf259 100644
> --- a/builtin/init-db.c
> +++ b/builtin/init-db.c
> @@ -501,7 +501,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
> const char *git_dir_parent = strrchr(git_dir, '/');
> if (git_dir_parent) {
> char *rel = xstrndup(git_dir, git_dir_parent - git_dir);
> - git_work_tree_cfg = xstrdup(make_absolute_path(rel));
> + git_work_tree_cfg = xstrdup(real_path(rel));
> free(rel);
> }
> if (!git_work_tree_cfg) {
> @@ -510,7 +510,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
> die_errno ("Cannot access current working directory");
> }
> if (work_tree)
> - set_git_work_tree(make_absolute_path(work_tree));
> + set_git_work_tree(real_path(work_tree));
> else
> set_git_work_tree(git_work_tree_cfg);
> if (access(get_git_work_tree(), X_OK))
> @@ -519,10 +519,10 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)
> }
> else {
> if (work_tree)
> - set_git_work_tree(make_absolute_path(work_tree));
> + set_git_work_tree(real_path(work_tree));
> }
>
> - set_git_dir(make_absolute_path(git_dir));
> + set_git_dir(real_path(git_dir));
>
> return init_db(template_dir, flags);
> }
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 760817d..d883585 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -740,7 +740,7 @@ static int add_refs_from_alternate(struct alternate_object_database *e, void *un
> const struct ref *extra;
>
> e->name[-1] = '\0';
> - other = xstrdup(make_absolute_path(e->base));
> + other = xstrdup(real_path(e->base));
> e->name[-1] = '/';
> len = strlen(other);
>
> diff --git a/cache.h b/cache.h
> index c7b0a28..dbc87be 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -716,9 +716,9 @@ static inline int is_absolute_path(const char *path)
> return path[0] == '/' || has_dos_drive_prefix(path);
> }
> int is_directory(const char *);
> -const char *make_absolute_path(const char *path);
> -const char *make_nonrelative_path(const char *path);
> -const char *make_relative_path(const char *abs, const char *base);
> +const char *real_path(const char *path);
> +const char *absolute_path(const char *path);
> +const char *relative_path(const char *abs, const char *base);
> int normalize_path_copy(char *dst, const char *src);
> int longest_ancestor_length(const char *path, const char *prefix_list);
> char *strip_path_suffix(const char *path, const char *suffix);
> diff --git a/dir.c b/dir.c
> index 570b651..5a9372a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1023,8 +1023,8 @@ char *get_relative_cwd(char *buffer, int size, const char *dir)
> if (!getcwd(buffer, size))
> die_errno("can't find the current directory");
>
> - if (!is_absolute_path(dir))
> - dir = make_absolute_path(dir);
> + /* getcwd resolves links and gives us the real path */
> + dir = real_path(dir);
Why remove the check? Again, this is not just renaming.
>
> while (*dir && *dir == *cwd) {
> dir++;
> diff --git a/environment.c b/environment.c
> index c3efbb9..cc670b1 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -139,7 +139,7 @@ static int git_work_tree_initialized;
> void set_git_work_tree(const char *new_work_tree)
> {
> if (git_work_tree_initialized) {
> - new_work_tree = make_absolute_path(new_work_tree);
> + new_work_tree = real_path(new_work_tree);
> if (strcmp(new_work_tree, work_tree))
> die("internal error: work tree has already been set\n"
> "Current worktree: %s\nNew worktree: %s",
> @@ -147,7 +147,7 @@ void set_git_work_tree(const char *new_work_tree)
> return;
> }
> git_work_tree_initialized = 1;
> - work_tree = xstrdup(make_absolute_path(new_work_tree));
> + work_tree = xstrdup(real_path(new_work_tree));
> }
>
> const char *get_git_work_tree(void)
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 38545e8..171e841 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -89,7 +89,7 @@ static void add_path(struct strbuf *out, const char *path)
> if (is_absolute_path(path))
> strbuf_addstr(out, path);
> else
> - strbuf_addstr(out, make_nonrelative_path(path));
> + strbuf_addstr(out, absolute_path(path));
>
> strbuf_addch(out, PATH_SEP);
> }
> diff --git a/lockfile.c b/lockfile.c
> index b0d74cd..c6fb77b 100644
> --- a/lockfile.c
> +++ b/lockfile.c
> @@ -164,10 +164,10 @@ static char *unable_to_lock_message(const char *path, int err)
> "If no other git process is currently running, this probably means a\n"
> "git process crashed in this repository earlier. Make sure no other git\n"
> "process is running and remove the file manually to continue.",
> - make_nonrelative_path(path), strerror(err));
> + absolute_path(path), strerror(err));
> } else
> strbuf_addf(&buf, "Unable to create '%s.lock': %s",
> - make_nonrelative_path(path), strerror(err));
> + absolute_path(path), strerror(err));
> return strbuf_detach(&buf, NULL);
> }
>
> diff --git a/path.c b/path.c
> index 8951333..4d73cc9 100644
> --- a/path.c
> +++ b/path.c
> @@ -397,7 +397,7 @@ int set_shared_perm(const char *path, int mode)
> return 0;
> }
>
> -const char *make_relative_path(const char *abs, const char *base)
> +const char *relative_path(const char *abs, const char *base)
> {
> static char buf[PATH_MAX + 1];
> int i = 0, j = 0;
> diff --git a/setup.c b/setup.c
> index dadc666..8cb1ad3 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -216,9 +216,7 @@ void setup_work_tree(void)
> if (initialized)
> return;
> work_tree = get_git_work_tree();
> - git_dir = get_git_dir();
> - if (!is_absolute_path(git_dir))
> - git_dir = make_absolute_path(git_dir);
> + git_dir = real_path(get_git_dir());
Check removed, why?
> if (!work_tree || chdir(work_tree))
> die("This operation must be run in a work tree");
>
> @@ -229,7 +227,7 @@ void setup_work_tree(void)
> if (getenv(GIT_WORK_TREE_ENVIRONMENT))
> setenv(GIT_WORK_TREE_ENVIRONMENT, ".", 1);
>
> - set_git_dir(make_relative_path(git_dir, work_tree));
> + set_git_dir(relative_path(git_dir, work_tree));
> initialized = 1;
> }
>
> @@ -309,7 +307,7 @@ const char *read_gitfile_gently(const char *path)
>
> if (!is_git_directory(dir))
> die("Not a git repository: %s", dir);
> - path = make_absolute_path(dir);
> + path = real_path(dir);
>
> free(buf);
> return path;
> @@ -389,7 +387,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,
>
> if (!prefixcmp(cwd, worktree) &&
> cwd[strlen(worktree)] == '/') { /* cwd inside worktree */
> - set_git_dir(make_absolute_path(gitdirenv));
> + set_git_dir(real_path(gitdirenv));
> if (chdir(worktree))
> die_errno("Could not chdir to '%s'", worktree);
> cwd[len++] = '/';
> @@ -414,7 +412,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
> /* --work-tree is set without --git-dir; use discovered one */
> if (getenv(GIT_WORK_TREE_ENVIRONMENT) || git_work_tree_cfg) {
> if (offset != len && !is_absolute_path(gitdir))
> - gitdir = xstrdup(make_absolute_path(gitdir));
> + gitdir = xstrdup(real_path(gitdir));
> if (chdir(cwd))
> die_errno("Could not come back to cwd");
> return setup_explicit_git_dir(gitdir, cwd, len, nongit_ok);
> @@ -422,7 +420,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,
>
> /* #16.2, #17.2, #20.2, #21.2, #24, #25, #28, #29 (see t1510) */
> if (is_bare_repository_cfg > 0) {
> - set_git_dir(offset == len ? gitdir : make_absolute_path(gitdir));
> + set_git_dir(offset == len ? gitdir : real_path(gitdir));
> if (chdir(cwd))
> die_errno("Could not come back to cwd");
> return NULL;
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index 8deec75..f4e8f43 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -435,7 +435,7 @@ test_expect_success 'update-index D/F conflict' '
> test $numpath0 = 1
> '
>
> -test_expect_success SYMLINKS 'absolute path works as expected' '
> +test_expect_success SYMLINKS 'real path works as expected' '
> mkdir first &&
> ln -s ../.git first/.git &&
> mkdir second &&
> @@ -443,14 +443,14 @@ test_expect_success SYMLINKS 'absolute path works as expected' '
> mkdir third &&
> dir="$(cd .git; pwd -P)" &&
> dir2=third/../second/other/.git &&
> - test "$dir" = "$(test-path-utils make_absolute_path $dir2)" &&
> + test "$dir" = "$(test-path-utils real_path $dir2)" &&
> file="$dir"/index &&
> - test "$file" = "$(test-path-utils make_absolute_path $dir2/index)" &&
> + test "$file" = "$(test-path-utils real_path $dir2/index)" &&
> basename=blub &&
> - test "$dir/$basename" = "$(cd .git && test-path-utils make_absolute_path "$basename")" &&
> + test "$dir/$basename" = "$(cd .git && test-path-utils real_path "$basename")" &&
> ln -s ../first/file .git/syml &&
> sym="$(cd first; pwd -P)"/file &&
> - test "$sym" = "$(test-path-utils make_absolute_path "$dir2/syml")"
> + test "$sym" = "$(test-path-utils real_path "$dir2/syml")"
> '
>
> test_expect_success 'very long name in the index handled sanely' '
> diff --git a/test-path-utils.c b/test-path-utils.c
> index d261398..e767159 100644
> --- a/test-path-utils.c
> +++ b/test-path-utils.c
> @@ -11,9 +11,9 @@ int main(int argc, char **argv)
> return 0;
> }
>
> - if (argc >= 2 && !strcmp(argv[1], "make_absolute_path")) {
> + if (argc >= 2 && !strcmp(argv[1], "real_path")) {
> while (argc > 2) {
> - puts(make_absolute_path(argv[2]));
> + puts(real_path(argv[2]));
> argc--;
> argv++;
> }
I think you should strictly separate the renaming issue from other
patches (and describe/motivate the latter). It's fine to have a large
patch with mechanical changes (renaming) if you stick to just those.
Cheers,
Michael
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html