On Wed, Oct 21, 2020 at 10:04:36PM +0200, Christian Weisgerber wrote:
> I tried to take another look at got_repo_map_path().
> 
> Reminder: If unveil() is disabled, some command line uses break.
> 
> $ cd /home/naddy/got    # a worktree
> $ cd tog
> $ got log -r /home/naddy/got.git tog.1
> got: home/naddy/got/tog/tog.1: no such entry found in tree
> 
> The problem is in got_repo_map_path(), principally in the block
> that starts with
>                 path = realpath(canonpath, NULL);
> 
> If unveil() is enabled, the realpath() call will fail, which looks
> like a bug.
> 
> However, without unveil() the rest of the block is executed and
> the example above ends up here:
>                         /*
>                          * Matched unrelated on-disk path.
>                          * Treat it as repository-relative.
>                          */
> 
> I'm struggling to understand the _intended_ semantics of the various
> cases handled by realpath(), so I can't tell what's actually wrong.
> 
> Generally speaking, the idea of converting a relative path into an
> absolute filesystem path and using that one again as relative path
> inside the repository, well, that seems odd.
> 
> Help!?

got_repo_map_path() originated very early on, when 'got log' could
only run in Git repositories since Got's work tree code had not yet
been written. One of the cases it is supposed to handle is to figure
out what to do with an on-disk path passed via the command line (e.g.
after tab-completion) in a non-bare Git repository.

The file logged in your example is located in a Got work tree.
I think good a first step towards clearing up the confusion is to
avoid calling got_repo_map_path() if we have a work tree. We can
trivially derive the in-repository path from an in-work-tree path.

This limits got_repo_map_path() to cases where we operate directly
on a Git repository. Your unveil/relpath problem should disappear
and perhaps we can even simplify got_repo_map_path() afterwards.

The following patch does this for 'got log'. Other commands are also
affected and could be fixed in a similar way. The patch even fixes
a bug where 'got log' run without any arguments in a work tree with
a path prefix will only show changes within that path prefix. I think
this should only happen if the user explicitly specified a path, such
as the current directory (via "."). If no path is specified 'got log'
should always log the root path and thus all commits on the branch.
There's a test I'm tweaking which shows this behaviour change.

diff 60ca2a50c4fa6806752129de244576f0d289ffa0 /home/stsp/src/got
blob - f7f14b9cb2d386242d73d208d79ef3e9df874bab
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -3816,13 +3816,7 @@ cmd_log(int argc, char *argv[])
 		error = NULL;
 	}
 
-	if (argc == 0) {
-		path = strdup("");
-		if (path == NULL) {
-			error = got_error_from_errno("strdup");
-			goto done;
-		}
-	} else if (argc == 1) {
+	if (argc == 1) {
 		if (worktree) {
 			error = got_worktree_resolve_path(&path, worktree,
 			    argv[0]);
@@ -3835,7 +3829,7 @@ cmd_log(int argc, char *argv[])
 				goto done;
 			}
 		}
-	} else
+	} else if (argc != 0)
 		usage_log();
 
 	if (repo_path == NULL) {
@@ -3885,17 +3879,23 @@ cmd_log(int argc, char *argv[])
 	}
 
 	if (worktree) {
-		const char *prefix = got_worktree_get_path_prefix(worktree);
-		char *p;
-		if (asprintf(&p, "%s%s%s", prefix,
-		    (strcmp(prefix, "/") != 0) ? "/" : "", path) == -1) {
-			error = got_error_from_errno("asprintf");
-			goto done;
+		/*
+		 * If a path was specified on the command line it was resolved
+		 * to a path in the work tree above. Prepend the work tree's
+		 * path prefix to obtain the corresponding in-repository path.
+		 */
+		if (path) {
+			const char *prefix;
+			prefix = got_worktree_get_path_prefix(worktree);
+			if (asprintf(&in_repo_path, "%s%s%s", prefix,
+			    (path[0] != '\0') ? "/" : "", path) == -1) {
+				error = got_error_from_errno("asprintf");
+				goto done;
+			}
 		}
-		error = got_repo_map_path(&in_repo_path, repo, p, 0);
-		free(p);
 	} else
-		error = got_repo_map_path(&in_repo_path, repo, path, 1);
+		error = got_repo_map_path(&in_repo_path, repo,
+		    path ? path : "", 1);
 	if (error != NULL)
 		goto done;
 	if (in_repo_path) {
@@ -3907,9 +3907,9 @@ cmd_log(int argc, char *argv[])
 	if (error)
 		goto done;
 
-	error = print_commits(start_id, end_id, repo, path, show_changed_paths,
-	    show_patch, search_pattern, diff_context, limit, log_branches,
-	    reverse_display_order, &refs);
+	error = print_commits(start_id, end_id, repo, path ? path : "",
+	    show_changed_paths, show_patch, search_pattern, diff_context,
+	    limit, log_branches, reverse_display_order, &refs);
 done:
 	free(path);
 	free(repo_path);
blob - 74ce60ea201a8a353667c2d1ef50a71807cd2377
file + regress/cmdline/log.sh
--- regress/cmdline/log.sh
+++ regress/cmdline/log.sh
@@ -138,6 +138,7 @@ test_log_in_worktree_with_path_prefix() {
 
 	echo "modified delta" > $testroot/repo/gamma/delta
 	git_commit $testroot/repo -m "modified delta"
+	local delta_rev=`git_show_head $testroot/repo`
 
 	got checkout -p epsilon $testroot/repo $testroot/wt > /dev/null
 	ret="$?"
@@ -146,10 +147,23 @@ test_log_in_worktree_with_path_prefix() {
 		return 1
 	fi
 
+	echo "commit $delta_rev (master)" > $testroot/stdout.expected
+	echo "commit $zeta_rev" >> $testroot/stdout.expected
+	echo "commit $head_rev" >> $testroot/stdout.expected
+
+	(cd $testroot/wt && got log | grep ^commit > $testroot/stdout)
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret="$?"
+	if [ "$ret" != "0" ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
 	echo "commit $zeta_rev" > $testroot/stdout.expected
 	echo "commit $head_rev" >> $testroot/stdout.expected
 
-	for p in "" "." zeta; do
+	for p in "." zeta; do
 		(cd $testroot/wt && got log $p | \
 			grep ^commit > $testroot/stdout)
 		cmp -s $testroot/stdout.expected $testroot/stdout