reftable/reader: add comments to `table_iter_next()`
While working on the optimizations in the preceding patches I stumbled
upon `table_iter_next()` multiple times. It is quite easy to miss the
fact that we don't call `table_iter_next_in_block()` twice, but that the
second call is in fact `table_iter_next_block()`.
Add comments to explain what exactly is going on here to make things
more obvious. While at it, touch up the code to conform to our code
style better.
Note that one of the refactorings merges two conditional blocks into
one. Before, we had the following code:
reftable/record: don't try to reallocate ref record name
When decoding reftable ref records we first release the pointer to the
record passed to us and then use realloc(3P) to allocate the refname
array. This is a bit misleading though as we know at that point that the
refname will always be `NULL`, so we would always end up allocating a
new char array anyway.
Refactor the code to use `REFTABLE_ALLOC_ARRAY()` instead. As the
following benchmark demonstrates this is a tiny bit more efficient. But
the bigger selling point really is the gained clarity.
Benchmark 1: show-ref: single matching ref (revision = HEAD~)
Time (mean ± σ): 150.1 ms ± 4.1 ms [User: 146.6 ms, System: 3.3 ms]
Range (min … max): 144.5 ms … 180.5 ms 1000 runs
Benchmark 2: show-ref: single matching ref (revision = HEAD)
Time (mean ± σ): 148.9 ms ± 4.5 ms [User: 145.2 ms, System: 3.4 ms]
Range (min … max): 143.0 ms … 185.4 ms 1000 runs
Summary
show-ref: single matching ref (revision = HEAD) ran
1.01 ± 0.04 times faster than show-ref: single matching ref (revision = HEAD~)
Ideally, we should try and reuse the memory of the old record instead of
first freeing and then immediately reallocating it. This requires some
more surgery though and is thus left for a future iteration.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When iterating towards the next record in a reftable block we need to
keep track of the key that the last record had. This is required because
reftable records use prefix compression, where subsequent records may
reuse parts of their preceding record's key.
This key is stored in the `block_iter::last_key`, which we update after
every call to `block_iter_next()`: we simply reset the buffer and then
add the current key to it.
This is a bit inefficient though because it requires us to copy over the
key on every iteration, which adds up when iterating over many records.
Instead, we can make use of the fact that the `block_iter::key` buffer
is basically only a scratch buffer. So instead of copying over contents,
we can just swap both buffers.
The following benchmark prints a single ref matching a specific pattern
out of 1 million refs via git-show-ref(1):
Benchmark 1: show-ref: single matching ref (revision = HEAD~)
Time (mean ± σ): 155.7 ms ± 5.0 ms [User: 152.1 ms, System: 3.4 ms]
Range (min … max): 150.8 ms … 185.7 ms 1000 runs
Benchmark 2: show-ref: single matching ref (revision = HEAD)
Time (mean ± σ): 150.8 ms ± 4.2 ms [User: 147.1 ms, System: 3.5 ms]
Range (min … max): 145.1 ms … 180.7 ms 1000 runs
Summary
show-ref: single matching ref (revision = HEAD) ran
1.03 ± 0.04 times faster than show-ref: single matching ref (revision = HEAD~)
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
reftable/pq: allocation-less comparison of entry keys
The priority queue is used by the merged iterator to iterate over
reftable records from multiple tables in the correct order. The queue
ends up having one record for each table that is being iterated over,
with the record that is supposed to be shown next at the top. For
example, the key of a ref record is equal to its name so that we end up
sorting the priority queue lexicographically by ref name.
To figure out the order we need to compare the reftable record keys with
each other. This comparison is done by formatting them into a `struct
strbuf` and then doing `strbuf_strcmp()` on the result. We then discard
the buffers immediately after the comparison.
This ends up being very expensive. Because the priority queue usually
contains as many records as we have tables, we call the comparison
function `O(log($tablecount))` many times for every record we insert.
Furthermore, when iterating over many refs, we will insert at least one
record for every ref we are iterating over. So ultimately, this ends up
being called `O($refcount * log($tablecount))` many times.
Refactor the code to use the new `refatble_record_cmp()` function that
has been implemented in a preceding commit. This function does not need
to allocate memory and is thus significantly more efficient.
The following benchmark prints a single ref matching a specific pattern
out of 1 million refs via git-show-ref(1), where the reftable stack
consists of three tables:
Benchmark 1: show-ref: single matching ref (revision = HEAD~)
Time (mean ± σ): 224.4 ms ± 6.5 ms [User: 220.6 ms, System: 3.6 ms]
Range (min … max): 216.5 ms … 261.1 ms 1000 runs
Benchmark 2: show-ref: single matching ref (revision = HEAD)
Time (mean ± σ): 172.9 ms ± 4.4 ms [User: 169.2 ms, System: 3.6 ms]
Range (min … max): 166.5 ms … 204.6 ms 1000 runs
Summary
show-ref: single matching ref (revision = HEAD) ran
1.30 ± 0.05 times faster than show-ref: single matching ref (revision = HEAD~)
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
reftable/merged: skip comparison for records of the same subiter
When retrieving the next entry of a merged iterator we need to drop all
records of other sub-iterators that would be shadowed by the record that
we are about to return. We do this by comparing record keys, dropping
all keys that are smaller or equal to the key of the record we are about
to return.
There is an edge case here where we can skip that comparison: when the
record in the priority queue comes from the same subiterator as the
record we are about to return then we know that its key must be larger
than the key of the record we are about to return. This property is
guaranteed by the sub-iterators, and if it didn't hold then the whole
merged iterator would return records in the wrong order, too.
While this may seem like a very specific edge case it's in fact quite
likely to happen. For most repositories out there you can assume that we
will end up with one large table and several smaller ones on top of it.
Thus, it is very likely that the next entry will sort towards the top of
the priority queue.
Special case this and break out of the loop in that case. The following
benchmark uses git-show-ref(1) to print a single ref matching a pattern
out of 1 million refs:
Benchmark 1: show-ref: single matching ref (revision = HEAD~)
Time (mean ± σ): 162.6 ms ± 4.5 ms [User: 159.0 ms, System: 3.5 ms]
Range (min … max): 156.6 ms … 188.5 ms 1000 runs
Benchmark 2: show-ref: single matching ref (revision = HEAD)
Time (mean ± σ): 156.8 ms ± 4.7 ms [User: 153.0 ms, System: 3.6 ms]
Range (min … max): 151.4 ms … 188.4 ms 1000 runs
Summary
show-ref: single matching ref (revision = HEAD) ran
1.04 ± 0.04 times faster than show-ref: single matching ref (revision = HEAD~)
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
reftable/merged: allocation-less dropping of shadowed records
The purpose of the merged reftable iterator is to iterate through all
entries of a set of tables in the correct order. This is implemented by
using a sub-iterator for each table, where the next entry of each of
these iterators gets put into a priority queue. For each iteration, we
do roughly the following steps:
1. Retrieve the top record of the priority queue. This is the entry we
want to return to the caller.
2. Retrieve the next record of the sub-iterator that this record came
from. If any, add it to the priority queue at the correct position.
The position is determined by comparing the record keys, which e.g.
corresponds to the refname for ref records.
3. Keep removing the top record of the priority queue until we hit the
first entry whose key is larger than the returned record's key.
This is required to drop "shadowed" records.
The last step will lead to at least one comparison to the next entry,
but may lead to many comparisons in case the reftable stack consists of
many tables with shadowed records. It is thus part of the hot code path
when iterating through records.
The code to compare the entries with each other is quite inefficient
though. Instead of comparing record keys with each other directly, we
first format them into `struct strbuf`s and only then compare them with
each other. While we already optimized this code path to reuse buffers
in 829231dc20 (reftable/merged: reuse buffer to compute record keys,
2023-12-11), the cost to format the keys into the buffers still adds up
quite significantly.
Refactor the code to use `reftable_record_cmp()` instead, which has been
introduced in the preceding commit. This function compares records with
each other directly without requiring any memory allocations or copying
and is thus way more efficient.
The following benchmark uses git-show-ref(1) to print a single ref
matching a pattern out of 1 million refs. This is the most direct way to
exercise ref iteration speed as we remove all overhead of having to show
the refs, too.
Benchmark 1: show-ref: single matching ref (revision = HEAD~)
Time (mean ± σ): 180.7 ms ± 4.7 ms [User: 177.1 ms, System: 3.4 ms]
Range (min … max): 174.9 ms … 211.7 ms 1000 runs
Benchmark 2: show-ref: single matching ref (revision = HEAD)
Time (mean ± σ): 162.1 ms ± 4.4 ms [User: 158.5 ms, System: 3.4 ms]
Range (min … max): 155.4 ms … 189.3 ms 1000 runs
Summary
show-ref: single matching ref (revision = HEAD) ran
1.11 ± 0.04 times faster than show-ref: single matching ref (revision = HEAD~)
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
reftable/record: introduce function to compare records by key
In some places we need to sort reftable records by their keys to
determine their ordering. This is done by first formatting the keys into
a `struct strbuf` and then using `strbuf_cmp()` to compare them. This
logic is needlessly roundabout and can end up costing quite a bit of CPU
cycles, both due to the allocation and formatting logic.
Introduce a new `reftable_record_cmp()` function that knows how to
compare two records with each other without requiring allocations.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "disable repository discovery of a bare repository" check,
triggered by setting safe.bareRepository configuration variable to
'explicit', has been loosened to exclude the ".git/" directory inside
a non-bare repository from the check. So you can do "cd .git &&
git cmd" to run a Git command that works on a bare repository without
explicitly specifying $GIT_DIR now.
Junio C Hamano [Tue, 30 Jan 2024 21:34:12 +0000 (13:34 -0800)]
Merge branch 'jx/remote-archive-over-smart-http'
"git archive --remote=<remote>" learned to talk over the smart
http (aka stateless) transport.
* jx/remote-archive-over-smart-http:
transport-helper: call do_take_over() in process_connect
transport-helper: call do_take_over() in connect_helper
http-backend: new rpc-service for git-upload-archive
transport-helper: protocol v2 supports upload-archive
remote-curl: supports git-upload-archive service
transport-helper: no connection restriction in connect_helper
Junio C Hamano [Tue, 30 Jan 2024 21:34:12 +0000 (13:34 -0800)]
Merge branch 'rj/advice-disable-how-to-disable'
All conditional "advice" messages show how to turn them off, which
becomes repetitive. Setting advice.* configuration explicitly on
now omits the instruction part.
* rj/advice-disable-how-to-disable:
advice: allow disabling the automatic hint in advise_if_enabled()
Junio C Hamano [Tue, 30 Jan 2024 00:03:00 +0000 (16:03 -0800)]
Merge branch 'ps/not-so-many-refs-are-special'
Define "special ref" as a very narrow set that consists of
FETCH_HEAD and MERGE_HEAD, and clarify everything else that used to
be classified as such are actually just pseudorefs.
* ps/not-so-many-refs-are-special:
Documentation: add "special refs" to the glossary
refs: redefine special refs
refs: convert MERGE_AUTOSTASH to become a normal pseudo-ref
sequencer: introduce functions to handle autostashes via refs
refs: convert AUTO_MERGE to become a normal pseudo-ref
sequencer: delete REBASE_HEAD in correct repo when picking commits
sequencer: clean up pseudo refs with REF_NO_DEREF
Junio C Hamano [Tue, 30 Jan 2024 00:02:59 +0000 (16:02 -0800)]
Merge branch 'ps/reftable-optimize-io'
Low-level I/O optimization for reftable.
* ps/reftable-optimize-io:
reftable/stack: fix race in up-to-date check
reftable/stack: unconditionally reload stack after commit
reftable/blocksource: use mmap to read tables
reftable/blocksource: refactor code to match our coding style
reftable/stack: use stat info to avoid re-reading stack list
reftable/stack: refactor reloading to use file descriptor
reftable/stack: refactor stack reloading to have common exit path
When $HOME/.gitignore is missing but XDG config file available, we
should write into the latter, not former. "git gc" and "git
maintenance" wrote into a wrong "global config" file, which have
been corrected.
* kh/maintenance-use-xdg-when-it-should:
maintenance: use XDG config if it exists
config: factor out global config file retrieval
config: rename global config function
config: format newlines
Junio C Hamano [Fri, 26 Jan 2024 16:54:47 +0000 (08:54 -0800)]
Merge branch 'ps/gitlab-ci-macos'
CI for GitLab learned to drive macOS jobs.
* ps/gitlab-ci-macos:
ci: add macOS jobs to GitLab CI
ci: make p4 setup on macOS more robust
ci: handle TEST_OUTPUT_DIRECTORY when printing test failures
Makefile: detect new Homebrew location for ARM-based Macs
t7527: decrease likelihood of racing with fsmonitor daemon
Junio C Hamano [Fri, 26 Jan 2024 16:54:46 +0000 (08:54 -0800)]
Merge branch 'ps/completion-with-reftable-fix'
Completion update to prepare for reftable
* ps/completion-with-reftable-fix:
completion: treat dangling symrefs as existing pseudorefs
completion: silence pseudoref existence check
completion: improve existence check for pseudo-refs
t9902: verify that completion does not print anything
completion: discover repo path in `__git_pseudoref_exists ()`
Junio C Hamano [Fri, 26 Jan 2024 16:54:46 +0000 (08:54 -0800)]
Merge branch 'mj/gitweb-unreadable-config-error'
When given an existing but unreadable file as a configuration file,
gitweb behaved as if the file did not exist at all, but now it
errors out. This is a change that may break backward compatibility.
* mj/gitweb-unreadable-config-error:
gitweb: die when a configuration file cannot be read
Junio C Hamano [Fri, 26 Jan 2024 16:54:46 +0000 (08:54 -0800)]
Merge branch 'ps/worktree-refdb-initialization'
Instead of manually creating refs/ hierarchy on disk upon a
creation of a secondary worktree, which is only usable via the
files backend, use the refs API to populate it.
* ps/worktree-refdb-initialization:
builtin/worktree: create refdb via ref backend
worktree: expose interface to look up worktree by name
builtin/worktree: move setup of commondir file earlier
refs/files: skip creation of "refs/{heads,tags}" for worktrees
setup: move creation of "refs/" into the files backend
refs: prepare `refs_init_db()` for initializing worktree refs
The error message given when "git branch -d branch" fails due to
commits unique to the branch has been split into an error and a new
conditional advice message.
* rj/advice-delete-branch-not-fully-merged:
branch: make the advice to force-deleting a conditional one
advice: fix an unexpected leading space
advice: sort the advice related lists
Philippe Blain [Sun, 21 Jan 2024 03:38:33 +0000 (03:38 +0000)]
ci(github): also skip logs of broken test cases
When a test fails in the GitHub Actions CI pipeline, we mark it up using
special GitHub syntax so it stands out when looking at the run log. We
also mark up "fixed" test cases, and skip passing tests since we want to
concentrate on the failures.
The finalize_test_case_output function in
test-lib-github-workflow-markup.sh which performs this markup is however
missing a fourth case: "broken" tests, i.e. tests using
'test_expect_failure' to document a known bug. This leads to these
"broken" tests appearing along with any failed tests, potentially
confusing the reader who might not be aware that "broken" is the status
for 'test_expect_failure' tests that indeed failed, and wondering what
their commits "broke".
Also skip these "broken" tests so that only failures and fixed tests
stand out.
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com> Acked-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jiang Xin [Sun, 21 Jan 2024 13:15:38 +0000 (21:15 +0800)]
transport-helper: call do_take_over() in process_connect
The existing pattern among all callers of process_connect() seems to be
if (process_connect(...)) {
do_take_over();
... dispatch to the underlying method ...
}
... otherwise implement the fallback ...
where the return value from process_connect() is the return value of the
call it makes to process_connect_service().
Move the call of do_take_over() inside process_connect(), so that
calling the process_connect() function is more concise and will not
miss do_take_over().
Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jiang Xin [Sun, 21 Jan 2024 13:15:37 +0000 (21:15 +0800)]
transport-helper: call do_take_over() in connect_helper
After successfully connecting to the smart transport by calling
process_connect_service() in connect_helper(), run do_take_over() to
replace the old vtable with a new one which has methods ready for the
smart transport connection. This fixes the exit code of git-archive
in test case "archive remote http repository" of t5003.
The connect_helper() function is used as the connect method of the
vtable in "transport-helper.c", and it is called by transport_connect()
in "transport.c" to setup a connection. The only place that we call
transport_connect() so far is in "builtin/archive.c". Without running
do_take_over(), it may fail to call transport_disconnect() in
run_remote_archiver() of "builtin/archive.c". This is because for a
stateless connection and a service like "git-upload-archive", the
remote helper may receive a SIGPIPE signal and exit early. Call
do_take_over() to have a graceful disconnect method, so that we still
call transport_disconnect() even if the remote helper exits early.
Helped-by: Linus Arver <linusa@google.com> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jiang Xin [Sun, 21 Jan 2024 13:15:36 +0000 (21:15 +0800)]
http-backend: new rpc-service for git-upload-archive
Add new rpc-service "upload-archive" in http-backend to add server side
support for remote archive over HTTP/HTTPS protocols.
Also add new test cases in t5003. In the test case "archive remote http
repository", git-archive exits with a non-0 exit code even though we
create the archive correctly. It will be fixed in a later commit.
Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We used to support only git-upload-pack service for protocol v2. In
order to support remote archive over HTTP/HTTPS protocols, add new
service support for git-upload-archive in protocol v2.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jiang Xin [Sun, 21 Jan 2024 13:15:34 +0000 (21:15 +0800)]
remote-curl: supports git-upload-archive service
Add new service (git-upload-archive) support in remote-curl, so we can
support remote archive over HTTP/HTTPS protocols. Differences between
git-upload-archive and other services:
1. The git-archive program does not expect to see protocol version and
capabilities when connecting to remote-helper, so do not send them
in remote-curl for the git-upload-archive service.
2. We need to detect protocol version by calling discover_refs().
Fallback to use the git-upload-pack service (which, like
git-upload-archive, is a read-only operation) to discover protocol
version.
Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jiang Xin [Sun, 21 Jan 2024 13:15:33 +0000 (21:15 +0800)]
transport-helper: no connection restriction in connect_helper
When commit b236752a (Support remote archive from all smart transports,
2009-12-09) added "remote archive" support for "smart transports", it
was for transport that supports the ".connect" method. The
"connect_helper()" function protected itself from getting called for a
transport without the method before calling process_connect_service(),
which only worked with the ".connect" method.
Later, commit edc9caf7 (transport-helper: introduce stateless-connect,
2018-03-15) added a way for a transport without the ".connect" method
to establish a "stateless" connection in protocol v2, where
process_connect_service() was taught to handle the ".stateless_connect"
method, making the old protection too strict. But commit edc9caf7 forgot
to adjust this protection accordingly. Even at the time of commit b236752a, this protection seemed redundant, since
process_connect_service() would return 0 if the connection could not be
established, and connect_helper() would still die() early.
Remove the restriction in connect_helper() and give the function
process_connect_service() the opportunity to establish a connection
using ".connect" or ".stateless_connect" for protocol v2. So we can
connect with a stateless-rpc and do something useful. E.g., in a later
commit, implements remote archive for a repository over HTTP protocol.
Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Linus Arver <linusa@google.com> Signed-off-by: Jiang Xin <zhiyou.jx@alibaba-inc.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
René Scharfe [Sun, 21 Jan 2024 17:56:39 +0000 (18:56 +0100)]
parse-options: simplify positivation handling
We accept the positive version of options whose long name starts with
"no-" and are defined without the flag PARSE_OPT_NONEG. E.g. git clone
has an explicitly defined --no-checkout option and also implicitly
accepts --checkout to override it.
parse_long_opt() handles that by restarting the option matching with the
positive version when it finds that only the current option definition
starts with "no-", but not the user-supplied argument. This code is
located almost at the end of the matching logic.
Avoid the need for a restart by moving the code up. We don't have to
check the positive arg against the negative long_name at all -- the
"no-" prefix of the latter makes a match impossible. Skip it and toggle
OPT_UNSET right away to simplify the control flow.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Kyle Lippincott [Sat, 20 Jan 2024 00:08:22 +0000 (00:08 +0000)]
setup: allow cwd=.git w/ bareRepository=explicit
The safe.bareRepository setting can be set to 'explicit' to disallow
implicit uses of bare repositories, preventing an attack [1] where an
artificial and malicious bare repository is embedded in another git
repository. Unfortunately, some tooling uses myrepo/.git/ as the cwd
when executing commands, and this is blocked when
safe.bareRepository=explicit. Blocking is unnecessary, as git already
prevents nested .git directories.
Teach git to not reject uses of git inside of the .git directory: check
if cwd is .git (or a subdirectory of it) and allow it even if
safe.bareRepository=explicit.
Achu Luma [Sat, 20 Jan 2024 02:15:47 +0000 (03:15 +0100)]
t2400: avoid losing exit status to pipes
The exit code of the preceding command in a pipe is disregarded. So
if that preceding command is a Git command that fails, the test would
not fail. Instead, by saving the output of that Git command to a file,
and removing the pipe, we make sure the test will fail if that Git
command fails.
Signed-off-by: Achu Luma <ach.lumap@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Fri, 19 Jan 2024 22:26:45 +0000 (14:26 -0800)]
Docs: majordomo@vger.kernel.org has been decomissioned
Update the instruction for subscribing to the Git mailing list
we have on a few documentation pages.
Reported-by: Kyle Lippincott <spectral@google.com> Helped-by: Konstantin Ryabitsev <konstantin@linuxfoundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
René Scharfe [Sat, 20 Jan 2024 14:39:38 +0000 (15:39 +0100)]
parse-options: fully disable option abbreviation with PARSE_OPT_KEEP_UNKNOWN
baa4adc66a (parse-options: disable option abbreviation with
PARSE_OPT_KEEP_UNKNOWN, 2019-01-27) turned off support for abbreviated
options when the flag PARSE_OPT_KEEP_UNKNOWN is given, as any shortened
option could also be an abbreviation for one of the unknown options.
The code for handling abbreviated options is guarded by an if, but it
can also be reached via goto. baa4adc66a only blocked the first way.
Add the condition to the other ones as well.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Replace pipe with redirection operator '>' to store the output
to a temporary file after 'git archive' command since the pipe
will swallow the command's exit code and a crash won't
necessarily be noticed.
Also fix an unwanted space after redirection '>' to match the
style described in CodingGuidelines.
Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In addition to (rather cryptic) Security Identifiers, show username
and domain in the error message when we barf on mismatch between
the Git directory and the current user on Windows.
* sk/mingw-owner-check-error-message-improvement:
mingw: give more details about unsafe directory's ownership
Junio C Hamano [Fri, 19 Jan 2024 23:04:45 +0000 (15:04 -0800)]
Merge branch 'tb/fetch-all-configuration'
"git fetch" learned to pay attention to "fetch.all" configuration
variable, which pretends as if "--all" was passed from the command
line when no remote parameter was given.
* tb/fetch-all-configuration:
fetch: add new config option fetch.all
Josh Steadmon [Fri, 19 Jan 2024 21:38:13 +0000 (13:38 -0800)]
ci: build and run minimal fuzzers in GitHub CI
To prevent bitrot, we would like to regularly exercise the fuzz tests in
order to make sure they still link & run properly. We already compile
the fuzz test objects as part of the default `make` target, but we do
not link the executables due to the fuzz tests needing specific
compilers and compiler features. This has lead to frequent build
breakages for the fuzz tests.
To remedy this, we can add a CI step to actually link the fuzz
executables, and run them (with finite input rather than the default
infinite random input mode) to verify that they execute properly.
Since the main use of the fuzz tests is via OSS-Fuzz [1], and OSS-Fuzz
only runs tests on Linux [2], we only set up a CI test for the fuzzers
on Linux.
Josh Steadmon [Fri, 19 Jan 2024 21:38:12 +0000 (13:38 -0800)]
fuzz: fix fuzz test build rules
When we originally added the fuzz tests in 5e47215080 (fuzz: add basic
fuzz testing target., 2018-10-12), we went to some trouble to create a
Makefile rule that allowed linking the fuzz executables without pulling
in common-main.o. This was necessary to prevent the
fuzzing-engine-provided main() from clashing with Git's main().
However, since 19d75948ef (common-main.c: move non-trace2 exit()
behavior out of trace2.c, 2022-06-02), it has been necessary to link
common-main.o due to moving the common_exit() function to that file.
Ævar suggested a set of compiler flags to allow this in [1], but this
was never reflected in the Makefile.
Since we now must include common-main.o, there's no reason to pick and
choose a subset of object files to link, so simplify the Makefile rule
for the fuzzer executables to just use libgit.a. While we're at it,
include the necessary linker flag to allow multiple definitions
directly in the Makefile rule, rather than requiring it to be passed on
the command-line each time. This means the Makefile rule as written is
now more compiler-specific, but this was already the case for the
fuzzers themselves anyway.
sequencer: introduce functions to handle autostashes via refs
We're about to convert the MERGE_AUTOSTASH ref to become non-special,
using the refs API instead of direct filesystem access to both read and
write the ref. The current interfaces to write autostashes is entirely
path-based though, so we need to extend them to also support writes via
the refs API instead.
Ideally, we would be able to fully replace the old set of path-based
interfaces. But the sequencer will continue to write state into
"rebase-merge/autostash". This path is not considered to be a ref at all
and will thus stay is-is for now, which requires us to keep both path-
and refs-based interfaces to handle autostashes.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
refs: convert AUTO_MERGE to become a normal pseudo-ref
In 70c70de616 (refs: complete list of special refs, 2023-12-14) we have
inrtoduced a new `is_special_ref()` function that classifies some refs
as being special. The rule is that special refs are exclusively read and
written via the filesystem directly, whereas normal refs exclucsively go
via the refs API.
The intent of that commit was to record the status quo so that we know
to route reads of such special refs consistently. Eventually, the list
should be reduced to its bare minimum of refs which really are special,
namely FETCH_HEAD and MERGE_HEAD.
Follow up on this promise and convert the AUTO_MERGE ref to become a
normal pseudo-ref by using the refs API to both read and write it
instead of accessing the filesystem directly.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
sequencer: delete REBASE_HEAD in correct repo when picking commits
When picking commits, we delete some state before executing the next
sequencer action on interactive rebases. But while we use the correct
repository to calculate paths to state files that need deletion, we use
the repo-less `delete_ref()` function to delete REBASE_HEAD. Thus, if
the sequencer ran in a different repository than `the_repository`, we
would end up deleting the ref in the wrong repository.
Fix this by using `refs_delete_ref()` instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When cleaning up the state-tracking pseudorefs CHERRY_PICK_HEAD or
REVERT_HEAD we do not set REF_NO_DEREF. In the unlikely case where those
refs are a symref we would thus end up deleting the symref targets, and
not the symrefs themselves.
Harden the code to use REF_NO_DEREF to fix this.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Victoria Dye [Thu, 18 Jan 2024 01:55:18 +0000 (01:55 +0000)]
submodule-config.c: strengthen URL fsck check
Update the validation of "curl URL" submodule URLs (i.e. those that specify
an "http[s]" or "ftp[s]" protocol) in 'check_submodule_url()' to catch more
invalid URLs. The existing validation using 'credential_from_url_gently()'
parses certain URLs incorrectly, leading to invalid submodule URLs passing
'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote
URLs in 'remote_get()' - correctly identifies the invalid URLs missed by
'credential_from_url_gently()'.
To catch more invalid cases, replace 'credential_from_url_gently()' with
'url_normalize()' followed by a 'url_decode()' and a check for newlines
(mirroring 'check_url_component()' in the 'credential_from_url_gently()'
validation).
Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Victoria Dye [Thu, 18 Jan 2024 01:55:17 +0000 (01:55 +0000)]
t7450: test submodule urls
Add tests to 't7450-bad-git-dotfiles.sh' to check the validity of different
submodule URLs. To verify this directly (without setting up test
repositories & submodules), add a 'check-url' subcommand to 'test-tool
submodule' that calls 'check_submodule_url' in the same way that
'check-name' calls 'check_submodule_name'.
Add two tests to separately address cases where the URL check correctly
filters out invalid URLs and cases where the check misses invalid URLs. Mark
the latter ("url check misses invalid cases") with 'test_expect_failure' to
indicate that this is currently broken, which will be fixed in the next step.
Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Sat, 13 Jan 2024 04:26:13 +0000 (04:26 +0000)]
diffcore-delta: avoid ignoring final 'line' of file
hash_chars() would hash lines to integers, and store them in a spanhash,
but cut lines at 64 characters. Thus, whenever it reached 64 characters
or a newline, it would create a new spanhash. The problem is, the final
part of the file might not end 64 characters after the previous 'line'
and might not end with a newline. This could, for example, cause an
85-byte file with 12 lines and only the first character in the file
differing to appear merely 23% similar rather than the expected 97%.
Ensure the last line is included, and add a testcase that would have
caught this problem.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
`git maintenance register` registers the repository in the user's global
config. `$XDG_CONFIG_HOME/git/config` is supposed to be used if
`~/.gitconfig` does not exist. However, this command creates a
`~/.gitconfig` file and writes to that one even though the XDG variant
exists.
This used to work correctly until 50a044f1e4 (gc: replace config
subprocesses with API calls, 2022-09-27), when the command started calling
the config API instead of git-config(1).
Also change `unregister` accordingly.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In 6fdfaf15a0 (reftable/stack: use stat info to avoid re-reading stack
list, 2024-01-11) we have introduced a new mechanism to avoid re-reading
the table list in case stat(3P) figures out that the stack didn't change
since the last time we read it.
While this change significantly improved performance when writing many
refs, it can unfortunately lead to false negatives in very specific
scenarios. Given two processes A and B, there is a feasible sequence of
events that cause us to accidentally treat the table list as up-to-date
even though it changed:
1. A reads the reftable stack and caches its stat info.
2. B updates the stack, appending a new table to "tables.list". This
will both use a new inode and result in a different file size, thus
invalidating A's cache in theory.
3. B decides to auto-compact the stack and merges two tables. The file
size now matches what A has cached again. Furthermore, the
filesystem may decide to recycle the inode number of the file we
have replaced in (2) because it is not in use anymore.
4. A reloads the reftable stack. Neither the inode number nor the
file size changed. If the timestamps did not change either then we
think the cached copy of our stack is up-to-date.
In fact, the commit introduced three related issues:
- Non-POSIX compliant systems may not report proper `st_dev` and
`st_ino` values in stat(3P), which made us rely solely on the
file's potentially coarse-grained mtime and ctime.
- `stat_validity_check()` and friends may end up not comparing
`st_dev` and `st_ino` depending on the "core.checkstat" config,
again reducing the signal to the mtime and ctime.
- `st_ino` can be recycled, rendering the check moot even on
POSIX-compliant systems.
Given that POSIX defines that "The st_ino and st_dev fields taken
together uniquely identify the file within the system", these issues led
to the most important signal to establish file identity to be ignored or
become useless in some cases.
Refactor the code to stop using `stat_validity_check()`. Instead, we
manually stat(3P) the file descriptors to make relevant information
available. On Windows and MSYS2 the result will have both `st_dev` and
`st_ino` set to 0, which allows us to address the first issue by not
using the stat-based cache in that case. It also allows us to make sure
that we always compare `st_dev` and `st_ino`, addressing the second
issue.
The third issue of inode recycling can be addressed by keeping the file
descriptor of "files.list" open during the lifetime of the reftable
stack. As the file will still exist on disk even though it has been
unlinked it is impossible for its inode to be recycled as long as the
file descriptor is still open.
This should address the race in a POSIX-compliant way. The only real
downside is that this mechanism cannot be used on non-POSIX-compliant
systems like Windows. But we at least have the second-level caching
mechanism in place that compares contents of "files.list" with the
currently loaded list of tables.
This new mechanism performs roughly the same as the previous one that
relied on `stat_validity_check()`:
Benchmark 1: update-ref: create many refs (HEAD~)
Time (mean ± σ): 4.754 s ± 0.026 s [User: 2.204 s, System: 2.549 s]
Range (min … max): 4.694 s … 4.802 s 20 runs
Benchmark 2: update-ref: create many refs (HEAD)
Time (mean ± σ): 4.721 s ± 0.020 s [User: 2.194 s, System: 2.527 s]
Range (min … max): 4.691 s … 4.753 s 20 runs
Summary
update-ref: create many refs (HEAD~) ran
1.01 ± 0.01 times faster than update-ref: create many refs (HEAD)
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
reftable/stack: unconditionally reload stack after commit
After we have committed an addition to the reftable stack we call
`reftable_stack_reload()` to reload the stack and thus reflect the
changes that were just added. This function will only conditionally
reload the stack in case `stack_uptodate()` tells us that the stack
needs reloading. This check is wasteful though because we already know
that the stack needs reloading.
Call `reftable_stack_reload_maybe_reuse()` instead, which will
unconditionally reload the stack. This is merely a conceptual fix, the
code in question was not found to cause any problems in practice.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a job to GitLab CI which runs tests on macOS, which matches the
equivalent "osx-clang" job that we have for GitHub Workflows. One
significant difference though is that this new job runs on Apple M1
machines and thus uses the "arm64" architecture. As GCC does not yet
support this comparatively new architecture we cannot easily include an
equivalent for the "osx-gcc" job that exists in GitHub Workflows.
Note that one test marked as `test_must_fail` is surprisingly passing:
t7815-grep-binary.sh (Wstat: 0 Tests: 22 Failed: 0)
TODO passed: 12
This seems to boil down to an unexpected difference in how regcomp(3P)
works when matching NUL bytes. Cross-checking with the respective GitHub
job shows that this is not an issue unique to the GitLab CI job as it
passes in the same way there.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When setting up Perforce on macOS we put both `p4` and `p4d` into
"$HOME/bin". On GitHub CI this directory is indeed contained in the PATH
environment variable and thus there is no need for additional setup than
to put the binaries there. But GitLab CI does not do this, and thus our
Perforce-based tests would be skipped there even though we download the
binaries.
Refactor the setup code to become more robust by downloading binaries
into a separate directory which we then manually append to our PATH.
This matches what we do on Linux-based jobs.
Note that it may seem like we already did append "$HOME/bin" to PATH
because we're actually removing the lines that adapt PATH. But we only
ever adapted the PATH variable in "ci/install-dependencies.sh", and
didn't adapt it when running "ci/run-build-and-test.sh". Consequently,
the required binaries wouldn't be found during the test run unless the
CI platform already had the "$HOME/bin" in PATH right from the start.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
ci: handle TEST_OUTPUT_DIRECTORY when printing test failures
The TEST_OUTPUT_DIRECTORY environment variable can be used to instruct
the test suite to write test data and test results into a different
location than into "t/". The "ci/print-test-failures.sh" script does not
know to handle this environment variable though, which means that it
will search for test results in the wrong location if it was set.
Update the script to handle TEST_OUTPUT_DIRECTORY so that we can start
to set it in our CI.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Makefile: detect new Homebrew location for ARM-based Macs
With the introduction of the ARM-based Macs the default location for
Homebrew has changed from "/usr/local" to "/opt/homebrew". We only
handle the former location though, which means that unless the user has
manually configured required search paths we won't be able to locate it.
Improve upon this by adding relevant paths to our CFLAGS and LDFLAGS as
well as detecting the location of msgfmt(1).
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
t7527: decrease likelihood of racing with fsmonitor daemon
In t7527, we test that the builtin fsmonitor daemon works well in
various edge cases. One of these tests is frequently failing because
events reported by the fsmonitor--daemon are missing an expected event.
This failure is essentially a race condition: we do not wait for the
daemon to flush out all events before we ask it to quit. Consequently,
it can happen that we miss some expected events.
In other testcases we counteract this race by sending a simple query to
the daemon. Quoting a comment:
We run a simple query after modifying the filesystem just to introduce
a bit of a delay so that the trace logging from the daemon has time to
get flushed to disk.
Now this workaround is not a "proper" fix as we do not wait for all
events to have been synchronized in a deterministic way. But this fix
seems to be sufficient for all the other tests to pass, so it must not
be all that bad.
Convert the failing test to do the same. While the test was previously
failing in about 50% of the test runs, I couldn't reproduce the failure
after the change anymore.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Toon Claes [Wed, 10 Jan 2024 14:15:59 +0000 (15:15 +0100)]
builtin/show-ref: treat directory as non-existing in --exists
9080a7f178 (builtin/show-ref: add new mode to check for reference
existence, 2023-10-31) added the option --exists to git-show-ref(1).
When you use this option against a ref that doesn't exist, but it is
a parent directory of an existing ref, you get the following error:
$ git show-ref --exists refs/heads
error: failed to look up reference: Is a directory
when the ref-files backend is in use. To be more clear to user,
hide the error about having found a directory. What matters to the
user is that the named ref does not exist. Instead, print the same
error as when the ref was not found:
error: reference does not exist
Signed-off-by: Toon Claes <toon@iotcl.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Victoria Dye [Thu, 18 Jan 2024 01:55:16 +0000 (01:55 +0000)]
test-submodule: remove command line handling for check-name
The 'check-name' subcommand to 'test-tool submodule' is documented as being
able to take a command line argument '<name>'. However, this does not work -
and has never worked - because 'argc > 0' triggers the usage message in
'cmd__submodule_check_name()'. To simplify the helper and avoid future
confusion around proper use of the subcommand, remove any references to
command line arguments for 'check-name' in usage strings and handling in
'check_name()'.
Helped-by: Jeff King <peff@peff.net> Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Victoria Dye [Thu, 18 Jan 2024 01:55:15 +0000 (01:55 +0000)]
submodule-config.h: move check_submodule_url
Move 'check_submodule_url' out of 'fsck.c' and into 'submodule-config.h' as
a public method, similar to 'check_submodule_name'. With the function now
accessible outside of 'fsck', it can be used in a later commit to extend
'test-tool submodule' to check the validity of submodule URLs as it does
with names in the 'check-name' subcommand.
Other than its location, no changes are made to 'check_submodule_url' in
this patch.
Signed-off-by: Victoria Dye <vdye@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Nikolay Borisov [Wed, 17 Jan 2024 08:53:47 +0000 (10:53 +0200)]
rebase: fix documentation about used shell in -x
The shell used when using the -x option is erroneously documented to be
the one pointed to by the $SHELL environmental variable. This was true
when rebase was implemented as a shell script but this is no longer
true.
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add tests for --only (-o) and --include (-i). This include testing
with or without staged changes for both -i and -o. Also to test
for committing untracked files with -i, -o and without -i/-o.
Some tests already exist in t7501 for testing --only, however,
it is only tested in combination with --amend and --allow-empty
and on to-be-born branch. The addition of these tests check, when
the pathspec is provided without using -only, that only the files
matching the pathspec get committed. This behavior is same when
we provide --only and it is checked by the tests.
(as --only is the default mode of operation when pathspec is
provided.)
As for --include, there is no prior test for checking if --include
also commits staged changes, thus add test for that. Along with
the tests also document a potential bug, in which, when provided
with -i and a pathspec that does not match any tracked path,
commit does not fail if there are staged changes. And when there
are no staged changes commit fails. However, no error is returned
to stderr in either of the cases. This is described in the TODO
comment before the relevent testcase.
And also add a test for checking incompatibilty when using -o and
-i together.
Thus, these tests belong in t7501 with other similar existing tests,
as described in the case of --only.
Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Tue, 16 Jan 2024 21:06:35 +0000 (13:06 -0800)]
Merge branch 'rj/advice-delete-branch-not-fully-merged' into rj/advice-disable-how-to-disable
* rj/advice-delete-branch-not-fully-merged:
branch: make the advice to force-deleting a conditional one
advice: fix an unexpected leading space
advice: sort the advice related lists
Junio C Hamano [Tue, 16 Jan 2024 18:11:58 +0000 (10:11 -0800)]
Merge branch 'jk/commit-graph-slab-clear-fix'
Clearing in-core repository (happens during e.g., "git fetch
--recurse-submodules" with commit graph enabled) made in-core
commit object in an inconsistent state by discarding the necessary
data from commit-graph too early, which has been corrected.
* jk/commit-graph-slab-clear-fix:
commit-graph: retain commit slab when closing NULL commit_graph
Junio C Hamano [Tue, 16 Jan 2024 18:11:57 +0000 (10:11 -0800)]
Merge branch 'ps/refstorage-extension'
Introduce a new extension "refstorage" so that we can mark a
repository that uses a non-default ref backend, like reftable.
* ps/refstorage-extension:
t9500: write "extensions.refstorage" into config
builtin/clone: introduce `--ref-format=` value flag
builtin/init: introduce `--ref-format=` value flag
builtin/rev-parse: introduce `--show-ref-format` flag
t: introduce GIT_TEST_DEFAULT_REF_FORMAT envvar
setup: introduce GIT_DEFAULT_REF_FORMAT envvar
setup: introduce "extensions.refStorage" extension
setup: set repository's formats on init
setup: start tracking ref storage format
refs: refactor logic to look up storage backends
worktree: skip reading HEAD when repairing worktrees
t: introduce DEFAULT_REPO_FORMAT prereq
projects / thirdparty / git.git / log
