-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
csplit: clarify how the regex option works #19157
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
Conversation
sbrl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me! wording improvement is good, yesyes 🌟
e18e26d to
8fd47fe
Compare
|
Please don't force push |
Are we not supposed to rebase the branch when it gets outdated? What if there are conflicts? |
|
The github UI will warn in the case of conflicts. They are a non-issue. |
|
Right, that's a good point. It's a pity that we still have to rely on untidy commit histories to avoid breaking the code review experience, which should be the responsibility of the forge, i.e. GitHub. Hopefully novel approaches like GitButler's patch-based code review system and stable change IDs (like those used by jj) will catch on, so that we can have our cake and eat it too (i.e. have clean commit histories but also be able to follow the evolution of a patchset during review). For the time being, I'll refrain from force-pushing except to resolve conflicts. |
|
Every PR gets squshed before merge. An untidy commit history is a non-issue. |
In minor changes like this, I certainly agree. But I wouldn't hold that as a general rule, since more complex changes definitely benefit from modular commits, even if they result in a squashed commit in the main branch — people would then be able, at any point in the future, to follow the link in the commit to the PR and use the clean commit history to better understand the changes. Anyway, sorry for spawning an off-topic discussion here. As I mentioned above, I'll make sure to avoid unnecessary force-pushes in the future. |
|
That's exactly the reason why we prefer an untidy commit history in a PR. It's easier to follow when everything is in chronological order. |
|
I'd avoid stating subjective things like your merge vs. rebase preference like a fact. I personally prefer rebases for example, and when people amend their commits instead of add new ones to fix issues introduced by the commits in their branch. Or in other words, I prefer when people keep their feature-branches as clean as we'd like to keep Especially as I believe squash-and-merge is a workaround for messy feature-branches, but not an excuse to have messy feature-branches. Forges already provide a way to only review the changes between force-pushes, though in the case of a rebase with the base-branch it will show those changes from I've contributed to other projects that enforce no force-pushes too I know you aren't the only maintainer of tldr-pages who doesn't like them, though! I'm just suggesting to to avoid saying "we"! You should say "I". (Or perhaps cite our guidelines if this is documented somewhere.) There's nothing wrong with talking about the views of maintainers as a collective, but only on things we've collectively agreed on. But then you can refer to that agreement, especially as the PR was opened by a fellow maintainer, so they clearly would've missed the memo then. Edit: Turns out this is documented in the contribution guidelines! So most of what I've said is actually redundant, I apologize for that. But a link would be appreciated next time!
|
|
Thanks for chiming in, @SethFalco. I appreciate that the guideline is documented, but I should point out that the PR that originated it (#18647) did not have any meaningful discussion, and while it was approved by several established maintainers with a good grasp of the project's history and ethos, it also lacked the input of some of the people who have had a significant hand in the gradual construction of the project's direction, vision and culture over the year, like yourself, @sbrl, @kbdharun (and, if you allow the immodesty, former maintainers like myself). The fact that you were unaware of it is pretty telling, IMHO. In particular, I am troubled by the unqualified statement that "a messy pull request will not matter". Off-topic aside: @Managor, just in case you had missed it, I should point out that I asked for a clarification about your request above. No rush, of course — just making sure it didn't slip your radar :) |
karthik-script
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Checklist
common,linux,osx,windows,sunos,android, etc.Reference issue: #