Skip to content

[tree] fix duplicated anchor in filename#18852

Merged
vepadulano merged 3 commits intoroot-project:masterfrom
ferdymercury:patch-4
Oct 23, 2025
Merged

[tree] fix duplicated anchor in filename#18852
vepadulano merged 3 commits intoroot-project:masterfrom
ferdymercury:patch-4

Conversation

@ferdymercury
Copy link
Collaborator

This Pull request:

Changes or fixes:

Fixes https://its.cern.ch/jira/browse/ROOT-5002

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@github-actions
Copy link

github-actions bot commented May 23, 2025

Test Results

    21 files      21 suites   3d 13h 31m 38s ⏱️
 3 691 tests  3 691 ✅ 0 💤 0 ❌
75 903 runs  75 903 ✅ 0 💤 0 ❌

Results for commit 45b62ea.

♻️ This comment has been updated with latest results.

@vepadulano
Copy link
Member

Apologies, I'm trying to understand better what this PR is solving. I can't see from the original issue or from the new reproducer what was the previously broken situation, e.g. what was the file name? In the original issue I read

The method TEntryList::GetEntryList called inside TChain::SetEntryList doesn't find any match between with the TChain 

Does the match happen via the file name?

@ferdymercury
Copy link
Collaborator Author

ferdymercury commented Jul 30, 2025

Hi, if I recall correctly, the problem was:

chain.Add("cernstaff.zip#cernstaff.root");

which later led to in TEntryList to:

filename = tree->GetTree()->GetCurrentFile()->GetName();
TUrl url(filename.Data(), true);

url copies the "hash part" as anchor as an internal variable of the class.

but then, we also call:

url.SetFile(filename);

where filename still contains the hash part.

so we ended up with

filename = url.GetUrl();

which appends the previous filename with the internal anchor, so we end up having two times the hash part.

so cernstaff.zip#filename.root#filename.root

Which means that then, the TEntryList does find the "filename-filename" file inside that zip container.

@vepadulano vepadulano added the clean build Ask CI to do non-incremental build on PR label Jul 30, 2025
@vepadulano vepadulano closed this Jul 30, 2025
@vepadulano vepadulano reopened this Jul 30, 2025
@ferdymercury ferdymercury requested a review from vepadulano August 1, 2025 13:41
@dpiparo dpiparo closed this Aug 24, 2025
@dpiparo dpiparo reopened this Aug 24, 2025
@dpiparo
Copy link
Member

dpiparo commented Aug 24, 2025

revamping this nice PR.

Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

LGTM, the CI failures look unrelated but I'll wait for another rerun to be sure

@ferdymercury ferdymercury requested a review from pcanal October 2, 2025 10:10
@ferdymercury ferdymercury added pr:squash on merge and removed clean build Ask CI to do non-incremental build on PR labels Oct 3, 2025
@ferdymercury ferdymercury added this to the 6.38.00 milestone Oct 6, 2025
@vepadulano vepadulano closed this Oct 22, 2025
@vepadulano vepadulano reopened this Oct 22, 2025
Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

Clean and simple, nice! I will wait for another CI run before merging.

@ferdymercury
Copy link
Collaborator Author

@vepadulano one Windows still failing but I guess it's unrelated.

@vepadulano vepadulano merged commit dc0b783 into root-project:master Oct 23, 2025
52 of 55 checks passed
@ferdymercury ferdymercury deleted the patch-4 branch October 23, 2025 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants