Separate git and add vcs choosing #436

kuznetsss · 2022-01-05T16:57:39Z

Hi! I've refactored the plugin to separate git related functionality and make it possible to add other VCS support easily and without affecting other parts of the plugin. In the company I'm working we are using custom VCS with git-like CLI. I'm not allowed to publish any patches adding support for our VCS but I really want to use your plugin at work. After my refactoring it would be much easier to add and maintain support of the our VCS without affecting the plugin itself.

I think it is good to rename all things called git-something to vcs-something. So please tell me your opinion about the PR and I will make renaming if you would be ok with the PR

lewis6991 · 2022-01-05T17:47:03Z

teal/gitsigns/cache.tl

@@ -14,7 +14,7 @@ local record M
    staged_diffs  : {Hunk}
    pending_signs : {integer:Sign}
    gitdir_watcher : vim.loop.FSPollObj -- Timer object watching the gitdir
-    git_obj       : GitObj
+    git_obj       : VcsObj


We should rename git_obj too.

lewis6991

Firstly, well done for endeavouring this change. Generally it looks pretty good.

Support for other VCS's is a non-goal of Gitsigns simply because I personally don't use any VCS other than git. Therefore, I would only feel comfortable accepting this change if:

You put yourself forward as a contributor who will support this going forward. Obviously I will help too, but opening the plugin up to other VCS will increase the work required to maintain it.
We add at least one more VCS with basic support. I don't care which, just something to demonstrate the viability of the refactoring. It's one thing to pull out the logic, so it's agnostic across a common interface, but this will inevitably lead to edge cases situations that question certain parts of the overall architecture.

lewis6991 · 2022-01-05T17:48:24Z

teal/gitsigns/diff_ext.tl

@@ -57,7 +57,11 @@ M.run_diff = function(
  -- We can safely ignore the warning, we turn it off by passing the '-c
  -- "core.safecrlf=false"' argument to git-diff.

-  local out = git.command{
+  local active_vcs = vcs.vcs_for_path(file_cmp)


file_cmp is just a temp file, so we won't be able to determine the vcs from it.

lewis6991 · 2022-01-05T17:50:23Z

teal/gitsigns/diff_ext.tl

+  if not active_vcs then
+    return results
+  end
+  local out = active_vcs.command{
    '-c', 'core.safecrlf=false',
    'diff',


We should factor this out into a git specific file and use regular diff as a fallback.

lewis6991 · 2022-01-05T17:53:55Z

teal/gitsigns/vcs_interface.tl

+
+  record Repo
+    toplevel   : string
+    gitdir     : string


Maybe we should rename this. Do other vcs have analogues?

lewis6991 · 2022-01-05T17:56:32Z

teal/gitsigns/vcs.tl

@@ -0,0 +1,24 @@
+local Vcs = require('gitsigns.vcs_interface').new_vcs()


We should create a vcs source directory and put this, the interface and the git stuff inside.

kuznetsss · 2022-01-05T18:45:18Z

Thank you for reviewing.

You put yourself forward as a contributor who will support this going forward. Obviously I will help too, but opening the plugin up to other VCS will increase the work required to maintain it.

Yeah, sure, I'm ok with it.

We add at least one more VCS with basic support. I don't care which, just something to demonstrate the viability of the refactoring. It's one thing to pull out the logic, so it's agnostic across a common interface, but this will inevitably lead to edge cases situations that question certain parts of the overall architecture.

I agree that there should be a proof that the refactoring is useful. But I'm using only git for personal projects and I don't have any experience with any other open source VCS. I definitely will test the refactoring with closed source VCS at work but I can't publish patches (I will ask for publishing permissions one more time in a week because it's holidays here now). We could even don't merge this until I check it at work with another VCS. And if one day someone would like to add support for any other VCS I will help. Is it ok? Or adding support of any other VCS is a strict requirement to merge this PR?

lewis6991 · 2022-01-06T10:02:33Z

Thought about this overnight and I'm a little torn on what to do.

I can't publish patches (I will ask for publishing permissions one more time in a week because it's holidays here now)

If the VCS you use is private, then there is no need to add integration to it here.

We could even don't merge this until I check it at work with another VCS. And if one day someone would like to add support for any other VCS I will help. Is it ok? Or adding support of any other VCS is a strict requirement to merge this PR?

This sounds like a good idea.

It would also be good to get a gauge what the appetite for other VCS's is. From my POV git is pretty ubiquitous and accounts for a vast majority of the VCS market share, meaning work done on this plugin benefits a lot of people. For other VCS's, the return-on-investment is much smaller.

Before we make a decision on what to do, I'd like to keep this PR open for a while and iterate on it a bit. At the moment it doesn't really provide any benefit to anyone (other than yourself) and it will make adding more git-specific features a little harder.

kuznetsss · 2022-01-06T10:39:36Z

At the moment it doesn't really provide any benefit to anyone (other than yourself) and it will make adding more git-specific features a little harder.

That's true. But I think code became a little better because now all logic about git is at one place. Maybe you have any ideas how to make this PR more useful for plugin?

Also another idea is to make choosing VCS under option and use only git by default. What do you think?

lewis6991 · 2022-01-06T10:51:49Z

That's true. But I think code became a little better because now all logic about git is at one place. Maybe you have any ideas how to make this PR more useful for plugin?

It's better in some regards but more effort to maintain as VCS stuff needs to go over an agnostic interface. Also some VCS' are drastically different so the interface might be wrong. If we want to add new features we need to be careful not to break other VCS's whereas at the moment we can always assume the VCS is git.

For example this PR (#339) which adds some github integration would be more difficult to write as github only works with git.

There's not much we can do about this, it will just be harder, so we need to ensure the added difficulty is worth it.

Also another idea is to make choosing VCS under option and use only git by default. What do you think?

If we're are going to support other VCS's then I think auto-detection is required, but maybe by default git is the only enabled VCS, otherwise the auto-detect might take a while if it loops through all supported VCS's

kuznetsss · 2022-01-06T11:53:12Z

There's not much we can do about this, it will just be harder, so we need to ensure the added difficulty is worth it.

Now I see that maybe such changes is too much. Because adding multiple VCS support may be a problem and it is not necessary.

I suggest to drop this PR. I could create another PR with small refactoring. The private VCS I use at work has git-like CLI so I think small refactoring to put all git logic to one place would be enough for me.

kuznetsss · 2022-01-07T12:30:11Z

Created small refactoring PR #437 instead of this one.

kuznetsss added 2 commits January 4, 2022 17:15

Move in_git_dir to git file

481609f

Switch to vcs choosing

66d2c1a

lewis6991 reviewed Jan 5, 2022

View reviewed changes

lewis6991 requested changes Jan 5, 2022

View reviewed changes

kuznetsss closed this Jan 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate git and add vcs choosing #436

Separate git and add vcs choosing #436

kuznetsss commented Jan 5, 2022 •

edited

Loading

lewis6991 Jan 5, 2022

lewis6991 left a comment

lewis6991 Jan 5, 2022

lewis6991 Jan 5, 2022

lewis6991 Jan 5, 2022

lewis6991 Jan 5, 2022

kuznetsss commented Jan 5, 2022

lewis6991 commented Jan 6, 2022

kuznetsss commented Jan 6, 2022

lewis6991 commented Jan 6, 2022

kuznetsss commented Jan 6, 2022

kuznetsss commented Jan 7, 2022

		@@ -0,0 +1,24 @@
		local Vcs = require('gitsigns.vcs_interface').new_vcs()

Separate git and add vcs choosing #436

Separate git and add vcs choosing #436

Conversation

kuznetsss commented Jan 5, 2022 • edited Loading

lewis6991 Jan 5, 2022

Choose a reason for hiding this comment

lewis6991 left a comment

Choose a reason for hiding this comment

lewis6991 Jan 5, 2022

Choose a reason for hiding this comment

lewis6991 Jan 5, 2022

Choose a reason for hiding this comment

lewis6991 Jan 5, 2022

Choose a reason for hiding this comment

lewis6991 Jan 5, 2022

Choose a reason for hiding this comment

kuznetsss commented Jan 5, 2022

lewis6991 commented Jan 6, 2022

kuznetsss commented Jan 6, 2022

lewis6991 commented Jan 6, 2022

kuznetsss commented Jan 6, 2022

kuznetsss commented Jan 7, 2022

kuznetsss commented Jan 5, 2022 •

edited

Loading