* [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 09/28] format_trailers_from_commit(): indirectly call trailer_info_get()
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.
Instead of calling trailer_info_get() directly, call parse_trailers()
which already calls trailer_info_get(). This change is a NOP because
format_trailer_info() only looks at the "trailers" string array, not the
trailer_item objects which parse_trailers() populates.
In the next patch, we'll change format_trailer_info() to use the parsed
trailer_item objects instead of the string array.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/trailer.c b/trailer.c
index e92d0154d90..e6665c99cc3 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1140,9 +1140,11 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
const char *msg,
struct strbuf *out)
{
+ LIST_HEAD(trailers);
struct trailer_info info;
- trailer_info_get(opts, msg, &info);
+ parse_trailers(opts, &info, msg, &trailers);
+
/* 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 &&
@@ -1152,6 +1154,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
} else
format_trailer_info(opts, &info, out);
+ free_trailers(&trailers);
trailer_info_release(&info);
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 10/28] format_trailer_info(): use trailer_item objects
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_trailer_info() operate on trailer_item objects, not the raw
string array.
This breaks t4205 and t6300. We will continue to make improvements until
the test suite passes again, ultimately renaming format_trailer_info()
to format_trailers(), at which point the unification of these formatters
will be complete.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/trailer.c b/trailer.c
index e6665c99cc3..6333dfe1c11 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1086,21 +1086,21 @@ void trailer_info_release(struct trailer_info *info)
}
static void format_trailer_info(const struct process_trailer_options *opts,
- const struct trailer_info *info,
+ struct list_head *trailers,
struct strbuf *out)
{
size_t origlen = out->len;
- size_t i;
-
- for (i = 0; i < info->trailer_nr; i++) {
- char *trailer = info->trailers[i];
- ssize_t separator_pos = find_separator(trailer, separators);
+ struct list_head *pos;
+ struct trailer_item *item;
- if (separator_pos >= 1) {
+ list_for_each(pos, trailers) {
+ item = list_entry(pos, struct trailer_item, list);
+ if (item->token) {
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
+ strbuf_addstr(&tok, item->token);
+ strbuf_addstr(&val, item->value);
- parse_trailer(&tok, &val, NULL, trailer, separator_pos);
if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
if (opts->unfold)
unfold_value(&val);
@@ -1127,13 +1127,12 @@ static void format_trailer_info(const struct process_trailer_options *opts,
if (opts->separator && out->len != origlen) {
strbuf_addbuf(out, opts->separator);
}
- strbuf_addstr(out, trailer);
+ strbuf_addstr(out, item->value);
if (opts->separator) {
strbuf_rtrim(out);
}
}
}
-
}
void format_trailers_from_commit(const struct process_trailer_options *opts,
@@ -1152,7 +1151,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
strbuf_add(out, msg + info.trailer_block_start,
info.trailer_block_end - info.trailer_block_start);
} else
- format_trailer_info(opts, &info, out);
+ format_trailer_info(opts, &trailers, out);
free_trailers(&trailers);
trailer_info_release(&info);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 11/28] format_trailer_info(): drop redundant unfold_value()
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.
In the last patch we made format_trailer_info() use trailer_item objects
instead of the "trailers" string array. This means that the call to
unfold_value() here is redundant because the trailer_item objects are
already unfolded in parse_trailers() which is a dependency of our
caller, format_trailers_from_commit().
Remove the redundant call.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/trailer.c b/trailer.c
index 6333dfe1c11..12cae5b73d2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1102,9 +1102,6 @@ static void format_trailer_info(const struct process_trailer_options *opts,
strbuf_addstr(&val, item->value);
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)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 12/28] format_trailer_info(): append newline for non-trailer lines
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 wraps up the preparatory refactors to unify the trailer formatters.
Two patches ago we made format_trailer_info() use trailer_item objects
instead of the "trailers" string array. The strings in the array
include trailing newlines, because the string array is split up with
trailer_lines = strbuf_split_buf(str + trailer_block_start,
end_of_log_message - trailer_block_start,
'\n',
0);
in trailer_info_get() and strbuf_split_buf() includes the terminator (in
this case the newline character '\n') for each split-up substring.
And before we made the transition to use trailer_item objects for it,
format_trailer_info() called parse_trailer() (which trims newlines) for
trailer lines but did _not_ call parse_trailer() for non-trailer lines.
So for trailer lines it had to add back the trimmed newline like this
if (!opts->separator)
strbuf_addch(out, '\n');
But for non-trailer lines it didn't have to add back the newline because
it could just reuse same string in the "trailers" string array (which
again, already included the trailing newline).
Now that format_trailer_info() uses trailer_item objects for all cases,
it can't rely on "trailers" string array anymore. And so it must be
taught to add a newline back when printing non-trailer lines, just like
it already does for trailer lines. Do so now.
The test suite passes again, so format_trailer_info() is in better shape
supersede format_trailers(), which we'll do in the next patch.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/trailer.c b/trailer.c
index 12cae5b73d2..0774a544c4f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1125,9 +1125,10 @@ static void format_trailer_info(const struct process_trailer_options *opts,
strbuf_addbuf(out, opts->separator);
}
strbuf_addstr(out, item->value);
- if (opts->separator) {
+ if (opts->separator)
strbuf_rtrim(out);
- }
+ else
+ strbuf_addch(out, '\n');
}
}
}
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 13/28] trailer: begin 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>
Now that the preparatory refactors are over, we can replace the call to
format_trailers() in interpret-trailers with format_trailer_info(). This
unifies the trailer formatting machinery, but at the cost of breaking
tests.
More specifically, this patch breaks t7502 and t7513, but only because
we haven't fully stolen the features present in format_trailers() (which
knows about opts->trim_empty) and print_tok_val() (which has
non-hardcoded printing of the separator and space).
We will teach format_trailer_info() these features in the next two
patches to make all tests pass again.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 2 +-
trailer.c | 12 ++++++------
trailer.h | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 11f4ce9e4a2..f57af0db37b 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -171,7 +171,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
}
/* Print trailer block. */
- format_trailers(opts, &head, &trailer_block);
+ format_trailer_info(opts, &head, &trailer_block);
free_trailers(&head);
fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
strbuf_release(&trailer_block);
diff --git a/trailer.c b/trailer.c
index 0774a544c4f..f4defad3dae 100644
--- a/trailer.c
+++ b/trailer.c
@@ -162,9 +162,9 @@ static void print_tok_val(struct strbuf *out, const char *tok, const char *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,
- struct strbuf *out)
+static void format_trailers(const struct process_trailer_options *opts,
+ struct list_head *trailers,
+ struct strbuf *out)
{
struct list_head *pos;
struct trailer_item *item;
@@ -1085,9 +1085,9 @@ void trailer_info_release(struct trailer_info *info)
free(info->trailers);
}
-static void format_trailer_info(const struct process_trailer_options *opts,
- struct list_head *trailers,
- struct strbuf *out)
+void format_trailer_info(const struct process_trailer_options *opts,
+ struct list_head *trailers,
+ struct strbuf *out)
{
size_t origlen = out->len;
struct list_head *pos;
diff --git a/trailer.h b/trailer.h
index 1d106b6dd40..3c13006a4c1 100644
--- a/trailer.h
+++ b/trailer.h
@@ -101,7 +101,7 @@ void trailer_info_get(const 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 *,
+void format_trailer_info(const struct process_trailer_options *,
struct list_head *trailers,
struct strbuf *out);
void free_trailers(struct list_head *);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 14/28] format_trailer_info(): teach it about opts->trim_empty
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 fixes 4 tests in t7513 to go from
t7513-interpret-trailers.sh (Wstat: 256 (exited 1) Tests: 94 Failed: 55)
Failed tests: 2-5, 8, 14, 24-28, 31-37, 43-62, 66-74
77-80, 82-85
to
t7513-interpret-trailers.sh (Wstat: 256 (exited 1) Tests: 94 Failed: 51)
Failed tests: 2-5, 14, 24-28, 31-32, 36-37, 43-62, 66-74
77-80, 82-85
. The next patch will fix the remaining broken test cases in t7513 and
t7502.
Even though the next patch fixes the vast majority of these test cases,
we have to position that patch after this one to avoid breaking the
build because of the way these patches delete relevant (and obsolete)
code.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/trailer.c b/trailer.c
index f4defad3dae..c28b6c11cc5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -162,20 +162,6 @@ static void print_tok_val(struct strbuf *out, const char *tok, const char *val)
strbuf_addf(out, "%s%c %s\n", tok, separators[0], val);
}
-static void format_trailers(const struct process_trailer_options *opts,
- struct list_head *trailers,
- struct strbuf *out)
-{
- 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(out, 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));
@@ -1101,6 +1087,15 @@ void format_trailer_info(const struct process_trailer_options *opts,
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->separator && out->len != origlen)
strbuf_addbuf(out, opts->separator);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 15/28] format_trailer_info(): avoid double-printing the separator
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>
Do not hardcode the printing of ": " as the separator and space (which
can result in double-printing these characters); instead only
print the separator and space if we cannot find any recognized separator
somewhere in the key (yes, keys may have a trailing separator in it ---
we will eventually fix this design but not now). Do so by copying the
code out of print_tok_val(), and deleting the same function.
The test suite passes again with this change.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 25 +++++--------------------
1 file changed, 5 insertions(+), 20 deletions(-)
diff --git a/trailer.c b/trailer.c
index c28b6c11cc5..5c42a19943a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -144,24 +144,6 @@ static char last_non_space_char(const char *s)
return '\0';
}
-static void print_tok_val(struct strbuf *out, const char *tok, const char *val)
-{
- char c;
-
- if (!tok) {
- strbuf_addf(out, "%s\n", val);
- return;
- }
-
- c = last_non_space_char(tok);
- if (!c)
- return;
- if (strchr(separators, c))
- strbuf_addf(out, "%s%s\n", tok, val);
- else
- strbuf_addf(out, "%s%c %s\n", tok, separators[0], val);
-}
-
static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
{
struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
@@ -1104,8 +1086,11 @@ void format_trailer_info(const struct process_trailer_options *opts,
if (!opts->key_only && !opts->value_only) {
if (opts->key_value_separator)
strbuf_addbuf(out, opts->key_value_separator);
- else
- strbuf_addstr(out, ": ");
+ else {
+ char c = last_non_space_char(tok.buf);
+ if (c && !strchr(separators, c))
+ strbuf_addf(out, "%c ", separators[0]);
+ }
}
if (!opts->key_only)
strbuf_addbuf(out, &val);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 16/28] trailer: finish 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>
Rename format_trailer_info() to format_trailers(). Finally, both
interpret-trailers and format_trailers_from_commit() can call
"format_trailers()"!
Update the comment in <trailer.h> to remove the (now obsolete) caveats
about format_trailers_from_commit().
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 2 +-
trailer.c | 8 ++++----
trailer.h | 15 ++++-----------
3 files changed, 9 insertions(+), 16 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index f57af0db37b..11f4ce9e4a2 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -171,7 +171,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
}
/* Print trailer block. */
- format_trailer_info(opts, &head, &trailer_block);
+ format_trailers(opts, &head, &trailer_block);
free_trailers(&head);
fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
strbuf_release(&trailer_block);
diff --git a/trailer.c b/trailer.c
index 5c42a19943a..4f3318802d1 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1053,9 +1053,9 @@ void trailer_info_release(struct trailer_info *info)
free(info->trailers);
}
-void format_trailer_info(const struct process_trailer_options *opts,
- struct list_head *trailers,
- struct strbuf *out)
+void format_trailers(const struct process_trailer_options *opts,
+ struct list_head *trailers,
+ struct strbuf *out)
{
size_t origlen = out->len;
struct list_head *pos;
@@ -1129,7 +1129,7 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
strbuf_add(out, msg + info.trailer_block_start,
info.trailer_block_end - info.trailer_block_start);
} else
- format_trailer_info(opts, &trailers, out);
+ format_trailers(opts, &trailers, out);
free_trailers(&trailers);
trailer_info_release(&info);
diff --git a/trailer.h b/trailer.h
index 3c13006a4c1..9f42aa75994 100644
--- a/trailer.h
+++ b/trailer.h
@@ -101,23 +101,16 @@ void trailer_info_get(const struct process_trailer_options *,
void trailer_info_release(struct trailer_info *info);
void trailer_config_init(void);
-void format_trailer_info(const struct process_trailer_options *,
+void format_trailers(const struct process_trailer_options *,
struct list_head *trailers,
struct strbuf *out);
void free_trailers(struct list_head *);
/*
- * Format the trailers from the commit msg "msg" into the strbuf "out".
- * Note two caveats about "opts":
- *
- * - this is primarily a helper for pretty.c, and not
- * all of the flags are supported.
- *
- * - this differs from process_trailers slightly in that we always format
- * only the trailer block itself, even if the "only_trailers" option is not
- * set.
+ * Convenience function to format the trailers from the commit msg "msg" into
+ * the strbuf "out". Reuses format_trailers() internally.
*/
-void format_trailers_from_commit(const struct process_trailer_options *opts,
+void format_trailers_from_commit(const struct process_trailer_options *,
const char *msg,
struct strbuf *out);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 17/28] trailer: teach iterator about non-trailer lines
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>
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).
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).
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.
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.
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.
For the existing use of the iterator in builtin/shortlog.c, we don't
have to change the code there because that code does
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 | 12 +++++-------
trailer.h | 8 ++++++++
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/trailer.c b/trailer.c
index 4f3318802d1..2cc4a910411 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1147,17 +1147,15 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
int trailer_iterator_advance(struct trailer_iterator *iter)
{
- while (iter->internal.cur < iter->internal.info.trailer_nr) {
- char *trailer = iter->internal.info.trailers[iter->internal.cur++];
- int separator_pos = find_separator(trailer, separators);
-
- if (separator_pos < 1)
- continue; /* not a real trailer */
+ if (iter->internal.cur < iter->internal.info.trailer_nr) {
+ 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;
diff --git a/trailer.h b/trailer.h
index 9f42aa75994..ebafa3657e4 100644
--- a/trailer.h
+++ b/trailer.h
@@ -125,6 +125,14 @@ 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
+ * either 100% trailer lines, or mixed in with non-trailer lines (in
+ * which case at least 25% must be trailer lines).
+ */
+ const char *raw;
+
struct strbuf key;
struct strbuf val;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 18/28] sequencer: use the trailer iterator
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 patch allows for the removal of "trailer_info_get()" from the
trailer.h API, which will be in the next patch.
Instead of calling "trailer_info_get()", which is a low-level function
in the trailers implementation (trailer.c), call
trailer_iterator_advance(), which was specifically designed for public
consumption in f0939a0eb1 (trailer: add interface for iterating over
commit trailers, 2020-09-27).
Avoiding "trailer_info_get()" means we don't have to worry about options
like "no_divider" (relevant for parsing trailers). We also don't have to
check for things like "info.trailer_start == info.trailer_end" to see
whether there were any trailers (instead we can just check to see
whether the iterator advanced at all).
Note how we have to use "iter.raw" in order to get the same behavior as
before when we iterated over the unparsed string array (trailers[]) in
trailer_info.
Signed-off-by: Linus Arver <linusa@google.com>
---
sequencer.c | 27 ++++++++++++---------------
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 8e199fc8a47..35462a6a9d9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -319,35 +319,32 @@ static const char *get_todo_path(const struct replay_opts *opts)
static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
size_t ignore_footer)
{
- struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
- struct trailer_info info;
- size_t i;
+ struct trailer_iterator iter;
+ size_t i = 0;
int found_sob = 0, found_sob_last = 0;
char saved_char;
- opts.no_divider = 1;
-
if (ignore_footer) {
saved_char = sb->buf[sb->len - ignore_footer];
sb->buf[sb->len - ignore_footer] = '\0';
}
- trailer_info_get(&opts, sb->buf, &info);
+ trailer_iterator_init(&iter, sb->buf);
if (ignore_footer)
sb->buf[sb->len - ignore_footer] = saved_char;
- if (info.trailer_block_start == info.trailer_block_end)
- return 0;
+ while (trailer_iterator_advance(&iter)) {
+ i++;
+ if (sob && !strncmp(iter.raw, sob->buf, sob->len))
+ found_sob = i;
+ }
+ trailer_iterator_release(&iter);
- 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;
- }
+ if (!i)
+ return 0;
- trailer_info_release(&info);
+ found_sob_last = (int)i == found_sob;
if (found_sob_last)
return 3;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 19/28] trailer: make trailer_info struct private
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 13211ae23f (trailer: separate public from internal portion of
trailer_iterator, 2023-09-09) we moved trailer_info behind an anonymous
struct to discourage use by trailer.h API users. However it still left
open the possibility of external use of trailer_info itself. Now that
there are no external users of trailer_info, we can make this struct
private.
Make this struct private by putting its definition inside trailer.c.
This has two benefits:
(1) it makes the surface area of the public facing
interface (trailer.h) smaller, and
(2) external API users are unable to peer inside this struct (because
it is only ever exposed as an opaque pointer).
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
in C++ and is a common pattern in that language (where, for example,
abstract classes only have pointers to concrete classes).
However, the original inspiration to use this idiom does not come from
C++, but instead the book "C Interfaces and Implementations: Techniques
for Creating Reusable Software" [1]. This book recommends opaque
pointers as a good design principle for designing C libraries, using the
term "interface" as the functions defined in *.h (header) files and
"implementation" as the corresponding *.c file which define the
interfaces.
The book says this about opaque pointers:
... clients can manipulate such pointers freely, but they can’t
dereference them; that is, they can’t look at the innards of the
structure pointed to by them. Only the implementation has that
privilege. Opaque pointers hide representation details and help
catch errors.
In our case, "struct trailer_info" is now hidden from clients, and the
ways in which this opaque pointer can be used is limited to the richness
of <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 | 12 +++---
trailer.c | 81 ++++++++++++++++++++++++++++--------
trailer.h | 40 +++++-------------
3 files changed, 80 insertions(+), 53 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 11f4ce9e4a2..6bf8cec005a 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -141,7 +141,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
struct strbuf trailer_block = STRBUF_INIT;
- struct trailer_info info;
+ struct trailer_info *info;
FILE *outfile = stdout;
trailer_config_init();
@@ -151,13 +151,13 @@ static void interpret_trailers(const struct process_trailer_options *opts,
if (opts->in_place)
outfile = create_in_place_tempfile(file);
- parse_trailers(opts, &info, sb.buf, &head);
+ info = parse_trailers(opts, sb.buf, &head);
/* Print the lines before the trailers */
if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+ fwrite(sb.buf, 1, trailer_block_start(info), outfile);
- if (!opts->only_trailers && !info.blank_line_before_trailer)
+ if (!opts->only_trailers && !blank_line_before_trailer_block(info))
fprintf(outfile, "\n");
@@ -178,8 +178,8 @@ static void interpret_trailers(const struct process_trailer_options *opts,
/* 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)
if (rename_tempfile(&trailers_tempfile, file))
diff --git a/trailer.c b/trailer.c
index 2cc4a910411..cc211dfeeae 100644
--- a/trailer.c
+++ b/trailer.c
@@ -11,6 +11,27 @@
* Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
*/
+struct trailer_info {
+ /*
+ * True if there is a blank line before the location pointed to by
+ * trailer_block_start.
+ */
+ int blank_line_before_trailer;
+
+ /*
+ * Offsets to the trailer block start and end positions in the input
+ * string. If no trailer block is found, these are both set to the
+ * "true" end of the input (find_end_of_log_message()).
+ */
+ size_t trailer_block_start, trailer_block_end;
+
+ /*
+ * Array of trailers found.
+ */
+ char **trailers;
+ size_t trailer_nr;
+};
+
struct conf_info {
char *name;
char *key;
@@ -953,20 +974,26 @@ static void unfold_value(struct strbuf *val)
strbuf_release(&out);
}
+static struct trailer_info *trailer_info_new(void)
+{
+ struct trailer_info *info = xcalloc(1, sizeof(*info));
+ return info;
+}
+
/*
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
*/
-void parse_trailers(const struct process_trailer_options *opts,
- struct trailer_info *info,
- const char *str,
- struct list_head *head)
+struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
+ const char *str,
+ struct list_head *head)
{
+ struct trailer_info *info;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
size_t i;
- trailer_info_get(opts, str, info);
+ info = trailer_info_get(opts, str);
for (i = 0; i < info->trailer_nr; i++) {
int separator_pos;
@@ -990,6 +1017,8 @@ void parse_trailers(const struct process_trailer_options *opts,
strbuf_detach(&val, NULL));
}
}
+
+ return info;
}
void free_trailers(struct list_head *trailers)
@@ -1001,10 +1030,25 @@ void free_trailers(struct list_head *trailers)
}
}
-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)
+{
+ return info->trailer_block_start;
+}
+
+size_t trailer_block_end(struct trailer_info *info)
+{
+ return info->trailer_block_end;
+}
+
+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;
@@ -1043,6 +1087,8 @@ 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)
@@ -1051,6 +1097,7 @@ void trailer_info_release(struct trailer_info *info)
for (i = 0; i < info->trailer_nr; i++)
free(info->trailers[i]);
free(info->trailers);
+ free(info);
}
void format_trailers(const struct process_trailer_options *opts,
@@ -1118,21 +1165,19 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
struct strbuf *out)
{
LIST_HEAD(trailers);
- struct trailer_info info;
-
- 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 &&
!opts->separator && !opts->key_only && !opts->value_only &&
!opts->key_value_separator) {
- strbuf_add(out, msg + info.trailer_block_start,
- info.trailer_block_end - info.trailer_block_start);
+ strbuf_add(out, msg + info->trailer_block_start,
+ info->trailer_block_end - info->trailer_block_start);
} else
format_trailers(opts, &trailers, out);
free_trailers(&trailers);
- trailer_info_release(&info);
+ trailer_info_release(info);
}
void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
@@ -1141,14 +1186,14 @@ 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(&opts, msg, &iter->internal.info);
+ iter->internal.info = trailer_info_get(&opts, msg);
iter->internal.cur = 0;
}
int trailer_iterator_advance(struct trailer_iterator *iter)
{
- if (iter->internal.cur < iter->internal.info.trailer_nr) {
- char *line = iter->internal.info.trailers[iter->internal.cur++];
+ if (iter->internal.cur < iter->internal.info->trailer_nr) {
+ char *line = iter->internal.info->trailers[iter->internal.cur++];
int separator_pos = find_separator(line, separators);
iter->raw = line;
@@ -1165,7 +1210,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
void trailer_iterator_release(struct trailer_iterator *iter)
{
- trailer_info_release(&iter->internal.info);
+ trailer_info_release(iter->internal.info);
strbuf_release(&iter->val);
strbuf_release(&iter->key);
}
diff --git a/trailer.h b/trailer.h
index ebafa3657e4..a63e97a2663 100644
--- a/trailer.h
+++ b/trailer.h
@@ -4,6 +4,8 @@
#include "list.h"
#include "strbuf.h"
+struct trailer_info;
+
enum trailer_where {
WHERE_DEFAULT,
WHERE_END,
@@ -29,27 +31,6 @@ int trailer_set_where(enum trailer_where *item, const char *value);
int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
-struct trailer_info {
- /*
- * True if there is a blank line before the location pointed to by
- * trailer_block_start.
- */
- int blank_line_before_trailer;
-
- /*
- * Offsets to the trailer block start and end positions in the input
- * string. If no trailer block is found, these are both set to the
- * "true" end of the input (find_end_of_log_message()).
- */
- size_t trailer_block_start, trailer_block_end;
-
- /*
- * Array of trailers found.
- */
- char **trailers;
- size_t trailer_nr;
-};
-
/*
* A list that represents newly-added trailers, such as those provided
* with the --trailer command line option of git-interpret-trailers.
@@ -89,14 +70,15 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
void process_trailers_lists(struct list_head *head,
struct list_head *arg_head);
-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 *,
+ const char *str,
+ struct list_head *head);
+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);
@@ -138,7 +120,7 @@ struct trailer_iterator {
/* private */
struct {
- struct trailer_info info;
+ struct trailer_info *info;
size_t cur;
} internal;
};
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 20/28] trailer: retire trailer_info_get() from 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>
Make it "static" to be file-scoped to trailer.c, because no one outside
of trailer.c uses it. Remove its declaration from <trailer.h>.
We have to also reposition it to be above parse_trailers(), which
depends on it.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 92 +++++++++++++++++++++++++++----------------------------
trailer.h | 2 --
2 files changed, 46 insertions(+), 48 deletions(-)
diff --git a/trailer.c b/trailer.c
index cc211dfeeae..36e49ab7cf5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -980,6 +980,52 @@ static struct trailer_info *trailer_info_new(void)
return info;
}
+static struct trailer_info *trailer_info_get(const struct process_trailer_options *opts,
+ const char *str)
+{
+ struct trailer_info *info = trailer_info_new();
+ size_t end_of_log_message = 0, trailer_block_start = 0;
+ struct strbuf **trailer_lines, **ptr;
+ char **trailer_strings = NULL;
+ size_t nr = 0, alloc = 0;
+ char **last = NULL;
+
+ trailer_config_init();
+
+ end_of_log_message = find_end_of_log_message(str, opts->no_divider);
+ trailer_block_start = find_trailer_block_start(str, end_of_log_message);
+
+ trailer_lines = strbuf_split_buf(str + trailer_block_start,
+ end_of_log_message - trailer_block_start,
+ '\n',
+ 0);
+ for (ptr = trailer_lines; *ptr; ptr++) {
+ if (last && isspace((*ptr)->buf[0])) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
+ strbuf_addbuf(&sb, *ptr);
+ *last = strbuf_detach(&sb, NULL);
+ continue;
+ }
+ ALLOC_GROW(trailer_strings, nr + 1, alloc);
+ trailer_strings[nr] = strbuf_detach(*ptr, NULL);
+ last = find_separator(trailer_strings[nr], separators) >= 1
+ ? &trailer_strings[nr]
+ : NULL;
+ nr++;
+ }
+ strbuf_list_free(trailer_lines);
+
+ info->blank_line_before_trailer = ends_with_blank_line(str,
+ trailer_block_start);
+ info->trailer_block_start = trailer_block_start;
+ info->trailer_block_end = end_of_log_message;
+ info->trailers = trailer_strings;
+ info->trailer_nr = nr;
+
+ return info;
+}
+
/*
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
@@ -1045,52 +1091,6 @@ 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;
- 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;
-}
-
void trailer_info_release(struct trailer_info *info)
{
size_t i;
diff --git a/trailer.h b/trailer.h
index a63e97a2663..1b7422fa2b0 100644
--- a/trailer.h
+++ b/trailer.h
@@ -73,8 +73,6 @@ void process_trailers_lists(struct list_head *head,
struct trailer_info *parse_trailers(const struct process_trailer_options *,
const char *str,
struct list_head *head);
-struct trailer_info *trailer_info_get(const struct process_trailer_options *,
- const char *str);
size_t trailer_block_start(struct trailer_info *);
size_t trailer_block_end(struct trailer_info *);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 21/28] trailer: spread usage of "trailer_block" language
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>
Deprecate the "trailer_info" struct name and replace it with
"trailer_block". The main reason is to help readability, because
"trailer_info" on the surface sounds like it's about a single trailer
when in reality it is a collection of contiguous lines, at least 25% of
which are trailers.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 25 +++++-----
trailer.c | 97 ++++++++++++++++++------------------
trailer.h | 18 +++----
3 files changed, 71 insertions(+), 69 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 6bf8cec005a..f76841c5280 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -140,8 +140,8 @@ static void interpret_trailers(const struct process_trailer_options *opts,
{
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
- struct strbuf trailer_block = STRBUF_INIT;
- struct trailer_info *info;
+ struct strbuf tb = STRBUF_INIT;
+ struct trailer_block *trailer_block;
FILE *outfile = stdout;
trailer_config_init();
@@ -151,13 +151,13 @@ static void interpret_trailers(const struct process_trailer_options *opts,
if (opts->in_place)
outfile = create_in_place_tempfile(file);
- info = parse_trailers(opts, sb.buf, &head);
+ trailer_block = parse_trailers(opts, sb.buf, &head);
- /* Print the lines before the trailers */
+ /* Print the lines before the trailer block */
if (!opts->only_trailers)
- fwrite(sb.buf, 1, trailer_block_start(info), outfile);
+ fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
- if (!opts->only_trailers && !blank_line_before_trailer_block(info))
+ if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
fprintf(outfile, "\n");
@@ -171,15 +171,16 @@ static void interpret_trailers(const struct process_trailer_options *opts,
}
/* 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);
+ fwrite(tb.buf, 1, tb.len, outfile);
+ strbuf_release(&tb);
- /* Print the lines after the trailers as is */
+ /* Print the lines after the trailer block as is. */
if (!opts->only_trailers)
- fwrite(sb.buf + trailer_block_end(info), 1, sb.len - trailer_block_end(info), outfile);
- trailer_info_release(info);
+ fwrite(sb.buf + trailer_block_end(trailer_block), 1,
+ sb.len - trailer_block_end(trailer_block), outfile);
+ trailer_block_release(trailer_block);
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
diff --git a/trailer.c b/trailer.c
index 36e49ab7cf5..49bf26e3211 100644
--- a/trailer.c
+++ b/trailer.c
@@ -11,19 +11,20 @@
* Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
*/
-struct trailer_info {
+struct trailer_block {
/*
* True if there is a blank line before the location pointed to by
- * trailer_block_start.
+ * "start".
*/
int blank_line_before_trailer;
/*
- * Offsets to the trailer block start and end positions in the input
- * string. If no trailer block is found, these are both set to the
- * "true" end of the input (find_end_of_log_message()).
+ * The locations of the start and end positions of the trailer block
+ * found, as offsets from the beginning of the source text from which
+ * this trailer block was parsed. If no trailer block is found, these
+ * are both set to 0.
*/
- size_t trailer_block_start, trailer_block_end;
+ size_t start, end;
/*
* Array of trailers found.
@@ -974,16 +975,16 @@ static void unfold_value(struct strbuf *val)
strbuf_release(&out);
}
-static struct trailer_info *trailer_info_new(void)
+static struct trailer_block *trailer_block_new(void)
{
- struct trailer_info *info = xcalloc(1, sizeof(*info));
- return info;
+ struct trailer_block *trailer_block = xcalloc(1, sizeof(*trailer_block));
+ return trailer_block;
}
-static struct trailer_info *trailer_info_get(const struct process_trailer_options *opts,
- const char *str)
+static struct trailer_block *trailer_block_get(const struct process_trailer_options *opts,
+ const char *str)
{
- struct trailer_info *info = trailer_info_new();
+ struct trailer_block *trailer_block = trailer_block_new();
size_t end_of_log_message = 0, trailer_block_start = 0;
struct strbuf **trailer_lines, **ptr;
char **trailer_strings = NULL;
@@ -1016,34 +1017,34 @@ static struct trailer_info *trailer_info_get(const struct process_trailer_option
}
strbuf_list_free(trailer_lines);
- info->blank_line_before_trailer = ends_with_blank_line(str,
- trailer_block_start);
- info->trailer_block_start = trailer_block_start;
- info->trailer_block_end = end_of_log_message;
- info->trailers = trailer_strings;
- info->trailer_nr = nr;
+ trailer_block->blank_line_before_trailer = ends_with_blank_line(str,
+ trailer_block_start);
+ trailer_block->start = trailer_block_start;
+ trailer_block->end = end_of_log_message;
+ trailer_block->trailers = trailer_strings;
+ trailer_block->trailer_nr = nr;
- return info;
+ return trailer_block;
}
/*
- * Parse trailers in "str", populating the trailer info and "head"
- * linked list structure.
+ * Parse trailers in "str", populating the trailer_block info and "head" linked
+ * list structure.
*/
-struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
- const char *str,
- struct list_head *head)
+struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
+ const char *str,
+ struct list_head *head)
{
- struct trailer_info *info;
+ struct trailer_block *trailer_block;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
size_t i;
- info = trailer_info_get(opts, str);
+ trailer_block = trailer_block_get(opts, str);
- for (i = 0; i < info->trailer_nr; i++) {
+ for (i = 0; i < trailer_block->trailer_nr; i++) {
int separator_pos;
- char *trailer = info->trailers[i];
+ char *trailer = trailer_block->trailers[i];
if (trailer[0] == comment_line_char)
continue;
separator_pos = find_separator(trailer, separators);
@@ -1064,7 +1065,7 @@ struct trailer_info *parse_trailers(const struct process_trailer_options *opts,
}
}
- return info;
+ return trailer_block;
}
void free_trailers(struct list_head *trailers)
@@ -1076,28 +1077,28 @@ void free_trailers(struct list_head *trailers)
}
}
-size_t trailer_block_start(struct trailer_info *info)
+size_t trailer_block_start(struct trailer_block *trailer_block)
{
- return info->trailer_block_start;
+ return trailer_block->start;
}
-size_t trailer_block_end(struct trailer_info *info)
+size_t trailer_block_end(struct trailer_block *trailer_block)
{
- return info->trailer_block_end;
+ return trailer_block->end;
}
-int blank_line_before_trailer_block(struct trailer_info *info)
+int blank_line_before_trailer_block(struct trailer_block *trailer_block)
{
- return info->blank_line_before_trailer;
+ return trailer_block->blank_line_before_trailer;
}
-void trailer_info_release(struct trailer_info *info)
+void trailer_block_release(struct trailer_block *trailer_block)
{
size_t i;
- for (i = 0; i < info->trailer_nr; i++)
- free(info->trailers[i]);
- free(info->trailers);
- free(info);
+ for (i = 0; i < trailer_block->trailer_nr; i++)
+ free(trailer_block->trailers[i]);
+ free(trailer_block->trailers);
+ free(trailer_block);
}
void format_trailers(const struct process_trailer_options *opts,
@@ -1165,19 +1166,19 @@ void format_trailers_from_commit(const struct process_trailer_options *opts,
struct strbuf *out)
{
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 &&
!opts->separator && !opts->key_only && !opts->value_only &&
!opts->key_value_separator) {
- strbuf_add(out, msg + info->trailer_block_start,
- info->trailer_block_end - info->trailer_block_start);
+ strbuf_add(out, msg + trailer_block->start,
+ trailer_block->end - trailer_block->start);
} else
format_trailers(opts, &trailers, out);
free_trailers(&trailers);
- trailer_info_release(info);
+ trailer_block_release(trailer_block);
}
void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
@@ -1186,14 +1187,14 @@ 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 = trailer_info_get(&opts, msg);
+ iter->internal.trailer_block = trailer_block_get(&opts, msg);
iter->internal.cur = 0;
}
int trailer_iterator_advance(struct trailer_iterator *iter)
{
- if (iter->internal.cur < iter->internal.info->trailer_nr) {
- char *line = iter->internal.info->trailers[iter->internal.cur++];
+ if (iter->internal.cur < iter->internal.trailer_block->trailer_nr) {
+ char *line = iter->internal.trailer_block->trailers[iter->internal.cur++];
int separator_pos = find_separator(line, separators);
iter->raw = line;
@@ -1210,7 +1211,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
void trailer_iterator_release(struct trailer_iterator *iter)
{
- trailer_info_release(iter->internal.info);
+ trailer_block_release(iter->internal.trailer_block);
strbuf_release(&iter->val);
strbuf_release(&iter->key);
}
diff --git a/trailer.h b/trailer.h
index 1b7422fa2b0..76e6d941a07 100644
--- a/trailer.h
+++ b/trailer.h
@@ -4,7 +4,7 @@
#include "list.h"
#include "strbuf.h"
-struct trailer_info;
+struct trailer_block;
enum trailer_where {
WHERE_DEFAULT,
@@ -70,15 +70,15 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
void process_trailers_lists(struct list_head *head,
struct list_head *arg_head);
-struct trailer_info *parse_trailers(const struct process_trailer_options *,
- const char *str,
- struct list_head *head);
+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 *);
-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 *);
void trailer_config_init(void);
void format_trailers(const struct process_trailer_options *,
@@ -118,7 +118,7 @@ struct trailer_iterator {
/* private */
struct {
- struct trailer_info *info;
+ struct trailer_block *trailer_block;
size_t cur;
} internal;
};
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 22/28] trailer: prepare to delete "parse_trailers_from_command_line_args()"
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>
Expose more functions in the trailer.h API, in preparation for deleting
parse_trailers_from_command_line_args()
from the trailers implementation, because it should not be concerned
with command line arguments (as they have nothing to do with trailers
themselves). Indeed, the interpret-trailers builtin is the only caller
of this function inside Git.
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>
---
builtin/interpret-trailers.c | 6 +--
trailer.c | 86 ++++++++++++++++++------------------
trailer.h | 13 ++++++
3 files changed, 58 insertions(+), 47 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index f76841c5280..f674b5f4b9e 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -45,7 +45,7 @@ static int option_parse_if_missing(const struct option *opt,
return trailer_set_if_missing(opt->value, arg);
}
-static void new_trailers_clear(struct list_head *trailers)
+static void free_new_trailers(struct list_head *trailers)
{
struct list_head *pos, *tmp;
struct new_trailer_item *item;
@@ -64,7 +64,7 @@ static int option_parse_trailer(const struct option *opt,
struct new_trailer_item *item;
if (unset) {
- new_trailers_clear(trailers);
+ free_new_trailers(trailers);
return 0;
}
@@ -237,7 +237,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
interpret_trailers(&opts, &trailers, NULL);
}
- new_trailers_clear(&trailers);
+ free_new_trailers(&trailers);
return 0;
}
diff --git a/trailer.c b/trailer.c
index 49bf26e3211..3b8f0ba103c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -33,7 +33,7 @@ struct trailer_block {
size_t trailer_nr;
};
-struct conf_info {
+struct trailer_conf {
char *name;
char *key;
char *command;
@@ -43,7 +43,7 @@ struct conf_info {
enum trailer_if_missing if_missing;
};
-static struct conf_info default_conf_info;
+static struct trailer_conf default_trailer_conf;
struct trailer_item {
struct list_head list;
@@ -59,7 +59,7 @@ struct arg_item {
struct list_head list;
char *token;
char *value;
- struct conf_info conf;
+ struct trailer_conf conf;
};
static LIST_HEAD(conf_head);
@@ -210,7 +210,7 @@ static int check_if_different(struct trailer_item *in_tok,
return 1;
}
-static char *apply_command(struct conf_info *conf, const char *arg)
+static char *apply_command(struct trailer_conf *conf, const char *arg)
{
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
@@ -424,7 +424,8 @@ int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
return 0;
}
-static void duplicate_conf(struct conf_info *dst, const struct conf_info *src)
+void duplicate_trailer_conf(struct trailer_conf *dst,
+ const struct trailer_conf *src)
{
*dst = *src;
dst->name = xstrdup_or_null(src->name);
@@ -447,7 +448,7 @@ static struct arg_item *get_conf_item(const char *name)
/* Item does not already exists, create it */
CALLOC_ARRAY(item, 1);
- duplicate_conf(&item->conf, &default_conf_info);
+ duplicate_trailer_conf(&item->conf, &default_trailer_conf);
item->conf.name = xstrdup(name);
list_add_tail(&item->list, &conf_head);
@@ -482,17 +483,17 @@ static int git_trailer_default_config(const char *conf_key, const char *value,
variable_name = strrchr(trailer_item, '.');
if (!variable_name) {
if (!strcmp(trailer_item, "where")) {
- if (trailer_set_where(&default_conf_info.where,
+ if (trailer_set_where(&default_trailer_conf.where,
value) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifexists")) {
- if (trailer_set_if_exists(&default_conf_info.if_exists,
+ if (trailer_set_if_exists(&default_trailer_conf.if_exists,
value) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifmissing")) {
- if (trailer_set_if_missing(&default_conf_info.if_missing,
+ if (trailer_set_if_missing(&default_trailer_conf.if_missing,
value) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
@@ -511,7 +512,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
{
const char *trailer_item, *variable_name;
struct arg_item *item;
- struct conf_info *conf;
+ struct trailer_conf *conf;
char *name = NULL;
enum trailer_info_type type;
int i;
@@ -585,9 +586,9 @@ void trailer_config_init(void)
return;
/* Default config must be setup first */
- default_conf_info.where = WHERE_END;
- default_conf_info.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
- default_conf_info.if_missing = MISSING_ADD;
+ default_trailer_conf.where = WHERE_END;
+ default_trailer_conf.if_exists = EXISTS_ADD_IF_DIFFERENT_NEIGHBOR;
+ default_trailer_conf.if_missing = MISSING_ADD;
git_config(git_trailer_default_config, NULL);
git_config(git_trailer_config, NULL);
configured = 1;
@@ -620,7 +621,7 @@ static int token_matches_item(const char *tok, struct arg_item *item, size_t tok
* distinguished from the non-well-formed-line case (in which this function
* returns -1) because some callers of this function need such a distinction.
*/
-static ssize_t find_separator(const char *line, const char *separators)
+ssize_t find_separator(const char *line, const char *separators)
{
int whitespace_found = 0;
const char *c;
@@ -645,28 +646,28 @@ static ssize_t find_separator(const char *line, const char *separators)
*
* If separator_pos is -1, interpret the whole trailer as a token.
*/
-static void parse_trailer(struct strbuf *tok, struct strbuf *val,
- const struct conf_info **conf, const char *trailer,
- ssize_t separator_pos)
+void parse_trailer(const char *line, ssize_t separator_pos,
+ struct strbuf *tok, struct strbuf *val,
+ const struct trailer_conf **conf)
{
struct arg_item *item;
size_t tok_len;
struct list_head *pos;
if (separator_pos != -1) {
- strbuf_add(tok, trailer, separator_pos);
+ strbuf_add(tok, line, separator_pos);
strbuf_trim(tok);
- strbuf_addstr(val, trailer + separator_pos + 1);
+ strbuf_addstr(val, line + separator_pos + 1);
strbuf_trim(val);
} else {
- strbuf_addstr(tok, trailer);
+ strbuf_addstr(tok, line);
strbuf_trim(tok);
}
/* Lookup if the token matches something in the config */
tok_len = token_len_without_separator(tok->buf, tok->len);
if (conf)
- *conf = &default_conf_info;
+ *conf = &default_trailer_conf;
list_for_each(pos, &conf_head) {
item = list_entry(pos, struct arg_item, list);
if (token_matches_item(tok->buf, item, tok_len)) {
@@ -690,14 +691,14 @@ static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
return new_item;
}
-static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
- const struct conf_info *conf,
- const struct new_trailer_item *new_trailer_item)
+void trailer_add_arg_item(struct list_head *arg_head, char *tok, char *val,
+ const struct trailer_conf *conf,
+ const struct new_trailer_item *new_trailer_item)
{
struct arg_item *new_item = xcalloc(1, sizeof(*new_item));
new_item->token = tok;
new_item->value = val;
- duplicate_conf(&new_item->conf, conf);
+ duplicate_trailer_conf(&new_item->conf, conf);
if (new_trailer_item) {
if (new_trailer_item->where != WHERE_DEFAULT)
new_item->conf.where = new_trailer_item->where;
@@ -718,10 +719,10 @@ void parse_trailers_from_config(struct list_head *config_head)
list_for_each(pos, &conf_head) {
item = list_entry(pos, struct arg_item, list);
if (item->conf.command)
- add_arg_item(config_head,
- xstrdup(token_from_item(item, NULL)),
- xstrdup(""),
- &item->conf, NULL);
+ trailer_add_arg_item(config_head,
+ xstrdup(token_from_item(item, NULL)),
+ xstrdup(""),
+ &item->conf, NULL);
}
}
@@ -730,7 +731,7 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
- const struct conf_info *conf;
+ const struct trailer_conf *conf;
struct list_head *pos;
/*
@@ -753,12 +754,11 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
(int) sb.len, sb.buf);
strbuf_release(&sb);
} else {
- parse_trailer(&tok, &val, &conf, tr->text,
- separator_pos);
- add_arg_item(arg_head,
- strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf, tr);
+ parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
+ trailer_add_arg_item(arg_head,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf, tr);
}
}
@@ -1044,20 +1044,19 @@ struct trailer_block *parse_trailers(const struct process_trailer_options *opts,
for (i = 0; i < trailer_block->trailer_nr; i++) {
int separator_pos;
- char *trailer = trailer_block->trailers[i];
- if (trailer[0] == comment_line_char)
+ char *line = trailer_block->trailers[i];
+ if (line[0] == comment_line_char)
continue;
- separator_pos = find_separator(trailer, separators);
+ separator_pos = find_separator(line, separators);
if (separator_pos >= 1) {
- parse_trailer(&tok, &val, NULL, trailer,
- separator_pos);
+ parse_trailer(line, separator_pos, &tok, &val, NULL);
if (opts->unfold)
unfold_value(&val);
add_trailer_item(head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL));
} else if (!opts->only_trailers) {
- strbuf_addstr(&val, trailer);
+ strbuf_addstr(&val, line);
strbuf_strip_suffix(&val, "\n");
add_trailer_item(head,
NULL,
@@ -1200,8 +1199,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
iter->raw = line;
strbuf_reset(&iter->key);
strbuf_reset(&iter->val);
- parse_trailer(&iter->key, &iter->val, NULL,
- line, separator_pos);
+ parse_trailer(line, separator_pos, &iter->key, &iter->val, NULL);
/* Always unfold values during iteration. */
unfold_value(&iter->val);
return 1;
diff --git a/trailer.h b/trailer.h
index 76e6d941a07..f80f8f7e63f 100644
--- a/trailer.h
+++ b/trailer.h
@@ -5,6 +5,7 @@
#include "strbuf.h"
struct trailer_block;
+struct trailer_conf;
enum trailer_where {
WHERE_DEFAULT,
@@ -45,6 +46,12 @@ struct new_trailer_item {
enum trailer_if_missing if_missing;
};
+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;
int trim_empty;
@@ -70,6 +77,12 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
void process_trailers_lists(struct list_head *head,
struct list_head *arg_head);
+ssize_t find_separator(const char *line, const char *separators);
+
+void parse_trailer(const char *line, ssize_t separator_pos,
+ struct strbuf *tok, struct strbuf *val,
+ const struct trailer_conf **);
+
struct trailer_block *parse_trailers(const struct process_trailer_options *,
const char *str,
struct list_head *head);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 23/28] trailer: add new helper functions to 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>
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 | 12 ---------
trailer.c | 49 ++++++++++++++++++++++++++++++++++++
trailer.h | 8 ++++++
3 files changed, 57 insertions(+), 12 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index f674b5f4b9e..9169c320921 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -45,18 +45,6 @@ static int option_parse_if_missing(const struct option *opt,
return trailer_set_if_missing(opt->value, arg);
}
-static void free_new_trailers(struct list_head *trailers)
-{
- struct list_head *pos, *tmp;
- struct new_trailer_item *item;
-
- list_for_each_safe(pos, tmp, trailers) {
- item = list_entry(pos, struct new_trailer_item, list);
- list_del(pos);
- free(item);
- }
-}
-
static int option_parse_trailer(const struct option *opt,
const char *arg, int unset)
{
diff --git a/trailer.c b/trailer.c
index 3b8f0ba103c..9b8cb94c021 100644
--- a/trailer.c
+++ b/trailer.c
@@ -66,6 +66,11 @@ static LIST_HEAD(conf_head);
static char *separators = ":";
+const char *trailer_default_separators(void)
+{
+ return separators;
+}
+
static int configured;
#define TRAILER_ARG_STRING "$ARG"
@@ -424,6 +429,29 @@ int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
return 0;
}
+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)
+{
+ conf->if_missing = if_missing;
+}
+
+struct trailer_conf *new_trailer_conf(void)
+{
+ return xcalloc(1, sizeof(struct trailer_conf));
+}
+
void duplicate_trailer_conf(struct trailer_conf *dst,
const struct trailer_conf *src)
{
@@ -434,6 +462,15 @@ void duplicate_trailer_conf(struct trailer_conf *dst,
dst->cmd = xstrdup_or_null(src->cmd);
}
+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 list_head *pos;
@@ -1076,6 +1113,18 @@ void free_trailers(struct list_head *trailers)
}
}
+void free_new_trailers(struct list_head *trailers)
+{
+ struct list_head *pos, *tmp;
+ struct new_trailer_item *item;
+
+ list_for_each_safe(pos, tmp, trailers) {
+ item = list_entry(pos, struct new_trailer_item, list);
+ list_del(pos);
+ free(item);
+ }
+}
+
size_t trailer_block_start(struct trailer_block *trailer_block)
{
return trailer_block->start;
diff --git a/trailer.h b/trailer.h
index f80f8f7e63f..a2569c10451 100644
--- a/trailer.h
+++ b/trailer.h
@@ -46,8 +46,14 @@ struct new_trailer_item {
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 *);
+
+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(struct list_head *arg_head, char *tok, char *val,
const struct trailer_conf *conf,
const struct new_trailer_item *new_trailer_item);
@@ -98,6 +104,8 @@ 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
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 24/28] trailer_add_arg_item(): drop new_trailer_item 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>
This is a preparatory refactor for deprecating "new_trailer_item" from
the API.
Instead of preserving the where/if_exists/if_missing information inside
the new_trailer_item struct in parse_trailers_from_command_line_args()
(only to look inside it again later on in trailer_add_arg_item()), pass
this information directly as a trailer_conf which trailer_add_arg_item()
already knows how to handle. This reduces the number of parameters we
have to pass to trailer_add_arg_item() without any behavioral change.
In the next patch we'll be able to delete "new_trailer_item" altogether.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 29 +++++++++++++++++------------
trailer.h | 3 +--
2 files changed, 18 insertions(+), 14 deletions(-)
diff --git a/trailer.c b/trailer.c
index 9b8cb94c021..6ab5cf7e5d7 100644
--- a/trailer.c
+++ b/trailer.c
@@ -729,21 +729,12 @@ static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
}
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)
+ const struct trailer_conf *conf)
{
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);
}
@@ -759,7 +750,7 @@ void parse_trailers_from_config(struct list_head *config_head)
trailer_add_arg_item(config_head,
xstrdup(token_from_item(item, NULL)),
xstrdup(""),
- &item->conf, NULL);
+ &item->conf);
}
}
@@ -791,11 +782,25 @@ void parse_trailers_from_command_line_args(struct list_head *arg_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);
+ 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, tr);
+ conf_current);
+ free_trailer_conf(conf_current);
}
}
diff --git a/trailer.h b/trailer.h
index a2569c10451..32fc93beb33 100644
--- a/trailer.h
+++ b/trailer.h
@@ -55,8 +55,7 @@ void duplicate_trailer_conf(struct trailer_conf *dst,
const struct trailer_conf *src);
const char *trailer_default_separators(void);
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);
+ const struct trailer_conf *conf);
struct process_trailer_options {
int in_place;
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 25/28] trailer: deprecate "new_trailer_item" struct from 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>
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.
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 | 29 +++++--------
trailer.c | 81 +++++++++++++++++-------------------
trailer.h | 21 +++-------
3 files changed, 54 insertions(+), 77 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 9169c320921..943be5b360e 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -49,7 +49,6 @@ 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);
@@ -59,12 +58,8 @@ 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;
}
@@ -132,8 +127,6 @@ static void interpret_trailers(const struct process_trailer_options *opts,
struct trailer_block *trailer_block;
FILE *outfile = stdout;
- trailer_config_init();
-
read_input_file(&sb, file);
if (opts->in_place)
@@ -148,15 +141,8 @@ 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);
@@ -180,6 +166,7 @@ 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[] = {
@@ -205,6 +192,10 @@ 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);
@@ -215,6 +206,8 @@ 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++)
diff --git a/trailer.c b/trailer.c
index 6ab5cf7e5d7..0893175553a 100644
--- a/trailer.c
+++ b/trailer.c
@@ -754,57 +754,54 @@ 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)
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
const struct trailer_conf *conf;
- struct list_head *pos;
/*
* In command-line arguments, '=' is accepted (in addition to the
* separators that are defined).
*/
- char *cl_separators = xstrfmt("=%s", separators);
+ 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);
- 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(tr->text, separator_pos, &tok, &val, &conf);
- 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_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)
@@ -1120,13 +1117,11 @@ void free_trailers(struct list_head *trailers)
void free_new_trailers(struct list_head *trailers)
{
- 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));
}
}
diff --git a/trailer.h b/trailer.h
index 32fc93beb33..2848a0d086c 100644
--- a/trailer.h
+++ b/trailer.h
@@ -32,20 +32,6 @@ int trailer_set_where(enum trailer_where *item, const char *value);
int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
-/*
- * A list that represents newly-added trailers, such as those provided
- * with the --trailer command line option of git-interpret-trailers.
- */
-struct new_trailer_item {
- struct list_head list;
-
- const char *text;
-
- enum trailer_where where;
- enum trailer_if_exists if_exists;
- enum trailer_if_missing if_missing;
-};
-
void trailer_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 *);
@@ -76,8 +62,11 @@ 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);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 26/28] trailer: unify "--trailer ..." arg handling
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>
Move the logic of parse_trailer_from_command_line_arg() into
option_parse_trailer(), because that is the only caller and there's no
benefit in keeping these two separate.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 42 +++++++++++++++++++++++++++++-
trailer.c | 50 ------------------------------------
trailer.h | 6 -----
3 files changed, 41 insertions(+), 57 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 943be5b360e..9657b0d067c 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -49,6 +49,11 @@ static int option_parse_trailer(const struct option *opt,
const char *arg, int unset)
{
struct list_head *trailers = opt->value;
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ const struct trailer_conf *conf;
+ ssize_t separator_pos;
+ static char *cl_separators;
if (unset) {
free_new_trailers(trailers);
@@ -58,7 +63,42 @@ static int option_parse_trailer(const struct option *opt,
if (!arg)
return -1;
- parse_trailer_from_command_line_arg(arg, where, if_exists, if_missing, trailers);
+ /*
+ * In command-line arguments, '=' is accepted (in addition to the
+ * separators that are defined).
+ */
+ cl_separators = xstrfmt("=%s", trailer_default_separators());
+ separator_pos = find_separator(arg, cl_separators);
+ free(cl_separators);
+
+ if (separator_pos == 0) {
+ 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);
+ } else {
+ struct trailer_conf *conf_current = new_trailer_conf();
+ parse_trailer(arg, separator_pos, &tok, &val, &conf);
+ duplicate_trailer_conf(conf_current, conf);
+
+ /*
+ * 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(trailers,
+ strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf_current);
+ free_trailer_conf(conf_current);
+ }
return 0;
}
diff --git a/trailer.c b/trailer.c
index 0893175553a..b0b067ab12c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -754,56 +754,6 @@ void parse_trailers_from_config(struct list_head *config_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)
-{
- struct strbuf tok = STRBUF_INIT;
- struct strbuf val = STRBUF_INIT;
- const struct trailer_conf *conf;
-
- /*
- * In command-line arguments, '=' is accepted (in addition to the
- * separators that are defined).
- */
- char *cl_separators = xstrfmt("=%s", trailer_default_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, 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 (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);
- }
-}
-
static const char *next_line(const char *str)
{
const char *nl = strchrnul(str, '\n');
diff --git a/trailer.h b/trailer.h
index 2848a0d086c..af55032625d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -62,12 +62,6 @@ struct process_trailer_options {
void parse_trailers_from_config(struct list_head *config_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);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 27/28] trailer_set_*(): put out parameter at the end
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 new trailer_config_set_*() functions which were introduced a few
patches ago put the out parameter (the variable being mutated) at the
end of the parameter list.
Put the out parameter at the end for these functions for these existing
trailer_set_*() functions for consistency. This also avoids confusion
for API users because otherwise these two sets of functions look rather
similar in <trailer.h> even though they have completely different out
parameters.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 6 +++---
trailer.c | 24 ++++++++++++------------
trailer.h | 6 +++---
3 files changed, 18 insertions(+), 18 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 9657b0d067c..d0c09d1d73b 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -28,21 +28,21 @@ static int option_parse_where(const struct option *opt,
const char *arg, int unset UNUSED)
{
/* unset implies NULL arg, which is handled in our helper */
- return trailer_set_where(opt->value, arg);
+ return trailer_set_where(arg, opt->value);
}
static int option_parse_if_exists(const struct option *opt,
const char *arg, int unset UNUSED)
{
/* unset implies NULL arg, which is handled in our helper */
- return trailer_set_if_exists(opt->value, arg);
+ return trailer_set_if_exists(arg, opt->value);
}
static int option_parse_if_missing(const struct option *opt,
const char *arg, int unset UNUSED)
{
/* unset implies NULL arg, which is handled in our helper */
- return trailer_set_if_missing(opt->value, arg);
+ return trailer_set_if_missing(arg, opt->value);
}
static int option_parse_trailer(const struct option *opt,
diff --git a/trailer.c b/trailer.c
index b0b067ab12c..7b0bdfcb27e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -380,7 +380,7 @@ void process_trailers_lists(struct list_head *head,
}
}
-int trailer_set_where(enum trailer_where *item, const char *value)
+int trailer_set_where(const char *value, enum trailer_where *item)
{
if (!value)
*item = WHERE_DEFAULT;
@@ -397,7 +397,7 @@ int trailer_set_where(enum trailer_where *item, const char *value)
return 0;
}
-int trailer_set_if_exists(enum trailer_if_exists *item, const char *value)
+int trailer_set_if_exists(const char *value, enum trailer_if_exists *item)
{
if (!value)
*item = EXISTS_DEFAULT;
@@ -416,7 +416,7 @@ int trailer_set_if_exists(enum trailer_if_exists *item, const char *value)
return 0;
}
-int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
+int trailer_set_if_missing(const char *value, enum trailer_if_missing *item)
{
if (!value)
*item = MISSING_DEFAULT;
@@ -520,18 +520,18 @@ static int git_trailer_default_config(const char *conf_key, const char *value,
variable_name = strrchr(trailer_item, '.');
if (!variable_name) {
if (!strcmp(trailer_item, "where")) {
- if (trailer_set_where(&default_trailer_conf.where,
- value) < 0)
+ if (trailer_set_where(value,
+ &default_trailer_conf.where) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifexists")) {
- if (trailer_set_if_exists(&default_trailer_conf.if_exists,
- value) < 0)
+ if (trailer_set_if_exists(value,
+ &default_trailer_conf.if_exists) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "ifmissing")) {
- if (trailer_set_if_missing(&default_trailer_conf.if_missing,
- value) < 0)
+ if (trailer_set_if_missing(value,
+ &default_trailer_conf.if_missing) < 0)
warning(_("unknown value '%s' for key '%s'"),
value, conf_key);
} else if (!strcmp(trailer_item, "separators")) {
@@ -600,15 +600,15 @@ static int git_trailer_config(const char *conf_key, const char *value,
conf->cmd = xstrdup(value);
break;
case TRAILER_WHERE:
- if (trailer_set_where(&conf->where, value))
+ if (trailer_set_where(value, &conf->where))
warning(_("unknown value '%s' for key '%s'"), value, conf_key);
break;
case TRAILER_IF_EXISTS:
- if (trailer_set_if_exists(&conf->if_exists, value))
+ if (trailer_set_if_exists(value, &conf->if_exists))
warning(_("unknown value '%s' for key '%s'"), value, conf_key);
break;
case TRAILER_IF_MISSING:
- if (trailer_set_if_missing(&conf->if_missing, value))
+ if (trailer_set_if_missing(value, &conf->if_missing))
warning(_("unknown value '%s' for key '%s'"), value, conf_key);
break;
default:
diff --git a/trailer.h b/trailer.h
index af55032625d..4193bedbae4 100644
--- a/trailer.h
+++ b/trailer.h
@@ -28,9 +28,9 @@ enum trailer_if_missing {
MISSING_DO_NOTHING
};
-int trailer_set_where(enum trailer_where *item, const char *value);
-int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
-int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
+int trailer_set_where(const char *, enum trailer_where *);
+int trailer_set_if_exists(const char *, enum trailer_if_exists *);
+int trailer_set_if_missing(const char *, enum trailer_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 *);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v4 28/28] trailer: introduce "template" term for readability
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 term "arg_item" is ambiguous because we use it to hold data for
(1) trailers specified as command line arguments (in
builtin/interpret-trailers.c), and
(2) trailers specified in configuration,
and these are both used to ultimately insert new trailers (based
on the contents of arg_item, acting as a kind of template) into some
other set of existing trailers (such as those found in a trailer block
inside a log message) that have already been parsed.
Rename "arg_item" to "trailer_template". This necessitates further
renames to make the functions that act on these templates match the data
structures (parameters) they act on:
- [*] add_arg_to_input_list() to apply_template_to_trailers()
- [*] apply_arg_if_exists() to maybe_add_if_exists()
- [*] apply_arg_if_missing() to maybe_add_if_missing()
- apply_command() to run_command_from_template()
- [*] apply_item_command() to populate_template_value()
- free_arg_item() to free_template() (non-API function)
- free_new_trailers() to free_trailer_templates() (API function)
- get_conf_item() to get_or_add_template_by()
- option_parse_trailer() to option_parse_trailer_template()
- parse_trailers_from_config() to parse_trailer_templates_from_config()
- [*] process_trailers_lists() to apply_trailer_templates()
- token_from_item() to token_from_template()
- token_matches_item to token_matches_template
- [*] trailer_add_arg_item() to add_trailer_template()
- trailer_from_arg() to trailer_from()
- [*] check_if_different() (reorder parameters only)
- [*] find_same_and_apply_arg() (reorder parameters only)
Reorder parameters 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
themselves have nothing to do with CLI argument handling.
Also note that these renames expose the previously liberal use of
"trailer" to mean both trailers we read from the input text (trailer
block) and trailer templates that are defined as CLI args or
configurations. Some functions implied a single action when they could
do two different things, so introduce words like "maybe" and "or" to
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.
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 would lose out on functionality).
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 50 +++---
trailer.c | 290 +++++++++++++++++++----------------
trailer.h | 12 +-
3 files changed, 193 insertions(+), 159 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index d0c09d1d73b..d3d2544c5f1 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -45,10 +45,14 @@ static int option_parse_if_missing(const struct option *opt,
return trailer_set_if_missing(arg, opt->value);
}
-static int option_parse_trailer(const struct option *opt,
- const char *arg, int unset)
+/*
+ * Interpret "--trailer ..." as trailer templates (trailers we want to add into
+ * the input text).
+ */
+static int option_parse_trailer_template(const struct option *opt,
+ const char *arg, int unset)
{
- struct list_head *trailers = opt->value;
+ struct list_head *templates = opt->value;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
const struct trailer_conf *conf;
@@ -56,7 +60,7 @@ static int option_parse_trailer(const struct option *opt,
static char *cl_separators;
if (unset) {
- free_new_trailers(trailers);
+ free_trailer_templates(templates);
return 0;
}
@@ -93,10 +97,10 @@ static int option_parse_trailer(const struct option *opt,
if (if_missing != MISSING_DEFAULT)
trailer_set_conf_if_missing(if_missing, conf_current);
- trailer_add_arg_item(trailers,
- strbuf_detach(&tok, NULL),
+ add_trailer_template(strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL),
- conf_current);
+ conf_current,
+ templates);
free_trailer_conf(conf_current);
}
@@ -158,10 +162,10 @@ static void read_input_file(struct strbuf *sb, const char *file)
}
static void interpret_trailers(const struct process_trailer_options *opts,
- struct list_head *new_trailer_head,
+ struct list_head *templates,
const char *file)
{
- LIST_HEAD(head);
+ LIST_HEAD(trailers_from_sb);
struct strbuf sb = STRBUF_INIT;
struct strbuf tb = STRBUF_INIT;
struct trailer_block *trailer_block;
@@ -172,7 +176,7 @@ static void interpret_trailers(const struct process_trailer_options *opts,
if (opts->in_place)
outfile = create_in_place_tempfile(file);
- trailer_block = parse_trailers(opts, sb.buf, &head);
+ trailer_block = parse_trailers(opts, sb.buf, &trailers_from_sb);
/* Print the lines before the trailer block */
if (!opts->only_trailers)
@@ -182,11 +186,11 @@ static void interpret_trailers(const struct process_trailer_options *opts,
fprintf(outfile, "\n");
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);
@@ -206,8 +210,8 @@ static void interpret_trailers(const struct process_trailer_options *opts,
int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
- LIST_HEAD(configured_trailers);
- LIST_HEAD(trailers);
+ LIST_HEAD(configured_templates);
+ LIST_HEAD(templates);
struct option options[] = {
OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
@@ -226,8 +230,8 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
- OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
- N_("trailer(s) to add"), option_parse_trailer),
+ OPT_CALLBACK(0, "trailer", &templates, N_("trailer"),
+ N_("trailer(s) to add"), option_parse_trailer_template),
OPT_END()
};
@@ -235,30 +239,30 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
trailer_config_init();
if (!opts.only_input)
- parse_trailers_from_config(&configured_trailers);
+ parse_trailer_templates_from_config(&configured_templates);
argc = parse_options(argc, argv, prefix, options,
git_interpret_trailers_usage, 0);
- 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, &trailers);
+ list_splice(&configured_templates, &templates);
if (argc) {
int i;
for (i = 0; i < argc; 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, &trailers, NULL);
+ interpret_trailers(&opts, &templates, NULL);
}
- free_new_trailers(&trailers);
+ free_trailer_templates(&templates);
return 0;
}
diff --git a/trailer.c b/trailer.c
index 7b0bdfcb27e..cdb235c27b5 100644
--- a/trailer.c
+++ b/trailer.c
@@ -55,14 +55,14 @@ struct trailer_item {
char *value;
};
-struct arg_item {
+struct trailer_template {
struct list_head list;
char *token;
char *value;
struct trailer_conf conf;
};
-static LIST_HEAD(conf_head);
+static LIST_HEAD(templates_from_conf);
static char *separators = ":";
@@ -105,7 +105,7 @@ static size_t token_len_without_separator(const char *token, size_t len)
return len;
}
-static int same_token(struct trailer_item *a, struct arg_item *b)
+static int same_token(struct trailer_item *a, struct trailer_template *b)
{
size_t a_len, b_len, min_len;
@@ -119,12 +119,12 @@ static int same_token(struct trailer_item *a, struct arg_item *b)
return !strncasecmp(a->token, b->token, min_len);
}
-static int same_value(struct trailer_item *a, struct arg_item *b)
+static int same_value(struct trailer_item *a, struct trailer_template *b)
{
return !strcasecmp(a->value, b->value);
}
-static int same_trailer(struct trailer_item *a, struct arg_item *b)
+static int same_trailer(struct trailer_item *a, struct trailer_template *b)
{
return same_token(a, b) && same_value(a, b);
}
@@ -151,15 +151,15 @@ static void free_trailer_item(struct trailer_item *item)
free(item);
}
-static void free_arg_item(struct arg_item *item)
+static void free_template(struct trailer_template *template)
{
- free(item->conf.name);
- free(item->conf.key);
- free(item->conf.command);
- free(item->conf.cmd);
- free(item->token);
- free(item->value);
- free(item);
+ free(template->conf.name);
+ free(template->conf.key);
+ free(template->conf.command);
+ free(template->conf.cmd);
+ free(template->token);
+ free(template->value);
+ free(template);
}
static char last_non_space_char(const char *s)
@@ -171,36 +171,36 @@ static char last_non_space_char(const char *s)
return '\0';
}
-static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
+static struct trailer_item *trailer_from(struct trailer_template *template)
{
struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
- new_item->token = arg_tok->token;
- new_item->value = arg_tok->value;
- arg_tok->token = arg_tok->value = NULL;
- free_arg_item(arg_tok);
+ new_item->token = template->token;
+ new_item->value = template->value;
+ template->token = template->value = NULL;
+ free_template(template);
return new_item;
}
-static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct arg_item *arg_tok)
+static void apply_template_to_trailers(struct trailer_template *template,
+ struct trailer_item *on_tok)
{
- int aoe = after_or_end(arg_tok->conf.where);
- struct trailer_item *to_add = trailer_from_arg(arg_tok);
+ int aoe = after_or_end(template->conf.where);
+ struct trailer_item *to_add = trailer_from(template);
if (aoe)
list_add(&to_add->list, &on_tok->list);
else
list_add_tail(&to_add->list, &on_tok->list);
}
-static int check_if_different(struct trailer_item *in_tok,
- struct arg_item *arg_tok,
- int check_all,
- struct list_head *head)
+static int check_if_different(struct trailer_template *template,
+ struct trailer_item *in_tok,
+ struct list_head *head,
+ int check_all)
{
- enum trailer_where where = arg_tok->conf.where;
+ enum trailer_where where = template->conf.where;
struct list_head *next_head;
do {
- if (same_trailer(in_tok, arg_tok))
+ if (same_trailer(in_tok, template))
return 0;
/*
* if we want to add a trailer after another one,
@@ -215,7 +215,8 @@ static int check_if_different(struct trailer_item *in_tok,
return 1;
}
-static char *apply_command(struct trailer_conf *conf, const char *arg)
+static char *run_command_from_template(struct trailer_conf *conf,
+ const char *arg)
{
struct strbuf cmd = STRBUF_INIT;
struct strbuf buf = STRBUF_INIT;
@@ -250,133 +251,142 @@ static char *apply_command(struct trailer_conf *conf, const char *arg)
return result;
}
-static void apply_item_command(struct trailer_item *in_tok, struct arg_item *arg_tok)
+/*
+ * Prepare the template by running the command (if any) requested by the
+ * template in order to populate the template's value field.
+ */
+static void populate_template_value(struct trailer_template *template,
+ struct trailer_item *in_tok)
{
- if (arg_tok->conf.command || arg_tok->conf.cmd) {
+ if (template->conf.command || template->conf.cmd) {
+ /*
+ * Determine argument to pass into the command.
+ */
const char *arg;
- if (arg_tok->value && arg_tok->value[0]) {
- arg = arg_tok->value;
+ if (template->value && template->value[0]) {
+ arg = template->value;
} else {
if (in_tok && in_tok->value)
arg = xstrdup(in_tok->value);
else
arg = xstrdup("");
}
- arg_tok->value = apply_command(&arg_tok->conf, arg);
+ template->value = run_command_from_template(&template->conf,
+ arg);
free((char *)arg);
}
}
-static void apply_arg_if_exists(struct trailer_item *in_tok,
- struct arg_item *arg_tok,
+static void maybe_add_if_exists(struct trailer_template *template,
+ struct trailer_item *in_tok,
struct trailer_item *on_tok,
- struct list_head *head)
+ struct list_head *trailers)
{
- switch (arg_tok->conf.if_exists) {
+ switch (template->conf.if_exists) {
case EXISTS_DO_NOTHING:
- free_arg_item(arg_tok);
+ free_template(template);
break;
case EXISTS_REPLACE:
- apply_item_command(in_tok, arg_tok);
- add_arg_to_input_list(on_tok, arg_tok);
+ populate_template_value(template, in_tok);
+ apply_template_to_trailers(template, on_tok);
list_del(&in_tok->list);
free_trailer_item(in_tok);
break;
case EXISTS_ADD:
- apply_item_command(in_tok, arg_tok);
- add_arg_to_input_list(on_tok, arg_tok);
+ populate_template_value(template, in_tok);
+ apply_template_to_trailers(template, on_tok);
break;
case EXISTS_ADD_IF_DIFFERENT:
- apply_item_command(in_tok, arg_tok);
- if (check_if_different(in_tok, arg_tok, 1, head))
- add_arg_to_input_list(on_tok, arg_tok);
+ populate_template_value(template, in_tok);
+ if (check_if_different(template, in_tok, trailers, 1))
+ apply_template_to_trailers(template, on_tok);
else
- free_arg_item(arg_tok);
+ free_template(template);
break;
case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
- apply_item_command(in_tok, arg_tok);
- if (check_if_different(on_tok, arg_tok, 0, head))
- add_arg_to_input_list(on_tok, arg_tok);
+ populate_template_value(template, in_tok);
+ if (check_if_different(template, on_tok, trailers, 0))
+ apply_template_to_trailers(template, on_tok);
else
- free_arg_item(arg_tok);
+ free_template(template);
break;
default:
BUG("trailer.c: unhandled value %d",
- arg_tok->conf.if_exists);
+ template->conf.if_exists);
}
}
-static void apply_arg_if_missing(struct list_head *head,
- struct arg_item *arg_tok)
+static void maybe_add_if_missing(struct trailer_template *template,
+ struct list_head *trailers)
{
enum trailer_where where;
struct trailer_item *to_add;
- switch (arg_tok->conf.if_missing) {
+ switch (template->conf.if_missing) {
case MISSING_DO_NOTHING:
- free_arg_item(arg_tok);
+ free_template(template);
break;
case MISSING_ADD:
- where = arg_tok->conf.where;
- apply_item_command(NULL, arg_tok);
- to_add = trailer_from_arg(arg_tok);
+ where = template->conf.where;
+ populate_template_value(template, NULL);
+ to_add = trailer_from(template);
if (after_or_end(where))
- list_add_tail(&to_add->list, head);
+ list_add_tail(&to_add->list, trailers);
else
- list_add(&to_add->list, head);
+ list_add(&to_add->list, trailers);
break;
default:
BUG("trailer.c: unhandled value %d",
- arg_tok->conf.if_missing);
+ template->conf.if_missing);
}
}
-static int find_same_and_apply_arg(struct list_head *head,
- struct arg_item *arg_tok)
+static int find_same_and_apply_arg(struct trailer_template *template,
+ struct list_head *trailers)
{
struct list_head *pos;
struct trailer_item *in_tok;
struct trailer_item *on_tok;
- enum trailer_where where = arg_tok->conf.where;
+ enum trailer_where where = template->conf.where;
int middle = (where == WHERE_AFTER) || (where == WHERE_BEFORE);
int backwards = after_or_end(where);
struct trailer_item *start_tok;
- if (list_empty(head))
+ if (list_empty(trailers))
return 0;
- start_tok = list_entry(backwards ? head->prev : head->next,
+ start_tok = list_entry(backwards ? trailers->prev : trailers->next,
struct trailer_item,
list);
- list_for_each_dir(pos, head, backwards) {
+ list_for_each_dir(pos, trailers, backwards) {
in_tok = list_entry(pos, struct trailer_item, list);
- if (!same_token(in_tok, arg_tok))
+ if (!same_token(in_tok, template))
continue;
on_tok = middle ? in_tok : start_tok;
- apply_arg_if_exists(in_tok, arg_tok, on_tok, head);
+ maybe_add_if_exists(template, in_tok, on_tok, trailers);
return 1;
}
return 0;
}
-void process_trailers_lists(struct list_head *head,
- struct list_head *arg_head)
+void apply_trailer_templates(struct list_head *templates,
+ struct list_head *trailers)
{
struct list_head *pos, *p;
- struct arg_item *arg_tok;
+ struct trailer_template *template;
- list_for_each_safe(pos, p, arg_head) {
+ list_for_each_safe(pos, p, templates) {
int applied = 0;
- arg_tok = list_entry(pos, struct arg_item, list);
+ template = list_entry(pos, struct trailer_template, list);
list_del(pos);
- applied = find_same_and_apply_arg(head, arg_tok);
+ applied = find_same_and_apply_arg(template, trailers);
if (!applied)
- apply_arg_if_missing(head, arg_tok);
+ maybe_add_if_missing(template, trailers);
}
}
@@ -471,26 +481,26 @@ void free_trailer_conf(struct trailer_conf *conf)
free(conf);
}
-static struct arg_item *get_conf_item(const char *name)
+static struct trailer_template *get_or_add_template_by(const char *name)
{
struct list_head *pos;
- struct arg_item *item;
+ struct trailer_template *template;
- /* Look up item with same name */
- list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct arg_item, list);
- if (!strcasecmp(item->conf.name, name))
- return item;
+ /* Look up template with same name. */
+ list_for_each(pos, &templates_from_conf) {
+ template = list_entry(pos, struct trailer_template, list);
+ if (!strcasecmp(template->conf.name, name))
+ return template;
}
- /* Item does not already exists, create it */
- CALLOC_ARRAY(item, 1);
- duplicate_trailer_conf(&item->conf, &default_trailer_conf);
- item->conf.name = xstrdup(name);
+ /* Template does not already exist; create it. */
+ CALLOC_ARRAY(template, 1);
+ duplicate_trailer_conf(&template->conf, &default_trailer_conf);
+ template->conf.name = xstrdup(name);
- list_add_tail(&item->list, &conf_head);
+ list_add_tail(&template->list, &templates_from_conf);
- return item;
+ return template;
}
enum trailer_info_type { TRAILER_KEY, TRAILER_COMMAND, TRAILER_CMD,
@@ -548,7 +558,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
void *cb UNUSED)
{
const char *trailer_item, *variable_name;
- struct arg_item *item;
+ struct trailer_template *template;
struct trailer_conf *conf;
char *name = NULL;
enum trailer_info_type type;
@@ -573,8 +583,8 @@ static int git_trailer_config(const char *conf_key, const char *value,
if (!name)
return 0;
- item = get_conf_item(name);
- conf = &item->conf;
+ template = get_or_add_template_by(name);
+ conf = &template->conf;
free(name);
switch (type) {
@@ -631,20 +641,22 @@ void trailer_config_init(void)
configured = 1;
}
-static const char *token_from_item(struct arg_item *item, char *tok)
+static const char *token_from_template(struct trailer_template *template, char *tok)
{
- if (item->conf.key)
- return item->conf.key;
+ if (template->conf.key)
+ return template->conf.key;
if (tok)
return tok;
- return item->conf.name;
+ return template->conf.name;
}
-static int token_matches_item(const char *tok, struct arg_item *item, size_t tok_len)
+static int token_matches_template(const char *tok,
+ struct trailer_template *template,
+ size_t tok_len)
{
- if (!strncasecmp(tok, item->conf.name, tok_len))
+ if (!strncasecmp(tok, template->conf.name, tok_len))
return 1;
- return item->conf.key ? !strncasecmp(tok, item->conf.key, tok_len) : 0;
+ return template->conf.key ? !strncasecmp(tok, template->conf.key, tok_len) : 0;
}
/*
@@ -687,7 +699,7 @@ void parse_trailer(const char *line, ssize_t separator_pos,
struct strbuf *tok, struct strbuf *val,
const struct trailer_conf **conf)
{
- struct arg_item *item;
+ struct trailer_template *template;
size_t tok_len;
struct list_head *pos;
@@ -705,13 +717,13 @@ void parse_trailer(const char *line, ssize_t separator_pos,
tok_len = token_len_without_separator(tok->buf, tok->len);
if (conf)
*conf = &default_trailer_conf;
- list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct arg_item, list);
- if (token_matches_item(tok->buf, item, tok_len)) {
+ list_for_each(pos, &templates_from_conf) {
+ template = list_entry(pos, struct trailer_template, list);
+ if (token_matches_template(tok->buf, template, tok_len)) {
char *tok_buf = strbuf_detach(tok, NULL);
if (conf)
- *conf = &item->conf;
- strbuf_addstr(tok, token_from_item(item, tok_buf));
+ *conf = &template->conf;
+ strbuf_addstr(tok, token_from_template(template, tok_buf));
free(tok_buf);
break;
}
@@ -728,29 +740,41 @@ static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
return new_item;
}
-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)
{
- struct arg_item *new_item = xcalloc(1, sizeof(*new_item));
- new_item->token = tok;
- new_item->value = val;
- duplicate_trailer_conf(&new_item->conf, conf);
- list_add_tail(&new_item->list, arg_head);
+ struct trailer_template *template = xcalloc(1, sizeof(*template));
+ template->token = tok;
+ template->value = val;
+ duplicate_trailer_conf(&template->conf, conf);
+ list_add_tail(&template->list, templates);
}
-void parse_trailers_from_config(struct list_head *config_head)
+void parse_trailer_templates_from_config(struct list_head *config_head)
{
- struct arg_item *item;
+ struct trailer_template *template;
struct list_head *pos;
- /* Add an arg item for each configured trailer with a command */
- list_for_each(pos, &conf_head) {
- item = list_entry(pos, struct arg_item, list);
- if (item->conf.command)
- trailer_add_arg_item(config_head,
- xstrdup(token_from_item(item, NULL)),
- xstrdup(""),
- &item->conf);
+ /*
+ * Get configured templates with a ".command" option.
+ *
+ * NEEDSWORK: If the interpret-trailers builtin sees a
+ * "trailer.foo.command = ..." setting, then the "foo" trailer will
+ * always be inserted, even if "--trailer foo" is not provided.
+ * Considering how ".command" is deprecated, it is a bit strange to see
+ * it getting special treatment like this over ".cmd". Instead, we
+ * should add a new option that explicitly lets the user decide if the
+ * configured trailer should always be added automatically, or if it
+ * should only be added if "--trailer foo" is provided (default).
+ * Then we can collect configured trailers that have either ".command"
+ * or ".cmd" below, instead of just ".command".
+ */
+ list_for_each(pos, &templates_from_conf) {
+ template = list_entry(pos, struct trailer_template, list);
+ if (template->conf.command)
+ add_trailer_template(xstrdup(token_from_template(template,
+ NULL)),
+ xstrdup(""), &template->conf, config_head);
}
}
@@ -906,10 +930,16 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
possible_continuation_lines = 0;
if (recognized_prefix)
continue;
- list_for_each(pos, &conf_head) {
- struct arg_item *item;
- item = list_entry(pos, struct arg_item, list);
- if (token_matches_item(bol, item,
+ /*
+ * The templates here are not used for actually
+ * adding trailers anywhere, but instead to help us
+ * identify trailer lines by comparing their keys with
+ * those found in configured templates.
+ */
+ list_for_each(pos, &templates_from_conf) {
+ struct trailer_template *template;
+ template = list_entry(pos, struct trailer_template, list);
+ if (token_matches_template(bol, template,
separator_pos)) {
recognized_prefix = 1;
break;
@@ -1065,13 +1095,13 @@ void free_trailers(struct list_head *trailers)
}
}
-void free_new_trailers(struct list_head *trailers)
+void free_trailer_templates(struct list_head *trailer_templates)
{
struct list_head *pos, *p;
- list_for_each_safe(pos, p, trailers) {
+ list_for_each_safe(pos, p, trailer_templates) {
list_del(pos);
- free_arg_item(list_entry(pos, struct arg_item, list));
+ free_template(list_entry(pos, struct trailer_template, list));
}
}
diff --git a/trailer.h b/trailer.h
index 4193bedbae4..2a21d74c263 100644
--- a/trailer.h
+++ b/trailer.h
@@ -40,8 +40,8 @@ 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(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 {
int in_place;
@@ -60,10 +60,10 @@ struct process_trailer_options {
#define PROCESS_TRAILER_OPTIONS_INIT {0}
-void parse_trailers_from_config(struct list_head *config_head);
+void parse_trailer_templates_from_config(struct list_head *config_head);
-void process_trailers_lists(struct list_head *head,
- struct list_head *arg_head);
+void apply_trailer_templates(struct list_head *templates,
+ struct list_head *trailers_head);
ssize_t find_separator(const char *line, const char *separators);
@@ -86,8 +86,8 @@ 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 *);
/*
* Convenience function to format the trailers from the commit msg "msg" into
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH] builtin/stash: report failure to write to index
From: Patrick Steinhardt @ 2024-02-06 5:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, moti sd
In-Reply-To: <xmqqo7cul1fi.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 3118 bytes --]
On Mon, Feb 05, 2024 at 07:24:49PM -0800, Junio C Hamano wrote:
> 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?
We do return a proper exit code as demonstrated by the tests. But if you
interactively use those commands in the shell then you're quite likely
to not notice error codes at all -- my shell certainly doesn't highlight
failed commands in any special way.
> 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?
As a user, my expectation is that if a command failed, it didn't do
anything. If it did something before failing and wasn't able to clean it
up, then it is the responsibility of the command to tell me that it
might have screwed up and left behind some partially-applied changes.
It could certainly be that my expectation is way off. But personally, I
don't think it's useful to say "We didn't do anything" in every case
where we failed without doing anything -- I'd rather feel that it is
quite spammy.
But anyway, I know that my UX skills are severely lacking. So in case
you or anybody else has a specific suggestion for how to make it better
then I'm certainly happy to adapt.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Patrick Steinhardt @ 2024-02-06 5:33 UTC (permalink / raw)
To: phillip.wood; +Cc: Karthik Nayak, git, gitster
In-Reply-To: <98d79d33-0d7e-4a9c-a6a3-ed9b58cd7445@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]
On Mon, Feb 05, 2024 at 06:48:52PM +0000, Phillip Wood wrote:
> Hi Karthik
>
> On 29/01/2024 11:35, Karthik Nayak wrote:
> > When the user uses an empty string pattern (""), we don't match any refs
> > in git-for-each-ref(1). This is because in git-for-each-ref(1), we use
> > path based matching and an empty string doesn't match any path.
> >
> > In this commit we change this behavior by making empty string pattern
> > match all references. This is done by introducing a new flag
> > `FILTER_REFS_NO_FILTER` in `ref-filter.c`, which uses the newly
> > introduced `refs_for_each_all_refs()` function to iterate over all the
> > refs in the repository.
>
> It actually iterates over all the refs in the current worktree, not all the
> refs in the repository. I have to say that I find it extremely unintuitive
> that "" behaves differently to not giving a pattern. I wonder if we can find
> a better UI here - perhaps a command line option to include pseudorefs?
I tend to agree, and have argued for a flag in another thread, too [1].
Something like `--show-all-refs` feels a lot more intuitive to me and
also doesn't have the potential to break preexisting scripts which pass
the empty prefix (which would have returned the empty set of refs, but
now returns all refs).
[1]: https://lore.kernel.org/git/ZZWCXFghtql4i4YE@tanuki/
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2] builtin/stash: report failure to write to index
From: Patrick Steinhardt @ 2024-02-06 5:34 UTC (permalink / raw)
To: git; +Cc: moti sd, Rubén Justo, Junio C Hamano
In-Reply-To: <CAPvDF0P7_s-iy_V7FNSHtXXc9v5E3Fm=AdgduDDsd0rM-zNg-g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 7465 bytes --]
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.
While at it, unify error messages when writing to the index fails. The
chosen error message is already used in "builtin/stash.c".
Reported-by: moti sd <motisd8@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Range-diff against v1:
1: f114688eac < -: ---------- drop dot
2: b13a5a10ac ! 1: cb098cf88c builtin/stash: report failure to write to index
@@ Commit message
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).
+ While at it, unify error messages when writing to the index fails. The
+ chosen error message is already used in "builtin/stash.c".
Reported-by: moti sd <motisd8@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## builtin/stash.c ##
+@@ builtin/stash.c: static void unstage_changes_unless_new(struct object_id *orig_tree)
+ repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
+ if (write_locked_index(&the_index, &lock,
+ COMMIT_LOCK | SKIP_IF_UNCHANGED))
+- die(_("Unable to write index."));
++ die(_("could not write index"));
+ }
+
+ static int do_apply_stash(const char *prefix, struct stash_info *info,
@@ builtin/stash.c: 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"));
++ return error(_("could not write index"));
if (write_index_as_tree(&c_tree, &the_index, get_index_file(), 0,
NULL))
@@ builtin/stash.c: static int do_create_stash(const struct pathspec *ps, struct st
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"));
++ ret = error(_("could not write index"));
goto done;
}
@@ builtin/stash.c: static int do_push_stash(const struct pathspec *ps, const char
if (repo_refresh_and_write_index(the_repository, REFRESH_QUIET, 0, 0,
NULL, NULL, NULL)) {
- ret = -1;
-+ ret = error(_("Cannot write to the index"));
++ ret = error(_("could not write index"));
goto done;
}
@@ t/t3903-stash.sh: test_expect_success 'restore untracked files even when we hit
+ touch .git/index.lock &&
+
+ cat >expect <<-EOF &&
-+ error: Cannot write to the index
++ error: could not write index
+ EOF
+ test_must_fail git stash create 2>err &&
+ test_cmp expect err
@@ t/t3903-stash.sh: test_expect_success 'restore untracked files even when we hit
+ touch .git/index.lock &&
+
+ cat >expect <<-EOF &&
-+ error: Cannot write to the index
++ error: could not write index
+ EOF
+ test_must_fail git stash push 2>err &&
+ test_cmp expect err
@@ t/t3903-stash.sh: test_expect_success 'restore untracked files even when we hit
+ touch .git/index.lock &&
+
+ cat >expect <<-EOF &&
-+ error: Cannot write to the index
++ error: could not write index
+ EOF
+ test_must_fail git stash apply 2>err &&
+ test_cmp expect err
builtin/stash.c | 8 ++++----
t/t3903-stash.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+), 4 deletions(-)
diff --git a/builtin/stash.c b/builtin/stash.c
index 26489b76c0..d65cd20ee6 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -520,7 +520,7 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)
repo_hold_locked_index(the_repository, &lock, LOCK_DIE_ON_ERROR);
if (write_locked_index(&the_index, &lock,
COMMIT_LOCK | SKIP_IF_UNCHANGED))
- die(_("Unable to write index."));
+ die(_("could not write index"));
}
static int do_apply_stash(const char *prefix, struct stash_info *info,
@@ -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(_("could not write 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(_("could not write 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(_("could not write index"));
goto done;
}
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 3319240515..00db82fb24 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: could not write 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: could not write 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: could not write 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 related
* Re: [PATCH] Add ideas for GSoC 2024
From: Patrick Steinhardt @ 2024-02-06 5:47 UTC (permalink / raw)
To: Christian Couder
Cc: git, Taylor Blau, Junio C Hamano, Victoria Dye, Karthik Nayak
In-Reply-To: <CAP8UFD3F95TzytdpKO=LLf6Y_ejxwh9QtgAxRNKgMXW-72hjgQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 6491 bytes --]
On Mon, Feb 05, 2024 at 05:43:17PM +0100, Christian Couder wrote:
> On Mon, Feb 5, 2024 at 9:39 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > Add project ideas for the GSoC 2024.
> > ---
> >
> > I came up with four different topics:
> >
> > - The reftable unit test refactorings. This topic can also be squashed
> > into the preexisting unit test topics, I wouldn't mind. In that case
> > I'd be happy to be a possible mentor, too.
> >
> > - Ref consistency checks for git-fsck(1). This should be rather
> > straight forward and make for an interesting topic.
> >
> > - Making git-bisect(1)'s state more self-contained as recently
> > discussed. This topic is easy to implement, but the backwards
> > compatibility issues might require a lot of attention.
> >
> > - Implementing support for reftables in the "dumb" HTTP protocol. It's
> > quite niche given that the dumb protocol isn't really used much
> > nowadays anymore. But it could make for an interesting project
> > regardless.
> >
> > It's hard to estimate for me whether their scope is either too small or
> > too big. So please feel free to chime in and share your concerns if you
> > think that any of those proposals don't make much sense in your opinion.
>
> Thanks a lot for these ideas! I have applied your patch and pushed it.
>
> I have a few concerns though, see below.
>
> > SoC-2024-Ideas.md | 129 ++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 129 insertions(+)
> >
> > diff --git a/SoC-2024-Ideas.md b/SoC-2024-Ideas.md
> > index 3efbcaf..286aea0 100644
> > --- a/SoC-2024-Ideas.md
> > +++ b/SoC-2024-Ideas.md
> > @@ -39,3 +39,132 @@ Languages: C, shell(bash)
> > Possible mentors:
> > * Christian Couder < <christian.couder@gmail.com> >
> >
> > +### Convert reftable unit tests to use the unit testing framework
> > +
> > +The "reftable" unit tests in "t0032-reftable-unittest.sh"
> > +predate the unit testing framework that was recently
> > +introduced into Git. These tests should be converted to use
> > +the new framework.
> > +
> > +See:
> > +
> > + - this discussion <https://lore.kernel.org/git/cover.1692297001.git.steadmon@google.com/>
> > +
> > +Expected Project Size: 175 hours or 350 hours
> > +
> > +Difficulty: Low
>
> "Difficulty: Low" might not be very accurate from the point of view of
> contributors. I think it's always quite difficult to contribute
> something significant to Git, and sometimes more than we expected.
That's certainly true. I understood the difficulty levels here as being
relative to the already-high bar that the Git project typically sets.
Otherwise there wouldn't be much use to specify difficulty in the first
place if all items had the same difficulty.
Or is the intent of the difficulty level rather on a global GSoC level?
In that case I agree that we should bump the difficulty to "medium".
> > +Languages: C, shell(bash)
> > +
> > +Possible mentors:
> > +* Patrick Steinhardt < <ps@pks.im> >
> > +* Karthik Nayak < <karthik.188@gmail.com> >
> > +
> > +### Implement consistency checks for refs
> > +
> > +The git-fsck(1) command is used to check various data
> > +structures for consistency. Notably missing though are
> > +consistency checks for the refdb. While git-fsck(1)
> > +implicitly checks some of the properties of the refdb
> > +because it uses its refs for a connectivity check, these
> > +checks aren't sufficient to properly ensure that all refs
> > +are properly consistent.
> > +
> > +The goal of this project would be to introduce consistency
> > +checks that can be implemented by the ref backend. Initially
> > +these checks may only apply to the "files" backend. With the
> > +ongoing efforts to upstream a new "reftable" backend the
> > +effort may be extended.
> > +
> > +See:
> > +
> > + - https://lore.kernel.org/git/6cfee0e4-3285-4f18-91ff-d097da9de737@rd10.de/
> > + - https://lore.kernel.org/git/cover.1706601199.git.ps@pks.im/
> > +
> > +Expected Project Size: 175 hours or 350 hours
> > +
> > +Difficulty: Medium
> > +
> > +Languages: C, shell(bash)
> > +
> > +Possible mentors:
> > +* Patrick Steinhardt < <ps@pks.im> >
> > +* Karthik Nayak < <karthik.188@gmail.com> >
> > +
> > +### Refactor git-bisect(1) to make its state self-contained
> > +
> > +The git-bisect(1) command is used to find a commit in a
> > +range of commits that introduced a specific bug. Starting a
> > +bisection run creates a set of state files into the Git
> > +repository which record various different parameters like
> > +".git/BISECT_START". These files look almost like refs
> > +due to their names being all-uppercase. This has led to
> > +confusion with the new "reftable" backend because it wasn't
> > +quite clear whether those files are in fact refs or not.
> > +
> > +As it turns out they are not refs and should never be
> > +treated like one. Overall, it has been concluded that the
> > +way those files are currently stored is not ideal. Instead
> > +of having a proliferation of files in the Git directory, it
> > +was discussed whether the bisect state should be moved into
> > +its own "bisect-state" subdirectory. This would make it more
> > +self-contained and thereby avoid future confusion. It is
> > +also aligned with the sequencer state used by rebases, which
> > +is neatly contained in the "rebase-apply" and "rebase-merge"
> > +directories.
> > +
> > +The goal of this project would be to realize this change.
> > +While rearchitecting the layout should be comparatively easy
> > +to do, the harder part will be to hash out how to handle
> > +backwards compatibility.
> > +
> > +See:
> > +
> > + - https://lore.kernel.org/git/Za-gF_Hp_lXViGWw@tanuki/
>
> From reading the discussion it looks like everyone is Ok with doing
> this. I really hope that we are not missing something that might make
> us decide early not to do this though.
I agree that this is a risky one, and that's what I tried to bring
across with the "harder part will be to hash out how to handle backwards
compatibility". Overall I think this project will be more about selling
the patch and reasoning about how it can be done without breaking
backwards compatibility.
We could bump the difficulty to high to reflect that better. But if you
deem the risk to be too high then I'm also happy to drop the topic
completely.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox