Ensure that the new --staged option warns users when files changed after being staged

[castarco]

This "request" stems from a discussion thread on a recent PR ( #2300 )

Problem statement

1. Let's say that we have 3 staged files: A, B, and C.
2. Let's also say that C has new changes after it has been staged.
3. When biome format/check/lint reads the file C
   1. It will see what we have in our local filesystem (including the changes after it has been staged)
   2. it will apply its fixes over that last version, not over what was staged.

Although this should be "obvious" for anyone with sufficient experience with Git, it can be disconcerting for many users, and still annoying for the ones who understand what's going on.

Solution Proposal

When there is a discrepancy between "staged" and "local filesystem", Biome could issue a warning message and return with an exit status different than 0 to signal a problem.

When there is a discrepancy between "staged" and "local filesystem", Biome could issue a warning message and, in case we also have flags such as --apply or --write (when we tell Biome to modify files), also return with an exit status different than 0 to signal a problem.

• An extra refinement related to when to return an exist status different than 0 would be to include an extra condition: Biome must have fixed (or tried to fix) some of the files that have changes in the "local filesystem" after being staged. Summarizing, the conditions would be:
  • --staged flag
  • there is at least one staged file F that was changed after being staged
  • Biome is configured to write changes on files (with --apply, --write or other mechanisms)
  • Biome "wants" to fix that problematic F file (at this point, we know that there is risk for "git conflicts", so returning an error would be justified).

This involves an extra git command execution when the --staged flag is passed, but it shouldn't impact execution time by much (in any case, given the use cases for --staged, I think the small time penalty is worth it).

Final notes

• I am willing to contribute a patch for this small change.

[Conaclos]

Biome could issue a warning message and return with an exit status different than 0 to signal a problem.

We could just emit a warning without changing the exit status. The exit status should certainly be set by the formatter/linter (pass or don't pass).

[Conaclos]

On second though I think your initial proposal is good.
We definitively need more opinion on this because it is not so easy.

Here my reasoning:
Assuming that --staged is used before committing to check that the committed changes are linted and formatted,
we should emit an error regardless of the options used (with and without --write/--apply) and exit with a non-zero code if there are unstaged changes. Because the result of the check is likely to be invalid. We could then suggest users to stash the unstaged changes using git stash push --keep-index.

We should certainly ignore unstaged changes to files ignored by Biome. Unfortunately, I am not sure that this is easy to implement in the current codebase. Perhaps we could accept this caveat.

Also, I think we should not change the files (i.e. ignore --write/--apply) if there are unstaged changes because this will add more unstaged changes and could confuse the user. We could simply abort early format/lint/check if there are unstaged changes?

[castarco]

(I'm responding "out of order")

We should certainly ignore unstaged changes to files ignored by Biome. Unfortunately, I am not sure that this is easy to implement in the current codebase

From what I saw in the code, I had the impression it would be feasible, but I admit I didn't investigate it thoroughly, so I was probably misguided.

On second though I think your initial proposal is good.

we should emit an error regardless of the options used

Also, I think we should not change the files (i.e. ignore --write/--apply) if there are unstaged changes because this will add more unstaged changes and could confuse the user.

At some point I became convinced that it was too coarse 😅 . In part because there will be many files that Biome leaves unprocessed (for example... idk, PHP files 🤣 ), and whether they are staged or not, or changed after being staged, (imho) should not affect Biome's exit status.

if there are unstaged changes because this will add more unstaged changes and could confuse the user.

I'm thinking on opening a new proposal for that thing I provisionally named --list-fixes 🤔 . Having an output that is only the list of fixed files makes it easy to work around that specific concern.

If --list-fixes (just a provisional name) was implemented before changes on --staged, it could be easier to have fine-grained checks without risking surprises or confusing behavior.

[Conaclos]

there will be many files that Biome leaves unprocessed (for example... idk, PHP files 🤣 ), and whether they are staged or not, or changed after being staged, (imho) should not affect Biome's exit status.

This is why I could be nice to "ignore unstaged changes to files ignored by Biome". We have a CLI option --files-ignore-unknown=true that makes Biome ignores unknown files.

[castarco]

--files-ignore-unknown=true wouldn't cover files that are explicitly added to the "ignore" list but are "known", right?

[Conaclos]

Yep. It only ignores files with extensions with don't handle such as .php. We already recommend its use inside a git hook.