[pull] master from git:master#202
Merged
Merged
Conversation
* convert commands to synopsis style
* use _<placeholder>_ for arguments
* fix conditional text to sentence limits
Signed-off-by: Jean-Noël Avila <jn.avila@free.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
* convert commands and options to synopsis style
* use _<placeholder>_ for arguments
* small style fixes
Signed-off-by: Jean-Noël Avila <jn.avila@free.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
* convert commands and options to synopsis style
* use _<placeholder>_ for arguments
* small style fixes
Signed-off-by: Jean-Noël Avila <jn.avila@free.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
* convert commands and options to synopsis style
* use _<placeholder>_ for arguments
Signed-off-by: Jean-Noël Avila <jn.avila@free.fr>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
…alid The negation in front of the object existence check in cache_tree_fully_valid() was lost in 062b914 (treewide: convert users of `repo_has_object_file()` to `has_object()`, 2025-04-29), turning `!repo_has_object_file(...)` into `has_object(...)` instead of `!has_object(...)`. This makes cache_tree_fully_valid() always report the cache tree as invalid when objects exist (the common case), forcing callers like write_index_as_tree() to call cache_tree_update() on every invocation. An odb_has_object() check inside update_one() avoids a full tree rebuild, but the unnecessary call still pays the cost of opening an ODB transaction and, in partial clones, a promisor remote check. Restore the missing negation and add a test that verifies write-tree takes the cache-tree shortcut when the cache tree is valid. Helped-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: David Lin <davidlin@stripe.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a server advertises promisor remotes and the client accepts some of them, those remotes carry the server's intent: 'fetch missing objects preferably from here', and the client agrees with that for the remotes it accepts. However promisor_remote_get_direct() actually iterates over all promisor remotes in list order, which is the order they appear in the config files (except perhaps for the one appearing in the `extensions.partialClone` config variable which is tried last). This means an existing, but not accepted, promisor remote, could be tried before the accepted ones, which does not reflect the intent of the agreement between client and server. If the client doesn't care about what the server suggests, it should accept nothing and rely on its remotes as they are already configured. To better reflect the agreement between client and server, let's make promisor_remote_get_direct() try the accepted promisor remotes before the non-accepted ones. Concretely, let's extract a try_promisor_remotes() helper and call it twice from promisor_remote_get_direct(): - first with an `accepted_only=true` argument to try only the accepted remotes, - then with `accepted_only=false` to fall back to any remaining remote. Ensuring that accepted remotes are preferred will be even more important if in the future a mechanism is developed to allow the client to auto-configure remotes that the server advertises. This will in particular avoid fetching from the server (which is already configured as a promisor remote) before trying the auto-configured remotes, as these new remotes would likely appear at the end of the config file, and as the server might not appear in the `extensions.partialClone` config variable. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `in_list == 0` path of all_fields_match() looks up the remote in `config_info` by `advertised->name` repeatedly, even though every caller in should_accept_remote() has already performed this lookup and holds the result in `p`. To avoid this useless work, let's replace the `int in_list` parameter with a `struct promisor_info *config_entry` pointer: - When NULL (ACCEPT_ALL mode): scan the whole `config_info` list, as the old `in_list == 1` path did. - When non-NULL: match against that single config entry directly, avoiding the redundant string_list_lookup() call. This removes the hidden dependency on `advertised->name` inside all_fields_match(), which would be wrong if in the future auto-configured remotes are implemented, as the local config name may differ from the server's advertised name. While at it, let's also add a comment before all_fields_match() and match_field_against_config() to help understand how things work and help avoid similar issues. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In should_accept_remote() and parse_one_advertised_remote(), when a remote is ignored, we tell users why it is ignored in a warning, but we don't tell them that the remote is actually ignored. Let's clarify that, so users have a better idea of what's actually happening. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In parse_one_advertised_remote(), we check for a NULL remote name and
remote URL, but not for empty ones. An empty URL seems possible as
url_percent_decode("") doesn't return NULL.
In promisor_config_info_list(), we ignore remotes with empty URLs, so a
Git server should not advertise remotes with empty URLs. It's possible
that a buggy or malicious server would do it though.
So let's tighten the check in parse_one_advertised_remote() to also
reject empty strings at parse time.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A previous commit made sure we now reject empty URLs early at parse time. This makes the existing warning() in case a remote URL is NULL or empty very unlikely to be useful. In future work, we also plan to add URL-based acceptance logic into should_accept_remote(). To adapt to previous changes and prepare for upcoming changes, let's restructure the control flow in should_accept_remote(). Concretely, let's: - Replace the warning() in case of an empty URL with a BUG(), as a previous commit made sure empty URLs are rejected early at parse time. - Move that modified empty-URL check to the very top of the function, so that every acceptance mode, instead of only ACCEPT_KNOWN_URL, is covered. - Invert the URL comparison: instead of returning on match and warning on mismatch, return early on mismatch and let the match case fall through. This opens a single exit path at the bottom of the function for future commits to extend. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In a future commit we are going to check if some strings contain control characters, so let's refactor the logic to do that in a new has_control_char() helper function. It cleans up the code a bit anyway. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In future commits, we are going to add more logic to filter_promisor_remote() which is already doing a lot of things. Let's alleviate that by moving the logic that checks and validates the value of the `promisor.acceptFromServer` config variable into its own accept_from_server() helper function. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In filter_promisor_remote(), the instances of `struct promisor_info` for accepted remotes are dismantled into separate parallel data structures (the 'accepted' strvec for server names, and 'accepted_filters' for filter strings) and then immediately freed. Instead, let's keep these instances on an 'accepted_remotes' list. This way the post-loop phase can iterate a single list to build the protocol reply, apply advertised filters, and mark remotes as accepted, rather than iterating three separate structures. This refactoring also prepares for a future commit that will add a 'local_name' member to 'struct promisor_info'. Since struct instances stay alive, downstream code will be able to simply read both names from them rather than needing yet another parallel strvec. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In a previous commit, filter_promisor_remote() was refactored to keep accepted 'struct promisor_info' instances alive instead of dismantling them into separate parallel data structures. Let's go one step further and replace the 'struct strvec *accepted' argument passed to filter_promisor_remote() with a 'struct string_list *accepted_remotes' argument. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In t5710, we frequently construct local file URIs using `file://$(pwd)`. On Unix-like systems, $(pwd) returns an absolute path starting with a slash (e.g., `/tmp/repo`), resulting in a valid 3-slash URI with an empty host (`file:///tmp/repo`). However, on Windows, $(pwd) returns a path starting with a drive letter (e.g., `D:/a/repo`). This results in a 2-slash URI (`file://D:/a/repo`). Standard URI parsers misinterpret this format, treating `D:` as the host rather than part of the absolute path. This is to be expected because RFC 8089 says that the `//` prefix with an empty local host must be followed by an absolute path starting with a slash. While this hasn't broken the existing tests (because the old `promisor.acceptFromServer` logic relies entirely on strict `strcmp()` without normalizing the URLs), it will break future commits that pass these URLs through `url_normalize()` or similar functions. To future-proof the tests and ensure cross-platform URI compliance, let's introduce a $TRASH_DIRECTORY_URL helper variable that explicitly guarantees a leading slash for the path component, ensuring valid 3-slash `file:///` URIs on all operating systems. While at it, let's also introduce $ENCODED_TRASH_DIRECTORY_URL to handle some common special characters in directory paths. To be extra safe, let's skip all the tests if there are uncommon special characters in the directory path. Then let's replace all instances of `file://$(pwd)` with $TRASH_DIRECTORY_URL across the test script, and let's simplify the `sendFields` and `checkFields` tests to use $ENCODED_TRASH_DIRECTORY_URL directly. Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
get_files_ref_lock_timeout_ms() calls repo_config_get_int() using the_repository, as no repository instance is available in its scope. Add a struct repository parameter and use it instead of the_repository. Update all callers accordingly. In files-backend.c, lock_raw_ref() can obtain repository instance from the struct ref_transaction via transaction->ref_store->repo and pass it down. For create_reflock(), which is used as a callback, introduce a small wrapper struct to pass both struct lock_file and struct repository through the callback data. This reduces reliance on the_repository global, though the function still uses static variables and is not yet fully repository-scoped. This can be addressed in a follow-up change. Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> Acked-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs.c uses the_hash_algo in multiple places, relying on global state for the object hash algorithm. Replace these uses with the appropriate repository-specific hash_algo. In transaction-related functions (ref_transaction_create, ref_transaction_delete, migrate_one_ref, and transaction_hook_feed_stdin), use transaction->ref_store->repo->hash_algo. In other cases, such as repo_get_submodule_ref_store(), use repo->hash_algo. Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> Acked-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
reftable_be_init() and reftable_be_create_on_disk() use the_repository even though a repository instance is already available, either directly or via struct ref_store. Replace these uses with the appropriate local repository instance (repo or ref_store->repo) to avoid relying on global state. Note that USE_THE_REPOSITORY_VARIABLE cannot be removed yet, as is_bare_repository() is still there in the file. Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> Acked-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We missed the cut-off for Rust by default in 2.53, but we still can enable it by default for 2.54, so update our breaking changes document accordingly. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We'll make Rust the default in a future commit, so be sure to install Cargo (which will also install Rust) to prepare for that case. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Older versions of Rust on Linux, such as that used in Debian 11 in our CI, require linking against libdl. Were we linking with Cargo, this would be included automatically, but since we're not, explicitly set it in the system-specific config. This library is part of libc, so linking against it if it happens to be unnecessary will add no dependencies to the resulting binary. In addition, it is provided by both glibc and musl, so it should be portable to almost all Linux systems. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Our breaking changes document says that we'll enable Rust by default in Git 2.54. Adjust the Makefile to switch the option from WITH_RUST to NO_RUST to enable it by default and update the help text accordingly. Similarly, for Meson, enable the option by default and do not automatically disable it if Cargo is missing, since the goal is to help users find where they are likely to have problems in the future. Update our CI tests to swap out the single Linux job with Rust to a single job without, both for Makefile and Meson. Similarly, update the Windows Makefile job to not use Rust, while the Meson job (which does not build with ci/lib.sh) will default to having it enabled. Move the check for Cargo in the Meson build because it is no longer needed in the main script. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
There is an old pre-existing memory leak in repo_init() due to failing to call clear_repository_format() in the error case. It went undetected because a specific bug is required to trigger it: enable a v1 extension in a repository with format v0. Obviously this can only happen in a development environment, so it does not trigger in normal usage, however the memleak is real and needs fixing. Fix it by also calling clear_repository_format() in the error case. Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Next commits add a 'hook.jobs' config option of type 'unsigned int', so add a helper to parse it since the API only supports int and ulong. An alternative is to make 'hook.jobs' an 'int' or parse it as an 'int' then cast it to unsigned, however it's better to use proper helpers for the type. Using 'ulong' is another option which already has helpers, but it's a bit excessive in size for just the jobs number. Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The hook.jobs config is a global way to set hook parallelization for all hooks, in the sense that it is not per-event nor per-hook. Finer-grained configs will be added in later commits which can override it, for e.g. via a per-event type job options. Next commits will also add to this item's documentation. Parse hook.jobs config key in hook_config_lookup_all() and store its value in hook_all_config_cb.jobs, then transfer it into r->jobs after the config pass completes. This is mostly plumbing and the cached value is not yet used. Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Hooks always run in sequential order due to the hardcoded jobs == 1 passed to run_process_parallel(). Remove that hardcoding to allow users to run hooks in parallel (opt-in). Users need to decide which hooks to run in parallel, by specifying "parallel = true" in the config, because Git cannot know if their specific hooks are safe to run or not in parallel (for e.g. two hooks might write to the same file or call the same program). Some hooks are unsafe to run in parallel by design: these will marked in the next commit using RUN_HOOKS_OPT_INIT_FORCE_SERIAL. The hook.jobs config specifies the default number of jobs applied to all hooks which have parallelism enabled. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
pre-push is the only hook that keeps stdout and stderr separate (for backwards compatibility with git-lfs and potentially other users). This prevents parallelizing it because run-command needs stdout_to_stderr=1 to buffer and de-interleave parallel outputs. Since we now default to jobs=1, backwards compatibility is maintained without needing any extension or extra config: when no parallelism is requested, pre-push behaves exactly as before. When the user explicitly opts into parallelism via hook.jobs > 1, hook.<event>.jobs > 1, or -jN, they accept the changed output behavior. Document this and let get_hook_jobs() set stdout_to_stderr=1 automatically when jobs > 1, removing the need for any extension infrastructure. Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Several hooks are known to be inherently non-parallelizable, so initialize them with RUN_HOOKS_OPT_INIT_FORCE_SERIAL. This pins jobs=1 and overrides any hook.jobs or runtime -j flags. These hooks are: applypatch-msg, pre-commit, prepare-commit-msg, commit-msg, post-commit, post-checkout, and push-to-checkout. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Expose the parallel job count as a command-line flag so callers can request parallelism without relying only on the hook.jobs config. Add tests covering serial/parallel execution and TTY behaviour under -j1 vs -jN. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a hook.<event>.jobs count config that allows users to override the
global hook.jobs setting for specific hook events.
This allows finer-grained control over parallelism on a per-event basis.
For example, to run `post-receive` hooks with up to 4 parallel jobs
while keeping other events at their global default:
[hook]
post-receive.jobs = 4
Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Allow -1 as a value for hook.jobs, hook.<event>.jobs, and the -j CLI flag to mean "use as many jobs as there are CPU cores", matching the convention used by fetch.parallel and other Git subsystems. The value is resolved to online_cpus() at parse time so the rest of the code always works with a positive resolved count. Other non-positive values (0, -2, etc) are rejected with a warning (config) or die (CLI). Suggested-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We recently fixed a bug in commit 2226ffa (run_processes_parallel(): fix order of sigpipe handling, 2026-04-08) where a hook that caused us to get SIGPIPE would accidentally trigger the run_processes_parallel() cleanup handler killing the child processes. For a single hook, this meant killing the already-exited hook. This case was triggered by our tests, but was only a problem on some platforms. But if you have multiple hooks running in parallel, this causes a problem everywhere, since one hook failing to read its input would take down all hooks. Now that we have parallel hook support, we can add a test for this case. It should pass already, due to the existing fix. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The scheme driver separates identifiers only at parentheses of all sorts and whitespace, except that vertical bars act as brackets that enclose an identifier. The test case attempts to demonstrate the vertical bars with a change from 'some-text' to '|a greeting|'. However, this misses the goal because the same word coloring would be applied if '|a greeting|' were parsed as two words. Have an identifier between vertical bars with a space in both the pre- and the post-image and change only one side of the space to show that the single word exists between the vertical bars. Also add cases that change parentheses of all kinds in a sequence of parentheses to show that they are their own word each. Signed-off-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Scott L. Burson <Scott@sympoiesis.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Common Lisp has top-level forms, such as 'defun' and 'defmacro', that are not matched by the current Scheme pattern. Also, it is more common in CL, when defining user macros intended as top-level forms, to prefix their names with "def" instead of "define"; such forms are also not matched. And some top-level forms don't even begin with "def". On the other hand, it is an established formatting convention in the Lisp community that only top-level forms start at the left margin. So matching any unindented line starting with an open parenthesis is an acceptable heuristic; false positives will be rare. However, there are also cases where notionally top-level forms are grouped together within some containing form. At least in the Common Lisp community, it is conventional to indent these by two spaces, or sometimes one. But matching just an open parenthesis indented by two spaces would be too broad; so the pattern added by this commit requires an indented form to start with "(def". It is believed that this strikes a good balance between potential false positives and false negatives. Signed-off-by: Scott L. Burson <Scott@sympoiesis.com> Acked-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Most unfortunately macOS does not support st_[amc]tim for timestamps down to nanosecond resolution as POSIX systems. Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have a couple of calls to `test_match_signal ()` where we execute a Git command and expect it to die with a specific signal. These calls will essentially execute the process in a subshell via `foo; echo $?`, but as we expect `foo` to fail this will cause the overall subshell to fail once we `set -e`. Fix this issue by using `foo && echo 0 || echo $?` instead. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The helper function `test_must_fail ()` executes a specific Git command that may or may not fail in a specific way. This is done by executing the command in question and then comparing its exit code against a set of conditions. This works, but once we run our test suite with `set -e` we may bail out of `test_must_fail ()` early in case the command actually fails, even though we expect it to fail. Prepare for this change by handling the failed case with `||`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have a couple of calls to `stop_git_daemon ()` outside of specific
test cases that will kill a backgrounded git-daemon(1) process and
expect the process with a specific error code. While these function
calls do end up killing git-daemon(1), the error handling we have in
those contexts is basically ineffective. So while we expect the process
to exit with a specific error code, we will just continue with any error
in case it doesn't.
This will change once we enable `set -e` in a subsequent commit. There's
two issues though that will make this _always_ fail:
- Our call to `wait` is expected to fail, but because it's not part of
a condition it will cause us to bail out immediately with `set -e`.
- We try to kill git-daemon(1) a second time via the pidfile. We can
generally expect that this is the same PID though as we had in the
"GIT_DAEMON_PID" environment variable, and thus it's more likely
than not that we have already killed it, and the call to kill will
fail.
Prepare for this change by handling the failure of `wait` with `||` and
by silencing failures of the second call to `kill`.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have a couple of calls to `git config --unset` that ultimately end up as no-ops as the configuration variables aren't set (anymore) in the first place. These calls are mostly intended to recover unconditionally from tests that may have executed only partially, but they'll ultimately fail during a normal test run. This hasn't been a problem until now as we aren't running tests with `set -e`. This is about to change though, so let's silence the case where we cannot unset the config keys. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have some test in our test suite where we use the pattern of `test ... && test_expect_succeess` to conditionally execute a test. The problem is that when we decide to not execute the test, we'll indeed skip the test, but the overall statement will also be unsuccessful. This will become a problem once we enable `set -e`. Prepare for this future by turning this into a proper conditional, which is also a bit easier to read overall. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Several of our tests verify whether a certain binary can be executed, potentially skipping tests in case we cannot, for example because the binary doesn't exist. In those cases we often run the binary outside of any conditionally. This will start to fail once we enable `set -e`, as that will cause us to bail out the test immediately. Improve these tests by executing them inside of a conditional instead. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Both `test_when_finished ()` and `test_atexit ()` build up a chain of
cleanup commands by prepending each new command to the existing cleanup
string. To preserve the exit code of the test body across cleanup
execution, we append the following logic:
} && (exit "$eval_ret"); eval_ret=$?; ...
The intent of this is to run the cleanup block and then unconditionally
restore `eval_ret`. The original behaviour of this is is:
+------------------+---------+------------------------------------+
|test body │ cleanup │ old behaviour │
+------------------+---------+------------------------------------+
│pass (eval_ret=0) | pass │ && taken -> (exit 0) -> eval_ret=0 |
+------------------+---------+------------------------------------+
│pass (eval_ret=0) | fail │ && not taken -> eval_ret=$? |
+------------------+---------+------------------------------------+
│fail (eval_ret=1) | pass │ && taken -> (exit 1) -> eval_ret=1 |
+------------------+---------+------------------------------------+
│fail (eval_ret=1) | fail | && not taken -> eval_ret=$? |
+------------------+---------+------------------------------------+
This logic will start to fail once we enable `set -e`. When `$eval_ret`
is non-zero, the subshell we create will fail, and with `set -e` we'll
thus bail out without evaluating the logic after the semicolon.
Fix this issue by instead using `|| eval_ret=\$?; ...`. Besides being
a bit simpler, it also retains the original behaviour:
+------------------+---------+------------------------------------+
|test body │ cleanup │ old behaviour │
+------------------+---------+------------------------------------+
│pass (eval_ret=0) | pass │ || not taken -> eval_ret unchanged |
+------------------+---------+------------------------------------+
│pass (eval_ret=0) | fail │ || taken -> eval_ret=$? |
+------------------+---------+------------------------------------+
│fail (eval_ret=1) | pass │ || not taken -> eval_ret unchanged |
+------------------+---------+------------------------------------+
│fail (eval_ret=1) | fail | || taken -> eval_ret=$? |
+------------------+---------+------------------------------------+
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In t0008 we use `grep -v` in a subshell, but expect that this command will sometimes not match anything. This would cause grep(1) to return an error code, but given that we don't run with `set -e` we swallow this error. We're about to enable `set -e`. Prepare for this by ignoring any errors. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In t1301 we're trying to remove any potentially-existing default ACLs that might exist on the transh directory by executing setfacl(1). According to 8ed0a74 (t1301-shared-repo.sh: don't let a default ACL interfere with the test, 2008-10-16), this is done because we play around with permissions and umasks in this test suite. The setfacl(1) binary may not exist on some systems though, even though tests ultimately still pass. This doesn't matter currently, but will cause the test to fail once we start running with `set -e`. Silence such failures by ignoring failures here. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In `test_bisection_diff ()` we use `expr` to perform some math. This command has some gotchas though in that it will only return success when the result is neither null nor zero. In some of our cases though it actually _is_ zero, and that will cause the expressions to fail once we enable `set -e`. Prepare for this change by instead using `$(( ))`, which doesn't have the same issue. While at it, modernize the function a tiny bit. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In t9902 we're using the `read` builtin to read some values into a variable. This is done by using `-d ""`, which cause us to read until the end of the heredoc. As the read is terminated by EOF, the command will end up returning a non-zero error code. This hasn't been an issue until now as we didn't run with `set -e`, but that'll change in a subsequent commit. Prepare for this change by not using read at all, as we can simply store the multi-line value directly. Suggested-by: SZEDER Gábor <szeder.dev@gmail.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have recently merged a patch series that had a simple misspelling of `test_expect_success`. Instead of making our tests fail though, this typo went completely undetected and all of our tests passed, which is of course unfortunate. This is a more general issue with our test suite: all commands that run outside of a specific test case can fail, and if we don't explicitly check for such failure then this failure will be silently ignored. Improve the status quo by enabling the errexit option so that any such unchecked failures will cause us to abort immediately. Note that for now, we only enable this option for Bash 5 and newer. This is because other shells have wildly different behaviour, and older versions of Bash (especially on macOS) are buggy. The list of enabled shells may be extended going forward. Helped-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Code clean-up to use the right instance of a repository instance in calls inside refs subsystem. * sp/refs-reduce-the-repository: refs/reftable-backend: drop uses of the_repository refs: remove the_hash_algo global state refs: add struct repository parameter in get_files_ref_lock_timeout_ms()
Doc mark-up updates. * ja/doc-difftool-synopsis-style: doc: convert git-describe manual page to synopsis style doc: convert git-shortlog manual page to synopsis style doc: convert git-range-diff manual page to synopsis style doc: convert git-difftool manual page to synopsis style
The check that implements the logic to see if an in-core cache-tree is fully ready to write out a tree object was broken, which has been corrected. * dl/cache-tree-fully-valid-fix: cache-tree: fix inverted object existence check in cache_tree_fully_valid
Promisor remote handling has been refactored and fixed in preparation for auto-configuration of advertised remotes. * cc/promisor-auto-config-url: t5710: use proper file:// URIs for absolute paths promisor-remote: remove the 'accepted' strvec promisor-remote: keep accepted promisor_info structs alive promisor-remote: refactor accept_from_server() promisor-remote: refactor has_control_char() promisor-remote: refactor should_accept_remote() control flow promisor-remote: reject empty name or URL in advertised remote promisor-remote: clarify that a remote is ignored promisor-remote: pass config entry to all_fields_match() directly promisor-remote: try accepted remotes before others in get_direct()
Hook scripts defined via the configuration system can now be configured to run in parallel. * ar/parallel-hooks: t1800: test SIGPIPE with parallel hooks hook: allow hook.jobs=-1 to use all available CPU cores hook: add hook.<event>.enabled switch hook: move is_known_hook() to hook.c for wider use hook: warn when hook.<friendly-name>.jobs is set hook: add per-event jobs config hook: add -j/--jobs option to git hook run hook: mark non-parallelizable hooks hook: allow pre-push parallel execution hook: allow parallel hook execution hook: parse the hook.jobs config config: add a repo_config_get_uint() helper repository: fix repo_init() memleak due to missing _clear()
Doc update. * jc/doc-timestamps-in-stat: CodingGuidelines: st_mtimespec vs st_mtim vs st_mtime
The userdiff driver for the Scheme language has been extended to cover other Lisp dialects. * sb/userdiff-lisp-family: userdiff: extend Scheme support to cover other Lisp dialects userdiff: tighten word-diff test case of the scheme driver
Rust support is enabled by default (but still allows opting out) in some future version of Git. * bc/rust-by-default: Enable Rust by default Linux: link against libdl ci: install cargo on Alpine docs: update version with default Rust support
The test suite harness and many individual test scripts have been updated to work correctly when 'set -e' is in effect, which helps detect misspelled test commands. * ps/test-set-e-clean: t: detect errors outside of test cases t9902: fix use of `read` with `set -e` t6002: fix use of `expr` with `set -e` t1301: don't fail in case setfacl(1) doesn't exist or fails t0008: silence error in subshell when using `grep -v` t: prepare `test_when_finished ()`/`test_atexit()` for `set -e` t: prepare execution of potentially failing commands for `set -e` t: prepare conditional test execution for `set -e` t: prepare `git config --unset` calls for `set -e` t: prepare `stop_git_daemon ()` for `set -e` t: prepare `test_must_fail ()` for `set -e` t: prepare `test_match_signal ()` calls for `set -e`
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )