From: Omar Polo <op@omarpolo.com>
Subject: got patch: recover from some kinds of errors
To: gameoftrees@openbsd.org
Date: Sun, 13 Mar 2022 21:42:53 +0100

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.

-----------------------------------------------
commit f426e7c05f171f70f7d3e198e6ac6ff312046f9b (perr)
from: Omar Polo <op@omarpolo.com>
date: Sun Mar 13 20:33:22 2022 UTC
 
 recover from some errors while applying a patch
 
 some kinds of errors are recoverable: 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 13cf866fe9fa67f92183c0e9a20c3cfdf33e0fcc
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 + 831b50adb48b7c5c6cd38212c3ede73d72ea649d
--- 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,7 +469,6 @@ patch_file(struct got_patch *p, const char *path, FILE
 		}
 	}
 
-	
 	if (p->new == NULL) {
 		struct stat sb;
 
@@ -471,7 +477,7 @@ patch_file(struct got_patch *p, const char *path, FILE
 		else if (sb.st_size != copypos)
 			err = got_error(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 + 83aa873d19cb35fdc53eaf24877efbc73c40d6ba
--- 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,32 @@ EOF
 -9
 EOF
 
-	(cd $testroot/wt && got patch patch) > /dev/null 2> $testroot/stderr
+	jot 100 > $testroot/wt/numbers
+
+	echo -n > $testroot/stdout.expected
+	cat <<EOF > $testroot/stderr.expected
+got: alpha: patch doesn't apply
+got: numbers: file has unexpected status
+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 +756,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
 }