* [PATCH v2 08/12] builtin/show-ref: refactor options for patterns subcommand
From: Patrick Steinhardt @ 2023-10-26 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3956 bytes --]
The patterns subcommand is the last command that still uses global
variables to track its options. Convert it to use a structure instead
with the same motivation as preceding commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 3e6ad6b6a84..87bc45d2d13 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -18,8 +18,6 @@ static const char * const show_ref_usage[] = {
NULL
};
-static int show_head, tags_only, heads_only, verify;
-
struct show_one_options {
int quiet;
int hash_only;
@@ -59,6 +57,7 @@ struct show_ref_data {
const struct show_one_options *show_one_opts;
const char **patterns;
int found_match;
+ int show_head;
};
static int show_ref(const char *refname, const struct object_id *oid,
@@ -66,7 +65,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
{
struct show_ref_data *data = cbdata;
- if (show_head && !strcmp(refname, "HEAD"))
+ if (data->show_head && !strcmp(refname, "HEAD"))
goto match;
if (data->patterns) {
@@ -180,22 +179,30 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
return 0;
}
-static int cmd_show_ref__patterns(const struct show_one_options *show_one_opts,
+struct patterns_options {
+ int show_head;
+ int heads_only;
+ int tags_only;
+};
+
+static int cmd_show_ref__patterns(const struct patterns_options *opts,
+ const struct show_one_options *show_one_opts,
const char **patterns)
{
struct show_ref_data show_ref_data = {
.show_one_opts = show_one_opts,
+ .show_head = opts->show_head,
};
if (patterns && *patterns)
show_ref_data.patterns = patterns;
- if (show_head)
+ if (opts->show_head)
head_ref(show_ref, &show_ref_data);
- if (heads_only || tags_only) {
- if (heads_only)
+ if (opts->heads_only || opts->tags_only) {
+ if (opts->heads_only)
for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
- if (tags_only)
+ if (opts->tags_only)
for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
} else {
for_each_ref(show_ref, &show_ref_data);
@@ -233,15 +240,17 @@ static int exclude_existing_callback(const struct option *opt, const char *arg,
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
struct exclude_existing_options exclude_existing_opts = {0};
+ struct patterns_options patterns_opts = {0};
struct show_one_options show_one_opts = {0};
+ int verify = 0;
const struct option show_ref_options[] = {
- OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
- OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
+ OPT_BOOL(0, "tags", &patterns_opts.tags_only, N_("only show tags (can be combined with heads)")),
+ OPT_BOOL(0, "heads", &patterns_opts.heads_only, N_("only show heads (can be combined with tags)")),
OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
"requires exact ref path")),
- OPT_HIDDEN_BOOL('h', NULL, &show_head,
+ OPT_HIDDEN_BOOL('h', NULL, &patterns_opts.show_head,
N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL(0, "head", &show_head,
+ OPT_BOOL(0, "head", &patterns_opts.show_head,
N_("show the HEAD reference, even if it would be filtered out")),
OPT_BOOL('d', "dereference", &show_one_opts.deref_tags,
N_("dereference tags into object IDs")),
@@ -267,5 +276,5 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
else if (verify)
return cmd_show_ref__verify(&show_one_opts, argv);
else
- return cmd_show_ref__patterns(&show_one_opts, argv);
+ return cmd_show_ref__patterns(&patterns_opts, &show_one_opts, argv);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 07/12] builtin/show-ref: stop using global vars for `show_one()`
From: Patrick Steinhardt @ 2023-10-26 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6148 bytes --]
The `show_one()` function implicitly receives a bunch of options which
are tracked via global variables. This makes it hard to see which
subcommands of git-show-ref(1) actually make use of these options.
Introduce a `show_one_options` structure that gets passed down to this
function. This allows us to get rid of more global state and makes it
more explicit which subcommands use those options.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 62 ++++++++++++++++++++++++++++++----------------
1 file changed, 40 insertions(+), 22 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index c30c01785cc..3e6ad6b6a84 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -18,10 +18,17 @@ static const char * const show_ref_usage[] = {
NULL
};
-static int deref_tags, show_head, tags_only, heads_only, verify,
- quiet, hash_only, abbrev;
+static int show_head, tags_only, heads_only, verify;
-static void show_one(const char *refname, const struct object_id *oid)
+struct show_one_options {
+ int quiet;
+ int hash_only;
+ int abbrev;
+ int deref_tags;
+};
+
+static void show_one(const struct show_one_options *opts,
+ const char *refname, const struct object_id *oid)
{
const char *hex;
struct object_id peeled;
@@ -30,25 +37,26 @@ static void show_one(const char *refname, const struct object_id *oid)
die("git show-ref: bad ref %s (%s)", refname,
oid_to_hex(oid));
- if (quiet)
+ if (opts->quiet)
return;
- hex = repo_find_unique_abbrev(the_repository, oid, abbrev);
- if (hash_only)
+ hex = repo_find_unique_abbrev(the_repository, oid, opts->abbrev);
+ if (opts->hash_only)
printf("%s\n", hex);
else
printf("%s %s\n", hex, refname);
- if (!deref_tags)
+ if (!opts->deref_tags)
return;
if (!peel_iterated_oid(oid, &peeled)) {
- hex = repo_find_unique_abbrev(the_repository, &peeled, abbrev);
+ hex = repo_find_unique_abbrev(the_repository, &peeled, opts->abbrev);
printf("%s %s^{}\n", hex, refname);
}
}
struct show_ref_data {
+ const struct show_one_options *show_one_opts;
const char **patterns;
int found_match;
};
@@ -81,7 +89,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
match:
data->found_match++;
- show_one(refname, oid);
+ show_one(data->show_one_opts, refname, oid);
return 0;
}
@@ -149,7 +157,8 @@ static int cmd_show_ref__exclude_existing(const struct exclude_existing_options
return 0;
}
-static int cmd_show_ref__verify(const char **refs)
+static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
+ const char **refs)
{
if (!refs || !*refs)
die("--verify requires a reference");
@@ -159,9 +168,9 @@ static int cmd_show_ref__verify(const char **refs)
if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
!read_ref(*refs, &oid)) {
- show_one(*refs, &oid);
+ show_one(show_one_opts, *refs, &oid);
}
- else if (!quiet)
+ else if (!show_one_opts->quiet)
die("'%s' - not a valid ref", *refs);
else
return 1;
@@ -171,9 +180,12 @@ static int cmd_show_ref__verify(const char **refs)
return 0;
}
-static int cmd_show_ref__patterns(const char **patterns)
+static int cmd_show_ref__patterns(const struct show_one_options *show_one_opts,
+ const char **patterns)
{
- struct show_ref_data show_ref_data = {0};
+ struct show_ref_data show_ref_data = {
+ .show_one_opts = show_one_opts,
+ };
if (patterns && *patterns)
show_ref_data.patterns = patterns;
@@ -196,11 +208,16 @@ static int cmd_show_ref__patterns(const char **patterns)
static int hash_callback(const struct option *opt, const char *arg, int unset)
{
- hash_only = 1;
+ struct show_one_options *opts = opt->value;
+ struct option abbrev_opt = *opt;
+
+ opts->hash_only = 1;
/* Use full length SHA1 if no argument */
if (!arg)
return 0;
- return parse_opt_abbrev_cb(opt, arg, unset);
+
+ abbrev_opt.value = &opts->abbrev;
+ return parse_opt_abbrev_cb(&abbrev_opt, arg, unset);
}
static int exclude_existing_callback(const struct option *opt, const char *arg,
@@ -216,6 +233,7 @@ static int exclude_existing_callback(const struct option *opt, const char *arg,
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
struct exclude_existing_options exclude_existing_opts = {0};
+ struct show_one_options show_one_opts = {0};
const struct option show_ref_options[] = {
OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
@@ -225,13 +243,13 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
N_("show the HEAD reference, even if it would be filtered out")),
OPT_BOOL(0, "head", &show_head,
N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL('d', "dereference", &deref_tags,
+ OPT_BOOL('d', "dereference", &show_one_opts.deref_tags,
N_("dereference tags into object IDs")),
- OPT_CALLBACK_F('s', "hash", &abbrev, N_("n"),
+ OPT_CALLBACK_F('s', "hash", &show_one_opts, N_("n"),
N_("only show SHA1 hash using <n> digits"),
PARSE_OPT_OPTARG, &hash_callback),
- OPT__ABBREV(&abbrev),
- OPT__QUIET(&quiet,
+ OPT__ABBREV(&show_one_opts.abbrev),
+ OPT__QUIET(&show_one_opts.quiet,
N_("do not print results to stdout (useful with --verify)")),
OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_opts,
N_("pattern"), N_("show refs from stdin that aren't in local repository"),
@@ -247,7 +265,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
- return cmd_show_ref__verify(argv);
+ return cmd_show_ref__verify(&show_one_opts, argv);
else
- return cmd_show_ref__patterns(argv);
+ return cmd_show_ref__patterns(&show_one_opts, argv);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 06/12] builtin/show-ref: stop using global variable to count matches
From: Patrick Steinhardt @ 2023-10-26 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1729 bytes --]
When passing patterns to git-show-ref(1) we're checking whether any
reference matches -- if none does, we indicate this condition via an
unsuccessful exit code.
We're using a global variable to count these matches, which is required
because the counter is getting incremented in a callback function. But
now that we have the `struct show_ref_data` in place, we can get rid of
the global variable and put the counter in there instead.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 90481c58492..c30c01785cc 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -18,7 +18,7 @@ static const char * const show_ref_usage[] = {
NULL
};
-static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
+static int deref_tags, show_head, tags_only, heads_only, verify,
quiet, hash_only, abbrev;
static void show_one(const char *refname, const struct object_id *oid)
@@ -50,6 +50,7 @@ static void show_one(const char *refname, const struct object_id *oid)
struct show_ref_data {
const char **patterns;
+ int found_match;
};
static int show_ref(const char *refname, const struct object_id *oid,
@@ -78,7 +79,7 @@ static int show_ref(const char *refname, const struct object_id *oid,
}
match:
- found_match++;
+ data->found_match++;
show_one(refname, oid);
@@ -187,7 +188,7 @@ static int cmd_show_ref__patterns(const char **patterns)
} else {
for_each_ref(show_ref, &show_ref_data);
}
- if (!found_match)
+ if (!show_ref_data.found_match)
return 1;
return 0;
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 05/12] builtin/show-ref: refactor `--exclude-existing` options
From: Patrick Steinhardt @ 2023-10-26 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 5814 bytes --]
It's not immediately obvious options which options are applicable to
what subcommand in git-show-ref(1) because all options exist as global
state. This can easily cause confusion for the reader.
Refactor options for the `--exclude-existing` subcommand to be contained
in a separate structure. This structure is stored on the stack and
passed down as required. Consequently, it clearly delimits the scope of
those options and requires the reader to worry less about global state.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 74 +++++++++++++++++++++++++---------------------
1 file changed, 40 insertions(+), 34 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index f95418d3d16..90481c58492 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -19,8 +19,7 @@ static const char * const show_ref_usage[] = {
};
static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
- quiet, hash_only, abbrev, exclude_arg;
-static const char *exclude_existing_arg;
+ quiet, hash_only, abbrev;
static void show_one(const char *refname, const struct object_id *oid)
{
@@ -95,6 +94,11 @@ static int add_existing(const char *refname,
return 0;
}
+struct exclude_existing_options {
+ int enabled;
+ const char *pattern;
+};
+
/*
* read "^(?:<anything>\s)?<refname>(?:\^\{\})?$" from the standard input,
* and
@@ -104,11 +108,11 @@ static int add_existing(const char *refname,
* (4) ignore if refname is a ref that exists in the local repository;
* (5) otherwise output the line.
*/
-static int cmd_show_ref__exclude_existing(const char *match)
+static int cmd_show_ref__exclude_existing(const struct exclude_existing_options *opts)
{
struct string_list existing_refs = STRING_LIST_INIT_DUP;
char buf[1024];
- int matchlen = match ? strlen(match) : 0;
+ int patternlen = opts->pattern ? strlen(opts->pattern) : 0;
for_each_ref(add_existing, &existing_refs);
while (fgets(buf, sizeof(buf), stdin)) {
@@ -124,11 +128,11 @@ static int cmd_show_ref__exclude_existing(const char *match)
for (ref = buf + len; buf < ref; ref--)
if (isspace(ref[-1]))
break;
- if (match) {
+ if (opts->pattern) {
int reflen = buf + len - ref;
- if (reflen < matchlen)
+ if (reflen < patternlen)
continue;
- if (strncmp(ref, match, matchlen))
+ if (strncmp(ref, opts->pattern, patternlen))
continue;
}
if (check_refname_format(ref, 0)) {
@@ -201,44 +205,46 @@ static int hash_callback(const struct option *opt, const char *arg, int unset)
static int exclude_existing_callback(const struct option *opt, const char *arg,
int unset)
{
+ struct exclude_existing_options *opts = opt->value;
BUG_ON_OPT_NEG(unset);
- exclude_arg = 1;
- *(const char **)opt->value = arg;
+ opts->enabled = 1;
+ opts->pattern = arg;
return 0;
}
-static const struct option show_ref_options[] = {
- OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
- OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
- OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
- "requires exact ref path")),
- OPT_HIDDEN_BOOL('h', NULL, &show_head,
- N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL(0, "head", &show_head,
- N_("show the HEAD reference, even if it would be filtered out")),
- OPT_BOOL('d', "dereference", &deref_tags,
- N_("dereference tags into object IDs")),
- OPT_CALLBACK_F('s', "hash", &abbrev, N_("n"),
- N_("only show SHA1 hash using <n> digits"),
- PARSE_OPT_OPTARG, &hash_callback),
- OPT__ABBREV(&abbrev),
- OPT__QUIET(&quiet,
- N_("do not print results to stdout (useful with --verify)")),
- OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_arg,
- N_("pattern"), N_("show refs from stdin that aren't in local repository"),
- PARSE_OPT_OPTARG | PARSE_OPT_NONEG, exclude_existing_callback),
- OPT_END()
-};
-
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
+ struct exclude_existing_options exclude_existing_opts = {0};
+ const struct option show_ref_options[] = {
+ OPT_BOOL(0, "tags", &tags_only, N_("only show tags (can be combined with heads)")),
+ OPT_BOOL(0, "heads", &heads_only, N_("only show heads (can be combined with tags)")),
+ OPT_BOOL(0, "verify", &verify, N_("stricter reference checking, "
+ "requires exact ref path")),
+ OPT_HIDDEN_BOOL('h', NULL, &show_head,
+ N_("show the HEAD reference, even if it would be filtered out")),
+ OPT_BOOL(0, "head", &show_head,
+ N_("show the HEAD reference, even if it would be filtered out")),
+ OPT_BOOL('d', "dereference", &deref_tags,
+ N_("dereference tags into object IDs")),
+ OPT_CALLBACK_F('s', "hash", &abbrev, N_("n"),
+ N_("only show SHA1 hash using <n> digits"),
+ PARSE_OPT_OPTARG, &hash_callback),
+ OPT__ABBREV(&abbrev),
+ OPT__QUIET(&quiet,
+ N_("do not print results to stdout (useful with --verify)")),
+ OPT_CALLBACK_F(0, "exclude-existing", &exclude_existing_opts,
+ N_("pattern"), N_("show refs from stdin that aren't in local repository"),
+ PARSE_OPT_OPTARG | PARSE_OPT_NONEG, exclude_existing_callback),
+ OPT_END()
+ };
+
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
- if (exclude_arg)
- return cmd_show_ref__exclude_existing(exclude_existing_arg);
+ if (exclude_existing_opts.enabled)
+ return cmd_show_ref__exclude_existing(&exclude_existing_opts);
else if (verify)
return cmd_show_ref__verify(argv);
else
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 04/12] builtin/show-ref: fix dead code when passing patterns
From: Patrick Steinhardt @ 2023-10-26 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 914 bytes --]
When passing patterns to `git show-ref` we have some code that will
cause us to die if `verify && !quiet` is true. But because `verify`
indicates a different subcommand of git-show-ref(1) that causes us to
execute `cmd_show_ref__verify()` and not `cmd_show_ref__patterns()`, the
condition cannot ever be true.
Let's remove this dead code.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index e55c38af478..f95418d3d16 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -183,11 +183,8 @@ static int cmd_show_ref__patterns(const char **patterns)
} else {
for_each_ref(show_ref, &show_ref_data);
}
- if (!found_match) {
- if (verify && !quiet)
- die("No match");
+ if (!found_match)
return 1;
- }
return 0;
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 03/12] builtin/show-ref: fix leaking string buffer
From: Patrick Steinhardt @ 2023-10-26 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]
Fix a leaking string buffer in `git show-ref --exclude-existing`. While
the buffer is technically not leaking because its variable is declared
as static, there is no inherent reason why it should be.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index cad5b8b5066..e55c38af478 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -106,7 +106,7 @@ static int add_existing(const char *refname,
*/
static int cmd_show_ref__exclude_existing(const char *match)
{
- static struct string_list existing_refs = STRING_LIST_INIT_DUP;
+ struct string_list existing_refs = STRING_LIST_INIT_DUP;
char buf[1024];
int matchlen = match ? strlen(match) : 0;
@@ -139,6 +139,8 @@ static int cmd_show_ref__exclude_existing(const char *match)
printf("%s\n", buf);
}
}
+
+ string_list_clear(&existing_refs, 0);
return 0;
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 02/12] builtin/show-ref: split up different subcommands
From: Patrick Steinhardt @ 2023-10-26 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4259 bytes --]
While not immediately obvious, git-show-ref(1) actually implements three
different subcommands:
- `git show-ref <patterns>` can be used to list references that
match a specific pattern.
- `git show-ref --verify <refs>` can be used to list references.
These are _not_ patterns.
- `git show-ref --exclude-existing` can be used as a filter that
reads references from standard input, performing some conversions
on each of them.
Let's make this more explicit in the code by splitting up the three
subcommands into separate functions. This also allows us to address the
confusingly named `patterns` variable, which may hold either patterns or
reference names.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 101 ++++++++++++++++++++++++---------------------
1 file changed, 54 insertions(+), 47 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 7efab14b96c..cad5b8b5066 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -104,7 +104,7 @@ static int add_existing(const char *refname,
* (4) ignore if refname is a ref that exists in the local repository;
* (5) otherwise output the line.
*/
-static int exclude_existing(const char *match)
+static int cmd_show_ref__exclude_existing(const char *match)
{
static struct string_list existing_refs = STRING_LIST_INIT_DUP;
char buf[1024];
@@ -142,6 +142,54 @@ static int exclude_existing(const char *match)
return 0;
}
+static int cmd_show_ref__verify(const char **refs)
+{
+ if (!refs || !*refs)
+ die("--verify requires a reference");
+
+ while (*refs) {
+ struct object_id oid;
+
+ if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
+ !read_ref(*refs, &oid)) {
+ show_one(*refs, &oid);
+ }
+ else if (!quiet)
+ die("'%s' - not a valid ref", *refs);
+ else
+ return 1;
+ refs++;
+ }
+
+ return 0;
+}
+
+static int cmd_show_ref__patterns(const char **patterns)
+{
+ struct show_ref_data show_ref_data = {0};
+
+ if (patterns && *patterns)
+ show_ref_data.patterns = patterns;
+
+ if (show_head)
+ head_ref(show_ref, &show_ref_data);
+ if (heads_only || tags_only) {
+ if (heads_only)
+ for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
+ if (tags_only)
+ for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
+ } else {
+ for_each_ref(show_ref, &show_ref_data);
+ }
+ if (!found_match) {
+ if (verify && !quiet)
+ die("No match");
+ return 1;
+ }
+
+ return 0;
+}
+
static int hash_callback(const struct option *opt, const char *arg, int unset)
{
hash_only = 1;
@@ -185,56 +233,15 @@ static const struct option show_ref_options[] = {
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
- struct show_ref_data show_ref_data = {0};
- const char **patterns;
-
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, show_ref_options,
show_ref_usage, 0);
if (exclude_arg)
- return exclude_existing(exclude_existing_arg);
-
- patterns = argv;
- if (!*patterns)
- patterns = NULL;
-
- if (verify) {
- if (!patterns)
- die("--verify requires a reference");
- while (*patterns) {
- struct object_id oid;
-
- if ((starts_with(*patterns, "refs/") || !strcmp(*patterns, "HEAD")) &&
- !read_ref(*patterns, &oid)) {
- show_one(*patterns, &oid);
- }
- else if (!quiet)
- die("'%s' - not a valid ref", *patterns);
- else
- return 1;
- patterns++;
- }
- return 0;
- }
-
- show_ref_data.patterns = patterns;
-
- if (show_head)
- head_ref(show_ref, &show_ref_data);
- if (heads_only || tags_only) {
- if (heads_only)
- for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
- if (tags_only)
- for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
- } else {
- for_each_ref(show_ref, &show_ref_data);
- }
- if (!found_match) {
- if (verify && !quiet)
- die("No match");
- return 1;
- }
- return 0;
+ return cmd_show_ref__exclude_existing(exclude_existing_arg);
+ else if (verify)
+ return cmd_show_ref__verify(argv);
+ else
+ return cmd_show_ref__patterns(argv);
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 01/12] builtin/show-ref: convert pattern to a local variable
From: Patrick Steinhardt @ 2023-10-26 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698314128.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3886 bytes --]
The `pattern` variable is a global variable that tracks either the
reference names (not patterns!) for the `--verify` mode or the patterns
for the non-verify mode. This is a bit confusing due to the slightly
different meanings.
Convert the variable to be local. While this does not yet fix the double
meaning of the variable, this change allows us to address it in a
subsequent patch more easily by explicitly splitting up the different
subcommands of git-show-ref(1).
Note that we introduce a `struct show_ref_data` to pass the patterns to
`show_ref()`. While this is overengineered now, we will extend this
structure in a subsequent patch.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/show-ref.c | 46 ++++++++++++++++++++++++++++------------------
1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 5110814f796..7efab14b96c 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -20,7 +20,6 @@ static const char * const show_ref_usage[] = {
static int deref_tags, show_head, tags_only, heads_only, found_match, verify,
quiet, hash_only, abbrev, exclude_arg;
-static const char **pattern;
static const char *exclude_existing_arg;
static void show_one(const char *refname, const struct object_id *oid)
@@ -50,15 +49,21 @@ static void show_one(const char *refname, const struct object_id *oid)
}
}
+struct show_ref_data {
+ const char **patterns;
+};
+
static int show_ref(const char *refname, const struct object_id *oid,
- int flag UNUSED, void *cbdata UNUSED)
+ int flag UNUSED, void *cbdata)
{
+ struct show_ref_data *data = cbdata;
+
if (show_head && !strcmp(refname, "HEAD"))
goto match;
- if (pattern) {
+ if (data->patterns) {
int reflen = strlen(refname);
- const char **p = pattern, *m;
+ const char **p = data->patterns, *m;
while ((m = *p++) != NULL) {
int len = strlen(m);
if (len > reflen)
@@ -180,6 +185,9 @@ static const struct option show_ref_options[] = {
int cmd_show_ref(int argc, const char **argv, const char *prefix)
{
+ struct show_ref_data show_ref_data = {0};
+ const char **patterns;
+
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, show_ref_options,
@@ -188,38 +196,40 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
if (exclude_arg)
return exclude_existing(exclude_existing_arg);
- pattern = argv;
- if (!*pattern)
- pattern = NULL;
+ patterns = argv;
+ if (!*patterns)
+ patterns = NULL;
if (verify) {
- if (!pattern)
+ if (!patterns)
die("--verify requires a reference");
- while (*pattern) {
+ while (*patterns) {
struct object_id oid;
- if ((starts_with(*pattern, "refs/") || !strcmp(*pattern, "HEAD")) &&
- !read_ref(*pattern, &oid)) {
- show_one(*pattern, &oid);
+ if ((starts_with(*patterns, "refs/") || !strcmp(*patterns, "HEAD")) &&
+ !read_ref(*patterns, &oid)) {
+ show_one(*patterns, &oid);
}
else if (!quiet)
- die("'%s' - not a valid ref", *pattern);
+ die("'%s' - not a valid ref", *patterns);
else
return 1;
- pattern++;
+ patterns++;
}
return 0;
}
+ show_ref_data.patterns = patterns;
+
if (show_head)
- head_ref(show_ref, NULL);
+ head_ref(show_ref, &show_ref_data);
if (heads_only || tags_only) {
if (heads_only)
- for_each_fullref_in("refs/heads/", show_ref, NULL);
+ for_each_fullref_in("refs/heads/", show_ref, &show_ref_data);
if (tags_only)
- for_each_fullref_in("refs/tags/", show_ref, NULL);
+ for_each_fullref_in("refs/tags/", show_ref, &show_ref_data);
} else {
- for_each_ref(show_ref, NULL);
+ for_each_ref(show_ref, &show_ref_data);
}
if (!found_match) {
if (verify && !quiet)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v2 00/12] show-ref: introduce mode to check for ref existence
From: Patrick Steinhardt @ 2023-10-26 9:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Eric Sunshine, Han-Wen Nienhuys
In-Reply-To: <cover.1698152926.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 14503 bytes --]
Hi,
this is the second version of my patch series that introduces a new `git
show-ref --exists` mode to check for reference existence.
Changes compared to v1:
- Various typo fixes in commit messages.
- Stopped using non-constant designated initializers.
- Renamed a varible from `matchlen` to `patternlen` to match another
renamed variable better.
- Improved the error message for mutually exclusive modes to be
easier to handle for translators.
- "--quiet" is not advertised for the pattern-based mode of
git-show-ref(1) anymore.
- Rephrased "error code" to "exit code" in the documentation.
Thanks for the feedback so far!
Patrick
Patrick Steinhardt (12):
builtin/show-ref: convert pattern to a local variable
builtin/show-ref: split up different subcommands
builtin/show-ref: fix leaking string buffer
builtin/show-ref: fix dead code when passing patterns
builtin/show-ref: refactor `--exclude-existing` options
builtin/show-ref: stop using global variable to count matches
builtin/show-ref: stop using global vars for `show_one()`
builtin/show-ref: refactor options for patterns subcommand
builtin/show-ref: ensure mutual exclusiveness of subcommands
builtin/show-ref: explicitly spell out different modes in synopsis
builtin/show-ref: add new mode to check for reference existence
t: use git-show-ref(1) to check for ref existence
Documentation/git-show-ref.txt | 20 ++-
builtin/show-ref.c | 280 ++++++++++++++++++++++-----------
t/t1403-show-ref.sh | 70 +++++++++
t/t1430-bad-ref-name.sh | 27 ++--
t/t3200-branch.sh | 33 ++--
t/t5521-pull-options.sh | 4 +-
t/t5605-clone-local.sh | 2 +-
t/test-lib-functions.sh | 55 +++++++
8 files changed, 369 insertions(+), 122 deletions(-)
Range-diff against v1:
1: 78163accbd2 = 1: 78163accbd2 builtin/show-ref: convert pattern to a local variable
2: 7e6ab5dee23 ! 2: 9a234622d99 builtin/show-ref: split up different subcommands
@@ builtin/show-ref.c: static int exclude_existing(const char *match)
+
+static int cmd_show_ref__patterns(const char **patterns)
+{
-+ struct show_ref_data show_ref_data = {
-+ .patterns = (patterns && *patterns) ? patterns : NULL,
-+ };
++ struct show_ref_data show_ref_data = {0};
++
++ if (patterns && *patterns)
++ show_ref_data.patterns = patterns;
+
+ if (show_head)
+ head_ref(show_ref, &show_ref_data);
3: ae2e401fbd8 = 3: bb0d656a0b4 builtin/show-ref: fix leaking string buffer
4: 29c0c0c6c97 ! 4: 87afcee830c builtin/show-ref: fix dead code when passing patterns
@@ Commit message
builtin/show-ref: fix dead code when passing patterns
When passing patterns to `git show-ref` we have some code that will
- cause us to die of `verify && !quiet` is true. But because `verify`
+ cause us to die if `verify && !quiet` is true. But because `verify`
indicates a different subcommand of git-show-ref(1) that causes us to
execute `cmd_show_ref__verify()` and not `cmd_show_ref__patterns()`, the
condition cannot ever be true.
5: 8d0b0b5700c ! 5: bed2a8a0769 builtin/show-ref: refactor `--exclude-existing` options
@@ Commit message
builtin/show-ref: refactor `--exclude-existing` options
It's not immediately obvious options which options are applicable to
- what subcommand ni git-show-ref(1) because all options exist as global
+ what subcommand in git-show-ref(1) because all options exist as global
state. This can easily cause confusion for the reader.
Refactor options for the `--exclude-existing` subcommand to be contained
in a separate structure. This structure is stored on the stack and
- passed down as required. Consequentially, it clearly delimits the scope
- of those options and requires the reader to worry less about global
- state.
+ passed down as required. Consequently, it clearly delimits the scope of
+ those options and requires the reader to worry less about global state.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
@@ builtin/show-ref.c: static int add_existing(const char *refname,
struct string_list existing_refs = STRING_LIST_INIT_DUP;
char buf[1024];
- int matchlen = match ? strlen(match) : 0;
-+ int matchlen = opts->pattern ? strlen(opts->pattern) : 0;
++ int patternlen = opts->pattern ? strlen(opts->pattern) : 0;
for_each_ref(add_existing, &existing_refs);
while (fgets(buf, sizeof(buf), stdin)) {
@@ builtin/show-ref.c: static int cmd_show_ref__exclude_existing(const char *match)
- if (match) {
+ if (opts->pattern) {
int reflen = buf + len - ref;
- if (reflen < matchlen)
+- if (reflen < matchlen)
++ if (reflen < patternlen)
continue;
- if (strncmp(ref, match, matchlen))
-+ if (strncmp(ref, opts->pattern, matchlen))
++ if (strncmp(ref, opts->pattern, patternlen))
continue;
}
if (check_refname_format(ref, 0)) {
6: 6e0f3d4e104 = 6: d52a5e8ced2 builtin/show-ref: stop using global variable to count matches
7: 2da1e65dd8f ! 7: 63f1dadf4c2 builtin/show-ref: stop using global vars for `show_one()`
@@ builtin/show-ref.c: static int cmd_show_ref__verify(const char **refs)
+static int cmd_show_ref__patterns(const struct show_one_options *show_one_opts,
+ const char **patterns)
{
- struct show_ref_data show_ref_data = {
+- struct show_ref_data show_ref_data = {0};
++ struct show_ref_data show_ref_data = {
+ .show_one_opts = show_one_opts,
- .patterns = (patterns && *patterns) ? patterns : NULL,
- };
++ };
+ if (patterns && *patterns)
+ show_ref_data.patterns = patterns;
@@ builtin/show-ref.c: static int cmd_show_ref__patterns(const char **patterns)
static int hash_callback(const struct option *opt, const char *arg, int unset)
8: 805889eda4c ! 8: 88dfeaa4871 builtin/show-ref: refactor options for patterns subcommand
@@ builtin/show-ref.c: static int cmd_show_ref__verify(const struct show_one_option
struct show_ref_data show_ref_data = {
.show_one_opts = show_one_opts,
+ .show_head = opts->show_head,
- .patterns = (patterns && *patterns) ? patterns : NULL,
};
+ if (patterns && *patterns)
+ show_ref_data.patterns = patterns;
+
- if (show_head)
+ if (opts->show_head)
head_ref(show_ref, &show_ref_data);
9: d0a991cf4f8 ! 9: 5ba566723e8 builtin/show-ref: ensure mutual exclusiveness of subcommands
@@ builtin/show-ref.c: int cmd_show_ref(int argc, const char **argv, const char *pr
show_ref_usage, 0);
+ if ((!!exclude_existing_opts.enabled + !!verify) > 1)
-+ die(_("only one of --exclude-existing or --verify can be given"));
++ die(_("only one of '%s' or '%s' can be given"),
++ "--exclude-existing", "--verify");
+
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
@@ t/t1403-show-ref.sh: test_expect_success 'show-ref --verify with dangling ref' '
+test_expect_success 'show-ref sub-modes are mutually exclusive' '
+ test_must_fail git show-ref --verify --exclude-existing 2>err &&
-+ grep "only one of --exclude-existing or --verify can be given" err
++ grep "only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given" err
+'
+
test_done
10: adcfa7a6a9d ! 10: b78ccc5f692 builtin/show-ref: explicitly spell out different modes in synopsis
@@ Commit message
Split up the synopsis for these two modes such that we can disambiguate
those differences.
+ While at it, drop "--quiet" from the pattern mode's synopsis. It does
+ not make a lot of sense to list patterns, but squelch the listing output
+ itself. The description for "--quiet" is adapted accordingly.
+
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## Documentation/git-show-ref.txt ##
@@ Documentation/git-show-ref.txt: git-show-ref - List references in a local reposi
--------
[verse]
-'git show-ref' [-q | --quiet] [--verify] [--head] [-d | --dereference]
-+'git show-ref' [-q | --quiet] [--head] [-d | --dereference]
++'git show-ref' [--head] [-d | --dereference]
[-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]
[--heads] [--] [<pattern>...]
+'git show-ref' --verify [-q | --quiet] [-d | --dereference]
@@ Documentation/git-show-ref.txt: git-show-ref - List references in a local reposi
'git show-ref' --exclude-existing[=<pattern>]
DESCRIPTION
+@@ Documentation/git-show-ref.txt: OPTIONS
+ -q::
+ --quiet::
+
+- Do not print any results to stdout. When combined with `--verify`, this
+- can be used to silently check if a reference exists.
++ Do not print any results to stdout. Can be used with `--verify` to
++ silently check if a reference exists.
+
+ --exclude-existing[=<pattern>]::
+
## builtin/show-ref.c ##
@@
@@ builtin/show-ref.c
static const char * const show_ref_usage[] = {
- N_("git show-ref [-q | --quiet] [--verify] [--head] [-d | --dereference]\n"
-+ N_("git show-ref [-q | --quiet] [--head] [-d | --dereference]\n"
++ N_("git show-ref [--head] [-d | --dereference]\n"
" [-s | --hash[=<n>]] [--abbrev[=<n>]] [--tags]\n"
" [--heads] [--] [<pattern>...]"),
+ N_("git show-ref --verify [-q | --quiet] [-d | --dereference]\n"
11: 2f876e61dd3 ! 11: 327942b1162 builtin/show-ref: add new mode to check for reference existence
@@ Commit message
- Dangling symrefs can be resolved via git-symbolic-ref(1), but this
requires the caller to special case existence checks depending on
- whteher or not a reference is symbolic or direct.
+ whether or not a reference is symbolic or direct.
Furthermore, git-rev-list(1) and other commands do not let the caller
distinguish easily between an actually missing reference and a generic
error.
- Taken together, this gseems like sufficient motivation to introduce a
+ Taken together, this seems like sufficient motivation to introduce a
separate plumbing command to explicitly check for the existence of a
reference without trying to resolve its contents.
This new command comes in the form of `git show-ref --exists`. This
new mode will exit successfully when the reference exists, with a
- specific error code of 2 when it does not exist, or with 1 when there
+ specific exit code of 2 when it does not exist, or with 1 when there
has been a generic error.
Note that the only way to properly implement this command is by using
@@ Documentation/git-show-ref.txt: OPTIONS
+--exists::
+
-+ Check whether the given reference exists. Returns an error code of 0 if
-+ it does, 2 if it is missing, and 128 in case looking up the reference
++ Check whether the given reference exists. Returns an exit code of 0 if
++ it does, 2 if it is missing, and 1 in case looking up the reference
+ failed with an error other than the reference being missing.
+
--abbrev[=<n>]::
@@ builtin/show-ref.c: static int cmd_show_ref__patterns(const struct patterns_opti
+ unsigned int unused_type;
+ int failure_errno = 0;
+ const char *ref;
-+ int ret = 1;
++ int ret = 0;
+
+ if (!refs || !*refs)
+ die("--exists requires a reference");
@@ builtin/show-ref.c: static int cmd_show_ref__patterns(const struct patterns_opti
+ error(_("reference does not exist"));
+ ret = 2;
+ } else {
-+ error(_("failed to look up reference: %s"), strerror(failure_errno));
++ errno = failure_errno;
++ error_errno(_("failed to look up reference"));
++ ret = 1;
+ }
+
+ goto out;
+ }
+
-+ ret = 0;
-+
+out:
+ strbuf_release(&unused_referent);
+ return ret;
@@ builtin/show-ref.c: int cmd_show_ref(int argc, const char **argv, const char *pr
show_ref_usage, 0);
- if ((!!exclude_existing_opts.enabled + !!verify) > 1)
-- die(_("only one of --exclude-existing or --verify can be given"));
+- die(_("only one of '%s' or '%s' can be given"),
+- "--exclude-existing", "--verify");
+ if ((!!exclude_existing_opts.enabled + !!verify + !!exists) > 1)
-+ die(_("only one of --exclude-existing, --exists or --verify can be given"));
++ die(_("only one of '%s', '%s' or '%s' can be given"),
++ "--exclude-existing", "--verify", "--exists");
if (exclude_existing_opts.enabled)
return cmd_show_ref__exclude_existing(&exclude_existing_opts);
@@ t/t1403-show-ref.sh: test_expect_success 'show-ref --verify with dangling ref' '
test_expect_success 'show-ref sub-modes are mutually exclusive' '
+ cat >expect <<-EOF &&
-+ fatal: only one of --exclude-existing, --exists or --verify can be given
++ fatal: only one of ${SQ}--exclude-existing${SQ}, ${SQ}--verify${SQ} or ${SQ}--exists${SQ} can be given
+ EOF
+
test_must_fail git show-ref --verify --exclude-existing 2>err &&
-- grep "only one of --exclude-existing or --verify can be given" err
+- grep "only one of ${SQ}--exclude-existing${SQ} or ${SQ}--verify${SQ} can be given" err
+ test_cmp expect err &&
+
+ test_must_fail git show-ref --verify --exists 2>err &&
12: a3a43d82e1f = 12: 226731c5f18 t: use git-show-ref(1) to check for ref existence
base-commit: a9ecda2788e229afc9b611acaa26d0d9d4da53ed
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 00/12] show-ref: introduce mode to check for ref existence
From: Patrick Steinhardt @ 2023-10-26 9:48 UTC (permalink / raw)
To: phillip.wood; +Cc: Han-Wen Nienhuys, Junio C Hamano, git, Eric Sunshine
In-Reply-To: <f87c95d6-b1e5-45c7-b380-bdc8b8143ab9@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]
On Wed, Oct 25, 2023 at 03:44:33PM +0100, Phillip Wood wrote:
> On 25/10/2023 15:26, Han-Wen Nienhuys wrote:
> > On Tue, Oct 24, 2023 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
> > >
> > > Patrick Steinhardt <ps@pks.im> writes:
> > >
> > > > this patch series introduces a new `--exists` mode to git-show-ref(1) to
> > > > explicitly check for the existence of a reference, only.
> > >
> > > I agree that show-ref would be the best place for this feature (not
> > > rev-parse, which is already a kitchen sink). After all, the command
> > > was designed for validating refs in 358ddb62 (Add "git show-ref"
> > > builtin command, 2006-09-15).
> > >
> > > Thanks. Hopefully I can take a look before I go offline.
> >
> > The series description doesn't say why users would care about this.
> >
> > If this is just to ease testing, I suggest adding functionality to a
> > suitable test helper. Anything you add to git-show-ref is a publicly
> > visible API that needs documentation and comes with a stability
> > guarantee that is more expensive to maintain than test helper
> > functionality.
>
> Does the new functionality provide a way for scripts to see if a branch is
> unborn (i.e. has not commits yet)? I don't think we have a way to
> distinguish between a ref that points to a missing object and an unborn
> branch at the moment.
You could do it with two commands:
```
target=$(git symbolic-ref HEAD)
git show-ref --exists "$target"
case "$?" in
2)
echo "unborn branch";;
0)
echo "branch exists";;
*)
echo "could be anything, dunno";;
esac
```
While you could use git-rev-parse(1) instead of git-show-ref(1), you
wouldn't be able to easily distinguish the case of a missing target
reference and any other kind of error.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 00/12] show-ref: introduce mode to check for ref existence
From: Patrick Steinhardt @ 2023-10-26 9:44 UTC (permalink / raw)
To: Han-Wen Nienhuys; +Cc: Junio C Hamano, git, Eric Sunshine
In-Reply-To: <CAFQ2z_PqNsz+zycSxz=q2cUVOpJS-AEjwHxEM-fiafxd3dxc9g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3457 bytes --]
On Wed, Oct 25, 2023 at 04:26:35PM +0200, Han-Wen Nienhuys wrote:
> On Tue, Oct 24, 2023 at 9:17 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Patrick Steinhardt <ps@pks.im> writes:
> >
> > > this patch series introduces a new `--exists` mode to git-show-ref(1) to
> > > explicitly check for the existence of a reference, only.
> >
> > I agree that show-ref would be the best place for this feature (not
> > rev-parse, which is already a kitchen sink). After all, the command
> > was designed for validating refs in 358ddb62 (Add "git show-ref"
> > builtin command, 2006-09-15).
> >
> > Thanks. Hopefully I can take a look before I go offline.
>
> The series description doesn't say why users would care about this.
>
> If this is just to ease testing, I suggest adding functionality to a
> suitable test helper. Anything you add to git-show-ref is a publicly
> visible API that needs documentation and comes with a stability
> guarantee that is more expensive to maintain than test helper
> functionality.
The first patch of the original patch series where I split this out from
did exactly that, see [1]. Junio questioned though whether this should
be part of production code instead of being a test helper.
And I tend to agree with him, or otherwise I wouldn't have written this
series. It's actually a bit surprising that we do not have any way to
test for reference existence in any of our helpers in a generic way. All
current tooling that I'm aware of is lacking in some ways:
- git-rev-parse(1) will fail to parse symbolic refs whose target
does not exist.
- git-symbolic-ref(1) can look up such unborn branches, but the
caller needs to be aware that
- git-show-ref(1) tries to resolve symbolic references.
- git-for-each-ref(1) is simply not an obvious way to check for ref
existence.
- All of these will fail to parse references with malformed names.
So the new `git show-ref --exists` mode is a trivial-to-use and generic
way to simply ask "Do you know this reference?". The lack of this option
likely shows that you can most often get away without such a tool, but I
still find it funny that there is no obvious way to perform this query
right now.
At the Contributor's Summit, we've also discussed the issue that our
plumbing layer has become less useful over the years. It is often
missing functionality that exists in user-facing commands. It also has
inherited many of the restrictions of our porcelain tools, like not
being able to look up references with bad names. So this series is a
small step into the direction of making our plumbing more useful again.
I also assume that it's only going to become more important to address
these limitations in our plumbing layer once we have something like the
reftable backend. A user or admin would have been able to fix issues
with misformatted referencen names rather easily in the reffiles backend
even without support in our plumbing layer, as it mostly was a single
"rm .git/refs/$broken_ref" away. But that's not going to be an easy
solution anymore with reftable due to the complexity of its format. So I
also see it as part of the upcoming preparatory work to make sure that
they have all the necessary tools to address such situations, at least
up to a reasonable point.
Patrick
[1]: <e947feb1c77f7e9f3c7f983bbe47137fbce42367.1697607222.git.ps@pks.im>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 5/5] ci: add support for GitLab CI
From: Oswald Buddenhagen @ 2023-10-26 9:07 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <35b07e5378d960b93ae8990a3abb525e1762d97d.1698305961.git.ps@pks.im>
On Thu, Oct 26, 2023 at 10:00:20AM +0200, Patrick Steinhardt wrote:
>project has. More esoteric jobs like "linux-TEST-vars" that also sets a
^
-s
>couple of environment variables do not exist in GitLab's custom CI
>setup, and maintaining them to keep up with what Git does feels like
>wasted time. The result is that we regularly send patch series upstream
>that would otherwise fail to compile or pass tests in GitHub Workflows.
^^^^^^^^^^^^^^^
that inverts the meaning
>[...]
>project to help us ensure to send better patch series upstream and thus
^^
"we"
>reduce overhead for the maintainer.
>--- a/ci/install-docker-dependencies.sh
>+++ b/ci/install-docker-dependencies.sh
>@@ -16,9 +19,13 @@ linux32)
> '
> ;;
> linux-musl)
>- apk add --update build-base curl-dev openssl-dev expat-dev gettext \
>+ apk add --update git shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
> pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
> ;;
>+linux-*)
>
you should probably choose a less generic name for the jobs, at least
debian-*.
>diff --git a/ci/lib.sh b/ci/lib.sh
>index 33005854520..102e9d04a1f 100755
>--- a/ci/lib.sh
>+++ b/ci/lib.sh
>@@ -15,6 +15,42 @@ then
> echo '::endgroup::' >&2
> }
>
>+ group () {
>[...]
>+ }
>
this counter-intutive patch structure reveals that the function is
duplicated between github and gitlab. you may want to factor it out and
alias it. or change the structure entirely (circling back to patch 1/5).
>+ CI_BRANCH="$CI_COMMIT_REF_NAME"
>+ CI_COMMIT="$CI_COMMIT_SHA"
>
assignments need no quoting to prevent word splitting.
repeats below.
>+ case "$CI_JOB_IMAGE" in
>
... as does the selector in case statements.
>--- a/ci/print-test-failures.sh
>+++ b/ci/print-test-failures.sh
>@@ -51,6 +51,12 @@ do
> tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
> continue
> ;;
>+ gitlab-ci)
>+ mkdir -p failed-test-artifacts
>+ cp "${TEST_EXIT%.exit}.out" failed-test-artifacts/
>+ tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
>
you're just following the precedent, but imo it's more legible to quote
the entire string, not just the variable expansion. the code doesn't
even agree with itself, as the line directly above apparently agrees
with me.
regards
^ permalink raw reply
* Re: [PATCH 4/5] ci: split out logic to set up failed test artifacts
From: Oswald Buddenhagen @ 2023-10-26 8:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <4a864a1d174f7d4d36202a375302d450fbe9f2a3.1698305961.git.ps@pks.im>
On Thu, Oct 26, 2023 at 10:00:16AM +0200, Patrick Steinhardt wrote:
>We have some logic in place to create a directory with the output from
>failed tests, which will then subsequently be uploaded as CI artifact.
^
+s
regards
^ permalink raw reply
* Re: [PATCH 3/5] ci: group installation of Docker dependencies
From: Oswald Buddenhagen @ 2023-10-26 8:34 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <a65d235dd3c14df4945b9753507d9cdab777966c.1698305961.git.ps@pks.im>
On Thu, Oct 26, 2023 at 10:00:12AM +0200, Patrick Steinhardt wrote:
>Pull in "lib.sh" into "install-docker-dependencies.sh" such that we can
>set up proper groups for those dependencise. This allows the reader to
^^ !!!
>collapse sections in the CI output on GitHub Actions (and later on on
>GitLab CI).
>
the structure of the text is kind of backwards - the fact that you need
to pull in a lib is just a consequence, not the intent, which imo should
come first. tough it mostly doesn't matter, as it's just one short
paragraph.
regards
^ permalink raw reply
* Re: [PATCH 1/5] ci: reorder definitions for grouping functions
From: Oswald Buddenhagen @ 2023-10-26 8:26 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <586a8d1003b6559177d238ceda2c6ef2f16cfb8d.1698305961.git.ps@pks.im>
On Thu, Oct 26, 2023 at 10:00:03AM +0200, Patrick Steinhardt wrote:
> [...]
>_not_ being GitLab Actions, where we define the non-stub logic in the
>
you meant GitHub here.
>else branch.
>
>Reorder the definitions such that we explicitly handle GitHub Actions.
>
i'd say something like "the conditional branches". imo that makes it
clearer that you're actually talking about code, not some markup or
whatever.
for that matter, this is my overall impression of the commit message -
it seems way too detached from the near-trivial fact that you're just
slightly adjusting the code structure to make it easier to implement a
cascade (aka a switch).
regards
^ permalink raw reply
* Re: [PATCH v5 3/5] bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
From: Patrick Steinhardt @ 2023-10-26 8:16 UTC (permalink / raw)
To: Eric Sunshine
Cc: Taylor Blau, git, Elijah Newren, Eric W. Biederman, Jeff King,
Junio C Hamano
In-Reply-To: <CAPig+cTjQe6FWo98LxvDS=s3dOs33SUUJa=x-bkyWHNBMx+XFw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]
On Wed, Oct 25, 2023 at 01:21:50PM -0400, Eric Sunshine wrote:
> On Wed, Oct 25, 2023 at 11:44 AM Taylor Blau <me@ttaylorr.com> wrote:
> > On Wed, Oct 25, 2023 at 09:58:06AM +0200, Patrick Steinhardt wrote:
> > > On Mon, Oct 23, 2023 at 06:45:01PM -0400, Taylor Blau wrote:
> > > > In order to support streaming from a location in memory, we must
> > > > implement a new kind of bulk_checkin_source that does just that. These
> > > > implementation in spread out across:
> > >
> > > Nit: the commit message is a bit off here. Probably not worth a reroll
> > > though.
> >
> > Your eyes are definitely mine, because I'm not seeing where the commit
> > message is off! But hopefully since you already don't think it's worth a
> > reroll, and I'm not even sure what the issue is that we can just leave
> > it ;-).
>
> Perhaps:
>
> s/implementation in/implementations are/
Yeah, that's what I referred to. Sorry for not pointing it out
explicitly :)
In any case, as stated before: I don't think any of my comments warrant
a reroll, and overall this patch series looks good to me.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 5/5] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-10-26 8:00 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1698305961.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7313 bytes --]
We already support Azure Pipelines and GitHub Workflows in the Git
project, but until now we do not have support for GitLab CI. While it is
arguably not in the interest of the Git project to maintain a ton of
different CI platforms, GitLab has recently ramped up its efforts and
tries to contribute to the Git project more regularly.
Part of a problem we hit at GitLab rather frequently is that our own,
custom CI setup we have is so different to the setup that the Git
project has. More esoteric jobs like "linux-TEST-vars" that also sets a
couple of environment variables do not exist in GitLab's custom CI
setup, and maintaining them to keep up with what Git does feels like
wasted time. The result is that we regularly send patch series upstream
that would otherwise fail to compile or pass tests in GitHub Workflows.
We would thus like to integrate the GitLab CI configuration into the Git
project to help us ensure to send better patch series upstream and thus
reduce overhead for the maintainer.
The integration does not necessarily have to be a first-class citizen,
which would in practice only add to the fallout that pipeline failures
have for the maintainer. That being said, we are happy to maintain this
alternative CI setup for the Git project and will make test results
available as part of our own mirror of the Git project at [1].
This commit introduces the integration into our regular CI scripts so
that most of the setup continues to be shared across all of the CI
solutions.
[1]: https://gitlab.com/gitlab-org/git
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.gitlab-ci.yml | 51 +++++++++++++++++++++++
ci/install-docker-dependencies.sh | 9 +++-
ci/lib.sh | 69 +++++++++++++++++++++++++++++++
ci/print-test-failures.sh | 6 +++
4 files changed, 134 insertions(+), 1 deletion(-)
create mode 100644 .gitlab-ci.yml
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
new file mode 100644
index 00000000000..43d3a961fa0
--- /dev/null
+++ b/.gitlab-ci.yml
@@ -0,0 +1,51 @@
+default:
+ timeout: 2h
+
+workflow:
+ rules:
+ - if: $CI_PIPELINE_SOURCE == "merge_request_event"
+ - if: $CI_COMMIT_TAG
+ - if: $CI_COMMIT_REF_PROTECTED == "true"
+
+test:
+ image: $image
+ before_script:
+ - ./ci/install-docker-dependencies.sh
+ script:
+ - useradd builder --home-dir "${CI_PROJECT_DIR}"
+ - chown -R builder "${CI_PROJECT_DIR}"
+ - sudo --preserve-env --set-home --user=builder ./ci/run-build-and-tests.sh
+ after_script:
+ - |
+ if test "$CI_JOB_STATUS" != 'success'
+ then
+ sudo --preserve-env --set-home --user=builder ./ci/print-test-failures.sh
+ fi
+ parallel:
+ matrix:
+ - jobname: linux-sha256
+ image: ubuntu:latest
+ CC: clang
+ - jobname: linux-gcc
+ image: ubuntu:20.04
+ CC: gcc
+ CC_PACKAGE: gcc-8
+ - jobname: linux-TEST-vars
+ image: ubuntu:20.04
+ CC: gcc
+ CC_PACKAGE: gcc-8
+ - jobname: linux-gcc-default
+ image: ubuntu:latest
+ CC: gcc
+ - jobname: linux-leaks
+ image: ubuntu:latest
+ CC: gcc
+ - jobname: linux-asan-ubsan
+ image: ubuntu:latest
+ CC: clang
+ - jobname: linux-musl
+ image: alpine:latest
+ artifacts:
+ paths:
+ - t/failed-test-artifacts
+ when: on_failure
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index d0bc19d3bb3..1cd92db1876 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -7,6 +7,9 @@
begin_group "Install dependencies"
+# Required so that apt doesn't wait for user input on certain packages.
+export DEBIAN_FRONTEND=noninteractive
+
case "$jobname" in
linux32)
linux32 --32bit i386 sh -c '
@@ -16,9 +19,13 @@ linux32)
'
;;
linux-musl)
- apk add --update build-base curl-dev openssl-dev expat-dev gettext \
+ apk add --update git shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
;;
+linux-*)
+ apt update -q &&
+ apt install -q -y sudo git make language-pack-is libsvn-perl apache2 libssl-dev libcurl4-openssl-dev libexpat-dev tcl tk gettext zlib1g-dev perl-modules liberror-perl libauthen-sasl-perl libemail-valid-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}}
+ ;;
pedantic)
dnf -yq update >/dev/null &&
dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
diff --git a/ci/lib.sh b/ci/lib.sh
index 33005854520..102e9d04a1f 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -15,6 +15,42 @@ then
echo '::endgroup::' >&2
}
+ group () {
+ set +x
+
+ group="$1"
+ shift
+ begin_group "$group"
+
+ # work around `dash` not supporting `set -o pipefail`
+ (
+ "$@" 2>&1
+ echo $? >exit.status
+ ) |
+ sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
+ res=$(cat exit.status)
+ rm exit.status
+
+ end_group "$group"
+ return $res
+ }
+elif test true = "$GITLAB_CI"
+then
+ begin_group () {
+ need_to_end_group=t
+ echo -e "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K$1"
+ trap "end_group '$1'" EXIT
+ set -x
+ }
+
+ end_group () {
+ test -n "$need_to_end_group" || return 0
+ set +x
+ need_to_end_group=
+ echo -e "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K"
+ trap - EXIT
+ }
+
group () {
set +x
@@ -209,6 +245,39 @@ then
MAKEFLAGS="$MAKEFLAGS --jobs=10"
test windows != "$CI_OS_NAME" ||
GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+elif test true = "$GITLAB_CI"
+then
+ CI_TYPE=gitlab-ci
+ CI_BRANCH="$CI_COMMIT_REF_NAME"
+ CI_COMMIT="$CI_COMMIT_SHA"
+ case "$CI_JOB_IMAGE" in
+ macos-*)
+ CI_OS_NAME=osx;;
+ alpine:*|ubuntu:*)
+ CI_OS_NAME=linux;;
+ *)
+ echo "Could not identify OS image" >&2
+ env >&2
+ exit 1
+ ;;
+ esac
+ CI_REPO_SLUG="$CI_PROJECT_PATH"
+ CI_JOB_ID="$CI_JOB_ID"
+ CC="${CC_PACKAGE:-${CC:-gcc}}"
+ DONT_SKIP_TAGS=t
+ handle_failed_tests () {
+ create_failed_test_artifacts
+ }
+
+ cache_dir="$HOME/none"
+
+ runs_on_pool=$(echo "$CI_JOB_IMAGE" | tr : -)
+
+ export GIT_PROVE_OPTS="--timer --jobs $(nproc)"
+ export GIT_TEST_OPTS="--verbose-log -x"
+ MAKEFLAGS="$MAKEFLAGS --jobs=$(nproc)"
+ test windows != "$CI_OS_NAME" ||
+ GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
else
echo "Could not identify CI type" >&2
env >&2
diff --git a/ci/print-test-failures.sh b/ci/print-test-failures.sh
index 57277eefcd0..c33ad4e3a22 100755
--- a/ci/print-test-failures.sh
+++ b/ci/print-test-failures.sh
@@ -51,6 +51,12 @@ do
tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
continue
;;
+ gitlab-ci)
+ mkdir -p failed-test-artifacts
+ cp "${TEST_EXIT%.exit}.out" failed-test-artifacts/
+ tar czf failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
+ continue
+ ;;
*)
echo "Unhandled CI type: $CI_TYPE" >&2
exit 1
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 4/5] ci: split out logic to set up failed test artifacts
From: Patrick Steinhardt @ 2023-10-26 8:00 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1698305961.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2380 bytes --]
We have some logic in place to create a directory with the output from
failed tests, which will then subsequently be uploaded as CI artifact.
We're about to add support for GitLab CI, which will want to reuse the
logic.
Split the logic into a separate function so that it is reusable.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 40 ++++++++++++++++++++++------------------
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 957fd152d9c..33005854520 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -137,6 +137,27 @@ handle_failed_tests () {
return 1
}
+create_failed_test_artifacts () {
+ mkdir -p t/failed-test-artifacts
+
+ for test_exit in t/test-results/*.exit
+ do
+ test 0 != "$(cat "$test_exit")" || continue
+
+ test_name="${test_exit%.exit}"
+ test_name="${test_name##*/}"
+ printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
+ echo "The full logs are in the 'print test failures' step below."
+ echo "See also the 'failed-tests-*' artifacts attached to this run."
+ cat "t/test-results/$test_name.markup"
+
+ trash_dir="t/trash directory.$test_name"
+ cp "t/test-results/$test_name.out" t/failed-test-artifacts/
+ tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
+ done
+ return 1
+}
+
# GitHub Action doesn't set TERM, which is required by tput
export TERM=${TERM:-dumb}
@@ -177,25 +198,8 @@ then
CC="${CC_PACKAGE:-${CC:-gcc}}"
DONT_SKIP_TAGS=t
handle_failed_tests () {
- mkdir -p t/failed-test-artifacts
echo "FAILED_TEST_ARTIFACTS=t/failed-test-artifacts" >>$GITHUB_ENV
-
- for test_exit in t/test-results/*.exit
- do
- test 0 != "$(cat "$test_exit")" || continue
-
- test_name="${test_exit%.exit}"
- test_name="${test_name##*/}"
- printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
- echo "The full logs are in the 'print test failures' step below."
- echo "See also the 'failed-tests-*' artifacts attached to this run."
- cat "t/test-results/$test_name.markup"
-
- trash_dir="t/trash directory.$test_name"
- cp "t/test-results/$test_name.out" t/failed-test-artifacts/
- tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
- done
- return 1
+ create_failed_test_artifacts
}
cache_dir="$HOME/none"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/5] ci: group installation of Docker dependencies
From: Patrick Steinhardt @ 2023-10-26 8:00 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1698305961.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]
Pull in "lib.sh" into "install-docker-dependencies.sh" such that we can
set up proper groups for those dependencise. This allows the reader to
collapse sections in the CI output on GitHub Actions (and later on on
GitLab CI).
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/install-docker-dependencies.sh | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index 78b7e326da6..d0bc19d3bb3 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -3,6 +3,10 @@
# Install dependencies required to build and test Git inside container
#
+. ${0%/*}/lib.sh
+
+begin_group "Install dependencies"
+
case "$jobname" in
linux32)
linux32 --32bit i386 sh -c '
@@ -20,3 +24,5 @@ pedantic)
dnf -yq install make gcc findutils diffutils perl python3 gettext zlib-devel expat-devel openssl-devel curl-devel pcre2-devel >/dev/null
;;
esac
+
+end_group "Install dependencies"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/5] ci: make grouping setup more generic
From: Patrick Steinhardt @ 2023-10-26 8:00 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1698305961.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]
Make the grouping setup more generic by always calling `begin_group ()`
and `end_group ()` regardless of whether we have stubbed those functions
or not. This ensures we can more readily add support for additional CI
platforms.
Second, this commit changes `end_group ()` to also accept a parameter
that indicates _which_ group should end. This will be required by a
later commit that introduces support for GitLab CI.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index eb384f4e952..957fd152d9c 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -14,12 +14,14 @@ then
need_to_end_group=
echo '::endgroup::' >&2
}
- trap end_group EXIT
group () {
set +x
- begin_group "$1"
+
+ group="$1"
shift
+ begin_group "$group"
+
# work around `dash` not supporting `set -o pipefail`
(
"$@" 2>&1
@@ -28,11 +30,10 @@ then
sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/'
res=$(cat exit.status)
rm exit.status
- end_group
+
+ end_group "$group"
return $res
}
-
- begin_group "CI setup"
else
begin_group () { :; }
end_group () { :; }
@@ -44,6 +45,9 @@ else
set -x
fi
+begin_group "CI setup"
+trap "end_group 'CI setup'" EXIT
+
# Set 'exit on error' for all CI scripts to let the caller know that
# something went wrong.
#
@@ -287,5 +291,5 @@ esac
MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
-end_group
+end_group "CI setup"
set -x
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/5] ci: add GitLab CI definition
From: Patrick Steinhardt @ 2023-10-26 7:59 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 3344 bytes --]
Hi,
this patch series adds GitLab CI definitions to the Git project.
At GitLab, we're have already and will continue to ramp up our
involvement with the Git project. I myself will be working almost
exclusively on Git in the context of the reftable reference backend.
This has surfaced some issues in our own workflow, and the current CI
integration is one of the biggest pain points I personally feel right
now.
It's happened multiple times already that we sent patch series upstream
that passed on our own self-made pipeline at GitLab, but that didn't
pass the GitHub Actions pipeline. This is because the latter is a lot
more involved than what we have. There are pipelines for:
- Various sanitizers in the form of the linux-leaks and
linux-asan-ubsan jobs.
- SHA256 in the form of the linux-sha256 job.
- The linux-TEST-vars job, which sets several environment variables
to non-default values.
- The linux-musl job that tests on Alpine Linux.
We have none of that. And while we could of course iterate on our own
pipeline definition, the current setup results in quite a convoluted
workflow on our side. While I realize that this is not the problem of
the Git project, I really hope that we can integrate GitLab CI into the
Git project.
And this is exactly what this patch series does: it adds GitLab-specific
knowledge to our CI scripts and adds a CI definition that builds on top
of those scripts. This is rather straight forward, as the scripts
already know to discern Azure Pipelines and GitHub Actions, and adding
a third item to this list feels quite natural. And by building on top of
the preexisting infra, the actual ".gitlab-ci.yml" is really quite
small.
I acknowledge that the Git project may not be willing to fully support
GitLab CI, and that's fine with me. If we want to further stress that
point then I'd also be perfectly happy to move the definitions into the
"contrib/" directory -- it would still be a huge win for our workflow.
In any case, I'm happy to keep on maintaining the intgeration with
GitLab CI, and if things break I'll do my best to fix them fast.
I hope that this sheds some light on my motivations here. I do not wish
to replace GitHub Actions and would be okay if this was only a
semi-supported thing. But it would help us at GitLab and by extension
also the Git project because we will hopefully send higher-quality patch
series to the mailing list. And maybe this is even useful to somebody
outside of GitLab.
If this is accepted I'll likely eventually iterate to also support macOS
and/or Windows. A full pipeline run of this can be found at [1].
Patrick
[1]: https://gitlab.com/gitlab-org/git/-/pipelines/1045746751
Patrick Steinhardt (5):
ci: reorder definitions for grouping functions
ci: make grouping setup more generic
ci: group installation of Docker dependencies
ci: split out logic to set up failed test artifacts
ci: add support for GitLab CI
.gitlab-ci.yml | 51 +++++++++++
ci/install-docker-dependencies.sh | 15 +++-
ci/lib.sh | 139 +++++++++++++++++++++++-------
ci/print-test-failures.sh | 6 ++
4 files changed, 179 insertions(+), 32 deletions(-)
create mode 100644 .gitlab-ci.yml
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH 1/5] ci: reorder definitions for grouping functions
From: Patrick Steinhardt @ 2023-10-26 8:00 UTC (permalink / raw)
To: git
In-Reply-To: <cover.1698305961.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]
We define a set of grouping functions that are used to group together
output in our CI, where these groups then end up as collapsible sections
in the respective pipeline platform. The way these functions are defined
is not easily extensible though as we have an up front check for the CI
_not_ being GitLab Actions, where we define the non-stub logic in the
else branch.
Reorder the definitions such that we explicitly handle GitHub Actions.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 6fbb5bade12..eb384f4e952 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -1,16 +1,7 @@
# Library of functions shared by all CI scripts
-if test true != "$GITHUB_ACTIONS"
+if test true = "$GITHUB_ACTIONS"
then
- begin_group () { :; }
- end_group () { :; }
-
- group () {
- shift
- "$@"
- }
- set -x
-else
begin_group () {
need_to_end_group=t
echo "::group::$1" >&2
@@ -42,6 +33,15 @@ else
}
begin_group "CI setup"
+else
+ begin_group () { :; }
+ end_group () { :; }
+
+ group () {
+ shift
+ "$@"
+ }
+ set -x
fi
# Set 'exit on error' for all CI scripts to let the caller know that
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: [PATCH v2] bugreport: reject positional arguments
From: Dragan Simic @ 2023-10-26 4:06 UTC (permalink / raw)
To: Eric Sunshine; +Cc: emilyshaffer, git, Emily Shaffer, Sheik
In-Reply-To: <CAPig+cTJFKp6RFdqJTpyL49V+M-zaTDbgpVd2OrgfWf4H-+K+g@mail.gmail.com>
On 2023-10-26 06:03, Eric Sunshine wrote:
> On Wed, Oct 25, 2023 at 11:52 PM Dragan Simic <dsimic@manjaro.org>
> wrote:
>> On 2023-10-26 05:43, Eric Sunshine wrote:
>> > On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@google.com> wrote:
>> >> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
>> >> @@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv,
>> >> const char *prefix)
>> >> + if (argc) {
>> >> + if (argv[0])
>> >> + error(_("unknown argument `%s'"), argv[0]);
>> >> + usage(bugreport_usage[0]);
>> >> + }
>> >
>> > Can it actually happen that argc is non-zero but argv[0] is NULL? (I
>> > don't have parse-options in front of me to check.) If not, then the
>> > extra `if (argv[0])` conditional may confuse future readers.
>>
>> According to https://stackoverflow.com/a/2794171/22330192 it can't,
>> but
>> argv[0] can be a zero-length string.
>
> This case is different, though, since, by this point, argv[] has been
> processed by Git's parse-options API. Here's the relevant comment from
> parse-options.h:
Ah, I see, thanks for the clarification.
> * parse_options() will filter out the processed options and leave
> the
> * non-option arguments in argv[]. argv0 is assumed program name and
> * skipped.
> *
> * Returns the number of arguments left in argv[].
>
> So, I think the `if (argv[0])` conditional is unnecessary, thus
> potentially confusing.
>
> It's possible that Emily meant `if (*argv[0])`, but even that seems
> undesirable since even a zero-length argv[0] provides some useful
> context.
>
> % git bugreport ""
> error: unknown argument `'
^ permalink raw reply
* Re: [PATCH v2] bugreport: reject positional arguments
From: Eric Sunshine @ 2023-10-26 4:03 UTC (permalink / raw)
To: Dragan Simic; +Cc: emilyshaffer, git, Emily Shaffer, Sheik
In-Reply-To: <8c82a138faa28a3c5d15a52b1d9c2c0f@manjaro.org>
On Wed, Oct 25, 2023 at 11:52 PM Dragan Simic <dsimic@manjaro.org> wrote:
> On 2023-10-26 05:43, Eric Sunshine wrote:
> > On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@google.com> wrote:
> >> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> >> @@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv,
> >> const char *prefix)
> >> + if (argc) {
> >> + if (argv[0])
> >> + error(_("unknown argument `%s'"), argv[0]);
> >> + usage(bugreport_usage[0]);
> >> + }
> >
> > Can it actually happen that argc is non-zero but argv[0] is NULL? (I
> > don't have parse-options in front of me to check.) If not, then the
> > extra `if (argv[0])` conditional may confuse future readers.
>
> According to https://stackoverflow.com/a/2794171/22330192 it can't, but
> argv[0] can be a zero-length string.
This case is different, though, since, by this point, argv[] has been
processed by Git's parse-options API. Here's the relevant comment from
parse-options.h:
* parse_options() will filter out the processed options and leave the
* non-option arguments in argv[]. argv0 is assumed program name and
* skipped.
*
* Returns the number of arguments left in argv[].
So, I think the `if (argv[0])` conditional is unnecessary, thus
potentially confusing.
It's possible that Emily meant `if (*argv[0])`, but even that seems
undesirable since even a zero-length argv[0] provides some useful
context.
% git bugreport ""
error: unknown argument `'
^ permalink raw reply
* Re: [PATCH v2] bugreport: reject positional arguments
From: Dragan Simic @ 2023-10-26 3:52 UTC (permalink / raw)
To: Eric Sunshine; +Cc: emilyshaffer, git, Emily Shaffer, Sheik
In-Reply-To: <CAPig+cT4G9vdu+se9Fbbs0TRCyPoAYFgVtkSwph_U=sWf-kQ9g@mail.gmail.com>
On 2023-10-26 05:43, Eric Sunshine wrote:
> On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@google.com> wrote:
>> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
>> @@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv,
>> const char *prefix)
>> + if (argc) {
>> + if (argv[0])
>> + error(_("unknown argument `%s'"), argv[0]);
>> + usage(bugreport_usage[0]);
>> + }
>
> Can it actually happen that argc is non-zero but argv[0] is NULL? (I
> don't have parse-options in front of me to check.) If not, then the
> extra `if (argv[0])` conditional may confuse future readers.
According to https://stackoverflow.com/a/2794171/22330192 it can't, but
argv[0] can be a zero-length string.
^ 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