Update open files when Git commands modify them

[ianhi]

Close: #668

If any files are open in jupyterlab that are modified by:

• reset to commit
• revert commit
• switch branch
• discard changes

then they will be updated to reflect the new contents. Unless they have unsaved changes, in which case the open document will not be modified.

Approach

Use the changed_files command to get the list of files changed by a git command and then revert those files if they are opened.

I didn't just loop over all open documents for two reasons:

1. I didn't think of doing that that until I started writing this just now
2. (Post hoc) This is is more parsimonious - if there are large notebooks that are unaffected by the Git command then it may be slow to reload them

Tests

I updated the tests to mock the changed_files endpoint and mocked the docmanager. While my mock of the docmanager seems to work I'm not sure it's the best way to approach the mock. But unfortunately I'm not sure how I ought to do it differently.

In action

Swapping branches

Discarding changes

Reverting to a commit

Reverting a specific commit

[ianhi]

I'm working on a rebase of this post #630 and have a quick question for either @fcollonval or @kgryte. Should the code that reverts the file should be inside the modal's try block? It seems to me the answer is yes, however if I do that then I will need to put the check on response.ok inside the try block which it looks as though @kgryte avoided. Is it a misuse of the try..finally to have return statements and error throwing inside the try block? For example I would be changing the code to look like this:

    try {
      const files = (await this.changedFiles(null, null, commitId))['files'];
      const response = await httpGitRequest('/git/delete_commit', 'POST', {
        commit_id: commitId,
        top_repo_path: path
      });
      if (!response.ok) {
        return response.json().then((data: any) => {
          throw new ServerConnection.ResponseError(response, data.message);
        });
      }
      if (files) {
        files.forEach(file => {
          this._revertFile(file);
        });
      }
      await this.commit(message);
      return response;
    } catch (err) {
      throw new ServerConnection.NetworkError(err);
    } finally {
      this._removeTask(tid);
    }

vs this:

jupyterlab-git/src/model.ts

Lines 624 to 640 in 4c3dbe2

	const tid = this._addTask('git:commit:revert');
	try {
	response = await httpGitRequest('/git/delete_commit', 'POST', {
	commit_id: hash,
	top_repo_path: path
	});
	} catch (err) {
	throw new ServerConnection.NetworkError(err);
	} finally {
	this._removeTask(tid);
	}
	if (!response.ok) {
	const data = await response.json();
	throw new ServerConnection.ResponseError(response, data.message);
	}
	await this.commit(message);
	return response;

[kgryte]

@ian-r-rose I recommend only wrapping what is absolutely necessary in a try/catch block. Or stated otherwise, the statement (here, a network request) which could potentially throw an error. This simplifies error handling and mitigates the potential for conflated errors.

In the above, including substantial code blocks (i.e., many lines of code) increases the risk of trapping unintentional errors, such as syntax errors, etc, in which the application continues to run as normal due to the trapping or even responds with an erroneous error message making debugging more difficult.

[ianhi]

@kgryte thanks for the help.

Or stated otherwise, the statement (here, a network request) which could potentially throw an error. This simplifies error handling and mitigates the potential for conflated errors.

Do you mean that reverting the files should be outside the try block? That seems problematic because reverting files should (I think) be hidden behind the modal to prevent further user modifications while files are being modified. Alternatively I could move the revertFiles call outside of the try..finally and give it it's own modal, but then there would end up being two modal's spawned which does not seem ideal.

[kgryte]

Ah, right. You could just move the _removeTask statement outside of the finally block. Sorry for not addressing the thrust of your question. Meaning, if the modal needs to bridge multiple actions, just ensure that those actions happen between the addTask and removeTask statements. How you want to individually wrap the statements in-between is up to you. You can individually wrap them in try/catch blocks and then removeTask in consolidated error handling logic.

[ianhi]

You could just move the _removeTask statement outside of the finally block

Ah ok that sounds reasonable. I was worried that it was important to have removeTask in a finally block to make sure the modal is removed, but maybe that is not necessary?.

[kgryte]

Nope. :) Just "bracket" the operations in which the modal should be present. For many of the operations for which I added the modal, there was only one operation for which we needed to show the modal. Hence, our ability to use finally. The main thing is that, during error handling, if, say, the first operation fails, you may need to early return and perform clean-up in the catch clause. In which case, you may have multiple removeTask statements, depending on how you handle failures.

[fcollonval]

A small note about future change. I plan to push a proposal to handle connection and response error directly within httpGitRequest. This will ease readability of the model code and remove plenty of duplicate statements.

Related to that @kgryte, would it make sense to create decorator kind of method to handle the _addTask/_removeTask:

executeTask(name, fn, ...args) {
this._addTask(name);
  try{
    fn(...args)
  } finally {
    this._removeTask(name)
  }
}

That will simplify the code and improve its robustness. But in light of the problem expose here. It could be better to have:

executeTask(name, fnAndArgs[]: Array<{fn, args[]}>) {
this._addTask(name);
  try{
    fnAndArgs.forEach({fn, args} => { fn(..args) });
  } finally {
    this._removeTask(name)
  }
}

[kgryte]

Personally, I don't see the need for using decorators, particularly when I may want exact control over when tasks are added and removed from the queue.

Pushing some of the error handling logic into httpGitRequest seems fine to me, so long as we have a mechanism for bubbling errors up in to UI components (e.g., global event listeners, etc).

[fcollonval]

LGTM Let's merge this

[fcollonval]

@meeseeksdev backport to 0.11.x

[lumberbot-app]

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

1. Checkout backport branch and update it.

$ git checkout 0.11.x
$ git pull

2. Cherry pick the first parent branch of the this PR on top of the older branch:

$ git cherry-pick -m1 88acc03f9910957657eed743f698c02538be850b

3. You will likely have some merge/cherry-pick conflict here, fix them and commit:

$ git commit -am 'Backport PR #676: Update open files when Git commands modify them'

4. Push to a named branch :

git push YOURFORK 0.11.x:auto-backport-of-pr-676-on-0.11.x

5. Create a PR against branch 0.11.x, I would have named this PR:

"Backport PR #676 on branch 0.11.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.