* [PATCH v3 01/10] trailer: prepare to expose functions as part of API
From: Linus Arver via GitGitGadget @ 2024-01-31 1:22 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v3.git.1706664144.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
In the next patch, we will move "process_trailers" from trailer.c to
builtin/interpret-trailers.c. That move will necessitate the growth of
the trailer.h API, forcing us to expose some additional functions in
trailer.h.
Rename relevant functions so that they include the term "trailer" in
their name, so that clients of the API will be able to easily identify
them by their "trailer" moniker, just like all the other functions
already exposed by trailer.h.
Take the opportunity to start putting trailer processions options (opts)
as the first parameter. This will be the pattern going forward in this
series.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 4 ++--
trailer.c | 26 +++++++++++++-------------
trailer.h | 6 +++---
3 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 033bd1556cf..85a3413baf5 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -132,11 +132,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
if (argc) {
int i;
for (i = 0; i < argc; i++)
- process_trailers(argv[i], &opts, &trailers);
+ interpret_trailers(&opts, &trailers, argv[i]);
} else {
if (opts.in_place)
die(_("no input file given for in-place editing"));
- process_trailers(NULL, &opts, &trailers);
+ interpret_trailers(&opts, &trailers, NULL);
}
new_trailers_clear(&trailers);
diff --git a/trailer.c b/trailer.c
index 3a0710a4583..66b6660f5a4 100644
--- a/trailer.c
+++ b/trailer.c
@@ -163,12 +163,12 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-static void print_all(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts)
+static void format_trailers(const struct process_trailer_options *opts,
+ struct list_head *trailers, FILE *outfile)
{
struct list_head *pos;
struct trailer_item *item;
- list_for_each(pos, head) {
+ list_for_each(pos, trailers) {
item = list_entry(pos, struct trailer_item, list);
if ((!opts->trim_empty || strlen(item->value) > 0) &&
(!opts->only_trailers || item->token))
@@ -589,7 +589,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
return 0;
}
-static void ensure_configured(void)
+static void trailer_config_init(void)
{
if (configured)
return;
@@ -1035,10 +1035,10 @@ static void parse_trailers(struct trailer_info *info,
}
}
-static void free_all(struct list_head *head)
+static void free_trailers(struct list_head *trailers)
{
struct list_head *pos, *p;
- list_for_each_safe(pos, p, head) {
+ list_for_each_safe(pos, p, trailers) {
list_del(pos);
free_trailer_item(list_entry(pos, struct trailer_item, list));
}
@@ -1075,16 +1075,16 @@ static FILE *create_in_place_tempfile(const char *file)
return outfile;
}
-void process_trailers(const char *file,
- const struct process_trailer_options *opts,
- struct list_head *new_trailer_head)
+void interpret_trailers(const struct process_trailer_options *opts,
+ struct list_head *new_trailer_head,
+ const char *file)
{
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
struct trailer_info info;
FILE *outfile = stdout;
- ensure_configured();
+ trailer_config_init();
read_input_file(&sb, file);
@@ -1110,9 +1110,9 @@ void process_trailers(const char *file,
process_trailers_lists(&head, &arg_head);
}
- print_all(outfile, &head, opts);
+ format_trailers(opts, &head, outfile);
- free_all(&head);
+ free_trailers(&head);
trailer_info_release(&info);
/* Print the lines after the trailers as is */
@@ -1135,7 +1135,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,
size_t nr = 0, alloc = 0;
char **last = NULL;
- ensure_configured();
+ trailer_config_init();
end_of_log_message = find_end_of_log_message(str, opts->no_divider);
trailer_block_start = find_trailer_block_start(str, end_of_log_message);
diff --git a/trailer.h b/trailer.h
index 1644cd05f60..37033e631a1 100644
--- a/trailer.h
+++ b/trailer.h
@@ -81,9 +81,9 @@ struct process_trailer_options {
#define PROCESS_TRAILER_OPTIONS_INIT {0}
-void process_trailers(const char *file,
- const struct process_trailer_options *opts,
- struct list_head *new_trailer_head);
+void interpret_trailers(const struct process_trailer_options *opts,
+ struct list_head *new_trailer_head,
+ const char *file);
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 00/10] Enrich Trailer API
From: Linus Arver via GitGitGadget @ 2024-01-31 1:22 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <pull.1632.v2.git.1706308737.gitgitgadget@gmail.com>
This patch series is the first 10 patches of a larger cleanup/bugfix series
(henceforth "larger series") I've been working on. The main goal of this
series is to begin the process of "libifying" the trailer API. By "API" I
mean the interface exposed in trailer.h. The larger series brings a number
of additional cleanups (exposing and fixing some bugs along the way), and
builds on top of this series.
When the larger series is merged, we will be in a good state to additionally
pursue the following goals:
1. "API reuse inside Git": make the API expressive enough to eliminate any
need by other parts of Git to use the interpret-trailers builtin as a
subprocess (instead they could just use the API directly);
2. "API stability": add unit tests to codify the expected behavior of API
functions; and
3. "API documentation": create developer-focused documentation to explain
how to use the API effectively, noting any API limitations or
anti-patterns.
The reason why the larger series itself doesn't tackle these goals directly
is because I believe that API code should be thought from the ground up with
a libification-focused perspective. Some structs and functions exposed in
the API today should probably not be libified (read: kept in trailer.h) as
is. For example, the "trailer_iterator" struct has a "private" member and it
feels wrong to allow API users to peek inside here (and take at face value
our future API users' pinky promise that they won't depend on those private
internals not meant for public consumption).
One pattern we could use here to cleanly separate "what is the API"
(publicly exposed) and "what is the implementation" (private) is the
pointer-to-implementation ("pimpl") idiom. There may be other appropriate
patterns, but I've chosen this one because it's a simple, low-level concept
(put structs in foo.c instead of foo.h), which has far-reaching high-level
consequences (API users must rely exclusively on the API to make use of such
private structs, via opaque pointers). The pimpl idiom for C comes from the
book "C Interfaces and Implementations" (see patch "trailer: make
trailer_info struct private").
The idea of turning a public struct into a private one is a fundamental
question of libification because it forces us to reconsider all of the data
structures we have and how they're actually used by already existing users.
For the trailer API, those existing users are the "interpret-trailers"
builtin command, and anything else that includes the "trailer.h" header file
(e.g., sequencer.c). One advantage of this idiom is that even the compiler
understands it --- the compiler will loudly complain if you try to access
the innards of a private struct through an opaque pointer.
Another advantage of this idiom is that it helps to reduce the probability
of breaking changes in the API. Because a private struct's members are out
of view from our users (they only know about opaque pointers to the private
struct, not its members), we are free to modify the members of the struct at
any time, as much as we like, as long as we don't break the semantics of the
exposed API functions (which is why unit-testing these API functions will be
crucial long-term).
If this pimpl idiom turns out to be a mistake, undoing it is easy --- just
move the relevant struct definition from foo.c to the header file. So it's a
great way to try things out without digging ourselves into a pit of despair
that will be difficult to get out of.
With the libification-focused goals out of the way, let's turn to this patch
series in more detail.
Currently, we have "process_trailers()" in trailer.h which does many
different things (parse command-line arguments, create temporary files, etc)
that are independent of the concept of "trailers". Keeping this function as
an API function would make unit-testing it difficult. While there is no
technical reason why we couldn't write unit tests for the smaller functions
that are called within process_trailers(), doing so would involve testing
private ("static" in trailer.c) functions instead of API functions, which
defeats the goal of "API stability" mentioned earlier above.
As an alternative to how things are done in this patch series, we could keep
trailer.h intact and decide to unit-test the existing "trailer_info_get()"
function which does most of the trailer parsing work (and is used by
sequencer.c). However this function wouldn't be easy to test either, because
the resulting "trailer_info" struct merely contains the unparsed "trailers"
lines. So the unit test (if it wants to inspect the result of parsing these
lines) would have to invoke additional parsing functions itself. And at that
point it would no longer be a unit test in the traditional sense, because it
would be invoking multiple functions at once.
In summary this series breaks up "process_trailers()" into smaller pieces,
exposing many of the parts relevant to trailer-related processing in
trailer.h. This will force us to eventually introduce unit tests for these
API functions, but that is a good thing for API stability.
In the future after libification is "complete", users external to Git will
be able to use the same trailer processing API used by the
interpret-trailers builtin. For example, a web server may want to parse
trailers the same way that Git would parse them, without having to call
interpret-trailers as a subprocess. This use case was the original
motivation behind my work in this area.
Thanks to the aggressive refactoring in this series, I've been able to
identify and fix several bugs in our existing implementation. Those fixes
build on top of this series but were not included here, in order to keep
this series small. Below is a "shortlog" of those fixes I have locally:
* "trailer: trailer replacement should not change its position" (If we
found a trailer we'd like to replace, preserve its position relative to
the other trailers found in the trailer block, instead of always moving
it to the beginning or end of the entire trailer block.)
* "interpret-trailers: preserve trailers coming from the input" (Sometimes,
the parsed trailers from the input will be formatted differently
depending on whether we provide --only-trailers or not. Make the trailers
that were not modified and which are coming directly from the input get
formatted the same way, regardless of this flag.)
* "interpret-trailers: do not modify the input if NOP" (Refrain from
subtracting or adding a newline around the patch divider "---" if we are
not adding new trailers.)
* "trailer formatter: split up format_trailer() monolith" (Fix a bug in
git-log where we still printed a blank newline even if we didn't want to
format anything.)
* "interpret-trailers: fail if given unrecognized arguments" (E.g., for
"--where", only accept recognized WHERE_* enum values. If we get
something unrecognized, fail with an error instead of silently doing
nothing. Ditto for "--if-exists" and "--if-missing".)
Notable changes in v3
=====================
* Squashed Patch 4 into Patch 3 ("trailer: unify trailer formatting
machinery"), to avoid breaking the build ("-Werror=unused-function"
violations)
* NEW (Patch 10): Introduce "trailer template" terminology for readability
(no behavioral change)
* (API function) Rename default_separators() to
trailer_default_separators()
* (API function) Rename new_trailers_clear() to free_trailer_templates()
* trailer.h: for single-parameter functions, anonymize the parameter name
to reduce verbosity
Notable changes in v2
=====================
* (cover letter) Discuss goals of the larger series in more detail,
especially the pimpl idiom
* (cover letter) List bug fixes pending in the larger series that depend on
this series
* Reorder function parameters to have trailer options at the beginning (and
out parameters toward the end)
* "sequencer: use the trailer iterator": prefer C string instead of strbuf
for new "raw" field
* Patch 1 (was Patch 2) also renames ensure_configured() to
trailer_config_init() (forgot to rename this one previously)
Linus Arver (10):
trailer: prepare to expose functions as part of API
trailer: move interpret_trailers() to interpret-trailers.c
trailer: unify trailer formatting machinery
sequencer: use the trailer iterator
trailer: make trailer_info struct private
trailer: spread usage of "trailer_block" language
trailer: prepare to move parse_trailers_from_command_line_args() to
builtin
trailer: move arg handling to interpret-trailers.c
trailer: delete obsolete argument handling code from API
trailer: introduce "template" term for readability
builtin/interpret-trailers.c | 183 ++++++--
builtin/shortlog.c | 7 +-
pretty.c | 2 +-
ref-filter.c | 2 +-
sequencer.c | 35 +-
trailer.c | 835 +++++++++++++++++------------------
trailer.h | 105 +++--
7 files changed, 631 insertions(+), 538 deletions(-)
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1632%2Flistx%2Ftrailer-api-refactor-part-1-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1632/listx/trailer-api-refactor-part-1-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1632
Range-diff vs v2:
1: e2d3ed9b5b6 ! 1: 72cc12a3066 trailer: prepare to expose functions as part of API
@@ Commit message
them by their "trailer" moniker, just like all the other functions
already exposed by trailer.h.
- The the opportunity to start putting trailer processions options (opts)
+ Take the opportunity to start putting trailer processions options (opts)
as the first parameter. This will be the pattern going forward in this
series.
2: eaca39fd7ea ! 2: cafa39d1048 trailer: move interpret_trailers() to interpret-trailers.c
@@ trailer.h: struct process_trailer_options {
+void trailer_config_init(void);
+void format_trailers(const struct process_trailer_options *opts,
+ struct list_head *trailers, FILE *outfile);
-+void free_trailers(struct list_head *trailers);
++void free_trailers(struct list_head *);
+
/*
* Format the trailers from the commit msg "msg" into the strbuf "out".
3: 9b7747d550e ! 3: 5c7a2354df0 trailer: unify trailer formatting machinery
@@ ref-filter.c: static void grab_sub_body_contents(struct atom_value *val, int der
} else if (atom->u.contents.option == C_BARE)
## trailer.c ##
-@@ trailer.c: static void print_tok_val(FILE *outfile, const char *tok, const char *val)
- fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
+@@ trailer.c: static char last_non_space_char(const char *s)
+ return '\0';
}
+-static void print_tok_val(FILE *outfile, const char *tok, const char *val)
+-{
+- char c;
+-
+- if (!tok) {
+- fprintf(outfile, "%s\n", val);
+- return;
+- }
+-
+- c = last_non_space_char(tok);
+- if (!c)
+- return;
+- if (strchr(separators, c))
+- fprintf(outfile, "%s%s\n", tok, val);
+- else
+- fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
+-}
+-
-void format_trailers(const struct process_trailer_options *opts,
- struct list_head *trailers, FILE *outfile)
-{
@@ trailer.c: static void unfold_value(struct strbuf *val)
/*
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
-@@ trailer.c: static void format_trailer_info(struct strbuf *out,
-
+@@ trailer.c: void trailer_info_release(struct trailer_info *info)
+ free(info->trailers);
}
--void format_trailers_from_commit(struct strbuf *out, const char *msg,
-- const struct process_trailer_options *opts)
+-static void format_trailer_info(struct strbuf *out,
+- const struct trailer_info *info,
+- const char *msg,
+- const struct process_trailer_options *opts)
+void format_trailers_from_commit(const struct process_trailer_options *opts,
+ const char *msg,
+ struct strbuf *out)
{
+- size_t origlen = out->len;
+- size_t i;
+ LIST_HEAD(head);
- struct trailer_info info;
-
-- trailer_info_get(&info, msg, opts);
-- format_trailer_info(out, &info, msg, opts);
-+ parse_trailers(opts, &info, msg, &head);
++ struct trailer_info info;
+
-+ /* If we want the whole block untouched, we can take the fast path. */
-+ if (!opts->only_trailers && !opts->unfold && !opts->filter &&
-+ !opts->separator && !opts->key_only && !opts->value_only &&
-+ !opts->key_value_separator) {
++ parse_trailers(opts, &info, msg, &head);
+
+ /* If we want the whole block untouched, we can take the fast path. */
+ if (!opts->only_trailers && !opts->unfold && !opts->filter &&
+ !opts->separator && !opts->key_only && !opts->value_only &&
+ !opts->key_value_separator) {
+- strbuf_add(out, msg + info->trailer_block_start,
+- info->trailer_block_end - info->trailer_block_start);
+- return;
+- }
+-
+- for (i = 0; i < info->trailer_nr; i++) {
+- char *trailer = info->trailers[i];
+- ssize_t separator_pos = find_separator(trailer, separators);
+-
+- if (separator_pos >= 1) {
+- struct strbuf tok = STRBUF_INIT;
+- struct strbuf val = STRBUF_INIT;
+-
+- parse_trailer(&tok, &val, NULL, trailer, separator_pos);
+- if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
+- if (opts->unfold)
+- unfold_value(&val);
+-
+- if (opts->separator && out->len != origlen)
+- strbuf_addbuf(out, opts->separator);
+- if (!opts->value_only)
+- strbuf_addbuf(out, &tok);
+- if (!opts->key_only && !opts->value_only) {
+- if (opts->key_value_separator)
+- strbuf_addbuf(out, opts->key_value_separator);
+- else
+- strbuf_addstr(out, ": ");
+- }
+- if (!opts->key_only)
+- strbuf_addbuf(out, &val);
+- if (!opts->separator)
+- strbuf_addch(out, '\n');
+- }
+- strbuf_release(&tok);
+- strbuf_release(&val);
+-
+- } else if (!opts->only_trailers) {
+- if (opts->separator && out->len != origlen) {
+- strbuf_addbuf(out, opts->separator);
+- }
+- strbuf_addstr(out, trailer);
+- if (opts->separator) {
+- strbuf_rtrim(out);
+- }
+- }
+- }
+-
+-}
+-
+-void format_trailers_from_commit(struct strbuf *out, const char *msg,
+- const struct process_trailer_options *opts)
+-{
+- struct trailer_info info;
+ strbuf_add(out, msg + info.trailer_block_start,
+ info.trailer_block_end - info.trailer_block_start);
+ } else
+ format_trailers(opts, &head, out);
-+
+
+- trailer_info_get(&info, msg, opts);
+- format_trailer_info(out, &info, msg, opts);
+ free_trailers(&head);
trailer_info_release(&info);
}
@@ trailer.h: void trailer_info_release(struct trailer_info *info);
- struct list_head *trailers, FILE *outfile);
+ struct list_head *trailers,
+ struct strbuf *out);
- void free_trailers(struct list_head *trailers);
+ void free_trailers(struct list_head *);
/*
- * Format the trailers from the commit msg "msg" into the strbuf "out".
4: f1171f5202f < -: ----------- trailer: delete obsolete formatting functions
5: 5ba842b5005 = 4: bf2b8e1a3c4 sequencer: use the trailer iterator
6: f0ac2f6c4b9 = 5: c19c1dcc592 trailer: make trailer_info struct private
7: 291aa83af55 = 6: 0a9a7438c3f trailer: spread usage of "trailer_block" language
8: 64ee07d0b53 ! 7: 97e5d86ddf0 trailer: prepare to move parse_trailers_from_command_line_args() to builtin
@@ Commit message
themselves). The interpret-trailers builtin is the only caller of this
function.
- Also rename "conf_info" to "trailer_conf" for readability, dropping the
- low-value "_info" suffix as we did earlier in this series for
- "trailer_info" to "trailer_block".
+ Rename add_arg_item() to trailer_add_arg_item() because it will have to
+ be exposed as an API function in the next patch. Rename
+ new_trailers_clear() to free_new_trailers() because it will be promoted
+ into an API function; the API already has free_trailers(), so using the
+ "free_*" naming style will keep it consistent. Also rename "conf_info"
+ to "trailer_conf" for readability, dropping the low-value "_info" suffix
+ as we did earlier in this series for "trailer_info" to "trailer_block".
Helped-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Linus Arver <linusa@google.com>
+ ## builtin/interpret-trailers.c ##
+@@ builtin/interpret-trailers.c: static int option_parse_if_missing(const struct option *opt,
+ return trailer_set_if_missing(opt->value, arg);
+ }
+
+-static void new_trailers_clear(struct list_head *trailers)
++static void free_new_trailers(struct list_head *trailers)
+ {
+ struct list_head *pos, *tmp;
+ struct new_trailer_item *item;
+@@ builtin/interpret-trailers.c: static int option_parse_trailer(const struct option *opt,
+ struct new_trailer_item *item;
+
+ if (unset) {
+- new_trailers_clear(trailers);
++ free_new_trailers(trailers);
+ return 0;
+ }
+
+@@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
+ interpret_trailers(&opts, &trailers, NULL);
+ }
+
+- new_trailers_clear(&trailers);
++ free_new_trailers(&trailers);
+
+ return 0;
+ }
+
## trailer.c ##
@@ trailer.c: struct trailer_block {
size_t trailer_nr;
@@ trailer.c: static ssize_t find_separator(const char *line, const char *separator
item = list_entry(pos, struct arg_item, list);
if (token_matches_item(tok->buf, item, tok_len)) {
@@ trailer.c: static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
+ return new_item;
}
- static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
+-static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
- const struct conf_info *conf,
-+ const struct trailer_conf *conf,
- const struct new_trailer_item *new_trailer_item)
+- const struct new_trailer_item *new_trailer_item)
++static void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
++ const struct trailer_conf *conf,
++ const struct new_trailer_item *new_trailer_item)
{
struct arg_item *new_item = xcalloc(1, sizeof(*new_item));
new_item->token = tok;
@@ trailer.c: static struct trailer_item *add_trailer_item(struct list_head *head,
if (new_trailer_item) {
if (new_trailer_item->where != WHERE_DEFAULT)
new_item->conf.where = new_trailer_item->where;
+@@ trailer.c: void parse_trailers_from_config(struct list_head *config_head)
+ list_for_each(pos, &conf_head) {
+ item = list_entry(pos, struct arg_item, list);
+ if (item->conf.command)
+- add_arg_item(config_head,
+- xstrdup(token_from_item(item, NULL)),
+- xstrdup(""),
+- &item->conf, NULL);
++ trailer_add_arg_item(config_head,
++ xstrdup(token_from_item(item, NULL)),
++ xstrdup(""),
++ &item->conf, NULL);
+ }
+ }
+
@@ trailer.c: void parse_trailers_from_command_line_args(struct list_head *arg_head,
{
struct strbuf tok = STRBUF_INIT;
@@ trailer.c: void parse_trailers_from_command_line_args(struct list_head *arg_head
} else {
- parse_trailer(&tok, &val, &conf, tr->text,
- separator_pos);
+- add_arg_item(arg_head,
+- strbuf_detach(&tok, NULL),
+- strbuf_detach(&val, NULL),
+- conf, tr);
+ parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
- add_arg_item(arg_head,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
++ trailer_add_arg_item(arg_head,
++ strbuf_detach(&tok, NULL),
++ strbuf_detach(&val, NULL),
++ conf, tr);
+ }
+ }
+
@@ trailer.c: struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
for (i = 0; i < trailer_block->trailer_nr; i++) {
9: 1b4bdde65bc ! 8: 465dc51cdcb trailer: move arg handling to interpret-trailers.c
@@ Commit message
trailer: move arg handling to interpret-trailers.c
We don't move the "arg_item" struct to interpret-trailers.c, because it
- is now a struct that contains information about trailers that should be
- injected into the input text's own trailers. We will rename this
- language as such in a follow-up patch to keep the diff here small.
+ is now a struct that contains information about trailers that could be
+ added into the input text's own trailers. This is a generic concept that
+ extends beyond trailers defined as CLI arguments (it applies to trailers
+ defined in configuration as well). We will rename "arg_item" to
+ "trailer_template" in a follow-up patch to keep the diff here small.
Signed-off-by: Linus Arver <linusa@google.com>
@@ builtin/interpret-trailers.c: static int option_parse_if_missing(const struct op
return trailer_set_if_missing(opt->value, arg);
}
--static void new_trailers_clear(struct list_head *trailers)
+-static void free_new_trailers(struct list_head *trailers)
-{
- struct list_head *pos, *tmp;
- struct new_trailer_item *item;
@@ builtin/interpret-trailers.c: static int option_parse_if_missing(const struct op
+ ssize_t separator_pos;
if (unset) {
- new_trailers_clear(trailers);
+ free_new_trailers(trailers);
@@ builtin/interpret-trailers.c: static int option_parse_trailer(const struct option *opt,
if (!arg)
return -1;
@@ builtin/interpret-trailers.c: static int option_parse_trailer(const struct optio
+ */
+ trailer_conf_set(where, if_exists, if_missing, conf_current);
+
-+ add_arg_item(strbuf_detach(&tok, NULL),
-+ strbuf_detach(&val, NULL),
-+ conf_current,
-+ trailers);
++ trailer_add_arg_item(strbuf_detach(&tok, NULL),
++ strbuf_detach(&val, NULL),
++ conf_current,
++ trailers);
+ } else {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addstr(&sb, arg);
@@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **
+ * In command-line arguments, '=' is accepted (in addition to the
+ * separators that are defined).
+ */
-+ cl_separators = xstrfmt("=%s", default_separators());
++ cl_separators = xstrfmt("=%s", trailer_default_separators());
argc = parse_options(argc, argv, prefix, options,
git_interpret_trailers_usage, 0);
@@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **
+ interpret_trailers(&opts, &arg_trailers, NULL);
}
-- new_trailers_clear(&trailers);
-+ new_trailers_clear(&arg_trailers);
+- free_new_trailers(&trailers);
++ free_new_trailers(&arg_trailers);
return 0;
}
@@ trailer.c: static LIST_HEAD(conf_head);
static char *separators = ":";
-+const char *default_separators(void)
++const char *trailer_default_separators(void)
+{
+ return separators;
+}
@@ trailer.c: ssize_t find_separator(const char *line, const char *separators)
* Obtain the token, value, and conf from the given trailer.
*
+ * The conf needs special handling. We first read hardcoded defaults, and
-+ * override them if we find a matching trailer configuration in the config.
++ * override them if we find a matching trailer configuration.
+ *
* separator_pos must not be 0, since the token cannot be an empty string.
*
@@ trailer.c: static struct trailer_item *add_trailer_item(struct list_head *head,
return new_item;
}
--static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
-- const struct trailer_conf *conf,
-- const struct new_trailer_item *new_trailer_item)
-+void add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
-+ struct list_head *arg_head)
-+
+-static void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
+- const struct trailer_conf *conf,
+- const struct new_trailer_item *new_trailer_item)
++void trailer_add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
++ struct list_head *arg_head)
{
struct arg_item *new_item = xcalloc(1, sizeof(*new_item));
new_item->token = tok;
@@ trailer.c: void parse_trailers_from_config(struct list_head *config_head)
list_for_each(pos, &conf_head) {
item = list_entry(pos, struct arg_item, list);
if (item->conf.command)
-- add_arg_item(config_head,
-- xstrdup(token_from_item(item, NULL)),
-+ add_arg_item(xstrdup(token_from_item(item, NULL)),
- xstrdup(""),
-- &item->conf, NULL);
-+ &item->conf,
-+ config_head);
+- trailer_add_arg_item(config_head,
+- xstrdup(token_from_item(item, NULL)),
++ trailer_add_arg_item(xstrdup(token_from_item(item, NULL)),
+ xstrdup(""),
+- &item->conf, NULL);
++ &item->conf,
++ config_head);
}
}
@@ trailer.c: void parse_trailers_from_command_line_args(struct list_head *arg_head
strbuf_release(&sb);
} else {
parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
-- add_arg_item(arg_head,
-- strbuf_detach(&tok, NULL),
-+ add_arg_item(strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
-- conf, tr);
-+ conf,
-+ arg_head);
+- trailer_add_arg_item(arg_head,
+- strbuf_detach(&tok, NULL),
++ trailer_add_arg_item(strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+- conf, tr);
++ conf,
++ arg_head);
}
}
@@ trailer.c: void free_trailers(struct list_head *trailers)
}
}
-+void new_trailers_clear(struct list_head *trailers)
++void free_new_trailers(struct list_head *trailers)
+{
+ struct list_head *pos, *p;
+
@@ trailer.h: struct new_trailer_item {
void duplicate_trailer_conf(struct trailer_conf *dst,
const struct trailer_conf *src);
-+const char *default_separators(void);
++const char *trailer_default_separators(void);
+
-+void add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
-+ struct list_head *arg_head);
++void trailer_add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
++ struct list_head *arg_head);
+
struct process_trailer_options {
int in_place;
@@ trailer.h: struct new_trailer_item {
@@ trailer.h: void format_trailers(const struct process_trailer_options *opts,
struct list_head *trailers,
struct strbuf *out);
- void free_trailers(struct list_head *trailers);
--
-+void new_trailers_clear(struct list_head *new_trailers);
+ void free_trailers(struct list_head *);
++void free_new_trailers(struct list_head *);
+
/*
* Convenience function to format the trailers from the commit msg "msg" into
- * the strbuf "out". Reuses format_trailers internally.
10: ed67ebf8647 ! 9: 885ac87a544 trailer: delete obsolete argument handling code from API
@@ Commit message
This commit was not squashed with its parent in order to keep the diff
separate (to make the additive changes in the parent easier to read).
+ Note that we remove the "new_trailer_item" struct, because it has some
+ overlap with "arg_item" struct that also deals with trailers coming from
+ the command line. The latter will be renamed to "trailer_template" in
+ the next patch.
+
Signed-off-by: Linus Arver <linusa@google.com>
## trailer.c ##
@@ trailer.c: void parse_trailers_from_config(struct list_head *config_head)
- strbuf_release(&sb);
- } else {
- parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
-- add_arg_item(strbuf_detach(&tok, NULL),
-- strbuf_detach(&val, NULL),
-- conf,
-- arg_head);
+- trailer_add_arg_item(strbuf_detach(&tok, NULL),
+- strbuf_detach(&val, NULL),
+- conf,
+- arg_head);
- }
- }
-
-: ----------- > 10: bcd3fc9660e trailer: introduce "template" term for readability
--
gitgitgadget
^ permalink raw reply
* [PATCH v3 02/10] trailer: move interpret_trailers() to interpret-trailers.c
From: Linus Arver via GitGitGadget @ 2024-01-31 1:22 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v3.git.1706664144.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
The interpret-trailers.c builtin is the only place we need to call
interpret_trailers(), so move its definition there.
Delete the corresponding declaration from trailer.h, which then forces
us to expose the working innards of that function. This enriches
trailer.h with a more granular API, which can then be unit-tested in the
future (because interpret_trailers() by itself does too many things to
be able to be easily unit-tested).
Take this opportunity to demote some file-handling functions out of the
trailer API implementation, as these have nothing to do with trailers.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 94 +++++++++++++++++++++++++++
trailer.c | 120 ++++-------------------------------
trailer.h | 20 +++++-
3 files changed, 124 insertions(+), 110 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 85a3413baf5..8556acde4aa 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -9,6 +9,7 @@
#include "gettext.h"
#include "parse-options.h"
#include "string-list.h"
+#include "tempfile.h"
#include "trailer.h"
#include "config.h"
@@ -91,6 +92,99 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
return 0;
}
+static struct tempfile *trailers_tempfile;
+
+static FILE *create_in_place_tempfile(const char *file)
+{
+ struct stat st;
+ struct strbuf filename_template = STRBUF_INIT;
+ const char *tail;
+ FILE *outfile;
+
+ if (stat(file, &st))
+ die_errno(_("could not stat %s"), file);
+ if (!S_ISREG(st.st_mode))
+ die(_("file %s is not a regular file"), file);
+ if (!(st.st_mode & S_IWUSR))
+ die(_("file %s is not writable by user"), file);
+
+ /* Create temporary file in the same directory as the original */
+ tail = strrchr(file, '/');
+ if (tail)
+ strbuf_add(&filename_template, file, tail - file + 1);
+ strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
+
+ trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
+ strbuf_release(&filename_template);
+ outfile = fdopen_tempfile(trailers_tempfile, "w");
+ if (!outfile)
+ die_errno(_("could not open temporary file"));
+
+ return outfile;
+}
+
+static void read_input_file(struct strbuf *sb, const char *file)
+{
+ if (file) {
+ if (strbuf_read_file(sb, file, 0) < 0)
+ die_errno(_("could not read input file '%s'"), file);
+ } else {
+ if (strbuf_read(sb, fileno(stdin), 0) < 0)
+ die_errno(_("could not read from stdin"));
+ }
+}
+
+static void interpret_trailers(const struct process_trailer_options *opts,
+ struct list_head *new_trailer_head,
+ const char *file)
+{
+ LIST_HEAD(head);
+ struct strbuf sb = STRBUF_INIT;
+ struct trailer_info info;
+ FILE *outfile = stdout;
+
+ trailer_config_init();
+
+ read_input_file(&sb, file);
+
+ if (opts->in_place)
+ outfile = create_in_place_tempfile(file);
+
+ parse_trailers(opts, &info, sb.buf, &head);
+
+ /* Print the lines before the trailers */
+ if (!opts->only_trailers)
+ fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+
+ if (!opts->only_trailers && !info.blank_line_before_trailer)
+ fprintf(outfile, "\n");
+
+
+ if (!opts->only_input) {
+ LIST_HEAD(config_head);
+ LIST_HEAD(arg_head);
+ parse_trailers_from_config(&config_head);
+ parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
+ list_splice(&config_head, &arg_head);
+ process_trailers_lists(&head, &arg_head);
+ }
+
+ format_trailers(opts, &head, outfile);
+
+ free_trailers(&head);
+ trailer_info_release(&info);
+
+ /* Print the lines after the trailers as is */
+ if (!opts->only_trailers)
+ fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+
+ if (opts->in_place)
+ if (rename_tempfile(&trailers_tempfile, file))
+ die_errno(_("could not rename temporary file to %s"), file);
+
+ strbuf_release(&sb);
+}
+
int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
diff --git a/trailer.c b/trailer.c
index 66b6660f5a4..d3899195876 100644
--- a/trailer.c
+++ b/trailer.c
@@ -5,7 +5,6 @@
#include "string-list.h"
#include "run-command.h"
#include "commit.h"
-#include "tempfile.h"
#include "trailer.h"
#include "list.h"
/*
@@ -163,8 +162,8 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-static void format_trailers(const struct process_trailer_options *opts,
- struct list_head *trailers, FILE *outfile)
+void format_trailers(const struct process_trailer_options *opts,
+ struct list_head *trailers, FILE *outfile)
{
struct list_head *pos;
struct trailer_item *item;
@@ -366,8 +365,8 @@ static int find_same_and_apply_arg(struct list_head *head,
return 0;
}
-static void process_trailers_lists(struct list_head *head,
- struct list_head *arg_head)
+void process_trailers_lists(struct list_head *head,
+ struct list_head *arg_head)
{
struct list_head *pos, *p;
struct arg_item *arg_tok;
@@ -589,7 +588,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
return 0;
}
-static void trailer_config_init(void)
+void trailer_config_init(void)
{
if (configured)
return;
@@ -719,7 +718,7 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
list_add_tail(&new_item->list, arg_head);
}
-static void parse_trailers_from_config(struct list_head *config_head)
+void parse_trailers_from_config(struct list_head *config_head)
{
struct arg_item *item;
struct list_head *pos;
@@ -735,8 +734,8 @@ static void parse_trailers_from_config(struct list_head *config_head)
}
}
-static void parse_trailers_from_command_line_args(struct list_head *arg_head,
- struct list_head *new_trailer_head)
+void parse_trailers_from_command_line_args(struct list_head *arg_head,
+ struct list_head *new_trailer_head)
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
@@ -775,17 +774,6 @@ static void parse_trailers_from_command_line_args(struct list_head *arg_head,
free(cl_separators);
}
-static void read_input_file(struct strbuf *sb, const char *file)
-{
- if (file) {
- if (strbuf_read_file(sb, file, 0) < 0)
- die_errno(_("could not read input file '%s'"), file);
- } else {
- if (strbuf_read(sb, fileno(stdin), 0) < 0)
- die_errno(_("could not read from stdin"));
- }
-}
-
static const char *next_line(const char *str)
{
const char *nl = strchrnul(str, '\n');
@@ -1000,10 +988,10 @@ static void unfold_value(struct strbuf *val)
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
*/
-static void parse_trailers(struct trailer_info *info,
- const char *str,
- struct list_head *head,
- const struct process_trailer_options *opts)
+void parse_trailers(const struct process_trailer_options *opts,
+ struct trailer_info *info,
+ const char *str,
+ struct list_head *head)
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
@@ -1035,7 +1023,7 @@ static void parse_trailers(struct trailer_info *info,
}
}
-static void free_trailers(struct list_head *trailers)
+void free_trailers(struct list_head *trailers)
{
struct list_head *pos, *p;
list_for_each_safe(pos, p, trailers) {
@@ -1044,88 +1032,6 @@ static void free_trailers(struct list_head *trailers)
}
}
-static struct tempfile *trailers_tempfile;
-
-static FILE *create_in_place_tempfile(const char *file)
-{
- struct stat st;
- struct strbuf filename_template = STRBUF_INIT;
- const char *tail;
- FILE *outfile;
-
- if (stat(file, &st))
- die_errno(_("could not stat %s"), file);
- if (!S_ISREG(st.st_mode))
- die(_("file %s is not a regular file"), file);
- if (!(st.st_mode & S_IWUSR))
- die(_("file %s is not writable by user"), file);
-
- /* Create temporary file in the same directory as the original */
- tail = strrchr(file, '/');
- if (tail)
- strbuf_add(&filename_template, file, tail - file + 1);
- strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
-
- trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
- strbuf_release(&filename_template);
- outfile = fdopen_tempfile(trailers_tempfile, "w");
- if (!outfile)
- die_errno(_("could not open temporary file"));
-
- return outfile;
-}
-
-void interpret_trailers(const struct process_trailer_options *opts,
- struct list_head *new_trailer_head,
- const char *file)
-{
- LIST_HEAD(head);
- struct strbuf sb = STRBUF_INIT;
- struct trailer_info info;
- FILE *outfile = stdout;
-
- trailer_config_init();
-
- read_input_file(&sb, file);
-
- if (opts->in_place)
- outfile = create_in_place_tempfile(file);
-
- parse_trailers(&info, sb.buf, &head, opts);
-
- /* Print the lines before the trailers */
- if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_block_start, outfile);
-
- if (!opts->only_trailers && !info.blank_line_before_trailer)
- fprintf(outfile, "\n");
-
-
- if (!opts->only_input) {
- LIST_HEAD(config_head);
- LIST_HEAD(arg_head);
- parse_trailers_from_config(&config_head);
- parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
- list_splice(&config_head, &arg_head);
- process_trailers_lists(&head, &arg_head);
- }
-
- format_trailers(opts, &head, outfile);
-
- free_trailers(&head);
- trailer_info_release(&info);
-
- /* Print the lines after the trailers as is */
- if (!opts->only_trailers)
- fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
-
- if (opts->in_place)
- if (rename_tempfile(&trailers_tempfile, file))
- die_errno(_("could not rename temporary file to %s"), file);
-
- strbuf_release(&sb);
-}
-
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts)
{
diff --git a/trailer.h b/trailer.h
index 37033e631a1..ca701c04f3b 100644
--- a/trailer.h
+++ b/trailer.h
@@ -81,15 +81,29 @@ struct process_trailer_options {
#define PROCESS_TRAILER_OPTIONS_INIT {0}
-void interpret_trailers(const struct process_trailer_options *opts,
- struct list_head *new_trailer_head,
- const char *file);
+void parse_trailers_from_config(struct list_head *config_head);
+
+void parse_trailers_from_command_line_args(struct list_head *arg_head,
+ struct list_head *new_trailer_head);
+
+void process_trailers_lists(struct list_head *head,
+ struct list_head *arg_head);
+
+void parse_trailers(const struct process_trailer_options *opts,
+ struct trailer_info *info,
+ const char *str,
+ struct list_head *head);
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts);
void trailer_info_release(struct trailer_info *info);
+void trailer_config_init(void);
+void format_trailers(const struct process_trailer_options *opts,
+ struct list_head *trailers, FILE *outfile);
+void free_trailers(struct list_head *);
+
/*
* Format the trailers from the commit msg "msg" into the strbuf "out".
* Note two caveats about "opts":
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 03/10] trailer: unify trailer formatting machinery
From: Linus Arver via GitGitGadget @ 2024-01-31 1:22 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v3.git.1706664144.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Currently have two functions for formatting trailers exposed in
trailer.h:
void format_trailers(FILE *outfile, struct list_head *head,
const struct process_trailer_options *opts);
void format_trailers_from_commit(struct strbuf *out, const char *msg,
const struct process_trailer_options *opts);
and previously these functions, although similar enough (even taking the
same process_trailer_options struct pointer), did not build on each
other.
Make format_trailers_from_commit() rely on format_trailers(). Teach
format_trailers() to process trailers with the additional
process_trailer_options fields like opts->key_only which is only used by
format_trailers_from_commit() and not builtin/interpret-trailers.c.
While we're at it, reorder parameters to put the trailer processing
options first, and the out parameter (strbuf we write into) at the end.
This unification will allow us to delete the format_trailer_info() and
print_tok_val() functions in the next patch. They are not deleted here
in order to keep the diff small.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 6 +-
pretty.c | 2 +-
ref-filter.c | 2 +-
trailer.c | 176 +++++++++++++++++------------------
trailer.h | 19 ++--
5 files changed, 98 insertions(+), 107 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 8556acde4aa..5352ee65bd1 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -140,6 +140,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
{
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
+ struct strbuf trailer_block = STRBUF_INIT;
struct trailer_info info;
FILE *outfile = stdout;
@@ -169,7 +170,10 @@ static void interpret_trailers(const struct process_trailer_options *opts,
process_trailers_lists(&head, &arg_head);
}
- format_trailers(opts, &head, outfile);
+ /* Print trailer block. */
+ format_trailers(opts, &head, &trailer_block);
+ fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
+ strbuf_release(&trailer_block);
free_trailers(&head);
trailer_info_release(&info);
diff --git a/pretty.c b/pretty.c
index cf964b060cd..bdbed4295aa 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1759,7 +1759,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
goto trailer_out;
}
if (*arg == ')') {
- format_trailers_from_commit(sb, msg + c->subject_off, &opts);
+ format_trailers_from_commit(&opts, msg + c->subject_off, sb);
ret = arg - placeholder + 1;
}
trailer_out:
diff --git a/ref-filter.c b/ref-filter.c
index 35b989e1dfe..d358953b0ce 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1985,7 +1985,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
struct strbuf s = STRBUF_INIT;
/* Format the trailer info according to the trailer_opts given */
- format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts);
+ format_trailers_from_commit(&atom->u.contents.trailer_opts, subpos, &s);
v->s = strbuf_detach(&s, NULL);
} else if (atom->u.contents.option == C_BARE)
diff --git a/trailer.c b/trailer.c
index d3899195876..71ea2bb67f8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -144,37 +144,6 @@ static char last_non_space_char(const char *s)
return '\0';
}
-static void print_tok_val(FILE *outfile, const char *tok, const char *val)
-{
- char c;
-
- if (!tok) {
- fprintf(outfile, "%s\n", val);
- return;
- }
-
- c = last_non_space_char(tok);
- if (!c)
- return;
- if (strchr(separators, c))
- fprintf(outfile, "%s%s\n", tok, val);
- else
- fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
-}
-
-void format_trailers(const struct process_trailer_options *opts,
- struct list_head *trailers, FILE *outfile)
-{
- struct list_head *pos;
- struct trailer_item *item;
- list_for_each(pos, trailers) {
- item = list_entry(pos, struct trailer_item, list);
- if ((!opts->trim_empty || strlen(item->value) > 0) &&
- (!opts->only_trailers || item->token))
- print_tok_val(outfile, item->token, item->value);
- }
-}
-
static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
{
struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
@@ -984,6 +953,78 @@ static void unfold_value(struct strbuf *val)
strbuf_release(&out);
}
+void format_trailers(const struct process_trailer_options *opts,
+ struct list_head *trailers,
+ struct strbuf *out)
+{
+ struct list_head *pos;
+ struct trailer_item *item;
+ int need_separator = 0;
+
+ list_for_each(pos, trailers) {
+ item = list_entry(pos, struct trailer_item, list);
+ if (item->token) {
+ char c;
+
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ strbuf_addstr(&tok, item->token);
+ strbuf_addstr(&val, item->value);
+
+ /*
+ * Skip key/value pairs where the value was empty. This
+ * can happen from trailers specified without a
+ * separator, like `--trailer "Reviewed-by"` (no
+ * corresponding value).
+ */
+ if (opts->trim_empty && !strlen(item->value))
+ continue;
+
+ if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
+ if (opts->unfold)
+ unfold_value(&val);
+
+ if (opts->separator && need_separator)
+ strbuf_addbuf(out, opts->separator);
+ if (!opts->value_only)
+ strbuf_addbuf(out, &tok);
+ if (!opts->key_only && !opts->value_only) {
+ if (opts->key_value_separator)
+ strbuf_addbuf(out, opts->key_value_separator);
+ else {
+ c = last_non_space_char(tok.buf);
+ if (c) {
+ if (!strchr(separators, c))
+ strbuf_addf(out, "%c ", separators[0]);
+ }
+ }
+ }
+ if (!opts->key_only)
+ strbuf_addbuf(out, &val);
+ if (!opts->separator)
+ strbuf_addch(out, '\n');
+
+ need_separator = 1;
+ }
+
+ strbuf_release(&tok);
+ strbuf_release(&val);
+ } else if (!opts->only_trailers) {
+ if (opts->separator && need_separator) {
+ strbuf_addbuf(out, opts->separator);
+ }
+ strbuf_addstr(out, item->value);
+ if (opts->separator)
+ strbuf_rtrim(out);
+ else
+ strbuf_addch(out, '\n');
+
+ need_separator = 1;
+ }
+
+ }
+}
+
/*
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
@@ -1083,74 +1124,25 @@ void trailer_info_release(struct trailer_info *info)
free(info->trailers);
}
-static void format_trailer_info(struct strbuf *out,
- const struct trailer_info *info,
- const char *msg,
- const struct process_trailer_options *opts)
+void format_trailers_from_commit(const struct process_trailer_options *opts,
+ const char *msg,
+ struct strbuf *out)
{
- size_t origlen = out->len;
- size_t i;
+ LIST_HEAD(head);
+ struct trailer_info info;
+
+ parse_trailers(opts, &info, msg, &head);
/* If we want the whole block untouched, we can take the fast path. */
if (!opts->only_trailers && !opts->unfold && !opts->filter &&
!opts->separator && !opts->key_only && !opts->value_only &&
!opts->key_value_separator) {
- strbuf_add(out, msg + info->trailer_block_start,
- info->trailer_block_end - info->trailer_block_start);
- return;
- }
-
- for (i = 0; i < info->trailer_nr; i++) {
- char *trailer = info->trailers[i];
- ssize_t separator_pos = find_separator(trailer, separators);
-
- if (separator_pos >= 1) {
- struct strbuf tok = STRBUF_INIT;
- struct strbuf val = STRBUF_INIT;
-
- parse_trailer(&tok, &val, NULL, trailer, separator_pos);
- if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
- if (opts->unfold)
- unfold_value(&val);
-
- if (opts->separator && out->len != origlen)
- strbuf_addbuf(out, opts->separator);
- if (!opts->value_only)
- strbuf_addbuf(out, &tok);
- if (!opts->key_only && !opts->value_only) {
- if (opts->key_value_separator)
- strbuf_addbuf(out, opts->key_value_separator);
- else
- strbuf_addstr(out, ": ");
- }
- if (!opts->key_only)
- strbuf_addbuf(out, &val);
- if (!opts->separator)
- strbuf_addch(out, '\n');
- }
- strbuf_release(&tok);
- strbuf_release(&val);
-
- } else if (!opts->only_trailers) {
- if (opts->separator && out->len != origlen) {
- strbuf_addbuf(out, opts->separator);
- }
- strbuf_addstr(out, trailer);
- if (opts->separator) {
- strbuf_rtrim(out);
- }
- }
- }
-
-}
-
-void format_trailers_from_commit(struct strbuf *out, const char *msg,
- const struct process_trailer_options *opts)
-{
- struct trailer_info info;
+ strbuf_add(out, msg + info.trailer_block_start,
+ info.trailer_block_end - info.trailer_block_start);
+ } else
+ format_trailers(opts, &head, out);
- trailer_info_get(&info, msg, opts);
- format_trailer_info(out, &info, msg, opts);
+ free_trailers(&head);
trailer_info_release(&info);
}
diff --git a/trailer.h b/trailer.h
index ca701c04f3b..244f29fc91f 100644
--- a/trailer.h
+++ b/trailer.h
@@ -101,22 +101,17 @@ void trailer_info_release(struct trailer_info *info);
void trailer_config_init(void);
void format_trailers(const struct process_trailer_options *opts,
- struct list_head *trailers, FILE *outfile);
+ struct list_head *trailers,
+ struct strbuf *out);
void free_trailers(struct list_head *);
/*
- * Format the trailers from the commit msg "msg" into the strbuf "out".
- * Note two caveats about "opts":
- *
- * - this is primarily a helper for pretty.c, and not
- * all of the flags are supported.
- *
- * - this differs from process_trailers slightly in that we always format
- * only the trailer block itself, even if the "only_trailers" option is not
- * set.
+ * Convenience function to format the trailers from the commit msg "msg" into
+ * the strbuf "out". Reuses format_trailers internally.
*/
-void format_trailers_from_commit(struct strbuf *out, const char *msg,
- const struct process_trailer_options *opts);
+void format_trailers_from_commit(const struct process_trailer_options *opts,
+ const char *msg,
+ struct strbuf *out);
/*
* An interface for iterating over the trailers found in a particular commit
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 04/10] sequencer: use the trailer iterator
From: Linus Arver via GitGitGadget @ 2024-01-31 1:22 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v3.git.1706664144.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
This patch allows for the removal of "trailer_info_get()" from the
trailer.h API, which will be in the next patch.
Instead of calling "trailer_info_get()", which is a low-level function
in the trailers implementation (trailer.c), call
trailer_iterator_advance(), which was specifically designed for public
consumption in f0939a0eb1 (trailer: add interface for iterating over
commit trailers, 2020-09-27).
Avoiding "trailer_info_get()" means we don't have to worry about options
like "no_divider" (relevant for parsing trailers). We also don't have to
check for things like "info.trailer_start == info.trailer_end" to see
whether there were any trailers (instead we can just check to see
whether the iterator advanced at all).
Also, teach the iterator about non-trailer lines, by adding a new field
called "raw" to hold both trailer and non-trailer lines. This is
necessary because a "trailer block" is a list of trailer lines of at
least 25% trailers (see 146245063e (trailer: allow non-trailers in
trailer block, 2016-10-21)), such that it may hold non-trailer lines.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/shortlog.c | 7 +++++--
sequencer.c | 35 +++++++++++++++--------------------
trailer.c | 17 +++++++++--------
trailer.h | 13 +++++++++++++
4 files changed, 42 insertions(+), 30 deletions(-)
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 1307ed2b88a..dc8fd5a5532 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -172,7 +172,7 @@ static void insert_records_from_trailers(struct shortlog *log,
const char *oneline)
{
struct trailer_iterator iter;
- const char *commit_buffer, *body;
+ const char *commit_buffer, *body, *value;
struct strbuf ident = STRBUF_INIT;
if (!log->trailers.nr)
@@ -190,7 +190,10 @@ static void insert_records_from_trailers(struct shortlog *log,
trailer_iterator_init(&iter, body);
while (trailer_iterator_advance(&iter)) {
- const char *value = iter.val.buf;
+ if (!iter.is_trailer)
+ continue;
+
+ value = iter.val.buf;
if (!string_list_has_string(&log->trailers, iter.key.buf))
continue;
diff --git a/sequencer.c b/sequencer.c
index 3cc88d8a800..bc7c82c5271 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -319,37 +319,32 @@ static const char *get_todo_path(const struct replay_opts *opts)
static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
size_t ignore_footer)
{
- struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
- struct trailer_info info;
- size_t i;
- int found_sob = 0, found_sob_last = 0;
- char saved_char;
-
- opts.no_divider = 1;
+ struct trailer_iterator iter;
+ size_t i = 0, found_sob = 0;
+ char saved_char = sb->buf[sb->len - ignore_footer];
if (ignore_footer) {
- saved_char = sb->buf[sb->len - ignore_footer];
sb->buf[sb->len - ignore_footer] = '\0';
}
- trailer_info_get(&info, sb->buf, &opts);
+ trailer_iterator_init(&iter, sb->buf);
+ while (trailer_iterator_advance(&iter)) {
+ i++;
+ if (sob &&
+ iter.is_trailer &&
+ !strncmp(iter.raw, sob->buf, sob->len)) {
+ found_sob = i;
+ }
+ }
+ trailer_iterator_release(&iter);
if (ignore_footer)
sb->buf[sb->len - ignore_footer] = saved_char;
- if (info.trailer_block_start == info.trailer_block_end)
+ if (!i)
return 0;
- for (i = 0; i < info.trailer_nr; i++)
- if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
- found_sob = 1;
- if (i == info.trailer_nr - 1)
- found_sob_last = 1;
- }
-
- trailer_info_release(&info);
-
- if (found_sob_last)
+ if (found_sob == i)
return 3;
if (found_sob)
return 2;
diff --git a/trailer.c b/trailer.c
index 71ea2bb67f8..5bcc9b0006c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1158,17 +1158,18 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
int trailer_iterator_advance(struct trailer_iterator *iter)
{
- while (iter->internal.cur < iter->internal.info.trailer_nr) {
- char *trailer = iter->internal.info.trailers[iter->internal.cur++];
- int separator_pos = find_separator(trailer, separators);
-
- if (separator_pos < 1)
- continue; /* not a real trailer */
-
+ char *line;
+ int separator_pos;
+ if (iter->internal.cur < iter->internal.info.trailer_nr) {
+ line = iter->internal.info.trailers[iter->internal.cur++];
+ separator_pos = find_separator(line, separators);
+ iter->is_trailer = (separator_pos > 0);
+
+ iter->raw = line;
strbuf_reset(&iter->key);
strbuf_reset(&iter->val);
parse_trailer(&iter->key, &iter->val, NULL,
- trailer, separator_pos);
+ line, separator_pos);
unfold_value(&iter->val);
return 1;
}
diff --git a/trailer.h b/trailer.h
index 244f29fc91f..a7599067acc 100644
--- a/trailer.h
+++ b/trailer.h
@@ -127,6 +127,19 @@ struct trailer_iterator {
struct strbuf key;
struct strbuf val;
+ /*
+ * Raw line (e.g., "foo: bar baz") before being parsed as a trailer
+ * key/val pair as part of a trailer block. A trailer block can be
+ * either 100% trailer lines, or mixed in with non-trailer lines (in
+ * which case at least 25% must be trailer lines).
+ */
+ const char *raw;
+
+ /*
+ * 1 if the raw line was parsed as a trailer line (key/val pair).
+ */
+ int is_trailer;
+
/* private */
struct {
struct trailer_info info;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 05/10] trailer: make trailer_info struct private
From: Linus Arver via GitGitGadget @ 2024-01-31 1:22 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v3.git.1706664144.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
In 13211ae23f (trailer: separate public from internal portion of
trailer_iterator, 2023-09-09) we moved trailer_info behind an anonymous
struct to discourage use by trailer.h API users. However it still left
open the possibility of external use of trailer_info itself. Now that
there are no external users of trailer_info, we can make this struct
private.
Make this struct private by putting its definition inside trailer.c.
This has two benefits:
(1) it makes the surface area of the public facing
interface (trailer.h) smaller, and
(2) external API users are unable to peer inside this struct (because
it is only ever exposed as an opaque pointer).
This change exposes some deficiencies in the API, mainly with regard to
information about the location of the trailer block that was parsed.
Expose new API functions to access this information (needed by
builtin/interpret-trailers.c).
The idea in this patch to hide implementation details behind an "opaque
pointer" is also known as the "pimpl" (pointer to implementation) idiom
in C++ and is a common pattern in that language (where, for example,
abstract classes only have pointers to concrete classes).
However, the original inspiration to use this idiom does not come from
C++, but instead the book "C Interfaces and Implementations: Techniques
for Creating Reusable Software" [1]. This book recommends opaque
pointers as a good design principle for designing C libraries, using the
term "interface" as the functions defined in *.h (header) files and
"implementation" as the corresponding *.c file which define the
interfaces.
The book says this about opaque pointers:
... clients can manipulate such pointers freely, but they can’t
dereference them; that is, they can’t look at the innards of the
structure pointed to by them. Only the implementation has that
privilege. Opaque pointers hide representation details and help
catch errors.
In our case, "struct trailer_info" is now hidden from clients, and the
ways in which this opaque pointer can be used is limited to the richness
of the trailer.h file. In other words, trailer.h exclusively controls
exactly how "trailer_info" pointers are to be used.
[1] Hanson, David R. "C Interfaces and Implementations: Techniques for
Creating Reusable Software". Addison Wesley, 1997. p. 22
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 13 +--
trailer.c | 154 +++++++++++++++++++++++------------
trailer.h | 37 ++-------
3 files changed, 117 insertions(+), 87 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 5352ee65bd1..9e6ed6b65e2 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -141,7 +141,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
struct strbuf trailer_block = STRBUF_INIT;
- struct trailer_info info;
+ struct trailer_info *info;
FILE *outfile = stdout;
trailer_config_init();
@@ -151,13 +151,13 @@ static void interpret_trailers(const struct process_trailer_options *opts,
if (opts->in_place)
outfile = create_in_place_tempfile(file);
- parse_trailers(opts, &info, sb.buf, &head);
+ info = parse_trailers(opts, sb.buf, &head);
/* Print the lines before the trailers */
if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+ fwrite(sb.buf, 1, trailer_block_start(info), outfile);
- if (!opts->only_trailers && !info.blank_line_before_trailer)
+ if (!opts->only_trailers && !blank_line_before_trailer_block(info))
fprintf(outfile, "\n");
@@ -176,11 +176,12 @@ static void interpret_trailers(const struct process_trailer_options *opts,
strbuf_release(&trailer_block);
free_trailers(&head);
- trailer_info_release(&info);
/* Print the lines after the trailers as is */
if (!opts->only_trailers)
- fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+ fwrite(sb.buf + trailer_block_end(info), 1, sb.len - trailer_block_end(info), outfile);
+
+ trailer_info_release(info);
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
diff --git a/trailer.c b/trailer.c
index 5bcc9b0006c..63774cd068d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -11,6 +11,27 @@
* Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
*/
+struct trailer_info {
+ /*
+ * True if there is a blank line before the location pointed to by
+ * trailer_block_start.
+ */
+ int blank_line_before_trailer;
+
+ /*
+ * Offsets to the trailer block start and end positions in the input
+ * string. If no trailer block is found, these are both set to the
+ * "true" end of the input (find_end_of_log_message()).
+ */
+ size_t trailer_block_start, trailer_block_end;
+
+ /*
+ * Array of trailers found.
+ */
+ char **trailers;
+ size_t trailer_nr;
+};
+
struct conf_info {
char *name;
char *key;
@@ -1025,20 +1046,72 @@ void format_trailers(const struct process_trailer_options *opts,
}
}
+static struct trailer_info *trailer_info_new(void)
+{
+ struct trailer_info *info = xcalloc(1, sizeof(*info));
+ return info;
+}
+
+static struct trailer_info *trailer_info_get(const struct process_trailer_options *opts,
+ const char *str)
+{
+ struct trailer_info *info = trailer_info_new();
+ size_t end_of_log_message = 0, trailer_block_start = 0;
+ struct strbuf **trailer_lines, **ptr;
+ char **trailer_strings = NULL;
+ size_t nr = 0, alloc = 0;
+ char **last = NULL;
+
+ trailer_config_init();
+
+ end_of_log_message = find_end_of_log_message(str, opts->no_divider);
+ trailer_block_start = find_trailer_block_start(str, end_of_log_message);
+
+ trailer_lines = strbuf_split_buf(str + trailer_block_start,
+ end_of_log_message - trailer_block_start,
+ '\n',
+ 0);
+ for (ptr = trailer_lines; *ptr; ptr++) {
+ if (last && isspace((*ptr)->buf[0])) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
+ strbuf_addbuf(&sb, *ptr);
+ *last = strbuf_detach(&sb, NULL);
+ continue;
+ }
+ ALLOC_GROW(trailer_strings, nr + 1, alloc);
+ trailer_strings[nr] = strbuf_detach(*ptr, NULL);
+ last = find_separator(trailer_strings[nr], separators) >= 1
+ ? &trailer_strings[nr]
+ : NULL;
+ nr++;
+ }
+ strbuf_list_free(trailer_lines);
+
+ info->blank_line_before_trailer = ends_with_blank_line(str,
+ trailer_block_start);
+ info->trailer_block_start = trailer_block_start;
+ info->trailer_block_end = end_of_log_message;
+ info->trailers = trailer_strings;
+ info->trailer_nr = nr;
+
+ return info;
+}
+
/*
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
*/
-void parse_trailers(const struct process_trailer_options *opts,
- struct trailer_info *info,
- const char *str,
- struct list_head *head)
+struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
+ const char *str,
+ struct list_head *head)
{
+ struct trailer_info *info;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
size_t i;
- trailer_info_get(info, str, opts);
+ info = trailer_info_get(opts, str);
for (i = 0; i < info->trailer_nr; i++) {
int separator_pos;
@@ -1062,6 +1135,8 @@ void parse_trailers(const struct process_trailer_options *opts,
strbuf_detach(&val, NULL));
}
}
+
+ return info;
}
void free_trailers(struct list_head *trailers)
@@ -1073,47 +1148,19 @@ void free_trailers(struct list_head *trailers)
}
}
-void trailer_info_get(struct trailer_info *info, const char *str,
- const struct process_trailer_options *opts)
+size_t trailer_block_start(struct trailer_info *info)
{
- size_t end_of_log_message = 0, trailer_block_start = 0;
- struct strbuf **trailer_lines, **ptr;
- char **trailer_strings = NULL;
- size_t nr = 0, alloc = 0;
- char **last = NULL;
-
- trailer_config_init();
-
- end_of_log_message = find_end_of_log_message(str, opts->no_divider);
- trailer_block_start = find_trailer_block_start(str, end_of_log_message);
+ return info->trailer_block_start;
+}
- trailer_lines = strbuf_split_buf(str + trailer_block_start,
- end_of_log_message - trailer_block_start,
- '\n',
- 0);
- for (ptr = trailer_lines; *ptr; ptr++) {
- if (last && isspace((*ptr)->buf[0])) {
- struct strbuf sb = STRBUF_INIT;
- strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
- strbuf_addbuf(&sb, *ptr);
- *last = strbuf_detach(&sb, NULL);
- continue;
- }
- ALLOC_GROW(trailer_strings, nr + 1, alloc);
- trailer_strings[nr] = strbuf_detach(*ptr, NULL);
- last = find_separator(trailer_strings[nr], separators) >= 1
- ? &trailer_strings[nr]
- : NULL;
- nr++;
- }
- strbuf_list_free(trailer_lines);
+size_t trailer_block_end(struct trailer_info *info)
+{
+ return info->trailer_block_end;
+}
- info->blank_line_before_trailer = ends_with_blank_line(str,
- trailer_block_start);
- info->trailer_block_start = trailer_block_start;
- info->trailer_block_end = end_of_log_message;
- info->trailers = trailer_strings;
- info->trailer_nr = nr;
+int blank_line_before_trailer_block(struct trailer_info *info)
+{
+ return info->blank_line_before_trailer;
}
void trailer_info_release(struct trailer_info *info)
@@ -1122,6 +1169,7 @@ void trailer_info_release(struct trailer_info *info)
for (i = 0; i < info->trailer_nr; i++)
free(info->trailers[i]);
free(info->trailers);
+ free(info);
}
void format_trailers_from_commit(const struct process_trailer_options *opts,
@@ -1129,30 +1177,30 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
struct strbuf *out)
{
LIST_HEAD(head);
- struct trailer_info info;
-
- parse_trailers(opts, &info, msg, &head);
+ struct trailer_info *info = parse_trailers(opts, msg, &head);
/* If we want the whole block untouched, we can take the fast path. */
if (!opts->only_trailers && !opts->unfold && !opts->filter &&
!opts->separator && !opts->key_only && !opts->value_only &&
!opts->key_value_separator) {
- strbuf_add(out, msg + info.trailer_block_start,
- info.trailer_block_end - info.trailer_block_start);
+ strbuf_add(out, msg + info->trailer_block_start,
+ info->trailer_block_end - info->trailer_block_start);
} else
format_trailers(opts, &head, out);
free_trailers(&head);
- trailer_info_release(&info);
+ trailer_info_release(info);
}
void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+ struct trailer_info *internal = trailer_info_new();
strbuf_init(&iter->key, 0);
strbuf_init(&iter->val, 0);
opts.no_divider = 1;
- trailer_info_get(&iter->internal.info, msg, &opts);
+ iter->internal.info = internal;
+ iter->internal.info = trailer_info_get(&opts, msg);
iter->internal.cur = 0;
}
@@ -1160,8 +1208,8 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
{
char *line;
int separator_pos;
- if (iter->internal.cur < iter->internal.info.trailer_nr) {
- line = iter->internal.info.trailers[iter->internal.cur++];
+ if (iter->internal.cur < iter->internal.info->trailer_nr) {
+ line = iter->internal.info->trailers[iter->internal.cur++];
separator_pos = find_separator(line, separators);
iter->is_trailer = (separator_pos > 0);
@@ -1178,7 +1226,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
void trailer_iterator_release(struct trailer_iterator *iter)
{
- trailer_info_release(&iter->internal.info);
+ trailer_info_release(iter->internal.info);
strbuf_release(&iter->val);
strbuf_release(&iter->key);
}
diff --git a/trailer.h b/trailer.h
index a7599067acc..e19ddf84e64 100644
--- a/trailer.h
+++ b/trailer.h
@@ -4,6 +4,8 @@
#include "list.h"
#include "strbuf.h"
+struct trailer_info;
+
enum trailer_where {
WHERE_DEFAULT,
WHERE_END,
@@ -29,27 +31,6 @@ int trailer_set_where(enum trailer_where *item, const char *value);
int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
-struct trailer_info {
- /*
- * True if there is a blank line before the location pointed to by
- * trailer_block_start.
- */
- int blank_line_before_trailer;
-
- /*
- * Offsets to the trailer block start and end positions in the input
- * string. If no trailer block is found, these are both set to the
- * "true" end of the input (find_end_of_log_message()).
- */
- size_t trailer_block_start, trailer_block_end;
-
- /*
- * Array of trailers found.
- */
- char **trailers;
- size_t trailer_nr;
-};
-
/*
* A list that represents newly-added trailers, such as those provided
* with the --trailer command line option of git-interpret-trailers.
@@ -89,13 +70,13 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
void process_trailers_lists(struct list_head *head,
struct list_head *arg_head);
-void parse_trailers(const struct process_trailer_options *opts,
- struct trailer_info *info,
- const char *str,
- struct list_head *head);
+struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
+ const char *str,
+ struct list_head *head);
-void trailer_info_get(struct trailer_info *info, const char *str,
- const struct process_trailer_options *opts);
+size_t trailer_block_start(struct trailer_info *info);
+size_t trailer_block_end(struct trailer_info *info);
+int blank_line_before_trailer_block(struct trailer_info *info);
void trailer_info_release(struct trailer_info *info);
@@ -142,7 +123,7 @@ struct trailer_iterator {
/* private */
struct {
- struct trailer_info info;
+ struct trailer_info *info;
size_t cur;
} internal;
};
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 06/10] trailer: spread usage of "trailer_block" language
From: Linus Arver via GitGitGadget @ 2024-01-31 1:22 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v3.git.1706664144.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Deprecate the "trailer_info" struct name and replace it with
"trailer_block". The main reason is to help readability, because
"trailer_info" on the surface sounds like it's about a single trailer
when in reality it is a collection of contiguous lines, at least 25% of
which are trailers.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 26 +++++-----
trailer.c | 99 ++++++++++++++++++------------------
trailer.h | 18 +++----
3 files changed, 71 insertions(+), 72 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 9e6ed6b65e2..9e41fa20b5f 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -140,8 +140,8 @@ static void interpret_trailers(const struct process_trailer_options *opts,
{
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
- struct strbuf trailer_block = STRBUF_INIT;
- struct trailer_info *info;
+ struct strbuf tb = STRBUF_INIT;
+ struct trailer_block *trailer_block;
FILE *outfile = stdout;
trailer_config_init();
@@ -151,13 +151,13 @@ static void interpret_trailers(const struct process_trailer_options *opts,
if (opts->in_place)
outfile = create_in_place_tempfile(file);
- info = parse_trailers(opts, sb.buf, &head);
+ trailer_block = parse_trailers(opts, sb.buf, &head);
- /* Print the lines before the trailers */
+ /* Print the lines before the trailer block */
if (!opts->only_trailers)
- fwrite(sb.buf, 1, trailer_block_start(info), outfile);
+ fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
- if (!opts->only_trailers && !blank_line_before_trailer_block(info))
+ if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
fprintf(outfile, "\n");
@@ -171,17 +171,17 @@ static void interpret_trailers(const struct process_trailer_options *opts,
}
/* Print trailer block. */
- format_trailers(opts, &head, &trailer_block);
- fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
- strbuf_release(&trailer_block);
+ format_trailers(opts, &head, &tb);
+ fwrite(tb.buf, 1, tb.len, outfile);
+ strbuf_release(&tb);
free_trailers(&head);
- /* Print the lines after the trailers as is */
+ /* Print the lines after the trailer block as is */
if (!opts->only_trailers)
- fwrite(sb.buf + trailer_block_end(info), 1, sb.len - trailer_block_end(info), outfile);
-
- trailer_info_release(info);
+ fwrite(sb.buf + trailer_block_end(trailer_block),
+ 1, sb.len - trailer_block_end(trailer_block), outfile);
+ trailer_block_release(trailer_block);
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
diff --git a/trailer.c b/trailer.c
index 63774cd068d..e2a48bea0ae 100644
--- a/trailer.c
+++ b/trailer.c
@@ -11,19 +11,20 @@
* Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
*/
-struct trailer_info {
+struct trailer_block {
/*
* True if there is a blank line before the location pointed to by
- * trailer_block_start.
+ * "start".
*/
int blank_line_before_trailer;
/*
- * Offsets to the trailer block start and end positions in the input
- * string. If no trailer block is found, these are both set to the
- * "true" end of the input (find_end_of_log_message()).
+ * The locations of the start and end positions of the trailer block
+ * found, as offsets from the beginning of the source text from which
+ * this trailer block was parsed. If no trailer block is found, these
+ * are both set to 0.
*/
- size_t trailer_block_start, trailer_block_end;
+ size_t start, end;
/*
* Array of trailers found.
@@ -1046,16 +1047,16 @@ void format_trailers(const struct process_trailer_options *opts,
}
}
-static struct trailer_info *trailer_info_new(void)
+static struct trailer_block *trailer_block_new(void)
{
- struct trailer_info *info = xcalloc(1, sizeof(*info));
- return info;
+ struct trailer_block *trailer_block = xcalloc(1, sizeof(*trailer_block));
+ return trailer_block;
}
-static struct trailer_info *trailer_info_get(const struct process_trailer_options *opts,
- const char *str)
+static struct trailer_block *trailer_block_get(const struct process_trailer_options *opts,
+ const char *str)
{
- struct trailer_info *info = trailer_info_new();
+ struct trailer_block *trailer_block = trailer_block_new();
size_t end_of_log_message = 0, trailer_block_start = 0;
struct strbuf **trailer_lines, **ptr;
char **trailer_strings = NULL;
@@ -1088,34 +1089,34 @@ static struct trailer_info *trailer_info_get(const struct process_trailer_option
}
strbuf_list_free(trailer_lines);
- info->blank_line_before_trailer = ends_with_blank_line(str,
- trailer_block_start);
- info->trailer_block_start = trailer_block_start;
- info->trailer_block_end = end_of_log_message;
- info->trailers = trailer_strings;
- info->trailer_nr = nr;
+ trailer_block->blank_line_before_trailer = ends_with_blank_line(str,
+ trailer_block_start);
+ trailer_block->start = trailer_block_start;
+ trailer_block->end = end_of_log_message;
+ trailer_block->trailers = trailer_strings;
+ trailer_block->trailer_nr = nr;
- return info;
+ return trailer_block;
}
/*
- * Parse trailers in "str", populating the trailer info and "head"
- * linked list structure.
+ * Parse trailers in "str", populating the trailer_block info and "head" linked
+ * list structure.
*/
-struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
- const char *str,
- struct list_head *head)
+struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
+ const char *str,
+ struct list_head *head)
{
- struct trailer_info *info;
+ struct trailer_block *trailer_block;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
size_t i;
- info = trailer_info_get(opts, str);
+ trailer_block = trailer_block_get(opts, str);
- for (i = 0; i < info->trailer_nr; i++) {
+ for (i = 0; i < trailer_block->trailer_nr; i++) {
int separator_pos;
- char *trailer = info->trailers[i];
+ char *trailer = trailer_block->trailers[i];
if (trailer[0] == comment_line_char)
continue;
separator_pos = find_separator(trailer, separators);
@@ -1136,7 +1137,7 @@ struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
}
}
- return info;
+ return trailer_block;
}
void free_trailers(struct list_head *trailers)
@@ -1148,28 +1149,28 @@ void free_trailers(struct list_head *trailers)
}
}
-size_t trailer_block_start(struct trailer_info *info)
+size_t trailer_block_start(struct trailer_block *trailer_block)
{
- return info->trailer_block_start;
+ return trailer_block->start;
}
-size_t trailer_block_end(struct trailer_info *info)
+size_t trailer_block_end(struct trailer_block *trailer_block)
{
- return info->trailer_block_end;
+ return trailer_block->end;
}
-int blank_line_before_trailer_block(struct trailer_info *info)
+int blank_line_before_trailer_block(struct trailer_block *trailer_block)
{
- return info->blank_line_before_trailer;
+ return trailer_block->blank_line_before_trailer;
}
-void trailer_info_release(struct trailer_info *info)
+void trailer_block_release(struct trailer_block *trailer_block)
{
size_t i;
- for (i = 0; i < info->trailer_nr; i++)
- free(info->trailers[i]);
- free(info->trailers);
- free(info);
+ for (i = 0; i < trailer_block->trailer_nr; i++)
+ free(trailer_block->trailers[i]);
+ free(trailer_block->trailers);
+ free(trailer_block);
}
void format_trailers_from_commit(const struct process_trailer_options *opts,
@@ -1177,30 +1178,28 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
struct strbuf *out)
{
LIST_HEAD(head);
- struct trailer_info *info = parse_trailers(opts, msg, &head);
+ struct trailer_block *trailer_block = parse_trailers(opts, msg, &head);
/* If we want the whole block untouched, we can take the fast path. */
if (!opts->only_trailers && !opts->unfold && !opts->filter &&
!opts->separator && !opts->key_only && !opts->value_only &&
!opts->key_value_separator) {
- strbuf_add(out, msg + info->trailer_block_start,
- info->trailer_block_end - info->trailer_block_start);
+ strbuf_add(out, msg + trailer_block->start,
+ trailer_block->end - trailer_block->start);
} else
format_trailers(opts, &head, out);
free_trailers(&head);
- trailer_info_release(info);
+ trailer_block_release(trailer_block);
}
void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
- struct trailer_info *internal = trailer_info_new();
strbuf_init(&iter->key, 0);
strbuf_init(&iter->val, 0);
opts.no_divider = 1;
- iter->internal.info = internal;
- iter->internal.info = trailer_info_get(&opts, msg);
+ iter->internal.trailer_block = trailer_block_get(&opts, msg);
iter->internal.cur = 0;
}
@@ -1208,8 +1207,8 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
{
char *line;
int separator_pos;
- if (iter->internal.cur < iter->internal.info->trailer_nr) {
- line = iter->internal.info->trailers[iter->internal.cur++];
+ if (iter->internal.cur < iter->internal.trailer_block->trailer_nr) {
+ line = iter->internal.trailer_block->trailers[iter->internal.cur++];
separator_pos = find_separator(line, separators);
iter->is_trailer = (separator_pos > 0);
@@ -1226,7 +1225,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
void trailer_iterator_release(struct trailer_iterator *iter)
{
- trailer_info_release(iter->internal.info);
+ trailer_block_release(iter->internal.trailer_block);
strbuf_release(&iter->val);
strbuf_release(&iter->key);
}
diff --git a/trailer.h b/trailer.h
index e19ddf84e64..e74f9189c0a 100644
--- a/trailer.h
+++ b/trailer.h
@@ -4,7 +4,7 @@
#include "list.h"
#include "strbuf.h"
-struct trailer_info;
+struct trailer_block;
enum trailer_where {
WHERE_DEFAULT,
@@ -70,15 +70,15 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
void process_trailers_lists(struct list_head *head,
struct list_head *arg_head);
-struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
- const char *str,
- struct list_head *head);
+struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
+ const char *str,
+ struct list_head *head);
-size_t trailer_block_start(struct trailer_info *info);
-size_t trailer_block_end(struct trailer_info *info);
-int blank_line_before_trailer_block(struct trailer_info *info);
+size_t trailer_block_start(struct trailer_block *trailer_block);
+size_t trailer_block_end(struct trailer_block *trailer_block);
+int blank_line_before_trailer_block(struct trailer_block *trailer_block);
-void trailer_info_release(struct trailer_info *info);
+void trailer_block_release(struct trailer_block *trailer_block);
void trailer_config_init(void);
void format_trailers(const struct process_trailer_options *opts,
@@ -123,7 +123,7 @@ struct trailer_iterator {
/* private */
struct {
- struct trailer_info *info;
+ struct trailer_block *trailer_block;
size_t cur;
} internal;
};
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 07/10] trailer: prepare to move parse_trailers_from_command_line_args() to builtin
From: Linus Arver via GitGitGadget @ 2024-01-31 1:22 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v3.git.1706664144.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Expose more functions in the trailer.h API, in preparation for moving
out
parse_trailers_from_command_line_args()
to interpret-trailer.c, because the trailer API should not be concerned
with command line arguments (as they have nothing to do with trailers
themselves). The interpret-trailers builtin is the only caller of this
function.
Rename add_arg_item() to trailer_add_arg_item() because it will have to
be exposed as an API function in the next patch. Rename
new_trailers_clear() to free_new_trailers() because it will be promoted
into an API function; the API already has free_trailers(), so using the
"free_*" naming style will keep it consistent. Also rename "conf_info"
to "trailer_conf" for readability, dropping the low-value "_info" suffix
as we did earlier in this series for "trailer_info" to "trailer_block".
Helped-by: Josh Steadmon <steadmon@google.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 6 +--
trailer.c | 86 ++++++++++++++++++------------------
trailer.h | 10 +++++
3 files changed, 55 insertions(+), 47 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 9e41fa20b5f..9f0ba39b317 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -45,7 +45,7 @@ static int option_parse_if_missing(const struct option *opt,
return trailer_set_if_missing(opt->value, arg);
}
-static void new_trailers_clear(struct list_head *trailers)
+static void free_new_trailers(struct list_head *trailers)
{
struct list_head *pos, *tmp;
struct new_trailer_item *item;
@@ -64,7 +64,7 @@ static int option_parse_trailer(const struct option *opt,
struct new_trailer_item *item;
if (unset) {
- new_trailers_clear(trailers);
+ free_new_trailers(trailers);
return 0;
}
@@ -238,7 +238,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
interpret_trailers(&opts, &trailers, NULL);
}
- new_trailers_clear(&trailers);
+ free_new_trailers(&trailers);
return 0;
}
diff --git a/trailer.c b/trailer.c
index e2a48bea0ae..c16f552b463 100644
--- a/trailer.c
+++ b/trailer.c
@@ -33,7 +33,7 @@ struct trailer_block {
size_t trailer_nr;
};
-struct conf_info {
+struct trailer_conf {
char *name;
char *key;
char *command;
@@ -43,7 +43,7 @@ struct conf_info {
enum trailer_if_missing if_missing;
};
-static struct conf_info default_conf_info;
+static struct trailer_conf default_trailer_conf;
struct trailer_item {
struct list_head list;
@@ -59,7 +59,7 @@ struct arg_item {
struct list_head list;
char *token;
char *value;
- struct conf_info conf;
+ struct trailer_conf conf;
};
static LIST_HEAD(conf_head);
@@ -210,7 +210,7 @@ static int check_if_different(struct trailer_item *in_tok,
return 1;
}
-static char *apply_command(struct conf_info *conf, const char *arg)
+static char *apply_command(struct trailer_conf *conf, const char *arg)
{
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
@@ -424,7 +424,8 @@ int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
return 0;
}
-static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
+void duplicate_trailer_conf(struct trailer_conf *dst,
+ const struct trailer_conf *src)
{
*dst = *src;
dst->name = xstrdup_or_null(src->name);
@@ -447,7 +448,7 @@ static struct arg_item *get_conf_item(const char *name)
/* Item does not already exists, create it */
CALLOC_ARRAY(item, 1);
- duplicate_conf(&item->conf, &default_conf_info);
+ duplicate_trailer_conf(&item->conf, &default_trailer_conf);
item->conf.name = xstrdup(name);
list_add_tail(&item->list, &conf_head);
@@ -482,17 +483,17 @@ static int git_trailer_default_config(const char *conf_key, const char *value,
variable_name = strrchr(trailer_item, '.');
if (!variable_name) {
if (!strcmp(trailer_item, "where")) {
- if (trailer_set_where(&default_conf_info.where,
+ if (trailer_set_where(&default_trailer_conf.where,
value) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifexists")) {
- if (trailer_set_if_exists(&default_conf_info.if_exists,
+ if (trailer_set_if_exists(&default_trailer_conf.if_exists,
value) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifmissing")) {
- if (trailer_set_if_missing(&default_conf_info.if_missing,
+ if (trailer_set_if_missing(&default_trailer_conf.if_missing,
value) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
@@ -511,7 +512,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
{
const char *trailer_item, *variable_name;
struct arg_item *item;
- struct conf_info *conf;
+ struct trailer_conf *conf;
char *name = NULL;
enum trailer_info_type type;
int i;
@@ -585,9 +586,9 @@ void trailer_config_init(void)
return;
/* Default config must be setup first */
- default_conf_info.where = WHERE_END;
- default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
- default_conf_info.if_missing = MISSING_ADD;
+ default_trailer_conf.where = WHERE_END;
+ default_trailer_conf.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+ default_trailer_conf.if_missing = MISSING_ADD;
git_config(git_trailer_default_config, NULL);
git_config(git_trailer_config, NULL);
configured = 1;
@@ -620,7 +621,7 @@ static int token_matches_item(const char *tok, struct arg_item *item, size_t tok
* distinguished from the non-well-formed-line case (in which this function
* returns -1) because some callers of this function need such a distinction.
*/
-static ssize_t find_separator(const char *line, const char *separators)
+ssize_t find_separator(const char *line, const char *separators)
{
int whitespace_found = 0;
const char *c;
@@ -645,28 +646,28 @@ static ssize_t find_separator(const char *line, const char *separators)
*
* If separator_pos is -1, interpret the whole trailer as a token.
*/
-static void parse_trailer(struct strbuf *tok, struct strbuf *val,
- const struct conf_info **conf, const char *trailer,
- ssize_t separator_pos)
+void parse_trailer(const char *line, ssize_t separator_pos,
+ struct strbuf *tok, struct strbuf *val,
+ const struct trailer_conf **conf)
{
struct arg_item *item;
size_t tok_len;
struct list_head *pos;
if (separator_pos != -1) {
- strbuf_add(tok, trailer, separator_pos);
+ strbuf_add(tok, line, separator_pos);
strbuf_trim(tok);
- strbuf_addstr(val, trailer + separator_pos + 1);
+ strbuf_addstr(val, line + separator_pos + 1);
strbuf_trim(val);
} else {
- strbuf_addstr(tok, trailer);
+ strbuf_addstr(tok, line);
strbuf_trim(tok);
}
/* Lookup if the token matches something in the config */
tok_len = token_len_without_separator(tok->buf, tok->len);
if (conf)
- *conf = &default_conf_info;
+ *conf = &default_trailer_conf;
list_for_each(pos, &conf_head) {
item = list_entry(pos, struct arg_item, list);
if (token_matches_item(tok->buf, item, tok_len)) {
@@ -690,14 +691,14 @@ static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
return new_item;
}
-static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
- const struct conf_info *conf,
- const struct new_trailer_item *new_trailer_item)
+static void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
+ const struct trailer_conf *conf,
+ const struct new_trailer_item *new_trailer_item)
{
struct arg_item *new_item = xcalloc(1, sizeof(*new_item));
new_item->token = tok;
new_item->value = val;
- duplicate_conf(&new_item->conf, conf);
+ duplicate_trailer_conf(&new_item->conf, conf);
if (new_trailer_item) {
if (new_trailer_item->where != WHERE_DEFAULT)
new_item->conf.where = new_trailer_item->where;
@@ -718,10 +719,10 @@ void parse_trailers_from_config(struct list_head *config_head)
list_for_each(pos, &conf_head) {
item = list_entry(pos, struct arg_item, list);
if (item->conf.command)
- add_arg_item(config_head,
- xstrdup(token_from_item(item, NULL)),
- xstrdup(""),
- &item->conf, NULL);
+ trailer_add_arg_item(config_head,
+ xstrdup(token_from_item(item, NULL)),
+ xstrdup(""),
+ &item->conf, NULL);
}
}
@@ -730,7 +731,7 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
- const struct conf_info *conf;
+ const struct trailer_conf *conf;
struct list_head *pos;
/*
@@ -753,12 +754,11 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
(int) sb.len, sb.buf);
strbuf_release(&sb);
} else {
- parse_trailer(&tok, &val, &conf, tr->text,
- separator_pos);
- add_arg_item(arg_head,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf, tr);
+ parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
+ trailer_add_arg_item(arg_head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf, tr);
}
}
@@ -1116,20 +1116,19 @@ struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
for (i = 0; i < trailer_block->trailer_nr; i++) {
int separator_pos;
- char *trailer = trailer_block->trailers[i];
- if (trailer[0] == comment_line_char)
+ char *line = trailer_block->trailers[i];
+ if (line[0] == comment_line_char)
continue;
- separator_pos = find_separator(trailer, separators);
+ separator_pos = find_separator(line, separators);
if (separator_pos >= 1) {
- parse_trailer(&tok, &val, NULL, trailer,
- separator_pos);
+ parse_trailer(line, separator_pos, &tok, &val, NULL);
if (opts->unfold)
unfold_value(&val);
add_trailer_item(head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL));
} else if (!opts->only_trailers) {
- strbuf_addstr(&val, trailer);
+ strbuf_addstr(&val, line);
strbuf_strip_suffix(&val, "\n");
add_trailer_item(head,
NULL,
@@ -1215,8 +1214,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
iter->raw = line;
strbuf_reset(&iter->key);
strbuf_reset(&iter->val);
- parse_trailer(&iter->key, &iter->val, NULL,
- line, separator_pos);
+ parse_trailer(line, separator_pos, &iter->key, &iter->val, NULL);
unfold_value(&iter->val);
return 1;
}
diff --git a/trailer.h b/trailer.h
index e74f9189c0a..d724263e4f6 100644
--- a/trailer.h
+++ b/trailer.h
@@ -5,6 +5,7 @@
#include "strbuf.h"
struct trailer_block;
+struct trailer_conf;
enum trailer_where {
WHERE_DEFAULT,
@@ -45,6 +46,9 @@ struct new_trailer_item {
enum trailer_if_missing if_missing;
};
+void duplicate_trailer_conf(struct trailer_conf *dst,
+ const struct trailer_conf *src);
+
struct process_trailer_options {
int in_place;
int trim_empty;
@@ -70,6 +74,12 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
void process_trailers_lists(struct list_head *head,
struct list_head *arg_head);
+ssize_t find_separator(const char *line, const char *separators);
+
+void parse_trailer(const char *line, ssize_t separator_pos,
+ struct strbuf *tok, struct strbuf *val,
+ const struct trailer_conf **conf);
+
struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
const char *str,
struct list_head *head);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 08/10] trailer: move arg handling to interpret-trailers.c
From: Linus Arver via GitGitGadget @ 2024-01-31 1:22 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v3.git.1706664144.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
We don't move the "arg_item" struct to interpret-trailers.c, because it
is now a struct that contains information about trailers that could be
added into the input text's own trailers. This is a generic concept that
extends beyond trailers defined as CLI arguments (it applies to trailers
defined in configuration as well). We will rename "arg_item" to
"trailer_template" in a follow-up patch to keep the diff here small.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 88 ++++++++++++++++++++++--------------
trailer.c | 62 ++++++++++++++++++-------
trailer.h | 12 +++++
3 files changed, 112 insertions(+), 50 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 9f0ba39b317..9a902012912 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -45,23 +45,17 @@ static int option_parse_if_missing(const struct option *opt,
return trailer_set_if_missing(opt->value, arg);
}
-static void free_new_trailers(struct list_head *trailers)
-{
- struct list_head *pos, *tmp;
- struct new_trailer_item *item;
-
- list_for_each_safe(pos, tmp, trailers) {
- item = list_entry(pos, struct new_trailer_item, list);
- list_del(pos);
- free(item);
- }
-}
+static char *cl_separators;
static int option_parse_trailer(const struct option *opt,
const char *arg, int unset)
{
struct list_head *trailers = opt->value;
- struct new_trailer_item *item;
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ const struct trailer_conf *conf;
+ struct trailer_conf *conf_current = new_trailer_conf();
+ ssize_t separator_pos;
if (unset) {
free_new_trailers(trailers);
@@ -71,12 +65,31 @@ static int option_parse_trailer(const struct option *opt,
if (!arg)
return -1;
- item = xmalloc(sizeof(*item));
- item->text = arg;
- item->where = where;
- item->if_exists = if_exists;
- item->if_missing = if_missing;
- list_add_tail(&item->list, trailers);
+ separator_pos = find_separator(arg, cl_separators);
+ if (separator_pos) {
+ parse_trailer(arg, separator_pos, &tok, &val, &conf);
+ duplicate_trailer_conf(conf_current, conf);
+
+ /*
+ * Override conf_current with settings specified via CLI flags.
+ */
+ trailer_conf_set(where, if_exists, if_missing, conf_current);
+
+ trailer_add_arg_item(strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf_current,
+ trailers);
+ } else {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addstr(&sb, arg);
+ strbuf_trim(&sb);
+ error(_("empty trailer token in trailer '%.*s'"),
+ (int) sb.len, sb.buf);
+ strbuf_release(&sb);
+ }
+
+ free(conf_current);
+
return 0;
}
@@ -135,7 +148,7 @@ static void read_input_file(struct strbuf *sb, const char *file)
}
static void interpret_trailers(const struct process_trailer_options *opts,
- struct list_head *new_trailer_head,
+ struct list_head *arg_trailers,
const char *file)
{
LIST_HEAD(head);
@@ -144,8 +157,6 @@ static void interpret_trailers(const struct process_trailer_options *opts,
struct trailer_block *trailer_block;
FILE *outfile = stdout;
- trailer_config_init();
-
read_input_file(&sb, file);
if (opts->in_place)
@@ -162,12 +173,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
if (!opts->only_input) {
- LIST_HEAD(config_head);
- LIST_HEAD(arg_head);
- parse_trailers_from_config(&config_head);
- parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
- list_splice(&config_head, &arg_head);
- process_trailers_lists(&head, &arg_head);
+ process_trailers_lists(&head, arg_trailers);
}
/* Print trailer block. */
@@ -193,7 +199,8 @@ static void interpret_trailers(const struct process_trailer_options *opts,
int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
- LIST_HEAD(trailers);
+ LIST_HEAD(configured_trailers);
+ LIST_HEAD(arg_trailers);
struct option options[] = {
OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
@@ -212,33 +219,48 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
- OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
+ OPT_CALLBACK(0, "trailer", &arg_trailers, N_("trailer"),
N_("trailer(s) to add"), option_parse_trailer),
OPT_END()
};
git_config(git_default_config, NULL);
+ trailer_config_init();
+
+ if (!opts.only_input) {
+ parse_trailers_from_config(&configured_trailers);
+ }
+
+ /*
+ * In command-line arguments, '=' is accepted (in addition to the
+ * separators that are defined).
+ */
+ cl_separators = xstrfmt("=%s", trailer_default_separators());
argc = parse_options(argc, argv, prefix, options,
git_interpret_trailers_usage, 0);
- if (opts.only_input && !list_empty(&trailers))
+ free(cl_separators);
+
+ if (opts.only_input && !list_empty(&arg_trailers))
usage_msg_opt(
_("--trailer with --only-input does not make sense"),
git_interpret_trailers_usage,
options);
+ list_splice(&configured_trailers, &arg_trailers);
+
if (argc) {
int i;
for (i = 0; i < argc; i++)
- interpret_trailers(&opts, &trailers, argv[i]);
+ interpret_trailers(&opts, &arg_trailers, argv[i]);
} else {
if (opts.in_place)
die(_("no input file given for in-place editing"));
- interpret_trailers(&opts, &trailers, NULL);
+ interpret_trailers(&opts, &arg_trailers, NULL);
}
- free_new_trailers(&trailers);
+ free_new_trailers(&arg_trailers);
return 0;
}
diff --git a/trailer.c b/trailer.c
index c16f552b463..19637ff295d 100644
--- a/trailer.c
+++ b/trailer.c
@@ -66,6 +66,11 @@ static LIST_HEAD(conf_head);
static char *separators = ":";
+const char *trailer_default_separators(void)
+{
+ return separators;
+}
+
static int configured;
#define TRAILER_ARG_STRING "$ARG"
@@ -424,6 +429,25 @@ int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
return 0;
}
+void trailer_conf_set(enum trailer_where where,
+ enum trailer_if_exists if_exists,
+ enum trailer_if_missing if_missing,
+ struct trailer_conf *conf)
+{
+ if (where != WHERE_DEFAULT)
+ conf->where = where;
+ if (if_exists != EXISTS_DEFAULT)
+ conf->if_exists = if_exists;
+ if (if_missing != MISSING_DEFAULT)
+ conf->if_missing = if_missing;
+}
+
+struct trailer_conf *new_trailer_conf(void)
+{
+ struct trailer_conf *new = xcalloc(1, sizeof(*new));
+ return new;
+}
+
void duplicate_trailer_conf(struct trailer_conf *dst,
const struct trailer_conf *src)
{
@@ -642,6 +666,9 @@ ssize_t find_separator(const char *line, const char *separators)
/*
* Obtain the token, value, and conf from the given trailer.
*
+ * The conf needs special handling. We first read hardcoded defaults, and
+ * override them if we find a matching trailer configuration.
+ *
* separator_pos must not be 0, since the token cannot be an empty string.
*
* If separator_pos is -1, interpret the whole trailer as a token.
@@ -691,22 +718,13 @@ static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
return new_item;
}
-static void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
- const struct trailer_conf *conf,
- const struct new_trailer_item *new_trailer_item)
+void trailer_add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
+ struct list_head *arg_head)
{
struct arg_item *new_item = xcalloc(1, sizeof(*new_item));
new_item->token = tok;
new_item->value = val;
duplicate_trailer_conf(&new_item->conf, conf);
- if (new_trailer_item) {
- if (new_trailer_item->where != WHERE_DEFAULT)
- new_item->conf.where = new_trailer_item->where;
- if (new_trailer_item->if_exists != EXISTS_DEFAULT)
- new_item->conf.if_exists = new_trailer_item->if_exists;
- if (new_trailer_item->if_missing != MISSING_DEFAULT)
- new_item->conf.if_missing = new_trailer_item->if_missing;
- }
list_add_tail(&new_item->list, arg_head);
}
@@ -719,10 +737,10 @@ void parse_trailers_from_config(struct list_head *config_head)
list_for_each(pos, &conf_head) {
item = list_entry(pos, struct arg_item, list);
if (item->conf.command)
- trailer_add_arg_item(config_head,
- xstrdup(token_from_item(item, NULL)),
+ trailer_add_arg_item(xstrdup(token_from_item(item, NULL)),
xstrdup(""),
- &item->conf, NULL);
+ &item->conf,
+ config_head);
}
}
@@ -755,10 +773,10 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
strbuf_release(&sb);
} else {
parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
- trailer_add_arg_item(arg_head,
- strbuf_detach(&tok, NULL),
+ trailer_add_arg_item(strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL),
- conf, tr);
+ conf,
+ arg_head);
}
}
@@ -1148,6 +1166,16 @@ void free_trailers(struct list_head *trailers)
}
}
+void free_new_trailers(struct list_head *trailers)
+{
+ struct list_head *pos, *p;
+
+ list_for_each_safe(pos, p, trailers) {
+ list_del(pos);
+ free_arg_item(list_entry(pos, struct arg_item, list));
+ }
+}
+
size_t trailer_block_start(struct trailer_block *trailer_block)
{
return trailer_block->start;
diff --git a/trailer.h b/trailer.h
index d724263e4f6..8fcf1969a3c 100644
--- a/trailer.h
+++ b/trailer.h
@@ -46,9 +46,20 @@ struct new_trailer_item {
enum trailer_if_missing if_missing;
};
+void trailer_conf_set(enum trailer_where where,
+ enum trailer_if_exists if_exists,
+ enum trailer_if_missing if_missing,
+ struct trailer_conf *conf);
+
+struct trailer_conf *new_trailer_conf(void);
void duplicate_trailer_conf(struct trailer_conf *dst,
const struct trailer_conf *src);
+const char *trailer_default_separators(void);
+
+void trailer_add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
+ struct list_head *arg_head);
+
struct process_trailer_options {
int in_place;
int trim_empty;
@@ -95,6 +106,7 @@ void format_trailers(const struct process_trailer_options *opts,
struct list_head *trailers,
struct strbuf *out);
void free_trailers(struct list_head *);
+void free_new_trailers(struct list_head *);
/*
* Convenience function to format the trailers from the commit msg "msg" into
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 09/10] trailer: delete obsolete argument handling code from API
From: Linus Arver via GitGitGadget @ 2024-01-31 1:22 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v3.git.1706664144.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
This commit was not squashed with its parent in order to keep the diff
separate (to make the additive changes in the parent easier to read).
Note that we remove the "new_trailer_item" struct, because it has some
overlap with "arg_item" struct that also deals with trailers coming from
the command line. The latter will be renamed to "trailer_template" in
the next patch.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 39 ---------------------------------------
trailer.h | 17 -----------------
2 files changed, 56 deletions(-)
diff --git a/trailer.c b/trailer.c
index 19637ff295d..bf1d2eee550 100644
--- a/trailer.c
+++ b/trailer.c
@@ -744,45 +744,6 @@ void parse_trailers_from_config(struct list_head *config_head)
}
}
-void parse_trailers_from_command_line_args(struct list_head *arg_head,
- struct list_head *new_trailer_head)
-{
- struct strbuf tok = STRBUF_INIT;
- struct strbuf val = STRBUF_INIT;
- const struct trailer_conf *conf;
- struct list_head *pos;
-
- /*
- * In command-line arguments, '=' is accepted (in addition to the
- * separators that are defined).
- */
- char *cl_separators = xstrfmt("=%s", separators);
-
- /* Add an arg item for each trailer on the command line */
- list_for_each(pos, new_trailer_head) {
- struct new_trailer_item *tr =
- list_entry(pos, struct new_trailer_item, list);
- ssize_t separator_pos = find_separator(tr->text, cl_separators);
-
- if (separator_pos == 0) {
- struct strbuf sb = STRBUF_INIT;
- strbuf_addstr(&sb, tr->text);
- strbuf_trim(&sb);
- error(_("empty trailer token in trailer '%.*s'"),
- (int) sb.len, sb.buf);
- strbuf_release(&sb);
- } else {
- parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
- trailer_add_arg_item(strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf,
- arg_head);
- }
- }
-
- free(cl_separators);
-}
-
static const char *next_line(const char *str)
{
const char *nl = strchrnul(str, '\n');
diff --git a/trailer.h b/trailer.h
index 8fcf1969a3c..5d4bacd9931 100644
--- a/trailer.h
+++ b/trailer.h
@@ -32,20 +32,6 @@ int trailer_set_where(enum trailer_where *item, const char *value);
int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
-/*
- * A list that represents newly-added trailers, such as those provided
- * with the --trailer command line option of git-interpret-trailers.
- */
-struct new_trailer_item {
- struct list_head list;
-
- const char *text;
-
- enum trailer_where where;
- enum trailer_if_exists if_exists;
- enum trailer_if_missing if_missing;
-};
-
void trailer_conf_set(enum trailer_where where,
enum trailer_if_exists if_exists,
enum trailer_if_missing if_missing,
@@ -79,9 +65,6 @@ struct process_trailer_options {
void parse_trailers_from_config(struct list_head *config_head);
-void parse_trailers_from_command_line_args(struct list_head *arg_head,
- struct list_head *new_trailer_head);
-
void process_trailers_lists(struct list_head *head,
struct list_head *arg_head);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 10/10] trailer: introduce "template" term for readability
From: Linus Arver via GitGitGadget @ 2024-01-31 1:22 UTC (permalink / raw)
To: git
Cc: Christian Couder, Junio C Hamano, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver, Linus Arver
In-Reply-To: <pull.1632.v3.git.1706664144.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
The term "arg_item" is ambiguous because we use it to hold data for
(1) trailers specified as command line arguments (in
builtin/interpret-trailers.c), and
(2) trailers specified in configuration,
and these are both used to ultimately insert new trailers (based
on the contents of arg_item, acting as a kind of template) into some
other set of existing trailers (such as those found in a trailer block
inside a log message) that have already been parsed.
Rename "arg_item" to "trailer_template". This necessitates further
renames to make the functions that act on these templates match the data
structures (parameters) they act on:
- [*] add_arg_to_input_list() to apply_template_to_trailers()
- [*] apply_arg_if_exists() to maybe_add_if_exists()
- [*] apply_arg_if_missing() to maybe_add_if_missing()
- apply_command() to run_command_from_template()
- [*] apply_item_command() to populate_template_value()
- free_arg_item() to free_template() (non-API function)
- free_new_trailers() to free_trailer_templates() (API function)
- get_conf_item() to get_or_add_template_by()
- option_parse_trailer() to option_parse_trailer_template()
- parse_trailers_from_config() to parse_trailer_templates_from_config()
- [*] process_trailers_lists() to apply_trailer_templates()
- token_from_item() to token_from_template()
- token_matches_item to token_matches_template
- [*] trailer_add_arg_item() to add_trailer_template()
- trailer_from_arg() to trailer_from()
- [*] check_if_different() (reorder parameters only)
- [*] find_same_and_apply_arg() (reorder parameters only)
Reorder parameters where appropriate; these functions have been marked
with an asterisk ([*]).
This removes the "arg" terminology (standing for "CLI arguments") from
the trailer implementation, which makes sense because trailers
themselves have nothing to do with CLI argument handling.
Also note that these renames expose the previously liberal use of
"trailer" to mean both trailers we read from the input text (trailer
block) and trailer templates that are defined as CLI args or
configurations. Some functions implied a single action when they could
do two different things, so introduce words like "maybe" and "or" to
unmask their behavior.
In summary this patch renames and reorders parameters for readability,
without any behavioral change. We don't rename
find_same_and_apply_arg(), because it will be refactored soon. As an
added benefit, we no longer use the term "arg" to mean "CLI arguments"
in the trailer implementation, because trailers themselves should not be
concerned about CLI arguments (this is the domain of the
interpret-trailers builtin).
For parse_trailers_from_config() (renamed to
parse_trailer_templates_from_config()), add a NEEDSWORK discussion about
how the deprecated trailer.*.command configuration option is oddly more
featureful than trailer.*.cmd (if we were to remove support for
trailer.*.command, users would not be able to replicate the behavior
with trailer.*.cmd and lose out on functionality).
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 48 +++---
trailer.c | 292 +++++++++++++++++++----------------
trailer.h | 11 +-
3 files changed, 192 insertions(+), 159 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 9a902012912..aae3f3119df 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -47,10 +47,14 @@ static int option_parse_if_missing(const struct option *opt,
static char *cl_separators;
-static int option_parse_trailer(const struct option *opt,
- const char *arg, int unset)
+/*
+ * Interpret "--trailer ..." as trailer templates (trailers we want to add into
+ * the input text).
+ */
+static int option_parse_trailer_template(const struct option *opt,
+ const char *arg, int unset)
{
- struct list_head *trailers = opt->value;
+ struct list_head *templates = opt->value;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
const struct trailer_conf *conf;
@@ -58,7 +62,7 @@ static int option_parse_trailer(const struct option *opt,
ssize_t separator_pos;
if (unset) {
- free_new_trailers(trailers);
+ free_trailer_templates(templates);
return 0;
}
@@ -75,10 +79,10 @@ static int option_parse_trailer(const struct option *opt,
*/
trailer_conf_set(where, if_exists, if_missing, conf_current);
- trailer_add_arg_item(strbuf_detach(&tok, NULL),
+ add_trailer_template(strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL),
conf_current,
- trailers);
+ templates);
} else {
struct strbuf sb = STRBUF_INIT;
strbuf_addstr(&sb, arg);
@@ -148,10 +152,10 @@ static void read_input_file(struct strbuf *sb, const char *file)
}
static void interpret_trailers(const struct process_trailer_options *opts,
- struct list_head *arg_trailers,
+ struct list_head *templates,
const char *file)
{
- LIST_HEAD(head);
+ LIST_HEAD(trailers_from_sb);
struct strbuf sb = STRBUF_INIT;
struct strbuf tb = STRBUF_INIT;
struct trailer_block *trailer_block;
@@ -162,7 +166,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
if (opts->in_place)
outfile = create_in_place_tempfile(file);
- trailer_block = parse_trailers(opts, sb.buf, &head);
+ trailer_block = parse_trailers(opts, sb.buf, &trailers_from_sb);
/* Print the lines before the trailer block */
if (!opts->only_trailers)
@@ -173,15 +177,15 @@ static void interpret_trailers(const struct process_trailer_options *opts,
if (!opts->only_input) {
- process_trailers_lists(&head, arg_trailers);
+ apply_trailer_templates(templates, &trailers_from_sb);
}
/* Print trailer block. */
- format_trailers(opts, &head, &tb);
+ format_trailers(opts, &trailers_from_sb, &tb);
fwrite(tb.buf, 1, tb.len, outfile);
strbuf_release(&tb);
- free_trailers(&head);
+ free_trailers(&trailers_from_sb);
/* Print the lines after the trailer block as is */
if (!opts->only_trailers)
@@ -199,8 +203,8 @@ static void interpret_trailers(const struct process_trailer_options *opts,
int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
- LIST_HEAD(configured_trailers);
- LIST_HEAD(arg_trailers);
+ LIST_HEAD(configured_templates);
+ LIST_HEAD(templates);
struct option options[] = {
OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
@@ -219,8 +223,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
- OPT_CALLBACK(0, "trailer", &arg_trailers, N_("trailer"),
- N_("trailer(s) to add"), option_parse_trailer),
+ OPT_CALLBACK(0, "trailer", &templates, N_("trailer"),
+ N_("trailer(s) to add"), option_parse_trailer_template),
OPT_END()
};
@@ -228,7 +232,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
trailer_config_init();
if (!opts.only_input) {
- parse_trailers_from_config(&configured_trailers);
+ parse_trailer_templates_from_config(&configured_templates);
}
/*
@@ -242,25 +246,25 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
free(cl_separators);
- if (opts.only_input && !list_empty(&arg_trailers))
+ if (opts.only_input && !list_empty(&templates))
usage_msg_opt(
_("--trailer with --only-input does not make sense"),
git_interpret_trailers_usage,
options);
- list_splice(&configured_trailers, &arg_trailers);
+ list_splice(&configured_templates, &templates);
if (argc) {
int i;
for (i = 0; i < argc; i++)
- interpret_trailers(&opts, &arg_trailers, argv[i]);
+ interpret_trailers(&opts, &templates, argv[i]);
} else {
if (opts.in_place)
die(_("no input file given for in-place editing"));
- interpret_trailers(&opts, &arg_trailers, NULL);
+ interpret_trailers(&opts, &templates, NULL);
}
- free_new_trailers(&arg_trailers);
+ free_trailer_templates(&templates);
return 0;
}
diff --git a/trailer.c b/trailer.c
index bf1d2eee550..a739e2ada86 100644
--- a/trailer.c
+++ b/trailer.c
@@ -55,14 +55,14 @@ struct trailer_item {
char *value;
};
-struct arg_item {
+struct trailer_template {
struct list_head list;
char *token;
char *value;
struct trailer_conf conf;
};
-static LIST_HEAD(conf_head);
+static LIST_HEAD(templates_from_conf);
static char *separators = ":";
@@ -105,7 +105,7 @@ static size_t token_len_without_separator(const char *token, size_t len)
return len;
}
-static int same_token(struct trailer_item *a, struct arg_item *b)
+static int same_token(struct trailer_item *a, struct trailer_template *b)
{
size_t a_len, b_len, min_len;
@@ -119,12 +119,12 @@ static int same_token(struct trailer_item *a, struct arg_item *b)
return !strncasecmp(a->token, b->token, min_len);
}
-static int same_value(struct trailer_item *a, struct arg_item *b)
+static int same_value(struct trailer_item *a, struct trailer_template *b)
{
return !strcasecmp(a->value, b->value);
}
-static int same_trailer(struct trailer_item *a, struct arg_item *b)
+static int same_trailer(struct trailer_item *a, struct trailer_template *b)
{
return same_token(a, b) && same_value(a, b);
}
@@ -151,15 +151,15 @@ static void free_trailer_item(struct trailer_item *item)
free(item);
}
-static void free_arg_item(struct arg_item *item)
+static void free_template(struct trailer_template *template)
{
- free(item->conf.name);
- free(item->conf.key);
- free(item->conf.command);
- free(item->conf.cmd);
- free(item->token);
- free(item->value);
- free(item);
+ free(template->conf.name);
+ free(template->conf.key);
+ free(template->conf.command);
+ free(template->conf.cmd);
+ free(template->token);
+ free(template->value);
+ free(template);
}
static char last_non_space_char(const char *s)
@@ -171,36 +171,36 @@ static char last_non_space_char(const char *s)
return '\0';
}
-static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
+static struct trailer_item *trailer_from(struct trailer_template *template)
{
struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
- new_item->token = arg_tok->token;
- new_item->value = arg_tok->value;
- arg_tok->token = arg_tok->value = NULL;
- free_arg_item(arg_tok);
+ new_item->token = template->token;
+ new_item->value = template->value;
+ template->token = template->value = NULL;
+ free_template(template);
return new_item;
}
-static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct arg_item *arg_tok)
+static void apply_template_to_trailers(struct trailer_template *template,
+ struct trailer_item *on_tok)
{
- int aoe = after_or_end(arg_tok->conf.where);
- struct trailer_item *to_add = trailer_from_arg(arg_tok);
+ int aoe = after_or_end(template->conf.where);
+ struct trailer_item *to_add = trailer_from(template);
if (aoe)
list_add(&to_add->list, &on_tok->list);
else
list_add_tail(&to_add->list, &on_tok->list);
}
-static int check_if_different(struct trailer_item *in_tok,
- struct arg_item *arg_tok,
- int check_all,
- struct list_head *head)
+static int check_if_different(struct trailer_template *template,
+ struct trailer_item *in_tok,
+ struct list_head *head,
+ int check_all)
{
- enum trailer_where where = arg_tok->conf.where;
+ enum trailer_where where = template->conf.where;
struct list_head *next_head;
do {
- if (same_trailer(in_tok, arg_tok))
+ if (same_trailer(in_tok, template))
return 0;
/*
* if we want to add a trailer after another one,
@@ -215,7 +215,8 @@ static int check_if_different(struct trailer_item *in_tok,
return 1;
}
-static char *apply_command(struct trailer_conf *conf, const char *arg)
+static char *run_command_from_template(struct trailer_conf *conf,
+ const char *arg)
{
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
@@ -250,133 +251,142 @@ static char *apply_command(struct trailer_conf *conf, const char *arg)
return result;
}
-static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
+/*
+ * Prepare the template by running the command (if any) requested by the
+ * template in order to populate the template's value field.
+ */
+static void populate_template_value(struct trailer_template *template,
+ struct trailer_item *in_tok)
{
- if (arg_tok->conf.command || arg_tok->conf.cmd) {
+ if (template->conf.command || template->conf.cmd) {
+ /*
+ * Determine argument to pass into the command.
+ */
const char *arg;
- if (arg_tok->value && arg_tok->value[0]) {
- arg = arg_tok->value;
+ if (template->value && template->value[0]) {
+ arg = template->value;
} else {
if (in_tok && in_tok->value)
arg = xstrdup(in_tok->value);
else
arg = xstrdup("");
}
- arg_tok->value = apply_command(&arg_tok->conf, arg);
+ template->value = run_command_from_template(&template->conf,
+ arg);
free((char *)arg);
}
}
-static void apply_arg_if_exists(struct trailer_item *in_tok,
- struct arg_item *arg_tok,
+static void maybe_add_if_exists(struct trailer_template *template,
+ struct trailer_item *in_tok,
struct trailer_item *on_tok,
- struct list_head *head)
+ struct list_head *trailers)
{
- switch (arg_tok->conf.if_exists) {
+ switch (template->conf.if_exists) {
case EXISTS_DO_NOTHING:
- free_arg_item(arg_tok);
+ free_template(template);
break;
case EXISTS_REPLACE:
- apply_item_command(in_tok, arg_tok);
- add_arg_to_input_list(on_tok, arg_tok);
+ populate_template_value(template, in_tok);
+ apply_template_to_trailers(template, on_tok);
list_del(&in_tok->list);
free_trailer_item(in_tok);
break;
case EXISTS_ADD:
- apply_item_command(in_tok, arg_tok);
- add_arg_to_input_list(on_tok, arg_tok);
+ populate_template_value(template, in_tok);
+ apply_template_to_trailers(template, on_tok);
break;
case EXISTS_ADD_IF_DIFFERENT:
- apply_item_command(in_tok, arg_tok);
- if (check_if_different(in_tok, arg_tok, 1, head))
- add_arg_to_input_list(on_tok, arg_tok);
+ populate_template_value(template, in_tok);
+ if (check_if_different(template, in_tok, trailers, 1))
+ apply_template_to_trailers(template, on_tok);
else
- free_arg_item(arg_tok);
+ free_template(template);
break;
case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
- apply_item_command(in_tok, arg_tok);
- if (check_if_different(on_tok, arg_tok, 0, head))
- add_arg_to_input_list(on_tok, arg_tok);
+ populate_template_value(template, in_tok);
+ if (check_if_different(template, on_tok, trailers, 0))
+ apply_template_to_trailers(template, on_tok);
else
- free_arg_item(arg_tok);
+ free_template(template);
break;
default:
BUG("trailer.c: unhandled value %d",
- arg_tok->conf.if_exists);
+ template->conf.if_exists);
}
}
-static void apply_arg_if_missing(struct list_head *head,
- struct arg_item *arg_tok)
+static void maybe_add_if_missing(struct trailer_template *template,
+ struct list_head *trailers)
{
enum trailer_where where;
struct trailer_item *to_add;
- switch (arg_tok->conf.if_missing) {
+ switch (template->conf.if_missing) {
case MISSING_DO_NOTHING:
- free_arg_item(arg_tok);
+ free_template(template);
break;
case MISSING_ADD:
- where = arg_tok->conf.where;
- apply_item_command(NULL, arg_tok);
- to_add = trailer_from_arg(arg_tok);
+ where = template->conf.where;
+ populate_template_value(template, NULL);
+ to_add = trailer_from(template);
if (after_or_end(where))
- list_add_tail(&to_add->list, head);
+ list_add_tail(&to_add->list, trailers);
else
- list_add(&to_add->list, head);
+ list_add(&to_add->list, trailers);
break;
default:
BUG("trailer.c: unhandled value %d",
- arg_tok->conf.if_missing);
+ template->conf.if_missing);
}
}
-static int find_same_and_apply_arg(struct list_head *head,
- struct arg_item *arg_tok)
+static int find_same_and_apply_arg(struct trailer_template *template,
+ struct list_head *trailers)
{
struct list_head *pos;
struct trailer_item *in_tok;
struct trailer_item *on_tok;
- enum trailer_where where = arg_tok->conf.where;
+ enum trailer_where where = template->conf.where;
int middle = (where == WHERE_AFTER) || (where == WHERE_BEFORE);
int backwards = after_or_end(where);
struct trailer_item *start_tok;
- if (list_empty(head))
+ if (list_empty(trailers))
return 0;
- start_tok = list_entry(backwards ? head->prev : head->next,
+ start_tok = list_entry(backwards ? trailers->prev : trailers->next,
struct trailer_item,
list);
- list_for_each_dir(pos, head, backwards) {
+ list_for_each_dir(pos, trailers, backwards) {
in_tok = list_entry(pos, struct trailer_item, list);
- if (!same_token(in_tok, arg_tok))
+ if (!same_token(in_tok, template))
continue;
on_tok = middle ? in_tok : start_tok;
- apply_arg_if_exists(in_tok, arg_tok, on_tok, head);
+ maybe_add_if_exists(template, in_tok, on_tok, trailers);
return 1;
}
return 0;
}
-void process_trailers_lists(struct list_head *head,
- struct list_head *arg_head)
+void apply_trailer_templates(struct list_head *templates,
+ struct list_head *trailers)
{
struct list_head *pos, *p;
- struct arg_item *arg_tok;
+ struct trailer_template *template;
- list_for_each_safe(pos, p, arg_head) {
+ list_for_each_safe(pos, p, templates) {
int applied = 0;
- arg_tok = list_entry(pos, struct arg_item, list);
+ template = list_entry(pos, struct trailer_template, list);
list_del(pos);
- applied = find_same_and_apply_arg(head, arg_tok);
+ applied = find_same_and_apply_arg(template, trailers);
if (!applied)
- apply_arg_if_missing(head, arg_tok);
+ maybe_add_if_missing(template, trailers);
}
}
@@ -458,26 +468,26 @@ void duplicate_trailer_conf(struct trailer_conf *dst,
dst->cmd = xstrdup_or_null(src->cmd);
}
-static struct arg_item *get_conf_item(const char *name)
+static struct trailer_template *get_or_add_template_by(const char *name)
{
struct list_head *pos;
- struct arg_item *item;
+ struct trailer_template *template;
- /* Look up item with same name */
- list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct arg_item, list);
- if (!strcasecmp(item->conf.name, name))
- return item;
+ /* Look up template with same name. */
+ list_for_each(pos, &templates_from_conf) {
+ template = list_entry(pos, struct trailer_template, list);
+ if (!strcasecmp(template->conf.name, name))
+ return template;
}
- /* Item does not already exists, create it */
- CALLOC_ARRAY(item, 1);
- duplicate_trailer_conf(&item->conf, &default_trailer_conf);
- item->conf.name = xstrdup(name);
+ /* Template does not already exist; create it. */
+ CALLOC_ARRAY(template, 1);
+ duplicate_trailer_conf(&template->conf, &default_trailer_conf);
+ template->conf.name = xstrdup(name);
- list_add_tail(&item->list, &conf_head);
+ list_add_tail(&template->list, &templates_from_conf);
- return item;
+ return template;
}
enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_CMD,
@@ -535,7 +545,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
void *cb UNUSED)
{
const char *trailer_item, *variable_name;
- struct arg_item *item;
+ struct trailer_template *template;
struct trailer_conf *conf;
char *name = NULL;
enum trailer_info_type type;
@@ -560,8 +570,8 @@ static int git_trailer_config(const char *conf_key, const char *value,
if (!name)
return 0;
- item = get_conf_item(name);
- conf = &item->conf;
+ template = get_or_add_template_by(name);
+ conf = &template->conf;
free(name);
switch (type) {
@@ -618,20 +628,22 @@ void trailer_config_init(void)
configured = 1;
}
-static const char *token_from_item(struct arg_item *item, char *tok)
+static const char *token_from_template(struct trailer_template *template, char *tok)
{
- if (item->conf.key)
- return item->conf.key;
+ if (template->conf.key)
+ return template->conf.key;
if (tok)
return tok;
- return item->conf.name;
+ return template->conf.name;
}
-static int token_matches_item(const char *tok, struct arg_item *item, size_t tok_len)
+static int token_matches_template(const char *tok,
+ struct trailer_template *template,
+ size_t tok_len)
{
- if (!strncasecmp(tok, item->conf.name, tok_len))
+ if (!strncasecmp(tok, template->conf.name, tok_len))
return 1;
- return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
+ return template->conf.key ? !strncasecmp(tok, template->conf.key, tok_len) : 0;
}
/*
@@ -677,7 +689,7 @@ void parse_trailer(const char *line, ssize_t separator_pos,
struct strbuf *tok, struct strbuf *val,
const struct trailer_conf **conf)
{
- struct arg_item *item;
+ struct trailer_template *template;
size_t tok_len;
struct list_head *pos;
@@ -695,13 +707,13 @@ void parse_trailer(const char *line, ssize_t separator_pos,
tok_len = token_len_without_separator(tok->buf, tok->len);
if (conf)
*conf = &default_trailer_conf;
- list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct arg_item, list);
- if (token_matches_item(tok->buf, item, tok_len)) {
+ list_for_each(pos, &templates_from_conf) {
+ template = list_entry(pos, struct trailer_template, list);
+ if (token_matches_template(tok->buf, template, tok_len)) {
char *tok_buf = strbuf_detach(tok, NULL);
if (conf)
- *conf = &item->conf;
- strbuf_addstr(tok, token_from_item(item, tok_buf));
+ *conf = &template->conf;
+ strbuf_addstr(tok, token_from_template(template, tok_buf));
free(tok_buf);
break;
}
@@ -718,29 +730,41 @@ static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
return new_item;
}
-void trailer_add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
- struct list_head *arg_head)
+void add_trailer_template(char *tok, char *val, const struct trailer_conf *conf,
+ struct list_head *templates)
{
- struct arg_item *new_item = xcalloc(1, sizeof(*new_item));
- new_item->token = tok;
- new_item->value = val;
- duplicate_trailer_conf(&new_item->conf, conf);
- list_add_tail(&new_item->list, arg_head);
+ struct trailer_template *template = xcalloc(1, sizeof(*template));
+ template->token = tok;
+ template->value = val;
+ duplicate_trailer_conf(&template->conf, conf);
+ list_add_tail(&template->list, templates);
}
-void parse_trailers_from_config(struct list_head *config_head)
+void parse_trailer_templates_from_config(struct list_head *config_head)
{
- struct arg_item *item;
+ struct trailer_template *template;
struct list_head *pos;
- /* Add an arg item for each configured trailer with a command */
- list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct arg_item, list);
- if (item->conf.command)
- trailer_add_arg_item(xstrdup(token_from_item(item, NULL)),
- xstrdup(""),
- &item->conf,
- config_head);
+ /*
+ * Get configured templates with a ".command" option.
+ *
+ * NEEDSWORK: If the interpret-trailers builtin sees a
+ * "trailer.foo.command = ..." setting, then the "foo" trailer will
+ * always be inserted, even if "--trailer foo" is not provided.
+ * Considering how ".command" is deprecated, it is a bit strange to see
+ * it getting special treatment like this over ".cmd". Instead, we
+ * should add a new option that explicitly lets the user decide if the
+ * configured trailer should always be added automatically, or if it
+ * should only be added if "--trailer foo" is provided (default).
+ * Then we can collect configured trailers that have either ".command"
+ * or ".cmd" below, instead of just ".command".
+ */
+ list_for_each(pos, &templates_from_conf) {
+ template = list_entry(pos, struct trailer_template, list);
+ if (template->conf.command)
+ add_trailer_template(xstrdup(token_from_template(template,
+ NULL)),
+ xstrdup(""), &template->conf, config_head);
}
}
@@ -852,7 +876,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
* Get the start of the trailers by looking starting from the end for a
* blank line before a set of non-blank lines that (i) are all
* trailers, or (ii) contains at least one Git-generated trailer and
- * consists of at least 25% trailers.
+ * consists of at least 25% configured trailers.
*/
for (l = last_line(buf, len);
l >= end_of_title;
@@ -896,10 +920,16 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
possible_continuation_lines = 0;
if (recognized_prefix)
continue;
- list_for_each(pos, &conf_head) {
- struct arg_item *item;
- item = list_entry(pos, struct arg_item, list);
- if (token_matches_item(bol, item,
+ /*
+ * The templates here are not used for actually
+ * adding trailers anywhere, but instead to help us
+ * identify trailer lines by comparing their keys with
+ * those found in configured templates.
+ */
+ list_for_each(pos, &templates_from_conf) {
+ struct trailer_template *template;
+ template = list_entry(pos, struct trailer_template, list);
+ if (token_matches_template(bol, template,
separator_pos)) {
recognized_prefix = 1;
break;
@@ -1127,13 +1157,13 @@ void free_trailers(struct list_head *trailers)
}
}
-void free_new_trailers(struct list_head *trailers)
+void free_trailer_templates(struct list_head *trailer_templates)
{
struct list_head *pos, *p;
- list_for_each_safe(pos, p, trailers) {
+ list_for_each_safe(pos, p, trailer_templates) {
list_del(pos);
- free_arg_item(list_entry(pos, struct arg_item, list));
+ free_template(list_entry(pos, struct trailer_template, list));
}
}
diff --git a/trailer.h b/trailer.h
index 5d4bacd9931..79aa123478d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -43,8 +43,8 @@ void duplicate_trailer_conf(struct trailer_conf *dst,
const char *trailer_default_separators(void);
-void trailer_add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
- struct list_head *arg_head);
+void add_trailer_template(char *tok, char *val, const struct trailer_conf *conf,
+ struct list_head *templates);
struct process_trailer_options {
int in_place;
@@ -63,10 +63,9 @@ struct process_trailer_options {
#define PROCESS_TRAILER_OPTIONS_INIT {0}
-void parse_trailers_from_config(struct list_head *config_head);
+void parse_trailer_templates_from_config(struct list_head *config_head);
-void process_trailers_lists(struct list_head *head,
- struct list_head *arg_head);
+void apply_trailer_templates(struct list_head *templates, struct list_head *trailers_head);
ssize_t find_separator(const char *line, const char *separators);
@@ -89,7 +88,7 @@ void format_trailers(const struct process_trailer_options *opts,
struct list_head *trailers,
struct strbuf *out);
void free_trailers(struct list_head *);
-void free_new_trailers(struct list_head *);
+void free_trailer_templates(struct list_head *);
/*
* Convenience function to format the trailers from the commit msg "msg" into
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] reftable/stack: fsync "tables.list" during compaction
From: Patrick Steinhardt @ 2024-01-31 5:46 UTC (permalink / raw)
To: Justin Tobler; +Cc: git, John Cai
In-Reply-To: <CAGAWz+4wyd13UYRH3ZSSZEq1Y=5HE_p+qaXeOJS-ENMpWoLabA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]
On Tue, Jan 30, 2024 at 01:04:46PM -0600, Justin Tobler wrote:
> On Mon, Jan 29, 2024 at 11:23 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > In 1df18a1c9a (reftable: honor core.fsync, 2024-01-23), we have added
> > code to fsync both newly written reftables as well as "tables.list" to
> > disk. But there are two code paths where "tables.list" is being written:
> >
> > - When appending a new table due to a normal ref update.
> >
> > - When compacting a range of tables during compaction.
> >
> > We have only addressed the former code path, but do not yet sync the new
> > "tables.list" file in the latter. Fix this ommission.
>
> nit: s/ommission/omission
I knew this looked weird when writing it! Should've looked it up.
> > Note that we are not yet adding any tests. These tests will be added
> > once the "reftable" backend has been upstreamed.
> >
>
> Nice catch! I noticed a small typo in the commit message but otherwise looks
> good to me.
Thanks. I'll refrain from sending out a new version for this typo alone,
but will fix it if other feedback requires a second iteration.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* FreeBSD CI suspended on git/git and gitgitgadget/git
From: Johannes Schindelin @ 2024-01-31 7:11 UTC (permalink / raw)
To: git
Team,
I noticed that there is a problem with the FreeBSD runs on Cirrus CI (see
e.g. https://cirrus-ci.com/task/6611218006278144):
Not enough compute credits to prioritize tasks!
Failed to start an instance: INVALID_ARGUMENT: Not Found 404 Not Found
POST https://compute.googleapis.com:443/compute/v1/projects/cirrus-ci-community/zones/us-central1-c/instances
{
"error":
{
"code": 404,
"message": "The resource 'projects/freebsd-org-cloud-dev/global/images/family/freebsd-12-3' was not found",
"errors": [
{
"message": "The resource 'projects/freebsd-org-cloud-dev/global/images/family/freebsd-12-3' was not found",
"domain": "global",
"reason": "notFound"
}
]
}
}
This makes all the FreeBSD CI builds fail immediately. Sadly, I do not
have time to investigate further, so I suspended the Cirrus CI
installations on https://github.com/git/git and
https://github.com/gitgitgadget/git.
Ciao,
Johannes
^ permalink raw reply
* [PATCH 0/9] reftable: code style improvements
From: Patrick Steinhardt @ 2024-01-31 8:00 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 2980 bytes --]
Hi,
this patch series contains a more or less random assortments of code
style improvements for the reftable library. The patch series is
structured as follows:
- Patches 1-2 introduce macros to allocate and grow arrays. These are
basically the same as our ALLOC_GROW and ALLOC_ARRAY macros, but
specific to the reftable library.
- Patches 3-6 refactor some code to use `size_t` to index into array
slices instead of `int`.
- Patches 7-9 are some more debatable cleanups that helped me
personally to make sense of the code better.
I've been a bit hesitant to send out these patches as they may be
considered "noise", especially the last three ones. But the reftable
library feels quite alien in our codebase, which does increase the
mental overhead at least for me when reading its ccode.
I think it should be a goal of ours to align it closer to our normal
coding style, which will of course end up in quite a bit of churn over
time. A different approach would be to do these refactorings while
already touching the code anyway due to other reasons. But I found
myself doing the same refactorings over and over again in different
contexts for patch series I ultimately didn't end up sending, so I'd
really rather want to get those out of my way.
Anyway, please let me know how you feel about patch series like these.
Patrick
Patrick Steinhardt (9):
reftable: introduce macros to grow arrays
reftable: introduce macros to allocate arrays
reftable/stack: fix parameter validation when compacting range
reftable/stack: index segments with `size_t`
reftable/stack: use `size_t` to track stack slices during compaction
reftable/stack: use `size_t` to track stack length
reftable/merged: refactor seeking of records
reftable/merged: refactor initialization of iterators
reftable/record: improve semantics when initializing records
reftable/basics.c | 15 ++--
reftable/basics.h | 17 ++++-
reftable/block.c | 25 +++---
reftable/block_test.c | 2 +-
reftable/blocksource.c | 4 +-
reftable/iter.c | 3 +-
reftable/merged.c | 100 +++++++++++-------------
reftable/merged_test.c | 52 ++++++-------
reftable/pq.c | 8 +-
reftable/publicbasics.c | 3 +-
reftable/reader.c | 12 ++-
reftable/readwrite_test.c | 8 +-
reftable/record.c | 43 +++--------
reftable/record.h | 10 +--
reftable/record_test.c | 8 +-
reftable/refname.c | 4 +-
reftable/reftable-merged.h | 2 +-
reftable/stack.c | 151 +++++++++++++++++--------------------
reftable/stack.h | 6 +-
reftable/stack_test.c | 7 +-
reftable/tree.c | 4 +-
reftable/writer.c | 21 ++----
22 files changed, 221 insertions(+), 284 deletions(-)
base-commit: bc7ee2e5e16f0d1e710ef8fab3db59ab11f2bbe7
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 1/9] reftable: introduce macros to grow arrays
From: Patrick Steinhardt @ 2024-01-31 8:01 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706687982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 10238 bytes --]
Throughout the reftable library we have many cases where we need to grow
arrays. In order to avoid too many reallocations, we roughly double the
capacity of the array on each iteration. The resulting code pattern is
thus duplicated across many sites.
We have similar patterns in our main codebase, which is why we have
eventually introduced an `ALLOC_GROW()` macro to abstract it away and
avoid some code duplication. We cannot easily reuse this macro here
though because `ALLOC_GROW()` uses `REALLOC_ARRAY()`, which in turn will
call realloc(3P) to grow the array. The reftable code is structured as a
library though (even if the boundaries are fuzzy), and one property this
brings with it is that it is possible to plug in your own allocators. So
instead of using realloc(3P), we need to use `reftable_realloc()` that
knows to use the user-provided implementation.
So let's introduce two new macros `REFTABLE_REALLOC_ARRAY()` and
`REFTABLE_ALLOC_GROW()` that mirror what we do in our main codebase,
with two modifications:
- They use `reftable_realloc()`, as explained above.
- They use a different growth factor of `2 * cap + 1` instead of `(cap
+ 16) * 3 / 2`.
The second change is because we know a bit more about the allocation
patterns in the reftable library. For In most cases, we end up only having a
single item in the array, so the initial capacity that our global growth
factor uses (which is 24), significantly overallocates in a lot of code
paths. This effect is indeed measurable:
Benchmark 1: update-ref: create many refs (growth factor = 2 * cap + 1)
Time (mean ± σ): 4.834 s ± 0.020 s [User: 2.219 s, System: 2.614 s]
Range (min … max): 4.793 s … 4.871 s 20 runs
Benchmark 2: update-ref: create many refs (growth factor = (cap + 16) * 3 + 2)
Time (mean ± σ): 4.933 s ± 0.021 s [User: 2.325 s, System: 2.607 s]
Range (min … max): 4.889 s … 4.962 s 20 runs
Summary
update-ref: create many refs (growth factor = 2 * cap + 1) ran
1.02 ± 0.01 times faster than update-ref: create many refs (growth factor = (cap + 16) * 3 + 2)
Convert the reftable library to use these new macros.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/basics.c | 8 ++------
reftable/basics.h | 11 +++++++++++
reftable/block.c | 7 +------
reftable/merged_test.c | 20 ++++++--------------
reftable/pq.c | 8 ++------
reftable/stack.c | 29 ++++++++++++-----------------
reftable/writer.c | 14 ++------------
7 files changed, 36 insertions(+), 61 deletions(-)
diff --git a/reftable/basics.c b/reftable/basics.c
index f761e48028..af9004cec2 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -89,17 +89,13 @@ void parse_names(char *buf, int size, char ***namesp)
next = end;
}
if (p < next) {
- if (names_len == names_cap) {
- names_cap = 2 * names_cap + 1;
- names = reftable_realloc(
- names, names_cap * sizeof(*names));
- }
+ REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap);
names[names_len++] = xstrdup(p);
}
p = next + 1;
}
- names = reftable_realloc(names, (names_len + 1) * sizeof(*names));
+ REFTABLE_REALLOC_ARRAY(names, names_len + 1);
names[names_len] = NULL;
*namesp = names;
}
diff --git a/reftable/basics.h b/reftable/basics.h
index 096b36862b..2f855cd724 100644
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -53,6 +53,17 @@ void *reftable_realloc(void *p, size_t sz);
void reftable_free(void *p);
void *reftable_calloc(size_t sz);
+#define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))
+#define REFTABLE_ALLOC_GROW(x, nr, alloc) \
+ do { \
+ if ((nr) > alloc) { \
+ alloc = 2 * (alloc) + 1; \
+ if (alloc < (nr)) \
+ alloc = (nr); \
+ REFTABLE_REALLOC_ARRAY(x, alloc); \
+ } \
+ } while (0)
+
/* Find the longest shared prefix size of `a` and `b` */
struct strbuf;
int common_prefix_size(struct strbuf *a, struct strbuf *b);
diff --git a/reftable/block.c b/reftable/block.c
index 1df3d8a0f0..6952d0facf 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -51,12 +51,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
if (2 + 3 * rlen + n > w->block_size - w->next)
return -1;
if (is_restart) {
- if (w->restart_len == w->restart_cap) {
- w->restart_cap = w->restart_cap * 2 + 1;
- w->restarts = reftable_realloc(
- w->restarts, sizeof(uint32_t) * w->restart_cap);
- }
-
+ REFTABLE_ALLOC_GROW(w->restarts, w->restart_len + 1, w->restart_cap);
w->restarts[w->restart_len++] = w->next;
}
diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index 46908f738f..e05351e035 100644
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -231,14 +231,10 @@ static void test_merged(void)
while (len < 100) { /* cap loops/recursion. */
struct reftable_ref_record ref = { NULL };
int err = reftable_iterator_next_ref(&it, &ref);
- if (err > 0) {
+ if (err > 0)
break;
- }
- if (len == cap) {
- cap = 2 * cap + 1;
- out = reftable_realloc(
- out, sizeof(struct reftable_ref_record) * cap);
- }
+
+ REFTABLE_ALLOC_GROW(out, len + 1, cap);
out[len++] = ref;
}
reftable_iterator_destroy(&it);
@@ -368,14 +364,10 @@ static void test_merged_logs(void)
while (len < 100) { /* cap loops/recursion. */
struct reftable_log_record log = { NULL };
int err = reftable_iterator_next_log(&it, &log);
- if (err > 0) {
+ if (err > 0)
break;
- }
- if (len == cap) {
- cap = 2 * cap + 1;
- out = reftable_realloc(
- out, sizeof(struct reftable_log_record) * cap);
- }
+
+ REFTABLE_ALLOC_GROW(out, len + 1, cap);
out[len++] = log;
}
reftable_iterator_destroy(&it);
diff --git a/reftable/pq.c b/reftable/pq.c
index dcefeb793a..2461daf5ff 100644
--- a/reftable/pq.c
+++ b/reftable/pq.c
@@ -75,13 +75,9 @@ void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry
{
int i = 0;
- if (pq->len == pq->cap) {
- pq->cap = 2 * pq->cap + 1;
- pq->heap = reftable_realloc(pq->heap,
- pq->cap * sizeof(struct pq_entry));
- }
-
+ REFTABLE_ALLOC_GROW(pq->heap, pq->len + 1, pq->cap);
pq->heap[pq->len++] = *e;
+
i = pq->len - 1;
while (i > 0) {
int j = (i - 1) / 2;
diff --git a/reftable/stack.c b/reftable/stack.c
index bf3869ce70..1dfab99e96 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -551,7 +551,7 @@ struct reftable_addition {
struct reftable_stack *stack;
char **new_tables;
- int new_tables_len;
+ size_t new_tables_len, new_tables_cap;
uint64_t next_update_index;
};
@@ -602,8 +602,9 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
static void reftable_addition_close(struct reftable_addition *add)
{
- int i = 0;
struct strbuf nm = STRBUF_INIT;
+ size_t i;
+
for (i = 0; i < add->new_tables_len; i++) {
stack_filename(&nm, add->stack, add->new_tables[i]);
unlink(nm.buf);
@@ -613,6 +614,7 @@ static void reftable_addition_close(struct reftable_addition *add)
reftable_free(add->new_tables);
add->new_tables = NULL;
add->new_tables_len = 0;
+ add->new_tables_cap = 0;
delete_tempfile(&add->lock_file);
strbuf_release(&nm);
@@ -631,8 +633,8 @@ int reftable_addition_commit(struct reftable_addition *add)
{
struct strbuf table_list = STRBUF_INIT;
int lock_file_fd = get_tempfile_fd(add->lock_file);
- int i = 0;
int err = 0;
+ size_t i;
if (add->new_tables_len == 0)
goto done;
@@ -660,12 +662,12 @@ int reftable_addition_commit(struct reftable_addition *add)
}
/* success, no more state to clean up. */
- for (i = 0; i < add->new_tables_len; i++) {
+ for (i = 0; i < add->new_tables_len; i++)
reftable_free(add->new_tables[i]);
- }
reftable_free(add->new_tables);
add->new_tables = NULL;
add->new_tables_len = 0;
+ add->new_tables_cap = 0;
err = reftable_stack_reload_maybe_reuse(add->stack, 1);
if (err)
@@ -792,11 +794,9 @@ int reftable_addition_add(struct reftable_addition *add,
goto done;
}
- add->new_tables = reftable_realloc(add->new_tables,
- sizeof(*add->new_tables) *
- (add->new_tables_len + 1));
- add->new_tables[add->new_tables_len] = strbuf_detach(&next_name, NULL);
- add->new_tables_len++;
+ REFTABLE_ALLOC_GROW(add->new_tables, add->new_tables_len + 1,
+ add->new_tables_cap);
+ add->new_tables[add->new_tables_len++] = strbuf_detach(&next_name, NULL);
done:
if (tab_fd > 0) {
close(tab_fd);
@@ -1367,17 +1367,12 @@ static int stack_check_addition(struct reftable_stack *st,
while (1) {
struct reftable_ref_record ref = { NULL };
err = reftable_iterator_next_ref(&it, &ref);
- if (err > 0) {
+ if (err > 0)
break;
- }
if (err < 0)
goto done;
- if (len >= cap) {
- cap = 2 * cap + 1;
- refs = reftable_realloc(refs, cap * sizeof(refs[0]));
- }
-
+ REFTABLE_ALLOC_GROW(refs, len + 1, cap);
refs[len++] = ref;
}
diff --git a/reftable/writer.c b/reftable/writer.c
index ee4590e20f..4483bb21c3 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -200,12 +200,7 @@ static void writer_index_hash(struct reftable_writer *w, struct strbuf *hash)
return;
}
- if (key->offset_len == key->offset_cap) {
- key->offset_cap = 2 * key->offset_cap + 1;
- key->offsets = reftable_realloc(
- key->offsets, sizeof(uint64_t) * key->offset_cap);
- }
-
+ REFTABLE_ALLOC_GROW(key->offsets, key->offset_len + 1, key->offset_cap);
key->offsets[key->offset_len++] = off;
}
@@ -674,12 +669,7 @@ static int writer_flush_nonempty_block(struct reftable_writer *w)
if (err < 0)
return err;
- if (w->index_cap == w->index_len) {
- w->index_cap = 2 * w->index_cap + 1;
- w->index = reftable_realloc(
- w->index,
- sizeof(struct reftable_index_record) * w->index_cap);
- }
+ REFTABLE_ALLOC_GROW(w->index, w->index_len + 1, w->index_cap);
ir.offset = w->next;
strbuf_reset(&ir.last_key);
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/9] reftable: introduce macros to allocate arrays
From: Patrick Steinhardt @ 2024-01-31 8:01 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706687982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 16607 bytes --]
Similar to the preceding commit, let's carry over macros to allocate
arrays with `REFTABLE_ALLOC_ARRAY()` and `REFTABLE_CALLOC_ARRAY()`. This
requires us to change the signature of `reftable_calloc()`, which only
takes a single argument right now and thus puts the burden on the caller
to calculate the final array's size. This is a net improvement though as
it means that we can now provide proper overflow checks when multiplying
the array size with the member size.
Convert callsites of `reftable_calloc()` to the new signature, using the
new macros where possible.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/basics.h | 4 +++-
reftable/block_test.c | 2 +-
reftable/blocksource.c | 4 ++--
reftable/iter.c | 3 +--
reftable/merged.c | 4 ++--
reftable/merged_test.c | 22 +++++++++++++---------
reftable/publicbasics.c | 3 ++-
reftable/reader.c | 8 +++-----
reftable/readwrite_test.c | 8 +++++---
reftable/record_test.c | 4 ++--
reftable/refname.c | 4 ++--
reftable/stack.c | 26 ++++++++++++--------------
reftable/tree.c | 4 ++--
reftable/writer.c | 7 +++----
14 files changed, 53 insertions(+), 50 deletions(-)
diff --git a/reftable/basics.h b/reftable/basics.h
index 2f855cd724..4c3ac963a3 100644
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -51,8 +51,10 @@ int names_length(char **names);
void *reftable_malloc(size_t sz);
void *reftable_realloc(void *p, size_t sz);
void reftable_free(void *p);
-void *reftable_calloc(size_t sz);
+void *reftable_calloc(size_t nelem, size_t elsize);
+#define REFTABLE_ALLOC_ARRAY(x, alloc) (x) = reftable_malloc(st_mult(sizeof(*(x)), (alloc)))
+#define REFTABLE_CALLOC_ARRAY(x, alloc) (x) = reftable_calloc((alloc), sizeof(*(x)))
#define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))
#define REFTABLE_ALLOC_GROW(x, nr, alloc) \
do { \
diff --git a/reftable/block_test.c b/reftable/block_test.c
index dedb05c7d8..e162c6e33f 100644
--- a/reftable/block_test.c
+++ b/reftable/block_test.c
@@ -36,7 +36,7 @@ static void test_block_read_write(void)
int j = 0;
struct strbuf want = STRBUF_INIT;
- block.data = reftable_calloc(block_size);
+ REFTABLE_CALLOC_ARRAY(block.data, block_size);
block.len = block_size;
block.source = malloc_block_source();
block_writer_init(&bw, BLOCK_TYPE_REF, block.data, block_size,
diff --git a/reftable/blocksource.c b/reftable/blocksource.c
index 8c41e3c70f..eeed254ba9 100644
--- a/reftable/blocksource.c
+++ b/reftable/blocksource.c
@@ -29,7 +29,7 @@ static int strbuf_read_block(void *v, struct reftable_block *dest, uint64_t off,
{
struct strbuf *b = v;
assert(off + size <= b->len);
- dest->data = reftable_calloc(size);
+ REFTABLE_CALLOC_ARRAY(dest->data, size);
memcpy(dest->data, b->buf + off, size);
dest->len = size;
return size;
@@ -132,7 +132,7 @@ int reftable_block_source_from_file(struct reftable_block_source *bs,
return REFTABLE_IO_ERROR;
}
- p = reftable_calloc(sizeof(*p));
+ REFTABLE_CALLOC_ARRAY(p, 1);
p->size = st.st_size;
p->data = xmmap(NULL, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
close(fd);
diff --git a/reftable/iter.c b/reftable/iter.c
index a8d174c040..8b5ebf6183 100644
--- a/reftable/iter.c
+++ b/reftable/iter.c
@@ -160,8 +160,7 @@ int new_indexed_table_ref_iter(struct indexed_table_ref_iter **dest,
int oid_len, uint64_t *offsets, int offset_len)
{
struct indexed_table_ref_iter empty = INDEXED_TABLE_REF_ITER_INIT;
- struct indexed_table_ref_iter *itr =
- reftable_calloc(sizeof(struct indexed_table_ref_iter));
+ struct indexed_table_ref_iter *itr = reftable_calloc(1, sizeof(*itr));
int err = 0;
*itr = empty;
diff --git a/reftable/merged.c b/reftable/merged.c
index c258ce953e..2031fd51b4 100644
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -190,7 +190,7 @@ int reftable_new_merged_table(struct reftable_merged_table **dest,
}
}
- m = reftable_calloc(sizeof(struct reftable_merged_table));
+ REFTABLE_CALLOC_ARRAY(m, 1);
m->stack = stack;
m->stack_len = n;
m->min = first_min;
@@ -240,7 +240,7 @@ static int merged_table_seek_record(struct reftable_merged_table *mt,
struct reftable_record *rec)
{
struct reftable_iterator *iters = reftable_calloc(
- sizeof(struct reftable_iterator) * mt->stack_len);
+ mt->stack_len, sizeof(*iters));
struct merged_iter merged = {
.stack = iters,
.typ = reftable_record_type(rec),
diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index e05351e035..e233a9d581 100644
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -93,10 +93,12 @@ merged_table_from_records(struct reftable_ref_record **refs,
int i = 0;
struct reftable_merged_table *mt = NULL;
int err;
- struct reftable_table *tabs =
- reftable_calloc(n * sizeof(struct reftable_table));
- *readers = reftable_calloc(n * sizeof(struct reftable_reader *));
- *source = reftable_calloc(n * sizeof(**source));
+ struct reftable_table *tabs;
+
+ REFTABLE_CALLOC_ARRAY(tabs, n);
+ REFTABLE_CALLOC_ARRAY(*readers, n);
+ REFTABLE_CALLOC_ARRAY(*source, n);
+
for (i = 0; i < n; i++) {
write_test_table(&buf[i], refs[i], sizes[i]);
block_source_from_strbuf(&(*source)[i], &buf[i]);
@@ -266,10 +268,12 @@ merged_table_from_log_records(struct reftable_log_record **logs,
int i = 0;
struct reftable_merged_table *mt = NULL;
int err;
- struct reftable_table *tabs =
- reftable_calloc(n * sizeof(struct reftable_table));
- *readers = reftable_calloc(n * sizeof(struct reftable_reader *));
- *source = reftable_calloc(n * sizeof(**source));
+ struct reftable_table *tabs;
+
+ REFTABLE_CALLOC_ARRAY(tabs, n);
+ REFTABLE_CALLOC_ARRAY(*readers, n);
+ REFTABLE_CALLOC_ARRAY(*source, n);
+
for (i = 0; i < n; i++) {
write_test_log_table(&buf[i], logs[i], sizes[i], i + 1);
block_source_from_strbuf(&(*source)[i], &buf[i]);
@@ -412,7 +416,7 @@ static void test_default_write_opts(void)
};
int err;
struct reftable_block_source source = { NULL };
- struct reftable_table *tab = reftable_calloc(sizeof(*tab) * 1);
+ struct reftable_table *tab = reftable_calloc(1, sizeof(*tab));
uint32_t hash_id;
struct reftable_reader *rd = NULL;
struct reftable_merged_table *merged = NULL;
diff --git a/reftable/publicbasics.c b/reftable/publicbasics.c
index bcb82530d6..44b84a125e 100644
--- a/reftable/publicbasics.c
+++ b/reftable/publicbasics.c
@@ -37,8 +37,9 @@ void reftable_free(void *p)
free(p);
}
-void *reftable_calloc(size_t sz)
+void *reftable_calloc(size_t nelem, size_t elsize)
{
+ size_t sz = st_mult(nelem, elsize);
void *p = reftable_malloc(sz);
memset(p, 0, sz);
return p;
diff --git a/reftable/reader.c b/reftable/reader.c
index 64dc366fb1..3e0de5e8ad 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -539,8 +539,7 @@ static int reader_seek_indexed(struct reftable_reader *r,
if (err == 0) {
struct table_iter empty = TABLE_ITER_INIT;
- struct table_iter *malloced =
- reftable_calloc(sizeof(struct table_iter));
+ struct table_iter *malloced = reftable_calloc(1, sizeof(*malloced));
*malloced = empty;
table_iter_copy_from(malloced, &next);
iterator_from_table_iter(it, malloced);
@@ -635,8 +634,7 @@ void reader_close(struct reftable_reader *r)
int reftable_new_reader(struct reftable_reader **p,
struct reftable_block_source *src, char const *name)
{
- struct reftable_reader *rd =
- reftable_calloc(sizeof(struct reftable_reader));
+ struct reftable_reader *rd = reftable_calloc(1, sizeof(*rd));
int err = init_reader(rd, src, name);
if (err == 0) {
*p = rd;
@@ -711,7 +709,7 @@ static int reftable_reader_refs_for_unindexed(struct reftable_reader *r,
uint8_t *oid)
{
struct table_iter ti_empty = TABLE_ITER_INIT;
- struct table_iter *ti = reftable_calloc(sizeof(struct table_iter));
+ struct table_iter *ti = reftable_calloc(1, sizeof(*ti));
struct filtering_ref_iterator *filter = NULL;
struct filtering_ref_iterator empty = FILTERING_REF_ITERATOR_INIT;
int oid_len = hash_size(r->hash_id);
diff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c
index b8a3224016..91ea7a10ef 100644
--- a/reftable/readwrite_test.c
+++ b/reftable/readwrite_test.c
@@ -56,7 +56,9 @@ static void write_table(char ***names, struct strbuf *buf, int N,
int i = 0, n;
struct reftable_log_record log = { NULL };
const struct reftable_stats *stats = NULL;
- *names = reftable_calloc(sizeof(char *) * (N + 1));
+
+ REFTABLE_CALLOC_ARRAY(*names, N + 1);
+
reftable_writer_set_limits(w, update_index, update_index);
for (i = 0; i < N; i++) {
char name[100];
@@ -188,7 +190,7 @@ static void test_log_overflow(void)
static void test_log_write_read(void)
{
int N = 2;
- char **names = reftable_calloc(sizeof(char *) * (N + 1));
+ char **names = reftable_calloc(N + 1, sizeof(*names));
int err;
struct reftable_write_options opts = {
.block_size = 256,
@@ -519,7 +521,7 @@ static void test_table_read_write_seek_index(void)
static void test_table_refs_for(int indexed)
{
int N = 50;
- char **want_names = reftable_calloc(sizeof(char *) * (N + 1));
+ char **want_names = reftable_calloc(N + 1, sizeof(*want_names));
int want_names_len = 0;
uint8_t want_hash[GIT_SHA1_RAWSZ];
diff --git a/reftable/record_test.c b/reftable/record_test.c
index 2876db7d27..999366ad46 100644
--- a/reftable/record_test.c
+++ b/reftable/record_test.c
@@ -231,8 +231,8 @@ static void test_reftable_log_record_roundtrip(void)
.value_type = REFTABLE_LOG_UPDATE,
.value = {
.update = {
- .new_hash = reftable_calloc(GIT_SHA1_RAWSZ),
- .old_hash = reftable_calloc(GIT_SHA1_RAWSZ),
+ .new_hash = reftable_calloc(GIT_SHA1_RAWSZ, 1),
+ .old_hash = reftable_calloc(GIT_SHA1_RAWSZ, 1),
.name = xstrdup("old name"),
.email = xstrdup("old@email"),
.message = xstrdup("old message"),
diff --git a/reftable/refname.c b/reftable/refname.c
index 9573496932..7570e4acf9 100644
--- a/reftable/refname.c
+++ b/reftable/refname.c
@@ -140,8 +140,8 @@ int validate_ref_record_addition(struct reftable_table tab,
{
struct modification mod = {
.tab = tab,
- .add = reftable_calloc(sizeof(char *) * sz),
- .del = reftable_calloc(sizeof(char *) * sz),
+ .add = reftable_calloc(sz, sizeof(*mod.add)),
+ .del = reftable_calloc(sz, sizeof(*mod.del)),
};
int i = 0;
int err = 0;
diff --git a/reftable/stack.c b/reftable/stack.c
index 1dfab99e96..d084823a92 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -50,8 +50,7 @@ static ssize_t reftable_fd_write(void *arg, const void *data, size_t sz)
int reftable_new_stack(struct reftable_stack **dest, const char *dir,
struct reftable_write_options config)
{
- struct reftable_stack *p =
- reftable_calloc(sizeof(struct reftable_stack));
+ struct reftable_stack *p = reftable_calloc(1, sizeof(*p));
struct strbuf list_file_name = STRBUF_INIT;
int err = 0;
@@ -114,7 +113,7 @@ int read_lines(const char *filename, char ***namesp)
int err = 0;
if (fd < 0) {
if (errno == ENOENT) {
- *namesp = reftable_calloc(sizeof(char *));
+ REFTABLE_CALLOC_ARRAY(*namesp, 1);
return 0;
}
@@ -191,8 +190,7 @@ void reftable_stack_destroy(struct reftable_stack *st)
static struct reftable_reader **stack_copy_readers(struct reftable_stack *st,
int cur_len)
{
- struct reftable_reader **cur =
- reftable_calloc(sizeof(struct reftable_reader *) * cur_len);
+ struct reftable_reader **cur = reftable_calloc(cur_len, sizeof(*cur));
int i = 0;
for (i = 0; i < cur_len; i++) {
cur[i] = st->readers[i];
@@ -208,9 +206,9 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,
int err = 0;
int names_len = names_length(names);
struct reftable_reader **new_readers =
- reftable_calloc(sizeof(struct reftable_reader *) * names_len);
+ reftable_calloc(names_len, sizeof(*new_readers));
struct reftable_table *new_tables =
- reftable_calloc(sizeof(struct reftable_table) * names_len);
+ reftable_calloc(names_len, sizeof(*new_tables));
int new_readers_len = 0;
struct reftable_merged_table *new_merged = NULL;
struct strbuf table_path = STRBUF_INIT;
@@ -344,7 +342,7 @@ static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,
goto out;
}
- names = reftable_calloc(sizeof(char *));
+ REFTABLE_CALLOC_ARRAY(names, 1);
} else {
err = fd_read_lines(fd, &names);
if (err < 0)
@@ -686,7 +684,7 @@ int reftable_stack_new_addition(struct reftable_addition **dest,
{
int err = 0;
struct reftable_addition empty = REFTABLE_ADDITION_INIT;
- *dest = reftable_calloc(sizeof(**dest));
+ REFTABLE_CALLOC_ARRAY(*dest, 1);
**dest = empty;
err = reftable_stack_init_addition(*dest, st);
if (err) {
@@ -871,7 +869,7 @@ static int stack_write_compact(struct reftable_stack *st,
{
int subtabs_len = last - first + 1;
struct reftable_table *subtabs = reftable_calloc(
- sizeof(struct reftable_table) * (last - first + 1));
+ last - first + 1, sizeof(*subtabs));
struct reftable_merged_table *mt = NULL;
int err = 0;
struct reftable_iterator it = { NULL };
@@ -979,9 +977,9 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
int compact_count = last - first + 1;
char **listp = NULL;
char **delete_on_success =
- reftable_calloc(sizeof(char *) * (compact_count + 1));
+ reftable_calloc(compact_count + 1, sizeof(*delete_on_success));
char **subtable_locks =
- reftable_calloc(sizeof(char *) * (compact_count + 1));
+ reftable_calloc(compact_count + 1, sizeof(*subtable_locks));
int i = 0;
int j = 0;
int is_empty_table = 0;
@@ -1204,7 +1202,7 @@ int fastlog2(uint64_t sz)
struct segment *sizes_to_segments(int *seglen, uint64_t *sizes, int n)
{
- struct segment *segs = reftable_calloc(sizeof(struct segment) * n);
+ struct segment *segs = reftable_calloc(n, sizeof(*segs));
int next = 0;
struct segment cur = { 0 };
int i = 0;
@@ -1268,7 +1266,7 @@ struct segment suggest_compaction_segment(uint64_t *sizes, int n)
static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
{
uint64_t *sizes =
- reftable_calloc(sizeof(uint64_t) * st->merged->stack_len);
+ reftable_calloc(st->merged->stack_len, sizeof(*sizes));
int version = (st->config.hash_id == GIT_SHA1_FORMAT_ID) ? 1 : 2;
int overhead = header_size(version) - 1;
int i = 0;
diff --git a/reftable/tree.c b/reftable/tree.c
index a5bf880985..528f33ae38 100644
--- a/reftable/tree.c
+++ b/reftable/tree.c
@@ -20,8 +20,8 @@ struct tree_node *tree_search(void *key, struct tree_node **rootp,
if (!insert) {
return NULL;
} else {
- struct tree_node *n =
- reftable_calloc(sizeof(struct tree_node));
+ struct tree_node *n;
+ REFTABLE_CALLOC_ARRAY(n, 1);
n->key = key;
*rootp = n;
return *rootp;
diff --git a/reftable/writer.c b/reftable/writer.c
index 4483bb21c3..47b3448132 100644
--- a/reftable/writer.c
+++ b/reftable/writer.c
@@ -49,7 +49,7 @@ static int padded_write(struct reftable_writer *w, uint8_t *data, size_t len,
{
int n = 0;
if (w->pending_padding > 0) {
- uint8_t *zeroed = reftable_calloc(w->pending_padding);
+ uint8_t *zeroed = reftable_calloc(w->pending_padding, sizeof(*zeroed));
int n = w->write(w->write_arg, zeroed, w->pending_padding);
if (n < 0)
return n;
@@ -123,8 +123,7 @@ struct reftable_writer *
reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
void *writer_arg, struct reftable_write_options *opts)
{
- struct reftable_writer *wp =
- reftable_calloc(sizeof(struct reftable_writer));
+ struct reftable_writer *wp = reftable_calloc(1, sizeof(*wp));
strbuf_init(&wp->block_writer_data.last_key, 0);
options_set_defaults(opts);
if (opts->block_size >= (1 << 24)) {
@@ -132,7 +131,7 @@ reftable_new_writer(ssize_t (*writer_func)(void *, const void *, size_t),
abort();
}
wp->last_key = reftable_empty_strbuf;
- wp->block = reftable_calloc(opts->block_size);
+ REFTABLE_CALLOC_ARRAY(wp->block, opts->block_size);
wp->write = writer_func;
wp->write_arg = writer_arg;
wp->opts = *opts;
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/9] reftable/stack: fix parameter validation when compacting range
From: Patrick Steinhardt @ 2024-01-31 8:01 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706687982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2892 bytes --]
The `stack_compact_range()` function receives a "first" and "last" index
that indicates which tables of the reftable stack should be compacted.
Naturally, "first" must be smaller than "last" in order to identify a
proper range of tables to compress, which we indeed also assert in the
function. But the validations happens after we have already allocated
arrays with a size of `last - first + 1`, leading to an underflow and
thus an invalid allocation size.
Fix this by reordering the array allocations to happen after we have
validated parameters. While at it, convert the array allocations to use
the newly introduced macros.
Note that the relevant variables pointing into arrays should also be
converted to use `size_t` instead of `int`. This is left for a later
commit in this series.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index d084823a92..b6b24c90bf 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -966,6 +966,7 @@ static int stack_write_compact(struct reftable_stack *st,
static int stack_compact_range(struct reftable_stack *st, int first, int last,
struct reftable_log_expiry_config *expiry)
{
+ char **delete_on_success = NULL, **subtable_locks = NULL, **listp = NULL;
struct strbuf temp_tab_file_name = STRBUF_INIT;
struct strbuf new_table_name = STRBUF_INIT;
struct strbuf lock_file_name = STRBUF_INIT;
@@ -974,12 +975,7 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
int err = 0;
int have_lock = 0;
int lock_file_fd = -1;
- int compact_count = last - first + 1;
- char **listp = NULL;
- char **delete_on_success =
- reftable_calloc(compact_count + 1, sizeof(*delete_on_success));
- char **subtable_locks =
- reftable_calloc(compact_count + 1, sizeof(*subtable_locks));
+ int compact_count;
int i = 0;
int j = 0;
int is_empty_table = 0;
@@ -989,6 +985,10 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
goto done;
}
+ compact_count = last - first + 1;
+ REFTABLE_CALLOC_ARRAY(delete_on_success, compact_count + 1);
+ REFTABLE_CALLOC_ARRAY(subtable_locks, compact_count + 1);
+
st->stats.attempts++;
strbuf_reset(&lock_file_name);
@@ -1146,12 +1146,14 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
done:
free_names(delete_on_success);
- listp = subtable_locks;
- while (*listp) {
- unlink(*listp);
- listp++;
+ if (subtable_locks) {
+ listp = subtable_locks;
+ while (*listp) {
+ unlink(*listp);
+ listp++;
+ }
+ free_names(subtable_locks);
}
- free_names(subtable_locks);
if (lock_file_fd >= 0) {
close(lock_file_fd);
lock_file_fd = -1;
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/9] reftable/stack: index segments with `size_t`
From: Patrick Steinhardt @ 2024-01-31 8:01 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706687982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3751 bytes --]
We use `int`s to index into arrays of segments and track the length of
them, which is considered to be a code smell in the Git project. Convert
the code to use `size_t` instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 25 +++++++++++--------------
reftable/stack.h | 6 +++---
reftable/stack_test.c | 7 +++----
3 files changed, 17 insertions(+), 21 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index b6b24c90bf..2be3d1e4ba 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -1202,12 +1202,11 @@ int fastlog2(uint64_t sz)
return l - 1;
}
-struct segment *sizes_to_segments(int *seglen, uint64_t *sizes, int n)
+struct segment *sizes_to_segments(size_t *seglen, uint64_t *sizes, size_t n)
{
struct segment *segs = reftable_calloc(n, sizeof(*segs));
- int next = 0;
struct segment cur = { 0 };
- int i = 0;
+ size_t next = 0, i;
if (n == 0) {
*seglen = 0;
@@ -1233,29 +1232,27 @@ struct segment *sizes_to_segments(int *seglen, uint64_t *sizes, int n)
return segs;
}
-struct segment suggest_compaction_segment(uint64_t *sizes, int n)
+struct segment suggest_compaction_segment(uint64_t *sizes, size_t n)
{
- int seglen = 0;
- struct segment *segs = sizes_to_segments(&seglen, sizes, n);
struct segment min_seg = {
.log = 64,
};
- int i = 0;
+ struct segment *segs;
+ size_t seglen = 0, i;
+
+ segs = sizes_to_segments(&seglen, sizes, n);
for (i = 0; i < seglen; i++) {
- if (segment_size(&segs[i]) == 1) {
+ if (segment_size(&segs[i]) == 1)
continue;
- }
- if (segs[i].log < min_seg.log) {
+ if (segs[i].log < min_seg.log)
min_seg = segs[i];
- }
}
while (min_seg.start > 0) {
- int prev = min_seg.start - 1;
- if (fastlog2(min_seg.bytes) < fastlog2(sizes[prev])) {
+ size_t prev = min_seg.start - 1;
+ if (fastlog2(min_seg.bytes) < fastlog2(sizes[prev]))
break;
- }
min_seg.start = prev;
min_seg.bytes += sizes[prev];
diff --git a/reftable/stack.h b/reftable/stack.h
index c1e3efa899..d919455669 100644
--- a/reftable/stack.h
+++ b/reftable/stack.h
@@ -32,13 +32,13 @@ struct reftable_stack {
int read_lines(const char *filename, char ***lines);
struct segment {
- int start, end;
+ size_t start, end;
int log;
uint64_t bytes;
};
int fastlog2(uint64_t sz);
-struct segment *sizes_to_segments(int *seglen, uint64_t *sizes, int n);
-struct segment suggest_compaction_segment(uint64_t *sizes, int n);
+struct segment *sizes_to_segments(size_t *seglen, uint64_t *sizes, size_t n);
+struct segment suggest_compaction_segment(uint64_t *sizes, size_t n);
#endif
diff --git a/reftable/stack_test.c b/reftable/stack_test.c
index 289e902146..2d5b24e5c5 100644
--- a/reftable/stack_test.c
+++ b/reftable/stack_test.c
@@ -711,7 +711,7 @@ static void test_sizes_to_segments(void)
uint64_t sizes[] = { 2, 3, 4, 5, 7, 9 };
/* .................0 1 2 3 4 5 */
- int seglen = 0;
+ size_t seglen = 0;
struct segment *segs =
sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes));
EXPECT(segs[2].log == 3);
@@ -726,7 +726,7 @@ static void test_sizes_to_segments(void)
static void test_sizes_to_segments_empty(void)
{
- int seglen = 0;
+ size_t seglen = 0;
struct segment *segs = sizes_to_segments(&seglen, NULL, 0);
EXPECT(seglen == 0);
reftable_free(segs);
@@ -735,8 +735,7 @@ static void test_sizes_to_segments_empty(void)
static void test_sizes_to_segments_all_equal(void)
{
uint64_t sizes[] = { 5, 5 };
-
- int seglen = 0;
+ size_t seglen = 0;
struct segment *segs =
sizes_to_segments(&seglen, sizes, ARRAY_SIZE(sizes));
EXPECT(seglen == 1);
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 5/9] reftable/stack: use `size_t` to track stack slices during compaction
From: Patrick Steinhardt @ 2024-01-31 8:01 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706687982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4072 bytes --]
We use `int`s to track reftable slices when compacting the reftable
stack, which is considered to be a code smell in the Git project.
Convert the code to use `size_t` instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/stack.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/reftable/stack.c b/reftable/stack.c
index 2be3d1e4ba..c1f8cf1cef 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -24,7 +24,8 @@ static int stack_try_add(struct reftable_stack *st,
void *arg),
void *arg);
static int stack_write_compact(struct reftable_stack *st,
- struct reftable_writer *wr, int first, int last,
+ struct reftable_writer *wr,
+ size_t first, size_t last,
struct reftable_log_expiry_config *config);
static int stack_check_addition(struct reftable_stack *st,
const char *new_tab_name);
@@ -820,7 +821,8 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st)
return 1;
}
-static int stack_compact_locked(struct reftable_stack *st, int first, int last,
+static int stack_compact_locked(struct reftable_stack *st,
+ size_t first, size_t last,
struct strbuf *temp_tab,
struct reftable_log_expiry_config *config)
{
@@ -864,22 +866,21 @@ static int stack_compact_locked(struct reftable_stack *st, int first, int last,
}
static int stack_write_compact(struct reftable_stack *st,
- struct reftable_writer *wr, int first, int last,
+ struct reftable_writer *wr,
+ size_t first, size_t last,
struct reftable_log_expiry_config *config)
{
int subtabs_len = last - first + 1;
struct reftable_table *subtabs = reftable_calloc(
last - first + 1, sizeof(*subtabs));
struct reftable_merged_table *mt = NULL;
- int err = 0;
struct reftable_iterator it = { NULL };
struct reftable_ref_record ref = { NULL };
struct reftable_log_record log = { NULL };
-
uint64_t entries = 0;
+ int err = 0;
- int i = 0, j = 0;
- for (i = first, j = 0; i <= last; i++) {
+ for (size_t i = first, j = 0; i <= last; i++) {
struct reftable_reader *t = st->readers[i];
reftable_table_from_reader(&subtabs[j++], t);
st->stats.bytes += t->size;
@@ -963,7 +964,8 @@ static int stack_write_compact(struct reftable_stack *st,
}
/* < 0: error. 0 == OK, > 0 attempt failed; could retry. */
-static int stack_compact_range(struct reftable_stack *st, int first, int last,
+static int stack_compact_range(struct reftable_stack *st,
+ size_t first, size_t last,
struct reftable_log_expiry_config *expiry)
{
char **delete_on_success = NULL, **subtable_locks = NULL, **listp = NULL;
@@ -972,12 +974,10 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
struct strbuf lock_file_name = STRBUF_INIT;
struct strbuf ref_list_contents = STRBUF_INIT;
struct strbuf new_table_path = STRBUF_INIT;
+ size_t i, j, compact_count;
int err = 0;
int have_lock = 0;
int lock_file_fd = -1;
- int compact_count;
- int i = 0;
- int j = 0;
int is_empty_table = 0;
if (first > last || (!expiry && first == last)) {
@@ -1172,17 +1172,17 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,
int reftable_stack_compact_all(struct reftable_stack *st,
struct reftable_log_expiry_config *config)
{
- return stack_compact_range(st, 0, st->merged->stack_len - 1, config);
+ return stack_compact_range(st, 0, st->merged->stack_len ?
+ st->merged->stack_len - 1 : 0, config);
}
-static int stack_compact_range_stats(struct reftable_stack *st, int first,
- int last,
+static int stack_compact_range_stats(struct reftable_stack *st,
+ size_t first, size_t last,
struct reftable_log_expiry_config *config)
{
int err = stack_compact_range(st, first, last, config);
- if (err > 0) {
+ if (err > 0)
st->stats.failures++;
- }
return err;
}
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 6/9] reftable/stack: use `size_t` to track stack length
From: Patrick Steinhardt @ 2024-01-31 8:01 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706687982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6804 bytes --]
While the stack length is already stored as `size_t`, we frequently use
`int`s to refer to those stacks throughout the reftable library. Convert
those cases to use `size_t` instead to make things consistent.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/basics.c | 7 +++----
reftable/basics.h | 2 +-
reftable/merged.c | 11 +++++------
reftable/merged_test.c | 14 ++++++--------
reftable/reftable-merged.h | 2 +-
reftable/stack.c | 21 ++++++++++-----------
6 files changed, 26 insertions(+), 31 deletions(-)
diff --git a/reftable/basics.c b/reftable/basics.c
index af9004cec2..0785aff941 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -64,12 +64,11 @@ void free_names(char **a)
reftable_free(a);
}
-int names_length(char **names)
+size_t names_length(char **names)
{
char **p = names;
- for (; *p; p++) {
- /* empty */
- }
+ while (*p)
+ p++;
return p - names;
}
diff --git a/reftable/basics.h b/reftable/basics.h
index 4c3ac963a3..91f3533efe 100644
--- a/reftable/basics.h
+++ b/reftable/basics.h
@@ -44,7 +44,7 @@ void parse_names(char *buf, int size, char ***namesp);
int names_equal(char **a, char **b);
/* returns the array size of a NULL-terminated array of strings. */
-int names_length(char **names);
+size_t names_length(char **names);
/* Allocation routines; they invoke the functions set through
* reftable_set_alloc() */
diff --git a/reftable/merged.c b/reftable/merged.c
index 2031fd51b4..e2c6253324 100644
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -45,11 +45,10 @@ static int merged_iter_init(struct merged_iter *mi)
static void merged_iter_close(void *p)
{
struct merged_iter *mi = p;
- int i = 0;
+
merged_iter_pqueue_release(&mi->pq);
- for (i = 0; i < mi->stack_len; i++) {
+ for (size_t i = 0; i < mi->stack_len; i++)
reftable_iterator_destroy(&mi->stack[i]);
- }
reftable_free(mi->stack);
strbuf_release(&mi->key);
strbuf_release(&mi->entry_key);
@@ -168,14 +167,14 @@ static void iterator_from_merged_iter(struct reftable_iterator *it,
}
int reftable_new_merged_table(struct reftable_merged_table **dest,
- struct reftable_table *stack, int n,
+ struct reftable_table *stack, size_t n,
uint32_t hash_id)
{
struct reftable_merged_table *m = NULL;
uint64_t last_max = 0;
uint64_t first_min = 0;
- int i = 0;
- for (i = 0; i < n; i++) {
+
+ for (size_t i = 0; i < n; i++) {
uint64_t min = reftable_table_min_update_index(&stack[i]);
uint64_t max = reftable_table_max_update_index(&stack[i]);
diff --git a/reftable/merged_test.c b/reftable/merged_test.c
index e233a9d581..442917cc83 100644
--- a/reftable/merged_test.c
+++ b/reftable/merged_test.c
@@ -88,18 +88,17 @@ static struct reftable_merged_table *
merged_table_from_records(struct reftable_ref_record **refs,
struct reftable_block_source **source,
struct reftable_reader ***readers, int *sizes,
- struct strbuf *buf, int n)
+ struct strbuf *buf, size_t n)
{
- int i = 0;
struct reftable_merged_table *mt = NULL;
- int err;
struct reftable_table *tabs;
+ int err;
REFTABLE_CALLOC_ARRAY(tabs, n);
REFTABLE_CALLOC_ARRAY(*readers, n);
REFTABLE_CALLOC_ARRAY(*source, n);
- for (i = 0; i < n; i++) {
+ for (size_t i = 0; i < n; i++) {
write_test_table(&buf[i], refs[i], sizes[i]);
block_source_from_strbuf(&(*source)[i], &buf[i]);
@@ -263,18 +262,17 @@ static struct reftable_merged_table *
merged_table_from_log_records(struct reftable_log_record **logs,
struct reftable_block_source **source,
struct reftable_reader ***readers, int *sizes,
- struct strbuf *buf, int n)
+ struct strbuf *buf, size_t n)
{
- int i = 0;
struct reftable_merged_table *mt = NULL;
- int err;
struct reftable_table *tabs;
+ int err;
REFTABLE_CALLOC_ARRAY(tabs, n);
REFTABLE_CALLOC_ARRAY(*readers, n);
REFTABLE_CALLOC_ARRAY(*source, n);
- for (i = 0; i < n; i++) {
+ for (size_t i = 0; i < n; i++) {
write_test_log_table(&buf[i], logs[i], sizes[i], i + 1);
block_source_from_strbuf(&(*source)[i], &buf[i]);
diff --git a/reftable/reftable-merged.h b/reftable/reftable-merged.h
index 1a6d16915a..c91a2d83a2 100644
--- a/reftable/reftable-merged.h
+++ b/reftable/reftable-merged.h
@@ -33,7 +33,7 @@ struct reftable_table;
the stack array.
*/
int reftable_new_merged_table(struct reftable_merged_table **dest,
- struct reftable_table *stack, int n,
+ struct reftable_table *stack, size_t n,
uint32_t hash_id);
/* returns an iterator positioned just before 'name' */
diff --git a/reftable/stack.c b/reftable/stack.c
index c1f8cf1cef..079ba7fde8 100644
--- a/reftable/stack.c
+++ b/reftable/stack.c
@@ -202,18 +202,18 @@ static struct reftable_reader **stack_copy_readers(struct reftable_stack *st,
static int reftable_stack_reload_once(struct reftable_stack *st, char **names,
int reuse_open)
{
- int cur_len = !st->merged ? 0 : st->merged->stack_len;
+ size_t cur_len = !st->merged ? 0 : st->merged->stack_len;
struct reftable_reader **cur = stack_copy_readers(st, cur_len);
- int err = 0;
- int names_len = names_length(names);
+ size_t names_len = names_length(names);
struct reftable_reader **new_readers =
reftable_calloc(names_len, sizeof(*new_readers));
struct reftable_table *new_tables =
reftable_calloc(names_len, sizeof(*new_tables));
- int new_readers_len = 0;
+ size_t new_readers_len = 0;
struct reftable_merged_table *new_merged = NULL;
struct strbuf table_path = STRBUF_INIT;
- int i;
+ int err = 0;
+ size_t i;
while (*names) {
struct reftable_reader *rd = NULL;
@@ -221,11 +221,10 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names,
/* this is linear; we assume compaction keeps the number of
tables under control so this is not quadratic. */
- int j = 0;
- for (j = 0; reuse_open && j < cur_len; j++) {
- if (cur[j] && 0 == strcmp(cur[j]->name, name)) {
- rd = cur[j];
- cur[j] = NULL;
+ for (i = 0; reuse_open && i < cur_len; i++) {
+ if (cur[i] && 0 == strcmp(cur[i]->name, name)) {
+ rd = cur[i];
+ cur[i] = NULL;
break;
}
}
@@ -870,7 +869,7 @@ static int stack_write_compact(struct reftable_stack *st,
size_t first, size_t last,
struct reftable_log_expiry_config *config)
{
- int subtabs_len = last - first + 1;
+ size_t subtabs_len = last - first + 1;
struct reftable_table *subtabs = reftable_calloc(
last - first + 1, sizeof(*subtabs));
struct reftable_merged_table *mt = NULL;
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 7/9] reftable/merged: refactor seeking of records
From: Patrick Steinhardt @ 2024-01-31 8:01 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706687982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2639 bytes --]
The code to seek reftable records in the merged table code is quite hard
to read and does not conform to our coding style in multiple ways:
- We have multiple exit paths where we release resources even though
that is not really necessary
- We use a scoped error variable `e` which is hard to reason about.
This variable is not required at all.
- We allocate memory in the variable declarations, which is easy to
miss.
Refactor the function so that it becomes more maintainable in the
future.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/merged.c | 54 ++++++++++++++++++-----------------------------
1 file changed, 21 insertions(+), 33 deletions(-)
diff --git a/reftable/merged.c b/reftable/merged.c
index e2c6253324..0abcda26e8 100644
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -238,50 +238,38 @@ static int merged_table_seek_record(struct reftable_merged_table *mt,
struct reftable_iterator *it,
struct reftable_record *rec)
{
- struct reftable_iterator *iters = reftable_calloc(
- mt->stack_len, sizeof(*iters));
struct merged_iter merged = {
- .stack = iters,
.typ = reftable_record_type(rec),
.hash_id = mt->hash_id,
.suppress_deletions = mt->suppress_deletions,
.key = STRBUF_INIT,
.entry_key = STRBUF_INIT,
};
- int n = 0;
- int err = 0;
- int i = 0;
- for (i = 0; i < mt->stack_len && err == 0; i++) {
- int e = reftable_table_seek_record(&mt->stack[i], &iters[n],
- rec);
- if (e < 0) {
- err = e;
- }
- if (e == 0) {
- n++;
- }
- }
- if (err < 0) {
- int i = 0;
- for (i = 0; i < n; i++) {
- reftable_iterator_destroy(&iters[i]);
- }
- reftable_free(iters);
- return err;
+ struct merged_iter *p;
+ int err;
+
+ REFTABLE_CALLOC_ARRAY(merged.stack, mt->stack_len);
+ for (size_t i = 0; i < mt->stack_len; i++) {
+ err = reftable_table_seek_record(&mt->stack[i],
+ &merged.stack[merged.stack_len], rec);
+ if (err < 0)
+ goto out;
+ if (!err)
+ merged.stack_len++;
}
- merged.stack_len = n;
err = merged_iter_init(&merged);
- if (err < 0) {
+ if (err < 0)
+ goto out;
+
+ p = reftable_malloc(sizeof(struct merged_iter));
+ *p = merged;
+ iterator_from_merged_iter(it, p);
+
+out:
+ if (err < 0)
merged_iter_close(&merged);
- return err;
- } else {
- struct merged_iter *p =
- reftable_malloc(sizeof(struct merged_iter));
- *p = merged;
- iterator_from_merged_iter(it, p);
- }
- return 0;
+ return err;
}
int reftable_merged_table_seek_ref(struct reftable_merged_table *mt,
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 8/9] reftable/merged: refactor initialization of iterators
From: Patrick Steinhardt @ 2024-01-31 8:01 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706687982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]
Refactor the initialization of the merged iterator to fit our code style
better. This refactoring prepares the code for a refactoring of how
records are being initialized.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/merged.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/reftable/merged.c b/reftable/merged.c
index 0abcda26e8..0e60e2a39b 100644
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -19,24 +19,23 @@ license that can be found in the LICENSE file or at
static int merged_iter_init(struct merged_iter *mi)
{
- int i = 0;
- for (i = 0; i < mi->stack_len; i++) {
- struct reftable_record rec = reftable_new_record(mi->typ);
- int err = iterator_next(&mi->stack[i], &rec);
- if (err < 0) {
+ for (size_t i = 0; i < mi->stack_len; i++) {
+ struct pq_entry e = {
+ .rec = reftable_new_record(mi->typ),
+ .index = i,
+ };
+ int err;
+
+ err = iterator_next(&mi->stack[i], &e.rec);
+ if (err < 0)
return err;
- }
-
if (err > 0) {
reftable_iterator_destroy(&mi->stack[i]);
- reftable_record_release(&rec);
- } else {
- struct pq_entry e = {
- .rec = rec,
- .index = i,
- };
- merged_iter_pqueue_add(&mi->pq, &e);
+ reftable_record_release(&e.rec);
+ continue;
}
+
+ merged_iter_pqueue_add(&mi->pq, &e);
}
return 0;
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 9/9] reftable/record: improve semantics when initializing records
From: Patrick Steinhardt @ 2024-01-31 8:01 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1706687982.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7146 bytes --]
According to our usual coding style, the `reftable_new_record()`
function would indicate that it is allocating a new record. This is not
the case though as the function merely initializes records without
allocating any memory.
Replace `reftable_new_record()` with a new `reftable_record_init()`
function that takes a record pointer as input and initializes it
accordingly.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 18 +++++++++---------
reftable/merged.c | 8 +++++---
reftable/reader.c | 4 ++--
reftable/record.c | 43 ++++++++++--------------------------------
reftable/record.h | 10 +++++-----
reftable/record_test.c | 4 ++--
6 files changed, 33 insertions(+), 54 deletions(-)
diff --git a/reftable/block.c b/reftable/block.c
index 6952d0facf..9ad220747e 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -380,23 +380,23 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,
.key = *want,
.r = br,
};
- struct reftable_record rec = reftable_new_record(block_reader_type(br));
- int err = 0;
struct block_iter next = BLOCK_ITER_INIT;
+ struct reftable_record rec;
+ int err = 0, i;
- int i = binsearch(br->restart_count, &restart_key_less, &args);
if (args.error) {
err = REFTABLE_FORMAT_ERROR;
goto done;
}
- it->br = br;
- if (i > 0) {
- i--;
- it->next_off = block_reader_restart_offset(br, i);
- } else {
+ i = binsearch(br->restart_count, &restart_key_less, &args);
+ if (i > 0)
+ it->next_off = block_reader_restart_offset(br, i - 1);
+ else
it->next_off = br->header_off + 4;
- }
+ it->br = br;
+
+ reftable_record_init(&rec, block_reader_type(br));
/* We're looking for the last entry less/equal than the wanted key, so
we have to go one entry too far and then back up.
diff --git a/reftable/merged.c b/reftable/merged.c
index 0e60e2a39b..a0f222e07b 100644
--- a/reftable/merged.c
+++ b/reftable/merged.c
@@ -21,11 +21,11 @@ static int merged_iter_init(struct merged_iter *mi)
{
for (size_t i = 0; i < mi->stack_len; i++) {
struct pq_entry e = {
- .rec = reftable_new_record(mi->typ),
.index = i,
};
int err;
+ reftable_record_init(&e.rec, mi->typ);
err = iterator_next(&mi->stack[i], &e.rec);
if (err < 0)
return err;
@@ -57,10 +57,12 @@ static int merged_iter_advance_nonnull_subiter(struct merged_iter *mi,
size_t idx)
{
struct pq_entry e = {
- .rec = reftable_new_record(mi->typ),
.index = idx,
};
- int err = iterator_next(&mi->stack[idx], &e.rec);
+ int err;
+
+ reftable_record_init(&e.rec, mi->typ);
+ err = iterator_next(&mi->stack[idx], &e.rec);
if (err < 0)
return err;
diff --git a/reftable/reader.c b/reftable/reader.c
index 3e0de5e8ad..5e6c8f30a1 100644
--- a/reftable/reader.c
+++ b/reftable/reader.c
@@ -444,13 +444,13 @@ static int reader_start(struct reftable_reader *r, struct table_iter *ti,
static int reader_seek_linear(struct table_iter *ti,
struct reftable_record *want)
{
- struct reftable_record rec =
- reftable_new_record(reftable_record_type(want));
struct strbuf want_key = STRBUF_INIT;
struct strbuf got_key = STRBUF_INIT;
struct table_iter next = TABLE_ITER_INIT;
+ struct reftable_record rec;
int err = -1;
+ reftable_record_init(&rec, reftable_record_type(want));
reftable_record_key(want, &want_key);
while (1) {
diff --git a/reftable/record.c b/reftable/record.c
index 5c3fbb7b2a..8b84b8157f 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -1257,45 +1257,22 @@ reftable_record_vtable(struct reftable_record *rec)
abort();
}
-struct reftable_record reftable_new_record(uint8_t typ)
+void reftable_record_init(struct reftable_record *rec, uint8_t typ)
{
- struct reftable_record clean = {
- .type = typ,
- };
+ memset(rec, 0, sizeof(*rec));
+ rec->type = typ;
- /* the following is involved, but the naive solution (just return
- * `clean` as is, except for BLOCK_TYPE_INDEX), returns a garbage
- * clean.u.obj.offsets pointer on Windows VS CI. Go figure.
- */
switch (typ) {
- case BLOCK_TYPE_OBJ:
- {
- struct reftable_obj_record obj = { 0 };
- clean.u.obj = obj;
- break;
- }
- case BLOCK_TYPE_INDEX:
- {
- struct reftable_index_record idx = {
- .last_key = STRBUF_INIT,
- };
- clean.u.idx = idx;
- break;
- }
case BLOCK_TYPE_REF:
- {
- struct reftable_ref_record ref = { 0 };
- clean.u.ref = ref;
- break;
- }
case BLOCK_TYPE_LOG:
- {
- struct reftable_log_record log = { 0 };
- clean.u.log = log;
- break;
- }
+ case BLOCK_TYPE_OBJ:
+ return;
+ case BLOCK_TYPE_INDEX:
+ strbuf_init(&rec->u.idx.last_key, 0);
+ return;
+ default:
+ BUG("unhandled record type");
}
- return clean;
}
void reftable_record_print(struct reftable_record *rec, int hash_size)
diff --git a/reftable/record.h b/reftable/record.h
index fd80cd451d..e64ed30c80 100644
--- a/reftable/record.h
+++ b/reftable/record.h
@@ -69,9 +69,6 @@ struct reftable_record_vtable {
/* returns true for recognized block types. Block start with the block type. */
int reftable_is_block_type(uint8_t typ);
-/* return an initialized record for the given type */
-struct reftable_record reftable_new_record(uint8_t typ);
-
/* Encode `key` into `dest`. Sets `is_restart` to indicate a restart. Returns
* number of bytes written. */
int reftable_encode_key(int *is_restart, struct string_view dest,
@@ -100,8 +97,8 @@ struct reftable_obj_record {
/* record is a generic wrapper for different types of records. It is normally
* created on the stack, or embedded within another struct. If the type is
* known, a fresh instance can be initialized explicitly. Otherwise, use
- * reftable_new_record() to initialize generically (as the index_record is not
- * valid as 0-initialized structure)
+ * `reftable_record_init()` to initialize generically (as the index_record is
+ * not valid as 0-initialized structure)
*/
struct reftable_record {
uint8_t type;
@@ -113,6 +110,9 @@ struct reftable_record {
} u;
};
+/* Initialize the reftable record for the given type */
+void reftable_record_init(struct reftable_record *rec, uint8_t typ);
+
/* see struct record_vtable */
int reftable_record_equal(struct reftable_record *a, struct reftable_record *b, int hash_size);
void reftable_record_print(struct reftable_record *rec, int hash_size);
diff --git a/reftable/record_test.c b/reftable/record_test.c
index 999366ad46..a86cff5526 100644
--- a/reftable/record_test.c
+++ b/reftable/record_test.c
@@ -16,11 +16,11 @@
static void test_copy(struct reftable_record *rec)
{
- struct reftable_record copy = { 0 };
+ struct reftable_record copy;
uint8_t typ;
typ = reftable_record_type(rec);
- copy = reftable_new_record(typ);
+ reftable_record_init(©, typ);
reftable_record_copy_from(©, rec, GIT_SHA1_RAWSZ);
/* do it twice to catch memory leaks */
reftable_record_copy_from(©, rec, GIT_SHA1_RAWSZ);
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: what should "git clean -n -f [-d] [-x] <pattern>" do?
From: Sergey Organov @ 2024-01-31 13:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Elijah Newren, git
In-Reply-To: <87il3enc1i.fsf@osv.gnss.ru>
Sergey Organov <sorganov@gmail.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> I'm still arguing in favor of fixing "-n", and I believe a fix is needed
>>> independently from decision about "-f -f".
>>
>> Even though I do not personally like it, I do not think "which
>> between do-it (f) and do-not-do-it (n) do you want to use?" is
>> broken.
>
> Well, you are right, but "-n" is not documented as "do-not-do-it" in the
> sense you use it here.
>
>> It sometimes irritates me to find "git clean" (without "-f"
>> or "-n", and with clean.requireForce not disabled) complain, and I
>> personally think "git clean" when clean.requireForce is in effect
>> and no "-n" or "-f" were given should pretend as if "-n" were given.
>> I wish if it were "without -n or -f, we pretend as if -n were given,
>> possibly with a warning that says 'you need -f if you actually want
>> to carry out these operations'".
>
> Yep, then we'd not need "-n" that much, only if to cancel explicit "-f"
> (provided "-f -f" feature is removed.)
>
>>
>> But that is a separate usability issue.
>
> Yep, and that'd be very different design.
>
>>
>> What I find broken is that giving one 'f' and one 'n' in different
>> order, i.e. "-f -n" and "-n -f", does not do what I expect. If you
>> are choosing between do-it (f) and do-not-do-it (n), you ought to be
>> able to rely on the usual last-one-wins rule. That I find broken.
>
> I fail to see where this expectation comes from, provided "-n" is not
> documented as anything opposed to "-f":
>
> -n, --dry-run
> Don’t actually remove anything, just show what would be done.
>
> This is typical convenient description of "dry run", and current "-n"
> implementation is rather close to the description, that I'd still
> rewrite to emphasize the primary goal of the --dry-run:
>
>
> With these descriptions, the last thing that I'd expect is "-n -f"
> removing my files.
>
> Overall, as I see it, we have buggy implementation of suitably
> documented "--dry-run" option, and the best course is to fix the
> bug, with no semantic changes to the option itself.
OTOH, to preserve current actual behavior as much as possible, we can
probably first fix documentation like this:
-n, --dry-run
Show what would be done, and don’t actually remove anything.
This sets 'clean.requireForce' to 'false' for the duration
of this command execution.
that to me looks like a match for current observable behavior.
Then we can fix '-n' implementation exactly according to this updated
specification, making '-n' really independent from '-f', yet keeping
pure "git clean -n" as well as "git clean -f -n", and "git clean -n -f"
backward compatible.
As a bonus, the above solution will also free our hands in [re]defining
'-f -f' later, if needed.
WDYT?
Thanks,
-- Sergey Organov
^ permalink raw reply
* Re: Git in GSoC 2024
From: Patrick Steinhardt @ 2024-01-31 13:10 UTC (permalink / raw)
To: Christian Couder
Cc: Kaartic Sivaraam, git, Taylor Blau, Junio C Hamano, Victoria Dye
In-Reply-To: <Zbi8pfvGpYrlZXAu@tanuki>
[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]
On Tue, Jan 30, 2024 at 10:08:53AM +0100, Patrick Steinhardt wrote:
> On Tue, Jan 30, 2024 at 09:38:48AM +0100, Christian Couder wrote:
> > On Mon, Jan 29, 2024 at 7:16 PM Kaartic Sivaraam
> > <kaartic.sivaraam@gmail.com> wrote:
> [snip]
> > > The GSoC contributor application deadline is April 2 - 18:00 UTC, so
> > > (co-)mentors and org admins are already welcome to volunteer. As usual,
> > > we also need project ideas to refresh our idea page from last year
> > > (https://git.github.io/SoC-2023-Ideas/). Feel free to share your
> > > thoughts and discuss.
> >
> > I am volunteering as both an Org Admin and a mentor too.
> >
> > I am not sure how many tests there are left to be ported to the new
> > unit test framework. Patrick told me about porting some reftable unit
> > tests to the new unit test framework though. So it might still work as
> > a GSoC project.
>
> Yes, the tests in t0032-reftable-unittest.sh should be ported over to
> the new unit test framework eventually, and I think that this might be a
> good GSoC project indeed.
>
> If there is interest I'd also be happy to draft up some more topics in
> the context of refs and the reftable backend. I'm sure there should be
> some topics here that would be a good fit for the GSoC project, and I'd
> be happy to mentor any such project in this context.
I noticed that the starting period falls right into my honeymoon from
June 17th until July 19th. This unfortunately makes it quite a lot
harder for me to mentor projects alone. Still, I'd be happy to co-mentor
or help out in other ways.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox