fish-shell icon indicating copy to clipboard operation
fish-shell copied to clipboard

Add a fish_delta helper function

Open faho opened this issue 3 years ago • 9 comments

Description

This goes through $fish_function_path/$fish_complete_path and checks which files you have that aren't in the stock fish install. So it helps you to figure out which functions and completions you've overridden.

For instance, on my system:

> fish_delta -fd | head -n 5
New file: /home/alfa/.config/fish/test/functions/__fish_supports_cursor.fish
New file: /home/alfa/.config/fish/test/functions/_fish_systemctl.fish
New file: /home/alfa/.config/fish/test/functions/aur.fish
New file: /home/alfa/.config/fish/test/functions/fish_describe_machine.fish
New file: /home/alfa/.config/fish/test/functions/fish_set_cursor.fish
> fish_delta --no-completions --no-new --no-diff
File /home/alfa/.config/fish/functions/fish_clipboard_copy.fish differs from /usr/share/fish/functions/fish_clipboard_copy.fish
File /home/alfa/.config/fish/functions/fish_greeting.fish differs from /usr/share/fish/functions/fish_greeting.fish
File /home/alfa/.config/fish/functions/fish_prompt.fish differs from /usr/share/fish/functions/fish_prompt.fish

(and without "-d"/"--no-diff" it would show you the diff on changed files, with automatic paging - but that's awkward to demonstrate here)

One wrinkle is that it'll only show function files - if you've e.g. overridden a function in config.fish, we won't be showing it. I've tried working on that, but it's awkward considering multi-function files like fish_git_prompt.fish.

I think it's still useful, it'll allow me to figure out if I have a completion I'm testing (hence the test/completions directory!) and need to merge.

TODOs:

  • [X] Changes to fish usage are reflected in user documentation/manpages.
  • [N/A] Tests have been added for regressions fixed
  • [ ] User-visible changes noted in CHANGELOG.rst
  • [x] Should we ignore vendor stuff, by default?
  • [x] Should we ignore new files by default?

faho avatar Oct 03 '22 14:10 faho

Okay, ignoring new files makes more sense - they don't change anything, they aren't relevant for debugging etc, and there may be a lot of them.

I'm still not happy with the vendor dirs - there are three ways to handle them:

  1. Treat them as "user" customization. Anything they do counts as "changed" or "new" compared to what we ship
  2. Treat them as "default" - anything the user has of the same name as them counts as "changed"
  3. Ignore them, like we do the generated completions - anything the user has of the same name counts as "new" (unless it's also in our dirs, in which case it might be changed)

In a certain sense, number 1 is true. These aren't things we ship, they are customizations compared to a stock fish.

But in another sense, that's entirely not the same as a user overriding things. I have a lot in these directories simply because my distro packages ship stuff there! So it's "default" according to what my system ships.

So number 3 feels like the least annoying option, possibly with a flag to switch to 1. (but tbh our vendor path logic is overcomplicated, especially with all the $XDG_DATA_DIRS stuff, and there's an actual possibility we end up getting these paths wrong)

faho avatar Oct 04 '22 18:10 faho

this is intriguing, seems very useful.

I'm still not happy with the vendor dirs

On a first glance, 2 or 3 sound like viable options. I certainly would like to simplify the vendor loading logic, somehow..

krobelus avatar Oct 06 '22 04:10 krobelus

In fact I probably need to see if --color=always needs to be checked.

Okay I wasn't in the mood to do feature detection so I just added coloring via string replace. I await your screams.

faho avatar Oct 06 '22 20:10 faho

Alright I implemented a "--vendor=" option to allow picking how these are counted.

By default they are counted as "default" (or "stock") files.

Note: This necessitates new global variables set via share/config.fish - along with e.g. $__fish_data_dir we now have $__fish_vendor_functionsdirs, $__fish_vendor_completionsdirs and $__fish_vendor_confdirs. These are set to the directories we used on startup.

The alternative is to redo the logic for finding the vendor files in fish_delta, which appears worse.

It just feels kind of awkward to make that change here, in a semi-related PR, but on the other hand I also don't love doing a separate PR and threading that in here?

faho avatar Oct 07 '22 14:10 faho

I think it helps to decide what the biggest use case for this would be in determining how to handle vendor dirs.

e.g. is this for the user's discoverability or for us to be able to determine if the user is running a stock config or not (we could ask them to run this when they file an issue and it appears that it's a config-related problem).

mqudsi avatar Oct 07 '22 18:10 mqudsi

e.g. is this for the user's discoverability or for us to be able to determine if the user is running a stock config or not (we could ask them to run this when they file an issue and it appears that it's a config-related problem).

As far as I'm concerned: Both.

One use case is finding out "what have I changed" - as an example I, personally, used previous evolutions of this to figure out what stuff I have left to commit, because I like test-driving things in my personal config for a while.

Another is finding out "what is changed over the stock configuration". This is both for debugging - bug reports to us could include something like this[0] - and for figuring out what sort of stuff differs from a stock fish so you can redo it on other systems.

Both of these necessitate treating vendor dirs differently - in the former, you don't really care about the separation between "the fish project's stuff", so it should all be counted together. This should be the most common and so it is the default mode of operation.

In the latter, you want to know what changed over "the fish project's stuff", so this is "--vendor=user" mode.

The idea behind "--vendor=ignore" was that you'd use it if the vendor stuff is annoying. You don't really choose what gets installed there and removing stuff there is frowned upon (because it's installed from packages and will just be overwritten on update), so you might end up with a bunch of vendor stuff that you don't care about.


[0]: But note that people love their aliases in config.fish and this won't help with that, so it's not a panacea.

faho avatar Oct 07 '22 18:10 faho

Okay, this now looks at config files as well.

Like the commit message calls out, it will count them as "changed" even if they don't shadow anything. This means you'll easily see all your snippets and config. On the other hand it means you'll see a bunch.

Let's see how that shakes out - this is easy enough to change later because it's not really a "script" thing.

faho avatar Oct 18 '22 16:10 faho

Do you want to add it to the changelog right now, while it's still fresh in your mind?

mqudsi avatar Oct 18 '22 18:10 mqudsi

Do you want to add it to the changelog right now, while it's still fresh in your mind?

I find doing changelog in the PR only leads to annoying merge conflicts.

faho avatar Oct 18 '22 18:10 faho