Pádraig Brady <[email protected]> writes: > That's quite a bit better. > > The logic in the patch looks sound. > I tried to make it more succinct but it looks optimal in that regard. > > A few remarks on the commit message. > > Generally the summary should describe the "why" not the "what". > So: "sort: prefer posix_spawnp to fork and execlp" would be better > as: "sort: use the more efficient posix_spawn to invoke helpers" > > Also "* NEWS: Mention the change." would be better > as: "* NEWS: Mention the improvement."
Ack. > I've attached a few naming tweaks your patch > and adjusted an error message slightly, > which you can squash in if you agree. Looks good to me. I was 50/50 on changing those names in my original patch. They aren't completely wrong, since we are still creating a new process with fork (or similar). But yours makes things a bit clearer. Thanks, Collin
