Skip to content

Conversation

@nfischer
Copy link
Contributor

Previously, :PlugDiff would show every new commit to a plugin's git
repo. This makes sense for the general case, but makes less sense when a
plugin lives in a subdirectory of the repo (and is configured with the
'rtp' option). This makes it difficult to determine which commits relate
to the plugin and which are unrelated.

This changes :PlugDiff to filter out any commits outside of the 'rtp'
folder.

Some consequences:

  • This does not change the :PlugUpdate UI. This means :PlugUpdate
    may pull down non-plugin commits, display that it has updated the
    plugin, and then :PlugDiff will show no updates (since such commits
    fall out of the 'rtp' path).
  • It also means there's no UI to revert non-plugin updates, as they
    don't show up in :PlugDiff.

Fixes #797
Test: Manual on Linux machine

Previously, `:PlugDiff` would show every new commit to a plugin's git
repo. This makes sense for the general case, but makes less sense when a
plugin lives in a subdirectory of the repo (and is configured with the
'rtp' option). This makes it difficult to determine which commits relate
to the plugin and which are unrelated.

This changes `:PlugDiff` to filter out any commits outside of the 'rtp'
folder.

Some consequences:

 * This does not change the `:PlugUpdate` UI. This means `:PlugUpdate`
   may pull down non-plugin commits, display that it has updated the
   plugin, and then `:PlugDiff` will show no updates (since such commits
   fall out of the 'rtp' path).
 * It also means there's no UI to revert non-plugin updates, as they
   don't show up in `:PlugDiff`.

Test: Manual on Linux machine
@nfischer
Copy link
Contributor Author

I see this breaks tests on my local machine, but I can't figure out why it would do so. I could cheat by only applying " -- ." if the rtp option is supplied (tests will pass), but this makes me wonder if the patch would fail under legitimate circumstances. Any help debugging would be appreciated.

Also, I'm happy to change this to an opt-in feature if the noted consequences are concerning.

@nfischer
Copy link
Contributor Author

Oh, now I see. 2 issues going on with the PR:

  1. It's not correct to unconditionally apply -- ., because a repo may have empty commits (--allow-empty). These are used all over tests, which is why some tests broke.
  2. If any test breaks, it appears to halt execution at the first failed assumption. Unfortunately, this often leaves an extra window open (the :q-cleanup is at the end of the test). So, each failed test may cause side effects, which caused some other failures related to winnr('$') (which has now changed).

@junegunn
Copy link
Owner

junegunn commented Nov 3, 2018

Hi, thanks for the patch! I just made a small change to your branch. I'll merge this once the test passes.

@junegunn junegunn merged commit 734d9a1 into junegunn:master Nov 3, 2018
@nfischer
Copy link
Contributor Author

nfischer commented Nov 4, 2018

Thanks, @junegunn (and good catch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants