Omar Polo <op@omarpolo.com> wrote:
> Stefan reminded me that the output the first line of `got diff' doesn't
> always have a commit ID, it may just as well have reference names.
> 
> the issue is that 'got patch' is expecting a commit id, otherwise it
> won't even try to do diff3 (it will still apply the patch as always,
> just not with the merge magic.)
> 
> diff below tries to fix this.  It makes got-read-patch and recv_patch
> slightly more permissive: we can have a blob id alone without a commit
> id, and changes the conflict markers to use the blob id if we don't have
> the commit id.

please ignore the previous patch.  I wrote most of it while talking on
irc with Stefan, but in the end he proposed to tweak the output of 'got
diff' in order to make the life easier for 'got patch'...
 
> it also fixes another issue, of which i'm not really sure of the way i'm
> addressing it.  open_blob may fails if we read some gibberish and
> thought it was a blob id.  i'm not really keen on inspecting errno in
> this case, should i always ignore open_blob failures regardless of the
> returned error?

this is the only thing to fix really.  The patch parser is quite loose
and, while it can be made somewhat more strict, since patches are
often included in other formats (e.g. emails) we might end up parsing
something that looks like a blob id but it isn't.

Since I'm not using the matching routines open_blob can't fail with
GOT_ERR_NO_REF anymore.  This was hidden by a change in the parser
(requiring commit id) that made the added test useless.

The diff attempts to fix both issue.  I'm still not too keen on looking
for an ENOENT thought.

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
@@ -1586,11 +1586,15 @@ test_patch_merge_unknown_blob() {
 		return 1
 	fi
 
+	local commit_id=`git_show_head $testroot/repo`
+
 	cat <<EOF > $testroot/wt/patch
 I've got a
+diff $commit_id foo/bar
+with a
 blob - aaaabbbbccccddddeeeeffff0000111122223333
 and also a
-blob + 0000111122223333444455556666888899990000
+blob + 0000111122223333444455556666777788889999
 for this dummy diff
 --- alpha
 +++ alpha
@@ -1612,7 +1616,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 $commit_id' 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
 }