Skip to content

[IO] Fix rootcp --replace flag not working with recursive copy #20344

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ahmedbilal9 wants to merge 9 commits into
base: master
Choose a base branch
from
+58 −15
Open

[IO] Fix rootcp --replace flag not working with recursive copy #20344

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
97e1291
[IO] Fix rootcp --replace flag not working with recursive copy
Nov 24, 2025
f387a14
Fix ruff formatting: remove trailing whitespace
Nov 25, 2025
d432681
Add test for rootcp --replace with recursive copy
Dec 1, 2025
029948a
Fix CMakeLists.txt syntax error: remove duplicate FIXTURES_SETUP line
Dec 18, 2025
9d29b5b
Added missing SimpleRootmv1PrepareInput fixture and ROOTMV TESTS sect…
Dec 19, 2025
6347c48
Update roottest/main/CMakeLists.txt
ahmedbilal9 Dec 22, 2025
a65caee
Removed duplicate SimpleRootmv1PrepareInput test and section header
Dec 22, 2025
bdb8bc5
Fix typos in test section headers: ROOLS->ROOTLS, ROORM->ROOTRM, ROOM…
Jan 7, 2026
aa8c090
Add clarifying comment for obj.Delete() call
Jan 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions main/python/cmdLineUtils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,19 +692,18 @@ def copyRootObjectRecursive(sourceFile, sourcePathSplit, destFile, destPathSplit
newT.Write()
else:
obj = key.ReadObj()
if replaceOption and isExisting(destFile, destPathSplit + [setName]):
changeDirectory(destFile, destPathSplit)
otherObj = getFromDirectory(setName)
retcodeTemp = deleteObject(destFile, destPathSplit + [setName])
# Determine the actual name that will be written
actualName = setName if setName != "" else objectName

# Delete existing object if replace option is enabled
if replaceOption and isExisting(destFile, destPathSplit + [actualName]):
retcodeTemp = deleteObject(destFile, destPathSplit + [actualName])
if retcodeTemp:
retcode += retcodeTemp
obj.Delete()
Copy link
Member

@vepadulano vepadulano Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something not super clear to me: we call deleteObject, then we also call obj.Delete conditionally, why the apparently double delete?

Copy link
Contributor Author

@ahmedbilal9 ahmedbilal9 Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleteObject() returns 0 on success and non-zero on failure. When deletion fails (retcodeTemp != 0), we track the error by adding to retcode, then call obj.Delete() to clean up the ROOT object in memory that we read earlier, and skip writing since we cannot replace an object that failed to delete. The obj.Delete() is for memory cleanup, not related to the file deletion.

continue
else:
if isinstance(obj, ROOT.TNamed):
obj.SetName(setName)
changeDirectory(destFile, destPathSplit)
obj.Write()
elif issubclass(obj.__class__, ROOT.TCollection):

if issubclass(obj.__class__, ROOT.TCollection):
# probably the object was written with kSingleKey
changeDirectory(destFile, destPathSplit)
obj.Write(setName, ROOT.TObject.kSingleKey)
Expand Down
25 changes: 20 additions & 5 deletions roottest/main/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,12 +368,27 @@ ROOTTEST_ADD_TEST(SimpleRootcp5CheckOutput
ROOTTEST_ADD_TEST(SimpleRootcp5Clean
COMMAND ${PY_TOOLS_PREFIX}/rootrm${pyext} -r copy5.root
FIXTURES_REQUIRED main-SimpleRootcp5CheckOutput-fixture)
#########################################################################

############################# ROOMV TESTS ############################
ROOTTEST_ADD_TEST(SimpleRootmv1PrepareInput
COMMAND ${PY_TOOLS_PREFIX}/rootcp${pyext} --recreate -r test.root move1.root
FIXTURES_SETUP main-SimpleRootmv1PrepareInput-fixture)
Comment on lines 376 to 381
Copy link
Collaborator

@ferdymercury ferdymercury Dec 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: was there a reason to remove this?
Was this Copilot again?


ROOTTEST_ADD_TEST(RootcpReplaceRecursivePrepareInput
COMMAND ${PY_TOOLS_PREFIX}/rootcp${pyext} --recreate -r test.root copy_replace_recursive.root
FIXTURES_SETUP main-RootcpReplaceRecursivePrepareInput-fixture)

ROOTTEST_ADD_TEST(RootcpReplaceRecursive
COMMAND ${PY_TOOLS_PREFIX}/rootcp${pyext} --replace -r copy_replace_recursive.root:tof copy_replace_recursive.root:tof
FIXTURES_REQUIRED main-RootcpReplaceRecursivePrepareInput-fixture
Comment on lines +377 to +379
Copy link
Member

@vepadulano vepadulano Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this even needed?

Copy link
Contributor Author

@ahmedbilal9 ahmedbilal9 Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test verifies that the --replace flag works correctly for in-place recursive copies. It copies from copy_replace_recursive.root:tof to the same location to ensure objects are replaced without creating new cycles (;2, ;3, etc.). The RootcpReplaceRecursiveCheckOutput test then verifies only cycle ;1 exists for each object, confirming replacement worked as intended.

FIXTURES_SETUP main-RootcpReplaceRecursive-fixture)

ROOTTEST_ADD_TEST(RootcpReplaceRecursiveCheckOutput
COMMAND ${TOOLS_PREFIX}/rootls${exeext} -1 copy_replace_recursive.root:tof
OUTREF RootcpReplaceRecursive.ref
FIXTURES_REQUIRED main-RootcpReplaceRecursive-fixture
FIXTURES_SETUP main-RootcpReplaceRecursiveCheckOutput-fixture
ENVIRONMENT ${test_env})

ROOTTEST_ADD_TEST(RootcpReplaceRecursiveClean
COMMAND ${PY_TOOLS_PREFIX}/rootrm${pyext} -r copy_replace_recursive.root
FIXTURES_REQUIRED main-RootcpReplaceRecursiveCheckOutput-fixture)

ROOTTEST_ADD_TEST(SimpleRootmv1
COMMAND ${PY_TOOLS_PREFIX}/rootmv${pyext} move1.root:hpx move1.root:histo
Expand Down
22 changes: 22 additions & 0 deletions roottest/main/RootcpReplaceRecursive.ref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
h0_0N
h0_1N
h0_2N
h0_3N
h0_4N
h0_5N
h0_6N
h0_7N
h0_8N
h0_9N
h1_0N
h1_1N
h1_2N
h1_3N
h1_4N
h1_5N
h1_6N
h1_7N
h1_8N
h1_9N
plane0
plane1