{"thread":{"id":"60129","subject":"[PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","startedAt":"2023-08-20T00:11:37Z","lastAt":"2023-09-05T22:02:07Z","messageCount":23,"participants":["Wesley Schwengle","Junio C Hamano","Phillip Wood","Wesley"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"480805","messageId":"20230819203528.562156-1-wesleys@opperschaap.net","threadId":"60129","inReplyTo":null,"subject":"[PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Wesley Schwengle","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-08-19T20:34:48Z","receivedAt":"2023-08-20T00:11:37Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"A couple of years ago I submitted d1e894c6d7 (Document `rebase.forkpoint` in\nrebase man page, 2021-09-16) and during that discussion there was some talk\nabout the behaviour of `git rebase'[1]. During that time I found that the\ndocumentation update was suffice. I wouldn't say it kept me awake at night but\nI do think that `git rebase' with or without an upstream supplied should behave\nthe same in regards to forkpoints. This patch series addresses this behaviour\nchange. It introduces a warning so users will have to set `rebase.forkpoint' in\ntheir configuration. In the future we can remove the warning and opt to pick\n`--no-fork-point' as a default value for `git rebase'.\n\nThere is one point where I'm a little confused, the `test_cmp' function in the\ntestsuite doesn't like the output that is captured from STDERR, it seems that\nthere is a difference in regards to whitespace. My workaround is to use\n`diff -wq`. I don't know if this is an accepted solution.\n\nAnother point of interest is that `git rebase' outputs `Successfully rebased\nand updated refs/heads/foo.' on STDERR and when everything is up to date it\noutputs `Current branch foo is up to date.' on STDOUT. I was a little confused\nby this. Especially since the output on STDOUT can be compared with `test_cmp'.\n\n[1] https://lore.kernel.org/git/xmqqmtocrxwq.fsf@gitster.g/\n\n\n"},{"id":"480806","messageId":"20230819203528.562156-2-wesleys@opperschaap.net","threadId":"60129","inReplyTo":"20230819203528.562156-1-wesleys@opperschaap.net","subject":"[PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Wesley Schwengle","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-08-19T20:34:49Z","receivedAt":"2023-08-20T00:11:37Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,\n2021-09-16) was submitted there was a discussion on if the forkpoint\nbehaviour of `git rebase' was sane. In my experience this wasn't sane.\nGit rebase doesn't work if you don't have an upstream branch configured\n(or something that says `merge = refs/heads/master' in the git config).\nThe behaviour of `git rebase' was that if you supply an upstream on the\ncommand line that it behaves as if `--no-forkpoint' was supplied and if\nyou don't supply an upstream, it behaves as if `--forkpoint' was\nsupplied. This can result in a loss of commits if you don't know that\nand if you don't know about `git reflog' or have other copies of your\nchanges. This can be seen with the following reproduction path:\n\n mkdir reproduction\n cd reproduction\n git init .\n echo \"commit a\" > file.txt\n git add file.txt\n git commit -m \"First commit\" file.txt\n echo \"commit b\" >> file.txt\n git commit -m \"Second commit\" file.txt\n\n git switch -c foo\n echo \"commit c\" >> file.txt\"\n git commit -m \"Third commit\" file.txt\n git branch --set-upstream-to=master\n\n git status\n On branch foo\n Your branch is ahead of 'master' by 1 commit.\n\n git switch master\n git merge foo\n git reset --hard HEAD^\n git switch foo\n Switched to branch 'foo'\n Your branch is ahead of 'master' by 1 commit.\n\n git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'\n 5f427e3 Third commit\n 03ad791 Second commit\n 411e6d4 First commit\n\n git rebase\n git status\n On branch foo\n Your branch is up to date with 'master'.\n\n git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'\n 03ad791 Second commit\n 411e6d4 First commit\n\nThis patch adds a warning where it will indicate that `rebase.forkpoint'\nmust be set in the git configuration and/or that you can supply a\n`--forkpoint' or `--no-forkpoint' command line option to your `git\nrebase' invocation.\n\nSigned-off-by: Wesley Schwengle \n---\n builtin/rebase.c | 15 ++++++++++-\n t/t3431-rebase-fork-point.sh | 50 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 64 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 50cb85751f..41dd9b6256 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1604,8 +1604,21 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t\t\t NULL);\n \t\t\tif (!options.upstream_name)\n \t\t\t\terror_on_missing_default_upstream();\n-\t\t\tif (options.fork_point < 0)\n+\t\t\tif (options.fork_point < 0) {\n+\t\t\t\twarning(_(\n+\t\t\t\t\t\"Rebasing without specifying a forkpoint is discouraged. You can squelch\\n\"\n+\t\t\t\t\t\"this message by running one of the following commands something before your\\n\"\n+\t\t\t\t\t\"next rebase:\\n\"\n+\t\t\t\t\t\"\\n\"\n+\t\t\t\t\t\" git config rebase.forkpoint = false # This will become the new default\\n\"\n+\t\t\t\t\t\" git config rebase.forkpoint = true # This is the old default\\n\"\n+\t\t\t\t\t\"\\n\"\n+\t\t\t\t\t\"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n+\t\t\t\t\t\"preference for all repositories. You can also pass --no-fork-point, --fork-point\\n\"\n+\t\t\t\t\t\"on the command line to override the configured default per invocation.\\n\"\n+\t\t\t\t));\n \t\t\t\toptions.fork_point = 1;\n+\t\t\t}\n \t\t} else {\n \t\t\toptions.upstream_name = argv[0];\n \t\t\targc--;\ndiff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\nindex 4bfc779bb8..a583ca6228 100755\n--- a/t/t3431-rebase-fork-point.sh\n+++ b/t/t3431-rebase-fork-point.sh\n@@ -113,4 +113,54 @@ test_expect_success 'rebase.forkPoint set to true and --root given' '\n \tgit rebase --root\n '\n \n+# The use of the diff -qw is because there is some kind of whitespace character\n+# magic going on which probably has to do with the tabs. It only occurs when we\n+# check STDERR\n+test_expect_success 'rebase without forkpoint' '\n+\tgit init rebase-forkpoint &&\n+\tcd rebase-forkpoint &&\n+\tgit status >/tmp/foo &&\n+\techo \"commit a\" > file.txt &&\n+\tgit add file.txt &&\n+\tgit commit -m \"First commit\" file.txt &&\n+\techo \"commit b\" >> file.txt &&\n+\tgit commit -m \"Second commit\" file.txt &&\n+\tgit switch -c foo &&\n+\techo \"commit c\" >> file.txt &&\n+\tgit commit -m \"Third commit\" file.txt &&\n+\tgit branch --set-upstream-to=main &&\n+\tgit switch main &&\n+\tgit merge foo &&\n+\tgit reset --hard HEAD^ &&\n+\tgit switch foo &&\n+\tcommit=$(git log -n1 --format=\"%h\") &&\n+\tgit rebase >out 2>err &&\n+\ttest_must_be_empty out &&\n+\tcat <<-\\OEF > expect &&\n+\twarning: Rebasing without specifying a forkpoint is discouraged. You can squelch\n+\tthis message by running one of the following commands something before your\n+\tnext rebase:\n+\n+\t git config rebase.forkpoint = false # This will become the new default\n+\t git config rebase.forkpoint = true # This is the old default\n+\n+\tYou can replace \"git config\" with \"git config --global\" to set a default\n+\tpreference for all repositories. You can also pass --no-fork-point, --fork-point\n+\ton the command line to override the configured default per invocation.\n+\n+\tSuccessfully rebased and updated refs/heads/foo.\n+\tOEF\n+\tdiff -qw expect err &&\n+\tgit reset --hard $commit &&\n+\tgit rebase --fork-point >out 2>err &&\n+\ttest_must_be_empty out &&\n+\techo \"Successfully rebased and updated refs/heads/foo.\" > expect &&\n+\tdiff -qw expect err &&\n+\tgit reset --hard $commit &&\n+\tgit rebase --no-fork-point >out 2>err &&\n+\ttest_must_be_empty err &&\n+\techo \"Current branch foo is up to date.\" > expect &&\n+\ttest_cmp out expect\n+'\n+\n test_done\n-- \n2.42.0.rc2.7.gf9972720e9.dirty\n\n"},{"id":"480807","messageId":"20230819203528.562156-3-wesleys@opperschaap.net","threadId":"60129","inReplyTo":"20230819203528.562156-1-wesleys@opperschaap.net","subject":"[PATCH 2/2] git-rebase.txt: Add deprecation notice to the --fork-point options","fromName":"Wesley Schwengle","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-08-19T20:34:50Z","receivedAt":"2023-08-20T00:11:37Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"Signed-off-by: Wesley Schwengle \n---\n Documentation/git-rebase.txt | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex e7b39ad244..e47b58bec2 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -462,7 +462,9 @@ ends up being empty, the `` will be used as a fallback.\n +\n If `` or `--keep-base` is given on the command line, then\n the default is `--no-fork-point`, otherwise the default is\n-`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1].\n+`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1]. This\n+behaviour will be changed in an upcoming release of git. It is advised that you\n+set `rebase.forkpoint` in your config or supply the command line switches.\n +\n If your branch was based on `` but `` was rewound and\n your branch contains commits which were dropped, this option can be used\n-- \n2.42.0.rc2.7.gf9972720e9.dirty\n\n"},{"id":"481238","messageId":"20230831144445.688269-1-wesleys@opperschaap.net","threadId":"60129","inReplyTo":"20230819203528.562156-1-wesleys@opperschaap.net","subject":"Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Wesley Schwengle","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-08-31T14:44:37Z","receivedAt":"2023-08-31T14:45:05Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"> [snip]\n\nI submitted this earlier this month. I think it wasn't noticed because of the\nrelease of v4.24.0. If someone could have a look at this, that would be\nappreciated. \n\nCheers,\nWesley\n"},{"id":"481255","messageId":"xmqqbkenszfa.fsf@gitster.g","threadId":"60129","inReplyTo":"20230819203528.562156-2-wesleys@opperschaap.net","subject":"Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T20:57:13Z","receivedAt":"2023-08-31T20:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wesley Schwengle writes:\n\n> The behaviour of `git rebase' was that if you supply an upstream on the\n> command line that it behaves as if `--no-forkpoint' was supplied and if\n> you don't supply an upstream, it behaves as if `--forkpoint' was\n> supplied.\n\nI actually think it is a reasonable, if a bit too clever (for my\ntaste at least), default for those who do not want to type the\n\"--fork-point\" option from the command line and still want to use\nthat option when they are pulling from or rebasing on the source\nthey usually interact with, while still allowing them to be precise\nwhen they do want to specify exactly what commit they want to base\nit on.\n\nAnd the way how you tell if they are using the \"usual\" source is to\nsee if they used the lazy \"git rebase\" (without arguments) form. So\nI do not think it is particularly a bad design to allow \"git rebase\nmaster\" and \"git rebase\" to behave differently. The latter may use\nthe \"fork point computed using 'master' branch\" (when the current\nbranch is configured to rebuild on top of 'master') while the former\nmay use \"exactly the commit pointed at by the 'master' branch\".\n\n> This can result in a loss of commits if you don't know that\n> and if you don't know about `git reflog' or have other copies of your\n> changes.\n\nSurely, but you would lose commits if you don't know these things\nand explicitly gave the --fork-point option the same way. So I am\nnot sure if switching of the default is warranted.\n\n> -\t\t\tif (options.fork_point < 0)\n> +\t\t\tif (options.fork_point < 0) {\n> +\t\t\t\twarning(_(\n> +\t\t\t\t\t\"Rebasing without specifying a forkpoint is discouraged. You can squelch\\n\"\n> +\t\t\t\t\t\"this message by running one of the following commands something before your\\n\"\n> +\t\t\t\t\t\"next rebase:\\n\"\n> +\t\t\t\t\t\"\\n\"\n> +\t\t\t\t\t\" git config rebase.forkpoint = false # This will become the new default\\n\"\n> +\t\t\t\t\t\" git config rebase.forkpoint = true # This is the old default\\n\"\n> +\t\t\t\t\t\"\\n\"\n\nThe message \"Rebasing without specifying a forkpoint\" reads as if\nyou are encouraging the use of forkpoint mode (which you are not, I\nknow), but then what the message advertises as a future default\nstops not make sense. \"If we hate the forkpoint mode so much to\ndisable it by default, why so we discourage running the command\nwithout specifying it?\" would be the confused message the users will\nread from it.\n\nYour \"git config\" example command lines are not correct, are they?\nThere should be no '=' assignment operator.\n\nI am also afraid that this is giving a way too broad an advice.\n\nWhat you want to discourage is to rebase without specifying what to\nrebase on and without saying if you want or you do not want the\nforkpoint behaviour, which will opt the user into the more dangerous\nforkpoint behaviour. The above makes it sound as if we will\ndiscourage even the more precise \"git rebase \" form, but I\ndo not think it is the case. We would and should not trigger the\nfolk-point behaviour if there is an explicit and the user\ndoes not say \"--fork-point\" from the command line.\n\nHere is my attempt to rewrite the above:\n\n When 'git rebase' is run without specifying on the\n command line, the current default is to use the fork-point\n heuristics, but this is expected to change in a future version\n of Git, and you will have to explicitly give \"--fork-point\" from\n the command line if you keep using the fork-point mode. You can\n run \"git config rebase.forkpoint false\" to adopt the new default\n in advance and that will also squelch the message.\n\nNote that the parsing of \"rebase.forkpoint\" is a bit peculiar in\nthat \n\n - By leaving it unspecified, the .fork_point = -1 in\n REBASE_OPTIONS_INIT takes effect (which is unsurprising);\n\n - By setting it to false, .fork_point becomes 0; but\n\n - If you set the configuration variable to true, .fork_point\n becomes -1, not 1.\n\nAnd this is very much deliberate if I understand it correctly [*1*].\nBy the time we get to this part of the code (i.e. .fork_point is\n-1), the user may already have rebase.forkpoint set to true. IOW,\nsetting it to 'true' is not a valid way to squelch this message.\n\nI am not commenting on the tests, as the above code probably needs\nto be corrected first so that folks who want to squelch the message\nand want the \"forkpoint behaviour by default when rebuilding on the\nusual upstream\" behaviour can do so by setting the variable to true.\n\nAnd that obviously need to be tested, too.\n\nThanks.\n\n\n[References]\n\n*1* https://lore.kernel.org/git/xmqqturbdxi2.fsf@gitster.c.googlers.com/\n"},{"id":"481269","messageId":"xmqq1qfiubg5.fsf@gitster.g","threadId":"60129","inReplyTo":"xmqqbkenszfa.fsf@gitster.g","subject":"Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-31T21:52:10Z","receivedAt":"2023-08-31T21:52:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I am not commenting on the tests, as the above code probably needs\n> to be corrected first so that folks who want to squelch the message\n> and want the \"forkpoint behaviour by default when rebuilding on the\n> usual upstream\" behaviour can do so by setting the variable to true.\n>\n> And that obviously need to be tested, too.\n\nAnother worrysome thing about rebase.forkpoint is that it will be\ninevitable for folks to start complaining that it does not work the\nway other configuration variables do. Setting the variable to\n'true' is not the same as passing '--fork-point=true' from the\ncommand line.\n\nI actually think it would be a lot larger behaviour change with a\nhuge potential to be received as a regression if we start making the\nvariable to mean the same thing as passing '--fork-point=true'.\nPeople may like the current \"if you are rebuilding your branch on\nits usual upstream, pay attention to the rebase and rewind of the\nupstream itself, but if you are giving an explicit upstream from the\ncommand line, the tool does not second guess you with the fork-point\nheuristics\" behaviour and prefer to set it to true. We would be\nbreaking them big time if suddenly the rebase.forkpoint=true they\nset previously starts triggering the fork-point heuristics when they\nrun \"git rebase upstream\". So that needs to be kept in mind when/if\nwe fix the \"setting the variable, even to 'true', will squelch the\nwarning\".\n\n"},{"id":"481287","messageId":"6127b570-5e9b-404f-9802-9135a1c9f31f@gmail.com","threadId":"60129","inReplyTo":"20230819203528.562156-2-wesleys@opperschaap.net","subject":"Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-09-01T13:19:18Z","receivedAt":"2023-09-01T13:19:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Wesley\n\nOn 19/08/2023 21:34, Wesley Schwengle wrote:\n> When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,\n> 2021-09-16) was submitted there was a discussion on if the forkpoint\n> behaviour of `git rebase' was sane. In my experience this wasn't sane.\n> Git rebase doesn't work if you don't have an upstream branch configured\n> (or something that says `merge = refs/heads/master' in the git config).\n> The behaviour of `git rebase' was that if you supply an upstream on the\n> command line that it behaves as if `--no-forkpoint' was supplied and if\n> you don't supply an upstream, it behaves as if `--forkpoint' was\n> supplied. This can result in a loss of commits if you don't know that\n> and if you don't know about `git reflog' or have other copies of your\n> changes. This can be seen with the following reproduction path:\n> \n> mkdir reproduction\n> cd reproduction\n> git init .\n> echo \"commit a\" > file.txt\n> git add file.txt\n> git commit -m \"First commit\" file.txt\n> echo \"commit b\" >> file.txt\n> git commit -m \"Second commit\" file.txt\n> \n> git switch -c foo\n> echo \"commit c\" >> file.txt\"\n> git commit -m \"Third commit\" file.txt\n> git branch --set-upstream-to=master\n> \n> git status\n> On branch foo\n> Your branch is ahead of 'master' by 1 commit.\n> \n> git switch master\n> git merge foo\n\nHere \"git merge\" fast-forwards I think, if instead it created a merge \ncommit there would be no problem as the tip of branch \"foo\" would not \nend up in master's reflog.\n\n> git reset --hard HEAD^\n> git switch foo\n> Switched to branch 'foo'\n> Your branch is ahead of 'master' by 1 commit.\n> \n> git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'\n\nFor a reproduction recipe I think \"git log --oneline\" would suffice.\n\n> 5f427e3 Third commit\n> 03ad791 Second commit\n> 411e6d4 First commit\n> \n> git rebase\n> git status\n> On branch foo\n> Your branch is up to date with 'master'.\n> \n> git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'\n> 03ad791 Second commit\n> 411e6d4 First commit\n\nThanks for the detailed reproduction recipe, I think it would be helpful \nto summarize what's happening in the commit message, especially as it \nseems to depend on \"git merge\" fast-forwarding. Do you often merge a \nbranch into it's upstream and then reset the upstream branch?\n\nI tend to agree with Junio that the current default is pretty \nreasonable. Looking through the links from the cover letter it seems \nthat the current behavior came from a desire for\n\n\tgit fetch && git rebase\n\nto behave like\n\n\tgit pull --rebase\n\nI think the commit message for any change to the default should address \nwhy that is undesirable. Also we should consider what problems may arise \nfrom not defaulting to --fork-point when rebasing on an upstream branch \nthat has itself been rebased or rewound.\n\nBest Wishes\n\nPhillip\n"},{"id":"481289","messageId":"4ee8802b-0b54-4ed3-8ead-61e7d7628bce@gmail.com","threadId":"60129","inReplyTo":"xmqq1qfiubg5.fsf@gitster.g","subject":"Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-09-01T13:33:53Z","receivedAt":"2023-09-01T13:33:57Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 31/08/2023 22:52, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> I am not commenting on the tests, as the above code probably needs\n>> to be corrected first so that folks who want to squelch the message\n>> and want the \"forkpoint behaviour by default when rebuilding on the\n>> usual upstream\" behaviour can do so by setting the variable to true.\n>>\n>> And that obviously need to be tested, too.\n> \n> Another worrysome thing about rebase.forkpoint is that it will be\n> inevitable for folks to start complaining that it does not work the\n> way other configuration variables do. Setting the variable to\n> 'true' is not the same as passing '--fork-point=true' from the\n> command line.\n\nIt does seem strange, it looks like the variable was really added as a \nway to turn off the current default. If we do change the default to \n--no-fork-point when no upstream is given on the commandline then I \nthink we should consider allowing \"auto\" for rebase.forkpoint with the \nsome meaning as \"true\" and recommend that instead.\n\nBest Wishes\n\nPhillip\n\n> I actually think it would be a lot larger behaviour change with a\n> huge potential to be received as a regression if we start making the\n> variable to mean the same thing as passing '--fork-point=true'.\n> People may like the current \"if you are rebuilding your branch on\n> its usual upstream, pay attention to the rebase and rewind of the\n> upstream itself, but if you are giving an explicit upstream from the\n> command line, the tool does not second guess you with the fork-point\n> heuristics\" behaviour and prefer to set it to true. We would be\n> breaking them big time if suddenly the rebase.forkpoint=true they\n> set previously starts triggering the fork-point heuristics when they\n> run \"git rebase upstream\". So that needs to be kept in mind when/if\n> we fix the \"setting the variable, even to 'true', will squelch the\n> warning\".\n> \n\n"},{"id":"481299","messageId":"xmqq5y4trgad.fsf@gitster.g","threadId":"60129","inReplyTo":"4ee8802b-0b54-4ed3-8ead-61e7d7628bce@gmail.com","subject":"Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-01T16:48:10Z","receivedAt":"2023-09-01T16:48:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> It does seem strange, it looks like the variable was really added as a\n> way to turn off the current default. If we do change the default to\n> --no-fork-point when no upstream is given on the commandline then I\n> think we should consider allowing \"auto\" for rebase.forkpoint with the\n> some meaning as \"true\" and recommend that instead.\n\nPerhaps. The current and existing users do not need to change\nanything and 'true' should keep working fine, but given that we are\ndiscouraging the use of fork-point heuristics, it is not clear if it\nmakes sense to entice new users with a new 'auto' synonym, so, I\ndunno\n\nThanks.\n.\n"},{"id":"481302","messageId":"a168fe69-f305-4280-b0e6-9406fbac796f@opperschaap.net","threadId":"60129","inReplyTo":"6127b570-5e9b-404f-9802-9135a1c9f31f@gmail.com","subject":"Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Wesley","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-09-01T17:13:37Z","receivedAt":"2023-09-01T17:14:04Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"\nHello Phillip,\n\n\nOn 9/1/23 09:19, Phillip Wood wrote:\n> Hi Wesley\n> \n> On 19/08/2023 21:34, Wesley Schwengle wrote:\n>> When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,\n>> 2021-09-16) was submitted there was a discussion on if the forkpoint\n>> behaviour of `git rebase' was sane. In my experience this wasn't sane.\n>> Git rebase doesn't work if you don't have an upstream branch configured\n>> (or something that says `merge = refs/heads/master' in the git config).\n>> The behaviour of `git rebase' was that if you supply an upstream on the\n>> command line that it behaves as if `--no-forkpoint' was supplied and if\n>> you don't supply an upstream, it behaves as if `--forkpoint' was\n>> supplied. This can result in a loss of commits if you don't know that\n>> and if you don't know about `git reflog' or have other copies of your\n>> changes. This can be seen with the following reproduction path:\n>>\n>>      mkdir reproduction\n>>      cd reproduction\n>>      git init .\n>>      echo \"commit a\" > file.txt\n>>      git add file.txt\n>>      git commit -m \"First commit\" file.txt\n>>      echo \"commit b\" >> file.txt\n>>      git commit -m \"Second commit\" file.txt\n>>\n>>      git switch -c foo\n>>      echo \"commit c\" >> file.txt\"\n>>      git commit -m \"Third commit\" file.txt\n>>      git branch --set-upstream-to=master\n>>\n>>      git status\n>>      On branch foo\n>>      Your branch is ahead of 'master' by 1 commit.\n>>\n>>      git switch master\n>>      git merge foo\n> \n> Here \"git merge\" fast-forwards I think, if instead it created a merge \n> commit there would be no problem as the tip of branch \"foo\" would not \n> end up in master's reflog.\n\nIf you do\n\ngit merge foo --no-ff\ngit reset --hard HEAD^\ngit switch foo\ngit rebase\n\nYou'll end up with just the commits that are in master. You'll lose all \ncommits from foo.\n\n>>      git log --format='%C(yellow)%h%Creset %Cgreen%s%Creset'\n> \n> For a reproduction recipe I think \"git log --oneline\" would suffice.\n\nWill update, thanks.\n\n >> [snip]\n> \n> Thanks for the detailed reproduction recipe, I think it would be helpful \n> to summarize what's happening in the commit message, especially as it \n> seems to depend on \"git merge\" fast-forwarding. Do you often merge a \n> branch into it's upstream and then reset the upstream branch?\n\nTricky question. When I encountered this behavior I was working on an \nepic/topic branch that I had locally. And I made a commit that I thought \nshould have been in another branch. I moved the commit to another branch \nand than later on rebased it.\n\nI didn't reply to Juno yet, but he refers to the discussion about \n--fork-point and --root command line options. This discussion links to a \nblogpost [*1*] where the same behavior is experienced.\n\nThe quirk is this: --fork-point looks at the reflog and reflog is local. \nMeaning, having an remote upstream branch will make --fork-point a noop. \nOnly where you have an upstream which is local and your reflog has seen \ndropped commits it does something. In all other cases (including \nsupplying the upstream) it behaves as if --no-fork-point was set. If you \ndo the same action in two different clones, you get a different result, \ndepending on what is in your reflog. I find this very tricky behavior \nfor a default. I've set it to false myself, to get a more consistent \nbehavior.\n\nI usually have a remote upstream (gitlab/github) and work with that, so \nthe --fork-point behaviour isn't present because there is no reflog for \nthat, so it behaves as --no-fork-point.\n\nCheers,\nWesley\n\n\n[1]: https://commaok.xyz/post/fork-point/\n-- \nWesley\n\nWhy not both?\n\n"},{"id":"481306","messageId":"xmqqledppxw3.fsf@gitster.g","threadId":"60129","inReplyTo":"a168fe69-f305-4280-b0e6-9406fbac796f@opperschaap.net","subject":"Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-01T18:10:52Z","receivedAt":"2023-09-01T18:11:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wesley writes:\n\n> The quirk is this: --fork-point looks at the reflog and reflog is\n> local. Meaning, having an remote upstream branch will make\n> --fork-point a noop. Only where you have an upstream which is local\n> and your reflog has seen dropped commits it does something.\n\nWhy do you lack reflog on your remote-tracking branches in the first\nplace? \n\nThe fork-point heuristics, as far as I understand it, was invented\nexactly to protect you from your upstream repository rewinding and\nrebuilding the branch you have been building on top of. The default\nfetch refspec +refs/heads/*:refs/remotes/origin/* has the \"force\"\noption \"+\" in front exactly because the fetching repository is\nexpected to keep the reflog for remote-tracking branches to help\nrecovering from such a rewind & rebuild.\n\nPuzzled. \n"},{"id":"481318","messageId":"fa702b47-ae29-4299-9226-4920620b9fff@opperschaap.net","threadId":"60129","inReplyTo":"xmqqledppxw3.fsf@gitster.g","subject":"Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Wesley","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-09-02T01:35:33Z","receivedAt":"2023-09-02T01:37:04Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"On 9/1/23 14:10, Junio C Hamano wrote:\n> Wesley writes:\n> \n>> The quirk is this: --fork-point looks at the reflog and reflog is\n>> local. Meaning, having an remote upstream branch will make\n>> --fork-point a noop. Only where you have an upstream which is local\n>> and your reflog has seen dropped commits it does something.\n> \n> Why do you lack reflog on your remote-tracking branches in the first\n> place?\n\nI do not know? I tested with a bare repo and two clones. And I also \ntested it with just a remote upstream in another branch.\n\nWhen in repo-1 I do the reset --hard HEAD^, and push the results, and \npull them in in repo-2 the behavior doesn't replicate. The git reflog \ncommand doesn't show the reset.\nHowever, if I delete the reflog entry for removal of the reset HEAD^, \ngit rebase exposes the fork-point behavior.\n\n> The fork-point heuristics, as far as I understand it, was invented\n> exactly to protect you from your upstream repository rewinding and\n> rebuilding the branch you have been building on top of. The default\n> fetch refspec +refs/heads/*:refs/remotes/origin/* has the \"force\"\n> option \"+\" in front exactly because the fetching repository is\n> expected to keep the reflog for remote-tracking branches to help\n> recovering from such a rewind & rebuild.\n\nI haven't force pushed anything btw, maybe that could explain things?\n\nCheers,\nWesley\n\n-- \nWesley\n\nWhy not both?\n\n"},{"id":"481335","messageId":"20230902221641.1399624-1-wesleys@opperschaap.net","threadId":"60129","inReplyTo":"xmqq1qfiubg5.fsf@gitster.g","subject":"[PATCH v2] Emit warning when rebasing without a forkpoint","fromName":"Wesley Schwengle","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-09-02T22:16:38Z","receivedAt":"2023-09-02T22:17:05Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"This is the second version of the patch series.\n\nPatch 1: Be able to use rebase.forkpoint and --root\nPatch 2: Adding the warning + tests\nPatch 3: Update documenation\n\nI think I have covered most of your concerns and feedback in this second\nversion.\n\nOn 8/31/23 16:57, Junio C Hamano wrote:\n> Wesley Schwengle writes:\n> \n> Here is my attempt to rewrite the above:\n> \n> When 'git rebase' is run without specifying on the\n> command line, the current default is to use the fork-point\n> heuristics, but this is expected to change in a future version\n> of Git, and you will have to explicitly give \"--fork-point\" from\n> the command line if you keep using the fork-point mode. You can\n> run \"git config rebase.forkpoint false\" to adopt the new default\n> in advance and that will also squelch the message.\n\nI agree. I'll change the text to your version.\n\n> Note that the parsing of \"rebase.forkpoint\" is a bit peculiar in\n> that\n> \n> - By leaving it unspecified, the .fork_point = -1 in\n> REBASE_OPTIONS_INIT takes effect (which is unsurprising);\n> \n> - By setting it to false, .fork_point becomes 0; but\n> \n> - If you set the configuration variable to true, .fork_point\n> becomes -1, not 1.\n\nI changed this in patch 1.\n\n> And this is very much deliberate if I understand it correctly [*1*].\n> By the time we get to this part of the code (i.e. .fork_point is\n> -1), the user may already have rebase.forkpoint set to true. IOW,\n> setting it to 'true' is not a valid way to squelch this message.\n\nSo this works now with patch 2.\n\n> Another worrysome thing about rebase.forkpoint is that it will be\n> inevitable for folks to start complaining that it does not work the\n> way other configuration variables do. Setting the variable to\n> 'true' is not the same as passing '--fork-point=true' from the\n> command line.\n\nI think it is now with the current series.\n\n> I actually think it would be a lot larger behaviour change with a\n> huge potential to be received as a regression if we start making the\n> variable to mean the same thing as passing '--fork-point=true'.\n> People may like the current \"if you are rebuilding your branch on\n> its usual upstream, pay attention to the rebase and rewind of the\n> upstream itself, but if you are giving an explicit upstream from the\n> command line, the tool does not second guess you with the fork-point\n> heuristics\" behaviour and prefer to set it to true. We would be\n> breaking them big time if suddenly the rebase.forkpoint=true they\n> set previously starts triggering the fork-point heuristics when they\n> run \"git rebase upstream\". So that needs to be kept in mind when/if\n> we fix the \"setting the variable, even to 'true', will squelch the\n> warning\".\n\nI get what you are saying. My solution is to make the --fork-point or\n--no-fork-point more explicit. People could use an alias for this?\n\nIt would mean a different approach to the problem and deprecating\nrebase.forkpoint as a boolean value. It could become one of three values:\n\"true\", \"false\" and \"legacy\". Where \"legacy\" can be \"implicit\" or \"auto\".\nAlthough you had some ideas on \"auto\" already. I'm not sure on how I would call\nit. \"no-upstream\"?\n\n-- \nWesley\n\nWhy not both?\n\n\n"},{"id":"481336","messageId":"20230902221641.1399624-3-wesleys@opperschaap.net","threadId":"60129","inReplyTo":"20230902221641.1399624-1-wesleys@opperschaap.net","subject":"[PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Wesley Schwengle","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-09-02T22:16:40Z","receivedAt":"2023-09-02T22:17:05Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"This patch adds a warning where it will indicate that `rebase.forkpoint'\nmust be set in the git configuration and/or that you can supply a\n`--fork-point' or `--no-fork-point' command line option to your `git\nrebase' invocation.\n\nWhen commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,\n2021-09-16) was submitted there was a discussion on if the forkpoint\nbehaviour of `git rebase' was sane. In my experience this wasn't sane.\n\nGit rebase doesn't work if you don't have an upstream branch configured\n(or something that says `merge = refs/heads/master' in the git config).\nYou would than need to use `git rebase ' to rebase. If you\nconfigure an upstream it would seem logical to be able to run `git\nrebase' without arguments. However doing so would trigger a different\nkind of behavior. `git rebase ' behaves as if\n`--no-fork-point' was supplied and without it behaves as if\n`--fork-point' was supplied. This behavior can result in a loss of\ncommits and can surprise users. The following reproduction path exposes\nthis behavior:\n\n git init reproduction\n cd reproduction\n echo \"commit a\" > file.txt\n git add file.txt\n git commit -m \"First commit\" file.txt\n echo \"commit b\" >> file.txt\n git commit -m \"Second commit\" file.txt\n\n git switch -c foo\n echo \"commit c\" >> file.txt\"\n git commit -m \"Third commit\" file.txt\n git branch --set-upstream-to=master\n\n git status\n On branch foo\n Your branch is ahead of 'master' by 1 commit.\n\n git switch master\n git merge foo\n git reset --hard HEAD^\n git switch foo\n Switched to branch 'foo'\n Your branch is ahead of 'master' by 1 commit.\n\n git log --oneline\n 5f427e3 Third commit\n 03ad791 Second commit\n 411e6d4 First commit\n\n git rebase\n git status\n On branch foo\n Your branch is up to date with 'master'.\n\n git log --oneline\n 03ad791 Second commit\n 411e6d4 First commit\n\nSigned-off-by: Wesley Schwengle \n---\n builtin/rebase.c | 16 +++++++++-\n t/t3431-rebase-fork-point.sh | 62 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 77 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 2108001600..ee7db9ba0c 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1608,8 +1608,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\t\t\t NULL);\n \t\t\tif (!options.upstream_name)\n \t\t\t\terror_on_missing_default_upstream();\n-\t\t\tif (options.fork_point < 0)\n+\t\t\tif (options.fork_point < 0) {\n+\t\t\t\twarning(_(\n+\t\t\t\t\t\"When \\\"git rebase\\\" is run without specifying on the\\n\"\n+\t\t\t\t\t\"command line, the current default is to use the fork-point\\n\"\n+\t\t\t\t\t\"heuristics. This is expected to change in a future version\\n\"\n+\t\t\t\t\t\"of Git, and you will have to explicitly give \\\"--fork-point\\\" from\\n\"\n+\t\t\t\t\t\"the command line if you keep using the fork-point mode. You can\\n\"\n+\t\t\t\t\t\"run \\\"git config rebase.forkpoint false\\\" to adopt the new default\\n\"\n+\t\t\t\t\t\"in advance and that will also squelch the message.\\n\"\n+\t\t\t\t\t\"\\n\"\n+\t\t\t\t\t\"You can replace \\\"git config\\\" with \\\"git config --global\\\" to set a default\\n\"\n+\t\t\t\t\t\"preference for all repositories. You can also pass --no-fork-point, --fork-point\\n\"\n+\t\t\t\t\t\"on the command line to override the configured default per invocation.\\n\"\n+\t\t\t\t));\n \t\t\t\toptions.fork_point = 1;\n+\t\t\t}\n \t\t} else {\n \t\t\toptions.upstream_name = argv[0];\n \t\t\targc--;\ndiff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\nindex 4bfc779bb8..908867ae0f 100755\n--- a/t/t3431-rebase-fork-point.sh\n+++ b/t/t3431-rebase-fork-point.sh\n@@ -113,4 +113,66 @@ test_expect_success 'rebase.forkPoint set to true and --root given' '\n \tgit rebase --root\n '\n \n+# The use of the diff -qw is because there is some kind of whitespace character\n+# magic going on which probably has to do with the tabs. It only occurs when we\n+# check STDERR\n+test_expect_success 'rebase without rebase.forkpoint' '\n+\tgit init rebase-forkpoint &&\n+\tcd rebase-forkpoint &&\n+\tgit status >/tmp/foo &&\n+\techo \"commit a\" > file.txt &&\n+\tgit add file.txt &&\n+\tgit commit -m \"First commit\" file.txt &&\n+\techo \"commit b\" >> file.txt &&\n+\tgit commit -m \"Second commit\" file.txt &&\n+\tgit switch -c foo &&\n+\techo \"commit c\" >> file.txt &&\n+\tgit commit -m \"Third commit\" file.txt &&\n+\tgit branch --set-upstream-to=main &&\n+\tgit switch main &&\n+\tgit merge foo &&\n+\tgit reset --hard HEAD^ &&\n+\tgit switch foo &&\n+\tcommit=$(git log -n1 --format=\"%h\") &&\n+\tgit rebase >out 2>err &&\n+\ttest_must_be_empty out &&\n+\tcat <<-\\OEF > expect &&\n+\twarning: When \"git rebase\" is run without specifying on the\n+\tcommand line, the current default is to use the fork-point\n+\theuristics. This is expected to change in a future version\n+\tof Git, and you will have to explicitly give \"--fork-point\" from\n+\tthe command line if you keep using the fork-point mode. You can\n+\trun \"git config rebase.forkpoint false\" to adopt the new default\n+\tin advance and that will also squelch the message.\n+\n+\tYou can replace \"git config\" with \"git config --global\" to set a default\n+\tpreference for all repositories. You can also pass --no-fork-point, --fork-point\n+\ton the command line to override the configured default per invocation.\n+\n+\tSuccessfully rebased and updated refs/heads/foo.\n+\tOEF\n+\tdiff -qw expect err &&\n+\tgit reset --hard $commit &&\n+\tgit rebase --fork-point >out 2>err &&\n+\ttest_must_be_empty out &&\n+\techo \"Successfully rebased and updated refs/heads/foo.\" > expect &&\n+\tdiff -qw expect err &&\n+\tgit reset --hard $commit &&\n+\tgit rebase --no-fork-point >out 2>err &&\n+\ttest_must_be_empty err &&\n+\techo \"Current branch foo is up to date.\" > expect &&\n+\ttest_cmp out expect &&\n+\tgit config --add rebase.forkpoint true &&\n+\tgit rebase >out 2>err &&\n+\ttest_must_be_empty out &&\n+\techo \"Successfully rebased and updated refs/heads/foo.\" > expect &&\n+\tdiff -qw expect err &&\n+\tgit reset --hard $commit &&\n+\tgit config --replace-all rebase.forkpoint false &&\n+\tgit rebase >out 2>err &&\n+\ttest_must_be_empty err &&\n+\techo \"Current branch foo is up to date.\" > expect &&\n+\ttest_cmp out expect\n+'\n+\n test_done\n-- \n2.42.0.103.g5622fd1409.dirty\n\n"},{"id":"481337","messageId":"20230902221641.1399624-2-wesleys@opperschaap.net","threadId":"60129","inReplyTo":"20230902221641.1399624-1-wesleys@opperschaap.net","subject":"[PATCH v2 1/3] rebase.c: Make a distiction between rebase.forkpoint and --fork-point arguments","fromName":"Wesley Schwengle","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-09-02T22:16:39Z","receivedAt":"2023-09-02T22:17:07Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"When you call `git rebase --root' we are not interested in the\nrebase.forkpoint configuration. The two options are not to be combined.\n\nBecause the implementation checks if the configured value for using a\nforkpoint > 0 I've opted to give the configured forkpoint the value 2.\nIf the user supplies --fork-point on the command line this has a value\nof 1. Now we can make a distinction between user input and the configured\nvalue of rebase.forkpoint.\n\nSigned-off-by: Wesley Schwengle \n---\n builtin/rebase.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 50cb85751f..2108001600 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -824,7 +824,7 @@ static int rebase_config(const char *var, const char *value,\n \t}\n \n \tif (!strcmp(var, \"rebase.forkpoint\")) {\n-\t\topts->fork_point = git_config_bool(var, value) ? -1 : 0;\n+\t\topts->fork_point = git_config_bool(var, value) ? 2 : 0;\n \t\treturn 0;\n \t}\n \n@@ -1264,8 +1264,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tif (options.fork_point < 0)\n \t\t\toptions.fork_point = 0;\n \t}\n-\tif (options.root && options.fork_point > 0)\n+\tif (options.root && options.fork_point == 1) {\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--root\", \"--fork-point\");\n+\t} else if (options.root && options.fork_point > 1) {\n+\t options.fork_point = 0;\n+\t}\n+\n \n \tif (options.action != ACTION_NONE && !in_progress)\n \t\tdie(_(\"No rebase in progress?\"));\n-- \n2.42.0.103.g5622fd1409.dirty\n\n"},{"id":"481338","messageId":"20230902221641.1399624-4-wesleys@opperschaap.net","threadId":"60129","inReplyTo":"20230902221641.1399624-1-wesleys@opperschaap.net","subject":"[PATCH v2 3/3] git-rebase.txt: Add deprecation notice to the --fork-point options","fromName":"Wesley Schwengle","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-09-02T22:16:41Z","receivedAt":"2023-09-02T22:17:11Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"Signed-off-by: Wesley Schwengle \n---\n Documentation/git-rebase.txt | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex e7b39ad244..e47b58bec2 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -462,7 +462,9 @@ ends up being empty, the `` will be used as a fallback.\n +\n If `` or `--keep-base` is given on the command line, then\n the default is `--no-fork-point`, otherwise the default is\n-`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1].\n+`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1]. This\n+behaviour will be changed in an upcoming release of git. It is advised that you\n+set `rebase.forkpoint` in your config or supply the command line switches.\n +\n If your branch was based on `` but `` was rewound and\n your branch contains commits which were dropped, this option can be used\n-- \n2.42.0.103.g5622fd1409.dirty\n\n"},{"id":"481341","messageId":"xmqqo7ikkxrq.fsf@gitster.g","threadId":"60129","inReplyTo":"fa702b47-ae29-4299-9226-4920620b9fff@opperschaap.net","subject":"Re: [PATCH 1/2] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-02T22:36:57Z","receivedAt":"2023-09-02T22:37:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wesley writes:\n\n> On 9/1/23 14:10, Junio C Hamano wrote:\n>> Wesley writes:\n>> \n>>> The quirk is this: --fork-point looks at the reflog and reflog is\n>>> local. Meaning, having an remote upstream branch will make\n>>> --fork-point a noop. Only where you have an upstream which is local\n>>> and your reflog has seen dropped commits it does something.\n>> Why do you lack reflog on your remote-tracking branches in the first\n>> place?\n>\n> I do not know? I tested with a bare repo and two clones. And I also\n> tested it with just a remote upstream in another branch.\n\nIIRC, a non-bare repository (i.e. with working tree) should get\ncore.logallrefupdates set to true by default, so all your refs, not\njust local and remote-tracking branches, should have records.\n\n> I haven't force pushed anything btw, maybe that could explain things?\n\nIf your \"remote\" is never force-pushed, then the movements of refs\nat the remote (which you will observe whenever you fetch from it)\nwill always fast-forward, and the remote-tracking branches in your\nlocal repository that keeps track of the movement will also record\nthe fast-forwarding movement in the reflog. But then there is no\nneed for the fork-point heurisitics to trigger, and even if it\ntriggered the heuristics would not change the outcome, when rebasing\nagainst such a remote branch, as their tip will always a decendant\nof all commits that ever sat at the tip of that remote branch.\n\n"},{"id":"481342","messageId":"xmqq4jkckuy7.fsf@gitster.g","threadId":"60129","inReplyTo":"20230902221641.1399624-3-wesleys@opperschaap.net","subject":"Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-02T23:37:52Z","receivedAt":"2023-09-02T23:37:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wesley Schwengle writes:\n\n> Subject: Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint\n\n(applies to all three patches) downcase \"Emit\" after the : prefix.\n\n> This patch adds a warning where it will indicate that `rebase.forkpoint'\n> must be set in the git configuration and/or that you can supply a\n> `--fork-point' or `--no-fork-point' command line option to your `git\n> rebase' invocation.\n>\n> When commit d1e894c6d7 (Document `rebase.forkpoint` in rebase man page,\n> 2021-09-16) was submitted there was a discussion on if the forkpoint\n> behaviour of `git rebase' was sane. In my experience this wasn't sane.\n\nI already said that the above is not true, so I will not repeat myself.\n\n> Git rebase doesn't work if you don't have an upstream branch configured\n\n\"git rebase foo\" works just fine, so this statement needs a lot of\ntightening.\n\n> (or something that says `merge = refs/heads/master' in the git config).\n> You would than need to use `git rebase ' to rebase. If you\n> configure an upstream it would seem logical to be able to run `git\n> rebase' without arguments. However doing so would trigger a different\n> kind of behavior. `git rebase ' behaves as if\n> `--no-fork-point' was supplied and without it behaves as if\n> `--fork-point' was supplied. This behavior can result in a loss of\n> commits and can surprise users.\n\nNo, what is causing the loss in this particular case is allowing to\nuse the fork-point heuristics. If you do not want it, you can\neither explicitly give --no-fork-point or (or both if you\nfeel that you need to absolutely be clear). Or you can set the\nconfiguration to \"false\" to disable this \"auto\" behaviour.\n\n> The following reproduction path exposes\n> this behavior:\n\nI actually do not think having this example in the proposed log\nmessage adds more value than it distracts readers from the real\npoint of this change.\n\nIf you rewind to lose commits from the branch you are (re)building\nagainst, and what was rewound and discarded was part of the work you\nare building, whether it is on a local branch or on a remote branch\nthat contains what you have already pushed, they will be discarded,\nit is by design, and it is a known deficiency with the fork-point\nheuristics. How the fork-point heuristics breaks down is rather\nwell known and it is pretty much orthogonal to the point of this\npatch, which is to make it harder to trigger by folks who are not\nfamiliar with \"git rebase\" and yet try to be lazy by not specifying\nthe from the command line.\n\nBy the way, while I do agree with the need to make users _aware_ of\nthe \"auto\" behaviour [*1*], I am not yet convinced that there is a\nneed to change the default in the future.\n\n\tSide note: It allows those who originally advocated the\n\tfork-point heuristics to be extra lazy and allow fork-point\n\theuristics to be used when they rebuild on top of what they\n\tusually rebuild on (and the \"usually\" part is signalled by\n\tusing \"git rebase\" without saying what to build on from the\n\tcommand line). The default allows them not to worry about\n\tthe heuristics to kick in when they explicitly say on which\n\texact commit they want to rebuild on.\n\nAnd when we do not know if the default will change, the new warning\nmessage will lose value. Many of those who see the message are\nalready familiar with when the forkpoint heuristics will kick in,\nand those who weren't familiar with will not know what the default\nchange is about, without consulting the documentation.\n\nIt might be better to extend the documentation instead, which will\nnot distract those who are using the tool just fine already.\n\n> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\n> index 4bfc779bb8..908867ae0f 100755\n> --- a/t/t3431-rebase-fork-point.sh\n> +++ b/t/t3431-rebase-fork-point.sh\n> @@ -113,4 +113,66 @@ test_expect_success 'rebase.forkPoint set to true and --root given' '\n> \tgit rebase --root\n> '\n> \n> +# The use of the diff -qw is because there is some kind of whitespace character\n> +# magic going on which probably has to do with the tabs. It only occurs when we\n> +# check STDERR\n> +test_expect_success 'rebase without rebase.forkpoint' '\n> +\tgit init rebase-forkpoint &&\n> +\tcd rebase-forkpoint &&\n> +\tgit status >/tmp/foo &&\n> +\techo \"commit a\" > file.txt &&\n\nStyle???\n\n> +\tgit add file.txt &&\n> +\tgit commit -m \"First commit\" file.txt &&\n> +\techo \"commit b\" >> file.txt &&\n> +\tgit commit -m \"Second commit\" file.txt &&\n> +\tgit switch -c foo &&\n> +\techo \"commit c\" >> file.txt &&\n> +\tgit commit -m \"Third commit\" file.txt &&\n> +\tgit branch --set-upstream-to=main &&\n> +\tgit switch main &&\n> +\tgit merge foo &&\n> +\tgit reset --hard HEAD^ &&\n> +\tgit switch foo &&\n> +\tcommit=$(git log -n1 --format=\"%h\") &&\n> +\tgit rebase >out 2>err &&\n> +\ttest_must_be_empty out &&\n> +\tcat <<-\\OEF > expect &&\n\nWhy does this have to be orgiinal in such a strange way? When\neverybody else uses string \"EOF\" as the end-of-here-doc-marker, and\nif there is no downside to use the same string here, we should just\nuse the same \"EOF\" to avoid distracting readers.\n\n> +\twarning: When \"git rebase\" is run without specifying on the\n> +\tcommand line, the current default is to use the fork-point\n> +\theuristics. This is expected to change in a future version\n> +\tof Git, and you will have to explicitly give \"--fork-point\" from\n> +\tthe command line if you keep using the fork-point mode. You can\n> +\trun \"git config rebase.forkpoint false\" to adopt the new default\n> +\tin advance and that will also squelch the message.\n> +\n> +\tYou can replace \"git config\" with \"git config --global\" to set a default\n> +\tpreference for all repositories. You can also pass --no-fork-point, --fork-point\n> +\ton the command line to override the configured default per invocation.\n> +\n> +\tSuccessfully rebased and updated refs/heads/foo.\n> +\tOEF\n> +\tdiff -qw expect err &&\n\nWhy not \"test_cmp expect actual\" like everybody else?\n\n> +...\n> +\techo \"Current branch foo is up to date.\" > expect &&\n> +\ttest_cmp out expect\n> +'\n> +\n> test_done\n"},{"id":"481344","messageId":"8354f569-dbbe-4d01-95af-0d23a949c22d@opperschaap.net","threadId":"60129","inReplyTo":"xmqq4jkckuy7.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Wesley","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-09-03T02:29:01Z","receivedAt":"2023-09-03T02:29:19Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"On 9/2/23 19:37, Junio C Hamano wrote:\n> Wesley Schwengle writes:\n\nThanks for the feedback. I won't continue the patch series because some \nof the feedback you've given below.\n\n>> However doing so would trigger a different\n>> kind of behavior. `git rebase ' behaves as if\n>> `--no-fork-point' was supplied and without it behaves as if\n>> `--fork-point' was supplied. This behavior can result in a loss of\n>> commits and can surprise users.\n> \n> No, what is causing the loss in this particular case is allowing to\n> use the fork-point heuristics. If you do not want it, you can\n> either explicitly give --no-fork-point or (or both if you\n> feel that you need to absolutely be clear). Or you can set the\n> configuration to \"false\" to disable this \"auto\" behaviour.\n\nIsn't that what I'm saying? At least I'm trying to say what you are saying.\n\n> By the way, while I do agree with the need to make users _aware_ of\n> the \"auto\" behaviour [*1*], I am not yet convinced that there is a\n> need to change the default in the future.\n\nIn that case, I'll abort this patch series. I don't agree with the `git \nrebase' in the lazy form and `git rebase ' acting differently, \nbut I already have the rebase.forkpoint set to false to counter it.\n\n> It might be better to extend the documentation instead, which will\n> not distract those who are using the tool just fine already.\n\nThat is with the current viewpoints the best option I think.\n\n>> +\tdiff -qw expect err &&\n> \n> Why not \"test_cmp expect actual\" like everybody else?\n\nAs said in the initial patch series and the comment above the tests:\n\n> There is one point where I'm a little confused, the `test_cmp' function in the\n> testsuite doesn't like the output that is captured from STDERR, it seems that\n> there is a difference in regards to whitespace. My workaround is to use\n> `diff -wq`. I don't know if this is an accepted solution.\n\nThat's why.\n\nCheers,\nWesley\n\n-- \nWesley\n\nWhy not both?\n\n"},{"id":"481346","messageId":"xmqqlednuagl.fsf@gitster.g","threadId":"60129","inReplyTo":"xmqq4jkckuy7.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-03T04:50:18Z","receivedAt":"2023-09-03T04:50:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> If you rewind to lose commits from the branch you are (re)building\n> against, and what was rewound and discarded was part of the work you\n> are building, whether it is on a local branch or on a remote branch\n> that contains what you have already pushed, they will be discarded,\n> it is by design, and it is a known deficiency with the fork-point\n> heuristics. How the fork-point heuristics breaks down is rather\n> well known ...\n\nAnother tangent, this time very closely related to this topic, is\nthat it may be worth warning when the fork-point heuristics chooses\nthe base commit that is different from the original upstream,\nregardless of how we ended up using fork-point heuristics.\n\nExperienced users may not be confused when the heuristics kicks in\nand when it does not (e.g. because they configured, because they\nused the \"lazy\" form, or because they gave \"--fork-point\" from the\ncommand line explicitly), but they still may get surprising results\nif a reflog entry chosen to be used as the base by the heuristics is\nnot what they expected to be used, and can lose their work that way.\nImagine that you pushed your work to the remote that is a shared\nrepository, and then continued building on top of it, while others\nrewound the remote branch to eject your work, and your \"git fetch\"\nupdated the remote-tracking branch. You'll be pretty much in the\nsame situation you had in your reproduction recipe that rewound your\nown local branch that you used to build your derived work on and\nwould lose your work the same way, if you do not notice that the\nremote branch has been rewound (and the fork-point heuristics chose\na \"wrong\" commit from the reflog of your remote-tracking branch.\n\nPerhaps something along the lines of this (not even compile tested,\nthough)... It might even be useful to show a shortlog between the\n.restrict_revision and .upstream, which is the list of commits that\nis potentially lost, but that might turn out to be excessively loud\nand noisy in the workflow of those who do benefit from the\nfork-point heuristics because their project rewinds branches too\noften and too wildly for them to manually keep track of. I dunno.\n\n\n builtin/rebase.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git c/builtin/rebase.c w/builtin/rebase.c\nindex 50cb85751f..432a97e205 100644\n--- c/builtin/rebase.c\n+++ w/builtin/rebase.c\n@@ -1721,9 +1721,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (keep_base && options.reapply_cherry_picks)\n \t\toptions.upstream = options.onto;\n \n-\tif (options.fork_point > 0)\n+\tif (options.fork_point > 0) {\n \t\toptions.restrict_revision =\n \t\t\tget_fork_point(options.upstream_name, options.orig_head);\n+\t\tif (options.restrict_revision &&\n+\t\t options.restrict_revision != options.upstream)\n+\t\t\twarning(_(\"fork-point heuristics using %s from the reflog of %s\"),\n+\t\t\t\toid_to_hex(&options.restrict_revision->object.oid),\n+\t\t\t\toptions.upstream_name);\n+\t}\n \n \tif (repo_read_index(the_repository) < 0)\n \t\tdie(_(\"could not read index\"));\n\n"},{"id":"481350","messageId":"2e1f1d2a-4aec-4b60-bc96-685a27055c06@opperschaap.net","threadId":"60129","inReplyTo":"xmqqlednuagl.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Wesley Schwengle","fromEmail":"wesleys@opperschaap.net","sentAt":"2023-09-03T12:34:54Z","receivedAt":"2023-09-03T12:35:13Z","isPatch":true,"sender":{"key":"wesleys@opperschaap.net","avatar":"https://avatars.githubusercontent.com/u/6317502?v=4"},"body":"On 9/3/23 00:50, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> If you rewind to lose commits from the branch you are (re)building\n>> against, and what was rewound and discarded was part of the work you\n>> are building, whether it is on a local branch or on a remote branch\n>> that contains what you have already pushed, they will be discarded,\n>> it is by design, and it is a known deficiency with the fork-point\n>> heuristics. How the fork-point heuristics breaks down is rather\n>> well known ...\n> \n> Another tangent, this time very closely related to this topic, is\n> that it may be worth warning when the fork-point heuristics chooses\n> the base commit that is different from the original upstream,\n> regardless of how we ended up using fork-point heuristics.\n> \n> [snip]\n> \n> Perhaps something along the lines of this (not even compile tested,\n> though)... It might even be useful to show a shortlog between the\n> .restrict_revision and .upstream, which is the list of commits that\n> is potentially lost, but that might turn out to be excessively loud\n> and noisy in the workflow of those who do benefit from the\n> fork-point heuristics because their project rewinds branches too\n> often and too wildly for them to manually keep track of. I dunno.\n\nI like the idea of the warning, but it could be loud indeed and you'll \nwant to turn it off in that case.\n\n-- \nWesley\n"},{"id":"481365","messageId":"d9710161-ddb8-4b0a-9729-6b54cd56427d@gmail.com","threadId":"60129","inReplyTo":"xmqqlednuagl.fsf@gitster.g","subject":"Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-09-04T10:16:28Z","receivedAt":"2023-09-04T10:16:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 03/09/2023 05:50, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> If you rewind to lose commits from the branch you are (re)building\n>> against, and what was rewound and discarded was part of the work you\n>> are building, whether it is on a local branch or on a remote branch\n>> that contains what you have already pushed, they will be discarded,\n>> it is by design, and it is a known deficiency with the fork-point\n>> heuristics. How the fork-point heuristics breaks down is rather\n>> well known ...\n> \n> Another tangent, this time very closely related to this topic, is\n> that it may be worth warning when the fork-point heuristics chooses\n> the base commit that is different from the original upstream,\n> regardless of how we ended up using fork-point heuristics.\n\nI think that is a good idea and would help to mitigate the surprise that \nsome users have expressed when --fork-point kicks and they didn't know \nabout it. I think we may want to compare \"branch_base\" which holds the \nmerge-base of HEAD and upstream with \"restrict_revision\" to decide when \nto warn.\n\nBest Wishes\n\nPhillip\n\n> Experienced users may not be confused when the heuristics kicks in\n> and when it does not (e.g. because they configured, because they\n> used the \"lazy\" form, or because they gave \"--fork-point\" from the\n> command line explicitly), but they still may get surprising results\n> if a reflog entry chosen to be used as the base by the heuristics is\n> not what they expected to be used, and can lose their work that way.\n> Imagine that you pushed your work to the remote that is a shared\n> repository, and then continued building on top of it, while others\n> rewound the remote branch to eject your work, and your \"git fetch\"\n> updated the remote-tracking branch. You'll be pretty much in the\n> same situation you had in your reproduction recipe that rewound your\n> own local branch that you used to build your derived work on and\n> would lose your work the same way, if you do not notice that the\n> remote branch has been rewound (and the fork-point heuristics chose\n> a \"wrong\" commit from the reflog of your remote-tracking branch.\n> \n> Perhaps something along the lines of this (not even compile tested,\n> though)... It might even be useful to show a shortlog between the\n> .restrict_revision and .upstream, which is the list of commits that\n> is potentially lost, but that might turn out to be excessively loud\n> and noisy in the workflow of those who do benefit from the\n> fork-point heuristics because their project rewinds branches too\n> often and too wildly for them to manually keep track of. I dunno.\n> \n> \n> builtin/rebase.c | 8 +++++++-\n> 1 file changed, 7 insertions(+), 1 deletion(-)\n> \n> diff --git c/builtin/rebase.c w/builtin/rebase.c\n> index 50cb85751f..432a97e205 100644\n> --- c/builtin/rebase.c\n> +++ w/builtin/rebase.c\n> @@ -1721,9 +1721,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tif (keep_base && options.reapply_cherry_picks)\n> \t\toptions.upstream = options.onto;\n> \n> -\tif (options.fork_point > 0)\n> +\tif (options.fork_point > 0) {\n> \t\toptions.restrict_revision =\n> \t\t\tget_fork_point(options.upstream_name, options.orig_head);\n> +\t\tif (options.restrict_revision &&\n> +\t\t options.restrict_revision != options.upstream)\n> +\t\t\twarning(_(\"fork-point heuristics using %s from the reflog of %s\"),\n> +\t\t\t\toid_to_hex(&options.restrict_revision->object.oid),\n> +\t\t\t\toptions.upstream_name);\n> +\t}\n> \n> \tif (repo_read_index(the_repository) < 0)\n> \t\tdie(_(\"could not read index\"));\n> \n\n"},{"id":"481415","messageId":"xmqqfs3ss2i1.fsf@gitster.g","threadId":"60129","inReplyTo":"2e1f1d2a-4aec-4b60-bc96-685a27055c06@opperschaap.net","subject":"Re: [PATCH v2 2/3] builtin/rebase.c: Emit warning when rebasing without a forkpoint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-05T22:01:58Z","receivedAt":"2023-09-05T22:02:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Wesley Schwengle writes:\n\n> I like the idea of the warning, but it could be loud indeed and you'll\n> want to turn it off in that case.\n\nI tend to think that a single-liner warning would not be too\nintrusive (it might actually be too subtle to be noticed),\nespecially given that it is issued only when the fork-point does\nmove the target commit from what was given.\n\nGiving a shortlog of what is lost in the history does sound like a\nbit too loud, I am afraind, though.\n"}]}