* [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 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
* Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere
From: Junio C Hamano @ 2024-01-30 22:30 UTC (permalink / raw)
To: Willem Verstraeten; +Cc: Eric Sunshine, phillip.wood, git, Jeff King
In-Reply-To: <CAGX9RpF=tvPwiLO6UYA+uR5f2oqOLUSNaDL-jfn=T=BQ9FNtkQ@mail.gmail.com>
Willem Verstraeten <willem.verstraeten@gmail.com> writes:
> Sorry for dropping out of the conversation, but I see that the changes
> landed on the master, ready for the 2.44.0 release.
>
> Thank you very much!
Thanks for your initial report that led to the update.
^ permalink raw reply
* Draft of Git Rev News edition 107
From: Christian Couder @ 2024-01-30 22:29 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jakub Narebski, Markus Jansen, Kaartic Sivaraam,
Štěpán Němec, Taylor Blau,
Johannes Schindelin, Ævar Arnfjörð Bjarmason,
Bruno Brito, Brandon Pugh, Jeremy Pridmore, Elijah Newren,
Philip Oakley
Hi everyone,
A draft of a new Git Rev News edition is available here:
https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-107.md
Everyone is welcome to contribute in any section either by editing the
above page on GitHub and sending a pull request, or by commenting on
this GitHub issue:
https://github.com/git/git.github.io/issues/679
You can also reply to this email.
In general all kinds of contributions, for example proofreading,
suggestions for articles or links, help on the issues in GitHub,
volunteering for being interviewed and so on, are very much
appreciated.
I tried to Cc everyone who appears in this edition, but maybe I missed
some people, sorry about that.
Jakub, Markus, Kaartic and I plan to publish this edition on
Thursday February 1st, 2024.
Thanks,
Christian.
^ permalink raw reply
* Re: [PATCH 0/2] refs: introduce reftable backend
From: Junio C Hamano @ 2024-01-30 22:08 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Han-Wen Nienhuys
In-Reply-To: <cover.1706601199.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> Patrick Steinhardt (2):
> refs: introduce reftable backend
> ci: add jobs to test with the reftable backend
I haven't checked the code in refs/reftable-backend.c at all, but
judging from the fallout to other files, it issurprisingly cleanly
done. I somehow expected that we would need more changes like
exposing the helper function to compute shared permission bits,
but I see that it was the only change needed. Everything else done
to other files are natural requirement to plug the new thing as a
new ref backend to the system. Very nicely done.
Thanks.
> .github/workflows/main.yml | 9 +
> .gitlab-ci.yml | 9 +
> Documentation/ref-storage-format.txt | 2 +
> .../technical/repository-version.txt | 5 +-
> Makefile | 1 +
> ci/lib.sh | 2 +-
> ci/run-build-and-tests.sh | 3 +
> contrib/workdir/git-new-workdir | 2 +-
> path.c | 2 +-
> path.h | 1 +
> refs.c | 1 +
> refs/refs-internal.h | 1 +
> refs/reftable-backend.c | 2286 +++++++++++++++++
> repository.h | 5 +-
> t/t0610-reftable-basics.sh | 887 +++++++
> t/t0611-reftable-httpd.sh | 26 +
> t/test-lib.sh | 2 +
> 17 files changed, 3237 insertions(+), 7 deletions(-)
> create mode 100644 refs/reftable-backend.c
> create mode 100755 t/t0610-reftable-basics.sh
> create mode 100755 t/t0611-reftable-httpd.sh
^ permalink raw reply
* Re: [PATCH] t0091: allow test in a repository without tags
From: Junio C Hamano @ 2024-01-30 21:46 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git, Martin Ågren
In-Reply-To: <CAPig+cSmSjifRXsotTrdjjFsh0N4w+BzbZXKffPyn_rpirBVUw@mail.gmail.com>
Eric Sunshine <sunshine@sunshineco.com> writes:
> On Tue, Jan 30, 2024 at 2:03 PM Junio C Hamano <gitster@pobox.com> wrote:
>> The beginning of the [System Info] section, which should match the
>> "git version --build-options" output, may not identify our version
>> as "git version 2.whatever". When build in a repository cloned
>
> s/build/built/
Thanks.
^ permalink raw reply
* [PATCH] win32: special-case `ENOSPC` when writing to a pipe
From: Johannes Schindelin via GitGitGadget @ 2024-01-30 21:36 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Since c6d3cce6f3c4 (pipe_command(): handle ENOSPC when writing to a
pipe, 2022-08-17), one `write()` call that results in an `errno` value
`ENOSPC` (which typically indicates out of disk space, which makes
little sense in the context of a pipe) is treated the same as `EAGAIN`.
However, contrary to expectations, as diagnosed in
https://github.com/python/cpython/issues/101881#issuecomment-1428667015,
when writing to a non-blocking pipe on Windows, an `errno` value of
`ENOSPC` means something else: the write _fails_. Completely. Because
more data was provided than the internal pipe buffer can handle.
Somewhat surprising, considering that `write()` is allowed to write less
than the specified amount, e.g. by writing only as much as fits in that
buffer. But it doesn't, it writes no byte at all in that instance.
Let's handle this by manually detecting when an `ENOSPC` indicates that
a pipe's buffer is smaller than what needs to be written, and re-try
using the pipe's buffer size as `size` parameter.
It would be plausible to try writing the entire buffer in a loop,
feeding pipe buffer-sized chunks, but experiments show that trying to
write more than one buffer-sized chunk right after that will immediately
fail because the buffer is unlikely to be drained as fast as `write()`
could write again. And the whole point of a non-blocking pipe is to be
non-blocking.
Which means that the logic that determines the pipe's buffer size
unfortunately has to be run potentially many times when writing large
amounts of data to a non-blocking pipe, as there is no elegant way to
cache that information between `write()` calls. It's the best we can do,
though, so it has to be good enough.
This fix is required to let t3701.60 (handle very large filtered diff)
pass with the MSYS2 runtime provided by the MSYS2 project: Without this
patch, the failed write would result in an infinite loop. This patch is
not required with Git for Windows' variant of the MSYS2 runtime only
because Git for Windows added an ugly work-around specifically to avoid
a hang in that test case.
The diff is slightly chatty because it extends an already-existing
conditional that special-cases a _different_ `errno` value for pipes,
and because this patch needs to account for the fact that
`_get_osfhandle()` potentially overwrites `errno`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
win32: special-case ENOSPC when writing to a pipe
This is part of my (slow, after-hours) effort to chip away at the
deviations between Git for Windows' variant of the MSYS2 runtime and
MSYS2's variant.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1648%2Fdscho%2Fwin32-pipe-enospc-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1648/dscho/win32-pipe-enospc-fix-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1648
compat/mingw.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 238a84ddbaa..320fb99a90e 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -707,13 +707,24 @@ ssize_t mingw_write(int fd, const void *buf, size_t len)
{
ssize_t result = write(fd, buf, len);
- if (result < 0 && errno == EINVAL && buf) {
+ if (result < 0 && (errno == EINVAL || errno == ENOSPC) && buf) {
+ int orig = errno;
+
/* check if fd is a pipe */
HANDLE h = (HANDLE) _get_osfhandle(fd);
- if (GetFileType(h) == FILE_TYPE_PIPE)
+ if (GetFileType(h) != FILE_TYPE_PIPE)
+ errno = orig;
+ else if (orig == EINVAL)
errno = EPIPE;
- else
- errno = EINVAL;
+ else {
+ DWORD buf_size;
+
+ if (!GetNamedPipeInfo(h, NULL, NULL, &buf_size, NULL))
+ buf_size = 4096;
+ if (len > buf_size)
+ return write(fd, buf, buf_size);
+ errno = orig;
+ }
}
return result;
base-commit: c5b454771e6b086f60c7f1f139025f174bcedac9
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] t0091: allow test in a repository without tags
From: Eric Sunshine @ 2024-01-30 20:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Martin Ågren
In-Reply-To: <xmqqv87aabk3.fsf@gitster.g>
On Tue, Jan 30, 2024 at 2:03 PM Junio C Hamano <gitster@pobox.com> wrote:
> The beginning of the [System Info] section, which should match the
> "git version --build-options" output, may not identify our version
> as "git version 2.whatever". When build in a repository cloned
s/build/built/
> without tags, for example, "git version unknown.g00000000" can be a
> legit version string.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [PATCH] merge-tree: accept 3 trees as arguments
From: Johannes Schindelin @ 2024-01-30 20:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <xmqqplxoozh9.fsf@gitster.g>
Hi Junio,
On Fri, 26 Jan 2024, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When specifying a merge base explicitly, there is actually no good
> > reason why the inputs need to be commits: that's only needed if the
> > merge base has to be deduced from the commit graph.
>
> yes, Yes, YES, YEAHHHHH!
:-D
> > diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> > index 12ac4368736..71f21bb834f 100755
> > --- a/t/t4301-merge-tree-write-tree.sh
> > +++ b/t/t4301-merge-tree-write-tree.sh
> > @@ -945,4 +945,12 @@ test_expect_success 'check the input format when --stdin is passed' '
> > test_cmp expect actual
> > '
> >
> > +test_expect_success '--merge-base with tree OIDs' '
> > + git merge-tree --merge-base=side1^ side1 side3 >tree &&
> > + tree=$(cat tree) &&
> > + git merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >tree2 &&
> > + tree2=$(cat tree2) &&
> > + test $tree = $tree2
> > +'
>
> You do not need $tree and $tree2 variables that would make it harder
> to diagnose a failure case when we break merge-tree. You have tree
> and tree2 as files, and I think it is sufficient to do
>
> git merge-tree ... >result-from-commits &&
> git merge-tree ... >result-from-trees &&
> test_cmp result-from-commits result-from-trees
That's valuable feedback, thank you! As you saw, I changed it accordingly
in v2.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH] merge-tree: accept 3 trees as arguments
From: Johannes Schindelin @ 2024-01-30 20:04 UTC (permalink / raw)
To: Elijah Newren; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <CABPp-BFPe_RrX5ZHo7-mMHHS96j_O+1wiEwGC5+zGPP5h+686Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3788 bytes --]
Hi Elijah,
On Mon, 29 Jan 2024, Elijah Newren wrote:
> On Fri, Jan 26, 2024 at 6:18 AM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > Documentation/git-merge-tree.txt | 5 +++-
> > builtin/merge-tree.c | 42 +++++++++++++++++++-------------
> > t/t4301-merge-tree-write-tree.sh | 8 ++++++
> > 3 files changed, 37 insertions(+), 18 deletions(-)
> >
> > diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> > index b50acace3bc..214e30c70ba 100644
> > --- a/Documentation/git-merge-tree.txt
> > +++ b/Documentation/git-merge-tree.txt
> > @@ -64,10 +64,13 @@ OPTIONS
> > share no common history. This flag can be given to override that
> > check and make the merge proceed anyway.
> >
> > ---merge-base=<commit>::
> > +--merge-base=<tree-ish>::
>
> A very minor point, but any chance we can just use `<tree>`, like
> git-restore does? I've never liked the '-ish' that we use as it seems
> to throw users, and I think they understand that they can use a commit
> or a tag where a tree is expected
That's funny: I asked Victoria Dye to look over the patch, and she pointed
out the exact opposite: I had written `<tree>` and she remarked that most
of Git's manual pages would call this a `<tree-ish>` :-)
On another funny note, I tried to establish the term "ent" for this category
almost 222 months ago because I also disliked the "-ish" convention:
https://lore.kernel.org/git/Pine.LNX.4.63.0508051655480.8418@wgmdd8.biozentrum.uni-wuerzburg.de/
> > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> > index 3bdec53fbe5..cbd8e15af6d 100644
> > --- a/builtin/merge-tree.c
> > +++ b/builtin/merge-tree.c
> > @@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,
> > struct merge_options opt;
> >
> > copy_merge_options(&opt, &o->merge_options);
> > - parent1 = get_merge_parent(branch1);
> > - if (!parent1)
> > - help_unknown_ref(branch1, "merge-tree",
> > - _("not something we can merge"));
> > -
> > - parent2 = get_merge_parent(branch2);
> > - if (!parent2)
> > - help_unknown_ref(branch2, "merge-tree",
> > - _("not something we can merge"));
> > -
> > opt.show_rename_progress = 0;
> >
> > opt.branch1 = branch1;
> > opt.branch2 = branch2;
>
> If branch1 and branch2 refer to trees, then when users hit conflicts
> they'll see e.g.
>
> <<<<<<< aaaaaaa
> somecode();
> =======
> othercode();
> >>>>>>> bbbbbbb
>
> but aaaaaaa and bbbbbbb are not commits that they can find.
That is true. And it is not totally obvious to many users that they could
then call `git show aaaaaaa:file` to see the full pre-image on the
first-parent side.
On the other hand, the label that is shown is precisely what the user
specified on the command-line. For example:
$ git merge-tree --merge-base=v2.42.0:t v2.43.0~11:t v2.43.0~10^2:t
will result in the following conflict markers:
$ git show 021c3ce211:t0091-bugreport.sh
[...]
<<<<<<< v2.43.0~11:t
grep usage output &&
test_path_is_missing git-bugreport-*
'
test_expect_success 'incorrect positional arguments abort with usage and hint' '
test_must_fail git bugreport false 2>output &&
grep usage output &&
grep false output &&
=======
test_grep usage output &&
>>>>>>> v2.43.0~10^2:t
[...]
which I personally find very pleasing output.
Besides, the manual page of `git merge-tree` says in no sugar-coated
words:
Do NOT look through the resulting toplevel tree to try to find which
files conflict; [...]
:-)
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 1/4] t0080: mark as leak-free
From: Junio C Hamano @ 2024-01-30 20:00 UTC (permalink / raw)
To: Jeff King; +Cc: Rubén Justo, Git List
In-Reply-To: <20240130055333.GB166761@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Setting the flag now just makes sure we continue correctly on that path,
> rather than getting surprised near the end of the road that t-basic has
> some dumb leak. Plus it avoids the script popping up as a false positive
> when checking for scripts which can be marked.
Alright. Any such "dumb leak" in the "basic" would hopefully be
caught by the real t-$other unit tests exercised under the leak
sanitizer, I hope, but it is not worth our time wondering if it
makes sense to special case t0080 specifically.
^ permalink raw reply
* Re: [PATCH] reftable/stack: fsync "tables.list" during compaction
From: Justin Tobler @ 2024-01-30 19:04 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, John Cai
In-Reply-To: <7bdafc9bd7f53f38a24d69a563615b6ad484e1ba.1706592127.git.ps@pks.im>
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
>
> 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.
-Justin
^ permalink raw reply
* [PATCH] t0091: allow test in a repository without tags
From: Junio C Hamano @ 2024-01-30 19:03 UTC (permalink / raw)
To: git; +Cc: Martin Ågren
The beginning of the [System Info] section, which should match the
"git version --build-options" output, may not identify our version
as "git version 2.whatever". When build in a repository cloned
without tags, for example, "git version unknown.g00000000" can be a
legit version string.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t0091-bugreport.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git c/t/t0091-bugreport.sh w/t/t0091-bugreport.sh
index 8798feefe3..fca39048fe 100755
--- c/t/t0091-bugreport.sh
+++ w/t/t0091-bugreport.sh
@@ -39,9 +39,9 @@ test_expect_success 'sanity check "System Info" section' '
sed -ne "/^\[System Info\]$/,/^$/p" <git-bugreport-format.txt >system &&
- # The beginning should match "git version --build-info" verbatim,
+ # The beginning should match "git version --build-options" verbatim,
# but rather than checking bit-for-bit equality, just test some basics.
- grep "git version [0-9]." system &&
+ grep "git version " system &&
grep "shell-path: ." system &&
# After the version, there should be some more info.
^ permalink raw reply related
* Re: [PATCH 1/4] t0080: mark as leak-free
From: Rubén Justo @ 2024-01-30 18:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqwmrrg0l6.fsf@gitster.g>
On 29-ene-2024 15:51:33, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
>
> >> The point of the t-basic tests is to ensure the lightweight unit
> >> test framework that requires nothing from Git behaves (and keeps
> >> behaving) sensibly. The point of running t[0-9][0-9][0-9][0-9]
> >> tests under leak sanitizer is to exercise production Git code to
> >> catch leaks in Git code.
> >>
> >> So it is not quite clear if we even want to run this t0080 under
> >> leak sanitizer to begin with. t0080 is a relatively tiny test, but
> >> do we even want to spend leak sanitizer cycles on it? I dunno.
> >
> > IIUC, that would imply building test-tool with a different set of flags
> > than Git, new artifacts ... or running test-tool with some LSAN_OPTIONS
> > options, to disable it ... or both ... or ...
> >
> > And that is assuming that with test-tool we won't catch a leak in Git
> > that we're not seeing in the other tests ...
>
> But t0080 does not even run test-tool, does it? The t-basic unit
> test is about testing the unit test framework and does not even
> trigger any of the half-libified Git code. So I am not sure why
> you are bringing up test-tool into the picture.
Of course, test-tool has nothing to do here. I think I got distracted
because:
$ ( cd t; ./t0080-unit-test-output.sh )
Bail out! You need to build test-tool; Run "make t/helper/test-tool" in the source (toplevel) directory
My reasoning was about t/unit-test/bin/t-basic (though also applies to
test-tool), due to:
$ make SANITIZE=leak -n t/unit-tests/bin/t-basic
...
echo ' ' LINK t/unit-tests/bin/t-basic;cc -g -O2 -Wall -I. \
-DHAVE_SYSINFO -fsanitize=leak -fno-sanitize-recover=leak \
-fno-omit-frame-pointer -DSUPPRESS_ANNOTATED_LEAKS -O0 \
-DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H -DUSE_CURL_FOR_IMAP_SEND \
-DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES \
-DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 \
-DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"git-compat-util.h\"" \
-DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK \
-DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME \
-DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_GETDELIM \
'-DPROCFS_EXECUTABLE_PATH="/proc/self/exe"' -DFREAD_READS_DIRECTORIES \
-DNO_STRLCPY -DSHELL_PATH='"/bin/sh"' -o t/unit-tests/bin/t-basic \
t/unit-tests/t-basic.o t/unit-tests/test-lib.o common-main.o libgit.a \
xdiff/lib.a reftable/libreftable.a libgit.a xdiff/lib.a \
reftable/libreftable.a libgit.a -lz -lpthread -lrt
Note that we inject this flags:
-fsanitize=leak -fno-sanitize-recover=leak -fno-omit-frame-pointer \
-DSUPPRESS_ANNOTATED_LEAKS -O0
>
> > Maybe this is tangential to this series but, while a decision is being
> > made, annotating the test makes GIT_TEST_PASSING_SANITIZE_LEAK=check
> > pass, which is the objective in this series.
>
> One major reason why we want to set TEST_PASSES_SANITIZE_LEAK to
> true is because that way the marked test will be run under the leak
> sanitizer in the CI.
>
> What do we expect to gain by running t0080, which is to run the
> t-basic unit test, under the leak sanitizer? Unlike other
> t[0-9][0-9][0-9][0-9] tests that exercise Git production code, would
> we care about a new leak found in t-basic run from t0080 in the
> first place?
>
> Annotating with TEST_PASSES_SANITIZE_LEAK is not a goal by itself.
Indeed. It points to a horizon.
> Annotating the tests that we want to run under the sanitizer and see
> them passing with it is.
Maybe this is also a horizon (not reachable by definition), and
expecting "make test" to be leak-free (including t0080) a good path
towards that horizon, IMHO. But you are right, those leak sanitizer
cycles may not be worth it.
> And obviously these tests that exercise
> Git production code are very good candidates for us to do so. It is
> unclear if t0080 falls into the same category. That is why I asked
> what we expect to gain by running it.
>
> Thanks.
Thank you for bringing up a good question.
I see you queued this as 4/4. OK. I'll consider that if a re-roll for
this series is needed.
^ permalink raw reply
* [RFH] disable bogus .maybe_tree suggestion by Coccinelle?
From: Junio C Hamano @ 2024-01-30 18:08 UTC (permalink / raw)
To: git; +Cc: Josh Steadmon
Josh at $WORK noticed that "make coccicheck" shows tons of false
positives to tell us to avoid direct access to the .maybe_tree
member of a commit object, even for those that are meant to be
excluded from the rewrite rule, like this:
diff -u -p a/commit-graph.c b/commit-graph.c
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -905,7 +905,7 @@ static void fill_commit_graph_info(struc
static inline void set_commit_tree(struct commit *c, struct tree *t)
{
- c->maybe_tree = t;
+ repo_get_commit_tree(specify_the_right_repo_here, c) = t;
}
static int fill_commit_in_graph(struct repository *r,
It turns out that "make coccicheck" shows the same undesirable
suggestions, even in a checkout of 301b8c7f (commit.c: add
repo_get_commit_tree(), 2019-04-16) that added these rules.
If nobody has a better idea (which would obviously to "fix" it so
that we still are reminded to think twice before directly accessing
the .maybe_tree member without these false positives), I'd propose
to remove these rules.
The box I observed the sympotom had this
$ spatch --version
spatch version 1.1.1 compiled with OCaml version 4.14.1
Flags passed to the configure script: --prefix=/usr --sysconfdir=/etc --libdir=/usr/lib --enable-ocaml --enable-python --with-python=python3 --enable-opt
OCaml scripting support: yes
Python scripting support: yes
Syntax of regular expressions: Str
---
contrib/coccinelle/commit.cocci | 23 -----------------------
1 file changed, 23 deletions(-)
diff --git c/contrib/coccinelle/commit.cocci w/contrib/coccinelle/commit.cocci
index af6dd4c20c..397f01b9ff 100644
--- c/contrib/coccinelle/commit.cocci
+++ w/contrib/coccinelle/commit.cocci
@@ -10,29 +10,6 @@ expression c;
- c->maybe_tree->object.oid.hash
+ get_commit_tree_oid(c)->hash
-@@
-identifier f !~ "^set_commit_tree$";
-expression c;
-expression s;
-@@
- f(...) {<...
-- c->maybe_tree = s
-+ set_commit_tree(c, s)
- ...>}
-
-// These excluded functions must access c->maybe_tree directly.
-// Note that if c->maybe_tree is written somewhere outside of these
-// functions, then the recommended transformation will be bogus with
-// repo_get_commit_tree() on the LHS.
-@@
-identifier f !~ "^(repo_get_commit_tree|get_commit_tree_in_graph_one|load_tree_for_commit|set_commit_tree)$";
-expression c;
-@@
- f(...) {<...
-- c->maybe_tree
-+ repo_get_commit_tree(specify_the_right_repo_here, c)
- ...>}
-
@@
struct commit *c;
expression E;
^ permalink raw reply related
* Re: [PATCH] merge-tree: accept 3 trees as arguments
From: Junio C Hamano @ 2024-01-30 17:15 UTC (permalink / raw)
To: Elijah Newren
Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin
In-Reply-To: <CABPp-BFPe_RrX5ZHo7-mMHHS96j_O+1wiEwGC5+zGPP5h+686Q@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
>> ---merge-base=<commit>::
>> +--merge-base=<tree-ish>::
>
> A very minor point, but any chance we can just use `<tree>`, like
> git-restore does? I've never liked the '-ish' that we use as it seems
> to throw users, and I think they understand that they can use a commit
> or a tag where a tree is expected
You are right that the "X-ish" was invented exactly so that folks
(including the nitpicky types among us) can tell if only "X" is
accepted, or other types of objects that uniquely can be peeled to a
"X" also can be used. Many commands at the Porcelain level may take
a commit or a tag that eventually peel to a tree where at the lowest
level we only need it to be a tree, but it still matters in some
lower level corners of the system where the plumbing commands that
only accept a tree but not a tree-ish exist. We've discussed this
in the past, e.g.:
https://lore.kernel.org/git/7voc8ihq4a.fsf@alter.siamese.dyndns.org/
In general, I am OK to update the documentation and usage strings to
drop the "-ish" suffix when it is clear from the context. But what
does it exactly mean that "--merge-base=<tree>" is meant to take any
tree-ish from the context of this documentation we are discussing?
- Is it because merge-tree is a Porcelain command and we would
adopt "Porcelains are to peel liberally what they accept, and
when they are documented to take X they always accept any X-ish"
rule?
- Is it because the description that follows "--merge-base=<tree>"
header does not mention "contrary to our usual convention, this
option only accepts a tree and not a commit or a tag that points
at a tree" and we would adopt "all commands and options that are
documented to take X take X-ish, unless explicitly documented
they only take X"?
As long as we clearly spell out such a ground rule and make sure
readers of the documentation understand it, I will not object to
such a tree-wide clean-up. The current ground rule is "We write X
when we mean to take only X and we write X-ish otherwise", if I am
not mistaken.
>> opt.branch1 = branch1;
>> opt.branch2 = branch2;
>
> If branch1 and branch2 refer to trees, then when users hit conflicts
> they'll see e.g.
>
> <<<<<<< aaaaaaa
> somecode();
> =======
> othercode();
>>>>>>>> bbbbbbb
>
> but aaaaaaa and bbbbbbb are not commits that they can find.
Correct. They are what they fed as two trees to be merged, aren't
they? They (or the script that drives merge-tree and fed these two
trees) should be recognise these two trees, as long as they are told
that is what we show, no?
> That raises the question: if the user passes trees in, should we
> require helpful names be provided as additional parameters?
If the user wants to give human-readable name to override whatever
the code would internally produce, such new options may help them,
and should probably apply equally to input that happens to be a
commit (or a tag that is a tree-ish) as well, I would think.
> Or are
> the usecases such that we don't expect callers to have any useful
> information about where these trees come from and these suboptimal
> conflicts are the best we can do?
I do not necessarily think the output is "suboptimal" from the point
of view of our users, exactly because that is what they fed into the
command themselves.
^ permalink raw reply
* Re: [PATCH v3] reftable/stack: adjust permissions of compacted tables
From: Justin Tobler @ 2024-01-30 16:56 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Karthik Nayak
In-Reply-To: <a211818108053754aca002726d0206623a347952.1706263589.git.ps@pks.im>
On Fri, Jan 26, 2024 at 4:09 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> When creating a new compacted table from a range of preexisting ones we
> don't set the default permissions on the resulting table when specified
> by the user. This has the effect that the "core.sharedRepository" config
> will not be honored correctly.
>
> Fix this bug and add a test to catch this issue. Note that we only test
> on non-Windows platforms because Windows does not use POSIX permissions
> natively.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>
> Changes compared to v2:
>
> - Extended the commit message to say why we don't test on Windows
> systems.
>
> - Renamed the `scratch` variable to `path`.
Thanks Patrick! This version looks good to me. I have nothing else to add
-Justin
^ permalink raw reply
* Re: [RFC PATCH 1/2] add-patch: compare object id instead of literal string
From: Junio C Hamano @ 2024-01-30 16:42 UTC (permalink / raw)
To: Ghanshyam Thakkar; +Cc: Patrick Steinhardt, git
In-Reply-To: <CYRU26F9KCDF.2XI7VRT7N04OC@gmail.com>
"Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:
> Yeah, my original motive was to support '@' as a shorthand for HEAD.
> But, since '@' can also be used as branch name, I thought of comparing
> object ids instead of string comparison in accordance with the
> NEEDSWORK comment. However, as Junio pointed out, treating a branch
> name revision that points to same commit as HEAD, as HEAD would just
> cause confusion.
FWIW, if we are not doing so in our documentation already, we may
want to discourage use of "refs/heads/@", given that "@" is used as
a synonym for "HEAD" in some[*] contexts, specifying the HEAD
(i.e. "work on the branch that is currently checked out, or in the
detached state") and specifying the concrete name of a branch
(i.e. "work on this branch") mean totally different things and may
result in (what may appear to the user as a) confusing behaviour.
Granted, the user who names their branch "@" is only hurting
themselves and it falls into the "Doctor, it hurts when I do
this. Then don't do that!" category.
But the documentation is where we tell them "Then don't do that!"
and we should know better how it hurts when they do so than those
who learn from the documentation, so ...
[Footnote]
* Even use of "@" as a synonym for "HEAD" may want to be
discouraged, as there are still unnecessary differences between
them that are not worth our engineering resource to fix. Do
people know what "git checkout HEAD" and "git checkout @" do, for
example?
^ permalink raw reply
* Re: [PATCH] diff: handle NULL meta-info when spawning external diff
From: Junio C Hamano @ 2024-01-30 16:29 UTC (permalink / raw)
To: Jeff King; +Cc: Wilfred Hughes, git
In-Reply-To: <20240130060658.GD166761@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The current behavior is somewhere in between, though. You get an "other"
> name passed to the external diff, but the metainfo argument makes no
> mention of a rename (it's either blank for an exact rename, or may
> contain an "index" line if there was a content change).
>
> I'm not sure anybody really cares that much either way, though. It's
> external diff, which I suspect hardly anybody uses, and those extra
> fields aren't even documented in the first place.
Oh, we probably should fix the documentation eventually, then.
But I agree that in this case, whatever stops the segfault would be
good enough.
I am surprised to learn that this 8th hidden parameter dates back to
427dcb4b ([PATCH] Diff overhaul, adding half of copy detection.,
2005-05-21), and it is more surprising that even before it happened,
the external diff interface with 7 parameters was already
documented, which happened with 03ea2802 ([PATCH 2/2] core-git
documentation update, 2005-05-08). Before the addition of the copy
detection, the presence of the "other" was how you learned if we saw
a rename (because there was no copy, the only reason "other" is
there was due to a rename). With copy detection added, extra bits
of information needed to be passed and we started passing the
xfrm_msg as well through the interface. At least, by dumping it to
the end-user, an external diff driver could help the end-user tell
if that "other" came from a rename or from a copy, even if it did
not understand it itself.
And of course, after merely 6 weeks since the inception, Git did not
have the "--no-index" mode (we did not even have a unified "git
diff" frontend), so this was never a problem back then.
^ permalink raw reply
* Re: [PATCH 0/4] mark tests as leak-free
From: Junio C Hamano @ 2024-01-30 16:01 UTC (permalink / raw)
To: Jeff King; +Cc: Rubén Justo, Git List
In-Reply-To: <20240130055448.GC166761@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Jan 29, 2024 at 10:04:10PM +0100, Rubén Justo wrote:
>
>> The tests: t0080, t5332 and t6113 can be annotated as leak-free.
>>
>> I used:
>> $ make SANITIZE=leak GIT_TEST_PASSING_SANITIZE_LEAK=check GIT_TEST_SANITIZE_LEAK_LOG=true test
>>
>> Rubén Justo (4):
>> t0080: mark as leak-free
>> t5332: mark as leak-free
>> t6113: mark as leak-free
>> test-lib: check for TEST_PASSES_SANITIZE_LEAK
>
> These all looked reasonable to me. Thank you for not just fixing them,
> but including the background for each case (e.g., leak-free as of commit
> XYZ, etc).
Yup, the background description was very useful to read.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/2] checkout: forbid "-B <branch>" from touching a branch used elsewhere
From: Willem Verstraeten @ 2024-01-30 12:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Sunshine, phillip.wood, git, Jeff King
In-Reply-To: <xmqqsf4c39e9.fsf@gitster.g>
Hi all,
Sorry for dropping out of the conversation, but I see that the changes
landed on the master, ready for the 2.44.0 release.
Thank you very much!
On Fri, 8 Dec 2023 at 18:13, Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
> > Needs review and documentation updates.
> >
> > I'm not sure if the "Needs review" comment is still applicable since
> > the patch did get some review comments, however, the mentioned
> > documentation update is probably still needed for this series to
> > graduate.
>
> Thanks. I think "-B" being defined as "branch -f <branch>" followed
> by "checkout <branch>" makes it technically unnecessary to add any
> new documentation (because "checkout <branch>" will refuse, so it
> naturally follows that "checkout -B <branch>" should), but giving
> the failure mode a bit more explicit mention would be more helpful
> to readers.
>
> Here is to illustrate what I have in mind. The mention of the
> "transactional" was already in the documentation for the "checkout"
> back when switch was described at d787d311 (checkout: split part of
> it to new command 'switch', 2019-03-29), but somehow was left out in
> the documentation of the "switch". While it is not incorrect to say
> that it is a convenient short-cut, it is more important to say what
> happens when one of them fails, so I am tempted to port that
> description over to the "switch" command, and give the "used elsewhere"
> as a sample failure mode.
>
> The test has been also enhanced to check the "transactional" nature.
>
> Documentation/git-checkout.txt | 4 +++-
> Documentation/git-switch.txt | 9 +++++++--
> t/t2400-worktree-add.sh | 18 ++++++++++++++++--
> 3 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git c/Documentation/git-checkout.txt w/Documentation/git-checkout.txt
> index 240c54639e..55a50b5b23 100644
> --- c/Documentation/git-checkout.txt
> +++ w/Documentation/git-checkout.txt
> @@ -63,7 +63,9 @@ $ git checkout <branch>
> ------------
> +
> that is to say, the branch is not reset/created unless "git checkout" is
> -successful.
> +successful (e.g., when the branch is in use in another worktree, not
> +just the current branch stays the same, but the branch is not reset to
> +the start-point, either).
>
> 'git checkout' --detach [<branch>]::
> 'git checkout' [--detach] <commit>::
> diff --git c/Documentation/git-switch.txt w/Documentation/git-switch.txt
> index c60fc9c138..6137421ede 100644
> --- c/Documentation/git-switch.txt
> +++ w/Documentation/git-switch.txt
> @@ -59,13 +59,18 @@ out at most one of `A` and `B`, in which case it defaults to `HEAD`.
> -c <new-branch>::
> --create <new-branch>::
> Create a new branch named `<new-branch>` starting at
> - `<start-point>` before switching to the branch. This is a
> - convenient shortcut for:
> + `<start-point>` before switching to the branch. This is the
> + transactional equivalent of
> +
> ------------
> $ git branch <new-branch>
> $ git switch <new-branch>
> ------------
> ++
> +that is to say, the branch is not reset/created unless "git switch" is
> +successful (e.g., when the branch is in use in another worktree, not
> +just the current branch stays the same, but the branch is not reset to
> +the start-point, either).
>
> -C <new-branch>::
> --force-create <new-branch>::
> diff --git c/t/t2400-worktree-add.sh w/t/t2400-worktree-add.sh
> index bbcb2d3419..5d5064e63d 100755
> --- c/t/t2400-worktree-add.sh
> +++ w/t/t2400-worktree-add.sh
> @@ -129,8 +129,22 @@ test_expect_success 'die the same branch is already checked out' '
> test_expect_success 'refuse to reset a branch in use elsewhere' '
> (
> cd here &&
> - test_must_fail git checkout -B newmain 2>actual &&
> - grep "already used by worktree at" actual
> +
> + # we know we are on detached HEAD but just in case ...
> + git checkout --detach HEAD &&
> + git rev-parse --verify HEAD >old.head &&
> +
> + git rev-parse --verify refs/heads/newmain >old.branch &&
> + test_must_fail git checkout -B newmain 2>error &&
> + git rev-parse --verify refs/heads/newmain >new.branch &&
> + git rev-parse --verify HEAD >new.head &&
> +
> + grep "already used by worktree at" error &&
> + test_cmp old.branch new.branch &&
> + test_cmp old.head new.head &&
> +
> + # and we must be still on the same detached HEAD state
> + test_must_fail git symbolic-ref HEAD
> )
> '
>
^ permalink raw reply
* Re: Git in GSoC 2024
From: Patrick Steinhardt @ 2024-01-30 9:08 UTC (permalink / raw)
To: Christian Couder
Cc: Kaartic Sivaraam, git, Taylor Blau, Junio C Hamano, Victoria Dye
In-Reply-To: <CAP8UFD1VAvnkM6afZvtpdXhA4csDBDwMnF9yUzSx_ut-Ypf+eA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]
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.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH] gitk: add "Hightlight commit name" menu entry
From: Raphael Gallais-Pou @ 2024-01-30 8:53 UTC (permalink / raw)
To: git, David Aguilar, Junio C Hamano, Denton Liu, Paul Mackerras,
Beat Bolli
When working with diverged branches, some patches can appear several times
on different branches without having the need to merge those branches.
On the other hand you may have to port a specific patch on another
branch you are working on. The search with a SHA1 cannot be applied here
since they would differ.
This patch adds an entry in the main context menu to highlight every
instance of a commit.
Signed-off-by: Raphael Gallais-Pou <rgallaispou@gmail.com>
---
gitk-git/gitk | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/gitk-git/gitk b/gitk-git/gitk
index 7a087f123d..4b15230a16 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2672,6 +2672,7 @@ proc makewindow {} {
{mc "Make patch" command mkpatch}
{mc "Create tag" command mktag}
{mc "Copy commit reference" command copyreference}
+ {mc "Highlight commit name" command highlightcommitname}
{mc "Write commit to file" command writecommit}
{mc "Create new branch" command mkbranch}
{mc "Cherry-pick this commit" command cherrypick}
@@ -9002,13 +9003,13 @@ proc rowmenu {x y id} {
if {$id ne $nullid && $id ne $nullid2} {
set menu $rowctxmenu
if {$mainhead ne {}} {
- $menu entryconfigure 8 -label [mc "Reset %s branch to here" $mainhead] -state normal
+ $menu entryconfigure 9 -label [mc "Reset %s branch to here" $mainhead] -state normal
} else {
- $menu entryconfigure 8 -label [mc "Detached head: can't reset" $mainhead] -state disabled
+ $menu entryconfigure 9 -label [mc "Detached head: can't reset" $mainhead] -state disabled
}
- $menu entryconfigure 10 -state $mstate
$menu entryconfigure 11 -state $mstate
$menu entryconfigure 12 -state $mstate
+ $menu entryconfigure 13 -state $mstate
} else {
set menu $fakerowmenu
}
@@ -9481,6 +9482,22 @@ proc copyreference {} {
clipboard append $reference
}
+proc highlightcommitname {} {
+ global rowmenuid autosellen findstring gdttype
+
+ set format "%s"
+ set cmd [list git show -s --pretty=format:$format --date=short]
+ if {$autosellen < 40} {
+ lappend cmd --abbrev=$autosellen
+ }
+ set reference [eval exec $cmd $rowmenuid]
+ set findstring $reference
+ set gdttype [mc "containing:"]
+
+ clipboard clear
+ clipboard append $reference
+}
+
proc writecommit {} {
global rowmenuid wrcomtop commitinfo wrcomcmd NS
--
2.43.0
^ permalink raw reply related
* Re: Git in GSoC 2024
From: Christian Couder @ 2024-01-30 8:38 UTC (permalink / raw)
To: Kaartic Sivaraam
Cc: git, Taylor Blau, Junio C Hamano, Victoria Dye,
Patrick Steinhardt
In-Reply-To: <1de82b27-116a-450e-98c0-52eb65a8f608@gmail.com>
Hi Kaartic and all,
On Mon, Jan 29, 2024 at 7:16 PM Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
>
> Hi everyone,
>
> GSoC Org Applications for 2024 are open now [1] and are due before
> Tuesday, February 6 at 1800 UTC. It's good to see that contributors have
> already started working on microprojects this year :-)
>
> I could help as an Org Admin like previous years. I could volunteer as a
> mentor depending on the set of available projects that we're able to
> gather for this year.
Thanks a lot for sending this and for volunteering as an Org Admin and
possibly as a mentor too!
> There are one noticeable change to the program that should be highlighted:
>
> "For 2024 we have the concept of Small ~90 hour projects that are a
> standard 8 weeks long (they can extended but it is suggest not
> extending beyond 12 weeks for the small projects). We aren't
> required to have small projects available but might be worth
> considering if we have any small projects that contributors could
> work on."
Unless some mentors are only willing to mentor Small projects, I don't
think it's a good idea to propose such projects. Except for some very
good contributors, I think it usually takes a long time for the
contributors we mentor to become efficient and autonomous enough by
themselves. So I think a Small project would likely leave the
contributor halfway there, and they would then have trouble continuing
to work on their project if it's not finished or contributing
significantly in some way afterwards.
> 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.
> Do feel free to ask if there's anything that needs to be clarified.
>
> Just like previous year, there will be a GSoC Meetup in Brussels during
> FOSDEM weekend on Saturday, February 3rd in the evening. If you are
> around, interested and haven't received the link to register directly
> from Google, let me know so I can send it to you.
>
> There's also seems to be a GSoC stand at FOSDEM on Saturday or Sunday.
> It's said to be located in building H, level 1.
Unfortunately I cannot attend the FOSDEM this year. Thanks for
mentioning this though! It's a great opportunity to meet GSoC people
and open source people in general.
^ permalink raw reply
* [PATCH 2/2] ci: add jobs to test with the reftable backend
From: Patrick Steinhardt @ 2024-01-30 8:05 UTC (permalink / raw)
To: git; +Cc: Han-Wen Nienhuys
In-Reply-To: <cover.1706601199.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3294 bytes --]
Add CI jobs for both GitHub Workflows and GitLab CI to run Git with the
new reftable backend.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.github/workflows/main.yml | 9 +++++++++
.gitlab-ci.yml | 9 +++++++++
ci/lib.sh | 2 +-
ci/run-build-and-tests.sh | 3 +++
4 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
index 4d97da57ec..1b43e49dad 100644
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -266,6 +266,9 @@ jobs:
- jobname: linux-sha256
cc: clang
pool: ubuntu-latest
+ - jobname: linux-reftable
+ cc: clang
+ pool: ubuntu-latest
- jobname: linux-gcc
cc: gcc
cc_package: gcc-8
@@ -277,6 +280,9 @@ jobs:
- jobname: osx-clang
cc: clang
pool: macos-13
+ - jobname: osx-reftable
+ cc: clang
+ pool: macos-13
- jobname: osx-gcc
cc: gcc
cc_package: gcc-13
@@ -287,6 +293,9 @@ jobs:
- jobname: linux-leaks
cc: gcc
pool: ubuntu-latest
+ - jobname: linux-reftable-leaks
+ cc: gcc
+ pool: ubuntu-latest
- jobname: linux-asan-ubsan
cc: clang
pool: ubuntu-latest
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 43bfbd8834..c0fa2fe90b 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -26,6 +26,9 @@ test:linux:
- jobname: linux-sha256
image: ubuntu:latest
CC: clang
+ - jobname: linux-reftable
+ image: ubuntu:latest
+ CC: clang
- jobname: linux-gcc
image: ubuntu:20.04
CC: gcc
@@ -40,6 +43,9 @@ test:linux:
- jobname: linux-leaks
image: ubuntu:latest
CC: gcc
+ - jobname: linux-reftable-leaks
+ image: ubuntu:latest
+ CC: gcc
- jobname: linux-asan-ubsan
image: ubuntu:latest
CC: clang
@@ -79,6 +85,9 @@ test:osx:
- jobname: osx-clang
image: macos-13-xcode-14
CC: clang
+ - jobname: osx-reftable
+ image: macos-13-xcode-14
+ CC: clang
artifacts:
paths:
- t/failed-test-artifacts
diff --git a/ci/lib.sh b/ci/lib.sh
index d5dd2f2697..0a73fc7bd1 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -367,7 +367,7 @@ linux-musl)
MAKEFLAGS="$MAKEFLAGS NO_REGEX=Yes ICONV_OMITS_BOM=Yes"
MAKEFLAGS="$MAKEFLAGS GIT_TEST_UTF8_LOCALE=C.UTF-8"
;;
-linux-leaks)
+linux-leaks|linux-reftable-leaks)
export SANITIZE=leak
export GIT_TEST_PASSING_SANITIZE_LEAK=true
export GIT_TEST_SANITIZE_LEAK_LOG=true
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index 7a1466b868..c192bd613c 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -37,6 +37,9 @@ linux-clang)
linux-sha256)
export GIT_TEST_DEFAULT_HASH=sha256
;;
+linux-reftable|linux-reftable-leaks|osx-reftable)
+ export GIT_TEST_DEFAULT_REF_FORMAT=reftable
+ ;;
pedantic)
# Don't run the tests; we only care about whether Git can be
# built.
--
2.43.GIT
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
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