"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Omar Polo <op@omarpolo.com>
Subject:
got patch vs git diff with renames
To:
gameoftrees@openbsd.org
Date:
Fri, 29 Apr 2022 11:08:41 +0200

Download raw body.

Thread
A recent mail on ports@ (the net/znc diff) reminded me that there's
still a corner case of git-style rename diffs that I forgot to handle:
renames without edits.  Such diffs looks like this

> diff --git a/net/znc/pkg/DESCR b/net/znc/pkg/DESCR-main
> similarity index 100%
> rename from net/znc/pkg/DESCR
> rename to net/znc/pkg/DESCR-main

and don't have any hunks.  I don't have any idea why they decided to
avoid the +++/--- syntax and include a "@@ -0,0 +0,0 @@" since
compatibility doesn't seem to be a concern...

Here's a straightforward diff to handle this situation.  I intend to
rename that "ok" variable in read_patch in a follow-up diff (needs the
same rename also in parse_hunk and parse_hdr.)  I've also simplified the
test_patch_rename function other than adding a test case for this.

comments/ok?

P.S.: i forgot that we supported the rename with an empty hunk "@@ -0,0
+0,0 @@".  Since git doesn't seem to handle it, and neither patch(1), I
intend to remove the support for it, but again, in a follow-up commit :)

diff a8acad7d8eeb9aca78302e45eba5eb2211e6f291 /home/op/w/got
blob - 879b77f8142d65729f38b9389c48cf5a78299027
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
@@ -123,14 +123,14 @@ filename(const char *at, char **name)
 }
 
 static const struct got_error *
-find_patch(FILE *fp)
+find_patch(int *empty, FILE *fp)
 {
 	const struct got_error *err = NULL;
 	char	*old = NULL, *new = NULL;
 	char	*line = NULL;
 	size_t	 linesize = 0;
 	ssize_t	 linelen;
-	int	 create, git = 0;
+	int	 create, rename = 0, git = 0;
 
 	while ((linelen = getline(&line, &linesize, fp)) != -1) {
 		/*
@@ -141,15 +141,34 @@ find_patch(FILE *fp)
 		if (!strncmp(line, "--- ", 4)) {
 			free(old);
 			err = filename(line+4, &old);
+		} else if (rename && !strncmp(line, "rename from ", 12)) {
+			free(old);
+			err = filename(line+12, &old);
 		} else if (!strncmp(line, "+++ ", 4)) {
 			free(new);
 			err = filename(line+4, &new);
-		} else if (!strncmp(line, "diff --git a/", 13))
+		} else if (rename && !strncmp(line, "rename to ", 10)) {
+			free(new);
+			err = filename(line + 10, &new);
+		} else if (git && !strncmp(line, "similarity index 100%", 21))
+			rename = 1;
+		else if (!strncmp(line, "diff --git a/", 13))
 			git = 1;
 
 		if (err)
 			break;
 
+		/*
+		 * Git-style diffs with "similarity index 100%" don't
+		 * have any hunks and ends with the "rename to foobar"
+		 * line.
+		 */
+		if (rename && old != NULL && new != NULL) {
+			*empty = 1;
+			err = send_patch(old, new, git);
+			break;
+		}
+
 		if (!strncmp(line, "@@ -", 4)) {
 			create = !strncmp(line+4, "0,0", 3);
 			if ((old == NULL && new == NULL) ||
@@ -412,7 +431,7 @@ read_patch(struct imsgbuf *ibuf, int fd)
 {
 	const struct got_error *err = NULL;
 	FILE *fp;
-	int ok, patch_found = 0;
+	int patch_found = 0;
 
 	if ((fp = fdopen(fd, "r")) == NULL) {
 		err = got_error_from_errno("fdopen");
@@ -421,16 +440,19 @@ read_patch(struct imsgbuf *ibuf, int fd)
 	}
 
 	while (!feof(fp)) {
-		err = find_patch(fp);
+		int empty = 0, ok = 1;
+
+		err = find_patch(&empty, fp);
 		if (err)
 			goto done;
 
 		patch_found = 1;
 		for (;;) {
-			err = parse_hunk(fp, &ok);
+			if (!empty)
+				err = parse_hunk(fp, &ok);
 			if (err)
 				goto done;
-			if (!ok) {
+			if (!ok || empty) {
 				err = send_patch_done();
 				if (err)
 					goto done;
blob - 43981599f51928fd929e8cc47538da750b875b97
file + regress/cmdline/patch.sh
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -647,14 +647,22 @@ test_patch_rename() {
 	fi
 
 	cat <<EOF > $testroot/wt/patch
+diff --git a/beta b/iota
+similarity index 100%
+rename from beta
+rename to iota
 diff --git a/alpha b/eta
 --- a/alpha
 +++ b/eta
-@@ -0,0 +0,0 @@
+@@ -1 +1 @@
+-alpha
++eta
 EOF
 
-	echo 'D  alpha' > $testroot/stdout.expected
-	echo 'A  eta'  >> $testroot/stdout.expected
+	echo 'D  beta'   > $testroot/stdout.expected
+	echo 'A  iota'  >> $testroot/stdout.expected
+	echo 'D  alpha' >> $testroot/stdout.expected
+	echo 'A  eta'   >> $testroot/stdout.expected
 
 	(cd $testroot/wt && got patch patch) > $testroot/stdout
 	ret=$?
@@ -671,77 +679,35 @@ EOF
 		return 1
 	fi
 
-	if [ -f $testroot/wt/alpha ]; then
-		echo "alpha was not removed" >&2
+	if [ -f $testroot/wt/alpha -o -f $testroot/wt/beta ]; then
+		echo "alpha or beta were not removed" >&2
 		test_done $testroot 1
 		return 1
 	fi
-	if [ ! -f $testroot/wt/eta ]; then
-		echo "eta was not created" >&2
+	if [ ! -f $testroot/wt/iota -o ! -f $testroot/wt/eta ]; then
+		echo "iota or eta were not created" >&2
 		test_done $testroot 1
 		return 1
 	fi
 
-	echo alpha > $testroot/wt/eta.expected
-	cmp -s $testroot/wt/eta.expected $testroot/wt/eta
+	echo beta > $testroot/wt/iota.expected
+	cmp -s $testroot/wt/iota.expected $testroot/wt/iota
 	ret=$?
 	if [ $ret -ne 0 ]; then
-		diff -u $testroot/wt/eta.expected $testroot/wt/eta
+		diff -u $testroot/wt/iota.expected $testroot/wt/iota
 		test_done $testroot $ret
 		return 1
 	fi
 
-	# revert the changes and try again with a rename + edit
-	(cd $testroot/wt && got revert alpha eta) > /dev/null
+	echo eta > $testroot/wt/eta.expected
+	cmp -s $testroot/wt/eta.expected $testroot/wt/eta
 	ret=$?
 	if [ $ret -ne 0 ]; then
+		diff -u $testroot/wt/eta.expected $testroot/wt/eta
 		test_done $testroot $ret
 		return 1
 	fi
-	rm $testroot/wt/eta
 
-	cat <<EOF > $testroot/wt/patch
-diff --git a/alpha b/eta
---- a/alpha
-+++ b/eta
-@@ -1 +1,2 @@
- alpha
-+but now is eta
-EOF
-
-	(cd $testroot/wt && got patch patch) > $testroot/stdout
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		test_done $testroot $ret
-		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
-
-	if [ -f $testroot/wt/alpha ]; then
-		echo "alpha was not removed" >&2
-		test_done $testroot 1
-		return 1
-	fi
-	if [ ! -f $testroot/wt/eta ]; then
-		echo "eta was not created" >&2
-		test_done $testroot 1
-		return 1
-	fi
-
-	echo alpha > $testroot/wt/eta.expected
-	echo 'but now is eta' >> $testroot/wt/eta.expected
-	cmp -s $testroot/wt/eta.expected $testroot/wt/eta
-	ret=$?
-	if [ $ret -ne 0 ]; then
-		diff -u $testroot/wt/eta.expected $testroot/wt/eta
-	fi
 	test_done $testroot $ret
 }