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