Skip to content

Conversation

@zegervdv
Copy link
Collaborator

@zegervdv zegervdv commented Nov 23, 2022

Current status:

  • DiffviewFileHistory basic list
  • FileHistory add all options
  • DiffviewOpen diff view
  • DiffviewOpen merge conflicts
  • documentation
  • CommitLogPanel

This PR includes a couple of fixes and refactors and adds a new hierarchy to the config (splitting options between hg and git).
I will create a separate PR for these soon, so they can be merged in first with the necessary deprecation logic and migration documentation. Those commits are not the intended focus of this PR.

I already wanted to open this one to ask some questions about the implementation.

As stated above, I have the file history and basic diffview working for mercurial repos, but I'm running into an issue when encountering a merge-conflicted file.
In git there is a single command (created via GitAdapter:get_namestat_args) which lists both modified and conflicted files.
For mercurial, these are two separate commands (hg status -mard and hg resolve --list).
I'm still looking for a command or combination of options that might give me the same result from a single command, but chances are it is not possible.

So, I propose to make a second job with a second args command, e.g. VCSAdapter:get_unresolved_args. If this returns nil, like will be the case for git, the job will not run and all remains the same as it is now.
If it does return a list of arguments, it will run and the results of both jobs (namestat and unresolved) is merged and parsed as if it was a single command.
Does that sound like a reasonable approach or do you know a better/cleaner way?

@sindrets
Copy link
Owner

I think it would make more sense to move the implementation of tracked_files() and untracked_files() into the adapters. It's natural that these would have a slightly different implementation for different vcs. This way you can use any number of jobs you need for mercurial (although it should be kept to a minimum to reduce I/O overhead).

@zegervdv
Copy link
Collaborator Author

@sindrets
I think I have most things working now, maybe just need to add some completions and some of the lesser used flags.

I'll test this for a while to cover all the corner cases.

I noticed that hg is quite a bit slower in generating the file history information: on a repo of ~50K commits it takes about 15 to 20 seconds to list 256 commits. Since hg is a python script, each invocation to the command takes a performance hit when it starts the python interpreter. There are some options to improve it: for example there is a experimental chg command which starts up much faster (which gets the file history down to about 5 seconds for the previously mentioned testcase).
Another option is to start hg as a command server (so it starts only once) and send commands to it. Implementing this seems a much bigger change :).

For maximum compatibility, I will keep the default config.hg_cmd as hg, but mention in the docs that a speed up can be achieved by substituting it with chg.

How would you prefer handling the docs?
Currently most of it is written with git commands in mind, which will probably still be 99.8% of the users after this is merged :).
So with that in mind I think a separate section is probably the better approach?

@sindrets
Copy link
Owner

@zegervdv Great work!

on a repo of ~50K commits it takes about 15 to 20 seconds to list 256 commits

Ouch, that's pretty bad 😅. Does the history size matter for the performance when you're only querying n commits? Are you passing the appropriate flags to limit the output? If this can't easily be remedied I think it's best to quite drastically lower the default commit limit for hg. While we do make sure to kill jobs if a history view is closed during an update, and we do make sure that jobs are non-blocking; lua coroutines are still single threaded, and the editor isn't exactly super responsive during file-history updates. To have the editor be like that for almost half a minute is far from ideal.

Another option is to start hg as a command server (so it starts only once) and send commands to it. Implementing this seems a much bigger change :).

This sounds interesting. If hg has a daemon like you describe, it sounds like the appropriate way to use it in a third party tool like this where we are making up to hundreds of calls to the VCS. But I also agree that it shouldn't be a blocker to get this PR merged. Just something to explore for the future.

How would you prefer handling the docs?

For the commands, we can just have one section of options for each VCS tool. I'll push a commit where I've started to amend the docs to show what I mean, and then you can just fill it out with descriptions of the different options you've implemented :)

@sindrets
Copy link
Owner

@zegervdv If you would like to, I'll make you a collaborator on the project after we've merged this. That way you can more effectively maintain the hg part of the code when you have time :)

@zegervdv
Copy link
Collaborator Author

I did a couple of tests on different repos and it seems the total number of commits in the repo is not a (huge) factor.

The limitation to 256 works, I can see the counter increment as they come in and setting it to a lower value also confirms that I get the right number of lines.
So perhaps reducing it to 64 as a default makes sense.

If hg has a daemon like you describe, it sounds like the appropriate way to use it in a third party tool

Yes, the vscode plugin for mercurial has it as an optional feature, so I'm not sure how easy/stable it is.
But it could be a fun exercise for later.

Definitively if the new sapling-scm also has it and would become a new major player.

Thanks for the reviews, there are so many features in this plugin I haven't even discovered them all, let alone had a chance to use them :)

@zegervdv If you would like to, I'll make you a collaborator on the project after we've merged this. That way you can more effectively maintain the hg part of the code when you have time :)

That would be nice, thanks for the trust :)

@zegervdv zegervdv marked this pull request as ready for review January 17, 2023 17:42
@zegervdv zegervdv requested a review from sindrets February 16, 2023 21:07
Copy link
Owner

@sindrets sindrets left a comment

Choose a reason for hiding this comment

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

https://github.com/zegervdv/diffview.nvim/blob/673b58fe1a0b13f4a4fdcca5166ac41a2156c5c9/lua/diffview/scene/views/diff/diff_view.lua#L128

This line should probably have an added condition of:

if --[[ ... ]] and self.adapter:instanceof(GitAdapter.__get()) then

This PR seems almost good to go! If you address the few things I mentioned here, I think I'd be ready to merge :)

@zegervdv
Copy link
Collaborator Author

Fixed the last comments. I think it is ready :)

Copy link
Owner

@sindrets sindrets left a comment

Choose a reason for hiding this comment

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

LGTM, let's get this merged!

@sindrets sindrets changed the title Implement mercurial (hg) adapter feat: Implement mercurial (hg) adapter Feb 22, 2023
@sindrets sindrets merged commit 503837d into sindrets:main Feb 22, 2023
@sindrets
Copy link
Owner

@zegervdv I've also sent you an invite to become a collaborator on the project :)

@zegervdv
Copy link
Collaborator Author

Thanks!

@zegervdv zegervdv mentioned this pull request Feb 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants