git format-patch produces invalid patch if the commit adds an empty file?

git format-patch produces invalid patch if the commit adds an empty file?

[Adam Williamson]

Hi folks! So I ran into an odd issue with git today. I'm kinda
surprised I can't find any prior discussion of it, but oh well. The
situation is this: I ran git format-patch on a commit that adds three
empty files to a repository - this commit:
https://github.com/mesonbuild/meson/commit/5c87167a34c6ed703444af180fffd8a45a7928ee
the relevant lines from the patch file it produced look like this:

===

diff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt
new file mode 100644
index 000000000..e69de29bb
diff --git a/test cases/common/56 array methods/b.txt b/test cases/common/56 array methods/b.txt
new file mode 100644
index 000000000..e69de29bb
diff --git a/test cases/common/56 array methods/c.txt b/test cases/common/56 array methods/c.txt
new file mode 100644
index 000000000..e69de29bb

===

but `patch` actually chokes on that (when called in an RPM package build):

===

+ /usr/bin/cat /home/adamw/build/meson/0001-interpreter-Fix-list-contains-for-Holders-fixes-9020.patch
+ /usr/bin/patch -p1 -s --fuzz=0 --no-backup-if-mismatch -f
The text leading up to this was:
--------------------------
|diff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt
|new file mode 100644
|index 000000000..e69de29bb
--------------------------
No file to patch.  Skipping patch.
The text leading up to this was:
--------------------------
|diff --git a/test cases/common/56 array methods/b.txt b/test cases/common/56 array methods/b.txt
|new file mode 100644
|index 000000000..e69de29bb
--------------------------
No file to patch.  Skipping patch.
The text leading up to this was:
--------------------------
|diff --git a/test cases/common/56 array methods/c.txt b/test cases/common/56 array methods/c.txt
|new file mode 100644
|index 000000000..e69de29bb
--------------------------
No file to patch.  Skipping patch.

===

To make the patch apply cleanly, I had to hand-edit it to add "---" and
"+++" lines, like this:

===

diff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/test cases/common/56 array methods/a.txt
diff --git a/test cases/common/56 array methods/b.txt b/test cases/common/56 array methods/b.txt
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/test cases/common/56 array methods/b.txt
diff --git a/test cases/common/56 array methods/c.txt b/test cases/common/56 array methods/c.txt
new file mode 100644
index 000000000..e69de29bb
--- /dev/null
+++ b/test cases/common/56 array methods/c.txt

===

This is with git-2.32.0-1.fc35.1.x86_64 in Fedora Rawhide.

I'm not subscribed to the list, so please CC me directly on any replies. Thanks!
-- 
Adam Williamson
Fedora QA
IRC: adamw | Twitter: adamw_ha
https://www.happyassassin.net

Re: git format-patch produces invalid patch if the commit adds an empty file?

[Junio C Hamano]

Adam Williamson <awilliam@redhat.com> writes:

> Hi folks! So I ran into an odd issue with git today. I'm kinda
> surprised I can't find any prior discussion of it, but oh well. The
> situation is this: I ran git format-patch on a commit that adds three
> empty files to a repository - this commit:
> https://github.com/mesonbuild/meson/commit/5c87167a34c6ed703444af180fffd8a45a7928ee
> the relevant lines from the patch file it produced look like this:
>
> ===
>
> diff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt
> new file mode 100644
> index 000000000..e69de29bb
> diff --git a/test cases/common/56 array methods/b.txt b/test cases/common/56 array methods/b.txt
> new file mode 100644
> index 000000000..e69de29bb
> diff --git a/test cases/common/56 array methods/c.txt b/test cases/common/56 array methods/c.txt
> new file mode 100644
> index 000000000..e69de29bb

I do not have very ancient build of Git handy, but I know Git as old
as v1.3.0 (which I consider is one of the two versions of historical
importance, the other being v1.5.3) behaved this way and we haven't
changed it ever since, so I am surprised too to learn that "GNU
patch" cannot grok it.  Even though you didn't mention it, am I
correct to assume that "patch" has a similar issue with a change
that removes an empty file?

I do not think our patch injestion machinery in "git apply" minds if
we added the "--- /dev/null" + "+++ b/<path>" headers (and the
reverse for removal of an empty file) to the current output, and I
am not fundamentally opposed to such a change.

But because it is such a rare event (and a discouraged practice) to
record a completely empty file, I wouldn't place a high priority on
doing so myself.

Thanks.

Re: git format-patch produces invalid patch if the commit adds an empty file?

[Adam Williamson]

On Thu, 2021-08-19 at 14:09 -0700, Junio C Hamano wrote:
> Adam Williamson <awilliam@redhat.com> writes:
> 
> > Hi folks! So I ran into an odd issue with git today. I'm kinda
> > surprised I can't find any prior discussion of it, but oh well. The
> > situation is this: I ran git format-patch on a commit that adds three
> > empty files to a repository - this commit:
> > https://github.com/mesonbuild/meson/commit/5c87167a34c6ed703444af180fffd8a45a7928ee
> > the relevant lines from the patch file it produced look like this:
> > 
> > ===
> > 
> > diff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt
> > new file mode 100644
> > index 000000000..e69de29bb
> > diff --git a/test cases/common/56 array methods/b.txt b/test cases/common/56 array methods/b.txt
> > new file mode 100644
> > index 000000000..e69de29bb
> > diff --git a/test cases/common/56 array methods/c.txt b/test cases/common/56 array methods/c.txt
> > new file mode 100644
> > index 000000000..e69de29bb
> 
> I do not have very ancient build of Git handy, but I know Git as old
> as v1.3.0 (which I consider is one of the two versions of historical
> importance, the other being v1.5.3) behaved this way and we haven't
> changed it ever since, so I am surprised too to learn that "GNU
> patch" cannot grok it.  Even though you didn't mention it, am I
> correct to assume that "patch" has a similar issue with a change
> that removes an empty file?

Hi Junio!

I didn't test that. It does seem likely, though.

> I do not think our patch injestion machinery in "git apply" minds if
> we added the "--- /dev/null" + "+++ b/<path>" headers (and the
> reverse for removal of an empty file) to the current output, and I
> am not fundamentally opposed to such a change.
> 
> But because it is such a rare event (and a discouraged practice) to
> record a completely empty file, I wouldn't place a high priority on
> doing so myself.

Thanks.
-- 
Adam Williamson
Fedora QA
IRC: adamw | Twitter: adamw_ha
https://www.happyassassin.net

Re: git format-patch produces invalid patch if the commit adds an empty file?

[Gwyneth Morgan]

On 2021-08-19 14:09:43-0700, Junio C Hamano wrote:
> I do not think our patch injestion machinery in "git apply" minds if
> we added the "--- /dev/null" + "+++ b/<path>" headers (and the
> reverse for removal of an empty file) to the current output, and I
> am not fundamentally opposed to such a change.
> 
> But because it is such a rare event (and a discouraged practice) to
> record a completely empty file, I wouldn't place a high priority on
> doing so myself.

GNU patch chokes in this case with an unquoted filename with spaces.
However if we output

	diff --git "a/test cases/common/56 array methods/a.txt" "b/test cases/common/56 array methods/a.txt"

instead of

	diff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt

GNU patch (and Git) will read it correctly. Rather than adding the "---"
"+++" lines, could we instead quote filenames in the "diff --git" line
when they contain spaces?

Re: git format-patch produces invalid patch if the commit adds an empty file?

[Adam Williamson]

On Fri, 2021-08-20 at 06:15 +0000, Gwyneth Morgan wrote:
> On 2021-08-19 14:09:43-0700, Junio C Hamano wrote:
> > I do not think our patch injestion machinery in "git apply" minds if
> > we added the "--- /dev/null" + "+++ b/<path>" headers (and the
> > reverse for removal of an empty file) to the current output, and I
> > am not fundamentally opposed to such a change.
> > 
> > But because it is such a rare event (and a discouraged practice) to
> > record a completely empty file, I wouldn't place a high priority on
> > doing so myself.
> 
> GNU patch chokes in this case with an unquoted filename with spaces.
> However if we output
> 
> 	diff --git "a/test cases/common/56 array methods/a.txt" "b/test cases/common/56 array methods/a.txt"
> 
> instead of
> 
> 	diff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt
> 
> GNU patch (and Git) will read it correctly. Rather than adding the "---"
> "+++" lines, could we instead quote filenames in the "diff --git" line
> when they contain spaces?

Aha, I did actually wonder about that, because even with the added
lines, the patches don't apply (via `patch`) on Fedora 33 and 34 (and
the error message after adding the lines does seem to indicate the
spaces in the filenames as the culprit). They only apply on Fedora 35
and 36. I hadn't thought to just add quote marks, though of course it
seems obvious now. So yeah, that seems likely to be the best fix. I'll
try and confirm your results tomorrow. Thanks!
-- 
Adam Williamson
Fedora QA
IRC: adamw | Twitter: adamw_ha
https://www.happyassassin.net

Re: git format-patch produces invalid patch if the commit adds an empty file?

[Junio C Hamano]

Gwyneth Morgan <gwymor@tilde.club> writes:

> GNU patch chokes in this case with an unquoted filename with spaces.

When we settled what bytes (not characters) in a pathname will cause
it to be quoted and how the quoting is done between us and GNU diff
and patch maintainer back in Oct 2005, I thought that we excluded
whitespace from the bytes that need quoting [*].  And I do not
recall us changing the rule for pathname quoting since then (other
than introduction of core.quotepath to disable quoting bytes with
the 8th bit set).

It may be a "recent" change on the GNU patch side, and I do not
think we mind tweaking our diff output to be more accomodating iff
that observation is true.  I however understand that spaces in
pathnames are not so uncommon especially among non-programmers and
they may feel irritating having to see any pathname with spaces
quoted.


[Reference]

* https://lore.kernel.org/git/Pine.LNX.4.64.0510111121030.14597@g5.osdl.org/