On Thu, Mar 14, 2024 at 9:07 PM James Coleman <jtc...@gmail.com> wrote: > Obviously I have reasons for the other changes I made: for example, > "no longer visible" improves the correctness, since being an old > version isn't sufficient. I removed the "In summary" sentence because > it simply doesn't follow from the prose before it. That sentence > simply restates information already appearing earlier in almost as > simple a form, so it's redundant. But more importantly it's just not > actually a summary of the text before it, so removing it improves the > documentation. > > I can explain my reasoning further if desired, but I fear it would > simply frustrate you further, so I'll stop here. > > If the goal here is the most minimal patch possible, then please > commit what you proposed. I am interested in improving the document > further, but I don't know how to do that easily if the requirement is > effectively "must only change one specific detail at a time". So, that > leaves me feeling a bit frustrated also.
I don't think the goal is to have the most minimal patch possible, but at the same time, I'm responsible for what I commit. If I commit a patch that changes something and somebody, especially another committer, writes an email and says "hey, why the heck did you change this, you dummy?" then I need to be able to answer that question, and saying "uh, well, James Coleman had it in his patch and he didn't explain why it was there and I didn't know either but I just kind of committed it anyway" is not where I want to be. Almost without exception, it's not the patch author who gets asked why they included a certain thing in the patch; it's the committer who gets asked why it got committed that way -- and at least as I see it, if there's not a good answer to that question, it's the committer who gets judged, not the patch author. In some cases, such questions and judgements arrive without warning YEARS after the commit. So, to protect myself against questions from other hackers that end, at least implicitly, in "you dummy," I try to make sure there's an adequate paper trail for everything I commit. Between the commit message, the comments, and the email discussion, it needs to be crystal clear why something was thought to be a good idea at the time. Hopefully, it will be clear enough that I never even get a question, because the reader will be able to understand ON THEIR OWN why something was done and either go "oh, that make sense" or "well, I get why they did that, but I disagree because $REASON" ... but if that doesn't quite work out, then I hope that at the very least the paper trail will be good enough that I can reconstruct the reasoning if needed. For a recent example of a case where I clearly failed do a good enough job to keep someone from asking a "you dummy" question, see the http://postgr.es/m/6030bdec-0de1-4da2-b0b3-335007eae...@iki.fi and in particular the paragraph that starts with "However". Therefore, if I realize when reading the patch that it contains odd-looking changes which I can't relate to the stated goal, it's getting bounced, pretty much 100% of the time. If it's a small patch and I really care about it and it's a new submitter who doesn't know any better, I might choose to remove those changes myself and commit the rest; and if I like those changes for other reasons, I might choose to commit them separately. In any other situation, I'm just going to tell the submitter that they need to take out the unrelated changes and move on to the next email thread. In this particular situation, I see that this whole discussion is slightly moot by virtue of 7a9328e8e40534fb4de41b4ac152e3c6989d84e7 (Jeff Davis, 2024-03-05) which removed the "In summary, heap-only tuple updates can only be created if columns used by indexes are not updated" sentence that you also removed. But there is an important difference between that patch and yours, at least IMHO. You argue that the sentence in question didn't flow from what precedes it, but I don't agree with that as a rationale; I think that sentence flows perfectly well from what precedes it and is a generally adequate summary of the point of the section. That we disagree on the merits of that sentence is fine; there's no hope that we're all going to agree on everything. What makes me frustrated is that you didn't provide that rationale, which greatly complicates the whole discussion, because now I have to spend time getting you to either take it out or provide your justification before we can even reach the point of arguing about the substance. At least in my judgment, the patch Jeff committed doesn't have that problem, because the rationale that he gives in the commit message (partly by reference to another commit) is that heap-only tuples now can, in circumstances, be created even if columns used by indexes ARE updated. Based on that justification, it seems really clear to me that it was right to do SOMETHING to the sentence in question. Whether removing the sentence was better than some other alternative is debatable, and I might well have done something different, but between reading the commit message and reading the diff, it's 100% understandable to me what his reasoning was for every single byte that is in that diff. So if I love what he committed, cool! And if I hate what he committed, I'm well-placed to argue, because I know what I'm arguing against. Also, if I do feel like arguing, the fact that I know his reasoning will also make it a heck of a lot easier to come up with a proposal that meets my goals without undermining his. I can see, for example, that I can't simply propose to put that sentence back the way that it was; based on his reason for removing it, he obviously isn't going to like that. But if, say, my goal were to have some kind of a summary sentence there because I just thought that was really helpful, I could write a patch to insert a new summary sentence there and then say "hey, Jeff, you took this summary sentence out because it's not accurate, but here's a new one which I think is accurate and I'd like to add it back." Maybe he'd like that, and maybe he wouldn't, but it would have a shot, because it would take his reasoning into account, which I can do, because I know what it was. I want people who read the patches that I commit to have the same opportunities. -- Robert Haas EDB: http://www.enterprisedb.com
