{"thread":{"id":"50505","subject":"BUG: 2.11-era rebase regression when @{upstream} is implicitly used","startedAt":"2019-02-14T13:23:10Z","lastAt":"2019-02-24T10:10:33Z","messageCount":12,"participants":["Ævar Arnfjörð Bjarmason","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"369310","messageId":"877ee2jyh3.fsf@evledraar.gmail.com","threadId":"50505","inReplyTo":null,"subject":"BUG: 2.11-era rebase regression when @{upstream} is implicitly used","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-14T13:23:04Z","receivedAt":"2019-02-14T13:23:10Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This is not a 2.21 release issue, and pre-dates the built-in rebase.\n\nWhen you clone any repository, e.g. git.git, and add one commit on top\nof the cloned branch, then run \"git rebase\" you'll get e.g.:\n\n    $ git rebase\n    First, rewinding head to replay your work on top of it...\n    Applying: foo\n\nBefore 4f21454b55 (\"merge-base: handle --fork-point without reflog\",\n2016-10-12) you'd get:\n\n    $ git rebase\n    Current branch master is up to date.\n\nThe results are not the same for \"git rebase @{u}\" or \"git rebase $(git\nrev-parse @{u})\":\n\n    $ git rev-parse HEAD; ~/g/git/git --exec-path=/home/avar/g/git rebase; git rev-parse HEAD; ~/g/git/git --exec-path=/home/avar/g/git rebase @{u}; git rev-parse HEAD; ~/g/git/git --exec-path=/home/avar/g/git rebase $(git rev-parse @{u}); git rev-parse HEAD\n    d0a1e49341cac6db3226eb0f76ec4a5912f18af8\n    First, rewinding head to replay your work on top of it...\n    Applying: foo\n    3a9261d6e34d9f6d00c8e8411d7ddd8cffa02d97\n    Current branch master is up to date.\n    3a9261d6e34d9f6d00c8e8411d7ddd8cffa02d97\n    Current branch master is up to date.\n    3a9261d6e34d9f6d00c8e8411d7ddd8cffa02d97\n\nWith 4f21454b55^ checked-out the SHA-1 always stays the same, i.e. no\nwork is done for the same command:\n\n    69bd93d9aa438c6d903b8e62c3bf1c6c5ab8ec0b\n    Current branch master is up to date.\n    69bd93d9aa438c6d903b8e62c3bf1c6c5ab8ec0b\n    Current branch master is up to date.\n    69bd93d9aa438c6d903b8e62c3bf1c6c5ab8ec0b\n    Current branch master is up to date.\n    69bd93d9aa438c6d903b8e62c3bf1c6c5ab8ec0b\n"},{"id":"369814","messageId":"20190221141042.GA21737@sigill.intra.peff.net","threadId":"50505","inReplyTo":"877ee2jyh3.fsf@evledraar.gmail.com","subject":"Re: BUG: 2.11-era rebase regression when @{upstream} is implicitly used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-21T14:10:42Z","receivedAt":"2019-02-21T14:10:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 14, 2019 at 02:23:04PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> This is not a 2.21 release issue, and pre-dates the built-in rebase.\n> \n> When you clone any repository, e.g. git.git, and add one commit on top\n> of the cloned branch, then run \"git rebase\" you'll get e.g.:\n> \n>     $ git rebase\n>     First, rewinding head to replay your work on top of it...\n>     Applying: foo\n> \n> Before 4f21454b55 (\"merge-base: handle --fork-point without reflog\",\n> 2016-10-12) you'd get:\n> \n>     $ git rebase\n>     Current branch master is up to date.\n\nI'm not entirely sure this is a regression, and not the patch bringing\nthe behavior into line with what would happen when you _do_ have a\nreflog.\n\n> The results are not the same for \"git rebase @{u}\" or \"git rebase $(git\n> rev-parse @{u})\":\n\nThose aren't using \"--fork-point\", so they're going to behave\ndifferently. The default with no arguments is basically \"--fork-point\n@{u}\".\n\n-Peff\n"},{"id":"369816","messageId":"87bm355h6p.fsf@evledraar.gmail.com","threadId":"50505","inReplyTo":"20190221141042.GA21737@sigill.intra.peff.net","subject":"Re: BUG: 2.11-era rebase regression when @{upstream} is implicitly used","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T14:50:38Z","receivedAt":"2019-02-21T14:50:43Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Feb 21 2019, Jeff King wrote:\n\n> On Thu, Feb 14, 2019 at 02:23:04PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> This is not a 2.21 release issue, and pre-dates the built-in rebase.\n>>\n>> When you clone any repository, e.g. git.git, and add one commit on top\n>> of the cloned branch, then run \"git rebase\" you'll get e.g.:\n>>\n>>     $ git rebase\n>>     First, rewinding head to replay your work on top of it...\n>>     Applying: foo\n>>\n>> Before 4f21454b55 (\"merge-base: handle --fork-point without reflog\",\n>> 2016-10-12) you'd get:\n>>\n>>     $ git rebase\n>>     Current branch master is up to date.\n>\n> I'm not entirely sure this is a regression, and not the patch bringing\n> the behavior into line with what would happen when you _do_ have a\n> reflog.\n>\n>> The results are not the same for \"git rebase @{u}\" or \"git rebase $(git\n>> rev-parse @{u})\":\n>\n> Those aren't using \"--fork-point\", so they're going to behave\n> differently. The default with no arguments is basically \"--fork-point\n> @{u}\".\n\nYeah, that's what it *should* do, but it's not equivalent to using\n--fork-point manually:\n\n    # my series on top of origin/master\n    $ git rev-parse HEAD\n    2a67977d3f70fa7fc4bce89db24a1218dc9ab2aa\n    \n    # Junio's origin/master upstream\n    $ git rev-parse @{u}\n    35ee755a8c43bcb3c2786522d423f006c23d32df\n    \n    # Where my fork point is\n    $ git merge-base --fork-point @{u}\n    35ee755a8c43bcb3c2786522d423f006c23d32df\n    \n    # OK\n    $ git rebase 35ee755a8c43bcb3c2786522d423f006c23d32df\n    Current branch master is up to date.\n    \n    # OK\n    $ git rebase $(git merge-base --fork-point @{u})\n    Current branch master is up to date.\n    \n    # ???\n    $ git rebase\n    First, rewinding head to replay your work on top of it...\n    [...]\n"},{"id":"369817","messageId":"20190221151009.GA24309@sigill.intra.peff.net","threadId":"50505","inReplyTo":"87bm355h6p.fsf@evledraar.gmail.com","subject":"Re: BUG: 2.11-era rebase regression when @{upstream} is implicitly used","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-21T15:10:10Z","receivedAt":"2019-02-21T15:10:13Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2019 at 03:50:38PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > Those aren't using \"--fork-point\", so they're going to behave\n> > differently. The default with no arguments is basically \"--fork-point\n> > @{u}\".\n> \n> Yeah, that's what it *should* do, but it's not equivalent to using\n> --fork-point manually:\n> \n>     # my series on top of origin/master\n>     $ git rev-parse HEAD\n>     2a67977d3f70fa7fc4bce89db24a1218dc9ab2aa\n>     \n>     # Junio's origin/master upstream\n>     $ git rev-parse @{u}\n>     35ee755a8c43bcb3c2786522d423f006c23d32df\n>     \n>     # Where my fork point is\n>     $ git merge-base --fork-point @{u}\n>     35ee755a8c43bcb3c2786522d423f006c23d32df\n>     \n>     # OK\n>     $ git rebase 35ee755a8c43bcb3c2786522d423f006c23d32df\n>     Current branch master is up to date.\n>     \n>     # OK\n>     $ git rebase $(git merge-base --fork-point @{u})\n>     Current branch master is up to date.\n>     \n>     # ???\n>     $ git rebase\n>     First, rewinding head to replay your work on top of it...\n>     [...]\n\nHave you tried with \"git rebase --fork-point\"? It does more than just\npass --fork-point to merge-base. It seems to also skip some of the \"is\nup to date\", I think due to 1e0dacdbdb (rebase: omit patch-identical\ncommits with --fork-point, 2014-07-16).\n\nI'm still not clear on whether my 4f21454b55 actually changed something\nmenaingful here, or if it's simply that you're getting the fork-point\nbehavior more consistently.\n\n-Peff\n"},{"id":"369858","messageId":"20190221214059.9195-1-avarab@gmail.com","threadId":"50505","inReplyTo":"20190221151009.GA24309@sigill.intra.peff.net","subject":"[PATCH 0/2] rebase: fix 2.11.0-era --fork-point regression","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T21:40:57Z","receivedAt":"2019-02-21T21:41:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, Feb 21 2019, Jeff King wrote:\n\n> On Thu, Feb 21, 2019 at 03:50:38PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> > Those aren't using \"--fork-point\", so they're going to behave\n>> > differently. The default with no arguments is basically \"--fork-point\n>> > @{u}\".\n>> \n>> Yeah, that's what it *should* do, but it's not equivalent to using\n>> --fork-point manually:\n>> \n>>     # my series on top of origin/master\n>>     $ git rev-parse HEAD\n>>     2a67977d3f70fa7fc4bce89db24a1218dc9ab2aa\n>>     \n>>     # Junio's origin/master upstream\n>>     $ git rev-parse @{u}\n>>     35ee755a8c43bcb3c2786522d423f006c23d32df\n>>     \n>>     # Where my fork point is\n>>     $ git merge-base --fork-point @{u}\n>>     35ee755a8c43bcb3c2786522d423f006c23d32df\n>>     \n>>     # OK\n>>     $ git rebase 35ee755a8c43bcb3c2786522d423f006c23d32df\n>>     Current branch master is up to date.\n>>     \n>>     # OK\n>>     $ git rebase $(git merge-base --fork-point @{u})\n>>     Current branch master is up to date.\n>>     \n>>     # ???\n>>     $ git rebase\n>>     First, rewinding head to replay your work on top of it...\n>>     [...]\n>\n> Have you tried with \"git rebase --fork-point\"? It does more than just\n> pass --fork-point to merge-base. It seems to also skip some of the \"is\n> up to date\", I think due to 1e0dacdbdb (rebase: omit patch-identical\n> commits with --fork-point, 2014-07-16).\n>\n> I'm still not clear on whether my 4f21454b55 actually changed something\n> menaingful here, or if it's simply that you're getting the fork-point\n> behavior more consistently.\n\nYes it's a regression. Here's a fix for it. As seen in 2/2 how we got\nto the point of regressing was an interesting combination of factors.\n\nÆvar Arnfjörð Bjarmason (2):\n  rebase tests: test linear branch topology\n  rebase: don't rebase linear topology with --fork-point\n\n builtin/rebase.c                  |  6 +++--\n t/t3421-rebase-topology-linear.sh | 44 +++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+), 2 deletions(-)\n\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369859","messageId":"20190221214059.9195-2-avarab@gmail.com","threadId":"50505","inReplyTo":"20190221151009.GA24309@sigill.intra.peff.net","subject":"[PATCH 1/2] rebase tests: test linear branch topology","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T21:40:58Z","receivedAt":"2019-02-21T21:41:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add tests rebasing a linear branch topology to linear rebase tests\nadded in 2aad7cace2 (\"add simple tests of consistency across rebase\ntypes\", 2013-06-06).\n\nThese tests are duplicates of two surrounding tests that do the same\nwith tags pointing to the same objects. Right now there's no change in\nbehavior being introduced, but as we'll see in a subsequent change\nrebase can have different behaviors when working implicitly with\nremote tracking branches.\n\nWhile I'm at it add a --fork-point test, strictly speaking this is\nredundant to the existing '' test, as no argument to rebase implies\n--fork-point. But now it's easier to grep for tests that explicitly\nstress --fork-point.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t3421-rebase-topology-linear.sh | 29 +++++++++++++++++++++++++++++\n 1 file changed, 29 insertions(+)\n\ndiff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh\nindex 7274dca40b..b847064f91 100755\n--- a/t/t3421-rebase-topology-linear.sh\n+++ b/t/t3421-rebase-topology-linear.sh\n@@ -31,6 +31,16 @@ test_run_rebase success -m\n test_run_rebase success -i\n test_have_prereq !REBASE_P || test_run_rebase success -p\n \n+test_expect_success 'setup branches and remote tracking' '\n+\tgit tag -l >tags &&\n+\tfor tag in $(cat tags)\n+\tdo\n+\t\tgit branch branch-$tag $tag || return 1\n+\tdone &&\n+\tgit remote add origin \"file://$PWD\" &&\n+\tgit fetch origin\n+'\n+\n test_run_rebase () {\n \tresult=$1\n \tshift\n@@ -57,10 +67,28 @@ test_run_rebase () {\n \t\"\n }\n test_run_rebase success ''\n+test_run_rebase success --fork-point\n test_run_rebase success -m\n test_run_rebase success -i\n test_have_prereq !REBASE_P || test_run_rebase failure -p\n \n+test_run_rebase () {\n+\tresult=$1\n+\tshift\n+\ttest_expect_$result \"rebase $* -f rewrites even if remote upstream is an ancestor\" \"\n+\t\treset_rebase &&\n+\t\tgit rebase $* -f branch-b branch-e &&\n+\t\t! test_cmp_rev branch-e origin/branch-e &&\n+\t\ttest_cmp_rev branch-b HEAD~2 &&\n+\t\ttest_linear_range 'd e' branch-b..\n+\t\"\n+}\n+test_run_rebase success ''\n+test_run_rebase success --fork-point\n+test_run_rebase success -m\n+test_run_rebase success -i\n+test_have_prereq !REBASE_P || test_run_rebase success -p\n+\n test_run_rebase () {\n \tresult=$1\n \tshift\n@@ -71,6 +99,7 @@ test_run_rebase () {\n \t\"\n }\n test_run_rebase success ''\n+test_run_rebase success --fork-point\n test_run_rebase success -m\n test_run_rebase success -i\n test_have_prereq !REBASE_P || test_run_rebase success -p\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369860","messageId":"20190221214059.9195-3-avarab@gmail.com","threadId":"50505","inReplyTo":"20190221151009.GA24309@sigill.intra.peff.net","subject":"[PATCH 2/2] rebase: don't rebase linear topology with --fork-point","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-21T21:40:59Z","receivedAt":"2019-02-21T21:41:15Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a regression introduced in 4f21454b55 (\"merge-base: handle\n--fork-point without reflog\", 2016-10-12).\n\nBefore that change having a linear history on top of an upstream\nmaster would with --fork-point (aka argument-less rebase) tell us\nthere was nothing to be done:\n\n    $ git rebase\n    Current branch master is up to date.\n\nAfter that change \"rebase\" will always redundantly find that it has\nwork to do (it doesn't):\n\n    $ git rebase\n    First, rewinding head to replay your work on top of it...\n    Applying: [...]\n\nWhereas equivalently running:\n\n    $ git rebase @{upstream}\n    $ git rebase $(git merge-base --fork-point @{u})\n\nGives us the old behavior of doing nothing.\n\nNow, why did we have this regression? Fully digging into it yields an\ninteresting combination of causes:\n\nWay back in 1308c17b3e (\"Allow rebase to run if upstream is completely\nmerged\", 2007-07-04) \"rebase\" learned to not do this redundant work\nwhen asked to rebase on a commit that was already an ancestor of the\ncurrent commit.\n\nThen in 1e0dacdbdb (\"rebase: omit patch-identical commits with\n--fork-point\", 2014-07-16) a rebase bug was fixed for a case where the\nhistory to be rebased was divergent by entirely skipping the 2007-era\nlogic if --fork-point was provided.\n\nBut here's the critical thing, *only* if the --fork-point was\ndivergent. At that time \"git merge-base --fork-point A B\" would return\nnothing if the two commits weren't divergent.\n\nThen in 4f21454b55 (\"merge-base: handle --fork-point without reflog\",\n2016-10-12) which introduced the regression being fixed here, a bug\nfix for \"git merge-base --fork-point\" being run stand-alone by proxy\nbroke this use-case git-rebase.sh was relying on, since it was still\nassuming that if we didn't have divergent history we'd have no output.\n\nFinally, when \"rebase\" was rewritten in C a combination of\n9a48a615b4 (\"builtin rebase: try to fast forward when possible\",\n2018-09-04), 103148aad8 (\"merge-base --fork-point: extract libified\nfunction\", 2018-09-04) and 92d0d74e8d (\"builtin rebase: support\n`fork-point` option\", 2018-09-04) faithfully re-implemented the\nthen-buggy behavior.\n\nSo let's fix this. It's easy enough, we just stop explicitly excluding\n--fork-point from the can_fast_forward(...) test we're doing, which as\ndiscussed above is faithfully ported over from buggy shellscript-era\nlogic.\n\nI'm not bothering to fix this in the legacy rebase mode. As discussed\nin 9aea5e9286 (\"rebase: fix regression in rebase.useBuiltin=false test\nmode\", 2019-02-13) it'll be going away shortly after 2.21.0 lands.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/rebase.c                  |  6 ++++--\n t/t3421-rebase-topology-linear.sh | 15 +++++++++++++++\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 7c7bc13e91..7a16b8051c 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1664,9 +1664,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t * and if this is not an interactive rebase.\n \t */\n \tif (can_fast_forward(options.onto, &options.orig_head, &merge_base) &&\n-\t    !is_interactive(&options) && !options.restrict_revision &&\n+\t    !is_interactive(&options) &&\n \t    options.upstream &&\n-\t    !oidcmp(&options.upstream->object.oid, &options.onto->object.oid)) {\n+\t    (options.restrict_revision\n+\t     ? !oidcmp(&options.upstream->object.oid, &options.restrict_revision->object.oid)\n+\t     : !oidcmp(&options.upstream->object.oid, &options.onto->object.oid))) {\n \t\tint flag;\n \n \t\tif (!(options.flags & REBASE_FORCE)) {\ndiff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh\nindex b847064f91..1754537789 100755\n--- a/t/t3421-rebase-topology-linear.sh\n+++ b/t/t3421-rebase-topology-linear.sh\n@@ -55,6 +55,21 @@ test_run_rebase success -m\n test_run_rebase success -i\n test_have_prereq !REBASE_P || test_run_rebase success -p\n \n+test_run_rebase () {\n+\tresult=$1\n+\tshift\n+\ttest_expect_$result \"rebase $* is no-op if remote upstream is an ancestor\" \"\n+\t\treset_rebase &&\n+\t\tGIT_TEST_REBASE_USE_BUILTIN=true git rebase $* branch-b branch-e &&\n+\t\ttest_cmp_rev e HEAD\n+\t\"\n+}\n+test_run_rebase success ''\n+test_run_rebase success --fork-point\n+test_run_rebase success -m\n+test_run_rebase success -i\n+test_have_prereq !REBASE_P || test_run_rebase success -p\n+\n test_run_rebase () {\n \tresult=$1\n \tshift\n-- \n2.21.0.rc0.258.g878e2cd30e\n\n"},{"id":"369922","messageId":"20190222145344.GA5090@sigill.intra.peff.net","threadId":"50505","inReplyTo":"20190221214059.9195-2-avarab@gmail.com","subject":"Re: [PATCH 1/2] rebase tests: test linear branch topology","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-22T14:53:44Z","receivedAt":"2019-02-22T14:53:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2019 at 10:40:58PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Add tests rebasing a linear branch topology to linear rebase tests\n> added in 2aad7cace2 (\"add simple tests of consistency across rebase\n> types\", 2013-06-06).\n\nI had trouble parsing this. Did you mean s/topology to/topology, similar\nto/?\n\n> These tests are duplicates of two surrounding tests that do the same\n> with tags pointing to the same objects. Right now there's no change in\n> behavior being introduced, but as we'll see in a subsequent change\n> rebase can have different behaviors when working implicitly with\n> remote tracking branches.\n\nIt took me a while to figure out what was new in these tests. Maybe:\n\n  These tests are duplicates of two surrounding tests, but with one\n  change: the existing tests refer to objects by their tag names, but\n  here we'll use branches (pointing at the same objects).\n\nBut then I'm left wondering why that's important.\n\n> While I'm at it add a --fork-point test, strictly speaking this is\n> redundant to the existing '' test, as no argument to rebase implies\n> --fork-point. But now it's easier to grep for tests that explicitly\n> stress --fork-point.\n\nThat makes sense.\n\n> +test_expect_success 'setup branches and remote tracking' '\n> +\tgit tag -l >tags &&\n> +\tfor tag in $(cat tags)\n> +\tdo\n> +\t\tgit branch branch-$tag $tag || return 1\n> +\tdone &&\n\nI don't think we need this extra tmpfile and cat, do we? I.e.,\n\n  for tag in $(git tag -l)\n\nwould work. We should probably avoid depending on the exact output of\nthe porcelain \"tag\", though. Maybe:\n\n  git for-each-ref \\\n    --format='create refs/heads/branch-%(refname:strip=2) %(objectname)' \\\n    refs/tags |\n  git update-ref --stdin\n\nwhich has the added bonus of using a constant number of processes.\n\n-Peff\n"},{"id":"369923","messageId":"20190222150852.GB5090@sigill.intra.peff.net","threadId":"50505","inReplyTo":"20190221214059.9195-3-avarab@gmail.com","subject":"Re: [PATCH 2/2] rebase: don't rebase linear topology with --fork-point","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-22T15:08:52Z","receivedAt":"2019-02-22T15:08:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 21, 2019 at 10:40:59PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Fix a regression introduced in 4f21454b55 (\"merge-base: handle\n> --fork-point without reflog\", 2016-10-12).\n> [...]\n\nOK, your explanation mostly makes sense to me, except for one thing.\n\n> Then in 4f21454b55 (\"merge-base: handle --fork-point without reflog\",\n> 2016-10-12) which introduced the regression being fixed here, a bug\n> fix for \"git merge-base --fork-point\" being run stand-alone by proxy\n> broke this use-case git-rebase.sh was relying on, since it was still\n> assuming that if we didn't have divergent history we'd have no output.\n\nI still don't quite see how 4f21454b55 is involved here, except by\nreturning a fork-point value when there is no reflog, and thus\ntriggering the bug in more cases.\n\nIn particular, imagine this case:\n\n  git init\n  for i in $(seq 1 3); do echo $i >$i; git add $i; git commit -m $i; done\n  git checkout -t -b other\n  for i in $(seq 4 6); do echo $i >$i; git add $i; git commit -m $i; done\n  git rebase\n\nWith the current code, that will rewind and replay 4-6, and I understand\nthat to be a bug from your description. And it happens at 4f21454b55,\ntoo. But it _also_ happens at 4f21454b55^.\n\nI.e., I still think that the only thing that commit changed is that we\nfound a fork-point in more cases. But the bug was still demonstrably\nthere when you actually have a reflog entry.\n\nWith the fix you have here, that case now produces \"Current branch other\nis up to date\".\n\nThis is splitting hairs a little (and of course I'm trying to exonerate\nthe commit I'm responsible for ;) ), but I just want to make sure we\nunderstand fully what's going on. Your fix looks plausibly correct to\nme, but I admit I don't quite grok all the details of that conditional.\n\n-Peff\n"},{"id":"369958","messageId":"871s3z6a4q.fsf@evledraar.gmail.com","threadId":"50505","inReplyTo":"20190222150852.GB5090@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] rebase: don't rebase linear topology with --fork-point","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-02-22T16:49:57Z","receivedAt":"2019-02-22T16:50:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 22 2019, Jeff King wrote:\n\n> On Thu, Feb 21, 2019 at 10:40:59PM +0100, Ævar Arnfjörð Bjarmason wrote:\n>\n>> Fix a regression introduced in 4f21454b55 (\"merge-base: handle\n>> --fork-point without reflog\", 2016-10-12).\n>> [...]\n>\n> OK, your explanation mostly makes sense to me, except for one thing.\n>\n>> Then in 4f21454b55 (\"merge-base: handle --fork-point without reflog\",\n>> 2016-10-12) which introduced the regression being fixed here, a bug\n>> fix for \"git merge-base --fork-point\" being run stand-alone by proxy\n>> broke this use-case git-rebase.sh was relying on, since it was still\n>> assuming that if we didn't have divergent history we'd have no output.\n>\n> I still don't quite see how 4f21454b55 is involved here, except by\n> returning a fork-point value when there is no reflog, and thus\n> triggering the bug in more cases.\n>\n> In particular, imagine this case:\n>\n>   git init\n>   for i in $(seq 1 3); do echo $i >$i; git add $i; git commit -m $i; done\n>   git checkout -t -b other\n>   for i in $(seq 4 6); do echo $i >$i; git add $i; git commit -m $i; done\n>   git rebase\n>\n> With the current code, that will rewind and replay 4-6, and I understand\n> that to be a bug from your description. And it happens at 4f21454b55,\n> too. But it _also_ happens at 4f21454b55^.\n>\n> I.e., I still think that the only thing that commit changed is that we\n> found a fork-point in more cases. But the bug was still demonstrably\n> there when you actually have a reflog entry.\n>\n> With the fix you have here, that case now produces \"Current branch other\n> is up to date\".\n>\n> This is splitting hairs a little (and of course I'm trying to exonerate\n> the commit I'm responsible for ;) ), but I just want to make sure we\n> understand fully what's going on.\n\nYes. I didn't dig far enough into this and will re-word & re-submit,\nalso with the feedback you had on 1/2.\n\nSo here's my current understanding of this.\n\nIt's b6266dc88b (\"rebase--am: use --cherry-pick instead of\n--ignore-if-in-upstream\", 2014-07-15) that broke this in the general\ncase.\n\nI.e. if you set a tracking branch within the same repo (which I'd\nbetnobody does) but *also* if you have an established clone you have a\nreflog for the upstream. Then we'll find the fork point, and we'll\nalways redundantly rebase.\n\nBut this hung on by a thread until your 4f21454b55 (\"merge-base: handle\n--fork-point without reflog\", 2016-10-12). In particular when you:\n\n 1. Clone some *new* repo\n 2. commit on top\n 3. git pull --rebase\n\nYou'll redundantly rebase on top, even though you have nothing to\ndo. Since there's no reflog.\n\nThis is why I ran into this most of the time, because my \"patch some\nrandom thing\" is that, and I have pull.rebase=true in my config.\n\nWhat had me confused about this being the primary cause was that when\ntrying to test this I was re-cloning, so I'd always get this empty\nreflog case.\n\n> Your fix looks plausibly correct to me, but I admit I don't quite grok\n> all the details of that conditional.\n\nWe just consider whether we can fast-forward now, and then don't need to\nrebase (unless \"git rebase -i\" etc.). I.e. that --fork-point was\nconsidered for \"do we need to do stuff\" was a bug introduced in\nb6266dc88b.\n"},{"id":"369966","messageId":"xmqq7edrr79m.fsf@gitster-ct.c.googlers.com","threadId":"50505","inReplyTo":"20190222145344.GA5090@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] rebase tests: test linear branch topology","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-02-22T18:46:13Z","receivedAt":"2019-02-22T18:46:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> While I'm at it add a --fork-point test, strictly speaking this is\n>> redundant to the existing '' test, as no argument to rebase implies\n>> --fork-point. But now it's easier to grep for tests that explicitly\n>> stress --fork-point.\n>\n> That makes sense.\n>\n>> +test_expect_success 'setup branches and remote tracking' '\n>> +\tgit tag -l >tags &&\n>> +\tfor tag in $(cat tags)\n>> +\tdo\n>> +\t\tgit branch branch-$tag $tag || return 1\n>> +\tdone &&\n>\n> I don't think we need this extra tmpfile and cat, do we? I.e.,\n>\n>   for tag in $(git tag -l)\n>\n> would work.\n\nI think it is being (overly) defensive not to lose the exit status\nof \"git tag\".\n\n> We should probably avoid depending on the exact output of\n> the porcelain \"tag\", though. Maybe:\n>\n>   git for-each-ref \\\n>     --format='create refs/heads/branch-%(refname:strip=2) %(objectname)' \\\n>     refs/tags |\n>   git update-ref --stdin\n>\n> which has the added bonus of using a constant number of processes.\n\nMuch better ;-)\n"},{"id":"370058","messageId":"20190224101029.GA13438@sigill.intra.peff.net","threadId":"50505","inReplyTo":"871s3z6a4q.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] rebase: don't rebase linear topology with --fork-point","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-02-24T10:10:29Z","receivedAt":"2019-02-24T10:10:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 22, 2019 at 05:49:57PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> Yes. I didn't dig far enough into this and will re-word & re-submit,\n> also with the feedback you had on 1/2.\n> \n> So here's my current understanding of this.\n> \n> It's b6266dc88b (\"rebase--am: use --cherry-pick instead of\n> --ignore-if-in-upstream\", 2014-07-15) that broke this in the general\n> case.\n> \n> I.e. if you set a tracking branch within the same repo (which I'd\n> betnobody does) but *also* if you have an established clone you have a\n> reflog for the upstream. Then we'll find the fork point, and we'll\n> always redundantly rebase.\n> \n> But this hung on by a thread until your 4f21454b55 (\"merge-base: handle\n> --fork-point without reflog\", 2016-10-12). In particular when you:\n> \n>  1. Clone some *new* repo\n>  2. commit on top\n>  3. git pull --rebase\n> \n> You'll redundantly rebase on top, even though you have nothing to\n> do. Since there's no reflog.\n> \n> This is why I ran into this most of the time, because my \"patch some\n> random thing\" is that, and I have pull.rebase=true in my config.\n> \n> What had me confused about this being the primary cause was that when\n> trying to test this I was re-cloning, so I'd always get this empty\n> reflog case.\n\nOK, thanks, that all makes sense to me and matches what I'm seeing. I\nthink we're on the same page now.\n\n> > Your fix looks plausibly correct to me, but I admit I don't quite grok\n> > all the details of that conditional.\n> \n> We just consider whether we can fast-forward now, and then don't need to\n> rebase (unless \"git rebase -i\" etc.). I.e. that --fork-point was\n> considered for \"do we need to do stuff\" was a bug introduced in\n> b6266dc88b.\n\nRight, that makes sense. But I'm just not clear on the new oidcmp() rule\nthat's put in place.\n\nWhen we're not using fork point, in the old code we'd check whether\nupstream and onto are the same. Which makes sense; if we're rebuilding\nonto the same spot, then it's a noop.\n\nAnd in the fork-point case, we'd instead want to do something similar\nwith restrict_revision. But you compare upstream and restrict_revision,\nand my understanding is that with --fork-point the latter basically\nreplaces the former.  Shouldn't we be comparing restrict_revision and\nonto?\n\nE.g., if I do:\n\n  git checkout -b foo origin\n  echo content >file && git add file && git commit -m foo\n  git rebase --onto origin^ --fork-point origin\n\nwe should do an actual rebase, but with your patch we get \"Current\nbranch foo is up to date\". Whereas dropping --fork-point, it works.\n\n-Peff\n"}]}