| Message ID | 20230223053410.644503-2-alexhenrie24@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/3] rebase: add documentation and test for --no-rebase-merges | expand |
Hi Alex, On Wed, 22 Feb 2023, Alex Henrie wrote: > The unusual syntax --rebase-merges="" (that is, --rebase-merges with an > empty string argument) has been an undocumented synonym of > --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid > confusion when a rebase.merges config option is introduced, where > rebase.merges="" will be equivalent to not passing --rebase-merges. Being undocumented and obscure might be a good reason for some to consider this a bug; I do not. You could deprecate it, but there are probably better ideas than to remove it without prior warning. If all you want to do is to support `rebase.merges=`, you can do that without having to change the meaning of `--rebase-merges=`. Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > On Wed, 22 Feb 2023, Alex Henrie wrote: > >> The unusual syntax --rebase-merges="" (that is, --rebase-merges with an >> empty string argument) has been an undocumented synonym of >> --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid >> confusion when a rebase.merges config option is introduced, where >> rebase.merges="" will be equivalent to not passing --rebase-merges. > > Being undocumented and obscure might be a good reason for some to consider > this a bug; I do not. You could deprecate it, but there are probably > better ideas than to remove it without prior warning. > > If all you want to do is to support `rebase.merges=`, you can do that > without having to change the meaning of `--rebase-merges=`. Thanks for a doze of sanity. Let me mark the topic as on hold to wait for a resolution.
On Fri, Feb 24, 2023 at 10:20 AM Junio C Hamano <gitster@pobox.com> wrote: > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > On Wed, 22 Feb 2023, Alex Henrie wrote: > > > >> The unusual syntax --rebase-merges="" (that is, --rebase-merges with an > >> empty string argument) has been an undocumented synonym of > >> --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid > >> confusion when a rebase.merges config option is introduced, where > >> rebase.merges="" will be equivalent to not passing --rebase-merges. > > > > Being undocumented and obscure might be a good reason for some to consider > > this a bug; I do not. You could deprecate it, but there are probably > > better ideas than to remove it without prior warning. > > > > If all you want to do is to support `rebase.merges=`, you can do that > > without having to change the meaning of `--rebase-merges=`. > > Thanks for a doze of sanity. Let me mark the topic as on hold to > wait for a resolution. As Johannes pointed out, it's going to be confusing if rebase.merges="" does not mean rebase.merges=false. It's also going to be confusing if rebase.merges="" does not mean --rebase-merges="". The only sane option I see is to drop --rebase-merges="", or to add a deprecation warning now and drop it later. -Alex
Alex Henrie <alexhenrie24@gmail.com> writes: > As Johannes pointed out, it's going to be confusing if > rebase.merges="" does not mean rebase.merges=false. It's also going to > be confusing if rebase.merges="" does not mean --rebase-merges="". The > only sane option I see is to drop --rebase-merges="", or to add a > deprecation warning now and drop it later. If we were doing anything, then I think the only sensible way forward is to warn and then drop long after everybody forgets about it. But does it really need to be changed? I am perfectly happy to declare that those who wants to set rebase.merges to say false should set it to false (or no or 0), not an empty string. Also, "[rebase] merges = " in the configuration files does not have to mean the same thing as "--rebase-merges=" from the command line. Can't we just reserve that strange "--rebase-merges=" given from the command line to those who are already using it, without even advertising it in the documentation, document and encourage the longhand "--rebase-merges=no-rebase-cousins" from the command line and take only the form that corresponds to it in the configuration file, i.e. "[rebase] merges = no-rebase-cousins". We could even error out "[rebase] merges = " in the configuration file, as nobody is using such a configuration variable today.
On Fri, Feb 24, 2023 at 11:08 AM Junio C Hamano <gitster@pobox.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > As Johannes pointed out, it's going to be confusing if > > rebase.merges="" does not mean rebase.merges=false. It's also going to > > be confusing if rebase.merges="" does not mean --rebase-merges="". The > > only sane option I see is to drop --rebase-merges="", or to add a > > deprecation warning now and drop it later. > > If we were doing anything, then I think the only sensible way > forward is to warn and then drop long after everybody forgets about > it. > > But does it really need to be changed? I am perfectly happy to > declare that those who wants to set rebase.merges to say false > should set it to false (or no or 0), not an empty string. > > Also, "[rebase] merges = " in the configuration files does not have > to mean the same thing as "--rebase-merges=" from the command line. > Can't we just reserve that strange "--rebase-merges=" given from the > command line to those who are already using it, without even > advertising it in the documentation, document and encourage the > longhand "--rebase-merges=no-rebase-cousins" from the command line > and take only the form that corresponds to it in the configuration > file, i.e. "[rebase] merges = no-rebase-cousins". We could even > error out "[rebase] merges = " in the configuration file, as nobody > is using such a configuration variable today. The only way to truly make "[rebase] merges =" invalid is to print an error message and die with that configuration. I think that would be confusing too, especially since it's now looking like rebase.merges needs to be a pure boolean and an independent rebase.cousins boolean option is needed as well. However, if you think that that's better than deprecating --rebase-merges="" with the plan to drop it long after everybody forgets about it, then I'm happy to oblige. Just let me know what is wanted. -Alex
Alex Henrie <alexhenrie24@gmail.com> writes: > The only way to truly make "[rebase] merges =" invalid is to print an > error message and die with that configuration. I think that would be > confusing too, especially since it's now looking like rebase.merges > needs to be a pure boolean and an independent rebase.cousins boolean > option is needed as well. Oh, I wasn't aware of that direction. I do not know why rebase.cousins, which would only be meaningful when rebase.merges is true, is a better design than rebase.merges that is an enum of "don't do the merges stuff" plus "do the merges stuff with cousins", "without cousins" (which may allow us to gain more different ways to do "merges stuff" later), but that is what gained consensus on the list, then "[rebase]merges=" would become a problem. But --rebase-merges from the command line is not a pure Boolean already, so what does "[rebase]merges" that is a pure Boolean aim to help?
On Fri, Feb 24, 2023 at 11:40 AM Junio C Hamano <gitster@pobox.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > The only way to truly make "[rebase] merges =" invalid is to print an > > error message and die with that configuration. I think that would be > > confusing too, especially since it's now looking like rebase.merges > > needs to be a pure boolean and an independent rebase.cousins boolean > > option is needed as well. > > Oh, I wasn't aware of that direction. > > I do not know why rebase.cousins, which would only be meaningful > when rebase.merges is true, is a better design than rebase.merges > that is an enum of "don't do the merges stuff" plus "do the merges > stuff with cousins", "without cousins" (which may allow us to gain > more different ways to do "merges stuff" later), but that is what > gained consensus on the list, then "[rebase]merges=" would become a > problem. > > But --rebase-merges from the command line is not a pure Boolean > already, so what does "[rebase]merges" that is a pure Boolean aim to > help? Phillip is concerned about people and scripts assuming that --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins, see [1]. Tao and others are probably not going to like it if --rebase-merges without an argument undoes a rebase.merges=rebase-cousins configuration. It seems to me that the only way to make everyone happy is to have separate rebase.merges and rebase.cousins options. You have a point that separating the options could cause problems if --rebase-merges starts accepting more arguments in the future, but if that happens we could deal with it by adding more possible values to rebase.cousins or introducing a third config option. -Alex [1] https://lore.kernel.org/git/CAMMLpeQ98BTCGE2tcVdZ99eU6cLh4Rd_hc8C_PmKvsBkjXUWPw@mail.gmail.com/
Alex Henrie <alexhenrie24@gmail.com> writes: > Phillip is concerned about people and scripts assuming that > --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins, > see [1]. Isn't that already broken when you introduce rebase.merges configuration? People and scripts are already relying on the lack of rebase-merges to flatten, and script writers will be surprised to receive a "bug report" complaining that their script does not work when the users set rebase.merges to anything but no. > Tao and others are probably not going to like it if --rebase-merges > without an argument undoes a rebase.merges=rebase-cousins > configuration. That is why I suggested to keep --rebase-merges= (with no value or an empty string) only for those who came from the world where it defaults to no-rebase-cousins and there was no rebase.merges configuration. If --rebase-merges= is given from the command line without value *and* rebase.merges configuration is there (which is Tao's concern?), the command line option can error out asking for an explicit value to countermand whatever value is configured. Wouldn't that work for folks from both camps?
On Fri, Feb 24, 2023 at 12:13 PM Junio C Hamano <gitster@pobox.com> wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > Phillip is concerned about people and scripts assuming that > > --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins, > > see [1]. > > Isn't that already broken when you introduce rebase.merges > configuration? People and scripts are already relying on the lack > of rebase-merges to flatten, and script writers will be surprised to > receive a "bug report" complaining that their script does not work > when the users set rebase.merges to anything but no. Yeah, I don't know why breaking the assumption that --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins is any worse than breaking the assumption that not passing --rebase-merges is equivalent to passing --no-rebase-merges. And the assumption is broken whether one new config option is introduced or two, so splitting the option up probably wouldn't make Phillip any happier. > > Tao and others are probably not going to like it if --rebase-merges > > without an argument undoes a rebase.merges=rebase-cousins > > configuration. > > That is why I suggested to keep --rebase-merges= (with no value or > an empty string) only for those who came from the world where it > defaults to no-rebase-cousins and there was no rebase.merges > configuration. If --rebase-merges= is given from the command line > without value *and* rebase.merges configuration is there (which is > Tao's concern?), the command line option can error out asking for an > explicit value to countermand whatever value is configured. > > Wouldn't that work for folks from both camps? Maybe. I still don't like the idea of --rebase-merges="" ever being not equivalent to rebase.merges="". -Alex
On 24/02/2023 19:13, Junio C Hamano wrote: > Alex Henrie <alexhenrie24@gmail.com> writes: > >> Phillip is concerned about people and scripts assuming that >> --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins, >> see [1]. > > Isn't that already broken when you introduce rebase.merges > configuration? Scripts using --rebase-merges are not broken by the introduction of rebase.merges so long as we follow our usual convention of always allowing the commandline to override the config (i.e. --rebase-merges is always equivalent to --rebase-merges=no-rebase-cousins). I don't really understand why Alex is suggesting splitting the config into two based on my comments. > People and scripts are already relying on the lack > of rebase-merges to flatten, and script writers will be surprised to > receive a "bug report" complaining that their script does not work > when the users set rebase.merges to anything but no. That is true. Best Wishes Phillip >> Tao and others are probably not going to like it if --rebase-merges >> without an argument undoes a rebase.merges=rebase-cousins >> configuration. > > That is why I suggested to keep --rebase-merges= (with no value or > an empty string) only for those who came from the world where it > defaults to no-rebase-cousins and there was no rebase.merges > configuration. If --rebase-merges= is given from the command line > without value *and* rebase.merges configuration is there (which is > Tao's concern?), the command line option can error out asking for an > explicit value to countermand whatever value is configured. > > Wouldn't that work for folks from both camps?
On Fri, Feb 24, 2023 at 12:24 PM Phillip Wood <phillip.wood123@gmail.com> wrote: > > On 24/02/2023 19:13, Junio C Hamano wrote: > > Alex Henrie <alexhenrie24@gmail.com> writes: > > > >> Phillip is concerned about people and scripts assuming that > >> --rebase-merges is equivalent to --rebase-merges=no-rebase-cousins, > >> see [1]. > > > > Isn't that already broken when you introduce rebase.merges > > configuration? > > Scripts using --rebase-merges are not broken by the introduction of > rebase.merges so long as we follow our usual convention of always > allowing the commandline to override the config (i.e. --rebase-merges is > always equivalent to --rebase-merges=no-rebase-cousins). I don't really > understand why Alex is suggesting splitting the config into two based on > my comments. I was thinking that it would be less surprising to users if the option that broke the no-rebase-cousins assumption had "cousins" in its name. I should have stopped to think that that wouldn't really address your concern because regardless of what the option is named, it could still result in surprising behavior. I apologize for the unhelpful suggestion. > > People and scripts are already relying on the lack > > of rebase-merges to flatten, and script writers will be surprised to > > receive a "bug report" complaining that their script does not work > > when the users set rebase.merges to anything but no. > > That is true. In addition to specifying --no-rebase-merges rather than assuming it, shouldn't the "usual convention" for writing scripts also include being explicit about --rebase-merges=rebase-cousins or --rebase-merges=no-rebase-cousins? And if that is the case, is it really much of a loss to let rebase.merges=rebase-cousins override --rebase-merges without an argument? -Alex
diff --git a/builtin/rebase.c b/builtin/rebase.c index 6635f10d52..b68fc2fbb7 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1140,7 +1140,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) {OPTION_STRING, 'r', "rebase-merges", &rebase_merges, N_("mode"), N_("try to rebase merges instead of skipping them"), - PARSE_OPT_OPTARG, NULL, (intptr_t)""}, + PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"}, OPT_BOOL(0, "fork-point", &options.fork_point, N_("use 'merge-base --fork-point' to refine upstream")), OPT_STRING('s', "strategy", &options.strategy, @@ -1437,9 +1437,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) imply_merge(&options, "--exec"); if (rebase_merges) { - if (!*rebase_merges) - ; /* default mode; do nothing */ - else if (!strcmp("rebase-cousins", rebase_merges)) + if (!strcmp("rebase-cousins", rebase_merges)) options.rebase_cousins = 1; else if (strcmp("no-rebase-cousins", rebase_merges)) die(_("Unknown mode: %s"), rebase_merges); diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh index d46d9545f2..c73949df18 100755 --- a/t/t3430-rebase-merges.sh +++ b/t/t3430-rebase-merges.sh @@ -278,6 +278,12 @@ test_expect_success 'do not rebase cousins unless asked for' ' EOF ' +test_expect_success '--rebase-merges="" is invalid syntax' ' + echo "fatal: Unknown mode: " >expect && + test_must_fail git rebase --rebase-merges="" HEAD^ 2>actual && + test_cmp expect actual +' + test_expect_success 'refs/rewritten/* is worktree-local' ' git worktree add wt && cat >wt/script-from-scratch <<-\EOF &&
The unusual syntax --rebase-merges="" (that is, --rebase-merges with an empty string argument) has been an undocumented synonym of --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid confusion when a rebase.merges config option is introduced, where rebase.merges="" will be equivalent to not passing --rebase-merges. Signed-off-by: Alex Henrie <alexhenrie24@gmail.com> --- builtin/rebase.c | 6 ++---- t/t3430-rebase-merges.sh | 6 ++++++ 2 files changed, 8 insertions(+), 4 deletions(-)