GitHub "squash and merge" not tracked in IB history

[fabiocos]

I notice that a "squash and merge" GitHub action, as used for cms-sw/cmssw#25386 to get rid of big files in the PR history, is apparently not properly managed as far as the IB history is concerned. Indeed as can be seen in https://cmssdt.cern.ch/SDT/html/cmssdt-ib/#/ib/CMSSW_10_2_X this PR seems not present, while the code is indeed there (checking with a git log or looking at the commit history for the branch.

@smuzaffar is this part managed by cms-bot? I am thinking to the PR history for the release-notes, is this going to be affected as well?

[cmsbuild]

A new Issue was created by @fabiocos Fabio Cossutti.

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[perrotta]

Indeed, I noticed the same in the release notes of 10_4_0_pre4, where the following PRs (at least) are not listed but their code changes are indeed present in the release:

• #25454: Fix clang warnings in TrackingTools/PatternTools
• #25449: Only call consumes in ElectronSeedProducer if actually needed
• #25016: DNN-based Tau-Id discriminants

Maybe the cause is the same...

[fabiocos]

@perrotta thank you for noticing it, that is interesting, it seems to be a bit unrelated from the use of the GitHub squash, as to merge those the "regular" procedure was used...

@smuzaffar any idea? A collateral damage of the history cleaning? Do we have a safe way to manually amend release notes? Ultimately this is what matters most to my mind for future reference

[smuzaffar]

the sha pointing in cms-sw/cmssw#25016 did not match the history of cmssw (because of re-write of history) that is why this release note generation script ignore this PR [a]. Anyway, I have updated the sha now ( cms-sw/cms-prs@7b04589#diff-f6d5d492efe81a492f3a54ccc5a2f3b6 ) and re-generated the release notes. Yuo should now see this PR in there.

[a]
Checking 25016 cms-tau-pog 5ea1151
sha mismatch: 77c09c7f7a29e205d814e9636511ee6ff46bf376
Invalid/Indirect PR

[fabiocos]

@smuzaffar thank you, this has fixed #25016 . No idea of what has happened to the other two PR that were mentioned by @perrotta

[smuzaffar]

ah ok, we also need to fix those PR too. Any PR which was merged after cms-sw/cmssw#25016 and before the re-write of history should have the same issue. I will fix these too and will -regenerate the release notes.

[smuzaffar]

OK this should be fixed now https://github.com/cms-sw/cmssw/releases/tag/CMSSW_10_4_0_pre4
Three PRs had issue , 2 which @perrotta mentioned plus

• Define new run2_miniAOD_devel and run2_nanoAOD_102Xv1 modifiers cmssw#25443 : Define new run2_miniAOD_devel and run2_nanoAOD_102Xv1 modifiers

[fabiocos]

thanks, comparing with the "git log" output now it should be ok.

[kpedro88]

@smuzaffar I'm not entirely sure how you did the history cleaning, but in the future (if necessary again, hopefully not), the option --preserve-merges can be used with git rebase.

[smuzaffar]

@kpedro88 , I used BFG tol https://github.com/IBM/BluePic/wiki/Using-BFG-Repo-Cleaner-tool-to-remove-sensitive-files-from-your-git-repo and it did kept the merge commits. problem is that we have this repo https://github.com/cms-sw/cms-prs where we keep track/cache of all PRs. Information from this repository is used for generating release-note. As the history was changed so the sha to PR mapping did not match for these few PRs which resulted in in-complete release notes.

In future if we have to do it then we can make sure that cms-sw/cms-prs is also updated with correct information.

[fabiocos]

@smuzaffar coming back to the original issue: I think that the source of the problem sits in the method here https://github.com/cms-sw/cms-bot/blob/master/github_utils.py#L216 where the pre-formatted output of the comment to a merge bot action is used to identify the commit corresponding to a merged PR. The simplest thing is clearly to use that message if one wants to (exceptionally) use the squash and merge action. The alternative would be to define a "squash" bot action, that is essentially a squashing commits into a single one + merge.

[fabiocos]

Coming back to this issue: from recent discussions with @Dr15Jones and @slava77 I understand that squashing of very long PRs with many commits is not encouraged, even if done in separate PR preserving the review history. Clearly for well organized and commented commits the separation contain useful information, which is lost in suqashing even though the history of commits is kept. Unfortunately they are often interleaved with trivial non-informative commits which tend to spoil the history.

In any case I have clarified with @smuzaffar that we are not tight in space in the GitHub repository (whose size I am constantly monitoring). Therefore I will avoid squashing in future PRs unless strictly necessary because of very large files sneaking into the history (like the december problem with the 50 MB file).

I leave to the reviewers to encourage possible intermediate squashing of non-informative commits by the authors.