Elijah Newren <newren@xxxxxxxxx> writes: > On Fri, Dec 18, 2020 at 1:45 PM Sergey Organov <sorganov@xxxxxxxxx> wrote: >> >> Elijah Newren <newren@xxxxxxxxx> writes: >> >> > On Fri, Dec 18, 2020 at 6:01 AM Sergey Organov <sorganov@xxxxxxxxx> wrote: >> >> >> >> Elijah Newren <newren@xxxxxxxxx> writes: >> >> >> >> > On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov >> >> > <sorganov@xxxxxxxxx> wrote: >> >> >> >> >> >> Move logic that handles implying -p on -c/--cc from >> >> >> log_setup_revisions_tweak() to diff_merges_setup_revs(), where it >> >> >> belongs. >> >> > >> >> > A very minor point, but I'd probably drop the "where it belongs"; >> >> > while I think the new place makes sense for it, it reads to me like >> >> > you're either relying on a consensus to move it or implying there was >> >> > a mistake to not put it here previously, neither of which makes sense. >> >> >> >> Well, it was meant to be an excuse for not moving it there earlier in >> >> the patch series indeed. I just overlooked this piece of code that >> >> logically belongs to the diff-merges module. I think you need to >> >> consider the state of the sources right before this patch to see the >> >> point of phrasing it like this. >> >> >> >> That said, I'm fine removing this either. >> > >> > If it should have been moved there earlier, then you should amend the >> > relevant previous commit instead of making a new one. rebase -i is >> > your friend and should be used, especially with long patch series. >> > :-) >> >> This is to be a separate commit anyway. I can move the commit itself >> more closer to the beginning, but I don't see how it'd make things >> any better. >> >> By "earlier" above I mostly meant that I should have noticed and moved >> it in the first issue or the patch series. > > Even if keeping the commit as-is, moving it earlier would have one benefit... > >> > >> >> > Much more importantly, this patch doesn't do what you said in >> >> > discussions on the previous round. It'd be helpful if the commit >> >> > message called out that you are just moving the logic for now and that >> >> > a subsequent patch will tweak the logic to only trigger this for >> >> > -c/--cc and not for --diff-merges=.* flags. >> >> >> >> I believe this patch is useful by itself, even without any future >> >> improvements (that we actually discussed), if any, so I don't see the >> >> point in describing what this patch doesn't do. >> >> >> >> OTOH, the commit message seems to be clear enough to expect this patch >> >> to be pure refactoring, without any functional changes, no? >> > >> > I'm just pointing out that reading the patch triggers a "wait, you >> > said you wanted to enable diffs for merges without diffs for regular >> > commits" reaction and makes reviewers start diving into the code to >> > check if they missed where that happened. Sometimes they'll even >> > respond to the commit asking about it...and then read a later patch >> > and find the answer. Perhaps I'm more attuned to this, because I've >> > done this to reviewers a number of times and they have asked me to add >> > a note in the earlier commit message to make it easier for other >> > reviewers to follow and read the series. You don't need to describe >> > in full detail the subsequent changes that will come, just highlight >> > that they are coming to give reviewers an aid. For example, this >> > could be as simple as: >> > >> > """ >> > Move logic that handles implying -p on -c/--cc from >> > log_setup_revisions_tweak() to diff_merges_setup_revs(). A >> > subsequent commit will tweak this logic further. >> > """ >> >> I think I see what you mean, but I still don't like this, sorry, as: >> >> First, this commit doesn't tweak the logic at all, so "further" doesn't >> sound right. > > Good point, I should have left off "further". > >> Second, the purpose of this move is not to have subsequent commits that >> will tweak this logic further in any particular way. One of the aims of >> this commit is rather to make it more simple to have /any/ further >> tweaks to the logic. >> >> Third, if the "tweak" you mention is not accepted, I'd need not to only >> get rid of the tweaking commit, but not to forget to edit the >> description of this one, that is basically unrelated? > > ...so, one advantage of moving this commit earlier in the series is > that if it appears before the introduction of --diff-merges, then it > doesn't trigger the "What? I thought we weren't making the > diff-merges flags trigger patches for non-merge commits" reaction, and > thus makes it clearer that the patch is just pure refactoring. > >> > >> > (Note that 'git log --grep=subsequent' in git.git will find you >> > several examples of where people have done this kind of thing.) >> >> Yeah, I agree it's useful when commits are tightly coupled and thus the >> purpose of single commit is unclear. I just don't think this one is such >> a case. > > I think where it appears in the series makes its purpose unclear. Fine, I'll move it earlier in the series then. Thanks, -- Sergey
