Add support for pushing custom git repositories #574

savente93 · 2023-10-12T19:20:38Z

Standards checklist

The PR title is descriptive.
I have read CONTRIBUTING.md
The code compiles (cargo build)
The code passes rustfmt (cargo fmt)
The code passes clippy (cargo clippy)
The code passes tests (cargo test)
Optional: I have tested the code myself

Fixes #562

Not the most elegant solution I'll admit. I'm a little sad about the duplication but I was having some real issues with the async code and opaque types when I tried to deduplicate it. This implementation works at least (at least it did for my system, don't have a setup for more robust testing) so hopefully this is good enough for a first pass. I've added config options that seemed reasonable to me. Happy reviewing!

config.example.toml

src/steps/git.rs

SteveLauC · 2023-10-13T02:54:29Z

Thanks for doing this! The core part of this PR LGTM, I have left some comments on some small things that need to be changed.

I have some thoughts on the public interfaces (i.e., the config file/how users would use this feature), and I wanna hear your thoughts as well:

In this PR, users would specify the repos they wish to pull or push in:
```
[git]
repos = []
```
When
```
[git]
push_custom_repos = true
```
is set to true, these repos would be pulled and pushed

What about changing it to:
```
[git]
pull_repos = []
push_repos = []
```
so that users can separate them, IMHO, a user won't wanna pull or push a repo at the same time?
Actually, everything in Topgrade is an opt-in feature, and you can see this from our code config.rs, every configuration entry is optional, you want to use it, you write it, and those options that are not present will be silently ignored

I can imagine this might have to be an opt in feature

So it is not that necessary to ask a user to manually turn this feature on or off
```
[git]
push_custom_repos = true
```

This arguments entry

[git] 
# Arguments to pass Git when pulling repositories
arguments = "--rebase --autostash"

should be renamed to pull_arguments since we now have a push_arguments

[git] 
# Arguments to pass Git when pulling repositories
pull_arguments = "--rebase --autostash"

# Arguments to pass Git when pushing repositories
push_arguments = "--all"

Don't worry about breaking changes to our user, it is fine to bump our major number in the next release, as long as I document these changes well in the release note, it shouldn't be a problem :)

savente93 · 2023-10-13T06:02:12Z

Thanks for the good comments, I've updated the ones I missed so that should be all good now. I'm also going to address your comments in reverse order:

(3) Indeed, I think that's a fairly easy migration path. We could also alias arguments to pull_arguments and add a deprecation warning if you want, I'm fairly ambivalent so I'll defer to your decision on that.

(2) agreed, I guess what I meant when I said "opt in" was, "we shouldn't just start pushing any repo we find" i.e. "we should not do the new thing unless told so" so we are in agreement there.

(1) I think this is a UX decision. Personally I have a slight preference for the current implementation because that works better with my workflow. I have two machienes I work across at no real defined schedule, and I have a bunch of repositories on both that need to both read and write. Think dotfiles, personal projects, prototypes, etc. Bascially I just have a projects folder and that folder is globbed into the repos so everything there I want as up to date as possible. Therefore the current impl works a bit nicer because that way I don't have to maintain essentially duplicate lists, however I'm not sure how common this workflow is, and I agree that your suggestions allows more flexibility. so tl;dr I slightly prefer my solution, but am happy to go with yours if you prefer.

In any case, thanks for the guidance and thoughtful comments, contributing here has been a very pleasant experience :)

SteveLauC · 2023-10-13T06:38:34Z

Personally I have a slight preference for the current implementation because that works better with my workflow.

So with the approach I suggested, your configuration file would be something like this, right?

[git]
pull_repos = [
    "project1", 
    "project2",
]
push_repos = [
    "project1", 
    "project2",
]

I don't have to maintain essentially duplicate lists

Indeed, duplicate paths are bad :<

So, what about the following approach:

[git]
# Repos specified here will be pulled AND pushed
repos = []
# Repos to pull
pull_repos = []
# Repos to push
push_repos = []

For the implementation, we still only store pull_repositories and push_repositories:

pub struct Repositories<'a> {
    git: &'a Git,
    pull_repositories: HashSet<String>,
    push_repositories: HashSet<String>,
    glob_match_options: MatchOptions,
    bad_patterns: Vec<String>,
}

in main.rs, we add all the repos specified in repos = [] to pull_repositories and push_repositories:

if config.should_run(Step::GitRepos) {
        if let Some(git_repos) = config.git_repos() {
            for git_repo in git_repos {
                git_repos.glob_insert_to_pull(git_repo);
                git_repos.glob_insert_to_push(git_repo);
            }
        }

        if let Some(git_pull_repos) = config.git_pull_repos() {
             for git_repo in git_pull_repos {
                git_repos.glob_insert_to_pull(git_repo);
            }
        }

        if let Some(git_push_repos) = config.git_push_repos() {
             for git_repo in git_push_repos {
                git_repos.glob_insert_to_push(git_repo);
            }
        }

  
        runner.execute(Step::GitRepos, "Git repositories", || {
            git.multi_repo_step(&git_repos, &ctx)
        })?;
}

savente93 · 2023-10-13T06:43:40Z

Oeh I like that, I'll make the changes!

savente93 · 2023-10-13T07:20:08Z

Should be all good now!

config.example.toml

src/config.rs

src/steps/git.rs

SteveLauC · 2023-10-13T07:58:47Z

Left some comments, I think this PR is ready for merge once the comments are resolved :)

src/config.rs

SteveLauC · 2023-10-13T08:23:17Z

The CI failure is for the reason that is_empty() is #[cfg(unix)], we need to remove the attribute then:

 #[cfg(unix)]
    pub fn is_empty(&self) -> bool {
        self.pull_repositories.is_empty() && self.push_repositories.is_empty()
    }

savente93 · 2023-10-13T08:25:48Z

Shouldn't we also remove it for remove? I'm not sure why those would need to be marked as unix anyway.

SteveLauC · 2023-10-13T08:31:40Z

Shouldn't we also remove it for remove? I'm not sure why those would need to be marked as unix anyway.

It seems that is_empty() and remove() are only used in the oh-my-zsh step, which is UNIX-only. It might be better to just remove the attribute for is_empty() or there will be a clippy warning on Windows (unused method or something like this)

SteveLauC · 2023-10-13T08:38:06Z

Yeah, clippy gave us a warning :<

Run cargo clippy --locked --target x86_64-pc-windows-msvc --all-features -- -D warnings
   Compiling topgrade v12.0.2 (D:\a\topgrade\topgrade)
error: method `remove` is never used
   --> src\steps\git.rs:394:[12](https://github.com/topgrade-rs/topgrade/actions/runs/6505914484/job/17670480656?pr=574#step:7:13)
    |
326 | impl<'a> Repositories<'a> {
    | ------------------------- method in this implementation
...
394 |     pub fn remove(&mut self, path: &str) {
    |            ^^^^^^
    |
    = note: `-D dead-code` implied by `-D warnings`

error: could not compile `topgrade` (bin "topgrade") due to previous error
Error: Process completed with exit code 1.

savente93 · 2023-10-13T08:38:46Z

my bad, added it back in!

SteveLauC · 2023-10-13T08:40:17Z

my bad, added it back in!

Removing it for is_empty() is fine as it is indeed used on Windows

savente93 · 2023-10-13T08:43:27Z

done!

SteveLauC · 2023-10-13T08:47:04Z

done!

Seems that you forgot to push your commit?

savente93 · 2023-10-13T08:54:14Z

I think we had a race condition, I pushed the commit just before you made the comment. As far as I can see it's all there now.

SteveLauC · 2023-10-13T08:58:24Z

I am sorry, my fault, it is indeed there, I am kinda dizzy this afternoon (my timezone)

savente93 · 2023-10-13T08:59:53Z

No worries, You've been very patient with me, so I think you deserve the same :)

SteveLauC · 2023-10-13T09:02:11Z

Thanks! And congratulations on your first contribution to Topgrade!

savente93 · 2023-10-13T09:03:38Z

Thank you very much, it was a pleasure ^^

savente93 added 2 commits October 12, 2023 21:05

add support for pushing custom git repos

df1db28

update docs

a254088

SteveLauC requested changes Oct 13, 2023

View reviewed changes

config.example.toml Outdated Show resolved Hide resolved

src/steps/git.rs Outdated Show resolved Hide resolved

src/steps/git.rs Outdated Show resolved Hide resolved

src/steps/git.rs Outdated Show resolved Hide resolved

update review comments

d1bb591

savente93 added 2 commits October 13, 2023 09:15

add configs for push or pull repos only

0603630

fix stray mistakes

e0dca16

savente93 requested a review from SteveLauC October 13, 2023 07:19

SteveLauC requested changes Oct 13, 2023

View reviewed changes

config.example.toml Outdated Show resolved Hide resolved

src/config.rs Outdated Show resolved Hide resolved

src/config.rs Show resolved Hide resolved

src/config.rs Outdated Show resolved Hide resolved

src/steps/git.rs Show resolved Hide resolved

savente93 added 2 commits October 13, 2023 10:10

remove use of unecessary macro

394b8ea

update comment

e41c6ed

SteveLauC reviewed Oct 13, 2023

View reviewed changes

src/config.rs Outdated Show resolved Hide resolved

fix typo

05df45a

savente93 requested a review from SteveLauC October 13, 2023 08:15

SteveLauC approved these changes Oct 13, 2023

View reviewed changes

remove unix cfg in git

7a839c5

readd cfg to fix clippy warning

c8290f5

SteveLauC merged commit fe9d877 into topgrade-rs:main Oct 13, 2023

This was referenced Oct 13, 2023

fix: shellexpand git.pull_only_repos & git.push_only_repos #576

Merged

If unknown config values are present, configuration is silently not loaded #580

Closed

p-linnane mentioned this pull request Oct 20, 2023

topgrade 13.0.0 Homebrew/homebrew-core#151889

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for pushing custom git repositories #574

Add support for pushing custom git repositories #574

savente93 commented Oct 12, 2023

SteveLauC commented Oct 13, 2023 •

edited

Loading

savente93 commented Oct 13, 2023 •

edited

Loading

SteveLauC commented Oct 13, 2023 •

edited

Loading

savente93 commented Oct 13, 2023

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

SteveLauC commented Oct 13, 2023 •

edited

Loading

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

savente93 commented Oct 13, 2023

Add support for pushing custom git repositories #574

Add support for pushing custom git repositories #574

Conversation

savente93 commented Oct 12, 2023

Standards checklist

SteveLauC commented Oct 13, 2023 • edited Loading

savente93 commented Oct 13, 2023 • edited Loading

SteveLauC commented Oct 13, 2023 • edited Loading

savente93 commented Oct 13, 2023

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

SteveLauC commented Oct 13, 2023 • edited Loading

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023

savente93 commented Oct 13, 2023

SteveLauC commented Oct 13, 2023 •

edited

Loading

savente93 commented Oct 13, 2023 •

edited

Loading

SteveLauC commented Oct 13, 2023 •

edited

Loading

SteveLauC commented Oct 13, 2023 •

edited

Loading