Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Apr 22, 2022 at 04:47:59PM +0200, Omar Polo wrote:
> > 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.
> 
> Oh, indeed! That is probably a mistake. I don't see a reason why paths
> returned from got_worktree_resolve_path() must be relative. I can look
> at fixing this. No reason for you to walk futher into that rabbit hole :)
> 
> > 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.
> 
> This kind of easy confusion is why standardizing on absolute paths
> for internal use is a good idea. Subversion made a switch from mixed
> relative/absolute to absolute-only in the 1.6 -> 1.7 timeframe, and
> this caught a lot of issues.

For what it's worth, I'm all in for using only absolute paths.  Maybe
also using only paths relative to the worktree root and resolving them
from a file descriptor associated with the worktree root is an option?
I'm missing the bigger picture here thought.

> > > 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.
> 
> Yes, this seems nicer indeed.
> The work of figuring out which path the patch wants to add, delete,
> or modify is now done by got_worktree_resolve_path(), correct?

just as before, yes.  (I need to simplify a bit the call chain...)

The paths are computed in worktree.c:got_worktree_patch_check_path via
patch_check_path, and they're build off got_worktree_resolve_path.

I'm still keeping the paths relative to the worktree root in the
pathlists, otherwise I'd broke everything.  I'm then building (again)
the absolute paths in apply_patch before running the patch machinery.

I'm only feeding absolute paths to lstat/fopen/rename & co.

> > (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.
> 
> > 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;;
> 
> What is the reason for switching these variable names to 'file'?
> I like having "path" as a part of names for variables which contain
> paths, and "file" as a part of names for variables which represent
> open files. This is just a matter of taste, of course.

I like *path more than *file too, the rename was to have another name
(old/newpath are already the parameters.)

In the updated diff I'm calling the argument of apply_patch "old" and
"new", and the absolute paths "oldpath" and "newpath".  less churn too.

> >  	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;
> > +	}
> 
> This looks like a step which would become unnecessary later, if work tree
> paths returned from lib/worktree.c were already absolute. Right?

Yes.  To be fair it could be somehow avoided right now, I just need to
propagate the path I build in worktree.c:patch_check_path up until
apply_patch.  (it's the "ondisk_path" from patch_check_path).  but that
can be done in a follow-up commit, diff belows already has three
different fixes, it's probably enough ;)

P.S.: sorry for the previous non-applying diff, i left an extra
character by mistake while reading the diff in my editor.

diff refs/heads/main refs/heads/ppath2
blob - 9b49b4b1349c2d2994dfa6e667c9265975aea642
blob + adf9459c88ef262d9070733fe62620364926f5db
--- lib/patch.c
+++ lib/patch.c
@@ -565,13 +565,14 @@ patch_add(void *arg, unsigned char status, const char 
 
 static const struct got_error *
 apply_patch(struct got_worktree *worktree, struct got_repository *repo,
-    const char *oldpath, const char *newpath, struct got_patch *p,
-    int nop, struct patch_args *pa, got_cancel_cb cancel_cb, void *cancel_arg)
+    const char *old, const char *new, struct got_patch *p, int nop,
+    struct patch_args *pa, got_cancel_cb cancel_cb, void *cancel_arg)
 {
 	const struct got_error *err = NULL;
 	struct got_pathlist_head oldpaths, newpaths;
 	struct got_pathlist_entry *pe;
 	int file_renamed = 0;
+	char *oldpath = NULL, *newpath = NULL;
 	char *tmppath = NULL, *template = NULL, *parent = NULL;;
 	FILE *tmp = NULL;
 	mode_t mode = GOT_DEFAULT_FILE_MODE;
@@ -579,13 +580,25 @@ apply_patch(struct got_worktree *worktree, struct got_
 	TAILQ_INIT(&oldpaths);
 	TAILQ_INIT(&newpaths);
 
-	err = got_pathlist_insert(&pe, &oldpaths, oldpath, NULL);
+	err = got_pathlist_insert(&pe, &oldpaths, old, NULL);
 	if (err)
 		goto done;
-	err = got_pathlist_insert(&pe, &newpaths, newpath, NULL);
+	err = got_pathlist_insert(&pe, &newpaths, new, NULL);
 	if (err)
 		goto done;
 
+	if (asprintf(&oldpath, "%s/%s", got_worktree_get_root_path(worktree),
+	    old) == -1) {
+		err = got_error_from_errno("asprintf");
+		goto done;
+	}
+
+	if (asprintf(&newpath, "%s/%s", got_worktree_get_root_path(worktree),
+	    new) == -1) {
+		err = got_error_from_errno("asprintf");
+		goto done;
+	}
+
 	file_renamed = strcmp(oldpath, newpath);
 
 	if (asprintf(&template, "%s/got-patch",
@@ -612,7 +625,7 @@ 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", tmppath);
 		goto done;
 	}
 
@@ -650,8 +663,7 @@ apply_patch(struct got_worktree *worktree, struct got_
 		if (err)
 			unlink(newpath);
 	} else
-		err = report_progress(pa, oldpath, newpath, GOT_STATUS_MODIFY,
-		    NULL);
+		err = report_progress(pa, old, new, GOT_STATUS_MODIFY, NULL);
 
 done:
 	got_pathlist_free(&oldpaths);
@@ -661,6 +673,8 @@ done:
 	if (tmppath != NULL)
 		unlink(tmppath);
 	free(tmppath);
+	free(oldpath);
+	free(newpath);
 	return err;
 }
 
blob - bfebb46526cc0f3f0deda90d0868edad7c6946c9
blob + bbe95ae655a519c5a9d4dc6eac25f7ace38c7452
--- 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
blob + af1fd72e33001d46b8f822e0b0a7f00a5b1ad7b7
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -1273,6 +1273,46 @@ EOF
 	test_done $testroot 0
 }
 
+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