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.

Sign up for GitHub

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

Jump to bottom

Refactor pull operations to use fetch and merge #241

Merged
merged 2 commits into from
Feb 24, 2018
Merged

Refactor pull operations to use fetch and merge #241

merged 2 commits into from
Feb 24, 2018

Conversation

xendk
Copy link
Contributor

@xendk xendk commented Feb 21, 2018 •
edited
Loading

straight-pull-all now feches first, before merging all.

I've removed the low-level git pull commands as they're functionally covered by fetch and merge, and keeping the VC interface as simple as possible seems like a good idea.

Addresses some of #103

@xendk
Copy link
Contributor Author

xendk commented Feb 21, 2018 •
edited by raxod502
Loading

straight-check-package is broken, but I get the same behavior on the develop branch.

raxod502
raxod502 requested changes Feb 23, 2018
View reviewed changes
straight.el Outdated
(straight-vc-git--validate-head
local-repo branch (format "%s/%s" remote remote-branch))
(cl-return-from straight-vc-git--pull-from-remote-raw t))))))
(straight-vc-fetch-from-remote recipe)
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to avoid round-tripping through the top-level VC API here. Can we use straight-vc-git-fetch-from-remote instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

straight.el Outdated
local-repo branch (format "%s/%s" remote remote-branch))
(cl-return-from straight-vc-git--pull-from-remote-raw t))))))
(straight-vc-fetch-from-remote recipe)
(cl-return-from straight-vc-git--pull-from-remote-raw
Copy link
Member

Choose a reason for hiding this comment

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

I think this cl-return-from is superfluous.

Copy link
Contributor Author

@xendk xendk Feb 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Disregard previous comment, I get it now.

straight.el Outdated
@@ -4223,9 +4152,11 @@ not just from primary remote but also from configured upstream."
(interactive (list (straight--select-package "Pull package" nil 'installed)
current-prefix-arg))
(let ((recipe (gethash package straight--recipe-cache)))
(and (straight-vc-pull-from-remote recipe)
(and (and (straight-vc-fetch-from-remote recipe)
(straight-vc-merge-from-remote recipe))
Copy link
Member

Choose a reason for hiding this comment

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

These two ands can be collapsed together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, of course.

@raxod502 raxod502 changed the title Refactor pull operartions to use fetch and merge Refactor pull operations to use fetch and merge Feb 23, 2018
@raxod502
Copy link
Member

I think this is a good idea. Further work is needed for the user experience, but this pull request is a step in the right direction. I just had a few stylistic comments, and then I will merge it.

@raxod502
Copy link
Member

Also, could you elaborate on what you mean by straight-check-package being broken? It works for me.

@xendk
Copy link
Contributor Author

xendk commented Feb 23, 2018

I'll fix those things when I have time.

I've tried taking a random package, doing a git checkout HEAD^, run straight-rebuild-package on it and restarting Emacs. After running fetch/merge (and telling it to reattach to origin/HEAD), I would expect it to rebuild the package, but it does nothing (as does straight-check-all). straight-check-for-modifications is live

@raxod502
Copy link
Member

If you have straight-check-for-modifications set to live, then not all modifications will be detected. In particular, straight.el's repository management operations are not detected as modifications, which is definitely a bug. (I would not classify live modification detection as production-ready yet, unlike eager modification detection.) Feel free to open another issue for that.

@xendk
Refactor pull operartions to use fetch and merge
a15bc10 
`straight-pull-all` now feches first, before merging all.
@xendk
Copy link
Contributor Author

xendk commented Feb 23, 2018

I've implemented your suggestions.

@raxod502 raxod502 mentioned this pull request Feb 24, 2018
Closed
@raxod502 raxod502 merged commit 8d5119e into radian-software:develop Feb 24, 2018
@xendk xendk mentioned this pull request Feb 25, 2018
Closed
@xendk xendk deleted the pull-refactor branch March 1, 2018 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raxod502 raxod502 raxod502 approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xendk @raxod502