1627405 - moz-phab deleted a file in the patch it uploaded (incorrect move/copy detection?)

Status: RESOLVED FIXED

(Conduit :: moz-phab, defect, P1)

Description

[Kartikaya Gupta (email:[e-mail])]

This is a bit of a weird one. I have a git/cinnabar repo, with my current branch pointing to https://github.com/staktrace/gecko/commits/dynamicsticky3. Note how the topmost commit modifies the dynamic-toolbar-fixed-bottom-1.html file along with adding a bunch of very similar files.

Now look at the corresponding Phabricator revision and see how it considers that dynamic-toolbar-fixed-bottom-1.html file "Deleted after being copied to multiple locations".

If my local git repo has the file, and phabricator thinks the file is deleted, the fault must lie in the tool that transferred the patch from my local repo to phabricator, i.e. moz-phab.

Interestingly, if you look at the history of that phabricator revision, the first version uploaded seems fine. The second one and third ones, however, have the file deletion. This might either be due to changes that I introduced in the second revision, or might be because the modified version were uploaded via moz-phab submit -s HEAD to just upload the single revision. Either way, it seems like whatever moz-phab is doing to generate the patch for upload isn't properly handling detection of file copies/moves; what should have been picked up as a copy got picked up as a move instead.

I'm about to try reuploading the patch both as a single revision, and if that doesn't work, as a full series to try and get the patchset relanded.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #0)

I'm about to try reuploading the patch both as a single revision, and if that doesn't work, as a full series to try and get the patchset relanded.

Neither of these worked, it still thinks the file is deleted. So I guess it's probably a bug in the move/copy detection in moz-phab.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

I ported the patch queue over to a mercurial clone of m-c and reuploaded that last patch, which seems to have worked. At least the view in Phabricator now longer talks about deleted files, and it looks sane. Sent it to autoland, hopefully it won't break again.

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

I bump into this bug in https://phabricator.services.mozilla.com/D72051. The file layout/reftests/pagination/table-cell-breaking-1-ref.html is marked as Deleted after being copied to multiple locations. I uploaded via moz-phab from a hg repo, and I did check locally the patch is OK.

Comment 4

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #2)

I ported the patch queue over to a mercurial clone of m-c and reuploaded that last patch, which seems to have worked. At least the view in Phabricator now longer talks about deleted files, and it looks sane. Sent it to autoland, hopefully it won't break again.

I suspect you just go lucky because it was fresh maybe? I ran into this in my pure hg workflow: bug 1631315.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1627405 (not an actual fix) part 1: add a testing text file.

A subsequent commit will make copies of this file.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1627405 (not an actual fix) part 2: Make an edited copy of the original test file.

This commit was made by doing:
hg cp test-file.txt test-file-2.txt
...and then making some edits to test-file-2.txt, and then committing.

Depends on D73531

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 1627405 (not an actual fix) part 3: Make two edited copies of the original test file.

This commit was made by doing:
hg cp test-file.txt test-file-3.txt
hg cp test-file.txt test-file-4.txt
...and then making some edits to test-file-3.txt (and -4.txt),
and then committing.

Depends on D73532

Comment 9

[Daniel Holbert [:dholbert]]

I've just attached a patch-stack as a "testcase" here (not as a fix, but as a testcase to help with reproducing/demonstrating the bug).

STR:

1. Import the testcase by doing moz-phab patch D73533
2. Update to your new tip (the 'part 3' commit, 'Make two edited copies of the original test file.')
3. Look for test-file.txt (e.g. ls *.txt or cat test-file.txt)
   ...or, if you prefer, hg export your tipmost commit (or look at https://d3kxowhw4s8amj.cloudfront.net/file/data/lk4xjt73ohphe2uaciny/PHID-FILE-itpehrfqbqe62hpzbt73/D73533.diff ) and see what the fate of the original test file was.

ACTUAL RESULTS:

• The file is missing.
• In the hg export / above-linked diff view of the tipmost commit, you can see that one of the copy operations somehow became a rename operation:

diff --git a/test-file.txt b/test-file-3.txt
rename from test-file.txt
rename to test-file-3.txt
--- a/test-file.txt
+++ b/test-file-3.txt
[...SNIP...]
diff --git a/test-file.txt b/test-file-4.txt
copy from test-file.txt
copy to test-file-4.txt
--- a/test-file.txt
+++ b/test-file-4.txt

EXPECTED RESULTS:
The file should have been copied, not renamed. I only did hg cp operations when generating this patch stack -- no hg mv/rename operations.

Note: I'm using MozPhab (0.1.85), which seems to be the latest available.

Comment 10

[Daniel Holbert [:dholbert]]

(The commit messages for each patch in my patch stack show the operations that i did to generate the patch, BTW.)

And I guess my patch-stack "testcase" is really more a "demonstration of results" than a testcase, since at this point the phabricator-hosted "part 3" is already horked, given that the "raw diff" view ( https://d3kxowhw4s8amj.cloudfront.net/file/data/lk4xjt73ohphe2uaciny/PHID-FILE-itpehrfqbqe62hpzbt73/D73533.diff ) already shows rename to... as noted above

Comment 11

[Daniel Holbert [:dholbert]]

also worth noting: part 2 here (making a single copy) isn't strictly necessary to reproduce the bug -- it's just there to demonstrate that we can successfully do a single copy operation just fine. It only seems to be copying from a single file to multiple targets (as in part 3) that's horked, as noted above. I suppose part 3 does show Deleted after being copied to multiple locations, too, which is an error that was mentioned in comment 0 here.

Comment 12

[Piotr Zalewa [:zalun]]

To replicate in a Suite environment:

hg cp README README1
hg cp README README2
moz-phab -b 1

In Phabricator in section File we will see:

P			README
Deleted after being copied to multiple locations:
README1
README2
	
P			README1
Copied from README
	
P			README2
Copied from README

Comment 13

[Piotr Zalewa [:zalun]]

Attachment: Bug 1627405 - Fix deletion of the source when copying to multiple targets. r=zeid

MULTICOPY change kind is treated as "moved to multiple locations".
The fix is to change this to "COPY_AWAY".