From: Omar Polo <op@omarpolo.com>
Subject: got patch: delete binary files too
To: gameoftrees@openbsd.org
Date: Sat, 31 Dec 2022 16:05:26 +0100

Spotted by Stefan with the gotweb removal diff, `got patch' fails to
delete binary files.

What happens is that got-read-patch doesn't see any hunks and
considers those lines noise.  For reference, a got diff that deletes a
binary file looks like this:

	blob - 460aa1299f8e9f37773618bcab2619794416fb49
	file + /dev/null
	Binary files foobar.png and /dev/null differ

git diffs are similar:

	diff --git a/foobar.png b/foobar.png
	deleted file mode 100644
	index 460aa129..00000000
	Binary files foobar.png and /dev/null differ

diff and cvs diff looks similar but they don't provide the filename in
a way that's easy to extract so they're left out.  Here's an example
for reference:

	% diff -u bin/got /dev/null
	Binary files bin/got and /dev/null differ

	% rm rpki-client.png
	% cvs rm rpki-client.png
	cvs remove: scheduling `rpki-client.png' for removal
	cvs remove: use 'cvs commit' to remove this file permanently
	% cvs diff
	cvs diff: Diffing .
	Index: rpki-client.png
	===================================================================
	RCS file: rpki-client.png
	diff -N rpki-client.png
	Binary files /tmp/cvsq7uY1c and /dev/null differ


The idea then is to parse the "file + /dev/null" or "deleted file
mode" lines and extract the filename from the "Binary files foo and
/dev/null differ."  There's no problem with ambiguous spaces since we
can treat "Binary files " as prefix and the " and /dev/null differ"
string as suffix and strip them, what remains is the filename.

(the patch file format has further restrictions on the bytes allowed
on the filenames, for e.g. newlines and tabs are not permitted
anyway.)

ok?

diff /home/op/w/got
commit - cf536071bc57734308f29cda79d67c88abb3b9f0
path + /home/op/w/got
blob - 6c31c4b5e3788850c30c78ed552e90f1eddd3982
file + lib/patch.c
--- lib/patch.c
+++ lib/patch.c
@@ -587,6 +587,10 @@ patch_file(struct got_patch *p, FILE *orig, FILE *tmp)
 		return apply_hunk(orig, tmp, h, &lineno, 0);
 	}
 
+	/* When deleting binary files there are no hunks to apply. */
+	if (p->new == NULL && STAILQ_EMPTY(&p->head))
+		return NULL;
+
 	if (fstat(fileno(orig), &sb) == -1)
 		return got_error_from_errno("fstat");
 
blob - becf27d3e00feb1fd2b32823931dc0d1b580cfb6
file + libexec/got-read-patch/got-read-patch.c
--- libexec/got-read-patch/got-read-patch.c
+++ libexec/got-read-patch/got-read-patch.c
@@ -128,6 +128,26 @@ static int
 	return NULL;
 }
 
+static const struct got_error *
+binaryfilename(const char *at, char **name)
+{
+	const char *trail = " and /dev/null differ\n";
+	size_t len, d;
+
+	len = strlen(at);
+	if (len <= strlen(trail))
+		return NULL;
+
+	d = len - strlen(trail);
+	if (strcmp(at + d, trail) != 0)
+		return NULL;
+
+	*name = strndup(at, d);
+	if (*name == NULL)
+		return got_error_from_errno("strndup");
+	return NULL;
+}
+
 static int
 filexbit(const char *line)
 {
@@ -212,7 +232,7 @@ find_diff(int *done, int *next, FILE *fp, int git, con
 	char	*line = NULL;
 	size_t	 linesize = 0;
 	ssize_t	 linelen;
-	int	 create, rename = 0, xbit = 0;
+	int	 create, delete = 0, rename = 0, xbit = 0, binary = 0;
 
 	*done = 0;
 	*next = 0;
@@ -231,13 +251,21 @@ find_diff(int *done, int *next, FILE *fp, int git, con
 		} else if (!strncmp(line, "+++ ", 4)) {
 			free(new);
 			err = filename(line+4, &new);
-		} else if (!strncmp(line, "blob + ", 7) ||
-		    !strncmp(line, "file + ", 7)) {
+		} else if (!strncmp(line, "blob + ", 7)) {
 			xbit = filexbit(line);
-		} else if (!git && !strncmp(line, "blob - ", 7)) {
+			delete = !strcmp(line + 7, "/dev/null\n");
+		} else if (!strncmp(line, "file + ", 7))
+			xbit = filexbit(line);
+		else if (!git && !strncmp(line, "blob - ", 7)) {
 			free(blob);
 			err = blobid(line + 7, &blob, git);
-		} else if (rename && !strncmp(line, "rename to ", 10)) {
+		} else if (!strncmp(line, "Binary files ", 13)) {
+			binary = 1;
+			free(old);
+			err = binaryfilename(line + 13, &old);
+		} else if (git && !strncmp(line, "deleted file mode ", 18))
+			delete = 1;
+		else if (rename && !strncmp(line, "rename to ", 10)) {
 			free(new);
 			err = filename(line + 10, &new);
 		} else if (git && !strncmp(line, "similarity index 100%", 21))
@@ -270,6 +298,16 @@ find_diff(int *done, int *next, FILE *fp, int git, con
 			break;
 		}
 
+		/*
+		 * Diffs that remove binary files have no hunks.
+		 */
+		if (delete && binary) {
+			*done = 1;
+			err = send_patch(old, new, commitid,
+			    blob, xbit, git);
+			break;
+		}
+
 		if (!strncmp(line, "@@ -", 4)) {
 			create = !strncmp(line+4, "0,0", 3);
 			if ((old == NULL && new == NULL) ||
blob - 9e3d4639d01241422b50bdddf316137ceb10d945
file + regress/cmdline/patch.sh
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -1881,6 +1881,61 @@ test_parseargs "$@"
 	test_done "$testroot" 0
 }
 
+test_patch_remove_binary_file() {
+	local testroot=`test_init patch_remove_binary_file`
+
+	if ! got checkout $testroot/repo $testroot/wt >/dev/null; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	dd if=/dev/zero of=$testroot/wt/x bs=1 count=16 2>/dev/null >&2
+	(cd $testroot/wt && got add x && got commit -m +x) >/dev/null
+
+	(cd $testroot/wt && \
+		got branch demo && \
+		got rm x && \
+		got ci -m -x &&
+		got log -l 1 -p >patch &&
+		got up -b master) >/dev/null
+
+	echo 'D  x' > $testroot/stdout.expected
+
+	(cd $testroot/wt && got patch <patch) > $testroot/stdout
+	if [ $? -ne 0 ]; then
+		echo 'patch failed' >&2
+		test_done $testroot 1
+		return 1
+	fi
+
+	if ! cmp -s $testroot/stdout.expected $testroot/stdout; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done $testroot 1
+		return 1
+	fi
+
+	# try again using a git produced diff
+	(cd $testroot/wt && got rv x) >/dev/null
+
+	local commit_id=`git_show_branch_head $testroot/repo demo`
+	(cd $testroot/repo && git show $commit_id) >$testroot/wt/patch
+
+	(cd $testroot/wt && got patch <patch) > $testroot/stdout
+	if [ $? -ne 0 ]; then
+		echo 'patch failed' >&2
+		test_done $testroot 1
+		return 1
+	fi
+
+	if ! cmp -s $testroot/stdout.expected $testroot/stdout; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done $testroot 1
+		return 1
+	fi
+
+	test_done $testroot 0
+}
+
 test_parseargs "$@"
 run_test test_patch_basic
 run_test test_patch_dont_apply
@@ -1910,3 +1965,4 @@ run_test test_patch_umask
 run_test test_patch_newfile_xbit_got_diff
 run_test test_patch_newfile_xbit_git_diff
 run_test test_patch_umask
+run_test test_patch_remove_binary_file