vimPlugins: Automate git commits when updating. #83788

ryneeverett · 2020-03-30T16:39:07Z

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

ryneeverett · 2020-03-30T17:18:43Z

ryneeverett · 2020-03-30T19:05:45Z

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 · 2020-03-30T19:13:47Z

pkgs/misc/vim-plugins/update.py


    return parser.parse_args()


-def main() -> None:
+def get_nixpkgs_repo() -> git.Repo:
+    repo = git.Repo(NIXPKGS_PATH)


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

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

Good idea, that is better.

timokau · 2020-03-30T19:17:57Z

pkgs/misc/vim-plugins/update.py

+def get_nixpkgs_repo() -> git.Repo:
+    repo = git.Repo(NIXPKGS_PATH)
+    if repo.is_dirty():
+        raise Exception("Please stash your changes before updating.")


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 · 2020-03-30T19:18:44Z

pkgs/misc/vim-plugins/update.py


+def main() -> None:


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

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 · 2020-03-30T19:20:27Z

doc/languages-frameworks/vim.section.md

@@ -263,7 +263,7 @@ Sometimes plugins require an override that must be changed when the plugin is up

 To add a new plugin:

-  1. run `./update.py` and create a commit named "vimPlugins: Update",
+  1. run `./update.py --commit`,


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 · 2020-03-30T19:22:12Z

pkgs/misc/vim-plugins/update.py

@@ -413,6 +416,11 @@ def generate_nix(plugins: List[Tuple[str, str, Plugin]], outfile: str):
    print(f"updated {outfile}")


+def commit_changes(repo: git.Repo, *files: Path):


I think files should be an explicit list instead.

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

timokau · 2020-03-30T19:38:13Z

pkgs/misc/vim-plugins/update.py

+        "-a",
+        dest="add_plugins",
+        action="append",
+        help="Plugin to add to vimPlugins in the form owner/repo",


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

timokau · 2020-03-30T19:40:49Z

pkgs/misc/vim-plugins/update.py

    rewrite_input(args.input_file, redirects)

    if args.commit:
-        nixpkgs_repo.commit("Update", [args.outfile])
+        nixpkgs_repo.commit("vimPlugins: Update", [args.outfile])


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.

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 · 2020-03-30T19:42:23Z

pkgs/misc/vim-plugins/update.py

        if redirects:
            update_plugins(args.input_file, args.outfile, cache)
            nixpkgs_repo.commit(
-                "Update redirects", [args.outfile, args.input_file, DEPRECATED]
+                "vimPlugins: Update redirects",


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

timokau · 2020-03-30T19:44:12Z

pkgs/misc/vim-plugins/update.py

+                "vimPlugins.{name}: init at {version}".format(
+                    name=plugin.normalized_name, version=plugin.version
+                ),
+                [args.outfile, args.input_file, OVERRIDES],


Why commit OVERRIDES?

timokau · 2020-03-30T19:47:40Z

pkgs/misc/vim-plugins/update.py

-        results = pool.map(prefetch_with_cache, plugin_names)
-    finally:
-        cache.store()
+    def __call__(self) -> Dict:


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 · 2020-03-30T19:54:42Z

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 · 2020-03-31T00:59:43Z

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 · 2020-03-31T09:50:54Z

pkgs/misc/vim-plugins/update.py

    name, repo = line.split("/")
    try:
        repo, alias = repo.split(" as ")
        return (name, repo, alias.strip())
    except ValueError:
        # no alias defined
-        return (name, repo.strip(), None)
+        return (name, repo.strip(), "")


Why did you change this?

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

Mic92 · 2020-03-31T09:57:23Z

pkgs/misc/vim-plugins/update.py

@@ -218,16 +220,17 @@ def get_current_plugins() -> List[Plugin]:


 def prefetch_plugin(
-    user: str, repo_name: str, alias: str, cache: "Cache"
+    user: str, repo_name: str, alias: str, cache: "Cache" = None


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

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 · 2020-03-31T16:40:29Z

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 · 2020-03-31T17:16:11Z

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 · 2020-03-31T18:17:23Z

pkgs/misc/vim-plugins/update.py

+        or repo.head.commit != repo.heads.master.commit
+    ):
+        raise Exception(
+            "Please create a new branch based off master before running " "this tool."


black probably dis something funny here.

timokau · 2020-03-31T18:23:49Z

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 · 2020-03-31T20:12:51Z

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 · 2020-03-31T20:18:45Z

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 · 2020-03-31T20:35:58Z

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 · 2020-03-31T20:54:20Z

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 · 2020-03-31T22:02:37Z

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 · 2020-04-01T02:28:18Z

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 · 2020-04-01T11:26:47Z

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.

- When redirections are detected, rather than printing instructions, update.py now creates two commits -- one with the plugin updates and another with redirected plugin names updated to their new repos. - Added a --allow-dirty flag so that one can run ./update.py --commit with a dirty nixpkgs repository, which is necessary for development. I wouldn't mind removing this before merging if it's not in our flag budget but it's necessary scaffolding for now.

- update.py's new --add argument replaces manual editing of vim-plugin-names for basic use cases.

Adding some abstraction makes main() more readable which is important since it's the main control flow of the script.

Parsing plugin lines in multiple places is hazardous and already manifested a bug!

- Use git.Repo(ROOT, search_parent_directories=True) to find nixpkgs repo. - Don't commit overrides.nix. - Remove "-a" short argument. - Remove "--commit" flag and commit by default. - Improve help/error messages. - Favor closure pattern over classes.Use a closure to wrap the update function with state rather than a callable class. - break NixpkgsRepo class into functions - Optional None-type arguments - Remove repo checks from update.py. Git is too flexible and permits too many workflows for my attempt to replace documentation with code to work. My goal would be to separate the `--add` functionality from the update functionality in the near term and then there will be no reason for this usage to create commits anyway.

timokau · 2020-04-01T18:29:06Z

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

ryneeverett · 2020-04-01T18:38:53Z

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 · 2020-04-01T19:31:36Z

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.

ryneeverett requested a review from jonringer as a code owner March 30, 2020 16:39

ofborg bot added 6.topic: vim 8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Mar 30, 2020

timokau requested changes Mar 30, 2020

View reviewed changes

timokau mentioned this pull request Mar 30, 2020

vimPlugins: Add --proc flag to update script. #83798

Merged

10 tasks

ryneeverett force-pushed the vim-plugins-update-commit branch 3 times, most recently from 49aeeb2 to 06615ab Compare March 31, 2020 05:15

timokau reviewed Mar 31, 2020

View reviewed changes

Mic92 reviewed Mar 31, 2020

View reviewed changes

ryneeverett force-pushed the vim-plugins-update-commit branch from 06615ab to 9bcb49c Compare March 31, 2020 15:30

ryneeverett force-pushed the vim-plugins-update-commit branch 4 times, most recently from 53d85a8 to b3f7df2 Compare March 31, 2020 18:15

timokau reviewed Mar 31, 2020

View reviewed changes

ryneeverett force-pushed the vim-plugins-update-commit branch from fb7cc0d to 1583f4e Compare April 1, 2020 02:07

ryneeverett force-pushed the vim-plugins-update-commit branch from 1583f4e to 8fe1c56 Compare April 1, 2020 14:26

ryneeverett added 5 commits April 1, 2020 14:30

vimPlugins: automatically commit update

df0db17

vimPlugins: update.py --add <plugin> argument

f1ae95f

- update.py's new --add argument replaces manual editing of vim-plugin-names for basic use cases.

vimPlugins: remove repetition from update.main

1f32eb7

Adding some abstraction makes main() more readable which is important since it's the main control flow of the script.

vimPlugins: normalize deprecation names

1c0f0de

Parsing plugin lines in multiple places is hazardous and already manifested a bug!

ryneeverett force-pushed the vim-plugins-update-commit branch from 8fe1c56 to 04f7bfe Compare April 1, 2020 14:31

ryneeverett force-pushed the vim-plugins-update-commit branch from 04f7bfe to 0a27594 Compare April 1, 2020 17:19

timokau approved these changes Apr 1, 2020

View reviewed changes

timokau merged commit 3abce7b into NixOS:master Apr 1, 2020

		@@ -413,6 +416,11 @@ def generate_nix(plugins: List[Tuple[str, str, Plugin]], outfile: str):
		print(f"updated {outfile}")


		def commit_changes(repo: git.Repo, *files: Path):

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

vimPlugins: Automate git commits when updating. #83788

vimPlugins: Automate git commits when updating. #83788

Conversation

ryneeverett commented Mar 30, 2020

Motivation for this change

Things done

ryneeverett commented Mar 30, 2020

ryneeverett commented Mar 30, 2020

timokau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timokau commented Mar 30, 2020

ryneeverett commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timokau commented Mar 31, 2020

ryneeverett commented Mar 31, 2020

Choose a reason for hiding this comment

timokau commented Mar 31, 2020

ryneeverett commented Mar 31, 2020

jonringer commented Mar 31, 2020

timokau commented Mar 31, 2020

ryneeverett commented Mar 31, 2020

timokau commented Mar 31, 2020

ryneeverett commented Apr 1, 2020

timokau commented Apr 1, 2020

timokau commented Apr 1, 2020

ryneeverett commented Apr 1, 2020

timokau left a comment

Choose a reason for hiding this comment

timokau commented Apr 1, 2020