Download raw body.
change got_worktree_init, open_worktree to use fds
Also, I noticed that got_worktree_open 'crawls up' directories
like got_repo_open. While it isn't an issue now, adapting it for
Capsicum will need a function like "is_worktree" (similar to
is_git_repo()). What would such a function need to do?
On Mon, Nov 30, 2020 at 9:42 AM Yang Zhong <yzhong@freebsdfoundation.org> wrote:
>
> Here are some simple changes for initializing and opening worktrees.
> In addition to open() to openat()-style changes, I've also added
> fd fields to the worktree struct, as I'll need it for changes like
> this in other parts of the program.
>
> The helper update_meta_file wasn't changed to use fds here, as it
> requires a function like mkostempsat().
>
> I've changed the signature of got_worktree_init to take in an fd, as
> it's only called in one place. I understand that changing
> got_worktree_open's signature should be avoided, so currently I have
> got_worktree_open open the fd inside the function instead of taking
> it in. I'm not sure what the best solution is in this case.
>
> I've also changed got_worktree_open to no longer call realpath() on
> the worktree_path. Instead, where it is needed I make the path
> absolute beforehand (It's only needed for checkout). This is more
> amenable to Capsicum, and doesn't require much change.
>
> I think these changes are mostly upstream-friendly. I would appreciate
> if you could evaluate the direction of these changes.
>
> diff --git a/got/got.c b/got/got.c
> index 0357368c..33c8cfa6 100644
> --- a/got/got.c
> +++ b/got/got.c
> @@ -2711,6 +2711,20 @@ cmd_checkout(int argc, char *argv[])
> goto done;
> }
> }
> + int worktree_fd = open(worktree_path, O_DIRECTORY);
> + if (worktree_fd == -1) {
> + error = got_error_from_errno2("open", worktree_path);
> + goto done;
> + }
> +
> + /* Make worktree path absolute */
> + char *worktree_path_abs = realpath(worktree_path, NULL);
> + if (worktree_path_abs == NULL) {
> + error = got_error_from_errno2("realpath", worktree_path_abs);
> + goto done;
> + }
> + free(worktree_path);
> + worktree_path = worktree_path_abs;
>
> error = apply_unveil(got_repo_get_path(repo), 0, worktree_path);
> if (error)
> @@ -2720,7 +2734,7 @@ cmd_checkout(int argc, char *argv[])
> if (error != NULL)
> goto done;
>
> - error = got_worktree_init(worktree_path, head_ref, path_prefix, repo);
> + error = got_worktree_init(worktree_fd, worktree_path, head_ref,
> path_prefix, repo);
> if (error != NULL && !(error->code == GOT_ERR_ERRNO && errno == EEXIST))
> goto done;
>
> diff --git a/include/got_path.h b/include/got_path.h
> index 0219d3d3..04558384 100644
> --- a/include/got_path.h
> +++ b/include/got_path.h
> @@ -132,3 +132,4 @@ const struct got_error *got_path_find_prog(char
> **, const char *);
>
> /* Create a new file at a specified path, with optional content. */
> const struct got_error *got_path_create_file(const char *, const char *);
> +const struct got_error *got_path_create_fileat(int, const char *,
> const char *);
> diff --git a/include/got_worktree.h b/include/got_worktree.h
> index 24fddd52..13b13521 100644
> --- a/include/got_worktree.h
> +++ b/include/got_worktree.h
> @@ -42,12 +42,11 @@ struct got_fileindex;
> /*
> * Attempt to initialize a new work tree on disk.
> * The first argument is the path to a directory where the work tree
> - * will be created. The path itself must not yet exist, but the dirname(3)
> - * of the path must already exist.
> + * will be created. The dirname(3) of the path must already exist.
> * The reference provided will be used to determine the new worktree's
> * base commit. The third argument speficies the work tree's path prefix.
> */
> -const struct got_error *got_worktree_init(const char *, struct got_reference *,
> +const struct got_error *got_worktree_init(int, const char *, struct
> got_reference *,
> const char *, struct got_repository *);
>
> /*
> diff --git a/lib/got_lib_worktree.h b/lib/got_lib_worktree.h
> index 067bac22..2a1f9582 100644
> --- a/lib/got_lib_worktree.h
> +++ b/lib/got_lib_worktree.h
> @@ -17,6 +17,8 @@
> struct got_worktree {
> char *root_path;
> char *repo_path;
> + int root_fd;
> + int repo_fd;
> char *path_prefix;
> struct got_object_id *base_commit_id;
> char *head_ref_name;
> diff --git a/lib/path.c b/lib/path.c
> index a670334c..655304a0 100644
> --- a/lib/path.c
> +++ b/lib/path.c
> @@ -513,11 +513,17 @@ got_path_find_prog(char **filename, const char *prog)
>
> const struct got_error *
> got_path_create_file(const char *path, const char *content)
> +{
> + return got_path_create_fileat(AT_FDCWD, path, content);
> +}
> +
> +const struct got_error *
> +got_path_create_fileat(int dir_fd, const char *path, const char *content)
> {
> const struct got_error *err = NULL;
> int fd = -1;
>
> - fd = open(path, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW,
> + fd = openat(dir_fd, path, O_RDWR | O_CREAT | O_EXCL | O_NOFOLLOW,
> GOT_DEFAULT_FILE_MODE);
> if (fd == -1) {
> err = got_error_from_errno2("open", path);
> diff --git a/lib/worktree.c b/lib/worktree.c
> index 2e265b00..30ce5dea 100644
> --- a/lib/worktree.c
> +++ b/lib/worktree.c
> @@ -67,15 +67,15 @@
> #define GOT_MERGE_LABEL_BASE "3-way merge base"
>
> static const struct got_error *
> -create_meta_file(const char *path_got, const char *name, const char *content)
> +create_meta_file(int worktree_fd, const char *name, const char *content)
> {
> const struct got_error *err = NULL;
> char *path;
>
> - if (asprintf(&path, "%s/%s", path_got, name) == -1)
> + if (asprintf(&path, "%s/%s", GOT_WORKTREE_GOT_DIR, name) == -1)
> return got_error_from_errno("asprintf");
>
> - err = got_path_create_file(path, content);
> + err = got_path_create_fileat(worktree_fd, path, content);
> free(path);
> return err;
> }
> @@ -120,7 +120,7 @@ done:
> }
>
> static const struct got_error *
> -read_meta_file(char **content, const char *path_got, const char *name)
> +read_meta_file(char **content, int worktree_fd, const char *path_got,
> const char *name)
> {
> const struct got_error *err = NULL;
> char *path;
> @@ -136,7 +136,7 @@ read_meta_file(char **content, const char
> *path_got, const char *name)
> goto done;
> }
>
> - fd = open(path, O_RDONLY | O_NOFOLLOW);
> + fd = openat(worktree_fd, path, O_RDONLY | O_NOFOLLOW);
> if (fd == -1) {
> if (errno == ENOENT)
> err = got_error_path(path, GOT_ERR_WORKTREE_META);
> @@ -204,7 +204,7 @@ write_head_ref(const char *path_got, struct
> got_reference *head_ref)
> }
>
> const struct got_error *
> -got_worktree_init(const char *path, struct got_reference *head_ref,
> +got_worktree_init(int fd, const char *path, struct got_reference *head_ref,
> const char *prefix, struct got_repository *repo)
> {
> const struct got_error *err = NULL;
> @@ -237,29 +237,23 @@ got_worktree_init(const char *path, struct
> got_reference *head_ref,
> return got_error_from_errno("asprintf");
> }
>
> - /* Create top-level directory (may already exist). */
> - if (mkdir(path, GOT_DEFAULT_DIR_MODE) == -1 && errno != EEXIST) {
> - err = got_error_from_errno2("mkdir", path);
> - goto done;
> - }
> -
> /* Create .got directory (may already exist). */
> if (asprintf(&path_got, "%s/%s", path, GOT_WORKTREE_GOT_DIR) == -1) {
> err = got_error_from_errno("asprintf");
> goto done;
> }
> - if (mkdir(path_got, GOT_DEFAULT_DIR_MODE) == -1 && errno != EEXIST) {
> + if (mkdirat(fd, GOT_WORKTREE_GOT_DIR, GOT_DEFAULT_DIR_MODE) == -1
> && errno != EEXIST) {
> err = got_error_from_errno2("mkdir", path_got);
> goto done;
> }
>
> /* Create an empty lock file. */
> - err = create_meta_file(path_got, GOT_WORKTREE_LOCK, NULL);
> + err = create_meta_file(fd, GOT_WORKTREE_LOCK, NULL);
> if (err)
> goto done;
>
> /* Create an empty file index. */
> - err = create_meta_file(path_got, GOT_WORKTREE_FILE_INDEX, NULL);
> + err = create_meta_file(fd, GOT_WORKTREE_FILE_INDEX, NULL);
> if (err)
> goto done;
>
> @@ -272,18 +266,18 @@ got_worktree_init(const char *path, struct
> got_reference *head_ref,
> err = got_object_id_str(&basestr, commit_id);
> if (err)
> goto done;
> - err = create_meta_file(path_got, GOT_WORKTREE_BASE_COMMIT, basestr);
> + err = create_meta_file(fd, GOT_WORKTREE_BASE_COMMIT, basestr);
> if (err)
> goto done;
>
> /* Store path to repository. */
> - err = create_meta_file(path_got, GOT_WORKTREE_REPOSITORY,
> + err = create_meta_file(fd, GOT_WORKTREE_REPOSITORY,
> got_repo_get_path(repo));
> if (err)
> goto done;
>
> /* Store in-repository path prefix. */
> - err = create_meta_file(path_got, GOT_WORKTREE_PATH_PREFIX,
> + err = create_meta_file(fd, GOT_WORKTREE_PATH_PREFIX,
> absprefix ? absprefix : prefix);
> if (err)
> goto done;
> @@ -299,7 +293,7 @@ got_worktree_init(const char *path, struct
> got_reference *head_ref,
> err = got_error_uuid(uuid_status, "uuid_to_string");
> goto done;
> }
> - err = create_meta_file(path_got, GOT_WORKTREE_UUID, uuidstr);
> + err = create_meta_file(fd, GOT_WORKTREE_UUID, uuidstr);
> if (err)
> goto done;
>
> @@ -308,7 +302,7 @@ got_worktree_init(const char *path, struct
> got_reference *head_ref,
> err = got_error_from_errno("asprintf");
> goto done;
> }
> - err = create_meta_file(path_got, GOT_WORKTREE_FORMAT, formatstr);
> + err = create_meta_file(fd, GOT_WORKTREE_FORMAT, formatstr);
> if (err)
> goto done;
>
> @@ -323,10 +317,9 @@ done:
> }
>
> static const struct got_error *
> -open_worktree(struct got_worktree **worktree, const char *path)
> +open_worktree(struct got_worktree **worktree, int worktree_fd, const
> char *path)
> {
> const struct got_error *err = NULL;
> - char *path_got;
> char *formatstr = NULL;
> char *uuidstr = NULL;
> char *path_lock = NULL;
> @@ -338,26 +331,20 @@ open_worktree(struct got_worktree **worktree,
> const char *path)
>
> *worktree = NULL;
>
> - if (asprintf(&path_got, "%s/%s", path, GOT_WORKTREE_GOT_DIR) == -1) {
> - err = got_error_from_errno("asprintf");
> - path_got = NULL;
> - goto done;
> - }
> -
> - if (asprintf(&path_lock, "%s/%s", path_got, GOT_WORKTREE_LOCK) == -1) {
> + if (asprintf(&path_lock, "%s/%s", GOT_WORKTREE_GOT_DIR,
> GOT_WORKTREE_LOCK) == -1) {
> err = got_error_from_errno("asprintf");
> path_lock = NULL;
> goto done;
> }
>
> - fd = open(path_lock, O_RDWR | O_EXLOCK | O_NONBLOCK);
> + fd = openat(worktree_fd, path_lock, O_RDWR | O_EXLOCK | O_NONBLOCK);
> if (fd == -1) {
> err = (errno == EWOULDBLOCK ? got_error(GOT_ERR_WORKTREE_BUSY)
> : got_error_from_errno2("open", path_lock));
> goto done;
> }
>
> - err = read_meta_file(&formatstr, path_got, GOT_WORKTREE_FORMAT);
> + err = read_meta_file(&formatstr, worktree_fd,
> GOT_WORKTREE_GOT_DIR, GOT_WORKTREE_FORMAT);
> if (err)
> goto done;
>
> @@ -379,27 +366,27 @@ open_worktree(struct got_worktree **worktree,
> const char *path)
> }
> (*worktree)->lockfd = -1;
>
> - (*worktree)->root_path = realpath(path, NULL);
> + (*worktree)->root_path = strdup(path);
> if ((*worktree)->root_path == NULL) {
> - err = got_error_from_errno2("realpath", path);
> + err = got_error_from_errno2("strdup", path);
> goto done;
> }
> - err = read_meta_file(&(*worktree)->repo_path, path_got,
> + err = read_meta_file(&(*worktree)->repo_path, worktree_fd,
> GOT_WORKTREE_GOT_DIR,
> GOT_WORKTREE_REPOSITORY);
> if (err)
> goto done;
>
> - err = read_meta_file(&(*worktree)->path_prefix, path_got,
> + err = read_meta_file(&(*worktree)->path_prefix, worktree_fd,
> GOT_WORKTREE_GOT_DIR,
> GOT_WORKTREE_PATH_PREFIX);
> if (err)
> goto done;
>
> - err = read_meta_file(&base_commit_id_str, path_got,
> + err = read_meta_file(&base_commit_id_str, worktree_fd,
> GOT_WORKTREE_GOT_DIR,
> GOT_WORKTREE_BASE_COMMIT);
> if (err)
> goto done;
>
> - err = read_meta_file(&uuidstr, path_got, GOT_WORKTREE_UUID);
> + err = read_meta_file(&uuidstr, worktree_fd, GOT_WORKTREE_GOT_DIR,
> GOT_WORKTREE_UUID);
> if (err)
> goto done;
> uuid_from_string(uuidstr, &(*worktree)->uuid, &uuid_status);
> @@ -417,7 +404,7 @@ open_worktree(struct got_worktree **worktree,
> const char *path)
> if (err)
> goto done;
>
> - err = read_meta_file(&(*worktree)->head_ref_name, path_got,
> + err = read_meta_file(&(*worktree)->head_ref_name, worktree_fd,
> GOT_WORKTREE_GOT_DIR,
> GOT_WORKTREE_HEAD_REF);
> if (err)
> goto done;
> @@ -431,10 +418,14 @@ open_worktree(struct got_worktree **worktree,
> const char *path)
>
> err = got_gotconfig_read(&(*worktree)->gotconfig,
> (*worktree)->gotconfig_path);
> +
> + int repo_fd = open(got_repo_get_path_git_dir(repo), O_DIRECTORY);
> +
> + (*worktree)->root_fd = worktree_fd;
> + (*worktree)->repo_fd = repo_fd;
> done:
> if (repo)
> got_repo_close(repo);
> - free(path_got);
> free(path_lock);
> free(base_commit_id_str);
> free(uuidstr);
> @@ -456,15 +447,20 @@ got_worktree_open(struct got_worktree
> **worktree, const char *path)
> {
> const struct got_error *err = NULL;
> char *worktree_path;
> + int worktree_fd;
>
> worktree_path = strdup(path);
> if (worktree_path == NULL)
> return got_error_from_errno("strdup");
>
> + worktree_fd = open(worktree_path, O_DIRECTORY);
> + if (worktree_fd == -1)
> + return got_error_from_errno2("open", worktree_path);
> +
> for (;;) {
> char *parent_path;
>
> - err = open_worktree(worktree, worktree_path);
> + err = open_worktree(worktree, worktree_fd, worktree_path);
> if (err && !(err->code == GOT_ERR_ERRNO && errno == ENOENT)) {
> free(worktree_path);
> return err;
> @@ -484,7 +480,9 @@ got_worktree_open(struct got_worktree **worktree,
> const char *path)
> break;
> }
> free(worktree_path);
> + close(worktree_fd);
> worktree_path = parent_path;
> + worktree_fd = open(parent_path, O_DIRECTORY);
> }
>
> free(worktree_path);
> @@ -507,6 +505,8 @@ got_worktree_close(struct got_worktree *worktree)
> free(worktree->gotconfig_path);
> got_gotconfig_free(worktree->gotconfig);
> free(worktree);
> + close(worktree->root_fd);
> + close(worktree->repo_fd);
> return err;
> }
change got_worktree_init, open_worktree to use fds