Git Commit.Template, Restore Previous Commit Msg On Undo

[ghost]

Relates to issues: #7830 and #6190. I have put most of my notes and discussion in #7830.

To effect the changes, there are basically four parts:

1. The git server-side CLI commands and state.
2. The git client-server pipeline to expose the new behavior/state.
3. The git changes view which contains the input box.
4. The git undo last commit action.

1. The git server-side CLI commands and state.

The two git CLI commands we need to run for these changes include git config --get commit.template and git log -1 --format=%B. The first reads the commit.template entry if it exists, and the second gets the previous commit message. This is very simple, but the distributed architecture makes the implementation of this somewhat extensive.

Since the commit.template command is a read on the config, this was most easily executed by using the repo's existing run command: repo.run(['config', '--get', 'commit.template']). I could have also done this with the log I suppose, but it seemed a good fit for its own method on the Repository class.

Also, there is a design decision on how to capture the state involved in these operations. This stems from the existing design requiring a status on each git operation execution. I chose to combine the information in an ICommitInfo to keep it cleaner for our use case. I then added this to the IModel because, AFAICT, this is how state is passed in these operations.

It essentially works like the following: Operation -> Status -> Model.

When an operation is executed, whatever it is, it then executes an updated status, which creates an updated model that is expected on the client/browser.

2. The git client-server pipeline to expose the new behavior/state.

There are several abstraction layers when communicating between the client/server with this being an electron application. I had a design decision on how to access the above behavior/state:

1. Add a git operation method.
2. Alter some existing operation.

I went with 1 and added a getCommitInfo method along client-server communication pipeline, because it didn't seem to fit anywhere in the existing code. After completion, it's conceivable that it could fit in the RawGitService.status method. Actually, I think that this is probably where it should be, but I wanted to go ahead and start this PR. If I change it to this, then I don't think I will need to change any of the existing client-server channel code, and it would simplify a great deal. But, on the other hand, it's working now as-is (except it does not work with a global commit.template file specified as ~/.somefile. It can handle this format for a local commit template in the repo's .git folder).

3. The git changes view which contains the input box. AND 4. The git undo last commit action.

This is pretty straight-forward on the ChangesView. I've added a onCommitInputShown method and hooked this in the setVisible method. The only real nuance to this is understanding the workflow of the UndoLastCommitAction implementation.

Because of the distributed design of the actions, there must be a way of sharing state between the action and the ChangesView.commitInputBox. It had to get the previous commit info before executing the undo/reset action, and we need a way to get this previous message into the input box when it is shown. So I simply store the previous commit message in local storage (without an explicit scope) using the IStorageService when running the UndoLastCommitAction. Then, the first time that the onCommitInputShown is called, it always checks this first. If it exists, then it immediately sets its value to that and removes it from local storage.

Other than this nuance, it should be obvious from the code how this works.

[mention-bot]

@bill-mybiz, thanks for your PR! By analyzing the annotation information on this pull request, we identified @joaomoreno and @egamma to be potential reviewers

[msftclas]

Hi @bill-mybiz, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

[msftclas]

@bill-mybiz, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

[joaomoreno]

Very cool!

I have a few suggestions:

• Install the tslint extension: it will given you many warnings such as double semicolons, missing semicolons, whitespace, double quotes. You can also get them by running gulp hygiene.
• I was maybe a bit too eager by trying to combine this with the previous commit info. I suggest to keep that one out and simply implement the templates. This involves removing the getLog and all that comes with it.
• I see no reason why commitInfo should be part of IRawStatus (and getCommitInfo a part of IModel as well). Why not simply return a TPromise<ICommitInfo> from getCommitInfo()? We don't really need all git operations to return an IModel.

[ghost]

Gracias!

Install the tslint extension: it will given you many warnings such as double semicolons, missing semicolons, whitespace, double quotes. You can also get them by running gulp hygiene.

Yes, tslint! I completely forgot to run that as I was just trying to get this thing going.

I was maybe a bit too eager by trying to combine this with the previous commit info. I suggest to keep that one out and simply implement the templates. This involves removing the getLog and all that comes with it.
I see no reason why commitInfo should be part of IRawStatus (and getCommitInfo a part of IModel as well). Why not simply return a TPromise from getCommitInfo()? We don't really need all git operations to return an IModel.

I think that these two comments are linked, and the third relates to why you would separate the log out. (Edit: This is poorly worded on my part. I'm saying that I am guessing that your design decision to remove the getLog part is affected by an assumption in your suggestion of removing the ICommitInfo from the model.)

In the beginning, I totally agreed with not seeing any reason to put it on the model. I originally had the ICommitInfo implemented without the model, but it doesn't work. There are side effects when the git operation is performed, and this has nothing to do with my decision. One of those you can see, if you look at the _run. It executes operation.run() which returns just a plain vanilla, untyped Promise, but you can see in the then clause that it expects an IRawStatus. I tried many workarounds, but in the end the only way to get it to work was returning the IRawStatus which then leads to returning the IModel. There was also another side effect implied with this, and it occurs after every git action is run. At the time, I couldn't debug node, but from the error, it was plain to see that there was a subscriber peeking at the operation and expecting a status that it wasn't finding. Again, this was not my design decision.

I think you would probably want the getLog part removed assuming that this getCommitInfo could be an isolated call. But it isn't, and a separate getLog call would also have the same issues. You could of course roll your own persistence mechanism for the previous commit message, but why? And either way, the previous commit message still is related to this issue for the original reason that I included it: it relates to loading of the commitInputBox.value.

[ghost]

Also, here are the implementations in the RawGitService, where you can see the common way to implement these types of commands is to return the status:

    init(): TPromise<IRawStatus> {
        return this.repo.init().then(() => this.status());
    }

    add(filePaths?: string[]): TPromise<IRawStatus> {
        return this.repo.add(filePaths).then(() => this.status());
    }

    stage(filePath: string, content: string): TPromise<IRawStatus> {
        return this.repo.stage(filePath, content).then(() => this.status());
    }

    branch(name: string, checkout?: boolean): TPromise<IRawStatus> {
        return this.repo.branch(name, checkout).then(() => this.status());
    }

    checkout(treeish?: string, filePaths?: string[]): TPromise<IRawStatus> {
        return this.repo.checkout(treeish, filePaths).then(() => this.status());
    }

These are just a few. You can check out them at RawGitService.

[joaomoreno]

You don't really need to implement this call as an operation, since it is a pure call (no consequences). You can just copy what is done by the detectMimetypes call. That way you won't have the restriction of having to return an IRawStatus.

[ghost]

That is certainly why I tried that approach the first time (and failed). But now everything was much clearer and this was pretty easy to make these changes. I went ahead and removed ICommitInfo completely and returned a string with the commit template's contents. So now it is much cleaner.

There is still a problem with locating paths that are in the form of ~/path/with/tilde. It is resolving to the repos .build path. Edit: I would expect this on Windows to resolve to the user's home directory. (My previous implementation was hacking it to just look in the repo's .git folder which is terribly wrong.)

[joaomoreno]

Awesome, I ended up doing another commit on top and merged it in:

• Implemented the same path resolution that git implements, to figure out the template file's location.
• Removed all sync method calls and implemented it in async
• Moved the implementation down to git.lib.ts
• Removed extra cruft left behind

Great job, and thanks! 🎆

[ghost]

@joaomoreno I think there is a bug from this line of code that was left over from my first go at removing the previous commit msg aspect. If you simply unstage a single file, it will wipe out the commit message.

[ghost]

To be more explicit, the bug is that the second clause in the || statement which I added to trigger the update of the previous commit message. So, you should remove the || e.operation.id === git.ServiceOperations.RESET part.

[joaomoreno]

Thanks! 8c38fca

[ghost]

@joaomoreno You're gonna hate me dude, but there looks to be another teensy bug. Now when the commit occurs, it is not stripping the # lines. So all of the comments are included in the commit msg.

I'm sorry I didn't catch this earlier, but I've had to use the competition (Atom 😱) for Elixir support, and I have been using vim via the command line. Anyway, I can look into it if you want. It's your call.

[joaomoreno]

That's quite alright, I don't hate you ❤️ I'll fix it in #11646

PS: 😱 The A word.