Replies: 9 comments 15 replies
-
|
I believe "squash and merge" policy dominate in this repository because almost all the time a single PR contains commits that fixes the PR itself and merging with "rebase and merge" policy of those PRs would create a mess in git history. Since the situation you are mentioning statistically unusual I would suggest not to put extra work for all authors for squashing patches before merge. Nevertheless, I agree that having several function patches from a single PR squashed is not the best practice either.
|
Beta Was this translation helpful? Give feedback.
-
|
That's a fair point in terms of most common usage, but even then, I think people should be in the habit of curating what they present to the community themselves, and not rely on github's machinery to understand their intention and context. As an example, consider the following:
I don't think this scenario is uncommon; I've come across it once or twice in history in the past couple of weeks. Additionally - as an aside - I've just now noticed that even in the single-commit-per-pr case, github is munging the commit and rewriting the author of the commit to be the name of the github account rather than whatever the user has in their |
Beta Was this translation helpful? Give feedback.
-
If this is an option, and we can communicate its purpose amongs reviewers, I'd be very happy with that as a solution. I'm assuming the author would be responsible for rebasing on top of the current merge base to ensure a fast forward merge? |
Beta Was this translation helpful? Give feedback.
-
@smaslov-intel mentioned in the past that rebases removes the ability to track how review comments are addressed.
|
Beta Was this translation helpful? Give feedback.
-
This is a tooling issue with github. Forcing users to create suboptimal history due to bad tooling is disappointing, but obviously making reviewers lives easier is a good thing. I'm guessing you just saw me reflexively do this in my libclc PR. I'm experiencing an impedance mismatch between my existing workflow where I expect a user to maintain a PR in an always-ready-to-fast-foward state with clean history, and the current squash-and-munge behaviour. A middle ground would be appreciated that optimizes for long-term cleanliness of the history rather than optimizing around github's broken UI. I like the tag idea proposed by @MrSidims. |
Beta Was this translation helpful? Give feedback.
-
I don't think we are forcing users to create suboptimal history. As Dmitry said it's specifically designed to keep clean history. Your proposal shifts the load to developers and require them to be quite proficient in git as they will have to curate the history in their private branches. From my experience this requirement sets the bar too high for most of the contributors. |
Beta Was this translation helpful? Give feedback.
-
Good! Why should reviewers and future bisectors have to have the cognitive overhead? Accepting low standards might increase velocity now, but for anyone who has to look at the past it's not a good trade-off - it's likely to cause significant issues in future. I don't believe that's a good long-term economy.
Assuming your contributors can't use git but can contribute to a compiler toolchain is fairly cynical. Why is history quality not something we require of our contributors, but code quality is? |
Beta Was this translation helpful? Give feedback.
-
You can rebase with fast-forward and don't need a merge commit. It's still linear history
llvm upstream does not build on every commit, and we're superset of those commits, therefore dpc++ doesn't build on every commit either |
Beta Was this translation helpful? Give feedback.
-
|
As discussed in #13073 I do not agree with changing this behavior. I will try not to regurgitate the points already made, most of which I agree with.
Ironically, this is what we prefer contributors do. It means not only a cleaner separation in changes for the sake of review, but also for the sake of the history, as is what you are requesting here. The drawback is indeed that it comes at a slightly higher overhead for the contributor, having to maintain multiple patches, potentially some blocked by others, but it makes life easier for reviewers for the following reasons:
So this part comes down to whether we want to make life "easier" for contributors (@bader made a great point about it actually raising the bar for contributors, so is it really easier for them?) or reviewers. In this case I would side with the reviewers. For what it is worth, I do not think the process is perfect, but I also think this is the lesser evil.
Which part are you referring to? To my knowledge, the description is made the commit message, so the contributor still has control, and responsibility, to keep the message up to date. The miss-contribution point is fair, but maybe it is something that can be fine-tuned somewhere.
That's a valid concern, but are we certain this is actually how upstream will feel when we do the separation as mentioned above? I imagine there are other things we need to do for upstreaming as well, w.r.t. how commits are done, so I'd rather not speculatively make large changes to the process like this without knowing for sure this will be a blocker. @elizabethandrews & @premanandrao - Feel free to comment here. |
Beta Was this translation helpful? Give feedback.
-
|
I do not think we should be squashing different cherry-picks from community into 1 commit in syclos. Having said that I do not agree with keeping commits in a PR separate. We've had PRs with 30+ commits making minor code review changes. IMO it makes no sense to keep those commits separate and it makes it harder to cherry-pick relevant patches for a functionality in the future. If we want to preserve the commits, like cherry-picks from community or different functional patches, I would recommend different PRs. I am also not opposed to adding a tag of some sort to add the 'do not squash' functionality to PRs while keeping 'Squash and Merge' the default. Also, I THINK community currently does 'Squash and Merge' for their repository. |
Beta Was this translation helpful? Give feedback.
-
|
Yes. Perhaps I've been unclear. I'm not suggesting that we always "don't squash"; I'm suggesting we need the mechanism to not squash as an available option so that we can do the right thing when we need to. |
Beta Was this translation helpful? Give feedback.
-
If by "community" you mean upstream llvm.org, then both options are available. It's quite possible and preferred by many to push directly to |
Beta Was this translation helpful? Give feedback.
-
If we had to go ahead with it, that would probably also be my preferred way of doing it, but we might want to teach people to wait with force-pushes until the very end (discussed further down) while we now tell people never to do it. I think it complicates the process for reviews and gatekeeping, but if we think there's enough benefit to having both, I can live with it. Note though that there is no good way (that I know of) to make sure gatekeepers know to not "squash" during merge. Tags can be missed pretty easily. |
Beta Was this translation helpful? Give feedback.
-
|
I don't want to re-iterate the points already discussed but I broadly agree with @ldrumm on this issue. I want to say that I feel that discussion has turned into one about "Squash & Merge" or "Rebase & Merge". I personally don't see why we can't have both options available to gatekeepers. I agree that most PRs are one commit followed by several fixups, which should definitely be squashed. This, in a sense, feels to me like the "default" merge strategy. There's no argument for keeping these as separate commits - that's not benefiting anybody. But I don't see why we can't allow the option of merging certain PRs as a series of smaller patch sets. I saw @bader say that having each commit build and pass tests is important and so "Rebase" is off the cards. If that's so then isn't that the end of this discussion? I don't want to go off into the weeds about this particular aspect without a concrete answer, as it feels like a separate discussion. Whatever the decision, for me it's ultimately about trusting contributors and letting them use all of the tools at their disposal. Each contributor has sole control of only the commits they push to a PR, and taking that power, expressiveness, and control away from them is a shame. We're allowing GitHub with its (in my opinion) very crude review/merging capabilities to dictate our repo's git history. If you'll allow me to take a more holistic view for a second: encouraging and helping contributors to maintain good commit histories is a valuable transferable skill they can take to other projects (possibly using something other than GitHub), and help improve those by encouraging good practices there. Speaking of best practices, we even link to the influential article https://cbea.ms/git-commit/ in our contributing guide, but seem to ignore most of it when it comes to actually merging the PR. What does it matter how contributors' git commits are formulated and the importance of a good git history as long as the PR description is in order? That's because the Squash & Merge commit message is ultimately taken from the PR description, not from the commits making up the PR. Through an opaque process - I may add - as it also reformats it to 80 columns and maybe performs other transformations I'm not aware of - who knows what it does with HTML. So the gatekeepers are responsible for editing the final commit message (e.g., taking out any GitHub web-related aspects) which again is taking final authorship and control away from the contributor. If we're relying on gatekeepers to check that the PR contents match the PR description at the point of merging, and to edit the commit message as necessary, we have to trust that gatekeepers would be able to make a sensible decision to merge a PR as a series of individual commits where appropriate through the "Rebase" option. I reckon 99% of the time they'd make the correct choice in this regard. |
Beta Was this translation helpful? Give feedback.
-
As I said before, that's not a useful requirement since our upstream isn't buildable on every commit. |
Beta Was this translation helpful? Give feedback.
-
|
Upstream commits (aka pulldown pull request) are merged with |
Beta Was this translation helpful? Give feedback.
-
|
It's a merge commit, which introduces multiple commits. Not all those commits are necessarily buildable, but the merge commit itself is. It's exactly the same situation, except the tested commit has more than one parent. |
Beta Was this translation helpful? Give feedback.
-
An anecdote: (Quite unaccountably, there's a similar accessible door two floors up that works flawlessly in assistance or traditional barge-through-the-door mode and the assistance buttons are convenient on both sides) My point is that github's deficiency in tracking review comments has altered our behaviour. If you understand how to present clean history (by fixing up your commits as you work or receive review), but suddenly the tooling stops you from doing it you do one of two things:
The unopenable door, and github are (2) here. We shouldn't be accepting the bad door, or github's bugs but here we are: maintainers tell people off for github's intransigence in fixing their tools, and building maintenance tell me to press the button. Contributors a forced through a less-expressive and powerful funnel of the github UI, and I lose out on an opportunity for daily activity.
People aren't CPU cores, and cognitive overhead of stitching together and ordering separate PRs is greater than looking at one diff that's already properly ordered and happens to be also reviewable as separate commits.
Good! As far as I'm concerned, that's a maintainer's job: we should review code and metadata both - asking contributors to correct either if they're wrong. Let's not let github take that opportunity for quality away from us. |
Beta Was this translation helpful? Give feedback.
-
|
I like the anecdote and don't necessarily disagree with it. I am not sure I understand the moral of the story though. Are you suggesting we push GH to make the tracking better? We could, but that doesn't change the here-and-now, in which review history becomes incredibly hard to track for reviewers, which is especially important since PRs can have many reviewers, each owning their own part.
If they are so tightly related that you need to do them in series, I might argue that having a single PR (and in turn a single commit) would be fine... Depending on how big it becomes. If they are wholly separate fixes, make them separate PRs and let people focus. I agree, there is a cognitive overhead understanding a chain of changes, but if they need a chain understanding, then maybe they should either be together or come in turns.
Sure, you could argue that way, but part of the argument here is also making people's workflows and lives easier. It seems to me that the only workflow this would eventually benefit is some bisecting, which in turn can also become harder if we don't know if individual commits after merge work by themselves. On the other hand, it comes at the peril of reviewers and gatekeepers who have to live with even more process, more dimensions to think about and review. Contributors outnumber reviewers, so my argument is just that, though this is what they signed up for, I like to think that the cost to their workflow outweighs the benefit added to contributors. Maybe I am biased. 😉 |
Beta Was this translation helpful? Give feedback.
-
I doubt github will listen, since github basically hasn't improved since MVP in 2013, but if another voice - perhaps one backed by a large and respected multinational corporation were to push them - that might be enough. What I'd prefer is reviewers actually understood git history, and that git != github. Once they realise that, they'll start treating the git history with the attention it deserves.
Definitely a case-by-case basis, I think. An example that comes to mind is an extension I'm working on where it makes sense to implement the common code in one commit, then the cuda, and hip, and opencl, and l0 changes in others, yet land the implementation of the feature all at once. Then it becomes trivial to reason about individual regressions, every commit has relevant documentation, and everyone gets the same feature at once. It's no harder to review - in fact for people who know what they're doing - it becomes easier to review since they can more easily split the review into architectural chunks by looking at individual commit changesets
Who's lives are you trying to make easier and at what point in time? My proposal to make this an optional feature doesn't change the status-quo; we can still do the (in my opinion sloppy and selling-out-the-future for the present) form of squash-everything, but making extra tools available to gatekeepers so that a contributor can present good work when needed given should not be banned because some people don't know or care about such features. |
Beta Was this translation helpful? Give feedback.
-
I think that is a matter of perspective. The way I see it, with squash & merge we get the history of how the sycl branch evolved, one change at a time. That is why we highly prefer PRs to be one change at a time. If I care for the individual patches made in the PR, including the "fix formatting" patches, I can open the PR which has the history. And I understand that it is the point you made with git != github, but to me it becomes a question of granularity and using the tools at our disposal.
To some extent, I think that is a fair case. Would you do a cleanup of any review changes coming in? For example, say you have 1 commit for the extension implementation, then 1 commit per backend, then you might have 7 commits on top making changes based on reviews. Would you want all 12 commits making it into the sycl branch, or would you do a cleanup run to get back to the 5 original commits? As mentioned a couple of times previously, force-pushing makes it harder for reviewers to cross-reference their old reviews with your changes, so I would hope such a cleanup would be at the end, but I suppose it would be gatekeepers/reviewers jobs to make sure you did such cleanup? An alternative is to open one patch for the interface. When that is merged, you can open patches for each of the backends in separation. I do understand that it potentially comes with the overhead of not being able to get reviews on your backend changes prior to the first changes being in, but you also don't get 5 reviewer groups talking over each other in your patch and then having to find the relevant changes for them.
What happens when the additional changes come in, in the form of additional patches, e.g. if CI fails or you get reviews? I would argue the current way of reviewing for most groups is fairly simple as the separation of ownership is generally directory-based, so you go to the changes and look for your directory and review that part. As a funny side-note, larger PRs on Github can sometimes make reviewing a huge pain, even if it's a small change you need to review. It's to the point where Github will show you the infamous unicorn just because it takes too long to load the changes. That's a quirk of the Github interface, so it's not an argument for or against your points.
My point here is, I only see this making reviewer and gatekeeper lives more difficult, for a handful of reasons:
And yet, I don't see much benefit from a contributor point-of-view, apart from them being able to include multiple changes into a single PR and keep it after merge. Depending on whether we use merge-commit-based merging or rebase-and-merge, using tools like |
Beta Was this translation helpful? Give feedback.
-
|
FYI an example of misattribution caused by this policy: #13358 |
Beta Was this translation helpful? Give feedback.
-
|
Although I am still on the fence about allowing the general use-case of non-squash-and-merge, this is a good example of why we should maybe unlock the option of rebase-and-merge(?) and trust gatekeepers to use it when it is required. |
Beta Was this translation helpful? Give feedback.
-
|
If we need an exception to the standard process, I suggest we extend our automation for |
Beta Was this translation helpful? Give feedback.
-
Git history and merge requests
I've been a bit disappointed to discover that we automatically squash merge
requests containing multiple commits. I've found this troubling and as I've got
a series of fixes that contain cherry-picks, NFC refactoring of troublesome
code, and then the fix to a real bug. They're conceptually and legally
separate entities, but part of the same workflow. I've resorted to created pull
requests for each commit to ensure the integrity of the history, but this is a
hack.
I find the current status-quo problematic for the following reasons
need a cleanup before a feature or bugfix. In many cases the commits
conceptually standalone. The squash everything practise breaks this separation
and will confuse future history spelunking efforts. It's possible to create
multiple merge requests per commit, but this increases traffic, CI overhead, and
requires much more effort from the author to corral the patch series to landing.
misattributing authorship
autogenerated which quite often involves github specific language, user
mentions, merge-request machinery etc, and often clobbers individual commit
messages which contain subtle and important standalone information
people from being able to craft their own commit messages jVgq
eventually want to merge up
I'm not sure whether this has been reasoned about in public before, but I can't
see specific mention of the aggressive squashing practise in the wiki nor the
source.
Relying on github to automatically "clean up" working history loses too much
information, makes it impossible for those who want to do the right thing to
present a clean history, and allows those who don't care to ignore the needs of
those who have to read the code later. I won't use this forum to proselitize on
why a well-curated git history is important, as there are plenty of convincing
text on the subject. However, I propose to require authors to squash
review-fixups themselves and present a curated patch or patchset before final PR
merge. This requires a little extra effort from authors and reviewers upfront
(to check on them) but I think the long-term maintainability and health of the
project goes up when history is properly curated.
patches to this proposal welcome ;)
Beta Was this translation helpful? Give feedback.
All reactions