| Message ID | cover.1554151449.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rebase: teach rebase --keep-base | expand |
On Mon, Apr 01 2019, Denton Liu wrote: > Thanks again for your feedback, Ævar! I think we're both on the same page now. > Hopefully I've addressed all of your high-level concerns with this patchset and > we can move into a discussion on implementation detail. Late in replying to this, have been off-list. This also applies for your v4. The current version you have still doesn't explain the "Why would we redundantly rebase every time in this case..." question I had in https://public-inbox.org/git/87tvfma8yt.fsf@evledraar.gmail.com/ I *think* it's closer to "it was easier to implement this in terms of --onto, which happens to behave that way now" than "it must work this way for --keep-base", which is fair enough. Although I see when I forward-port my POC patch from that E-Mail that one test fails now, which is good, that wasn't the case before, but it looks like that might be testing something else than just the lazy behavior. It would be good to know in terms of commit message or (better) explicit tests so that if we teach these various rebase modes the same lazyness --fork-point uses in the future it's clear if that's OK or not.
On Sat, Apr 06, 2019 at 09:44:49PM +0200, Ævar Arnfjörð Bjarmason wrote: > > On Mon, Apr 01 2019, Denton Liu wrote: > > > Thanks again for your feedback, Ævar! I think we're both on the same page now. > > Hopefully I've addressed all of your high-level concerns with this patchset and > > we can move into a discussion on implementation detail. > > Late in replying to this, have been off-list. This also applies for your > v4. > > The current version you have still doesn't explain the "Why would we > redundantly rebase every time in this case..." question I had in > https://public-inbox.org/git/87tvfma8yt.fsf@evledraar.gmail.com/ > > I *think* it's closer to "it was easier to implement this in terms of > --onto, which happens to behave that way now" than "it must work this > way for --keep-base", which is fair enough. Correct, the reason why --keep-base was not lazy initially was because "--onto did it that way". You are correct in that --keep-base should be lazily rebasing so I changed --onto's behaviour in 3/4 because it would also benefit from laziness. Thus, now --keep-base lazily rebases because --onto also does. > > Although I see when I forward-port my POC patch from that E-Mail that > one test fails now, which is good, that wasn't the case before, but it > looks like that might be testing something else than just the lazy > behavior. The test fails because the patch disables fork_point if --keep-base is set. So, with the patch applied, C is rebased even though it is excluded when fork_point is set. > > It would be good to know in terms of commit message or (better) explicit > tests so that if we teach these various rebase modes the same lazyness > --fork-point uses in the future it's clear if that's OK or not. Sorry, could you please clarify what you mean by the "lazyness --fork-point uses"? I don't understand what laziness is introduced by using --fork-point. Also, are the tests in t3432 not sufficient for testing fast-forwarding (aka lazy) behaviour? Thanks, Denton
On Sat, Apr 06 2019, Denton Liu wrote: > On Sat, Apr 06, 2019 at 09:44:49PM +0200, Ævar Arnfjörð Bjarmason wrote: >> >> On Mon, Apr 01 2019, Denton Liu wrote: >> >> > Thanks again for your feedback, Ævar! I think we're both on the same page now. >> > Hopefully I've addressed all of your high-level concerns with this patchset and >> > we can move into a discussion on implementation detail. >> >> Late in replying to this, have been off-list. This also applies for your >> v4. >> >> The current version you have still doesn't explain the "Why would we >> redundantly rebase every time in this case..." question I had in >> https://public-inbox.org/git/87tvfma8yt.fsf@evledraar.gmail.com/ >> >> I *think* it's closer to "it was easier to implement this in terms of >> --onto, which happens to behave that way now" than "it must work this >> way for --keep-base", which is fair enough. > > Correct, the reason why --keep-base was not lazy initially was because > "--onto did it that way". You are correct in that --keep-base should be > lazily rebasing so I changed --onto's behaviour in 3/4 because it would > also benefit from laziness. Thus, now --keep-base lazily rebases because > --onto also does. > >> >> Although I see when I forward-port my POC patch from that E-Mail that >> one test fails now, which is good, that wasn't the case before, but it >> looks like that might be testing something else than just the lazy >> behavior. > > The test fails because the patch disables fork_point if --keep-base is > set. So, with the patch applied, C is rebased even though it is excluded > when fork_point is set. > >> >> It would be good to know in terms of commit message or (better) explicit >> tests so that if we teach these various rebase modes the same lazyness >> --fork-point uses in the future it's clear if that's OK or not. > > Sorry, could you please clarify what you mean by the "lazyness > --fork-point uses"? I don't understand what laziness is introduced by > using --fork-point. Also, are the tests in t3432 not sufficient for > testing fast-forwarding (aka lazy) behaviour? Late reply, sorry. I mean if I do e.g. on git.git: git checkout -b avar/test 041f5ea1cf I.e. branch of Junio's "The third batch" (we're now on the 4th) and then: git branch --set-upstream-to origin/master And commit a dummy change, I'm now: On branch avar/test Your branch and 'origin/master' have diverged, and have 1 and 33 different commits each, respectively. Then I run a --keep-base rebase twice: $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --keep-base; git rev-parse HEAD fc3e916c5f foo 041f5ea1cf The third batch First, rewinding head to replay your work on top of it... Applying: foo b10e672074dfee6b6e8b2901c9bb49f856a13708 $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --keep-base; git rev-parse HEAD b10e672074 foo 041f5ea1cf The third batch First, rewinding head to replay your work on top of it... Applying: foo fd3029e73b89f5a799653ff17199d00f2a6ee2e2 I.e. I'll keep on getting a new commit, even though by any criteria I can think of for this type of case there's no work to do. I.e. we're already based on 041f5ea1cf, no need to rebase again. Of course this is also currently the case with --fork-point without argument, which'll settle on (note fourth, not third): $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --fork-point; git rev-parse HEAD 85a1861cec foo e35b8cb8e2 The fourth batch First, rewinding head to replay your work on top of it... Applying: foo c6046e97af29d71bf6270080acf188c095e0cb7c $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase --fork-point; git rev-parse HEAD c6046e97af foo e35b8cb8e2 The fourth batch First, rewinding head to replay your work on top of it... Applying: foo 672d22d58e9aa9b6a28054531c21e1f1b598b013 Whereas a --keep-base in *this* case should be identical to: $ git log --oneline --no-decorate -2; ~/g/git/git --exec-path=$PWD rebase $(git merge-base --fork-point origin/master HEAD); git rev-parse HEAD fc3e916c5f foo 041f5ea1cf The third batch Current branch avar/test is up to date. fc3e916c5fc707cfc83f28b3faca81046306b305 The reason I'm asking is that as noted in the thread starting at https://public-inbox.org/git/20190224101029.GA13438@sigill.intra.peff.net/ we used to in *some* cases do the same for: rebase $(git merge-base --fork-point origin/master HEAD) As for just: rebase --fork-point I.e. say "Current branch is up-to-date". I'm planning to loop back to that and fix it for the --fork-point case. So I was wondering if you could think of a reason for why --keep-base couldn't also do the same, or if there was some intrinsic difference I'm missing. Anyway. Don't worry about it. When I poke at it it'll likely shake out of the respective tests. I was just wondering if you having looked at many of the same things recently could understand my ramblings and knew if this was a case of "yup, we could add a bit more to that can_fast_forward(...) case and make it lazy too", or "no, --keep-base is special for reasons you're missing...".