* [PATCH v4 08/28] format_trailer_info(): move "fast path" to caller
From: Linus Arver via GitGitGadget @ 2024-02-06 5:12 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.v4.git.1707196348.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
This is another preparatory refactor to unify the trailer formatters.
This allows us to drop the "msg" parameter from format_trailer_info(),
so that it take 3 parameters, similar to format_trailers() which also
takes 3 parameters:
void format_trailers(const struct process_trailer_options *opts,
struct list_head *trailers,
struct strbuf *out)
The short-term goal is to make format_trailer_info() be smart enough to
deprecate format_trailers(). And then ultimately we will rename
format_trailer_info() to format_trailers().
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/trailer.c b/trailer.c
index cbd643cd1fe..e92d0154d90 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1087,21 +1087,11 @@ void trailer_info_release(struct trailer_info *info)
static void format_trailer_info(const struct process_trailer_options *opts,
const struct trailer_info *info,
- const char *msg,
struct strbuf *out)
{
size_t origlen = out->len;
size_t i;
- /* 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);
@@ -1153,7 +1143,15 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
struct trailer_info info;
trailer_info_get(opts, msg, &info);
- format_trailer_info(opts, &info, msg, out);
+ /* 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);
+ } else
+ format_trailer_info(opts, &info, out);
+
trailer_info_release(&info);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 07/28] format_trailers(): use strbuf instead of FILE
From: Linus Arver via GitGitGadget @ 2024-02-06 5:12 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.v4.git.1707196348.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
This is another preparatory refactor to unify the trailer formatters.
Make format_trailers() also write to a strbuf, to align with
format_trailers_from_commit() which also does the same. Doing this makes
format_trailers() behave similar to format_trailer_info() (which will
soon help us replace one with the other).
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 6 +++++-
trailer.c | 13 +++++++------
trailer.h | 3 ++-
3 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index d1cf0aa33a2..11f4ce9e4a2 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,8 +170,11 @@ 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);
free_trailers(&head);
+ fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
+ strbuf_release(&trailer_block);
/* Print the lines after the trailers as is */
if (!opts->only_trailers)
diff --git a/trailer.c b/trailer.c
index f92d844361a..cbd643cd1fe 100644
--- a/trailer.c
+++ b/trailer.c
@@ -144,12 +144,12 @@ static char last_non_space_char(const char *s)
return '\0';
}
-static void print_tok_val(FILE *outfile, const char *tok, const char *val)
+static void print_tok_val(struct strbuf *out, const char *tok, const char *val)
{
char c;
if (!tok) {
- fprintf(outfile, "%s\n", val);
+ strbuf_addf(out, "%s\n", val);
return;
}
@@ -157,13 +157,14 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
if (!c)
return;
if (strchr(separators, c))
- fprintf(outfile, "%s%s\n", tok, val);
+ strbuf_addf(out, "%s%s\n", tok, val);
else
- fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
+ strbuf_addf(out, "%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 *trailers,
+ struct strbuf *out)
{
struct list_head *pos;
struct trailer_item *item;
@@ -171,7 +172,7 @@ void format_trailers(const struct process_trailer_options *opts,
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);
+ print_tok_val(out, item->token, item->value);
}
}
diff --git a/trailer.h b/trailer.h
index 410c61b62be..1d106b6dd40 100644
--- a/trailer.h
+++ b/trailer.h
@@ -102,7 +102,8 @@ void trailer_info_release(struct trailer_info *info);
void trailer_config_init(void);
void format_trailers(const struct process_trailer_options *,
- struct list_head *trailers, FILE *outfile);
+ struct list_head *trailers,
+ struct strbuf *out);
void free_trailers(struct list_head *);
/*
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 06/28] trailer_info_get(): reorder parameters
From: Linus Arver via GitGitGadget @ 2024-02-06 5:12 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.v4.git.1707196348.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
This is another preparatory refactor to unify the trailer formatters.
Take
const struct process_trailer_options *opts
as the first parameter, because these options are required for
parsing trailers (e.g., whether to treat "---" as the end of the log
message). And take
struct trailer_info *info
last, because it's an "out parameter" (something that the caller wants
to use as the output of this function).
Signed-off-by: Linus Arver <linusa@google.com>
---
sequencer.c | 2 +-
trailer.c | 11 ++++++-----
trailer.h | 5 +++--
3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 3cc88d8a800..8e199fc8a47 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -332,7 +332,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
sb->buf[sb->len - ignore_footer] = '\0';
}
- trailer_info_get(&info, sb->buf, &opts);
+ trailer_info_get(&opts, sb->buf, &info);
if (ignore_footer)
sb->buf[sb->len - ignore_footer] = saved_char;
diff --git a/trailer.c b/trailer.c
index 5025be97899..f92d844361a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -997,7 +997,7 @@ void parse_trailers(const struct process_trailer_options *opts,
struct strbuf val = STRBUF_INIT;
size_t i;
- trailer_info_get(info, str, opts);
+ trailer_info_get(opts, str, info);
for (i = 0; i < info->trailer_nr; i++) {
int separator_pos;
@@ -1032,8 +1032,9 @@ void free_trailers(struct list_head *trailers)
}
}
-void trailer_info_get(struct trailer_info *info, const char *str,
- const struct process_trailer_options *opts)
+void trailer_info_get(const struct process_trailer_options *opts,
+ const char *str,
+ struct trailer_info *info)
{
size_t end_of_log_message = 0, trailer_block_start = 0;
struct strbuf **trailer_lines, **ptr;
@@ -1150,7 +1151,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
{
struct trailer_info info;
- trailer_info_get(&info, msg, opts);
+ trailer_info_get(opts, msg, &info);
format_trailer_info(opts, &info, msg, out);
trailer_info_release(&info);
}
@@ -1161,7 +1162,7 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
strbuf_init(&iter->key, 0);
strbuf_init(&iter->val, 0);
opts.no_divider = 1;
- trailer_info_get(&iter->internal.info, msg, &opts);
+ trailer_info_get(&opts, msg, &iter->internal.info);
iter->internal.cur = 0;
}
diff --git a/trailer.h b/trailer.h
index c6d3ee49bbf..410c61b62be 100644
--- a/trailer.h
+++ b/trailer.h
@@ -94,8 +94,9 @@ void parse_trailers(const struct process_trailer_options *,
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_get(const struct process_trailer_options *,
+ const char *str,
+ struct trailer_info *);
void trailer_info_release(struct trailer_info *info);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 05/28] trailer: start preparing for formatting unification
From: Linus Arver via GitGitGadget @ 2024-02-06 5:12 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.v4.git.1707196348.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Currently there are two functions for formatting trailers in
<trailer.h>:
void format_trailers(const struct process_trailer_options *,
struct list_head *trailers, FILE *outfile);
void format_trailers_from_commit(struct strbuf *out, const char *msg,
const struct process_trailer_options *opts);
and although they are similar enough (even taking the same
process_trailer_options struct pointer) they are used quite differently.
One might intuitively think that format_trailers_from_commit() builds on
top of format_trailers(), but this is not the case. Instead
format_trailers_from_commit() calls format_trailer_info() and
format_trailers() is never called in that codepath.
This is a preparatory refactor to help us deprecate format_trailers() in
favor of format_trailer_info() (at which point we can rename the latter
to the former). When the deprecation is complete, both
format_trailers_from_commit(), and the interpret-trailers builtin will
be able to call into the same helper function (instead of
format_trailers() and format_trailer_info(), respectively). Unifying the
formatters is desirable because it simplifies the API.
Reorder parameters for format_trailers_from_commit() to prefer
const struct process_trailer_options *opts
as the first parameter, because these options are intimately tied to
formatting trailers. And take
struct strbuf *out
last, because it's an "out parameter" (something that the caller wants
to use as the output of this function).
Signed-off-by: Linus Arver <linusa@google.com>
---
pretty.c | 2 +-
ref-filter.c | 2 +-
trailer.c | 11 ++++++-----
trailer.h | 5 +++--
4 files changed, 11 insertions(+), 9 deletions(-)
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 d23afa0a65c..5025be97899 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1083,10 +1083,10 @@ void trailer_info_release(struct trailer_info *info)
free(info->trailers);
}
-static void format_trailer_info(struct strbuf *out,
+static void format_trailer_info(const struct process_trailer_options *opts,
const struct trailer_info *info,
const char *msg,
- const struct process_trailer_options *opts)
+ struct strbuf *out)
{
size_t origlen = out->len;
size_t i;
@@ -1144,13 +1144,14 @@ static void format_trailer_info(struct strbuf *out,
}
-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)
{
struct trailer_info info;
trailer_info_get(&info, msg, opts);
- format_trailer_info(out, &info, msg, opts);
+ format_trailer_info(opts, &info, msg, out);
trailer_info_release(&info);
}
diff --git a/trailer.h b/trailer.h
index c292d44b62f..c6d3ee49bbf 100644
--- a/trailer.h
+++ b/trailer.h
@@ -115,8 +115,9 @@ void free_trailers(struct list_head *);
* only the trailer block itself, even if the "only_trailers" option is not
* set.
*/
-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 v4 04/28] trailer: move interpret_trailers() to interpret-trailers.c
From: Linus Arver via GitGitGadget @ 2024-02-06 5:12 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.v4.git.1707196348.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 (together with a few
helper functions called only by it) and remove its external declaration
from <trailer.h>.
Several helper functions that are called by interpret_trailers() remain
in trailer.c because other callers in the same file still call them.
Declare them in <trailer.h> so that interpret_trailers() (now in
builtin/interpret-trailers.c) can continue calling them as a trailer API
user.
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.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 93 +++++++++++++++++++++++++++
trailer.c | 119 ++++-------------------------------
trailer.h | 20 +++++-
3 files changed, 123 insertions(+), 109 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 85a3413baf5..d1cf0aa33a2 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,98 @@ 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);
+
+ /* 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);
+ trailer_info_release(&info);
+
+ 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 916175707d8..d23afa0a65c 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,87 +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);
-
- /* 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);
- trailer_info_release(&info);
-
- 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..c292d44b62f 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 *,
+ struct trailer_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 *,
+ 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 v4 03/28] trailer: prepare to expose functions as part of API
From: Linus Arver via GitGitGadget @ 2024-02-06 5:12 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.v4.git.1707196348.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 processing 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 f74915bd8cd..916175707d8 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,8 +1110,8 @@ void process_trailers(const char *file,
process_trailers_lists(&head, &arg_head);
}
- print_all(outfile, &head, opts);
- free_all(&head);
+ format_trailers(opts, &head, outfile);
+ free_trailers(&head);
/* Print the lines after the trailers as is */
if (!opts->only_trailers)
@@ -1134,7 +1134,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 v4 02/28] shortlog: add test for de-duplicating folded trailers
From: Linus Arver via GitGitGadget @ 2024-02-06 5:12 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.v4.git.1707196348.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
The shortlog builtin was taught to use the trailer iterator interface in
47beb37bc6 (shortlog: match commit trailers with --group, 2020-09-27).
The iterator always unfolds values and this has always been the case
since the time the iterator was first introduced in f0939a0eb1 (trailer:
add interface for iterating over commit trailers, 2020-09-27). Add a
comment line to remind readers of this behavior.
The fact that the iterator always unfolds values is important
(at least for shortlog) because unfolding allows it to recognize both
folded and unfolded versions of the same trailer for de-duplication.
Capture the existing behavior in a new test case to guard against
regressions in this area. This test case is based off of the existing
"shortlog de-duplicates trailers in a single commit" just above it. Now
if we were to remove the call to
unfold_value(&iter->val);
inside the iterator, this new test case will break.
Signed-off-by: Linus Arver <linusa@google.com>
---
t/t4201-shortlog.sh | 32 ++++++++++++++++++++++++++++++++
trailer.c | 1 +
2 files changed, 33 insertions(+)
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index d7382709fc1..f698d0c9ad2 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -312,6 +312,38 @@ test_expect_success 'shortlog de-duplicates trailers in a single commit' '
test_cmp expect actual
'
+# Trailers that have unfolded (single line) and folded (multiline) values which
+# are otherwise identical are treated as the same trailer for de-duplication.
+test_expect_success 'shortlog de-duplicates trailers in a single commit (folded/unfolded values)' '
+ git commit --allow-empty -F - <<-\EOF &&
+ subject one
+
+ this message has two distinct values, plus a repeat (folded)
+
+ Repeated-trailer: Foo foo foo
+ Repeated-trailer: Bar
+ Repeated-trailer: Foo
+ foo foo
+ EOF
+
+ git commit --allow-empty -F - <<-\EOF &&
+ subject two
+
+ similar to the previous, but without the second distinct value
+
+ Repeated-trailer: Foo foo foo
+ Repeated-trailer: Foo
+ foo foo
+ EOF
+
+ cat >expect <<-\EOF &&
+ 2 Foo foo foo
+ 1 Bar
+ EOF
+ git shortlog -ns --group=trailer:repeated-trailer -2 HEAD >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'shortlog can match multiple groups' '
git commit --allow-empty -F - <<-\EOF &&
subject one
diff --git a/trailer.c b/trailer.c
index e1d83390b66..f74915bd8cd 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1270,6 +1270,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
strbuf_reset(&iter->val);
parse_trailer(&iter->key, &iter->val, NULL,
trailer, separator_pos);
+ /* Always unfold values during iteration. */
unfold_value(&iter->val);
return 1;
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 00/28] Enrich Trailer API
From: Linus Arver via GitGitGadget @ 2024-02-06 5:12 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.v3.git.1706664144.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 v4
=====================
* Patches 3, 4, 5, and 8 have been broken up into smaller steps. There are
28 instead of 10 patches now, but these 28 should be much easier to
review than the (previously condensed) 10.
* NEW Patch 1: "trailer: free trailer_info after all related usage" fixes
awkward use-after-free coding style
* NEW Patch 2: "shortlog: add test for de-duplicating folded trailers"
increases test coverage related to trailer iterators and "unfold_value()"
* NEW Patch 27: "trailer_set_*(): put out parameter at the end" is a small
refactor to reorder parameters.
* Patches 5-16: These smaller patches make up Patch 3 from v3.
* Patches 17-18: These smaller patches make up Patch 4 from v3.
* Patches 19-20: These smaller patches make up Patch 5 from v3.
* Patches 23-26: These smaller patches make up Patch 8 from v3.
* Anonymize unambiguous parameters in <trailer.h>.
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 (28):
trailer: free trailer_info _after_ all related usage
shortlog: add test for de-duplicating folded trailers
trailer: prepare to expose functions as part of API
trailer: move interpret_trailers() to interpret-trailers.c
trailer: start preparing for formatting unification
trailer_info_get(): reorder parameters
format_trailers(): use strbuf instead of FILE
format_trailer_info(): move "fast path" to caller
format_trailers_from_commit(): indirectly call trailer_info_get()
format_trailer_info(): use trailer_item objects
format_trailer_info(): drop redundant unfold_value()
format_trailer_info(): append newline for non-trailer lines
trailer: begin formatting unification
format_trailer_info(): teach it about opts->trim_empty
format_trailer_info(): avoid double-printing the separator
trailer: finish formatting unification
trailer: teach iterator about non-trailer lines
sequencer: use the trailer iterator
trailer: make trailer_info struct private
trailer: retire trailer_info_get() from API
trailer: spread usage of "trailer_block" language
trailer: prepare to delete "parse_trailers_from_command_line_args()"
trailer: add new helper functions to API
trailer_add_arg_item(): drop new_trailer_item usage
trailer: deprecate "new_trailer_item" struct from API
trailer: unify "--trailer ..." arg handling
trailer_set_*(): put out parameter at the end
trailer: introduce "template" term for readability
builtin/interpret-trailers.c | 189 ++++++--
pretty.c | 2 +-
ref-filter.c | 2 +-
sequencer.c | 27 +-
t/t4201-shortlog.sh | 32 ++
trailer.c | 811 +++++++++++++++++------------------
trailer.h | 109 ++---
7 files changed, 642 insertions(+), 530 deletions(-)
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1632%2Flistx%2Ftrailer-api-refactor-part-1-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1632/listx/trailer-api-refactor-part-1-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1632
Range-diff vs v3:
-: ----------- > 1: 652df25f30e trailer: free trailer_info _after_ all related usage
-: ----------- > 2: fdccaca2ba0 shortlog: add test for de-duplicating folded trailers
1: 72cc12a3066 ! 3: 4372af244f0 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.
- Take the opportunity to start putting trailer processions options (opts)
+ Take the opportunity to start putting trailer processing options (opts)
as the first parameter. This will be the pattern going forward in this
series.
@@ trailer.c: void process_trailers(const char *file,
}
- print_all(outfile, &head, opts);
-+ format_trailers(opts, &head, outfile);
-
- free_all(&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)
@@ trailer.c: void trailer_info_get(struct trailer_info *info, const char *str,
size_t nr = 0, alloc = 0;
char **last = NULL;
2: cafa39d1048 ! 4: 4073b8eb510 trailer: move interpret_trailers() to interpret-trailers.c
@@ Commit message
trailer: move interpret_trailers() to interpret-trailers.c
The interpret-trailers.c builtin is the only place we need to call
- interpret_trailers(), so move its definition there.
+ interpret_trailers(), so move its definition there (together with a few
+ helper functions called only by it) and remove its external declaration
+ from <trailer.h>.
- 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).
+ Several helper functions that are called by interpret_trailers() remain
+ in trailer.c because other callers in the same file still call them.
+ Declare them in <trailer.h> so that interpret_trailers() (now in
+ builtin/interpret-trailers.c) can continue calling them as a trailer API
+ user.
+
+ 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.
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
## builtin/interpret-trailers.c ##
@@ builtin/interpret-trailers.c: static int parse_opt_parse(const struct option *op
+ }
+
+ 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);
++ trailer_info_release(&info);
+
+ if (opts->in_place)
+ if (rename_tempfile(&trailers_tempfile, file))
@@ trailer.c: static void free_trailers(struct list_head *trailers)
- }
-
- 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);
+- trailer_info_release(&info);
-
- if (opts->in_place)
- if (rename_tempfile(&trailers_tempfile, file))
@@ trailer.h: struct process_trailer_options {
+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,
++void parse_trailers(const struct process_trailer_options *,
++ struct trailer_info *,
+ const char *str,
+ struct list_head *head);
@@ trailer.h: struct process_trailer_options {
void trailer_info_release(struct trailer_info *info);
+void trailer_config_init(void);
-+void format_trailers(const struct process_trailer_options *opts,
++void format_trailers(const struct process_trailer_options *,
+ struct list_head *trailers, FILE *outfile);
+void free_trailers(struct list_head *);
+
3: 5c7a2354df0 ! 5: b2a0f7829a1 trailer: unify trailer formatting machinery
@@ Metadata
Author: Linus Arver <linusa@google.com>
## Commit message ##
- trailer: unify trailer formatting machinery
+ trailer: start preparing for formatting unification
- Currently have two functions for formatting trailers exposed in
- trailer.h:
+ Currently there are two functions for formatting trailers in
+ <trailer.h>:
- void format_trailers(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts);
+ void format_trailers(const struct process_trailer_options *,
+ struct list_head *trailers, FILE *outfile);
void format_trailers_from_commit(struct strbuf *out, const char *msg,
- const struct process_trailer_options *opts);
+ 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.
+ and although they are similar enough (even taking the same
+ process_trailer_options struct pointer) they are used quite differently.
+ One might intuitively think that format_trailers_from_commit() builds on
+ top of format_trailers(), but this is not the case. Instead
+ format_trailers_from_commit() calls format_trailer_info() and
+ format_trailers() is never called in that codepath.
- 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 is a preparatory refactor to help us deprecate format_trailers() in
+ favor of format_trailer_info() (at which point we can rename the latter
+ to the former). When the deprecation is complete, both
+ format_trailers_from_commit(), and the interpret-trailers builtin will
+ be able to call into the same helper function (instead of
+ format_trailers() and format_trailer_info(), respectively). Unifying the
+ formatters is desirable because it simplifies the API.
- 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.
+ Reorder parameters for format_trailers_from_commit() to prefer
- Helped-by: Junio C Hamano <gitster@pobox.com>
- Signed-off-by: Linus Arver <linusa@google.com>
+ const struct process_trailer_options *opts
- ## builtin/interpret-trailers.c ##
-@@ builtin/interpret-trailers.c: 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;
-
-@@ builtin/interpret-trailers.c: 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);
+ as the first parameter, because these options are intimately tied to
+ formatting trailers. And take
+
+ struct strbuf *out
+
+ last, because it's an "out parameter" (something that the caller wants
+ to use as the output of this function).
+
+ Signed-off-by: Linus Arver <linusa@google.com>
## pretty.c ##
@@ pretty.c: static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
@@ 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 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));
-@@ trailer.c: 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.
@@ trailer.c: 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,
++static void format_trailer_info(const struct process_trailer_options *opts,
+ 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)
++ struct strbuf *out)
{
-- size_t origlen = out->len;
-- size_t i;
-+ LIST_HEAD(head);
-+ struct trailer_info info;
-+
-+ parse_trailers(opts, &info, msg, &head);
+ size_t origlen = out->len;
+ size_t i;
+@@ trailer.c: static void format_trailer_info(struct strbuf *out,
+
+ }
- /* 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);
++void format_trailers_from_commit(const struct process_trailer_options *opts,
++ const char *msg,
++ struct strbuf *out)
+ {
+ struct trailer_info info;
-- trailer_info_get(&info, msg, opts);
+ trailer_info_get(&info, msg, opts);
- format_trailer_info(out, &info, msg, opts);
-+ free_trailers(&head);
++ format_trailer_info(opts, &info, msg, out);
trailer_info_release(&info);
}
## trailer.h ##
-@@ trailer.h: 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.
+@@ trailer.h: void free_trailers(struct list_head *);
+ * only the trailer block itself, even if the "only_trailers" option is not
+ * set.
*/
-void format_trailers_from_commit(struct strbuf *out, const char *msg,
- const struct process_trailer_options *opts);
-: ----------- > 6: c1760f80356 trailer_info_get(): reorder parameters
-: ----------- > 7: 9dc912b5bc5 format_trailers(): use strbuf instead of FILE
-: ----------- > 8: b97c06d8bc3 format_trailer_info(): move "fast path" to caller
-: ----------- > 9: 6906910417a format_trailers_from_commit(): indirectly call trailer_info_get()
-: ----------- > 10: f5b7ba08aa7 format_trailer_info(): use trailer_item objects
-: ----------- > 11: 457f2a839d5 format_trailer_info(): drop redundant unfold_value()
-: ----------- > 12: a72eca301f7 format_trailer_info(): append newline for non-trailer lines
-: ----------- > 13: ad77c33e457 trailer: begin formatting unification
-: ----------- > 14: 11f854399db format_trailer_info(): teach it about opts->trim_empty
-: ----------- > 15: ba1f387747b format_trailer_info(): avoid double-printing the separator
-: ----------- > 16: 31725832224 trailer: finish formatting unification
4: bf2b8e1a3c4 ! 17: 6f17c022b15 sequencer: use the trailer iterator
@@ Metadata
Author: Linus Arver <linusa@google.com>
## Commit message ##
- sequencer: use the trailer iterator
+ trailer: teach iterator about non-trailer lines
- This patch allows for the removal of "trailer_info_get()" from the
- trailer.h API, which will be in the next patch.
+ Previously the iterator did not iterate over non-trailer lines. This was
+ somewhat unfortunate, because trailer blocks could have non-trailer
+ lines in them since 146245063e (trailer: allow non-trailers in trailer
+ block, 2016-10-21), which was before the iterator was created in
+ f0939a0eb1 (trailer: add interface for iterating over commit trailers,
+ 2020-09-27).
- 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).
+ So if trailer API users wanted to iterate over all lines in a trailer
+ block (including non-trailer lines), they could not use the iterator and
+ were forced to use the lower-level trailer_info struct directly (which
+ provides a raw string array that includes all lines in the trailer
+ block).
- 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).
+ Change the iterator's behavior so that we also iterate over non-trailer
+ lines, instead of skipping over them. The new "raw" member of the
+ iterator allows API users to access previously inaccessible non-trailer
+ lines. Reword the variable "trailer" to just "line" because this
+ variable can now hold both trailer lines _and_ non-trailer lines.
- 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.
+ The new "raw" member is important because anyone currently not using the
+ iterator is using trailer_info's raw string array directly to access
+ lines to check what the combined key + value looks like. If we didn't
+ provide a "raw" member here, iterator users would have to re-construct
+ the unparsed line by concatenating the key and value back together again
+ --- which places an undue burden for iterator users.
- Signed-off-by: Linus Arver <linusa@google.com>
+ The next patch demonstrates the use of the iterator in sequencer.c as an
+ example of where "raw" will be useful, so that it can start using the
+ iterator.
- ## builtin/shortlog.c ##
-@@ builtin/shortlog.c: 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)
-@@ builtin/shortlog.c: 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;
+ For the existing use of the iterator in builtin/shortlog.c, we don't
+ have to change the code there because that code does
- ## sequencer.c ##
-@@ sequencer.c: 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;
+ trailer_iterator_init(&iter, body);
+ while (trailer_iterator_advance(&iter)) {
+ const char *value = iter.val.buf;
+
+ if (!string_list_has_string(&log->trailers, iter.key.buf))
+ continue;
+
+ ...
+
+ and the
+
+ if (!string_list_has_string(&log->trailers, iter.key.buf))
+
+ condition already skips over non-trailer lines (iter.key.buf is empty
+ for non-trailer lines, making the comparison still work unmodified even
+ with this patch).
+
+ Signed-off-by: Linus Arver <linusa@google.com>
## trailer.c ##
@@ trailer.c: void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
@@ trailer.c: void trailer_iterator_init(struct trailer_iterator *iter, const char
-
- 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);
-+
++ char *line = iter->internal.info.trailers[iter->internal.cur++];
++ int separator_pos = find_separator(line, separators);
+
+ iter->raw = line;
strbuf_reset(&iter->key);
strbuf_reset(&iter->val);
parse_trailer(&iter->key, &iter->val, NULL,
- trailer, separator_pos);
+ line, separator_pos);
+ /* Always unfold values during iteration. */
unfold_value(&iter->val);
return 1;
- }
## trailer.h ##
-@@ trailer.h: struct trailer_iterator {
- struct strbuf key;
- struct strbuf val;
-
+@@ trailer.h: void format_trailers_from_commit(const struct process_trailer_options *,
+ * trailer_iterator_release(&iter);
+ */
+ struct trailer_iterator {
+ /*
+ * 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
@@ trailer.h: struct trailer_iterator {
+ */
+ 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;
+ struct strbuf key;
+ struct strbuf val;
+
-: ----------- > 18: cc92dfb0bda sequencer: use the trailer iterator
5: c19c1dcc592 ! 19: f5f0d06613f trailer: make trailer_info struct private
@@ Commit message
(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).
+ There are a couple disadvantages:
+
+ (A) every time the member of the struct is accessed an extra pointer
+ dereference must be done, and
+
+ (B) for users of trailer_info outside trailer.c, this struct can no
+ longer be allocated on the stack and may only be allocated on the
+ heap (because its definition is hidden away in trailer.c) and
+ appropriately deallocated by the user.
+
+ This patch believes that the benefits outweight the advantages for
+ designing APIs, as explained below.
+
+ Making trailer_info private exposes existing deficiencies in the API.
+ This is because users of this struct had full access to its internals,
+ so there wasn't much need to actually design it to be "complete" in the
+ sense that API users only needed to use what was provided by the API.
+ For example, the location of the trailer block (start/end offsets
+ relative to the start of the input text) was accessible by looking at
+ these struct members directly. Now that the struct is private, we have
+ to expose new API functions to allow clients to access this
+ information (see 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
@@ Commit message
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.
+ of <trailer.h>. 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
+ Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
## builtin/interpret-trailers.c ##
@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct proces
@@ builtin/interpret-trailers.c: 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);
+- trailer_info_release(&info);
+ fwrite(sb.buf + trailer_block_end(info), 1, sb.len - trailer_block_end(info), outfile);
-+
+ trailer_info_release(info);
if (opts->in_place)
@@ trailer.c
struct conf_info {
char *name;
char *key;
-@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
- }
+@@ trailer.c: static void unfold_value(struct strbuf *val)
+ strbuf_release(&out);
}
+static struct trailer_info *trailer_info_new(void)
@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
+ 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"
@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
struct strbuf val = STRBUF_INIT;
size_t i;
-- trailer_info_get(info, str, opts);
+- trailer_info_get(opts, str, info);
+ info = trailer_info_get(opts, str);
for (i = 0; i < info->trailer_nr; i++) {
@@ trailer.c: void free_trailers(struct list_head *trailers)
}
}
--void trailer_info_get(struct trailer_info *info, const char *str,
-- const struct process_trailer_options *opts)
+-void trailer_info_get(const struct process_trailer_options *opts,
+- const char *str,
+- struct trailer_info *info)
+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;
++}
++
++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;
+@@ trailer.c: void trailer_info_get(const struct process_trailer_options *opts,
+ info->trailer_block_end = end_of_log_message;
+ info->trailers = trailer_strings;
+ info->trailer_nr = nr;
++
++ return info;
}
void trailer_info_release(struct trailer_info *info)
@@ trailer.c: void trailer_info_release(struct trailer_info *info)
+ free(info);
}
- void format_trailers_from_commit(const struct process_trailer_options *opts,
+ void format_trailers(const struct process_trailer_options *opts,
@@ trailer.c: void format_trailers_from_commit(const struct process_trailer_options *opts,
struct strbuf *out)
{
- LIST_HEAD(head);
+ LIST_HEAD(trailers);
- struct trailer_info info;
-
-- parse_trailers(opts, &info, msg, &head);
-+ struct trailer_info *info = parse_trailers(opts, msg, &head);
+- parse_trailers(opts, &info, msg, &trailers);
++ struct trailer_info *info = parse_trailers(opts, msg, &trailers);
/* If we want the whole block untouched, we can take the fast path. */
if (!opts->only_trailers && !opts->unfold && !opts->filter &&
@@ trailer.c: void format_trailers_from_commit(const struct process_trailer_options
+ strbuf_add(out, msg + info->trailer_block_start,
+ info->trailer_block_end - info->trailer_block_start);
} else
- format_trailers(opts, &head, out);
+ format_trailers(opts, &trailers, out);
- free_trailers(&head);
+ free_trailers(&trailers);
- 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();
+@@ trailer.c: void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
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;
+- trailer_info_get(&opts, msg, &iter->internal.info);
+ iter->internal.info = trailer_info_get(&opts, msg);
iter->internal.cur = 0;
}
-@@ trailer.c: int trailer_iterator_advance(struct trailer_iterator *iter)
+ 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++];
+- char *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);
++ char *line = iter->internal.info->trailers[iter->internal.cur++];
+ int separator_pos = find_separator(line, separators);
+ iter->raw = line;
@@ trailer.c: int trailer_iterator_advance(struct trailer_iterator *iter)
void trailer_iterator_release(struct trailer_iterator *iter)
@@ trailer.h: 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,
+-void parse_trailers(const struct process_trailer_options *,
+- struct trailer_info *,
- const char *str,
- struct list_head *head);
-+struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
++struct trailer_info *parse_trailers(const struct process_trailer_options *,
+ 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);
++struct trailer_info *trailer_info_get(const struct process_trailer_options *,
++ const char *str);
+
+-void trailer_info_get(const struct process_trailer_options *,
+- const char *str,
+- struct trailer_info *);
++size_t trailer_block_start(struct trailer_info *);
++size_t trailer_block_end(struct trailer_info *);
++int blank_line_before_trailer_block(struct trailer_info *);
void trailer_info_release(struct trailer_info *info);
-: ----------- > 20: 607ae7a90cd trailer: retire trailer_info_get() from API
6: 0a9a7438c3f ! 21: 38f4b4c4135 trailer: spread usage of "trailer_block" language
@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct proces
/* Print trailer block. */
- format_trailers(opts, &head, &trailer_block);
++ format_trailers(opts, &head, &tb);
+ free_trailers(&head);
- 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 */
++ /* 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);
++ 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)
@@ trailer.c
/*
* Array of trailers found.
-@@ trailer.c: void format_trailers(const struct process_trailer_options *opts,
- }
+@@ trailer.c: static void unfold_value(struct strbuf *val)
+ strbuf_release(&out);
}
-static struct trailer_info *trailer_info_new(void)
@@ trailer.c: void free_trailers(struct list_head *trailers)
+ free(trailer_block);
}
- void format_trailers_from_commit(const struct process_trailer_options *opts,
+ void format_trailers(const struct process_trailer_options *opts,
@@ trailer.c: 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);
+ LIST_HEAD(trailers);
+- struct trailer_info *info = parse_trailers(opts, msg, &trailers);
++ struct trailer_block *trailer_block = parse_trailers(opts, msg, &trailers);
/* If we want the whole block untouched, we can take the fast path. */
if (!opts->only_trailers && !opts->unfold && !opts->filter &&
@@ trailer.c: void format_trailers_from_commit(const struct process_trailer_options
+ strbuf_add(out, msg + trailer_block->start,
+ trailer_block->end - trailer_block->start);
} else
- format_trailers(opts, &head, out);
+ format_trailers(opts, &trailers, out);
- free_trailers(&head);
+ free_trailers(&trailers);
- 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();
+@@ trailer.c: void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
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;
}
-@@ trailer.c: int trailer_iterator_advance(struct trailer_iterator *iter)
+ 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++];
+- char *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);
++ char *line = iter->internal.trailer_block->trailers[iter->internal.cur++];
+ int separator_pos = find_separator(line, separators);
+ iter->raw = line;
@@ trailer.c: int trailer_iterator_advance(struct trailer_iterator *iter)
void trailer_iterator_release(struct trailer_iterator *iter)
@@ trailer.h: 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,
+-struct trailer_info *parse_trailers(const struct process_trailer_options *,
- const char *str,
- struct list_head *head);
-+struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
++struct trailer_block *parse_trailers(const struct process_trailer_options *,
+ 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);
+-size_t trailer_block_start(struct trailer_info *);
+-size_t trailer_block_end(struct trailer_info *);
+-int blank_line_before_trailer_block(struct trailer_info *);
++size_t trailer_block_start(struct trailer_block *);
++size_t trailer_block_end(struct trailer_block *);
++int blank_line_before_trailer_block(struct trailer_block *);
-void trailer_info_release(struct trailer_info *info);
-+void trailer_block_release(struct trailer_block *trailer_block);
++void trailer_block_release(struct trailer_block *);
void trailer_config_init(void);
- void format_trailers(const struct process_trailer_options *opts,
+ void format_trailers(const struct process_trailer_options *,
@@ trailer.h: struct trailer_iterator {
/* private */
7: 97e5d86ddf0 ! 22: 94bf182e3ff trailer: prepare to move parse_trailers_from_command_line_args() to builtin
@@ Metadata
Author: Linus Arver <linusa@google.com>
## Commit message ##
- trailer: prepare to move parse_trailers_from_command_line_args() to builtin
+ trailer: prepare to delete "parse_trailers_from_command_line_args()"
- Expose more functions in the trailer.h API, in preparation for moving
- out
+ Expose more functions in the trailer.h API, in preparation for deleting
parse_trailers_from_command_line_args()
- to interpret-trailer.c, because the trailer API should not be concerned
+ from the trailers implementation, because it 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.
+ themselves). Indeed, the interpret-trailers builtin is the only caller
+ of this function inside Git.
- 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".
+ Rename add_arg_item() to trailer_add_arg_item() to expose it as an API
+ function. 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>
@@ trailer.c: static struct trailer_item *add_trailer_item(struct list_head *head,
-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)
++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: int trailer_iterator_advance(struct trailer_iterator *iter)
- parse_trailer(&iter->key, &iter->val, NULL,
- line, separator_pos);
+ parse_trailer(line, separator_pos, &iter->key, &iter->val, NULL);
+ /* Always unfold values during iteration. */
unfold_value(&iter->val);
return 1;
- }
## trailer.h ##
@@
@@ trailer.h: struct new_trailer_item {
+void duplicate_trailer_conf(struct trailer_conf *dst,
+ const struct trailer_conf *src);
++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 process_trailer_options {
int in_place;
@@ trailer.h: void parse_trailers_from_command_line_args(struct list_head *arg_head
+
+void parse_trailer(const char *line, ssize_t separator_pos,
+ struct strbuf *tok, struct strbuf *val,
-+ const struct trailer_conf **conf);
++ const struct trailer_conf **);
+
- struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
+ struct trailer_block *parse_trailers(const struct process_trailer_options *,
const char *str,
struct list_head *head);
8: 465dc51cdcb ! 23: 3bfe4809ecb trailer: move arg handling to interpret-trailers.c
@@ Metadata
Author: Linus Arver <linusa@google.com>
## Commit message ##
- trailer: move arg handling to interpret-trailers.c
+ trailer: add new helper functions to API
- 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.
+ This is a preparatory refactor for deprecating "new_trailer_item" from
+ the API (which will let us deprecate
+ parse_trailers_from_command_line_args()).
+
+ Expose new helper functions from the API, because we'll be calling them
+ from interpret-trailers.c soon when we move
+ parse_trailers_from_command_line_args() there.
+
+ Move free_new_trailers() from the builtin to trailer.c because later on
+ we will adjust it to free arg_item structs, which are private to
+ trailer.c.
Signed-off-by: Linus Arver <linusa@google.com>
@@ builtin/interpret-trailers.c: static int option_parse_if_missing(const struct op
- 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);
-@@ builtin/interpret-trailers.c: 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;
- }
-
-@@ builtin/interpret-trailers.c: 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);
-@@ builtin/interpret-trailers.c: 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)
-@@ builtin/interpret-trailers.c: 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. */
-@@ builtin/interpret-trailers.c: 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")),
-@@ builtin/interpret-trailers.c: 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;
- }
## trailer.c ##
@@ trailer.c: static LIST_HEAD(conf_head);
@@ trailer.c: int trailer_set_if_missing(enum trailer_if_missing *item, const char
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)
++void trailer_set_conf_where(enum trailer_where where,
++ struct trailer_conf *conf)
++{
++ conf->where = where;
++}
++
++void trailer_set_conf_if_exists(enum trailer_if_exists if_exists,
++ struct trailer_conf *conf)
++{
++ conf->if_exists = if_exists;
++}
++
++void trailer_set_conf_if_missing(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;
++ conf->if_missing = if_missing;
+}
+
+struct trailer_conf *new_trailer_conf(void)
+{
-+ struct trailer_conf *new = xcalloc(1, sizeof(*new));
-+ return new;
++ return xcalloc(1, sizeof(struct trailer_conf));
+}
+
void duplicate_trailer_conf(struct trailer_conf *dst,
const struct trailer_conf *src)
{
-@@ 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.
-+ *
- * 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.
-@@ trailer.c: static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
- return new_item;
+@@ trailer.c: void duplicate_trailer_conf(struct trailer_conf *dst,
+ dst->cmd = xstrdup_or_null(src->cmd);
}
--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)
++void free_trailer_conf(struct trailer_conf *conf)
++{
++ free(conf->name);
++ free(conf->key);
++ free(conf->command);
++ free(conf->cmd);
++ free(conf);
++}
++
+ static struct arg_item *get_conf_item(const char *name)
{
- 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);
- }
-
-@@ 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)
-- 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);
-- 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);
- }
- }
-
+ struct list_head *pos;
@@ trailer.c: void free_trailers(struct list_head *trailers)
}
}
+void free_new_trailers(struct list_head *trailers)
+{
-+ struct list_head *pos, *p;
++ struct list_head *pos, *tmp;
++ struct new_trailer_item *item;
+
-+ list_for_each_safe(pos, p, trailers) {
++ list_for_each_safe(pos, tmp, trailers) {
++ item = list_entry(pos, struct new_trailer_item, list);
+ list_del(pos);
-+ free_arg_item(list_entry(pos, struct arg_item, list));
++ free(item);
+ }
+}
+
@@ trailer.h: 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);
++void trailer_set_conf_where(enum trailer_where, struct trailer_conf *);
++void trailer_set_conf_if_exists(enum trailer_if_exists, struct trailer_conf *);
++void trailer_set_conf_if_missing(enum trailer_if_missing, struct trailer_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;
-@@ trailer.h: void format_trailers(const struct process_trailer_options *opts,
+ 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);
+@@ trailer.h: void format_trailers(const struct process_trailer_options *,
struct list_head *trailers,
struct strbuf *out);
void free_trailers(struct list_head *);
+void free_new_trailers(struct list_head *);
++void free_trailer_conf(struct trailer_conf *);
/*
* Convenience function to format the trailers from the commit msg "msg" into
-: ----------- > 24: 80e1958bb8d trailer_add_arg_item(): drop new_trailer_item usage
9: 885ac87a544 ! 25: a9080597a28 trailer: delete obsolete argument handling code from API
@@ Metadata
Author: Linus Arver <linusa@google.com>
## Commit message ##
- trailer: delete obsolete argument handling code from API
+ trailer: deprecate "new_trailer_item" struct from API
- 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).
+ Previously, the "new_trailer_item" struct served only one purpose --- to
+ capture the unparsed raw string <RAW> in "--trailer <RAW>", as well as
+ the current state of the "where", "if_exists", and "if_missing" global
+ variables at the time that the "--trailer <RAW>" CLI argument was
+ encountered.
- 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.
+ In addition, the previous CLI argument handling behavior was to capture
+ the <RAW> string in all "--trailer <RAW>" arguments and to collect
+ them (via option_parse_trailer()) into the "new_trailer_head" list. We
+ would then iterate over this list again in
+ parse_trailers_from_command_line_args() and convert these
+ "new_trailer_item" objects into "arg_item" objects.
+
+ Skip this intermediate storage of "new_trailer_item" objects in favor of
+ just storing "arg_item" objects. Remove the looping behavior of
+ parse_trailers_from_command_line_args() so that it parses a single
+ "--trailer ..." argument at a time. Rename it to
+ parse_trailer_from_command_line_arg() to reflect this new behavior of
+ only looking at one string (not multiple strings) at a time. Make
+ option_parse_trailer() call parse_trailer_from_command_line_arg() so
+ that the CLI arguments it sees are parsed immediately without the need
+ for intermediate storage.
+
+ Delete "new_trailer_item", because we don't need it any more.
+
+ In the next patch we will retire parse_trailer_from_command_line_arg()
+ as well, combining it with option_parse_trailer().
Signed-off-by: Linus Arver <linusa@google.com>
+ ## builtin/interpret-trailers.c ##
+@@ builtin/interpret-trailers.c: 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;
+
+ if (unset) {
+ free_new_trailers(trailers);
+@@ builtin/interpret-trailers.c: 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);
++ parse_trailer_from_command_line_arg(arg, where, if_exists, if_missing, trailers);
++
+ return 0;
+ }
+
+@@ builtin/interpret-trailers.c: 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)
+@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
+ if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
+ 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);
+- }
++ if (!opts->only_input)
++ process_trailers_lists(&head, new_trailer_head);
+
+ /* Print trailer block. */
+ format_trailers(opts, &head, &tb);
+@@ builtin/interpret-trailers.c: 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(trailers);
+
+ struct option options[] = {
+@@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
+ };
+
+ git_config(git_default_config, NULL);
++ trailer_config_init();
++
++ if (!opts.only_input)
++ parse_trailers_from_config(&configured_trailers);
+
+ argc = parse_options(argc, argv, prefix, options,
+ git_interpret_trailers_usage, 0);
+@@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
+ git_interpret_trailers_usage,
+ options);
+
++ list_splice(&configured_trailers, &trailers);
++
+ if (argc) {
+ int i;
+ for (i = 0; i < argc; i++)
+
## trailer.c ##
@@ trailer.c: void parse_trailers_from_config(struct list_head *config_head)
}
@@ trailer.c: 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;
++void parse_trailer_from_command_line_arg(const char *line,
++ enum trailer_where where,
++ enum trailer_if_exists if_exists,
++ enum trailer_if_missing if_missing,
++ struct list_head *arg_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).
-- */
+
+ /*
+ * In command-line arguments, '=' is accepted (in addition to the
+ * separators that are defined).
+ */
- char *cl_separators = xstrfmt("=%s", separators);
--
++ char *cl_separators = xstrfmt("=%s", trailer_default_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);
--
++ /* Add an arg item for a trailer from the command line */
++ ssize_t separator_pos = find_separator(line, cl_separators);
++ free(cl_separators);
+
- if (separator_pos == 0) {
- struct strbuf sb = STRBUF_INIT;
- strbuf_addstr(&sb, tr->text);
@@ trailer.c: void parse_trailers_from_config(struct list_head *config_head)
- (int) sb.len, sb.buf);
- strbuf_release(&sb);
- } else {
+- struct trailer_conf *conf_current = new_trailer_conf();
- parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
-- trailer_add_arg_item(strbuf_detach(&tok, NULL),
+- duplicate_trailer_conf(conf_current, conf);
++ if (separator_pos == 0) {
++ struct strbuf sb = STRBUF_INIT;
++ strbuf_addstr(&sb, line);
++ strbuf_trim(&sb);
++ error(_("empty trailer token in trailer '%.*s'"),
++ (int) sb.len, sb.buf);
++ strbuf_release(&sb);
++ } else {
++ struct trailer_conf *conf_current = new_trailer_conf();
++ parse_trailer(line, separator_pos, &tok, &val, &conf);
++ duplicate_trailer_conf(conf_current, conf);
+
+- /*
+- * Override conf_current with settings specified via CLI flags.
+- */
+- if (tr->where != WHERE_DEFAULT)
+- trailer_set_conf_where(tr->where, conf_current);
+- if (tr->if_exists != EXISTS_DEFAULT)
+- trailer_set_conf_if_exists(tr->if_exists, conf_current);
+- if (tr->if_missing != MISSING_DEFAULT)
+- trailer_set_conf_if_missing(tr->if_missing, conf_current);
+-
+- trailer_add_arg_item(arg_head,
+- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
-- conf,
-- arg_head);
+- conf_current);
+- free_trailer_conf(conf_current);
- }
-- }
++ /*
++ * Override conf_current with settings specified via CLI flags.
++ */
++ if (where != WHERE_DEFAULT)
++ trailer_set_conf_where(where, conf_current);
++ if (if_exists != EXISTS_DEFAULT)
++ trailer_set_conf_if_exists(if_exists, conf_current);
++ if (if_missing != MISSING_DEFAULT)
++ trailer_set_conf_if_missing(if_missing, conf_current);
++
++ trailer_add_arg_item(arg_head,
++ strbuf_detach(&tok, NULL),
++ strbuf_detach(&val, NULL),
++ conf_current);
++ free_trailer_conf(conf_current);
+ }
-
- free(cl_separators);
--}
--
+ }
+
static const char *next_line(const char *str)
+@@ trailer.c: void free_trailers(struct list_head *trailers)
+
+ void free_new_trailers(struct list_head *trailers)
{
- const char *nl = strchrnul(str, '\n');
+- struct list_head *pos, *tmp;
+- struct new_trailer_item *item;
++ struct list_head *pos, *p;
+
+- list_for_each_safe(pos, tmp, trailers) {
+- item = list_entry(pos, struct new_trailer_item, list);
++ list_for_each_safe(pos, p, trailers) {
+ list_del(pos);
+- free(item);
++ free_arg_item(list_entry(pos, struct arg_item, list));
+ }
+ }
+
## trailer.h ##
@@ trailer.h: int trailer_set_where(enum trailer_where *item, const char *value);
@@ trailer.h: int trailer_set_where(enum trailer_where *item, const char *value);
- 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,
+ void trailer_set_conf_where(enum trailer_where, struct trailer_conf *);
+ void trailer_set_conf_if_exists(enum trailer_if_exists, struct trailer_conf *);
+ void trailer_set_conf_if_missing(enum trailer_if_missing, struct trailer_conf *);
@@ trailer.h: 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 parse_trailer_from_command_line_arg(const char *line,
++ enum trailer_where where,
++ enum trailer_if_exists if_exists,
++ enum trailer_if_missing if_missing,
++ struct list_head *arg_head);
+
void process_trailers_lists(struct list_head *head,
struct list_head *arg_head);
-
-: ----------- > 26: 9720526dd8a trailer: unify "--trailer ..." arg handling
-: ----------- > 27: 26df2514acb trailer_set_*(): put out parameter at the end
10: bcd3fc9660e ! 28: 14927038d85 trailer: introduce "template" term for readability
@@ Commit message
- [*] 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 ([*]).
+ Reorder parameters to prefer input parameters toward the beginning and
+ out parameters at the end; 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
@@ Commit message
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.
+ make their behavior more explicit.
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).
+ find_same_and_apply_arg(), because it will be refactored soon.
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).
+ with trailer.*.cmd and would lose out on functionality).
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,
-
- static char *cl_separators;
+ return trailer_set_if_missing(arg, opt->value);
+ }
-static int option_parse_trailer(const struct option *opt,
- const char *arg, int unset)
@@ builtin/interpret-trailers.c: static int option_parse_if_missing(const struct op
struct strbuf val = STRBUF_INIT;
const struct trailer_conf *conf;
@@ builtin/interpret-trailers.c: static int option_parse_trailer(const struct option *opt,
- ssize_t separator_pos;
+ static char *cl_separators;
if (unset) {
- free_new_trailers(trailers);
@@ builtin/interpret-trailers.c: static int option_parse_trailer(const struct optio
}
@@ builtin/interpret-trailers.c: static int option_parse_trailer(const struct option *opt,
- */
- trailer_conf_set(where, if_exists, if_missing, conf_current);
+ if (if_missing != MISSING_DEFAULT)
+ trailer_set_conf_if_missing(if_missing, conf_current);
-- trailer_add_arg_item(strbuf_detach(&tok, NULL),
+- trailer_add_arg_item(trailers,
+- strbuf_detach(&tok, NULL),
+ add_trailer_template(strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL),
- conf_current,
-- trailers);
+- conf_current);
++ conf_current,
+ templates);
- } else {
- struct strbuf sb = STRBUF_INIT;
- strbuf_addstr(&sb, arg);
+ free_trailer_conf(conf_current);
+ }
+
@@ builtin/interpret-trailers.c: 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 *new_trailer_head,
+ struct list_head *templates,
const char *file)
{
@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct proces
/* Print the lines before the trailer block */
if (!opts->only_trailers)
@@ builtin/interpret-trailers.c: static void interpret_trailers(const struct process_trailer_options *opts,
+ fprintf(outfile, "\n");
-
- if (!opts->only_input) {
-- process_trailers_lists(&head, arg_trailers);
+ if (!opts->only_input)
+- process_trailers_lists(&head, new_trailer_head);
+ apply_trailer_templates(templates, &trailers_from_sb);
- }
/* Print trailer block. */
- format_trailers(opts, &head, &tb);
+- free_trailers(&head);
+ format_trailers(opts, &trailers_from_sb, &tb);
++ free_trailers(&trailers_from_sb);
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)
@@ builtin/interpret-trailers.c: 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(trailers);
+ LIST_HEAD(configured_templates);
+ LIST_HEAD(templates);
@@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **
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"),
+- OPT_CALLBACK(0, "trailer", &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),
@@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **
@@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
trailer_config_init();
- if (!opts.only_input) {
+ if (!opts.only_input)
- parse_trailers_from_config(&configured_trailers);
+ parse_trailer_templates_from_config(&configured_templates);
- }
-
- /*
-@@ builtin/interpret-trailers.c: int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
- free(cl_separators);
+ argc = parse_options(argc, argv, prefix, options,
+ git_interpret_trailers_usage, 0);
-- if (opts.only_input && !list_empty(&arg_trailers))
+- if (opts.only_input && !list_empty(&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_trailers, &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, &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, &trailers, NULL);
+ interpret_trailers(&opts, &templates, NULL);
}
-- free_new_trailers(&arg_trailers);
+- free_new_trailers(&trailers);
+ free_trailer_templates(&templates);
return 0;
@@ trailer.c: static char *apply_command(struct trailer_conf *conf, const char *arg
}
}
-@@ trailer.c: void duplicate_trailer_conf(struct trailer_conf *dst,
- dst->cmd = xstrdup_or_null(src->cmd);
+@@ trailer.c: void free_trailer_conf(struct trailer_conf *conf)
+ free(conf);
}
-static struct arg_item *get_conf_item(const char *name)
@@ trailer.c: static struct trailer_item *add_trailer_item(struct list_head *head,
return new_item;
}
--void trailer_add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
-- struct list_head *arg_head)
+-void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
+- const struct trailer_conf *conf)
+void add_trailer_template(char *tok, char *val, const struct trailer_conf *conf,
+ struct list_head *templates)
{
@@ trailer.c: static struct trailer_item *add_trailer_item(struct list_head *head,
- 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)),
+- trailer_add_arg_item(config_head,
+- xstrdup(token_from_item(item, NULL)),
- xstrdup(""),
-- &item->conf,
-- config_head);
+- &item->conf);
+ /*
+ * Get configured templates with a ".command" option.
+ *
@@ trailer.c: static struct trailer_item *add_trailer_item(struct list_head *head,
}
}
-@@ trailer.c: 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;
@@ trailer.c: static size_t find_trailer_block_start(const char *buf, size_t len)
possible_continuation_lines = 0;
if (recognized_prefix)
@@ trailer.c: void free_trailers(struct list_head *trailers)
## trailer.h ##
-@@ trailer.h: void duplicate_trailer_conf(struct trailer_conf *dst,
-
+@@ trailer.h: 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);
-+void add_trailer_template(char *tok, char *val, const struct trailer_conf *conf,
+-void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
+- const struct trailer_conf *conf);
++void add_trailer_template(char *tok, char *val, const struct trailer_conf *,
+ struct list_head *templates);
struct process_trailer_options {
@@ trailer.h: struct process_trailer_options {
-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);
++void apply_trailer_templates(struct list_head *templates,
++ struct list_head *trailers_head);
ssize_t find_separator(const char *line, const char *separators);
-@@ trailer.h: void format_trailers(const struct process_trailer_options *opts,
+@@ trailer.h: void format_trailers(const struct process_trailer_options *,
struct list_head *trailers,
struct strbuf *out);
void free_trailers(struct list_head *);
-void free_new_trailers(struct list_head *);
+ void free_trailer_conf(struct trailer_conf *);
+void free_trailer_templates(struct list_head *);
/*
--
gitgitgadget
^ permalink raw reply
* [PATCH v4 01/28] trailer: free trailer_info _after_ all related usage
From: Linus Arver via GitGitGadget @ 2024-02-06 5:12 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.v4.git.1707196348.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
In de7c27a186 (trailer: use offsets for trailer_start/trailer_end,
2023-10-20), we started using trailer block offsets in trailer_info. In
particular, we dropped the use of a separate stack variable "size_t
trailer_end", in favor of accessing the new "trailer_block_end" member
of trailer_info (as "info.trailer_block_end").
At that time, we forgot to also move the
trailer_info_release(&info);
line to be _after_ this new use of the trailer_info struct. Move it now.
Note that even without this patch, we didn't have leaks or any other
problems because trailer_info_release() only frees memory allocated on
the heap. The "trailer_block_end" member was allocated on the stack back
then (as it is now) so it was still safe to use for all this time.
Reported-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/trailer.c b/trailer.c
index 3a0710a4583..e1d83390b66 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1111,13 +1111,12 @@ void process_trailers(const char *file,
}
print_all(outfile, &head, opts);
-
free_all(&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);
+ trailer_info_release(&info);
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] builtin/stash: report failure to write to index
From: Patrick Steinhardt @ 2024-02-06 5:06 UTC (permalink / raw)
To: Rubén Justo; +Cc: git, moti sd
In-Reply-To: <f07d33e4-5595-43e3-838e-fd89c5621485@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9056 bytes --]
On Mon, Feb 05, 2024 at 11:22:01PM +0100, Rubén Justo wrote:
> On 05-feb-2024 08:01:04, Patrick Steinhardt wrote:
> > The git-stash(1) command needs to write to the index for many of its
> > operations. When the index is locked by a concurrent writer it will thus
> > fail to operate, which is expected. What is not expected though is that
> > we do not print any error message at all in this case. The user can thus
> > easily miss the fact that the command didn't do what they expected it to
> > do and would be left wondering why that is.
> >
> > Fix this bug and report failures to write to the index. Add tests for
> > the subcommands which hit the respective code paths.
> >
> > Note that the chosen error message ("Cannot write to the index") does
> > not match our guidelines as it starts with a capitalized letter. This is
> > intentional though and matches the style of all the other messages used
> > in git-stash(1).
>
> Your message made me curious, so I ran:
>
> $ git grep -E '(die|error)\(' builtin/stash.c
>
> builtin/stash.c:168: die(_("'%s' is not a stash-like commit"), revision);
> builtin/stash.c:214: return error(_("%s is not a valid reference"), revision);
> builtin/stash.c:261: return error(_("git stash clear with arguments is "
> builtin/stash.c:303: return error(_("unable to write new index file"));
> builtin/stash.c:487: die("Failed to move %s to %s\n",
> builtin/stash.c:523: die(_("Unable to write index."));
> builtin/stash.c:544: return error(_("cannot apply a stash in the middle of a merge"));
> builtin/stash.c:555: return error(_("could not generate diff %s^!."),
> builtin/stash.c:562: return error(_("conflicts in index. "
> builtin/stash.c:569: return error(_("could not save index tree"));
> builtin/stash.c:610: ret = error(_("could not write index"));
> builtin/stash.c:630: ret = error(_("could not restore untracked files from stash"));
> builtin/stash.c:702: return error(_("%s: Could not drop stash entry"),
> builtin/stash.c:721: return error(_("'%s' is not a stash reference"), info->revision.buf);
> builtin/stash.c:872: die(_("failed to parse tree"));
> builtin/stash.c:883: die(_("failed to unpack trees"));
> builtin/stash.c:1547: if (report_path_error(ps_matched, ps)) {
> builtin/stash.c:1763: die("subcommand wasn't specified; 'push' can't be assumed due to unexpected token '%s'",
> builtin/stash.c:1773: die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--patch");
> builtin/stash.c:1776: die(_("options '%s' and '%s' cannot be used together"), "--pathspec-from-file", "--staged");
> builtin/stash.c:1779: die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
> builtin/stash.c:1785: die(_("the option '%s' requires '%s'"), "--pathspec-file-nul", "--pathspec-from-file");
>
> Certainly, there are some error messages in builtin/stash.c that do not
> follow the notes in Documentation/CodingGuideLines, but it does not seem
> to be "the style of all other messages" in git-stash.
>
> However, your message is clear ... What error messages are you
> considering?
That's because many of the code paths don't use either `error()` or
`die()`, but use `fprintf_ln()` instead:
$ git grep 'fprintf_ln(stderr' -- builtin/stash.c
builtin/stash.c: fprintf_ln(stderr, _("Too many revisions specified:%s"),
builtin/stash.c: fprintf_ln(stderr, _("No stash entries found."));
builtin/stash.c: fprintf_ln(stderr, _("Index was not unstashed."));
builtin/stash.c: fprintf_ln(stderr, _("No branch name specified"));
builtin/stash.c: fprintf_ln(stderr, _("Cannot update %s with %s"),
builtin/stash.c: fprintf_ln(stderr, _("\"git stash store\" requires one "
builtin/stash.c: fprintf_ln(stderr, _("Cannot update %s with %s"),
builtin/stash.c: fprintf_ln(stderr, _("No staged changes"));
builtin/stash.c: fprintf_ln(stderr, _("No changes selected"));
builtin/stash.c: fprintf_ln(stderr, _("You do not have "
builtin/stash.c: fprintf_ln(stderr, _("Cannot save the current "
builtin/stash.c: fprintf_ln(stderr, _("Cannot save "
builtin/stash.c: fprintf_ln(stderr, _("Cannot save the current "
builtin/stash.c: fprintf_ln(stderr, _("Cannot save the current "
builtin/stash.c: fprintf_ln(stderr, _("Cannot save the current "
builtin/stash.c: fprintf_ln(stderr, _("Cannot record "
builtin/stash.c: fprintf_ln(stderr, _("Can't use --patch and --include-untracked"
builtin/stash.c: fprintf_ln(stderr, _("Can't use --staged and --include-untracked"
builtin/stash.c: fprintf_ln(stderr, _("Did you forget to 'git add'?"));
builtin/stash.c: fprintf_ln(stderr, _("Cannot initialize stash"));
builtin/stash.c: fprintf_ln(stderr, _("Cannot save the current status"));
builtin/stash.c: fprintf_ln(stderr, _("Cannot remove "
Overall, "builtin/stash.c" is a wild mixture of error styles. Some are
likely user-facing, where it might be a good idea to use `fprinf_ln()`
instead of `error()`. But some of them are closer to what we do in this
patch and likely should be converted to use `error()`, too.
I dunno, I find this to be confusing. But I spotted that there's an
instance already where we say `error("cannot write index")`, so I'll
just use that.
Patrick
> >
> > Reported-by: moti sd <motisd8@gmail.com>
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > builtin/stash.c | 6 +++---
> > t/t3903-stash.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 55 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/stash.c b/builtin/stash.c
> > index b2813c614c..9df072b459 100644
> > --- a/builtin/stash.c
> > +++ b/builtin/stash.c
> > @@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
> > repo_read_index_preload(the_repository, NULL, 0);
> > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
> > NULL, NULL, NULL))
> > - return -1;
> > + return error(_("Cannot write to the index"));
> >
> > if (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0,
> > NULL))
> > @@ -1364,7 +1364,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
> > repo_read_index_preload(the_repository, NULL, 0);
> > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
> > NULL, NULL, NULL) < 0) {
> > - ret = -1;
> > + ret = error(_("Cannot write to the index"));
> > goto done;
> > }
> >
> > @@ -1555,7 +1555,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
> >
> > if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
> > NULL, NULL, NULL)) {
> > - ret = -1;
> > + ret = error(_("Cannot write to the index"));
> > goto done;
> > }
> >
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> > index 3319240515..770881e537 100755
> > --- a/t/t3903-stash.sh
> > +++ b/t/t3903-stash.sh
> > @@ -1516,4 +1516,56 @@ test_expect_success 'restore untracked files even when we hit conflicts' '
> > )
> > '
> >
> > +test_expect_success 'stash create reports a locked index' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + (
> > + cd repo &&
> > + test_commit A A.file &&
> > + echo change >A.file &&
> > + touch .git/index.lock &&
> > +
> > + cat >expect <<-EOF &&
> > + error: Cannot write to the index
> > + EOF
> > + test_must_fail git stash create 2>err &&
> > + test_cmp expect err
> > + )
> > +'
> > +
> > +test_expect_success 'stash push reports a locked index' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + (
> > + cd repo &&
> > + test_commit A A.file &&
> > + echo change >A.file &&
> > + touch .git/index.lock &&
> > +
> > + cat >expect <<-EOF &&
> > + error: Cannot write to the index
> > + EOF
> > + test_must_fail git stash push 2>err &&
> > + test_cmp expect err
> > + )
> > +'
> > +
> > +test_expect_success 'stash apply reports a locked index' '
> > + test_when_finished "rm -rf repo" &&
> > + git init repo &&
> > + (
> > + cd repo &&
> > + test_commit A A.file &&
> > + echo change >A.file &&
> > + git stash push &&
> > + touch .git/index.lock &&
> > +
> > + cat >expect <<-EOF &&
> > + error: Cannot write to the index
> > + EOF
> > + test_must_fail git stash apply 2>err &&
> > + test_cmp expect err
> > + )
> > +'
> > +
> > test_done
> > --
> > 2.43.GIT
> >
>
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
From: Junio C Hamano @ 2024-02-06 3:54 UTC (permalink / raw)
To: Vegard Nossum; +Cc: Kristoffer Haugsbakk, git, Jonathan Nieder, Phillip Wood
In-Reply-To: <b3ec5d0b-ac17-4d1e-a17d-d5adfbfc6ccf@oracle.com>
Vegard Nossum <vegard.nossum@oracle.com> writes:
> On 06/02/2024 00:09, Junio C Hamano wrote:
>> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>>
>>> On Mon, Feb 5, 2024, at 15:13, Vegard Nossum wrote:
>>>> Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/
>>>> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
>>>> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>>>
>>> `Link` is not really used a lot. Junio’s `refs/notes/amlog` will point
>>> back to the patch (which is often close to the “suggested by” and so
>>> on).
>> Good. Also, is there [PATCH 1/2] that comes before this patch?
>
> Yes, kind of -- that's the testcase at the root of the thread:
>
> https://lore.kernel.org/git/20240202091850.160203-1-vegard.nossum@oracle.com/
>
> ("t/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken
> behavior")
If the first one was NOT marked as [1/2], it is customary to call
such an "we thought just one patch was sufficient, but here is
another" step [2/1] instead, and that was why I was confused.
Perhaps it is a good idea to squash them together as a single bugfix
patch?
Thanks.
^ permalink raw reply
* Re: [PATCH 1/4] notes: print note blob to stdout directly
From: Junio C Hamano @ 2024-02-06 3:44 UTC (permalink / raw)
To: Maarten Bosmans; +Cc: git, Teng Long, Maarten Bosmans
In-Reply-To: <20240205204932.16653-2-maarten.bosmans@vortech.nl>
Maarten Bosmans <mkbosmans@gmail.com> writes:
> From: Maarten Bosmans <mkbosmans@gmail.com>
>
> From: Maarten Bosmans <maarten.bosmans@vortech.nl>
Which one of you are you? Please make up your mind and use only one
;-) IOW, the first one is unneeded, as the latter matches what you
have on the S-o-b line.
> Avoid the need to launch a subprocess by calling stream_blob_to_fd
> directly. This does not only get rid of the overhead of a separate
> child process, but also avoids the initalization of the whole log
> machinery that `git show` does. That is needed for example to show
> decorated commits and applying the mailmap. For simply displaying
> a blob however, the only useful thing show does is enabling the pager.
>
> Signed-off-by: Maarten Bosmans <maarten.bosmans@vortech.nl>
> ---
> builtin/notes.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
I am not sure if we want to accept an approach that feels somewhat
narrow/short sighted, like this patch. When "git show" learns an
improved way to show blob objects, who will update the code this
patch touches to teach it to use the same improved way to show the
notes?
I actually was hoping, after seeing the use case description in the
cover letter, that the series would be introducing a batch mode
interface to allow callers to ask notes for many objects and have
the command respond notes for these objects in a way that which
piece of output corresponds to which object in the request, reducing
the process overhead amortised over many objects.
^ permalink raw reply
* Re: [PATCH] branch: clarify <oldbranch> and <newbranch> terms further
From: Dragan Simic @ 2024-02-06 3:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kyle Lippincott, git
In-Reply-To: <xmqqttmmlahf.fsf@gitster.g>
Hello Junio,
On 2024-02-06 01:09, Junio C Hamano wrote:
> Kyle Lippincott <spectral@google.com> writes:
>
>> I'm not super pleased with that second sentence, and maybe we
>> shouldn't include it here. Maybe it belongs on the documentation for
>> --move and --copy instead? It's sort of mentioned in the text at the
>> top describing the -m/-M and -c/-C options, though it's not clear from
>> that text what actually happens to the existing copy of <newbranch> if
>> one uses --force. If we could include a better description of what
>> happens to the existing branch when one uses --force, that'd be nice.
>
> My preference is to limit the "OPTIONS" section to dashed options.
> If "--move" takes one or two arguments, update its description to
> talk about how these one or two arguments are used, perhaps like
>
> -m [<oldbranch>] <newbranch>::
> --move [<oldbranch>] <newbranch>::
>
> Rename an existing branch <oldbranch>, which
> defaults to the current branch, to <newbranch>. The
> configuration variables about and the reflog of
> <oldbranch> are also renamed appropriately to be
> used with <newbranch>. It is an error if <newbranch>
> exists (you can use `--force` to clobber an existing
> <newbranch>).
>
> or something like that.
Thank you for your detailed feedback!
I like it and I fully agree that describing the operation arguments
fits and flows much better in the descriptions of their respective
operations. Describing the outcome of forced operations is also
needed for completeness, and for safety.
I'll prepare and send a v2 that takes that approach.
> Listing non-options in the list may have been a misguided attempt to
> "save" description on arguments that are common to multiple options,
> but it is not working. We can see the bad effect of that approach
> only by looking at the current description of the above option,
> which reads:
>
> -m::
> --move::
> Move/rename a branch, together with its config and reflog.
>
> It does not mentioning what arguments "--move" takes, and does not
> even refer the readers to the entries for <newbranch> and
> <oldbranch>, so the only plausible way the users can learn what they
> want about this single option is by reading the page from top to
> bottom.
... or the users can perhaps learn by simply experimenting a bit
and observing what happens, after getting a bit disappointed by the
current descriptions of the operations and resorting to the rather
usual "tl;dr" approach.
Avoiding such "tl;dr" scenarios is the way to move forward with the
improvements to the git man pages, if you agree.
> And trim the DESCRIPTION part. A lot. Because things are explained
> redundantly between there and the OPTIONS part, and their details
> are waiting to drift apart unless we are careful.
>
> I think I laid all this out and more in a separate message.
>
> https://lore.kernel.org/git/xmqq8r4zln08.fsf@gitster.g/
I agree about this as well, but that will perhaps be handled in some
separate patch for the git-branch(1) man page.
^ permalink raw reply
* Re: [PATCH] builtin/stash: report failure to write to index
From: Junio C Hamano @ 2024-02-06 3:24 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, moti sd
In-Reply-To: <2cd44b01dc29b099a07658499481a6847c46562d.1707116449.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> The git-stash(1) command needs to write to the index for many of its
> operations. When the index is locked by a concurrent writer it will thus
> fail to operate, which is expected. What is not expected though is that
> we do not print any error message at all in this case. The user can thus
> easily miss the fact that the command didn't do what they expected it to
> do and would be left wondering why that is.
Hopefully, they know they notice the exit status of the command, or
do we throw the error away and exit(0) from the program?
In any case, telling the users what did (and did not) happen is a
good idea.
> Fix this bug and report failures to write to the index. Add tests for
> the subcommands which hit the respective code paths.
>
> Note that the chosen error message ("Cannot write to the index") does
> not match our guidelines as it starts with a capitalized letter. This is
> intentional though and matches the style of all the other messages used
> in git-stash(1).
Style may be OK, but I wonder if they should say different things,
to hint what failed. For example:
> @@ -537,7 +537,7 @@ static int do_apply_stash(const char *prefix, struct stash_info *info,
> repo_read_index_preload(the_repository, NULL, 0);
> if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
> NULL, NULL, NULL))
> - return -1;
> + return error(_("Cannot write to the index"));
>
> if (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0,
> NULL))
This failure and message comes before anything interesting happens.
We attempted to refresh the current index and failed to write out
the result, meaning that whatever index we had on disk did not get
overwritten. Is this new message enough to tell the user that we
didn't touch the working tree or the index, which would happen if
even some part of "stash apply" happened? Or is it obvious that we
did not do anything?
> @@ -1364,7 +1364,7 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
> repo_read_index_preload(the_repository, NULL, 0);
> if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
> NULL, NULL, NULL) < 0) {
> - ret = -1;
> + ret = error(_("Cannot write to the index"));
> goto done;
> }
Ditto.
> @@ -1555,7 +1555,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>
> if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
> NULL, NULL, NULL)) {
> - ret = -1;
> + ret = error(_("Cannot write to the index"));
> goto done;
> }
>
Ditto.
Thanks.
^ permalink raw reply
* Re: [PATCH] branch: clarify <oldbranch> and <newbranch> terms further
From: Dragan Simic @ 2024-02-06 3:16 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: git
In-Reply-To: <CAO_smViHVZRObZjg0tEPXezJZb7wvs9LQdHUFJQTK4-ASCfrmw@mail.gmail.com>
Hello Kyle,
On 2024-02-06 00:53, Kyle Lippincott wrote:
> On Mon, Feb 5, 2024 at 4:45 AM Dragan Simic <dsimic@manjaro.org> wrote:
>>
>> Clarify further the uses for <oldbranch> and describe the additional
>> use
>> for <newbranch>. Mentioning both renaming and copying in these places
>> might
>> seem a bit redundant, but it should actually make understanding these
>> terms
>> easier to the readers of the git-branch(1) man page.
>>
>> Signed-off-by: Dragan Simic <dsimic@manjaro.org>
>> ---
>> Documentation/git-branch.txt | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/git-branch.txt
>> b/Documentation/git-branch.txt
>> index 0b0844293235..7392c2f0797d 100644
>> --- a/Documentation/git-branch.txt
>> +++ b/Documentation/git-branch.txt
>> @@ -312,12 +312,14 @@ superproject's "origin/main", but tracks the
>> submodule's "origin/main".
>> option is omitted, the current HEAD will be used instead.
>>
>> <oldbranch>::
>> - The name of an existing branch. If this option is omitted,
>> - the name of the current branch will be used instead.
>> + The name of an existing branch to be renamed or copied.
>> + If this option is omitted, the name of the current branch
>> + will be used instead.
>>
>> <newbranch>::
>> - The new name for an existing branch. The same restrictions as
>> for
>> - <branchname> apply.
>> + The new name for an existing branch, when renaming a branch,
>> + or the name for a new branch, when copying a branch. The same
>> + naming restrictions apply as for <branchname>.
>
> The precision here makes me worry that I'm potentially missing
> something when reading this, and has made me re-read it multiple times
> to try to figure out what it is.
Thank you for your feedback!
I'd agree that the first sentence I sent isn't exactly a textbook
example of clarity, :) but it also isn't that bad; it tries to say
something like "one thing when this, other thing when that".
> I think this would be cleaner:
>
> The name to give the branch created by the rename or copy operation.
> The operation fails if <newbranch> already exists, use --force to
> ignore
> this error. The same naming restrictions apply as for <branchname>.
>
> I'm not super pleased with that second sentence, and maybe we
> shouldn't include it here. Maybe it belongs on the documentation for
> --move and --copy instead? It's sort of mentioned in the text at the
> top describing the -m/-M and -c/-C options, though it's not clear from
> that text what actually happens to the existing copy of <newbranch> if
> one uses --force. If we could include a better description of what
> happens to the existing branch when one uses --force, that'd be nice.
I agree that moving everything to the descriptions of the move and
copy operations, as Junio described further in his message, is a much
better way moving forward. Describing the results of forced operation
is also needed for completeness.
I'll prepare and send a v2 that takes that approach.
>>
>> --sort=<key>::
>> Sort based on the key given. Prefix `-` to sort in descending
>>
^ permalink raw reply
* [PATCH v5 6/7] completion: bisect: complete log opts for visualize subcommand
From: Britton Leo Kerin @ 2024-02-06 2:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240206020930.312164-1-britton.kerin@gmail.com>
Arguments passed to the "visualize" subcommand of git-bisect(1) get
forwarded to git-log(1). It thus supports the same options as git-log(1)
would, but our Bash completion script does not know to handle this.
Make completion of porcelain git-log options and option arguments to the
visualize subcommand work by calling __git_complete_log_opts when the
start of an option to the subcommand is seen (visualize doesn't support
any options besides the git-log options). Add test.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 4 ++++
t/t9902-completion.sh | 14 ++++++++++++++
2 files changed, 18 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 8c3b1b8e96..b4cd94182e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1490,6 +1490,10 @@ _git_bisect ()
__gitcomp "--term-good --term-old --term-bad --term-new"
return
;;
+ visualize)
+ __git_complete_log_opts
+ return
+ ;;
bad|new|"$term_bad"|good|old|"$term_good"|reset|skip)
__git_complete_refs
;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index f4d3aa67e3..74132699b1 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1360,6 +1360,20 @@ test_expect_success 'git-bisect - options to terms subcommand are candidates' '
)
'
+test_expect_success 'git-bisect - git-log options to visualize subcommand are candidates' '
+ (
+ cd git-bisect &&
+ # The completion used for git-log and here does not complete
+ # every git-log option, so rather than hope to stay in sync
+ # with exactly what it does we will just spot-test here.
+ test_completion "git bisect visualize --sta" <<-\EOF &&
+ --stat Z
+ EOF
+ test_completion "git bisect visualize --summar" <<-\EOF
+ --summary Z
+ EOF
+ )
+'
test_expect_success 'git checkout - completes refs and unique remote branches for DWIM' '
test_completion "git checkout " <<-\EOF
--
2.43.0
^ permalink raw reply related
* [PATCH v5 7/7] completion: bisect: recognize but do not complete view subcommand
From: Britton Leo Kerin @ 2024-02-06 2:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240206020930.312164-1-britton.kerin@gmail.com>
The "view" alias for the visualize subcommand is neither completed nor
recognized. It's undesirable to complete it because it's first letters
are the same as for visualize, making completion less rather than more
efficient without adding much in the way of interface discovery.
However, it needs to be recognized in order to enable log option
completion for it.
Recognize but do not complete the view command by creating and using
separate lists of completable_subcommands and all_subcommands. Add
tests.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 15 +++++++++++----
t/t9902-completion.sh | 24 ++++++++++++++++++++++++
2 files changed, 35 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b4cd94182e..b3d5468c15 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1462,12 +1462,19 @@ _git_bisect ()
# more usual bad/new/good/old because git bisect gives a good error
# message if these are given when not in use, and that's better than
# silent refusal to complete if the user is confused.
- local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
- local subcommand="$(__git_find_on_cmdline "$subcommands")"
+ #
+ # We want to recognize 'view' but not complete it, because it overlaps
+ # with 'visualize' too much and is just an alias for it.
+ #
+ local completable_subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+ local all_subcommands="$completable_subcommands view"
+
+ local subcommand="$(__git_find_on_cmdline "$all_subcommands")"
+
if [ -z "$subcommand" ]; then
__git_find_repo_path
if [ -f "$__git_repo_path"/BISECT_START ]; then
- __gitcomp "$subcommands"
+ __gitcomp "$completable_subcommands"
else
__gitcomp "replay start"
fi
@@ -1490,7 +1497,7 @@ _git_bisect ()
__gitcomp "--term-good --term-old --term-bad --term-new"
return
;;
- visualize)
+ visualize|view)
__git_complete_log_opts
return
;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 74132699b1..6a6019b0a8 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1375,6 +1375,30 @@ test_expect_success 'git-bisect - git-log options to visualize subcommand are ca
)
'
+test_expect_success 'git-bisect - view subcommand is not a candidate' '
+ (
+ cd git-bisect &&
+ test_completion "git bisect vi" <<-\EOF
+ visualize Z
+ EOF
+ )
+'
+
+test_expect_success 'git-bisect - existing view subcommand is recognized and enables completion of git-log options' '
+ (
+ cd git-bisect &&
+ # The completion used for git-log and here does not complete
+ # every git-log option, so rather than hope to stay in sync
+ # with exactly what it does we will just spot-test here.
+ test_completion "git bisect view --sta" <<-\EOF &&
+ --stat Z
+ EOF
+ test_completion "git bisect view --summar" <<-\EOF
+ --summary Z
+ EOF
+ )
+'
+
test_expect_success 'git checkout - completes refs and unique remote branches for DWIM' '
test_completion "git checkout " <<-\EOF
HEAD Z
--
2.43.0
^ permalink raw reply related
* [PATCH v5 5/7] completion: new function __git_complete_log_opts
From: Britton Leo Kerin @ 2024-02-06 2:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240206020930.312164-1-britton.kerin@gmail.com>
The options accepted by git-log are also accepted by at least one other
command (git-bisect). Factor the common option completion code into
a new function and use it from _git_log.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 57c6e09968..8c3b1b8e96 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2089,10 +2089,12 @@ __git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-c
__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-_git_log ()
+# Complete porcelain (i.e. not git-rev-list) options and at least some
+# option arguments accepted by git-log. Note that this same set of options
+# are also accepted by some other git commands besides git-log.
+__git_complete_log_opts ()
{
- __git_has_doubledash && return
- __git_find_repo_path
+ COMPREPLY=""
local merge=""
if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
@@ -2186,6 +2188,16 @@ _git_log ()
return
;;
esac
+}
+
+_git_log ()
+{
+ __git_has_doubledash && return
+ __git_find_repo_path
+
+ __git_complete_log_opts
+ [ -z "$COMPREPLY" ] || return
+
__git_complete_revlist
}
--
2.43.0
^ permalink raw reply related
* [PATCH v5 4/7] completion: bisect: complete missing --first-parent and --no-checkout options
From: Britton Leo Kerin @ 2024-02-06 2:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240206020930.312164-1-britton.kerin@gmail.com>
The --first-parent and --no-checkout options to the start subcommand of
git-bisect(1) are not completed.
Enable completion of the --first-parent and --no-checkout options to the
start subcommand. Add test.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 2 +-
t/t9902-completion.sh | 11 +++++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6a3d9c7760..57c6e09968 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1478,7 +1478,7 @@ _git_bisect ()
start)
case "$cur" in
--*)
- __gitcomp "--term-new --term-bad --term-old --term-good"
+ __gitcomp "--first-parent --no-checkout --term-new --term-bad --term-old --term-good"
return
;;
*)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 409a5a49d5..f4d3aa67e3 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1271,6 +1271,17 @@ test_expect_success 'git bisect - when not bisecting, complete only replay and s
EOF
'
+test_expect_success 'git bisect - complete options to start subcommand' '
+ test_completion "git bisect start --" <<-\EOF
+ --term-new Z
+ --term-bad Z
+ --term-old Z
+ --term-good Z
+ --no-checkout Z
+ --first-parent Z
+ EOF
+'
+
test_expect_success 'setup for git-bisect tests requiring a repo' '
git init git-bisect &&
(
--
2.43.0
^ permalink raw reply related
* [PATCH v5 3/7] completion: bisect: complete custom terms and related options
From: Britton Leo Kerin @ 2024-02-06 2:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240206020930.312164-1-britton.kerin@gmail.com>
git bisect supports the use of custom terms via the --term-(new|bad) and
--term-(old|good) options, but the completion code doesn't know about
these options or the new subcommands they define.
Add support for these options and the custom subcommands by checking for
BISECT_TERMS and adding them to the list of subcommands. Add tests.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 32 ++++++++++++++++++++++++--
t/t9902-completion.sh | 15 ++++++++++++
2 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 06d0b156e7..6a3d9c7760 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1449,7 +1449,20 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad new good old skip reset visualize replay log run help"
+ __git_find_repo_path
+
+ # If a bisection is in progress get the terms being used.
+ local term_bad term_good
+ if [ -f "$__git_repo_path"/BISECT_TERMS ]; then
+ term_bad=$(__git bisect terms --term-bad)
+ term_good=$(__git bisect terms --term-good)
+ fi
+
+ # We will complete any custom terms, but still always complete the
+ # more usual bad/new/good/old because git bisect gives a good error
+ # message if these are given when not in use, and that's better than
+ # silent refusal to complete if the user is confused.
+ local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__git_find_repo_path
@@ -1462,7 +1475,22 @@ _git_bisect ()
fi
case "$subcommand" in
- bad|new|good|old|reset|skip|start)
+ start)
+ case "$cur" in
+ --*)
+ __gitcomp "--term-new --term-bad --term-old --term-good"
+ return
+ ;;
+ *)
+ __git_complete_refs
+ ;;
+ esac
+ ;;
+ terms)
+ __gitcomp "--term-good --term-old --term-bad --term-new"
+ return
+ ;;
+ bad|new|"$term_bad"|good|old|"$term_good"|reset|skip)
__git_complete_refs
;;
*)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 7388c892cf..409a5a49d5 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1321,9 +1321,12 @@ test_expect_success 'git-bisect - when bisecting all subcommands are candidates'
test_completion "git bisect " <<-\EOF
start Z
bad Z
+ custom_new Z
+ custom_old Z
new Z
good Z
old Z
+ terms Z
skip Z
reset Z
visualize Z
@@ -1334,6 +1337,18 @@ test_expect_success 'git-bisect - when bisecting all subcommands are candidates'
EOF
)
'
+test_expect_success 'git-bisect - options to terms subcommand are candidates' '
+ (
+ cd git-bisect &&
+ test_completion "git bisect terms --" <<-\EOF
+ --term-bad Z
+ --term-good Z
+ --term-new Z
+ --term-old Z
+ EOF
+ )
+'
+
test_expect_success 'git checkout - completes refs and unique remote branches for DWIM' '
test_completion "git checkout " <<-\EOF
--
2.43.0
^ permalink raw reply related
* [PATCH v5 0/7] completion: improvements for git-bisect
From: Britton Leo Kerin @ 2024-02-06 2:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240128223447.342493-1-britton.kerin@gmail.com>
Relative to v4 this make the following actual changes:
* fixes GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME to 'master' for all of
t9902-completion.sh as suggested by Junio. This change affects all
of t9902-completion.sh so I've put it by itself in it's own commit.
* uses BISECT_TERMS to avoid pointless processes as suggested by Patrick.
The commits are also refactored as follows:
* squashes the introduction of __git_complete_log_opts in with it's
first use a suggested by Patrick.
* spreads tests across commits as suggest by Patrick.
Thanks for the reviews.
Britton Leo Kerin (7):
completion: tests: always use 'master' for default initial branch name
completion: bisect: complete bad, new, old, and help subcommands
completion: bisect: complete custom terms and related options
completion: bisect: complete missing --first-parent and --no-checkout
options
completion: new function __git_complete_log_opts
completion: bisect: complete log opts for visualize subcommand
completion: bisect: recognize but do not complete view subcommand
contrib/completion/git-completion.bash | 65 ++++++++++--
t/t9902-completion.sh | 140 +++++++++++++++++++++++++
2 files changed, 198 insertions(+), 7 deletions(-)
Range-diff against v4:
1: 66153024c3 < -: ---------- completion: bisect: complete bad, new, old, and help subcommands
-: ---------- > 1: 71b73de914 completion: tests: always use 'master' for default initial branch name
8: 451b7a4467 ! 2: 3a478a7a08 completion: add tests for git-bisect
@@ Metadata
Author: Britton Leo Kerin <britton.kerin@gmail.com>
## Commit message ##
- completion: add tests for git-bisect
+ completion: bisect: complete bad, new, old, and help subcommands
- There aren't any tests for completion of git bisect and it's
- subcommands.
+ The bad, new, old and help subcommands to git-bisect(1) are not
+ completed.
+ Add the bad, new, old, and help subcommands to the appropriate lists
+ such that the commands and their possible ref arguments are completed.
Add tests.
+ Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.c
+
+ ## contrib/completion/git-completion.bash ##
+@@ contrib/completion/git-completion.bash: _git_bisect ()
+ {
+ __git_has_doubledash && return
+
+- local subcommands="start bad good skip reset visualize replay log run"
++ local subcommands="start bad new good old skip reset visualize replay log run help"
+ local subcommand="$(__git_find_on_cmdline "$subcommands")"
+ if [ -z "$subcommand" ]; then
+ __git_find_repo_path
+@@ contrib/completion/git-completion.bash: _git_bisect ()
+ fi
+
+ case "$subcommand" in
+- bad|good|reset|skip|start)
++ bad|new|good|old|reset|skip|start)
+ __git_complete_refs
+ ;;
+ *)
+
## t/t9902-completion.sh ##
@@ t/t9902-completion.sh: test_expect_success 'git switch - with no options, complete local branches and u
EOF
@@ t/t9902-completion.sh: test_expect_success 'git switch - with no options, comple
+ EOF
+'
+
-+test_expect_success 'git bisect - complete options to start subcommand' '
-+ test_completion "git bisect start --" <<-\EOF
-+ --term-new Z
-+ --term-bad Z
-+ --term-old Z
-+ --term-good Z
-+ --no-checkout Z
-+ --first-parent Z
-+ EOF
-+'
-+
+test_expect_success 'setup for git-bisect tests requiring a repo' '
+ git init git-bisect &&
+ (
@@ t/t9902-completion.sh: test_expect_success 'git switch - with no options, comple
+ test_completion "git bisect " <<-\EOF
+ start Z
+ bad Z
-+ custom_new Z
-+ custom_old Z
+ new Z
+ good Z
+ old Z
-+ terms Z
+ skip Z
+ reset Z
+ visualize Z
@@ t/t9902-completion.sh: test_expect_success 'git switch - with no options, comple
+ EOF
+ )
+'
-+test_expect_success 'git-bisect - options to terms subcommand are candidates' '
-+ (
-+ cd git-bisect &&
-+ test_completion "git bisect terms --" <<-\EOF
-+ --term-bad Z
-+ --term-good Z
-+ --term-new Z
-+ --term-old Z
-+ EOF
-+ )
-+'
-+
-+test_expect_success 'git-bisect - git-log options to visualize subcommand are candidates' '
-+ (
-+ cd git-bisect &&
-+ # The completion used for git-log and here does not complete
-+ # every git-log option, so rather than hope to stay in sync
-+ # with exactly what it does we will just spot-test here.
-+ test_completion "git bisect visualize --sta" <<-\EOF &&
-+ --stat Z
-+ EOF
-+ test_completion "git bisect visualize --summar" <<-\EOF
-+ --summary Z
-+ EOF
-+ )
-+'
-+
-+test_expect_success 'git-bisect - view subcommand is not a candidate' '
-+ (
-+ cd git-bisect &&
-+ test_completion "git bisect vi" <<-\EOF
-+ visualize Z
-+ EOF
-+ )
-+'
-+
-+test_expect_success 'git-bisect - existing view subcommand is recognized and enables completion of git-log options' '
-+ (
-+ cd git-bisect &&
-+ # The completion used for git-log and here does not complete
-+ # every git-log option, so rather than hope to stay in sync
-+ # with exactly what it does we will just spot-test here.
-+ test_completion "git bisect view --sta" <<-\EOF &&
-+ --stat Z
-+ EOF
-+ test_completion "git bisect view --summar" <<-\EOF
-+ --summary Z
-+ EOF
-+ )
-+'
+
test_expect_success 'git checkout - completes refs and unique remote branches for DWIM' '
test_completion "git checkout " <<-\EOF
2: 7eb8c842a3 ! 3: fab7159cf4 completion: bisect: complete custom terms and related options
@@ Commit message
these options or the new subcommands they define.
Add support for these options and the custom subcommands by checking for
- them if a bisection is in progress and adding them to the list of
- subcommands.
+ BISECT_TERMS and adding them to the list of subcommands. Add tests.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
@@ contrib/completion/git-completion.bash: _git_bisect ()
+
+ # If a bisection is in progress get the terms being used.
+ local term_bad term_good
-+ if [ -f "$__git_repo_path"/BISECT_START ]; then
++ if [ -f "$__git_repo_path"/BISECT_TERMS ]; then
+ term_bad=$(__git bisect terms --term-bad)
+ term_good=$(__git bisect terms --term-good)
+ fi
@@ contrib/completion/git-completion.bash: _git_bisect ()
__git_complete_refs
;;
*)
+
+ ## t/t9902-completion.sh ##
+@@ t/t9902-completion.sh: test_expect_success 'git-bisect - when bisecting all subcommands are candidates'
+ test_completion "git bisect " <<-\EOF
+ start Z
+ bad Z
++ custom_new Z
++ custom_old Z
+ new Z
+ good Z
+ old Z
++ terms Z
+ skip Z
+ reset Z
+ visualize Z
+@@ t/t9902-completion.sh: test_expect_success 'git-bisect - when bisecting all subcommands are candidates'
+ EOF
+ )
+ '
++test_expect_success 'git-bisect - options to terms subcommand are candidates' '
++ (
++ cd git-bisect &&
++ test_completion "git bisect terms --" <<-\EOF
++ --term-bad Z
++ --term-good Z
++ --term-new Z
++ --term-old Z
++ EOF
++ )
++'
++
+
+ test_expect_success 'git checkout - completes refs and unique remote branches for DWIM' '
+ test_completion "git checkout " <<-\EOF
3: 5f5076bb93 < -: ---------- completion: bisect: complete missing --first-parent and --no-checkout options
4: c8ffa0e915 < -: ---------- completion: new function __git_complete_log_opts
5: 733613d1ed < -: ---------- completion: log: use __git_complete_log_opts
-: ---------- > 4: 73f3343b94 completion: bisect: complete missing --first-parent and --no-checkout options
-: ---------- > 5: a20846bbd3 completion: new function __git_complete_log_opts
6: 06f5973b3b ! 6: fe5545c9a3 completion: bisect: complete log opts for visualize subcommand
@@ Commit message
Make completion of porcelain git-log options and option arguments to the
visualize subcommand work by calling __git_complete_log_opts when the
start of an option to the subcommand is seen (visualize doesn't support
- any options besides the git-log options).
+ any options besides the git-log options). Add test.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
@@ contrib/completion/git-completion.bash: _git_bisect ()
bad|new|"$term_bad"|good|old|"$term_good"|reset|skip)
__git_complete_refs
;;
+
+ ## t/t9902-completion.sh ##
+@@ t/t9902-completion.sh: test_expect_success 'git-bisect - options to terms subcommand are candidates' '
+ )
+ '
+
++test_expect_success 'git-bisect - git-log options to visualize subcommand are candidates' '
++ (
++ cd git-bisect &&
++ # The completion used for git-log and here does not complete
++ # every git-log option, so rather than hope to stay in sync
++ # with exactly what it does we will just spot-test here.
++ test_completion "git bisect visualize --sta" <<-\EOF &&
++ --stat Z
++ EOF
++ test_completion "git bisect visualize --summar" <<-\EOF
++ --summary Z
++ EOF
++ )
++'
+
+ test_expect_success 'git checkout - completes refs and unique remote branches for DWIM' '
+ test_completion "git checkout " <<-\EOF
7: 1dc9323f24 ! 7: c9102ac532 completion: bisect: recognize but do not complete view subcommand
@@ Commit message
completion for it.
Recognize but do not complete the view command by creating and using
- separate lists of completable_subcommands and all_subcommands.
+ separate lists of completable_subcommands and all_subcommands. Add
+ tests.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
@@ contrib/completion/git-completion.bash: _git_bisect ()
__git_complete_log_opts
return
;;
+
+ ## t/t9902-completion.sh ##
+@@ t/t9902-completion.sh: test_expect_success 'git-bisect - git-log options to visualize subcommand are ca
+ )
+ '
+
++test_expect_success 'git-bisect - view subcommand is not a candidate' '
++ (
++ cd git-bisect &&
++ test_completion "git bisect vi" <<-\EOF
++ visualize Z
++ EOF
++ )
++'
++
++test_expect_success 'git-bisect - existing view subcommand is recognized and enables completion of git-log options' '
++ (
++ cd git-bisect &&
++ # The completion used for git-log and here does not complete
++ # every git-log option, so rather than hope to stay in sync
++ # with exactly what it does we will just spot-test here.
++ test_completion "git bisect view --sta" <<-\EOF &&
++ --stat Z
++ EOF
++ test_completion "git bisect view --summar" <<-\EOF
++ --summary Z
++ EOF
++ )
++'
++
+ test_expect_success 'git checkout - completes refs and unique remote branches for DWIM' '
+ test_completion "git checkout " <<-\EOF
+ HEAD Z
--
2.43.0
^ permalink raw reply
* [PATCH v5 2/7] completion: bisect: complete bad, new, old, and help subcommands
From: Britton Leo Kerin @ 2024-02-06 2:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240206020930.312164-1-britton.kerin@gmail.com>
The bad, new, old and help subcommands to git-bisect(1) are not
completed.
Add the bad, new, old, and help subcommands to the appropriate lists
such that the commands and their possible ref arguments are completed.
Add tests.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.c
---
contrib/completion/git-completion.bash | 4 +-
t/t9902-completion.sh | 71 ++++++++++++++++++++++++++
2 files changed, 73 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 185b47d802..06d0b156e7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1449,7 +1449,7 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad good skip reset visualize replay log run"
+ local subcommands="start bad new good old skip reset visualize replay log run help"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__git_find_repo_path
@@ -1462,7 +1462,7 @@ _git_bisect ()
fi
case "$subcommand" in
- bad|good|reset|skip|start)
+ bad|new|good|old|reset|skip|start)
__git_complete_refs
;;
*)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a5d4e900a2..7388c892cf 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1264,6 +1264,77 @@ test_expect_success 'git switch - with no options, complete local branches and u
EOF
'
+test_expect_success 'git bisect - when not bisecting, complete only replay and start subcommands' '
+ test_completion "git bisect " <<-\EOF
+ replay Z
+ start Z
+ EOF
+'
+
+test_expect_success 'setup for git-bisect tests requiring a repo' '
+ git init git-bisect &&
+ (
+ cd git-bisect &&
+ echo "initial contents" >file &&
+ git add file &&
+ git commit -am "Initial commit" &&
+ git tag initial &&
+ echo "new line" >>file &&
+ git commit -am "First change" &&
+ echo "another new line" >>file &&
+ git commit -am "Second change" &&
+ git tag final
+ )
+'
+
+test_expect_success 'git bisect - start subcommand arguments before double-dash are completed as revs' '
+ (
+ cd git-bisect &&
+ test_completion "git bisect start " <<-\EOF
+ HEAD Z
+ final Z
+ initial Z
+ master Z
+ EOF
+ )
+'
+
+# Note that these arguments are <pathspec>s, which in practice the fallback
+# completion (not the git completion) later ends up completing as paths.
+test_expect_success 'git bisect - start subcommand arguments after double-dash are not completed' '
+ (
+ cd git-bisect &&
+ test_completion "git bisect start final initial -- " ""
+ )
+'
+
+test_expect_success 'setup for git-bisect tests requiring ongoing bisection' '
+ (
+ cd git-bisect &&
+ git bisect start --term-new=custom_new --term-old=custom_old final initial
+ )
+'
+
+test_expect_success 'git-bisect - when bisecting all subcommands are candidates' '
+ (
+ cd git-bisect &&
+ test_completion "git bisect " <<-\EOF
+ start Z
+ bad Z
+ new Z
+ good Z
+ old Z
+ skip Z
+ reset Z
+ visualize Z
+ replay Z
+ log Z
+ run Z
+ help Z
+ EOF
+ )
+'
+
test_expect_success 'git checkout - completes refs and unique remote branches for DWIM' '
test_completion "git checkout " <<-\EOF
HEAD Z
--
2.43.0
^ permalink raw reply related
* [PATCH v5 1/7] completion: tests: always use 'master' for default initial branch name
From: Britton Leo Kerin @ 2024-02-06 2:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Patrick Steinhardt, Britton Leo Kerin
In-Reply-To: <20240206020930.312164-1-britton.kerin@gmail.com>
The default initial branch name can normally be configured using the
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME environment variable. However,
when testing e.g. <rev> completion it's convenient to know the
exact initial branch name that will be used.
To achieve that without too much trouble it is considered sufficient
to force the default initial branch name to 'master' for all of
t9902-completion.sh.
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
t/t9902-completion.sh | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index aa9a614de3..a5d4e900a2 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -5,6 +5,11 @@
test_description='test bash completion'
+# Override environment and always use master for the default initial branch
+# name for these tests, so that rev completion candidates are as expected.
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
. ./lib-bash.sh
complete ()
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2 5/5] reftable: document reading and writing indices
From: jltobler @ 2024-02-06 1:43 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Toon Claes, Kristoffer Haugsbakk
In-Reply-To: <c3492bbd42b9d2c17764208e814faccf395cb282.1706773842.git.ps@pks.im>
On 24/02/01 08:52AM, Patrick Steinhardt wrote:
> The way the index gets written and read is not trivial at all and
> requires the reader to piece together a bunch of parts to figure out how
> it works. Add some documentation to hopefully make this easier to
> understand for the next reader.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> reftable/reader.c | 27 +++++++++++++++++++++++++++
> reftable/writer.c | 23 +++++++++++++++++++++++
> 2 files changed, 50 insertions(+)
>
> diff --git a/reftable/reader.c b/reftable/reader.c
> index 278f727a3d..6011d0aa04 100644
> --- a/reftable/reader.c
> +++ b/reftable/reader.c
> @@ -508,11 +508,38 @@ static int reader_seek_indexed(struct reftable_reader *r,
> if (err < 0)
> goto done;
>
> + /*
> + * The index may consist of multiple levels, where each level may have
> + * multiple index blocks. We start by doing a linear search in the
> + * highest layer that identifies the relevant index block as well as
> + * the record inside that block that corresponds to our wanted key.
> + */
> err = reader_seek_linear(&index_iter, &want_index);
> if (err < 0)
> goto done;
>
> + /*
> + * Traverse down the levels until we find a non-index entry.
> + */
> while (1) {
> + /*
> + * In case we seek a record that does not exist the index iter
> + * will tell us that the iterator is over. This works because
> + * the last index entry of the current level will contain the
> + * last key it knows about. So in case our seeked key is larger
> + * than the last indexed key we know that it won't exist.
The last block in the highest-level index section should end with the
record key of greatest value. Doesn't that mean the initial linear seek
should be sufficient to stop the iterator from continuing if the wanted
record key is of a greater value?
> + *
> + * There is one subtlety in the layout of the index section
> + * that makes this work as expected: the highest-level index is
> + * at end of the section and will point backwards and thus we
s/end/the end
> + * start reading from the end of the index section, not the
> + * beginning.
> + *
> + * If that wasn't the case and the order was reversed then the
> + * linear seek would seek into the lower levels and traverse
> + * all levels of the index only to find out that the key does
> + * not exist.
> + */
> err = table_iter_next(&index_iter, &index_result);
> table_iter_block_done(&index_iter);
> if (err != 0)
-Justin
^ permalink raw reply
* Re: [PATCH] commit.c: ensure strchrnul() doesn't scan beyond range
From: Kyle Lippincott @ 2024-02-06 1:41 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget; +Cc: git, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1652.git.1707153705840.gitgitgadget@gmail.com>
On Mon, Feb 5, 2024 at 9:23 AM Chandra Pratap via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> commit.c: ensure strchrnul() doesn't scan beyond range
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1652%2FChand-ra%2Fstrchrnul-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1652/Chand-ra/strchrnul-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1652
>
> commit.c | 8 +-------
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index ef679a0b939..a65b8e92e94 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1743,15 +1743,9 @@ const char *find_header_mem(const char *msg, size_t len,
> int key_len = strlen(key);
> const char *line = msg;
>
> - /*
> - * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
> - * given by len. However, current callers are safe because they compute
> - * len by scanning a NUL-terminated block of memory starting at msg.
> - * Nonetheless, it would be better to ensure the function does not look
> - * at msg beyond the len provided by the caller.
> - */
> while (line && line < msg + len) {
> const char *eol = strchrnul(line, '\n');
> + assert(eol - line <= len);
I don't think this is sufficient to address the NEEDSWORK. `assert` is
only active in debug builds, and strchrnul would have already
potentially exceeded the bounds of its memory by the time this check
is happening. We'd need a safe version of strchrnul that took the
maximum length and never exceeded it.
>
> if (line == eol)
> return NULL;
>
> base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
> --
> gitgitgadget
>
^ 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