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

From: Sergey Organov <sorganov@gmail.com>
Date: Mar 5, 2023, 22:54 UTC
Message-ID: <87ttyyn71f.fsf@osv.gnss.ru>
In-Reply-To: <CAMMLpeSowxoJgPOt84Dos8BVKU=y=M+Ph0xaGe6UTxwwE5zsXQ@mail.gmail.com>

Alex Henrie <alexhenrie24@gmail.com> writes:

Show 35 quoted lines

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

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