From: Alex Henrie <alexhenrie24@gmail.com>
To: phillip.wood@dunelm.org.uk
Cc: git@vger.kernel.org, tao@klerks.biz, gitster@pobox.com,
newren@gmail.com, phillip.wood123@gmail.com,
Johannes.Schindelin@gmx.de, sorganov@gmail.com,
Glen Choo <chooglen@google.com>,
Calvin Wan <calvinwan@google.com>
Subject: Re: [PATCH v5 3/3] rebase: add a config option for --rebase-merges
Date: Sun, 12 Mar 2023 14:57:28 -0600 [thread overview]
Message-ID: <CAMMLpeTUbG+b89acan-GXGS4H=J7aQupbK8zdxwNg__U_We2dw@mail.gmail.com> (raw)
In-Reply-To: <5551d67b-3021-8cfc-53b5-318f223ded6d@dunelm.org.uk>
On Tue, Mar 7, 2023 at 9:23 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 04/03/2023 23:24, Alex Henrie wrote:
> > On Thu, Mar 2, 2023 at 2:37 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >
> >> On 25/02/2023 18:03, Alex Henrie wrote:
> >
> >>> +rebase.merges::
> >>> + Whether and how to set the `--rebase-merges` option by default. Can
> >>> + be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> >>> + true is equivalent to `--rebase-merges` without an argument, setting to
> >>> + `rebase-cousins` or `no-rebase-cousins` is equivalent to
> >>> + `--rebase-merges` with that value as its argument, and setting to false
> >>> + is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> >>> + command line without an argument overrides a `rebase.merges=false`
> >>> + configuration but does not override other values of `rebase.merge`.
> >>
> >> I'm still not clear why the commandline doesn't override the config in
> >> all cases as is our usual practice. After all if the user has set
> >> rebase.merges then they don't need to pass --rebase-merges unless they
> >> want to override the config.
> >
> > Given the current push to turn rebase-merges on by default, it seems
> > likely that rebase-cousins will also be turned on by default at some
> > point after that.
>
> It is good to try and future proof things but this seems rather
> hypothetical. I don't really see how the choice of whether
> --rebase-merges is turned on by default is related to the choice of
> whether or not to rebase cousins by default. It is worth noting that the
> default was changed to from rebase-cousins to no-rebase-cousins early in
> the development of --rebase-merges[1] as it was felt to be less surprising.
> [1]
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.1801292251240.35@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/
Thank you for sharing that link. Even though I got the tests right, I
got confused and started thinking that rebase-cousins was a more
thorough version of rebase-merges. In fact, they do opposite things:
rebase-merges tries to preserve the graph and rebase-cousins
intentionally restructures the graph. In my opinion using the word
"rebase" in the names of both options was another unfortunate UI
decision, but I understand the difference now.
> > There will be a warning about the default changing,
> > and we'll want to let users suppress that warning by setting
> > rebase.rebaseMerges=rebase-cousins. It would then be very confusing if
> > a --rebase-merges from some old alias continued to mean
> > --rebase-merges=no-rebase-cousins when the user expects it to start
> > behaving as though the default has already changed.
>
> But aren't you breaking those aliases now when
> rebase.rebaseMerges=rebase-cousins? That's what I'm objecting to. It
> seems like we're breaking things now to avoid a hypothetical future
> change breaking them which does not seem like the right trade off to me.
>
> It also does not fit with the way other optional arguments interact with
> their associated config setting. For example "git branch/checkout/switch
> --track" and branch.autoSetupMerge. If the optional argument to --track
> is omitted it defaults to "direct" irrespective of the config.
What I really don't want is to paint ourselves into a corner. You're
right that it's unlikely that the default will ever change from
no-rebase-cousins to rebase-cousins; I was mistaken. However, Glen
thinks that in the future we might have some kind of
rebase-evil-merges mode as well, and that that might become the
default. If we don't let the rebase.rebaseMerges config value control
the default behavior of --rebase-merges without an argument on the
command line, we would have to introduce a separate config option for
the transition, which would be ugly.
More voices would be helpful here. Does anyone else have an opinion on
how likely it is that the default rebase-merges mode will change in
the future? Or on whether rebase.rebaseMerges should be allowed to
affect --rebase-merges in order to facilitate such a change?
> >>> +test_expect_success '--rebase-merges overrides rebase.merges=false' '
> >>> + test_config rebase.merges false &&
> >>> + git checkout -b override-config-merges-false E &&
> >>> + before="$(git rev-parse --verify HEAD)" &&
> >>> + test_tick &&
> >>> + git rebase --rebase-merges C &&
> >>> + test_cmp_rev HEAD $before
> >>
> >> This test passes if the rebase does nothing, maybe pass --force and
> >> check the graph?
> >
> > The rebase is supposed to do nothing here.
>
> It's not doing nothing though - it is rebasing the branch, it just
> happens that everything fast-forwards so HEAD ends up unchanged. My
> point is that this test should verify the branch has been rebased. Maybe
> you could check the reflog message for HEAD@{0} is
>
> rebase (finish): returning to refs/heads/override-config-merges-false
>
> > Checking that the commit
> > hash is the same is just a quick way to check that the entire graph is
> > the same. What more would be checked by checking the graph instead of
> > the hash?
>
> By using --force and checking the graph you check that the rebase
> actually happened.
I got the impression that people like that not checking the graph
itself (or the reflog) makes the tests more concise, but I don't care
much either way. For what it's worth, the way I did it matches the
existing tests in the file. If you can find at least one other person
who thinks that it ought to change for this patch series to be
accepted, and no one else objects, I'll change it.
> Thanks for working on this
You're welcome; hopefully we can get the remaining details ironed out quickly.
-Alex
next prev parent reply other threads:[~2023-03-12 20:58 UTC|newest]
Thread overview: 96+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-23 5:34 [PATCH v4 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-02-23 5:34 ` [PATCH v4 2/3] rebase: stop accepting --rebase-merges="" Alex Henrie
2023-02-24 13:54 ` Johannes Schindelin
2023-02-24 17:20 ` Junio C Hamano
2023-02-24 17:50 ` Alex Henrie
2023-02-24 18:08 ` Junio C Hamano
2023-02-24 18:23 ` Alex Henrie
2023-02-24 18:40 ` Junio C Hamano
2023-02-24 18:55 ` Alex Henrie
2023-02-24 19:13 ` Junio C Hamano
2023-02-24 19:24 ` Alex Henrie
2023-02-24 19:24 ` Phillip Wood
2023-02-24 19:56 ` Alex Henrie
2023-02-23 5:34 ` [PATCH v4 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-02-24 13:53 ` Johannes Schindelin
2023-02-24 17:49 ` Alex Henrie
2023-02-24 14:55 ` Phillip Wood
2023-02-24 17:51 ` Alex Henrie
2023-02-23 17:28 ` [PATCH v4 1/3] rebase: add documentation and test for --no-rebase-merges Junio C Hamano
2023-02-24 13:57 ` Johannes Schindelin
2023-02-24 19:16 ` Junio C Hamano
2023-02-25 18:09 ` Alex Henrie
2023-02-25 18:03 ` [PATCH v5 0/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-02-25 18:03 ` [PATCH v5 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-01 23:23 ` Glen Choo
2023-02-25 18:03 ` [PATCH v5 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
2023-03-01 23:46 ` Glen Choo
2023-03-02 10:07 ` Phillip Wood
2023-03-02 18:02 ` Calvin Wan
2023-02-25 18:03 ` [PATCH v5 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-01 23:43 ` Glen Choo
2023-03-02 9:37 ` Phillip Wood
2023-03-04 23:24 ` Alex Henrie
2023-03-07 16:23 ` Phillip Wood
2023-03-12 20:57 ` Alex Henrie [this message]
2023-03-13 14:20 ` Phillip Wood
2023-03-13 16:12 ` Felipe Contreras
2023-03-13 19:46 ` Junio C Hamano
2023-03-24 14:47 ` About replaying "evil" merges... " Johannes Schindelin
2023-03-02 18:02 ` Calvin Wan
2023-03-04 23:24 ` Alex Henrie
2023-03-01 23:14 ` [PATCH v5 0/3] " Glen Choo
2023-03-02 5:02 ` Alex Henrie
2023-03-02 5:09 ` Alex Henrie
2023-03-05 5:07 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-05 5:07 ` [PATCH v6 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-08 22:25 ` Sergey Organov
2023-03-05 5:07 ` [PATCH v6 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
2023-03-07 14:59 ` Phillip Wood
2023-03-05 5:07 ` [PATCH v6 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-07 14:56 ` Phillip Wood
2023-03-07 18:32 ` Junio C Hamano
2023-03-12 21:01 ` Alex Henrie
2023-03-08 0:09 ` Glen Choo
2023-03-08 0:02 ` Glen Choo
2023-03-12 21:03 ` Alex Henrie
2023-03-15 2:52 ` Alex Henrie
2023-03-16 17:32 ` Glen Choo
2023-03-16 18:11 ` Felipe Contreras
2023-03-16 22:46 ` Glen Choo
2023-03-16 23:48 ` Felipe Contreras
2023-03-16 20:27 ` Alex Henrie
2023-03-16 22:39 ` Glen Choo
2023-03-18 5:59 ` Alex Henrie
2023-03-24 15:05 ` Johannes Schindelin
2023-03-25 16:59 ` Sergey Organov
2023-03-05 12:22 ` [PATCH v6 0/3] rebase: document, clean up, and introduce " Sergey Organov
2023-03-05 21:33 ` Alex Henrie
2023-03-05 22:54 ` Sergey Organov
2023-03-06 0:02 ` Alex Henrie
2023-03-06 13:23 ` Sergey Organov
2023-03-06 19:08 ` Junio C Hamano
2023-03-06 17:19 ` Junio C Hamano
2023-03-06 16:24 ` Phillip Wood
2023-03-06 17:36 ` Alex Henrie
2023-03-07 15:07 ` Phillip Wood
2023-03-08 0:13 ` Glen Choo
2023-03-12 21:04 ` [PATCH v7 " Alex Henrie
2023-03-12 21:04 ` [PATCH v7 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-12 21:04 ` [PATCH v7 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
2023-03-12 21:04 ` [PATCH v7 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-20 5:59 ` [PATCH v8 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-20 5:59 ` [PATCH v8 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-20 5:59 ` [PATCH v8 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
2023-03-20 5:59 ` [PATCH v8 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-22 16:54 ` Phillip Wood
2023-03-23 18:45 ` Junio C Hamano
2023-03-24 14:52 ` Phillip Wood
2023-03-25 5:23 ` Alex Henrie
2023-03-25 5:21 ` Alex Henrie
2023-03-26 3:06 ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Alex Henrie
2023-03-26 3:06 ` [PATCH v9 1/3] rebase: add documentation and test for --no-rebase-merges Alex Henrie
2023-03-26 3:06 ` [PATCH v9 2/3] rebase: deprecate --rebase-merges="" Alex Henrie
2023-03-26 3:06 ` [PATCH v9 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-03-26 15:12 ` [PATCH v9 0/3] rebase: document, clean up, and introduce " Phillip Wood
2023-03-27 16:33 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMMLpeTUbG+b89acan-GXGS4H=J7aQupbK8zdxwNg__U_We2dw@mail.gmail.com' \
--to=alexhenrie24@gmail.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=calvinwan@google.com \
--cc=chooglen@google.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=newren@gmail.com \
--cc=phillip.wood123@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
--cc=sorganov@gmail.com \
--cc=tao@klerks.biz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).