| Message ID | 20201216184929.3924-25-sorganov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-log: implement new --diff-merge options | expand |
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. 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. > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > builtin/log.c | 4 ---- > diff-merges.c | 7 ++++++- > 2 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/builtin/log.c b/builtin/log.c > index 63875c3aeec9..c3caf0955b2b 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -723,10 +723,6 @@ static void log_setup_revisions_tweak(struct rev_info *rev, > rev->prune_data.nr == 1) > rev->diffopt.flags.follow_renames = 1; > > - /* Turn --cc/-c into -p --cc/-c when -p was not given */ > - if (!rev->diffopt.output_format && rev->combine_merges) > - rev->diffopt.output_format = DIFF_FORMAT_PATCH; > - > if (rev->first_parent_only) > diff_merges_default_to_first_parent(rev); > } > diff --git a/diff-merges.c b/diff-merges.c > index 0165fa22fcd1..2ac25488d53e 100644 > --- a/diff-merges.c > +++ b/diff-merges.c > @@ -127,6 +127,11 @@ void diff_merges_setup_revs(struct rev_info *revs) > revs->first_parent_merges = 0; > if (revs->combined_all_paths && !revs->combine_merges) > die("--combined-all-paths makes no sense without -c or --cc"); > - if (revs->combine_merges) > + if (revs->combine_merges) { > revs->diff = 1; > + /* Turn --cc/-c into -p --cc/-c when -p was not given */ > + if (!revs->diffopt.output_format) > + revs->diffopt.output_format = DIFF_FORMAT_PATCH; > + } > + > } > -- > 2.25.1 >
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. > 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? Thanks, -- Sergey > > >> Signed-off-by: Sergey Organov <sorganov@gmail.com> >> --- >> builtin/log.c | 4 ---- >> diff-merges.c | 7 ++++++- >> 2 files changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/builtin/log.c b/builtin/log.c >> index 63875c3aeec9..c3caf0955b2b 100644 >> --- a/builtin/log.c >> +++ b/builtin/log.c >> @@ -723,10 +723,6 @@ static void log_setup_revisions_tweak(struct rev_info *rev, >> rev->prune_data.nr == 1) >> rev->diffopt.flags.follow_renames = 1; >> >> - /* Turn --cc/-c into -p --cc/-c when -p was not given */ >> - if (!rev->diffopt.output_format && rev->combine_merges) >> - rev->diffopt.output_format = DIFF_FORMAT_PATCH; >> - >> if (rev->first_parent_only) >> diff_merges_default_to_first_parent(rev); >> } >> diff --git a/diff-merges.c b/diff-merges.c >> index 0165fa22fcd1..2ac25488d53e 100644 >> --- a/diff-merges.c >> +++ b/diff-merges.c >> @@ -127,6 +127,11 @@ void diff_merges_setup_revs(struct rev_info *revs) >> revs->first_parent_merges = 0; >> if (revs->combined_all_paths && !revs->combine_merges) >> die("--combined-all-paths makes no sense without -c or --cc"); >> - if (revs->combine_merges) >> + if (revs->combine_merges) { >> revs->diff = 1; >> + /* Turn --cc/-c into -p --cc/-c when -p was not given */ >> + if (!revs->diffopt.output_format) >> + revs->diffopt.output_format = DIFF_FORMAT_PATCH; >> + } >> + >> } >> -- >> 2.25.1 >>
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. :-) > > 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.)
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. > >> > 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
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.
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
diff --git a/builtin/log.c b/builtin/log.c index 63875c3aeec9..c3caf0955b2b 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -723,10 +723,6 @@ static void log_setup_revisions_tweak(struct rev_info *rev, rev->prune_data.nr == 1) rev->diffopt.flags.follow_renames = 1; - /* Turn --cc/-c into -p --cc/-c when -p was not given */ - if (!rev->diffopt.output_format && rev->combine_merges) - rev->diffopt.output_format = DIFF_FORMAT_PATCH; - if (rev->first_parent_only) diff_merges_default_to_first_parent(rev); } diff --git a/diff-merges.c b/diff-merges.c index 0165fa22fcd1..2ac25488d53e 100644 --- a/diff-merges.c +++ b/diff-merges.c @@ -127,6 +127,11 @@ void diff_merges_setup_revs(struct rev_info *revs) revs->first_parent_merges = 0; if (revs->combined_all_paths && !revs->combine_merges) die("--combined-all-paths makes no sense without -c or --cc"); - if (revs->combine_merges) + if (revs->combine_merges) { revs->diff = 1; + /* Turn --cc/-c into -p --cc/-c when -p was not given */ + if (!revs->diffopt.output_format) + revs->diffopt.output_format = DIFF_FORMAT_PATCH; + } + }
Move logic that handles implying -p on -c/--cc from log_setup_revisions_tweak() to diff_merges_setup_revs(), where it belongs. Signed-off-by: Sergey Organov <sorganov@gmail.com> --- builtin/log.c | 4 ---- diff-merges.c | 7 ++++++- 2 files changed, 6 insertions(+), 5 deletions(-)