From: Omar Polo <op@omarpolo.com>
Subject: Re: got-build-regress.sh regress failure
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Tue, 08 Mar 2022 10:33:21 +0100

Stefan Sperling <stsp@stsp.name> wrote:
> On Tue, Mar 08, 2022 at 09:18:27AM +0100, Omar Polo wrote:
> > woops, i've been a bit sloppy in the way I'm generating the temporary
> > file while applying the patch.  (but i couldn't replicate locally)
> > 
> > I need to open a new temporary file during the patch operation to
> > accumulate the changes/additions to the target file before rename(2) it
> > to the destination file.
> > 
> > It's fine to just use "./"?  it's the only instance in the codebase, but
> > I can't use /tmp because I need both files (temp and destination) to be
> > on the same filesystem for rename(2) to work.  got_opentemp guarantees
> > us that it's a new file, so no chance to corrupt existing files in the
> > worktree.
> 
> Using relative paths internally is a receipe for bugs.
> We should always use absolute paths internally, and convert to relative
> paths only at the presentation layer where the user gets to see them.

agreed.

> > I could have used GOT_WORKTREE_GOT_DIR (concatenated to the worktree
> > root path) but that would require to build a string and "./" saves us an
> > allocation :)
> 
> Using absolute paths you will have to build a path in a new buffer.
> 
> The tests are failing because the generated temp path is outside the
> unveiled path space. That is why open reports "no such file or directory".

right.  we unveil /tmp and the regress suite uses that for the tests,
that's why I haven't noticed.  thanks for the explanation.

this is better?

diff 63868eefd7fce8dc07000a925bb6405100a596bb f69a1b4827a2ffcff36a4f54310ea5bd714951fb
blob - 84226a57dca7da7a69187a76d804e8ceda7558ba
blob + 672f8dac069d4fdcde1088037bbf462287cd0264
--- lib/patch.c
+++ lib/patch.c
@@ -383,7 +383,7 @@ apply_patch(struct got_worktree *worktree, struct got_
 	const struct got_error *err = NULL;
 	struct got_pathlist_head paths;
 	struct got_pathlist_entry *pe;
-	char *path = NULL, *tmppath = NULL;
+	char *path = NULL, *tmppath = NULL, *template = NULL;
 	FILE *orig = NULL, *tmp = NULL;
 	struct got_patch_hunk *h;
 	size_t i;
@@ -419,8 +419,13 @@ apply_patch(struct got_worktree *worktree, struct got_
 		goto done;
 	}
 
-	err = got_opentemp_named(&tmppath, &tmp,
-	    got_worktree_get_root_path(worktree));
+	if (asprintf(&template, "%s/got-patch",
+	    got_worktree_get_root_path(worktree)) == -1) {
+		err = got_error_from_errno(template);
+		goto done;
+	}
+
+	err = got_opentemp_named(&tmppath, &tmp, template);
 	if (err)
 		goto done;
 
@@ -509,6 +514,7 @@ rename:
 	else
 		printf("M  %s\n", path); /* XXX */
 done:
+	free(template);
 	if (err != NULL && p->old == NULL && path != NULL)
 		unlink(path);
 	if (tmp != NULL)