Integrate git-commit-mode into Magit #612

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

artagnon commented Apr 2, 2013

There have been previous failed attempts. This time, I rolled up my sleeves and finally did it for good.

[11/11] is most interesting, as it fixes the big problem: the magit header interfering with the regexp matcher in git-commit-mode.

As you can see from some of the other patches, there is plenty of opportunity to beat commit-mode.el into shape. So start hammering away!

Thanks.

Contributor

artagnon commented Apr 2, 2013

Ugh, sorry for sending this off in such a hurry: I forgot to (require 'magit) in commit-mode.el. I'll fix this up in the morning after setting up mocker.el and all.

Feel free to fix it up yourself though.

Contributor

artagnon commented Apr 6, 2013

Should be good to merge now.

artagnon added some commits Apr 2, 2013

@artagnon artagnon commit-mode.el: initial import of git-commit-mode
git-commit-mode [1] is a great mode for editing git commit messages,
but is unfortunately maintained poorly.  Import it into the magit tree
as commit-mode.el (similar to rebase-mode.el), so we can have tighter
magit integration and better maintenance.  Subsequent commits will
modify the file significantly and integrate it into
magit-log-edit-mode.

[1]: https://github.com/rafl/git-commit-mode

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
0323e16
@artagnon artagnon commit-mode.el: change git-commit-* to commit-mode-*
Change names of functions and variables in the file to reflect the
fact that it's now commit-mode.el, not git-commit.el.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
0e05459
@artagnon artagnon commit-mode.el: remove git-define-git-commit macro
As opposed to git-define-commit-mode-self, which just uses the current
user's name and email, this macro prompts for a name and email on the
minibuffer.  This is unnecessary, as the user is perfectly capable of
editing text after it is filled into the buffer.  Also migrate its
callers to use git-define-commit-mode-self.

While at it, give git-define-commit-self a more sensible name:
commit-mode-define-header.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
0eecc1d
@artagnon artagnon commit-mode.el: loop is now cl-loop, needs cl-lib
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
f8bbd03
@artagnon artagnon commit-mode.el: derive magit-log-edit-mode from commit-mode
This practically means that we get the face highlighting and
keybindings from commit-mode.  The first caveat is that "C-c C-s" and
"C-c C-a" are redefined by magit-log-edit-mode-map.  The other caveat
is that a Magit header breaks all the face highlighting.  We plan to
fix these problems in future patches.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
233d9dd
@artagnon artagnon commit-mode.el: remove commit-mode-commit
Since we assume that editing is happening in a server (like in
rebase-mode.el), we can replace the call to commit-mode-commit with a
simple server-edit.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
95e8cd1
@artagnon artagnon commit-mode.el: remove redundant function proxy
Strip the suffix from commit-mode-font-lock-keywords-1.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
3bc7e8b
@artagnon artagnon commit-mode.el: remove session.el integration
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
588fd12
@artagnon artagnon commit-mode.el: use define-derived-mode appropriately
commit-mode is defined using a defun, and does a lot of things by
hand.  Use define-derived-mode instead, and remove these hacks.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
6cd36e6
@artagnon artagnon magit.el: factor out a magit-log-header-re
We're doing this so we can match it in commit-mode.el in a future
patch.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
b282269
@artagnon artagnon commit-mode.el: handle the magit header properly
Match magit-log-header-re, and give it a preprocessor face.  Then, add
a matcher for the summary that doesn't assume that it begins at the
beginning-of-buffer.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
4a27884
Owner

tarsius commented May 11, 2013

Here is a link to the original version: https://github.com/rafl/git-commit-mode

Ultimately @sigma has to decide whether this should be bundled.

Also I think we should get @rafl approval before we considering being the borgs.

Is this only useful together with magit or could it also be used with vc-git?
Could this also work with other vcs (possibly after making some minor changes)?

As the maintainer of the Emacsmirror I occasionally encourage moving libraries that could be useful outside of the package the originated from into their own repositories. So I don't want to be involved in going in the other direction.

Maybe it would be a better option if someone (you?) took over maintaining the package instead of absorbing it? Does @rafl just not merge pull requests or does he not respond at all? If we cannot get any feedback from him we could put the fork in the magit organisation to give it more exposure. But let us first hear him out.

Contributor

artagnon commented May 11, 2013

There are plenty of forks of git-commit-mode: https://github.com/lunaryorn/git-modes/blob/master/git-commit-mode.el (this one claims to be compatible with magit, for instance)

I put the file in this repository for three reasons:

  1. I think it can benefit from being part of a large project like magit. It will certainly have more users and developer eyes on it.
  2. I want tight integration with magit. I've only done some initial work; there's really a lot of scope for future work.
  3. It's not a big deal. It's quite straightforward to merge in changes made at rafl/git-commit-mode (if that were to happen).

Yes, it is potentially useful outside magit. However, let's not discuss hypotheticals: how many people are contributing to rafl/git-commit-mode now? (and what does that tell us?)

Let's try to work towards a speedy conclusion + merge. I really like this feature, and I think our users should have it too.

Owner

tarsius commented May 11, 2013

I just don't want to step on @rafl s tows. So again: have you asked him? If he does not reply: fine, but asking does not hurt.

And what about making it a repository of the magit organisation for now instead of merging into magit/magit?

If rafl is okay with us taking over maintainership (and is willing to do a tiny bit of work) and we don't merge straight into magit/magit there would be this benefit:

rafl could transfer the repository to the magit organisation. All forks of rafl/git-commit-mode would the become forks of magit/{git-,magit-,}commit-mode.

how many people are contributing to rafl/git-commit-mode now? (and what does that tell us?)

Few, and that tells us exactly nothing. Might be because nobody has any interest in doing so. Or potential contributors are discouraged because they see that old request have not been merged yet. Since there are plenty of forks I think the latter is more likely.

Also why remove the git- prefix (which would be a good move if we were to make it vcs-agnostic) instead of changing it to magit- or you know keep the existing prefix? Let's not create facts :-)

Contributor

artagnon commented May 12, 2013

Um, I'm not entirely sure I understand the problem.

  1. What should we ask rafl about? What toes are we stepping on? Shouldn't he be more than happy that his code is being put to good use? Isn't that the point of open source? I certainly don't get people emailing me everytime they use some code I authored; do you? I've preserved the copyright and clearly mentioned where the code comes from in my first patch's commit message, so we aren't doing anything even mildly discourteous.
  2. Create a separate repository for it if you like. There's nothing to transfer: just hit fork on rafl/git-commit-mode and apply my patches to that repository. People will automatically use the best maintained fork, and developers will send patches. There is no need to hypothesize about why a particular fork isn't getting developer attention.
  3. What I'm against is breaking user experience by asking users to clone five different repositories to use magit (rebase-mode.el can easily be a separate repository as well, by extension). Keep commit-mode.el and rebase-mode.el in magit.git; like I said earlier, it's trivial to merge changes from a dedicated git-commit-mode.git repository.
  4. I didn't put a magit- prefix, because magit-commit-mode already exists. I removed the git- prefix it in the same spirit as rebase-mode.el. No issues if you want to keep the git- prefix, or put another prefix; that's a trivial change.

The bottomline is: I just want our users to have this feature. Now. The work has been done, and merging it is one click away. I don't see the point in delaying it because of your desire to go around collecting forks and blessing one "canonical repository".

Owner

tarsius commented May 12, 2013

All I was trying to do was to have the facts written out so that sigma can come and make a decision without having to do any research. Maintaining magit is a lot of work and I tried making it easier for him. There is a social side to software - obviously I am not very good at it, but I tried. From here it is probably going downhill.

  1. What I don't understand is why you implicitly refuse to talk to rafl (or refuse to mention that you already did). You are probably right that we have no obligation to do so.

    Your pull request could be paraphrased as: "Here is some useful code. I want you to take it from it's author and you then maintain it. I will do the initial work and might or might not help in the future."

    Now you argue that nothing is being taken away which is quite reasonable. The issue is that I feel differently about it. I might be the one who is wrong here but does it really matter? I kindly asked you to do so, why don't you just do it?

    @sigma tell me if I am overstepping here.

  2. "People will automatically use the best maintained fork." And that will require that they do some research. If the best option is at the root of the "network tree" (as visible from an fork on github), that helps keeping the required research to a minimum.

  3. Yes, there are issues related to installation. I will try to improve the installation instructions. There is one more issue that I want to address first, as it affects what I am going to write.

  4. That's reasonable.

Let's try to work towards a speedy conclusion + merge. I really like this feature, and I think our users should have it too.

You really got a point here. I for one haven't been using this mode so far. If it were part of magit I would have.

My intention when I first commented on this pull request actually was to speed it up. I saw that sigma hasn't even commented yet, and since it is really his call to make, I though having more information available to him might help.

Member

sigma commented May 12, 2013

(first of all, sorry for not being so active at the moment, I'm kinda swamped...)

@tarsius mostly I feel the same as you do. I would like it a lot more if we could get the original author's take on the subject. If he doesn't really care about that project anymore, then fine, but that should be only the backup plan.

One of the reasons for that is obviously cost of maintenance. Let's face it, magit has become a huge beast, and its evolution is quite slow due to that (architectural changes are basically stalled). Generally speaking we need to rely on more external packages, not less.

From a high-level point of view, there are very few things that are magit specific in that code (and that is a good thing because there's no reason to prevent for example vc-git, or egg users to use it). So I feel in the current state it doesn't really belong in magit: it's not fair to ask a vc user to instal magit just to get that code.
The main thing to remember here is that by developing magit we're not creating a standalone tool: we're building blocks for the emacs platform and need to make sure we don't fragment that platform. This means we're willing to pay a slightly higher integration cost, if that means less development cost for other block builders.

I think the solution I'd like best is: we patch git-commit-mode (that the original author, or the magit organization, or whoever maintains, in a separate repo, and more importantly, as a separate package) so that it has the right hooks to achieve magit integration, but keep the actual integration code separated (and then magit contrib would be the ideal place). Bottom-line, that would mean getting rid of any "magit" reference there, and have a second file called "magit-git-commit-mode.el" that would contain exactly that delta.

Alternate solutions are: this contains magit code, but is still maintained separately. Bit of a shame, but as long as it doesn't break for other users...
Finally, we can indeed fork it entirely and make it magit-specific (because in that case we wouldn't validate it with anything else). But then I'd argue we need to make our mind, and merge it with the existing magit-commit-mode. If we decide that it's superior, why make it a high-level choice instead of providing the relevant set of custom options that would allow people to personalize it to their liking ?
I'd really really really like to minimize the set of independent code paths that one user can follow during a regular workflow.
For that reason, even if we go with one of the first 2 scenarios, I still think we should revisit the reasons why there should be 2 commit modes for magit. If we could get rid of the code currently in magit core, I'd be really happy :)

Independently of all this, commit-mode.el is a bad name and should be changed: there's no reason to "steal" that name for something that definitely doesn't deserve it. That code is git-specific, the file name should contain git (and if it's magit-specific it should contain magit)
Don't get me wrong, I'd love to see such a generic commit-mode project, with backends for the various version control systems. But that's not what it is right now, and I don't think we're the right persons to do it (as the magit org I mean).
By the way, someone mentioned that rebase-mode could be maintained separately. Yep, and I think it should, as it has no dependency to magit :)

My, that was a long comment, I hope the reasoning is not too chaotic...

Contributor

artagnon commented May 12, 2013

@tarsius Hey, don't get me wrong: I have nothing against talking to rafl. I've even emailed him before (and initiated the previous effort which went nowhere). You've already mentioned him several times in this discussion thread (it's fine if he pops up and says something): do you want me to specially send him more email? I just wanted to point out that his response is not strictly necessary to proceed. Your thoughts on the subject are very much appreciated: we're both trying to work towards getting our users this feature.

Your pull request could be paraphrased as: "Here is some useful code. I want you to take it from it's author and you then maintain it. I will do the initial work and might or might not help in the future."

Yes, that is exactly correct. This code will potentially become a maintenance burden, and that is a valid issue. However, we've already determined that the code is not getting developer attention, and I'm not making the situation any worse. If anything, the situation will improve and more users/ developers will use/ work on the code.

@sigma Yeah, I agree that magit has become much too bloated for its own good.

I think the solution I'd like best is: we patch git-commit-mode (that the original author, or the magit organization, or whoever maintains, in a separate repo, and more importantly, as a separate package) so that it has the right hooks to achieve magit integration, but keep the actual integration code separated (and then magit contrib would be the ideal place). Bottom-line, that would mean getting rid of any "magit" reference there, and have a second file called "magit-git-commit-mode.el" that would contain exactly that delta.

It's not worth the pain at all. Not one bit. Have you seen the last commit in the series: "commit-mode.el: handle the magit header properly "? Tight integration makes the code simpler to write and understand. More importantly, if the person isn't using commit-mode.el with magit, the magit references aren't going to hurt one bit.

I'd really really really like to minimize the set of independent code paths that one user can follow during a regular workflow.
For that reason, even if we go with one of the first 2 scenarios, I still think we should revisit the reasons why there should be 2 commit modes for magit. If we could get rid of the code currently in magit core, I'd be really happy :)

Exactly. As I stated many times earlier, my plan was to shave off parts of magit-commit-mode and gradually meld this into magit-core. Asking for that to be done now will only result in a much larger patch series and much larger discussion (which will eventually end up being stalled).

Alternate solutions are:

Okay, I have another solution. Let us maintain exactly one other repository called magit-contrib, and move {rebase-mode.el -> git-rebase-mode.el} and {commit-mode.el -> git-commit-mode.el} into it. We can probably pick up a couple of other modes from lunaryorn/git-modes at a later point. Mention clearly that magit-contrib can be maintained and used independently of magit (although majority of the users/ developers are likely to be magit people themselves). I recommend a regular subtree-merge into magit.git: that's how gitweb.git and git-gui.git are maintained in git.git. [1]

No project is more important than its users. Please, let's just make a decision and go forward. We all want a solution that:

  1. Is least likely to get stalled, and most likely to get completed now. I'm willing to do a reasonable amount of work to ensure that this happens; just don't pick something insane like "let's refactor the whole magit codebase now!"
  2. Will not break user experience. This means: don't pick a solution that involves the user cloning five different repositories.
  3. Will not be a maintenance burden.

Having the various git modes as a separate repository that's integrated using a subtree-merge is the perfect compromise, in my opinion.

[1] Just snap your fingers and I'll have the histories ready in ten minutes.

Member

sigma commented May 12, 2013

It's not worth the pain at all. Not one bit. Have you seen the last commit in the series: "commit-mode.el: handle the magit header properly "? Tight integration makes the code simpler to write and understand. More importantly, if the person isn't using commit-mode.el with magit, the magit references aren't going to hurt one bit.

The "pain" doesn't bother me, as I don't think it's real. The emacs way
is to have hooks and defcustom everywhere, so tight integration is
really like an anti-goal here :)

Also, I really don't want to forget the people that don't use magit in
the process: if I'm not using magit, yet still have a use for a git
commit-mode, I shouldn't be forced to install, let alone load, magit.
In that sense, (require 'magit) is already hurting them.
That's really a matter of defining boundaries for various projects that
happen to have an intersection.

Exactly. As I stated many times earlier, my plan was to shave off parts of magit-commit-mode` and gradually meld this into magit-core. Asking for that to be done now will only result in a much larger patch series and much larger discussion (which will eventually end up being stalled).

I really want to stress that it's not an ideal scenario for me. That's
why I want to discuss now, before we take that road.
Ideally, the outcome of using this commit mode would to replace
all the current code in magit core with an external dependency.

Okay, I have another solution. Let us maintain exactly one other repository called magit-contrib, and move {rebase-mode.el -> git-rebase-mode.el} and {commit-mode.el -> git-commit-mode.el} into it. We can probably pick up a couple of other modes from lunaryorn/git-modes at a later point. Mention clearly that magit-contrib can be maintained and used independently of magit (although majority of the users/ developers are likely to be magit people themselves). I recommend a regular subtree-merge into magit.git: that's how gitweb.git and git-gui.git are maintained in git.git. [1]

I don't see how it would help. The rules are already quite relaxed for
contrib/, and it's not part of the final package.
My point is that even a magit-contrib repo still wouldn't be the right
place for what is not (or should not) be strongly tied to magit.
The comparison to gitweb or git-gui is not fair: those tools definitely
depend on git; a git-commit-mode has value outside of magit (magit can
depend on it though).

No project is more important than its users. Please, let's just make a decision and go forward. We all want a solution that:

  1. Is least likely to get stalled, and most likely to get completed now. I'm willing to do a reasonable amount of work to ensure that this happens; just don't pick something insane like "let's refactor the whole magit codebase now!"

Far from me to ask for that. Quite the opposite actually.
If I was to do it I would:

  • clone git-commit-mode into the magit org
  • fix what needs to be fixed in the original code, write a tiny magit-git-commit-mode.el integration there
  • submit the patches to the original code "upstream"
    • potentially create a new upstream in the process
    • once we have a new canonical location for git-commit-mode.el, move
      the integration bits to magit/contrib, or even to magit core to
      replace the current magit-commit-mode
  • from there we can operate with new package dependencies if we want to
  1. Will not break user experience. This means: don't pick a solution that involves the user cloning five different repositories.

The way of using magit is to M-x package-install it. Cloning
repositories and building them is a developer thing, not a user
one. Like it or not, that's where the emacs community is going, and
I personally think it's a good thing.
So, I'm not worried about those aspects, it won't be more difficult
for magit than it is for the rest of emacs packages (not saying it's not
difficult, see how we need to sort out the cl-lib story before we can
release master)

  1. Will not be a maintenance burden.

top priority as far as I'm concerned ;)

Contributor

artagnon commented May 12, 2013

Up to you entirely. I'm not the maintainer, and I really don't care about how it's done as long as it's done. So, hit "fork" and create that magit/git-commit-mode repository. Now. I'll get the patches ready; let's finish this.

Contributor

artagnon commented May 12, 2013

The comparison to gitweb or git-gui is not fair: those tools definitely
depend on git; a git-commit-mode has value outside of magit (magit can
depend on it though).

For the record: gitweb.git and git-gui.git do not depend on git.git at all. They just operate on git repositories: and there are various implementations of git that can be used to create/ access a git repository. It's just that git.git is an overwhelmingly popular way to operate on git repositories. (Analogy: magit.git is an overwhelmingly popular way to access git repositories via Emacs; atleast I'd like to think that ;)

The way of using magit is to M-x package-install it. Cloning
repositories and building them is a developer thing, not a user
one. Like it or not, that's where the emacs community is going, and
I personally think it's a good thing.

I personally never use package-install, because I need history to tinker with these scripts. And I want bleeding edge stuff. If most people use package-install (and it recursively installs dependencies), we don't have a problem: just document the process for developers in magit's README.md.

If I was to do it I would:

I'm still waiting for that magit/git-commit-mode repository to come up. Don't be surprised if we end up with lots of tiny repositories like magit/git-ignore-mode containing one file each (as opposed to one magit-extras.git repository).

Member

sigma commented May 12, 2013

alright, so I had a look around so figure out this mess. I still have one question: since @lunaryorn seems to be maintaining his fork, and that (unless I'm mistaken) it's the one that's distributed on marmalade anyway (which is where magit is also distributed), did you try and see whether your patches can live there (or if some of them are redundant)? If we can centralize that logic there, I'm more than happy to put a link to lunaryorn/git-modes so that magit users can find them.

And if @lunaryorn is willing to maintain them in the future and we're convinced it's strictly better than the builtin magit-commit-mode, I'd be more than happy to make it a dependency of magit and get rid of the corresponding code in magit core.
How does that sound ?

Contributor

artagnon commented May 12, 2013

Fine, forget this. I'll port my patches to lunaryorn/git-modes and use it.

artagnon closed this May 12, 2013

Member

sigma commented May 12, 2013

great, I forked lunaryorn/git-modes so that patches from magit developers (if any) can have a home.

Contributor

artagnon commented May 12, 2013

Wow, @lunaryorn has worked on git-commit-mode.el extensively: 105 patches at this point. My series is redundant.

artagnon deleted the unknown repository branch May 12, 2013

Contributor

lunaryorn commented May 12, 2013

Thank you for inviting me into this discussion.

I maintain my own fork of git-commit-mode, after its original maintainer didn't react on Github. My fork is heavily refactored and cleaned up, to get the code in good shape, to remove obscure and unused features, and to add some new features. I do not add new features now, simply because I don't have any idea of what could be missing, but I accept patches and pull requests, and fix occasional issues reported by users.

I do not know, whether Magit should depend on git-commit-mode. I for my part see no immediate need for this. The mode already replaces the standard Magit commit mode, and this works perfectly (I'm using it myself every day).

Also note, that this mode enforces a certain style upon commit messages, namely the guidelines of @tpope. When depending on my Git commit mode, Magit would effectively enforce these guidelines, too. This may or may not be what you want. It may upset people that dislike these guidelines.

If you decide to depend on Git commit mode, I'd like to see this mode move into the Magit organisation, along with the other Git-related modes I maintain. That'd make sure that the mode continues to be maintained, even if I can't work on it anymore. I have no plans on stopping maintenance now, but you never know… :)

Member

sigma commented May 12, 2013

@lunaryorn That's a very valid point, enforcing some guidelines is certainly a good default, but we need to keep the ability to tweak/ignore them.
Thanks for maintaining this anyway, I'll try to start using it and might come back to you with feature requests :)

tarsius added this to the 2.1.0 milestone Feb 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment