Stefan Sperling <stsp@stsp.name> wrote:
> On Tue, Apr 19, 2022 at 10:40:11PM +0200, Omar Polo wrote:
> > hello,
> > 
> > as reminded by naddy@, 'got patch' resolve paths from the worktree root
> > and not from the current working directory.
> > 
> > let's fix it.
> > 
> > The attached diff changes `patch_check_path' to build the paths from the
> > current directory.  I'm also moving the pathlist handling from got_patch
> > to patch_check_path too, it makes more sense to fix the pathlist where
> > we're also doing the other path operations.
> > 
> > The only bit I don't really like is the changes at the end of
> > apply_patch.  I want to call report_progress with paths that are
> > absolute from the worktree root, and that's what happens when
> 
> You mean "relative to the work tree root"?

yep, exactly.

> > report_progress is called via patch_add/delete (the callbacks for
> > got_worktree_schedule_add/delete.)  But for the "edited file" case I
> > have to peek into the tailq.  The `newpath' and `oldpath' paths are
> > relative to $PWD, so I can't use them.  It's not incredibly bad, I know
> > for sure that the pathlists have one and exactly one element in them,
> > but it doesn't read too well IMHO.  (but neither too bad maybe?)
> 
> Would it help to convert paths to relative form only just before passing
> them on for display purposes? You can always obtain the worktree's root
> path if you can get at a pointer to your struct got_worktree somehow,
> to make any absolute path relative the work tree root.
> 
> I don't understand why you would need to store relative paths on a queue.
> In general, any paths which get passed around should be absolute.
> Someone might adjust this code later, not realizing which paths are
> relative and which are absolute, and easily create a bug.

i agree with using absolute paths as much as possible, but we're already
storing "relative" paths in pathlists.  the current code use
got_worktree_resolve_path to resolve a path from the patch which returns
a path that's relative to the work tree root.

I was confused by which functions wants an absolute path and which ones
are fine with relative ones.  For example (fixed in the diff below)
got_fileindex_entry_get wants (for what i can see) a path relative to
the work tree root, but get_file_status wants an absolute path (even if
it doesn't enforce it for the case dir=-1) while I was passing to it a
relative (to the worktree) path.

> You are already converting relative paths found in the patch file to
> absolute ones based on $PWD and the work tree root. That code looks
> correct to me. Why not keep using this absolute path as much as possible?

I've come up with a different approach, that's probably simpler.  The
issue with 'got patch' and $PWD arise from the fact that I'm using paths
relative to the work tree as they were relative to $PWD: this can work
only when $PWD is the root of a worktree.  In patch below I'm keeping
the paths relative to the work tree root as they're now, and computing
absolute paths from those when needed.

(i'm also sneakingly fixing a typo in an error reporting: if fchmod
fails it failed on tmppath -- now tmpfile -- not on 'newfile' which has
yet to be created)

If you're happy with the direction I can then avoid computing the
"ondisk_path" twice (once in patch_check_path and then again in
apply_patch) and just propagate it from patch_check_path.

> >  		*staged_status = GOT_STATUS_NO_CHANGE;
> >  		*status = GOT_STATUS_UNVERSIONED;
> > -		if (lstat(ondisk_path, &sb) == -1) {
> > +		if (lstat(*resolved_path, &sb) == -1) {
> 
> At this point *resolved_path is a relative path, correct?
> I would refrain from passing a relative path to a function that will
> use it for anything other than displaying the path to the user.

diff 3313bcd840cd85a1b349abf664992608a42bbbde /home/op/w/got
blob - 9b49b4b1349c2d2994dfa6e667c9265975aea642
file + lib/patch.c
--- lib/patch.c
+++ lib/patch.c
@@ -572,7 +572,8 @@ apply_patch(struct got_worktree *worktree, struct got_
 	struct got_pathlist_head oldpaths, newpaths;
 	struct got_pathlist_entry *pe;
 	int file_renamed = 0;
-	char *tmppath = NULL, *template = NULL, *parent = NULL;;
+	char *oldfile = NULL, *newfile = NULL;
+	char *tmpfile = NULL, *template = NULL, *parent = NULL;;
 	FILE *tmp = NULL;
 	mode_t mode = GOT_DEFAULT_FILE_MODE;
 
@@ -588,6 +589,18 @@ apply_patch(struct got_worktree *worktree, struct got_
 
 	file_renamed = strcmp(oldpath, newpath);
 
+	if (asprintf(&oldfile, "%s/%s", got_worktree_get_root_path(worktree),
+	    oldpath) == -1) {
+		err = got_error_from_errno("asprintf");
+		goto done;
+	}
+
+	if (asprintf(&newfile, "%s/%s", got_worktree_get_root_path(worktree),
+	    newpath) == -1) {
+		err = got_error_from_errno("asprintf");
+		goto done;
+	}
+
 	if (asprintf(&template, "%s/got-patch",
 	    got_worktree_get_root_path(worktree)) == -1) {
 		err = got_error_from_errno(template);
@@ -595,10 +608,10 @@ apply_patch(struct got_worktree *worktree, struct got_
 	}
 
 	if (!nop)
-		err = got_opentemp_named(&tmppath, &tmp, template);
+		err = got_opentemp_named(&tmpfile, &tmp, template);
 	if (err)
 		goto done;
-	err = patch_file(p, oldpath, tmp, nop, &mode);
+	err = patch_file(p, oldfile, tmp, nop, &mode);
 	if (err)
 		goto done;
 
@@ -612,26 +625,26 @@ apply_patch(struct got_worktree *worktree, struct got_
 	}
 
 	if (fchmod(fileno(tmp), mode) == -1) {
-		err = got_error_from_errno2("chmod", newpath);
+		err = got_error_from_errno2("chmod", tmpfile);
 		goto done;
 	}
 
-	if (rename(tmppath, newpath) == -1) {
+	if (rename(tmpfile, newfile) == -1) {
 		if (errno != ENOENT) {
-			err = got_error_from_errno3("rename", tmppath,
-			    newpath);
+			err = got_error_from_errno3("rename", tmpfile,
+			    newfile);
 			goto done;
 		}
 
-		err = got_path_dirname(&parent, newpath);
+		err = got_path_dirname(&parent, newfile);
 		if (err != NULL)
 			goto done;
 		err = got_path_mkdir(parent);
 		if (err != NULL)
 			goto done;
-		if (rename(tmppath, newpath) == -1) {
-			err = got_error_from_errno3("rename", tmppath,
-			    newpath);
+		if (rename(tmpfile, newfile) == -1) {
+			err = got_error_from_errno3("rename", tmpfile,
+			    newfile);
 			goto done;
 		}
 	}
@@ -643,12 +656,12 @@ apply_patch(struct got_worktree *worktree, struct got_
 			err = got_worktree_schedule_add(worktree, &newpaths,
 			    patch_add, pa, repo, 1);
 		if (err)
-			unlink(newpath);
+			unlink(newfile);
 	} else if (p->old == NULL) {
 		err = got_worktree_schedule_add(worktree, &newpaths,
 		    patch_add, pa, repo, 1);
 		if (err)
-			unlink(newpath);
+			unlink(newfile);
 	} else
 		err = report_progress(pa, oldpath, newpath, GOT_STATUS_MODIFY,
 		    NULL);
@@ -658,9 +671,11 @@ done:
 	got_pathlist_free(&newpaths);
 	free(parent);
 	free(template);
-	if (tmppath != NULL)
-		unlink(tmppath);
-	free(tmppath);
+	if (tmpfile != NULL)
+		unlink(tmpfile);
+	free(tmpfile);
+	free(oldfile);
+	free(newfile);
 	return err;
 }
 
blob - bfebb46526cc0f3f0deda90d0868edad7c6946c9
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -8795,7 +8795,8 @@ patch_check_path(const char *p, char **path, unsigned 
 	ie = got_fileindex_entry_get(fileindex, *path, strlen(*path));
 	if (ie) {
 		*staged_status = get_staged_status(ie);
-		err = get_file_status(status, &sb, ie, *path, -1, NULL, repo);
+		err = get_file_status(status, &sb, ie, ondisk_path, -1, NULL,
+		    repo);
 		if (err)
 			goto done;
 	} else {
blob - b3f10d2dc6e282f6548c695befab125a7f8f680e
file + regress/cmdline/patch.sh
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -1273,6 +1273,46 @@ EOF
 	test_done $testroot 0
 }
 
v+test_patch_relative_paths() {
+	local testroot=`test_init patch_orig`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	cat <<EOF > $testroot/wt/gamma/patch
+--- delta
++++ delta
+@@ -1 +1 @@
+-delta
++DELTA
+--- /dev/null
++++ eta
+@@ -0,0 +1 @@
++eta
+EOF
+
+	(cd $testroot/wt/gamma && got patch patch) > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	echo 'M  gamma/delta' > $testroot/stdout.expected
+	echo 'A  gamma/eta' >> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
+	test_done $testroot $ret
+}
+
 test_parseargs "$@"
 run_test test_patch_simple_add_file
 run_test test_patch_simple_rm_file
@@ -1294,3 +1334,4 @@ run_test test_patch_with_offset
 run_test test_patch_prefer_new_path
 run_test test_patch_no_newline
 run_test test_patch_strip
+run_test test_patch_relative_paths