Move over to Transient

[hokomo]

Here it is as promised. Suggestions welcome!

I've kept the conversion of each command as a separate commit so that it's easier to follow and review, but we can of course squash some of these later if this is too verbose.

The conversions all share a pattern -- convert the popup to a transient and slightly rework the corresponding command (update the docstring, make the code more idiomatic, fix any easy stuff, etc.).

As mentioned, I've tried not to break or significantly change any existing functionality, but I did take the opportunity to change a couple of small things here and there. In particular:

• We now require a minibuffer match when edit, refresh and sink (its target) prompt for a patch. Since magit-stgit-read-patches doesn't have a parameter for a default, I've removed the defaults from the prompts for now (not requiring a match and manually handling empty input, as the code was doing, is not the best way to handle the default I think).

• The commit command now also tries to read a patch from the minibuffer as a last resort (I'm not sure why the code was passing t for REQUIRE-MATCH but then nil for PROMPT, to magit-stgit-read-patches).

• The commit function now takes care to look out not just for --all but also --number. If these are specified, PATCHES is ignored (I don't know exactly why the code was expecting to find just a single patch in case of --all).

Questions:

• I don't know what names to use to differentiate between the transient and the function that does the work. Before we used to have the -popup suffix. For now I just decided to temporarily name them magit-stgit-CMD and magit-stgit--CMD. The double-dash name is definitely not good, as they're all public functions that the users can use (both interactively and programmatically). Ideas?

• Almost all of the commands rely on magit-stgit-read-patches to try and gather some patches. When it inspects both the region and the point, I've documented this as "patches around point"; when it inspects the point but not the region, I've used "patch at point" instead; and when it prompts for a patch, I've used "read one from the minibuffer". Is the around/at distinction good enough?

• My magit-stgit--commit-need-patches-p has to handle both --number and --number=, since we want magit-stgit--commit to be able to function regardless of the syntax the user/programmer decides to use for the options and whether or not it was invoked through Transient. There's no way around this, right?

• The key binding for the main magit-stgit popup was /, which I've left as is and added just after the Forge @ key binding in magit-dispatch. Any better ideas?

Questions @tarsius:

• I used Transient 0.3.7. Is this too recent to use as a requirement?

• magit-define-popup used magit-stgit-commands as the Custom group. I wasn't able to find the equivalent of this in Transient so I removed it for now.

• I assume transient-current-prefix is the equivalent of magit-current-popup.

• I've used :man-page in all of the prefixes, but how do I do it for suffixes (e.g. magit-stgit-init)?

• Do infix arguments have to be strings, i.e. do I have to use number-to-string after read-number?

• Is there any benefit to always mentioning both the short and the long form for a Transient option (using a list of the form (SHORT LONG))? Right now I don't do this anywhere.

Questions @jpgrayson:

• I assume StGit guarantees that it supports both --long-option VAL and --long-option=VAL option syntax, right?

• I've explicitly used -- as a separator when invoking StGit commands to be able to handle any funny patch names. Could this be a problem, according to stacked-git/stgit@7b87a0d?

[tarsius]

• I don't know what names to use to differentiate between the transient and the function that does the work. Before we used to have the -popup suffix. For now I just decided to temporarily name them magit-stgit-CMD and magit-stgit--CMD. The double-dash name is definitely not good, as they're all public functions that the users can use (both interactively and programmatically). Ideas?

You can use the same function for both tasks, see magit-patch-apply for an example. In some cases that might not make sense or require unfortunate argument gymnastics.

[jpgrayson]

This is great! Thank you, @hokomo. Here's answers to your specific questions for me. I'm going give this PR more review and exercise the changes over the next few days.

I assume StGit guarantees that it supports both --long-option VAL and --long-option=VAL option syntax, right?

Yes, any long option that takes a value may use either --long=value or --long value.

I've explicitly used -- as a separator when invoking StGit commands to be able to handle any funny patch names. Could this be a problem, according to stacked-git/stgit@7b87a0d?

What comes after a -- separator is command-specific, but generally what comes after that separator are pathspecs, not patchnames (this is more true for StGit 2.0). Some example usages are:

stg show [OPTIONS] [patch]... [-- <path>...]
stg refresh [--] [path]...
stg delete <patch>...

It is only commands with the last form, i.e. those that only have patch arguments and do not take pathspec arguments, that a superfluous -- separator may be added. E.g. stg delete p0 and stg delete -- p0 would be equivalent.

[tarsius]

I'm going give this PR more review and exercise the changes over the next few days.

Same here, except I am going on a mini-vacation soon, so it will take a bit longer.

[jpgrayson]

Overall this looks really clean and on-target for the goals you stated--really well done and a great step forward!

I am unable to run a several commands due to the use of the undefined first function (see comment for details). I may have more comments once that issue is resolved.

Also, I am also still trying to figure out the best way to load/enable this extension in my doom-emacs setup. I can get it working with a combination of config-time and runtime hacks, but I don't yet have a clean/reproducible way to turn-on magit-stgit. I'm not yet able to tease apart what might be intrinsic to doom-emacs versus my doom configuation versus the magit-stgit extension itself. So this is something that I'll continue to try to get to the bottom of.

[jpgrayson]

I see a lint that using alist-get demands a minimum emacs version of 25.1. whereas the current requirement is emacs 24.4.

Seems like upping the minimum emacs version to 25.1, which was released in 2016, should be reasonably uncontroversial.

That said, one thing I experimented with was whether removing the dash dependency would be feasible. If we were to set the minimum emacs to 27.1 (2020-08-10), then we could use the builtin flatten-list instead of -flatten and a (flatten-list (mapcar ...)) instead of -mapcat to eliminate dash.

I'm comfortable with having emacs 27.1 as the minimum version if it enables removal of a dependency.

[hokomo]

Nice catch. Perhaps we should set up some Makefiles soon to make the compilation reproducible? I think at least a couple of times the byte-compiler didn't manage to catch some issues for me just because there's always a bunch of stuff loaded into my Emacs. I've noticed @tarsius already has a streamlined set of scripts that he uses in his packages, so maybe we should use something similar. :-)

I'm comfortable with bumping up the version as well, even as high as 27.1, as I use at least Emacs 28 on all of my machines. Given that we would like to target StGit 2.0 which is even more cutting edge, I don't think this would be too big of a deal, but perhaps some people might want to use an older version of Emacs while testing out StGit 2.0? @tarsius Given that you have a lot of experience as an Emacs package maintainer, do you have any thoughts on this (and also regarding the appropriate versions of Magit and Transient)?

[hokomo]

Thanks! Take your time, both. :-)

You can use the same function for both tasks, see magit-patch-apply for an example. In some cases that might not make sense or require unfortunate argument gymnastics.

I was wondering if I should use that for all of the commands. I haven't yet gone through to try and identify if and how much of argument gymnastics might be required, but I've been thinking about the non-transient interactive interface that each command provides. At first it seemed like we're effectively duplicating work and implementing a subset of the interactivity that the transient already gives us. But on a second thought, that kind of interface is sort of necessary, since we definitely want to be able to call the commands with the point on a patch in the StGit section, etc.

I'll give it some thought and check if there are any cases where stuffing all of it in a transient would just complicate things. In case it turns out to be too complicated, we might want to have the separation after all, in which case we need to think of some better names.

What comes after a -- separator is command-specific, but generally what comes after that separator are pathspecs, not patchnames (this is more true for StGit 2.0).

I see. I just realized I'm on StGit 1.4 since that's what my package manager has. Here's an example of what I see in the manpages:

stg show [-b] [-s] [-O] [--] [<patch1>] [<patch2>] [<patch3>..<patch4>]

stg sink [-t <target patch>] [-n] [--] [<patches>]

stg float [--] <patches>

stg delete [options] [--] <patch1> [<patch2>] [<patch3>..<patch4>]

Which in StGit 2.0 becomes:

stg show [OPTIONS] [patch-or-rev] [-- <path>]

stg sink [OPTIONS] [patch]

stg float [OPTIONS] [patch]

stg delete [OPTIONS] <patch>

I'll rework the code to drop the -- soon. How does StGit 2.0 handle "funny" patch names then (e.g. patch names that begin with --)? Are those cases even worth considering?

[hokomo]

Overall this looks really clean and on-target for the goals you stated--really well done and a great step forward!

Thanks! :-) I'll address your comments with some new commits soon.

I am unable to run a several commands due to the use of the undefined first function (see comment for details). I may have more comments once that issue is resolved.

Ah, that should be cl-first then, thanks!

Also, I am also still trying to figure out the best way to load/enable this extension in my doom-emacs setup. I can get it working with a combination of config-time and runtime hacks, but I don't yet have a clean/reproducible way to turn-on magit-stgit. I'm not yet able to tease apart what might be intrinsic to doom-emacs versus my doom configuation versus the magit-stgit extension itself. So this is something that I'll continue to try to get to the bottom of.

I'm assuming the hook mentioned in the file's commentary is not enough? That's the only thing I've had to add, and I don't think Spacemacs has any special functionality for StGit.

[hokomo]

@jpgrayson Since we plan on using StGit 2.0 as a base, but it hasn't even been released yet, do you think we should try and check (in a future PR perhaps) that the user is running version 2.0 and issue an appropriate error if not, until version 2.0 becomes the norm?

[jpgrayson]

I'll rework the code to drop the -- soon. How does StGit 2.0 handle "funny" patch names then (e.g. patch names that begin with --)? Are those cases even worth considering?

Thanks for bringing up the possibility of patch names starting with "-". I thought "surely such patch names are prohibited by StGit", but I was able to use stg rename -- -- to rename the topmost patch to "--". I'm going to repair StGit to explicitly disallow patch names starting with "-". So for magit-stgit's purposes, let's say that we don't need any special handling.

Also, I am also still trying to figure out the best way to load/enable this extension in my doom-emacs setup.

I'm assuming the hook mentioned in the file's commentary is not enough? That's the only thing I've had to add, and I don't think Spacemacs has any special functionality for StGit.

No, the (add-hook 'magit-mode-hook 'magit-stgit-mode) did not appear to be sufficient in my environment. It is also unclear to me whether adding stgit to magit.exensions is required and/or useful. But doom-emacs plays a lot of games at build/compile, startup, and runtime to be really fast, so it remains somewhat opaque to me if/when magit is loaded and if/when magit-stgit is loaded. I'm having similar difficulty trying to understand the circumstances under which local changes in my magit-stgit repo become live. I'll note that yet another complicating factor seems to be which functions and forms are decorated with autoload. I think there are a mix of "me" problems and magit-stgit problems.

On this topic, why is there a magit-stgit-mode minor mode? I note that neither forge or magit-annex provide a minor mode. Both define their top-level transient (forge-dispatch and magit-annex-dispatch) and insert/append a suffix to 'magit-dispatch that maps their dispatch transient. forge also hooks 'magit-status-sections-hook (but magit-annex does not). Should/could magit-stgit follow the same patterns as forge and magit-annex?

Since we plan on using StGit 2.0 as a base, but it hasn't even been released yet, do you think we should try and check (in a future PR perhaps) that the user is running version 2.0 and issue an appropriate error if not, until version 2.0 becomes the norm?

Yes, but a warning instead of an error. We most StGit commands, with the notable exceptions of mail and the new spill command, will mostly just work if the user has StGit 1.5.

[hokomo]

Thinking more about the keys, I'm not so sure if / is the best key binding for the dispatch transient. This might be annoying for Evil users (such as myself), since / is usually expected to be bound to evil-ex-search-forward. Otherwise the only option is to explicitly go through magit-dispatch every time (unless you're within the StGit Magit section in which case those key bindings will probably fire first).

Are there any other options we can use? I'll leave it on / for now in any case, but I'm just wondering if there's anything else available.

[hokomo]

On this topic, why is there a magit-stgit-mode minor mode? I note that neither forge or magit-annex provide a minor mode. Both define their top-level transient (forge-dispatch and magit-annex-dispatch) and insert/append a suffix to 'magit-dispatch that maps their dispatch transient. forge also hooks 'magit-status-sections-hook (but magit-annex does not). Should/could magit-stgit follow the same patterns as forge and magit-annex?

I suppose it's just to provide the ability to toggle the presence of the patch section. I have nothing against following the same pattern since I doubt you'd want to frequently enable/disable magit-stgit. forge seems to set up its section-inserting hooks once and for all, without a way to toggle it later, while magit-annex doesn't have any sections, as you mentioned. I'll remove the mode in a separate commit so we can easily revert if we happen to think of some use-case.

Speaking of top-level transients, I will rename magit-stgit's to magit-stgit-dispatch as well.

[hokomo]

I've now dropped all of the -- options (I've installed StGit 2.0 and double checked with the manpages to be sure). I've also made PATCHES optional for magit-stgit--sink.

@jpgrayson One thing that caught my eye is stg-float's manpage. It seems to suggest that patch is optional, but I don't think it is:

stg float [OPTIONS] [patch]...

That should most likely be <patch> instead. I've tried looking through the stgit repo to fix it myself, but I couldn't quickly identify where the manpage in question is.

[jpgrayson]

One thing that caught my eye is stg-float's manpage. It seems to suggest that patch is optional, but I don't think it is: ...

Good catch. The default usage is generated by clap based on the defined arguments. In the case of stg float, the patch name is optional iff the --series option is used; which is to say that stg float has two distinct usage modes. I've added a custom usage string that properly shows both modes:

stg-float 
Push patches to the top, even if applied

USAGE:
    stg float [OPTIONS] <patch>...
    stg float [OPTIONS] <-s|--series> <file>

N.B. it seems unlikely that we would want to go to the effort to support stg float --series in magit-stgit.

[jpgrayson]

I'm pretty happy with the latest set of commits in this PR. I'd like @tarsius' feedback too, of course, but from my perspective these changes are good foundation to keep building on.

Not sure what process we want to use for PR's and other changes to this repo. If you want to either merge this PR yourself or push your changes directly, that would be okay with me. If you're more comfortable with someone else gatekeeping changes, I can merge the PR. You're doing a big lift here, which I appreciate, so I'd like the process to be most helpful to you.

[kyleam]

Should/could magit-stgit follow the same patterns as forge and magit-annex?

Speaking just for magit-annex, if I were to write it today, I wouldn't add the key to magit-mode-map and instead would just document a suggested binding for users to add to their config. Related discussion: magit/magit#4709

Re minor mode: I haven't looked at the code in this PR, but, for what it's worth, I think that's a good (and idiomatic) way to let users toggle this package's changes to Magit's modes.

[hokomo]

No, the (add-hook 'magit-mode-hook 'magit-stgit-mode) did not appear to be sufficient in my environment. It is also unclear to me whether adding stgit to magit.exensions is required and/or useful.

I think the instructions must be pretty old, since I can't find magit.extensions mentioned anywhere in the current Magit manual. Googling for it turns up search results that have to do with Magit 1.4 for example.

I'm having similar difficulty trying to understand the circumstances under which local changes in my magit-stgit repo become live.

If I understood you correctly, I believe you have to manually refresh Magit's status buffer if you've made changes to the repo outside of Magit.

[hokomo]

Not sure what process we want to use for PR's and other changes to this repo. If you want to either merge this PR yourself or push your changes directly, that would be okay with me. If you're more comfortable with someone else gatekeeping changes, I can merge the PR. You're doing a big lift here, which I appreciate, so I'd like the process to be most helpful to you.

I don't mind merging or pushing directly myself. :-) The only question that I have is what sort of git workflow we prefer. Usually I like to rebase on top of master to keep the history nice and clean (I suppose this is what you meant by pushing directly). Any opinions on whether some of the commits in this PR should be squashed when the time comes?

Should/could magit-stgit follow the same patterns as forge and magit-annex?

Speaking just for magit-annex, if I were to write it today, I wouldn't add the key to magit-mode-map and instead would just document a suggested binding for users to add to their config. Related discussion: magit/magit#4709

That sounds good to me actually, since we can avoid making that tricky decision. I'm assuming we don't need to do the same for the series section key bindings since these are very localized anyway.

Re minor mode: I haven't looked at the code in this PR, but, for what it's worth, I think that's a good (and idiomatic) way to let users toggle this package's changes to Magit's modes.

Thanks for the input! After giving it a second thought, I think I'm leaning more toward keeping it around after all, because why not give the users the ability to toggle the functionality when it's not too much work anyway. :-)

[jpgrayson]

I'm having similar difficulty trying to understand the circumstances under which local changes in my magit-stgit repo become live.

If I understood you correctly, I believe you have to manually refresh Magit's status buffer if you've made changes to the repo outside of Magit.

No, I was talking about if/when/how modified magit-stgit.el code takes effect when I restart emacs. I've since figured out what I need to do in my doom-emacs config to ensure the latest code runs. Basically had to tell doom to exclude magit-stgit from its compilation system so that it is just interpreted. My emacs game is improving.

I don't mind merging or pushing directly myself. :-) The only question that I have is what sort of git workflow we prefer. Usually I like to rebase on top of master to keep the history nice and clean (I suppose this is what you meant by pushing directly).

I like a linear history as well. Rebasing is good.

Regarding pushing directly vs. going through a PR: I make a distinction between classes of changes that warrant review vs. those that don't. Since we've started a PR for these changes, we should finish the PR with a "Rebase and merge" when the time comes. I'm anticipating other changes you or I will want to make that do not warrant a PR workflow. I'm thinking of docstring changes or trivial/obvious repairs.

Any opinions on whether some of the commits in this PR should be squashed when the time comes?

I wouldn't squash these changes; it's a nice series.

Should/could magit-stgit follow the same patterns as forge and magit-annex?

Speaking just for magit-annex, if I were to write it today, I wouldn't add the key to magit-mode-map and instead would just document a suggested binding for users to add to their config. Related discussion: magit/magit#4709

That sounds good to me actually, since we can avoid making that tricky decision. I'm assuming we don't need to do the same for the series section key bindings since these are very localized anyway.

I concur. Also, I really appreciate @kyleam chiming in on this PR. I'm now using magit-imerge as my preferred reference.

[hokomo]

I like a linear history as well. Rebasing is good.

Regarding pushing directly vs. going through a PR: I make a distinction between classes of changes that warrant review vs. those that don't. Since we've started a PR for these changes, we should finish the PR with a "Rebase and merge" when the time comes. I'm anticipating other changes you or I will want to make that do not warrant a PR workflow. I'm thinking of docstring changes or trivial/obvious repairs.

Ah, I see what you mean, and I completely agree on both points (landing PRs by rebasing, and not opening PRs for trivial fixes).

I wouldn't squash these changes; it's a nice series.

Ok. :-)