[tree] Delegate SetBranchAddress to friends if necessary

[vepadulano]

In the case of a TTree with a friend TChain, the call to GetEntry that needs to move to a new tree in the friend TChain was not correctly updating the branch addresses. SetBranchAddress now makes a distinction between the case of the branch belonging to the main tree versus the case of the branch belonging to one of the friends. In order for this to work, GetBranch functionality has been split in two: GetBranchFromSelf checks whether the branch name corresponds to a branch of this tree and returns it if so; GetBranchFromFriends checks whether the branch name corresponds to a branch found in one of the trees and returns it if so.

Fixes #16804

[github-actions[bot]]

Test Results

    20 files      20 suites   3d 8h 15m 43s ⏱️  3 193 tests  3 193 ✅ 0 💤 0 ❌ 62 261 runs  62 261 ✅ 0 💤 0 ❌

Results for commit 06c97dc9.

:recycle: This comment has been updated with latest results.

[lmoureaux]

This solves #16804 but not the related #16805 (my analysis there is likely outdated, but the test case remains valid).

Moreover, adding c2.Scan("index:value"); to the test case in #16805 results in a SEGV.

[vepadulano]

Dear @lmoureaux ,

Thank you for confirming this fixes the linked issue. I would not tie fixing both issues together in the same PR. Once the review process is done for this one I will merge it, then move to fixing #16805

[lmoureaux]

Thanks @vepadulano for the clarification, this sounds reasonable!

[pcanal]

Side note: this PR was arbitrarily picked to check the test coverage feature on a PR.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 66.26506% with 28 lines in your changes missing coverage. Please review.

Project coverage is 35.67%. Comparing base (a481509) to head (06c97dc). Report is 34 commits behind head on master.

Files with missing lines	Patch %	Lines
tree/tree/src/TTree.cxx	68.85%	3 Missing and 16 partials :warning:
tree/tree/src/TChain.cxx	55.00%	2 Missing and 7 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #18871      +/-   ##
==========================================
- Coverage   35.69%   35.67%   -0.02%     
==========================================
  Files        3374     3374              
  Lines      514365   514407      +42     
  Branches   241518   241532      +14     
==========================================
- Hits       183581   183505      -76     
- Misses     218816   219021     +205     
+ Partials   111968   111881      -87     

Flag	Coverage Δ
unittests	35.67% <66.26%> (-0.02%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[vepadulano]

@pcanal I have provided a solution to the problem of the spurious error message, it's contained in the new commit, let me know what you think

[pcanal]

Does the new code 'properly' warning if a branch is actually missing?

[vepadulano]

Does the new code 'properly' warning if a branch is actually missing?

Yes, the errors still appear properly within SetBranchAddress. I have added more tests to demonstrate this, see all the WrongBranchName* test cases.

~The error printout disappears, for this very specific case, from SetBranchStatus. I believe this is actually desired, as the extra output is at most redundant and in the case highlighted with this PR simply wrong. There is one roottest test named roottest-root-tree-branches-make which was actually testing for this extra printout. My current understanding is we could simply change the reference file there and remove the extra error statements, I will double check that it makes sense~

EDIT: after investigation, the issue with the roottest test was simply that the new boolean data member in TChainElement wasn't properly initialised, now that is done and the test should be working again.

[vepadulano]

The latest commit highlighted a pre-existing bug that was relying on a side-effect of the status of master before this PR. Now it is tracked at https://github.com/root-project/root/issues/19220 . Once that is fixed we can resume the work of this PR.