refactor: Refactor git module into vcs-type agnostic module

[zegervdv]

Part 1 of adding support for other VCS'es (#233).

This creates a vcs module, which replaces the git.utils module. Upon requiring it, the vcs type is determined and the right module is returned (currently hardcoded to always return git).

I've changed the Commit class into a base class and made a GitCommit class for git-specific stuff.
The rev module will also need to be split, but this will probably will have to be done similarly to how the vcs module automagically returns the correct implementation.

Still to do:

• handle the rev module
• move common parts from git.utils to vcs.utls (all the job handling etc)

@sindrets is this in line with your expectations?

[sindrets]

EDIT: I started typing out this comment before you pushed the latest changes, but I think most of it still applies.

---

I think we should probably create a VCS adapter interface that all the VCS adapters implement. The VCS module should not return an adapter on require. The appropriate adapter needs to be determined from a set of paths. Which is why i referred to the find_git_toplevel() function previously. I think the way we implement adapter acquisition should be similar to how this function works: we give it an ordered list of paths that might indicate what repository we are part of, and then it goes through the list and checks if any of these paths are part of a repository of any of the supported VCS'.

Making the adapters more object oriented than what the current git module is, is a good call I think. Currently pretty much all the git functions requires a reference to either one, or both of the git top-level and the git dir. These are context variables that I think we can safely assume will not change for the lifetime of a view, and thus we would benefit from storing these in the adapter instances (I don't know if Mercurial uses the equivalent of a git dir, but it necessarily needs the top-level path regardless).

This change is going to require a fair amount of refactoring. We need to instantiate the adapters when we create views in lib.lua, and then pass these adapters to the view constructors. All the subcomponents of a view will acquire the adapter through their parent view.

Refactoring the project to use this model should be the focus of this first PR I think. From there it seems pretty straight forward to implement other VCS adapters.

[zegervdv]

So, instead of local git = require("diffview.git.utils") we do something like:

local vcs = require("diffview.vcs")
local adapter = vcs.get_adapter(path)

once, and pass that created adapter object around to all functions?

For example, in init.lua there are a few calls to git.git_dir. These would then need to be called on the adapter instantiation, I'm not sure it you want all those functions to pass around this object?

[sindrets]

The views are already passing around a GitContext object, because it holds data that is necessary to run any git command. We could just pass an adapter instance instead that would hold this same data, as well as offering the VCS interface. This is actually simpler than what I'm currently doing.

-- Instantiate adapter:
local adapter = require("diffview.vcs").get_adapter(paths)

-- Now the adapter instance would have access to all the necessary context data
-- for the repo derived from the given paths:
adapter.ctx.toplevel
adapter.ctx.git_dir

---

For example, in init.lua there are a few calls to git.git_dir. These would then need to be called on the adapter instantiation, I'm not sure it you want all those functions to pass around this object?

The git stuff in init.lua is all related to offering git completion. This would have to be moved into the adapters regardless. So in these cases the adapter instance approach is simpler as well. Because currently, in all these cases, we're currently deriving both the git top-level and the git dir because this data is a prerequisite to run the necessary git commands.

So instead of doing this:

local toplevel = lib.find_git_toplevel(paths)
if toplevel then
  local git_dir = git.git_dir(toplevel)
  if git_dir then
    git.thing_we_actually_need(toplevel, git_dir)
  end
end

We would do this:

local adapter = vcs.get_adapter(paths)
if adapter then
  adapter.thing_we_actually_need()
end

[zegervdv]

I see, I was missing the scale of the refactoring here :)

So to summarize:

• create a VCSAdapter class which will define the agnostic interface and implement some common functionality (exec_sync etc) now in git.utils and extend with init and lib functions
• implement the GitAdapter with code from git.utils and init/lib
• instantiate the adapter

I'll be pushing some commits whenever I find some time, when I have something worth reviewing I'll ping you again.

[zegervdv]

@sindrets
I think I'm almost there: DiffviewFileHistory and DiffviewOpen seem to work again, as far as the arguments that I tested. But there are so many options, I'm sure I haven't covered all possibilities :).

The merge view also seems to work, but I'm not sure about the cleanest implementation there.
I moved the parse_conflicts to the common vcs.utils because I think most VCS'es use the same format (at least the one without the BASE part, which I believe your implementation also supports?). If needed that could also be moved to the VCSAdapter, but then the actions would need to either take the adapter as an argument, or move to the VCSAdapter too (the latter doesn't seem right).

I found 2 functions that don't seem to be called anywhere:

• get_file_status
• get_file_stats

I've removed these, but maybe you were planning to use these for some new features?

Currently I have a vcs/init.lua and a vcs/utils.lua which are both imported as vcs in different files, I would probably be cleaner to have everything in the init.lua file?

I want to do a final pass through the code to make sure I've updated all old function calls and clean up the annotations.
So in the next couple of days it should be ready for a formal code review.

[sindrets]

@zegervdv Brilliant! I'll see if I can find the time to review this soon.

(at least the one without the BASE part, which I believe your implementation also supports?)

Yes, it does :)

parse_conflicts can remain in utils until there's a reason to move it.

I found 2 functions that don't seem to be called anywhere:

I believe they are just left over from previous refactors, so removing them is probably the right thing to do. But I'll take a closer look.

[sindrets]

Great work!

A lot of the comments I made are requesting changes to address diagnostics. I highly recommend that you use a recent version of the lua-language-server, as that is what's driving the typing system here, and the reason why I'm strict about using accurate annotations.

There was one particularly nasty performance regression to the file history rendering, that was caused by - what I assume was - a left over debug line. Easily addressed of course, but take a look at the comment I left in file_history/render.lua :)

[sindrets]

You defined this API to take a list of paths, but that is not what's passed here. It's also not what the implementation expects in the GitAdapter. Suggestion: make the implementation handle lists of paths.

[sindrets]

Same as add_file.

[sindrets]

reset_file: If this API should support no arguments; this needs to be reflected in its annotations.

[zegervdv]

Thanks for the thorough review, I think I fixed all of the points, but those I wasn't sure about I left unresolved.

After properly setting up lua-language-server (I probably had a old or broken install) I am still seeing some diagnostic warnings in vcs/init.lua at line 36. I'll have a look at that later.

[zegervdv]

I think I've cleaned up the last diagnostic issues and expanded the type definitions for the flags and ctx a bit more.

I've updated the implementation for add_file and reset_file (renamed them to plural, since that makes more sense).

[sindrets]

Great! I updated the API for plugins that integrate diffview (currently only Neogit), and I made a few other small changes. I'll have to give the Neogit guys a heads up because we have now changed one of the require paths that they use. Other than that, I reckon this PR is ready!

I'm gonna test a few more things over the weekend to make sure that everything is in order, just because this is such a big refactor.

[zegervdv]

Alright, thanks!

I made a lot of intermediate commits, do you want me to push a single squashed commit or will you do that via the merge?

I ran stylua on the code, but noticed that a lot of files/regions I did not touch also changed. I'm using stylua 0.15.1. You want me to only format the files this PR touches or just leave it as is?

[sindrets]

I made a lot of intermediate commits, do you want me to push a single squashed commit or will you do that via the merge?

Don't worry about it; I squash all PR's on merge.

I ran stylua on the code, but noticed that a lot of files/regions I did not touch also changed. I'm using stylua 0.15.1. You want me to only format the files this PR touches or just leave it as is?

Yeah, I haven't been using stylua in a while. I don't really like any of the lua formatters I've tried. It particularly bothers me how stylua will collapse multiple lines into one when I've explicitly broken it into multiple lines for better readability. And to my knowledge there's no way around this other than to litter your code with -- stylua: ignore. And at that point I feel like the effort of circumventing the formatter outweighs its utility.

Sorry about the rant. TLDR: Don't worry about it, just leave it as is :)

---

In my testing over the weekend the only issues I found were with cases of passing invalid options to the commands. We should handle these cases gracefully instead of waiting until the program runs into a runtime error.

[sindrets]

Alright, let's merge this :)

Thank you for your hard work!

[zegervdv]

Thanks for being so supportive! It was a bit daunting at first, doing such a refactor on someone else’s code :).

I’ve already started implementing the hg adapter so I hope to have a next PR in a couple of weeks.