Re: [PATCH v2 5/7] rebase: drop support for `--preserve-merges`

Hi Phillip,

On Tue, 7 Sep 2021, Phillip Wood wrote:

On 07/09/2021 13:32, Johannes Schindelin wrote:

quoted

On Mon, 6 Sep 2021, Phillip Wood wrote:

quoted

On 01/09/2021 12:57, Johannes Schindelin via GitGitGadget wrote:

quoted

From: Johannes Schindelin <redacted>

This option was deprecated in favor of `--rebase-merges` some time ago,
and now we retire it.

This all looks good to me. I did see the comment below in builtin/rebase.c
that could be tweaked if you reroll, but it is a very minor issue.

/* -i followed by -p is still explicitly interactive, but -p alone is not
*/
static int parse_opt_interactive(const struct option *opt, const char
*arg,
                                  int unset)

Right, without `-p` this comment does not make sense anymore. But once I
replace the `-p` by `-r`, it _does_ make sense: `git rebase -r` is not
interactive, but `git rebase -ir` _is_.

quoted

I do wonder if we need these option parsing functions now but that is a
question for another day.

As the function parses the `-i`/`--interactive` option, which is not going
anywhere, we still need it.

Sure I was just wondering if we could simplify things by using something like
OPT_BOOL rather than OPT_CALLBACK_F for "--am", "-i" and "-m". I haven't
looked too closely though

It's definitely a good question, and I wondered, too, but failed to share
my conclusions with you.

The callback does two things: first, it switches the rebase type to
`merge`, and then it also sets the flag (by setting a bit in the bit
field) that the rebase is explicitly interactive.

The reason for this "explicitly interactive" wording is historical, of
course, because historically some functionality was implemented by
`git-rebase--interactive.sh` even if the rebase itself was intended _not_
to be run interactively. Think `git rebase --exec <command>`, for example.

These days, it is still true that `rebase-interactive.c` implements some
functionality that is not necessarily meant to be executed interactively
(again, `--exec`, but also `--rebase-merges`).

So there _is_ a need for that option to do more than one thing, and as
some options can try to set, say, the rebase type, it is important to
handle those options immediately (as opposed to trying to set a flag based
on `--interactive` and then trying to infer the rebase type from that flag
_after_ `parse_options()` is done).

Ciao,
Dscho