search

InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.37k stars 660 forks source link

squash and merge #4750

Closed thewtex closed 5 days ago

thewtex commented 6 days ago
N-Dekker commented 6 days ago

@thewtex Thanks for addressing this issue. I agree that squash and merge can be used as a last resort, when we really feel it's necessary. Especially when a contributor has indicated that the contributor does not have the time to learn about amend and force-push.

However, for a promising new ITK contributor, I think it's really necessary to learn how to do that. And I believe that most contributors are eager enough to get their work merged, to put some extra effort into their pull request.

Looking at the result of your squash-and-merge in my local git clone, I feel a bit disappointed about GitHub, actually! It has recognized Stephen as the author, but it did not mention you (Matt!), as committer. Instead, GitHub has mentioned itself as committer!

$ git log dfd2d42f56abeb0daec50d40ae0f480eed396e2e --format=full
commit dfd2d42f56abeb0daec50d40ae0f480eed396e2e
Author: Stephen R. Aylward <stephen.aylward@kitware.com>
Commit: GitHub <noreply@github.com>

    ENH: Update SpatialObjects for MetaIO v1 (#4743)
    ....
thewtex commented 5 days ago

Neils, I agree that there are many contributors that there are some contributors who are eager to contribute to ITK, know Git already, and are willing to put the extra effort to learn the amend and commit process. I think there are more potential contributors, though, who are still trying to wrap their head around Git, and they are likely to see the steps required to create a perfectly craft commit history as requiring knowledge beyond what they are currently struggling with and overly onerous.

After we use these practices on a near daily basis, they become trivial to us, but nearly all new contributors, who are often not computer engineers or computer scientists in the scientific computing space, struggle to grok Git basics.

I completely agree that it is preferred to have the person who merged the pull request be recorded in the commit as the merger. I also find value in merging the PR via the squash and commit button. In the big picture, ITK is an awesome project with an amazing community, and like you said, developers should be eager to contribute effort :smile: . By lowering the barrier to entry by enabling squash and merge for new contributors, more contributors will discover this for themselves :joy_cat: