Elijah Newren <newren@xxxxxxxxx> writes: > On Wed, Nov 30, 2022 at 5:16 AM Sergey Organov <sorganov@xxxxxxxxx> wrote: >> >> Elijah Newren <newren@xxxxxxxxx> writes: >> >> > On Sun, Nov 27, 2022 at 1:37 AM Sergey Organov <sorganov@xxxxxxxxx> 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
