Alex Henrie <alexhenrie24@xxxxxxxxx> writes: > On Sun, Mar 5, 2023 at 5:22 AM Sergey Organov <sorganov@xxxxxxxxx> wrote: >> >> Alex Henrie <alexhenrie24@xxxxxxxxx> writes: >> >> [...] >> >> > - Rephrase the warning about --rebase-merges="" to recommend >> > --rebase-merges without an argument instead, and clarify that that >> > will do the same thing >> >> Not a strong objection to the series as they are, but, provided options >> with optional arguments are considered to be a bad practice in general >> (unless something had recently changed), I'd like to vote for adding: >> >> --rebase-merges=on (≡ --reabase-merges="") >> >> and for suggesting to use that one instead of --rebase-merges="" rather >> than naked --rebase-merges. >> >> It'd be best to actually fix --rebase-merges to always have an argument, >> the more so as we have '-r' shortcut, but as this is unlikely to fly due >> to backward compatibility considerations, this way we would at least >> avoid promoting bad practices further. >> >> If accepted, please consider to introduce >> >> --rebase-merges=off (≡ --no-rebase-merges) >> >> as well for completeness. >> >> BTW, that's how we settled upon in the implementation of --diff-merges, >> so these two will then be mutually consistent. > > I am curious as to why you say that flags with optional arguments are > considered bad practice. As far as I can tell, the core problem with such options is that generic options parsing code can't tell if in "--option value" "value" is an argument to "--option", or separate argument, that in turn may lead to very surprising behaviors and bugs. I believe it's a common knowledge here. If I'm wrong, then the rest simply doesn't make sense. > I don't see an advantage to adding --rebase-merges=on and then telling > users that they ought to always type the extra three characters, even > if we were designing the feature from scratch. The advantage is that we avoid optional arguments. That said, there is '-r' that is 13 characters shorter than --rebase-merges, so another option is to advertise that, provided you are still opposed to "--rebase-merges=on". > As it is, we're certainly not going to drop support for > --no-rebase-merges --no-rebase-merges is fine, as it has no optional arguments > or --rebase-merges without an argument, Yep, but that's a pity and the whole point of my comment: if we can't fix it, at least don't promote it. > so it seems fine to advertise them to users. --no-rebase-merges is fine, but then you don't advertise it anyway. > And if we do want to add support for passing a boolean value as an > argument to --rebase-merges, that can be done after the > rebase.rebaseMerges config option is added; it's not a prerequisite. Yep, that's why I said at the beginning that this is not an objection to the series as they are, rather a nitpick. Thanks, -- Sergey
