| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
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 9c8398f0c9 (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>
|
|
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>
|
|
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 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>
|
|
"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>
|
|
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>
|
|
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 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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
* 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
|
|
Comment lines have always been ignored but this is not documented.
The primary motivation here is to be reasonably complete in the
documentation of how trailers are parsed; this is after all the only
documentation page that documents this format. However, and going beyond
that point, we could imagine that someone would want to use this format
outside a commit (or tag) message context, like say in Git notes.
On the other hand, it seems far-fetched that someone would be caught
off guard by this considering that comment characters/strings are not
likely to be alphanumeric,[1] which would mean that these comment lines
would be treated as non-trailer lines if they were *not* detected and
removed as comment lines.
† 1: A notable exception is that Jujutsu VCS uses `JJ:` as
the comment string
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
Two commits ago we moved new-trailers paragraph next to each other.
But there is something curious about two of them:
By default the new trailer will appear at the end of the trailer
block. [...]
Then a source block and a paragraph later:
By default, a `<key>=<value>` or `<key>:<value>` argument given
using `--trailer` will be appended after the existing trailers only
if [...]
Why are there two paragraphs that talk about how “By default” a trailer
will be appended?
We can make these paragraphs flow better, and with a more distinct
character each, by dividing the flow like this:
1. Declare that we are about to talk about `--trailer` appending
2. Explain the default behavior
3. Explain how this affects the trailer block
4. Then discuss what each trailer line will look like
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
We chose to introduce the term “trailer block” into the documentation a
few commits ago.[1] It is used in the code though, so it is not a newly
invented term.
That term was useful to explain where the trailers are found (they
*trail* the message). But it is also useful here, where we explain
how trailers are added to existing messages, how trailer blocks are
found (beyond the simple case in the introduction), and how the end
of the message is found.
Also note that we simplify the “blank line” point. The text says:
A blank line will be added before the new trailer if there isn't one
already.
But this isn’t quite coherent. The previous sentence says “If there is
no existing trailer”, so we are in one of these modes:
1. discussing trailer blocks in general; or
2. discussing creating a new trailer block in particular.
If (1), then we shouldn’t add a blank line before the new trailer if
there exists a trailer block already. And if (2), then the “if there
isn’t one already” is redundant.[2] So just talking about the higher-
level “trailer block” simplifies the text, since we don’t have to worry
about the different contexts that *trailers* can find themselves in.
† 1: in commit “explain the format after the intro”
† 2: Note that non-trailer lines don’t matter here; if you have a
trailer block consisting of `(cherry picked from commit <commit>)`,
then you still shouldn’t insert a blank line before the new trailer
since that would create a new trailer block
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
There are three paragraphs that talk about how a new trailer is added.
But the first one is separated from the other two by two paragraphs
about how `key-alias` can make using `--trailer` more convenient. This
short how-to does not follow thematically from the previous paragraph,
and can wait until we have fully described how a new trailer is
added. So let’s move the three paragraphs about the new-trailer topic
together and move the how-to paragraphs after that.
***
Let’s now review the history of the document. Even if the document
is not quite correct in its current state, just doing the apparently
obvious edit without considering the history does not respect the
effort that went into changing the document in the past.
These three paragraphs were originally next to each other, in the first
version of the doc.[1] But extra sentences about this how-to topic was
added to the first paragraph nine years later:[2]
[...]
`': '` (one colon followed by one space). For convenience, the
<token> can be a shortened string key (e.g., "sign") instead of the
full string which should [...]
And then it was split into it’s own paragraph a little later.[3]
This evolution shows, in my opinion, that this how-to never followed
thematically from the existing topic. Which means that there is nothing
that was potentially lost to time that we need to restore or respect.
† 1: dfd66ddf (Documentation: add documentation for 'git
interpret-trailers', 2014-10-13)
† 2: eda2c44c (doc: trailer: mention 'key' in DESCRIPTION, 2023-06-15)
† 3: 6ccbc667 (trailer doc: <token> is a <key> or <keyAlias>, not both,
2023-09-07)
Suggested-by: D. Ben Knoble <ben.knoble+github@gmail.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
All of the examples speak of the Happy Path where everything works
as intended. But failure examples can also be instructive. Especially
for explaining again, by example, the key format (see previous commit).
This also allows us to demonstrate trailer block detection with a
concrete example.
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
A trailer key must consist of ASCII alphanumeric characters and
hyphens *only*. Let’s document it explicitly instead of relying on
readers being conservative and only basing their trailer keys on the
documentation examples.[1]
The previous commit provided us with an appropriate paragraph to
describe the key format.
† 1: Technically they would then miss out on using digits in them since
all of the example keys just use letters and hyphens
Reported-by: Brendan Jackman <jackmanb@google.com>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
You need to read the entire “Description” section in order to understand
the full trailer format. But there are many nuances, so that’s fine.
As a starter though we have an introductory example.[1] That turns out
to be crucial; the rest of this section talks about the mechanics of the
command and only incidentally the format itself.
Now, although the example might arguably be self-explanatory, we can
add a little preamble which defines the format in its simplest form as
well as define the most important terms.
Note that we name the “blank line” rule since I want to use that term
every time it comes up. It gets very mildly obfuscated if you call it
a “blank line” in one place[2] and “empty (or whitespace-only) ...” in
another one.[3]
We will define the format of the *key* in the next commit.
† 1: from d57fa7fc (doc: trailer: add more examples in DESCRIPTION,
2023-06-15)
† 2: `Documentation/git-interpret-trailers.adoc:86` in
5361983c (The 22nd batch, 2026-03-27)
† 3: `Documentation/git-interpret-trailers.adoc:93` in
5361983c (The 22nd batch, 2026-03-27)
Suggested-by: D. Ben Knoble <ben.knoble+github@gmail.com>
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 066cef77 (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>
|
|
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>
|
|
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: 68e3c69e (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>
|
|
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>
|
|
%(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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
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
|
|
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 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
|
|
'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
|
|
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
|
|
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
|
|
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>
|
|
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 5ce91c059e41090e7d2cffad39c04af8acf98dc1 side-band no-progress
00000009done
EOF
$ echo 5ce91c059e41090e7d2cffad39c04af8acf98dc1 | 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>
|
|
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>
|
|
In the preceding commit we have added a compatibility wrapper for the
writev(3p) syscall. Introduce some generic wrappers for this function
that we nowadays take for granted in the Git codebase.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
|
|
In a subsequent commit we're going to add the first caller to
writev(3p). Introduce a compatibility wrapper for this syscall that we
can use on systems that don't have this syscall.
The syscall exists on modern Unixes like Linux and macOS, and seemingly
even for NonStop according to [1]. It doesn't seem to exist on Windows
though.
[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>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
