Omar Polo <op@omarpolo.com> wrote:
> Hello,
> 
> (probably last diff for the day)
> 
> The attached diff makes `got patch' continue upon encountering some
> kinds of errors; it still exits with an error, but proceed instead of
> stopping at the first issue.
> 
> Not every kind of error is recoverable thought: a malformed patch for
> instance, or others I/O errors are fatal.
> 
> The diff should be short, but there is some long changes in the regress
> suite where I'm taking advantage of this behaviour to shorten some tests
> quite a bit.

The change in test_patch_dont_apply was wrong and I missed one
got_error/got_error_path conversion in patch_file, updated diff.

sorry for the noise

-----------------------------------------------
commit 43a61642701486d74f4bb5846d07295818e1150c (perr)
from: Omar Polo <op@omarpolo.com>
date: Sun Mar 13 20:53:09 2022 UTC
 
 recover from some errors while applying a patch
 
 some kinds of errors are recoverable: like a patch that doesn't apply or
 a file with a wrong status.  This adds the needed callbacks to got_patch
 so that callers can decide what to do when error occurs; got(1) makes
 use of them to print the error message but try to continue.
 
 While here, also improve the error message in some cases and adjust
 regress to take advantage of this new behavior.
 
diff 431acc1fddc58b59c3b3093f70c08cc306c76cc5 d72bf8e20984975ec769afdb7f9b873f3efc3fea
blob - d595de198f4880c3458bb395b994e889acf68df3
blob + eb43966ea918674bfe95efe920cbff19c7b04dce
--- got/got.c
+++ got/got.c
@@ -7186,6 +7186,17 @@ patch_from_stdin(int *patchfd)
 }
 
 static const struct got_error *
+patch_failure(void *arg, const char *old, const char *new,
+    const struct got_error *error)
+{
+	int *seen_failure = arg;
+
+	*seen_failure = 1;
+	fprintf(stderr, "%s: %s\n", getprogname(), error->msg);
+	return NULL;
+}
+
+static const struct got_error *
 cmd_patch(int argc, char *argv[])
 {
 	const struct got_error *error = NULL, *close_error = NULL;
@@ -7193,7 +7204,7 @@ cmd_patch(int argc, char *argv[])
 	struct got_repository *repo = NULL;
 	char *cwd = NULL;
 	int ch, nop = 0;
-	int patchfd;
+	int patchfd, seen_failure = 0;
 
 	while ((ch = getopt(argc, argv, "n")) != -1) {
 		switch (ch) {
@@ -7248,7 +7259,10 @@ cmd_patch(int argc, char *argv[])
 #endif
 
 	error = got_patch(patchfd, worktree, repo, nop, &print_remove_status,
-	    NULL, &add_progress, NULL, check_cancelled, NULL);
+	    NULL, &add_progress, NULL, check_cancelled, NULL, patch_failure,
+	    &seen_failure);
+	if (error == NULL && seen_failure)
+		error = got_error(GOT_ERR_PATCH_FAILED);
 
 done:
 	if (repo) {
blob - 11708ccf458de5506eccdad8dafcaaed715432bb
blob + a8f2e50c6d6f078e00120784797ee9b2ce8d232e
--- include/got_error.h
+++ include/got_error.h
@@ -166,6 +166,7 @@
 #define GOT_ERR_PATCH_TRUNCATED	148
 #define GOT_ERR_PATCH_DONT_APPLY 149
 #define GOT_ERR_NO_PATCH	150
+#define GOT_ERR_PATCH_FAILED	151
 
 static const struct got_error {
 	int code;
@@ -346,6 +347,7 @@ static const struct got_error {
 	{ GOT_ERR_PATCH_TRUNCATED, "patch truncated" },
 	{ GOT_ERR_PATCH_DONT_APPLY, "patch doesn't apply" },
 	{ GOT_ERR_NO_PATCH, "no patch found" },
+	{ GOT_ERR_PATCH_FAILED, "failed to apply the patch" },
 };
 
 /*
blob - 5f28ffc619f1b4e32ba37ff8e5361c636009ba74
blob + 55b26574341f02b1cdd42f56c94f7412417fc195
--- include/got_patch.h
+++ include/got_patch.h
@@ -15,6 +15,14 @@
  */
 
 /*
+ * Callback invoked upon encountering any recoverable error.  If it
+ * returns NULL then the error is discarded and got_patch continues,
+ * otherwise it terminates returning the same error.
+ */
+typedef const struct got_error *(*got_patch_error_cb)(void *,
+    const char *, const char *, const struct got_error *);
+
+/*
  * Apply the (already opened) patch to the repository and register the
  * status of the added and removed files.
  *
@@ -23,4 +31,4 @@
 const struct got_error *
 got_patch(int, struct got_worktree *, struct got_repository *, int,
     got_worktree_delete_cb, void *, got_worktree_checkout_cb, void *,
-    got_cancel_cb, void *);
+    got_cancel_cb, void *, got_patch_error_cb, void *);
blob - 6e5ec0f2143d39ff9eeca89060522c5155706a77
blob + 8865f5cfc7ab538b19d0b1a448ff0d297c085aeb
--- lib/patch.c
+++ lib/patch.c
@@ -247,7 +247,7 @@ done:
  * If pos is -1, copy until EOF.
  */
 static const struct got_error *
-copy(FILE *tmp, FILE *orig, off_t copypos, off_t pos)
+copy(const char *path, FILE *tmp, FILE *orig, off_t copypos, off_t pos)
 {
 	char buf[BUFSIZ];
 	size_t len, r, w;
@@ -269,14 +269,15 @@ copy(FILE *tmp, FILE *orig, off_t copypos, off_t pos)
 		if (r != len && feof(orig)) {
 			if (pos == -1)
 				return NULL;
-			return got_error(GOT_ERR_PATCH_DONT_APPLY);
+			return got_error_path(path, GOT_ERR_PATCH_DONT_APPLY);
 		}
 	}
 	return NULL;
 }
 
 static const struct got_error *
-locate_hunk(FILE *orig, struct got_patch_hunk *h, off_t *pos, long *lineno)
+locate_hunk(const char *path, FILE *orig, struct got_patch_hunk *h,
+    off_t *pos, long *lineno)
 {
 	const struct got_error *err = NULL;
 	char *line = NULL;
@@ -292,7 +293,8 @@ locate_hunk(FILE *orig, struct got_patch_hunk *h, off_
 			if (ferror(orig))
 				err = got_error_from_errno("getline");
 			else if (match == -1)
-				err = got_error(GOT_ERR_PATCH_DONT_APPLY);
+				err = got_error_path(path,
+				    GOT_ERR_PATCH_DONT_APPLY);
 			break;
 		}
 		(*lineno)++;
@@ -325,7 +327,7 @@ locate_hunk(FILE *orig, struct got_patch_hunk *h, off_
 }
 
 static const struct got_error *
-test_hunk(FILE *orig, struct got_patch_hunk *h)
+test_hunk(const char *path, FILE *orig, struct got_patch_hunk *h)
 {
 	const struct got_error *err = NULL;
 	char *line = NULL;
@@ -343,12 +345,13 @@ test_hunk(FILE *orig, struct got_patch_hunk *h)
 				if (ferror(orig))
 					err = got_error_from_errno("getline");
 				else
-					err = got_error(
+					err = got_error_path(path,
 					    GOT_ERR_PATCH_DONT_APPLY);
 				goto done;
 			}
 			if (strcmp(h->lines[i]+1, line)) {
-				err = got_error(GOT_ERR_PATCH_DONT_APPLY);
+				err = got_error_path(path,
+				    GOT_ERR_PATCH_DONT_APPLY);
 				goto done;
 			}
 			break;
@@ -420,16 +423,16 @@ patch_file(struct got_patch *p, const char *path, FILE
 			break;
 
 	tryagain:
-		err = locate_hunk(orig, h, &pos, &lineno);
+		err = locate_hunk(path, orig, h, &pos, &lineno);
 		if (err != NULL)
 			goto done;
 		if (!p->nop)
-			err = copy(tmp, orig, copypos, pos);
+			err = copy(path, tmp, orig, copypos, pos);
 		if (err != NULL)
 			goto done;
 		copypos = pos;
 
-		err = test_hunk(orig, h);
+		err = test_hunk(path, orig, h);
 		if (err != NULL && err->code == GOT_ERR_PATCH_DONT_APPLY) {
 			/*
 			 * try to apply the hunk again starting the search
@@ -441,7 +444,11 @@ patch_file(struct got_patch *p, const char *path, FILE
 			}
 			linelen = getline(&line, &linesize, orig);
 			if (linelen == -1) {
-				err = got_error_from_errno("getline");
+				if (ferror(orig))
+					err = got_error_from_errno("getline");
+				else
+					err = got_error_path(path,
+					    GOT_ERR_PATCH_DONT_APPLY);
 				goto done;
 			}
 			lineno++;
@@ -462,16 +469,15 @@ patch_file(struct got_patch *p, const char *path, FILE
 		}
 	}
 
-	
 	if (p->new == NULL) {
 		struct stat sb;
 
 		if (fstat(fileno(orig), &sb) == -1)
 			err = got_error_from_errno("fstat");
 		else if (sb.st_size != copypos)
-			err = got_error(GOT_ERR_PATCH_DONT_APPLY);
+			err = got_error_path(path, GOT_ERR_PATCH_DONT_APPLY);
 	} else if (!p->nop && !feof(orig))
-		err = copy(tmp, orig, copypos, -1);
+		err = copy(path, tmp, orig, copypos, -1);
 
 done:
 	if (orig != NULL)
@@ -656,7 +662,7 @@ const struct got_error *
 got_patch(int fd, struct got_worktree *worktree, struct got_repository *repo,
     int nop, got_worktree_delete_cb delete_cb, void *delete_arg,
     got_worktree_checkout_cb add_cb, void *add_arg, got_cancel_cb cancel_cb,
-    void *cancel_arg)
+    void *cancel_arg, got_patch_error_cb err_cb, void *err_arg)
 {
 	const struct got_error *err = NULL;
 	struct imsgbuf *ibuf;
@@ -707,6 +713,14 @@ got_patch(int fd, struct got_worktree *worktree, struc
 		p.nop = nop;
 		err = apply_patch(worktree, repo, &p, delete_cb, delete_arg,
 		    add_cb, add_arg, cancel_cb, cancel_arg);
+		if (err != NULL && err_cb != NULL) {
+			/* recoverable errors */
+			if (err->code == GOT_ERR_PATCH_DONT_APPLY ||
+			    err->code == GOT_ERR_FILE_STATUS ||
+			    err->code == GOT_ERR_BAD_PATH ||
+			    (err->code == GOT_ERR_ERRNO && errno == ENOENT))
+				err = err_cb(err_arg, p.old, p.new, err);
+		}
 		patch_free(&p);
 		if (err)
 			break;
blob - 41f290c529e09c20b73b0a6752c3351664a9559d
blob + 4c06605ecc02bbde00f6b28a98d469a0ee30c677
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -409,47 +409,6 @@ test_patch_dont_apply() {
 @@ -1 +1,2 @@
 +hatsuseno
  alpha something
-EOF
-
-	echo -n > $testroot/stdout.expected
-	echo "got: patch doesn't apply" > $testroot/stderr.expected
-
-	(cd $testroot/wt && got patch patch) \
-		 > $testroot/stdout \
-		2> $testroot/stderr
-	ret=$?
-	if [ $ret -eq 0 ]; then # should fail
-		test_done $testroot 1
-		return 1
-	fi
-
-	cmp -s $testroot/stdout.expected $testroot/stdout
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		diff -u $testroot/stdout.expected $testroot/stdout
-		test_done $testroot $ret
-		return 1
-	fi
-
-	cmp -s $testroot/stderr.expected $testroot/stderr
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		diff -u $testroot/stderr.expected $testroot/stderr
-		test_done $testroot $ret
-		return 1
-	fi
-
-	# try to delete a file with a patch that doesn't match
-	jot 100 > $testroot/wt/numbers
-	(cd $testroot/wt && got add numbers && got commit -m 'add numbers') \
-		>/dev/null
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		test_done $testroot $ret
-		return 1
-	fi
-
-	cat <<EOF > $testroot/wt/patch
 --- numbers
 +++ /dev/null
 @@ -1,9 +0,0 @@
@@ -464,14 +423,39 @@ EOF
 -9
 EOF
 
-	(cd $testroot/wt && got patch patch) > /dev/null 2> $testroot/stderr
+	jot 100 > $testroot/wt/numbers
+	(cd $testroot/wt && got add numbers && got commit -m 'add numbers') \
+		>/dev/null
 	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	echo -n > $testroot/stdout.expected
+	cat <<EOF > $testroot/stderr.expected
+got: alpha: patch doesn't apply
+got: numbers: patch doesn't apply
+got: failed to apply the patch
+EOF
+
+	(cd $testroot/wt && got patch patch) \
+		 > $testroot/stdout \
+		2> $testroot/stderr
+	ret=$?
 	if [ $ret -eq 0 ]; then # should fail
 		test_done $testroot 1
 		return 1
 	fi
 
-	echo "got: patch doesn't apply" > $testroot/stderr.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done $testroot $ret
+		return 1
+	fi
+
 	cmp -s $testroot/stderr.expected $testroot/stderr
 	ret=$?
 	if [ $ret -ne 0 ]; then
@@ -779,138 +763,63 @@ test_patch_illegal_status() {
 		return 1
 	fi
 
-	# edit an non-existent and unknown file
+	# edit an non-existent file, an unversioned file, an
+	# obstructed one and delete a non-existent one.
 	cat <<EOF > $testroot/wt/patch
+--- alpha
++++ alpha
+@@ -1 +1,2 @@
+ alpha
++was edited
+--- /dev/null
++++ beta
+@@ -0,0 +1 @@
++beta
 --- iota
 +++ iota
 @@ -1 +1 @@
-- iota
-+ IOTA
+-iota
++IOTA
+--- kappa
++++ kappa
+@@ -1 +1 @@
+-kappa
++kappa
+--- lambda
++++ /dev/null
+@@ -1 +0,0 @@
+-lambda
 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 kappa > $testroot/wt/kappa
 
-	echo "got: iota: No such file or directory" \
-		> $testroot/stderr.expected
-	cmp -s $testroot/stderr.expected $testroot/stderr
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		diff -u $testroot/stderr.expected $testroot/stderr
-		test_done $testroot $ret
-		return 1
-	fi
-
-	# create iota and re-try
-	echo iota > $testroot/wt/iota
-
-	(cd $testroot/wt && got patch patch) > /dev/null \
-		2> $testroot/stderr
-	ret=$?
-	if [ $ret -eq 0 ]; then
-		echo "patched an unknown file" >&2
-		test_done $testroot $ret
-		return 1
-	fi
-
-	echo "got: iota: file has unexpected status" \
-		> $testroot/stderr.expected
-	cmp -s $testroot/stderr.expected $testroot/stderr
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		diff -u $testroot/stderr.expected $testroot/stderr
-		test_done $testroot $ret
-		return 1
-	fi
-	
-	rm $testroot/wt/iota
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		test_done $testroot $ret
-		return 1
-	fi
-
-	# edit obstructed file
+	# obstruct alpha
 	rm $testroot/wt/alpha
 	mkdir $testroot/wt/alpha
-	cat <<EOF > $testroot/wt/patch
---- alpha
-+++ alpha
-@@ -1 +1,2 @@
- alpha
-+was edited
-EOF
 
 	(cd $testroot/wt && got patch patch) > /dev/null \
 		2> $testroot/stderr
 	ret=$?
 	if [ $ret -eq 0 ]; then
-		echo "edited a missing file" >&2
+		echo "added/removed/edited files with wrong status" >&2
 		test_done $testroot $ret
 		return 1
 	fi
 
-	echo "got: alpha: file has unexpected status" \
-		> $testroot/stderr.expected
+	cat <<EOF > $testroot/stderr.expected
+got: alpha: file has unexpected status
+got: beta: file has unexpected status
+got: iota: No such file or directory
+got: kappa: file has unexpected status
+got: lambda: No such file or directory
+got: failed to apply the patch
+EOF
+
 	cmp -s $testroot/stderr.expected $testroot/stderr
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		diff -u $testroot/stderr.expected $testroot/stderr
-		test_done $testroot $ret
-		return 1
 	fi
-
-	# delete an unknown file
-	cat <<EOF > $testroot/wt/patch
---- iota
-+++ /dev/null
-@@ -1 +0,0 @@
--iota
-EOF
-
-	(cd $testroot/wt && got patch patch) > /dev/null \
-		2> $testroot/stderr
-	ret=$?
-	if [ $ret -eq 0 ]; then
-		echo "deleted a missing file?" >&2
-		test_done $testroot $ret
-		return 1
-	fi
-
-	echo "got: iota: No such file or directory" \
-		> $testroot/stderr.expected
-	cmp -s $testroot/stderr.expected $testroot/stderr
-	ret=$?
-	if [ $ret -eq 0 ]; then
-		diff -u $testroot/stderr.expected $testroot/stderr
-		test_done $testroot $ret
-		return 1
-	fi
-
-	# try again with iota in place but still not registered
-	echo iota > $testroot/wt/iota
-	(cd $testroot/wt && got patch patch) > /dev/null \
-		2> $testroot/stderr
-	ret=$?
-	if [ $ret -eq 0 ]; then
-		echo "deleted an unversioned file?" >&2
-		test_done $testroot $ret
-		return 1
-	fi
-
-	echo "got: iota: file has unexpected status" \
-		> $testroot/stderr.expected
-	cmp -s $testroot/stderr.expected $testroot/stderr
-	ret=$?
-	if [ $ret -eq 0 ]; then
-		diff -u $testroot/stderr.expected $testroot/stderr
-	fi
 	test_done $testroot $ret
 }