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.

> > 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?
 
> (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.

>  	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?