E - #1
Open
rfrown177 wants to merge 454 commits into
Open
Conversation
When initializing a repository's object database we have to respect the GIT_OBJECT_DIRECTORY and GIT_ALTERNATE_OBJECT_DIRECTORIES environment variables, which can be set by the user to override the default location of where we write objects to and read objects from. This is handled in `apply_repository_format()`, which is fine. But in a subsequent commit we'll have to defer constructing the object database to a later point in some cases, and that will require a second site where we call `odb_new()`. And of course, that second site would have to handle those environment variables, as well. It would be somewhat awkward to duplicate the logic though. But there's a better alternative: instead of handling this logic in "setup.c", we can easily handle environment variables in `odb_new()` itself. This ensures that object database creation is neatly self-contained, and we don't have to duplicate any of the logic. Another benefit is that in a future patch series we plan to move handling of alternates into the backends themselves [1], and that will require us to also handle those environment variables in the "files" backend itself. So moving the logic into the ODB level already gets us one step closer to that goal. Refactor the logic accordingly. [1]: https://lore.kernel.org/git/amLgMqkqxR8mKIbT@pks.im/ Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In a subsequent commit we'll make the creation of the on-disk data structures of an object database pluggable. This will lead to an in-between state where we have already configured the repository's object database, but it's not usable yet until we eventually call `create_object_directory()`. Lift the call to `odb_new()` out of `apply_repository_format()` so that callers have more wiggle room with when exactly they call it, and adapt them accordingly. The only exception is `init_db()`, where we now defer creating the object database until we call `create_object_database()`. With this change, initializing and creating the object database on disk is now neatly encapsulated in a single function, which will make it easier for a subsequent commit to move creation of the on-disk data structures into the `struct odb_source` backends. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Introduce a new function that maps an object source's type to a human-readable name. Use the function to provide better human-readable error messages for the downcasting functions. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When creating a new "files" object database source we have to create a couple of directories. These directories are of course specific to this particular backend, and a different backend may require a setup that is completely different. Make the creation of on-disk structures pluggable to accommodate for this. Note that there is one exception though: the "objects" directory must exist in a repository regardless of which backend is in use. If it doesn't exist then the repository is not treated as a Git repository at all. Consequently, we create this directory regardless of the backend. 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. [1]: http://nonstoptools.com/manuals/OSS-SystemCalls.pdf [2]: https://www.gnu.org/software/gnulib/manual/html_node/writev.html Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> 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>
Some systems like NonStop set a comparatively small `MAX_IO_SIZE`, which
limits the maximum number of bytes we're allowed to write in a single
call. We already handle this limit properly in `xwrite()`, but we have
recently introduced wrappers for writev(3p) where we don't. This will
cause the syscall to return EINVAL in case somebody passes an iovec
entry to writev(3p) that is larger than `MAX_IO_SIZE`.
Introduce a new function `xwritev()` that is similar to `xwrite()` in
that it handles such platform-specific nuances:
- We only pass the leading iovec entries to writev(3p) that fit into
`MAX_IO_SIZE`, pretending that the underlying syscall performed a
short write. This mirrors how `xwrite()` chomps overly large
requests before handing them to write(3p). As a consequence, callers
will never see writev(3p)'s EINVAL error for requests whose summed
length would overflow an ssize_t, but observe a short write instead.
- If already the first iovec entry exceeds the limit we instead punt
to `xwrite()`, which knows to handle this case for us.
- We restart the underlying syscall on EINTR and EAGAIN, just like
`xwrite()` does for write(3p).
Adapt `writev_in_full()` to use this new wrapper. With the retry logic
now living in `xwritev()`, the calling loop becomes the exact mirror
image of `write_in_full()`, which also retains the responsibility of
translating a zero-length write into ENOSPC.
Reported-by: Randall Becker <randall.becker@nexbridge.ca>
Helped-by: Jeff King <peff@peff.net>
Helped-by: Junio C Hamano <gitster@pobox.com>
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). This change leads to a performance improvement for git-upload-pack(1), but we have to cheat a bit to really make it measurable. Usually, the time is strongly dominated by generating the packfile itself. But if we precompute the pack and serve it via the pack-objects hook then we can essentially eliminate that overhead. The following setup is executed in the Git repository: $ cat >request <<-EOF 0048want 5ce91c0 side-band no-progress 00000009done EOF $ echo 5ce91c0 | git pack-objects --revs --stdout >pack $ cat >hook <<-EOF #!/bin/sh cat >/dev/null cat "$(pwd)"/pack EOF $ chmod u+x hook $ git -c uploadpack.packObjectsHook="$(pwd)"/hook upload-pack . <request Benchmarking the last command leads to the following results: Benchmark 1: HEAD~ Time (mean ± σ): 192.9 ms ± 0.6 ms [User: 106.5 ms, System: 95.3 ms] Range (min … max): 191.7 ms … 194.1 ms 50 runs Benchmark 2: HEAD Time (mean ± σ): 141.1 ms ± 0.7 ms [User: 63.2 ms, System: 86.6 ms] Range (min … max): 139.8 ms … 142.7 ms 50 runs Summary HEAD ran 1.37 ± 0.01 times faster than HEAD~ This might not be impressive in absolute numbers when you also take into account the time it takes to generate the packfile itself. But GitLab (and supposedly other forges) have caching mechanisms in place that work exactly like the above setup, where repeated incoming requests can be served from the same cached packfile. And in those cases, the impact is sizeable. More importantly though, as hinted at above, GitLab has observed in the past that with enough cache hits we eventually start to saturate a semaphore in the Linux kernel itself in the pipe write path. This bottleneck is being moved a bit by having to do less syscalls. Suggested-by: Jeff King <peff@peff.net> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When answering a `cat-blob` command, `cat_blob()` issues three separate
calls to write(3p) on the cat-blob fd: one for the header line, one for
the full blob payload, and one for the trailing newline. Frontends like
git-filter-repo issue these commands in bulk, once per rewritten blob,
so the syscall overhead adds up.
Use `writev_in_full()` to send all three parts with a single syscall.
This can be benchmarked with the following setup:
$ git cat-file --unordered --filter=object:type=blob
--batch-check='cat-blob %(objectname)' --batch-all-objects >request
$ git fast-import --cat-blob-fd=3 <request
Executing this with 100,000 objects in linux.git:
Benchmark 1: HEAD~
Time (mean ± σ): 1.320 s ± 0.003 s [User: 1.154 s, System: 0.161 s]
Range (min … max): 1.314 s … 1.324 s 10 runs
Benchmark 2: HEAD
Time (mean ± σ): 1.270 s ± 0.022 s [User: 1.133 s, System: 0.132 s]
Range (min … max): 1.209 s … 1.282 s 10 runs
Summary
HEAD ran
1.04 ± 0.02 times faster than HEAD~
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The code path that deals with relative paths in the 'diff-lib' has been cleaned up. * jk/diff-relative-cached-unmerged-more: diff-lib: skip paths outside prefix in oneway_diff() diff-lib: drop stale comment about advancing o->pos
The get_commit_action() function has been refactored to be a pure predicate by moving the side-effecting line-level log range folding to simplify_commit(). This ensures that evaluating a commit's action before the walk reaches it does not prematurely mutate its tracked line ranges, making it safer for potential lookahead evaluations. * mm/revision-pure-get-commit-action: revision: make get_commit_action() a pure predicate
'git cat-file --batch-command' that asked for 'contents' without 'type' segfaults, which has been corrected. * jk/cat-file-batch-wo-type-fix: cat-file: handle content request for --batch-command without type
A memory leak in 'git merge' when run without arguments (which triggers the default-to-upstream path) has been fixed. A test has been added to cover this case. * tc/merge-default-to-upstream-leakfix: merge: fix leak with merge.defaultToUpstream
A boundary case check in reachability bitmap traversal has been corrected to properly handle the object at position zero, which was previously skipped, leading to redundant bitmap loading. * dl/pack-bitmap-position-zero: pack-bitmap: handle objects at bitmap position zero
A crash in the 'sparse-index' collapse code when encountering an invalidated cache-tree node (due to an intent-to-add path) has been fixed by avoiding collapsing such subtrees. * ds/sparse-index-ita-crash: sparse-index: avoid crash on intent-to-add entry outside the cone
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The 'basics of object-info' test runs 'wc -c | xargs' twice to get the size of two.t. The pipe to xargs is only there to strip the blanks that some platforms pad the output of wc with. Use the test_file_size() helper, which outputs the size directly, and store the result in a variable. Because 'git rev-parse two:two.t' is also run multiple times, store its output in a variable as well. Storing them in variables outside the HERE-document has the added benefit of preserving their exit statuses. Mentored-by: Karthik Nayak <karthik.188@gmail.com> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The loop reading the object-info response stops as soon as the reader returns something other than PACKET_READ_NORMAL, or once it has read as many lines as we requested. Neither end is checked. A server that answers with fewer objects leaves the end of the result arrays empty, and the caller trusts that every requested object was filled in. A server that answers with more leaves the extra packets unread. On stateless transports check_stateless_delimiter() notices, but on the others it passes unnoticed. Check both limits by extracting the packet_reader_read() from the loop condition, so the loop no longer consumes the last packet (flush). If while looping the read is different from a PACKET_READ_NORMAL, die() meaning there are fewer objects than expected. After iterating, we only expect a flush, so if the last packet is not a flush, die(). Helped-by: Junio C Hamano <gitster@pobox.com> Mentored-by: Karthik Nayak <karthik.188@gmail.com> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
struct object_info_args groups three pointers that already live in the transport and are given to fetch_object_info(). Grouping them into a struct reduces the number of parameters, but it suggests that the three belong together, when they are unrelated and end up being accessed as args->* independently. Drop the struct and pass those parameters directly to fetch_object_info() and send_object_info_request(). This should have no change in behavior. Helped-by: Jeff King <peff@peff.net> Helped-by: Junio C Hamano <gitster@pobox.com> Mentored-by: Karthik Nayak <karthik.188@gmail.com> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
fetch_object_info() collects information about N objects, but it stores the results in an array of object_info. That struct holds the extended parameters of read_object_info() (The optional outputs the caller wants filled). Its pointers tell that function where to write the answers for a single object. object_info is not meant to be the final storage, and since fetch_object_info() does not call read_object_info(), there is no reason to use it. Using it means allocating one scalar per object per attribute just to have those pointers somewhere to point at. Add struct fetch_object_info_results. The caller sets the wants_* flags to say what it is interested in, and fetch_object_info() allocates one array per attribute. A set wants_* flag means "asked for", while a non-NULL array means "available". The caller releases the arrays with free_fetch_object_info_results(). The object_info_options string list is no longer needed. Filtering against the server's advertisement now sets local ask_* flags, and send_object_info_request() turns those into the v2 protocol option strings. remote_atom_map[] existed only to map those strings back into atom names, so drop it and build remote_allowed_atoms from the result arrays. Currently for wants_* and ask_* there is only the 'size' variant but a subsequent commit will add '*_type'. free_object_info_contents() loses its only caller and is dropped. Dropping the allow-list check makes the final else reachable from the wire, so die() instead of BUG(): an unknown attribute is the server's error, not ours. Helped-by: Jeff King <peff@peff.net> Helped-by: Junio C Hamano <gitster@pobox.com> Mentored-by: Karthik Nayak <karthik.188@gmail.com> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Every failure in fetch_object_info() dies except one: a short read while parsing the attribute lines returns -1. That -1 is then passed through fetch_object_info_via_pack() and get_remote_info() up to cat-file, only to die() with a generic message. Die in fetch_object_info() instead, consistently with the rest of its error paths, and make fetch_object_info() void. Mentored-by: Karthik Nayak <karthik.188@gmail.com> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
A remote object-info request needs three things: the transport for contacting the remote, the list of oids to request, and a place to store the output. Rather than take these as function parameters, we take only the transport object, and expect the caller to have placed the other two into special fields in the transport struct. But this doesn't make much sense. The set of oids and results are really only valid for one request. There is no reason the transport would need to hang on to them outside of the single function call. Even though we save a few lines passing the parameters around through the various vtable functions, the result is harder to understand (for example, who is responsible for cleaning up results, and when should it happen?). It also opens up the possibility of a subtle bug. A caller is likely to point those fields to stack variables which could go out of scope, and the transport struct would be left holding invalid pointers. This is mostly harmless now, as we disconnect the transport immediately after the sole caller of transport_fetch_object_info(). But conceptually we could keep the transport open and make multiple fetch calls (and reuse the same connection to the helper, to a remote HTTP server, and so on). So let's pull these out of the struct and pass them as function parameters. It's a little more verbose, but I think more clearly illustrates the intent. I've also tweaked a few function signatures to mark the input oid array as const, since it is purely an input to the function. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Teach the server-side object-info handler to accept type as a requested field. When the client includes type in its object-info request, the server returns the requested object type. While touching send_info(), wrap an over-long line and fix the bit field style of requested_info.size. Mentored-by: Karthik Nayak <karthik.188@gmail.com> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The server can handle type requests but does not advertise the capability yet. Prepare the client to know how to parse the server response once the server advertises the type capability. Mentored-by: Karthik Nayak <karthik.188@gmail.com> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The server and the client can handle type requests but the client won't ask for it until the server advertises it. Add type to the advertised capabilities so the client knows that it can request it. Mentored-by: Karthik Nayak <karthik.188@gmail.com> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
%(objecttype) is supported both by the client and by the server. Change the temporary default format to the unified version that the other commands use. Update documentation to remove %(objecttype) from the caveats of remote-object-info and show %(objecttype) support. Now that type is supported and the default format unified, update the tests to expect the new default format. Mentored-by: Karthik Nayak <karthik.188@gmail.com> Mentored-by: Chandra Pratap <chandrapratap3519@gmail.com> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
This command handles the trailer metadata format. But the command isn’t introduced as such; it is instead introduced by stating that these trailer lines look similar to RFC 822 email headers. This is overwrought; most people do not deal directly with email headers, and certainly not email RFCs. Trailers are just key–value pairs that, like email headers, use colon as the separator. The format in its simplest form is easy to describe directly without comparing it to anything else; we will do that in the upcoming commit “explain the format after the intro”. For now, let’s: • remove the first mention of email headers; • keep the second, innocuous comparison with email line folding in the middle; and • remove the now-unneeded disclaimer that trailers do not share many of the features of RFC 822 email headers—there is no invitation to speculate that trailers would follow any other email format rules since we do not compare them directly any more. *** Talking about trailers as an RFC 822/2822-like format seems to go back to the `--fixes`/`Fixes:` trailer topic,[1] the thread that precipitated this command and in turn the first trailer support in git(1) beyond adding s-o-b lines. † 1: https://lore.kernel.org/all/20131027071407.GA11683@leaf/ Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We removed the initial comparison to email headers in the previous commit. Now the introduction paragraph just says “trailer lines”, and the only hint that this is metadata/structured information is the “otherwise free-form” phrase. Let’s replace “lines” with “metadata” since that is their purpose. This also makes the introduction more consistent with how I chose to define trailers in the glossary:[1] “Key-value metadata”. (We will introduce “key–value” in the upcoming commit “explain the format after the intro”.) † 1: 68e3c69 (Documentation/glossary: describe "trailer", 2024-11-17) Let’s not emphasize “trailer” here since we are going to define the term in the upcoming commit “explain the format after the intro”. Let’s call it “trailer metadata” rather than “trailers metadata”. At first it seemed better to use the latter: 1. We’re introducing the jargon, and the format is often discussed as plural “trailers”, with its constituent parts being singular “trailer” 2. What this replaces uses “trailer”, but it rescues the plural mood with “lines” 3. This is very soon going to go into the constituent parts, including each trailer, so we’re contrasting the concept name (trailers) with its parts But: 1. The former reads better (most important) 2. “Trailer *metadata*” suggests plurality, similar to “trailer *lines*” Helped-by: Matt Hunter <m@lfurio.us> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We now since the previous commit introduce the format as “trailer metadata”. We can replace “structured information” with “metadata” in the “Name” section to be consistent. While “structured information” does emphasize that the data is not loosely structured, we also say that this command adds to or parses this format. I don’t think that we need to emphasize that it is structured since clearly there is some structure there. Both “metadata” and “structured information” can convey the same information. But “metadata” is shorter and easier to deploy since it’s just one word. Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
This command doesn’t interface with commits directly. You can interpret or modify any kind of text, even though commit messages are the most relevant. The git(1) suite also isn’t restricted to only direct commit support since git-tag(1) learned `--trailer` in 066cef7 (builtin/tag: add --trailer option, 2024-05-05) Now, we already introduce the command in the “Name” section as dealing with commit messages as well. That is fine since that intro line needs to remain pretty short. Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The threshold for geometric repacking to trigger based on loose object count has been adjusted to match that of 'git gc --auto', preventing over-aggressive repacking during concurrent writes. * ps/odb-geometric-repack-loose-threshold: odb/files: be less aggressive with geometric repacking
The 'git receive-pack' command has been updated to use a new ODB transaction interface for writing incoming packfiles, making it more backend-agnostic. * jt/receive-pack-pluggable-writes: odb/transaction: add transaction interface to write packfiles odb: return temporary ODB source when set builtin/receive-pack: explicitly pass packfile fd builtin/receive-pack: report unpack errors via strbuf builtin/receive-pack: lift global state out of unpack() builtin/receive-pack: read unpack limit config lazily builtin/receive-pack: pass shallow file explicitly odb/transaction: add transaction finalize interface builtin/receive-pack: properly clean up keep files
The mechanism to generate a packfile corresponding to the result of a fetch/push has been made pluggable through a set of object database callback functions, removing hardcoded references to 'pack-objects' and enabling alternative ODBs to serve packfiles themselves. * ps/odb-pluggable-pack-generation: bundle: generate packfiles via the object database bundle: get (mostly) rid of `the_repository` builtin/bundle: refactor option handling for progress meter send-pack: generate packfiles via the object database upload-pack: generate packfiles via the object database odb: introduce interface to generate packfiles
The pack-objects command has been updated to record the total bytes written to pack files in trace2 output, allowing performance analysis of different compression settings by comparing the resulting pack sizes. * fr/pack-objects-trace-pack-bytes: pack-objects: trace pack bytes written
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The function replay_revisions() in replay.c is rather lengthy. Extract the logic to put a commit entry into a `struct mapped_commits` into a helper function put_mapped_commit(). While at it, rename mapped_commit() to get_mapped_commit() to pair with this new function. Signed-off-by: Toon Claes <toon@iotcl.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Depending on what gets passed into the function pick_regular_commit(), it decides the new base for the replayed commit. It first tries to find the replayed results of `pickme`'s parent in the `replayed_commits` map. If not found, it falls back to `onto`. When using git-replay(1) with --onto, the fallback is the revision passed in with this option, but when using --revert, the fallback is `last_commit`. It's rather confusing the base is decided partly inside pick_regular_commit() and partly by its caller. Move the base selection completely into the caller: replay_revisions(). This bundles all the logic of deciding on the base together. Also, this reduces the number of parameters of pick_regular_commit(), making its interface cleaner. This refactoring doesn't bring any behavior changes. Signed-off-by: Toon Claes <toon@iotcl.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
One of the stated goals of git-replay(1) is to allow implementing the
git-rebase(1) functionality on the server side.
The default mode of git-rebase(1) is to act as if `--no-rebase-merges`
was given. This mode drops merge commits instead of replaying them, and
linearizes the history into a sequence of regular (single-parent)
commits.
Add option `--linearize` to git-replay(1) to do the same. Each replayed
commit is stacked on top of the previously replayed one. When a merge is
encountered, the commits reachable from all of its sides are replayed
into the single line and the merge itself is dropped.
If a ref was pointing to a merge commit, that ref is updated to the
merge's last replayed ancestor.
git-replay(1) accepts multiple branches, for example:
$ git replay --onto main topic1 topic2
Without `--linearize` this replays 'topic1' and 'topic2' onto 'main'
(keeping shared portions of history shared and divergent parts
divergent) and updates both refs.
Due to current implementation limitations, replaying multiple branches
with `--linearize` is disallowed to avoid concatenating unrelated
histories into a single line. For the same reason disallow the use of
`--contained` with `--linearize`.
Users who want to linearize multiple branches are advised to do this in
separate git-replay(1) invocations. Linearizing multiple branches at
once might be added later.
Note that `--linearize` is not modeled after git-rebase(1)'s
`--rebase-merges[=<mode>]` interface. Recreating merges, by preserving
their topology, is a distinct operation that would be a separate mode.
`--linearize` only drops merges and replays commits linearly. So
git-replay(1) uses its own option rather than reusing that interface.
Based-on-patches-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Toon Claes <toon@iotcl.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The CI workflow groups all runs by commit hash using
`group: ${{ github.sha }}`. This means every push to a pull
request starts a separate workflow run, and all workflows
triggered by the same commit share the same concurrency group.
With this change, pull request runs are grouped by pull request
number instead of commit hash, and runs superseded by a newer
push are canceled. The concurrency group becomes
`${{ github.workflow }}-${{ github.event.pull_request.number ||
github.sha }}` and `cancel-in-progress` is set to true for
pull request events.
For pull request events, the group is `<workflow>-<pull-request-number>`
(e.g., "main-workflow-42"). If you push a new commit to an
existing pull request before the CI working on it finishes, the
new request will be placed in the same group and cancel the
currently running run.
For non-pull-request events, the group is `${{ github.workflow }}-${{
github.sha }}` and `cancel-in-progress` defaults to false, so
there is no regression in behavior.
Note that the previous configuration used `group: ${{ github.sha }}`,
which meant all workflows sharing the same commit hash were in the
same group. The new configuration includes the workflow name in
the group, so each workflow has its own concurrency group per
commit/PR.
Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The patch fixes two typos in two places. versioncmp.c: "fractionnal" -> "fractional" t/t0022-crlf-rename.sh: "similiarity" -> "similarity" No functional changes, only update a comment and a test_description. Signed-off-by: Hardik Kumar <hardikxk@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In setup_revisions() we rewrite the incoming argv array, losing references to the strings it contains. For a synthetic argv array constructed from heap strings, that traditionally meant we leaked those allocated strings. We fixed the leak in cd43948 (revision: manage memory ownership of argv in setup_revisions(), 2025-09-19). Now callers can tell the revision code that argv entries are allocated and should be freed, which it will do before overwriting them. But this introduced a new bug! The overwritten entries go away as soon as option parsing is finished, but a few options may actually create new references to those strings. And once we free the strings, those stale references become use-after-free bugs. For example, running: git stash show --src-prefix=foo/ demonstrates the problem: 1. The stash command generates its own synthetic argv (because it has to treat the stash specifiers specially) which it then passes to setup_revisions(). 2. Parsing will create a reference to the partial string "foo/" in revs.diffopt.a_prefix. 3. When setup_revisions() finishes, we rewrite argv to throw away parsed strings. This frees the entry holding "--src-prefix=foo", at which point we have a dangling reference in revs.diffopt. 4. We generate an actual diff, accessing garbage memory via revs.diffopt.a_prefix. The output is usually garbled, but ASan also detects this reliably. One obvious fix here is to allocate new strings when we pull data out of the argv array. But doing so is error prone (every string option must remember to do it or risk a subtle bug), and creates more questions about memory ownership (e.g., some callers assign string literals directly to a_prefix, and we would not want to free those). Instead we can fix this centrally by delaying the free() calls. We'll collect any "freed" strings in a new array, hold on to it for the life of the rev_info struct, and then release it at the end. We can easily use a strvec for this, since it handles growth and cleanup for us. This fixes the prefix case above (which is now tested in t3903), and should fix any other stray cases. Though I could not find any; we use OPT_STRING only in the prefix diff options, and very few revision opts store strings. Those that do (like --format and --encoding) already make a copy of the string. They do not need for us to hold on to the memory longer, but it does not hurt them if we do. One may note that combined with cd43948 we have approached a simpler solution in a roundabout way. We are still hacking up argv, but now carefully constructing a parallel argv of old strings we've overwritten (and will eventually free). In an alternate universe, we could instead leave the original argv pristine and return a new reduced-size argv. This is conceptually simpler, though it does mean that every caller must free that new argv array itself (not the entries). That's not something they traditionally had to do, so it would mean tweaking every caller. So even though the combination of this cd43948 and this patch is a little convoluted, it should make things just work (no leaks and no use-after-free) without modifying any callers. Reported-by: Nicolas Le Cam <niko.lecam@gmail.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
You do not want to mark an argv element for freeing unless the caller has given us the free_removed_argv_elements flag. Originally we just called free() in this case, so each caller checked the flag itself. Now that we mark them via a helper function, we can push the check down into the helper. This saves a little bit of duplicated code, but also hopefully makes the result conceptually simpler. Every caller but one was already checking this flag. The exception is setup_revisions_from_strvec(), but it always sets the flag explicitly (since its whole purpose is managing argv memory). So even though it was not checking the flag, doing so is OK (it will always be set). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The global variable 'fetch_if_missing' has been moved to a member in 'struct repository', continuing the libification process and allowing per-repository control (such as for submodules). * ty/repository-fetch-if-missing: repository: move fetch_if_missing into struct repository
A missing preposition in the rerere technical documentation has been fixed. * jc/rerere-doc-typofix: rerere: technical documentation typofix
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The reftable code has been optimized to avoid an unnecessary stat/reload of the stack when an addition already holds the list_file lock, reducing the number of newfstatat syscalls from linear to constant when writing refs. * kn/reftable-optimize-reloading: reftable/stack: avoid reloading the stack when already locked reftable/stack: move list lock to `struct reftable_stack` reftable/stack: rename reftable_stack_new_addition() reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`
'git checkout' and 'git worktree add' makes guesses based on a name of a remote-tracking branch, but does not give an error when such a remote-tracking branch cannot be uniquely identified, which has been corrected. * yn/worktree-ambiguous-remote-advice: worktree add: treat multiple matches with --guess-remote as an error worktree add: improve message for ambiguous remote branch name checkout: improve message for ambiguous remote branch name checkout: extract function to display advice for ambiguous remotes
The instructions for deprecated commands emitted by you_still_use_that() have been reworded to clarify that the removal decision is final and to provide more assertive guidance on finding a replacement. * jc/you-still-use-that: you_still_use_that(): reword the instructions
The application of the edited patch in 'git add -e' has been refactored to use the internal apply API directly, avoiding the need to spawn a 'git apply' subprocess. * gr/add-e-use-apply-api: builtin/add.c: replace run_command() with direct apply_all_patches() call
The zsh completion script (in 'contrib/') has been updated to correctly locate the Git command after global options like '-C' by properly skipping them, similar to how the bash completion does. * ll/zsh-complete-git-potty-options: completion: zsh: support completion after "git -C <path>"
The git worktree repair command failed to rewrite the .git file of a working tree from a relative path to an absolute path when the command was run in the working tree itself. The read_gitfile_gently() function was modified to also return whether the path originally recorded in the file was absolute, and this new capability is used to correctly detect such mismatches. * yn/worktree-repair-relative: worktree repair: detect relative path in .git file correctly
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A few tests for the reference handling subsystem have been added to exercise the handling of forbidden characters and symbolic references. * ns/ref-symref-additional-tests: t1402: test forbidden characters in refnames t1401: check symbolic-ref failure and --quiet silence on a non-symbolic ref
GitHub Actions CI workflow runs triggered by pull requests have been configured to cancel older runs when a new push is made to the same pull request. * hn/ci-cancel-stale-pr-runs: ci: cancel stale pull request workflow runs
The string extraction logic for the branch name and worktree name from the given path in 'git worktree add' has been corrected and simplified to avoid out-of-bounds reads and improper handling of trailing slashes. * rs/worktree-add-basename-fixes: worktree add: let worktree_basename() return string copy worktree add: trim slashes when deriving branch name from path worktree add: reject separator-only path worktree add: don't read out of bounds in worktree_basename()
Various spelling mistakes in comments and test descriptions have been corrected. * hk/typofix: versioncmp: fix typo in versioncmp.c, t/t0022-crlf-rename.sh
The 'git replay' command has been taught the '--linearize' option to drop merge commits and linearize the replayed history, mimicking 'git rebase --no-rebase-merges'. * tc/replay-linearize: replay: offer an option to linearize the commit topology replay: resolve the replay base outside pick_regular_commit() replay: add helper to put entry into replayed_commits
The memory ownership of argv elements passed to the revision machinery has been made more robust by keeping logically "freed" elements alive until the rev_info struct is released, preventing use-after-free bugs when options store references to them. * jk/rev-info-argv-to-free: revision: simplify mark_argv_for_free() callers revision: hang on to "freed" argv elements
The object lookup machinery has been taught to gracefully recover when a multi-pack-index points to an owning pack that was removed during a concurrent geometric repack, and 'git replay' has been fixed to not segfault when reading such missing objects. * en/midx-missing-pack-fallback: packfile: recover when a multi-pack-index names a removed pack mktree: do not use OBJECT_INFO_QUICK when checking objects mktree: plug per-tree leak in --batch mode replay: fail gracefully when a merge input is unreadable
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for taking the time to contribute to Git! Please be advised that the
Git community does not use github.com for their contributions. Instead, we use
a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/)
to conveniently send your Pull Requests commits to our mailing list.
For a single-commit pull request, please leave the pull request description
empty: your commit message itself should describe your changes.
Please read the "guidelines for contributing" linked above!