Fix three bugs in .zi-check-for-git-changes() that broke zinit self-update

[alberti42]

Summary

Fix three bugs in zinit self-update.

Bug 1: No fetch before change detection

.zi-check-for-git-changes() compared HEAD against the local tracking ref (@{u}) without fetching first, so the tracking ref was always stale. The git fetch that would have updated it lived inside .zinit-self-update(), in the block guarded by that very check — a chicken-and-egg problem.

Result: zinit self-update always reported "Already up-to-date." even when upstream had new commits.

Bug 2: git --work-tree instead of git -C

.zi-check-for-git-changes() used git --work-tree "$1" for all its git commands. This flag overrides where git looks for tracked files, but does not change where git discovers the .git directory — that still happens by searching upward from the CWD. The correct flag is git -C "$1", which changes directory first so that git finds the right .git and operates on the right repository.

The original author likely confused --work-tree with -C, interpreting it as "run git in this directory." In reality, --work-tree is meant for setups where the .git directory and working tree are in separate locations (e.g., bare repos with external worktrees, or dotfile management patterns). That's not the case for zinit, which is a standard clone.

Result: If the user's CWD was inside a different git repo, rev-list ran against the wrong repository, returning bogus commit counts and triggering unnecessary recompile/reload.

Bug 3: -q flag never worked

Two separate issues prevented quiet mode from working:

1. Dispatcher didn't forward arguments. In zinit.zsh, the dispatcher for the self-update subcommand called .zinit-self-update without forwarding arguments:

   (self-update)
       .zinit-self-update
       ;;

   Other dispatcher entries (e.g., times) use "${@[2-correct,-1]}" to forward arguments. Because -q was never passed through, .zinit-self-update always ran in verbose mode — all the [[ $1 != -q ]] checks always evaluated to true. (The only caller that actually passed -q was the internal .zinit-self-update -q call on line 3405.)

2. .zi-check-for-git-changes() didn't accept -q. Even if the dispatcher had forwarded -q, the log messages emitted by .zi-check-for-git-changes() (the "fetching latest changes" and "Already up-to-date." messages) had no quiet mode support.

Result: zinit self-update -q behaved identically to zinit self-update.

Why bugs 2 and 3 went unnoticed

Bug 1 masked bug 2. Without a fetch, the local @{u} tracking ref was always equal to HEAD, so the rev-list comparison always returned down=0. The function always took the "no changes" exit path — making the --work-tree bug impossible to observe. Fixing the fetch (bug 1) made the rev-list comparison meaningful for the first time, which exposed bug 2.

Bug 3 went unnoticed because .zi-check-for-git-changes() never entered the code path where verbosity mattered — it always short-circuited with "Already up-to-date." due to bug 1. There was no visible difference between quiet and verbose when the function did nothing.

Changes

• Replace all git --work-tree "$1" with git -C "$1" in .zi-check-for-git-changes() so git operates on the correct repository regardless of CWD
• Move git fetch into .zi-check-for-git-changes() so remote tracking refs are up-to-date before comparing
• Have .zi-check-for-git-changes() resolve the current branch name and return it via $REPLY, avoiding a duplicate git rev-parse call in .zinit-self-update()
• Move the "fetching latest changes" log message into .zi-check-for-git-changes() alongside the fetch
• Remove the now-redundant git fetch and log line from the .zinit-self-update() subshell
• Forward arguments from the dispatcher to .zinit-self-update in zinit.zsh using "${@[2-correct,-1]}", matching the pattern used by other subcommands

What was tested

• Run zinit self-update from a different git repo directory when there are no upstream changes — reports "Already up-to-date." without recompiling
• Run zinit self-update when upstream has new commits — fetches, display changelog, pull, and recompile
• Run zinit self-update again immediately — reports "Already up-to-date."
• Run zinit self-update -q — quiet mode still works correctly
• Verified non-main branch warning still appears for forked repos on custom branches

[alberti42]

@pschmitt and @vladdoster

This seems a huge bug, and I am not sure how it went unnoticed so far. Essentially the self-update mechanism of zinit was broken, and no one could ever update to any version.

I think I fixed properly; I prefer to wait someone of you taking a look into. In principle I should now be able to do merges of PR myself, but as I am new to this community I prefer to be careful.

PS: There are also errors thrown by the test units. I investigated those and I think they have nothing to do with my changes. I could in the future look into this and repair the test units if it is broken (as I suspect; but would need extra work to understand the mechanics).

[vladdoster]

looks good other than my one change suggestion

[vladdoster]

@alberti42,

PS: There are also errors thrown by the test units. I investigated those and I think they have nothing to do with my changes. I could in the future look into this and repair the test units if it is broken (as I suspect; but would need extra work to understand the mechanics).

The gh-r tests are most likely transient errors. I'll take a look/fix.

[vladdoster]

@alberti42,

Users would expect -q and --quiet flags.

Let's refactor this to use the current zinit command/option parser.

This has following benefits we get for free:

• self-update command usage via -h/--help
• global -q variable removing need to pass -q around and validate it
  -attempt to follow current command convention

I can get it done today when I am free or I'm happy to help if you hit any issues with the refactor.

Refactor would be:

• Add self-update here and you can reuse these value

• Add it here as well

• update your code and check for quiet flag via (( !OPTS[opt_-q,--quiet] ))

• Update completion for self-update to have help and quiet flags like cclear

[vladdoster]

@alberti42 ,

LOL, I've had a PR open for 2 years addressing most of the self-update refactor in fix(self-update): respect option flags. Whoops

[alberti42]

Let's refactor this to use the current zinit command/option parser.

@vladdoster Thanks! That was a very good idea. It makes the code much cleaner and easier to maintain in the future. I implemented all changes into two separate commits: 726f94f and 85a0721.

• zinit.zsh: self-update added to ___opt_map (for help text + flag validation), added to the early-exit block (for -h/--help support), and the dispatcher now calls .zinit-parse-opts then .zinit-self-update with no arguments.
• zinit-autoload.zsh: Both .zi-check-for-git-changes() and .zinit-self-update() drop the ad-hoc local quiet / [[ $1 = -q ]] pattern and use OPTS[opt_-q,--quiet] throughout. The internal call in .zinit-update-or-status-all() sets up a local OPTS with the quiet flag before calling .zinit-self-update.
• _zinit: _zinit_self_update() now declares -h/--help and -q/--quiet flags for shell completion.

I think we are done. Do you want to take a final look?

[alberti42]

One last comment on the cosmetic. What are the guidelines?

For example, we are still using print magic "%F{33}" for the colors in

```zsh
print -P "%F{33} %F{34}Installation successful.%f%b" || \
print -P "%F{160} The clone has failed.%f%b"

The style is also not in line with zinit.

But my question is more general. Based on your feedback, I could open a new PR to harmonize the style of lof messages. They are not always consistent. I would let an AI model distill a guideline document for how messages need to be styled. I would edit it and then post it in some `zinit` doc directory. We could then review the document and converge on a commonly accepted guideline. This is where AI can help a lot. After we have a guideline, we can ask to systematically review all log messages and adjust them to the desired style. It is a small improvement, but a nice consistent cosmetics helps popularize zinit. Its a good sign of love and care.

[alberti42]

I realized there are too many occurrences of print -P "%F{xyz}... overall. It makes no sense to patch up just a couple. It is more meaningful to understand how we want to have it and then handle it in a separate PR in a consistent manner.

[alberti42]

@vladdoster You have an open "requested changes" flag. Is it the old one you originally opened or a new one?

Shall we proceed with the merge? @pschmitt @alichtman Do you also want to take a look? You were all flagged for review (probably by @vladdoster).

PS: Do we have a policy in this community on how to handle PR? How many reviewers do we typically expect/need, etc? Most likely it also depends on the extent and scope of the PR, but I was wondering whether we have general guidelines.

[pschmitt]

Yeah it's been very yolo and random.
I don't mind if you merge your own PRs, esp. when like this one here when there are no comments left.

[alberti42]

Thanks, Philipp. I just wait a couple of days to see whether Vlad will also agree. He was much more involved, already initially with a first PR.

[vladdoster]

Test flags to avoid regression

[alberti42]

@vladdoster I added the tests. Everything is now green. Do you want to check and merge?

[vladdoster]

@alberti42,

I think this can be refactored to be ... Idk why it checked twice

Suggested change

	if (( $@[(I)-*] || OPTS[opt_-h,--help] )); then
	.zinit-parse-opts "$cmd" "$@"
	if (( OPTS[opt_-h,--help] )); then
	+zinit-prehelp-usage-message $cmd $___opt_map[$cmd] $@
	return 1;
	elif (( ${reply[(I)-*]} )); then
	+zinit-prehelp-usage-message $cmd $___opt_map[$cmd] ${reply[@]}
	return 1
	fi
	fi
	.zinit-parse-opts "$cmd" "$@"
	if (( OPTS[opt_-h,--help] )) || (( ${reply[(I)-*]} )); then
	+zinit-prehelp-usage-message $cmd $___opt_map[$cmd] ${reply[@]}
	return 1;
	fi

[alberti42]

My code before introduced (93e4182):

  @test 'self-update' {
    run zinit self-update
    assert $output matches 'Already up to date'; assert $state equals 0

    run zinit self-update --help
    assert $output matches '.*(HELP).*(self-update).*'; assert $state equals 1

    run zinit self-update --quiet
    assert $output is_empty; assert $state equals 0
  }
  @test 'self-update --help / -h shows help and exits 1' {
    for flag in '--help' '-h'; do
      run zinit self-update $flag
      assert $output contains 'self-update'
      assert $output contains '--quiet'
      assert $state equals 1
    done
  }
  @test 'self-update --quiet / -q suppresses output' {
    for flag in '--quiet' '-q'; do
      run zinit self-update $flag
      assert $output is_empty
      assert $state equals 0
    done
  }
  @test 'self-update rejects unknown flags' {
    run zinit self-update --foo
    assert $output contains 'Incorrect options given'
    assert $output contains '--foo'
    assert $state equals 1
  }

They were perfectly readable. Each test has a self-describing name. Each assertion is explicit. The unknown-flags test uses two simple contains checks — easy to understand at a glance. But then you insulted them as cruft? I would invite to use a more polite tone.

After your 4 commits:

  @test 'self-update' {
    local cmd='self-update'
    run zinit $cmd
    ...
    for flag in '-h' '--help'; do
      run zinit $cmd $flag
      assert $output matches ".*(HELP).*($cmd).*"; assert $state equals 1
    done
    ...
    run zinit $cmd --invalid
    assert $output matches ".*(Incorrect options given).*(--invalid).*(Allowed for the subcommand.*$cmd).*"; assert $state equals 1
  }

I feel these changes are a regress for the reasons:

1. local cmd='self-update' adds an indirection that saves nothing → znit self-update is more readable than zinit $cmd.
2. The for loops collapse separate named tests into an anonymous blob → if one case fails, zunit now reports one failure instead of pinpointing which test by name.
3. The --invalid regex is a 60-character multi-group regex that replaces two plain contains checks. More fragile, harder to read, harder to maintain.
4. (cruft) — my tests were not cruft. They were well-structured with descriptive names.
5. Three commits to fix two syntax errors @vladdoster introduced themselves; the file was literally broken at 1e37baa → if you insist to keep those commits, please squash them into a single commit to avoid broken commits; also choose descriptive commit text for the changes.

On 0021ab4 (removing .zinit-parse-opts "self-update" "$@" from the case block): technically safe because the pre-guard at zinit.zsh:2632–2643 already calls it when any -* flags are present. But removing it makes the case block less self-contained, and the correctness now depends on understanding that earlier guard, which is non-obvious.

My assessment: overall, these changes make the code less readable, the tests less useful, and the history noisier, and for no gain.

I am sure you did not mean to be rude, and your changes were well intended. However, I would recommend reverting them or, at least in the future, avoiding such unnecessary chores if we want to keep a positive cooperation atmosphere.

[alberti42]

I have rebased the branch onto main, creating a clean linear history without unnecessary merge commits that had accumulated. git diff between the old and new branch tip shows no differences.

This PR fixes a long-standing bug that prevented zinit self-update from ever detecting upstream changes, meaning every user who ran it would silently get "Already up to date." regardless of reality. I find it a priority to get this merged.

If there are any remaining style or refactoring preferences, those can be addressed in a follow-up PR.

This PR fixes a long-standing bug that prevented zinit self-update from ever detecting upstream changes, meaning every user who ran it would silently get "Already up to date." regardless of reality. I find it a priority to get this merged.

If there are any remaining style or refactoring preferences, those can be addressed in a follow-up PR.