stacked-git / magit-stgit

StGit extension for Magit
7 stars 7 forks source link

Better Magit integration #14

Open hokomo opened 1 year ago

hokomo commented 1 year ago

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 5df6566d 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 commented 1 year ago

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 5df6566d 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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

Great! :partying_face:

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 commented 1 year ago

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 commented 1 year ago

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. :roll_eyes:

jpgrayson commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

I've updated the Melpa recipe.