AckslD commented 2 years ago

Here's a WIP implementation of what was discussed in #662. This is not solving #662 but rather provides a way to preview any update before actually performing it. This is done by fetching (but not pulling), and allowing the user to view any diffs to FETCH_HEAD before deciding to update (a subset of) them.

Currently the implementation is quite hacky and ugly and I have some questions on how to do some things better. But I thought I would first show my intended behaviour here to see if it makes sense to others. And if so we can go into the details in the implementation and I can ask a bunch of questions.

See the following screencast for how it currently works. I ask packer to fetch three plugins (similarly to update). After the fetch is done I see that one is up to date and two others have changes which are not yet pulled. I can look at the diff of each individually and toggle which ones are selected. When I'm done I can update the ones I selected.

packer_fetch_diff

Note that the querying of diffs does not need to query remote since the changes have already been fetched.

Tagging @akinsho @pedro757 @gegoune who may be interested in this.

gegoune commented 2 years ago

Good start. Will test it out during the week. One thing to add would be list of commits to easily see changes without looking at diffs.

akinsho commented 2 years ago

@AckslD as mentioned this looks great IMO, one thing that differs from what I had expected (other than the list of commits mentioned already) was that I thought of this as a configuration options like review_before_update or something not an actual suggestion just an idea. So that user doing a normal update routine with that enabled would have all their plugins fetched with a list of changes toggleable the same way commits currently are that they can then move to and diff and mark some to not update otherwise accept all or some similar workflow.

I assumed it would work quite similarly to the normal flow, except if you noticed something flagged as a breaking change you could go and mark it as not for updating and then accept everything else.

This is rather than as a separate flow where you would manually add in plugin names, although you probably did this as a POC maybe

AckslD commented 2 years ago

@akinsho I think it mostly work as the current workflow (although entry point is not the same and display looks a bit different). The explicit plugin names was just an example, you can also call it without any arguments and then it will fetch all plugins and you can check the diffs, in the same way as update handles arguments.

Actually, initially I started with an implementation which would branch inside update but found it difficult to unify the actual pulling in the two cases, due to the order of things as currently done. But that's an implementation detail.

But I might be misunderstanding what you mean so feel free to expand on how you find it different :)

AckslD commented 2 years ago

Actually I think I now understand the code better and maybe have an idea to better integrate the code and behaviour of update. I'll push an update here when I have it :)

pidgeon777 commented 2 years ago

I love the concept behind it.

It would be great to implement a buffer showing previews of plugin updates, also offering a chance to manually update any of them.

In the future, maybe even a function to roll back the chosen plugin to a previous version?

Anyway, can't wait for the outcome 👍👍👍.

wbthomason commented 2 years ago

This is neat, thanks a lot! (and sorry for not getting to this sooner). I'm holding off on reviewing in detail until you push updates, but @ me when it's ready.

wbthomason commented 2 years ago

(also @ me with implementation questions if you have them, or ping me on Matrix)

AckslD commented 2 years ago

Thanks! Things have been pretty busy last week but I have a new version almost ready which mostly just needs some cleanup so I'm hoping to push this soon for some feedback :+1:

AckslD commented 2 years ago

Hi all! Here's now a new version. I think there is a lot to discuss about the implementation and I have some thoughts questions below. But would be good to first see if people think the workflow makes sense in this new version. There is now a new function update_preview which is basically the same as update except that it instead fetches and allows you to preview the changes before continuing with the updates you want. See the following screencast where I wrote some notes on the side:

https://user-images.githubusercontent.com/23341710/181730633-0838559a-7a6d-49c8-8e6c-ddbdf7952919.mp4

Currently it's no real difference between the display before continuing with the updates and after, other than that before is says "Can update..." and after it says "Updated...". So it might be a bit hard to quickly see at which stage you are. But I'd say that's a separate issue and one could think about improving the UI.

Implementation questions

[ ] I added an opts to _update (previously update) which is called by update and the two new update_preview/update_head. This is used to specify eg if we're only fetching and previewing, or updating, or only updating with the local fetched version (this is to avoid the unlikely scenario where remote gets an update between the fetch and the actual update). This opts is passed down all the way to the plugin.updater and to the display. Is this a bad way to do things?
[x] I needed some way to refresh final_results, so for now just cached it's inputs and added a simple update_final_results. Is this fine? I guess ideally one would do something like how things are done in eg task_succeeded with extmarks but would require some rework of final_results.

I changed this now to a better way using extmarks.
[x] There are two new keybinds (x to remove a plugin from being updated and c to continue). Since these actually only makes sense in the preview display they should probably be only be set there. Just haven't fully thought about yet how to do that best. One could dynamically build keymap_display_order inside final_results instead of outside maybe?

Now the keymaps relevant to the preview are shown only in the preview (although they are still mapped).

Some other thoughs

The following two things is what I basically also included in the first version. But I realise now that their are actually independent features and would there be better to decouple them:

I think it would be nice to optionally be able to only show one plugin per line in the final_results, ie without the commits. This could be opt-in to not change the current view. But I think I would find this nicer to just see what plugin got updated or can be updated and only check the commits etc if I want to.

Started implementing this in #1035
Related to this, the current way to see diffs (d) can only show the diff of a single commit. At least from what I can tell. Even if you press d on the line of the plugin which has a range of commits you only see the diff of the first. I think it would be nice that if the line has a range of commits you see the full diff.

Started implementing this in #1034

fyi @wbthomason @akinsho @gegoune @pidgeon777 @pedro757

akinsho commented 2 years ago

Will have a look when next I get a chance @AckslD 👍🏿

AckslD commented 2 years ago

Just checking if anyone having some thoughts on this? :) I've been using it for quite some time and it works well for me. I improved the implementation a bit (solving two of the questions above). So if people are happy with this I'd say it's ready to be merged.

akinsho commented 2 years ago

@AckslD I've just pulled the branch and will run it locally for a couple of days and feedback. Two things to check:

Does enabling it just require setting config.diff_preview = true?
When running packer sync it shows a failed update status symbol for most of my plugins is that expected?

AckslD commented 2 years ago

Cool @akinsho, no atm it is not done by any config but instead calling require('packer').update_preview() instead of require('packer').update().

akinsho commented 2 years ago

@AckslD so other than the issue I mentioned above

When running packer sync it shows a failed update status symbol for most of my plugins is that expected?

I'm also noticing that when using the update_preview command that even after pressing c to continue when next I press the command it's still referencing some much older previous commits in already up-to-date plugins. I'm not sure if relates to my use of tags for some plugins though

AckslD commented 2 years ago

@akinsho, thanks a lot for trying. I think

When running packer sync it shows a failed update status symbol for most of my plugins is that expected? should be fixed now.

Regarding your second issue, do you mean that after the actual update the commits shown also include older commits? That's a bit strange since the second command should be equivalent to the current update. Do you have some more details on exactly when this happens?

akinsho commented 2 years ago

I'll try to get a reproduction +/- GIF, it's more that for example I have nvim-dap installed using tag = "*" when I run the command multiple times it still flags nvim-dap each time (as an example) as though there are changes to review. Probably because it's not diffing against tags but just against main but since I'm restricting things to tags I don't expect to keep getting asked about updating that plugin

akinsho commented 2 years ago

@AckslD I can definitely confirm that the issue I described as showing updates for plugins that don't need to be updated is because of the tag key

There are way too many to show but all the plugins above have been pinned to a tag or use tag = '*' so there are changes in master but since in the case of those plugins all I'm really interested in is if there is a new tag, and then it's worth showing the commits in between.

Not sure how to handle it, but I think the options are to, ignore plugins with tags this would be unfortunate since most people are moving to using tags. Or I think better would be to check for new tags and if so show those commits, otherwise omit those plugins from the to update view especially since updating them a) doesn't work b) would be quite undesirable if it did.

AckslD commented 2 years ago

Ah right I see @akinsho, that makes sense! I just pushed a fix now that makes sure that plugins pinned to a tag or commit don't show potential updates. I at the same time added a similar sync_preview and the corresponding UpdatePreview and SyncPreview commands.

AckslD commented 2 years ago

Oh but I hadn't realised that tag="*" was an option and saw your message to late. I thought that one only could pin to a fixed tag and therefore ignored plugins which had plugin.tag ~= nil. Hmm, I'll try to think how to check for possible updates from only tags.

AckslD commented 2 years ago

@akinsho This should now be fixed and we now check previews of updates if there is a new tag (if wildcard) but otherwise not.

akinsho commented 2 years ago

@AckslD just tried it out and works quite nicely 👍🏿 what are your thoughts on adding these commands as flags to the existing commands +/- as options in the config. I think rather than proliferating the number of commands packer has, it would be better (IMO) to just allow this to be something that can be configured via the normal commands e.g. PackerSync --preview or something as well config.update_preview

AckslD commented 2 years ago

@akinsho cool :) Yeah sounds good. Adding it as an option in the config is easy.

As a flag to eg PackerSync should be fine but maybe a bit more tricky since it currently accepts any number of arguments which are assumed to all be plugin names. Would it make sense to filter this list for eg --preview or do you have some other thoughts how to handle such a flag?

akinsho commented 2 years ago

@AckslD yeah that is a shame re. that command's structure, one thing that worries me would be allowing flags to be inside a list of plugin names e.g. PackerSync plug1 plug2 --preview plug3 I feel like the flags should be at the beginning, before the plugin name list. PackerSync --preview name1 name2 name3

AckslD commented 2 years ago

@akinsho I refactored things a bit so there is now a preview_updates config setting and you can also do PackerUpdate --preview or PackerSync --preview but only as the first argument.

Furthermore you can also do eg require('packer').update({preview_updates = true}, ...) where the first (and only the first) argument can optionally be a table of options.

I also remove the other functions and commands I previously added and checked that things work.

AckslD commented 2 years ago

Hmm, seems there was some formatting done causing conflicts

akinsho commented 2 years ago

So from a usability perspective, I think this PR works quite well. I think it would be nice in the future to add the ability to decide which plugins should be updated or not e.g. marking some as not for update, but I've been using it for a few days and this is super helpful functionality.

I think if you clean up the merge conflicts then I think it'd be fine to merge but would be good to get @wbthomason input on the implementation.

AckslD commented 2 years ago

Cool, thanks @akinsho! Just so I understand what you mean with:

I think it would be nice in the future to add the ability to decide which plugins should be updated or not e.g. marking some as not for update,

You can do this by pressing x on a plugin you don't want to update. It will then go away and not be updated when you continue (c).

akinsho commented 2 years ago

Oh, cool @AckslD I didn't realize it was already handled 🚀

AckslD commented 2 years ago

Conflicts are now resolved. Feel free to maybe also try the feature of removing plugins you don't want to update. I've also been using it for a while but always good if someone else also tries it :)

AckslD commented 2 years ago

Thanks a lot for checking @wbthomason! I've handled the inline comments above.

I've been able to repeat this behavior with other plugins (e.g. if I manually run git checkout HEAD~5 in some cloned plugin, then run packer.update(), packer will run the actual update before I confirm/reject).

Could you try in the latest version (where it's opt-in) with PackerUpdate vs PackerUpdate --preview (or packer.update() vs packer.update({preview_results = true}))? Note that the results of the two looks quite similar, but one says Updated ... and the other Can update ....

In the preview you press x to disable updating some plugins and c do actually update the rest.

As a final nitpick: I would prefer that rejecting an update toggles its status in the display, rather than the current behavior (which seems to remove it), in case users mistakenly hit the keybinding. I may have missed something here, as I see in the thread above (and demo video) that toggling was enabled?

Maybe this would be better indeed. Since I now added extmarks to the final results this should be pretty easy. Any opinions of how to display this? I guess there can be three symbols:

✓ for plugins which will be updated when pressing c.
• for plugins which won't be updated
✗ for plugins that failed

and you can toggle between 1. and 2. with x.

Btw, how should 3. be handled? Ie plugins with failed when just fetching updates? I think we can just not continuing updating those and the user cannot change that at this stage.

AckslD commented 2 years ago

@wbthomason @akinsho In the latest version I changed so that instead of removing plugins to remove one toggle the status and can therefore re-enable it for updating again. Here's a short demo how it looks like now:

https://user-images.githubusercontent.com/23341710/188269763-642014e1-9d2e-480b-bc0a-7525dd7e6ac0.mp4

(Note that I'm using the compact flag from #1035)

You can see I have 3 plugins with updates and one failing, I can then inspect the diffs etc and toggle two to not update and in the end continue which only updates the last one (the failed one it not attempted to update again).

Since we're now toggling instead I changed the mapping from x to u.

wbthomason commented 2 years ago

@AckslD Thanks for the changes! I'm still having trouble with the feature - maybe it's because I'm only testing with local changes (i.e. using git checkout HEAD~5 to artificially generate updates), but (1) the updates get applied every time before I confirm, and (2) running packer.update({preview_results = true}) with preview_updates = false in config does not give me the preview display (maybe I'm misunderstanding how that argument is meant to work).

Do you mind sending instructions for how you've been testing this, so I can figure out if it's a problem with my approach/setup, or with the feature?

AckslD commented 2 years ago

Sorry I misstyped, it should have been packer.update({preview_updates = true}), ie the option is the same as the config setting.

For testing I run a bash script with something like:

(cd ~/.local/share/nvim/site/pack/packer/opt/nvim-treesitter-refactor && git reset --hard origin/repeat-smart-rename~1)
(cd ~/.local/share/nvim/site/pack/packer/opt/plenary.nvim && git reset --hard origin/master~3)
(cd ~/.local/share/nvim/site/pack/packer/opt/nvim-pytrize.lua && git reset --hard origin/dev~2)
(cd ~/.local/share/nvim/site/pack/packer/opt/nvim-dap && git reset --hard 0.1.0)

nvim +'PackerUpdate --preview'

which of course need to be updated for some plugins of yours. Let me know if it still doesn't work.

wbthomason commented 2 years ago

Thanks! I had been using soft resets (i.e. git reset HEAD~N); using the hard reset shows me the expected behavior. I think the hard reset method is a more accurate simulation of the use case of this feature (i.e., updates from upstream), so I'm OK with it not working as expected only in the soft reset case.

One last question: the current continue behavior if all plugins are toggled off is to display a warning and do nothing. I would personally expect it to maybe print a warning, but still continue as usual, completing immediately because there are no tasks. Which behavior do you (and @akinsho) think makes more sense? I can see an argument for either.

AckslD commented 2 years ago

Cool!

I think the current behaviour, when no plugins are selected, makes more sense but happy to change if you prefer.

My reasoning is that, if no plugins are selected and you continue, it doesn't do anything. So it's most likely a mistake from the user, since I don't see a reason why you would want to do this null-operation. And if it's a mistake from the user, I'd say it is better to display a warning/message and stay on the current screen, allowing the user to correct the mistake (or just quit if it wasn't a mistake).

wbthomason commented 2 years ago

Fair enough. I'm good with that reasoning. Thanks for the cool new feature implementation!

wbthomason / packer.nvim

Diff preview #975

Implementation questions

Some other thoughs