"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
avoid got_repo_map_path() in 'got blame'
To:
gameoftrees@openbsd.org
Date:
Tue, 3 Nov 2020 17:09:12 +0100

Download raw body.

Thread
  • Stefan Sperling:

    avoid got_repo_map_path() in 'got blame'

This patch eliminates got_repo_map_path() for the path argument of
'got blame' if the corresponding in-repository path can be resolved
via a work tree.

'got blame' does not need access to the work tree. So far the work tree
was completely hidden with unveil(). We must now expose the work tree
while resolving the path, so unveil() calls are shuffled around slightly.
Failing realpath() calls would mess with path resolution otherwise.

There's a bug in got_worktree_resolve_path() where it failed to canonicalize
a path constructed from a user-specified path that does not exist on disk.
Note that this path falls into strncmp() a few lines down. I am fixing this
by adding canonicalization. Generally, joining paths with asprintf() and
comparing paths with strncmp() is fragile. A more general solution might be
needed but I am leaving that for another day.

ok?

diff 7f9bfb3188bd9d77317f3205b61e96c7a55b005a /home/stsp/src/got
blob - 3d41cd3c7dba250edd22bdb2f232bd169acc4944
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -4503,24 +4503,26 @@ cmd_blame(int argc, char *argv[])
 	if (error != NULL)
 		goto done;
 
-	error = apply_unveil(got_repo_get_path(repo), 1, NULL);
-	if (error)
-		goto done;
-
 	if (worktree) {
 		const char *prefix = got_worktree_get_path_prefix(worktree);
-		char *p, *worktree_subdir = cwd +
-		    strlen(got_worktree_get_root_path(worktree));
-		if (asprintf(&p, "%s%s%s%s%s",
-		    prefix, (strcmp(prefix, "/") != 0) ? "/" : "",
-		    worktree_subdir, worktree_subdir[0] ? "/" : "",
-		    path) == -1) {
+		char *p;
+
+		error = got_worktree_resolve_path(&p, worktree, path);
+		if (error)
+			goto done;
+		if (asprintf(&in_repo_path, "%s%s%s", prefix,
+		    (p[0] != '\0' && !got_path_is_root_dir(prefix)) ? "/" : "",
+		    p) == -1) {
 			error = got_error_from_errno("asprintf");
+			free(p);
 			goto done;
 		}
-		error = got_repo_map_path(&in_repo_path, repo, p, 0);
 		free(p);
+		error = apply_unveil(got_repo_get_path(repo), 1, NULL);
 	} else {
+		error = apply_unveil(got_repo_get_path(repo), 1, NULL);
+		if (error)
+			goto done;
 		error = got_repo_map_path(&in_repo_path, repo, path, 1);
 	}
 	if (error)
blob - 3d3b9381b3045c394b4e7d7f5a5e247f44cb5ecc
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -3625,6 +3625,8 @@ got_worktree_resolve_path(char **wt_path, struct got_w
 	char *resolved = NULL, *cwd = NULL, *path = NULL;
 	size_t len;
 	struct stat sb;
+	char *abspath = NULL;
+	char canonpath[PATH_MAX];
 
 	*wt_path = NULL;
 
@@ -3645,8 +3647,6 @@ got_worktree_resolve_path(char **wt_path, struct got_w
 		 * But we can make the path absolute, assuming it is relative
 		 * to the current working directory, and then canonicalize it.
 		 */
-		char *abspath = NULL;
-		char canonpath[PATH_MAX];
 		if (!got_path_is_absolute(arg)) {
 			if (asprintf(&abspath, "%s/%s", cwd, arg) == -1) {
 				err = got_error_from_errno("asprintf");
@@ -3670,10 +3670,19 @@ got_worktree_resolve_path(char **wt_path, struct got_w
 				err = got_error_from_errno2("realpath", arg);
 				goto done;
 			}
-			if (asprintf(&resolved, "%s/%s", cwd, arg) == -1) {
+			if (asprintf(&abspath, "%s/%s", cwd, arg) == -1) {
 				err = got_error_from_errno("asprintf");
 				goto done;
 			}
+			err = got_canonpath(abspath, canonpath,
+			    sizeof(canonpath));
+			if (err)
+				goto done;
+			resolved = strdup(canonpath);
+			if (resolved == NULL) {
+				err = got_error_from_errno("strdup");
+				goto done;
+			}
 		}
 	}
 
@@ -3703,6 +3712,7 @@ got_worktree_resolve_path(char **wt_path, struct got_w
 		len--;
 	}
 done:
+	free(abspath);
 	free(resolved);
 	free(cwd);
 	if (err == NULL)