Stefan Sperling <stsp@stsp.name> wrote:
> On Tue, Jun 21, 2022 at 09:35:14AM +0200, Omar Polo wrote:
> > The diff attempts to fix both issue.  I'm still not too keen on looking
> > for an ENOENT thought.
> 
> Catching an error with code == GOT_ERR_ERRNO is perfectly valid.
> GOT_ERR_ERRNO + ENOENT is indeed the correct error condition to check for.
> Your open_blob() function eventually ends up in got_object_open_loose_fd()
> which runs open(2) and converts failures to got_error_from_errno2().
> 
> On IRC I was thinking of the packed object case, in which case the
> error would be GOT_ERR_NO_OBJ. But it turns out all the got_object_open()
> function hide this error from you and fall back on trying to open a loose
> object instead. So if the object does not exist at all, neither in packed
> nor loose form, you'll only ever see the open(2) failure.
> 
> > diff 6131ff18e81056001a823f913094a92c10580cba /home/op/w/got
> > blob - 9209e54ce7111b54b69980a44a48a92a333e5888
> > file + lib/patch.c
> > --- lib/patch.c
> > +++ lib/patch.c
> > @@ -643,7 +643,11 @@ apply_patch(int *overlapcnt, struct got_worktree *work
> >  	/* don't run the diff3 merge on creations/deletions */
> >  	if (*p->blob != '\0' && p->old != NULL && p->new != NULL) {
> >  		err = open_blob(&apath, &afile, p->blob, repo);
> > -		if (err && err->code != GOT_ERR_NOT_REF)
> > +		/*
> > +		 * ignore failures to open this blob, we might have
> > +		 * parsed gibberish.
> > +		 */
> > +		if (err && err->code != GOT_ERR_ERRNO && errno == ENOENT)
> >  			return err;
> >  		else if (err == NULL)
> >  			do_merge = 1;
> 
> Above condition looks wrong. Did you mean this?
> 
> 		if (err && !(err->code == GOT_ERR_ERRNO && errno == ENOENT))
> 
> Which could of course also be expressed as below, if you prefer:
> 
> 		if (err && (err->code != GOT_ERR_ERRNO || errno != ENOENT))

yes...  thanks for spotting it!

diff below also tweaks the test case by using a dummy commit id instead
of the real one.

diff 6131ff18e81056001a823f913094a92c10580cba /home/op/w/got
blob - 9209e54ce7111b54b69980a44a48a92a333e5888
file + lib/patch.c
--- lib/patch.c
+++ lib/patch.c
@@ -643,7 +643,11 @@ apply_patch(int *overlapcnt, struct got_worktree *work
 	/* don't run the diff3 merge on creations/deletions */
 	if (*p->blob != '\0' && p->old != NULL && p->new != NULL) {
 		err = open_blob(&apath, &afile, p->blob, repo);
-		if (err && err->code != GOT_ERR_NOT_REF)
+		/*
+		 * ignore failures to open this blob, we might have
+		 * parsed gibberish.
+		 */
+		if (err && !(err->code == GOT_ERR_ERRNO && errno == ENOENT))
 			return err;
 		else if (err == NULL)
 			do_merge = 1;
blob - 20e899eab3a352dc9e8636b15e894c186595d20a
file + regress/cmdline/patch.sh
--- regress/cmdline/patch.sh
+++ regress/cmdline/patch.sh
@@ -1588,9 +1588,11 @@ test_patch_merge_unknown_blob() {
 
 	cat <<EOF > $testroot/wt/patch
 I've got a
+diff aaaabbbbccccddddeeeeffff0000111122223333 foo/bar
+with a
 blob - aaaabbbbccccddddeeeeffff0000111122223333
 and also a
-blob + 0000111122223333444455556666888899990000
+blob + 0000111122223333444455556666777788889999
 for this dummy diff
 --- alpha
 +++ alpha
@@ -1612,7 +1614,40 @@ EOF
 	ret=$?
 	if [ $ret -ne 0 ]; then
 		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done $testroot $ret
+		return 1
 	fi
+
+	# try again without a `diff' header
+
+	cat <<EOF > $testroot/wt/patch
+I've got a
+blob - aaaabbbbccccddddeeeeffff0000111122223333
+and also a
+blob + 0000111122223333444455556666777788889999
+for this dummy diff
+--- alpha
++++ alpha
+@@ -1 +1 @@
+-alpha
++ALPHA
+will it work?
+EOF
+
+	(cd $testroot/wt && got revert alpha > /dev/null && got patch patch) \
+		> $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done $testroot $ret
+		return 1
+	fi
+
+	echo 'M  alpha' > $testroot/stdout.expected
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+	fi
 	test_done $testroot $ret
 }