Re: [PATCH v6 0/3] rebase: document, clean up, and introduce a config option for --rebase-merges

[Alex Henrie <alexhenrie24@gmail.com>]

On Sun, Mar 5, 2023 at 3:54 PM Sergey Organov <sorganov@gmail.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > On Sun, Mar 5, 2023 at 5:22 AM Sergey Organov <sorganov@gmail.com> wrote:
> >>
> >> Alex Henrie <alexhenrie24@gmail.com> 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.

Interesting, thank you for clarifying. I just tried it and it appears
that --rebase-merges requires an equals sign when it has an argument.
For example:

$ git rebase --rebase-merges=rebase-cousins
Current branch master is up to date.

$ git rebase --rebase-merges rebase-cousins
fatal: invalid upstream 'rebase-cousins'

So there is no ambiguity because if an argument to a flag is optional,
an equals sign is required.

Conversely, because the argument to --diff-merges is required, the
equals sign is optional.

I can see the argument (no pun intended) for avoiding optional
arguments to flags because it is confusing that '=' is required for
optional arguments but not for mandatory arguments, and it's easy to
forget which arguments are mandatory and which are optional.

However, --rebase-merges already has an optional argument. If we make
it look more like --diff-merges by accepting the value "on", it could
actually be even more confusing because users would be more likely to
try "--rebase-merges on", thinking that that will work because
"--diff-merges on" works.

Given the current situation, users are just going to have to learn
that in Git, optional arguments to flags must be preceded by an equals
sign. Maybe someday Git will make breaking changes to get rid of
optional arguments to flags, but for better or for worse, that day
will not come soon :-/

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

I am not sure what you mean by this. The first patch of the series
adds documentation and a test for --no-rebase-merges, so I am
advertising it. Or are you saying that I /should/ advertise neither
--no-rebase-merges nor --rebase-merges without an argument, because
you think --rebase-merges=off and --rebase-merges=on would be better?

-Alex