-
Notifications
You must be signed in to change notification settings - Fork 25
Add merge, rebase and reset operations for branch management #1095
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
base: feature/branching
Are you sure you want to change the base?
Conversation
970f0e4 to
ce3b475
Compare
ce3b475 to
b2a5f9f
Compare
…set functionality
…ommit reversal processing
…tract cancellation
…ls in documentation
…ion and add tests
…tions for clarity and accuracy
…logging for commit processing
495b4b5 to
e4d3906
Compare
- Resolved conflict in src/fluree/db/commit/storage.cljc - Preferred feature/branching version using storage/content-read-json - Kept cleaner hash calculation logic from feature/branching - Fixed branch-graph test by correcting read-commit-jsonld function call - Removed extra hash parameter that was causing test failures - Merged all branch improvements including: - Branch metadata flattening for index optimization - Helper functions consolidation in util.branch namespace - Secondary publisher support - Validation improvements and extracted validate-publish function - Main branch helper functions - Drop API error message enhancements - Deterministic test timestamps - All tests now pass
… unnecessary hash extraction
…ove deprecated merge-branches! function
| can-ff? (<? (merge-branch/can-fast-forward? conn source-db target-db | ||
| source-branch-info target-branch-info))] | ||
|
|
||
| (when preview? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't short circuit because of the when. The subsequent merging code here will always run whether or not the preview? option is set, which is the opposite of what we want when people set that option.
| source-branch-info target-branch-info))] | ||
|
|
||
| (when preview? | ||
| (reduced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't look like it's taking place inside of a reduction. What is this reduced doing?
| ;; Delete the existing branch | ||
| (<? (api.branch/delete-branch! conn branch-spec)) | ||
| ;; Recreate it pointing to the new commit | ||
| (<? (api.branch/create-branch! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it possible to do this in a non-destructive way? i'm worried about an error upon recreation after the original branch data has been deleted.
| :squash? - Combine all commits into one (default false) | ||
| :preview? - Dry run without changes (default false) | ||
| Returns promise resolving to merge result with anomalies report." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does not return a promise; it returns a core.async channel.
| :squash? - Combine all commits into one (default false) | ||
| :preview? - Dry run without changes (default false) | ||
| Returns promise resolving to rebase result." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does not return a promise.
| :from from | ||
| :to to | ||
| :lca lca | ||
| :strategy (if squash? "squash" "replay")})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the squash? case should be handled by a separate function. squashed? is referenced many times in subsequent code, which is a source of future bugs as these functions get modified.
| @@ -0,0 +1,103 @@ | |||
| (ns fluree.db.merge.db | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There needs to be a protocol for this functionality. A lot of the code here assumes we'll be operating on a FlakeDB explicitly, and there's a function that assumes that an AsyncDB will always wrap a FlakeDB. We should be able to use any dbs we receive interchangeably without ever having to explicitly check what kind of dbs they are, so new functionality on dbs should be defined in terms of new protocols.
| (merge-branch/validate-same-ledger! from to) | ||
| (let [{:keys [ff squash? preview?] | ||
| :or {ff :auto | ||
| squash? false}} opts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to control the behavior of these functions through options maps, then we need to have an explicit options validation step. It's really easy for a user to use {:preview true} instead of {:preview? true} and perform some destructive process that they didn't intend.
This is part of the reason why I don't like options maps. I would rather have separate, explicit functions for all the legal options combinations we accept (like preview-merge ...). Those functions could share internal helper functions for shared functionality. This makes the api explicit, reduces the complexity of both the code and documentation, and makes it obvious what is and is not supported from what functions are available.
| (defn- branch-origin | ||
| "Get the commit ID a branch was created from." | ||
| [branch-info] | ||
| (or (get-in branch-info [:created-from "f:commit" "@id"]) ; nameservice expanded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should operate on a single format internally, and convert between external representations as necessary. Checking all these cases whenever we want to find information about a branch seems like a source of future bugs.
| (let [commit-catalog (:commit-catalog conn) | ||
| latest-expanded (<? (merge-commit/expand-latest-commit conn db)) | ||
| error-ch (async/chan) | ||
| tuples (commit-storage/trace-commits commit-catalog latest-expanded 0 error-ch)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't handle any errors as nothing ever takes anything off of the error channel. It looks like this code will just hang, possibly locking the whole jvm process, if an error occurs.
| #### Response | ||
|
|
||
| ```clojure | ||
| {:status :success ; or :error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the return should just be the same as a regular commit - return the db with the new HEAD.
|
|
||
| | Mode | Status | Description | | ||
| |------|--------|-------------| | ||
| | Safe mode (`:mode :safe`) | ✅ Implemented | Creates a revert commit to target state | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think revert is a distinct operation from reset, I would vote we move this to a different api and leave reset unimplemented for now.
| |------|--------|-------------| | ||
| | Safe mode (`:mode :safe`) | ✅ Implemented | Creates a revert commit to target state | | ||
| | Hard mode (`:mode :hard`) | ❌ Not Implemented | Will move branch pointer (rewrite history) | | ||
| | Preview (`:preview? true`) | ✅ Implemented | Dry-run to see what would happen | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought, but if we're copying git's api we might want to call this dry-run? instead : )
| ;; Returns: | ||
| {:common-ancestor "fluree:commit:sha256:..." | ||
| :can-fast-forward true ; or false | ||
| :fast-forward-direction :branch1-to-branch2} ; or :branch2-to-branch1, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this datastructure can be made more useful:
{:common-ancestor <lca>
:target [<commita> <commitb>] ;; commits since lca
:source [<commitc> <commitd> <commite>]} ;; commits since lcacan-fast-forward is implicit - only possible when :target is empty. This way we don't have to try to decipher what the direction is by parsing the :fast-forward-direction and we have the information we need to check for conflicts.
Summary
This sits on top of PR #1095 which in turn sits on top of several others. The final part of the full branching support which sits on top of this branch is #1102 which adds cuckoo filters for index garbage collection.
This PR implements core branch management operations for Fluree, providing Git-like capabilities for database version control:
Features Implemented
1. Merge Operations (Updates Target Branch)
Fast-forward
Automatically moves target branch pointer when no divergence exists: