Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix files changed and reverting merge commits from the history panel bug #1227

Merged
merged 19 commits into from
Apr 6, 2023
Merged

Fix files changed and reverting merge commits from the history panel bug #1227

merged 19 commits into from
Apr 6, 2023

Conversation

basokant
Copy link
Contributor

@basokant basokant commented Feb 27, 2023 •
edited by fcollonval
Loading

Fix for #621

The goals for this PR are to:

  • Correctly display the files changed for any merge commit
  • Allow for correctly reverting a merge commit
  • Correctly display diffs involving merge commits
  • Add integration tests

@github-actions
Copy link

Binder 👈 Launch a binder notebook on branch basokant/jupyterlab-git/fix-issue-621

@basokant basokant closed this Feb 27, 2023
@basokant basokant changed the title account for merge commit logs when retrieving the detailed log for a … Fix files changed and reverting merge commits in history panel Feb 27, 2023
@basokant basokant changed the title Fix files changed and reverting merge commits in history panel Fix files changed and reverting merge commits from the history panel bug Feb 27, 2023
@basokant basokant reopened this Feb 27, 2023
@basokant
Copy link
Contributor Author

Currently, the files changed in a merge commit can be displayed in the "Changed" area. However, some merge commits show no files changed (notably, merge commits created by merged pull requests).

fix-merge-commits.mov

fcollonval
fcollonval reviewed Mar 3, 2023
View reviewed changes
jupyterlab_git/git.py Outdated
@@ -567,12 +567,13 @@ async def log(self, path, history_count=10, follow_path=None):

async def detailed_log(self, selected_hash, path):
"""
Execute git log -1 --numstat --oneline -z command (used to get
Execute git log -m -1 --numstat --oneline -z command (used to get
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess the trick is in the various options for --diff-merges, isn't it?

I'm looking forward to hearing what you learn by trying different options.

@basokant basokant marked this pull request as ready for review March 9, 2023 04:34
@basokant basokant requested a review from fcollonval March 9, 2023 04:34
@basokant
Copy link
Contributor Author

basokant commented Mar 9, 2023 •
edited
Loading

I added -m --cc to the detailed git log

  • Detailed logs now use the options -m (specifies to split each merge, computing a diff for each parent of the merge)
  • On it's own, this will show files changed compared to the base branch, and separately the files that were changed compared to the merged in branch.
    • This is not ideal, as it shows too many files changed, and the virtual commits for each parent changes the way we must parse the log.
  • The --cc produces a combined diff, which solves both of these problems.

https://stackoverflow.com/questions/37801342/using-git-log-to-display-files-changed-during-merge

As for reverting, I added -m 1, to revert to the tree of the first parent of the merge.

https://stackoverflow.com/a/7100005

@basokant
Copy link
Contributor Author

basokant commented Mar 9, 2023 •
edited
Loading

Here is a demo of the fixes:

Merge.Commit.Fixes.mov

@basokant
Copy link
Contributor Author

basokant commented Apr 1, 2023 •
edited
Loading

image

@fcollonval I got my environment all fixed up, and after some rewriting the integration tests for merge commits now work!
However, one of the file selection tests is failing, but it doesn't seem to be because of anything I added in this PR.

@basokant @fcollonval
account for merge commit logs when retrieving the detailed log for a …
c116e9a 
…commit, as they have an extra line for the branch that was merged into the base branch
@basokant @fcollonval
add --cc option for detailed logging
ce38e8c
@basokant @fcollonval
fix reverting merge commits, assuming the mainline branch is the firs…
1058aee 
…t parent
@basokant @fcollonval
change test to reflect changes to detailed log
97d3676
@basokant @fcollonval
correct detailed logger handler comment to include --cc -z
840a06e
@basokant @fcollonval
init merge commit integration test
fd7035d
@basokant @fcollonval
add @jupyterlab/galata and @playwright/test
1a24134
@basokant @fcollonval
finish merge commit integration test 1
0819ed1
@basokant @fcollonval
finish merge commit integration test 2
e14177b
@basokant @fcollonval
finish merge commit integration test 3
00c2dda
@basokant @fcollonval
finish merge commit integration test 4 (revert merge commit)
7a2685b
@basokant @fcollonval
fix multiple installations of @playwright/test and upgrade @playwrigh…
1ffca83 
…t/test dependency
@basokant @fcollonval
complete final working ui-tests for merge commits
ad4412e
@fcollonval
Copy link
Member

Hey @basokant thanks for pushing this PR. I fixed the CI and now only your four new integration tests are failing.

The error is always the same: waiting for getByText('basokant8d6c5d09 days agoworkingmainMerge branch \'sort-names\''). And my guess from the test videos is that you are hit by the ellipsis. See:

image

I would suggest simplifying the text selector to only look for the commit message: getByText('Merge branch \'sort-names\'')

@basokant
Copy link
Contributor Author

basokant commented Apr 5, 2023

I would suggest simplifying the text selector to only look for the commit message: getByText('Merge branch \'sort-names\'')

@fcollonval Made that requested change, and the integration tests have passed! Thanks for fixing the CI!!

fcollonval
fcollonval approved these changes Apr 6, 2023
View reviewed changes
Copy link
Member

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @basokant

@fcollonval fcollonval merged commit 2d186cd into jupyterlab:master Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fcollonval fcollonval fcollonval approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@basokant @fcollonval