Page MenuHomePhabricator
Create Task
Maniphest T12702

Unable to apply raw diff when files are moved
Open, Needs TriagePublic
Actions

Description

Version information:

phabricator 644529ab98061ce6ed3acf14a96001398290ff3a (Mon, Apr 24)
arcanist 40f7d68f75ea9e062f4018b00e1e012cdfb13c9d (Sat, Apr 22)
phutil 7c7b02216f335adfdf48cf7ec28e21776af8a3c2 (Fri, Apr 14)

This diff was generated with the command git format-patch -U999999 HEAD~1 and manually uploaded to our Phabricator instance. Arc was not used. When viewing the unified or side by side diff, everything appears correct. 3 files are correctly identified as having been moved.

However, when I click Download Raw Diff, the resulting diff does include any instructions to delete the old copy of the file and apply the new copy of the file. For example, one of the moved files was moved from lld/unittests/Core/ParallelTest.cpp to llvm/unittests/Support/ParallelTest.cpp, and then a few minor fixups were applied in the destination. The *only* commands in the raw diff related to this file are as follows:

Index: llvm/unittests/Support/ParallelTest.cpp
===================================================================
--- llvm/unittests/Support/ParallelTest.cpp
+++ llvm/unittests/Support/ParallelTest.cpp
@@ -1,4 +1,4 @@
-//===- lld/unittest/ParallelTest.cpp --------------------------------------===//
+//===- llvm/unittest/Support/ParallelTest.cpp -----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
@@ -12,21 +12,23 @@
 ///
 //===----------------------------------------------------------------------===//

There is nothing in the diff to actually put the entire contents of the file there in the first place, and as such the resulting diff does not apply.

Related Objects
Search...

Event Timeline

zjturner created this task.May 10 2017, 4:46 PM
zjturner added a subscriber: ioeric.
zjturner added a comment.May 10 2017, 4:53 PM

Note that a workaround is to do the following:

git mv lld/unittests/Core/ParallelTest.cpp llvm/unittests/Support/ParallelTest.cpp
git mv lld/include/lld/Core/Parallel.h llvm/include/llvm/Support/Parallel.h
git mv lld/lib/Core/TaskGroup.cpp llvm/lib/Support/Parallel.cpp

git apply -p0 D33024.diff.txt

In other words, as long as we attempt to apply the patch on top of files that have already been manually moved, the patch applies.

zjturner mentioned this in Z1336: General Chat.May 10 2017, 7:52 PM
avivey added a subscriber: avivey.May 10 2017, 7:52 PM

Which version of git was used? Can you upload the original patch file used to create the diff?
Which version of git is the server using?

Also, your phabricator version was modified locally.

zjturner added a comment.May 10 2017, 8:13 PM

This is rather strange. I don't have the original patch used to create the revision anymore, but I tried it again using the same command I always use to generate patches, and when I upload this new patch, I do not see the same issue. In the first (broken) one, some of the files are marked with V, but in the new (working) one, none of the files are marked as V. This is even though git status shows that they are renames:

D:\src\llvm-mono>git status
On branch Parallel
Your branch is up-to-date with 'origin/master'.
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

        modified:   lld/COFF/ICF.cpp
        modified:   lld/COFF/MapFile.cpp
        modified:   lld/COFF/Writer.cpp
        modified:   lld/ELF/Threads.h
        deleted:    lld/include/lld/Core/TaskGroup.h
        modified:   lld/lib/Core/CMakeLists.txt
        modified:   lld/lib/ReaderWriter/MachO/LayoutPass.cpp
        modified:   lld/unittests/CMakeLists.txt
        deleted:    lld/unittests/CoreTests/CMakeLists.txt
        renamed:    lld/include/lld/Core/Parallel.h -> llvm/include/llvm/Support/Parallel.h
        modified:   llvm/lib/Support/CMakeLists.txt
        renamed:    lld/lib/Core/TaskGroup.cpp -> llvm/lib/Support/Parallel.cpp
        modified:   llvm/unittests/Support/CMakeLists.txt
        renamed:    lld/unittests/CoreTests/ParallelTest.cpp -> llvm/unittests/Support/ParallelTest.cpp

The only thing I can think of is that I generated the original diff from my home computer instead of my work computer. I can test that out tonight. At work, where I currently cannot reproduce the problem, I'm using git version 2.7.4.windows.1

epriestley added a parent task: T12664: Update diff/patch parsing to extract more metadata and parse a wider range of formats.May 12 2017, 5:37 PM
epriestley added a subscriber: epriestley.

(We probably won't look at this until T12664 moves forward, but when it does it would definitely be helpful to have a specific set of steps we can take to reproduce the issue locally in a clean repository.)

zjturner added a comment.May 12 2017, 5:47 PM

Sorry, I've been slacking on getting you this information. Been swamped at
work. I'll try to get it to you this weekend.

epriestley added a comment.May 12 2017, 5:48 PM

Oh, no problem -- we likely won't look into this for a while anyway (T12664 is a big mess of related issues that we'll probably try to fix all at once).

zjturner added a comment.May 14 2017, 1:37 AM

0001-Move-lld-Parallel-algorithms-to-llvm.patch89 KBDownload

The original patch which causes this problem is attached. It was generated using git version 2.10.1.windows.1. git am 0001-Move-lld-Parallel-algorithms-to-llvm.patch will successfully apply the patch, but after uploading it to phabricator and then redownloading the raw diff, it no longer applies. The original patch contains text like this:

diff --git a/lld/unittests/CoreTests/ParallelTest.cpp b/llvm/unittests/Support/ParallelTest.cpp
similarity index 82%
rename from lld/unittests/CoreTests/ParallelTest.cpp
rename to llvm/unittests/Support/ParallelTest.cpp
index 601a2b0..f381631 100644
--- a/lld/unittests/CoreTests/ParallelTest.cpp
+++ b/llvm/unittests/Support/ParallelTest.cpp
@@ -1,46 +1,48 @@
-//===- lld/unittest/ParallelTest.cpp --------------------------------------===//
+//===- llvm/unittest/Support/ParallelTest.cpp -----------------------------===//

But when I download it from Phabricator, it doesn't have the rename lines, it just looks like this:

Index: llvm/unittests/Support/ParallelTest.cpp
===================================================================
--- llvm/unittests/Support/ParallelTest.cpp
+++ llvm/unittests/Support/ParallelTest.cpp
@@ -1,4 +1,4 @@
-//===- lld/unittest/ParallelTest.cpp --------------------------------------===//
+//===- llvm/unittest/Support/ParallelTest.cpp -----------------------------===//
 //
 //                     The LLVM Compiler Infrastructure

So obviously this isn't going to work. A phabricator review that will allow you to download the "broken" diff can be found here

zjturner added a comment.May 14 2017, 1:39 AM

BTW, the original patch was generated using git format-patch, not git diff

• GoogleLegacy added a subscriber: • GoogleLegacy.Feb 19 2018, 4:36 AM
Phabricator · Built by Phacility