A full editor can be used as git commit message editor #95266

JohnnyCrazy · 2020-04-14T20:13:06Z

This PR fixes #30562

This PR introduces the ability to use a full editor pane to write a commit message.

XidmaU9T4x.mp4

Settings

`git.useEditorToCommit: boolean`

If enabled, a full editor will be created for every commit action. These include triggered git commands like Git: Commit All and also CTRL+ENTER on an empty input box in the repository pane. If the input box is not empty, the full editor will not be used. This still allows for quick, small commit messages.

Once the editor is created, it behaves like GIT_EDITOR="code --wait" git commit: As soon as the file is closed, the contents will be used as message and the commit continues. If it's empty, the commit aborts and a Aborting commit due to empty commit message warning is shown.

`git.verboseCommit: boolean`

If enabled, the opened COMMIT_EDITMSG will contain more output. It's basically appending --verbose to the git call.

If git.verboseCommit is enabled but git.useEditorToCommit is not, a warning will be shown as the setting is pointless.

Implementation

The implementation of the git commit integration is similar to the askpass one:

We use the already existing IPC Server of the git extension
Once a git commit is triggered, a custom .sh/.bat file is passed as the GIT_EDITOR environment variable to the git commit call
This file starts a JS Process (no electron overhead) and passes all its arguments (Which is just the path to the COMMIT_EDITMSG) via IPC to the main electron process and waits for its answer.
The main electron process opens the file and waits until its closed. If its closed, a dummy response is sent to the client, which terminates itself and thus, finishes the git commit process.

vscode: git commit waits for git: commit waits for ipc-client-js: GIT_EDITOR waits for vscode: close of file

Testing

Testing was successful on windows and linux. I wanted to test the WSL Integration as well, but was unable to get it working with the downloaded VSIX in Code-OSS.

Initially I had a problem with the timeout of the IPC. This was solved by removing the timeout. This will also be the default in node 13

JohnnyCrazy · 2020-04-14T20:14:18Z

extensions/git/src/commands.ts

+				window.showInformationMessage(
+					localize('useless verbose commit', "Verbose committing has no effect. The setting 'git.useEditorToCommit' is not enabled.")
+				);


I was not sure whether to include this warning or not. Up to you guys :)

JohnnyCrazy · 2020-04-14T20:16:41Z

extensions/git/src/git.ts

+		const args = ['commit', '--quiet'];
+		const options: SpawnOptions = {};
+
+		if (!opts.useEditor || message) {


If a message is supplied, it should still be used for the commit. This allows to use the input box for quick commits, although git.useEditorToCommit is enabled.

JohnnyCrazy · 2020-04-14T20:18:40Z

extensions/git/src/gitEditor.ts

+			// We don't want to remember the cursor position.
+			// One should be able to start writing the message immediately.
+			const position = new Position(0, 0);
+			editor.selection = new Selection(position, position);


vscode seems to remember where my cursor was in an older file. This was quite annoying since I couldn't start writing the commit message immediately.

This is probably because the path to the git-commit is always .git/COMMIT_EDITMSG or something. If you change how vscode calls git commit, you can make this a randomly named temp file. The downside is that you will lose the automated report that git commit gives you (especially when using --verbose) but then again, you have a whole IDE that can provide this information so maybe that's a good future path.

This is probably because the path to the git-commit is always .git/COMMIT_EDITMSG or something. If you change how vscode calls git commit, you can make this a randomly named temp file.

does not using .git/COMMIT_EDITMSG mean no proper integration with prepare-commit-msg hook? it would be nice to preserve this

Yes, that's why I refrained from using a custom/tmp file. I think the overall experience is still the best when the original file is used.

JohnnyCrazy · 2020-04-14T20:19:58Z

extensions/git/src/gitEditor.ts

+			return new Promise((c, e) => {
+				const onDidChange = window.onDidChangeVisibleTextEditors((editors) => {
+					if (editors.indexOf(editor) < 0) {
+						onDidChange.dispose();
+						return c(true);
+					}
+				});
+			});


I'm not 100% sure about this part. Could be, that I missed some edge cases where this fails?

JohnnyCrazy · 2020-04-14T20:35:08Z

This is also particularly interesting for #2718 and #93150.

JohnnyCrazy · 2020-04-15T11:16:12Z

One test in windows is failing, seems to be not connected to my changes tho

joshk0 · 2020-05-06T12:08:33Z

@JohnnyCrazy How can I help with this PR? I see that there are some merge conflicts. I could try to help you clean that up with a rebased version or a merge commit. Thanks for doing this!

JohnnyCrazy · 2020-05-06T15:16:39Z

@JohnnyCrazy How can I help with this PR? I see that there are some merge conflicts. I could try to help you clean that up with a rebased version or a merge commit. Thanks for doing this!

Thanks for the ping, missed that there are new changes and conflicts 👍

I will have a look today/tomorrow and will report, if I run into any issues. Most changes seem to be straightforward to fix.

joaomoreno · 2020-05-07T09:10:59Z

No worries about the conflicts @JohnnyCrazy, I can take care of them.

Assigning this to May.

joaomoreno · 2020-05-08T09:01:09Z

Fantastic work here.

But because git is an extension, the way we handle the "closing of the COMMIT_MESSAGE" file is problematic:

It's very easy for the user to simply opening another tab, thus cancelling (or worse, accepting) the commit operation
In that situation, having an already opened COMMIT_MESSAGE, possibly dirty, will prevent further commits from being made

For that reason, I've brought these changes into joao/git-editor and will see if I can improve this using the FS Provider API.

Everything else works to perfection.

We might need to realign these settings and simply consider having this turned on by default.

JohnnyCrazy · 2020-05-08T09:23:32Z

Yes, I already had a weird feeling when I was writing that "on close" part 😉

Regarding the settings, one thing which held me back to make it the default action was the command trigger Git: Commit .... I was unsure if some people might find the input box better than a whole file and don't like the change in the process.

Anyway, if you need anything or want me to further explore solutions using the FS Provider API, feel free to ping me!

joaomoreno · 2020-05-08T09:35:11Z

Anyway, if you need anything or want me to further explore solutions using the FS Provider API, feel free to ping me!

Actually, yeah, if you'd like to give it a go, here's what I was thinking.

We can't really figure out when the file has been closed or not, this is a long standing open feature request into our API which won't get resolved any time soon. So we're going to have to go with the same semantics as you currently have. But in doing so, we need to use a different file on for each commit, so we avoid having potential save conflicts. Idea:

When we get pinged to open that COMMIT_MESSAGE file, instead of directly opening it we copy its contents into an in-memory virtual file, coming in from the git fs provider (or possibly a new fs provider, so we don't clash with the existing one). We can have different files by doing something like gitcommit:/<generated id>/COMMIT_MESSAGE, that way the file would still have colorization etc. And we can simply delete the file once the commit operation is done.

What do you think?

If you agree I can simply push the merge conflict resolution to your branch and you can pick it up from there?

JohnnyCrazy · 2020-05-08T09:56:28Z

We can't really figure out when the file has been closed or not, this is a long standing open feature request into our API which won't get resolved any time soon. So we're going to have to go with the same semantics as you currently have. But in doing so, we need to use a different file on for each commit, so we avoid having potential save conflicts. Idea:

When we get pinged to open that COMMIT_MESSAGE file, instead of directly opening it we copy its contents into an in-memory virtual file, coming in from the git fs provider (or possibly a new fs provider, so we don't clash with the existing one). We can have different files by doing something like gitcommit://COMMIT_MESSAGE, that way the file would still have colorization etc. And we can simply delete the file once the commit operation is done.

Sounds good, the only question/reassurement I currently have is regarding So we're going to have to go with the same semantics as you currently have:

So, as I understood, we will continue to use window.onDidChangeVisibleTextEditors, but not with the real file but with a in-memory mapped file provided by a FS Provider. And if for some reason the commit aborts/continues while the file is still open, we simply "delete" the in-memory mapped file and the user is unable to edit the file anymore, even if it is open in an editor. (it probably shows the (deleted) suffix).

If you agree I can simply push the merge conflict resolution to your branch and you can pick it up from there?

Sure!

joaomoreno · 2020-05-08T12:00:43Z

So, as I understood, we will continue to use window.onDidChangeVisibleTextEditors, but not with the real file but with a in-memory mapped file provided by a FS Provider. And if for some reason the commit aborts/continues while the file is still open, we simply "delete" the in-memory mapped file and the user is unable to edit the file anymore, even if it is open in an editor. (it probably shows the (deleted) suffix).

Absolutely right! It's definitely better than the current approach. And we can discuss with @jrieken and @bpasero if we can't have the notion of editors which should automatically close if their document gets deleted, at a later stage.

joaomoreno · 2020-05-08T12:06:34Z

@JohnnyCrazy Just resolved the conflicts. Looking forward to the further changes! 💪

joaomoreno · 2020-05-08T16:05:30Z

Open Editors API could be coming this month: #15178 (comment)! 🔥 At least in some proposed state, which we could use in git. Let's keep an eye on that, it would be very helpful here.

I wonder if, given that API, we should still do the FS Provider dance. 🤔 My gut feeling tells me we don't actually have to go with that hassle, especially because git can't handle multiple simultaneous commits. So I propose we wait for that proposed API to advance and experiment with that, what do you think @JohnnyCrazy?

JohnnyCrazy · 2020-05-08T18:06:50Z

Yes, I agree. In the end we would abuse a rather unrelated API and the code would get more complicated.

Looking forward to an Editor Tab API! 👍

JohnnyCrazy · 2020-06-30T08:44:35Z

As far as I understood #15178, it will take some time.

I would happily invest some more time finding a solution for the whole open editor problem, maybe involving the FS Provider API like you suggested. What is your take on this @joaomoreno ? I could imagine once the Editor API is out, the refactoring amount is rather minimal. But in the end you have more experience here, so up to you 👍

joaomoreno · 2020-06-30T09:34:35Z

#15178 is definitely in the horizon... we're just a bit overloaded and other stuff got prioritized.

We can definitely see if the FS Provider gives us a better experience here, so we're not blocked by that feature request. Let me know how it goes.

zpanderson · 2020-07-17T15:12:09Z

Just curious if this would get merged any time soon, we'd love to start using this in our process.

JohnnyCrazy · 2020-07-27T10:30:42Z

@joaomoreno I pushed a first implementation of a fs-provider based solution in f18ba9a

Some notes for that:

Dirty commit message files are not a problem anymore. We're handling all the contents in-memory and the real file is only touched 2 times: When starting the commit and when it finishes up. Using an uuid as the URI authority make it also unique.

When we get pinged to open that COMMIT_MESSAGE file, instead of directly opening it we copy its contents into an in-memory virtual file, coming in from the git fs provider (or possibly a new fs provider, so we don't clash with the existing one). We can have different files by doing something like gitcommit://COMMIT_MESSAGE

Right now, the URI (incl. uuid) for the file is generated when we receive the IPC call. Thus, we can only delete the in-memory file once the callback to onDidChangeVisibleTextEditors finishes. The ideal solution would be to delete the file when the commit operation fails/succeeds. This would require generating the URI/uuid already in the git.commit call and passing it to the GIT_EDITOR proxy script. I think the only way to do correctly do this is via a env variable. What do you think of that? This would also mean git.ts using the workspace.fs api.

Instead of directly comparing if the spawned editor is still open, I changed it to compare based on the URI. This means if you split/duplicate the opened COMMIT_MSG and close the original editor, the commit is still in progress and only finishes if no editor with the corresponding URI is visible.

The problem with opening a new tab of course still exists due to onDidChangeVisibleTextEditors. The more I think about it, the more I think this is a blocker. Imagine writing a commit message and in the process of it, you want to lookup something in the code. You hit save and open a new tab --> The commit goes through with a unfinished message. Not transparent and not clear to the user.

Khady · 2020-09-30T15:14:02Z

I don't know if you are aware of https://github.com/kahole/edamagit but it solves the problem tackled by this PR. Maybe it doesn't cover all the edge cases. But it is a very good solution in the meantime.

…ia_full_editor

lszomoru · 2022-05-25T10:14:29Z

@joaomoreno, this PR is now ready to be reviewed. Let me know if you have any questions or concerns. Thanks!

rmunn · 2022-05-25T10:37:28Z

@joaomoreno, this PR is not ready to be reviewed. Let me know if you have any questions or concerns. Thanks!

@lszomoru - Did you mean "not" ready? Or "now" ready?

lszomoru · 2022-05-25T11:59:43Z

@rmunn, thanks! Updated my comment. This is now ready to be reviewed.

extensions/git/src/gitEditor/gitEditor.ts

src/vs/workbench/contrib/scm/browser/util.ts

joaomoreno · 2022-05-25T18:32:18Z

This is awesome. Great job @lszomoru for getting this done! And many thanks to @JohnnyCrazy for being so patient with us and collaborating with us along the way! 🚀 👏

lszomoru · 2022-05-25T19:54:59Z

All the credit goes to @JohnnyCrazy 🚀

This reverts commit 97f8e66.

…" (#150487) This reverts commit 97f8e66.

JohnnyCrazy · 2022-06-06T12:42:10Z

Good stuff @lszomoru ! Really enjoyed working on this one, thanks for the guidance and help along the way @joaomoreno ! 😊

lszomoru · 2022-06-08T08:40:00Z

@JohnnyCrazy, after we have merged the PR we ran into some issues and we had to revert the changes. Unfortunately, we ran out of time before we had to shut down the main branch for the release, so these changes did not make it into the May release. I have create a new PR (#151491) to get these changes into the June release.

JohnnyCrazy commented Apr 14, 2020

View reviewed changes

chrmarti assigned joaomoreno Apr 15, 2020

joaomoreno added the git GIT issues label Apr 17, 2020

joaomoreno added this to the May 2020 milestone May 7, 2020

Khady mentioned this pull request May 23, 2020

Git: Support prepare-commit-msg hook #94274

Closed

joaomoreno modified the milestones: May 2020, Backlog Jun 16, 2020

stkb mentioned this pull request Jul 20, 2020

Rewrap in commit message edit box stkb/Rewrap#224

Closed

joaomoreno removed this from the Backlog milestone Oct 1, 2020

lszomoru added 8 commits May 23, 2022 15:07

refactor git commit editor with latest main

f3cb88c

Tweak notification + open document

7475efc

Disable commit action while commit in progress

1e23127

Refactor commit arguments

1f193d1

Remove file system provider

9651827

Merge remote-tracking branch 'origin/main' into t-jodell/git_commit_v…

b21d1b9

…ia_full_editor

Adopt scmInput enablement api

fa7cead

Remove files that were moved

264d5ed

lszomoru requested a review from joaomoreno May 25, 2022 10:13

lszomoru modified the milestones: Backlog, May 2022 May 25, 2022

joaomoreno reviewed May 25, 2022

View reviewed changes

extensions/git/src/gitEditor/gitEditor.ts Show resolved Hide resolved

src/vs/workbench/contrib/scm/browser/util.ts Show resolved Hide resolved

joaomoreno approved these changes May 25, 2022

View reviewed changes

lszomoru merged commit 97f8e66 into microsoft:main May 25, 2022

Tyriar mentioned this pull request May 26, 2022

GIT_EDITOR set without asking and throwing error when I try amend a commit #150482

Closed

lszomoru added a commit that referenced this pull request May 26, 2022

Revert "A full editor can be used as git commit message editor (#95266)"

ca90050

This reverts commit 97f8e66.

lszomoru added a commit that referenced this pull request May 26, 2022

Revert "A full editor can be used as git commit message editor (#95266)…

9bfd3c1

…" (#150487) This reverts commit 97f8e66.

ArturoDent mentioned this pull request Jun 4, 2022

PR adding git commit message editing in the editor has been reverted and should be deleted from Release Notes mention microsoft/vscode-docs#5385

Closed

lszomoru modified the milestones: May 2022, June 2022 Jun 8, 2022

lszomoru mentioned this pull request Jun 8, 2022

Git - use editor as commit message input #151491

Merged

JohnnyCrazy deleted the t-jodell/git_commit_via_full_editor branch June 17, 2022 09:33

github-actions bot locked and limited conversation to collaborators Jul 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A full editor can be used as git commit message editor #95266

A full editor can be used as git commit message editor #95266

JohnnyCrazy commented Apr 14, 2020 •

edited

Loading

JohnnyCrazy Apr 14, 2020

JohnnyCrazy Apr 14, 2020

JohnnyCrazy Apr 14, 2020

joshk0 May 6, 2020

Khady May 8, 2020 •

edited

Loading

JohnnyCrazy May 8, 2020

JohnnyCrazy Apr 14, 2020

JohnnyCrazy commented Apr 14, 2020

JohnnyCrazy commented Apr 15, 2020

joshk0 commented May 6, 2020

JohnnyCrazy commented May 6, 2020

joaomoreno commented May 7, 2020

joaomoreno commented May 8, 2020

JohnnyCrazy commented May 8, 2020

joaomoreno commented May 8, 2020 •

edited

Loading

JohnnyCrazy commented May 8, 2020 •

edited

Loading

joaomoreno commented May 8, 2020 •

edited

Loading

joaomoreno commented May 8, 2020

joaomoreno commented May 8, 2020

JohnnyCrazy commented May 8, 2020 •

edited

Loading

JohnnyCrazy commented Jun 30, 2020

joaomoreno commented Jun 30, 2020

zpanderson commented Jul 17, 2020

JohnnyCrazy commented Jul 27, 2020

Khady commented Sep 30, 2020

lszomoru commented May 25, 2022 •

edited

Loading

rmunn commented May 25, 2022

lszomoru commented May 25, 2022

joaomoreno commented May 25, 2022

lszomoru commented May 25, 2022

JohnnyCrazy commented Jun 6, 2022

lszomoru commented Jun 8, 2022

A full editor can be used as git commit message editor #95266

A full editor can be used as git commit message editor #95266

Conversation

JohnnyCrazy commented Apr 14, 2020 • edited Loading

Settings

git.useEditorToCommit: boolean

git.verboseCommit: boolean

Implementation

Testing

JohnnyCrazy Apr 14, 2020

Choose a reason for hiding this comment

JohnnyCrazy Apr 14, 2020

Choose a reason for hiding this comment

JohnnyCrazy Apr 14, 2020

Choose a reason for hiding this comment

joshk0 May 6, 2020

Choose a reason for hiding this comment

Khady May 8, 2020 • edited Loading

Choose a reason for hiding this comment

JohnnyCrazy May 8, 2020

Choose a reason for hiding this comment

JohnnyCrazy Apr 14, 2020

Choose a reason for hiding this comment

JohnnyCrazy commented Apr 14, 2020

JohnnyCrazy commented Apr 15, 2020

joshk0 commented May 6, 2020

JohnnyCrazy commented May 6, 2020

joaomoreno commented May 7, 2020

joaomoreno commented May 8, 2020

JohnnyCrazy commented May 8, 2020

joaomoreno commented May 8, 2020 • edited Loading

JohnnyCrazy commented May 8, 2020 • edited Loading

joaomoreno commented May 8, 2020 • edited Loading

joaomoreno commented May 8, 2020

joaomoreno commented May 8, 2020

JohnnyCrazy commented May 8, 2020 • edited Loading

JohnnyCrazy commented Jun 30, 2020

joaomoreno commented Jun 30, 2020

zpanderson commented Jul 17, 2020

JohnnyCrazy commented Jul 27, 2020

Khady commented Sep 30, 2020

lszomoru commented May 25, 2022 • edited Loading

rmunn commented May 25, 2022

lszomoru commented May 25, 2022

joaomoreno commented May 25, 2022

lszomoru commented May 25, 2022

JohnnyCrazy commented Jun 6, 2022

lszomoru commented Jun 8, 2022

JohnnyCrazy commented Apr 14, 2020 •

edited

Loading

`git.useEditorToCommit: boolean`

`git.verboseCommit: boolean`

Khady May 8, 2020 •

edited

Loading

joaomoreno commented May 8, 2020 •

edited

Loading

JohnnyCrazy commented May 8, 2020 •

edited

Loading

joaomoreno commented May 8, 2020 •

edited

Loading

JohnnyCrazy commented May 8, 2020 •

edited

Loading

lszomoru commented May 25, 2022 •

edited

Loading