patchapply: gitdiff_verify_name(): accept "/dev/null\r"

The edk2 (EFI Development Kit II) project at <https://github.com/tianocore/edk2/> uses CRLF line endings.

The following small reproducer demonstrates how gitdiff_verify_name() breaks when it meets the usual git patches workflow in combination with CRLF line endings:

1. Prepare the test repo:

  mkdir testdir
  cd testdir
  git init
  git config core.whitespace cr-at-eol
  git config am.keepcr true
  touch f1
  git add f1
  git commit -m 'initial import'

2. In the contributor role, write a patch that creates a new file
(adhering to the CRLF convention), submit it, then clean up:

  git checkout -b branch1 master
  echo 'hello world' | unix2dos >f2
  git add f2
  git commit -m 'add f2'
  git format-patch master..branch1
  git send-email 0001-add-f2.patch
  # send it to yourself -- make sure it goes through your MTA
  git clean -fdx

3. In the reviewer / tester / maintainer role, save the patch from your
email client to a local file. Assume that your email client does not
corrupt the patch when saving it. (Thunderbird doesn't corrupt it, for
example.) Once saved, try to apply the patch email to a new branch.

  git checkout -b branch2 master
  git am /path/to/the/saved/file

  -> Applying: add f2
  -> fatal: git apply: bad git-diff - expected /dev/null on line 9

This happens because am.keepcr==true keeps the CRLFs intact (as it should in fact), but then "/dev/null\r" in the diff header trips up gitdiff_verify_name().

Fix it by reusing the is_dev_null() helper function, which in effect changes the condition from

  memcmp("/dev/null", line, 9) || line[9] != '\n'

to

  memcmp("/dev/null", line, 9) || !isspace(line[9])

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    I'm not subscribed to the list; please keep me CC'd. Thanks.

 builtin/apply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Laszlo Ersek <lersek@redhat.com> writes:

The output from this has these (excerpt from "od -xc" output):

0000360       f   2  \n  \n   d   i   f   f       -   -   g   i   t
           6620    0a32    640a    6669    2066    2d2d    6967    2074
0000400   a   /   f   2       b   /   f   2  \n   n   e   w       f   i
           2f61    3266    6220    662f    0a32    656e    2077    6966
0000420   l   e       m   o   d   e       1   0   0   6   4   4  \n   i
           656c    6d20    646f    2065    3031    3630    3434    690a
0000440   n   d   e   x       0   0   0   0   0   0   0   .   .   f   3
           646e    7865    3020    3030    3030    3030    2e2e    3366
0000460   5   d   3   e   6  \n   -   -   -       /   d   e   v   /   n
           6435    6533    0a36    2d2d    202d    642f    7665    6e2f
0000500   u   l   l  \n   +   +   +       b   /   f   2  \n   @   @
           6c75    0a6c    2b2b    202b    2f62    3266    400a    2040
0000520   -   0   ,   0       +   1       @   @  \n   +   h   e   l   l
           302d    302c    2b20    2031    4040    2b0a    6568    6c6c
0000540   o       w   o   r   l   d  \r  \n   -   -      \n   2   .   1
           206f    6f77    6c72    0d64    2d0a    202d    320a    312e

The structural parts of the diff, including "--- /dev/null" line, are all terminated by "\n" (as they should be), and the only CR appears in the message is at the end of "+hello world" line.

So I do not think apply should need to loosen its sanity check and take a random whitespace after the "/dev/null" as a valid "this is a creation event for the path" marker (e.g. "--- /dev/null whoa"?).

is_dev_null() is used to in the fallback code path that parses traditional patch output (e.g. GNU diff) which throws random cruft (e.g. timestamp) after the /dev/null marker, e.g.

    $ diff -u /dev/null f2
    --- /dev/null   2014-09-17 18:22:57.995111003 -0700
    +++ f2  2014-09-23 11:37:09.000000000 -0700
    @@ -0,0 +1 @@
    +hello world

and we'd be hesitant to allow that kind of looseness for Git patches where we know we end the line after the "/dev/null" marker.

Perhaps compare this saved file with the output from the above format-patch to see where things got broken?

SMTP transport may be CRLF-unsafe, so I have a suspicion that it may turn out that what you are trying to do might be an equilvalent of

	git format-patch ... |
        # first lose all \r\n
        dos2unix | 
	# then make everything \r\n
        unix2dos |
        # and apply
        git am

which is not workable in the first place.  I dunno.

On 09/23/14 20:54, Junio C Hamano wrote:

That's right -- until the patch email goes through an MTA that turns all line endings into CRLF. (Did you email the patch to yourself as requested in the reproducer?)

Such CRLFs are normally transparent because git-am strips them. The keepcr=true setting preserves them, but not only for the source code lines (where it's the right thing to do): it also preserves them in the git diff header lines. Which is not a problem in general, *except* when said header line includes /dev/null.

Okay, let's say I only relax the check in question to accept "\r\n" in addition to the current "\n". Will you take that?

I agree with your analysis. It is indeed the MTA (or the MUA, not sure) that turns all line endings into uniform CRLFs -- it is a requirement in RFC 2822 / 822bis.

http://cr.yp.to/docs/smtplf.html http://www.rfc-editor.org/rfc/rfc2822.txt

But why is this situation "not workable"? The same happens with *all* patches that people mail around, it's just not visible to them, because git-am strips all CRs indiscriminately.

People who are forced to work with CRLF repositories don't have this luxury with git-am, and bump into the /dev/null problem all the time.

What do you think about accepting only "/dev/null\n" and "/dev/null\r\n"?

Another question I had about gitdiff_verify_name() -- what ensures there that the memcmp(), with the fixed size of 9 bytes, won't fall off the end of the line? Some of the outer caller functions verify the line length before their comparisons, but I don't see any length checks in gitdiff_verify_name() for /dev/null specifically.

Thank you, Laszlo

Laszlo Ersek <lersek@redhat.com> writes:

I thought we agreed that what you are doing is not workable in the first place, no?

I suspect one way to handle "In this project, the files that are checked out must be with CRLF line endings no matter what the platform is" might be to use the line ending attributes to force that while keeping the in-repository data with LF line endings. The diff output (format-patch output is just one of them) comes from comparing the in-repository representation, so you won't have \r\n that will be stripped via MTA in it, "apply" and "am" will apply the patch without having to worry about \r\n, _and_ the line ending attributes would end the lines in your in-working-tree files with CRLF that way.

On 09/23/14 21:56, Junio C Hamano wrote:

This would be a perfect solution if the git repository was not a mirror of a Subversion repository that contains all files with embedded CRLFs.

Anyway I accept defeat, thanks for your time.

Laszlo

Laszlo Ersek <lersek@redhat.com> writes:

Yikes.

I do not consider that a "defeat". It is just I do not think it is the right solution for the problem you are having to butcher "apply". You'd need to find some other way to solve it, and other people on the list may be able to offer a solution neither of us thought of in this thread.

Perhaps those who are on Windows have more experience in situations like yours?

On 09/23/14 22:40, Junio C Hamano wrote:

I'm not on Windows, "obviously". :)

The overwhelming majority of the EDK II developers use windows, and connect directly to subversion. They work with CRLF line endings "natively".

Jordan (CC'd) operates a robot that mirrors SVN commits to the git repo on github, with "git svn". "git svn rebase --use-log-author" fetches the new SVN commits to the robots local git clone, and then "git push" (as usual) pushes them to github. This is being done so that people knowing git don't lose their sanity, trying to work with SVN.

The process works very well, up to a point (git-loving people clone the github mirror, and submit patches with git-format-patch / git send-email to edk2-devel). The problem is when you want to apply patches with git-am from the list -- the CRLFs are a mess. Hence my thread starter here.

We can get around this by maintaining personal forks on github, pushing our patches there too in parallel with the email postings. People can then fetch directly, and avoid git-am altogether. But this is very cumbersome -- you need a github account, you need an edk2 fork on github, others need to add your repo as a remote, etc etc, while the review occurs anyway in our MUAs.

Thanks Laszlo

Laszlo Ersek <lersek@redhat.com> writes:

It is not "git am" or "git apply" that "strips all CRs indiscriminately". I just tried to apply 0001-add-f2 without letting your MTA/MUA corrupt it on "master" branch in the repository you prepared that patch from, i.e.

	git checkout master^0 ;# go back
        git am 0001-add-f2* ;# apply that "+hello world\r\n" patch
        git diff branch ;# nothing

That may be worth fixing.

On 09/23/14 22:02, Junio C Hamano wrote:

When you did this, was am.keepcr=true in effect?

Thanks Laszlo

Laszlo Ersek <lersek@redhat.com> writes:

I actually briefly scratched my head but realized when I saw it work "as expected" without me passing "--keep-cr" to "am" myself.

But I did that experiment in the repository created by following your reproduction recipe, in which it had these:

  git config core.whitespace cr-at-eol
  git config am.keepcr true

so yes I had keepcr set.

On 09/23/14 22:35, Junio C Hamano wrote:

Thank you for confirming, I expected so.

Because in this case the test doesn't refute my claim that "git-am strips all CRs indiscriminately".

Git-am *does* strip all CRs indiscriminately, undoing the CRs that the email servers / clients introduce. Your above test worked out because you prevented git-am from stripping the CRs, with the keepcr=true setting. If you turn that off, then either your git am command won't succeed (because it will run into context conflicts due to different line endings -- although not in this example), or the final git-diff will report differences.

In summary:
- the email infrastructure turns all line terminators into CRLFs
- git-am strips these by default, from source code lines and from git
  diff header lines alike,
- this is fine for repos that store files with \n terminators,
- not fine for repos with embedded \r\n terminators -- the default
  stripping behavior of git-am breaks the source code in that case
  (runs into conflicts with existing files, and creates new files with
  wrong line endings)
- if you set am.keepcr=true, then the source code remains intact (no
  conflicts for existing files), but new files cannot be created,
  because the /dev/null\r header lines are rejected.

Thanks Laszlo

Laszlo Ersek <lersek@redhat.com> writes:

This is not entirely correct, though. But it suggests an avenue for a possible enhancement.

Yes, but that is within MTAs and is expected to be invisible at MUA level. Typically mailbox files are stored in platform's native format (e.g. I see LF endings in my mailbox), be it CRLF of LF.

The important thing to note here is that use of text/plain for patches, if you want to have distinction between CRLF and LF in your payload, is not designed to work over e-mails.

On a platform whose native line endings are LF, the stripping may not even happen (i.e. "am/apply" may not see CRLF in the mailbox in the first place).

On a platform whose mbox has CRLF, or if you "unix2dos" a mbox on a platform whose mbox has LF to manufacture a mbox with CRLF line endings, you would have an opposite issue. Information is lost in MTAs while your e-mail is in transit and you cannot tell patch to which paths had CRLF line endings and patch to which paths had LF line endings. If all files you are tracking uniformly use CRLF, you can assume that everything had CRLF but it is still an assumption and that is a special case that is not particularly interesting.

Applying such a CRLF patch with your "/dev/null\r\n" patch applied to "am" will _add_ unwanted CR before LF to files that are meant to use LF line endings.

Now if we accept that this issue is coming from lossy nature of transporting patches via e-mails, we would realize that the right place to solve this is not in "apply"'s parsing of structural part of the "diff" output (e.g. "diff --git ...", "rename from ..." or "--- filename"), but the payload part (i.e. " " followed by context, "-" followed by removed and "+" followed by added). Removal of CR by "am -> mailsplit -> mailinfo -> apply" callchain is not losing any information, as the input does not have useful information to let us answer "are the lines in this path supposed to end with CRLF?" in the first place; "/dev/null\r" patch is barking up a wrong tree.

Our line-endings infrastructure (e.g. core.autocrlf configuration variable, `eol` attribute) is all geared toward supporting cross platform projects in that the BCP is to use LF line endings as the canonical line endings for in-repository data and convert it to CRLF for working tree files when necessary. We do not have direct support (other than declaring contents for paths as "binary" aka "no conversion") to use CRLF in in-repository data (and that is partly deliberate).

But it is conceivable to enhance the attribute system to allow us to mark certain paths (or "all paths", which is a trivial special case) as using CRLF line endings in in-repository and in-working-tree. It could be setting `eol` attribute to `both-crlf` or something.

Then "am -> mailsplit -> mailinfo -> apply" chain could:

 * "mailsplit" and "mailinfo" does not have to do anything special,
   other than stripping CR and make sure "apply" only sees LF
   endings;

 * "apply" is taught a new option "--fix-mta-corruption" which
   causes it to pay attention to the `eol` attribute set to
   `both-crlf`, and when it reads a patch

	diff --git a/one b/one
        --- one
        +++ one
        @@ -4,6 +4,6 @@
         common1
	 common2
        -old1
        -old2
        +new1
        +new2
         common3
         common4

   and notices that path "one" is marked as such, it pretends as if
   the input were

	diff --git a/one b/one
        --- one
        +++ one
        @@ -4,6 +4,6 @@
         common1^M
	 common2^M
        -old1^M
        -old2^M
        +new1^M
        +new2^M
         common3^M
         common4^M

   to compensate for possible breakage during the mail transit.

 * "am" is taught to pass "--fix-mta-corruption" to "apply" perhaps
   by default.

Because code paths that internally run "git am", e.g. "git rebase", work on data that is *not* corrupt by mta (we generate diff and apply ourselves), these places need to tell "am" not to use the "--fix-mta-corruption" when running "apply".

On 09/23/14 23:35, Junio C Hamano wrote:

That's good to know, thanks!

I can agree with this, yes.

OK.

I see. The edk2 "mirror-of-svn" git repo is then somewhat "incompatible" with the original design.

Thank you for taking the time to describe this. It does look like the by-the-book solution.

Obviously, I can't implement it myself -- first, I have no experience with the git codebase, second, I have no time nor mandate to get familiar with it and to make a serious effort to improve it in this direction. (IOW "git" is a tool for my job, not my job.) The one-liner patch and this discussion is all I can manage -- I thought I'd take a stab at it myself rather than just "complain".

If someone finds the time to implement and document this feature, a small part of the community will be very grateful. (Not much of a compensation for a corner case like this, admittedly.)

Thanks, Laszlo

Laszlo Ersek <lersek@redhat.com> writes:

Oh, I wasn't expecting that anyway ;-).

The reason I outlined a possible approach was to ask you to sanity check the envisioned outcome (i.e. "if somebody made effort to implement it, would the end result help your workflow?") and to give hints to contributors who are looking for things to work on ;-)

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

This is a tangent, but if the problem were slightly different, I would be more sympathetic. For example

    A popular MUA, when asked to write out a message in the mbox
    format, ends _all_ the lines in its output with CRLF, whether
    the original was sent as LF-only or CRLF, and there is no way to
    convince it to use LF-only.  "git apply" fails to grok such an
    input.

could be, if the use of such an MUA is very prevalent, a common problem worth working around.

But then I would suspect that the workaround for such a case may not be "accept /dev/null\n and /dev/null\r\n equally". It is likely that the right workaround for such a case would be to "turn all \r\n into \n before processing".