GH-3851 improve 'amend' experience

[westbury]

This PR improves the user experience when using the 'Amend' button.

1. A list of the commits being amended is shown above the last commit.
2. There is an 'Unamend' button that reverses the previous 'Amend'
3. All commits being amended can be unamended using the '-' button in the section header
4. The movement of commits in the 'last commit' section and the 'commits being amended' section are animated so that the user readily sees how commits are moving between the sections.

In the screenshot below you will see how the view looks after pressing 'Amend' twice.

[westbury]

@lmcbout I was wondering if this PR solves the UX issues you mentioned around the 'Amend' button.

This will need a bit of merging to work with #4279

[vince-fugnitto]

@westbury I think the Amend section requires some form of minimum height to avoid the following:

[lmcbout]

Hi @westbury
I am testing this current issue #3851 . I can see the section "Commits Being Amended", but the "Unamend" button shows on last commit only. This is OK , but when I reached the last "Commit" to "Unamend, I only see a little box that I cannot read, so unable to access the "Unamend" button .

To test, I select the "Amend" button a few times until the list of files was filling the height of the window view. When the number of files fill the height, we come to a point I don't see the section "COMMITS BEING AMENDED". The section becomes too small, there is no minimum height being kept to see it on the view. Then to select the "Unamend" button is not visible.

I think this functionality is very good since it allows the end-user to reset to the previous commit

[vince-fugnitto]

Minor, but I'm not sure the removal of the spacing here is required.
Throughout Theia, we use this spacing in other css files.

[westbury]

@vince-fugnitto Thanks for looking at this. Yes, something looks very wrong there. I was aware it looked odd as the section got too small but I have not seen that. I will do further testing on Linux (I developed it on Windows).

[westbury]

The removal of the space was unintentional and I did not notice it. I'll restore it.

[westbury]

@lmcbout Thanks for trying it out. The 'Unamend' is only on the bottom commit because otherwise we are switching the order of commits with possible conflicts and other complications. I'll do some more tweaking of the UI to improve it when the sections need to shrink.

[svenefftinge]

It looks pretty nice and improves the current state significantly. I wonder how we can keep this once we switch to the SCM + VS Code git extension. But I definitely would want to find a way.

@kittaakos Could you review as well?

[kittaakos]

I can see the following when trying to amend to the initial commit:

Uncaught (in promise) Error: Request 'exec' failed
    at Proxy.<anonymous> (bundle.js:98716)
    at GitWidget.<anonymous> (:3000/74.bundle.js:7034)
    at step (:3000/74.bundle.js:6712)
    at Object.next (:3000/74.bundle.js:6693)
    at fulfilled (:3000/74.bundle.js:6684)
08:27:53.578 :3000/74.bundle.js:6685 Uncaught (in promise) Error: Request 'exec' failed
    at Proxy.<anonymous> (bundle.js:98716)
    at GitWidget.<anonymous> (:3000/74.bundle.js:7034)
    at step (:3000/74.bundle.js:6712)
    at Object.next (:3000/74.bundle.js:6693)
    at fulfilled (:3000/74.bundle.js:6684)
08:27:59.595 git-widget.tsx:15 Uncaught (in promise) Error: Request 'exec' failed
    at Proxy.<anonymous> (proxy-factory.ts:232)
    at GitWidget.<anonymous> (git-widget.tsx:277)
    at step (git-widget.tsx:15)
    at Object.next (git-widget.tsx:15)
    at fulfilled (git-widget.tsx:15)

Perhaps the same issue exisits on the master as well.

[akosyakov]

My problem with this approach that it is not based on the single sourcing of git state, but keeps some in memory state. You can just open another page or reload page to see how it is confusing. We could build up shared state on the backend but it's rather complicate things.

[kittaakos]

👍 It works great, I have added a few remarks.

[kittaakos]

Why do we need the guard against instance? Can this be null or undefined?

[kittaakos]

If something is wrong please log it, or delete the if.

[kittaakos]

This logic is very similar to the amend logic, can we extract and reuse it? If we ever hit a bug, we can fix it one place ;)

[kittaakos]

Can we have HEAD Commit? What do you think?

[kittaakos]

[minor]: map -> array (or commits)?

[kittaakos]

trying to amend to the initial commit:

And here is the corresponding log from the backend:

root ERROR The command `git reset HEAD~ --soft` exited with an unexpected code: 128. The caller should either handle this error, or expect that exit code.
root ERROR fatal: ambiguous argument 'HEAD~': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

root ERROR Request exec failed with error: fatal: ambiguous argument 'HEAD~': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
 GitError: fatal: ambiguous argument 'HEAD~': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

    at new GitError (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:81:28)
    at Object.<anonymous> (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:256:27)
    at step (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:50:23)
    at Object.next (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:31:53)
    at fulfilled (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:22:58)
    at <anonymous>
root ERROR Uncaught Exception:  GitError: fatal: ambiguous argument 'HEAD~': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

root ERROR GitError: fatal: ambiguous argument 'HEAD~': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

    at new GitError (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:81:28)
    at Object.<anonymous> (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:256:27)
    at step (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:50:23)
    at Object.next (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:31:53)
    at fulfilled (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:22:58)
    at <anonymous>
root ERROR Uncaught Exception:  GitError: fatal: ambiguous argument 'HEAD~': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

root ERROR GitError: fatal: ambiguous argument 'HEAD~': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

    at new GitError (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:81:28)
    at Object.<anonymous> (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:256:27)
    at step (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:50:23)
    at Object.next (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:31:53)
    at fulfilled (/Users/akos.kitta/Desktop/theia/node_modules/dugite-extra/lib/core/git.js:22:58)
    at <anonymous>

[westbury]

My problem with this approach that it is not based on the single sourcing of git state, but keeps some in memory state. You can just open another page or reload page to see how it is confusing. We could build up shared state on the backend but it's rather complicate things.

Yes, I had thought of creating a special tag for the head of the amending commits, but the user may see the tag when using other tools and be confused by it. I think we can solve this simply using the storage service, and a key that contains the repository URI. The only thing we actually need to store is the SHA of the first amended commit. I'm working on this now.

[westbury]

I think the Amend section requires some form of minimum height to avoid the following

@vince-fugnitto I have re-written it to introduce a minimum height. This also changes the scroll bars so the commits being amended are now separately scrollable underneath the header for the amending commits.

[westbury]

I wonder how we can keep this once we switch to the SCM + VS Code git extension. But I definitely would want to find a way.

@svenefftinge We want these changes and we also want the SCM + VS Code get extension, so we will definitely find a way. I had been waiting for #4279 before finalizing this PR but it looks like there is still quite a lot of work on that PR to get back to the same functionality.

[westbury]

I have now addressed all the code review comments and it is ready for reconsideration for merging.

@akosyakov I have made the changes to use the storage service to store the head of the list of commits being amended. This solves the issue of amend commits not showing after a refresh or opening another client instance. The changes are in a separate commit so can be reviewed separately 2bc1b45

@kittaakos I believe all your comments have been addressed. After the rewrite of the way transitions and scrolling were done, some comments were obsolete.

The git-widget file is getting big at 1365 lines. I'm sure it will get refactored a lot when/if it is merged into #4279 so I have not worried about the size and complexity for now.

[kittaakos]

@kittaakos I believe all your comments have been addressed. After the rewrite of the way transitions and scrolling were done, some comments were obsolete.

👍 Thank you, @westbury! I am a bit busy with something else now, but the review will be done by the end of this week.

[kittaakos]

This solves the issue of amend commits not showing after a refresh or opening another client instance.

If I do a git reset --hard from the terminal, the view will be stuck in an inconsistent state which is OKish; it happens with VS Code too on a daily bases. However, when in VS Code, I Refresh the widget, it gets back to sync with the git status. It's not the case in Theia.

[kittaakos]

Refreshing the browser does not help either. Would it be possible to add some logic that checks the git status state? If the git status is "empty", can we forcefully discard the amend state in the view?

[westbury]

@kittaakos thanks for testing it. What I would expect is that making changes to the index or working tree will not affect the 'unamend'. So if one amends a commit, does a reset hard, and then unamends the commit, I would expect the commit to be restored and the working tree containing the reversion of the commit. The amend list is cleared only if HEAD changes. I have not tested the 'reset hard' case and will do that shortly.

[westbury]

@kittaakos I've re-tested it and it is working as I expect. Changing the index or the working tree does not affect the list of amending commits. If one edits files, stages or unstages changes, or use any other way of changing the index or working tree such as 'git reset HEAD --hard', the list of amending commits remains and one can still restore commits. This is as I would expect. Changing the head, however, does clear the list. For example, committing some changes or git reset to something other than HEAD does clear the list.

It is a concern that this caused confusion as if it caused confusion to you it is sure to cause confusion to others. I have added another button to the 'Commits Being Amended' header that clears the list. This gives users a way of clearing the list after they find a refresh does not achieve that. Of course, doing a refresh should not be necessary to clear any 'problem'.

I'm not convinced that the amend list should be cleared simply because the index and working tree are clean. That just does not seem the right trigger for clearing the list. It is still valid to unamend a commit when clean.

[akosyakov]

I would prefer if we focus on testing and landing #4279 instead of adding more changes to git extension, after that we can add more functionality with a new approach. Otherwise it is only more deviations and adds mode burden on @vinokurig. cc @svenefftinge

[westbury]

The code in this PR was merged into #4279 which has now been merged to master. Therefore I am closing this PR.