#384 View alternate parent changes at merge commits

[dan1994]

Closes #384

[mhutchie]

Hi @dan1994,

Thank you for raising this pull request, based on our ongoing discussion on Discord. It's great to have a working proof-of-concept of this feature request.

I've tried it out, and agree with you that your current CTRL + Click parent selector isn't obvious enough to users. It would be great if you can experiment with some other approaches, to explore what would work best.

An alternative approach that I'd thought of, is to display a new action "View Changes with this Parent" on the right-click context menu on the parent commit hashes. It would be great if you could try this out, in addition to any other idea's you have. If this approach was to be used:

• Keep the bolding behaviour you've implemented, I think that it's a nice & clear way to indicate which parent is currently being used.
• Keep all the parents on a single line, like the existing UI. This was done to save some lines, and maximise the space available to display the commit body (without needing to scroll the Commit Details View).
• The existing implementation only adds internal links to parent commits that are loaded in the view. For this approach to be used, I think we'd have to make it so that all parents are linked, and show an error message if the user tries to follow the link when the parent can't be found in the loaded commits.
• This context menu action could be done in addition to your current CTRL + Click behaviour - users can use whichever they prefer.

FYI: Stashes are handled separately in the back-end, so some minor adjustments need to be made to properly handle them with the changes made with this PR.

[mhutchie]

GitHub duplicated my review - please ignore this comment, as it's a duplicate of what I'd written above.

[dan1994]

An alternative approach that I'd thought of, is to display a new action "View Changes with this Parent" on the right-click context menu on the parent commit hashes.

I hope to come up with a solution that is immediately visible to users, so that they can learn about this feature intuitively, but given the size constraints you mentioned, I'm not sure I'll have something better than the context menu. I just think that when I see a link I don't think "let's see what happens when I right click this".

The existing implementation only adds internal links to parent commits that are loaded in the view. For this approach to be used, I think we'd have to make it so that all parents are linked, and show an error message if the user tries to follow the link when the parent can't be found in the loaded commits.

Don't you think a better approach would be to load more commits, and show an error only as a last resort? I try to imagine myself getting this error, and I think it will be confusing.

Stashes are handled separately in the back-end, so some minor adjustments need to be made to properly handle them with the changes made with this PR.

I'll take a look, and try to perform the adjustments.

[mhutchie]

I hope to come up with a solution that is immediately visible to users, so that they can learn about this feature intuitively...

I completely agree. I'm keen to see what you come up with! The only alternative that I could think of, that would be immediately visible to users, would be to add a button the Commit Details View Control Bar (right side). For example, the icon could be "^1" (number is the parent index), and the action displays a context menu of parents to choose from. The button would only need to be shown if the commit has multiple parents.

The existing implementation only adds internal links to parent commits that are loaded in the view. For this approach to be used, I think we'd have to make it so that all parents are linked, and show an error message if the user tries to follow the link when the parent can't be found in the loaded commits.

Don't you think a better approach would be to load more commits, and show an error only as a last resort? I try to imagine myself getting this error, and I think it will be confusing.

I've discussed this on GitHub before, but in summary:

• There's no way that myself, or anyone I've mentioned this to in the past, have found to determine how many more commits need to be loaded for an arbitrary commit to be included in the view (respecting filters and order).
• There's no feasible way to determine whether the user has deliberately filtered out the commit from the view (in which case it can never be loaded, so loading more & more commits will load all the commits in the repository, and still not find it).
• This case is generally so rare that most user's won't ever even encounter the scenario - so even if a complex solution existed, it might be hard to justify implementing it.

In the context my alternative approach, and how it would impact the internal links, it's possibly still a better solution than the existing behaviour, as at least the user would be shown an error message explaining the situation (e.g. "The commit could not be found in the loaded commits, either: load more commits, or check any filtering that has been applied"), instead of wondering why one parent doesn't have a link, when almost all of them do. The error dialog could even provide an action to "Load More Commits".

[dan1994]

The only alternative that I could think of, that would be immediately visible to users, would be to add a button the Commit Details View Control Bar (right side). For example, the icon could be "^1" (number is the parent index), and the action displays a context menu of parents to choose from. The button would only need to be shown if the commit has multiple parents.

That's actually a pretty good idea. I kept thinking about how I integrate it inside the parents line, but having a button on the sidebar can be great. Now I only need to find the right button (P.S.: already have a PoC for a context menu action).

Regarding the behavior when a commit can't be found, I'll try to make the error message as informative as possible as you have suggested.

[dan1994]

Stashes are handled separately in the back-end, so some minor adjustments need to be made to properly handle them with the changes made with this PR.

I've taken a look, and am trying to figure out the correct course of action. Here are my observations so far:

1. In the UI, parents which are not standard commits are not marked as links, and so the user can't interact with them.
2. Theoretically, the user shouldn't even know about these commits (I didn't know they existed until I saw your comment and did some research). Maybe we should just not list them in the parents section?
3. Given that they either shouldn't be interacted with (no link) or shouldn't even appear, I think there's no need to change the backend.
4. Assuming we do want the option to diff them against other parents, I would be glad if you pointed out where they are first populated as I'm trying to follow the trail between the frontend and backend, and it is less obvious than I'd hoped.

Please let me know which observations are correct, and which are not, and what you think would be the best course of action.

[mhutchie]

Theoretically, the user shouldn't even know about these commits (I didn't know they existed until I saw your comment and did some research). Maybe we should just not list them in the parents section?

I’d considered that when I first implemented stashes, however ultimately I decided against it, as I thought some users would complain and think it was a bug if only some of the parents are shown. There’s even some rare use cases I’ve encountered where it’s actually quite useful to know all the internal commits within a stash.

Given that they either shouldn't be interacted with (no link) or shouldn't even appear, I think there's no need to change the backend.

I don’t think we need to allow alternative parents to be selected for stashes, however it depends on how it’s implemented in the UI as to whether we should do it. If we stay with the Ctrl + Click / Context Menu on the parent commits, for consistency we’d need to support alternate parents for stashes. However, if we decide to just have a button on the Commit Details View Control Bar, then we can just only show the button when the commit has multiple parents, and they’re not a stash.

Assuming we do want the option to diff them against other parents, I would be glad if you pointed out where they are first populated as I'm trying to follow the trail between the frontend and backend, and it is less obvious than I'd hoped.

You should just need to replace stash.baseHash on these lines with the same logic you implemented for the Commit Details View parents. (stash.baseHash is always the first parent)

[dan1994]

I don’t think we need to allow alternative parents to be selected for stashes, however it depends on how it’s implemented in the UI as to whether we should do it.

I based my changes on the original functionality, which decided if the commit should be a link by checking if it can be found in the lookup table. From what I've seen, stash internal commits are never styled as links according to this logic.

This means you will always see one parent (the actual one) as a link, and the others as text. According to that, even given my ctrl click mechanism, the user can never display diffs to internal commits.

In my opinion we should consistently not allow to show diffs with internal commits, and maybe open another issue if you think it's worth it.

stash.baseHash is always the first parent

This is the problem. I don't need the parent, but rather the hash of the stash itself. Applying my mechanism to the base hash will get me to the grandparents.

[mhutchie]

From what I've seen, stash internal commits are never styled as links according to this logic.

That's true only when the stash is shown as a single commit. If the user enables "Include commits only mentioned by reflogs" in the Repository Settings Widget, then all the internal commits are shown in the view.

In my opinion we should consistently not allow to show diffs with internal commits, and maybe open another issue if you think it's worth it.

For now you can just ignore the stash case. We can circle back to stashes once everything else is handled, and can decide what we'll do then. (That's why I only mentioned it as an FYI in my original response)

stash.baseHash is always the first parent

This is the problem. I don't need the parent, but rather the hash of the stash itself. Applying my mechanism to the base hash will get me to the grandparents.

The hash of the stash itself is commitHash on these lines. What I meant was that you just need to use you fromCommit logic from getCommitDetails, in the place of where stash.baseHash is currently used.

[dan1994]

That's true only when the stash is shown as a single commit. If the user enables "Include commits only mentioned by reflogs" in the Repository Settings Widget, then all the internal commits are shown in the view.

Cool, I just keep learning new things about this extension :). I'll enable this setting and test things out.

For now you can just ignore the stash case. We can circle back to stashes once everything else is handled, and can decide what we'll do then. (That's why I only mentioned it as an FYI in my original response)

Given that there are relevant use cases, I think I will implement it in this issue, but I will start with the standard use case.

The hash of the stash itself is commitHash on these lines. What I meant was that you just need to use you fromCommit logic from getCommitDetails, in the place of where stash.baseHash is currently used.

I wasn't sure how stash.baseHash behaves in respect to commitHash, but if replacing it with ${commitHash}^{parentIndex} works, then problem solved.

[dan1994]

Change Summary:

Frontend:

• Add context menu item to parent links to choose them to diff against
  • Add a class of the form parent-<index> to find the index on click
• Add a button (icon from icons8 and is listed in the correct place) to toggle between parents
  • Add a variable called loadedParents that is a subset of the commit parents. It is used both for rendering of parents and making sure we never toggle to an unloaded parent.
• Fix bugs where parentIndex is not reset properly

Backend:

• Stash loading now supports parent choice to diff with

[dan1994]

Hey @mhutchie, I've noticed you haven't reviewed my branch yet. Would you have time for that soon?

[mhutchie]

Hi @dan1994,

Thanks for making all those changes, this feature is progressing nicely!

Please specifically ask for a review in the future when you're wanting one. I just assumed your "Change Summary" comment was giving a progress-update, alongside the recent commit's you'd made (I'd even liked it two weeks ago) - I didn't know you wanted me to do a code review.

[dan1994]

My bad then, I'll ask next time.

[dan1994]

Hey @mhutchie, I've made all the requested changes. Please review :)

[mhutchie]

Thanks for making those changes, it's looking really good!

I've made some additional comments, that I'd appreciate you resolving.

Additionally:

• Please merge the current develop branch into your PR's branch. This last week I added complete unit test coverage for ./src/gitGraphView.ts, which was long overdue. You'll now be able to properly unit test your changes to ./src/gitGraphView.ts.
• In several places in this pull request you use template literals. It's really important for the extension's codebase to be as consistent as possible, so that it's easy to understand for all current and future contributors. I acknowledge that the codebase still isn't as consistent as I'd like, so it's a constant effort of mine that whenever I work on some code, I improve it, and make it more consistent. Throughout the entire codebase, I only use template literals in a single place in the extension (to be consistent with Visual Studio Code's sample implementation), everywhere else uses standard string concatenation. Please use standard string concatenation instead of template literals (purely for consistency).

[dan1994]

I've added the missing field in some tests, but they are not passing because of a timeout.
I'm a little less familiar with all of your mocks, and would be glad if you could look at my latest commit and point me towards the source of the error.

[mhutchie]

Hi @dan1994,

The test are failing because the expect(messages).toStrictEqual([...]) assertion is not receiving the expected messages within a timeout period. From the code it looks like you added parentIndex in the response message, but you haven't added it in the expected messages. You should just need to add it on lines 977, 1015 & 1060.

[dan1994]

Oh, yeah, I see it now. The error message misled me to believe that functions weren't called at all, when they were actually called with the wrong parameters. Thanks for the pointer.

[dan1994]

I've addressed your requests, please review.

[mhutchie]

Hi @dan1994,

I've reviewed your changes, and I'm really happy with how this feature has turned out. Thanks for all your hard work!

I'm going to use it for a few days just to see if there's anything else I can think of. I'll merge it, and include it in the next beta release.

FYI: Since our last messages on the unit tests timing out when async expectations aren't met, I've tweaked waitForExpect so it now propagates the Jest expectation exceptions, so it's trivial to spot the root cause.

[dan1994]

Glad to hear that!

[mhutchie]

Hi @dan1994,

After further testing this last week, I noticed:

• Due to the nature of stash's internal commit structure, prior to this feature instead of just displaying the file changes relative to the first parent, file changes in the third parent (containing the untracked files) are also included. This is so that when the user clicks on a stash, all of the untracked, working tree, and staged changes are included. Unfortunately this doesn't fit well with viewing the changes with a specific parent, as multiple parents are used by default. I'd forgotten this nuance with stashes in our previous discussions.
• When viewing a file diff, your implementation always shows the file diff relative to the first parent, regardless of which parent the user has selected in the Commit Details View.

Both of these are quite tricky to resolve, so I've been working on them this weekend. I'll push the changes to your branch and merge this PR as soon as I've completed them.

[dan1994]

Thanks fur the update! Let me know if you need any help.