[pull] master from git:master - #248
Merged
Merged
Conversation
…tion * kk/commit-reach-find-all-fix: commit-reach: guard !FIND_ALL early exit with generation ordering check t6600: add test for merge-base early exit with clock skew
…-object-info-type * ps/cat-file-remote-object-info: cat-file: make remote-object-info allow-list adapt to the server cat-file: add remote-object-info to batch-command transport: add client support for object-info serve: advertise object-info feature protocol-caps: check object existence regardless of the attributes requested fetch-pack: move fetch initialization connect: make write_fetch_command_and_capabilities() more generic fetch-pack: move write_fetch_command_and_capabilities() to connect.c fetch-pack: use unsigned int for hash_algo variable fetch-pack: drop the static advertise_sid variable t1006: extract helper functions into new 'lib-cat-file.sh' cat-file: declare loop counter inside for() transport-helper: fix memory leak of helper on disconnect
When passing around a `struct odb_write_stream` we typically also have to pass the number of bytes that the stream will yield. This is required because the object header itself contains that size, and consequently we cannot write the header without that information. Move this information into the stream itself so that it becomes self- describing. In addition to that, this also brings the `struct odb_write_stream` a bit closer to the `struct odb_read_stream` so that we can eventually merge both stream types. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `is_finished` field is used to track whether a write stream is done writing all of its data. Tracking this field as part of the stream itself shouldn't be required though: callers will already know when the stream is done when the stream's read function returns zero bytes, same as when reading from a file descriptor. There is one exception where it gets a bit more complicated: when consuming data in "builtin/unpack-objects.c" it may happen that we don't yield any new bytes after reading from the pipe. This is addressed by looping until we have produced at least a single byte of output. Drop the field from `struct odb_write_stream`. Again, same as in the preceding commit, this brings the structure a bit closer to its sibling `struct odb_read_stream`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The object database supports the ability to write object streams into it. This functionality is used when we encounter a blob that is larger than "core.bigFileThreshold" so that we don't have to soak large files into memory. As we only ever write large files, the infrastructure doesn't support specifying any other object type than "blob". This limitation is quite artificial though: there is no reason why we shouldn't support writing arbitrary large objects with a stream. While it's very unlikely that we encounter a huge object other than a blob, users are known to be creative and sometimes like to inflict pain on themselves by creating commits or trees that are huge. Extend the infrastructure to support streaming arbitrary object types. For now we don't use this functionality anywhere, but it brings us a bit closer to unify `struct odb_read_stream` and `struct odb_write_stream`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Rename `struct odb_read_stream` to just `struct odb_stream`. This prepares for unification of the two different types of streams, as these provide the same functionality with the preceding refactorings. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `struct odb_read_stream` and `struct odb_write_stream` both provide
the same functionality: they allow a caller to read object data from an
arbitrary source. Historically, the only difference was that the read
stream was used to read data out of the object database, whereas the
write stream was used to write data into the object database, but the
interfaces were mostly the same.
Over the preceding commits we have refactored the write stream to have
almost exactly the same interface as the read stream. With these
refactorings we can now easily merge those two streams into a single
interface that's used for both use cases.
While most of the changes are mechanical, there are two sites that need
special mention:
- "builtin/unpack-objects.c" creates a write stream from compressed
object data.
- "odb/streaming.c" creates a write stream from a file descriptor.
Adapting these sites to yield the new stream type requires a couple more
changes. Most importantly, instead of embedding the pointer to the data
in `struct odb_write_stream`, we now allocate a structure that wraps the
new `struct odb_stream` base. Other than that though, the changes are
rather straight forward.
Some of the structures and functions are now somewhat misnamed. These
will be fixed in subsequent commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
With the preceding refactorings the `struct read_object_fd_data` is now somewhat misnamed, as it doesn't only contain the data anymore, but also the stream itself. Rename the structure to `struct fd_stream` to better match the new structure. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
With the preceding refactorings the `struct input_zstream_data` is now somewhat misnamed, as it doesn't only contain the data anymore, but also the stream itself. Rename the structure to `struct zlib_stream` to better match the new structure. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Unify the function names to create new streams from different sources so that they follow a common schema. While at it, document the ownership of the file descriptor passed to `odb_stream_from_fd()`. Signed-off-by: Patrick Steinhardt <ps@pks.im> 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>
Clarify that a message file is missing a 'Subject:' line. Terminate the error with a newline so Perl does not append its internal source location. Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Hidden options are not shown by `git <cmd> -h`, but are still shown by
`git <cmd> --help-all`. If there are a lot of hidden options or if they
don't belong to the same categories as other options, there is
currently no way to properly group them.
Using `OPT_GROUP("Foo")` means that "Foo" will always be shown which we
don't want if that group contains only hidden options.
To provide a way to have groups shown only when hidden options are
shown, let's implement an OPT_HIDDEN_GROUP macro.
To test this new macro, let's also improve `test-tool parse-options`
and test its output with `--help-all`.
Signed-off-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "Flags" section in "Documentation/technical/api-parse-options.adoc" documents the flags that can be passed to parse_options() itself. It does not, however, document the flags that can be set on individual options through the `flags` member of `struct option` (and through the `OPT_*_F()` macro variants). These per-option flags are used throughout the codebase (for example `PARSE_OPT_HIDDEN` is used to hide an option from `-h` while still showing it with `--help-all`), but a reader currently has to dig into "parse-options.h" to find them. To remediate that, let's add an "Option flags" subsection to the "Data Structure" section, just before the list of option macros. Let's also make it explicit that these are distinct from the parse_options() flags described earlier, and let's describe the `-h` versus `--help-all` behavior for `PARSE_OPT_HIDDEN`. Signed-off-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In "Documentation/technical/api-parse-options.adoc", the list of option
macros does not mention the `OPT_*_F()` macro variants that take a
trailing `flags` argument, nor the `OPT_HIDDEN_GROUP()` and
`OPT_HIDDEN_BOOL()` convenience macros.
Now that a previous commit documents the per-option flags, let's
document these macros too:
- Add a paragraph explaining the `OPT_*_F` convention and how it
relates to the per-option flags.
- Document `OPT_HIDDEN_GROUP()`, introduced in a previous commit,
right after `OPT_GROUP()`.
- Document `OPT_HIDDEN_BOOL()` right after `OPT_BOOL()`.
Signed-off-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
In cmd_fast_import(), a local variable 'i' is defined as an
`unsigned int` and then used as a loop counter in four different
`for (i = ...; i < ...; i++) { ... }` loops.
But in three out of the four cases, `unsigned int` isn't the best type
to use.
To give each loop counter the type matching its bound
(int/unsigned/size_t), let's localize 'i' into each loop that uses it.
Signed-off-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `show_stats` and `quiet` flags are meant to be parsed and used as boolean flags. To easily parse them using OPT_BOOL in a following commit, let's change their type from 'unsigned int' to just 'int'. Signed-off-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In a following commit we are going to use the parse-options API to
start parsing options. Some options will have to be parsed using
OPT_CALLBACK as they process their arguments in special ways.
When the processing code is already factored out in an option_*()
function, like for `--date-format`, we can reuse that function.
Unfortunately for other options the processing code has not been
factored out yet.
Let's do it now and factor out the code that handles the following
options:
- `--max-pack-size=<n>`
- `--big-file-threshold=<n>`
- `--signed-commits=<mode>`
- `--signed-tags=<mode>`
- `--quiet`
into new option_*() functions:
- option_max_pack_size()
- option_big_file_threshold()
- option_signed_commits()
- option_signed_tags()
- option_quiet()
so that we can reuse these functions in following commits when the
parse-option API will be used.
Note that there are some behavior changes as we now die() with a
proper error message when git_parse_ulong() cannot parse the argument
from --max-pack-size or from --big-file-threshold. Previously we would
end up calling die("unknown option") instead.
Signed-off-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"builtin/fast-import.c" uses a large number of global variables. This makes it harder than necessary to reason about and improve. Especially adding new features requires adding more global variables, while modernizing and eventually libifying the code becomes more and more difficult. To start reverting the sad trend to more and more globals and to start cleaning things up, let's introduce a 'struct fast_import_state' and pass an instance of it as the first argument to many functions. This is similar to what was done for "builtin/apply.c" by introducing a 'struct apply_state', see 07d7e290ff (apply: move 'struct apply_state' to a header file, 2016-08-11) and related commits. As a first step only the 'global_argc', 'global_argv' and 'global_prefix' variables are moved into the new struct. More variables will be moved into it in the following commits. Some functions receive the new 'state' parameter only to pass it along or for future use, so they are marked with UNUSED for now to satisfy '-Werror=unused-parameter'. This is a mostly mechanical refactoring with no intended behavior change. Signed-off-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
A previous commit introduced 'struct fast_import_state' to hold some command state, and reduce the need for global variables. Let's continue in the same direction and move two more global variables that describe the command state into it: 'seen_data_command' and 'allow_unsafe_features'. All the sites accessing these variables are already in functions that receive the 'state' parameter (or in cmd_fast_import() which owns the struct), so no additional threading is needed. As 'state->allow_unsafe_features' is now dereferenced in check_unsafe_feature(), its 'state' parameter is no longer unused, so the UNUSED marker is removed. The fast_import_state_init() call is moved up before the early command-line scan for '--allow-unsafe-features', so that this option can be recorded directly into the struct without being clobbered by the memset() in fast_import_state_init(). This is a mechanical refactoring with no intended behavior change. Signed-off-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Currently `git fast-import -h` shows the following on a single line:
usage : git fast-import [--date-format=<f>] [--max-pack-size=<n>] \
[--big-file-threshold=<n>] [--depth=<n>] \
[--active-branches=<n>] \
[--export-marks=<marks.file>]
This output has a number of issues like:
- It's missing a lot of options.
- It's not consistent with the SYNOPSIS section of the doc.
- With `--help-all` instead of `-h` additional hidden options should
be shown, but that's not the case.
- It's not standard style anymore.
- Most other Git commands show additional lines for most of the
options they support.
Also while most commands use the parse-options API to handle their
options, "builtin/fast-import.c" still doesn't use it.
Let's improve on that by using the parse-options API to display the
options when `-h` and `--help-all` are used.
While at it, let's make the SYNOPSIS section of
"Documentation/git-fast-import.adoc" consistent with the new usage
string.
This deliberately leaves it to future work to also use the
parse-options API to actually parse the options.
Signed-off-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
A previous commit started using the parse-option API to generate proper `git fast-import -h` and `git fast-import --help-all` output. Let's prepare for when we can use that API to also parse the options by using OPT_CALLBACK for some options that require special processing of their arguments. A following commit will actually parse the options using these callbacks. Signed-off-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Previous commits have started to use the parse-options API to display
output from `git fast-import -h` and `git fast-import --help-all` and
to prepare for parsing the command line options using this API.
Let's now actually use the API to parse command line options.
This brings a number of changes that are mostly beneficial:
- The `--alias`, `--get-mark`, `--cat-blob`, `--ls` and `--notes`
options are no longer accepted on the command line. They were
previously accepted as no-ops because parse_argv() fell through to
parse_one_feature(). They are not documented in the OPTIONS section
and are only meaningful as in-stream feature assertions, so
accepting them on the command line was an accident of code sharing
dating back to 9c8398f (fast-import: add option command,
2009-12-04).
- Abbreviated options like `--dep=5` now work since parse_options()
allows unambiguous prefixes.
- As `--cat-blob` is an abbreviation of `--cat-blob-fd`, using the
former on the command line will fail with "option `cat-blob-fd'
requires a value" unlike the other four options that are not
accepted anymore on the command line (see above).
- Value-taking options now also accept the space-separated
`--opt value` form, like `--depth 5`, in addition to the
`--opt=value` form.
- A bare or trailing `--` is now accepted and the stream is read
normally, while it used to be a usage error.
- The error messages for some options might differ a bit.
- The code is shorter and more standard.
Note that parse_one_feature() is now always called with its
`from_stream` argument set to 1, but the code simplifications that
can be made are left for a following clean-up commit.
Signed-off-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Now that a previous commit has removed a call to parse_one_feature() from parse_argv(), the former is always called with its `from_stream` argument set to 1. Let's take advantage of that to simplify and cleanup the code a bit. Signed-off-by: Christian Couder <christian.couder@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a technical document describing the paint_down_to_common() algorithm used for merge-base computation, covering the paint walk, generation number regions, and termination conditions. Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
test_trace2_data is a bare grep that silently exits on failure.
Add a more informative variant that verifies the event appears
exactly once and reports what went wrong: key not found, multiple
entries, or value mismatch. Diagnostics go to FD 4 like test_grep.
Before (value mismatch):
$ test_trace2_data status count/changed 999 <trace2.txt
$ echo $?
1
(no output)
After:
$ test_trace2_data_singular status count/changed 999 <trace2.txt
error: trace2 data 'status/count/changed'
expected: 999
actual: 0
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add test cases to t6600-test-reach.sh that exercise edge cases in the side-exhaustion optimization for paint_down_to_common(): - in_merge_bases_many:self: commit is both A and one of the X inputs - get_merge_bases_many:duplicate-twos: duplicate entries in X list - get_merge_bases_many:pending-stale: STALE transition on an already-painted commit (ps-* diamond topology) - get_merge_bases_many:infinity-both-sides: both tips outside the commit-graph with non-monotonic dates (pi-* topology) Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add t6099 to test the case where multiple merge-base candidates exist and one is an ancestor of another. This exercises the side-exhaustion optimization in paint_down_to_common together with the remove_redundant safety net in get_merge_bases_many_0. Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a step counter and trace2_data_intmax() call so that the number of commits visited during the paint walk is observable via GIT_TRACE2_EVENT. This provides a way to measure the impact of future optimizations without relying on wall-clock benchmarks alone. Some step counts already vary across commit-graph modes (e.g. in_merge_bases_many:self) because the pre-existing min_generation optimization short-circuits the walk when generation data is available. Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add topologies and tests exercising paint_down_to_common() under clock skew, where commit-date ordering (v1 commit-graph without corrected commit dates) violates the topological invariant that children are dequeued before parents: - se-*: side-exhaustion fires too early when one paint side fully drains from the queue while a low-date ancestor on the other side is still queued - se2-*: side-exhaustion returns a too-deep merge base because the correct (closer) base never receives both paint sides Also add step counts to the edge-case tests from the previous commit, a mixed finite/INFINITY generation topology exercising the transition from INFINITY-generation commits to graph-backed commits, and step counts for the grid-based merge-base test. Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add a paint_state struct for use by paint_down_to_common() that wraps a prio_queue with per-side commit counters. Each non-stale queued commit occupies exactly one counter bucket based on its paint flags: PARENT1-only, PARENT2-only, or both sides (a pending merge-base candidate). The counters are maintained by paint_count_update() which adjusts the appropriate bucket by a signed delta. An exhaustive switch on the paint+stale bits documents all valid flag combinations in one place. Convert paint_down_to_common() to use paint_state. The loop now drains the queue via paint_queue_get() which returns NULL when all counters reach zero, replacing the old pointer-based termination (max_nonstale). This is equivalent behavior -- both conditions detect that no non-stale entries remain. paint_queue_get() uses a "pop first" form: it dequeues a commit, then checks the counters. This means the loop exits one iteration earlier than the old code in some topologies (the popped stale commit is never processed), so a few step counts drop by one. The existing nonstale_queue is left in place for ahead_behind(), though nonstale_queue_put_dedup() and nonstale_queue_get_dedup() become unused and are removed. Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Add an early termination check to paint_down_to_common() using the
per-side counters introduced earlier. Once the walk enters the
ordered region, terminate early when one side's exclusive count
drops to zero -- no new merge-base can form without both paint
sides meeting.
The check also waits for pending_merge_bases to reach zero, ensuring
all merge-base candidates have been dequeued and recorded before
exiting.
The optimization is gated by gen_ordered (which excludes v1
commit-graphs that use the date-ordering fallback) and by a
generation check against topo_ceiling. topo_ceiling is
GENERATION_NUMBER_INFINITY for v2 graphs and
GENERATION_NUMBER_V1_MAX for v1 graphs, so that saturated commits
are treated as unordered. Together these ensure the check only
fires in the ordered region where topological ordering holds.
The same topo_ceiling boundary is applied to the existing
single-result early exit so that all generation-dependent gates
express the same saturation-aware boundary consistently.
Step counts measured with trace2 on git.git with commit-graph:
merge-base --all v2.0.0 v2.55.0-rc1:
before: 72264 steps after: 44589 steps
merge-base --all v2.55.0-rc1 v2.55.0-rc1~5:
before: 110 steps after: 7 steps
Helped-by: Derrick Stolee <stolee@gmail.com>
Helped-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Consolidate the min_generation termination condition into paint_queue_get(), alongside the existing stale-entry and side-exhaustion checks. Move last_gen into struct paint_state so that commit_graph_generation() is called exactly once per dequeued commit and the result is shared across all termination checks and the monotonicity BUG assertion. Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Remove the fallback that switched paint_down_to_common() from generation ordering to commit-date ordering when the commit-graph lacks corrected commit dates (v1 graph with topo levels only). The fallback was added in 091f4cf (commit: don't use generation numbers if not needed, 2018-08-30) to avoid a performance regression on the Linux kernel repo where v1 topo levels caused "git merge-base v4.8 v4.9" to walk 636k commits instead of 167k. A side branch with a low topo level stayed in the queue behind a long chain, preventing early STALE propagation. Side-exhaustion (added in the previous commits) solves this differently by terminating the walk as soon as one paint side empties from the queue, preventing the deep walk regardless of queue ordering. Benchmarks of "git merge-base --all v4.8 v4.9" on the Linux kernel repo show that side-exhaustion reduces the step count far below what the date-ordering fallback achieved: steps time no graph, baseline: 167,413 3.25 s v1 graph, baseline: 167,413 0.25 s v2 graph, baseline: 167,441 0.29 s v1 graph, this series: 5,725 0.02 s v2 graph, this series: 3,887 0.01 s With generation ordering always active, the existing min_generation check in paint_queue_get() can safely terminate once the walk crosses below the caller's generation floor. The date ordering fallback broke this invariant: a commit could have a finite topo level while the queue was date-ordered, causing the early exit to fire before all merge bases were found. With the fallback removed, gen_ordered is always true and can be dropped. The topo_ceiling field (introduced earlier) already handles V1_MAX saturation, so the early exit gates need no further changes. Signed-off-by: Kristofer Karlsson <krka@spotify.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
As of 4557f1a (rebase--helper: add a builtin helper for interactive rebases, 2017-02-09), continuing an interactive rebase uses the builtin sequencer, which spawns `git commit`. The child may trigger auto-maintenance, which may need to replace files for which the sequencer still holds resources. See git-for-windows#6315: on Windows, this produces unlink retry prompts that cannot succeed while the sequencer waits for the child. Resources such as file handles or memory mappings must be released before spawning a command that may run auto-maintenance, as established by 28d04e1 (run-command: offer to close the object store before running, 2021-09-09): release the ODB file handles and memory mappings, so that auto-gc can repack (potentially deleting existing packfiles in the process); If the sequencer needs to access the ODB afterwards, it will gracefully (re-)open the ODB. Release the sequencer's ODB before spawning `git commit`. The regression test uses the legacy-delete trick introduced by 69ed0e3 (mingw: optionally use legacy (non-POSIX) delete semantics, 2026-05-07) to trigger the failure on modern Windows. Assisted-by: GPT-5.6 Sol Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The 'remote-object-info' command for 'git cat-file --batch-command' has been extended to support the '%(objecttype)' placeholder. * ps/cat-file-remote-object-info-type: cat-file: unify default format serve: advertise type capability fetch-object-info: parse type from server response protocol-caps: add type support to object-info transport: drop remote object-info fields from transport struct fetch-object-info: die() on the remaining error path fetch-object-info: use dedicated struct for the results fetch-object-info: pass arguments directly instead of a struct fetch-object-info: detect malformed server responses t5701: use test_file_size() to get the size of a file
The usage string of 'git fast-import' has been updated to use the parse_options() API for displaying help, and its SYNOPSIS in the documentation has been standardized to match. * cc/fast-import-usage: fast-import: remove useless from_stream argument fast-import: use parse_options() for command line options fast-import: use callbacks to parse some options fast-import: use struct option for usage string fast-import: move command state globals into 'struct fast_import_state' fast-import: introduce 'struct fast_import_state' fast-import: factor out option_*() functions fast-import: use int for some bool flags fast-import: localize 'i' into the 'for' loops using it api-parse-options.adoc: document hidden and OPT_*_F option macros api-parse-options.adoc: document per-option flags parse-options: introduce OPT_HIDDEN_GROUP
The 'struct odb_read_stream' and 'struct odb_write_stream' structures have been consolidated into a single unified 'struct odb_stream' structure, simplifying object database streaming APIs and enabling streaming of arbitrary object types. * ps/odb-streams: odb/streaming: unify function names to create new streams odb/streaming: rename `struct input_zstream_data` odb/streaming: rename `struct read_object_fd_data` odb/streaming: consolidate read and write streams odb/streaming: rename `struct odb_read_stream` odb/streaming: support streaming arbitrary object types odb/streaming: drop `is_finished` field odb/streaming: track write stream size in the structure
The error message given by 'git send-email' when a message file is missing a 'Subject:' header has been clarified, and the error string is now terminated with a newline so that Perl avoids appending its internal source location data. * hn/send-email-missing-subject-error: send-email: clarify missing subject error
The sequencer has been updated to release the object database before spawning 'git commit'. This prevents open file handles from blocking auto-maintenance tasks, such as repacking, on systems like Windows where open files cannot be easily unlinked. * js/sequencer-release-odb-before-commit: sequencer: release the ODB before spawning git commit
The merge-base computation has been optimized by stopping the walk early when one side's exclusive commits in the queue are exhausted, yielding significant speedups for queries with one-sided histories. * kk/merge-base-exhaustion: commit-reach: remove commit-date ordering fallback commit-reach: move min_generation check into paint_queue_get() commit-reach: terminate merge-base walk when one paint side is exhausted commit-reach: introduce struct paint_state with per-side counters t6600: add clock-skew topologies and step counts for edge cases commit-reach: add trace2 instrumentation to paint_down_to_common() t6099: add side-exhaustion regression test t6600: add test cases for side-exhaustion edge cases test-lib-functions: improve diagnostic output for trace2 data assertions Documentation/technical: add paint-down-to-common doc
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )