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

Adam Williamson [off-list ref] writes:

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.

On Thu, 2021-08-19 at 14:09 -0700, Junio C Hamano wrote:

Adam Williamson [off-list ref] writes:

quoted

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

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?

On Fri, 2021-08-20 at 06:15 +0000, Gwyneth Morgan wrote:

On 2021-08-19 14:09:43-0700, Junio C Hamano wrote:

quoted

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

Gwyneth Morgan [off-list ref] 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/