From: Stefan Sperling <stsp@stsp.name>
Subject: Re: got patch: resolve paths from $PWD
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Fri, 22 Apr 2022 13:42:35 +0200

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

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

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?

>  		*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.