4 ms·

The prefix/postfix change is a red herring, that doesn't do anything (since the expression result isn't used). Which means that the security fix is interspersed

The prefix/postfix change is a red herring, that doesn't do anything (since the expression result isn't used). Which means that the security fix is interspersed with code style changes. Maybe there's a reason to do this here, but it's often not considered good practice because it makes it harder to review the code. AFAICT, the only real changes are in (new) line 411, resetting the cp variable after the loop, and line 416, ignoring write errors. The former is probably the relevant one.

MaulingMonkey 7y ago
198 also disables password feedback mode in !tty mode. So, style changes, behavior changes, and security fixes all in one changelist. This is a small enough diff that I wouldn't complain if given this to review - but I probably also would've reverted the style changes (and would give myself at least 50/50 odds of splitting the changelist in two) Perhaps I've just been too broken by 1000+ LOC diffs that intersperse multiple behavior changes and style and refactoring and ...
stanferder 7y ago
> The prefix/postfix change is a red herring, that doesn't do anything The prefix/postfix change does something to the reader. It's a distraction trying to figure out if that change was intended, whether the author knows it has no effect, or whether I'm wrong about the change having no effect. I appreciate the author doing the hard work of fixing this, especially with the code in full view of the public, but if I were an official reviewer I'd ask to get all the unnecessary changes removed.
nullc 7y ago
Better than OpenSSL putting out critical security fixes in a 400kloc diff...
kohtatsu 7y ago
PSA: git add -p and git commit -p exist and are beautiful.
- jwilk 7y ago
  FWIW, sudo uses Mercurial, not git. https://github.com/sudo-project/sudo https://github.com/sudo-project/sudo is just a mirror.
  - masklinn 7y ago
    Doesn't really make a difference, hg offers similar facilities (formerly the record extension, now hg commit -i). There's no index, but it's essentially the same as git commit -p.

MaulingMonkey 7y ago

198 also disables password feedback mode in !tty mode. So, style changes, behavior changes, and security fixes all in one changelist. This is a small enough diff that I wouldn't complain if given this to review - but I probably also would've reverted the style changes (and would give myself at least 50/50 odds of splitting the changelist in two) Perhaps I've just been too broken by 1000+ LOC diffs that intersperse multiple behavior changes and style and refactoring and ...

stanferder 7y ago

> The prefix/postfix change is a red herring, that doesn't do anything The prefix/postfix change does something to the reader. It's a distraction trying to figure out if that change was intended, whether the author knows it has no effect, or whether I'm wrong about the change having no effect. I appreciate the author doing the hard work of fixing this, especially with the code in full view of the public, but if I were an official reviewer I'd ask to get all the unnecessary changes removed.

nullc 7y ago

Better than OpenSSL putting out critical security fixes in a 400kloc diff...

kohtatsu 7y ago

PSA: git add -p and git commit -p exist and are beautiful.

jwilk 7y ago
FWIW, sudo uses Mercurial, not git. https://github.com/sudo-project/sudo https://github.com/sudo-project/sudo is just a mirror.
- masklinn 7y ago
  Doesn't really make a difference, hg offers similar facilities (formerly the record extension, now hg commit -i). There's no index, but it's essentially the same as git commit -p.