search

cms-sw / cms-git-tools

CMS Git Helpers
34 stars 26 forks source link

Introduce cms-squash-topic #125

Closed kpedro88 closed 1 year ago

kpedro88 commented 1 year ago

As requested in https://github.com/cms-sw/cms-bot/issues/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 commented 1 year ago

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 commented 1 year ago

Pull request #125 was updated.

kpedro88 commented 1 year ago

https://github.com/cms-sw/cms-sw.github.io/pull/111 has the documentation update

kpedro88 commented 1 year ago

please test

smuzaffar commented 1 year ago

@kpedro88 I have used the new git-cms-squash-topic tool (after downloading it locally) to squash https://github.com/cms-sw/cmssw/pull/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 commented 1 year ago

@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 commented 1 year ago

@kpedro88, the commit in the message points to cms-sw/cmssw (e.g. https://github.com/cms-sw/cmssw/commit/04defae54a80de2d99cf91f705313b9a85afe31d) 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c6f0dc/35246/summary.html COMMIT: 247f3dc5c3f156eb4c1608d47aeb2b0efd522162 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:

smuzaffar commented 1 year ago

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

cmsbuild commented 1 year ago

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)

kpedro88 commented 1 year ago

@smuzaffar can you merge the correspnding https://github.com/cms-sw/cms-sw.github.io/pull/111 as well?

smuzaffar commented 1 year ago

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

mmusich commented 9 months ago

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

kpedro88 commented 9 months ago

@mmusich apologies, there was a single-character typo. Fixed here: https://github.com/cms-sw/cms-git-tools/pull/126

mmusich commented 9 months ago

thank you @kpedro88