search

systemd / zram-generator

Systemd unit generator for zram devices
MIT License
584 stars 50 forks source link

man: describe how to apply config changes at runtime #105

Closed keszybz closed 3 years ago

keszybz commented 3 years ago

Fixes #78.

@polarathene PTAL.

keszybz commented 3 years ago

I applied the suggestions above, except as commented. And I added a second commit to remove some formatting.

keszybz commented 3 years ago

Also, if you want me to participate further in review if needed, I'd appreciate it if you just add new commits rather than rewrite history and force push. It's easier to review diffs from new commits, than parse the whole set of changes all over again.

This would maybe make the review a bit easier, but we care about having "perfect" history much more. I.e. any commits that didn't make it onto an official branch are subject to being rewritten and rebased freely. In case of a pr with one or two commits this doesn't matter that much, but I try to apply the same workflow in all cases. It's just too easy to forget to squash otherwise. When you get a notification from github, there should be a link to a diff, though that doesn't work sometimes. But there is a link to the old version above ("keszybz force-pushed the keszybz:man-config-restart branch from 70c5d1a to 7662ebc 5 minutes ago"), and you can get the old commits from there.

polarathene commented 3 years ago

but we care about having "perfect" history much more. I.e. any commits that didn't make it onto an official branch are subject to being rewritten and rebased freely.

You can rebase prior to merging, once reviews are all done. Then merge from that rebase however you see fit.

If you want review feedback, make it easier for reviewers to do so, expecting us to review the entire PR again and manually look for differences, or put in additional effort to bring up previous commits to diff between is friction you can avoid adding.

You can have whatever history you like on the primary branch, but for PR you're going to have messy force push branch history through the thread multiple times. If you ever need to git blame and come back to the PR discussion, it would be more beneficial to have a clear commit flow with a possible rebase force push at the end, rather than rewriting history multiple times and trying to follow the commits being discussed to understand something.

When you get a notification from github, there should be a link to a diff, though that doesn't work sometimes.

This only happens for me when you push new commits without force pushing to overwrite the history.

But there is a link to the old version above ("keszybz force-pushed the keszybz:man-config-restart branch from 70c5d1a to 7662ebc 5 minutes ago"), and you can get the old commits from there.

That's not really helpful, I still have to do additional manual work to diff between the two that I otherwise would not need to.

Just like you've done here with a rebase and force push, you could have done so at the end of a review when ready to merge, there was no necessary benefit from doing this multiple times prior.

I will come back later to look at the changes you made and respond to any other feedback comments, as I can't be bothered sorting out a diff where I have to wade out each individual change that would be lumped into the same rewritten commit.

keszybz commented 3 years ago

This pull request is essentially a single paragraph. And when reviewing changes to text you generally need to look at the whole paragraph, to control for style consistency, lack of repeats, clarity, etc. So splitting the change into multiple commits doesn't actually help with review. (It would only be useful for trivial things like you point a typo and I fix a typo. But we don't need a re-review for those changes.)

Also, when reviewing pull requests, I want to be able to review the clarity of the split into commits, and commit messages themselves. With history which consists of real commits and fixup commits you can't really do this.

But more generally, in my experience, pull requests with multiple "as you go" commits lead to those fixup commits being merged into history. Once the pull request is done one would have to go back, and split the fixup commits into pieces which apply to the "real" commits. But rebasing is hard, and I'm likely to introduce some errors then.

So overall, if at the end you want to have clean history, I think it's better to maintain such state at all times. It may make the review a bit harder in some cases, but in other cases it is either neutral or actually makes review easier.

keszybz commented 3 years ago

I'll merge this as is. I think it's good enough, and the proposed changes are just a matter of personal preferences.

polarathene commented 3 years ago

So splitting the change into multiple commits doesn't actually help with review.

You misunderstand. When you perform a review on the original PR content and discuss changes, you would like to review those changes via a diff.

You lose that convenience when you rewrite the history and force push when that is not necessary. The reviewer must start over again and reference their notes, or dig up the old content to manually diff themselves.

The only advantage I can see with your approach here is to add friction to reviewers so that you can push whatever changes you want and reduce reviewers providing feedback, in which case, why even bother for the feedback?

Also, when reviewing pull requests, I want to be able to review the clarity of the split into commits, and commit messages themselves. With history which consists of real commits and fixup commits you can't really do this.

Once you have multiple commits for your pristine history, you're already going to have to juggle a bit if you need to update older commits with new content. Sometimes that's not a problem unless the file has a series of logical commits where some conflicts may get introduced when you interactively rebase.

Regardless, keep a separate local branch if you wish to take your current approach used here, and swap it with the PR branch to force push to at the end. Problem solved.

But more generally, in my experience, pull requests with multiple "as you go" commits lead to those fixup commits being merged into history.

Not if you take the approach of squashing all PR commits into a single merge commit. You can retain history on the PR as separate commits, while Github performs the squash merge to master.

Once the pull request is done one would have to go back, and split the fixup commits into pieces which apply to the "real" commits. But rebasing is hard, and I'm likely to introduce some errors then.

Yes, it can be a hassle.

I generally take the approach of carefully crafting my logical commit history before opening a PR, for PRs that have multiple commits and some of those are layered in a way that I'd have to deal with resolving merge commits to rewrite the history, it doesn't matter if I rewrite and force push for keeping pristine history, or if append review commits and optionally cleanup afterwards before merge, that issue would still be the same.

If it would be different for you, then taking the approach of adding a new commit, and then migrating that over to your local extra branch copy with pristine commit history should work fine for you.

So overall, if at the end you want to have clean history, I think it's better to maintain such state at all times. It may make the review a bit harder in some cases, but in other cases it is either neutral or actually makes review easier.

I disagree. As stated earlier, you're adding friction to reviewers. I only had time to spare today to go over the other changes due to the lack of diff, but already too late. You worry about quality of PR content and risk of introducing problems, yet you're fine with making a reviewers job more difficult.

For documentation PR like this, it's not really a big issue, at worse I raise some concerns about the content I did not get time to review and you agree with whatever I say requiring a new PR to update that...well there goes the pristine history and commits as a follow-up/fixup commit would effectively be the same, just not all in the same PR.

I speak this from experience though... Look at this rather lengthy PR discussion. We don't go through such a process often, but if we did not take an iterative commit approach (which I believe the author had taken with past contributions), that would not be very practical to review.

Here is one of my PRs, documentation large PR with 12 commits. The history is pristine like you prefer it, I did many more local commits but interactively rebased it many times as I went along and it was of the kind that had merge conflicts to resolve often when doing so, the commit structure was to ease review as the full diff all at once wasn't exactly practical.

One more from me, this one required changes during review. I could have interactively rebased to rewrite history, but would you as a reviewer be ok with that? Especially if the changes were more involved to warrant further review. The reviewer shouldn't have to think "what changed?" during the process. As that project is happy with squashing changes on merge, no history rewrite was done as it didn't really need such.

Apologies for verbosity, if your still adamant on your approach that's fine. I just thought I'd take a shot at raising awareness of why appending new commits during the review process is more beneficial than blanket rewriting history.

Hopefully you take it into consideration in future, especially for PRs of a larger nature with code involved.

polarathene commented 3 years ago

Review finished (despite missing the merge):

Please do not ping me for review in future if you're going to dismiss the bulk of it and are unwilling to be accommodating when making changes.

From what I could see, very little was actually changed in the changes that I had to manually lookup commits and diff:

--- https://github.com/systemd/zram-generator/commit/0a3c88f236817137d85432397f7d0ff36408d28c
+++ https://github.com/systemd/zram-generator/commit/40aa017abd69b72a066b8ca87e6f2eb5c1c23494
@@ -5,10 +5,11 @@

 Nevertheless, sometimes it may be useful to add new devices or apply config changes at runtime.
 Applying new configuration means restarting the units, and that in turn means recreating the zram devices.
-This means that filesystems are temporarily unmounted and their contents lost, and pages are moved out of swap devices into other memory.
-If this is acceptable, `systemctl restart systemd-zram-setup@zram<N>` or `systemctl restart systemd-zram-setup@*`
+This means that *file systems are temporarily unmounted and their contents lost*, and *pages are moved out of the compressed swap device* into other memory.
+If this is acceptable, `systemctl restart systemd-zram-setup@zramN` or `systemctl restart systemd-zram-setup@*`
 may be used to recreate a specific device or all configured devices.
+(If the device didn't exist, `restart` will create it.)
 If the way the device is used (e.g. the mount point or file system type) is changed,
-`systemctl daemon-reload` needs to be called first to recreate `systemd` units.
+`systemctl daemon-reload` needs to be called first to recreate systemd units.
 If a device or mount point is removed from configuration, the unit should be stopped before calling `daemon-reload`.
 Otherwise, systemd will not know how to stop the unit properly.