[pull] master from git:master - #252
Merged
Merged
Conversation
* kh/doc-trailers: doc: interpret-trailers: document comment line treatment doc: interpret-trailers: rewrite new-trailers paragraphs doc: interpret-trailers: commit to “trailer block” term doc: interpret-trailers: join new-trailers again doc: interpret-trailers: add key format example doc: interpret-trailers: explain key format doc: interpret-trailers: explain the format after the intro doc: interpret-trailers: not just for commit messages doc: interpret-trailers: use “metadata” in Name as well doc: interpret-trailers: replace “lines” with “metadata” doc: interpret-trailers: stop fixating on RFC 822
…d-alternates * ps/odb-make-creation-pluggable: odb: make creation of on-disk structures pluggable odb/source: introduce function to map source type to name setup: defer object database creation setup: handle ODB-related environment variables in `odb_new()` setup: detangle loading of loose object maps loose: load loose object map for the correct source
Use the parse-options completion helpers for the git history subcommands and their options. All current history subcommands take a revision as their first positional argument, so complete that argument as a revision. Once the revision is present, leave any further positional arguments to subcommand-specific completion. This allows a subcommand to complete another kind of argument, such as the pathspec accepted by git history split or another revision if a future subcommand accepts one. Signed-off-by: Vincent Mailhol <mailhol@kernel.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "--empty" option accepts "drop", "keep", or "abort" for the "drop" and "fixup" subcommands. Complete these values for the documented --empty=<value> form. While parse-options also accepts the split --empty <value> form, it is not documented. Omit it from completion as a trade-off for code simplicity. Signed-off-by: Vincent Mailhol <mailhol@kernel.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The "--update-refs" option accepts either "branches" or "head". Complete these values for the documented --update-refs=<value> form. While parse-options also accepts the split --update-refs <value> form, it is not documented. Omit it from completion as a trade-off for code simplicity. Signed-off-by: Vincent Mailhol <mailhol@kernel.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Arguments following the required revision of "git history split" are pathspecs. Complete them from tracked paths, including after an explicit "--". Signed-off-by: Vincent Mailhol <mailhol@kernel.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When creating a new repository we create both the reference and object databases after we have finalized the repository. This ensures that those subsystems find a fully-configured repository at the time where they are asked to create their own on-disk data structures. There is one exception though: while we have already fully configured the repository at this point, we haven't yet written both "core.sharedRepository" and "receive.denyNonFastforwards". The latter configuration doesn't really matter to us, but the first one does as the "files" object database source reads it. This doesn't cause any problems right now, but it will in a subsequent patch where we will start to read "core.ignoreCase" when creating the object database. Move the initialization of both of these data structures towards the end of `init_db()`. The only thing that now comes after is status reporting, but that's it. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When registering alternates we deduplicate object database sources by their path so that the same source won't be added twice. Ever since cf2dc1c (speed up alt_odb_usable() with many alternates, 2021-07-07) this duplicate check is backed by a map keyed by the source's path, using `fspathhash()` and `fspatheq()` as hash and equality functions, respectively. These functions are problematic in this context for two reasons: - They implicitly depend on `the_repository` instead of the repository that owns the object database. - They derive case-sensitivity from `repo_ignore_case()`, which returns a default value in case the repository's configuration has not been parsed yet. Object database sources may be registered before that is the case, so the answer may flip depending on when a source gets registered. Fix this by making the comparison self-contained in the object database. Instead of using `fspathhash()` and `fspatheq()` we resolve "core.ignoreCase" manually and then use the correct comparison function based on the result. This requires us to migrate to a `struct hashmap`, as the khash interface does not give us the ability to pass an arbitrary payload to these functions, and hence we'd have to use global state to decide which of those to use. Note that we can unconditionally use `strihash()` to compute entry hashes regardless of case sensitivity: a hash function only needs to guarantee that equal keys have equal hashes, and a case-insensitive hash satisfies this requirement for both case-sensitive and case-insensitive equality. Overall it's quite debatable whether all of this complexity really is worth it, out of two reasons: - We could linearly search through all sources to find duplicates. But the mentioned commit cares about cases with thousands of alternates, and a linear search would of course regress performance quite a bit. This doesn't really feel like a reasonable case to care about, but I don't feel comfortable regressing it anyway. - It's dubious whether we should handle "core.ignoreCase" in the first place. The downside would be that we might add the same alternate multiple times with different casing. But this is an edge case, and it's not even fully fixed because we don't resolve symlinks or mountpoints, either. So for now, keep this infrastructure in-place while removing the global dependency on `the_repository`. We may want to revisit this in the future though. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When creating the object database we initialize the main object database source, but we don't yet initialize its alternates. Instead, we have many calls to `odb_prepare_alternates()` cluttered around the code base whenever we are about to iterate through the sources. This lazy loading doesn't really add much value: the moment where we read any object we _have_ to load the alternates anyway. So given that most of our commands would access the object database this optimization is not really buying us much in the first place. Quite on the contrary, it makes the code harder to understand and is a potential source of bugs in case any callsite forgot to prepare alternates before we iterate through the sources. Historically though there was a reason why we deferred lazy-loading: it may happen that the repository has "core.ignoreCase" configured, and we use that to deduplicate the list of alternates in case we had the same alternate configured multiple times, but with different casing. We used to initialize the object database before we had fully configured the owning repository though, and consequently we couldn't access that configuration yet. This has changed in the preceding commit though where we started to parse "core.ignoreCase" manually. Eagerly prepare alternates both when creating the object database and when flushing its caches. Drop the now-unneeded calls to prepare the alternates that are scattered across the code base. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `struct object_database::loaded_alternates` field tells us whether or not alternates have been loaded already. This field was useful before the preceding commit as we were indeed lazy-loading alternates. But now that we started to eagerly load them we can assume them to be loaded after `odb_new()`, and hence the field does not serve any purpose anymore. Remove it. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `struct object_database::alternates_db` field tracks the value of the "GIT_ALTERNATE_OBJECT_DIRECTORIES" environment variable and is used in `odb_prepare_alternates()`. It's not necessary to store it as a separate field anymore though, as we stopped lazy-loading alternates. Consequently, we can simply pass it to `odb_prepare_alternates()` via `odb_new()` now. Do so and remove the field. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When reading packed objects we know to tell apart missing objects and corrupt objects by returning a positive error code in the former case, and a negative one in the latter case. We do that by distinguishing between errors returned by `find_pack_entry()`, which yields the offset of the object, and `packed_object_info()`, which reads the object contents. But even though we already distinguish those cases when reading packed objects, the logic is broken in case a caller tries to read an object that has been marked as corrupt. In that case, `find_pack_entry()` will tell us that the object in question does not exist, and consequently we'll not flag the object as corrupt but as missing. Fix this issue by bubbling up whether the object is corrupt and, if so, which packfile contains the corrupted object. Note that we don't yet need the information about the specific packfile, so we could've just as well made this a `bool *corrupted` pointer. But we'll need information about the containing packfile in a subsequent commit so that we can generate a proper error message telling the user which packfile contains the broken object. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The `read_object_info()` callback of `struct odb_source` is documented
to return a negative error code in case reading the object has failed,
and zero otherwise. This is overly broad though, as there are two very
different kinds of failures:
- The object may not exist in the source at all.
- The object exists, but reading it has failed, for example because
its on-disk state is corrupt.
This distinction matters to callers: when an object is corrupt in one
source we may still find a good copy of it in another source, so we may
still be able to proceed with a given operation.
The "packed" source already distinguishes these cases by returning a
positive value for missing objects and a negative value in case reading
the object has failed. But it is the only such source that distinguishes
those cases, and the returned value is translated into a negative error
code by the "files" backend anyway.
Introduce a new error status that is specific to reading objects and
adapt the infrastructure to return it. For now, we only discern
successful reads from generic failures, which mostly matches the status
quo. In subsequent commits though we're about to add an error that
explicitly tells the caller that an object does not exist.
Note that we keep the "packed" backend as-is with its positive return
code for missing objects. This will be fixed in the next commit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
As explained in the preceding commits, reading objects can either fail because the object truly does not exist or because it exists, but its data is corrupt. Some callers do care about this distinction, but there is no way to tell these two cases apart right now. Introduce a new `ODB_READ_NOT_FOUND` value that ought to be returned by the backends in case the object truly does not exist and adapt backends to use it. Note that we don't yet return this error from `odb_read_object_info()` itself. This will be fixed in a subsequent commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When reading an object fails even though it exists, the sources know best what exactly went wrong and where the corrupt object is located. This information is lost though when bubbling up the error to the object database layer, which forces that layer to reconstruct it after the fact. This is exactly what `do_oid_object_info_extended()` does via `has_packed_and_bad()`, but that function only really knows to handle the "files" backend by reaching into its internals. Introduce a new `errmsg` parameter for the `read_object_info()` callback that sources are expected to populate with a human-readable message in case reading the object has failed. Adapt the packed and loose sources to populate the buffer with the messages that we ultimately want to surface to the user. For now, all callers are adapted to pass a `NULL` pointer. We will add a user of this new infrastructure in a subsequent commit. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When a lookup with `OBJECT_INFO_DIE_IF_CORRUPT` fails we want to die in
case the object exists, but cannot be read. This flag is handled in two
different spots right now:
- `do_oid_object_info_extended()` calls `has_packed_and_bad()` to
check whether the object is known to be corrupt in any packfile.
This function reaches into the internals of the packed source and
thus breaks the abstraction provided by our object sources.
- The loose source handles the flag itself and dies directly in
`read_object_info_from_path()`, which means that we die even in
cases where another source may still have a good copy of the
object.
Besides being inconsistent, it also ties us to the specific backend used
by the database sources because `has_packed_and_bad()` assumes that they
use the "files" backend. Any other backend will instead cause us to die
when calling `odb_source_files_downcast()`, even if the object was
simply nonexistent.
In the preceding commits we've carved out the infrastructure to make
this mechanism fully generic. On the one hand, all backends now tell us
whether the object is missing or corrupt via their return values. And
on the other hand, they have been taught to provide a readable error
message to the caller.
Adapt `do_oid_object_info_extended()` to use those new mechanisms. This
means that we won't die immediately anymore when a loose object is
corrupt, and we properly handle backends other than the "files" backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
We want to measure how compression settings affect push performance on the client. Different settings can produce different-sized packs from the same objects. Trace2 records the object count, but we also need the pack size to compare those settings. Add a write_pack_file/wrote_bytes Trace2 datum alongside write_pack_file/wrote. Count packs written to stdout or disk, including each pack's header and trailing checksum. When pack.packSizeLimit splits the output, report the sum of the pack sizes. Signed-off-by: Friel <friel@openai.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
'git worktree add <path> <branch>' DWIMs <branch> to a remote-tracking branch when neither -b, -B, nor --detach is given. However, 'git worktree add -b <new-branch> <path> <branch>' can still DWIM <branch>, causing <new-branch> to be ignored. This is a regression introduced by 128e549 (worktree add: extend DWIM to infer --orphan, 2023-05-17), which appeared in Git 2.42. Signed-off-by: Yoichi NAKAYAMA <yoichi.nakayama@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When git-receive-pack(1) stores an incoming packfile with git-index-pack(1), a ".keep" file is written alongside it in the transaction quarantine directory and also gets migrated to the main ODB when the ODB transaction is committed. This keep lockfile ensures the packfile remains in place until the references have been updated and is removed afterwards. The path used to remove it is derived via `index_pack_lockfile()` from the repository's primary object directory. In bdee7b3 (builtin/receive-pack: stage incoming objects via ODB transactions, 2026-07-10), git-receive-pack(1) started using the ODB transaction interfaces instead of managing a temporary directory directly. When starting an ODB transaction, the sources list is reordered to insert the newly created transaction source first as the primary to ensure writes are routed to it accordingly. Prior to using ODB transactions, git-receive-pack(1) would only set the temporary directory as the primary source for the child git-index-pack(1) and git-unpack-objects(1) processes it spawned and the parent process would set the temporary directory set as an alternate only. By using ODB transactions, the ODB source list is also reordered for the parent process which results in `index_pack_lockfile()` deriving the ".keep" path relative to the temporary directory instead of the actual main ODB source path. Consequently, this prevents the ".keep" file from being properly removed after being migrated into the main ODB source post-commit. Update `index_pack_lockfile()` to operate on an ODB source explicitly provided to it and update call sites accordingly to pass the expected ODB source. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When committing an ODB transaction via `odb_transaction_commit()`, the staged objects are made visible and the underlying transaction is freed at the same time. Coupling these two steps does not leave room for any post-commit transaction operations to be introduced though. Such a capability is useful if an ODB transaction backend needs to hold on to lockfiles after transaction commit until references are updated, as is the case with the existing "files" backend in git-receive-pack(1). Stop freeing the transaction in `odb_transaction_commit()` and introduce `odb_transaction_finalize()` to explicitly clean up the transaction accordingly. Note that the finalize interface also provides an optional callback for any backend-specific deferred cleanup. In a subsequent commit, the "files" transaction backend will use this to remove ".keep" files generated for packfiles received via git-receive-pack(1) after references have been updated. In preparation for this, the `odb_transaction_finalize()` call site in git-receive-pack(1) is made after the reference updates are finished. All other callers commit a transaction and immediately finalize it without any work happening in between those two operations. Consequently, they cannot meaningfully recover in case either of them would fail, and spelling out these two separate steps with proper error handling would be quite repetitive and pointless. Introduce a helper `odb_transaction_commit_and_finalize_or_die()` for those call sites and update them accordingly. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
If shallow information is provided during `unpack()`, a temporary shallow file is created and stored in global state. In a subsequent commit, the `unpack()` logic is moved behind a generic ODB transaction interface to handle writing packfiles and thus can no longer rely on such global state. Lift the setup of the temporary shallow file out of `unpack()` and wire it through to its call sites explicitly. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In git-receive-pack(1), the `receive.unpackLimit` and `transfer.unpackLimit` configuration decides whether an incoming packfile should be exploded into loose objects or kept as a packfile on-disk. In a subsequent commit, the logic to write the incoming packfile is made ODB backend agnostic and moved behind a pluggable ODB transaction interface. Consequently, whether to explode a packfile is a detail of how a particular backend stores objects and should not be a part of the generic interface itself. In preparation for this, instead resolve the unpack limit lazily inside `unpack()` by reading the configuration directly. The now-unused unpack limit globals are dropped accordingly. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In git-receive-pack(1), writing the packfile to the transaction is handled via `unpack()` which relies on global variables to decide how to invoke the underlying git-index-pack(1) or git-unpack-objects(1) child processes. In a subsequent commit, the `unpack()` logic is moved behind a generic ODB transaction interface to handle writing packfiles and thus can no longer rely on these globals. Lift the global state out of `unpack()` by instead storing this state in a `struct unpack_opts` that gets passed to the function explicitly. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When writing packfiles via `unpack()`, error messages are returned directly by the function. In preparation for `unpack()` logic being moved behind a generic ODB transaction interface, update the function to instead write any error messages to a caller provided strbuf and return a negative value on error. Call sites are updated to use the error strbuf accordingly. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When processing the incoming packfile in git-receive-pack(1), `unpack()` assumes it should always read it from stdin. In preparation for `unpack()` logic being moved behind a generic ODB transaction interface, update the function signature to take the an explicit fd provided by callers to read the incoming packfile from instead. Call sites are updated accordingly. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When invoked, `odb_set_temporary_primary_source()` installs a temporary object directory as the new primary ODB source. A caller that wants to operate on the ODB source of the open transaction must assume that it is the first entry in the ODB source list which is a bit awkward and fragile. Instead, return the newly installed source directly and report the previous primary source via a new `prev_source` out parameter. Propagate the installed source through `tmp_objdir_replace_primary_odb()` and start storing it in the "files" ODB transaction so a subsequent commit can easily access it without relying on the ODB source list ordering. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
In git-receive-pack(1), the incoming packfile is written to the ODB via `unpack()`, which spawns git-index-pack(1) or git-unpack-objects(1) directly. With pluggable object databases, an alternative backend may need to handle writing packfile data differently though. Introduce `odb_transaction_write_pack()` as a generic interface to handle writing a packfile to a transaction and use the logic from `unpack()` as the "files" backend implementation. Note that when storing the objects as a packfile, git-index-pack(1) also writes a ".keep" lockfile next to it to prevent a concurrent repack from removing the new pack prior to reference updates being performed. The "files" transaction backend is responsible for managing these ".keep" files and removes them post-commit once the transaction is finalized. Call sites in git-receive-pack(1) are updated accordingly. Signed-off-by: Justin Tobler <jltobler@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
An HTTPS URL starts with an alphanumeric scheme followed by a colon.
That means that they will be recognized as trailers in a trailer block.
That turns out to be a problem in practice. Let’s stop recognizing these
as trailers by failing the trailer parsing when we:
1. find the separator;
2. the separator and the next two characters form `://`; and
3. we haven’t parsed any whitespace yet.
The simplest example of how this can be a problem is for people who do
not use trailers but may leave URLs at the end of the commit message.
Now, while these authors might not use trailers themselves, other
authors may have used trailers and this metadata confusion can become a
problem once someone tries to extract that metadata (and non-metadata).
Let’s now look at some examples in the Linux Kernel[1] to see how this
is a problem in practice.
There are commits which contain intended non-trailer lines which start
with URLs. These are comments. Example with just the trailers:[2]
Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
[bhelgaas: squash fixes:
https://lore.kernel.org/r/20260108013956.14351-2-bagasdotme@gmail.com
https://lore.kernel.org/r/20260108013956.14351-3-bagasdotme@gmail.com]
Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Link: https://patch.msgid.link/20251210132907.58799-4-xueshuai@linux.alibaba.com
Those `[]` pairs delimit the “squash fixes” comment.
Now, any of these two commands:
git log --format='%(trailers:only)' -1 <commit>
git log -1 --format=%B <commit> |
git interpret-trailers --only-trailers
Will both wrongly (according to the surmised user intent) include these
two URL lines as trailers and also mangle the URLs, e.g.:
https: //lore.kernel.org/r/20260108013956.14351-2-bagasdotme@gmail.com
Because the `--only-trailers` mode (or `only` for the git-log(1) format)
normalizes the output to a colon and a space.
Another example is linewrapping mistakes; a `Link` trailer with a
URL where the URL ended up on the next line, presumably because the
user’s editor linewrapped the “too long” line. Example with just the
trailers:[3]
Link: https://patch.msgid.link/20260216-work-xattr-socket-v1-4-c2efa4f74cb7@kernel.org
Link:
https://lore.kernel.org/3cnmtqmakpbb2uwhenrj7kdqu3uefykiykjllgfbtpkiwhaa4s@sghkevv7jned [1]
Acked-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner <brauner@kernel.org>
Now, this intended trailer is already ruined, but interpreting the URL
as a standalone trailer only compounds the mistake.
Yet another example is the trailer machinery normalizing the trailer
block before application, resulting in a `https` trailer key in the
commit message itself. Example with just the trailers:[4]
https: //sashiko.dev/#/patchset/20260429114208.941011-1-holger.brunck%40hitachienergy.com
Fixes: c19b6d2 ("drivers/net: support hdlc function for QE-UCC")
Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
Link: https://patch.msgid.link/20260507155332.3452319-1-holger.brunck@hitachienergy.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
We have a helpful `Link` that points to the original patch.[5] Following
it we can see that that `https` trailer was indeed a URL
originally (again just the trailer block here):
https://sashiko.dev/#/patchset/20260429114208.941011-1-holger.brunck%40hitachienergy.com
Fixes: c19b6d2 ("drivers/net: support hdlc function for QE-UCC")
Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
So how did it end up as a `https` trailer? My theory is that the trailer
block was normalized on patch application, causing a URL comment to be
wrongly normalized and cemented in the commit message as a trailer.[6]
† 1: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/
† 2: commit 8236fc613d44e59f6736d6c3e9efffaf26ab7f00
† 3: commit 5bd97f5c5f241a5610c4412d1b93995a26241f81
† 4: commit 496c0c4c53bbe1bad97e82cd12103df61a6e459d
† 5: https://patch.msgid.link/20260507155332.3452319-1-holger.brunck@hitachienergy.com
† 6: There are only four commits in the Linux Kernel of this kind, and
three of them have the same recurring person in the signoff chain.
***
Note that this check has some benign false positives. A trailer key
can start with a digit, but a URL scheme can not start with a digit.
That means that a line that starts with `1://` will be rejected even
though it cannot be a URL. I don’t think this will reject any real
trailers, so I think the implementation simplicity is worth it.
And these false positives are just for a limited start fragment check;
a mere heuristic, not a URL parser.
Helped-by: Jeff King <peff@peff.net>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Packfiles have two primary use cases:
- They are used to store objects at rest in a Git repository.
- They are used on the transport layer to transfer objects between two
repositories.
The first class is closely tied to a given object database backend, and
as such this use is highly specific to how such a backend decides to
store its data. This shows in git-pack-objects(1), which is used by
git-repack(1) et al to optimize the object database, which supports lots
of options that are closely coupled with how data is stored.
But the second class is quite a lot more generic: we don't care about
specifics of how the object database stores its objects, but to generate
the packfiles we only care about the object graph itself. Still, this
use case is also coupled with git-pack-objects(1).
Unfortunately, because git-pack-objects(1) covers both classes, the
result is that it is very hard to port the whole command to properly
support pluggable object databases. There are simply way too many
options that an alternative implementation will have a very hard time to
support in the first place.
And despite being hard to implement, it's also quite unnecessary to
implement those backend-specific options. Optimizing the object database
has already been made pluggable, and an alternative implementation is
unlikely to care about cruft packs, unpacked objects, keep packs and the
like. But we still need to make at least _parts_ of the packfile
generation pluggable so that backends can generate packfiles for the
transport layer itself.
Introduce a new interface that lets backends generate a new packfile and
implement that interface for the "files" backend. The options supported
by the callback are exactly the set of options that are required for the
transport layer, but nothing more.
This means that git-pack-objects(1) itself cannot be ported over to this
new interface, but as explained above that's a hard feat to pull off due
to the backend-specific features. Ideally though, we should expose the
ability to generate arbitrary packfiles using this interface. The intent
of this is to eventually introduce a git-objects(1) subcommand (similar
to git-refs(1)) that exposes generic interfaces for accessing everything
related to the object database. In that case, we are able to expose only
those options that are generic.
Subsequent commits will convert git-upload-pack(1), git-send-pack(1) and
git-bundle(1) to use this interface.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
When serving a fetch, git-upload-pack(1) spawns git-pack-objects(1) directly to generate the packfile that gets sent to the client. This hard-codes the assumption that the object database is able to serve packfiles via git-pack-objects(1), which is specific to the "files" backend. Convert git-upload-pack(1) to instead use the pack generation interface of the object database. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When pushing, git-send-pack(1) spawns git-pack-objects(1) directly to
generate the packfile that gets sent to the remote. Same as with
git-upload-pack(1), which has been adapted in the preceding commit,
this hard-codes the assumption that objects can be packed via
git-pack-objects(1), which is specific to the "files" backend.
Convert git-send-pack(1) to use the pack generation interface of the
object database instead.
Note that this requires us to adapt t5516 because the parameters passed
to git-pack-objects(1) are changing:
- The order of arguments changes.
- We pass "--quiet" instead of "-q".
- We don't pass "--all-progress-implied" anymore when not generating
output.
All of these changes are benign though and should not result in a change
in behaviour.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
The git-bundle(1) command has a couple of command line options that
relate to whether or not progress should be reported. These options
match the options that git-pack-objects(1) expects, and consequently
they mostly get passed through to it directly.
This results in somewhat of a confusing interface: there are four
different options that relate to whether or not progress should be
displayed and how verbose it should be. But in reality, there's really
only two modes:
- "--progress" and "--all-progress" result in the same outcome, which
is also documented as such.
- "--all-progress-implied" does nothing as we pass that argument to
git-pack-objects(1) unconditionally anyway.
So in the end, the options only control whether or not progress should
be displayed at all, nothing else.
Refactor the interface to instead use a simple `progress` boolean. This
makes argument handling a lot more straight-forward and it prepares us
for the next commit, where we're migrating git-bundle(1) to the generic
interface for generating a packfile.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Refactor "bundle.c" so that we don't depend on `the_repository` anymore. This conversion is trivial for most of the part, as we already have a repository available in all calling conexts. The only exception is that we use `get_log_output_encoding()`, which implicitly depends on `the_repository`. Add an `extern` declaration for this function so that we can drop `USE_THE_REPOSITORY_VARIABLE` and not accidentally introduce more uses of `the_repository`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
git-bundle(1) spawns git-pack-objects(1) directly to generate the pack data that gets appended to the bundle header. While bundles are not part of the wire protocol, they are a transfer mechanism for packs all the same, so convert them to use the pack generation interface of the object database as well. This makes the pack generator the single spawn point for all pack streams that leave the repository, leaving only local maintenance tasks like git-repack(1) with direct knowledge of git-pack-objects(1). Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
When performing auto-maintenance with geometric repacking we have two conditions that may trigger a repack: - Either the geometric sequence of packfiles is invalidated. - Or we have too many loose objects. The first condition shouldn't trigger all that often: it may be hit when we fetch a new packfile, but users tend to not do that all the time. The second condition is what typically triggers more regularly though, as every command that ends up writing new objects may cause us to cross the threshold of loose objects. It is thus preferable to not be too aggressive here, as otherwise we may end up repacking objects quite often. For the geometric-repacking strategy though we have a default of 100 objects, only. As we're approximating the count of objects by only reading the "objects/17/" shared, we'd only need 2 objects in there before we perform a repack by default, which is quite aggressive. git-gc(1) on the other hand has a default of 6700, so it is quite a bit more conservative here. Being this aggressive is also causing problems as reported by our users. When running lots of concurrent writers, those writes will constantly end up spawning maintenance jobs that end up repacking objects. As we also prune objects, a concurrently running process that tries to write an object may see that the sharding directories get removed under their feet. While we try re-creating such leading directories, we only do so a single time, and it may happen that the directory vanishes again before we had the chance to create the loose object. This is not a new problem, but it is exacerbated by us running maintenance this aggressively. Improve the status quo by reducing the frequency at which we pack loose objects to the same frequency that git-gc(1) uses. Reported-by: Stefan Haller <lists@haller-berlin.de> Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The DWIM logic in 'git worktree add' sometimes tried to infer a remote-tracking branch when an explicit '-b' or '-B' option was given to create a new branch, causing the explicit branch name to be ignored, which has been corrected. * yn/worktree-add-no-dwim-with-b: worktree add: shouldn't dwim if -b or -B is given
The object database (odb) API has been refactored to distinguish between missing objects and corrupt ones by returning more descriptive error statuses. Both the packed and loose backends now faithfully propagate error details using a generic strbuf error mechanism, removing backend-specific leakage from central lookup paths. * ps/odb-generic-corrupt-objects: odb: handle `OBJECT_INFO_DIE_IF_CORRUPT` generically odb/source: allow `read_object_info()` to bubble up error messages odb/source: let callers discern missing and corrupt objects odb/source: introduce error status when reading objects odb/source-packed: flag known-bad objects as corrupt and not missing
The command line completion (in contrib/) has been taught to handle the experimental 'git history' command. * vm/complete-history: completion: complete 'git history split' pathspecs completion: complete 'git history --update-refs' values completion: complete 'git history --empty' values completion: add 'git history' subcommands
The object database layer has been simplified by eagerly loading alternate object directories upon initialization, instead of deferring it to the first object lookup. This eliminates the need for scattered lazy-loading calls throughout the codebase and paves the way for integrating alternates with the pluggable backends. * ps/odb-eagerly-load-alternates: odb: drop `alternates_db` field odb: drop `loaded_alternates` field odb: eagerly initialize alternates odb: decouple source path comparisons from `the_repository` setup: create ref and object databases after config is written
The trailer parsing machinery has been updated to avoid mistaking lines that begin with a URL (e.g., 'https://...') as trailer lines. This prevents intended textual URLs from being mangled or mistakenly treated as metadata keys. * kh/trailers-no-urls: trailers: stop recognizing URLs as trailers
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>
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.
7 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 : )