Reducing kernel-maintainer burnout

Posted Nov 25, 2023 20:33 UTC (Sat) by viro (subscriber, #7872)
In reply to: Reducing kernel-maintainer burnout by roc
Parent article: Reducing kernel-maintainer burnout

Save the email in question into a (local) mailblox, then git am -s mailbox_file_name and you are done. If you want it as a merge, start a new branch, git am into it, then git merge. Or, if you are asked to pull from <branch name> in <repo>, you just say git pull <repo> <branch name>, assuming you can manage a cut and paste between your mail reader and your shell...

In any case, you are supposed to read and review the stuff in the branch; that's, er, somewhat more mentally taxing effort than any of the above.

As for the editors, I'm yet to find one in a browser that wouldn't be atrocious; used to be possible to have the damn thing start xterm and a sane editor in it, but that got broken. And yes, I'm using a browser to type that comment in - and hating every minute of it. For anything that requires minimally non-trivial manipulations of text (like, you know, search and replace with regular expressions, or pipe a block of text through a filter - trivial things like that) it would be extremely painful.

Reducing kernel-maintainer burnout

Posted Nov 25, 2023 22:47 UTC (Sat) by laurent.pinchart (subscriber, #71290) [Link] (2 responses)

> As for the editors, I'm yet to find one in a browser that wouldn't be atrocious;

I stopped counting the number of times I pressed ctrl-w to delete the word I had just typed in a browser-based text editor. Just thinking about it makes me want to scream.

Reducing kernel-maintainer burnout

Posted Nov 26, 2023 2:59 UTC (Sun) by Cyberax (✭ supporter ✭, #52523) [Link] (1 responses)

There are several extensions for FF (and probably Chrome) that override the hotkeys.

Reducing kernel-maintainer burnout

Posted Nov 26, 2023 17:13 UTC (Sun) by gray_-_wolf (subscriber, #131074) [Link]

> There are several extensions for FF (and probably Chrome) that override the hotkeys.

But those (as far as I know) do not (and cannot) work on privileged pages (about:config, addons.mozzila.org, ...). Which is annoying.

What did work for me is to just build my own firefox with modified key bindings: https://git.sr.ht/~graywolf/gecko-dev/commit/7648188f9751... . I assume it is possible without the compilation, but I found now documentation for it. And I am rebuilding anyway...

It can be combined with following in .config/gtk-3.0/gtk.css to get actual C-w working everywhere:

@binding-set text-entry
{
bind "<Control>w"
{
"delete-from-cursor" (word-ends, -1)
};
}

entry { -gtk-key-bindings: text-entry; }
textview { -gtk-key-bindings: text-entry; }

Offtopic: I also prefer SHIFT+INSERT to paste PRIMARY instead of CLIPBOARD, so https://git.sr.ht/~graywolf/gecko-dev/commit/46b121a7a550... is nice as well (albeit bit hackish).

Reducing kernel-maintainer burnout

Posted Nov 26, 2023 13:03 UTC (Sun) by kleptog (subscriber, #1183) [Link] (8 responses)

> Save the email in question into a (local) mailblox, then git am -s mailbox_file_name and you are done. If you want it as a merge, start a new branch, git am into it, then git merge.

Maybe it's just me, but the idea I actually need to apply a patch to a local branch to review it seems, well, suboptimal. After all, I may be managing several projects, and many patches and having to manage all those branches just seems like unnecessary work. Compared to something like Gerrit where the patch is already applied, I can simply view the change and move through the diff and make comments directly just seems much easier. Patches that don't apply cleanly, or fail the build step are already filtered out for you.

With email patches, rebase failures are made the problem of the maintainer rather than the submitter, and that just seems wrong. If I actually want to try out the patch, I can just pull the version from Gerrit directly.

> For anything that requires minimally non-trivial manipulations of text (like, you know, search and replace with regular expressions, or pipe a block of text through a filter - trivial things like that) it would be extremely painful.

Why on earth would you want search/replace while entering a comment? If you're actually want to propose a change to the patch itself, you can edit it within the browser itself (VSCode can be embedded anywhere AIUI)

Reducing kernel-maintainer burnout

Posted Nov 27, 2023 4:58 UTC (Mon) by viro (subscriber, #7872) [Link]

git am is when you are sent patches over email; if you are given a branch + repo (in the same email) - use git pull.

If patches are trivial and don't need me to look outside of context diff, sure, I can reply directly to email. What's the benefit of web-based anything in that respect, anyway? For anything more involved I very much prefer to be able to do it in a real editor, where I can poke around the tree, etc. Can you do the same in browser? As in, something in the patch gets me to look for the places where this field of this structure is ever accessed, to verify that their locking is, indeed, sufficient. Or something like "their change seems to remove a bunch of calls of that primitive; how many are left afterwards? If there's none, this and that could've been done much simpler and cut down the complexity of patch series big way... <greps> Umm... only two callers left; let's look around and see if they can be massaged out of existence as a preliminary step - looks like that might be a serious win overall". Or writing an explanation that would be longer than a few lines, with e.g. pseudo-code snippets in it. Or any number of other things, really...

As for rebase failures - huh? If I don't know which version the series is based on, how the hell could I review it in the first place, not to mention pulling it in? If the patches do not apply, you ask the submitter what are they supposed to be against; if they can't tell that, you don't want to deal with the series anyway. Oh, and throwaway branches are not a problem to remove, you know...

Reducing kernel-maintainer burnout

Posted Nov 27, 2023 12:19 UTC (Mon) by farnz (subscriber, #17727) [Link]

FWIW, I use local branches for review all the time with web-based review systems like Gerrit and GitHub PRs. It's just convenient to have the full power of my local setup available for reviewing the code, looking at what things outside the change are doing (which in turn allows me to suggest helpful follow-ups). git makes it trivial to check out a branch or commit and look at it, to try out changes I'm going to suggest (complete with building and/or testing them).

And git worktree add makes it trivial to create a new working copy for the duration of one review, which I can then delete after I'm done.

Reducing kernel-maintainer burnout

Posted Nov 27, 2023 18:43 UTC (Mon) by wtarreau (subscriber, #51152) [Link] (5 responses)

> Why on earth would you want search/replace while entering a comment?

This question is funny! This happens to be all the time: just because your ideas develop as you review while you're commenting. You start by suggesting something and you change your mind in the middle for something better, so you just want to s/foo/bar/g to replace all the variables prefixes. And similarly, wanting to just indent a block to put an "if" clause around is super frequent when reviewing. All things that browsers make impossible to do without having to press space 100 times (since tab will get you out of the box and if you're unlucky you'll even press "Post" while touching the space bar). I, too, despise every single minute I type in a browser. These tools are entirely designed to make you consume contents, not produce them.

Reducing kernel-maintainer burnout

Posted Nov 29, 2023 12:26 UTC (Wed) by kleptog (subscriber, #1183) [Link] (4 responses)

> You start by suggesting something and you change your mind in the middle for something better, so you just want to s/foo/bar/g to replace all the variables prefixes.

I guess we mean different things by reviewing. Reviewing for me does not involve writing any code, the idea is to provide the submitter enough information to fix it themselves and resubmit.

Reducing kernel-maintainer burnout

Posted Nov 29, 2023 14:42 UTC (Wed) by wtarreau (subscriber, #51152) [Link]

> Reviewing for me does not involve writing any code, the idea is to provide the submitter enough information to fix it themselves and resubmit.

It's often much more efficient (and friendly) to write "I think maybe a better approach could be something around this:" and give the start of an example, than "no, still not what I'm expecting, try again". Look at reviews done on LKML, a non-negligible number of them suggest code snippets to discuss around. This is particularly true for bug fixes, where it's often a tradeoff and multiple approaches need to be explorated. That's precisely one of the reasons some tools are quite bad at this, when they don't allow the reviewer to express what they're thinking about, and cause many round trips.

Reducing kernel-maintainer burnout

Posted Nov 29, 2023 20:34 UTC (Wed) by viro (subscriber, #7872) [Link]

Alas, some of us just can't aspire to such heights of managerial Tao and feel an occasional urge to explain things, even at the expense of sullying the proper forms with actual details. Or believe that Lao Tzu had been taking the piss and those who take his teachings too seriously are utter wankers....

Reducing kernel-maintainer burnout

Posted Nov 30, 2023 11:48 UTC (Thu) by farnz (subscriber, #17727) [Link]

I often find myself wanting to quickly confirm that a suggestion I have is workable before I make it - I'm not infallible, but I can quickly write a small amount of code (ignoring error checking, ripple effects outside the area I'm changing, good style etc) to see what my suggestion will do to the code I'm reviewing. If I like what I see, I'll make the suggestion, while if I see that it's actually obfuscating the code, I'll not suggest it.

Reducing kernel-maintainer burnout

Posted Dec 4, 2023 14:02 UTC (Mon) by Hattifnattar (subscriber, #93737) [Link]

I would hate to get a review that just said "this line is wrong" without giving any reason, or "this function can be made more efficient" without any suggestion. Do you expect me to read your mind?

And a reason or a suggestion often involve, surprise-surprise, code.

Reviewing is just a conversation on a specific topic. The topic being code, it's entirely reasonable that the conversation will include code fragments...

Reducing kernel-maintainer burnout

Posted Nov 26, 2023 13:41 UTC (Sun) by nevets (subscriber, #11875) [Link] (1 responses)

I usually just cut and paste text out of a browser and into an editor, make my changes, then cut and paste it back into the browser.

Reducing kernel-maintainer burnout

Posted Nov 27, 2023 5:08 UTC (Mon) by viro (subscriber, #7872) [Link]

Cut'n'paste of more than a screenful tends to be unpleasant, and there's whitespace/tab/etc. damage to deal with. It's all survivable, of course, but really unpleasant.

Reducing kernel-maintainer burnout

Posted Dec 26, 2024 12:00 UTC (Thu) by alx.manpages (subscriber, #145117) [Link]

> Save the email in question into a (local) mailblox, then git am -s mailbox_file_name and you are done.

Actually, you can optimize that a little bit:

In neomutt(1)/mutt(1), you can press '|' within a message to pipe it to a command, which can be 'git am -s'. So, when reviewing patches for a project, I cd(1) into the worktree where I apply patches from contributors, then read mail, and from mail directly `|git am -s`, omitting the "save" step. You can even do that with encrypted mail (with a configuration line to tell it to decrypt while piping).

I expect other MUAs should have similar abilities.

BTW, another advantage of email:

You can use PGP to sign and/or encrypt the email.