The sound distributed version control system

#559 incorrect change when moving file back to previous name

Closed on December 4, 2021
rohan on October 29, 2021

When a git repo has a file which was moved then moved back again in a later commit then Pijul is recording that as an undeletion, even though the loggings shows it as a move.

INFO Loading Git history…
INFO Importing commit 37f4e72b137973855eb3c9f7ede38ded8ae1b13d: readme

INFO mv "readme" "READ_ME"
INFO Importing commit 1b93d7bc71284fb96d21877ff8b94439ec019d96: READ_ME

INFO mv "READ_ME" "ReadMe"
INFO Importing commit aeab9a49035ceaf16e1cab3f9922a5e080a447e9: ReadMe

INFO mv "ReadMe" "readme"
INFO Importing commit aa45cb97f8dce3b9942d2b85672f1690abda2f20: readme again

Initial move changes:

1. Moved: "readme" "READ_ME" 1.0
BF:BFD 1.0 -> 2.16:46/2
up 1.0, down 2.1

and

1. Moved: "READ_ME" "ReadMe" 1.0
BF:BFD 1.0 -> 2.0:31/2
up 1.0, down 3.1

but then

1. File un-deletion: "readme" 1.0 "UTF-8"
BFD:BF 1.0 -> 3.16:46/4, BF:BF 2.30 -> 3.1:1/2, BF:BF 3.46 -> 3.1:1/3, BF:BF 2.30 -> 3.1:1/2, BF:BF 4.31 -> 3.1:1/4, BF:BF 3.46 -> 3.1:1/3, BF:BFD 1.0 -> 2.0:30/2

The sample git repo is https://drive.google.com/file/d/1o8DwtryDeWuPwGV8y-QJsVkgIBj38aEj/view?usp=sharing

rohan on November 4, 2021

Plain pijul commands have the same issue.

pijul init
echo 'Hello Pijul!' > readme
pijul record --all --message readme
pijul mv readme READ_ME
pijul record --all --message READ_ME
pijul mv READ_ME readme
pijul record --all --message 'readme again'
rohan on November 5, 2021

Looking at the logs it’s correctly determining that there’s a file move via parent but also that it’s resurrecting the file though a different parent.

This loop in collect_moved_edges needs something to prioritise the move

for parent in iter_adjacent(
        txn,
        channel,
        current_pos.inode_vertex(),
        EdgeFlags::FOLDER | EdgeFlags::PARENT,
        EdgeFlags::all(),
    )

Log with both move and resurrect

libpijul::record] record_moved_file RecordItem { v_papa: Position { change: Some(ChangeId(AAAAAAAAAAAAA)), pos: ChangePosition(L64(0)) }, papa: Inode(AAAAAAAAAAAAA), inode: Inode(AT4G5WYZ2U5JA), basename: "readme", full_path: "readme", metadata: InodeMetadata(0) }
libpijul::record] collect_moved_edges Position { change: ChangeId(MVNNDZX6BPVP4), pos: ChangePosition(L64(1)) }
libpijul::record] parent = E(BLOCK | FOLDER | PARENT, T2LKWRJ7XXHDI[31], T2LKWRJ7XXHDI)
libpijul::pristine::sanakirja] find_block_end, loop, k = V(ChangeId(MVNNDZX6BPVP4)[1:1]), p = Position { change: ChangeId(T2LKWRJ7XXHDI), pos: ChangePosition(L64(31)) }
libpijul::pristine::sanakirja] find_block_end, loop, k = V(ChangeId(T2LKWRJ7XXHDI)[0:31]), p = Position { change: ChangeId(T2LKWRJ7XXHDI), pos: ChangePosition(L64(31)) }
libpijul::pristine::sanakirja] find_block_end, V(ChangeId(T2LKWRJ7XXHDI)[0:31]) Position { change: ChangeId(T2LKWRJ7XXHDI), pos: ChangePosition(L64(31)) }
libpijul::pristine::sanakirja] find_block_end, V(ChangeId(T2LKWRJ7XXHDI)[0:31]) Position { change: ChangeId(T2LKWRJ7XXHDI), pos: ChangePosition(L64(31)) }
libpijul::changestore::filesystem] get_contents V(ChangeId(T2LKWRJ7XXHDI)[0:31])
libpijul::changestore::filesystem] get_contents 31
libpijul::record] parent_dest V(ChangeId(T2LKWRJ7XXHDI)[0:31]) InodeMetadata(0) "READ_ME"
libpijul::record] new_meta = InodeMetadata(0), parent_meta = InodeMetadata(0)
libpijul::record] grandparent: E(BLOCK | FOLDER | PARENT, AAAAAAAAAAAAA[0], T2LKWRJ7XXHDI)
libpijul::record] grandparent_dest V(ChangeId(AAAAAAAAAAAAA)[0:0]) Ok("\u{7}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}READ_ME\u{1}\u{5}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}UTF-8")
libpijul::record] change = false true false     <========= move detected
libpijul::record] parent = E(BLOCK | FOLDER | PARENT, MVNNDZX6BPVP4[46], MVNNDZX6BPVP4)
libpijul::pristine::sanakirja] find_block_end, loop, k = V(ChangeId(MVNNDZX6BPVP4)[16:46]), p = Position { change: ChangeId(MVNNDZX6BPVP4), pos: ChangePosition(L64(46)) }
libpijul::pristine::sanakirja] find_block_end, V(ChangeId(MVNNDZX6BPVP4)[16:46]) Position { change: ChangeId(MVNNDZX6BPVP4), pos: ChangePosition(L64(46)) }
libpijul::pristine::sanakirja] find_block_end, V(ChangeId(MVNNDZX6BPVP4)[16:46]) Position { change: ChangeId(MVNNDZX6BPVP4), pos: ChangePosition(L64(46)) }
libpijul::changestore::filesystem] get_contents V(ChangeId(MVNNDZX6BPVP4)[16:46])
libpijul::changestore::filesystem] get_contents 30
libpijul::record] parent_dest V(ChangeId(MVNNDZX6BPVP4)[16:46]) InodeMetadata(0) "readme"
libpijul::record] new_meta = InodeMetadata(0), parent_meta = InodeMetadata(0)
libpijul::record] grandparent: E(BLOCK | FOLDER | PARENT | DELETED, AAAAAAAAAAAAA[0], T2LKWRJ7XXHDI)
libpijul::record] grandparent_dest V(ChangeId(AAAAAAAAAAAAA)[0:0]) Ok("\u{6}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}readme\u{1}\u{5}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}\u{0}UTF-8")
libpijul::record] change = false false false   <======= move not detected
rohan added a change on November 12, 2021
PR32ALRGHSEWKZWV4GAYOTJZNGBKYMHKH7X6RWLLBFRQUHYFQWWAC
main
pmeunier on November 27, 2021

Hi! thanks for the patch. However, due to a bug in the Nest, it seems I’ve lost it. Would you mind pushing it again to this discussion? Sorry about that.

rohan on November 28, 2021

Sure. I’ve hit a related issue where some moves were not applied resulting in a “Path deletion conflict”. I’ve not yet checked whether that’s due to this change.

rohan on November 30, 2021

Confirmed that there is an issue with Pijul. Without this change there is no deletion conflict but the same files are now in both the correct and original place and the original ones are no longer part of the recorded repo. I’ll try and create a minimal reproduction.

pmeunier added a change on December 2, 2021
VXFXV55PVPRLLMTV5LR3WUVFSMC3KK3HD5GCU25DYSBGPZBWV5PQC
pmeunier added a change on December 2, 2021
TELBT3CINIPFLPR2NAW75U3T76CU5LEC2NHTXTDAM64ZKNHRVSKQC
main
pmeunier on December 2, 2021

A path deletion conflict should be triggered by a file move or edit, done in parallel with a deletion, either in that file or in a directory above.

What do you think of the patch I just pushed, avoiding two loops? Is there any reason why we shouldn’t refactor these loops?

pmeunier on December 2, 2021

(also, thanks again for re-pushing)

rohan on December 2, 2021

The directory the file was moved from was deleted but I wouldn’t have expected a conflict as it was done in the same change as the move.

The reason I did it in two loops was I couldn’t tell from the iterator code whether the rename would always be found before the un-deletion. That was the ordering in my single file moved tests but I haven’t tried more complex scenarios.

rohan on December 3, 2021

Haven’t been able to replicate the directory deletion + file move self-conflict but I have found that deleting multiple directory levels displays incorrectly in the diff even if the change appears to work correctly. Given a directory hierarchy a/b/c when removing that entirely the recorded change shows that the top level a was deleted multiple times and the others aren’t mentioned.

pmeunier added a change on December 4, 2021
SMMRGKCXLDFNI7RAACOTGRGUPZLWY5MKVBL6WB2ZPEUCKDQ4PLIAC
main
pmeunier on December 4, 2021

The little descriptions in the patch format are just metadata allowing users to make sense of the patches. The actual work is done by the actual graph changes. I’ll close this since it is fixed on our test cases (I tried as well).

pmeunier closed this discussion on December 4, 2021