On Sat, Mar 12, 2022 at 11:22:37PM +0100, Omar Polo wrote:
> Hello,
> 
> currently 'got patch' is happy to do all sorts of things to all sorts of
> files.  Well, it's not really ideal I think.  The following patch adds
> some checks to the targeted files before patching 'em.
> 
> I think it's fine to allow patching modified files, but 'got patch'
> should disallow patching non-existant or unknown files. Or add files
> that are already there, or delete files there are not known.
> 
> There's a bit of churn because I just had to re-introduce in apply_patch
> the pathlists i segregated to schedule_* in previous commit.  Also, at
> this points those two helpers are useless.
> 
> This actually made the rename test to fail because in the last part
> `eta' (the target of the rename) exists (untracked) and so it won't
> proceed with the rename.
> 
> note that it needs the latest commit by stsp "fix 'got status' with an
> obstructed file".
> 
> thoughts?

This is a great start, yes.

Some comments below.

> +can_rm(void *arg, unsigned char status, unsigned char staged_status,

> +can_add(void *arg, unsigned char status, unsigned char staged_status,

> +can_edit(void *arg, unsigned char status, unsigned char staged_status,

Please document the status checks done by these functions in
the man page. These are essentially pre-conditions which are
specific to 'got patch' and should be documented.
Otherwise errors raised here will be difficult to interpret.

If a patch file contains a series of patches which will be applied
in sequence, then patches which pass pre-conditions will affect
the work tree, and any later patch which does not pass pre-conditions
will make 'got patch' error out, leaving any modifications made by
previous patches behind.
I think this behaviour is correct. The application of patch N to
the work tree can affect pre-condition checks for patch N+1, so
there is no way to check pre-conditions for patch N+1 before
applying patch N to the work tree.
Still, this might be worth documenting because users might otherwise
expect that 'got patch' will not modify the work tree in any way
unless *all* patches in the input pass pre-condition checks.

> +static const struct got_error *
>  apply_patch(struct got_worktree *worktree, struct got_repository *repo,
>      struct got_patch *p, got_worktree_delete_cb delete_cb, void *delete_arg,
> -    got_worktree_checkout_cb add_cb, void *add_arg)
> +    got_worktree_checkout_cb add_cb, void *add_arg, got_cancel_cb cancel_cb,
> +    void *cancel_arg)
>  {
>  	const struct got_error *err = NULL;
> +	struct got_pathlist_head oldpaths, newpaths;
>  	int file_renamed = 0;
>  	char *oldpath = NULL, *newpath = NULL;
>  	char *tmppath = NULL, *template = NULL;
>  	FILE *tmp = NULL;
>  
> -	err = got_worktree_resolve_path(&oldpath, worktree,
> -	    p->old != NULL ? p->old : p->new);
> +	TAILQ_INIT(&oldpaths);
> +	TAILQ_INIT(&newpaths);
> +
> +	err = build_pathlist(p->old != NULL ? p->old : p->new, &oldpath,
> +	    &oldpaths, worktree);
>  	if (err)
>  		goto done;

Maybe worktree.c should provide a build_pathlist style funcion?
This function returns a pathlist that corresponds to a single path
in the work tree, and could be useful elsewhere, e.g. in got.c?
In any case, we can refactor this later.

>  	if (p->old != NULL && p->new == NULL) {
>  		/*
>  		 * special case: delete a file.  don't try to match
>  		 * the lines but just schedule the removal.
>  		 */

In the long term, this code should try to match the lines.
If lines don't match then we are deleting "with fuzz", which
might not be what the user wants. It should at least result in a
warning or fuzz factor or similar appearing in progress output.

> blob - 5916921f260f5a0eaefdbf6036b813fbf3570dd5
> blob + 8fc24e71d8c31a7d54af30d9cfce4f5c598f3299
> --- regress/cmdline/patch.sh
> +++ regress/cmdline/patch.sh
> @@ -730,6 +731,99 @@ EOF
>  	test_done $testroot $ret
>  }
>  
> +test_patch_illegal_status() {
> +	local testroot=`test_init patch_illegal_status`
> +
> +	got checkout $testroot/repo $testroot/wt > /dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done $testroot $ret
> +		return 1
> +	fi
> +
> +	# edit an non-existant and unknown file

typo: should say "non-existent"

> +	cat <<EOF > $testroot/wt/patch
> +--- iota
> ++++ iota
> +@@ -1 +1 @@
> +- iota
> ++ IOTA
> +EOF
> +
> +	(cd $testroot/wt && got patch patch) > /dev/null \
> +		2> $testroot/stderr
> +	ret=$?
> +	if [ $ret -eq 0 ]; then
> +		echo "edited a missing file" >&2
> +		test_done $testroot $ret
> +		return 1
> +	fi
> +
> +	echo "got: iota: file has unexpected status" \
> +		> $testroot/stderr.expected

Maybe refine this error to say "iota: No such file or directory"?
You could use got_error_set_errno(ENOENT, path) somewhere in this
specific case.