Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix inconsistencies of dolt checkout and pull commands #3725

Merged
merged 12 commits into from
Jul 5, 2022

Conversation

jennifersp
Copy link
Contributor

@jennifersp jennifersp commented Jun 30, 2022

This PR fixes inconsistent behaviors of Dolt CLI against Git commands AND of Dolt SQL against Dolt CLI commands.

Inconsistencies between Dolt CLI and Git:

  • dolt checkout new_branch: checking out a new branch without -b flag. If there is a remote branch with matching name, sets upstream.
  • dolt checkout -b new_branch: if there is a remote branch with matching name, it does not set upstream.
  • dolt pull and dolt pull remote_name: each gives separate errors when run on a branch that does not have upstream

Inconsistencies between Dolt CLI and Dolt SQL:

  • dolt pull should match dolt_pull()
  • dolt_checkout() and dolt_push() should set upstream that persists outside of sql session (added skipped tests)

@jennifersp jennifersp linked an issue Jun 30, 2022 that may be closed by this pull request
@jennifersp jennifersp requested a review from fulghum June 30, 2022 20:26
@jennifersp jennifersp changed the title [WIP] fix inconsistencies of dolt checkout, push and pull commands fix inconsistencies of dolt checkout, push and pull commands Jun 30, 2022
@jennifersp jennifersp linked an issue Jun 30, 2022 that may be closed by this pull request
@fulghum
Copy link
Contributor

fulghum commented Jul 1, 2022

Are you sure about this one?

dolt push origin new_branch: if remote name and branch name is given, it sets upstream without --set-upstream flag

I didn't see anything in the git docs about automatically setting the upstream without having to supply --set-upstream and a quick test with git didn't record an upstream for me. Let me know if you can repro git setting the upstream this way.

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of good fixes in this PR!! Good job digging in deep and finding all these details. No major concerns from me; let me know if any comments don't make sense.

Two larger comments/thoughts popped out at me while reviewing, too:

  • there is a LOT of duplication between the CLI path and SQL path! 😊 It'll be hard to keep those exactly in sync, just like you found. There's an interesting question about when to refactor a little bit more while we're in here and see if we can chip away at the duplication. For example, with dolt pull, it seems like there might be some low hanging fruit around combining the code paths. Not necessary for this PR, but something to think about as we're in this code.
  • we should talk more about layers of testing between BATS and the enginetests. I both are super important, and could be a good conversation to talk through a mental framework for what to test where.

go/libraries/doltcore/sqle/dfunctions/dolt_checkout.go Outdated Show resolved Hide resolved
@@ -1030,6 +1133,12 @@ SQL
[ "$status" -eq 0 ]
[[ "$output" =~ "pk" ]] || false
[[ "$output" =~ "1" ]] || false

skip # above checkout command should set upstream persisting outside of session
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting that the SQL path doesn't persist the tracking metadata. have you already figured out why that is, or found any clues?

Does this mean if a customer has to restart their sql-server, they'll lose all this tracking metadata? Or is it just that the metadata can't be read outside of the SQL path?

Given that we are so focused on the SQL experience, I think this one is worth fixing quickly, but totally fine to tackle it in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We store upstream tracking metadata in repo_state.json file, and we do not have access to that file from inside the SQL session. As long as, user is in the same session, any changes on upstream tracking works fine, but once out of the session, the changes made inside the session will not be saved since we cannot access the repo_state file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extra context. If we don't have a tracking issue for this, could you create one? Seems like something that will confuse/surprise customers and would be great to close this gap before someone falls in it.

go/cmd/dolt/commands/checkout.go Outdated Show resolved Hide resolved
go/cmd/dolt/commands/checkout.go Outdated Show resolved Hide resolved
go/cmd/dolt/commands/checkout.go Outdated Show resolved Hide resolved
go/libraries/doltcore/env/remotes.go Outdated Show resolved Hide resolved
go/libraries/doltcore/env/remotes.go Outdated Show resolved Hide resolved
go/libraries/doltcore/env/remotes.go Outdated Show resolved Hide resolved
go/libraries/doltcore/sqle/dfunctions/dolt_checkout.go Outdated Show resolved Hide resolved
integration-tests/bats/remotes.bats Outdated Show resolved Hide resolved
@jennifersp jennifersp changed the title fix inconsistencies of dolt checkout, push and pull commands fix inconsistencies of dolt checkout and pull commands Jul 1, 2022
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Just one comment on a small tweak for an error message to match Git's response a little closer.

go/libraries/doltcore/env/actions/table.go Outdated Show resolved Hide resolved
@jennifersp jennifersp merged commit 6229f6a into main Jul 5, 2022
@jennifersp jennifersp deleted the jennifer/remote-tracking branch July 5, 2022 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dolt checkout -b incorrectly sets upstream tracking info
2 participants