Diff and resolve files on merge conflict #1013

navn-r · 2021-08-16T16:34:10Z

Fixes #896

github-actions · 2021-08-16T16:34:24Z

👈 Launch a binder notebook on branch MLH-Fellowship/jupyterlab-git/feat/diff-merge-conflicts

src/commandsAndMenu.tsx

- [WIP] See TODOs

- [WIP] console logs content - TODO: display diff

+ got eslint to shut up

- Remove MergeDiffModel - Delete PlainTextMergeDiff - Add unit test for base content

Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>

- TODO: update file with resolved conflicts

navn-r

Hey @fcollonval, recent commit has the ability to view the merge conflict in theory.

I'm running into an issue which I can't seem to figure out. I've made a merge conflict on my local, and when I press the diff button, i get this error.

I'm not sure about what it means, but what I did notice is in the response body, the one merge decision that's in the array merge_decisions, which is supposed to be one, since there is only one conflict in one cell, has the property remote_diff as null. Is this intended to be null?

Am I missing anything else for the view to show up?

All the properties in the IMergeDecision interface is all optional sooooo no way to tell anything 😝

Response from Server

{
    "base": {
        "cells": [
            {
                "cell_type": "code",
                "execution_count": 5,
                "id": "054d802b-e56b-4594-9a8a-58564bb38d2c",
                "metadata": {},
                "outputs": [
                    {
                        "name": "stdout",
                        "output_type": "stream",
                        "text": "modified by HEAD\n"
                    }
                ],
                "source": "print('modified by HEAD')"
            },
            {
                "cell_type": "code",
                "execution_count": 2,
                "id": "b72f12bc-22b2-4e60-8321-07c2fd218414",
                "metadata": {},
                "outputs": [
                    {
                        "name": "stdout",
                        "output_type": "stream",
                        "text": "modified by ORIG_HEAD\n"
                    }
                ],
                "source": "print('modified by ORIG_HEAD')"
            },
            {
                "cell_type": "code",
                "execution_count": 6,
                "id": "287ff473-5b96-4a27-98f2-55c080217a34",
                "metadata": {},
                "outputs": [
                    {
                        "name": "stdout",
                        "output_type": "stream",
                        "text": "modified by HEAD\n"
                    }
                ],
                "source": "print('modified by HEAD')"
            }
        ],
        "metadata": {
            "kernelspec": {
                "display_name": "Python 3",
                "language": "python",
                "name": "python3"
            },
            "language_info": {
                "codemirror_mode": {
                    "name": "ipython",
                    "version": 3
                },
                "file_extension": ".py",
                "mimetype": "text/x-python",
                "name": "python",
                "nbconvert_exporter": "python",
                "pygments_lexer": "ipython3",
                "version": "3.9.5"
            }
        },
        "nbformat": 4,
        "nbformat_minor": 5
    },
    "merge_decisions": [
        {
            "common_path": [
                "cells"
            ],
            "conflict": false,
            "action": "local",
            "local_diff": [
                {
                    "op": "addrange",
                    "key": 0,
                    "valuelist": [
                        {
                            "cell_type": "code",
                            "execution_count": 2,
                            "id": "054d802b-e56b-4594-9a8a-58564bb38d2c",
                            "metadata": {},
                            "outputs": [
                                {
                                    "name": "stdout",
                                    "output_type": "stream",
                                    "text": "modified by MERGE_HEAD\n"
                                }
                            ],
                            "source": "print('modified by MERGE_HEAD')"
                        }
                    ]
                },
                {
                    "op": "patch",
                    "key": 0,
                    "diff": [
                        {
                            "op": "replace",
                            "key": "execution_count",
                            "value": 1
                        },
                        {
                            "op": "patch",
                            "key": "id",
                            "diff": [
                                {
                                    "op": "addrange",
                                    "key": 0,
                                    "valuelist": [
                                        "b72f12bc-22b2-4e60-8321-07c2fd218414"
                                    ]
                                },
                                {
                                    "op": "removerange",
                                    "key": 0,
                                    "length": 1
                                }
                            ]
                        },
                        {
                            "op": "patch",
                            "key": "outputs",
                            "diff": [
                                {
                                    "op": "patch",
                                    "key": 0,
                                    "diff": [
                                        {
                                            "op": "patch",
                                            "key": "text",
                                            "diff": [
                                                {
                                                    "op": "patch",
                                                    "key": 0,
                                                    "diff": [
                                                        {
                                                            "op": "addrange",
                                                            "key": 12,
                                                            "valuelist": "MERGE_"
                                                        }
                                                    ]
                                                }
                                            ]
                                        }
                                    ]
                                }
                            ]
                        },
                        {
                            "op": "patch",
                            "key": "source",
                            "diff": [
                                {
                                    "op": "patch",
                                    "key": 0,
                                    "diff": [
                                        {
                                            "op": "addrange",
                                            "key": 19,
                                            "valuelist": "MERGE_"
                                        }
                                    ]
                                }
                            ]
                        }
                    ]
                },
                {
                    "op": "patch",
                    "key": 1,
                    "diff": [
                        {
                            "op": "replace",
                            "key": "execution_count",
                            "value": 3
                        },
                        {
                            "op": "patch",
                            "key": "id",
                            "diff": [
                                {
                                    "op": "addrange",
                                    "key": 0,
                                    "valuelist": [
                                        "287ff473-5b96-4a27-98f2-55c080217a34"
                                    ]
                                },
                                {
                                    "op": "removerange",
                                    "key": 0,
                                    "length": 1
                                }
                            ]
                        }
                    ]
                },
                {
                    "op": "removerange",
                    "key": 2,
                    "length": 1
                }
            ],
            "remote_diff": null
        }
    ]
}

tsconfig.json

fcollonval · 2021-08-22T09:28:04Z

I'm not sure about what it means, but what I did notice is in the response body, the one merge decision that's in the array merge_decisions, which is supposed to be one, since there is only one conflict in one cell, has the property remote_diff as null. Is this intended to be null?

Am I missing anything else for the view to show up?

This is definitely related to the new cell id. I thought ttps://github.com/jupyter/nbdime/pull/566 would have avoid the error. But obviously this is not the case...

Pinging @krassowski and @vidartf on this one.

A workaround to allow you to move forward with the integration is to strip the id property in all cells in the three notebook versions before calling merge_notebooks. Something like that on the read content:

for cell in nb.cells:
    del cell['id']

navn-r

So far this is looking good, last thing TODO, is to finish the getResolvedFile method for notebooks. Are there methods on the Notebook Widget that I should look at?

Oh and unit tests of course. :)

src/components/diff/NotebookDiff.ts

fcollonval

Some minor changes.

I would like to rename isConflict to hasConflict that sounds better; hence the high number of suggestion 😉

jupyterlab_git/git.py

src/commandsAndMenu.tsx

src/components/diff/model.ts

src/tokens.ts

src/components/diff/model.ts

Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>

fcollonval · 2021-08-23T15:10:13Z

On the design front, it would be nice to bring the top checkboxes Clear _all_ cell outputs and Clear _conflicted_ cell outputs in the toolbar.

And as for the notebook this is a three view display, the middle header RESULT feels strange. Not displaying anything may be better.

navn-r · 2021-08-23T15:34:08Z

it would be nice to bring the top checkboxes Clear_all_cell outputs and Clear_conflicted_cell outputs in the toolbar.

Since those checkboxes are part of the nbdime view, I think we would need

Remove those checkboxes from the DOM, setting a display: none on .jp-Merge-notebook-controls should do the trick for it
Ability to call the widget to do those actions, so something like this._nbWidget.clearAllCellOuputs, etc.

Is there a function I can call in the widget/model to do part 2) ?

fcollonval · 2021-08-23T15:43:34Z

Is there a function I can call in the widget/model to do part 2) ?

Code in nbdime is there: https://github.com/jupyter/nbdime/blob/a74b538386d05e3e9c26753ad21faf9ff4d269d7/packages/nbdime/src/merge/widget/notebook.ts#L200

fcollonval

Hey @navn-r this looks great. I added mainly doc comments and a request to remove the templating on the diff model. Then we can merge this one to work on integration tests.

src/commandsAndMenu.tsx

src/components/diff/NotebookDiff.ts

src/components/diff/PlainTextDiff.ts

src/components/diff/model.ts

src/tokens.ts

Rename widget accessor + Update doc Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>

fcollonval · 2021-09-02T08:23:02Z

@all-contributors please add @navn-r for code

allcontributors · 2021-09-02T08:23:11Z

@fcollonval

I've put up a pull request to add @navn-r! 🎉

navn-r force-pushed the feat/diff-merge-conflicts branch from d9ae4fc to aca3d81 Compare August 16, 2021 16:38

navn-r changed the title ~~Diff and resolve files on merge conflict~~ [WIP] Diff and resolve files on merge conflict Aug 16, 2021

navn-r force-pushed the feat/diff-merge-conflicts branch from 5260b7e to 7078a87 Compare August 16, 2021 22:54

navn-r commented Aug 16, 2021

View reviewed changes

src/commandsAndMenu.tsx Outdated Show resolved Hide resolved

navn-r and others added 8 commits August 21, 2021 14:39

Add section for merge conflict files (jupyterlab#896)

033ecb8

- [WIP] See TODOs

Fetch three-way-diff content for merge conflicts (jupyterlab#896)

bc13205

- [WIP] console logs content - TODO: display diff

Display three-way-diff content for text files (jupyterlab#896)

9c89658

Tweak unmerged row style

fe36150

+ got eslint to shut up

Refactor existing diff for merge conflicts

18ed234

- Remove MergeDiffModel - Delete PlainTextMergeDiff - Add unit test for base content

Apply suggestions from code review

fc54eca

Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>

Fetch the resolved file contents during merge conflict

6282b4e

- TODO: update file with resolved conflicts

Save resolved text file from merge conflict

1f26e0d

navn-r force-pushed the feat/diff-merge-conflicts branch from 88f7176 to 1f26e0d Compare August 21, 2021 18:40

Start setup for notebook merge diffs

9fdbb29

navn-r commented Aug 21, 2021

View reviewed changes

tsconfig.json Show resolved Hide resolved

navn-r added 2 commits August 22, 2021 10:55

Fix viewing merge conflict diff in notebooks

f153938

Probably fix 'hide unchanged cells' for merge notebooks, i hope?

97829b9

navn-r commented Aug 22, 2021

View reviewed changes

src/components/diff/NotebookDiff.ts Outdated Show resolved Hide resolved

navn-r requested a review from fcollonval August 22, 2021 15:42

fcollonval requested changes Aug 23, 2021

View reviewed changes

Apply suggestions from code review

dbe6985

Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>

navn-r requested a review from fcollonval August 23, 2021 14:13

navn-r changed the title ~~[WIP] Diff and resolve files on merge conflict~~ Diff and resolve files on merge conflict Aug 23, 2021

Remove middle header label

03526ed

Resolve notebook merge conflicts

e4b8012

Fix eslint complaints

2966786

fcollonval requested changes Aug 24, 2021

View reviewed changes

Apply suggestions from code review

41cc8ee

Rename widget accessor + Update doc Co-authored-by: Frédéric Collonval <fcollonval@gmail.com>

navn-r requested a review from fcollonval August 24, 2021 09:30

Remove template from DiffModel

8f2b16d

navn-r force-pushed the feat/diff-merge-conflicts branch from 5466b5f to 8f2b16d Compare August 24, 2021 16:11

fcollonval approved these changes Aug 24, 2021

View reviewed changes

fcollonval merged commit b4876a4 into jupyterlab:master Aug 24, 2021

fcollonval deleted the feat/diff-merge-conflicts branch August 24, 2021 16:12

allcontributors bot mentioned this pull request Sep 2, 2021

docs: add navn-r as a contributor for code #1025

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diff and resolve files on merge conflict #1013

Diff and resolve files on merge conflict #1013

navn-r commented Aug 16, 2021 •

edited

Loading

github-actions bot commented Aug 16, 2021

navn-r left a comment •

edited

Loading

fcollonval commented Aug 22, 2021

navn-r left a comment

fcollonval left a comment

fcollonval commented Aug 23, 2021

navn-r commented Aug 23, 2021

fcollonval commented Aug 23, 2021

fcollonval left a comment

fcollonval commented Sep 2, 2021

allcontributors bot commented Sep 2, 2021

Diff and resolve files on merge conflict #1013

Diff and resolve files on merge conflict #1013

Conversation

navn-r commented Aug 16, 2021 • edited Loading

github-actions bot commented Aug 16, 2021

navn-r left a comment • edited Loading

Choose a reason for hiding this comment

fcollonval commented Aug 22, 2021

navn-r left a comment

Choose a reason for hiding this comment

fcollonval left a comment

Choose a reason for hiding this comment

fcollonval commented Aug 23, 2021

navn-r commented Aug 23, 2021

fcollonval commented Aug 23, 2021

fcollonval left a comment

Choose a reason for hiding this comment

fcollonval commented Sep 2, 2021

allcontributors bot commented Sep 2, 2021

navn-r commented Aug 16, 2021 •

edited

Loading

navn-r left a comment •

edited

Loading