The property-based test exposed another bug; this can be reproduced in
the following way:
$ touch empty-file
$ jj commit -m "add non-executable file"
$ jj split
...then select the file mode change interactively and give the split-off
change a name. Since all the changes have been selected, one would
expect the first part to contain the file mode change and the second
part to be empty, but in fact:
First part: nrkmknyr 785bfe6e (empty) bla
Second part: qmwwknqx b5a92967 (no description set)
Doing the equivalent thing non-interactively has the expected result:
$ jj undo
$ jj split empty-file
Warning: All changes have been selected, so the second commit will be empty
First part: nrkmknyr 3f156f9e first
Second part: zlurmpxm 06e24b2d (empty) (no description set)
This problem only occurs when the file is empty.
When the file mode of an empty text file changes, scm-record produces
`SelectedContents::Unchanged`, even though there is a change that
needs to be addressed. AFAICT, empty files are the only situation
where `Unchanged` is produced. If the file mode of a non-empty file
is changed, `SelectedContent::Text` is produced, even though the file
itself has not changed and does not have to be written to the store.
With the current semantics, IMHO `Unchanged` is a little misleading and
acts more like an `Empty`.
Knowing this, it is possible to fix the bug by just writing an empty
file. However, this is a little wasteful to have to write the file blob
anew when it hasn't changed (both when the file is empty or not). It
would be nicer to have scm-record report an actual `Unchanged` (also
when a non-empty file hasn't changed) and to have some way to update the
executable flag in `MergedTreeBuilder` rather then having to provide a
new value from scratch.
scm-record yields a selected change with `FileMode::Absent` in one of
two cases:
- when an existing file is deleted and this change is selected,
- when a new file is created and this change is not selected.
From the information provided by `File::get_selected_contents`, it is
not possible to distinguish these two cases. In the first case, the
tree definitely needs to change to reflect the deletion, so a "tombstone"
override is set on the tree builder. In the second case, that tombstone
is usually ignored when `TreeBuilder::write_tree()` processes the
overrides because the associated file does not exist.
However, when the tombstone happens to coindice with a deleted directory
and a new file has been created in its place, but neither change is
selected, then the tombstone had the effect of deleting the directory
from the tree.
This is fixed now by ignoring tombstones on trees. When a directory is
actually intentionally deleted, this will result in a bunch of tombstones
on the files therein; after processing the file overrides, `write_tree()`
will remove any now-empty directory trees anyway.
Perhaps, a better solution would see scm-record provide information to
distinguish a selected deletion from an unselected creation to avoid
placing the tombstone altogether. For now, this fixes the test case
`test_edit_diff_builtin_replace_directory_with_file` and one failure
source of the property-based tests.
This adds the proptest crate for property-based testing as well as the
proptest-state-machine crate as direct dev dependencies of jj-cli and as
dependencies of the internal testutils crate.
Within testutils, a `proptest` module provides a reference state machine
which models a repository workspace as a tree-like data structure whose
leaves represent `File`s. The possible transitions of this state machine
are for now limited to the creation of new files (including replacements
of existing files or directories) and deletions of files (pruning the
tree of empty directory nodes). Additional transitions (moving files,
modifying file contents incrementally, ...) and states (symlinks,
submodules, conflicts, ...) may be added in the future.
The `ReferenceStateMachine` trait implementation provides proptest
with strategies for the generation and shrinking of both the initial
state and the transitions that are replayed on it; by shrinking the
transitions rather than another independent reference state, proptest
can search of a failing test input with a minimal diff. This makes this
approach quite suited to VCS problems.
This reference state machine is then applied to the builtin merge-tool's
test suite:
- The initial state is used to build a corresponding `MergedTree`. Its
ID is used for the fixed "left" tree and serves as the starting point
for the right tree.
- As transitions are applied, the right tree is updated accordingly.
- Each step of the way, the same test logic as in the manual
`test_edit_diff_builtin*` tests is run to check that splitting off
none or all of the changes results in the left or right tree,
respectively.
Aside from the bug already captured by `*_replace_directory_with_file`,
the property-based test found an independent problem related to file
mode changes of empty files. Regression test seeds for both of these
issues are also checked in. This ensures that others / CI will reproduce
known edge cases deterministically before randomly exploring additional
onwards.
This is a manual regression test for issue #5189. When this change is
replaced onto `v0.28.2`, the test produces a panic, as described in the
issue.
As of #6411, the panic no longer happens. However, the test still fails
and rightfully so: When following the reproduction and not selecting any
change in `jj split`, the split-off (first) commit will still record the
deletion of `folder/.keep` but not the creation of the file `folder`.
This change, from an enum to a struct, is a more accurate representation
of the actual way that a ConfigPath works; additionally, it lets us add
different information without modifying every single enumeration field.
I'm trying to refactor property wrapping functions, and noticed that it's odd
that .wrap_<property>() does boxing internally whereas .wrap_template() doesn't.
Also, it sometimes makes sense to turn property into trait object earlier. For
example, we can deduplicate L::wrap_boolean() in build_binary_operation().
This also adds a test case for the completion of arguments following
multi-argument aliases, to cover the bug reported in issue #5377.
The default command is like a special kind of alias, one which is
expanded from zero tokens. Consequently, it also triggers the bug
#5377 on bash/zsh, even if the `default-command` is just a single token.
The fix is along the lines sketched out by the "TODO" comment. Bash and
Zsh don't behave identical, so the padding ([""]) still needs to be
applied (and removed) conditionally in a disciplined manner.
The completion mechanism works differently in different shells:
For example, when the command line `jj aaa bb ccc` is completed at the
end of the `bb` token, bash and zsh pass the completer the whole line
`-- jj aaa bb ccc` and an index of 2 which refers to the `bb` token;
they are then expected to complete `bb`. Meanwhile, fish and Powershell
only pass the command up to the completion point, so `-- jj aaa bb`;
the completer is always expected to complete the last token. In all
cases, the shell ultimately decides what to do with the completions,
e.g. to complete up to a common prefix (bash), to show an interactive
picker (zsh, fish), or to insert the completion if it is the only one
(all shells). Remaining tokens (`ccc`) are also always appended by the
shell and not part of the completion.
While this is mostly handled by the clap_complete crate, we do expand
aliases and present clap_complete with a fake view of the real command
line, thereby reaching into its internals by wrapping the interface
between the completion shell script that is provided by clap_complete
and its Rust code in `CommandEnv::try_complete()`. If we get this wrong,
completion might yield unexpected results, so it is worth testing
completion for both flavors of shells whenever aliases are potentially
in the mix.
To avoid redundancy, the shell-specific invocation of `jj` is factored
into a `complete_at()` function of the test fixture. The `test-case`
crate is then used to instantiate each test case for different values of
clap_complete's `Shell` enum.
filter
bash/zsh specific behavior
move impl
The `CliRunner` lets a custom binary add processing that should happen
before running a command. This patch replaces that by a hook that can
do processing before and/or after. I thought I would want to use this
for adding telemetry (such as timings) to our custom binary at
Google. I ended up adding that logging outside of `CliRunner::run()`
instead, so it gives a more accurate timing of the whole invocation. I
think this patch is still an improvement, as it's more generic than
the start hook.
In builtin diff editor, we materializes conflicts, so we need to parse them
back to reproduce the original (or partially-resolved) contents. OTOH, the
merge editor should write the merged contents transparently.
This change also revealed that binary hunks wouldn't be processed correctly in
the merge editor.
make_diff_files() will be async function that uses materialized_diff_stream()
internally. apply_diff_builtin() will take callbacks to handle binary/conflict
files.
If `git.fetch` contains remotes that are not available, we currently error even
if other remotes are available. For common fork workflows with separate
`upstream` and `origin` remotes (for example), this requires a user to either
set both remotes in their user config and override single-remote repos or set
only one in their user config and override all multi-remote repos to fetch from
`upstream` (or both).
This change updates fetching to only *warn* about unknown remotes **if** other
remotes are available. If none of the configured remotes are available, an error
is still raised as before.
These days, we usually think of conflicts as one base state and series
of diffs between other states. The base state is normally the parent
when rebasing.
Also, we're deprecated `jj backout` in favor of `jj revert`, so let's
use that terminology.
When we ask the user to prodive a commit description, we currently
write a file to `.jj/repo/` with the draft description and then pass
that to the editor. If the editor exits with an error status, we leave
the file in place and tell the user about the path so they can recover
the description. I'm not sure I've ever used one of these files. I
have certainly never used a file that's not from the most recent
edit. I have, however, cleaned up old such files. This patch changes
the code so we write them to /tmp instead, so we get the cleanup for
free.
A pattern has emerged where a integration tests check for the
availability of an external tool (`git`, `taplo`, `gpg`, ...) and skip
the test (by simply passing it) when it is not available. To check this,
the program is run with the `--version` flag.
Some tests require that the program be available at least when running
in CI, by calling `ensure_running_outside_ci` conditionally on the
outcome. The decision is up to each test, though, the utility merely
returns a `bool`.
The `CommandNameAndArgs` struct is used in multiple places to specify
external tools. Previously, the schema only allowed for this in
`ui.pager`.
This commit adds a few sample configs which define variables editors and
fix tools as commands with env vars.
The schema has also been updated to make these valid.
Not sure if that is intentional or should rather be considered a bug,
but currently the "structured" option for specifying an external tool
requires that both the command "command" and the "env" keys are
specified. The "command" key makes sense; for the "env" key it came as
a surprise, but it can be argued that this form should only be used when
environment variables need to be specified and the plain string or array
form should be used otherwise.
Either way, the schema did not accurately reflect the current behavior;
now it does. Two sample configs have been added as schema test cases.
Anytime an external tool is referenced in the config, the command can be
provided as a string or as a token array. In the latter case, the array
must not be empty; at least the command name must be provided.
The schema didn't previously object to an empty array, though; this has
now been rectified. I've added more sample configs to cover this case.
Those same configs can also be used to illustrate that this is indeed
jj's current behavior:
$ jj --config-file cli/tests/sample-configs/invalid/ui.pager_empty_array.toml show
Config error: Invalid type or value for ui.pager
Caused by: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/ui.pager.command_empty_array.toml show
Config error: Invalid type or value for ui.pager
Caused by: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/ui.editor_empty_array.toml config edit --user
Config error: Invalid type or value for ui.editor
Caused by: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/ui.diff-editor_empty_array.toml split
Error: Failed to load tool configuration
Caused by:
1: Invalid type or value for ui.diff-editor
2: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/ui.merge-editor_empty_array.toml resolve
Error: Failed to load tool configuration
Caused by:
1: Invalid type or value for ui.merge-editor
2: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/ui.diff.tool_empty_array.toml diff
Config error: Invalid type or value for ui.diff.tool
Caused by: data did not match any variant of untagged enum CommandNameAndArgs
$ jj --config-file cli/tests/sample-configs/invalid/fix.tools.command_empty_array.toml fix
Config error: Invalid type or value for fix.tools.black
Caused by: data did not match any variant of untagged enum CommandNameAndArgs
in `command`
As a notable exception, `ui.default-command` *is* allowed to be an empty
array. In that case, `jj` will print a usage message. This is also
covered by a valid sample config.
While `ui.pager` can be a string which will be tokenized on whitespace,
and argument token array, or a command/env table, the `command` key
within that table currently must be an array. The schema previously
explicitly also allowed it to be a string but that does not actually
work, as exemplified by running:
```sh
$ jj --config-file cli/tests/sample-configs/invalid/ui.pager_command-env_string.toml config list
Config error: Invalid type or value for ui.pager
Caused by: data did not match any variant of untagged enum CommandNameAndArgs
```
`CommandNameAndArgs` should potentially be changed to allow strings.
For now, the schema has been updated to reflect the status quo. A new
sample toml has been added to the `invalid` directory to cover this;
prior to updating the schema, this new test case failed. Once the
behavior is changed to allow string, the file merely needs to be moved
to `valid`.
These are two more instances where the default values were wrong and in
fact not even consistent with the schema itself.
I've found these by running
```sh
jq -r 'paths(type == "object" and has("default")) as $p | getpath($p).default | tojson as $v | $p | map("\"\(select(. != "properties"))\"") | join(".") as $k | "\($k) = \($v)"' cli/src/config-schema.json | taplo check --schema=file://$PWD/cli/src/config-schema.json -
```
which uses `jq` to filter the default values from the schema definition
to create a rudimentary TOML file containing all the defaults according
to the schema and then uses `taplo` the validate this TOML against the
schema.
This approach could be developed further to also parse the intermediate
TOML file and compare the result with the default config (from parsing
an empty config). That would not only test for self-consistency of the
schema's proclaimed defaults but also for consistency with the actual
defaults as assumed by jj.
Adds a bunch of additional sample config toml files. Via the
`datatest_runner`, these each correspond to a test case to check that
the toml is correctly (in-)validated according to the schema.
The `valid/*.toml` files typically define multiple related config
options at once. Where there's some overlap with the default configs in
`cli/src/config`, the aim was to choose different allowed values, e.g.
hex colors, file size in bytes (numeric), etc.
The `invalid/*.toml` files typically only define a single offending
property such as to not obscure individual false negatives. All of the
"invalid" files are still valid toml as the aim is not to test the
`toml_edit` crate or Taplo.
The sample files all contain a Taplo schema directive. This allows them
to be validated against the schema on the fly by Taplo's LSP and derived
IDE plugins to speed up editing and immediately highlight offending
options.
Closes#5695.
The `datatest-stable` crate allows to dynamically instantiate test cases
based on available files. This is applied to `test_config_schema` to
create one test case per config file. As case in point, the test case
for `hints.toml` was missing previously, hence the total number of tests
is up one.
This will become useful when adding more config examples to somewhat
exhaust the schema.
`datatest-stable` uses a custom test harness and thus cannot be used in
the same integration test binary that all of the other test modules run
in. However, if data-driven tests are to be used for other applications,
they can share in the same binary, so the module structure is already
set up to mirror the central "runner" approach.
The previous implementation of `assert_no_forgotten_test_files`
hard-coded the name of the `runner` integration test and required all
other source files to appear in matching `mod` declarations. Thus, this
approach cannot handle multiple integration tests.
However, additional integration tests may be desirable
- to support tests using a custom test harness (see upcoming commits)
- to balance the trade-off between test run time and compile time as
the test suite grows in the future.
The new implementation first uses `taplo` to parse the `[[test]]`
sections of the manifest to identify integration test main modules,
and then searches in those for `mod` declarations. This is then compared
to the list of source files in the tests directory. Like the previous
implementation, the new one does not attempt to recurse into submodules
or to handle directory-style modules; just like before it only treats
source files without a module declaration as an error and relies on the
compiler to complain about the other way around.
When `taplo` is not installed, the check is skipped unless it is running
in CI where we require `taplo` to be available.
