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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux