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 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.
