When creating a read stream we first populate the structure with the
open callback function and then subsequently call the function. This
layout is somewhat weird though:
- The structure needs to be allocated and partially populated with the
open function before we can properly initialize it.
- We only ever call the `open()` callback function right after having
populated the `struct odb_read_stream::open` member, and it's never
called thereafter again. So it is somewhat pointless to store the
callback in the first place.
Especially the first point creates a problem for us. In subsequent
commits we'll want to fully move construction of the read source into
the respective object sources. E.g., the loose object source will be the
one that is responsible for creating the structure. But this creates a
problem: if we first need to create the structure so that we can call
the source-specific callback we cannot fully handle creation of the
structure in the source itself.
We could of course work around that and have the loose object source
create the structure and populate its `open()` callback, only. But
this doesn't really buy us anything due to the second bullet point
above.
Instead, drop the callback entirely and refactor `istream_source()` so
that we open the streams immediately. This unblocks a subsequent step,
where we'll also start to allocate the structure in the source-specific
logic.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
streaming: rename `git_istream` into `odb_read_stream`
In the following patches we are about to make the `git_istream` more
generic so that it becomes fully controlled by the specific object
source that wants to create it. As part of these refactorings we'll
fully move the structure into the object database subsystem.
Prepare for this change by renaming the structure from `git_istream`
to `odb_read_stream`. This mirrors the `odb_write_stream` structure that
we already have.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Fri, 21 Nov 2025 17:14:15 +0000 (09:14 -0800)]
Merge branch 'kn/maintenance-is-needed'
"git maintenance" command learned "is-needed" subcommand to tell if
it is necessary to perform various maintenance tasks.
* kn/maintenance-is-needed:
maintenance: add 'is-needed' subcommand
maintenance: add checking logic in `pack_refs_condition()`
refs: add a `optimize_required` field to `struct ref_storage_be`
reftable/stack: add function to check if optimization is required
reftable/stack: return stack segments directly
Junio C Hamano [Fri, 21 Nov 2025 17:14:15 +0000 (09:14 -0800)]
Merge branch 'rs/diff-quiet-no-rename'
As "git diff --quiet" only cares about the existence of any
changes, disable rename/copy detection to skip more expensive
processing whose result will be discarded anyway.
* rs/diff-quiet-no-rename:
diff: disable rename detection with --quiet
Karthik Nayak [Fri, 21 Nov 2025 11:13:45 +0000 (12:13 +0100)]
fetch: extract out reference committing logic
The `do_fetch()` function contains the core of the `git-fetch(1)` logic.
Part of this is to fetch and store references. This is done by
1. Creating a reference transaction (non-atomic mode uses batched
updates).
2. Adding individual reference updates to the transaction.
3. Committing the transaction.
4. When using batched updates, handling the rejected updates.
The following commit, will fix a bug wherein fetching tags with
conflicts was causing other reference updates to fail. Fixing this
requires utilizing this logic in different regions of the function.
In preparation of the follow up commit, extract the committing and
rejection handling logic into a separate function called
`commit_ref_transaction()`.
Helped-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
This option could create a commit history which violates the assumption
that commits have non-decreasing commit timestamps. Warn against that in
both git-am(1) and git-rebase(1).
The genesis of this option is from git-am(1) and was added in 3f01ad66 (am: Add --committer-date-is-author-date option,
2009-01-22). The commit message doesn’t give us an example
of a use case, but the thread starter does:[1]
I've a big set of patches in a mbox file: there's sufficient info
inside for git-am to work.
Yet, each time I do import these, my sha1sums are changing because of
different commit dates.
I'd like to force the commit date to match the info/date from the time
I received the email (and therefore always get back the right
sha1sums).
So the motivation was to treat git-am(1) as an import command that
creates the same commit IDs.
Putting aside the question of whether you should be using git-am(1) for
importing commits, this approach is problematic:
• you still need to apply the commits to the same base if you want the
same hashes; and
• you need the same committer.
And if you expect the same committer, why is this person applying the
same patches multiple times with the goal of making *identical* commits?
That was all for git-am(1).
It was added to git-rebase(1) in 570ccad3 (rebase: add options passed to
git-am, 2009-03-18)[2] in order to plug options that could not be sent
on to git-am(1). At this point the utility of the option graduated to
making no sense; a use case for `git rebase --committer-date-is-author-
date` is still yet to be found.
Just warn against using this option on both commands and remind the user
to consider whether they really need it.
† 2: See also 7573cec5 (rebase -i: support
--committer-date-is-author-date, 2020-08-17) for the commit for the
merge backend
Suggested-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Acked-by: Johannes Sixt <j6t@kdbg.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function `odb_clear()` releases all resources allocated to an object
database and ensures that all fields become zero'd out. Despite its
naming though it doesn't really clear the object database so that it
becomes ready for reuse afterwards again -- the caller would first have
to reinitialize it, and that contradicts the terminology of "clearing"
as we have defined it in our coding guidelines.
There isn't really only a reason to have "clearing" semantics, either.
There's only a single caller of `odb_clear()`, and that caller also ends
up freeing the object database structure itself.
Refactor the function to have "freeing" semantics instead, so that the
structure itself is also freed, which allows us to drop some useless
boilerplate to zero out the structure's members.
This refactoring reveals that we're trying to close the commit graph
multiple times: once directly via `free_commit_graph()`, and once via
`odb_close()`. Drop the former call.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The logic to close an object database is currently contained in the
packfile subsystem. That choice is somewhat relatable, as most of the
logic really is to close resources associated with the packfile store
itself. But we also end up handling object sources and commit graphs,
which certainly is not related to packfiles.
Move the function into the object database subsystem and rename it to
`odb_close()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We don't have any external callers of `set_git_dir()` anymore now that
`enter_repo()` has been moved into "setup.c". Remove the declaration and
mark the function as static.
Note that this change requires us to move the implementation around so
that we can avoid adding any new forward declarations.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function `enter_repo()` is used to enter a repository at a given
path. As such it sits way closer to setting up a repository than it does
with handling paths, but regardless of that it's located in "path.c"
instead of in "setup.c".
Move the function into "setup.c".
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jean-Noël Avila [Wed, 19 Nov 2025 21:40:04 +0000 (21:40 +0000)]
doc: convert git push to synopsis style
- Switch the synopsis to a synopsis block which will automatically
format placeholders in italics and keywords in monospace
- Use _<placeholder>_ instead of <placeholder> in the description
- Use `backticks` for keywords and more complex option
descriptions. The new rendering engine will apply synopsis rules to
these spans.
Signed-off-by: Jean-Noël Avila <jn.avila@free.fr> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jean-Noël Avila [Wed, 19 Nov 2025 21:40:03 +0000 (21:40 +0000)]
doc: convert git pull to synopsis style
- Switch the synopsis to a synopsis block which will automatically
format placeholders in italics and keywords in monospace
- Use _<placeholder>_ instead of <placeholder> in the description
- Use `backticks` for keywords and more complex option
descriptions. The new rendering engine will apply synopsis rules to
these spans.
Signed-off-by: Jean-Noël Avila <jn.avila@free.fr> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jean-Noël Avila [Wed, 19 Nov 2025 21:40:02 +0000 (21:40 +0000)]
doc: convert git fetch to synopsis style
- Switch the synopsis to a synopsis block which will automatically
format placeholders in italics and keywords in monospace
- Use _<placeholder>_ instead of <placeholder> in the description
- Use `backticks` for keywords and more complex option
descriptions. The new rendering engine will apply synopsis rules to
these spans.
Signed-off-by: Jean-Noël Avila <jn.avila@free.fr> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Wed, 19 Nov 2025 18:55:40 +0000 (10:55 -0800)]
Merge branch 'kn/refs-optim-cleanup'
Code clean-up.
* kn/refs-optim-cleanup:
t/pack-refs-tests: move the 'test_done' to callees
refs: rename 'pack_refs_opts' to 'refs_optimize_opts'
refs: move to using the '.optimize' functions
Junio C Hamano [Wed, 19 Nov 2025 18:55:39 +0000 (10:55 -0800)]
Merge branch 'ps/ref-peeled-tags'
Some ref backend storage can hold not just the object name of an
annotated tag, but the object name of the object the tag points at.
The code to handle this information has been streamlined.
* ps/ref-peeled-tags:
t7004: do not chdir around in the main process
ref-filter: fix stale parsed objects
ref-filter: parse objects on demand
ref-filter: detect broken tags when dereferencing them
refs: don't store peeled object IDs for invalid tags
object: add flag to `peel_object()` to verify object type
refs: drop infrastructure to peel via iterators
refs: drop `current_ref_iter` hack
builtin/show-ref: convert to use `reference_get_peeled_oid()`
ref-filter: propagate peeled object ID
upload-pack: convert to use `reference_get_peeled_oid()`
refs: expose peeled object ID via the iterator
refs: refactor reference status flags
refs: fully reset `struct ref_iterator::ref` on iteration
refs: introduce `.ref` field for the base iterator
refs: introduce wrapper struct for `each_ref_fn`
Junio C Hamano [Wed, 19 Nov 2025 18:55:37 +0000 (10:55 -0800)]
Merge branch 'ps/packed-git-in-object-store'
The list of packfiles used in a running Git process is moved from
the packed_git structure into the packfile store.
* ps/packed-git-in-object-store:
packfile: track packs via the MRU list exclusively
packfile: always add packfiles to MRU when adding a pack
packfile: move list of packs into the packfile store
builtin/pack-objects: simplify logic to find kept or nonlocal objects
packfile: fix approximation of object counts
http: refactor subsystem to use `packfile_list`s
packfile: move the MRU list into the packfile store
packfile: use a `strmap` to store packs by name
Ezekiel Newren [Tue, 18 Nov 2025 22:34:22 +0000 (22:34 +0000)]
xdiff: rename rindex -> reference_index
The classic diff adds only the lines that it's going to consider,
during the diff, to an array. A mapping between the compacted
array, and the lines of the file that they reference, is
facilitated by this array.
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ezekiel Newren [Tue, 18 Nov 2025 22:34:18 +0000 (22:34 +0000)]
xdiff: split xrecord_t.ha into line_hash and minimal_perfect_hash
The ha field is serving two different purposes, which makes the code
harder to read. At first glance, it looks like many places assume
there could never be hash collisions between lines of the two input
files. In reality, line_hash is used together with xdl_recmatch() to
ensure correct comparisons of lines, even when collisions occur.
To make this clearer, the old ha field has been split:
* line_hash: a straightforward hash of a line, independent of any
external context. Its type is uint64_t, as it comes from a fixed
width hash function.
* minimal_perfect_hash: Not a new concept, but now a separate
field. It comes from the classifier's general-purpose hash table,
which assigns each line a unique and minimal hash across the two
files. A size_t is used here because it's meant to be used to
index an array. This also avoids ` as usize` casts on the Rust
side when using it to index a slice.
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Ezekiel Newren [Tue, 18 Nov 2025 22:34:17 +0000 (22:34 +0000)]
xdiff: use unambiguous types in xdl_hash_record()
Convert the function signature and body to use unambiguous types. char
is changed to uint8_t because this function processes bytes in memory.
unsigned long to uint64_t so that the hash output is consistent across
platforms. `flags` was changed from long to uint64_t to ensure the
high order bits are not dropped on platforms that treat long as 32
bits.
Signed-off-by: Ezekiel Newren <ezekielnewren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a new flag `--all` to git-repo-info for requesting values for all
the available keys. By using this flag, the user can retrieve all the
values instead of searching what are the desired keys for what they
wants.
Helped-by: Karthik Nayak <karthik.188@gmail.com> Helped-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
repo: factor out field printing to dedicated function
Move the field printing in git-repo-info to a new function called
`print_field`, allowing it to be called by functions other than
`print_fields`.
Also change its use of quote_c_style() helper to output directly to
the standard output stream, instead of taking a result in a strbuf
and then printing it outselves.
Signed-off-by: Lucas Seiki Oshiro <lucasseikioshiro@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Tue, 18 Nov 2025 16:07:33 +0000 (16:07 +0000)]
worktree list: quote paths
If a worktree path contains newlines or other control characters
it messes up the output of "git worktree list". Fix this by using
quote_path() to display the worktree path. The output of "git worktree
list" is designed for human consumption, scripts should be using the
"--porcelain" option so this change should not break them.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Phillip Wood [Tue, 18 Nov 2025 16:07:32 +0000 (16:07 +0000)]
worktree list: fix column spacing
The output of "git worktree list" displays a table containing the
worktree path, HEAD OID and branch name for each worktree. The code
aligns the columns by measuring the visual width of the worktree path
when it is printed. Unfortunately it fails to use the visual width
when calculating the width of the column so, if any of the paths
contain a multibyte character, we can end up with excess padding
between columns. The simplest fix would be to replace strlen() with
utf8_strwidth() in measure_widths(). However that leaves us measuring
the visual width twice and the byte length once. By caching the visual
width and printing the padding separately to the worktree path, we only
need to calculate the visual width once and do not need the byte length
at all. The visual widths are stored in an arrays of structs rather
than an array of ints as the next commit will add more struct members.
Even if there are no multibyte characters in any of the paths we still
print an extra space between the path and the object id as the field
width is calculated as one plus the length of the path and we print an
explicit space as well. This is fixed by not printing the extra space.
The tests are updated to include multibyte characters in one of the
worktree paths and to check the spacing of the columns.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 12:21:24 +0000 (07:21 -0500)]
test-mktemp: plug memory and descriptor leaks
We test xmkstemp() in our helper by just calling:
xmkstemp(xstrdup(argv[1]));
This leaks both the copied string as well as the descriptor returned by
the function. In practice this isn't a big deal, since we immediately
exit the program, but:
1. LSan will complain about the memory leak. The only reason we did
not notice this in our leak-checking builds is that both of the
callers in the test suite (both in t0070) pass a broken template
(and expect failure). So the function calls die() before we can
actually leak.
But it's an accident waiting to happen if anybody adds a call which
succeeds.
2. Coverity complains about the descriptor leak. There's a long list
of uninteresting or false positives in Coverity's results, but
since we're here we might as well fix it, too.
I didn't bother adding a new test that triggers the leak. It's not even
in real production code, but just in the test-helper itself.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 09:35:19 +0000 (04:35 -0500)]
ci(windows-meson-test): handle options and output like other test jobs
The GitHub windows-meson-test jobs directly run "meson test" with the
--slice option. This means they skip all of the ci/lib.sh
infrastructure, and in particular:
1. They do not actually set any GIT_TEST_OPTS like --verbose-log or
-x.
2. They do not do the usual handle_failed_tests() magic to print test
failures or tar up failed directories.
As a result, you get almost no feedback at all when a test fails in this
job, making debugging rather tricky.
Let's try to make this behave more like the other CI jobs. Because we're
on Windows, we can't just use the normal run-build-and-tests.sh script.
Our build runs as a separate job (like the non-meson Windows job), and
then we parallelize the tests across several job slices. So we need
something like the run-test-slice.sh script that the "windows-test" job
uses.
In theory we could just swap out the "make" invocation there for
"meson". But it doesn't quite work, because "make" knows how to pull
GIT_TEST_OPTS out of GIT-BUILD-OPTIONS automatically. But for meson, we
have to extract them into the --test-args option ourselves. I tried
making the logic in run-test-slice.sh conditional, but there ended up
being hardly any common code at all (and there are some tricky ordering
constraints). So I added up with a new meson-specific test-slice runner.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 09:32:43 +0000 (04:32 -0500)]
unit-test: ignore --no-chain-lint
In the same spirit as 9faf3963b6 (t: introduce compatibility options to
clar-based tests, 2024-12-13), we should ignore --no-chain-lint passed
to our clar tests, since it may appear in GIT_TEST_OPTS to be used with
other tests.
This is particularly important on Windows CI, where --no-chain-lint is
added to the test options by default, and the meson build will pass all
options to the unit tests. The only reason our meson Windows CI job does
not run into this currently is that it is not respecting GIT_TEST_OPTS
at all! So ignoring this option is a prerequisite to fixing that
situation.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 09:12:30 +0000 (04:12 -0500)]
t: enable ASan's strict_string_checks option
ASan has an option to enable strict string checking, where any pointer
passed to a function that expects a NUL-terminated string will be
checked for that NUL termination. This can sometimes produce false
positives. E.g., it is not wrong to pass a buffer with { '1', '2', '\n' }
into strtoul(). Even though it is not NUL-terminated, it will stop at
the newline.
But in trying it out, it identified two problematic spots in our test
suite (which have now been adjusted):
1. The strtol() parsing in cache-tree.c was a real potential problem,
which would have been very hard to find otherwise (since it
required constructing a very specific broken index file).
2. The use of string functions in fsck_ident() were false positives,
because we knew that there was always a trailing newline which
would stop the functions from reading off the end of the buffer.
But the reasoning behind that is somewhat fragile, and silencing
those complaints made the code easier to reason about.
So even though this did not find any earth-shattering bugs, and even had
a few false positives, I'm sufficiently convinced that its complaints
are more helpful than hurtful. Let's turn it on by default (since the
test suite now runs cleanly with it) and see if it ever turns up any
other instances.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 09:12:28 +0000 (04:12 -0500)]
fsck: avoid parse_timestamp() on buffer that isn't NUL-terminated
In fsck_ident(), we parse the timestamp with parse_timestamp(), which is
really an alias for strtoumax(). But since our buffer may not be
NUL-terminated, this can trigger a complaint from ASan's
strict_string_checks mode. This is a false positive, since we know that
the buffer contains a trailing newline (which we checked earlier in the
function), and that strtoumax() would stop there.
But it is worth working around ASan's complaint. One is because that
will let us turn on strict_string_checks by default, which has helped
catch other real problems. And two is that the safety of the current
code is very hard to reason about (it subtly depends on distant code
which could change).
One option here is to just parse the number left-to-right ourselves. But
we care about the size of a timestamp_t and detecting overflow, since
that's part of the point of these checks. And doing that correctly is
tricky. So we'll instead just pull the digits into a separate,
NUL-terminated buffer, and use that to call parse_timestamp().
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 09:12:25 +0000 (04:12 -0500)]
fsck: remove redundant date timestamp check
After calling "parse_timestamp(p, &end, 10)", we complain if "p == end",
which would imply that we did not see any digits at all. But we know
this cannot be the case, since we would have bailed already if we did
not see any digits, courtesy of extra checks added by 8e4309038f (fsck:
do not assume NUL-termination of buffers, 2023-01-19). Since then,
checking "p == end" is redundant and we can drop it.
This will make our lives a little easier as we refactor further.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 09:12:23 +0000 (04:12 -0500)]
fsck: avoid strcspn() in fsck_ident()
We may be operating on a buffer that is not NUL-terminated, but we use
strcspn() to parse it. This is OK in practice, as discussed in 8e4309038f (fsck: do not assume NUL-termination of buffers, 2023-01-19),
because we know there is at least a trailing newline in our buffer, and
we always pass "\n" to strcspn(). So we know it will stop before running
off the end of the buffer.
But this is a subtle point to hang our memory safety hat on. And it
confuses ASan's strict_string_checks mode, even though it is technically
a false positive (that mode complains that we have no NUL, which is
true, but it does not know that we have verified the presence of the
newline already).
Let's instead open-code the loop. As a bonus, this makes the logic more
obvious (to my mind, anyway). The current code skips forward with
strcspn until it hits "<", ">", or "\n". But then it must check which it
saw to decide if that was what we expected or not, duplicating some
logic between what's in the strcspn() and what's in the domain logic.
Instead, we can just check each character as we loop and act on it
immediately.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 09:12:20 +0000 (04:12 -0500)]
fsck: assert newline presence in fsck_ident()
The fsck code purports to handle buffers that are not NUL-terminated,
but fsck_ident() uses some string functions. This works OK in practice,
as explained in 8e4309038f (fsck: do not assume NUL-termination of
buffers, 2023-01-19). Before calling fsck_ident() we'll have called
verify_headers(), which makes sure we have at least a trailing newline.
And none of our string-like functions will walk past that newline.
However, that makes this code at the top of fsck_ident() very confusing:
*ident = strchrnul(*ident, '\n');
if (**ident == '\n')
(*ident)++;
We should always see that newline, or our memory safety assumptions have
been violated! Further, using strchrnul() is weird, since the whole
point is that if the newline is not there, we don't necessarily have a
NUL at all, and might read off the end of the buffer.
So let's have callers pass in the boundary of our buffer, which lets us
safely find the newline with memchr(). And if it is not there, this is a
BUG(), because it means our caller did not validate the input with
verify_headers() as it was supposed to (and we are better off bailing
rather than having memory-safety problems).
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 09:12:18 +0000 (04:12 -0500)]
cache-tree: avoid strtol() on non-string buffer
A cache-tree extension entry in the index looks like this:
<name> NUL <entry_nr> SPACE <subtree_nr> NEWLINE <binary_oid>
where the "_nr" items are human-readable base-10 ASCII. We parse them
with strtol(), even though we do not have a NUL-terminated string (we'd
generally have an mmap() of the on-disk index file). For a well-formed
entry, this is not a problem; strtol() will stop when it sees the
newline. But there are two problems:
1. A corrupted entry could omit the newline, causing us to read
further. You'd mostly get stopped by seeing non-digits in the oid
field (and if it is likewise truncated, there will still be 20 or
more bytes of the index checksum). So it's possible, though
unlikely, to read off the end of the mmap'd buffer. Of course a
malicious index file can fake the oid and the index checksum to all
(ASCII) 0's.
This is further complicated by the fact that mmap'd buffers tend to
be zero-padded up to the page boundary. So to run off the end, the
index size also has to be a multiple of the page size. This is also
unlikely, though you can construct a malicious index file that
matches this.
The security implications aren't too interesting. The index file is
a local file anyway (so you can't attack somebody by cloning, but
only if you convince them to operate in a .git directory you made,
at which point attacking .git/config is much easier). And it's just
a read overflow via strtol(), which is unlikely to buy you much
beyond a crash.
2. ASan has a strict_string_checks option, which tells it to make sure
that options to string functions (like strtol) have some eventual
NUL, without regard to what the function would actually do (like
stopping at a newline here). This option sometimes has false
positives, but it can point to sketchy areas (like this one) where
the input we use doesn't exhibit a problem, but different input
_could_ cause us to misbehave.
Let's fix it by just parsing the values ourselves with a helper function
that is careful not to go past the end of the buffer. There are a few
behavior changes here that should not matter:
- We do not consider overflow, as strtol() would. But nor did the
original code. However, we don't trust the value we get from the
on-disk file, and if it says to read 2^30 entries, we would notice
that we do not have that many and bail before reading off the end of
the buffer.
- Our helper does not skip past extra leading whitespace as strtol()
would, but according to gitformat-index(5) there should not be any.
- The original quit parsing at a newline or a NUL byte, but now we
insist on a newline (which is what the documentation says, and what
Git has always produced).
Since we are providing our own helper function, we can tweak the
interface a bit to make our lives easier. The original code does not use
strtol's "end" pointer to find the end of the parsed data, but rather
uses a separate loop to advance our "buf" pointer to the trailing
newline. We can instead provide a helper that advances "buf" as it
parses, letting us read strictly left-to-right through the buffer.
I didn't add a new test here. It's surprisingly difficult to construct
an index of exactly the right size due to the way we pad entries. But it
is easy to trigger the problem in existing tests when using ASan's
strict string checking, coupled with a recent change to use NO_MMAP with
ASan builds. So:
make SANITIZE=address
cd t
ASAN_OPTIONS=strict_string_checks=1 ./t0090-cache-tree.sh
triggers it reliably. Technically it is not deterministic because there
is ~8% chance (it's 1-(255/256)^20, or ^32 for sha256) that the trailing
checksum hash has a NUL byte in it. But we compute enough cache-trees in
the course of that script that we are very likely to hit the problem in
one of them.
We can look at making strict_string_checks the default for ASan builds,
but there are some other cases we'd want to fix first.
Reported-by: correctmost <cmlists@sent.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 09:12:13 +0000 (04:12 -0500)]
Makefile: turn on NO_MMAP when building with ASan
Git often uses mmap() to access on-disk files. This leaves a blind spot
in our SANITIZE=address builds, since ASan does not seem to handle mmap
at all. Nor does the OS notice most out-of-bounds access, since it tends
to round up to the nearest page size (so depending on how big the map
is, you might have to overrun it by up to 4095 bytes to trigger a
segfault).
The previous commit demonstrates a memory bug that we missed. We could
have made a new test where the out-of-bounds access was much larger, or
where the mapped file ended closer to a page boundary. But the point of
running the test suite with sanitizers is to catch these problems
without having to construct specific tests.
Let's enable NO_MMAP for our ASan builds by default, which should give
us better coverage. This does increase the memory usage of Git, since
we're copying from the filesystem into heap. But the repositories in the
test suite tend to be small, so the overhead isn't really noticeable
(and ASan already has quite a performance penalty).
There are a few other known bugs that this patch will help flush out.
However, they aren't directly triggered in the test suite (yet). So
it's safe to turn this on now without breaking the test suite, which
will help us add new tests to demonstrate those other bugs as we fix
them.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 09:12:06 +0000 (04:12 -0500)]
pack-bitmap: handle name-hash lookups in incremental bitmaps
If a bitmap has a name-hash cache, it is an array of 32-bit integers,
one per entry in the bitmap, which we've mmap'd from the .bitmap file.
We access it directly like this:
if (bitmap_git->hashes)
hash = get_be32(bitmap_git->hashes + index_pos);
That works for both regular pack bitmaps and for non-incremental midx
bitmaps. There is one bitmap_index with one "hashes" array, and
index_pos is within its bounds (we do the bounds-checking when we load
the bitmap).
But for an incremental midx bitmap, we have a linked list of
bitmap_index structs, and each one has only its own small slice of the
name-hash array. If index_pos refers to an object that is not in the
first bitmap_git of the chain, then we'll access memory outside of the
bounds of its "hashes" array, and often outside of the mmap.
Instead, we should walk through the list until we find the bitmap_index
which serves our index_pos, and use its hash (after adjusting index_pos
to make it relative to the slice we found). This is exactly what we do
elsewhere for incremental midx lookups (like the pack_pos_to_midx() call
a few lines above). But we can't use existing helpers like
midx_for_object() here, because we're walking through the chain of
bitmap_index structs (each of which refers to a midx), not the chain of
incremental multi_pack_index structs themselves.
The problem is triggered in the test suite, but we don't get a segfault
because the out-of-bounds index is too small. The OS typically rounds
our mmap up to the nearest page size, so we just end up accessing some
extra zero'd memory. Nor do we catch it with ASan, since it doesn't seem
to instrument mmaps at all. But if we build with NO_MMAP, then our maps
are replaced with heap allocations, which ASan does check. And so:
make NO_MMAP=1 SANITIZE=address
cd t
./t5334-incremental-multi-pack-index.sh
does show the problem (and this patch makes it go away).
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Tue, 18 Nov 2025 09:11:56 +0000 (04:11 -0500)]
compat/mmap: mark unused argument in git_munmap()
Our mmap compat code emulates mapping by using malloc/free. Our
git_munmap() must take a "length" parameter to match the interface of
munmap(), but we don't use it (it is up to the allocator to know how big
the block is in free()).
Let's mark it as UNUSED to avoid complaints from -Wunused-parameter.
Otherwise you cannot build with "make DEVELOPER=1 NO_MMAP=1".
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Bumps actions/setup-go from 5 to 6. This upgrade includes dependency
updates that incorporate a fix for a critical vulnerability.
[Originally opened at https://github.com/git-for-windows/git/pull/5811]
The pattern `return errno = ..., -1;` is observed several times in
`compat/mingw.c`. It has served us well over the years, but now clang
starts complaining:
compat/mingw.c:723:24: error: possible misuse of comma operator here [-Werror,-Wcomma]
723 | return errno = ENOSYS, -1;
| ^
See for example this failing workflow run:
https://github.com/git-for-windows/git-sdk-arm64/actions/runs/15457893907/job/43513458823#step:8:201
Let's appease clang (and also reduce the use of the no longer common
comma operator).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
cmake: stop trying to build the reftable and xdiff libraries
In the `en/make-libgit-a` topic branch, more precisely in the commits f3b4c89d59f1 (make: delete REFTABLE_LIB, add reftable to LIB_OBJS,
2025-10-02) and cf680cdb9543 (make: delete XDIFF_LIB, add xdiff to
LIB_OBJS, 2025-10-02), the strategy to build three static libraries was
rethought, and instead only one static library is now built.
This is good.
However, the CMake definition was not changed accordingly, and now
CMake-based builds fail thusly:
[...]
Generating hook-list.h
CMake Error at CMakeLists.txt:122 (string):
string sub-command REPLACE requires at least four arguments.
Call Stack (most recent call first):
CMakeLists.txt:711 (parse_makefile_for_sources)
CMake Error at CMakeLists.txt:122 (string):
string sub-command REPLACE requires at least four arguments.
Call Stack (most recent call first):
CMakeLists.txt:717 (parse_makefile_for_sources)
-- Configuring incomplete, errors occurred!
Fix that by removing the parts that expect the reftable and xdiff
objects to be defined separately in the Makefile, still.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
David Macek [Mon, 17 Nov 2025 20:39:44 +0000 (20:39 +0000)]
wincred: avoid memory corruption
`wcsncpy_s()` wants to write the terminating null character so we need
to allocate one more space for it in the target memory block.
This should fix crashes when trying to read passwords. When this
happened, the password/token wouldn't print out and Git would therefore
ask for a new password every time.
Signed-off-by: David Macek <david.macek.0@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Mon, 3 Nov 2025 18:01:48 +0000 (18:01 +0000)]
merge-ort: fix failing merges in special corner case
At GitHub, we had a repository that was triggering
git: merge-ort.c:3032: process_renames: Assertion `newinfo && !newinfo->merged.clean` failed.
during git replay.
This sounds similar to the somewhat recent f6ecb603ff8a (merge-ort: fix
directory rename on top of source of other rename/delete, 2025-08-06),
but the cause is different. Unlike that case, there are no
rename-to-self situations arising in this case, and new to this case it
can only be triggered during a replay operation on the 2nd or later
commit being replayed, never on the first merge in the sequence.
To trigger, the repository needs:
* an upstream which:
* renames a file to a different directory, e.g.
old/file -> new/file
* leaves other files remaining in the original directory (so that
e.g. "old/" still exists upstream even though file has been
removed from it and placed elsewhere)
* a topic branch being rebased where:
* a commit in the sequence:
* modifies old/file
* a subsequent commit in the sequence being replayed:
* does NOT touch *anything* under new/
* does NOT touch old/file
* DOES modify other paths under old/
* does NOT have any relevant renames that we need to detect
_anywhere_ elsewhere in the tree (meaning this interacts
interestingly with both directory renames and cached renames)
In such a case, the assertion will trigger. The fix turns out to be
surprisingly simple. I have a very vague recollection that I actually
considered whether to add such an if-check years ago when I added the
very similar one for oldinfo in 1b6b902d95a5 (merge-ort:
process_renames() now needs more defensiveness, 2021-01-19), but I think
I couldn't figure out a possible way to trigger it and was worried at
the time that if I didn't know how to trigger it then I wasn't so sure
that simply skipping it was correct. Waiting did give me a chance to
put more thorough tests and checks into place for the rename-to-self
cases a few months back, which I might not have found as easily
otherwise. Anyway, put the check in place now and add a test that
demonstrates the fix.
Note that this bug, as demonstrated by the conditions listed above,
runs at the intersection of relevant renames, trivial directory
resolutions, and cached renames. All three of those optimizations are
ones that unfortunately make the code (and testcases!) a bit more
complex, and threading all three makes it a bit more so. However, the
testcase isn't crazy enough that I'd expect no one to ever hit it in
practice, and was confused why we didn't see it before. After some
digging, I discovered that merge.directoryRenames=false is a workaround
to this bug, and GitHub used that setting until recently (it was a
"temporary" match-what-libgit2-does piece of code that lasted years
longer than intended). Since the conditions I gave above for triggering
this bug rule out the possibility of there being directory renames, one
might assume that it shouldn't matter whether you try to detect such
renames if there aren't any. However, due to commit a16e8efe5c2b
(merge-ort: fix merge.directoryRenames=false, 2025-03-13), the heavy
hammer used there means that merge.directoryRenames=false ALSO turns off
rename caching, which is critical to triggering the bug. This becomes
a bit more than an aside since...
Re-reading that old commit, a16e8efe5c2b (merge-ort: fix
merge.directoryRenames=false, 2025-03-13), it appears that the solution
to this latest bug might have been at least a partial alternative
solution to that old commit. And it may have been an improved
alternative (or at least help implement one), since it may be able to
avoid the heavy-handed disabling of rename cache. That might be an
interesting future thing to investigate, but is not critical for the
current fix. However, since I spent time digging it all up, at least
leave a small comment tweak breadcrumb to help some future reader
(myself or others) who wants to dig further to connect the dots a little
quicker.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Mon, 3 Nov 2025 18:01:47 +0000 (18:01 +0000)]
merge-ort: remove debugging crud
While developing commit a16e8efe5c2b (merge-ort: fix
merge.directoryRenames=false, 2025-03-13), I was testing things out and
had an extra condition on one of the if-blocks that I occasionally
swapped between '&& 0' and '&& 1' to see the effects of the changes. I
forgot to remove it before submitting and it wasn't caught in review.
Remove it now.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Elijah Newren [Mon, 3 Nov 2025 18:01:46 +0000 (18:01 +0000)]
t6429: update comment to mention correct tool
A comment at the top of t6429 mentions why the test doesn't exercise git
rebase or git cherry-pick. However, it claims that it uses `test-tool
fast-rebase`. That was true when the comment was written, but commit f920b0289ba3 (replay: introduce new builtin, 2023-11-24) changed it to
use git replay without updating this comment.
We could potentially just strike this second comment, since git replay
is a bona fide built-in, but perhaps the explanation about why it focuses
on git replay is still useful. Update the comment to make it accurate
again.
Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When Scalar was made a canonical part of Git in 7b5c93c6c68 (scalar:
include in standard Git build & installation, 2022-09-02), it was added
to all relevant Makefile targets except for the `strip` target.
Let's correct that.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
René Scharfe [Mon, 17 Nov 2025 19:42:55 +0000 (20:42 +0100)]
wrapper: simplify xmkstemp()
Call xmkstemp_mode() instead of duplicating its error handling code.
This switches the implementation from the system's mkstemp(3) to our own
git_mkstemp_mode(), which works just as well.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Antonin Delpeuch [Mon, 17 Nov 2025 08:04:32 +0000 (08:04 +0000)]
blame: make diff algorithm configurable
The diff algorithm used in 'git-blame(1)' is set to 'myers',
without the possibility to change it aside from the `--minimal` option.
There has been long-standing interest in changing the default diff
algorithm to "histogram", and Git 3.0 was floated as a possible occasion
for taking some steps towards that:
As a preparation for this move, it is worth making sure that the diff
algorithm is configurable where useful.
Make it configurable in the `git-blame(1)` command by introducing the
`--diff-algorithm` option and make honor the `diff.algorithm` config
variable. Keep Myers diff as the default.
Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Antonin Delpeuch [Mon, 17 Nov 2025 08:04:31 +0000 (08:04 +0000)]
xdiff: add 'minimal' to XDF_DIFF_ALGORITHM_MASK
The XDF_DIFF_ALGORITHM_MASK bit mask only includes bits for the patience
and histogram diffs, not for the minimal one. This means that when
reseting the diff algorithm to the default one, one needs to separately
clear the bit for the minimal diff. There are places in the code that fail
to do that: merge-ort.c and builtin/merge-file.c.
Add the XDF_NEED_MINIMAL bit to the bit mask, and remove the separate
clearing of this bit in the places where it hasn't been forgotten.
Signed-off-by: Antonin Delpeuch <antonin@delpeuch.eu> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Mon, 17 Nov 2025 15:00:12 +0000 (07:00 -0800)]
Merge branch 'jc/ci-use-arm64-p4-on-macos'
We replaced deprecated macos-13 with macos-14 image in GitHub
Actions CI, but we forgot that the image is for arm64. We have
been seeing a lot of test failures ever since. Switch to arm64
binary for Perforce tests.
* jc/ci-use-arm64-p4-on-macos:
Use Perforce arm64 binary on macOS CI jobs
Christian Couder [Mon, 17 Nov 2025 04:34:49 +0000 (05:34 +0100)]
commit: refactor verify_commit_buffer()
In a following commit, we are going to check commit signatures, but we
won't have a commit yet, only a commit buffer, and we are going to
discard this commit buffer if the signature is invalid. So it would be
wasteful to create a commit that we might discard, just to be able to
check a commit signature.
It would be simpler instead to be able to check commit signatures
using only a commit buffer instead of a commit.
To be able to do that, let's extract some code from the
check_commit_signature() function into a new verify_commit_buffer()
function, and then let's make check_commit_signature() call
verify_commit_buffer().
Note that this doesn't fundamentally change how
check_commit_signature() works. It used to call parse_signed_commit()
which calls repo_get_commit_buffer(), parse_buffer_signed_by_header()
and repo_unuse_commit_buffer(). Now these 3 functions are called
directly by verify_commit_buffer().
Signed-off-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The previous implementation used simple width formatting with printf()
which didn't properly handle multi-byte UTF-8 characters, causing
misaligned table columns when displaying repository structure
information.
This change modifies the stats_table_print_structure function to use
strbuf_utf8_align() instead of basic printf width specifiers. This
ensures proper column alignment regardless of the character encoding of
the content being displayed.
Also add test cases for strbuf_utf8_align(), a function newly introduced
in "builtin/repo.c".
Signed-off-by: Jiang Xin <worldhello.net@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jiang Xin [Sat, 15 Nov 2025 13:36:10 +0000 (08:36 -0500)]
t/unit-tests: add UTF-8 width tests for CJK chars
The file "builtin/repo.c" uses utf8_strwidth() to calculate the display
width of UTF-8 characters in a table, but the resulting output is still
misaligned. Add test cases for both utf8_strwidth and utf8_strnwidth to
verify that they correctly compute the display width for UTF-8
characters.
Also updated the build configuration in Makefile and meson.build to
include the new test suite in the build process.
Signed-off-by: Jiang Xin <worldhello.net@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Sun, 16 Nov 2025 23:10:28 +0000 (15:10 -0800)]
Use Perforce arm64 binary on macOS CI jobs
The previous step replaced deprecated macos-13 image with macos-14
image on GitHub Actions CI. While x86-64 binaries can work there,
because macos-14 images are arm64 based (we could replace it with
macos-14-large that is x86-64), it makes more sense to use arm64
binary there. Without this change, we have been getting unusually
higher rate of failures from random macOS CI jobs railing to run
t98xx series of tests.
Helped-by: Koji Nakamaru <koji.nakamaru@gree.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Sat, 15 Nov 2025 00:58:18 +0000 (00:58 +0000)]
read-cache: drop submodule check from add_to_cache()
In add_to_cache(), we treat any directories as submodules, and complain
if we can't resolve their HEAD. This call to resolve_gitlink_ref() was
added by f937bc2f86 (add: error appropriately on repository with no
commits, 2019-04-09), with the goal of improving the error message for
empty repositories.
But we already resolve the submodule HEAD in index_path(), which is
where we find the actual oid we're going to use. Resolving it again here
introduces some downsides:
1. It's more work, since we have to open up the submodule repository's
files twice.
2. There are call paths that get to index_path() without going through
add_to_cache(). For instance, we'd want a similar informative
message if "git diff empty" finds that it can't resolve the
submodule's HEAD. (In theory we can also get there through
update-index, but AFAICT it refuses to consider directories as
submodules at all, and just complains about them).
3. The resolution in index_path() catches more errors that we don't
handle here. In particular, it will validate that the object format
for the submodule matches that of the superproject. This isn't a
bug, since our call in add_to_cache() throws away the oid it gets
without looking at it. But it certainly caused confusion for me
when looking at where the object-format check should go.
So instead of resolving the submodule HEAD in add_to_cache(), let's just
teach the call in index_path() to actually produce an error message
(which it already does for other cases). That's probably what f937bc2f86
should have done in the first place, and it gives us a single point of
resolution when adding a submodule to the index.
The resulting output is slightly more verbose, as we propagate the error
up the call stack, but I think that's OK (and again, matches many other
errors we get when indexing fails).
I've left the text of the error message as-is, though it is perhaps
overly specific. There are many reasons that resolving the submodule
HEAD might fail, though outside of corruption or system errors it is
probably most likely that the submodule HEAD is simply on an unborn
branch.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
brian m. carlson [Sat, 15 Nov 2025 00:58:17 +0000 (00:58 +0000)]
object-file: disallow adding submodules of different hash algo
The design of the hash algorithm transition plan is that objects stored
must be entirely in one algorithm since we lack any way to indicate a
mix of algorithms. This also includes submodules, but we have
traditionally not enforced this, which leads to various problems when
trying to clone or check out the the submodule from the remote.
Since this cannot work in the general case, restrict adding a submodule
of a different algorithm to the index. Add tests for git add and git
submodule add that these are rejected.
Note that we cannot check this in git fsck because the malformed
submodule is stored in the tree as an object ID which is either
truncated (when a SHA-256 submodule is added to a SHA-1 repository) or
padded with zeros (when a SHA-1 submodule is added to a SHA-256
repository). We cannot detect even the latter case because someone
could have an actual submodule that actually ends in 24 zeros, which
would be a false positive.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
doc: commit: link to git-status(1) on all format options
`--branch` and `--long` refer to git-status(1) options but they don’t tell us
what `short-format` and `long-format` are, respectively. And `--null`
mentions “status” but does not link to the command.
Refer to git-config(1) on `--branch` like `--short` does.
`long-format` is the git-status(1) output. So we can just say that
directly.
Replace “status” with a `linkgit` on `--null`.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Koji Nakamaru [Fri, 14 Nov 2025 06:04:30 +0000 (06:04 +0000)]
osxkeychain: avoid incorrectly skipping store operation
git-credential-osxkeychain skips storing a credential if its "get"
action sets "state[]=osxkeychain:seen=1". This behavior was introduced
in e1ab45b2 (osxkeychain: state to skip unnecessary store operations,
2024-05-15), which appeared in v2.46.
However, this state[] persists even if a credential returned by
"git-credential-osxkeychain get" is invalid and a subsequent helper's
"get" operation returns a valid credential. Another subsequent helper
(such as [1]) may expect git-credential-osxkeychain to store the valid
credential, but the "store" operation is incorrectly skipped because it
only checks "state[]=osxkeychain:seen=1".
To solve this issue, "state[]=osxkeychain:seen" needs to contain enough
information to identify whether the current "store" input matches the
output from the previous "get" operation (and not a credential from
another helper).
Set "state[]=osxkeychain:seen" to a value encoding the credential output
by "get", and compare it with a value encoding the credential input by
"store".
Reported-by: Petter Sælen <petter@saelen.eu> Helped-by: Junio C Hamano <gitster@pobox.com> Helped-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Koji Nakamaru <koji.nakamaru@gree.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Wed, 12 Nov 2025 22:02:58 +0000 (14:02 -0800)]
attr: enable incomplete-line whitespace error for this project
Now "git diff --check" and "git apply --whitespace=warn/fix" learned
incomplete line is a whitespace error, enable them for this project
to prevent patches to add new incomplete lines to our source to both
code and documentation files.
Junio C Hamano [Wed, 12 Nov 2025 22:02:57 +0000 (14:02 -0800)]
diff: highlight and error out on incomplete lines
Teach "git diff" to highlight "\ No newline at end of file" message
as a whitespace error when incomplete-line whitespace error class is
in effect. Thanks to the previous refactoring of complete rewrite
code path, we can do this at a single place.
Unlike whitespace errors in the payload where we need to annotate in
line, possibly using colors, the line that has whitespace problems,
we have a dedicated line already that can serve as the error
message, so paint it as a whitespace error message.
Also teach "git diff --check" to notice incomplete lines as
whitespace errors and report when incomplete-line whitespace error
class is in effect.
Junio C Hamano [Wed, 12 Nov 2025 22:02:56 +0000 (14:02 -0800)]
apply: check and fix incomplete lines
The final line of a file that lacks the terminating newline at its
end is called an incomplete line. In general they are frowned upon
for many reasons (imagine concatenating two files with "cat A B" and
what happens when A ends in an incomplete line, for example), and
text-oriented tools often mishandle such a line.
Implement checks in "git apply" for incomplete lines, which is off
by default for backward compatibility's sake, so that "git apply
--whitespace={fix,warn,error}" can notice, warn against, and fix
them.
As one of the new test shows, if you modify contents on an
incomplete line in the original and leave the resulting line
incomplete, it is still considered a whitespace error, the reasoning
being that "you'd better fix it while at it if you are making a
change on an incomplete line anyway", which may controversial.
Junio C Hamano [Wed, 12 Nov 2025 22:02:55 +0000 (14:02 -0800)]
whitespace: allocate a few more bits and define WS_INCOMPLETE_LINE
Reserve a few more bits in the diff flags word to be used for future
whitespace rules. Add WS_INCOMPLETE_LINE without implementing the
behaviour (yet).
Junio C Hamano [Wed, 12 Nov 2025 22:02:54 +0000 (14:02 -0800)]
apply: revamp the parsing of incomplete lines
A patch file represents the incomplete line at the end of the file
with two lines, one that is the usual "context" with " " as the
first letter, "added" with "+" as the first letter, or "removed"
with "-" as the first letter that shows the content of the line,
plus an extra "\ No newline at the end of file" line that comes
immediately after it.
Ever since the apply machinery was written, the "git apply"
machinery parses "\ No newline at the end of file" line
independently, without even knowing what line the incomplete-ness
applies to, simply because it does not even remember what the
previous line was.
This poses a problem if we want to check and warn on an incomplete
line. Revamp the code that parses a fragment, to actually drop the
'\n' at the end of the incoming patch file that terminates a line,
so that check_whitespace() calls made from the code path actually
sees an incomplete as incomplete.
Note that the result of this parsing is not directly used by the
code path that applies the patch. apply_one_fragment() function
already checks if each of the patch text it handles is followed by a
line that begins with a backslash to drop the newline at the end of
the current line it is looking at. In a sense, this patch harmonizes
the behaviour of the parsing side to what is already done in the
application side.
Junio C Hamano [Wed, 12 Nov 2025 22:02:53 +0000 (14:02 -0800)]
diff: update the way rewrite diff handles incomplete lines
The diff_symbol based output framework uses one DIFF_SYMBOL_* enum
value per the kind of output lines of "git diff", which corresponds
to one output line from the xdiff machinery used internally. Most
notably, DIFF_SYMBOL_PLUS and DIFF_SYMBOL_MINUS that correspond to
"+" and "-" lines are designed to always take a complete line, even
if the output from xdiff machinery may produce "\ No newline at the
end of file" immediately after them.
But this is not true in the rewrite-diff codepath, which completely
bypasses the xdiff machinery. Since the code path feeds the bytes
directly from the payload to the output routines, the output layer
has to deal with an incomplete line with DIFF_SYMBOL_PLUS and
DIFF_SYMBOL_MINUS, which never would see an incomplete line in the
normal code paths. This lack of final newline is compensated by an
ugly hack for a fabricated DIFF_SYMBOL_NO_LF_EOF token to inject an
extra newline to the output to simulate output coming from the xdiff
machinery.
Revamp the way the complete-rewrite code path feeds the lines to the
output layer by treating the last line of the pre/post image when it
is an incomplete line specially.
This lets us remove the DIFF_SYMBOL_NO_LF_EOF hack and use the usual
DIFF_SYMBOL_CONTEXT_INCOMPLETE code path, which will later learn how
to handle whitespace errors.
Junio C Hamano [Wed, 12 Nov 2025 22:02:52 +0000 (14:02 -0800)]
diff: call emit_callback ecbdata everywhere
Everybody else, except for emit_rewrite_lines(), calls the
emit_callback data ecbdata. Make sure we call the same thing by
the same name for consistency.
Junio C Hamano [Wed, 12 Nov 2025 22:02:51 +0000 (14:02 -0800)]
diff: refactor output of incomplete line
Create a helper function that reacts to "\ No newline at the end of
file" in preparation for unifying the incomplete line handling in
the code path that handles xdiff output and the code path that
bypasses xdiff and produces a complete-rewrite patch.
Currently the output from the DIFF_SYMBOL_CONTEXT_INCOMPLETE case
still (ab)uses the same code as what is used for context lines, but
that would change in a later step where we introduce support to treat
an incomplete line as a whitespace error.
Junio C Hamano [Wed, 12 Nov 2025 22:02:50 +0000 (14:02 -0800)]
diff: keep track of the type of the last line seen
The "\ No newline at the end of the file" can come after any of the
"-" (deleted preimage line), " " (unchanged line), or "+" (added
postimage line). In later steps in this series, we will start
treating a change that makes a file to end in an incomplete line
as a whitespace error, and we would need to know what the previous
line was when we react to "\ No newline" in the diff output. If
the previous line was a context (i.e., unchanged) line, the file
lacked the final newline before the change, and the change did not
touch that line and left it still incomplete, so we do not want to
warn in such a case.
Teach fn_out_consume() function to keep track of what the previous
line was, and prepare an otherwise empty switch statement to let us
react differently to "\ No newline" based on that.
Note that there is an existing curiosity (read: likely to be a bug)
in the code that increments line number in the preimage file every
time it sees a line with "\ No newline" on it, regardless of what
the previous line was. I left it as-is, because it does not affect
the main theme of this series, and more importantly, I do not think
it matters, as these numbers are used only to compare them with
blank_at_eof_in_{pre,post}image to issue a warning when we see more
empty line was added at the end, but by definition, after we see
"\ No newline at the end of the file" for an added line, we will not
see an added line for the file.
An independent audit to ensure that this curious increment can be
safely removed would make a good #leftoverbits clean-up (we may even
find some code that decrements this counter or over-increments the
other quantity this counter is compared with that compensates the
effect of this curious increment that hides a bug, in which case we
may also need to remove them).
Junio C Hamano [Wed, 12 Nov 2025 22:02:49 +0000 (14:02 -0800)]
diff: correct suppress_blank_empty hack
The suppress-blank-empty feature abused the CONTEXT_INCOMPLETE
symbol that was meant to be used only for "\ No newline at the end
of file" code path.
The intent of the feature was to turn a context line we receive from
xdiff machinery (which always uses ' ' for context lines, even an
empty one) and spit it out as a truly empty line.
Perform such a conversion very locally at where a line from xdiff
that begins with ' ' is handled for output; there are many checks
before the control reaches such place that checks the first letter
of the diff output line to see if it is a context line, and having
to check for '\n' and treat it as a special case is error prone.
In order to catch similar hacks in the future, make sure the code
path that is meant for "\ No newline" case checks the first byte is
indeed a backslash.
projects / thirdparty / git.git / log
