From: Omar Polo <op@omarpolo.com>
Subject: Re: got patch vs git diff with renames
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Mon, 02 May 2022 11:24:42 +0200

Stefan Sperling <stsp@stsp.name> wrote:
> On Fri, Apr 29, 2022 at 11:08:41AM +0200, Omar Polo wrote:
> > 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 committed also the rename, it makes read_patch easier too :)

> > I've also simplified the
> > test_patch_rename function other than adding a test case for this.
> > 
> > comments/ok?
> 
> Makes sense, ok.
> 
> No idea yet what kind of output 'got diff' will eventually produce
> for renames. But we need to handle Git-style diffs in any case.
> 
> > 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 :)
> 
> Agreed. If nobody else uses such empty hunks as a rename marker, then we
> shouldn't rely on this either.

here's the diff for it.

I'm still undecided if checking for empty hunks in patch.c too as a
safety net is a good idea given that got-read-patch now fails for those.

The rationale would be that if got-read-patch somehow misses that case,
we end up with a NULL h->lines that causes a segfault in patch_file.
Maybe we can just assume the libexec helper sends good data?  I'm
probably too paranoic.

-----------------------------------------------
commit 7ffb37ca4fa4da4ca16da7ace2601662bf382899 (main)
from: Omar Polo <op@omarpolo.com>
date: Mon May  2 09:17:26 2022 UTC
 
 got patch: fail when reading empty hunks
 
diff d52174ea58f415155c2d134d834ebec18057f9e1 3a53a56ddc8859aaa9ae2f8aeeef8e2c2b8e3ad6
blob - 9a66825918efbd6b934add0ed0636e397fb31edd
blob + d7d7afe8b98daec948e859725cc2686ec675b8f5
--- lib/patch.c
+++ lib/patch.c
@@ -215,9 +215,12 @@ recv_patch(struct imsgbuf *ibuf, int *done, struct got
 
 		switch (imsg.hdr.type) {
 		case GOT_IMSG_PATCH_DONE:
+			if (h != NULL && h->len == 0)
+				err = got_error(GOT_ERR_PATCH_MALFORMED);
 			goto done;
 		case GOT_IMSG_PATCH_HUNK:
-			if (h != NULL && (h->old_nonl || h->new_nonl)) {
+			if (h != NULL &&
+			    (h->len == 0 || h->old_nonl || h->new_nonl)) {
 				err = got_error(GOT_ERR_PATCH_MALFORMED);
 				goto done;
 			}
@@ -470,9 +473,6 @@ patch_file(struct got_patch *p, const char *path, FILE
 
 	copypos = 0;
 	STAILQ_FOREACH(h, &p->head, entries) {
-		if (h->lines == NULL)
-			break;
-
 	tryagain:
 		err = locate_hunk(orig, h, &pos, &lineno);
 		if (err != NULL && err->code == GOT_ERR_HUNK_FAILED)
blob - f52ad654228e0ebd0a2dbc9fbda2e21ce3f3fd62
blob + 05b366e7f3618252dc0954c2851d63463fd5b22f
--- libexec/got-read-patch/got-read-patch.c
+++ libexec/got-read-patch/got-read-patch.c
@@ -267,7 +267,8 @@ parse_hdr(char *s, int *done, struct got_imsg_patch_hu
 	if (hdr->oldfrom >= LONG_MAX - hdr->oldlines ||
 	    hdr->newfrom >= LONG_MAX - hdr->newlines ||
 	    /* not so sure about this one */
-	    hdr->oldlines >= LONG_MAX - hdr->newlines - 1)
+	    hdr->oldlines >= LONG_MAX - hdr->newlines - 1 ||
+	    (hdr->oldlines == 0 && hdr->newlines == 0))
 		return got_error(GOT_ERR_PATCH_MALFORMED);
 
 	if (hdr->oldlines == 0) {
blob - b30e9e02052c205e40d442f72d8988369b6eea06
blob + 155c33f0173f68f8c1375feaf38ffc73113b0b7e
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -537,6 +537,40 @@ EOF
 		return 1
 	fi
 
+	# empty hunk
+	cat <<EOF > $testroot/wt/patch
+diff --git a/alpha b/iota
+--- a/alpha
++++ b/iota
+@@ -0,0 +0,0 @@
+EOF
+
+	(cd $testroot/wt && got patch patch) \
+		 > $testroot/stdout \
+		2> $testroot/stderr
+	ret=$?
+	if [ $ret -eq 0 ]; then
+		echo "got managed to apply an invalid patch"
+		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
+
 	test_done $testroot $ret
 }