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.

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

Don't change default find-file behaviour #28

Merged
merged 1 commit into from
Apr 30, 2016

Conversation

Wilfred
Copy link
Contributor

@Wilfred Wilfred commented Apr 5, 2016

Currently, opening any file that is read-only and not owned by the
current user triggers a root login prompt. This occurs for viewing
system files (e.g /etc/fstab), built-in elisp
libraries (e.g. simple.el.gz) and simply viewing files owned by other
users on a shared system.

We can't assume that this is the behaviour users want, and changing
built-in functions without asking is surprising.

Fixes #20.

@bbatsov
Copy link
Owner

bbatsov commented Apr 5, 2016

I'll take a look at this. There's a writable check that should prevent this for files you can actually write to. Add if the hook's removed it should be documented.

@bbatsov
Copy link
Owner

bbatsov commented Apr 5, 2016

To be clear - I agree with the spirit of the change, but:

  • this functionality should be discoverable
  • this functionality shouldn't cause the effect you described even when enabled

@Wilfred
Copy link
Contributor Author

Wilfred commented Apr 5, 2016

@bbatsov I'm still not clear what the purpose is of adding this function to the hook. crux-reopen-as-root is definitely useful, but when would users want it automatically called?

@Wilfred
Copy link
Contributor Author

Wilfred commented Apr 30, 2016

@bbatsov ping, would you be able to take a look at this?

@bbatsov
Copy link
Owner

bbatsov commented Apr 30, 2016

@bbatsov I'm still not clear what the purpose is of adding this function to the hook. crux-reopen-as-root is definitely useful, but when would users want it automatically called?

The idea was that you won't have to think about the command at all as it would be applied automatically to some files if necessary. As I said before, I think the behaviour you've experienced is a bug in the file check done before reopening the file. The original idea was for this to affect just files owned by root. Anyways, I can imagine this being annoying, but I'm guessing some people also find it useful...

@Wilfred
Copy link
Contributor Author

Wilfred commented Apr 30, 2016

There are two issues here:

1: This applies to reading all files that aren't owned by the current user. For example, (find-file "/var/log/postgresql.log") triggers this even though the file isn't owned by root. I think this is the bug in the check you're referring to.

2: Users who want to simply view a file owned by root are prompted for sudo credentials. If they don't have sudo credentials, or don't want to use them, they're prompted every single time. This is my concern.

How would you feel about making this opt-in? We could move this line of code to the readme or define a crux-reopen-as-root-mode. Currently this change in behaviour is not documented AFAICS.

@bbatsov
Copy link
Owner

bbatsov commented Apr 30, 2016

How would you feel about making this opt-in? We could move this line of code to the readme or define a crux-reopen-as-root-mode. Currently this change in behaviour is not documented AFAICS.

I agree. Probably making this a trivial mode that simply adds and removes this hook would be best. Or we can hook into saving the file so people are not prompted to do anything unless they actually try to change it...

@Wilfred Wilfred force-pushed the dont_hook_file_loading branch 2 times, most recently from 6cc6490 to 5969cc8 Compare April 30, 2016 21:50
@Wilfred
Copy link
Contributor Author

Wilfred commented Apr 30, 2016

Wonderful, thanks for being supportive :)

I've written a little global minor mode, let me know what you think.

@bbatsov
Copy link
Owner

bbatsov commented Apr 30, 2016

Looks good. Just mention it somewhere in the README.

Rather than changing the default find-file behavior, providing a minor
mode enables users to opt-in or toggle the behavior.

Fixes bbatsov#20.
@Wilfred Wilfred force-pushed the dont_hook_file_loading branch from 5969cc8 to da6ca15 Compare April 30, 2016 22:59
@Wilfred
Copy link
Contributor Author

Wilfred commented Apr 30, 2016

README updated.

@bbatsov bbatsov merged commit a6e0a52 into bbatsov:master Apr 30, 2016
@Wilfred Wilfred deleted the dont_hook_file_loading branch May 1, 2016 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants