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.
