2 ms·

Deleting three lines to keep both comments doesn’t seem too bad. You might even just delete your comment entirely if it’s for the same thing.

palata 3y ago
I completely agree, if you don't make many reviews and if it does not happen often. But still it puts the burden on the reviewer rather than on the patch author. And the reviewer's time is never less precious. If you need a review from Linus Torvalds, I think it makes sense to make it as seamless as possible for him. And that is what the e-mail workflow does, I would say.
- withinboredom 3y ago
  It depends on what you value in a review. For me, a review is for me to get a glimpse at new parts of the code and learn what is coming. It is important that I understand the WHY more than the WHAT. I also put code reviews as the highest priority item in my todo list. I will always do a code review next, if there is one to do. If you send me a request, you'll have a review from me before the end of my day. Why? Because unblocking my teammates is the most important thing I can do, and I expect the same from my teammates. I never want the reason our team is late to be "I had more important things to do" because (cliché incoming), "the is no 'I' in 'team'". A reviewers time isn't more valuable than mine, and my time is no more valuable than anyone else (assuming accountants don't exist). If changing 3 lines is really an issue, then create a merge-tool for this workflow and add it to your git configuration file.
  - palata 3y ago
    Sure, but what if your lead has a review list so big that they almost never get to empty it totally? I would consider that the time of such a lead is definitely more valuable than mine. Again, it's easy for me to accept that Linus Torvalds' time is more valuable than mine. > If changing 3 lines is really an issue, then create a merge-tool for this workflow and add it to your git configuration file. Or use the e-mail workflow, where the lead can just comment on the patch right away, without having to even pull the branch (and worse, push comment and potentially fix conflicts), right?
    - withinboredom 3y ago
      I understood that the presented workflow meant that it had already been reviewed, it just needed to be merged. But on a team, I’d say there is no Most Valuable Player.

palata 3y ago

I completely agree, if you don't make many reviews and if it does not happen often. But still it puts the burden on the reviewer rather than on the patch author. And the reviewer's time is never less precious. If you need a review from Linus Torvalds, I think it makes sense to make it as seamless as possible for him. And that is what the e-mail workflow does, I would say.

withinboredom 3y ago
It depends on what you value in a review. For me, a review is for me to get a glimpse at new parts of the code and learn what is coming. It is important that I understand the WHY more than the WHAT. I also put code reviews as the highest priority item in my todo list. I will always do a code review next, if there is one to do. If you send me a request, you'll have a review from me before the end of my day. Why? Because unblocking my teammates is the most important thing I can do, and I expect the same from my teammates. I never want the reason our team is late to be "I had more important things to do" because (cliché incoming), "the is no 'I' in 'team'". A reviewers time isn't more valuable than mine, and my time is no more valuable than anyone else (assuming accountants don't exist). If changing 3 lines is really an issue, then create a merge-tool for this workflow and add it to your git configuration file.
- palata 3y ago
  Sure, but what if your lead has a review list so big that they almost never get to empty it totally? I would consider that the time of such a lead is definitely more valuable than mine. Again, it's easy for me to accept that Linus Torvalds' time is more valuable than mine. > If changing 3 lines is really an issue, then create a merge-tool for this workflow and add it to your git configuration file. Or use the e-mail workflow, where the lead can just comment on the patch right away, without having to even pull the branch (and worse, push comment and potentially fix conflicts), right?
  - withinboredom 3y ago
    I understood that the presented workflow meant that it had already been reviewed, it just needed to be merged. But on a team, I’d say there is no Most Valuable Player.