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

Better Magit integration #14

Open
hokomo opened this issue Sep 11, 2022 · 9 comments
Open

Better Magit integration #14

hokomo opened this issue Sep 11, 2022 · 9 comments

Comments

@hokomo
Copy link
Collaborator

hokomo commented Sep 11, 2022

Hello,

I recently started testing out StGit and naturally went to look for a package that offers integration with Magit, which is how I stumbled here. After fixing a few minor things (#13) the package loads fine and appears to work, although I haven't tested it extensively yet.

Some key bindings and popups are a bit off and need to be integrated better so I was thinking of giving it a shot. I also saw your message in 5df6566 and was wondering if you have any suggestions on other things should be fixed/modernized and perhaps some advice on the best way to go about it and what to look out for. Seeing that you contributed heavily to the package in the past, do you think this would be a thing worth doing?

@tarsius
Copy link
Collaborator

tarsius commented Sep 13, 2022

Seeing that you contributed heavily to the package in the past, do you think
this would be a thing worth doing?

I've just tried to reduce the bit rot. This package had a bit of a "thrown over
the wall" issue. It used to be part of Magit itself but I had no interest in
using or fully maintain it myself, so I removed it after a while. But I tried
to keep it in a working state for some time.

Some key bindings and popups are a bit off and need to be integrated
better so I was thinking of giving it a shot.

I wouldn't start by changing the actual keys used but I would convert from
magit-popup to transient. This should make things a bit more readable and
thus should also help when it comes to changing bindings--if after a while
of using it you still think that should be done.

Resources:

I also saw your message in 5df6566 and was wondering if you have any
suggestions on other things should be fixed/modernized and perhaps some advice
on the best way to go about it and what to look out for.

There are still two warnings:

In magit-stgit-read-patches:
magit-stgit.el:196:48: Warning: reference to free variable
    `magit-stgit-marked-patches'
magit-stgit.el:203:35: Warning: `magit-section-when' is an obsolete macro (as
    of Magit 2.90.0); instead use `magit-section-match' or
    `magit-section-value-if'.

I would probably replace the mapc with dolist or try to move away from
side-effects, e.g.:

(defun magit-stgit-patches-sorted (patches)
  "Return elements in PATCHES with the same partial order as the series."
  (mapcan (lambda (patch)
            (and (member patch patches)
                 (list patch)))
          (magit-stgit-lines "series" "--noprefix")))

Quote function as #'such.

Try to use "new" built-in functions instead of equivalents from dash.
Using dash is okay and I will keep doing so in many packages, but here
it is completely unnecessary.

@jpgrayson
Copy link
Collaborator

Maintainer of StGit here. I also just stumbled upon this repo as I was doing some preliminary research into whether and how magit could be extended with StGit capabilities. Despite being a doom-emacs user and occasional magit user for the past several years, I had no idea this extension existed!

Thank you to @tarsius for your in-depth comments and links to relevant resources. And also for your outstanding stewardship of magit and many other essentials of the emacs ecosystem!

I am also interested in helping get this package updated. My elisp skills are currently sub-rudimentary, but I am viewing this as a great opportunity to get better with that. I was able to successfully load magit-stgit and exercise it a little bit. My next step will be to study this module with an eye toward replacing magit-popup with transient, as @tarsius suggests.

@hokomo, thanks for reviving this extension. Happy to coordinate with you on this.

@hokomo
Copy link
Collaborator Author

hokomo commented Sep 14, 2022

@jpgrayson Hi! Great to see that the maintainer of StGit is interested in this extension. I'd be happy to coordinate! :-)

I've already started working on fixing up the rest of the byte-compiler warnings, modernizing some imperative code and converting magit-popup to transient. Could you give me a few days and I'll have a pull request ready that we can discuss? My focus is to first just modernize everything without breaking or adding any existing functionality.

Once magit-stgit is up to speed we can start adding missing functionality. I've only started using StGit recently so your input will be very valuable. :-) Some of the things I would like are e.g. a nice interactive way of quickly reordering patches in the stack (just like Magit's interactive rebase interface), a way to show the log without all of StGit's commits cluttering up the view, etc. I suppose we can discuss these things in separate issues once the time comes.

Do you perhaps have a rundown of the differences and similarities between stgit-mode that ships with StGit and magit-stgit?

@tarsius
Copy link
Collaborator

tarsius commented Sep 14, 2022

Great! 🥳

I've just given both of you commit access.

I think it would be best if we moved this repository to the stacked-git organization. I cannot do that directly because I am not a member, so I'll have to transfer to @jpgrayson and you can then transfer it to the organization. I can update the recipe and you can still ask me to do reviews after the move.

@jpgrayson
Copy link
Collaborator

I think it would be best if we moved this repository to the stacked-git organization.

I'm okay with having this in the stacked-git organization. I'll execute the transfer once I get the appropriate permissions from you, @tarsius.

@tarsius
Copy link
Collaborator

tarsius commented Sep 14, 2022

I've just created a transfer request. The repository is taking an additional detour through my user account, because if the source is an organization, then the only allowed destination is one's own user account. 🙄

@jpgrayson
Copy link
Collaborator

@hokomo

I've already started working on fixing up the rest of the byte-compiler warnings, modernizing some imperative code and converting magit-popup to transient. Could you give me a few days and I'll have a pull request ready that we can discuss?

Absolutely. That sounds like a fine approach.

[...] I suppose we can discuss these things in separate issues once the time comes.

Also sounds like a good approach. Each of the things you've mentioned could warrant a separate discussion.

Do you perhaps have a rundown of the differences and similarities between stgit-mode that ships with StGit and magit-stgit?

I'm not really an expert on stgit.el either. I've only done a little bit of toying around with it. I think the goals and capabilities are similar, but stgit.el does not use the transient command UI paradigm nor have any integration with magit. I don't know of any special features that wouldn't naturally be covered by a complete magit-stgit extension. My sense is that a StGit extension that fits into the magit ecosystem is going to be higher-value and easier to implement than the standalone strategy taken by stgit.el.

If we can get magit-stgit working well, I wonder if retiring stgit.el might be the right move? I note that stgit.el has been mostly unmaintained since 2012, has significant bitrot, and it weighs in at about 3000 LoC, or about 4x magit-stgit.

@jpgrayson
Copy link
Collaborator

I've just created a transfer request.

I think we're all set. I accepted the transfer and then transferred to the stacked-git organization.

https://github.com/stacked-git/magit-stgit

Thank you again, @tarsius!

@tarsius
Copy link
Collaborator

tarsius commented Sep 14, 2022

I've updated the Melpa recipe.

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

No branches or pull requests

3 participants