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. :-) > > 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. """ (Note that 'git log --grep=subsequent' in git.git will find you several examples of where people have done this kind of thing.)
