object-file: move logic to compute packed abbreviation length
Same as the preceding commit, move the logic that computes the minimum
required prefix length to make a given object ID unique for the packfile
store into a new function `packfile_store_find_abbrev_len()` that is
part of "packfile.c". This prepares for making the logic fully generic
via pluggable object databases.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
object-name: move logic to compute loose abbreviation length
The function `repo_find_unique_abbrev_r()` takes as input an object ID
as well as a minimum object ID length and returns the minimum required
prefix to make the object ID unique.
The logic that computes the abbreviation length for loose objects is
deeply tied to the loose object storage format. As such, it would fail
in case a different object storage format was used.
Prepare for making this logic generic to the backend by moving the logic
into a new `odb_source_loose_find_abbrev_len()` function that is part of
"object-file.c".
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function `extend_abbrev_len()` computes the length of common hex
characters between two object IDs. This is done by:
- Making the caller provide the `hex` string for the needle object ID.
- Comparing every hex position of the haystack object ID with
`get_hex_char_from_oid()`.
Turning the binary representation into hex first is roundabout though:
we can simply compare the binary representation and give some special
attention to the final nibble.
Introduce a new function `oid_common_prefix_hexlen()` that does exactly
this and refactor the code to use the new function. This allows us to
drop the `struct min_abbrev_data::hex` field. Furthermore, this function
will be used in by some other callsites in subsequent commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
object-name: abbreviate loose object names without `disambiguate_state`
The function `find_short_object_filename()` takes an object ID and
computes the minimum required object name length to make it unique. This
is done by reusing the object disambiguation infrastructure, where we
iterate through every loose object and then update the disambiguate
state one by one.
Ultimately, we don't care about the disambiguate state though. It is
used because this infrastructure knows how to enumerate only those
objects that match a given prefix. But now that we have extended the
`odb_for_each_object()` function to do this for us we have an easier way
to do this. Consequently, we really only use the disambiguate state now
to propagate `struct min_abbrev_data`.
Refactor the code and drop this indirection so that we use `struct
min_abbrev_data` directly. This also allows us to drop some now-unused
logic from the disambiguate infrastructure.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
object-name: merge `update_candidates()` and `match_prefix()`
There's only a single callsite for `match_prefix()`, and that function
is a rather trivial wrapper of `update_candidates()`. Merge these two
functions into a single `update_disambiguate_state()` function.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function `get_short_oid()` takes as input an abbreviated object ID
and tries to turn that object ID into the full object ID. This is done
by iterating through all objects that have the user-provided prefix. If
that yields exactly one object we know that the abbreviated object ID is
unambiguous, otherwise it is ambiguous and we print the list of objects
that match the prefix.
We iterate through all objects with the given prefix by calling both
`find_short_packed_object()` and `find_short_object_filename()`, which
is of course specific to the "files" backend. But we now have a generic
way to iterate through objects with a specific prefix.
Refactor the code to use `odb_for_each_object()` instead so that it
works with object backends different than the "files" backend.
Remove the now-unused `find_short_packed_object()` function.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function `repo_collect_ambiguous()` is responsible for collecting
objects whose IDs match a specific prefix. The information is then
used to inform the user about which objects they could have meant in
case a short object ID is ambiguous.
The logic to do this uses the object disambiguation infrastructure and
calls into backend-specific functions to iterate through loose and
packed objects. This isn't really required anymore though: all we want
to do is to enumerate objects that have such a prefix and then append
those objects to a `struct oid_array`. This can be trivially achieved
in a generic way now that `odb_for_each_object()` has learned to yield
only objects that match such a prefix.
Refactor the code to use the backend-generic infrastructure instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
object-name: move logic to iterate through loose prefixed objects
The logic to iterate through loose objects that have a certain prefix is
currently hosted in "object-name.c". This logic reaches into specifics
of the loose object source, so it breaks once a different backend is
used for the object storage.
Move the logic to iterate through loose objects with a prefix into
"object-file.c". This is done by extending the for-each-object options
to support an optional prefix that is then honored by the loose source.
Naturally, we'll also have this support in the packfile store. This is
done in the next commit.
Furthermore, there are no users of the loose cache outside of
"object-file.c" anymore. As such, convert `odb_source_loose_cache()` to
have file scope.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `odb_for_each_object()` function only accepts a bitset of flags. In
a subsequent commit we'll want to change object iteration to also
support iterating over only those objects that have a specific prefix.
While we could of course add the prefix to the function signature, or
alternatively introduce a new function, both of these options don't
really seem to be that sensible.
Instead, introduce a new `struct odb_for_each_object_options` that can
be passed to a new `odb_for_each_object_ext()` function. Splice through
the options structure into the respective object database sources.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
oidtree: extend iteration to allow for arbitrary return codes
The interface `cb_each()` iterates through a crit-bit tree and calls a
specific callback function for each of the contained items. The callback
function is expected to return either:
- `CB_CONTINUE` in case iteration shall continue.
- `CB_BREAK` to abort iteration.
This is needlessly restrictive though, as callers may want to return
arbitrary values and have them be bubbled up to the `cb_each()` call
site. In fact, this is a rather common pattern we have: whenever such a
callback function returns a non-zero error code, we abort iteration and
bubble up the code as-is.
Refactor both the crit-bit tree and oidtree subsystems to behave
accordingly.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "oidtree.c" subsystem is rather small and self-contained and tends
to just work. It thus doesn't typically receive a lot of attention,
which has as a consequence that it's coding style is somewhat dated
nowadays.
Modernize the style of this subsystem a bit:
- Rename the `oidtree_iter()` function to `oidtree_each_cb()`.
- Rename `struct oidtree_iter_data` to `struct oidtree_each_data` to
match the renamed callback function type.
- Rename parameters and variables to clarify their intent.
- Add comments that explain what some of the functions do.
- Adapt the return value of `oidtree_contains()` to be a boolean.
This prepares for some changes to the subsystem that'll happen in the
next commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
path-walk: fix NULL pointer dereference in error message
When lookup_tree() or lookup_blob() cannot find a tree entry's object,
'o' is set to NULL via:
o = child ? &child->object : NULL;
The subsequent null-check catches this correctly, but then dereferences
'o' to format the error message:
error(_("failed to find object %s"), oid_to_hex(&o->oid));
This causes a segfault instead of the intended diagnostic output.
Fix this by using &entry.oid instead. 'entry' is the struct name_entry
populated by tree_entry() on each loop iteration and holds the OID of
the failing lookup -- which is exactly what the error should report.
This crash is reachable via git-backfill(1) when a tree entry's object
is absent from the local object database.
Signed-off-by: Yuvraj Singh Chauhan <ysinghcin@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Johannes Sixt [Fri, 20 Mar 2026 08:23:32 +0000 (09:23 +0100)]
Merge branch 'jx/i18n-fix' of github.com:jiangxin/gitk
* 'jx/i18n-fix' of github.com:jiangxin/gitk:
gitk: l10n: make PO headers identify the Gitk project
gitk: ignore generated POT file
gitk: i18n: use "Gitk" as package name in POT file
Jeff King [Tue, 17 Mar 2026 23:02:23 +0000 (19:02 -0400)]
contrib/diff-highlight: do not highlight identical pairs
We pair lines for highlighting based on their position in the hunk. So
we should never see two identical lines paired, like:
-one
-two
+one
+something else
which would pair -one/+one, because that implies that the diff could
easily be shrunk by turning line "one" into context.
But there is (at least) one exception: removing a newline at the end of
a file will produce a diff like:
-foo
+foo
\No newline at end of file
And we will pair those two lines. As a result, we end up marking the
whole line, including the newline, as the shared prefix. And there's an
empty suffix.
The most obvious bug here is that when we try to print the highlighted
lines, we remove the trailing newline from the suffix, but do not bother
with the prefix (under the assumption that there had to be a difference
_somewhere_ in the line, and thus the prefix would not eat all the way
up to the newline). And so you get an extra line like:
-foo
+foo
\No newline at end of file
This is obviously ugly, but also causes interactive.diffFilter to
(rightly) complain that the input and output do not match their lines
1-to-1.
This could easily be fixed by chomping the prefix, too, but I think the
problem is deeper. For one, I suspect some of the other logic gets
confused by forming an array with zero-indexed element "3" in a
3-element array. But more importantly, we try not to highlight whole
lines, as there's nothing interesting to show there. So let's catch this
early in is_pair_interesting() and bail to our usual passthrough
strategy.
Reported-by: Scott Baker <scott@perturb.org> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jiang Xin [Thu, 19 Mar 2026 09:11:17 +0000 (17:11 +0800)]
gitk: l10n: make PO headers identify the Gitk project
Commit f697d08 (gitk: i18n: use "Gitk" as package name in POT file,
2026-03-19) updated the generated POT template to use "Gitk" in its
Project-Id-Version header. Several existing PO files still carry older
header values such as "git" or "git-gui", so they do not consistently
identify themselves as Gitk translations.
Update the Project-Id-Version field in all Gitk PO files so that they
identify the Gitk project consistently.
The "Project-Id-Version" field in the PO header helps tools identify
which project a PO file belongs to. For example, Git's
"git-po-helper" uses it to choose project-specific checks and POT
handling rules. Without this change, some Gitk PO files are
misidentified because their headers still refer to other projects.
Junio C Hamano [Thu, 19 Mar 2026 22:39:02 +0000 (15:39 -0700)]
cocci: strbuf.buf is never NULL
We recently noticed one old code from 19 years ago protecting
against an ancient strbuf convention that the .buf member can be
NULL for an empty strbuf. As that is no longer the case in the
modern codebase, let's catch such a construct.
Jiang Xin [Thu, 19 Mar 2026 09:08:47 +0000 (17:08 +0800)]
gitk: ignore generated POT file
"po/gitk.pot" is generated from the source for translation maintenance.
Ignore it in the working tree so regenerating the template does not
introduce unnecessary noise in `git status`.
Jiang Xin [Thu, 19 Mar 2026 09:06:25 +0000 (17:06 +0800)]
gitk: i18n: use "Gitk" as package name in POT file
Use "Gitk" instead of the placeholder "PACKAGE" in the header of the
generated po/gitk.pot file. In particular, the "Project-Id-Version"
field in the header entry should be set to:
"Project-Id-Version: Gitk\n"
New PO files generated from this POT file will inherit that package
name.
René Scharfe [Thu, 19 Mar 2026 16:24:40 +0000 (17:24 +0100)]
commit-reach: simplify cleanup of remaining bitmaps in ahead_behind ()
Don't bother extracting the last few remaining prio_queue items in
order when we only want to free their associated bitmaps; just iterate
over the item array.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Thu, 19 Mar 2026 16:54:56 +0000 (09:54 -0700)]
Merge branch 'ps/unit-test-c-escape-names.txt'
The unit test helper function was taught to use backslash +
mnemonic notation for certain control characters like "\t", instead
of octal notation like "\011".
Junio C Hamano [Thu, 19 Mar 2026 07:15:59 +0000 (00:15 -0700)]
rerere: update to modern representation of empty strbufs
Back when b4833a2c (rerere: Fix use of an empty strbuf.buf,
2007-09-26) was written, a freshly initialized empty strbuf
had NULL in its .buf member, with .len set to 0. The code this
patch touches in rerere.c was written to _fix_ the original code
that assumed that the .buf member is always pointing at a NUL-terminated
string, even for an empty string, which did not hold back then.
That changed in b315c5c0 (strbuf change: be sure ->buf is never ever
NULL., 2007-09-27), and it has again become safe to assume that .buf
is never NULL, and .buf[0] has '\0' for an empty string (i.e., a
strbuf with its .len member set to 0).
A funny thing is, this piece of code has been moved around from
builtin-rerere.c to rerere.c and also adjusted for updates to the
hash function API over the years, but nobody bothered to question
if this special casing for an empty strbuf was still necessary:
b4833a2c62 (rerere: Fix use of an empty strbuf.buf, 2007-09-26) 5b2fd95606 (rerere: Separate libgit and builtin functions, 2008-07-09) 9126f0091f (fix openssl headers conflicting with custom SHA1 implementations, 2008-10-01) c0f16f8e14 (rerere: factor out handle_conflict function, 2018-08-05) 0d7c419a94 (rerere: convert to use the_hash_algo, 2018-10-15) 0578f1e66a (global: adapt callers to use generic hash context helpers, 2025-01-31)
Finally get rid of the special casing that was unnecessary for the
last 19 years.
Every compilation unit in Git is expected to include "git-compat-util.h"
first, either directly or indirectly via "builtin.h". This header papers
over differences between platforms so that we can expect the typical
POSIX functions to exist. Furthermore, it provides functionality that we
end up using everywhere.
This header is thus quite heavy as a consequence. Preprocessing it as a
standalone unit via `clang -E git-compat-util.h` yields over 23,000
lines of code overall. Naturally, it takes quite some time to compile
all of this.
Luckily, this is exactly the kind of use case that precompiled headers
aim to solve: instead of recompiling it every single time, we compile it
once and then link the result into the executable. If include guards are
set up properly it means that the file won't need to be reprocessed.
Set up such a precompiled header for "git-compat-util.h" and wire it up
via Meson. This causes Meson to implicitly include the precompiled
header in all compilation units. With GCC and Clang for example this is
done via the "-include" statement [1].
This leads to a significant speedup when performing full builds:
Benchmark 1: ninja (rev = HEAD~)
Time (mean ± σ): 14.467 s ± 0.126 s [User: 248.133 s, System: 31.298 s]
Range (min … max): 14.195 s … 14.633 s 10 runs
Benchmark 2: ninja (rev = HEAD)
Time (mean ± σ): 10.307 s ± 0.111 s [User: 173.290 s, System: 23.998 s]
Range (min … max): 10.030 s … 10.433 s 10 runs
Summary
ninja (rev = HEAD) ran
1.40 ± 0.02 times faster than ninja (rev = HEAD~)
In the next commit we're about to introduce a precompiled header for
"git-compat-util.h". The consequence of this change is that we'll
implicitly include that header for every compilation unit that uses the
precompiled headers.
This is okay for our "normal" library sources and our builtins. But some
of our compatibility sources do not include the header on purpose, and
doing so would cause compilation errors.
Prepare for this change by splitting out compatibility sources into
their static library. Like this, we can selectively enable precompiled
headers for the library sources.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
git-compat-util.h: move warning infra to prepare for PCHs
The "git-compat-util.h" header is supposed to be the first header
included by every code compilation unit. As such, a subsequent commit
will start to precompile this header to speed up compilation of Git.
This will cause an issue though with the way that we have set up the
"-Wsign-compare" warnings. It is expected that any compilation unit that
fails with that compiler warning sets `DISABLE_SIGN_COMPARE_WARNINGS`
before including "git-compat-util.h". If so, we'll disable the warning
right away via a compiler pragma.
But with precompiled headers we do not know ahead of time whether the
code unit wants to disable those warnings, and thus we'll have to
precompile the header without defining `DISABLE_SIGN_COMPARE_WARNINGS`.
But as the pragma statement is wrapped by our include guards, the second
include of that file will not have the desired effect of disabling the
warnings anymore.
We could fix this issue by declaring a new macro that compilation units
are expected to invoke after having included the file. In retrospect,
that would have been the better way to handle this as it allows for
more flexibility: we could for example toggle the warning for specific
code blocks, only. But changing this now would require a bunch of
changes, and the churn feels excessive for what we gain.
Instead, prepare for the precompiled headers by moving the code outside
of the include guards.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have a bunch of scripts used by our different build systems that are
all located in the top-level directory. Now that we have introduced the
new "tools/" directory though we have a better home for them.
Move the scripts into the "tools/" directory.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
contrib: move "update-unicode.sh" script into "tools/"
The "update-unicode.sh" script is used to update the unicode data
compiled into Git whenever a new version of the Unicode standard has
been released. As such, it is a natural part of our developer-facing
tooling, and its presence in "contrib/" is misleading.
Promote the script into the new "tools/" directory.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
contrib: move "coverage-diff.sh" script into "tools/"
The "coverage-diff.sh" script can be used to get information about test
coverage fro the Git codebase. It is thus rather specific to our build
and test infrastructure and part of the developer-facing tooling. The
fact that this script is part of "contrib/" is thus rather misleading
and a historic wart.
Promote the tool into the new "tools/" directory.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
contrib: move "coccinelle/" directory into "tools/"
The Coccinelle tool is an ingrained part of our build infrastructure. It
is executed by our CI to detect antipatterns and is used to detect
misuses of certain interfaces. It's presence in "contrib/" is thus
rather misleading.
Promote the configuration into the new "tools/" directory.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
According to its readme, the "contrib/" directory's main intent is to
collect stuff that is not an official part of Git, either because it is
too specialized or because it is still considered experimental. The
reality tells a bit of a different story though: while it _does_ contain
such things, it also contains other things:
- Our credential helpers, which are being distributed by many
packagers nowadays and which can be considered "stable".
- A bunch of tooling that relates to our build and test
infrastructure.
Especially the second category is somewhat of a sore spot. You really
wouldn't expect build-related tooling to be considered an optional part
of Git. Quite the opposite.
Create a new top-level "tools/" directory to fix this discrepancy. This
directory will contain all kind of tools that are related to our build
infrastructure and that Git developers are likely to use day to day.
For now, this directory doesn't contain anything yet except for a
readme and a Meson skeleton. This will change in subsequent commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Aditya [Wed, 18 Mar 2026 20:07:35 +0000 (20:07 +0000)]
t2107: modernize path existence check
Replace '! test -f' with 'test_path_is_missing' to get better
debugging information by reporting loudly what expectation was
not met when the assertion fails.
Signed-off-by: Aditya <adityabnw07@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jialong Wang [Wed, 18 Mar 2026 19:09:42 +0000 (15:09 -0400)]
object-name: turn INTERPRET_BRANCH_* constants into enum values
Replace the INTERPRET_BRANCH_* preprocessor constants with enum
values and use that type where these flags are stored or passed
around.
These flags describe which kinds of branches may be considered during
branch-name interpretation, so represent them as an enum describing
branch kinds while keeping the existing bitmask semantics and
INTERPRET_BRANCH_* element names.
Signed-off-by: Jialong Wang <jerrywang183@yahoo.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Mathias Rav [Wed, 11 Mar 2026 06:44:06 +0000 (06:44 +0000)]
merge-file: fix BUG when --object-id is used in a worktree
The `--object-id` option was added in commit e1068f0ad4
(merge-file: add an option to process object IDs, 2023-11-01)
together with a call to setup_git_directory() to avoid crashing
when run outside a repository.
However, the call to setup_git_directory() is redundant when run inside
a repository, as merge-file runs with RUN_SETUP_GENTLY, so the
repository has already been set up. The redundant call is harmless
when linked worktrees are not used, but in a linked worktree,
the repo_set_gitdir() function ends up being called twice.
Calling repo_set_gitdir() used to be silently accepted, but commit 2816b748e5 (odb: handle changing a repository's commondir, 2025-11-19)
changed this to a BUG in repository.c with the error message:
"cannot reinitialize an already-initialized object directory".
Guard the redundant call to setup_git_directory() behind a repo pointer
check, to ensure that we continue to give the correct "not a git repo"
error whilst avoiding the BUG when running in a linked worktree.
Signed-off-by: Mathias Rav <m@git.strova.dk> Signed-off-by: Junio C Hamano <gitster@pobox.com>
René Scharfe [Tue, 17 Mar 2026 21:40:07 +0000 (22:40 +0100)]
use commit_stack instead of prio_queue in LIFO mode
A prio_queue with a NULL compare function acts as a stack -- the last
element in is the first one out (LIFO). Use an actual commit_stack
instead where possible, as it documents the behavior better, provides
type safety and saves some memory because prio_queue stores an
additional tie-breaking counter per element.
Signed-off-by: René Scharfe <l.s.r@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Tue, 17 Mar 2026 18:01:38 +0000 (11:01 -0700)]
apply: fix new-style empty context line triggering incomplete-line check
A new-style unified context diff represents an empty context line
with an empty line (instead of a line with a single SP on it). The
code to check whitespace errors in an incoming patch is designed to
omit the first byte of a line (typically SP, "-", or "+") and pass the
remainder of the line to the whitespace checker.
Usually we do not pass a context line to the whitespace error checker,
but when we are correcting errors, we do. This "remove the first
byte and send the remainder" strategy of checking a line ended up
sending a zero-length string to the whitespace checker when seeing a
new-style empty context line, which caused the whitespace checker to
say "ah, you do not even have a newline at the end!", leading to an
"incomplete line" in the middle of the patch!
Fix this by pretending that we got a traditional empty context line
when we drive the whitespace checker.
Jialong Wang [Tue, 17 Mar 2026 16:23:21 +0000 (12:23 -0400)]
apply: report input location in binary and garbage patch errors
Several binary parsing paths in apply.c still report only line
numbers. When more than one patch input is fed to a single
invocation, that does not tell the user which input the line belongs
to.
Report the patch input location for corrupt and unrecognized binary
patches, as well as the "patch with only garbage" case, and update
the related tests.
Signed-off-by: Jialong Wang <jerrywang183@yahoo.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jialong Wang [Tue, 17 Mar 2026 16:23:20 +0000 (12:23 -0400)]
apply: report input location in header parsing errors
Several header parsing errors in apply.c still report only line
numbers. When applying more than one input, that does not tell the
user which input the line belongs to.
Report the patch input location for these header parsing errors, and
update the related tests.
While touching parse_git_diff_header(), update the helper state to use
the current header line when reporting these errors.
Signed-off-by: Jialong Wang <jerrywang183@yahoo.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
add-patch: use repository instance from add_i_state instead of the_repository
Functions parse_diff(), edit_hunk_manually() and patch_update_file() use
the_repository even though a repository instance is already available via
struct add_i_state s which is defined in struct add_p_state *s.
Use 's->s.r' instead of the_repository to avoid relying on global state. All
callers pass a valid add_p_state and this does not change any behavior.
This aligns with the ongoing effort to reduce usage of the_repository global
state.
Signed-off-by: Shreyansh Paliwal <shreyanshpaliwalcmsmn@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add retry logic for HTTP 429 (Too Many Requests) responses to handle
server-side rate limiting gracefully. When Git's HTTP client receives
a 429 response, it can now automatically retry the request after an
appropriate delay, respecting the server's rate limits.
The implementation supports the RFC-compliant Retry-After header in
both delay-seconds (integer) and HTTP-date (RFC 2822) formats. If a
past date is provided, Git retries immediately without waiting.
Retry behavior is controlled by three new configuration options
(http.maxRetries, http.retryAfter, and http.maxRetryTime) which are
documented in git-config(1).
The retry logic implements a fail-fast approach: if any delay
(whether from server header or configuration) exceeds maxRetryTime,
Git fails immediately with a clear error message rather than capping
the delay. This provides better visibility into rate limiting issues.
The implementation includes extensive test coverage for basic retry
behavior, Retry-After header formats (integer and HTTP-date),
configuration combinations, maxRetryTime limits, invalid header
handling, environment variable overrides, and edge cases.
Signed-off-by: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
strbuf_attach: fix call sites to pass correct alloc
strbuf_attach(sb, buf, len, alloc) requires alloc > len (the buffer
must have at least len+1 bytes to hold the NUL). Several call sites
passed alloc == len, relying on strbuf_grow(sb, 0) inside strbuf_attach
to reallocate. Fix these in mailinfo, am, refs/files-backend,
fast-import, and trailer by passing len+1 when the buffer is a
NUL-terminated string (or from strbuf_detach).
Signed-off-by: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
strbuf: pass correct alloc to strbuf_attach() in strbuf_reencode()
reencode_string_len() allocates len+1 bytes (including the NUL) and
returns the string length in len. strbuf_reencode() was calling
strbuf_attach(sb, out, len, len), so alloc was one byte too small.
strbuf_attach() then calls strbuf_grow(sb, 0). With alloc < len+1,
ALLOC_GROW always reallocates, so we reallocated immediately after
attach even when the strbuf was not extended further. Pass len+1 as
the alloc argument so the existing buffer is reused and the
reallocation is avoided.
Signed-off-by: Vaidas Pilkauskas <vaidas.pilkauskas@shopify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jialong Wang [Tue, 17 Mar 2026 01:15:44 +0000 (21:15 -0400)]
t2203: avoid suppressing git status exit code
When git status is piped into grep, the exit status of the Git
command is hidden by the pipeline. Capture the status output in a
temporary file first, and then filter it as needed, so that any
failure from git status is still noticed by the test suite.
Signed-off-by: Jialong Wang <jerrywang183@yahoo.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Michael Montalbo [Tue, 17 Mar 2026 02:21:35 +0000 (02:21 +0000)]
doc: note that -L supports patch formatting and pickaxe options
Now that -L output flows through the standard diff pipeline,
document that patch formatting options like --word-diff,
--color-moved, --no-prefix, whitespace handling (-w, -b),
and pickaxe options (-S, -G) are supported.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Michael Montalbo [Tue, 17 Mar 2026 02:21:34 +0000 (02:21 +0000)]
t4211: add tests for -L with standard diff options
Now that -L output flows through the standard diff pipeline, verify
that previously-ignored diff options work: formatting (--word-diff,
--word-diff-regex, --no-prefix, --src/dst-prefix, --full-index,
--abbrev), whitespace handling (-w, -b), output indicators
(--output-indicator-new/old/context), direction reversal (-R),
--color-moved, and pickaxe options (-S, -G).
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Michael Montalbo [Tue, 17 Mar 2026 02:21:33 +0000 (02:21 +0000)]
line-log: route -L output through the standard diff pipeline
`git log -L` has always bypassed the standard diff pipeline.
`dump_diff_hacky()` in line-log.c hand-rolls its own diff headers and
hunk output, which means most diff formatting options are silently
ignored. A NEEDSWORK comment has acknowledged this since the feature
was introduced:
/*
* NEEDSWORK: manually building a diff here is not the Right
* Thing(tm). log -L should be built into the diff pipeline.
*/
Remove `dump_diff_hacky()` and its helpers and route -L output through
`builtin_diff()` / `fn_out_consume()`, the same path used by `git diff`
and `git log -p`. The mechanism is a pair of callback wrappers that sit
between `xdi_diff_outf()` and `fn_out_consume()`, filtering xdiff's
output to only the tracked line ranges. To ensure xdiff emits all lines
within each range as context, the context length is inflated to span the
largest range.
Wire up the `-L` implies `--patch` default in revision setup rather
than forcing it at output time, so `line_log_print()` is just
`diffcore_std()` + `diff_flush()` with no format save/restore.
Rename detection is a no-op since pairs are already resolved during
the history walk in `queue_diffs()`, but running `diffcore_std()`
means `-S`/`-G` (pickaxe), `--orderfile`, and `--diff-filter` now
work with `-L`, and `diff_resolve_rename_copy()` sets pair statuses
correctly without manual assignment.
Switch `diff_filepair_dup()` from `xmalloc` to `xcalloc` so that new
fields (including `line_ranges`) are zero-initialized by default.
As a result, diff formatting options that were previously silently
ignored (e.g. --word-diff, --no-prefix, -w, --color-moved) now work
with -L, and output gains `index` lines, `new file mode` headers, and
funcname context in `@@` headers. This is a user-visible output change:
tools that parse -L output may need to handle the additional header
lines.
The context-length inflation means xdiff may process more output than
needed for very wide line ranges, but benchmarks on files up to 7800
lines show no measurable regression.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Michael Montalbo [Tue, 17 Mar 2026 02:21:32 +0000 (02:21 +0000)]
line-log: fix crash when combined with pickaxe options
queue_diffs() passes the caller's diff_options, which may carry
user-specified pickaxe state, to diff_tree_oid() and diffcore_std()
when detecting renames for line-level history tracking. When pickaxe
options are present on the command line (-G and -S to filter by text
pattern, --find-object to filter by object identity), diffcore_std()
also runs diffcore_pickaxe(), which may discard diff pairs that are
relevant for rename detection. Losing those pairs breaks rename
following.
Before a2bb801f6a (line-log: avoid unnecessary full tree diffs,
2019-08-21), this silently truncated history at rename boundaries.
That commit moved filter_diffs_for_paths() inside the rename-
detection block, so it only runs when diff_might_be_rename() returns
true. When pickaxe discards a rename pair, the rename goes
undetected, and a deletion pair at a subsequent commit passes
through uncleaned, reaching process_diff_filepair() with an invalid
filespec and triggering an assertion failure.
Fix this by building a private diff_options for the rename-detection
path inside queue_diffs(), following the same pattern used by blame's
find_rename(). This isolates the rename machinery from unrelated
user-specified options.
Reported-by: Matthew Hughes <matthewhughes934@gmail.com> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Eric Ju [Tue, 17 Mar 2026 02:36:24 +0000 (22:36 -0400)]
refs: add 'preparing' phase to the reference-transaction hook
The "reference-transaction" hook is invoked multiple times during a ref
transaction. Each invocation corresponds to a different phase:
- The "prepared" phase indicates that references have been locked.
- The "committed" phase indicates that all updates have been written to disk.
- The "aborted" phase indicates that the transaction has been aborted and that
all changes have been rolled back.
This hook can be used to learn about the updates that Git wants to perform.
For example, forges use it to coordinate reference updates across multiple
nodes.
However, the phases are insufficient for some specific use cases. The earliest
observable phase in the "reference-transaction" hook is "prepared", at which
point Git has already taken exclusive locks on every affected reference. This
makes it suitable for last-chance validation, but not for serialization. So by
the time a hook sees the "prepared" phase, it has no way to defer locking, and
thus it cannot rearrange multiple concurrent ref transactions relative to one
another.
Introduce a new "preparing" phase that runs before the "prepared" phase, that
is before Git acquires any reference lock on disk. This gives callers a
well-defined window to perform validation, enable higher-level ordering of
concurrent transactions, or reject the transaction entirely, all without
interfering with the locking state.
This change is strictly speaking not backwards compatible. Existing hook
scripts that do not know how to handle unknown phases may treat 'preparing'
as an error and return non-zero. But the hook is considered to expose
internal implementation details of how Git works, and as such we have
been a bit more lenient with changing its exact semantics, like for example
in a8ae923f85 (refs: support symrefs in 'reference-transaction' hook, 2024-05-07).
An alternative would be to introduce a "reference-transaction-v2" hook that
knows about the new phase. This feels like a rather heavy-weight option though,
and was thus discarded.
Helped-by: Patrick Steinhardt <ps@pks.im> Helped-by: Justin Tobler <jltobler@gmail.com> Helped-by: Karthik Nayak <karthik.188@gmail.com> Signed-off-by: Eric Ju <eric.peijian@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
doc: interpret-trailers: normalize and fill out options
Some negated options are missing according to
`git interpret-trailers -h`.
Also normalize to the “stuck form” (see gitcli(7)) like what was done
in 806337c7 (doc: notes: use stuck form throughout, 2025-05-27).[1]
Also normalize the order of the regular and negated options according to
the current convention.[2]
Also note that `--no-trailer` will reset the list.
† 1: See also https://lore.kernel.org/git/6f7d027e-088a-4d66-92af-b8d1c32d730c@app.fastmail.com/
† 2: https://lore.kernel.org/git/xmqqcyct1mtq.fsf@gitster.g/
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
doc: interpret-trailers: convert to synopsis style
See e.g. 0ae23ab5 (doc: convert git worktree to synopsis style,
2025-10-05) for the markup rules for this style.
There aren’t many subtleties to the transformation of this doc since it
doesn’t use any advanced constructs. The only thing is that "`:`{nbsp}" is
used instead of `': '` to refer to effective inline-verbatim with
a space (␠).[1] I also use (_) for emphasis although (') gives the
same result.
Also prefer linking to Git commands instead of saying e.g. `git
format-patch`. But for this command we can type out git-interpret-
trailers(1) to avoid a self-reference.
Also replace camel case `<keyAlias>` with kebab case `<key-alias>`.
And while doing that make sure to replace `trailer.*` with
`trailer.<key-alias>`.
† 1: Similar to "`tag:`{nbsp}" in `Documentation/pretty-formats.adoc`
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jeff King [Sat, 14 Mar 2026 16:08:14 +0000 (12:08 -0400)]
transport: plug leaks in transport_color_config()
We retrieve config values with repo_config_get_string(), which will
allocate a new copy of the string for us. But we don't hold on to those
strings, since they are just fed to git_config_colorbool() and
color_parse(). But nor do we free them, which means they leak.
We can fix this by using the "_tmp" form of repo_config_get_string(),
which just hands us a pointer directly to the internal storage. This is
OK for our purposes, since we don't need it to last for longer than our
parsing calls.
Two interesting side notes here:
1. Many types already have a repo_config_get_X() variant that handles
this for us (e.g., repo_config_get_bool()). But neither colorbools
nor colors themselves have such helpers. We might think about
adding them, but converting all callers is a larger task, and out
of scope for this fix.
2. As far as I can tell, this leak has been there since 960786e761
(push: colorize errors, 2018-04-21), but wasn't detected by LSan in
our test suite. It started triggering when we applied dd3693eb08
(transport-helper, connect: use clean_on_exit to reap children on
abnormal exit, 2026-03-12) which is mostly unrelated.
Even weirder, it seems to trigger only with clang (and not gcc),
and only with GIT_TEST_DEFAULT_REF_FORMAT=reftable. So I think this
is another odd case where the pointers happened to be hanging
around in stack memory, but changing the pattern of function calls
in nearby code was enough for them to be incidentally overwritten.
Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
PRASHANT S BISHT [Mon, 16 Mar 2026 17:24:57 +0000 (22:54 +0530)]
t4200: convert test -[df] checks to test_path_* helpers
Replace old-style path existence checks in t4200-rerere.sh with
the appropriate test_path_* helper functions. These helpers provide
clearer diagnostic messages on failure than the raw shell test
builtin.
Signed-off-by: Prashant S Bisht <prashantjee2025@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Mirko Faina [Mon, 16 Mar 2026 00:51:16 +0000 (01:51 +0100)]
apply.c: fix -p argument parsing
"git apply" has an option -p that takes an integer as its argument.
Unfortunately the function apply_option_parse_p() in charge of parsing
this argument uses atoi() to convert from string to integer, which
allows a non-digit after the number (e.g. "1q") to be silently ignored.
As a consequence, an argument that does not begin with a digit silently
becomes a zero. Despite this command working fine when a non-positive
argument is passed, it might be useful for the end user to know that
their input contains non-digits that might've been unintended.
Replace atoi() with strtol_i() to catch malformed inputs.
Signed-off-by: Mirko Faina <mroik@delayed.space> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Mirko Faina [Mon, 16 Mar 2026 01:15:42 +0000 (02:15 +0100)]
t0008: improve test cleanup to fix failing test
The "large exclude file ignored in tree" test fails. This is due to an
additional warning message that is generated in the test. "warning:
unable to access 'subdir/.gitignore': Too many levels of symbolic
links", the extra warning that is not supposed to be there, happens
because of some leftover files left by previous tests.
To fix this we improve cleanup on "symlinks not respected in-tree", and
because the tests in t0008 in general have poor cleanup, at the start of
"large exclude file ignored in tree" we search for any leftover
.gitignore and remove them before starting the test.
Improve post-test cleanup and add pre-test cleanup to make sure that we
have a workable environment for the test.
Signed-off-by: Mirko Faina <mroik@delayed.space> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Junio C Hamano [Mon, 16 Mar 2026 19:42:54 +0000 (12:42 -0700)]
Merge branch 'mf/format-patch-cover-letter-format' into mf/format-patch-commit-list-format
* mf/format-patch-cover-letter-format:
docs: add usage for the cover-letter fmt feature
format-patch: add commitListFormat config
format-patch: add ability to use alt cover format
format-patch: move cover letter summary generation
pretty.c: add %(count) and %(total) placeholders
Junio C Hamano [Mon, 16 Mar 2026 17:48:15 +0000 (10:48 -0700)]
Merge branch 'jk/unleak-mmap'
Plug a few leaks where mmap'ed memory regions are not unmapped.
* jk/unleak-mmap:
meson: turn on NO_MMAP when building with LSan
Makefile: turn on NO_MMAP when building with LSan
object-file: fix mmap() leak in odb_source_loose_read_object_stream()
pack-revindex: avoid double-loading .rev files
check_connected(): fix leak of pack-index mmap
check_connected(): delay opening new_pack
Junio C Hamano [Mon, 16 Mar 2026 17:48:14 +0000 (10:48 -0700)]
Merge branch 'ty/setup-error-tightening'
While discovering a ".git" directory, the code treats any stat()
failure as a sign that a filesystem entity .git does not exist
there, and ignores ".git" that is not a "gitdir" file or a
directory. The code has been tightened to notice and report
filesystem corruption better.
* ty/setup-error-tightening:
setup: improve error diagnosis for invalid .git files
Junio C Hamano [Mon, 16 Mar 2026 17:48:14 +0000 (10:48 -0700)]
Merge branch 'jt/repo-structure-extrema'
"git repo structure" command learns to report maximum values on
various aspects of objects it inspects.
* jt/repo-structure-extrema:
builtin/repo: find tree with most entries
builtin/repo: find commit with most parents
builtin/repo: add OID annotations to table output
builtin/repo: collect largest inflated objects
builtin/repo: add helper for printing keyvalue output
builtin/repo: update stats for each object
Junio C Hamano [Mon, 16 Mar 2026 17:48:13 +0000 (10:48 -0700)]
Merge branch 'sp/wt-status-wo-the-repository'
Reduce dependence on the global the_hash_algo and the_repository
variables of wt-status code path.
* sp/wt-status-wo-the-repository:
wt-status: use hash_algo from local repository instead of global the_hash_algo
wt-status: replace uses of the_repository with local repository instances
wt-status: pass struct repository through function parameters
clar: update to fix compilation on platforms without PATH_MAX
Update clar to e4172e3 (Merge pull request #134 from
clar-test/ethomson/const, 2026-01-10). Besides some changes to
"generate.py" which don't have any impact on us, this commit also fixes
compilation on platforms that don't have PATH_MAX, like for example
GNU/Hurd.
Reported-by: Samuel Thibault <samuel.thibault@ens-lyon.org> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Deveshi Dwivedi [Sun, 15 Mar 2026 09:44:44 +0000 (09:44 +0000)]
stash: do not pass strbuf by value
save_untracked_files() takes its 'files' parameter as struct strbuf
by value. Passing a strbuf by value copies the struct but shares
the underlying buffer between caller and callee, risking a dangling
pointer and double-free if the callee reallocates.
The function needs both the buffer and its length for
pipe_command(), so a plain const char * is not sufficient here.
Switch the parameter to struct strbuf * and update the caller to
pass a pointer.
Signed-off-by: Deveshi Dwivedi <deveshigurgaon@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Deveshi Dwivedi [Sun, 15 Mar 2026 09:44:43 +0000 (09:44 +0000)]
coccinelle: detect struct strbuf passed by value
Passing a struct strbuf by value to a function copies the struct
but shares the underlying character array between caller and callee.
If the callee causes a reallocation, the caller's copy becomes a
dangling pointer, leading to a double-free when strbuf_release() is
called. There is no coccinelle rule to catch this pattern.
Jeff King suggested adding one during review of the
write_worktree_linking_files() fix [1], and noted that a reporting
rule using coccinelle's Python scripting extensions could emit a
descriptive warning, but we do not currently require Python support
in coccinelle.
Add a transformation rule that rewrites a by-value strbuf parameter
to a pointer. The detection is identical to what a Python-based
reporting rule would catch; only the presentation differs. The
resulting diff will not produce compilable code on its own (callers
and the function body still need updating), but the spatch output
alerts the developer that the signature needs attention. This is
consistent with the other rules in strbuf.cocci, which also rewrite
to the preferred form.
The pack-refs tests previously used raw 'test -f' and 'test -e' checks
with negation. Update them to use Git's standard helper function
test_path_is_missing for consistency and clearer failure reporting.
As suggested in review, replaced the negated 'test_path_exists' with
test_path_is_missing to better reflect the expected absence of paths.
Signed-off-by: Ritesh Singh Jadoun <riteshjd75@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
builtin/pack-objects: reduce lock contention when writing packfile data
When running `git pack-objects --stdout` we feed the data through
`hashfd_ext()` with a progress meter and a smaller-than-usual buffer
length of 8kB so that we can track throughput more granularly. But as
packfiles tend to be on the larger side, this small buffer size may
cause a ton of write(3p) syscalls.
Originally, the buffer we used in `hashfd()` was 8kB for all use cases.
This was changed though in 2ca245f8be (csum-file.h: increase hashfile
buffer size, 2021-05-18) because we noticed that the number of writes
can have an impact on performance. So the buffer size was increased to
128kB, which improved performance a bit for some use cases.
But the commit didn't touch the buffer size for `hashd_throughput()`.
The reasoning here was that callers expect the progress indicator to
update frequently, and a larger buffer size would of course reduce the
update frequency especially on slow networks.
While that is of course true, there was (and still is, even though it's
now a call to `hashfd_ext()`) only a single caller of this function in
git-pack-objects(1). This command is responsible for writing packfiles,
and those packfiles are often on the bigger side. So arguably:
- The user won't care about increments of 8kB when packfiles tend to
be megabytes or even gigabytes in size.
- Reducing the number of syscalls would be even more valuable here
than it would be for multi-pack indices, which was the benchmark
done in the mentioned commit, as MIDXs are typically significantly
smaller than packfiles.
- Nowadays, many internet connections should be able to transfer data
at a rate significantly higher than 8kB per second.
Update the buffer to instead have a size of `LARGE_PACKET_DATA_MAX - 1`,
which translates to ~64kB. This limit was chosen because `git
pack-objects --stdout` is most often used when sending packfiles via
git-upload-pack(1), where packfile data is chunked into pktlines when
using the sideband. Furthermore, most internet connections should have a
bandwidth signifcantly higher than 64kB/s, so we'd still be able to
observe progress updates at a rate of at least once per second.
This change significantly reduces the number of write(3p) syscalls from
355,000 to 44,000 when packing the Linux repository. While this results
in a small performance improvement on an otherwise-unused system, this
improvement is mostly negligible. More importantly though, it will
reduce lock contention in the kernel on an extremely busy system where
we have many processes writing data at once.
Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `hashfd_throughput()` function is used by a single callsite in
git-pack-objects(1). In contrast to `hashfd()`, this function uses a
progress meter to measure throughput and a smaller buffer length so that
the progress meter can provide more granular metrics.
We're going to change that caller in the next commit to be a bit more
specific to packing objects. As such, `hashfd_throughput()` will be a
somewhat unfitting mechanism for any potential new callers.
Drop the function and replace it with a call to `hashfd_ext()`.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Every pktline that we send out via `send_sideband()` currently requires
two syscalls: one to write the pktline's length, and one to send its
data. This typically isn't all that much of a problem, but under extreme
load the syscalls may cause contention in the kernel.
Refactor the code to instead use the newly introduced writev(3p) infra
so that we can send out the data with a single syscall. This reduces the
number of syscalls from around 133,000 calls to write(3p) to around
67,000 calls to writev(3p).
Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In the preceding commit we have added a compatibility wrapper for the
writev(3p) syscall. Introduce some generic wrappers for this function
that we nowadays take for granted in the Git codebase.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In a subsequent commit we're going to add the first caller to
writev(3p). Introduce a compatibility wrapper for this syscall that we
can use on systems that don't have this syscall.
The syscall exists on modern Unixes like Linux and macOS, and seemingly
even for NonStop according to [1]. It doesn't seem to exist on Windows
though.
upload-pack: reduce lock contention when writing packfile data
In our production systems we have recently observed write contention in
git-upload-pack(1). The system in question was consistently streaming
packfiles at a rate of dozens of gigabits per second, but curiously the
system was neither bottlenecked on CPU, memory or IOPS.
We eventually discovered that Git was spending 80% of its time in
`pipe_write()`, out of which almost all of the time was spent in the
`ep_poll_callback` function in the kernel. Quoting the reporter:
This infrastructure is part of an event notification queue designed to
allow for multiple producers to emit events, but that concurrency
safety is guarded by 3 layers of locking. The layer we're hitting
contention in uses a simple reader/writer lock mode (a.k.a. shared
versus exclusive mode), where producers need shared-mode (read mode),
and various other actions use exclusive (write) mode.
The system in question generates workloads where we have hundreds of
git-upload-pack(1) processes active at the same point in time. These
processes end up contending around those locks, and the consequence is
that the Git processes stall.
Now git-upload-pack(1) already has the infrastructure in place to buffer
some of the data it reads from git-pack-objects(1) before actually
sending it out. We only use this infrastructure in very limited ways
though, so we generally end up matching one read(3p) call with one
write(3p) call. Even worse, when the sideband is enabled we end up
matching one read with _two_ writes: one for the pkt-line length, and
one for the packfile data.
Extend our use of the buffering infrastructure so that we soak up bytes
until the buffer is filled up at least 2/3rds of its capacity. The
change is relatively simple to implement as we already know to flush the
buffer in `create_pack_file()` after git-pack-objects(1) has finished.
This significantly reduces the number of write(3p) syscalls we need to
do. Before this change, cloning the Linux repository resulted in around
400,000 write(3p) syscalls. With the buffering in place we only do
around 130,000 syscalls.
Now we could of course go even further and make sure that we always fill
up the whole buffer. But this might cause an increase in read(3p)
syscalls, and some tests show that this only reduces the number of
write(3p) syscalls from 130,000 to 100,000. So overall this doesn't seem
worth it.
Note that the issue could also be fixed by adapting the write buffer
that we use in the downstream git-pack-objects(1) command, and such a
change would have roughly the same result. But the command that
generates the packfile data may not always be git-pack-objects(1) as it
can be changed via "uploadpack.packObjectsHook", so such a fix would
only help in _some_ cases. Regardless of that, we'll also adapt the
write buffer size of git-pack-objects(1) in a subsequent commit.
Helped-by: Matt Smiley <msmiley@gitlab.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
upload-pack: prefer flushing data over sending keepalive
When using the sideband in git-upload-pack(1) we know to send out
keepalive packets in case generating the pack takes too long. These
keepalives take the form of a simple empty pktline.
In the preceding commit we have adapted git-upload-pack(1) to buffer
data more aggressively before sending it to the client. This creates an
obvious optimization opportunity: when we hit the keepalive timeout
while we still hold on to some buffered data, then it makes more sense
to flush out the data instead of sending the empty keepalive packet.
This is overall not going to be a significant win. Most keepalives will
come before the pack data starts, and once pack-objects starts producing
data, it tends to do so pretty consistently. And of course we can't send
data before we see the PACK header, because the whole point is to buffer
the early bit waiting for packfile URIs. But the optimization is easy
enough to realize.
Do so and flush out data instead of sending an empty pktline.
Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function `create_pack_file()` is responsible for sending the
packfile data to the client of git-upload-pack(1). As generating the
bytes may take significant computing resources we also have a mechanism
in place that optionally sends keepalive pktlines in case we haven't
sent out any data.
The keepalive logic is purely based poll(3p): we pass a timeout to that
syscall, and if the call times out we send out the keepalive pktline.
While reasonable, this logic isn't entirely sufficient: even if the call
to poll(3p) ends because we have received data on any of the file
descriptors we may not necessarily send data to the client.
The most important edge case here happens in `relay_pack_data()`. When
we haven't seen the initial "PACK" signature from git-pack-objects(1)
yet we buffer incoming data. So in the worst case, if each of the bytes
of that signature arrive shortly before the configured keepalive
timeout, then we may not send out any data for a time period that is
(almost) four times as long as the configured timeout.
This edge case is rather unlikely to matter in practice. But in a
subsequent commit we're going to adapt our buffering mechanism to become
more aggressive, which makes it more likely that we don't send any data
for an extended amount of time.
Adapt the logic so that instead of using a fixed timeout on every call
to poll(3p), we instead figure out how much time has passed since the
last-sent data.
Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
projects / thirdparty / git.git / log
