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. > >> > 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. 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? > > (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. Thanks, -- Sergey
