Skip to content
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.

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

Introduce cms-squash-topic #125

Merged
merged 5 commits into from
Oct 19, 2023
Merged

Introduce cms-squash-topic #125

merged 5 commits into from
Oct 19, 2023

Conversation

kpedro88
Copy link
Contributor

As requested in cms-sw/cms-bot#2080.

This tool is implemented as another git-cms-merge-topic variant, primarily to take advantage of the existing backup and old-base options.

It can squash a remote branch (by performing git-cms-checkout-topic first) or the current branch in a local working area (using the --current flag and not specifying the positional argument for the remote branch name).

By default, it populates the commit message of the squashed commit with all the messages of the original commits, but a custom message can be provided with the --message option.

Also did some cleanup in the code and added enforcement of unsupported options (previously, unsupported options weren't shown in the help message but could still be provided, potentially causing chaos).

Intending to update http://cms-sw.github.io/tutorial-resolve-conflicts.html accordingly (especially since squashing can also help make backports less painful), will submit a PR there soon

@cmsbuild
Copy link

cmsbuild commented Oct 12, 2023

A new Pull Request was created by @kpedro88 (Kevin Pedro) for branch master.

@cmsbuild, @aandvalenzuela, @iarspider, @smuzaffar can you please review it and eventually sign? Thanks.
@antoniovilela, @sextonkennedy, @rappoccio you are the release manager for this.
cms-bot commands are listed here

@cmsbuild
Copy link

Pull request #125 was updated.

@kpedro88
Copy link
Contributor Author

cms-sw/cms-sw.github.io#111 has the documentation update

@kpedro88
Copy link
Contributor Author

please test

@smuzaffar
Copy link
Contributor

@kpedro88 I have used the new git-cms-squash-topic tool (after downloading it locally) to squash cms-sw/cmssw#43030. It worked fine but the commits sha's it has used [a] (which belong to changes before squash) does not exist. Note that I forced push the squash commit to my original branch smuzaffarfix-O0. Is this expected?

[a]

commit https://github.com/cms-sw/cmssw/commit/04defae54a80de2d99cf91f705313b9a85afe31d
Author: Shahzad Malik Muzaffar <shahzad.malik.muzaffar@cern.ch>
Date:   Wed Oct 18 14:24:27 2023 +0200

    apply fixes as suggested

@kpedro88
Copy link
Contributor Author

@smuzaffar yes, force-pushing the squashed branch will remove the original commits from GitHub. cms-squash-topic creates a backup branch; you can also put that branch to GitHub if you want the original commits to stay around.

@smuzaffar
Copy link
Contributor

@kpedro88, the commit in the message points to cms-sw/cmssw (e.g. cms-sw/cmssw@04defae) so pushing it to user branch is not going to find this commit. Question is , should we keep these broken links in the commit message ? Could it be that in future these broken commits sha start pointing to some wrong change set?

@smuzaffar
Copy link
Contributor

ah my bad, it says This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository. so commit itself is accessible but does not belong to any branch in cms-sw/cmssw. Does this mean it can go away when we run a GC on repo?

@kpedro88
Copy link
Contributor Author

I am honestly not sure if GC on the upstream repo would affect the persistence of the commits when they're only in a fork.

I'm not particularly worried about SHA collisions. In any case, the commit message would obviously be different in the very rare occasion of such a collision.

@cmsbuild
Copy link

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c6f0dc/35246/summary.html
COMMIT: 247f3dc
CMSSW: CMSSW_13_3_X_2023-10-17-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cms-git-tools/125/35246/install.sh to create a dev area with all the needed externals and cmssw changes.

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:

You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c6f0dc/35246/git-recent-commits.json
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c6f0dc/35246/git-merge-result

Comparison Summary

Summary:

  • You potentially added 10 lines to the logs
  • Reco comparison results: 62639 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3357400
  • DQMHistoTests: Total failures: 155151
  • DQMHistoTests: Total nulls: 269
  • DQMHistoTests: Total successes: 3201958
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.10599999999999998 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 10224.0 ): -0.308 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 12634.0 ): 0.288 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 250202.181 ): -0.288 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 25202.0 ): 0.978 KiB SiStrip/MechanicalView
  • DQMHistoSizes: changed ( 7.3 ): -0.564 KiB SiStrip/MechanicalView
  • Checked 214 log files, 167 edm output root files, 50 DQM output files
  • TriggerResults: found differences in 18 / 48 workflows

@smuzaffar
Copy link
Contributor

+externals
@kpedro88 this looks good. I am happy to include these changes for next IB.

@cmsbuild
Copy link

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @rappoccio, @antoniovilela, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

@smuzaffar smuzaffar merged commit 79e8684 into cms-sw:master Oct 19, 2023
9 checks passed
smuzaffar added a commit to cms-sw/cmsdist that referenced this pull request Oct 19, 2023
@kpedro88
Copy link
Contributor Author

@smuzaffar can you merge the correspnding cms-sw/cms-sw.github.io#111 as well?

@smuzaffar
Copy link
Contributor

yes I will merge that PR once new cms-git-tools is deployed to cvmfs

@mmusich
Copy link

mmusich commented Jan 26, 2024

judging from the feedback given at cms-sw/cmssw#43592 (comment), seems the Co-authored-by is not working as intended (or at least it doesn't show in gitHub).

@kpedro88
Copy link
Contributor Author

@mmusich apologies, there was a single-character typo. Fixed here: #126

@mmusich
Copy link

mmusich commented Jan 26, 2024

thank you @kpedro88

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