* Re: [PATCH 5/6] t5526: break test submodule differently
From: Patrick Steinhardt @ 2024-01-10 7:41 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cTB5OH1hCD-EagxNAcaw1=RR7yCeeZ_AzeqHtFTGxT-0g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]
On Tue, Jan 09, 2024 at 02:23:55PM -0500, Eric Sunshine wrote:
> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > In 10f5c52656 (submodule: avoid auto-discovery in
> > prepare_submodule_repo_env(), 2016-09-01) we fixed a bug when doing a
> > recursive fetch with submodule in the case where the submodule is broken
> > due to whatever reason. The test to exercise that the fix works breaks
> > the submodule by deleting its `HEAD` reference, which will cause us to
> > not detect the directory as a Git repository.
> >
> > While this is perfectly fine in theory, this way of breaking the repo
> > becomes problematic with the current efforts to introduce another refdb
> > backend into Git. The new reftable backend has a stub HEAD file that
> > always contains "ref: refs/heads/.invalid" so that tools continue to be
> > able to detect such a repository. But as the reftable backend will never
> > delete this file even when asked to delete `HEAD` the current way to
> > delete the `HEAD` reference will stop working.
>
> This patch is not the appropriate place to bikeshed (but since this is
> the first time I've read or paid attention to it), if I saw "ref:
> refs/heads/.invalid", the word ".invalid" would make me think
> something was broken in my repository. Would it make sense to use some
> less alarming word, such as perhaps ".placeholder", ".stand-in",
> ".synthesized" or even the name of the non-file-based backend in use?
Well, something _is_ broken in your repository in case Git ever tries to
read the "HEAD" placeholder in a reftable-enabled repository. But I
guess you rather come from the angle of using `cat HEAD` as a user. I do
agree that using a better hint could help users, but this detail has
already been recorded as such in "Documentation/technical/reftable.txt".
We can of course change this to be "ref: refs/heads/.reftable", but as
you already say this is something that should be discussed in a separate
thread.
> > Adapt the code to instead delete the objects database. Going back with
> > this new way to cuase breakage confirms that it triggers the infinite
> > recursion just the same, and there are no equivalent ongoing efforts to
> > replace the object database with an alternate backend.
>
> s/cuase/cause/
>
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
> > @@ -771,7 +771,7 @@ test_expect_success 'fetching submodule into a broken repository' '
> > # Break the receiving submodule
> > - test-tool -C dst/sub ref-store main delete-refs REF_NO_DEREF msg HEAD &&
> > + rm -r dst/sub/.git/objects &&
>
> If there is no guarantee that .git/objects will exist when any
> particular backend is in use, would it be more robust to use -f here,
> as well?
>
> rm -rf dst/sub/.git/objects &&
`.git/objects` always exists in a healthy repository. If it doesn't,
then `is_git_directory()` would return a false-ish value and we wouldn't
recognize the repository as such. Or are you saying that this could
potentially change in the future if there was ever to be an alternate
ODB format? If so that is a valid remark, but the test would break
regardless of whether we use `-f` or not: if a missing ".git/objects"
directory does not lead to a corrupted repository then the whole premise
of the test is broken.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/6] t1419: mark test suite as files-backend specific
From: Patrick Steinhardt @ 2024-01-10 7:30 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cTAiEFu9p1nRe9LC3mxyPmfQ9m4r7aQUj_9OC8pSbwbig@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2756 bytes --]
On Tue, Jan 09, 2024 at 02:40:50PM -0500, Eric Sunshine wrote:
> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > With 59c35fac54 (refs/packed-backend.c: implement jump lists to avoid
> > excluded pattern(s), 2023-07-10) we have implemented logic to handle
> > excluded refs more efficiently in the "packed" ref backend. This logic
> > allows us to skip emitting refs completely which we know to not be of
> > any interest to the caller, which can avoid quite some allocaitons and
> > object lookups.
>
> s/allocaitons/allocations/
>
> > This was wired up via a new `exclude_patterns` parameter passed to the
> > backend's ref iterator. The backend only needs to handle them on a best
> > effort basis though, and in fact we only handle it for the "packed-refs"
> > file, but not for loose references. Consequentially, all callers must
> > still filter emitted refs with those exclude patterns.
>
> s/Consequentially/Consequently/
Hum. I had the last time when you mentioned the in mind while writing
the commit message, but seemingly misremembered the outcome. So I now
looked things up in a dictionary, and both words seem to be used in
equivalent ways. As a non-native speaker, could you maybe elaborate a
bit to help me out? :)
> > The result is that handling exclude patterns is completely optional in
> > the ref backend, and any future backends may or may not implement it.
> > Let's thus mark the test for t1419 to depend on the REFFILES prereq.
>
> This change seems to be abusing the meaning of the REFFILES
> prerequisite. Instead the above description argues for introduction of
> a new prerequisite which indicates whether or not the backend honors
> the exclude patterns. Or, am I misunderstanding this?
I wouldn't say that this is abuse. We know the logic is only implemented
by certain backends, and for the time being the only backend that does
is the "files" backend. Furthermore, no test outside of t1419 ever cares
for whether the backend knows to handle exclude patterns, so introducing
a separate prereq that simply maps to REFFILES doesn't really feel worth
it. If we ever implement this behaviour in the "reftable" backend, then
we can easily extend the prereq like follows:
```
if ! test_have_prereq REFFILES && ! test_have_prereq REFTABLE
then
skip_all='skipping `git for-each-ref --exclude` tests; need files backend'
test_done
fi
```
Now we could of course make the prereq clever and auto-detect whether
the ref backend supports excludes. But this has the downside that it
could lead to silent failures in case the exclude pattern handling ever
breaks because now the prereq would potentially evaluate to "false".
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/6] t1300: mark tests to require default repo format
From: Patrick Steinhardt @ 2024-01-10 7:17 UTC (permalink / raw)
To: Eric Sunshine; +Cc: git
In-Reply-To: <CAPig+cRdDSMACzB6mEfwbijLHHSJuQ_Tk8ggNkvFxEd1aSqw2A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1494 bytes --]
On Tue, Jan 09, 2024 at 02:35:29PM -0500, Eric Sunshine wrote:
> On Tue, Jan 9, 2024 at 7:17 AM Patrick Steinhardt <ps@pks.im> wrote:
> > The t1300 test suite exercises the git-config(1) tool. To do so we
> > overwrite ".git/config" to contain custom contents. While this is easy
> > enough to do, it may create problems when using a non-default repository
> > format because we also overwrite the repository format version as well
> > as any potential extensions.
> >
> > Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the
> > problem. An alternative would be to carry over mandatory config keys
> > into the rewritten config file. But the effort does not seem worth it
> > given that the system under test is git-config(1), which is at a lower
> > level than the repository format.
>
> If I'm understanding correctly, with the approach taken by this patch,
> won't we undesirably lose some git-config test coverage if the
> file-based backend is ever retired, or if tests specific to it are
> ever disabled by default? As such, it seems like the alternative "fix"
> you mention above would be preferable to ensure that coverage of
> git-config doesn't get diluted.
>
> Or am I misunderstanding something?
A valid remark indeed, even though this is thinking quite far into the
future. I'll investigate how much of a pain it would be to instead "do
the right thing" and retain the repositroy format version as well as
extensions.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 1/6] t1300: mark tests to require default repo format
From: Patrick Steinhardt @ 2024-01-10 7:15 UTC (permalink / raw)
To: Taylor Blau; +Cc: git
In-Reply-To: <ZZ2TdYlcrLN9zckR@nand.local>
[-- Attachment #1: Type: text/plain, Size: 3303 bytes --]
On Tue, Jan 09, 2024 at 01:41:57PM -0500, Taylor Blau wrote:
> On Tue, Jan 09, 2024 at 01:17:04PM +0100, Patrick Steinhardt wrote:
> > The t1300 test suite exercises the git-config(1) tool. To do so we
> > overwrite ".git/config" to contain custom contents. While this is easy
> > enough to do, it may create problems when using a non-default repository
> > format because we also overwrite the repository format version as well
> > as any potential extensions.
> >
> > Mark these tests with the DEFAULT_REPO_FORMAT prerequisite to avoid the
> > problem. An alternative would be to carry over mandatory config keys
> > into the rewritten config file. But the effort does not seem worth it
> > given that the system under test is git-config(1), which is at a lower
> > level than the repository format.
>
> I think I am missing something obvious here ;-).
>
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > t/t1300-config.sh | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> > index f4e2752134..1e953a0fc2 100755
> > --- a/t/t1300-config.sh
> > +++ b/t/t1300-config.sh
> > @@ -1098,7 +1098,7 @@ test_expect_success SYMLINKS 'symlink to nonexistent configuration' '
> > test_must_fail git config --file=linktolinktonada --list
> > '
> >
> > -test_expect_success 'check split_cmdline return' "
> > +test_expect_success DEFAULT_REPO_FORMAT 'check split_cmdline return' "
> > git config alias.split-cmdline-fix 'echo \"' &&
> > test_must_fail git split-cmdline-fix &&
> > echo foo > foo &&
> > @@ -1156,7 +1156,7 @@ test_expect_success 'git -c works with aliases of builtins' '
> > test_cmp expect actual
> > '
>
> Looking at this first test, for example, I see two places where we
> modify the configuration file:
>
> - git config alias.split-cmdline-fix 'echo \"'
> - git config branch.main.mergeoptions 'echo \"'
>
> I think I am missing some detail about why we can't do this when we have
> extensions enabled?
The issue is not directly visible in the tests I'm amending here, but
happens in the setup code. What we do is to overwrite the repository's
config like this:
```
cat > .git/config << EOF
[beta] ; silly comment # another comment
noIndent= sillyValue ; 'nother silly comment
# empty line
; comment
haha ="beta" # last silly comment
haha = hello
haha = bello
[nextSection] noNewline = ouch
EOF
```
The problem here is that we drop any extensions that the repository has
been initialized with originally. This seems to work alright in the
context of SHA256 repositories. But with the reftable backend this
pattern will cause test failures because the discarded "refStorage"
extension will make us assume that the repostiory uses the "files"
backend instead of the "reftable" backend. And that starts to go
downhill quite fast when trying to read or write refs.
A "proper" fix for this issue would be to rewrite tests such that we
know to retain those extensions. But I'm just not sure whether that is
really worth it, mostly because the system under test is at a lower
level and thus shouldn't care about repository extensions. After all,
extensions build on top of our config code.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: ps/reftable-optimize-io (was: What's cooking in git.git (Jan 2024, #03; Tue, 9))
From: Patrick Steinhardt @ 2024-01-10 6:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqsf36dotl.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]
On Tue, Jan 09, 2024 at 04:17:26PM -0800, Junio C Hamano wrote:
> * ps/reftable-optimize-io (2024-01-08) 4 commits
> - reftable/blocksource: use mmap to read tables
> - reftable/stack: use stat info to avoid re-reading stack list
> - reftable/stack: refactor reloading to use file descriptor
> - reftable/stack: refactor stack reloading to have common exit path
>
> Low-level I/O optimization for reftable.
>
> Will merge to 'next'?
> source: <cover.1704714575.git.ps@pks.im>
Let's wait a few days for reviews. I don't expect there to be a ton of
reviews from the usual contributors due to the still-limited knowledge
around reftables in our community. But I have been trying to rope in
fellow team members at GitLab to get their feet wet with reviewing
topics on the Git mailing list directly so that there is at least some
more pairs of eyes for the reftable-related topics (and eventually more
people who start to contribute to Git regularly).
Also, the optimizations I'm doing in the reftable library are generally
not part of the critical path to get the backend itself upstream, so I
don't mind waiting a bit longer for those to land.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 10/10] trailer: delete obsolete argument handling code from API
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
This commit was not squashed with its parent in order to keep the diff
separate (to make the additive changes in the parent easier to read).
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 39 ---------------------------------------
trailer.h | 17 -----------------
2 files changed, 56 deletions(-)
diff --git a/trailer.c b/trailer.c
index 0a86e0d5afa..27bb2195f53 100644
--- a/trailer.c
+++ b/trailer.c
@@ -745,45 +745,6 @@ void parse_trailers_from_config(struct list_head *config_head)
}
}
-void parse_trailers_from_command_line_args(struct list_head *arg_head,
- struct list_head *new_trailer_head)
-{
- struct strbuf tok = STRBUF_INIT;
- struct strbuf val = STRBUF_INIT;
- const struct trailer_conf *conf;
- struct list_head *pos;
-
- /*
- * In command-line arguments, '=' is accepted (in addition to the
- * separators that are defined).
- */
- char *cl_separators = xstrfmt("=%s", separators);
-
- /* Add an arg item for each trailer on the command line */
- list_for_each(pos, new_trailer_head) {
- struct new_trailer_item *tr =
- list_entry(pos, struct new_trailer_item, list);
- ssize_t separator_pos = find_separator(tr->text, cl_separators);
-
- if (separator_pos == 0) {
- struct strbuf sb = STRBUF_INIT;
- strbuf_addstr(&sb, tr->text);
- strbuf_trim(&sb);
- error(_("empty trailer token in trailer '%.*s'"),
- (int) sb.len, sb.buf);
- strbuf_release(&sb);
- } else {
- parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
- add_arg_item(strbuf_detach(&tok, NULL),
- strbuf_detach(&val, NULL),
- conf,
- arg_head);
- }
- }
-
- free(cl_separators);
-}
-
static const char *next_line(const char *str)
{
const char *nl = strchrnul(str, '\n');
diff --git a/trailer.h b/trailer.h
index 9b86acfe2d4..8a89e95c171 100644
--- a/trailer.h
+++ b/trailer.h
@@ -32,20 +32,6 @@ int trailer_set_where(enum trailer_where *item, const char *value);
int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
-/*
- * A list that represents newly-added trailers, such as those provided
- * with the --trailer command line option of git-interpret-trailers.
- */
-struct new_trailer_item {
- struct list_head list;
-
- const char *text;
-
- enum trailer_where where;
- enum trailer_if_exists if_exists;
- enum trailer_if_missing if_missing;
-};
-
void trailer_conf_set(enum trailer_where where,
enum trailer_if_exists if_exists,
enum trailer_if_missing if_missing,
@@ -79,9 +65,6 @@ struct process_trailer_options {
void parse_trailers_from_config(struct list_head *config_head);
-void parse_trailers_from_command_line_args(struct list_head *arg_head,
- struct list_head *new_trailer_head);
-
void process_trailers_lists(struct list_head *head,
struct list_head *arg_head);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 09/10] trailer: move arg handling to interpret-trailers.c
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
We don't move the "arg_item" struct to interpret-trailers.c, because it
is now a struct that contains information about trailers that should be
injected into the input text's own trailers. We will rename this
language as such in a follow-up patch to keep the diff here small.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 88 ++++++++++++++++++++++--------------
trailer.c | 63 +++++++++++++++++++-------
trailer.h | 12 +++++
3 files changed, 113 insertions(+), 50 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 42d9ca07a56..4da4eac3b46 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -45,23 +45,17 @@ static int option_parse_if_missing(const struct option *opt,
return trailer_set_if_missing(opt->value, arg);
}
-static void new_trailers_clear(struct list_head *trailers)
-{
- struct list_head *pos, *tmp;
- struct new_trailer_item *item;
-
- list_for_each_safe(pos, tmp, trailers) {
- item = list_entry(pos, struct new_trailer_item, list);
- list_del(pos);
- free(item);
- }
-}
+static char *cl_separators;
static int option_parse_trailer(const struct option *opt,
const char *arg, int unset)
{
struct list_head *trailers = opt->value;
- struct new_trailer_item *item;
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ const struct trailer_conf *conf;
+ struct trailer_conf *conf_current = new_trailer_conf();
+ ssize_t separator_pos;
if (unset) {
new_trailers_clear(trailers);
@@ -71,12 +65,31 @@ static int option_parse_trailer(const struct option *opt,
if (!arg)
return -1;
- item = xmalloc(sizeof(*item));
- item->text = arg;
- item->where = where;
- item->if_exists = if_exists;
- item->if_missing = if_missing;
- list_add_tail(&item->list, trailers);
+ separator_pos = find_separator(arg, cl_separators);
+ if (separator_pos) {
+ parse_trailer(arg, separator_pos, &tok, &val, &conf);
+ duplicate_trailer_conf(conf_current, conf);
+
+ /*
+ * Override conf_current with settings specified via CLI flags.
+ */
+ trailer_conf_set(where, if_exists, if_missing, conf_current);
+
+ add_arg_item(strbuf_detach(&tok, NULL),
+ strbuf_detach(&val, NULL),
+ conf_current,
+ trailers);
+ } else {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_addstr(&sb, arg);
+ strbuf_trim(&sb);
+ error(_("empty trailer token in trailer '%.*s'"),
+ (int) sb.len, sb.buf);
+ strbuf_release(&sb);
+ }
+
+ free(conf_current);
+
return 0;
}
@@ -136,7 +149,7 @@ static void read_input_file(struct strbuf *sb, const char *file)
static void interpret_trailers(const char *file,
const struct process_trailer_options *opts,
- struct list_head *new_trailer_head)
+ struct list_head *arg_trailers)
{
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
@@ -144,8 +157,6 @@ static void interpret_trailers(const char *file,
struct trailer_block *trailer_block;
FILE *outfile = stdout;
- trailer_config_init();
-
read_input_file(&sb, file);
if (opts->in_place)
@@ -162,12 +173,7 @@ static void interpret_trailers(const char *file,
if (!opts->only_input) {
- LIST_HEAD(config_head);
- LIST_HEAD(arg_head);
- parse_trailers_from_config(&config_head);
- parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
- list_splice(&config_head, &arg_head);
- process_trailers_lists(&head, &arg_head);
+ process_trailers_lists(&head, arg_trailers);
}
/* Print trailer block. */
@@ -193,7 +199,8 @@ static void interpret_trailers(const char *file,
int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
- LIST_HEAD(trailers);
+ LIST_HEAD(configured_trailers);
+ LIST_HEAD(arg_trailers);
struct option options[] = {
OPT_BOOL(0, "in-place", &opts.in_place, N_("edit files in place")),
@@ -212,33 +219,48 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("alias for --only-trailers --only-input --unfold"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
- OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
+ OPT_CALLBACK(0, "trailer", &arg_trailers, N_("trailer"),
N_("trailer(s) to add"), option_parse_trailer),
OPT_END()
};
git_config(git_default_config, NULL);
+ trailer_config_init();
+
+ if (!opts.only_input) {
+ parse_trailers_from_config(&configured_trailers);
+ }
+
+ /*
+ * In command-line arguments, '=' is accepted (in addition to the
+ * separators that are defined).
+ */
+ cl_separators = xstrfmt("=%s", default_separators());
argc = parse_options(argc, argv, prefix, options,
git_interpret_trailers_usage, 0);
- if (opts.only_input && !list_empty(&trailers))
+ free(cl_separators);
+
+ if (opts.only_input && !list_empty(&arg_trailers))
usage_msg_opt(
_("--trailer with --only-input does not make sense"),
git_interpret_trailers_usage,
options);
+ list_splice(&configured_trailers, &arg_trailers);
+
if (argc) {
int i;
for (i = 0; i < argc; i++)
- interpret_trailers(argv[i], &opts, &trailers);
+ interpret_trailers(argv[i], &opts, &arg_trailers);
} else {
if (opts.in_place)
die(_("no input file given for in-place editing"));
- interpret_trailers(NULL, &opts, &trailers);
+ interpret_trailers(NULL, &opts, &arg_trailers);
}
- new_trailers_clear(&trailers);
+ new_trailers_clear(&arg_trailers);
return 0;
}
diff --git a/trailer.c b/trailer.c
index e2d541372a3..0a86e0d5afa 100644
--- a/trailer.c
+++ b/trailer.c
@@ -66,6 +66,11 @@ static LIST_HEAD(conf_head);
static char *separators = ":";
+const char *default_separators(void)
+{
+ return separators;
+}
+
static int configured;
#define TRAILER_ARG_STRING "$ARG"
@@ -424,6 +429,25 @@ int trailer_set_if_missing(enum trailer_if_missing *item, const char *value)
return 0;
}
+void trailer_conf_set(enum trailer_where where,
+ enum trailer_if_exists if_exists,
+ enum trailer_if_missing if_missing,
+ struct trailer_conf *conf)
+{
+ if (where != WHERE_DEFAULT)
+ conf->where = where;
+ if (if_exists != EXISTS_DEFAULT)
+ conf->if_exists = if_exists;
+ if (if_missing != MISSING_DEFAULT)
+ conf->if_missing = if_missing;
+}
+
+struct trailer_conf *new_trailer_conf(void)
+{
+ struct trailer_conf *new = xcalloc(1, sizeof(*new));
+ return new;
+}
+
void duplicate_trailer_conf(struct trailer_conf *dst,
const struct trailer_conf *src)
{
@@ -642,6 +666,9 @@ ssize_t find_separator(const char *line, const char *separators)
/*
* Obtain the token, value, and conf from the given trailer.
*
+ * The conf needs special handling. We first read hardcoded defaults, and
+ * override them if we find a matching trailer configuration in the config.
+ *
* separator_pos must not be 0, since the token cannot be an empty string.
*
* If separator_pos is -1, interpret the whole trailer as a token.
@@ -691,22 +718,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 trailer_conf *conf,
- const struct new_trailer_item *new_trailer_item)
+void add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
+ struct list_head *arg_head)
+
{
struct arg_item *new_item = xcalloc(1, sizeof(*new_item));
new_item->token = tok;
new_item->value = val;
duplicate_trailer_conf(&new_item->conf, conf);
- if (new_trailer_item) {
- if (new_trailer_item->where != WHERE_DEFAULT)
- new_item->conf.where = new_trailer_item->where;
- if (new_trailer_item->if_exists != EXISTS_DEFAULT)
- new_item->conf.if_exists = new_trailer_item->if_exists;
- if (new_trailer_item->if_missing != MISSING_DEFAULT)
- new_item->conf.if_missing = new_trailer_item->if_missing;
- }
list_add_tail(&new_item->list, arg_head);
}
@@ -719,10 +738,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)),
+ add_arg_item(xstrdup(token_from_item(item, NULL)),
xstrdup(""),
- &item->conf, NULL);
+ &item->conf,
+ config_head);
}
}
@@ -755,10 +774,10 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
strbuf_release(&sb);
} else {
parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
- add_arg_item(arg_head,
- strbuf_detach(&tok, NULL),
+ add_arg_item(strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL),
- conf, tr);
+ conf,
+ arg_head);
}
}
@@ -1148,6 +1167,16 @@ void free_trailers(struct list_head *head)
}
}
+void new_trailers_clear(struct list_head *trailers)
+{
+ struct list_head *pos, *p;
+
+ list_for_each_safe(pos, p, trailers) {
+ list_del(pos);
+ free_arg_item(list_entry(pos, struct arg_item, list));
+ }
+}
+
size_t trailer_block_start(struct trailer_block *trailer_block)
{
return trailer_block->start;
diff --git a/trailer.h b/trailer.h
index fe49a9bad52..9b86acfe2d4 100644
--- a/trailer.h
+++ b/trailer.h
@@ -46,9 +46,20 @@ struct new_trailer_item {
enum trailer_if_missing if_missing;
};
+void trailer_conf_set(enum trailer_where where,
+ enum trailer_if_exists if_exists,
+ enum trailer_if_missing if_missing,
+ struct trailer_conf *conf);
+
+struct trailer_conf *new_trailer_conf(void);
void duplicate_trailer_conf(struct trailer_conf *dst,
const struct trailer_conf *src);
+const char *default_separators(void);
+
+void add_arg_item(char *tok, char *val, const struct trailer_conf *conf,
+ struct list_head *arg_head);
+
struct process_trailer_options {
int in_place;
int trim_empty;
@@ -92,6 +103,7 @@ void trailer_block_release(struct trailer_block *trailer_block);
void trailer_config_init(void);
void free_trailers(struct list_head *trailers);
+void new_trailers_clear(struct list_head *trailers);
void format_trailers(struct list_head *head,
const struct process_trailer_options *opts,
--
gitgitgadget
^ permalink raw reply related
* [PATCH 08/10] trailer: prepare to move parse_trailers_from_command_line_args() to builtin
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Expose more functions in the trailer.h API, in preparation for moving
out
parse_trailers_from_command_line_args()
to interpret-trailer.c, because the trailer API should not be concerned
with command line arguments (as it has nothing to do with trailers
themselves). The interpret-trailers builtin is the only user of the
above function.
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 66 +++++++++++++++++++++++++++----------------------------
trailer.h | 10 +++++++++
2 files changed, 42 insertions(+), 34 deletions(-)
diff --git a/trailer.c b/trailer.c
index 360e76376b8..e2d541372a3 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)) {
@@ -691,13 +692,13 @@ static struct trailer_item *add_trailer_item(struct list_head *head, char *tok,
}
static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
- const struct conf_info *conf,
+ const struct trailer_conf *conf,
const struct new_trailer_item *new_trailer_item)
{
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;
@@ -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,8 +754,7 @@ 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);
+ parse_trailer(tr->text, separator_pos, &tok, &val, &conf);
add_arg_item(arg_head,
strbuf_detach(&tok, NULL),
strbuf_detach(&val, NULL),
@@ -1116,20 +1116,19 @@ struct trailer_block *parse_trailers(const char *str,
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,
@@ -1217,8 +1216,7 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
strbuf_addstr(&iter->raw, line);
strbuf_reset(&iter->key);
strbuf_reset(&iter->val);
- parse_trailer(&iter->key, &iter->val, NULL,
- line, separator_pos);
+ parse_trailer(line, separator_pos, &iter->key, &iter->val, NULL);
unfold_value(&iter->val);
return 1;
}
diff --git a/trailer.h b/trailer.h
index 5c8503ade78..fe49a9bad52 100644
--- a/trailer.h
+++ b/trailer.h
@@ -5,6 +5,7 @@
#include "strbuf.h"
struct trailer_block;
+struct trailer_conf;
enum trailer_where {
WHERE_DEFAULT,
@@ -45,6 +46,9 @@ struct new_trailer_item {
enum trailer_if_missing if_missing;
};
+void duplicate_trailer_conf(struct trailer_conf *dst,
+ const struct trailer_conf *src);
+
struct process_trailer_options {
int in_place;
int trim_empty;
@@ -70,6 +74,12 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
void process_trailers_lists(struct list_head *head,
struct list_head *arg_head);
+ssize_t find_separator(const char *line, const char *separators);
+
+void parse_trailer(const char *line, ssize_t separator_pos,
+ struct strbuf *tok, struct strbuf *val,
+ const struct trailer_conf **conf);
+
struct trailer_block *parse_trailers(const char *str,
const struct process_trailer_options *opts,
struct list_head *head);
--
gitgitgadget
^ permalink raw reply related
* [PATCH 07/10] trailer: spread usage of "trailer_block" language
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Deprecate the "trailer_info" struct name and replace it with
"trailer_block". The main reason is to help readability, because
"trailer_info" on the surface sounds like it's about a single trailer
when in reality it is a collection of contiguous lines, at least 25% of
which are trailers.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 26 +++++-----
trailer.c | 99 ++++++++++++++++++------------------
trailer.h | 18 +++----
3 files changed, 71 insertions(+), 72 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 0838a57e157..42d9ca07a56 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -140,8 +140,8 @@ static void interpret_trailers(const char *file,
{
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 char *file,
if (opts->in_place)
outfile = create_in_place_tempfile(file);
- info = parse_trailers(sb.buf, &head, opts);
+ trailer_block = parse_trailers(sb.buf, opts, &head);
- /* Print the lines before the trailers */
+ /* Print the lines before the trailer block */
if (!opts->only_trailers)
- fwrite(sb.buf, 1, trailer_block_start(info), outfile);
+ fwrite(sb.buf, 1, trailer_block_start(trailer_block), outfile);
- if (!opts->only_trailers && !blank_line_before_trailer_block(info))
+ if (!opts->only_trailers && !blank_line_before_trailer_block(trailer_block))
fprintf(outfile, "\n");
@@ -171,17 +171,17 @@ static void interpret_trailers(const char *file,
}
/* Print trailer block. */
- format_trailers(&head, opts, &trailer_block);
- fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
- strbuf_release(&trailer_block);
+ format_trailers(&head, opts, &tb);
+ fwrite(tb.buf, 1, tb.len, outfile);
+ strbuf_release(&tb);
free_trailers(&head);
- /* Print the lines after the trailers as is */
+ /* Print the lines after the trailer block as is */
if (!opts->only_trailers)
- fwrite(sb.buf + trailer_block_end(info), 1, sb.len - trailer_block_end(info), outfile);
-
- trailer_info_release(info);
+ fwrite(sb.buf + trailer_block_end(trailer_block),
+ 1, sb.len - trailer_block_end(trailer_block), outfile);
+ trailer_block_release(trailer_block);
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
diff --git a/trailer.c b/trailer.c
index 0c66e2d3812..360e76376b8 100644
--- a/trailer.c
+++ b/trailer.c
@@ -11,19 +11,20 @@
* Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
*/
-struct trailer_info {
+struct trailer_block {
/*
* True if there is a blank line before the location pointed to by
- * trailer_block_start.
+ * "start".
*/
int blank_line_before_trailer;
/*
- * Offsets to the trailer block start and end positions in the input
- * string. If no trailer block is found, these are both set to the
- * "true" end of the input (find_end_of_log_message()).
+ * The locations of the start and end positions of the trailer block
+ * found, as offsets from the beginning of the source text from which
+ * this trailer block was parsed. If no trailer block is found, these
+ * are both set to 0.
*/
- size_t trailer_block_start, trailer_block_end;
+ size_t start, end;
/*
* Array of trailers found.
@@ -1046,16 +1047,16 @@ void format_trailers(struct list_head *head,
}
}
-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 char *str,
- const struct process_trailer_options *opts)
+static struct trailer_block *trailer_block_get(const char *str,
+ const struct process_trailer_options *opts)
{
- struct trailer_info *info = trailer_info_new();
+ struct trailer_block *trailer_block = trailer_block_new();
size_t end_of_log_message = 0, trailer_block_start = 0;
struct strbuf **trailer_lines, **ptr;
char **trailer_strings = NULL;
@@ -1088,34 +1089,34 @@ static struct trailer_info *trailer_info_get(const char *str,
}
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 char *str,
- struct list_head *head,
- const struct process_trailer_options *opts)
+struct trailer_block *parse_trailers(const char *str,
+ const struct process_trailer_options *opts,
+ 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(str, opts);
+ trailer_block = trailer_block_get(str, opts);
- for (i = 0; i < info->trailer_nr; i++) {
+ for (i = 0; i < trailer_block->trailer_nr; i++) {
int separator_pos;
- char *trailer = info->trailers[i];
+ char *trailer = trailer_block->trailers[i];
if (trailer[0] == comment_line_char)
continue;
separator_pos = find_separator(trailer, separators);
@@ -1136,7 +1137,7 @@ struct trailer_info *parse_trailers(const char *str,
}
}
- return info;
+ return trailer_block;
}
void free_trailers(struct list_head *head)
@@ -1148,28 +1149,28 @@ void free_trailers(struct list_head *head)
}
}
-size_t trailer_block_start(struct trailer_info *info)
+size_t trailer_block_start(struct trailer_block *trailer_block)
{
- return info->trailer_block_start;
+ return trailer_block->start;
}
-size_t trailer_block_end(struct trailer_info *info)
+size_t trailer_block_end(struct trailer_block *trailer_block)
{
- return info->trailer_block_end;
+ return trailer_block->end;
}
-int blank_line_before_trailer_block(struct trailer_info *info)
+int blank_line_before_trailer_block(struct trailer_block *trailer_block)
{
- return info->blank_line_before_trailer;
+ return trailer_block->blank_line_before_trailer;
}
-void trailer_info_release(struct trailer_info *info)
+void trailer_block_release(struct trailer_block *trailer_block)
{
size_t i;
- for (i = 0; i < info->trailer_nr; i++)
- free(info->trailers[i]);
- free(info->trailers);
- free(info);
+ for (i = 0; i < trailer_block->trailer_nr; i++)
+ free(trailer_block->trailers[i]);
+ free(trailer_block->trailers);
+ free(trailer_block);
}
void format_trailers_from_commit(const char *msg,
@@ -1177,31 +1178,29 @@ void format_trailers_from_commit(const char *msg,
struct strbuf *out)
{
LIST_HEAD(head);
- struct trailer_info *info = parse_trailers(msg, &head, opts);
+ struct trailer_block *trailer_block = parse_trailers(msg, opts, &head);
/* If we want the whole block untouched, we can take the fast path. */
if (!opts->only_trailers && !opts->unfold && !opts->filter &&
!opts->separator && !opts->key_only && !opts->value_only &&
!opts->key_value_separator) {
- strbuf_add(out, msg + info->trailer_block_start,
- info->trailer_block_end - info->trailer_block_start);
+ strbuf_add(out, msg + trailer_block->start,
+ trailer_block->end - trailer_block->start);
} else
format_trailers(&head, opts, out);
free_trailers(&head);
- trailer_info_release(info);
+ trailer_block_release(trailer_block);
}
void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
- struct trailer_info *internal = trailer_info_new();
strbuf_init(&iter->key, 0);
strbuf_init(&iter->val, 0);
strbuf_init(&iter->raw, 0);
opts.no_divider = 1;
- iter->internal.info = internal;
- iter->internal.info = trailer_info_get(msg, &opts);
+ iter->internal.trailer_block = trailer_block_get(msg, &opts);
iter->internal.cur = 0;
}
@@ -1209,8 +1208,8 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
{
char *line;
int separator_pos;
- if (iter->internal.cur < iter->internal.info->trailer_nr) {
- line = iter->internal.info->trailers[iter->internal.cur++];
+ if (iter->internal.cur < iter->internal.trailer_block->trailer_nr) {
+ line = iter->internal.trailer_block->trailers[iter->internal.cur++];
separator_pos = find_separator(line, separators);
iter->is_trailer = (separator_pos > 0);
@@ -1228,7 +1227,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);
strbuf_release(&iter->raw);
diff --git a/trailer.h b/trailer.h
index b06da1a7d3a..5c8503ade78 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 char *str,
- struct list_head *head,
- const struct process_trailer_options *opts);
+struct trailer_block *parse_trailers(const char *str,
+ const struct process_trailer_options *opts,
+ struct list_head *head);
-size_t trailer_block_start(struct trailer_info *info);
-size_t trailer_block_end(struct trailer_info *info);
-int blank_line_before_trailer_block(struct trailer_info *info);
+size_t trailer_block_start(struct trailer_block *trailer_block);
+size_t trailer_block_end(struct trailer_block *trailer_block);
+int blank_line_before_trailer_block(struct trailer_block *trailer_block);
-void trailer_info_release(struct trailer_info *info);
+void trailer_block_release(struct trailer_block *trailer_block);
void trailer_config_init(void);
void free_trailers(struct list_head *trailers);
@@ -123,7 +123,7 @@ struct trailer_iterator {
/* private */
struct {
- struct trailer_info *info;
+ struct trailer_block *trailer_block;
size_t cur;
} internal;
};
--
gitgitgadget
^ permalink raw reply related
* [PATCH 06/10] trailer: make trailer_info struct private
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
In 13211ae23f (trailer: separate public from internal portion of
trailer_iterator, 2023-09-09) we moved trailer_info behind an anonymous
struct to discourage use by trailer.h API users. However it still left
open the possibility of external use of trailer_info itself. Now that
there are no external users of trailer_info, we can make this struct
private.
Make this struct private by putting its definition inside trailer.c.
This has two benefits:
(1) it makes the surface area of the public facing interface (trailer.h)
smaller, and
(2) external API users are unable to peer inside this struct (because it
is only ever exposed as an opaque pointer).
This change exposes some deficiencies in the API, mainly with regard to
information about the location of the trailer block that was parsed.
Expose new API functions to access this information (needed by
builtin/interpret-trailers.c).
The idea in this patch to hide implementation details behind an "opaque
pointer" is also known as the "pimpl" (pointer to implementation) idiom
in C++ and is a common pattern in that language (where, for example,
abstract classes only have pointers to concrete classes).
However, the original inspiration to use this idiom does not come from
C++, but instead the book "C Interfaces and Implementations: Techniques
for Creating Reusable Software" [1]. This book recommends opaque
pointers as a good design principle for designing C libraries, using the
term "interface" as the functions defined in *.h (header) files and
"implementation" as the corresponding *.c file which define the
interfaces.
The book says this about opaque pointers:
... clients can manipulate such pointers freely, but they can’t
dereference them; that is, they can’t look at the innards of the
structure pointed to by them. Only the implementation has that
privilege. Opaque pointers hide representation details and help
catch errors.
In our case, "struct trailer_info" is now hidden from clients, and the
ways in which this opaque pointer can be used is limited to the richness
of the trailer.h file. In other words, trailer.h exclusively controls
exactly how "trailer_info" pointers are to be used.
[1] Hanson, David R. "C Interfaces and Implementations: Techniques for
Creating Reusable Software". Addison Wesley, 1997. p. 22
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 13 +--
trailer.c | 154 +++++++++++++++++++++++------------
trailer.h | 37 ++-------
3 files changed, 117 insertions(+), 87 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 934833a4645..0838a57e157 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -141,7 +141,7 @@ static void interpret_trailers(const char *file,
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 char *file,
if (opts->in_place)
outfile = create_in_place_tempfile(file);
- parse_trailers(&info, sb.buf, &head, opts);
+ info = parse_trailers(sb.buf, &head, opts);
/* Print the lines before the trailers */
if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+ fwrite(sb.buf, 1, trailer_block_start(info), outfile);
- if (!opts->only_trailers && !info.blank_line_before_trailer)
+ if (!opts->only_trailers && !blank_line_before_trailer_block(info))
fprintf(outfile, "\n");
@@ -176,11 +176,12 @@ static void interpret_trailers(const char *file,
strbuf_release(&trailer_block);
free_trailers(&head);
- trailer_info_release(&info);
/* Print the lines after the trailers as is */
if (!opts->only_trailers)
- fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+ fwrite(sb.buf + trailer_block_end(info), 1, sb.len - trailer_block_end(info), outfile);
+
+ trailer_info_release(info);
if (opts->in_place)
if (rename_tempfile(&trailers_tempfile, file))
diff --git a/trailer.c b/trailer.c
index 593717fd56c..0c66e2d3812 100644
--- a/trailer.c
+++ b/trailer.c
@@ -11,6 +11,27 @@
* Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
*/
+struct trailer_info {
+ /*
+ * True if there is a blank line before the location pointed to by
+ * trailer_block_start.
+ */
+ int blank_line_before_trailer;
+
+ /*
+ * Offsets to the trailer block start and end positions in the input
+ * string. If no trailer block is found, these are both set to the
+ * "true" end of the input (find_end_of_log_message()).
+ */
+ size_t trailer_block_start, trailer_block_end;
+
+ /*
+ * Array of trailers found.
+ */
+ char **trailers;
+ size_t trailer_nr;
+};
+
struct conf_info {
char *name;
char *key;
@@ -1025,20 +1046,72 @@ void format_trailers(struct list_head *head,
}
}
+static struct trailer_info *trailer_info_new(void)
+{
+ struct trailer_info *info = xcalloc(1, sizeof(*info));
+ return info;
+}
+
+static struct trailer_info *trailer_info_get(const char *str,
+ const struct process_trailer_options *opts)
+{
+ struct trailer_info *info = trailer_info_new();
+ size_t end_of_log_message = 0, trailer_block_start = 0;
+ struct strbuf **trailer_lines, **ptr;
+ char **trailer_strings = NULL;
+ size_t nr = 0, alloc = 0;
+ char **last = NULL;
+
+ trailer_config_init();
+
+ end_of_log_message = find_end_of_log_message(str, opts->no_divider);
+ trailer_block_start = find_trailer_block_start(str, end_of_log_message);
+
+ trailer_lines = strbuf_split_buf(str + trailer_block_start,
+ end_of_log_message - trailer_block_start,
+ '\n',
+ 0);
+ for (ptr = trailer_lines; *ptr; ptr++) {
+ if (last && isspace((*ptr)->buf[0])) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
+ strbuf_addbuf(&sb, *ptr);
+ *last = strbuf_detach(&sb, NULL);
+ continue;
+ }
+ ALLOC_GROW(trailer_strings, nr + 1, alloc);
+ trailer_strings[nr] = strbuf_detach(*ptr, NULL);
+ last = find_separator(trailer_strings[nr], separators) >= 1
+ ? &trailer_strings[nr]
+ : NULL;
+ nr++;
+ }
+ strbuf_list_free(trailer_lines);
+
+ info->blank_line_before_trailer = ends_with_blank_line(str,
+ trailer_block_start);
+ info->trailer_block_start = trailer_block_start;
+ info->trailer_block_end = end_of_log_message;
+ info->trailers = trailer_strings;
+ info->trailer_nr = nr;
+
+ return info;
+}
+
/*
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
*/
-void parse_trailers(struct trailer_info *info,
- const char *str,
- struct list_head *head,
- const struct process_trailer_options *opts)
+struct trailer_info *parse_trailers(const char *str,
+ struct list_head *head,
+ const struct process_trailer_options *opts)
{
+ struct trailer_info *info;
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
size_t i;
- trailer_info_get(info, str, opts);
+ info = trailer_info_get(str, opts);
for (i = 0; i < info->trailer_nr; i++) {
int separator_pos;
@@ -1062,6 +1135,8 @@ void parse_trailers(struct trailer_info *info,
strbuf_detach(&val, NULL));
}
}
+
+ return info;
}
void free_trailers(struct list_head *head)
@@ -1073,47 +1148,19 @@ void free_trailers(struct list_head *head)
}
}
-void trailer_info_get(struct trailer_info *info, const char *str,
- const struct process_trailer_options *opts)
+size_t trailer_block_start(struct trailer_info *info)
{
- size_t end_of_log_message = 0, trailer_block_start = 0;
- struct strbuf **trailer_lines, **ptr;
- char **trailer_strings = NULL;
- size_t nr = 0, alloc = 0;
- char **last = NULL;
-
- trailer_config_init();
-
- end_of_log_message = find_end_of_log_message(str, opts->no_divider);
- trailer_block_start = find_trailer_block_start(str, end_of_log_message);
+ return info->trailer_block_start;
+}
- trailer_lines = strbuf_split_buf(str + trailer_block_start,
- end_of_log_message - trailer_block_start,
- '\n',
- 0);
- for (ptr = trailer_lines; *ptr; ptr++) {
- if (last && isspace((*ptr)->buf[0])) {
- struct strbuf sb = STRBUF_INIT;
- strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
- strbuf_addbuf(&sb, *ptr);
- *last = strbuf_detach(&sb, NULL);
- continue;
- }
- ALLOC_GROW(trailer_strings, nr + 1, alloc);
- trailer_strings[nr] = strbuf_detach(*ptr, NULL);
- last = find_separator(trailer_strings[nr], separators) >= 1
- ? &trailer_strings[nr]
- : NULL;
- nr++;
- }
- strbuf_list_free(trailer_lines);
+size_t trailer_block_end(struct trailer_info *info)
+{
+ return info->trailer_block_end;
+}
- info->blank_line_before_trailer = ends_with_blank_line(str,
- trailer_block_start);
- info->trailer_block_start = trailer_block_start;
- info->trailer_block_end = end_of_log_message;
- info->trailers = trailer_strings;
- info->trailer_nr = nr;
+int blank_line_before_trailer_block(struct trailer_info *info)
+{
+ return info->blank_line_before_trailer;
}
void trailer_info_release(struct trailer_info *info)
@@ -1122,6 +1169,7 @@ void trailer_info_release(struct trailer_info *info)
for (i = 0; i < info->trailer_nr; i++)
free(info->trailers[i]);
free(info->trailers);
+ free(info);
}
void format_trailers_from_commit(const char *msg,
@@ -1129,31 +1177,31 @@ void format_trailers_from_commit(const char *msg,
struct strbuf *out)
{
LIST_HEAD(head);
- struct trailer_info info;
-
- parse_trailers(&info, msg, &head, opts);
+ struct trailer_info *info = parse_trailers(msg, &head, opts);
/* 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(&head, opts, out);
free_trailers(&head);
- trailer_info_release(&info);
+ trailer_info_release(info);
}
void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
+ struct trailer_info *internal = trailer_info_new();
strbuf_init(&iter->key, 0);
strbuf_init(&iter->val, 0);
strbuf_init(&iter->raw, 0);
opts.no_divider = 1;
- trailer_info_get(&iter->internal.info, msg, &opts);
+ iter->internal.info = internal;
+ iter->internal.info = trailer_info_get(msg, &opts);
iter->internal.cur = 0;
}
@@ -1161,8 +1209,8 @@ int trailer_iterator_advance(struct trailer_iterator *iter)
{
char *line;
int separator_pos;
- if (iter->internal.cur < iter->internal.info.trailer_nr) {
- line = iter->internal.info.trailers[iter->internal.cur++];
+ if (iter->internal.cur < iter->internal.info->trailer_nr) {
+ line = iter->internal.info->trailers[iter->internal.cur++];
separator_pos = find_separator(line, separators);
iter->is_trailer = (separator_pos > 0);
@@ -1180,7 +1228,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);
strbuf_release(&iter->raw);
diff --git a/trailer.h b/trailer.h
index d50c9fd79b2..b06da1a7d3a 100644
--- a/trailer.h
+++ b/trailer.h
@@ -4,6 +4,8 @@
#include "list.h"
#include "strbuf.h"
+struct trailer_info;
+
enum trailer_where {
WHERE_DEFAULT,
WHERE_END,
@@ -29,27 +31,6 @@ int trailer_set_where(enum trailer_where *item, const char *value);
int trailer_set_if_exists(enum trailer_if_exists *item, const char *value);
int trailer_set_if_missing(enum trailer_if_missing *item, const char *value);
-struct trailer_info {
- /*
- * True if there is a blank line before the location pointed to by
- * trailer_block_start.
- */
- int blank_line_before_trailer;
-
- /*
- * Offsets to the trailer block start and end positions in the input
- * string. If no trailer block is found, these are both set to the
- * "true" end of the input (find_end_of_log_message()).
- */
- size_t trailer_block_start, trailer_block_end;
-
- /*
- * Array of trailers found.
- */
- char **trailers;
- size_t trailer_nr;
-};
-
/*
* A list that represents newly-added trailers, such as those provided
* with the --trailer command line option of git-interpret-trailers.
@@ -89,13 +70,13 @@ void parse_trailers_from_command_line_args(struct list_head *arg_head,
void process_trailers_lists(struct list_head *head,
struct list_head *arg_head);
-void parse_trailers(struct trailer_info *info,
- const char *str,
- struct list_head *head,
- const struct process_trailer_options *opts);
+struct trailer_info *parse_trailers(const char *str,
+ struct list_head *head,
+ const struct process_trailer_options *opts);
-void trailer_info_get(struct trailer_info *info, const char *str,
- const struct process_trailer_options *opts);
+size_t trailer_block_start(struct trailer_info *info);
+size_t trailer_block_end(struct trailer_info *info);
+int blank_line_before_trailer_block(struct trailer_info *info);
void trailer_info_release(struct trailer_info *info);
@@ -142,7 +123,7 @@ struct trailer_iterator {
/* private */
struct {
- struct trailer_info info;
+ struct trailer_info *info;
size_t cur;
} internal;
};
--
gitgitgadget
^ permalink raw reply related
* [PATCH 05/10] sequencer: use the trailer iterator
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
This patch allows for the removal of "trailer_info_get()" from the
trailer.h API, which will be in the next patch.
Instead of calling "trailer_info_get()", which is a low-level function
in the trailers implementation (trailer.c), call
trailer_iterator_advance(), which was specifically designed for public
consumption in f0939a0eb1 (trailer: add interface for iterating over
commit trailers, 2020-09-27).
Avoiding "trailer_info_get()" means we don't have to worry about options
like "no_divider" (relevant for parsing trailers). We also don't have to
check for things like "info.trailer_start == info.trailer_end" to see
whether there were any trailers (instead we can just check to see
whether the iterator advanced at all).
Also, teach the iterator about non-trailer lines, by adding a new field
called "raw" to hold both trailer and non-trailer lines. This is
necessary because a "trailer block" is a list of trailer lines of at
least 25% trailers (see 146245063e (trailer: allow non-trailers in
trailer block, 2016-10-21)), such that it may hold non-trailer lines.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/shortlog.c | 7 +++++--
sequencer.c | 35 +++++++++++++++--------------------
trailer.c | 20 ++++++++++++--------
trailer.h | 13 +++++++++++++
4 files changed, 45 insertions(+), 30 deletions(-)
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 1307ed2b88a..dc8fd5a5532 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -172,7 +172,7 @@ static void insert_records_from_trailers(struct shortlog *log,
const char *oneline)
{
struct trailer_iterator iter;
- const char *commit_buffer, *body;
+ const char *commit_buffer, *body, *value;
struct strbuf ident = STRBUF_INIT;
if (!log->trailers.nr)
@@ -190,7 +190,10 @@ static void insert_records_from_trailers(struct shortlog *log,
trailer_iterator_init(&iter, body);
while (trailer_iterator_advance(&iter)) {
- const char *value = iter.val.buf;
+ if (!iter.is_trailer)
+ continue;
+
+ value = iter.val.buf;
if (!string_list_has_string(&log->trailers, iter.key.buf))
continue;
diff --git a/sequencer.c b/sequencer.c
index 3cc88d8a800..d199869cda9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -319,37 +319,32 @@ static const char *get_todo_path(const struct replay_opts *opts)
static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
size_t ignore_footer)
{
- struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
- struct trailer_info info;
- size_t i;
- int found_sob = 0, found_sob_last = 0;
- char saved_char;
-
- opts.no_divider = 1;
+ struct trailer_iterator iter;
+ size_t i = 0, found_sob = 0;
+ char saved_char = sb->buf[sb->len - ignore_footer];
if (ignore_footer) {
- saved_char = sb->buf[sb->len - ignore_footer];
sb->buf[sb->len - ignore_footer] = '\0';
}
- trailer_info_get(&info, sb->buf, &opts);
+ trailer_iterator_init(&iter, sb->buf);
+ while (trailer_iterator_advance(&iter)) {
+ i++;
+ if (sob &&
+ iter.is_trailer &&
+ !strncmp(iter.raw.buf, sob->buf, sob->len)) {
+ found_sob = i;
+ }
+ }
+ trailer_iterator_release(&iter);
if (ignore_footer)
sb->buf[sb->len - ignore_footer] = saved_char;
- if (info.trailer_block_start == info.trailer_block_end)
+ if (!i)
return 0;
- for (i = 0; i < info.trailer_nr; i++)
- if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
- found_sob = 1;
- if (i == info.trailer_nr - 1)
- found_sob_last = 1;
- }
-
- trailer_info_release(&info);
-
- if (found_sob_last)
+ if (found_sob == i)
return 3;
if (found_sob)
return 2;
diff --git a/trailer.c b/trailer.c
index 132f22b3dd7..593717fd56c 100644
--- a/trailer.c
+++ b/trailer.c
@@ -1151,6 +1151,7 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
strbuf_init(&iter->key, 0);
strbuf_init(&iter->val, 0);
+ strbuf_init(&iter->raw, 0);
opts.no_divider = 1;
trailer_info_get(&iter->internal.info, msg, &opts);
iter->internal.cur = 0;
@@ -1158,17 +1159,19 @@ void trailer_iterator_init(struct trailer_iterator *iter, const char *msg)
int trailer_iterator_advance(struct trailer_iterator *iter)
{
- while (iter->internal.cur < iter->internal.info.trailer_nr) {
- char *trailer = iter->internal.info.trailers[iter->internal.cur++];
- int separator_pos = find_separator(trailer, separators);
-
- if (separator_pos < 1)
- continue; /* not a real trailer */
-
+ char *line;
+ int separator_pos;
+ if (iter->internal.cur < iter->internal.info.trailer_nr) {
+ line = iter->internal.info.trailers[iter->internal.cur++];
+ separator_pos = find_separator(line, separators);
+ iter->is_trailer = (separator_pos > 0);
+
+ strbuf_reset(&iter->raw);
+ strbuf_addstr(&iter->raw, line);
strbuf_reset(&iter->key);
strbuf_reset(&iter->val);
parse_trailer(&iter->key, &iter->val, NULL,
- trailer, separator_pos);
+ line, separator_pos);
unfold_value(&iter->val);
return 1;
}
@@ -1180,4 +1183,5 @@ void trailer_iterator_release(struct trailer_iterator *iter)
trailer_info_release(&iter->internal.info);
strbuf_release(&iter->val);
strbuf_release(&iter->key);
+ strbuf_release(&iter->raw);
}
diff --git a/trailer.h b/trailer.h
index 50f70556302..d50c9fd79b2 100644
--- a/trailer.h
+++ b/trailer.h
@@ -127,6 +127,19 @@ struct trailer_iterator {
struct strbuf key;
struct strbuf val;
+ /*
+ * Raw line (e.g., "foo: bar baz") before being parsed as a trailer
+ * key/val pair. This field can contain non-trailer lines because it's
+ * valid for a trailer block to contain such lines (i.e., we only
+ * require 25% of the lines in a trailer block to be trailer lines).
+ */
+ struct strbuf raw;
+
+ /*
+ * 1 if the raw line was parsed as a separate key/val pair.
+ */
+ int is_trailer;
+
/* private */
struct {
struct trailer_info info;
--
gitgitgadget
^ permalink raw reply related
* [PATCH 04/10] trailer: delete obsolete formatting functions
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Signed-off-by: Linus Arver <linusa@google.com>
---
trailer.c | 79 -------------------------------------------------------
1 file changed, 79 deletions(-)
diff --git a/trailer.c b/trailer.c
index 315d90ee1ab..132f22b3dd7 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(FILE *outfile, const char *tok, const char *val)
-{
- char c;
-
- if (!tok) {
- fprintf(outfile, "%s\n", val);
- return;
- }
-
- c = last_non_space_char(tok);
- if (!c)
- return;
- if (strchr(separators, c))
- fprintf(outfile, "%s%s\n", tok, val);
- else
- fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
-}
-
static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
{
struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
@@ -1142,67 +1124,6 @@ void trailer_info_release(struct trailer_info *info)
free(info->trailers);
}
-static void format_trailer_info(struct strbuf *out,
- const struct trailer_info *info,
- const char *msg,
- const struct process_trailer_options *opts)
-{
- 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);
-
- if (separator_pos >= 1) {
- struct strbuf tok = STRBUF_INIT;
- struct strbuf val = STRBUF_INIT;
-
- parse_trailer(&tok, &val, NULL, trailer, separator_pos);
- if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
- if (opts->unfold)
- unfold_value(&val);
-
- if (opts->separator && out->len != origlen)
- strbuf_addbuf(out, opts->separator);
- if (!opts->value_only)
- strbuf_addbuf(out, &tok);
- if (!opts->key_only && !opts->value_only) {
- if (opts->key_value_separator)
- strbuf_addbuf(out, opts->key_value_separator);
- else
- strbuf_addstr(out, ": ");
- }
- if (!opts->key_only)
- strbuf_addbuf(out, &val);
- if (!opts->separator)
- strbuf_addch(out, '\n');
- }
- strbuf_release(&tok);
- strbuf_release(&val);
-
- } else if (!opts->only_trailers) {
- if (opts->separator && out->len != origlen) {
- strbuf_addbuf(out, opts->separator);
- }
- strbuf_addstr(out, trailer);
- if (opts->separator) {
- strbuf_rtrim(out);
- }
- }
- }
-
-}
-
void format_trailers_from_commit(const char *msg,
const struct process_trailer_options *opts,
struct strbuf *out)
--
gitgitgadget
^ permalink raw reply related
* [PATCH 03/10] trailer: unify trailer formatting machinery
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
Currently have two functions for formatting trailers exposed in
trailer.h:
void format_trailers(FILE *outfile, struct list_head *head,
const struct process_trailer_options *opts);
void format_trailers_from_commit(struct strbuf *out, const char *msg,
const struct process_trailer_options *opts);
and previously these functions, although similar enough (even taking the
same process_trailer_options struct pointer), did not build on each
other.
Make format_trailers_from_commit() rely on format_trailers(). Teach
format_trailers() to process trailers with the additional
process_trailer_options fields like opts->key_only which is only used by
format_trailers_from_commit() and not builtin/interpret-trailers.c.
This will allow us to delete the format_trailer_info() and
print_tok_val() functions in the next patch. They are not deleted here
in order to keep the diff small.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 5 +-
pretty.c | 2 +-
ref-filter.c | 2 +-
trailer.c | 105 +++++++++++++++++++++++++++++------
trailer.h | 21 +++----
5 files changed, 102 insertions(+), 33 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index adb74276281..934833a4645 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -140,6 +140,7 @@ static void interpret_trailers(const char *file,
{
LIST_HEAD(head);
struct strbuf sb = STRBUF_INIT;
+ struct strbuf trailer_block = STRBUF_INIT;
struct trailer_info info;
FILE *outfile = stdout;
@@ -170,7 +171,9 @@ static void interpret_trailers(const char *file,
}
/* Print trailer block. */
- format_trailers(outfile, &head, opts);
+ format_trailers(&head, opts, &trailer_block);
+ fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
+ strbuf_release(&trailer_block);
free_trailers(&head);
trailer_info_release(&info);
diff --git a/pretty.c b/pretty.c
index cf964b060cd..f0721a5214f 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1759,7 +1759,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
goto trailer_out;
}
if (*arg == ')') {
- format_trailers_from_commit(sb, msg + c->subject_off, &opts);
+ format_trailers_from_commit(msg + c->subject_off, &opts, sb);
ret = arg - placeholder + 1;
}
trailer_out:
diff --git a/ref-filter.c b/ref-filter.c
index 35b989e1dfe..7fb13818686 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1985,7 +1985,7 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp
struct strbuf s = STRBUF_INIT;
/* Format the trailer info according to the trailer_opts given */
- format_trailers_from_commit(&s, subpos, &atom->u.contents.trailer_opts);
+ format_trailers_from_commit(subpos, &atom->u.contents.trailer_opts, &s);
v->s = strbuf_detach(&s, NULL);
} else if (atom->u.contents.option == C_BARE)
diff --git a/trailer.c b/trailer.c
index 0ce7e9079ca..315d90ee1ab 100644
--- a/trailer.c
+++ b/trailer.c
@@ -162,19 +162,6 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-void format_trailers(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts)
-{
- struct list_head *pos;
- struct trailer_item *item;
- list_for_each(pos, head) {
- item = list_entry(pos, struct trailer_item, list);
- if ((!opts->trim_empty || strlen(item->value) > 0) &&
- (!opts->only_trailers || item->token))
- print_tok_val(outfile, item->token, item->value);
- }
-}
-
static struct trailer_item *trailer_from_arg(struct arg_item *arg_tok)
{
struct trailer_item *new_item = xcalloc(1, sizeof(*new_item));
@@ -984,6 +971,78 @@ static void unfold_value(struct strbuf *val)
strbuf_release(&out);
}
+void format_trailers(struct list_head *head,
+ const struct process_trailer_options *opts,
+ struct strbuf *out)
+{
+ struct list_head *pos;
+ struct trailer_item *item;
+ int need_separator = 0;
+
+ list_for_each(pos, head) {
+ item = list_entry(pos, struct trailer_item, list);
+ if (item->token) {
+ char c;
+
+ struct strbuf tok = STRBUF_INIT;
+ struct strbuf val = STRBUF_INIT;
+ strbuf_addstr(&tok, item->token);
+ strbuf_addstr(&val, item->value);
+
+ /*
+ * Skip key/value pairs where the value was empty. This
+ * can happen from trailers specified without a
+ * separator, like `--trailer "Reviewed-by"` (no
+ * corresponding value).
+ */
+ if (opts->trim_empty && !strlen(item->value))
+ continue;
+
+ if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
+ if (opts->unfold)
+ unfold_value(&val);
+
+ if (opts->separator && need_separator)
+ strbuf_addbuf(out, opts->separator);
+ if (!opts->value_only)
+ strbuf_addbuf(out, &tok);
+ if (!opts->key_only && !opts->value_only) {
+ if (opts->key_value_separator)
+ strbuf_addbuf(out, opts->key_value_separator);
+ else {
+ c = last_non_space_char(tok.buf);
+ if (c) {
+ if (!strchr(separators, c))
+ strbuf_addf(out, "%c ", separators[0]);
+ }
+ }
+ }
+ if (!opts->key_only)
+ strbuf_addbuf(out, &val);
+ if (!opts->separator)
+ strbuf_addch(out, '\n');
+
+ need_separator = 1;
+ }
+
+ strbuf_release(&tok);
+ strbuf_release(&val);
+ } else if (!opts->only_trailers) {
+ if (opts->separator && need_separator) {
+ strbuf_addbuf(out, opts->separator);
+ }
+ strbuf_addstr(out, item->value);
+ if (opts->separator)
+ strbuf_rtrim(out);
+ else
+ strbuf_addch(out, '\n');
+
+ need_separator = 1;
+ }
+
+ }
+}
+
/*
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
@@ -1144,13 +1203,25 @@ static void format_trailer_info(struct strbuf *out,
}
-void format_trailers_from_commit(struct strbuf *out, const char *msg,
- const struct process_trailer_options *opts)
+void format_trailers_from_commit(const char *msg,
+ const struct process_trailer_options *opts,
+ struct strbuf *out)
{
+ LIST_HEAD(head);
struct trailer_info info;
- trailer_info_get(&info, msg, opts);
- format_trailer_info(out, &info, msg, opts);
+ parse_trailers(&info, msg, &head, opts);
+
+ /* 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_trailers(&head, opts, out);
+
+ free_trailers(&head);
trailer_info_release(&info);
}
diff --git a/trailer.h b/trailer.h
index 0e4f0ece9b3..50f70556302 100644
--- a/trailer.h
+++ b/trailer.h
@@ -102,21 +102,16 @@ void trailer_info_release(struct trailer_info *info);
void trailer_config_init(void);
void free_trailers(struct list_head *trailers);
-void format_trailers(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts);
+void format_trailers(struct list_head *head,
+ const struct process_trailer_options *opts,
+ struct strbuf *out);
/*
- * 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 format_trailers slightly in that we always format
- * only the trailer block itself, even if the "only_trailers" option is not
- * set.
+ * Convenience function to format the trailers from the commit msg "msg" into
+ * the strbuf "out". Reuses format_trailers internally.
*/
-void format_trailers_from_commit(struct strbuf *out, const char *msg,
- const struct process_trailer_options *opts);
+void format_trailers_from_commit(const char *msg,
+ const struct process_trailer_options *opts,
+ struct strbuf *out);
/*
* An interface for iterating over the trailers found in a particular commit
--
gitgitgadget
^ permalink raw reply related
* [PATCH 02/10] trailer: include "trailer" term in API functions
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
These functions are exposed to clients and so they should include
"trailer" in their names for easier identification, just like all the
other functions already exposed by trailer.h.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 7 ++++---
trailer.c | 10 +++++-----
trailer.h | 10 +++++-----
3 files changed, 14 insertions(+), 13 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 444f8fb70c9..adb74276281 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -143,7 +143,7 @@ static void interpret_trailers(const char *file,
struct trailer_info info;
FILE *outfile = stdout;
- ensure_configured();
+ trailer_config_init();
read_input_file(&sb, file);
@@ -169,9 +169,10 @@ static void interpret_trailers(const char *file,
process_trailers_lists(&head, &arg_head);
}
- print_all(outfile, &head, opts);
+ /* Print trailer block. */
+ format_trailers(outfile, &head, opts);
- free_all(&head);
+ free_trailers(&head);
trailer_info_release(&info);
/* Print the lines after the trailers as is */
diff --git a/trailer.c b/trailer.c
index 9d70c9946bd..0ce7e9079ca 100644
--- a/trailer.c
+++ b/trailer.c
@@ -162,8 +162,8 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-void print_all(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts)
+void format_trailers(FILE *outfile, struct list_head *head,
+ const struct process_trailer_options *opts)
{
struct list_head *pos;
struct trailer_item *item;
@@ -588,7 +588,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
return 0;
}
-void ensure_configured(void)
+void trailer_config_init(void)
{
if (configured)
return;
@@ -1023,7 +1023,7 @@ void parse_trailers(struct trailer_info *info,
}
}
-void free_all(struct list_head *head)
+void free_trailers(struct list_head *head)
{
struct list_head *pos, *p;
list_for_each_safe(pos, p, head) {
@@ -1041,7 +1041,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,
size_t nr = 0, alloc = 0;
char **last = NULL;
- ensure_configured();
+ trailer_config_init();
end_of_log_message = find_end_of_log_message(str, opts->no_divider);
trailer_block_start = find_trailer_block_start(str, end_of_log_message);
diff --git a/trailer.h b/trailer.h
index b3e4a5e127d..0e4f0ece9b3 100644
--- a/trailer.h
+++ b/trailer.h
@@ -99,11 +99,11 @@ void trailer_info_get(struct trailer_info *info, const char *str,
void trailer_info_release(struct trailer_info *info);
-void ensure_configured(void);
-void print_all(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts);
-void free_all(struct list_head *head);
+void trailer_config_init(void);
+void free_trailers(struct list_head *trailers);
+void format_trailers(FILE *outfile, struct list_head *head,
+ const struct process_trailer_options *opts);
/*
* Format the trailers from the commit msg "msg" into the strbuf "out".
* Note two caveats about "opts":
@@ -111,7 +111,7 @@ void free_all(struct list_head *head);
* - 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
+ * - this differs from format_trailers slightly in that we always format
* only the trailer block itself, even if the "only_trailers" option is not
* set.
*/
--
gitgitgadget
^ permalink raw reply related
* [PATCH 01/10] trailer: move process_trailers() to interpret-trailers.c
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git
Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver,
Linus Arver
In-Reply-To: <pull.1632.git.1704869487.gitgitgadget@gmail.com>
From: Linus Arver <linusa@google.com>
The interpret-trailers.c builtin is the only place we need to call
process_trailers(). As it stands, process_trailers() is inherently tied
to how the builtin behaves, so move its definition there.
Delete the corresponding declaration from trailer.h, which then forces
us to expose the working innards of that function. This enriches
trailer.h to include a more granular API, which can then be unit-tested
in the future (because process_trailers() by itself does too many things
to be able to be easily unit-tested).
Take this opportunity to demote some file-handling functions out of the
trailer API implementation, as these have nothing to do with trailers.
While we're at it, rename process_trailers() to interpret_trailers() in
the builtin for consistency with the existing cmd_interpret_trailers(),
which wraps around this function.
Signed-off-by: Linus Arver <linusa@google.com>
---
builtin/interpret-trailers.c | 98 +++++++++++++++++++++++++++-
trailer.c | 120 ++++-------------------------------
trailer.h | 20 +++++-
3 files changed, 126 insertions(+), 112 deletions(-)
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 033bd1556cf..444f8fb70c9 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -9,6 +9,7 @@
#include "gettext.h"
#include "parse-options.h"
#include "string-list.h"
+#include "tempfile.h"
#include "trailer.h"
#include "config.h"
@@ -91,6 +92,99 @@ static int parse_opt_parse(const struct option *opt, const char *arg,
return 0;
}
+static struct tempfile *trailers_tempfile;
+
+static FILE *create_in_place_tempfile(const char *file)
+{
+ struct stat st;
+ struct strbuf filename_template = STRBUF_INIT;
+ const char *tail;
+ FILE *outfile;
+
+ if (stat(file, &st))
+ die_errno(_("could not stat %s"), file);
+ if (!S_ISREG(st.st_mode))
+ die(_("file %s is not a regular file"), file);
+ if (!(st.st_mode & S_IWUSR))
+ die(_("file %s is not writable by user"), file);
+
+ /* Create temporary file in the same directory as the original */
+ tail = strrchr(file, '/');
+ if (tail)
+ strbuf_add(&filename_template, file, tail - file + 1);
+ strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
+
+ trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
+ strbuf_release(&filename_template);
+ outfile = fdopen_tempfile(trailers_tempfile, "w");
+ if (!outfile)
+ die_errno(_("could not open temporary file"));
+
+ return outfile;
+}
+
+static void read_input_file(struct strbuf *sb, const char *file)
+{
+ if (file) {
+ if (strbuf_read_file(sb, file, 0) < 0)
+ die_errno(_("could not read input file '%s'"), file);
+ } else {
+ if (strbuf_read(sb, fileno(stdin), 0) < 0)
+ die_errno(_("could not read from stdin"));
+ }
+}
+
+static void interpret_trailers(const char *file,
+ const struct process_trailer_options *opts,
+ struct list_head *new_trailer_head)
+{
+ LIST_HEAD(head);
+ struct strbuf sb = STRBUF_INIT;
+ struct trailer_info info;
+ FILE *outfile = stdout;
+
+ ensure_configured();
+
+ read_input_file(&sb, file);
+
+ if (opts->in_place)
+ outfile = create_in_place_tempfile(file);
+
+ parse_trailers(&info, sb.buf, &head, opts);
+
+ /* Print the lines before the trailers */
+ if (!opts->only_trailers)
+ fwrite(sb.buf, 1, info.trailer_block_start, outfile);
+
+ if (!opts->only_trailers && !info.blank_line_before_trailer)
+ fprintf(outfile, "\n");
+
+
+ if (!opts->only_input) {
+ LIST_HEAD(config_head);
+ LIST_HEAD(arg_head);
+ parse_trailers_from_config(&config_head);
+ parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
+ list_splice(&config_head, &arg_head);
+ process_trailers_lists(&head, &arg_head);
+ }
+
+ print_all(outfile, &head, opts);
+
+ free_all(&head);
+ trailer_info_release(&info);
+
+ /* Print the lines after the trailers as is */
+ if (!opts->only_trailers)
+ fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
+
+ if (opts->in_place)
+ if (rename_tempfile(&trailers_tempfile, file))
+ die_errno(_("could not rename temporary file to %s"), file);
+
+ strbuf_release(&sb);
+}
+
int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
{
struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT;
@@ -132,11 +226,11 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
if (argc) {
int i;
for (i = 0; i < argc; i++)
- process_trailers(argv[i], &opts, &trailers);
+ interpret_trailers(argv[i], &opts, &trailers);
} else {
if (opts.in_place)
die(_("no input file given for in-place editing"));
- process_trailers(NULL, &opts, &trailers);
+ interpret_trailers(NULL, &opts, &trailers);
}
new_trailers_clear(&trailers);
diff --git a/trailer.c b/trailer.c
index 3a0710a4583..9d70c9946bd 100644
--- a/trailer.c
+++ b/trailer.c
@@ -5,7 +5,6 @@
#include "string-list.h"
#include "run-command.h"
#include "commit.h"
-#include "tempfile.h"
#include "trailer.h"
#include "list.h"
/*
@@ -163,8 +162,8 @@ static void print_tok_val(FILE *outfile, const char *tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
}
-static void print_all(FILE *outfile, struct list_head *head,
- const struct process_trailer_options *opts)
+void print_all(FILE *outfile, struct list_head *head,
+ const struct process_trailer_options *opts)
{
struct list_head *pos;
struct trailer_item *item;
@@ -366,8 +365,8 @@ static int find_same_and_apply_arg(struct list_head *head,
return 0;
}
-static void process_trailers_lists(struct list_head *head,
- struct list_head *arg_head)
+void process_trailers_lists(struct list_head *head,
+ struct list_head *arg_head)
{
struct list_head *pos, *p;
struct arg_item *arg_tok;
@@ -589,7 +588,7 @@ static int git_trailer_config(const char *conf_key, const char *value,
return 0;
}
-static void ensure_configured(void)
+void ensure_configured(void)
{
if (configured)
return;
@@ -719,7 +718,7 @@ static void add_arg_item(struct list_head *arg_head, char *tok, char *val,
list_add_tail(&new_item->list, arg_head);
}
-static void parse_trailers_from_config(struct list_head *config_head)
+void parse_trailers_from_config(struct list_head *config_head)
{
struct arg_item *item;
struct list_head *pos;
@@ -735,8 +734,8 @@ static void parse_trailers_from_config(struct list_head *config_head)
}
}
-static void parse_trailers_from_command_line_args(struct list_head *arg_head,
- struct list_head *new_trailer_head)
+void parse_trailers_from_command_line_args(struct list_head *arg_head,
+ struct list_head *new_trailer_head)
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
@@ -775,17 +774,6 @@ static void parse_trailers_from_command_line_args(struct list_head *arg_head,
free(cl_separators);
}
-static void read_input_file(struct strbuf *sb, const char *file)
-{
- if (file) {
- if (strbuf_read_file(sb, file, 0) < 0)
- die_errno(_("could not read input file '%s'"), file);
- } else {
- if (strbuf_read(sb, fileno(stdin), 0) < 0)
- die_errno(_("could not read from stdin"));
- }
-}
-
static const char *next_line(const char *str)
{
const char *nl = strchrnul(str, '\n');
@@ -1000,10 +988,10 @@ static void unfold_value(struct strbuf *val)
* Parse trailers in "str", populating the trailer info and "head"
* linked list structure.
*/
-static void parse_trailers(struct trailer_info *info,
- const char *str,
- struct list_head *head,
- const struct process_trailer_options *opts)
+void parse_trailers(struct trailer_info *info,
+ const char *str,
+ struct list_head *head,
+ const struct process_trailer_options *opts)
{
struct strbuf tok = STRBUF_INIT;
struct strbuf val = STRBUF_INIT;
@@ -1035,7 +1023,7 @@ static void parse_trailers(struct trailer_info *info,
}
}
-static void free_all(struct list_head *head)
+void free_all(struct list_head *head)
{
struct list_head *pos, *p;
list_for_each_safe(pos, p, head) {
@@ -1044,88 +1032,6 @@ static void free_all(struct list_head *head)
}
}
-static struct tempfile *trailers_tempfile;
-
-static FILE *create_in_place_tempfile(const char *file)
-{
- struct stat st;
- struct strbuf filename_template = STRBUF_INIT;
- const char *tail;
- FILE *outfile;
-
- if (stat(file, &st))
- die_errno(_("could not stat %s"), file);
- if (!S_ISREG(st.st_mode))
- die(_("file %s is not a regular file"), file);
- if (!(st.st_mode & S_IWUSR))
- die(_("file %s is not writable by user"), file);
-
- /* Create temporary file in the same directory as the original */
- tail = strrchr(file, '/');
- if (tail)
- strbuf_add(&filename_template, file, tail - file + 1);
- strbuf_addstr(&filename_template, "git-interpret-trailers-XXXXXX");
-
- trailers_tempfile = xmks_tempfile_m(filename_template.buf, st.st_mode);
- strbuf_release(&filename_template);
- outfile = fdopen_tempfile(trailers_tempfile, "w");
- if (!outfile)
- die_errno(_("could not open temporary file"));
-
- return outfile;
-}
-
-void process_trailers(const char *file,
- const struct process_trailer_options *opts,
- struct list_head *new_trailer_head)
-{
- LIST_HEAD(head);
- struct strbuf sb = STRBUF_INIT;
- struct trailer_info info;
- FILE *outfile = stdout;
-
- ensure_configured();
-
- read_input_file(&sb, file);
-
- if (opts->in_place)
- outfile = create_in_place_tempfile(file);
-
- parse_trailers(&info, sb.buf, &head, opts);
-
- /* Print the lines before the trailers */
- if (!opts->only_trailers)
- fwrite(sb.buf, 1, info.trailer_block_start, outfile);
-
- if (!opts->only_trailers && !info.blank_line_before_trailer)
- fprintf(outfile, "\n");
-
-
- if (!opts->only_input) {
- LIST_HEAD(config_head);
- LIST_HEAD(arg_head);
- parse_trailers_from_config(&config_head);
- parse_trailers_from_command_line_args(&arg_head, new_trailer_head);
- list_splice(&config_head, &arg_head);
- process_trailers_lists(&head, &arg_head);
- }
-
- print_all(outfile, &head, opts);
-
- free_all(&head);
- trailer_info_release(&info);
-
- /* Print the lines after the trailers as is */
- if (!opts->only_trailers)
- fwrite(sb.buf + info.trailer_block_end, 1, sb.len - info.trailer_block_end, outfile);
-
- if (opts->in_place)
- if (rename_tempfile(&trailers_tempfile, file))
- die_errno(_("could not rename temporary file to %s"), file);
-
- strbuf_release(&sb);
-}
-
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts)
{
diff --git a/trailer.h b/trailer.h
index 1644cd05f60..b3e4a5e127d 100644
--- a/trailer.h
+++ b/trailer.h
@@ -81,15 +81,29 @@ struct process_trailer_options {
#define PROCESS_TRAILER_OPTIONS_INIT {0}
-void process_trailers(const char *file,
- const struct process_trailer_options *opts,
- struct list_head *new_trailer_head);
+void parse_trailers_from_config(struct list_head *config_head);
+
+void parse_trailers_from_command_line_args(struct list_head *arg_head,
+ struct list_head *new_trailer_head);
+
+void process_trailers_lists(struct list_head *head,
+ struct list_head *arg_head);
+
+void parse_trailers(struct trailer_info *info,
+ const char *str,
+ struct list_head *head,
+ const struct process_trailer_options *opts);
void trailer_info_get(struct trailer_info *info, const char *str,
const struct process_trailer_options *opts);
void trailer_info_release(struct trailer_info *info);
+void ensure_configured(void);
+void print_all(FILE *outfile, struct list_head *head,
+ const struct process_trailer_options *opts);
+void free_all(struct list_head *head);
+
/*
* Format the trailers from the commit msg "msg" into the strbuf "out".
* Note two caveats about "opts":
--
gitgitgadget
^ permalink raw reply related
* [PATCH 00/10] Enrich Trailer API
From: Linus Arver via GitGitGadget @ 2024-01-10 6:51 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer, Junio C Hamano, Christian Couder, Linus Arver
This patch series is the first 10 patches of a much larger series I've been
working. The main goal of this series is to enrich the API in trailer.h. The
larger series brings a number of additional code simplifications and
cleanups (exposing and fixing some bugs along the way), and builds on top of
this series. The goal of the larger series is to make the trailer interface
ready for unit testing. By "trailer API" I mean those functions exposed in
trailer.h.
Currently, we have "process_trailers()" in trailer.h which does many
different things (parse command-line arguments, create temporary files, etc)
that are independent of the concept of "trailers", whose interface trailer.h
defines. This makes unit-testing this function difficult. While there is no
technical reason why we couldn't write unit tests for the smaller functions
that are called within process_trailers(), doing so would involve testing
private ("static" in trailer.c) functions instead of the public functions
exposed in trailer.h which authoritatively define the API.
As an alternative to this patch series, we could keep trailer.h intact and
decide to unit-test the existing "trailer_info_get()" function which does
most of the trailer parsing work. However this function wouldn't be easy to
test either, because the resulting data structure merely contains the
unparsed "trailers" lines. So the unit test (if it wants to inspect the
result of parsing these lines) would have to invoke additional parsing
functions itself. And at that point it would not longer be a unit test in
the traditional sense, because it would be invoking multiple functions at
once.
This series breaks up "process_trailers()" into smaller pieces, exposing
many of the parts relevant to trailer-related processing in trailer.h. This
forces us to start writing unit tests for these now public functions, but
that is a good thing because those same unit tests should be easy to write
(due to their small(er) sizes), but also, because those unit tests will now
ensure some degree of stability across new versions of trailer.h (we will
start noticing when the behavior of any of these API functions change).
Another benefit is that more clients (perhaps those outside of Git in the
future) will be able to use the same trailer processing algorithms used by
the interpret-trailers builtin. For example, a web server may want to parse
trailers the same way that interpret-trailers would parse them, without
having to shell out to interpret-trailers. Importing the new (richer)
trailer.h file that this series promotes would help them achieve that goal.
This use case was the original motivation behind my work in this area of
Git.
Thanks to the aggressive refactoring in this series, I've been able to
identify and fix a couple bugs in our existing implementation. Those fixes
build on top of this series but were not included here, in order to keep
this series small.
Linus Arver (10):
trailer: move process_trailers() to interpret-trailers.c
trailer: include "trailer" term in API functions
trailer: unify trailer formatting machinery
trailer: delete obsolete formatting functions
sequencer: use the trailer iterator
trailer: make trailer_info struct private
trailer: spread usage of "trailer_block" language
trailer: prepare to move parse_trailers_from_command_line_args() to
builtin
trailer: move arg handling to interpret-trailers.c
trailer: delete obsolete argument handling code from API
builtin/interpret-trailers.c | 169 ++++++++--
builtin/shortlog.c | 7 +-
pretty.c | 2 +-
ref-filter.c | 2 +-
sequencer.c | 35 +--
trailer.c | 579 ++++++++++++++++-------------------
trailer.h | 106 ++++---
7 files changed, 482 insertions(+), 418 deletions(-)
base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1632%2Flistx%2Ftrailer-api-refactor-part-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1632/listx/trailer-api-refactor-part-1-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1632
--
gitgitgadget
^ permalink raw reply
* Request fixing a vulnerability for git project
From: Reika Arakawa @ 2024-01-10 6:47 UTC (permalink / raw)
To: git@vger.kernel.org; +Cc: vuls@jpcert.or.jp
[-- Attachment #1.1: Type: text/plain, Size: 1601 bytes --]
To Whom it May Concern: git Product Development Manager,
We are a cybersecurity research team affiliated with NTT in Japan, and are engaged in research related to vulnerability detection, analysis, and modification.
We investigated git (https://github.com/git/git) released as OSS. A known vulnerability and known code were found, so please fix them with a patch. We hope to prevent cyber-attacks from occurring by working together with Japan's representative, JPCERT/CC (vuls@jpcert.or.jp<mailto:vuls@jpcert.or.jp>).
How to discover vulnerabilities:
We inspected all function codes for OSS software code using patch information and code information (2000-2023) for known vulnerabilities. We would like to inform you of the code information for which there is a high possibility that a public patch has not been applied with high severity. Vulnerability code information and countermeasures are listed in the attached file ‘git@@git_vulnerability_details.txt’.
Please note that in the real world, a vulnerable state may not necessarily be true, as it may become a vulnerable state under certain combinations of conditions in the real world.
Additional confirmation details:
We are planning to submit a paper on this content between January and March, and we would like to include the source code characteristics in the paper without revealing the OSS name or file name. Are there any concerns about this?
--
Best regards, NTT Social Informatics Laboratories（ https://www.rd.ntt/e/sil/）
E-mail: reika.arakawa@ntt.com<mailto:reika.arakawa@ntt.com>
[-- Attachment #1.2: Type: text/html, Size: 5350 bytes --]
[-- Attachment #2: git@@git_vulnerability_details.txt --]
[-- Type: text/plain, Size: 370 bytes --]
Software name：git@@git
Version tag：v2.43.0
How to fix: Search by CVE number, refer to the commit listed in reference on the NVD/CVE site, and fix it.
NVD:https://nvd.nist.gov/vuln/detail/CVE-2009-5155
â‘ git@@git/compat/regex/regcomp.c [parse_reg_exp]
・CVE-2009-5155，CWE-19_5.0_0
・git.savannah.gnu.org##gnulib_regcomp.c, [parse_reg_exp]
^ permalink raw reply
* Re: [PATCH 3/3] submodule-config.c: strengthen URL fsck check
From: Patrick Steinhardt @ 2024-01-10 6:33 UTC (permalink / raw)
To: Victoria Dye via GitGitGadget; +Cc: git, Victoria Dye
In-Reply-To: <893071530d3b77d6b72b7f69a6dfb9947579865e.1704822817.git.gitgitgadget@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1426 bytes --]
On Tue, Jan 09, 2024 at 05:53:37PM +0000, Victoria Dye via GitGitGadget wrote:
> From: Victoria Dye <vdye@github.com>
>
> Update the validation of "curl URL" submodule URLs (i.e. those that specify
> an "http[s]" or "ftp[s]" protocol) in 'check_submodule_url()' to catch more
> invalid URLs. The existing validation using 'credential_from_url_gently()'
> parses certain URLs incorrectly, leading to invalid submodule URLs passing
> 'git fsck' checks. Conversely, 'url_normalize()' - used to validate remote
> URLs in 'remote_get()' - correctly identifies the invalid URLs missed by
> 'credential_from_url_gently()'.
Okay, so we retain the wrong behavior of `credential_from_url_gently()`,
right? I wonder whether this can be abused in any way, doubly so because
the function gets invoked with untrusted input from the remote server
when we handle redirects in `http_request_reauth()`. But the redirect
URL we end up passing to `credential_from_url_gently()` would have to
contain a non-numeric port, and curl seemingly does not know to handle
those either.
Other callsites include fsck (which you're fixing) and the credential
store (which is entirely user-controlled). It would be great regardless
to fix the underlying bug in `credential_from_url_gently()` eventually
though. But I do not think that this has to be part this patch series
here, which is a strict improvement.
Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v2 5/5] completion: custom git-bisect terms
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240110020347.673155-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ad80df6630..87cf7b2561 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1591,13 +1591,22 @@ _git_bisect ()
term_good=`__git bisect terms --term-good`
fi
- local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+ # We will complete any custom terms, but still always complete the
+ # more usual bad/new/good/old because git bisect gives a good error
+ # message if these are given when not in use and that's better than
+ # silent refusal to complete if the user is confused.
+ #
+ # We want to recognize 'view' but not complete it, because it overlaps
+ # with 'visualize' too much and is just an alias for it.
+ #
+ local completable_subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+ local all_subcommands="$completable_subcommands view"
- local subcommand="$(__git_find_on_cmdline "$subcommands")"
+ local subcommand="$(__git_find_on_cmdline "$all_subcommands")"
if [ -z "$subcommand" ]; then
if [ -f "$__git_repo_path"/BISECT_START ]; then
- __gitcomp "$subcommands"
+ __gitcomp "$completable_subcommands"
else
__gitcomp "replay start"
fi
@@ -1615,7 +1624,7 @@ _git_bisect ()
;;
esac
;;
- visualize)
+ visualize|view)
case "$cur" in
-*)
__git_complete_log_opts
--
2.43.0
^ permalink raw reply related
* [PATCH v2 3/5] completion: move to maintain define-before-use
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240110020347.673155-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 269 ++++++++++++-------------
1 file changed, 134 insertions(+), 135 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c16aded36c..63ca8082a4 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1445,6 +1445,140 @@ _git_archive ()
__git_complete_file
}
+# Options that go well for log, shortlog and gitk
+__git_log_common_options="
+ --not --all
+ --branches --tags --remotes
+ --first-parent --merges --no-merges
+ --max-count=
+ --max-age= --since= --after=
+ --min-age= --until= --before=
+ --min-parents= --max-parents=
+ --no-min-parents --no-max-parents
+"
+# Options that go well for log and gitk (not shortlog)
+__git_log_gitk_options="
+ --dense --sparse --full-history
+ --simplify-merges --simplify-by-decoration
+ --left-right --notes --no-notes
+"
+# Options that go well for log and shortlog (not gitk)
+__git_log_shortlog_options="
+ --author= --committer= --grep=
+ --all-match --invert-grep
+"
+# Options accepted by log and show
+__git_log_show_options="
+ --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
+"
+
+__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
+
+__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
+__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
+
+# Check for only porcelain (i.e. not git-rev-list) option (not argument)
+# and selected option argument completions for git-log options and if any
+# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
+# and will be empty on return if no candidates are found.
+__git_complete_log_opts ()
+{
+ [ -z "$COMPREPLY" ] || return 1 # Precondition
+
+ local merge=""
+ if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
+ merge="--merge"
+ fi
+ case "$prev,$cur" in
+ -L,:*:*)
+ return # fall back to Bash filename completion
+ ;;
+ -L,:*)
+ __git_complete_symbol --cur="${cur#:}" --sfx=":"
+ return
+ ;;
+ -G,*|-S,*)
+ __git_complete_symbol
+ return
+ ;;
+ esac
+ case "$cur" in
+ --pretty=*|--format=*)
+ __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
+ " "" "${cur#*=}"
+ return
+ ;;
+ --date=*)
+ __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
+ return
+ ;;
+ --decorate=*)
+ __gitcomp "full short no" "" "${cur##--decorate=}"
+ return
+ ;;
+ --diff-algorithm=*)
+ __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
+ return
+ ;;
+ --submodule=*)
+ __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
+ return
+ ;;
+ --ws-error-highlight=*)
+ __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
+ return
+ ;;
+ --no-walk=*)
+ __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
+ return
+ ;;
+ --diff-merges=*)
+ __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
+ return
+ ;;
+ --*)
+ __gitcomp "
+ $__git_log_common_options
+ $__git_log_shortlog_options
+ $__git_log_gitk_options
+ $__git_log_show_options
+ --root --topo-order --date-order --reverse
+ --follow --full-diff
+ --abbrev-commit --no-abbrev-commit --abbrev=
+ --relative-date --date=
+ --pretty= --format= --oneline
+ --show-signature
+ --cherry-mark
+ --cherry-pick
+ --graph
+ --decorate --decorate= --no-decorate
+ --walk-reflogs
+ --no-walk --no-walk= --do-walk
+ --parents --children
+ --expand-tabs --expand-tabs= --no-expand-tabs
+ $merge
+ $__git_diff_common_options
+ "
+ return
+ ;;
+ -L:*:*)
+ return # fall back to Bash filename completion
+ ;;
+ -L:*)
+ __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
+ return
+ ;;
+ -G*)
+ __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
+ return
+ ;;
+ -S*)
+ __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
+ return
+ ;;
+ esac
+}
+
_git_bisect ()
{
__git_has_doubledash && return
@@ -2052,141 +2186,6 @@ _git_ls_tree ()
__git_complete_file
}
-# Options that go well for log, shortlog and gitk
-__git_log_common_options="
- --not --all
- --branches --tags --remotes
- --first-parent --merges --no-merges
- --max-count=
- --max-age= --since= --after=
- --min-age= --until= --before=
- --min-parents= --max-parents=
- --no-min-parents --no-max-parents
-"
-# Options that go well for log and gitk (not shortlog)
-__git_log_gitk_options="
- --dense --sparse --full-history
- --simplify-merges --simplify-by-decoration
- --left-right --notes --no-notes
-"
-# Options that go well for log and shortlog (not gitk)
-__git_log_shortlog_options="
- --author= --committer= --grep=
- --all-match --invert-grep
-"
-# Options accepted by log and show
-__git_log_show_options="
- --diff-merges --diff-merges= --no-diff-merges --dd --remerge-diff
-"
-
-__git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-combined cc remerge r"
-
-__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
-__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-
-
-# Check for only porcelain (i.e. not git-rev-list) option (not argument)
-# and selected option argument completions for git-log options and if any
-# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
-# and will be empty on return if no candidates are found.
-__git_complete_log_opts ()
-{
- [ -z "$COMPREPLY" ] || return 1 # Precondition
-
- local merge=""
- if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
- merge="--merge"
- fi
- case "$prev,$cur" in
- -L,:*:*)
- return # fall back to Bash filename completion
- ;;
- -L,:*)
- __git_complete_symbol --cur="${cur#:}" --sfx=":"
- return
- ;;
- -G,*|-S,*)
- __git_complete_symbol
- return
- ;;
- esac
- case "$cur" in
- --pretty=*|--format=*)
- __gitcomp "$__git_log_pretty_formats $(__git_pretty_aliases)
- " "" "${cur#*=}"
- return
- ;;
- --date=*)
- __gitcomp "$__git_log_date_formats" "" "${cur##--date=}"
- return
- ;;
- --decorate=*)
- __gitcomp "full short no" "" "${cur##--decorate=}"
- return
- ;;
- --diff-algorithm=*)
- __gitcomp "$__git_diff_algorithms" "" "${cur##--diff-algorithm=}"
- return
- ;;
- --submodule=*)
- __gitcomp "$__git_diff_submodule_formats" "" "${cur##--submodule=}"
- return
- ;;
- --ws-error-highlight=*)
- __gitcomp "$__git_ws_error_highlight_opts" "" "${cur##--ws-error-highlight=}"
- return
- ;;
- --no-walk=*)
- __gitcomp "sorted unsorted" "" "${cur##--no-walk=}"
- return
- ;;
- --diff-merges=*)
- __gitcomp "$__git_diff_merges_opts" "" "${cur##--diff-merges=}"
- return
- ;;
- --*)
- __gitcomp "
- $__git_log_common_options
- $__git_log_shortlog_options
- $__git_log_gitk_options
- $__git_log_show_options
- --root --topo-order --date-order --reverse
- --follow --full-diff
- --abbrev-commit --no-abbrev-commit --abbrev=
- --relative-date --date=
- --pretty= --format= --oneline
- --show-signature
- --cherry-mark
- --cherry-pick
- --graph
- --decorate --decorate= --no-decorate
- --walk-reflogs
- --no-walk --no-walk= --do-walk
- --parents --children
- --expand-tabs --expand-tabs= --no-expand-tabs
- $merge
- $__git_diff_common_options
- "
- return
- ;;
- -L:*:*)
- return # fall back to Bash filename completion
- ;;
- -L:*)
- __git_complete_symbol --cur="${cur#-L:}" --sfx=":"
- return
- ;;
- -G*)
- __git_complete_symbol --pfx="-G" --cur="${cur#-G}"
- return
- ;;
- -S*)
- __git_complete_symbol --pfx="-S" --cur="${cur#-S}"
- return
- ;;
- esac
-}
-
_git_log ()
{
__git_has_doubledash && return
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/5] completion: git-log opts to bisect visualize
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240110020347.673155-1-britton.kerin@gmail.com>
To do this the majority of _git_log has been factored out into the new
__git_complete_log_opts. This is needed because the visualize command
accepts git-log options but not rev arguments (they are fixed to the
commits under bisection).
__git_complete_log_opts has a precondition that COMPREPLY be empty. In
a completion context it doesn't seem advisable to implement
preconditions as noisy or hard failures, so instead it becomes a no-op
on violation. This should be detectable and quick to debug for devels,
without ever aggravating a user (besides completion failure).
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 30 +++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 15d22ff7d9..c16aded36c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1472,6 +1472,16 @@ _git_bisect ()
;;
esac
;;
+ visualize)
+ case "$cur" in
+ -*)
+ __git_complete_log_opts
+ return
+ ;;
+ *)
+ ;;
+ esac
+ ;;
esac
case "$subcommand" in
@@ -2074,10 +2084,14 @@ __git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-c
__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-_git_log ()
+
+# Check for only porcelain (i.e. not git-rev-list) option (not argument)
+# and selected option argument completions for git-log options and if any
+# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
+# and will be empty on return if no candidates are found.
+__git_complete_log_opts ()
{
- __git_has_doubledash && return
- __git_find_repo_path
+ [ -z "$COMPREPLY" ] || return 1 # Precondition
local merge=""
if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
@@ -2171,6 +2185,16 @@ _git_log ()
return
;;
esac
+}
+
+_git_log ()
+{
+ __git_has_doubledash && return
+ __git_find_repo_path
+
+ __git_complete_log_opts
+ [ -z "$COMPREPLY" ] || return
+
__git_complete_revlist
}
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/5] completion: improvements for git-bisect
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin, Junio C Hamano
In-Reply-To: <9e180f50-4bf4-4822-9b02-2a1b50114e09@smtp-relay.sendinblue.com>
Bring completion for bisect up to date with current features.
I didn't hear back on my previous RFC Patch for these improvements, and
ultimately decided that simply aborting special completion efforts and
doing nothing on precondition violation is the best policy, because devs
will likely notice and users won't be irritated more than necessary.
Besides that the series has just been cleaned up a tiny bit.
Britton Leo Kerin (5):
completion: complete new old actions, start opts
completion: git-log opts to bisect visualize
completion: move to maintain define-before-use
completion: custom git-bisect terms
completion: custom git-bisect terms
contrib/completion/git-completion.bash | 312 +++++++++++++++----------
1 file changed, 183 insertions(+), 129 deletions(-)
Range-diff against v1:
1: e16264bfb9 = 1: e16264bfb9 completion: complete new old actions, start opts
2: 90466cdfa5 ! 2: 130abe3460 completion: git-log opts to bisect visualize
@@ Commit message
accepts git-log options but not rev arguments (they are fixed to the
commits under bisection).
+ __git_complete_log_opts has a precondition that COMPREPLY be empty. In
+ a completion context it doesn't seem advisable to implement
+ preconditions as noisy or hard failures, so instead it becomes a no-op
+ on violation. This should be detectable and quick to debug for devels,
+ without ever aggravating a user (besides completion failure).
+
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
## contrib/completion/git-completion.bash ##
@@ contrib/completion/git-completion.bash: __git_diff_merges_opts="off none on firs
__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-_git_log ()
--{
++
++# Check for only porcelain (i.e. not git-rev-list) option (not argument)
++# and selected option argument completions for git-log options and if any
++# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
++# and will be empty on return if no candidates are found.
++__git_complete_log_opts ()
+ {
- __git_has_doubledash && return
- __git_find_repo_path
++ [ -z "$COMPREPLY" ] || return 1 # Precondition
-+# Find only porcelain (i.e. not git-rev-list) option (not argument) and
-+# selected option argument completions for git-log options and put them in
-+# COMPREPLY.
-+__git_complete_log_opts ()
-+{
local merge=""
if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
- merge="--merge"
@@ contrib/completion/git-completion.bash: _git_log ()
return
;;
esac
-+
+}
+
+_git_log ()
3: 0edfb54dd5 ! 3: d659ace9c2 completion: move to maintain define-before-use
@@ contrib/completion/git-completion.bash: _git_archive ()
+__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
+__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
+
-+# Find only porcelain (i.e. not git-rev-list) option (not argument) and
-+# selected option argument completions for git-log options and put them in
-+# COMPREPLY.
++# Check for only porcelain (i.e. not git-rev-list) option (not argument)
++# and selected option argument completions for git-log options and if any
++# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
++# and will be empty on return if no candidates are found.
+__git_complete_log_opts ()
+{
++ [ -z "$COMPREPLY" ] || return 1 # Precondition
++
+ local merge=""
+ if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
+ merge="--merge"
@@ contrib/completion/git-completion.bash: _git_archive ()
+ return
+ ;;
+ esac
-+
+}
+
_git_bisect ()
@@ contrib/completion/git-completion.bash: _git_ls_tree ()
-__git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
-
-
--# Find only porcelain (i.e. not git-rev-list) option (not argument) and
--# selected option argument completions for git-log options and put them in
--# COMPREPLY.
+-# Check for only porcelain (i.e. not git-rev-list) option (not argument)
+-# and selected option argument completions for git-log options and if any
+-# are found put them in COMPREPLY. COMPREPLY must be empty at the start,
+-# and will be empty on return if no candidates are found.
-__git_complete_log_opts ()
-{
+- [ -z "$COMPREPLY" ] || return 1 # Precondition
+-
- local merge=""
- if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
- merge="--merge"
@@ contrib/completion/git-completion.bash: _git_ls_tree ()
- return
- ;;
- esac
--
-}
-
_git_log ()
4: a8a730ffbe = 4: c5bee633b2 completion: custom git-bisect terms
5: 44d3a36e20 ! 5: 220650f0ba completion: recognize but do not complete 'view'
@@ Metadata
Author: Britton Leo Kerin <britton.kerin@gmail.com>
## Commit message ##
- completion: recognize but do not complete 'view'
-
- Completing it might annoy some existing users by creating completion
- ambiguity on 'v' and 'vi' without adding anything useful in terms of
- interface discovery/recall (because 'view' is just an alias anyway).
+ completion: custom git-bisect terms
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
@@ contrib/completion/git-completion.bash: _git_bisect ()
fi
- local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
++ # We will complete any custom terms, but still always complete the
++ # more usual bad/new/good/old because git bisect gives a good error
++ # message if these are given when not in use and that's better than
++ # silent refusal to complete if the user is confused.
++ #
+ # We want to recognize 'view' but not complete it, because it overlaps
+ # with 'visualize' too much and is just an alias for it.
++ #
+ local completable_subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+ local all_subcommands="$completable_subcommands view"
6: 7b88549fbe < -: ---------- completion: add comment
--
2.43.0
^ permalink raw reply
* [PATCH v2 1/5] completion: complete new old actions, start opts
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240110020347.673155-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 185b47d802..15d22ff7d9 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1449,7 +1449,7 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad good skip reset visualize replay log run"
+ local subcommands="start bad new good old terms skip reset visualize replay log run help"
local subcommand="$(__git_find_on_cmdline "$subcommands")"
if [ -z "$subcommand" ]; then
__git_find_repo_path
@@ -1462,7 +1462,20 @@ _git_bisect ()
fi
case "$subcommand" in
- bad|good|reset|skip|start)
+ start)
+ case "$cur" in
+ --*)
+ __gitcomp "--term-new --term-bad --term-old --term-good --first-parent --no-checkout"
+ return
+ ;;
+ *)
+ ;;
+ esac
+ ;;
+ esac
+
+ case "$subcommand" in
+ bad|new|good|old|reset|skip|start)
__git_complete_refs
;;
*)
--
2.43.0
^ permalink raw reply related
* [PATCH v2 4/5] completion: custom git-bisect terms
From: Britton Leo Kerin @ 2024-01-10 2:03 UTC (permalink / raw)
To: git; +Cc: Britton Leo Kerin
In-Reply-To: <20240110020347.673155-1-britton.kerin@gmail.com>
Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
---
contrib/completion/git-completion.bash | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 63ca8082a4..ad80df6630 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1583,10 +1583,19 @@ _git_bisect ()
{
__git_has_doubledash && return
- local subcommands="start bad new good old terms skip reset visualize replay log run help"
+ __git_find_repo_path
+
+ local term_bad term_good
+ if [ -f "$__git_repo_path"/BISECT_START ]; then
+ term_bad=`__git bisect terms --term-bad`
+ term_good=`__git bisect terms --term-good`
+ fi
+
+ local subcommands="start bad new $term_bad good old $term_good terms skip reset visualize replay log run help"
+
local subcommand="$(__git_find_on_cmdline "$subcommands")"
+
if [ -z "$subcommand" ]; then
- __git_find_repo_path
if [ -f "$__git_repo_path"/BISECT_START ]; then
__gitcomp "$subcommands"
else
@@ -1619,7 +1628,7 @@ _git_bisect ()
esac
case "$subcommand" in
- bad|new|good|old|reset|skip|start)
+ bad|new|"$term_bad"|good|old|"$term_good"|reset|skip|start)
__git_complete_refs
;;
*)
--
2.43.0
^ permalink raw reply related
* Problem in gitweb.cgi
From: Marcelo Roberto Jimenez @ 2024-01-10 0:43 UTC (permalink / raw)
To: git
Hi,
I have recently run into a problem with gitweb that was triggered by
my distribution, OpenSUSE Tumbleweed. Due to a misconfiguration of the
application AppArmour, "git instaweb" was having problems in accessing
the configuration file "/etc/gitweb-common.conf". That was half of the
problem and has been reported downstream here:
https://bugzilla.suse.com/show_bug.cgi?id=1218664
The other half of the problem is in gitweb.cgi itself. There is a
logic to select which configuration file is going to be used. That
logic is ok, although a bit unclear from the documentation. Reading
the code I understood that $GITWEB_CONFIG_COMMON (usually
/etc/gitweb-common.conf) should always be read if it exists, and
$GITWEB_CONFIG_SYSTEM (usually /etc/gitweb.conf) will never be read if
$GITWEB_CONFIG has been read before.
The problem is that $GITWEB_CONFIG_COMMON was never read, even though
the code that reads it was being called before the code that reads the
other two files. As I said before, the problem was caused by a lack of
permission in AppArmour, but gitweb should have been able to catch the
error. By not signaling it properly, users get lost because the
problem is a little tricky to debug.
After some "printf" debugging, I converged to this routine, line 728
of gitweb.cgi:
# read and parse gitweb config file given by its parameter.
# returns true on success, false on recoverable error, allowing
# to chain this subroutine, using first file that exists.
# dies on errors during parsing config file, as it is unrecoverable.
sub read_config_file {
my $filename = shift;
return unless defined $filename;
# die if there are errors parsing config file
if (-e $filename) {
do $filename;
die $@ if $@;
return 1;
}
return;
}
Perl uses two different variables to manage errors from a "do". One is
"$@", which is set in this case when do is unable to compile the file.
The other is $!, which is set in case "do" cannot read the file. By
printing the value of $! I found out that it was set to "Permission
denied". Since the script does not currently test for $!, the error
goes unnoticed.
I suppose a proper fix would be to put a line before the test for $@,
like "die $! if $!".
For the curious, I have a report explaining the full problem here, but
the part relevant to gitweb is in this e-mail:
https://stackoverflow.com/questions/77789216/problem-with-git-instaweb-on-opensuse-tumbleweed-etc-gitweb-common-conf-is-n
Best regards,
Marcelo.
^ 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