Skip to content

[math] add DeltaR to GenVector #18728

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

Jump to bottom
Merged
mdessole merged 11 commits into from
May 16, 2025
Merged

[math] add DeltaR to GenVector #18728

mdessole merged 11 commits into from
May 16, 2025

Conversation

@ferdymercury
Copy link
Collaborator

@ferdymercury ferdymercury commented May 14, 2025

This Pull request:

Changes or fixes:

Fixes https://root-forum.cern.ch/t/usage-of-deltar/63543

since TLorentzVector is a legacy class

Checklist:

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

@ferdymercury ferdymercury requested a review from lmoneta as a code owner May 14, 2025 18:22
@ferdymercury ferdymercury requested a review from mdessole May 14, 2025 18:23
ferdymercury
ferdymercury commented May 14, 2025
View reviewed changes
ferdymercury
ferdymercury commented May 14, 2025
View reviewed changes
ferdymercury
ferdymercury commented May 14, 2025
View reviewed changes
ferdymercury
ferdymercury commented May 14, 2025
View reviewed changes
@github-actions
Copy link

github-actions bot commented May 14, 2025 •
edited
Loading

Test Results

    19 files      19 suites   3d 21h 13m 42s ⏱️
 2 745 tests  2 745 ✅ 0 💤 0 ❌
50 721 runs  50 721 ✅ 0 💤 0 ❌

Results for commit 8fdfc75.

♻️ This comment has been updated with latest results.

@ferdymercury ferdymercury added this to the 6.38.00 milestone May 15, 2025
mdessole
mdessole requested changes May 15, 2025
View reviewed changes
@mdessole mdessole self-assigned this May 15, 2025
@mdessole mdessole self-requested a review May 15, 2025 15:05
mdessole
mdessole approved these changes May 15, 2025
View reviewed changes
Copy link
Contributor

@mdessole mdessole left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you @ferdymercury! LGTM after fixing the test.

@ferdymercury
Copy link
Collaborator Author

ferdymercury commented May 15, 2025

Thank you @ferdymercury! LGTM after fixing the test.

Thanks for the review and the fix :)

@mdessole mdessole merged commit dd272ab into root-project:master May 16, 2025
22 of 23 checks passed
@ferdymercury ferdymercury deleted the patch-2 branch May 16, 2025 06:51
pcanal added a commit to pcanal/root that referenced this pull request May 19, 2025
@pcanal
[ci] Restrict clang-format to PR changes
9a60765 
Related github discussion: https://github.com/orgs/community/discussions/59677

For a given PR, the value of `github.event.pull_request.base.sha` always
corresponds to the SHA of the head/tip of the base branch at the moment
of either creation of the PR or the last force-push (regardless of whether
that force-push actually rebased the PR branch against base or was just
modifying an existing commit in it) to the PR branch.

In addition the merge commit is updated whenever a new commit is push
onto the PR branch but this merge commit is not against the (fixed)
`github.event.pull_request.base.sha` but is instead against the (moving)
tip of the target branch.

Currently (before this commit), our clang-format action will check the
differences between that commit and the merge commit.

In practice this means that if a PR languish and one or more PR is
actually merged into the master branch then the comparison will
now involved not only the commit introduced the PR but also all
the commits added to master since the PR creation (or its last
force push).

There is 2 general solutions to replace the `github.event.pull_request.base.sha`:

1. Find and use the non PR branch parent of the merge commit
2. Find and use the common ancestor of the master and the PR

This commit implement 2nd options by using `git merge-base` with the
arguments 'tip of the PR branch' and `github.event.pull_request.base.sha`
which is guaranteed to be ahead of/have a commong ancestor with
the PR branch.

Until there is a resolution to the above mentioned github discussion
this is more complex than before but more stable.

NOTE: This matters in our case since most existing files and some
PRs are merge without passing clang-format (in part because clang-format
still gives some significantly sub-optimal result in a few important
cases)

As a concrete example, the PR root-project#18587
was at some point in a state where the merge commit was:

*   354e754 - (3 hours ago) Merge e54a1ea into 59c28d5 — Philippe Canal

The tip of master was:

* | 59c28d5 - (5 hours ago) [nfc] mention deprecated ConcatFileName (root-project#18755) — ferdymercury

but the `github.event.pull_request.base.sha` was pointing to:

* | bda0b84 - (3 days ago) [cmake] Check if cplusplus is 23 by default — ferdymercury

As seen below this meant that the clang-format stage was reporting about
a lot more than what that PR had actually changed.

*   354e754 - (3 hours ago) Merge e54a1ea into 59c28d5 — Philippe Canal
|\
| * e54a1ea - (3 hours ago) [NFC] clang-format — Philippe Canal
| * 4c9bbf3 - (4 days ago) meta: Extend ClassInfo loading test — Philippe Canal
| * 7137bf0 - (2 weeks ago) roottest: switch test from DEPENDS to FIXTURES. — Philippe Canal
| * ca68ceb - (2 weeks ago) meta: Correct TClass::LoadClassInfo. — Philippe Canal
* | 59c28d5 - (5 hours ago) [nfc] mention deprecated ConcatFileName (root-project#18755) — ferdymercury
* | 0fe2f1f - (2 days ago) [RF] Fix memory leak of ConcatFileName return value in RooWorkspace — Jonas Rembser
* | eb0b903 - (6 hours ago) core: Add semantic ROOT::EnableImplicitMT (root-project#18694) — Philippe Canal
* | c814428 - (9 hours ago) [jsroot] dev 16/05/2025 with ws private members — Sergey Linev
* | bbff7ab - (9 hours ago) [qtweb] rename unload handler — Sergey Linev
* | af9d1e4 - (10 hours ago) [webgui][ui5] use websocket.isStandalone() — Sergey Linev
* | 768f741 - (10 hours ago) [webgui][ui5] replace title by tooltip in CheckBox — Sergey Linev
* | 2056891 - (10 hours ago) [webgui][ui5] use isChannel function of ws handler — Sergey Linev
* | 600573f - (2 days ago) [df] Add test for nested data members with same name — Vincenzo Eduardo Padulano
* | c8fd528 - (5 weeks ago) [tree] Speedup retrieval of branch names — Vincenzo Eduardo Padulano
* | 98d8160 - (33 hours ago) [Tutorials] Add Python dependencies for df101 and rf617. — Stephan Hageboeck
* | dd272ab - (15 hours ago) [math] add DeltaR to GenVector (root-project#18728) — ferdymercury
* | ddd8512 - (13 days ago) [test] Add test case — ferdymercury
* | ea465be - (2 weeks ago) [tree,hist] Fix parsing bugs in TTreeFormula and TFormula_v5 — ferdymercury
* | bda0b84 - (3 days ago) [cmake] Check if cplusplus is 23 by default — ferdymercury
* | 3049b5e - (2 weeks ago) [metacling] copy-paste from RTypes ClassDefBase into interpreted version — ferdymercury
* | aac51aa - (2 days ago) [MC] Correctly report stability of particles — Danilo Piparo
* | 6d6be1a - (2 days ago) [webgui] adjust stressGraphics_web.ref file — Sergey Linev
* | a198988 - (2 days ago) [jsroot] dev 15/05/2025 without RHist classes — Sergey Linev
* | b65a5d6 - (2 days ago) [roottest] fix leak by using TSystem::ConcatFileName — Sergey Linev
* | 470e732 - (2 days ago) [math] fix harmless initialization bug in halfsamplemode algorithm — ferdymercury
* | be3d7c1 - (2 days ago) [Math][GenVector] Substitute `M_PI*` with `TMath` function calls (root-project#18659) — Monica Dessole (origin/master, origin/HEAD)
* | 9a5f6ca - (7 days ago) [RF][HF] Don't create output directory if not needed — Jonas Rembser
* | a4a0933 - (7 days ago) [RF][HF] Add configuration parameter to disable workspace writing — Jonas Rembser
* | 6d570f6 - (7 days ago) [RF][HF] Make sure that RooMsgService is correctly reset — Jonas Rembser
* | 42a1c56 - (10 days ago) [core] Add tests for TClass lookup of STL containers. — Stephan Hageboeck
* | 9fe1956 - (10 days ago) [core] Fix a TClass lookup for STL containers. — Stephan Hageboeck
* | be95a45 - (4 days ago) [NFC] Move functions to anonymous namespace/clang-format TClassEdit. — Stephan Hageboeck
* | d0e79a5 - (2 days ago) [math] Add HalfSampleMode calculation to TMath — ferdymercury
* | ca36f75 - (3 days ago) [ci] Enable C++23 for macOS Beta and 15 — Danilo Piparo
* | 8ddf345 - (3 days ago) [nfc][DF] Clarify documentation of RDFHistoModels — ferdymercury
* | 073e81f - (3 days ago) [roottest] fine-tuning of io/xml test — Sergey Linev
* | 17500f5 - (4 days ago) [roottest] modernize io-xml tests — Sergey Linev
* | 078afaa - (4 days ago) [roottest] use cmake for io-xml tests — Sergey Linev
* | 85b49c7 - (3 days ago) [nfc][meta] Code formatting — Danilo Piparo
* | 65df0a9 - (3 days ago) [meta] Test handling of C11 _Atomic type specifier — Danilo Piparo
* | db23e0b - (3 days ago) [meta] Properly handle C11 atomic type specifier in normalisation routines — Danilo Piparo
|/
* 7a222ec - (4 days ago) build: Actually record dep from RooFitJSONInterface to Core. — Philippe Canal
hageboeck pushed a commit to hageboeck/root that referenced this pull request May 22, 2025
@pcanal @hageboeck
[ci] Restrict clang-format to PR changes
d136e47 
Related github discussion: https://github.com/orgs/community/discussions/59677

For a given PR, the value of `github.event.pull_request.base.sha` always
corresponds to the SHA of the head/tip of the base branch at the moment
of either creation of the PR or the last force-push (regardless of whether
that force-push actually rebased the PR branch against base or was just
modifying an existing commit in it) to the PR branch.

In addition the merge commit is updated whenever a new commit is push
onto the PR branch but this merge commit is not against the (fixed)
`github.event.pull_request.base.sha` but is instead against the (moving)
tip of the target branch.

Currently (before this commit), our clang-format action will check the
differences between that commit and the merge commit.

In practice this means that if a PR languish and one or more PR is
actually merged into the master branch then the comparison will
now involved not only the commit introduced the PR but also all
the commits added to master since the PR creation (or its last
force push).

There is 2 general solutions to replace the `github.event.pull_request.base.sha`:

1. Find and use the non PR branch parent of the merge commit
2. Find and use the common ancestor of the master and the PR

This commit implement 2nd options by using `git merge-base` with the
arguments 'tip of the PR branch' and `github.event.pull_request.base.sha`
which is guaranteed to be ahead of/have a commong ancestor with
the PR branch.

Until there is a resolution to the above mentioned github discussion
this is more complex than before but more stable.

NOTE: This matters in our case since most existing files and some
PRs are merge without passing clang-format (in part because clang-format
still gives some significantly sub-optimal result in a few important
cases)

As a concrete example, the PR root-project#18587
was at some point in a state where the merge commit was:

*   354e754 - (3 hours ago) Merge e54a1ea into 59c28d5 — Philippe Canal

The tip of master was:

* | 59c28d5 - (5 hours ago) [nfc] mention deprecated ConcatFileName (root-project#18755) — ferdymercury

but the `github.event.pull_request.base.sha` was pointing to:

* | bda0b84 - (3 days ago) [cmake] Check if cplusplus is 23 by default — ferdymercury

As seen below this meant that the clang-format stage was reporting about
a lot more than what that PR had actually changed.

*   354e754 - (3 hours ago) Merge e54a1ea into 59c28d5 — Philippe Canal
|\
| * e54a1ea - (3 hours ago) [NFC] clang-format — Philippe Canal
| * 4c9bbf3 - (4 days ago) meta: Extend ClassInfo loading test — Philippe Canal
| * 7137bf0 - (2 weeks ago) roottest: switch test from DEPENDS to FIXTURES. — Philippe Canal
| * ca68ceb - (2 weeks ago) meta: Correct TClass::LoadClassInfo. — Philippe Canal
* | 59c28d5 - (5 hours ago) [nfc] mention deprecated ConcatFileName (root-project#18755) — ferdymercury
* | 0fe2f1f - (2 days ago) [RF] Fix memory leak of ConcatFileName return value in RooWorkspace — Jonas Rembser
* | eb0b903 - (6 hours ago) core: Add semantic ROOT::EnableImplicitMT (root-project#18694) — Philippe Canal
* | c814428 - (9 hours ago) [jsroot] dev 16/05/2025 with ws private members — Sergey Linev
* | bbff7ab - (9 hours ago) [qtweb] rename unload handler — Sergey Linev
* | af9d1e4 - (10 hours ago) [webgui][ui5] use websocket.isStandalone() — Sergey Linev
* | 768f741 - (10 hours ago) [webgui][ui5] replace title by tooltip in CheckBox — Sergey Linev
* | 2056891 - (10 hours ago) [webgui][ui5] use isChannel function of ws handler — Sergey Linev
* | 600573f - (2 days ago) [df] Add test for nested data members with same name — Vincenzo Eduardo Padulano
* | c8fd528 - (5 weeks ago) [tree] Speedup retrieval of branch names — Vincenzo Eduardo Padulano
* | 98d8160 - (33 hours ago) [Tutorials] Add Python dependencies for df101 and rf617. — Stephan Hageboeck
* | dd272ab - (15 hours ago) [math] add DeltaR to GenVector (root-project#18728) — ferdymercury
* | ddd8512 - (13 days ago) [test] Add test case — ferdymercury
* | ea465be - (2 weeks ago) [tree,hist] Fix parsing bugs in TTreeFormula and TFormula_v5 — ferdymercury
* | bda0b84 - (3 days ago) [cmake] Check if cplusplus is 23 by default — ferdymercury
* | 3049b5e - (2 weeks ago) [metacling] copy-paste from RTypes ClassDefBase into interpreted version — ferdymercury
* | aac51aa - (2 days ago) [MC] Correctly report stability of particles — Danilo Piparo
* | 6d6be1a - (2 days ago) [webgui] adjust stressGraphics_web.ref file — Sergey Linev
* | a198988 - (2 days ago) [jsroot] dev 15/05/2025 without RHist classes — Sergey Linev
* | b65a5d6 - (2 days ago) [roottest] fix leak by using TSystem::ConcatFileName — Sergey Linev
* | 470e732 - (2 days ago) [math] fix harmless initialization bug in halfsamplemode algorithm — ferdymercury
* | be3d7c1 - (2 days ago) [Math][GenVector] Substitute `M_PI*` with `TMath` function calls (root-project#18659) — Monica Dessole (origin/master, origin/HEAD)
* | 9a5f6ca - (7 days ago) [RF][HF] Don't create output directory if not needed — Jonas Rembser
* | a4a0933 - (7 days ago) [RF][HF] Add configuration parameter to disable workspace writing — Jonas Rembser
* | 6d570f6 - (7 days ago) [RF][HF] Make sure that RooMsgService is correctly reset — Jonas Rembser
* | 42a1c56 - (10 days ago) [core] Add tests for TClass lookup of STL containers. — Stephan Hageboeck
* | 9fe1956 - (10 days ago) [core] Fix a TClass lookup for STL containers. — Stephan Hageboeck
* | be95a45 - (4 days ago) [NFC] Move functions to anonymous namespace/clang-format TClassEdit. — Stephan Hageboeck
* | d0e79a5 - (2 days ago) [math] Add HalfSampleMode calculation to TMath — ferdymercury
* | ca36f75 - (3 days ago) [ci] Enable C++23 for macOS Beta and 15 — Danilo Piparo
* | 8ddf345 - (3 days ago) [nfc][DF] Clarify documentation of RDFHistoModels — ferdymercury
* | 073e81f - (3 days ago) [roottest] fine-tuning of io/xml test — Sergey Linev
* | 17500f5 - (4 days ago) [roottest] modernize io-xml tests — Sergey Linev
* | 078afaa - (4 days ago) [roottest] use cmake for io-xml tests — Sergey Linev
* | 85b49c7 - (3 days ago) [nfc][meta] Code formatting — Danilo Piparo
* | 65df0a9 - (3 days ago) [meta] Test handling of C11 _Atomic type specifier — Danilo Piparo
* | db23e0b - (3 days ago) [meta] Properly handle C11 atomic type specifier in normalisation routines — Danilo Piparo
|/
* 7a222ec - (4 days ago) build: Actually record dep from RooFitJSONInterface to Core. — Philippe Canal
pcanal added a commit to pcanal/root that referenced this pull request Jun 9, 2025
@pcanal
[ci] Restrict clang-format to PR changes
bb42f8e 
Related github discussion: https://github.com/orgs/community/discussions/59677

For a given PR, the value of `github.event.pull_request.base.sha` always
corresponds to the SHA of the head/tip of the base branch at the moment
of either creation of the PR or the last force-push (regardless of whether
that force-push actually rebased the PR branch against base or was just
modifying an existing commit in it) to the PR branch.

In addition the merge commit is updated whenever a new commit is push
onto the PR branch but this merge commit is not against the (fixed)
`github.event.pull_request.base.sha` but is instead against the (moving)
tip of the target branch.

Currently (before this commit), our clang-format action will check the
differences between that commit and the merge commit.

In practice this means that if a PR languish and one or more PR is
actually merged into the master branch then the comparison will
now involved not only the commit introduced the PR but also all
the commits added to master since the PR creation (or its last
force push).

There is 2 general solutions to replace the `github.event.pull_request.base.sha`:

1. Find and use the non PR branch parent of the merge commit
2. Find and use the common ancestor of the master and the PR

This commit implement 2nd options by using `git merge-base` with the
arguments 'tip of the PR branch' and `github.event.pull_request.base.sha`
which is guaranteed to be ahead of/have a commong ancestor with
the PR branch.

Until there is a resolution to the above mentioned github discussion
this is more complex than before but more stable.

NOTE: This matters in our case since most existing files and some
PRs are merge without passing clang-format (in part because clang-format
still gives some significantly sub-optimal result in a few important
cases)

As a concrete example, the PR root-project#18587
was at some point in a state where the merge commit was:

*   354e754 - (3 hours ago) Merge e54a1ea into 59c28d5 — Philippe Canal

The tip of master was:

* | 59c28d5 - (5 hours ago) [nfc] mention deprecated ConcatFileName (root-project#18755) — ferdymercury

but the `github.event.pull_request.base.sha` was pointing to:

* | bda0b84 - (3 days ago) [cmake] Check if cplusplus is 23 by default — ferdymercury

As seen below this meant that the clang-format stage was reporting about
a lot more than what that PR had actually changed.

*   354e754 - (3 hours ago) Merge e54a1ea into 59c28d5 — Philippe Canal
|\
| * e54a1ea - (3 hours ago) [NFC] clang-format — Philippe Canal
| * 4c9bbf3 - (4 days ago) meta: Extend ClassInfo loading test — Philippe Canal
| * 7137bf0 - (2 weeks ago) roottest: switch test from DEPENDS to FIXTURES. — Philippe Canal
| * ca68ceb - (2 weeks ago) meta: Correct TClass::LoadClassInfo. — Philippe Canal
* | 59c28d5 - (5 hours ago) [nfc] mention deprecated ConcatFileName (root-project#18755) — ferdymercury
* | 0fe2f1f - (2 days ago) [RF] Fix memory leak of ConcatFileName return value in RooWorkspace — Jonas Rembser
* | eb0b903 - (6 hours ago) core: Add semantic ROOT::EnableImplicitMT (root-project#18694) — Philippe Canal
* | c814428 - (9 hours ago) [jsroot] dev 16/05/2025 with ws private members — Sergey Linev
* | bbff7ab - (9 hours ago) [qtweb] rename unload handler — Sergey Linev
* | af9d1e4 - (10 hours ago) [webgui][ui5] use websocket.isStandalone() — Sergey Linev
* | 768f741 - (10 hours ago) [webgui][ui5] replace title by tooltip in CheckBox — Sergey Linev
* | 2056891 - (10 hours ago) [webgui][ui5] use isChannel function of ws handler — Sergey Linev
* | 600573f - (2 days ago) [df] Add test for nested data members with same name — Vincenzo Eduardo Padulano
* | c8fd528 - (5 weeks ago) [tree] Speedup retrieval of branch names — Vincenzo Eduardo Padulano
* | 98d8160 - (33 hours ago) [Tutorials] Add Python dependencies for df101 and rf617. — Stephan Hageboeck
* | dd272ab - (15 hours ago) [math] add DeltaR to GenVector (root-project#18728) — ferdymercury
* | ddd8512 - (13 days ago) [test] Add test case — ferdymercury
* | ea465be - (2 weeks ago) [tree,hist] Fix parsing bugs in TTreeFormula and TFormula_v5 — ferdymercury
* | bda0b84 - (3 days ago) [cmake] Check if cplusplus is 23 by default — ferdymercury
* | 3049b5e - (2 weeks ago) [metacling] copy-paste from RTypes ClassDefBase into interpreted version — ferdymercury
* | aac51aa - (2 days ago) [MC] Correctly report stability of particles — Danilo Piparo
* | 6d6be1a - (2 days ago) [webgui] adjust stressGraphics_web.ref file — Sergey Linev
* | a198988 - (2 days ago) [jsroot] dev 15/05/2025 without RHist classes — Sergey Linev
* | b65a5d6 - (2 days ago) [roottest] fix leak by using TSystem::ConcatFileName — Sergey Linev
* | 470e732 - (2 days ago) [math] fix harmless initialization bug in halfsamplemode algorithm — ferdymercury
* | be3d7c1 - (2 days ago) [Math][GenVector] Substitute `M_PI*` with `TMath` function calls (root-project#18659) — Monica Dessole (origin/master, origin/HEAD)
* | 9a5f6ca - (7 days ago) [RF][HF] Don't create output directory if not needed — Jonas Rembser
* | a4a0933 - (7 days ago) [RF][HF] Add configuration parameter to disable workspace writing — Jonas Rembser
* | 6d570f6 - (7 days ago) [RF][HF] Make sure that RooMsgService is correctly reset — Jonas Rembser
* | 42a1c56 - (10 days ago) [core] Add tests for TClass lookup of STL containers. — Stephan Hageboeck
* | 9fe1956 - (10 days ago) [core] Fix a TClass lookup for STL containers. — Stephan Hageboeck
* | be95a45 - (4 days ago) [NFC] Move functions to anonymous namespace/clang-format TClassEdit. — Stephan Hageboeck
* | d0e79a5 - (2 days ago) [math] Add HalfSampleMode calculation to TMath — ferdymercury
* | ca36f75 - (3 days ago) [ci] Enable C++23 for macOS Beta and 15 — Danilo Piparo
* | 8ddf345 - (3 days ago) [nfc][DF] Clarify documentation of RDFHistoModels — ferdymercury
* | 073e81f - (3 days ago) [roottest] fine-tuning of io/xml test — Sergey Linev
* | 17500f5 - (4 days ago) [roottest] modernize io-xml tests — Sergey Linev
* | 078afaa - (4 days ago) [roottest] use cmake for io-xml tests — Sergey Linev
* | 85b49c7 - (3 days ago) [nfc][meta] Code formatting — Danilo Piparo
* | 65df0a9 - (3 days ago) [meta] Test handling of C11 _Atomic type specifier — Danilo Piparo
* | db23e0b - (3 days ago) [meta] Properly handle C11 atomic type specifier in normalisation routines — Danilo Piparo
|/
* 7a222ec - (4 days ago) build: Actually record dep from RooFitJSONInterface to Core. — Philippe Canal
pcanal added a commit that referenced this pull request Jun 10, 2025
@pcanal
[ci] Restrict clang-format to PR changes
c37af5a 
Related github discussion: https://github.com/orgs/community/discussions/59677

For a given PR, the value of `github.event.pull_request.base.sha` always
corresponds to the SHA of the head/tip of the base branch at the moment
of either creation of the PR or the last force-push (regardless of whether
that force-push actually rebased the PR branch against base or was just
modifying an existing commit in it) to the PR branch.

In addition the merge commit is updated whenever a new commit is push
onto the PR branch but this merge commit is not against the (fixed)
`github.event.pull_request.base.sha` but is instead against the (moving)
tip of the target branch.

Currently (before this commit), our clang-format action will check the
differences between that commit and the merge commit.

In practice this means that if a PR languish and one or more PR is
actually merged into the master branch then the comparison will
now involved not only the commit introduced the PR but also all
the commits added to master since the PR creation (or its last
force push).

There is 2 general solutions to replace the `github.event.pull_request.base.sha`:

1. Find and use the non PR branch parent of the merge commit
2. Find and use the common ancestor of the master and the PR

This commit implement 2nd options by using `git merge-base` with the
arguments 'tip of the PR branch' and `github.event.pull_request.base.sha`
which is guaranteed to be ahead of/have a commong ancestor with
the PR branch.

Until there is a resolution to the above mentioned github discussion
this is more complex than before but more stable.

NOTE: This matters in our case since most existing files and some
PRs are merge without passing clang-format (in part because clang-format
still gives some significantly sub-optimal result in a few important
cases)

As a concrete example, the PR #18587
was at some point in a state where the merge commit was:

*   354e754 - (3 hours ago) Merge e54a1ea into 59c28d5 — Philippe Canal

The tip of master was:

* | 59c28d5 - (5 hours ago) [nfc] mention deprecated ConcatFileName (#18755) — ferdymercury

but the `github.event.pull_request.base.sha` was pointing to:

* | bda0b84 - (3 days ago) [cmake] Check if cplusplus is 23 by default — ferdymercury

As seen below this meant that the clang-format stage was reporting about
a lot more than what that PR had actually changed.

*   354e754 - (3 hours ago) Merge e54a1ea into 59c28d5 — Philippe Canal
|\
| * e54a1ea - (3 hours ago) [NFC] clang-format — Philippe Canal
| * 4c9bbf3 - (4 days ago) meta: Extend ClassInfo loading test — Philippe Canal
| * 7137bf0 - (2 weeks ago) roottest: switch test from DEPENDS to FIXTURES. — Philippe Canal
| * ca68ceb - (2 weeks ago) meta: Correct TClass::LoadClassInfo. — Philippe Canal
* | 59c28d5 - (5 hours ago) [nfc] mention deprecated ConcatFileName (#18755) — ferdymercury
* | 0fe2f1f - (2 days ago) [RF] Fix memory leak of ConcatFileName return value in RooWorkspace — Jonas Rembser
* | eb0b903 - (6 hours ago) core: Add semantic ROOT::EnableImplicitMT (#18694) — Philippe Canal
* | c814428 - (9 hours ago) [jsroot] dev 16/05/2025 with ws private members — Sergey Linev
* | bbff7ab - (9 hours ago) [qtweb] rename unload handler — Sergey Linev
* | af9d1e4 - (10 hours ago) [webgui][ui5] use websocket.isStandalone() — Sergey Linev
* | 768f741 - (10 hours ago) [webgui][ui5] replace title by tooltip in CheckBox — Sergey Linev
* | 2056891 - (10 hours ago) [webgui][ui5] use isChannel function of ws handler — Sergey Linev
* | 600573f - (2 days ago) [df] Add test for nested data members with same name — Vincenzo Eduardo Padulano
* | c8fd528 - (5 weeks ago) [tree] Speedup retrieval of branch names — Vincenzo Eduardo Padulano
* | 98d8160 - (33 hours ago) [Tutorials] Add Python dependencies for df101 and rf617. — Stephan Hageboeck
* | dd272ab - (15 hours ago) [math] add DeltaR to GenVector (#18728) — ferdymercury
* | ddd8512 - (13 days ago) [test] Add test case — ferdymercury
* | ea465be - (2 weeks ago) [tree,hist] Fix parsing bugs in TTreeFormula and TFormula_v5 — ferdymercury
* | bda0b84 - (3 days ago) [cmake] Check if cplusplus is 23 by default — ferdymercury
* | 3049b5e - (2 weeks ago) [metacling] copy-paste from RTypes ClassDefBase into interpreted version — ferdymercury
* | aac51aa - (2 days ago) [MC] Correctly report stability of particles — Danilo Piparo
* | 6d6be1a - (2 days ago) [webgui] adjust stressGraphics_web.ref file — Sergey Linev
* | a198988 - (2 days ago) [jsroot] dev 15/05/2025 without RHist classes — Sergey Linev
* | b65a5d6 - (2 days ago) [roottest] fix leak by using TSystem::ConcatFileName — Sergey Linev
* | 470e732 - (2 days ago) [math] fix harmless initialization bug in halfsamplemode algorithm — ferdymercury
* | be3d7c1 - (2 days ago) [Math][GenVector] Substitute `M_PI*` with `TMath` function calls (#18659) — Monica Dessole (origin/master, origin/HEAD)
* | 9a5f6ca - (7 days ago) [RF][HF] Don't create output directory if not needed — Jonas Rembser
* | a4a0933 - (7 days ago) [RF][HF] Add configuration parameter to disable workspace writing — Jonas Rembser
* | 6d570f6 - (7 days ago) [RF][HF] Make sure that RooMsgService is correctly reset — Jonas Rembser
* | 42a1c56 - (10 days ago) [core] Add tests for TClass lookup of STL containers. — Stephan Hageboeck
* | 9fe1956 - (10 days ago) [core] Fix a TClass lookup for STL containers. — Stephan Hageboeck
* | be95a45 - (4 days ago) [NFC] Move functions to anonymous namespace/clang-format TClassEdit. — Stephan Hageboeck
* | d0e79a5 - (2 days ago) [math] Add HalfSampleMode calculation to TMath — ferdymercury
* | ca36f75 - (3 days ago) [ci] Enable C++23 for macOS Beta and 15 — Danilo Piparo
* | 8ddf345 - (3 days ago) [nfc][DF] Clarify documentation of RDFHistoModels — ferdymercury
* | 073e81f - (3 days ago) [roottest] fine-tuning of io/xml test — Sergey Linev
* | 17500f5 - (4 days ago) [roottest] modernize io-xml tests — Sergey Linev
* | 078afaa - (4 days ago) [roottest] use cmake for io-xml tests — Sergey Linev
* | 85b49c7 - (3 days ago) [nfc][meta] Code formatting — Danilo Piparo
* | 65df0a9 - (3 days ago) [meta] Test handling of C11 _Atomic type specifier — Danilo Piparo
* | db23e0b - (3 days ago) [meta] Properly handle C11 atomic type specifier in normalisation routines — Danilo Piparo
|/
* 7a222ec - (4 days ago) build: Actually record dep from RooFitJSONInterface to Core. — Philippe Canal
martinfoell pushed a commit to martinfoell/root that referenced this pull request Oct 9, 2025
@pcanal
[ci] Restrict clang-format to PR changes
bf17ec4 
Related github discussion: https://github.com/orgs/community/discussions/59677

For a given PR, the value of `github.event.pull_request.base.sha` always
corresponds to the SHA of the head/tip of the base branch at the moment
of either creation of the PR or the last force-push (regardless of whether
that force-push actually rebased the PR branch against base or was just
modifying an existing commit in it) to the PR branch.

In addition the merge commit is updated whenever a new commit is push
onto the PR branch but this merge commit is not against the (fixed)
`github.event.pull_request.base.sha` but is instead against the (moving)
tip of the target branch.

Currently (before this commit), our clang-format action will check the
differences between that commit and the merge commit.

In practice this means that if a PR languish and one or more PR is
actually merged into the master branch then the comparison will
now involved not only the commit introduced the PR but also all
the commits added to master since the PR creation (or its last
force push).

There is 2 general solutions to replace the `github.event.pull_request.base.sha`:

1. Find and use the non PR branch parent of the merge commit
2. Find and use the common ancestor of the master and the PR

This commit implement 2nd options by using `git merge-base` with the
arguments 'tip of the PR branch' and `github.event.pull_request.base.sha`
which is guaranteed to be ahead of/have a commong ancestor with
the PR branch.

Until there is a resolution to the above mentioned github discussion
this is more complex than before but more stable.

NOTE: This matters in our case since most existing files and some
PRs are merge without passing clang-format (in part because clang-format
still gives some significantly sub-optimal result in a few important
cases)

As a concrete example, the PR root-project#18587
was at some point in a state where the merge commit was:

*   354e754 - (3 hours ago) Merge e54a1ea into 59c28d5 — Philippe Canal

The tip of master was:

* | 59c28d5 - (5 hours ago) [nfc] mention deprecated ConcatFileName (root-project#18755) — ferdymercury

but the `github.event.pull_request.base.sha` was pointing to:

* | bda0b84 - (3 days ago) [cmake] Check if cplusplus is 23 by default — ferdymercury

As seen below this meant that the clang-format stage was reporting about
a lot more than what that PR had actually changed.

*   354e754 - (3 hours ago) Merge e54a1ea into 59c28d5 — Philippe Canal
|\
| * e54a1ea - (3 hours ago) [NFC] clang-format — Philippe Canal
| * 4c9bbf3 - (4 days ago) meta: Extend ClassInfo loading test — Philippe Canal
| * 7137bf0 - (2 weeks ago) roottest: switch test from DEPENDS to FIXTURES. — Philippe Canal
| * ca68ceb - (2 weeks ago) meta: Correct TClass::LoadClassInfo. — Philippe Canal
* | 59c28d5 - (5 hours ago) [nfc] mention deprecated ConcatFileName (root-project#18755) — ferdymercury
* | 0fe2f1f - (2 days ago) [RF] Fix memory leak of ConcatFileName return value in RooWorkspace — Jonas Rembser
* | eb0b903 - (6 hours ago) core: Add semantic ROOT::EnableImplicitMT (root-project#18694) — Philippe Canal
* | c814428 - (9 hours ago) [jsroot] dev 16/05/2025 with ws private members — Sergey Linev
* | bbff7ab - (9 hours ago) [qtweb] rename unload handler — Sergey Linev
* | af9d1e4 - (10 hours ago) [webgui][ui5] use websocket.isStandalone() — Sergey Linev
* | 768f741 - (10 hours ago) [webgui][ui5] replace title by tooltip in CheckBox — Sergey Linev
* | 2056891 - (10 hours ago) [webgui][ui5] use isChannel function of ws handler — Sergey Linev
* | 600573f - (2 days ago) [df] Add test for nested data members with same name — Vincenzo Eduardo Padulano
* | c8fd528 - (5 weeks ago) [tree] Speedup retrieval of branch names — Vincenzo Eduardo Padulano
* | 98d8160 - (33 hours ago) [Tutorials] Add Python dependencies for df101 and rf617. — Stephan Hageboeck
* | dd272ab - (15 hours ago) [math] add DeltaR to GenVector (root-project#18728) — ferdymercury
* | ddd8512 - (13 days ago) [test] Add test case — ferdymercury
* | ea465be - (2 weeks ago) [tree,hist] Fix parsing bugs in TTreeFormula and TFormula_v5 — ferdymercury
* | bda0b84 - (3 days ago) [cmake] Check if cplusplus is 23 by default — ferdymercury
* | 3049b5e - (2 weeks ago) [metacling] copy-paste from RTypes ClassDefBase into interpreted version — ferdymercury
* | aac51aa - (2 days ago) [MC] Correctly report stability of particles — Danilo Piparo
* | 6d6be1a - (2 days ago) [webgui] adjust stressGraphics_web.ref file — Sergey Linev
* | a198988 - (2 days ago) [jsroot] dev 15/05/2025 without RHist classes — Sergey Linev
* | b65a5d6 - (2 days ago) [roottest] fix leak by using TSystem::ConcatFileName — Sergey Linev
* | 470e732 - (2 days ago) [math] fix harmless initialization bug in halfsamplemode algorithm — ferdymercury
* | be3d7c1 - (2 days ago) [Math][GenVector] Substitute `M_PI*` with `TMath` function calls (root-project#18659) — Monica Dessole (origin/master, origin/HEAD)
* | 9a5f6ca - (7 days ago) [RF][HF] Don't create output directory if not needed — Jonas Rembser
* | a4a0933 - (7 days ago) [RF][HF] Add configuration parameter to disable workspace writing — Jonas Rembser
* | 6d570f6 - (7 days ago) [RF][HF] Make sure that RooMsgService is correctly reset — Jonas Rembser
* | 42a1c56 - (10 days ago) [core] Add tests for TClass lookup of STL containers. — Stephan Hageboeck
* | 9fe1956 - (10 days ago) [core] Fix a TClass lookup for STL containers. — Stephan Hageboeck
* | be95a45 - (4 days ago) [NFC] Move functions to anonymous namespace/clang-format TClassEdit. — Stephan Hageboeck
* | d0e79a5 - (2 days ago) [math] Add HalfSampleMode calculation to TMath — ferdymercury
* | ca36f75 - (3 days ago) [ci] Enable C++23 for macOS Beta and 15 — Danilo Piparo
* | 8ddf345 - (3 days ago) [nfc][DF] Clarify documentation of RDFHistoModels — ferdymercury
* | 073e81f - (3 days ago) [roottest] fine-tuning of io/xml test — Sergey Linev
* | 17500f5 - (4 days ago) [roottest] modernize io-xml tests — Sergey Linev
* | 078afaa - (4 days ago) [roottest] use cmake for io-xml tests — Sergey Linev
* | 85b49c7 - (3 days ago) [nfc][meta] Code formatting — Danilo Piparo
* | 65df0a9 - (3 days ago) [meta] Test handling of C11 _Atomic type specifier — Danilo Piparo
* | db23e0b - (3 days ago) [meta] Properly handle C11 atomic type specifier in normalisation routines — Danilo Piparo
|/
* 7a222ec - (4 days ago) build: Actually record dep from RooFitJSONInterface to Core. — Philippe Canal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mdessole mdessole mdessole approved these changes

@lmoneta lmoneta Awaiting requested review from lmoneta lmoneta is a code owner

Assignees

@mdessole mdessole

Labels

None yet

Projects

None yet

Milestone

6.38.00

Development

Successfully merging this pull request may close these issues.

2 participants

@ferdymercury @mdessole