Re: [PATCH v2 24/33] diff-merges: handle imply -p on -c/--cc logic for log.c

From: Sergey Organov <sorganov@gmail.com>
To: Elijah Newren <newren@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	Philip Oakley <philipoakley@iee.email>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH v2 24/33] diff-merges: handle imply -p on -c/--cc logic for log.c
Date: Sat, 19 Dec 2020 01:17:03 +0300	[thread overview]
Message-ID: <87k0te7o9s.fsf@osv.gnss.ru> (raw)
In-Reply-To: <CABPp-BG-Nv=pzwTO3J0OK20gFVV67_qFL+m-cL7d9xrMvTjH-Q@mail.gmail.com> (Elijah Newren's message of "Fri, 18 Dec 2020 14:12:46 -0800")

Elijah Newren <newren@gmail.com> writes:

> On Fri, Dec 18, 2020 at 1:45 PM Sergey Organov <sorganov@gmail.com> wrote:
>>
>> Elijah Newren <newren@gmail.com> writes:
>>
>> > On Fri, Dec 18, 2020 at 6:01 AM Sergey Organov <sorganov@gmail.com> wrote:
>> >>
>> >> Elijah Newren <newren@gmail.com> writes:
>> >>
>> >> > On Wed, Dec 16, 2020 at 10:50 AM Sergey Organov
>> >> > <sorganov@gmail.com> 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