Add Co-Author support to new commits

[2KAbhishek]

• Adds Co-Author support to main commit panel, (<c-e> by default)
• Cleans up and amend commit attribute menu related code

• [x] Cheatsheets are up-to-date (run go generate ./...)
• [x] Code has been formatted (see here)
• [x] Tests have been added/updated (see here for the integration test guide)
• [x] Text is internationalised (see here)
• [x] Docs (specifically docs/Config.md) have been updated if necessary
• [x] You've read through your own file changes for silly mistakes etc

[2KAbhishek]

@jesseduffield would love to get this out in the next release, can you please review it or point me to maintainers with whom I can work with. Thank you

[2KAbhishek]

@stefanhaller can you take a look at this please, will appreciate it

[stefanhaller]

I only skimmed the commits so far (just back from vacation). One general feeling is that you have too many commits, with a bit of back-and-forth that makes the branch hard to review. (I might be wrong about that, in which case I'll be happy to have a closer look again.) Also, some commits don't have succeeding tests, which is bad for bisecting; would be good to squash the fixes to tests into the commits where they are needed.

As for the feature itself: I see how it's preferable to have this available while typing the message (as opposed to having to issue a command afterwards to do it), but the chosen keyboard shortcut is not a very good one, as <c-e> is the emacs binding for moving to the end of the line. Also, I'm not sure we want to go into the direction of adding more and more keybindings for doing special things in the commits panel; maybe we should have a menu instead?

All in all I don't feel comfortable approving this, I'd like @jesseduffield to have a general look at the direction first, I'll then be happy to do a more in-depth code review.

[2KAbhishek]

Thanks a lot for the feedback @stefanhaller!

• I will try to structure the commits better
• Yes, I do agree that <c-e> is not the most suitable option but I couldn't find any other good alternatives (would love suggestions), I too wanted to introduce a menu option initially but I wasn't sure if that would be a correct choice and how to add it (this is my first contribution to a go codebase 😄)

Would love to get inputs from Jesse on this

[2KAbhishek]

Hi @stefanhaller I have restructured the commits, can you please take a look 🙏🏽

[jesseduffield]

Thanks for raising this PR @2KAbhishek :)

I've talked to Stefan about this offline and agree that we should use a menu here for a couple reasons:

• less chance of a collision with a user's keybinding for something else
• less clutter in the subtitle of the commit description view

[2KAbhishek]

Thanks for the update @jesseduffield I will implement a menu for this!

[stefanhaller]

A few thoughts about adding a menu:

• It looks like we might initially have a menu with only a single command (add co-author), plus a cancel entry. While that's a bit strange, I think it's fine to start with, we can add more commands later.
• I'm torn about moving the existing <c-o> (switch to editor) command into the menu as well. I use it very often, and it would be an extra key press, which would be slightly annoying. On the other hand, it would allow us to use the <c-o> key binding for the menu ("o" for "options?").
  • Also, I'm still working on the auto-wrapping functionality for the commit message panel (see here), and once we have that, I'm hoping that I won't need the "switch to editor" command nearly as often, so it's probably fine to move it into the menu.
• We should pay attention to the consistency of the menu entries. The ones we are talking about here are commands, but in the past we were also considering adding a menu for options that you can toggle (e.g. --no-verify to skip git hooks). Those would appear as a checkbox in the menu, and we'll have to see if it's ok to have a mixture of commands and toggle-able options in the same menu. Nothing to act on now, just a consideration for the future.

[2KAbhishek]

I like the idea of moving the editor command to this menu as well, that will let us do <c-o> e to open the editor, <c-o> c to add co authors, will go ahead and implement it.

This will also cleanup the description help message with just one option open menu

[stefanhaller]

Sounds good.

Did you actually test the version that you pushed? It immediately crashes for me when I choose "add co-author" from the menu.

[2KAbhishek]

I have fixed the co author issue @stefanhaller @jesseduffield

Here's how the panel and menu look now

Commit panel

New commit menu

LMK what you think. would love to get this out with the next release 🚀 🤞🏽

[2KAbhishek]

@jhrv @stefanhaller @jesseduffield hi guys, just checking in if there's any feedback on the PR

[stefanhaller]

Yes, I definitely still plan to review it. I didn't get around to it yet, sorry for the delay. I'm also quite busy this week, so I probably won't get around to it before next week.

[stefanhaller]

OK, I spent some time with this now. The first thing I did is make it reviewable. I found it really hard to review the branch, with all the commits that change code one way and then a later commit changes the same code again some other way; plus, with master being merged into the branch multiple times, this made it too hard to see what's happening. I could of course have simply reviewed the "All file changes" tab (like Jesse usually does), but I don't like that.

So instead of explaining what I think a good commit history should look like, I just went ahead and did it in a way that I think makes more sense, and force pushed that; hope you don't mind. I didn't change your code (as you can verify by diffing 9ce18759a (your last version) against bf4839dc4 (my version); the diff is empty.

From here I'll go ahead and rebase onto master, and add a few fixup commits, and then force-push again.

[2KAbhishek]

Thanks a lot for the in depth review and feedback @stefanhaller, I'm gonna work on resolving the comments

Some pointers on integration test you mentioned will be really helpful 🙏🏽

[stefanhaller]

Some pointers on integration test you mentioned will be really helpful 🙏🏽

We already have a test called add_co_author.go, but that one is for the after-the-fact amend menu. So we need a new one with a different name (it could go in the same folder though), maybe add_co_author_in_message_panel.go or something like that. It would simply create an untracked file, stage it, press keys.Files.CommitChanges, and then invoke the menu and the "add co-author" command, and then assert on the resulting description. Look around in the other tests in this folder for how to do these things.

[stefanhaller]

@2KAbhishek Do you intend to keep working on this? It looks like there's not much missing to get it over the finish line. Just let me know if you either don't have time or lost interest, I can take care of it in that case.

And please, don't merge master into feature branches! Rebase on master instead. Thanks.

[2KAbhishek]

@stefanhaller thank you for the message, I will wrap the integration tests within this week. Will drop a message once done from my end

[2KAbhishek]

@stefanhaller I have addressed the comments you had left and things look good now, I was unable to add an integration test for this, would really appreciate if you can add it and help me get this out.

Thank you 🙏🏽

[2KAbhishek]

Hi @stefanhaller, just checking in, is there anything I can do to help move this.

[stefanhaller]

Hi @stefanhaller, just checking in, is there anything I can do to help move this.

Sorry, I kind of lost track of this. Thanks for pinging, I hope to pick it up again on the weekend. I already resolved the conflicts locally, so don't bother with that.

But for me it also kind of depends on #3173, because as long as that one isn't merged, the "switch to editor" action is such a commonly needed feature that I don't want to move it to a menu where it requires two key presses instead of one.

[stefanhaller]

So, I extracted a function AddCoAuthorToDescription that takes care of adding zero, one, or two line feeds as necessary (167bbd3fa9fb), to be reused by both "add co-author" commands; and added an integration test.

[stefanhaller]

@2KAbhishek The PR description no longer matches the behavior now (especially the screenshot). Could you update it please to describe the new behavior? I think this is important because we link to PRs from the release notes, so people might be confused reading it.

[2KAbhishek]

Thanks for the heads up @stefanhaller, I have updated the PR description!