vimPlugins: Automate git commits when updating.

[ryneeverett]

Motivation for this change

This is a followup and continuation of the work and discussion in #83119. Highlights:

• A new --commit flag eliminates the need for a multi-step process for updates and updating redirections.
• A new --add argument eliminates the need for a multi-step process for updates and adding packages and also eliminates the need for manually editing vim-plugin-names.
• A minor bugfix in the deprecation script.

Things done

• Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
• Built on platform(s)
  • NixOS
  • macOS
  • other Linux distributions
• Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
• Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
• Tested execution of all binary files (usually in ./result/bin/)
• Determined the impact on package closure size (by running nix path-info -S before and after)
• Ensured that relevant documentation is up to date
• Fits CONTRIBUTING.md.

[ryneeverett]

@timokau

[ryneeverett]

The roadmap I envision going forward is to break out the --add functionality so a single package can be added without updating the whole package set. I suspect this will be a large diff so I'd rather hold off for another PR and just automate the current process in this PR. Once we have a bot doing updates contributors would then be able to do single-commit PR's to add packages. My premise is that the processes of updating and adding packages should be completely separated.

[timokau]

Went through the commits in order and added my thoughts inline. Probably went a bit overboard with the nitpicky-ness, you don't need to agree everywhere of course.

Thanks for continuing to work on this! I'm worrying about the increasing complexity of this "little helper" a bit, but I do think the changes are worth it.

[timokau]

We can get rid of NIXPKGS_PATH by using git.Repo(".", search_parent_directories=True).

[ryneeverett]

I would think if we used ROOT rather than . we could avoid the assumption that the current working directory is vim-plugins.

[timokau]

Good idea, that is better.

[timokau]

This may be confusing to a git novice. How about "You have uncommitted changes. Please commit your changes or stash them before running this tool."

[timokau]

Is this type annotation necessary? Or is it common practice? I don't have much experience with mypy.

[ryneeverett]

No idea, but I didn't intentionally make this change so I'll revert it. This is my first experience with mypy and I must say it's been great and saved a lot of time.

[timokau]

Similar to the discussion in #83119, I think we should just do this by default. We could keep a --no-commit flag, but I don't think its necessary.

[timokau]

I think files should be an explicit list instead.

[ryneeverett]

I knew you would and changed it in a subsequent commit.

[timokau]

It would be nice to mention that this has to be hosted on github.

[timokau]

As a minor pet-peeve of mine, I wish we had a standard on capitalization after :. Lower-case seems more prevalent, so I would prefer that. Again, I don't feel very strongly about this though.

[ryneeverett]

Yeah, I was going with the precedent in vim.section.md but now that we're in a position to correct this I'll do so because I think nixpkg's CONTRIBUTING.md should be the global standard.

[timokau]

Something more explicit might be nice here, such as resolve github repository redirects.

[timokau]

Why commit OVERRIDES?

[timokau]

I'd prefer to give this function a name, such as updater.update(), to be more explicit (PluginUpdater doesn't seem "callable" to me).

[timokau]

The roadmap I envision going forward is to break out the --add functionality so a single package can be added without updating the whole package set. I suspect this will be a large diff so I'd rather hold off for another PR and just automate the current process in this PR. Once we have a bot doing updates contributors would then be able to do single-commit PR's to add packages. My premise is that the processes of updating and adding packages should be completely separated.

That would be nice. I think it would be even better to save every plugin in a separate file. That way we wouldn't have to worry about merge conflicts any more (though your proposal would already help with that).

One downside is that we currently get semi-regular updates "for free" whenever someone adds a plugin. But that could be done better with appropriate automation.

[ryneeverett]

Also I want to say that I think the contribution, code and git commits are of very high quality, which might get lost in my nitpickyness here.

I'd like to say that the review has been of very high quality which might get lost in my defensive responses to many of the review comments. :)

[timokau]

Why did you change this?

[ryneeverett]

I believe it had something to do with mypy but it doesn't seem necessary any more. I'll try reverting it.

[Mic92]

Should this one not be catched by mypy?

Suggested change

	user: str, repo_name: str, alias: str, cache: "Cache" = None
	user: str, repo_name: str, alias: str, cache: "Optional[Cache]" = None

[ryneeverett]

For some reason mypy doesn't seem to care whether this is done for optional arguments but I'll go and make this change.

[timokau]

Everything else looks good to me, but this needs a rebase because of #83798 and there's the outstanding issue with the dirty/master check.

[ryneeverett]

I agree with you to a certain extent about the dirty/master checks and at the very least the flag should be renamed.

I'm concerned that this auto-committing behavior is going to be a footgun for people who just want to add a damn package. For example I would probably run this script on whatever branch I have checked out at the time, stash the changes, and checkout a new branch to make a PR. The git experts are going to figure out what happened (eventually) and be able to rebase --onto X Y Z but the majority are going to be confused and frustrated.

I think we should be particularly conscious of this kind of user in our interface choices, not only because we'll be fielding their questions and PR's, but because vim configuration in nixos is already uniquely complicated compared to any other operating system. Would it be too much to ask those of us with advanced use cases to pass a --force/-f flag?

[timokau]

black probably dis something funny here.

[timokau]

I agree with you to a certain extent about the dirty/master checks and at the very least the flag should be renamed.

I'm concerned that this auto-committing behavior is going to be a footgun for people who just want to add a damn package. For example I would probably run this script on whatever branch I have checked out at the time, stash the changes, and checkout a new branch to make a PR. The git experts are going to figure out what happened (eventually) and be able to rebase --onto X Y Z but the majority are going to be confused and frustrated.

That's a good point I hadn't thought about. The current solution is not only inconvenient though, its also not very reliable. For example, I never use a local tracking branch for master. I just use the remote upstream/master directly.

$ git show master
fatal: ambiguous argument 'master': unknown revision or path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'

So I would still rather remove those checks. Maybe print a warning that commits were created on the current branch. If there was a mistake, there's no complicated rebasing necessary. The contributor can just repeat the changes on the appropriate branch. If you think this is necessary, you could also ask for confirmation interactively (with a flag to disable this).

And the other part of the issue, the dirty tree, is not really affected by this. Is there any downside to only checking the relevant files for "dirt"?

I think we should be particularly conscious of this kind of user in our interface choices, not only because we'll be fielding their questions and PR's, but because vim configuration in nixos is already uniquely complicated compared to any other operating system. Would it be too much to ask those of us with advanced use cases to pass a --force/-f flag?

Keep in mind that this complexity is opt-in though. Users can still use and configure vim as they do on any other distro, including plugin management. We should still try to make the experience as frictionless as possible of course.

[ryneeverett]

The particular scenario you describe could be worked around:

    try:
        if not allow_dirty and (
            repo.head.ref == repo.heads.master
            or repo.head.commit != repo.heads.master.commit
        ):
        raise Exception(
            "Please create a new branch based off master before running this tool."
        )
    except AttributeError:
        raise Exception(
            "For advanced use cases, please pass the --allow-dirty flag."
        )

If there was a mistake, there's no complicated rebasing necessary. The contributor can just repeat the changes on the appropriate branch.

They would also need to reset HEAD on the branch they were on.

And the other part of the issue, the dirty tree, is not really affected by this. Is there any downside to only checking the relevant files for "dirt"?

My thinking is that a dirty tree is indicative of being unprepared to commit. If you checked out a fresh branch, why would the tree be dirty?

My thinking here is that "indicative" is good enough. IMO, if these checks are helpful (or benign) for the vast majority of use cases, making the minority pass a flag to bypass them is acceptable. Any kind of safety device is a trade off that is somewhat annoying to "power users".

[jonringer]

My thinking is that a dirty tree is indicative of being unprepared to commit. If you checked out a fresh branch, why would the tree be dirty?

I agree. If I did have unstaged changes, it was likely something else i was working on and not related to running the plugin update script

[timokau]

The particular scenario you describe could be worked around:

How does this work around it? Or do you mean that the workaround is telling the user to use --alow-dirty? I wouldn't consider my use case advanced, I think its quite common (for a read-only branch its more convenient to just use the remote directly).

They would also need to reset HEAD on the branch they were on.

True. Or they could use the XKCD approach: https://xkcd.com/1597/

And the other part of the issue, the dirty tree, is not really affected by this. Is there any downside to only checking the relevant files for "dirt"?

My thinking is that a dirty tree is indicative of being unprepared to commit. If you checked out a fresh branch, why would the tree be dirty?

Uncommitted changes are preserved when checking out a fresh branch. I might have some unstaged package update that I haven't gotten around to committing yet. I don't see much value in this check. Its not a very reliable indicator of "not being on the correct branch" and I do see it as being annoying often enough. Though it seems I'm outvoted on this one, and I don't feel that strongly about it as long as there's a short -f flag to get around it. So I won't insist.

My thinking here is that "indicative" is good enough. IMO, if these checks are helpful (or benign) for the vast majority of use cases, making the minority pass a flag to bypass them is acceptable. Any kind of safety device is a trade off that is somewhat annoying to "power users".

There's a tradeoff here though. I think the indicator is unreliable, the benefit is small and we can expect some git competence from committers here. Resetting a branch is very basic. They'll have to find a way to do it eventually anyway. At the same time I'm pretty sure I would've unnecessarily tripped up these safety checks several times already.

The other advantage is that it makes the --allow-dirty flag unnecessary for infrastructure development.

[ryneeverett]

I wouldn't consider my use case advanced, I think its quite common (for a read-only branch its more convenient to just use the remote directly)

If it's common then it could be more explicitly supported by the check.

True. Or they could use the XKCD approach: https://xkcd.com/1597/

Yeah, that's best avoided with 200,000+ commit projects.

Uncommitted changes are preserved when checking out a fresh branch. I might have some unstaged package update that I haven't gotten around to committing yet.

Interesting. I never intentionally do this this myself but perhaps git just supports too many workflows for checks like these to have high indicative value.

[timokau]

I wouldn't consider my use case advanced, I think its quite common (for a read-only branch its more convenient to just use the remote directly)

If it's common then it could be more explicitly supported by the check.

Then you'd have to worry about identifying the correct remote. There may be multiple to choose of. For example I have upstream and timokau.

True. Or they could use the XKCD approach: https://xkcd.com/1597/

Yeah, that's best avoided with 200,000+ commit projects.

Tongue-in-cheek of course.

Uncommitted changes are preserved when checking out a fresh branch. I might have some unstaged package update that I haven't gotten around to committing yet.

Interesting. I never intentionally do this this myself but perhaps git just supports too many workflows for checks like these to have high indicative value.

Yes, in my opinion that is the case.

[ryneeverett]

Ok, I'm convinced. The checks are removed. I don't see much value in checking just the committed files status as that doesn't seem like that probable of an issue to have. I can squash the review commits if desired.

[timokau]

Great, looks good to me! Yes I think squashing would be good. Some history can (and should) be preserved, but everything that changed due to review can be squashed. For example, I think the --add commit should remain separate from the auto-commit one.

[timokau]

There's still one commit that only contains review changes. Is that intentional? If so, that's also fine by me.

[ryneeverett]

Yes, it was intentional because I don't know how to rebase it into the other commits in a reasonable way.

[timokau]

If git was the issue, this might help: #82586 (comment) It would probably be a hassle though, and also kind of a shame to waste your well-written commit message.

Thank you for sticking with this and responding so well to feedback.

[timokau]

If you need a review in the future or you have reviewed a PR that is ready for merge, feel free to ping me. No guarantees of course.