* Re: [PATCH v3 0/5] config-parse: create config parsing library
From: Josh Steadmon @ 2023-10-25 19:37 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Taylor Blau, Junio C Hamano, git, calvinwan, glencbz
In-Reply-To: <20231024225005.1191555-1-jonathantanmy@google.com>
On 2023.10.24 15:50, Jonathan Tan wrote:
> Taylor Blau <me@ttaylorr.com> writes:
> > But I am not sure that I agree that this series is moving us in the
> > right direction necessarily. Or at least I am not convinced that
> > shipping the intermediate state is worth doing before we have callers
> > that could drop '#include "config.h"' for just the parser.
> >
> > This feels like churn that does not yield a tangible pay-off, at least
> > in the sense that the refactoring and code movement delivers us
> > something that we can substantively use today.
> >
> > I dunno.
> >
> > Thanks,
> > Taylor
>
> Thanks for calling this out. We do want our changes to be good for both
> the libification and the non-libification cases as much as possible. As
> it is, I do agree that since we won't have callers that can use the new
> parser header (I think the likeliest cause of having such a caller is
> if we have a "interpret-config" command, like "interpret-trailers"), we
> probably shouldn't merge this (at least, the last 2 patches).
>
> I think patches 1-3 are still usable (they make some internals of config
> parsing less confusing) but I'm also OK if we hold off on them until
> we find a compelling use case that motivates refactoring on the config
> parser.
Thanks everyone for the revived discussion here. I think I agree, this
series is not going in the right direction. Additionally, our internal
use case for this change has evaporated, so let's just drop the series.
We can pick it up again later if interest returns.
^ permalink raw reply
* Cloning new bare repository using ssh does not respect bare repository --initial-branch
From: Sheik @ 2023-10-25 19:57 UTC (permalink / raw)
To: git
Hi Maintainers,
When a new bare repository is cloned using SSH then cloned HEAD does not
point to initial branch from the bare repository. However if cloned
using filesystem then HEAD points to initial branch as expected.
Shouldn't cloning via ssh also exhibit the same behaviour as cloning via
filesystem?
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
#Create bare repository
su - $user
git init --bare --initial-branch=test test.git
#Clone repository
git clone ssh://$user@$computer:/home/$user/test.git test1
cd test1
#git remote show origin
#Output [ORIGIN1]
echo abc >> abc.txt && git add * && git commit -a -m test && git push
#Output...
* [new branch] master -> master
#git remote show origin
#Output [ORIGIN2]
What did you expect to happen? (Expected behavior)
Repository cloned using ssh should push to test branch just like
repository cloned using filesystem does below as configured in the bare
repository.
#Clone repository
git clone home/$user/test.git test2
#git remote show origin
#Output [ORIGIN3]
echo abc >> abc.txt && git add * && git commit -a -m test && git push
#Output...
* [new branch] test -> test
#git remote show origin
#Output [ORIGIN4]
What happened instead? (Actual behavior)
Repository cloned using ssh pushed to master branch disregarding
configuration in bare repository.
What's different between what you expected and what actually happened?
For ssh cloned repository test branch should been the default branch
however master was the default.
Anything else you want to add:
[ORIGIN1]
* remote origin
HEAD branch: (unknown)
Local branch configured for 'git pull':
master merges with remote master
[ORIGIN2]
* remote origin
HEAD branch: (unknown)
Remote branch:
master tracked
Local branch configured for 'git pull':
master merges with remote master
Local ref configured for 'git push':
master pushes to master (up to date)
[ORIGIN3]
* remote origin
HEAD branch: (unknown)
Local branch configured for 'git pull':
test merges with remote test
[ORIGIN4]
* remote origin
HEAD branch: test
Remote branches:
master new (next fetch will store in remotes/origin)
test tracked
Local branch configured for 'git pull':
test merges with remote test
Local ref configured for 'git push':
test pushes to test (up to date)
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1
(2023-10-07) x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
^ permalink raw reply
* Repository cloned using SSH does not respect bare repository initial branch
From: Sheik @ 2023-10-25 20:36 UTC (permalink / raw)
To: git
Hi Maintainers,
Repository cloned using SSH does not use the branch configured in the
bare repository however repository cloned using filesystem does as
expected. Shouldn't they both behave the same?
Thanks
Sheik
Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.
What did you do before the bug happened? (Steps to reproduce your issue)
#Create bare repository
su - $user
git init --bare --initial-branch=test test.git
#Clone repository
git clone ssh://$user@$computer:/home/$user/test.git test1
cd test1
git remote show origin
#Output [ORIGIN1]
echo abc >> abc.txt && git add * && git commit -a -m test && git push
#Output...
#* [new branch] master -> master
git remote show origin
#Output [ORIGIN2]
What did you expect to happen? (Expected behavior)
Repository cloned using ssh should push to test branch just like
repository cloned using filesystem does below as configured in the bare
repository.
#Clone repository
git clone home/$user/test.git test2
git remote show origin
#Output [ORIGIN3]
echo abc >> abc.txt && git add * && git commit -a -m test && git push
#Output...
#* [new branch] test -> test
git remote show origin
#Output [ORIGIN4]
What happened instead? (Actual behavior)
Repository cloned using ssh pushed to master branch disregarding
configuration in bare repository.
What's different between what you expected and what actually happened?
For ssh cloned repository test branch should been the default branch
however master was the default.
Anything else you want to add:
[ORIGIN1]
* remote origin
HEAD branch: (unknown)
Local branch configured for 'git pull':
master merges with remote master
[ORIGIN2]
* remote origin
HEAD branch: (unknown)
Remote branch:
master tracked
Local branch configured for 'git pull':
master merges with remote master
Local ref configured for 'git push':
master pushes to master (up to date)
[ORIGIN3]
* remote origin
HEAD branch: (unknown)
Local branch configured for 'git pull':
test merges with remote test
[ORIGIN4]
* remote origin
HEAD branch: test
Remote branches:
master new (next fetch will store in remotes/origin)
test tracked
Local branch configured for 'git pull':
test merges with remote test
Local ref configured for 'git push':
test pushes to test (up to date)
Please review the rest of the bug report below.
You can delete any lines you don't wish to share.
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1
(2023-10-07) x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
^ permalink raw reply
* git diagnose in non git directory results in core dump
From: Sheik @ 2023-10-25 20:41 UTC (permalink / raw)
To: git
Hi Maintainers,
Running git diagnose in a non git directory results in a core dump.
Expected behaviour would be that it reports an error.
mkdir test
cd test
git diagnose
#Output...
Segmentation fault (core dumped)
Thanks
Sheik
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1
(2023-10-07) x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
^ permalink raw reply
* git diagnose with invalid CLI argument does not report error
From: Sheik @ 2023-10-25 20:49 UTC (permalink / raw)
To: git
Hi Maintainers,
Running git diagnose with an invalid CLI argument in a valid Git
directory does not report error. Expected behaviour would be that it
reports an error.
#Example shell commands which should have reported an error but
continues to succeed
cd $ToAGitDirectory
git diagnose mod
git diagnose mode
git diagnose mode=all
Thanks
Sheik
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1
(2023-10-07) x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
^ permalink raw reply
* git bugreport with invalid CLI argument does not report error
From: Sheik @ 2023-10-25 20:59 UTC (permalink / raw)
To: git
Hi Maintainers,
Running git bugreport with an invalid CLI argument in a valid Git
directory does not report error. Expected behaviour would be that it
reports an error.
#Example git commands which should have reported an error but continues
to succeed
cd $ToAnyDirectory
git bugreport diagnose
Thanks
Sheik
[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.5.0-2-amd64 #1 SMP PREEMPT_DYNAMIC Debian 6.5.6-1
(2023-10-07) x86_64
compiler info: gnuc: 13.2
libc info: glibc: 2.37
$SHELL (typically, interactive shell): /bin/bash
[Enabled Hooks]
^ permalink raw reply
* Re: git diagnose in non git directory results in core dump
From: Kristoffer Haugsbakk @ 2023-10-25 21:01 UTC (permalink / raw)
To: Sheik; +Cc: git
In-Reply-To: <3e409165-2468-47be-895e-f3ec86b94730@gmail.com>
Being worked on: https://lore.kernel.org/git/CAKFQ_Q9WjF9i-Rx2jdCw-adPVQrWNfNKrDY-em8Rpa5RNLXz4A@mail.gmail.com/
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: Cloning new bare repository using ssh does not respect bare repository --initial-branch
From: Sheik @ 2023-10-25 22:24 UTC (permalink / raw)
To: sahibzone; +Cc: git
In-Reply-To: <39e013af-69c7-4185-99ae-1e8fe5edef5c@gmail.com>
Hi Maintainers,
Sorry thought this wouldn't make it through due to html, please
disregard this and see latest posted here
https://lore.kernel.org/git/63eb269e-72b9-4830-98fc-aeef8b8180d7@gmail.com/
Thanks
Sheik
^ permalink raw reply
* Re: git bugreport with invalid CLI argument does not report error
From: Emily Shaffer @ 2023-10-25 22:53 UTC (permalink / raw)
To: Sheik; +Cc: git
In-Reply-To: <7a92b537-ba88-4667-bb18-2e8c74aa9915@gmail.com>
On Thu, Oct 26, 2023 at 07:59:16AM +1100, Sheik wrote:
>
> Hi Maintainers,
>
>
> Running git bugreport with an invalid CLI argument in a valid Git directory
> does not report error. Expected behaviour would be that it reports an error.
>
>
> #Example git commands which should have reported an error but continues to
> succeed
>
> cd $ToAnyDirectory
>
> git bugreport diagnose
It looks like parse-options.[ch] helps us here for misspelled dashed
options, like `--diaggnose`. But it doesn't complain when there are
unexpected positional arguments. I think we can just notice if there are
any argc left over, complain, and print usage.
I put together a quick patch; could be that we don't need to leave this
error about "positional arguments" and can leave it as an exercise to
the reader to compare their previous command to the usage text. I guess
we could also unroll remaining argv but it was just a hair more time
than I wanted to spend ;)
- Emily
--- 8< ---
From 2031c7f55652559b8b4ec3c67ce4c4f94a355762 Mon Sep 17 00:00:00 2001
From: Emily Shaffer <nasamuffin@google.com>
Date: Wed, 25 Oct 2023 15:45:25 -0700
Subject: [PATCH] bugreport: reject positional arguments
git-bugreport already rejected unrecognized flag arguments, like
`--diaggnose`, but this doesn't help if the user's mistake was to forget
the `--` in front of the argument. This can result in a user's intended
argument not being parsed with no indication to the user that something
went wrong. Since git-bugreport presently doesn't take any positionals
at all, let's reject all positionals and give the user a usage hint.
Signed-off-by: Emily Shaffer <nasamuffin@google.com>
---
builtin/bugreport.c | 5 +++++
t/t0091-bugreport.sh | 6 ++++++
2 files changed, 11 insertions(+)
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index d2ae5c305d..eb6234a50d 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -126,6 +126,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, bugreport_options,
bugreport_usage, 0);
+ if (argc) {
+ error(_("git bugreport does not take positional arguments"));
+ usage(bugreport_usage[0]);
+ }
+
/* Prepare the path to put the result */
prefixed_filename = prefix_filename(prefix,
option_output ? option_output : "");
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index f6998269be..2061d6f386 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -69,6 +69,12 @@ test_expect_success 'incorrect arguments abort with usage' '
test_path_is_missing git-bugreport-*
'
+test_expect_success 'incorrect positional arguments abort with usage' '
+ test_must_fail git bugreport false 2>output &&
+ test_i18ngrep usage output &&
+ test_path_is_missing git-bugreport-*
+'
+
test_expect_success 'runs outside of a git dir' '
test_when_finished rm non-repo/git-bugreport-* &&
nongit git bugreport
--
2.42.0.758.gaed0368e0e-goog
^ permalink raw reply related
* Re: git bugreport with invalid CLI argument does not report error
From: Eric Sunshine @ 2023-10-25 23:11 UTC (permalink / raw)
To: Emily Shaffer; +Cc: Sheik, git
In-Reply-To: <ZTmcVJaY2TjFCVyw@google.com>
On Wed, Oct 25, 2023 at 6:53 PM Emily Shaffer <nasamuffin@google.com> wrote:
> On Thu, Oct 26, 2023 at 07:59:16AM +1100, Sheik wrote:
> > Running git bugreport with an invalid CLI argument in a valid Git directory
> > does not report error. Expected behaviour would be that it reports an error.
>
> It looks like parse-options.[ch] helps us here for misspelled dashed
> options, like `--diaggnose`. But it doesn't complain when there are
> unexpected positional arguments. I think we can just notice if there are
> any argc left over, complain, and print usage.
>
> I put together a quick patch; could be that we don't need to leave this
> error about "positional arguments" and can leave it as an exercise to
> the reader to compare their previous command to the usage text. I guess
> we could also unroll remaining argv but it was just a hair more time
> than I wanted to spend ;)
>
> --- 8< ---
> Subject: [PATCH] bugreport: reject positional arguments
>
> git-bugreport already rejected unrecognized flag arguments, like
> `--diaggnose`, but this doesn't help if the user's mistake was to forget
> the `--` in front of the argument. This can result in a user's intended
> argument not being parsed with no indication to the user that something
> went wrong. Since git-bugreport presently doesn't take any positionals
> at all, let's reject all positionals and give the user a usage hint.
>
> Signed-off-by: Emily Shaffer <nasamuffin@google.com>
> ---
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> @@ -126,6 +126,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
> + if (argc) {
> + error(_("git bugreport does not take positional arguments"));
> + usage(bugreport_usage[0]);
> + }
This error message may be somewhat difficult to interpret. Providing
additional context could help the user understand the actual problem.
For instance:
% git bugreport diagnose
error: unrecognized argument "diagnose"
^ permalink raw reply
* [PATCH v2] bugreport: reject positional arguments
From: emilyshaffer @ 2023-10-26 0:55 UTC (permalink / raw)
To: git; +Cc: Emily Shaffer, Eric Sunshine, Sheik
In-Reply-To: <CAPig+cQtoEpTDK1U5R+wUD4qovFtpcx6+zyOQE-SAYp5SE2q2Q@mail.gmail.com>
From: Emily Shaffer <nasamuffin@google.com>
git-bugreport already rejected unrecognized flag arguments, like
`--diaggnose`, but this doesn't help if the user's mistake was to forget
the `--` in front of the argument. This can result in a user's intended
argument not being parsed with no indication to the user that something
went wrong. Since git-bugreport presently doesn't take any positionals
at all, let's reject all positionals and give the user a usage hint.
Signed-off-by: Emily Shaffer <nasamuffin@google.com>
---
Per Eric's suggestion, added a citation of the first positional arg
found. I don't think it's necessary to unroll the entire argv array
here, though.
- Emily
builtin/bugreport.c | 6 ++++++
t/t0091-bugreport.sh | 7 +++++++
2 files changed, 13 insertions(+)
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index d2ae5c305d..8a69a23397 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -126,6 +126,12 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, bugreport_options,
bugreport_usage, 0);
+ if (argc) {
+ if (argv[0])
+ error(_("unknown argument `%s'"), argv[0]);
+ usage(bugreport_usage[0]);
+ }
+
/* Prepare the path to put the result */
prefixed_filename = prefix_filename(prefix,
option_output ? option_output : "");
diff --git a/t/t0091-bugreport.sh b/t/t0091-bugreport.sh
index f6998269be..5b1b3e8d07 100755
--- a/t/t0091-bugreport.sh
+++ b/t/t0091-bugreport.sh
@@ -69,6 +69,13 @@ test_expect_success 'incorrect arguments abort with usage' '
test_path_is_missing git-bugreport-*
'
+test_expect_success 'incorrect positional arguments abort with usage and hint' '
+ test_must_fail git bugreport false 2>output &&
+ test_i18ngrep usage output &&
+ test_i18ngrep false output &&
+ test_path_is_missing git-bugreport-*
+'
+
test_expect_success 'runs outside of a git dir' '
test_when_finished rm non-repo/git-bugreport-* &&
nongit git bugreport
--
2.42.0.758.gaed0368e0e-goog
^ permalink raw reply related
* Re: Why sometimes branch merging is associated with this commit: Merge remote-tracking branch 'remotes/p4/HEAD'
From: Thomas Guyot @ 2023-10-26 3:23 UTC (permalink / raw)
To: Yuri, Git Mailing List
In-Reply-To: <dd4befe1-dea7-b27d-720d-9f1616b129ee@tsoft.com>
Hi Yuri,
On 2023-10-20 15:46, Yuri wrote:
> I use git-p4 to use the perforce repository through git.
>
>
> There are 3 branches:
>
> * master
> remotes/p4/HEAD -> p4/master
> remotes/p4/master
>
> git-p4 syncs remotes/p4/HEAD with the perforce repository.
>
> Then the user (me) needs to merge remotes/p4/HEAD into master.
>
> Initially such merge doesn't cause "Merge remote-tracking branch
> 'remotes/p4/HEAD'" commits.
>
> But then, after several cycles of submit/sync something happens, and git
> forces me to commit with the "Merge remote-tracking branch
> 'remotes/p4/HEAD'" comment.
>
>
> What makes the merge from remotes/p4/HEAD into master to require "Merge
> remote-tracking branch 'remotes/p4/HEAD'"?
I believe what is happening is once other developers merge code in the
same branch your master branch ends up like a topic branch that never
merge back to the remote tracking branch.
I've worked a lot more with git-svn but both behaves somewhat similarly
AFAIK - when you "send" your commit upstream (I'm using "send" as I
don't recall the exact terminology for git-p4) what really happen is
like if your commit was cherry-picked on top of the remote branch - P4
tracks commits linearly and has an incompatible concept of merges.
When you sync later, it will get both your commits and other developer's
commit, but because they don't have the same history anymore git has no
choice but to merge the branches (this doesn't happen initially if
you're the only committer as your branch remain sync with the remote).
> What does this mean?
>
> What is changed in remotes/p4/HEAD or master that later requires this?
>
> How to eliminate the need for "Merge remote-tracking branch
> 'remotes/p4/HEAD'"?
>
You probably don't "need" to eliminate this, you can just keep creating
new commits on top and pushing them. The end result will be the same for
other p4 users.
If you still want to re-sync with upstream all you have to do is to
rebase on top of the remote tracking branch. You'll end up with the same
work tree but your history will get "linearized" based on the commits in p4.
Let's take for example this branch:
A---B---C [master, remotes/p4/HEAD]
Now imagine you and another developer both work on the repo separately
(let's assume different files, no possible conflicts) and the other
developer commits before you in p4, you end up with:
A---B---C---E [remotes/p4/HEAD]
\
`-D [master]
When you send *your* change, D, on the p4 repo, it cannot merge like git
does. It will instead apply your commit on top of E - we'll call your
commit D' as it's now a different commit hash, although it's the same
change:
A---B---C---E---D' [remotes/p4/HEAD]
\
`-D [master]
Now if you sync with p4, git can't fast-forward anymore because D and D'
are different commits (and don't even have the same parent!) therefore
it will merge (F) with the remote tracking branch:
A---B---C---E---D' [remotes/p4/HEAD]
\ \
`-D-----+-F [master]
F and D' are identical, they have the same work tree contents, but have
a different commit hash (implied by the difference in parents). If you
rebase master to remotes/p4/HEAD git will discard the duplicate D commit
from your branch and restore it at the tip of remotes/p4/HEAD. If you
has any new commit after F, those would be applied after D', for ex:
A---B---C---E---D' [remotes/p4/HEAD]
\ \
`-D-----+-F---G [master]
Rebases to:
A---B---C---E---D' [remotes/p4/HEAD]
\
`-G' [master]
Personally I would always rebase with the remote repo, and with git-svn
I was actually doing that systematically before pushing anything (I
think I had no choice anyway, or maybe I just didn't want any local
merge commits...) In any case you should be able to rebase with the p4
remote tracking branch before or after committing into p4 and that will
keep your history in line with p4's history.
Regards,
-
Thomas
^ permalink raw reply
* Re: [PATCH v2] bugreport: reject positional arguments
From: Eric Sunshine @ 2023-10-26 3:43 UTC (permalink / raw)
To: emilyshaffer; +Cc: git, Emily Shaffer, Sheik
In-Reply-To: <20231026005542.872301-1-nasamuffin@google.com>
On Wed, Oct 25, 2023 at 8:55 PM <emilyshaffer@google.com> wrote:
> git-bugreport already rejected unrecognized flag arguments, like
> `--diaggnose`, but this doesn't help if the user's mistake was to forget
> the `--` in front of the argument. This can result in a user's intended
> argument not being parsed with no indication to the user that something
> went wrong. Since git-bugreport presently doesn't take any positionals
> at all, let's reject all positionals and give the user a usage hint.
>
> Signed-off-by: Emily Shaffer <nasamuffin@google.com>
> ---
> Per Eric's suggestion, added a citation of the first positional arg
> found. I don't think it's necessary to unroll the entire argv array
> here, though.
Thanks. I had the same thought about the first positional argument
being sufficient since it should provide enough context on its own.
> 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.
^ 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
* 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 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
* [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
* [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 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 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 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 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
* 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
* 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 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
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