threads / discuss / 56315

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

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

## tl;dr

6 messages between Aug 17, 2021 and Aug 20, 2021.

replies: 5people: 3as markdown or json

Adam Williamson· Aug 17, 2021, 18:50 UTC · lore

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
Junio C Hamano· Aug 19, 2021, 21:09 UTC · re: Adam Williamson · lore

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

Adam Williamson <awilliam@redhat.com> writes:
Show 18 quoted lines
> 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.
Adam Williamson· Aug 19, 2021, 21:25 UTC · re: Junio C Hamano · lore

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

On Thu, 2021-08-19 at 14:09 -0700, Junio C Hamano wrote:
Show 28 quoted lines
> 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.
Show 8 quoted lines
> 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
Gwyneth Morgan· Aug 20, 2021, 06:15 UTC · re: Junio C Hamano · lore

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

On 2021-08-19 14:09:43-0700, Junio C Hamano wrote:
Show 8 quoted lines
> 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?

Adam Williamson· Aug 20, 2021, 06:46 UTC · re: Gwyneth Morgan · lore

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

On Fri, 2021-08-20 at 06:15 +0000, Gwyneth Morgan wrote:
Show 22 quoted lines
> 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
Junio C Hamano· Aug 20, 2021, 21:09 UTC · re: Gwyneth Morgan · lore

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

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/

← back to recent threads