Re: patch(1): fix locate_hunk for empty files

[Stefan Sperling]

On Mon, Aug 01, 2022 at 03:38:59PM +0200, Omar Polo wrote:
> there's an edge case in locate_hunk with empty files; if `first_guess'
> is zero then main() assumes that locate_hunk has failed and aborts the
> patch operation.  Instead, make sure to return 1 (the line number) so
> that the patch operation can continue.
> 
> this issue was found by Neels Hofmeyr (I presume) in the diff
> implementation for got.  The regress suite for diff.git expects that
> `patch && patch -R' is an identity and so currently fails with base
> patch(1).
> 
> (while here i've also added a comment about t19 that was missing.)
> 
> ok?

ok stsp@

Unrelated to the problem you are fixing, but worth mentioning anyway:

Both 'got patch' and Larry's patch(1) fail to create t20.in during
the application of your diff. This can be trivially worked around
by running "touch t20.in".

I am unsure how a new empty file should be represented in unidiff.
I would expect to see at least a hunk header of the form @@ -0,0 +0,0 @@
but none of 'cvs diff', 'svn diff', and 'git diff' produce any hunk
headers for added empty files. They just produce their own custom
meta-data lines. ('hg diff' is even worse and doesn't print anything).
Which probably means patch(1) would need to be tought about lines such
as "Index" or similar, which are usually treated as noise? Horrible.

Testing the above hunk header idea, this unidiff successfully tricks
Larry patch(1) into creating an empty file:

--- /dev/null
+++ regress/usr.bin/patch/t20.in
@@ -0,0 +0,0 @@

So I suppose this issue should be fixed in diff tooling rather than
trying to work around it in patch(1).

patch(1): fix locate_hunk for empty files

[Omar Polo]

there's an edge case in locate_hunk with empty files; if `first_guess'
is zero then main() assumes that locate_hunk has failed and aborts the
patch operation.  Instead, make sure to return 1 (the line number) so
that the patch operation can continue.

this issue was found by Neels Hofmeyr (I presume) in the diff
implementation for got.  The regress suite for diff.git expects that
`patch && patch -R' is an identity and so currently fails with base
patch(1).

(while here i've also added a comment about t19 that was missing.)

ok?


P.S.: this actually partially solved by the other diff i sent previously
to tech, the "fix dwim for reversed patch", because with that `where' is
set back to one in the dwim code if the patch creates a file.  This is a
more general fix that kicks in before and works with -f.

diff /usr/src
commit - eeea9009e79d5923b3e3b65ab5436c46f05cf3c1
path + /usr/src
blob - dd8e9b713219196763460cc7179d891a4c288dfa
file + regress/usr.bin/patch/Makefile
--- regress/usr.bin/patch/Makefile
+++ regress/usr.bin/patch/Makefile
@@ -5,7 +5,7 @@ PATCHFLAGS= -sN
 CLEANFILES=*.copy *.orig *.rej t5 d19/*
 
 REGRESS_TARGETS= t1  t2  t3  t4  t5  t6  t7  t8  t9 \
-   t10 t11 t12 t13 t14 t15 t16 t17 t18 t19
+   t10 t11 t12 t13 t14 t15 t16 t17 t18 t19 t20
 
 # .in: input file
 # .diff: patch
@@ -27,6 +27,8 @@ REGRESS_TARGETS= t1  t2  t3  t4  t5  t6  t7  t8  t
 # t16: diff in ed format.
 # t17: diff in ed format that inserts a dot-line.
 # t18: diff in ed format that fully replaces input content.
+# t19: git-produced unified diff.
+# t20: reversal application of a patch to create a file with a single line.
 
 .SUFFIXES: .in
 
@@ -71,6 +73,13 @@ t19:
@${PATCH} -t ${PATCHFLAGS} < ${.CURDIR}/t19.diff
@cmp -s ${.CURDIR}/t19.out d19/file || (echo "XXX t19 failed" && false)
 
+t20:
+   @echo ${*}
+   @cp ${.CURDIR}/${*}.in ${*}.copy
+   @${PATCH} ${PATCHFLAGS} -Rf ${*}.copy ${.CURDIR}/${*}.diff
+   @cmp -s ${*}.copy ${.CURDIR}/${*}.out || \
+   (echo "XXX ${*} failed" && false)
+
 .PHONY: t5
 
 .include 
blob - /dev/null
file + regress/usr.bin/patch/t20.diff
--- /dev/null
+++ regress/usr.bin/patch/t20.diff
@@ -0,0 +1,4 @@
+--- full
 empty
+@@ -1 +0,0 @@
+-A
blob - /dev/null
file + regress/usr.bin/patch/t20.in
blob - /dev/null
file + regress/usr.bin/patch/t20.out
--- /dev/null
+++ regress/usr.bin/patch/t20.out
@@ -0,0 +1 @@
+A
blob - 67415a706e14c699af0da4dbd13bda955e60222b
file + usr.bin/patch/patch.c
--- usr.bin/patch/patch.c
+++ usr.bin/patch/patch.c
@@ -651,6 +651,8 @@ locate_hunk(LINENUM fuzz)
|| diff_type == UNI_DIFF)) {
say("Empty context always matches.\n");
}
+   if (first_guess == 0)
+   return 1;
return (first_guess);
}
if (max_neg_offset >= first_guess)  /* do not try lines < 0 */