#562 changed Git repo behavior in an unethical way

[KlfJoat]

Erroneous Behavior

I ran topgrade as normal. However, when it got to the "Git repositories" stage, after pulling (as normal), it started pushing my repos!!!!!!

That's a HORRIBLE change to make UNANNOUNCED, since IT AFFECTS THE WORLD OUTSIDE OF MY HOST WITHOUT MY PERMISSION!!

I don't want to push these repos and the assumption that I do is ETHICALLY wrong.

I run topgrade to change my host and only my host (plus remote hosts that I have explicitly told topgrade to change). I DID NOT run topgrade to change anything else (unless I've programmed it in a custom command).

It gets even worse bc topgrade hung up at this point since in many cases, I'm only pulling these repos for local use and do not have ANY ability to push to them, so I got prompts for usernames.

── 15:13:53 - Git repositories ─────────────────────────────────────────────────
Pulling /home/klfjoat/src/repo1
Pulling /home/klfjoat/src/repo2
Pulling /home/klfjoat/src/repo3
Pulling /home/klfjoat/repo4
Up-to-date /home/klfjoat/src/repo1
Pulling /home/klfjoat/src/repo5
Up-to-date /home/klfjoat/src/repo3
Up-to-date /home/klfjoat/src/repo2
Up-to-date /home/klfjoat/src/repo5
Changed /home/klfjoat/repo4:
189877d cleanup: remove cruft

Pushing /home/klfjoat/repo4
Pushing /home/klfjoat/src/repo5
Pushing /home/klfjoat/src/repo2
Pushing /home/klfjoat/src/repo1
Pushing /home/klfjoat/src/repo3
Username for 'https://github.com': Username for 'https://github.com': Pushing /home/klfjoat/src/repo2
Failed pushing /home/klfjoat/src/repo2

Hitting <enter> a bunch or CTRL-C got me out of this.

Expected Behavior

When changing the behavior of a step, if I make no changes I expect the behavior to not change unless you're fixing a bug.

To fix, I expect that there is some way to identify which repos I want pulled only (this should be the default repos list) and which repos I want pulled AND pushed (maybe some new repos-push list).

Steps to reproduce

Run topgrade and have anything in your topgrade.toml file's repos list.

Possible Cause (Optional)

#562

Problem persists without calling from topgrade

• Yes
• No

Did you run topgrade through Remote Execution

• Yes
• No

If yes, does the issue still occur when you run topgrade directlly in your
remote host

• Yes
• No

Configuration file (Optional)

# Additional git repositories to pull                                                               
repos = [
#    "~/src/*/",
#    "~/.config/something"
    "~/src/repo1",
    "~/src/repo3",                                                                            
    "~/src/repo2",                            
    "~/repo4",                                                                                   
    "~/src/repo5"                              
]  

Additional Details

• Operation System/Version

  • Ubuntu 23.04

• Installation

  • cargo install topgrade

• Topgrade version (topgrade -V)

  • Topgrade 13.0.0

Verbose Output (topgrade -v)

This isn't a bug requiring verbose output, IMO, since this was a stated goal in #562. But if you want it I'll do it in the future.

[SteveLauC]

That's a HORRIBLE change to make UNANNOUNCED, since IT AFFECTS THE WORLD OUTSIDE OF MY HOST WITHOUT MY PERMISSION!!

I am sorry about what happened to you, but this is not an unannounced change, it is documented in the release note

To fix, I expect that there is some way to identify which repos I want pulled only (this should be the default repos list) and which repos I want pulled AND pushed (maybe some new repos-push list).

We do have this, see config.example.toml

---

If there are breaking changes, I will bump the major version number and document these changes in the release note, but it seems like we should do it in another way to make our user better informed

[SteveLauC]

I made a PR #606 to mention that "Users should take a look at the release note when updated to a major release" in case something like this would happen in the future

[Silveere]

While I see this feature being very useful for some cases (especially for the management of dotfiles), I agree that enabling it by default is an absolutely horrible choice. From my understanding, the main goal of Topgrade is to keep the local system up-to-date, so it should absolutely not affect any state outside of the system unless it is explicitly configured to do so. There are some cases where pushing unwanted changes can be, at worst, completely irreversible (e.g., AUR package repos, which do not accept force pushing).

[KlfJoat]

I am sorry about what happened to you, but this is not an unannounced change, it is documented in the release note

... topgrade itself is self-updating.

In what world am I able to read the release notes or even to know where they are kept?

You changed the behavior of an existing feature in a way that was unethical--meaning it did not remain within the social contract of the stated goals of the tool.

Please acknowledge that rather than saying "well, I wrote a note about it in this place that users don't have to acknowledge bc the tool auto-updates".

If there are breaking changes, I will bump the major version number and document these changes in the release note, but it seems like we should do it in another way to make our user better informed

Will topgrade itself pause at the major version bump and require manual intervention to upgrade? If not, then your plan does nothing to address the concern.

The real fix is to roll back your breaking change and to create a different list name entirely for your push-pull feature. Do not reuse an existing configuration name and arbitrarily change what it does!

[hakamadare]

to second @KlfJoat 's objections: i spent a while digging through my shell config and looking at other tools to see whether i had inadvertently made some change that triggered attempts to push to remote repos. it did not occur to me to check topgrade's release notes because i couldn't imagine that this change in behavior was intentional 🤦

i'd be much happier to see a git.pull_and_push_repos config option as a way of introducing this functionality nondestructively; i'm happy to try to implement this change if the maintainers are open to it and are willing to revert to the previous pull-only default behavior.

[SteveLauC]

cc @savente93

---

From my understanding, the main goal of Topgrade is to keep the local system up-to-date, so it should absolutely not affect any state outside of the system unless it is explicitly configured to do so.

I agree that pushing Git repos technically is not an update procedure, the reasons why I allow this feature to be added are:

1. It is useful for some cases
2. Almost everything in Topgrade is optional, you don't enable it, you won't get it, so it is fine to add such a feature

---

... topgrade itself is self-updating.

In what world am I able to read the release notes or even to know where they are kept?

Well, you won't see a release note with cargo install, please consider using the pre-built binaries with the self_update feature enabled, they will print the release note when a new version gets installed.

Will topgrade itself pause at the major version bump and require manual intervention to upgrade? If not, then your plan does nothing to address the concern.

I admit that this is bad for our user

The real fix is to roll back your breaking change and to create a different list name entirely for your push-pull feature. Do not reuse an existing configuration name and arbitrarily change what it does!

While implementing this feature, I do prefer the repos name rather than something like push_pull_repos as it is shorter, but changing it to push_pull_repos still looks good to me

[SteveLauC]

What about we implement a feature that will prompt the user to read the release note when updated to a major release? Something like:

$ topgrade

It seems that this is the first run of this major release: x.0.0, please read the release note to check out braking changes first.

I have read the release note (y)es/(N)o

ONLY when the user inputs yes will topgrade do its job

[savente93]

@SteveLauC I don't really mind what the config option is called, I'm happy to defer to your wisdom. Any action needed on my part?

[SteveLauC]

Folks, let's change the field name in v14.0.0:

1. use push_pull_repos for push and pull

2. use pull_only_repos for pull

3. use push_only_repos for push

4. remove repos

   treat it as an alias for push_pull_repos in v14.0.0 is also fine, a warning will be printed when it is being used, then we remove it in v15.0.0

And this:

What about we implement a feature that will prompt the user to read the release note when updated to a major release? Something like:

$ topgrade

It seems that this is the first run of this major release: x.0.0, please read the release note to check out braking changes first.

I have read the release note (y)es/(N)o

ONLY when the user inputs yes will topgrade do its job

will be implemented as well.

---

Thoughts on this? I would like to hear what you guys think about this solution

---

Any action needed on my part?

I will handle this since it is not a big refactor

[savente93]

I'm okay with all of this. Holler if you need anything but I'll be unsubscribing from this thread since I don't enjoy being yelled at.

[Neirda24]

Hi. Just found out about that fork and reviewing issues is usually a good start on identifying the reliability of a tool. Seeing this is scary as hell to me. I use topgrade regularly locally to update everything LOCALLY. Anything that is done towards remote servers or else should be a plugin explicitly opt-in for any user. Too much issues can happen.

Anyway I am not sure whether or not it is disabled by default now. For both push & pull. Could you confirm?

Thank you.

[SteveLauC]

Anything that is done towards remote servers or else should be a plugin explicitly opt-in for any user. Too much issues can happen.

If you don't explicitly specify the repos to push in your configuration, you won't get this

One can always check what Topgrade will do with the --dry-run option, it will just tell u what should happen and won't actually do them

[KlfJoat]

the reasons why I allow this feature to be added are:
It is useful for some cases

I wholeheartedly agree! This is a great feature, and I'm glad you added it!

Almost everything in Topgrade is optional, you don't enable it, you won't get it, so it is fine to add such a feature

I absolutely agree that this principle is fundamental and inherent to Topgrade!

You also VIOLATED that principle in a way that DESTROYS TRUST with your user base. I still see ZERO acknowledgement of this by you or @savente93.

While implementing this feature, I do prefer the repos name rather than something like push_pull_repos as it is shorter, but changing it to push_pull_repos still looks good to me
...
use push_pull_repos for push and pull
use pull_only_repos for pull
use push_only_repos for push
...
Thoughts on this? I would like to hear what you guys think about this solution

Principle of Least Astonishment.

You had/have the following options, as I see it. This is in preferential order from a UX perspective.
(1&2 are equivalent UX, IMO, but GH auto-markdowns any list-like convention so I can't explicitly label them as "1" and "1")

1. Keep repos to mean "pull", and add the 2 new config file options, and keep this code in forever.
2. Keep repos to mean "pull" (repos is an internal alias for pull_only_repos), and keep this code in forever. Then add the 3 new config file options.
3. Add code that changes the config file option name in the user's config file from repos to pull_only_repos, and keep this code in forever. Then add the 3 new config file options.
4. Throw an error on repos, possibly with a hint to the config option name change, and keep this code in forever. Then add the 3 new config file options.

999,999) Provide some method of limiting topgrade upgrades along semver breaking version numbers via all methods, self-update and cargo install.

1,000,000) Your current plan of interrupting user upgrades, forcing upgrade notes, removing code after 1 version, etc.

You can absolutely do 1-4, but you're choosing not to... why?

If you don't explicitly specify the repos to push in your configuration, you won't get this

Why are you lying? I did not specify any repos to push in my configuration, and yet they were pushed! You planned this in the open in #562 and you admitted this in earlier comments in this issue! So why are you telling users this lie?

I'm sorry, but the absolute lack of responsibility on display here is maddening for a tool that I trust with system-updating root privileges. I stopped using Windows as my desktop OS over 15 years ago because they changed a setting on my computer without my consent. This is PERILOUSLY close to the same activity! I don't expect Micro$oft to care. But I do expect FOSS developers to care!

Looks like I really should have taken r-darwish's warning to heart...

I am not involved in this effort nor do I know the people behind it, so I encourage you to inspect their work before using the fork.

[SteveLauC]

You can absolutely do 1-4, but you're choosing not to... why?

Because I think the root cause of this issue is how we handle breaking changes and get our users informed

Anyway, let's revert that feature, I am tired of this conversation

[SteveLauC]

Close as this functionality has been reverted

[SteveLauC]

#619 implements that breaking changes notification feature, which could hopefully prevent issues like this one:

$ ./target/debug/topgrade

── 09:54:15 - Topgrade 13.0.0 Breaking Changes ─────────────────────────────────
No Breaking changes

Confirmed? (y)es/(N)o