customizable default action and flags for commands

[eschulte]

Introduces the magit-command-defaults customization variable which
allows users to specify default flags to select in the pop-up command
interface and/or default actions to perform without raising the pop-up
command interface at all.

See the documentation of magit-command-defaults for full usage
information. The default value of magit-command-defaults is taken from
two hacks previously hard-coded into the magit-key-mode-generate
function.

[npostavs]

This slightly breaks magit-key-mode-popup-diff-options: it forgets what options have been set.

[eschulte]

Agreed, but perhaps the entire magit-diff-options, magit-set-default-diff-options, magit-key-mode-popup-diff-options, magit-set-diff-options scheme is overly complicated with multiple variables and support functions for a customization which (at least to me) looks like it should be global. Am I missing why someone would want to set this in a buffer-local way? If so then I could add support for buffer-local functionality to this commit.

I propose updating this pull request to remove all of the existing magit-diff-options stuff since it is now a special case of the new magit-command-defaults functionality. This would significantly simplify magit.el.

[eschulte]

If it helps I could also add a command to set default actions and flags with nice read-from-minibuffer prompts.

[npostavs]

Am I missing why someone would want to set this in a buffer-local way?

I like to turn on whitespace ignoring just depending on what diff I'm viewing, I don't want it in all magit buffers. Your patch actually doesn't break this use case, just the UI of the popup becomes a little inconsistent (you can't use it to see what options are currently set).

[eschulte]

Ah, thanks for explaining, I've just pushed up an additional commit which resurrects the display of these options in the UI popup.

[eschulte]

I just pushed up a new version of this branch which simplifies the code and removes the need to re-generate magit-key-mode functions after resetting the value of magit-command-defaults.

[tarsius]

If I had more time I would have written a shorter comment :-)

I was a bit shocked when this pr arrived. Let me explain. Two days earlier I got frustrated with the lack of progress in the key-mode department. Remember this is actually one of the areas that I would very much enjoy working on, but I have originally chosen to delay doing so, in order to not further jeopardize the goal of an "immanent release".

Well, two days ago I decided that enough is enough, and that for a change I wanted to work on some features I myself am longing for (instead of architectural cleanup and features I am being asked to implement) and starting hacking.

Out of necessity and habit I started by cleaning up the existing key-mode code. After a day's work I had reduced the code to half of its original size, with no loss of (significant) functionality, and while keeping the changes atomic and gaining a few minor features and abstractions for future development along the way. At that point I was both hopeful [1] and a bit frustrated [2].

1. Trimming down to the essentials was easier than expected and building new features on top of the remnant suddenly looked easy.
2. I had spend several hours on something that I could have done in two. Most of the time was not spend on actual improvements but on tearing the old implementation down step by step, while making sure it kept working at each commit, and documenting each step in form of a commit message. Making sure neither the behavior nor the display did change in little ways (to make the transition as painless as possible for users) also did not exactly help making quick progress.

At that point I started to think that I should have stuck to my original plan: implement a new mode, inspired by magit-key-mode but not actually deriving from it.

So I took a break and made the mistake of having a look at the Magit issue tracker as part of that. That's when I saw your pr... after the work I had done just before your pr came in, your patch no longer applied... not as a patch and also not conceptually... and now I would have to explain that to you...

---

So I took another break, this time for two days, to consider what my options are and this is what I arrived at:

1. I am going to write idefix from scratch, instead of gradually improve upon `magit-key-mode'. Once that is done Magit can switch to using that instead. And other packages can do the same.
2. Because idefix is going to take some time to write, I can no longer justify not merging/implementing the change you are proposing. This is a much needed feature and I can fully understand you and others (including myself) want it now, not in some distant future.
3. I still don't think what you suggest is the best approach, but that's beside the point. Given the existing implementation it actually is the obvious way of doing it.
4. One reason why I originally refused to take this approach, is that I already know I would eventually have to replace it with something else. That would involve removing an existing variable, and that in turn (and I know that from experience) would not be appreciated by some users that have grown accustomed to it, and who would feel like I am needlessly taking something away from them.

So basically I had to decide between (a) making someone happy now at the cost of making someone unhappy later on, and write code I already knew I would later throw away; or (b) making someone unhappy now and not write any code. I have chosen (b) because its opportunity cost is 0 while that of (b) is 1.

But the situation has changed: time has passed and I have not written the improved key-mode.

---

I have looked at your patch and while I now agree that this is the right thing to do at this point, there are some problems with the actual implementation. Tomorrow I will write my own variant and merge it into master. I hope you understand that I am writing it myself, instead of explaining what's not so great about your implementation. I think that will save both of us some work.

A very general description of what is not so good with you implementation is this: it changes the behavior in unexpected and potentially destructive ways. And then a few minor things.

[tarsius]

So after having looked at your proposed patch and having tried to implement this myself, I have to take back some of the things I have said above.

Even when ignoring all the bugs in you patch and that it is rather naive, this is not how it should be done, not even with the current implementation. The current implementation is completely insufficient for this, and we should not try to go beyond its boundaries like this.

I am very disappointed in myself for once not having brought up the strength to say no.

Now my answer is just that: a strong and definite no.

There is still hope for those wanting these features. I have already started working on idefix. But I make no promises that I will release it any time soon.

Your patch (as well as mine) attempted to do two things:

• Default actions. This is absolutely out of question. Key-mode is not powerful enough to completely replace prefix arguments, so over-loading prefix arguments to do something else is insanity. What about actions/commands that take prefix arguments?
• Default arguments. This is not as problematic as the former. Not all arguments in a popup necessarily work with all of its actions. One possible way to address this is to automatically filter out invalid arguments.

If you want to keep going down this rabbit hole I recommend you have a look at my attempt which I have pushed to: https://github.com/tarsius/magit/tree/dead/key-mode-defaults. The doc-strings are probably more valuable than the code.

[eschulte]

Even when ignoring all the bugs in you patch and that it is rather naive, this is not how it should be done, not even with the current implementation.

Can you articulate a specific bug or problem with this patch?

The current implementation is completely insufficient for this, and we should not try to go beyond its boundaries like this.

Without any technical specifics I have no idea what the above is supposed to mean. I've been using this functionality as implemented by this patch for a while now and I find that it works well, so I disagree that it is impossible.

Default actions. This is absolutely out of question. Key-mode is not powerful enough to completely replace prefix arguments, so over-loading prefix arguments to do something else is insanity. What about actions/commands that take prefix arguments?

I thought about this some recently. To my mind the simplest solution is to absorb the first prefix argument by key-mode and pass along any additional prefix arguments to the default action. I don't know of any key-mode commands which take complex prefix arguments and thus wouldn't work with this scheme.

Default arguments. This is not as problematic as the former. Not all arguments in a popup necessarily work with all of its actions. One possible way to address this is to automatically filter out invalid arguments.

Since when do Emacs packages avoid adding functionality simply because it could be miss-used?

Anyway, I'm happy for you to not merge this, and I'm happy to keep using it (and the multi-proc support) from my own fork. In the future it would be nice to know what functionality has any chance of being merged before submitting a pull request, but without an active mailing list I don't see how this could happen.

Thanks for your consideration.

[tarsius]

Anyway, I'm happy for you to not merge this, and I'm happy to keep using it (and the multi-proc support) from my own fork.

I am glad you take it in this light. I do realise I am coming on strongly here (and a bit insecurely :-/).

In the future it would be nice to know what functionality has any chance of being merged before submitting a pull request, but without an active mailing list I don't see how this could happen.

The issue tracker might not be as convenient, but you can make you intentions to work on something known here.

Thanks for your consideration.

Thanks for your understanding :-)

To the other questions I will reply later. Meanwhile please have a look at my patch, especially the doc-strings, I think it addresses some of them.