Skip to content

Fix off-by-one error for merging nested directories - #23878

Merged
dpiparo merged 3 commits into
root-project:masterfrom
enirolf:23666-hadd-nested-dirs
Oct 28, 2026
Merged

dpiparo merged 3 commits into
root-project:masterfrom
enirolf:23666-hadd-nested-dirs

Conversation

@enirolf

@enirolf enirolf commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

The problem was observed when calling hadd with the -n flag, or equivalently setting TFileMerger::SetMaxOpenedFiles to a number smaller than the number of files to merge, as well as when the number of files to merge was larger than 127.

The underlying issue is that instead of comparing the file of the source directory to the file of the target directory, we were be comparing it to the target directory itself.

The initial PR (#23791) pointed us in the right direction, but still required non-insignificant time to properly understand and validate the changes.

Fixes #23666.

Instead of comparing the *file* of the source directory to the target
directory, we should be comparing it to the *file* of the target
directory.
@enirolf enirolf self-assigned this Oct 28, 2026
@enirolf enirolf added bug in:I/O experiment Affects an experiment / reported by its software & computimng experts labels Oct 28, 2026
@enirolf
enirolf requested review from jblomer, pcanal and vepadulano and removed request for bellenot, dpiparo, jblomer and pcanal October 28, 2026 14:34
@enirolf enirolf added this to the 6.42.00-rc1 milestone Oct 28, 2026

@pcanal pcanal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks.

Comment thread io/io/test/TFileMergerTests.cxx Resolved
enirolf and others added 2 commits October 28, 2026 16:28
Regression test for root-project#23666

Co-authored-by: Vincenzo Eduardo Padulano <v.e.padulano@gmail.com>
...directly using `hadd`, and not only the TFileMerger
@enirolf
enirolf force-pushed the 23666-hadd-nested-dirs branch from b9c9cd5 to c973835 Compare October 28, 2026 15:28
@enirolf
enirolf requested a review from vepadulano October 28, 2026 15:28

@vepadulano vepadulano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@github-actions

Copy link
Copy Markdown

Test Results

    23 files      23 suites   3d 19h 47m 37s ⏱️
 3 904 tests  3 904 ✅ 0 💤 0 ❌
79 769 runs  79 769 ✅ 0 💤 0 ❌

Results for commit c973835.

@dpiparo
dpiparo merged commit df488ea into root-project:master Oct 28, 2026
35 of 36 checks passed
@enirolf
enirolf deleted the 23666-hadd-nested-dirs branch October 29, 2026 07:34
@enirolf
enirolf restored the 23666-hadd-nested-dirs branch October 29, 2026 07:49
@enirolf

enirolf commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

/backport to 6.40, 6.36

@root-project-bot

Copy link
Copy Markdown

Preparing to backport PR #23878 to branches 6.40 and 6.36 requested by enirolf

@root-project-bot

Copy link
Copy Markdown

Something went wrong with the creation of the PR to backport to 6.36: @enirolf please see the logs

@root-project-bot

Copy link
Copy Markdown

This PR has been backported to branch 6.40: #23882

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug experiment Affects an experiment / reported by its software & computimng experts in:I/O

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hadd -n omits object from last file for objects in nested TDirectories

5 participants