Elijah Newren [off-list ref] writes:
On Thu, Mar 12, 2020 at 8:13 AM Emily Shaffer [off-list ref] wrote:
quoted
...
This broke quite a few upstream users for us today...
One more thing I've been wondering was if we should really have said
"ok, the two known gotchas have been dealt with, so let's ship it".
It is quite likely that users may be hit by remaining differences
between the merge and apply backends that we assume to be merely
subtle and easy-to-work-around ones, and it may be prudent to use
the "let's not change the default just yet, but ask people to test
it in their workflow by advertising the configuration" patch.
On Thu, Mar 12, 2020 at 12:12 PM Junio C Hamano [off-list ref] wrote:
Elijah Newren [off-list ref] writes:
quoted
On Thu, Mar 12, 2020 at 8:13 AM Emily Shaffer [off-list ref] wrote:
quoted
...
This broke quite a few upstream users for us today...
One more thing I've been wondering was if we should really have said
"ok, the two known gotchas have been dealt with, so let's ship it".
It is quite likely that users may be hit by remaining differences
between the merge and apply backends that we assume to be merely
subtle and easy-to-work-around ones, and it may be prudent to use
the "let's not change the default just yet, but ask people to test
it in their workflow by advertising the configuration" patch.
Good question, but there's several things to unpack here...
First, note that this particular breakage would have occurred
regardless of the default setting, because the problem was that they
setting rebase.backend to an unrecognized value, not that we used a
different backend than they were used to.
Second, for the particular case of the post-commit hook that they
referenced in their rationale, that item was documented near the end
of the 2.25 cycle and mentioned in previous patchsets[1, 2] so I
figured this case was already considered.
Those two points may obscure the issue, though; your question is still
valid. I think the bigger question is whether there are other unknown
differences, or even known differences that are a bigger issue than we
currently realize. That's hard to judge, and it may not be possible
to judge until we've flipped the default. As such, it's a judgement
call. I can see the judgement call going either way. A couple things
to weigh in on how to go:
- Making rebase.backend default to 'apply' for 2.26 is certainly
the conservative option and may give us more feedback and time to iron
out differences
- We had multiple complaints this cycle about rebase.backend=apply
merging things incorrectly with the only workaround being to use the
merge backend[3,4]
- The rebase-backend topic wasn't merged down to master until less
than a week before -rc0. (For a variety of reasons.) A big change
like this probably would have been better to merge down earlier in
some cycle.
To be honest, if I was maintainer, I'm not sure which direction I'd
pick. If you feel safer switching the default backend to apply for
this cycle and then (re-)revert it early next cycle to the merge
backend, I think that's totally reasonable.
[1] https://lore.kernel.org/git/pull.679.v4.git.git.1579155273.gitgitgadget@gmail.com/
[2] https://lore.kernel.org/git/CABPp-BHONuRyt8VJqRuoCF2rGYZ5EhH9KJXQZ3NO69rYwA5J3g@mail.gmail.com/
[3] https://lore.kernel.org/git/CABPp-BGiu2nVMQY_t-rnFR5GQUz_ipyEE8oDocKeO+h+t4Mn4A@mail.gmail.com/
[4] https://lore.kernel.org/git/20200108223557.GE32750@szeder.dev/
On Thu, Mar 12, 2020 at 12:29:45PM -0700, Elijah Newren wrote:
First, note that this particular breakage would have occurred
regardless of the default setting, because the problem was that they
setting rebase.backend to an unrecognized value, not that we used a
different backend than they were used to.
If I understand correctly, it was also a setting that never worked in
any released version of Git. It was magic that was only ever in 'next'.
As annoying as it is to experience breakage, I'm not _too_ sympathetic
to this case, because that is part of the cost of running the bleeding
edge of 'next' or even 'master'. I.e., I think we have to make a cutoff
_somewhere_ to say "this is something that made it to the general
public, and therefore we can't break backwards compatibility" to keep
our sanity during development. And it seems like tagged releases are a
pretty good cutoff to me.
Though in this particular case, I don't mind too much just leaving "am"
as an alias for "apply" (it was actually the first thing I tried when
writing my earlier emails, but I'm probably not a representative user
there). Putting that in a release, though, may mean supporting it
forever. :)
- We had multiple complaints this cycle about rebase.backend=apply
merging things incorrectly with the only workaround being to use the
merge backend[3,4]
- The rebase-backend topic wasn't merged down to master until less
than a week before -rc0. (For a variety of reasons.) A big change
like this probably would have been better to merge down earlier in
some cycle.
It did feel a bit quick to me, hitting near the end of the cycle. We've
had the apply backend as the default for a decade, so even if there are
problems with it, they're known issues. So I don't think there's a
particular hurry. I'm not entirely convinced that cooking it longer
during the next cycle will turn up a lot of new data (I did find a few
issues, but the real test is the long-tail of weird use cases that we
won't see until an actual release). But it probably doesn't hurt much to
take it slow; it just delays a few bug-fixes (which people can still get
by setting a config option).
I guess like your email I'm going back and forth between the two
options. I think that means it probably doesn't matter _too_ much either
way.
-Peff