| Message ID | 20221127093721.31012-1-sorganov@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | diff-merges: more features | expand |
Sergey Organov <sorganov@gmail.com> writes: > 2. log.diffMerges-m-imply-p > > Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc' > options do. Simply fixing this inconsistency by unconditional > modification of '-m' semantics appeared to be a bad idea, as it broke > some legacy scripts/aliases. This patch rather provides configuration > variable to tweak '-m' behavior accordingly. I do not know how this can be a good idea. For those users who set the configuration variables, those scripts and aliases get broken anyway, don't they? IOW, I am not sure how this is better than the "modification of '-m'" that is "a bad idea". I would understand why it may be a safer and more sensible solution, if the proposed approach were to find an unused letter $X and to introduce "-$X" that is the same as "-m" but implies "-p", though. > 3. log.diffMergesForce > > Force specific log format for -c, --cc, and --remerge-diff options > instead of their respective formats. The override is useful when some > external tool hard-codes diff for merges format option. Not convinced it is a good idea for the same reason as above (not convinced it is a bad idea, either, though). > 4. Support list of values for --diff-merges > > This allows for shorter --diff-merges=on,hide forms. Good, probably.
Junio C Hamano <gitster@pobox.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> 2. log.diffMerges-m-imply-p >> >> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc' >> options do. Simply fixing this inconsistency by unconditional >> modification of '-m' semantics appeared to be a bad idea, as it broke >> some legacy scripts/aliases. This patch rather provides configuration >> variable to tweak '-m' behavior accordingly. > > I do not know how this can be a good idea. For those users who set > the configuration variables, those scripts and aliases get broken > anyway, don't they? IOW, I am not sure how this is better than the > "modification of '-m'" that is "a bad idea". The history behind this is that before the patch #1 of these series there was no way to get exact semantics of current '-m' option using new --diff-merges option, so there was no sensible way to "fix" these scripts/aliases. That was in fact the primary objection for the new '-m' semantics. Now, when one stomps on such script/alias (by explicitly enabling the new configuration option), they can fix it by replacing '-m' with '--diff-merges=on,hide', that, along with the last patch of these series that produces warning when lone '-m' is detected, looks to me as a way to eventually get rid of the legacy and surprising '-m' semantics. > I would understand why it may be a safer and more sensible solution, > if the proposed approach were to find an unused letter $X and to > introduce "-$X" that is the same as "-m" but implies "-p", though. I've in fact considered this as well, and '-d' was free for such use last time I've checked. It was very tempting to just add '-d' that always shows diff to first parent for everything, be it merge or not, and call it a day, but it won't fix '-m', which inconsistent behavior still surprises people, and besides started the whole --diff-merges business in the first place. > >> 3. log.diffMergesForce >> >> Force specific log format for -c, --cc, and --remerge-diff options >> instead of their respective formats. The override is useful when some >> external tool hard-codes diff for merges format option. > > Not convinced it is a good idea for the same reason as above (not > convinced it is a bad idea, either, though). Here the intention was entirely different though. I'm using magit and it does hard-code --cc in one place that I'd like to be able to override. If I got this problem, I figured chances are high somebody else will get it as well, and that's the rationale. Then I figure everybody here has own favorite format for merge commits, and this toy gives you exactly this: whatever option is used (once), you get what you prefer instead, or use an option second time to enforce it. For stable scripting, we for rather long time now have --diff-merges=c, and --diff-merges=cc, as well as slightly more recent --diff-merges=remerge, that are not affected by the configuration in question. That said, it looks useful to me, but I won't insist on the feature should you guys object. > >> 4. Support list of values for --diff-merges >> >> This allows for shorter --diff-merges=on,hide forms. > > Good, probably. Was useless before 'hide' though, as the rest of options just override each other entirely. Thanks, -- Sergey Organov
On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote: > > 1. --diff-merges=[no]-hide This seems problematic to me. Currently, all the options to diff-merges are exclusive of each other; the user is picking one of them to determine how to format diffs for merges. Now you are introducing the ability to combine various options, leading users to think that perhaps they can run with all three of `--diff-merges=combined-dense --diff-merges=remerge --diff-merges=separate` or other nonsensical combinations. Shouldn't this [no-]hide stuff be a separate flag rather than reusing --diff-merges? > The set of diff-merges options happened to be incomplete, and failed > to implement exact semantics of -m option that hides output of diffs > for merge commits unless -p option is active as well. > > The new "hide" option fixes this issue, so that now > > --diff-merges=on --diff-merges=hide > > combo is the exact synonym for -m. Why is completeness important here? Perhaps I should state this another way: when would users ever want to use this new "hide" option? I got through your cover letter not knowing the answer to this, but was hoping it'd at least be covered in one of your commit messages or documentation changes. Maybe it was there, but I somehow missed it. Is the only goal some sense of developer completeness for these options, or are these end-user-facing options of utility to actual end users? I'm hoping the latter, but if so, can that be documented and explained somewhere? I'm pretty sure this is explained somewhere in an old mailing list discussion, but where? > The log.diffMerges configuration also accepts "hide" and "no-hide" > values, and governs the default value for the hide bit. The > configuration behaves as if "--diff-merges=[no-]hide" is inserted > first in the command-line. > > 2. log.diffMerges-m-imply-p > > Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc' > options do. Simply fixing this inconsistency by unconditional > modification of '-m' semantics appeared to be a bad idea, as it broke > some legacy scripts/aliases. This patch rather provides configuration > variable to tweak '-m' behavior accordingly. > 3. log.diffMergesForce > > Force specific log format for -c, --cc, and --remerge-diff options > instead of their respective formats. The override is useful when some > external tool hard-codes diff for merges format option. Why just these three options and not -m (or --diff-merges=separate)? Also, I read this and didn't quite fully grasp the intent; your explanation in response to Junio seemed much more enlightening. Perhaps the wording/explanation could be cleaned up a bit? I'll comment more on that specific patch... > 4. Support list of values for --diff-merges > > This allows for shorter --diff-merges=on,hide forms. And thus making users think they can pass --diff-merges=combined-dense,remerge,separate and suspecting that it'll do something useful? Seems like this is reinforcing a mistake to me. > 5. Issue warning for lone '-m'. > > Lone '-m' is in use by scripts/aliases that aim at enabling diff > output for merge commits, but only if '-p' is then specified as well. > > As '-m' may now be configured to imply '-p' (using > 'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified, > and suggest to instead use '--diff-merges=on,hide' that does not > depend on user configuration. > > This is expected to give a provision for enabling > log.diffMerges-m-imply-p by default in the future. > > Sergey Organov (5): > diff-merges: implement [no-]hide option and log.diffMergesHide config > diff-merges: implement log.diffMerges-m-imply-p config > diff-merges: implement log.diffMergesForce config > diff-merges: support list of values for --diff-merges > diff-merges: issue warning on lone '-m' option > > Documentation/config/log.txt | 20 ++++ > Documentation/diff-options.txt | 20 +++- > builtin/log.c | 6 + > diff-merges.c | 108 +++++++++++++++--- > diff-merges.h | 6 + > t/t4013-diff-various.sh | 89 ++++++++++++++- > ...ges=first-parent_--diff-merges=hide_master | 34 ++++++ > t/t9902-completion.sh | 9 ++ > 8 files changed, 272 insertions(+), 20 deletions(-) > create mode 100644 t/t4013/diff.log_--diff-merges=first-parent_--diff-merges=hide_master > > -- > 2.37.3.526.g5f84746cb16b >
Elijah Newren <newren@gmail.com> writes: > On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> 1. --diff-merges=[no]-hide > > This seems problematic to me. Currently, all the options to > diff-merges are exclusive of each other; the user is picking one of > them to determine how to format diffs for merges. Now you are > introducing the ability to combine various options, leading users to > think that perhaps they can run with all three of > `--diff-merges=combined-dense --diff-merges=remerge > --diff-merges=separate` or other nonsensical combinations. Shouldn't > this [no-]hide stuff be a separate flag rather than reusing > --diff-merges? Yes, it's a precedent indeed, but I don't see any actual problem here. Unlike git silently dropping changes on rebase, this can cause no damage. I think I can emphasize that we now have "formats" and "flags" in the documentation, where "formats" are mutually exclusive (the latest specified wins), while "flags" are cumulative. > >> The set of diff-merges options happened to be incomplete, and failed >> to implement exact semantics of -m option that hides output of diffs >> for merge commits unless -p option is active as well. >> >> The new "hide" option fixes this issue, so that now >> >> --diff-merges=on --diff-merges=hide >> >> combo is the exact synonym for -m. > > Why is completeness important here? Perhaps I should state this > another way: when would users ever want to use this new "hide" option? > I got through your cover letter not knowing the answer to this, but > was hoping it'd at least be covered in one of your commit messages or > documentation changes. Maybe it was there, but I somehow missed it. > > Is the only goal some sense of developer completeness for these > options, or are these end-user-facing options of utility to actual end > users? I'm hoping the latter, but if so, can that be documented and > explained somewhere? I'm pretty sure this is explained somewhere in > an old mailing list discussion, but where? Completeness is essential as I want '--diff-merges' to provide all the needed capabilities, and one of them was actually missing, that is there in the '-m' semantics, exactly as I said in the descriptions. > >> The log.diffMerges configuration also accepts "hide" and "no-hide" >> values, and governs the default value for the hide bit. The >> configuration behaves as if "--diff-merges=[no-]hide" is inserted >> first in the command-line. >> >> 2. log.diffMerges-m-imply-p >> >> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc' >> options do. Simply fixing this inconsistency by unconditional >> modification of '-m' semantics appeared to be a bad idea, as it broke >> some legacy scripts/aliases. This patch rather provides configuration >> variable to tweak '-m' behavior accordingly. > >> 3. log.diffMergesForce >> >> Force specific log format for -c, --cc, and --remerge-diff options >> instead of their respective formats. The override is useful when some >> external tool hard-codes diff for merges format option. > > Why just these three options and not -m (or --diff-merges=separate)? As I said in my answer to your other mail, '-m' is already configurable, so it is not needed to be included. None of --diff-merges= options are affected by diffMergesForce, only 3 specific options from the documentation. > > Also, I read this and didn't quite fully grasp the intent; your > explanation in response to Junio seemed much more enlightening. > Perhaps the wording/explanation could be cleaned up a bit? I'll > comment more on that specific patch... Yeah, thanks, I got your suggestion you put in another mail. > >> 4. Support list of values for --diff-merges >> >> This allows for shorter --diff-merges=on,hide forms. > > And thus making users think they can pass > --diff-merges=combined-dense,remerge,separate and suspecting that > it'll do something useful? Seems like this is reinforcing a mistake > to me. Yes, they can. For now it's useful only for 'hide', but we might add more flags in the future. It's also harmless, so I don't see it as a serious issue. Thanks, -- Sergey Organov
On Wed, Nov 30, 2022 at 5:16 AM Sergey Organov <sorganov@gmail.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote: > >> > >> 1. --diff-merges=[no]-hide > > > > This seems problematic to me. Currently, all the options to > > diff-merges are exclusive of each other; the user is picking one of > > them to determine how to format diffs for merges. Now you are > > introducing the ability to combine various options, leading users to > > think that perhaps they can run with all three of > > `--diff-merges=combined-dense --diff-merges=remerge > > --diff-merges=separate` or other nonsensical combinations. Shouldn't > > this [no-]hide stuff be a separate flag rather than reusing > > --diff-merges? > > Yes, it's a precedent indeed, but I don't see any actual problem here. > Unlike git silently dropping changes on rebase, this can cause no > damage. Sure, read-only options for querying things won't cause future damage. That doesn't mean that the UI for commands like diff/log/grep/etc are unimportant, though, and certainly doesn't excuse intentionally creating bad UI for them. > I think I can emphasize that we now have "formats" and "flags" > in the documentation, where "formats" are mutually exclusive (the latest > specified wins), while "flags" are cumulative. Why not just give it a different flag name, so that "formats" and "flags" are clearly separated without even needing a lengthy explanation? That'd be much simpler to understand and explain. > >> The set of diff-merges options happened to be incomplete, and failed > >> to implement exact semantics of -m option that hides output of diffs > >> for merge commits unless -p option is active as well. > >> > >> The new "hide" option fixes this issue, so that now > >> > >> --diff-merges=on --diff-merges=hide > >> > >> combo is the exact synonym for -m. > > > > Why is completeness important here? Perhaps I should state this > > another way: when would users ever want to use this new "hide" option? > > I got through your cover letter not knowing the answer to this, but > > was hoping it'd at least be covered in one of your commit messages or > > documentation changes. Maybe it was there, but I somehow missed it. > > > > Is the only goal some sense of developer completeness for these > > options, or are these end-user-facing options of utility to actual end > > users? I'm hoping the latter, but if so, can that be documented and > > explained somewhere? I'm pretty sure this is explained somewhere in > > an old mailing list discussion, but where? > > Completeness is essential as I want '--diff-merges' to provide all the > needed capabilities, and one of them was actually missing, that is there > in the '-m' semantics, exactly as I said in the descriptions. I ask you why a user would want to use this option, and you simply assert that it's a "needed capabilit[y]"? Could you explain *why* it's needed or helpful for users instead of just repeating the assertion that it is needed? If I can't figure out why it's needed or useful for users despite having read your cover letter, commit messages, underlying source code and documentation, and this full thread, then there may well be something wrong with me...but it seems likely that many users will also have difficulty figuring out why this option is useful.
Elijah Newren <newren@gmail.com> writes: > On Wed, Nov 30, 2022 at 5:16 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> Elijah Newren <newren@gmail.com> writes: >> >> > On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@gmail.com> wrote: >> >> >> >> 1. --diff-merges=[no]-hide >> > >> > This seems problematic to me. Currently, all the options to >> > diff-merges are exclusive of each other; the user is picking one of >> > them to determine how to format diffs for merges. Now you are >> > introducing the ability to combine various options, leading users to >> > think that perhaps they can run with all three of >> > `--diff-merges=combined-dense --diff-merges=remerge >> > --diff-merges=separate` or other nonsensical combinations. Shouldn't >> > this [no-]hide stuff be a separate flag rather than reusing >> > --diff-merges? >> >> Yes, it's a precedent indeed, but I don't see any actual problem here. >> Unlike git silently dropping changes on rebase, this can cause no >> damage. > > Sure, read-only options for querying things won't cause future damage. > That doesn't mean that the UI for commands like diff/log/grep/etc are > unimportant, though, and certainly doesn't excuse intentionally > creating bad UI for them. I just don't see it as a bad UI, sorry. > >> I think I can emphasize that we now have "formats" and "flags" >> in the documentation, where "formats" are mutually exclusive (the latest >> specified wins), while "flags" are cumulative. > > Why not just give it a different flag name, so that "formats" and > "flags" are clearly separated without even needing a lengthy > explanation? That'd be much simpler to understand and explain. What I did is the best I was able to think of. The --diff-merges introduces namespace for everything related to formats of output of diffs for merge commits, and 'hide' fits there perfectly. User doesn't need to look elsewhere to figure entire set of capabilities. I honestly don't see how to improve the UI by introducing yet another option, especially provided the letter has its own drawbacks. > >> >> The set of diff-merges options happened to be incomplete, and failed >> >> to implement exact semantics of -m option that hides output of diffs >> >> for merge commits unless -p option is active as well. >> >> >> >> The new "hide" option fixes this issue, so that now >> >> >> >> --diff-merges=on --diff-merges=hide >> >> >> >> combo is the exact synonym for -m. >> > >> > Why is completeness important here? Perhaps I should state this >> > another way: when would users ever want to use this new "hide" option? >> > I got through your cover letter not knowing the answer to this, but >> > was hoping it'd at least be covered in one of your commit messages or >> > documentation changes. Maybe it was there, but I somehow missed it. >> > >> > Is the only goal some sense of developer completeness for these >> > options, or are these end-user-facing options of utility to actual end >> > users? I'm hoping the latter, but if so, can that be documented and >> > explained somewhere? I'm pretty sure this is explained somewhere in >> > an old mailing list discussion, but where? >> >> Completeness is essential as I want '--diff-merges' to provide all the >> needed capabilities, and one of them was actually missing, that is there >> in the '-m' semantics, exactly as I said in the descriptions. > > I ask you why a user would want to use this option, and you simply > assert that it's a "needed capabilit[y]"? Could you explain *why* > it's needed or helpful for users instead of just repeating the > assertion that it is needed? I'm trying to explain that the use-case(s) is(are) at least the same as for existing '-m' option, and '-m' is used in practice, so it must be useful, right? Who am I to judge? So I don't. For me one of the goals is to let people replace '-m' in scripts/aliases with '--diff-merges=on,hide' and eventually let '-m' behave better as a short user option, similar to '-c/--cc/--remrege-diff'. And then it might happen that 'hide' is actually useful elsewhere (see below for a try), as it often happens once particular functionality is properly factored out of given use-case. > If I can't figure out why it's needed or useful for users despite > having read your cover letter, commit messages, underlying source code > and documentation, and this full thread, then there may well be > something wrong with me...but it seems likely that many users will > also have difficulty figuring out why this option is useful. Then they are still free not to use it, and I doubt they will try to find why it's useful in the commit messages anyway, so do we need to put something into the documentation then? What do you suggest in addition to the functional explanation of the 'hide' that is already there? That said, what comes to mind, as a use-case, I figure you might try to define an alias for 'diff log' that will use 'remerge' format by default once diffs are requested using '-p': $ git config alias.lr 'log --diff-merges=remerge,hide' and check if this 'git lr' is useful. Thanks, -- Sergey Organov
We covered this series in Review Club, thanks for coming Sergey! For those who are interested, the notes are here: https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?usp=sharing and reviewers will send feedback to the list separately anyway. I mostly had comments on the design, so I'll leave most of my comments here. Commenting on the cover letter as a whole, on first read, it wasn't obvious to me what this series was trying to achieve because the CL presents the 5 patches individually instead of a cohesive story. From my understanding, the story is as follows: we want '-m' (enable diff-merges) to also imply '-p', but we can't just change the default behavior, so we do the following instead: - Add a config option that gives the behavior that we want (2/5). - Deprecate '-m' by giving '--diff-merges=on;hide' as a synonym and encouraging users to use that instead (1,4,5/5). Patch 3/5 is completely separate. There was some resistance to it during Review Club, but if we still want this, it might be worth splitting off into its own series so that we can keep the discussions separate. During the discussion, it also appeared that this "modification of '-m' semantics" refers to a patch that changed the default but got reverted due to breaking legacy scripts. It would be extremely useful to include a link to that previous patch and the discussion around its revert, especially given the discussion about whether users actually need '-diff-merges=hide' ([1] and elsewhere). [1] https://lore.kernel.org/git/CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@mail.gmail.com/ Sergey Organov <sorganov@gmail.com> writes: > 1. --diff-merges=[no]-hide > > The set of diff-merges options happened to be incomplete, and failed > to implement exact semantics of -m option that hides output of diffs > for merge commits unless -p option is active as well. > > The new "hide" option fixes this issue, so that now > > --diff-merges=on --diff-merges=hide > > combo is the exact synonym for -m. > > The log.diffMerges configuration also accepts "hide" and "no-hide" > values, and governs the default value for the hide bit. The > configuration behaves as if "--diff-merges=[no-]hide" is inserted > first in the command-line. I had the same concerns as Elijah, which is that this behavior is probably clearer as a separate flag (like "--hide-diff-merges"), which is more consistent with how '--diff-options' is used today, which means that: a) it is easier to explain to users b) the implementation is simpler (I'll comment on Patch 1 code separately) c) it makes Patch 4 obsolete But I'm not convinced that we actually want this behavior at all. I don't see why a user would use a flag that says "do nothing unless other flags are given". I don't find the 'alias use case' compelling, because the user still has to choose whether to pass '-p', so at that point they could just add a different alias. I haven't dug through the code/ML to figure out whether '-m' requiring '-p' was an intentional feature or not, but if you could find the old thread where you changed the default (and it got reverted), that would help the discussion a lot :) > 2. log.diffMerges-m-imply-p > > Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc' > options do. Simply fixing this inconsistency by unconditional > modification of '-m' semantics appeared to be a bad idea, as it broke > some legacy scripts/aliases. This patch rather provides configuration > variable to tweak '-m' behavior accordingly. I thought that Junio's suggestion to implement a flag that acts like '-m' with '-p' [2] was quite a good one (maybe '-M' or '--diff-merges=show'), since I think that very few users would actually set this config, but the ones that would actually use it can just replace '-m' with '-M'. [2] https://lore.kernel.org/git/xmqqfse37c0n.fsf@gitster.g/ > 3. log.diffMergesForce > > Force specific log format for -c, --cc, and --remerge-diff options > instead of their respective formats. The override is useful when some > external tool hard-codes diff for merges format option. This might be better off as its own series, since the change isn't related to '-m', but I'm worried about the precedent that this sets. To my knowledge, CLI options always overwrite config, but this is the opposite. I would prefer not to do this, especially if the use case is to work around an external tool (since it is arguably the tool that is broken). > 5. Issue warning for lone '-m'. > > Lone '-m' is in use by scripts/aliases that aim at enabling diff > output for merge commits, but only if '-p' is then specified as well. > > As '-m' may now be configured to imply '-p' (using > 'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified, > and suggest to instead use '--diff-merges=on,hide' that does not > depend on user configuration. > > This is expected to give a provision for enabling > log.diffMerges-m-imply-p by default in the future. Since '-m' without '-p' is a mistake in most cases, I wonder if we should just emit this warning today (maybe via the advise() API). Even if we don't keep '--diff-options=hide', deprecating lone '-m' and giving a warning seems good to keep.
Glen Choo <chooglen@google.com> writes: > We covered this series in Review Club, thanks for coming Sergey! For > those who are interested, the notes are here: Thank you all guys for the review and for valuable discussion! [...] > During the discussion, it also appeared that this "modification of '-m' > semantics" refers to a patch that changed the default but got reverted > due to breaking legacy scripts. It would be extremely useful to include > a link to that previous patch and the discussion around its revert, > especially given the discussion about whether users actually need > '-diff-merges=hide' ([1] and elsewhere). The last time '-m' issue appeared on the list, it all started here: https://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/ In particular, the final patch and its revert is deeper down this tread: https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t and https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/ Where do you prefer these references to be put, in the cover letter, in the commit message, or in both places? -- Sergey Organov
Glen Choo <chooglen@google.com> writes: > We covered this series in Review Club, thanks for coming Sergey! For > those who are interested, the notes are here: > > https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?usp=sharing > > and reviewers will send feedback to the list separately anyway. I mostly > had comments on the design, so I'll leave most of my comments here. > > Commenting on the cover letter as a whole, on first read, it wasn't > obvious to me what this series was trying to achieve because the CL > presents the 5 patches individually instead of a cohesive story. From my > understanding, the story is as follows: we want '-m' (enable > diff-merges) to also imply '-p', but we can't just change the default > behavior, so we do the following instead: > > - Add a config option that gives the behavior that we want (2/5). > - Deprecate '-m' by giving '--diff-merges=on;hide' as a synonym and > encouraging users to use that instead (1,4,5/5). Well, very close, but not exactly. I'd rather say: 1. Provide exact semantics of '-m' trough --diff-merges UI by introducing 'hide' option, after which '--diff-merges=on,hide' and '-m' have exactly the same behavior. 2. Add a config option that gives the new behavior we want to '-m': to rather be a synonym for '--diff-merges=on[,hide] -p". (Please notice that 'hide' is not needed here as it's immediately canceled by '-p'.) So, essentially, after (1) is there, the config option turns '-m' meaning from default '--diff-merges=on,hide' to desired '--diff-merges=on -p'. Please also notice that at this point we may instead decide to just switch '-m' meaning to new semantics, either without config at all, or with config that'd rather restore previous semantics. In fact, the primary reason why previously such patch has been reverted was absence of (1), and so with (2) maybe I was just overly cautious. > Patch 3/5 is completely separate. There was some resistance to it during > Review Club, but if we still want this, it might be worth splitting off > into its own series so that we can keep the discussions separate. OK, I'll cut it off for now. > > During the discussion, it also appeared that this "modification of '-m' > semantics" refers to a patch that changed the default but got reverted > due to breaking legacy scripts. It would be extremely useful to include > a link to that previous patch and the discussion around its revert, > especially given the discussion about whether users actually need > '-diff-merges=hide' ([1] and elsewhere). Yep, please see references I've sent in my previous answer to this message. > > [1] https://lore.kernel.org/git/CABPp-BHaPpQdO-uBT6ENHAM1Y-c=SBxktH-S_BTtxJvfd1qSpw@mail.gmail.com/ > > Sergey Organov <sorganov@gmail.com> writes: > >> 1. --diff-merges=[no]-hide >> >> The set of diff-merges options happened to be incomplete, and failed >> to implement exact semantics of -m option that hides output of diffs >> for merge commits unless -p option is active as well. >> >> The new "hide" option fixes this issue, so that now >> >> --diff-merges=on --diff-merges=hide >> >> combo is the exact synonym for -m. >> >> The log.diffMerges configuration also accepts "hide" and "no-hide" >> values, and governs the default value for the hide bit. The >> configuration behaves as if "--diff-merges=[no-]hide" is inserted >> first in the command-line. > > I had the same concerns as Elijah, which is that this behavior is > probably clearer as a separate flag (like "--hide-diff-merges"), which > is more consistent with how '--diff-options' is used today, which means > that: > > a) it is easier to explain to users > b) the implementation is simpler (I'll comment on Patch 1 code > separately) > c) it makes Patch 4 obsolete I'd postpone implementation/design discussion till we get to agreement of the need for this option in the first place. > But I'm not convinced that we actually want this behavior at all. I > don't see why a user would use a flag that says "do nothing unless > other flags are given". don't find the 'alias use case' compelling, > because the user still has to choose whether to pass '-p', so at that > point they could just add a different alias. If one travels back the history, they will find that originally all -m, -c, and --cc were behaving exactly this way: "do nothing, unless diffs are actually requested", i.e. they specified only diff format to be used once requested, and did not request the output themselves. I prefer to stay on the safe side, and assume that such behavior is still useful, even though -c/--cc turned to imply '-p' eventually, as doing the same to '-m' caused so much desire and resistance simultaneously. OTOH, --diff-merges=<format> does two things: specifies the format and requests the output for merge commits. It could have been design mistake, even though it has been discussed at the time of introduction, but now the only way to get another behavior is to turn off one of the actions they do: turn off requesting the actual output for merge commits, and that's what proposed 'hide' means. > I haven't dug through the code/ML to figure out whether '-m' requiring > '-p' was an intentional feature or not, but if you could find the old > thread where you changed the default (and it got reverted), that would > help the discussion a lot :) Yep, I gave the links in my first answer to this message. > >> 2. log.diffMerges-m-imply-p >> >> Historically, '-m' doesn't imply '-p' whereas similar '-c' and '--cc' >> options do. Simply fixing this inconsistency by unconditional >> modification of '-m' semantics appeared to be a bad idea, as it broke >> some legacy scripts/aliases. This patch rather provides configuration >> variable to tweak '-m' behavior accordingly. > > I thought that Junio's suggestion to implement a flag that acts like > '-m' with '-p' [2] was quite a good one (maybe '-M' or > '--diff-merges=show'), since I think that very few users would actually > set this config, but the ones that would actually use it can just > replace '-m' with '-M'. Introducing new short convenience option(s) is out of scope of these series, and suggested --diff-merges=show, as I see it, is essentially "--diff-merges=on -p" that I find hard to explain inside the '--diff-merges=' context which name suggests it affects merge commits only. That said, saying: "we have slightly broken '-m' that we refrain to fix, so let's introduce '-M' instead of fixing '-m'" does not sound very convincing either. > > [2] https://lore.kernel.org/git/xmqqfse37c0n.fsf@gitster.g/ > >> 3. log.diffMergesForce >> >> Force specific log format for -c, --cc, and --remerge-diff options >> instead of their respective formats. The override is useful when some >> external tool hard-codes diff for merges format option. > > This might be better off as its own series, since the change isn't > related to '-m', Yep, I'm sorry to mix it into the series, -- the only excuse is that it looked very relevant to me when I did it :) > but I'm worried about the precedent that this sets. > To my knowledge, CLI options always overwrite config, but this is the > opposite. I would prefer not to do this, especially if the use case is > to work around an external tool (since it is arguably the tool that is > broken). The tool was only an initial motivation for the patch. From following a few discussions on the list I got feeling that every person has their own preference for --diff-merges format, and rarely wants to see anything else. This config essentially gives them a way to say "please use my preferred format everywhere, unless I explicitly say otherwise", in a centralized manner. > >> 5. Issue warning for lone '-m'. >> >> Lone '-m' is in use by scripts/aliases that aim at enabling diff >> output for merge commits, but only if '-p' is then specified as well. >> >> As '-m' may now be configured to imply '-p' (using >> 'log.diffMerges-m-imply-p'), issue warning if lone '-m' is specified, >> and suggest to instead use '--diff-merges=on,hide' that does not >> depend on user configuration. >> >> This is expected to give a provision for enabling >> log.diffMerges-m-imply-p by default in the future. > > Since '-m' without '-p' is a mistake in most cases, I wonder if we > should just emit this warning today (maybe via the advise() API). Even > if we don't keep '--diff-options=hide', deprecating lone '-m' and giving > a warning seems good to keep. I'd tend to rather not. Actually, as far as I'm aware, the only actual use that has been detected was "--fist-parent -m", and that use case was exactly what you guys don't find useful: specify default format for merge commits. In this particular case it is the diff to the first parent, and dates back to the days before --diff-merges, when using history traversal option --first-parent was the only way to get diffs with respect to the first parent for merges. Maybe we should instead flag the "--first-parent -m" as a warning, as producing a warning for lone "-m" without these patches is effectively getting users out of using lone "-m" instead of fixing the latter. I rather see the bright future as using "-m" all the time, as it's now extremely configurable. Thanks,
Sergey Organov <sorganov@gmail.com> writes: > The last time '-m' issue appeared on the list, it all started here: > > https://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/ > > In particular, the final patch and its revert is deeper down this tread: > > https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t > > and > > https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/ Thanks, these provide extremely helpful context :) In particular: - Junio describes this "do nothing unless -p" is given behavior as an accident [1]. - Jonathan Nieder notes that this change accidentally broke scripts where "-m" probably wasn't doing anything useful, but we wanted to avoid breaking the scripts for backwards compatibility anyway [2]. I got the sense that the closest thing to an intentional use case of "-m" is for users who thought that "-m" would affect path limiting [3], although it doesn't actually do that. So what I've reads so far suggests that "do nothing unless -p" (aka --diff-merges=hide) is not actually useful, and we should just drop it. We could keep the warning for "-m" without "-p" (Patch 5), and recommend "--diff-merges=(on|m)". [1] https://lore.kernel.org/git/xmqqwnsl93m3.fsf@gitster.g/ [2] https://lore.kernel.org/git/YQtYEftByY8cNMml@google.com/ [3] https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/ > > Where do you prefer these references to be put, in the cover letter, in > the commit message, or in both places? > > -- Sergey Organov
Sergey Organov <sorganov@gmail.com> writes: > Where do you prefer these references to be put, in the cover letter, in > the commit message, or in both places? Wherever it might be relevant as motivation behind the change, so probably both, but especially the cover letter :) > > -- Sergey Organov
Glen Choo <chooglen@google.com> writes: > Sergey Organov <sorganov@gmail.com> writes: > >> The last time '-m' issue appeared on the list, it all started here: >> >> https://lore.kernel.org/git/CAMMLpeR-W35Qq6a343ifrxJ=mwBc_VcXZtVrBYDpJTySNBroFw@mail.gmail.com/ >> >> In particular, the final patch and its revert is deeper down this tread: >> >> https://lore.kernel.org/git/20210520214703.27323-11-sorganov@gmail.com/#t >> >> and >> >> https://lore.kernel.org/git/YQyUM2uZdFBX8G0r@google.com/ > > Thanks, these provide extremely helpful context :) In particular: > > - Junio describes this "do nothing unless -p" is given behavior as an > accident [1]. I rather read it as Junio saying that "-m does not imply -p" is historical accident, and yes, it is, provided '-c/--cc' were fixed at some point, and '-m' was not, so in fact I figure Junio meant: "-m differs from -c/--cc in not implying '-p'" is historical accident. And then he suggests to leave it as is, with which I disagree. In addition, in all the discussions, I believe Junio at least once said he does see valid usages for current hush '-m': <quote> But "stash list" example shows that "log --first-parent -m" without "-p" in a script has a valid reason, and a change that hurts those who correctly used a command and an option in a way they were intended to do _is_ problematic. </quote> here: https://lore.kernel.org/git/xmqqy29chim6.fsf@gitster.g/ > - Jonathan Nieder notes that this change accidentally broke scripts > where "-m" probably wasn't doing anything useful, but we wanted to > avoid breaking the scripts for backwards compatibility anyway [2]. > > I got the sense that the closest thing to an intentional use case of > "-m" is for users who thought that "-m" would affect path limiting [3], > although it doesn't actually do that. I don't think so. Dunno why you got such feeling. It's rather that for some time "--first-parent -m" was the only way to produce *most* useful form of "-m" format: show diff with respect to the *first* parent only, whereas without "--first-parent" "-m" produced diff output for *every* parent in turn(!) giving extremely confusing result. Please notice how --first-parent appears in most of those discussions. Overall, the (simplified) history of '-m' goes like this, as far as I can tell: 0. Original '-m', documented only for 'diff-tree'. The diffs were produced to all the parents that was probably very logical for plumbing 'diff-tree', as it reflects symmetric nature of merge commits in the DAG, that is the core of git data model. However, while "git log -p" does not produce patches for merge commits (apparently to get rid of often large output), '-m' in fact enforces the output for merge commits, in the format produced by 'diff-tree -m', i.e., diffs to all the parents in turn. [The latter was probably the first mistake, it should have rather produced the diff with respect to the first parent that is more suitable for "git log" being porcelain, to show changes introduced by the commit to the mainline, exactly as for non-merge commits] 1. '-c' is introduced, then '--cc' is introduced, with semantics similar to '-m' with respect to '-p', but different kinds of output. At this point we have consistent behavior of '-m', '-c', and '--cc' with respect to '-p', none of which produce any output unless '-p' is specified as well. 2. '-c' and '--cc' are changed to imply '-p'[0]. '-m' is left alone, supposedly forgotten as being undocumented for "git log" and of limited use, due to its large and surprising output. [I think that was the second mistake, forgetting to change '-m' accordingly] 3. '-m' is changed to produce diff with respect to the first parent only when '--first-parent' is specified [1]. '-m' finally starts to (sometimes) give useful output, and starts to be actually used, but only together with '--first-parent' most of times. BTW, this is the first time '-m' has been documented as part of "git log": "This patch properly documents the -m switch, which has so far been mentioned only as a fairly special diff-tree flag." [I think there are more mistakes here: not changing '-m' to imply '-p' at this point, and not producing single-parent diff even without '--first-parent'] 4. "--first-parent" is suggested to imply "-m"(!) to let "--first-parent -p" to produce diff for merge commits[2]. That in turn needed an option that will negate implied "-m", and that's where --[no-]diff-merges was suggested. Please notice that if '-m' implied '-p' (as it should) at this point, there should be little needed for these patches, as just saying "git log --first-parent -m" would produce required result. So, mistakes above caused a need to fix them. 5. At this point, in the referenced discussion I suggested '--diff-merges=on|off' instead of '--[no-]diff-merges', to allow for further extensions. 6. '--diff-merges=' option actually born to provide some missing functionality and to get rid of inconsistencies[3]. 7. '-m' format becomes configurable using new "log.diffMerges" configuration[4] so that we can make it conveniently useful even without "--first-parent". This was immediately implemented in generalized manner to allow to configure '-m' to produce not only single-parent diff, but any supported format. 8. As a reply to yet another request on the mailing list "why '-m' produces no output", I tried "-m imply -p" patch series[5], which were accepted, but then the last patch only(!) from the series, that actually introduced required behavior, has been reverted. This left me feel I got some unfinished job to do. 9. These patch series, trying "-m imply -p" again, now more carefully. > So what I've reads so far suggests that "do nothing unless -p" (aka > --diff-merges=hide) is not actually useful, and we should just drop > it. Again, I've tried exactly this before, and that was first accepted, and then reverted, that's why --diff-merges=hide has been introduced in these series, to address the issues raised during revert request. References: [0]: Showing merges easier with "git log": https://lore.kernel.org/git/1440110591-12941-1-git-send-email-gitster@pobox.com/ [1]: git log -p -m: Document, honor --first-parent https://lore.kernel.org/git/20100210011149.GR9553@machine.or.cz/ [2]: making --first-parent imply -m: https://lore.kernel.org/git/20200728163617.GA2649887@coredump.intra.peff.net/ [3]: git-log: implement new --diff-merge options: https://lore.kernel.org/git/20201221152000.13134-1-sorganov@gmail.com/ [4]: git log: configurable default format for merge diffs https://lore.kernel.org/git/20210413114118.25693-1-sorganov@gmail.com/ [5]: diff-merges: let -m imply -p https://lore.kernel.org/git/20210520214703.27323-1-sorganov@gmail.com/ Thanks, -- Sergey Organov