* [PATCH v3 2/2] merge-file: add an option to process object IDs
From: brian m. carlson @ 2023-11-01 19:24 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren, Phillip Wood, Eric Sunshine,
Taylor Blau, Martin Ågren
In-Reply-To: <20231101192419.794162-1-sandals@crustytoothpaste.net>
From: "brian m. carlson" <bk2204@github.com>
git merge-file knows how to merge files on the file system already. It
would be helpful, however, to allow it to also merge single blobs.
Teach it an `--object-id` option which means that its arguments are
object IDs and not files to allow it to do so.
We handle the empty blob specially since read_mmblob doesn't read it
directly and otherwise users cannot specify an empty ancestor.
Signed-off-by: brian m. carlson <bk2204@github.com>
---
Documentation/git-merge-file.txt | 19 +++++++++-
builtin/merge-file.c | 62 +++++++++++++++++++++++---------
t/t6403-merge-file.sh | 58 ++++++++++++++++++++++++++++++
3 files changed, 122 insertions(+), 17 deletions(-)
diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
index bf0a18cf02..6a081eacb7 100644
--- a/Documentation/git-merge-file.txt
+++ b/Documentation/git-merge-file.txt
@@ -11,7 +11,7 @@ SYNOPSIS
[verse]
'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]]
[--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
- [--[no-]diff3] <current> <base> <other>
+ [--[no-]diff3] [--object-id] <current> <base> <other>
DESCRIPTION
@@ -41,6 +41,10 @@ however, these conflicts are resolved favouring lines from `<current>`,
lines from `<other>`, or lines from both respectively. The length of the
conflict markers can be given with the `--marker-size` option.
+If `--object-id` is specified, exactly the same behavior occurs, except that
+instead of specifying what to merge as files, it is specified as a list of
+object IDs referring to blobs.
+
The exit value of this program is negative on error, and the number of
conflicts otherwise (truncated to 127 if there are more than that many
conflicts). If the merge was clean, the exit value is 0.
@@ -53,6 +57,14 @@ linkgit:git[1].
OPTIONS
-------
+--object-id::
+ Specify the contents to merge as blobs in the current repository instead of
+ files. In this case, the operation must take place within a valid repository.
++
+If the `-p` option is specified, the merged file (including conflicts, if any)
+goes to standard output as normal; otherwise, the merged file is written to the
+object store and the object ID of its blob is written to standard output.
+
-L <label>::
This option may be given up to three times, and
specifies labels to be used in place of the
@@ -94,6 +106,11 @@ EXAMPLES
merges tmp/a123 and tmp/c345 with the base tmp/b234, but uses labels
`a` and `c` instead of `tmp/a123` and `tmp/c345`.
+`git merge-file -p --object-id abc1234 def567 890abcd`::
+
+ combines the changes of the blob abc1234 and 890abcd since def567,
+ tries to merge them and writes the result to standard output
+
GIT
---
Part of the linkgit:git[1] suite
diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index d7eb4c6540..832c93d8d5 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -1,5 +1,8 @@
#include "builtin.h"
#include "abspath.h"
+#include "hex.h"
+#include "object-name.h"
+#include "object-store.h"
#include "config.h"
#include "gettext.h"
#include "setup.h"
@@ -31,10 +34,11 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
mmfile_t mmfs[3] = { 0 };
mmbuffer_t result = { 0 };
xmparam_t xmp = { 0 };
- int ret = 0, i = 0, to_stdout = 0;
+ int ret = 0, i = 0, to_stdout = 0, object_id = 0;
int quiet = 0;
struct option options[] = {
OPT_BOOL('p', "stdout", &to_stdout, N_("send results to standard output")),
+ OPT_BOOL(0, "object-id", &object_id, N_("use object IDs instead of filenames")),
OPT_SET_INT(0, "diff3", &xmp.style, N_("use a diff3 based merge"), XDL_MERGE_DIFF3),
OPT_SET_INT(0, "zdiff3", &xmp.style, N_("use a zealous diff3 based merge"),
XDL_MERGE_ZEALOUS_DIFF3),
@@ -71,8 +75,12 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
return error_errno("failed to redirect stderr to /dev/null");
}
+ if (object_id)
+ setup_git_directory();
+
for (i = 0; i < 3; i++) {
char *fname;
+ struct object_id oid;
mmfile_t *mmf = mmfs + i;
if (!names[i])
@@ -80,12 +88,22 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
fname = prefix_filename(prefix, argv[i]);
- if (read_mmfile(mmf, fname))
+ if (object_id) {
+ if (repo_get_oid(the_repository, argv[i], &oid))
+ ret = error(_("object '%s' does not exist"),
+ argv[i]);
+ else if (!oideq(&oid, the_hash_algo->empty_blob))
+ read_mmblob(mmf, &oid);
+ else
+ read_mmfile(mmf, "/dev/null");
+ } else if (read_mmfile(mmf, fname)) {
ret = -1;
- else if (mmf->size > MAX_XDIFF_SIZE ||
- buffer_is_binary(mmf->ptr, mmf->size))
+ }
+ if (ret != -1 && (mmf->size > MAX_XDIFF_SIZE ||
+ buffer_is_binary(mmf->ptr, mmf->size))) {
ret = error("Cannot merge binary files: %s",
argv[i]);
+ }
free(fname);
if (ret)
@@ -99,20 +117,32 @@ int cmd_merge_file(int argc, const char **argv, const char *prefix)
ret = xdl_merge(mmfs + 1, mmfs + 0, mmfs + 2, &xmp, &result);
if (ret >= 0) {
- const char *filename = argv[0];
- char *fpath = prefix_filename(prefix, argv[0]);
- FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
+ if (object_id && !to_stdout) {
+ struct object_id oid;
+ if (result.size) {
+ if (write_object_file(result.ptr, result.size, OBJ_BLOB, &oid) < 0)
+ ret = error(_("Could not write object file"));
+ } else {
+ oidcpy(&oid, the_hash_algo->empty_blob);
+ }
+ if (ret >= 0)
+ printf("%s\n", oid_to_hex(&oid));
+ } else {
+ const char *filename = argv[0];
+ char *fpath = prefix_filename(prefix, argv[0]);
+ FILE *f = to_stdout ? stdout : fopen(fpath, "wb");
- if (!f)
- ret = error_errno("Could not open %s for writing",
- filename);
- else if (result.size &&
- fwrite(result.ptr, result.size, 1, f) != 1)
- ret = error_errno("Could not write to %s", filename);
- else if (fclose(f))
- ret = error_errno("Could not close %s", filename);
+ if (!f)
+ ret = error_errno("Could not open %s for writing",
+ filename);
+ else if (result.size &&
+ fwrite(result.ptr, result.size, 1, f) != 1)
+ ret = error_errno("Could not write to %s", filename);
+ else if (fclose(f))
+ ret = error_errno("Could not close %s", filename);
+ free(fpath);
+ }
free(result.ptr);
- free(fpath);
}
if (ret > 127)
diff --git a/t/t6403-merge-file.sh b/t/t6403-merge-file.sh
index 1a7082323d..2c92209eca 100755
--- a/t/t6403-merge-file.sh
+++ b/t/t6403-merge-file.sh
@@ -65,11 +65,30 @@ test_expect_success 'merge with no changes' '
test_cmp test.txt orig.txt
'
+test_expect_success 'merge with no changes with --object-id' '
+ git add orig.txt &&
+ git merge-file -p --object-id :orig.txt :orig.txt :orig.txt >actual &&
+ test_cmp actual orig.txt
+'
+
test_expect_success "merge without conflict" '
cp new1.txt test.txt &&
git merge-file test.txt orig.txt new2.txt
'
+test_expect_success 'merge without conflict with --object-id' '
+ git add orig.txt new2.txt &&
+ git merge-file --object-id :orig.txt :orig.txt :new2.txt >actual &&
+ git rev-parse :new2.txt >expected &&
+ test_cmp actual expected
+'
+
+test_expect_success 'can accept object ID with --object-id' '
+ git merge-file --object-id $(test_oid empty_blob) $(test_oid empty_blob) :new2.txt >actual &&
+ git rev-parse :new2.txt >expected &&
+ test_cmp actual expected
+'
+
test_expect_success 'works in subdirectory' '
mkdir dir &&
cp new1.txt dir/a.txt &&
@@ -138,6 +157,31 @@ test_expect_success "expected conflict markers" '
test_cmp expect.txt test.txt
'
+test_expect_success "merge with conflicts with --object-id" '
+ git add backup.txt orig.txt new3.txt &&
+ test_must_fail git merge-file -p --object-id :backup.txt :orig.txt :new3.txt >actual &&
+ sed -e "s/<< test.txt/<< :backup.txt/" \
+ -e "s/>> new3.txt/>> :new3.txt/" \
+ expect.txt >expect &&
+ test_cmp expect actual &&
+ test_must_fail git merge-file --object-id :backup.txt :orig.txt :new3.txt >oid &&
+ git cat-file blob "$(cat oid)" >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success "merge with conflicts with --object-id with labels" '
+ git add backup.txt orig.txt new3.txt &&
+ test_must_fail git merge-file -p --object-id \
+ -L test.txt -L orig.txt -L new3.txt \
+ :backup.txt :orig.txt :new3.txt >actual &&
+ test_cmp expect.txt actual &&
+ test_must_fail git merge-file --object-id \
+ -L test.txt -L orig.txt -L new3.txt \
+ :backup.txt :orig.txt :new3.txt >oid &&
+ git cat-file blob "$(cat oid)" >actual &&
+ test_cmp expect.txt actual
+'
+
test_expect_success "merge conflicting with --ours" '
cp backup.txt test.txt &&
@@ -256,6 +300,14 @@ test_expect_success 'binary files cannot be merged' '
grep "Cannot merge binary files" merge.err
'
+test_expect_success 'binary files cannot be merged with --object-id' '
+ cp "$TEST_DIRECTORY"/test-binary-1.png . &&
+ git add orig.txt new1.txt test-binary-1.png &&
+ test_must_fail git merge-file --object-id \
+ :orig.txt :test-binary-1.png :new1.txt 2> merge.err &&
+ grep "Cannot merge binary files" merge.err
+'
+
test_expect_success 'MERGE_ZEALOUS simplifies non-conflicts' '
sed -e "s/deerit.\$/deerit;/" -e "s/me;\$/me./" <new5.txt >new6.txt &&
sed -e "s/deerit.\$/deerit,/" -e "s/me;\$/me,/" <new5.txt >new7.txt &&
@@ -389,4 +441,10 @@ test_expect_success 'conflict sections match existing line endings' '
test $(tr "\015" Q <nolf.txt | grep "^[<=>].*Q$" | wc -l) = 0
'
+test_expect_success '--object-id fails without repository' '
+ empty="$(test_oid empty_blob)" &&
+ nongit test_must_fail git merge-file --object-id $empty $empty $empty 2>err &&
+ grep "not a git repository" err
+'
+
test_done
^ permalink raw reply related
* [PATCH v3 1/2] git-merge-file doc: drop "-file" from argument placeholders
From: brian m. carlson @ 2023-11-01 19:24 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Elijah Newren, Phillip Wood, Eric Sunshine,
Taylor Blau, Martin Ågren
In-Reply-To: <20231101192419.794162-1-sandals@crustytoothpaste.net>
From: Martin Ågren <martin.agren@gmail.com>
`git merge-file` takes three positional arguments. Each of them is
documented as `<foo-file>`. In preparation for teaching this command to
alternatively take three object IDs, make these placeholders a bit more
generic by dropping the "-file" parts. Instead, clarify early that the
three arguments are filenames. Even after the next commit, we can afford
to present this file-centric view up front and in the general
discussion, since it will remain the default one.
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: brian m. carlson <bk2204@github.com>
---
Documentation/git-merge-file.txt | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
index 7e9093fab6..bf0a18cf02 100644
--- a/Documentation/git-merge-file.txt
+++ b/Documentation/git-merge-file.txt
@@ -11,19 +11,20 @@ SYNOPSIS
[verse]
'git merge-file' [-L <current-name> [-L <base-name> [-L <other-name>]]]
[--ours|--theirs|--union] [-p|--stdout] [-q|--quiet] [--marker-size=<n>]
- [--[no-]diff3] <current-file> <base-file> <other-file>
+ [--[no-]diff3] <current> <base> <other>
DESCRIPTION
-----------
-'git merge-file' incorporates all changes that lead from the `<base-file>`
-to `<other-file>` into `<current-file>`. The result ordinarily goes into
-`<current-file>`. 'git merge-file' is useful for combining separate changes
-to an original. Suppose `<base-file>` is the original, and both
-`<current-file>` and `<other-file>` are modifications of `<base-file>`,
+Given three files `<current>`, `<base>` and `<other>`,
+'git merge-file' incorporates all changes that lead from `<base>`
+to `<other>` into `<current>`. The result ordinarily goes into
+`<current>`. 'git merge-file' is useful for combining separate changes
+to an original. Suppose `<base>` is the original, and both
+`<current>` and `<other>` are modifications of `<base>`,
then 'git merge-file' combines both changes.
-A conflict occurs if both `<current-file>` and `<other-file>` have changes
+A conflict occurs if both `<current>` and `<other>` have changes
in a common segment of lines. If a conflict is found, 'git merge-file'
normally outputs a warning and brackets the conflict with lines containing
<<<<<<< and >>>>>>> markers. A typical conflict will look like this:
@@ -36,8 +37,8 @@ normally outputs a warning and brackets the conflict with lines containing
If there are conflicts, the user should edit the result and delete one of
the alternatives. When `--ours`, `--theirs`, or `--union` option is in effect,
-however, these conflicts are resolved favouring lines from `<current-file>`,
-lines from `<other-file>`, or lines from both respectively. The length of the
+however, these conflicts are resolved favouring lines from `<current>`,
+lines from `<other>`, or lines from both respectively. The length of the
conflict markers can be given with the `--marker-size` option.
The exit value of this program is negative on error, and the number of
@@ -62,7 +63,7 @@ OPTIONS
-p::
Send results to standard output instead of overwriting
- `<current-file>`.
+ `<current>`.
-q::
Quiet; do not warn about conflicts.
^ permalink raw reply related
* Re: [PATCH v2 1/1] merge-file: add an option to process object IDs
From: brian m. carlson @ 2023-11-01 19:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Martin Ågren, git, Elijah Newren, Phillip Wood,
Eric Sunshine, Taylor Blau
In-Reply-To: <xmqqedhaw31d.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 514 bytes --]
On 2023-11-01 at 03:44:30, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > This seems reasonable. Junio, do you want to sneak this in and fix the
> > commit message above, or do you want me to do a v3?
>
> As it hasn't hit 'next' on my end, I'd prefer to see a version I can
> blindly apply without having to care all the details of what was
> discussed. Thanks.
I'll send out a v3 shortly.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply
* Re: [PATCH 2/2] pretty: add '%aA' to show domain-part of email addresses
From: Liam Beguin @ 2023-11-01 19:06 UTC (permalink / raw)
To: Jeff King; +Cc: Andy Koppe, Junio C Hamano, Kousik Sanagavarapu, git
In-Reply-To: <20231030091011.GB84866@coredump.intra.peff.net>
On Mon, Oct 30, 2023 at 05:10:11AM -0400, Jeff King wrote:
> On Sat, Oct 28, 2023 at 07:58:31AM +0100, Andy Koppe wrote:
>
> > > I chose the "a" for "address", but I'm not sold on %aa either.
> > > I just couldn't find anything better that wasn't already taken.
> > >
> > > What about "a@"?
> >
> > Makes sense, and I suppose there's "%G?" as precedent for using a symbol
> > rather than letter in these.
>
> This is pretty subjective, but I somehow find "%a@" hard to parse
> visually (despite the fact that yes, "%G?" already crossed that bridge).
> But I think the real nail in the coffin is your later comment that we
> cannot use capitalization to make the raw/mailmap distinction.
>
> > If that's not suitable though, how about "m" for "mail domain"? It also
> > immediately follows "l" for "local-part" in the alphabet.
>
> FWIW, that makes sense to me over "a" (though admittedly it is not
> really any less vague than "a", so it really might vary from person to
> person).
Okay, I like 'm' better as well. And '@' is a no go because the
mailmaped version. I'll resend.
Cheers,
Liam
^ permalink raw reply
* 'git stash' in one subfolder fails because some other subfolder has unmerged files
From: Yuri @ 2023-11-01 18:44 UTC (permalink / raw)
To: Git Mailing List
Here is the log:
[yuri@yv /usr/ports/audio/giada]$ git stash push -m "audio/giada: Update
to 0.26.0 has run-time issues" -- /usr/ports/audio/gia
da
audio/triceratops-lv2/Makefile: needs merge
audio/triceratops-lv2/pkg-descr: needs merge
audio/triceratops-lv2/pkg-plist: needs merge
science/py-dftbplus/Makefile: needs merge
science/py-dftbplus/distinfo: needs merge
science/py-dftbplus/pkg-descr: needs merge
I asked git to stash only the audio/giada subfolder. Unmerged files in
science/py-dftbplus and audio/triceratops-lv2 shouldn't prevent stashing
files in audio/giada.
I believe that this is a bug.
Thanks,
Yuri
^ permalink raw reply
* Re: [PATCH v8 2.5/3] fixup! unit tests: add TAP unit test framework
From: Josh Steadmon @ 2023-11-01 17:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, calvinwan, git, linusa, rsbecker
In-Reply-To: <xmqq1qduo6yr.fsf@gitster.g>
On 2023.10.16 09:41, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > Here are a couple of cleanups for the unit test framework that I
> > noticed.
>
> Thanks. I trust that this will be squashed into the next update,
> but in the meantime, I'll include it in the copy of the series I
> have (without squashing). Here is another one I noticed.
>
> ----- >8 --------- >8 --------- >8 -----
> Subject: [PATCH] fixup! ci: run unit tests in CI
>
> A CI job failed due to contrib/coccinelle/equals-null.cocci
> and suggested this change, which seems sensible.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> t/unit-tests/t-strbuf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied in v9, thanks!
^ permalink raw reply
* Re: [PATCH v8 2.5/3] fixup! unit tests: add TAP unit test framework
From: Josh Steadmon @ 2023-11-01 17:54 UTC (permalink / raw)
To: Phillip Wood; +Cc: calvinwan, git, gitster, linusa, phillip.wood123, rsbecker
In-Reply-To: <20231016134421.21659-1-phillip.wood123@gmail.com>
On 2023.10.16 14:43, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Here are a couple of cleanups for the unit test framework that I
> noticed.
>
> Update the documentation of the example custom check to reflect the
> change in return value of test_assert() and mention that
> checks should be careful when dereferencing pointer arguments.
>
> Also avoid evaluating macro augments twice in check_int() and
> friends. The global variable test__tmp was introduced to avoid
> evaluating the arguments to these macros more than once but the macros
> failed to use it when passing the values being compared to
> check_int_loc().
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
> t/unit-tests/test-lib.h | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
Applied in v9, thanks!
^ permalink raw reply
* Re: [PATCH v8 1/3] unit tests: Add a project plan document
From: Josh Steadmon @ 2023-11-01 17:47 UTC (permalink / raw)
To: Christian Couder
Cc: git, phillip.wood123, linusa, calvinwan, gitster, rsbecker
In-Reply-To: <CAP8UFD26X4MPbJs4KfNOgicLMb-wiuFZj3Hw17acMmmc_=vcqQ@mail.gmail.com>
On 2023.10.27 22:12, Christian Couder wrote:
> On Tue, Oct 10, 2023 at 12:22 AM Josh Steadmon <steadmon@google.com> wrote:
> >
> > In our current testing environment, we spend a significant amount of
> > effort crafting end-to-end tests for error conditions that could easily
> > be captured by unit tests (or we simply forgo some hard-to-setup and
> > rare error conditions). Describe what we hope to accomplish by
> > implementing unit tests, and explain some open questions and milestones.
> > Discuss desired features for test frameworks/harnesses, and provide a
> > preliminary comparison of several different frameworks.
>
> Nit: Not sure why the test framework comparison is "preliminary" as we
> have actually selected a unit test framework and are adding it in the
> next patch of the series. I understand that this was perhaps written
> before the choice was made, but maybe we might want to update that
> now.
Fixed in v9, thanks.
> > diff --git a/Documentation/technical/unit-tests.txt b/Documentation/technical/unit-tests.txt
> > new file mode 100644
> > index 0000000000..b7a89cc838
> > --- /dev/null
> > +++ b/Documentation/technical/unit-tests.txt
> > @@ -0,0 +1,220 @@
> > += Unit Testing
> > +
> > +In our current testing environment, we spend a significant amount of effort
> > +crafting end-to-end tests for error conditions that could easily be captured by
> > +unit tests (or we simply forgo some hard-to-setup and rare error conditions).
> > +Unit tests additionally provide stability to the codebase and can simplify
> > +debugging through isolation. Writing unit tests in pure C, rather than with our
> > +current shell/test-tool helper setup, simplifies test setup, simplifies passing
> > +data around (no shell-isms required), and reduces testing runtime by not
> > +spawning a separate process for every test invocation.
> > +
> > +We believe that a large body of unit tests, living alongside the existing test
> > +suite, will improve code quality for the Git project.
>
> I agree with that.
>
> > +== Choosing a framework
> > +
> > +We believe the best option is to implement a custom TAP framework for the Git
> > +project. We use a version of the framework originally proposed in
> > +https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[1].
>
> Nit: Logically I would think that our opinion should come after the
> comparison and be backed by it.
I intended this to be a quick summary for those who don't want to read
the whole doc. I clarified that and added a link to the selection
rationale.
> > +== Choosing a test harness
> > +
> > +During upstream discussion, it was occasionally noted that `prove` provides many
> > +convenient features, such as scheduling slower tests first, or re-running
> > +previously failed tests.
> > +
> > +While we already support the use of `prove` as a test harness for the shell
> > +tests, it is not strictly required. The t/Makefile allows running shell tests
> > +directly (though with interleaved output if parallelism is enabled). Git
> > +developers who wish to use `prove` as a more advanced harness can do so by
> > +setting DEFAULT_TEST_TARGET=prove in their config.mak.
> > +
> > +We will follow a similar approach for unit tests: by default the test
> > +executables will be run directly from the t/Makefile, but `prove` can be
> > +configured with DEFAULT_UNIT_TEST_TARGET=prove.
>
> Nice that it can be used.
>
> The rest of the file looks good.
Thanks for the review!
^ permalink raw reply
* Re: [PATCH v8 1/3] unit tests: Add a project plan document
From: Josh Steadmon @ 2023-11-01 17:31 UTC (permalink / raw)
To: Oswald Buddenhagen
Cc: git, phillip.wood123, linusa, calvinwan, gitster, rsbecker
In-Reply-To: <ZScqLzGiDPZvLh2k@ugly>
On 2023.10.12 01:05, Oswald Buddenhagen wrote:
> On Wed, Oct 11, 2023 at 02:14:03PM -0700, Josh Steadmon wrote:
> > On 2023.10.10 10:57, Oswald Buddenhagen wrote:
> > > On Mon, Oct 09, 2023 at 03:21:20PM -0700, Josh Steadmon wrote:
> > > > +=== Comparison
> > > > +
> > > > +[format="csv",options="header",width="33%"]
> > > > +|=====
> > > > +Framework,"<<license,License>>","<<vendorable-or-ubiquitous,Vendorable or ubiquitous>>","<<maintainable-extensible,Maintainable / extensible>>","<<major-platform-support,Major platform support>>","<<tap-support,TAP support>>","<<diagnostic-output,Diagnostic output>>","<<runtime--skippable-tests,Runtime- skippable tests>>","<<parallel-execution,Parallel execution>>","<<mock-support,Mock support>>","<<signal-error-handling,Signal & error handling>>","<<project-kloc,Project KLOC>>","<<adoption,Adoption>>"
> > > > the redundancy seems unnecessary; asciidoc should automatically
> > > use each
> > > target's section title as the xreflabel.
> >
> > Hmm, this doesn't seem to work for me. It only renders as
> > "[anchor-label]".
> >
> i thought
> https://docs.asciidoctor.org/asciidoc/latest/attributes/id/#customize-automatic-xreftext
> is pretty clear about it, though. maybe the actual tooling uses an older
> version of the spec? or is buggy? or the placement of the titles is
> incorrect? or this applies to different links or targets only? or am i
> misreading something? or ...?
>
> regards
I think the issue may be that asciidoc is the default formatter in
Documentation/Makefile, not asciidoctor.
^ permalink raw reply
* Re: [PATCH 2/2] tests: teach callers of test_i18ngrep to use test_grep
From: Phillip Wood @ 2023-11-01 14:44 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <20231031052330.3762989-3-gitster@pobox.com>
Hi Junio
On 31/10/2023 05:23, Junio C Hamano wrote:
> They are equivalents and the former still exists, so as long as the
> only change this commit makes are to rewrite test_i18ngrep to
> test_grep, there won't be any new bug, even if there still are
> callers of test_i18ngrep remaining in the tree, or when merged to
> other topics that add new uses of test_i18ngrep.
>
> This patch was produced more or less with
>
> git grep -l -e 'test_i18ngrep ' 't/t[0-9][0-9][0-9][0-9]-*.sh' |
> xargs perl -p -i -e 's/test_i18ngrep /test_grep /'
>
> and a good way to sanity check the result yourself is to run the
> above in a checkout of c4603c1c (test framework: further deprecate
> test_i18ngrep, 2023-10-31) and compare the resulting working tree
> contents with the result of applying this patch to the same commit.
> You'll see that test_i18ngrep in a few t/lib-*.sh files corrected,
> in addition to the manual reproduction.
Thanks for working on this. I have checked what you have in seen by
checking out the first parent of ce56983dd3 (Merge branch
'jc/test-i18ngrep' into seen, 2023-11-01) and then converting
"test_i18ngrep" to "test_grep" and diffing the result as you suggest
above. The diff looks good and shows that you've corrected the
additional t/lib-*.sh files. Grepping ce56983dd3 for "test_i18ngrep"
shows there is one instance of test_i18ngrep that has not been coverted
in contrib/mw-to-git/t/t9363-mw-to-git-export-import.sh.
Best Wishes
Phillip
^ permalink raw reply
* [PATCH v2] max_tree_depth: lower it for MSVC to avoid stack overflows
From: Johannes Schindelin via GitGitGadget @ 2023-11-01 13:03 UTC (permalink / raw)
To: git; +Cc: Jeff King, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1604.git.1698680732691.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
There seems to be some internal stack overflow detection in MSVC's
`malloc()` machinery that seems to be independent of the `stack reserve`
and `heap reserve` sizes specified in the executable (editable via
`EDITBIN /STACK:<n> <exe>` and `EDITBIN /HEAP:<n> <exe>`).
In the newly test cases added by `jk/tree-name-and-depth-limit`, this
stack overflow detection is unfortunately triggered before Git can print
out the error message about too-deep trees and exit gracefully. Instead,
it exits with `STATUS_STACK_OVERFLOW`. This corresponds to the numeric
value -1073741571, something the MSYS2 runtime we sadly need to use to
run Git's test suite cannot handle and which it internally maps to the
exit code 127. Git's test suite, in turn, mistakes this to mean that the
command was not found, and fails both test cases.
Here is an example stack trace from an example run:
[0x0] ntdll!RtlpAllocateHeap+0x31 0x4212603f50 0x7ff9d6d4cd49
[0x1] ntdll!RtlpAllocateHeapInternal+0x6c9 0x42126041b0 0x7ff9d6e14512
[0x2] ntdll!RtlDebugAllocateHeap+0x102 0x42126042b0 0x7ff9d6dcd8b0
[0x3] ntdll!RtlpAllocateHeap+0x7ec70 0x4212604350 0x7ff9d6d4cd49
[0x4] ntdll!RtlpAllocateHeapInternal+0x6c9 0x42126045b0 0x7ff9596ed480
[0x5] ucrtbased!heap_alloc_dbg_internal+0x210 0x42126046b0 0x7ff9596ed20d
[0x6] ucrtbased!heap_alloc_dbg+0x4d 0x4212604750 0x7ff9596f037f
[0x7] ucrtbased!_malloc_dbg+0x2f 0x42126047a0 0x7ff9596f0dee
[0x8] ucrtbased!malloc+0x1e 0x42126047d0 0x7ff730fcc1ef
[0x9] git!do_xmalloc+0x2f 0x4212604800 0x7ff730fcc2b9
[0xa] git!do_xmallocz+0x59 0x4212604840 0x7ff730fca779
[0xb] git!xmallocz_gently+0x19 0x4212604880 0x7ff7311b0883
[0xc] git!unpack_compressed_entry+0x43 0x42126048b0 0x7ff7311ac9a4
[0xd] git!unpack_entry+0x554 0x42126049a0 0x7ff7311b0628
[0xe] git!cache_or_unpack_entry+0x58 0x4212605250 0x7ff7311ad3a8
[0xf] git!packed_object_info+0x98 0x42126052a0 0x7ff7310a92da
[0x10] git!do_oid_object_info_extended+0x3fa 0x42126053b0 0x7ff7310a44e7
[0x11] git!oid_object_info_extended+0x37 0x4212605460 0x7ff7310a38ba
[0x12] git!repo_read_object_file+0x9a 0x42126054a0 0x7ff7310a6147
[0x13] git!read_object_with_reference+0x97 0x4212605560 0x7ff7310b4656
[0x14] git!fill_tree_descriptor+0x66 0x4212605620 0x7ff7310dc0a5
[0x15] git!traverse_trees_recursive+0x3f5 0x4212605680 0x7ff7310dd831
[0x16] git!unpack_callback+0x441 0x4212605790 0x7ff7310b4c95
[0x17] git!traverse_trees+0x5d5 0x42126058a0 0x7ff7310dc0f2
[0x18] git!traverse_trees_recursive+0x442 0x4212605980 0x7ff7310dd831
[0x19] git!unpack_callback+0x441 0x4212605a90 0x7ff7310b4c95
[0x1a] git!traverse_trees+0x5d5 0x4212605ba0 0x7ff7310dc0f2
[0x1b] git!traverse_trees_recursive+0x442 0x4212605c80 0x7ff7310dd831
[0x1c] git!unpack_callback+0x441 0x4212605d90 0x7ff7310b4c95
[0x1d] git!traverse_trees+0x5d5 0x4212605ea0 0x7ff7310dc0f2
[0x1e] git!traverse_trees_recursive+0x442 0x4212605f80 0x7ff7310dd831
[0x1f] git!unpack_callback+0x441 0x4212606090 0x7ff7310b4c95
[0x20] git!traverse_trees+0x5d5 0x42126061a0 0x7ff7310dc0f2
[0x21] git!traverse_trees_recursive+0x442 0x4212606280 0x7ff7310dd831
[...]
[0xfad] git!cmd_main+0x2a2 0x42126ff740 0x7ff730fb6345
[0xfae] git!main+0xe5 0x42126ff7c0 0x7ff730fbff93
[0xfaf] git!wmain+0x2a3 0x42126ff830 0x7ff731318859
[0xfb0] git!invoke_main+0x39 0x42126ff8a0 0x7ff7313186fe
[0xfb1] git!__scrt_common_main_seh+0x12e 0x42126ff8f0 0x7ff7313185be
[0xfb2] git!__scrt_common_main+0xe 0x42126ff960 0x7ff7313188ee
[0xfb3] git!wmainCRTStartup+0xe 0x42126ff990 0x7ff9d5ed257d
[0xfb4] KERNEL32!BaseThreadInitThunk+0x1d 0x42126ff9c0 0x7ff9d6d6aa78
[0xfb5] ntdll!RtlUserThreadStart+0x28 0x42126ff9f0 0x0
I verified manually that `traverse_trees_cur_depth` was 562 when that
happened, which is far below the 2048 that were already accepted into
Git as a hard limit.
Despite many attempts to figure out which of the internals trigger this
`STATUS_STACK_OVERFLOW` and how to maybe increase certain sizes to avoid
running into this issue and let Git behave the same way as under Linux,
I failed to find any build-time/runtime knob we could turn to that
effect.
Note: even switching to using a different allocator (I used mimalloc
because that's what Git for Windows uses for its GCC builds) does not
help, as the zlib code used to unpack compressed pack entries _still_
uses the regular `malloc()`. And runs into the same issue.
Note also: switching to using a different allocator _also_ for zlib code
seems _also_ not to help. I tried that, and it still exited with
`STATUS_STACK_OVERFLOW` that seems to have been triggered by a
`mi_assert_internal()`, i.e. an internal assertion of mimalloc...
So the best bet to work around this for now seems to just lower the
maximum allowed tree depth _even further_ for MSVC builds.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Fix t6700.[45] in win+VS test
These two test cases have been failing for a while in Git for Windows'
shears/* branches. Took a good while to figure out, too.
Changes since v1:
* Rewrite the patch to instead lower the max_allowed_tree_depth
threshold even further for MSVC, side-stepping the stack overflow.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1604%2Fdscho%2Ffix-vs-win-test-with-new-depth-limit-test-cases-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1604/dscho/fix-vs-win-test-with-new-depth-limit-test-cases-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1604
Range-diff vs v1:
1: 0e6e53bd824 < -: ----------- tests: handle "funny" exit code 127 produced by MSVC-compiled exes
-: ----------- > 1: 5f738a78eb1 max_tree_depth: lower it for MSVC to avoid stack overflows
environment.c | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/environment.c b/environment.c
index bb3c2a96a33..9e37bf58c0c 100644
--- a/environment.c
+++ b/environment.c
@@ -81,7 +81,20 @@ int merge_log_config = -1;
int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
unsigned long pack_size_limit_cfg;
enum log_refs_config log_all_ref_updates = LOG_REFS_UNSET;
-int max_allowed_tree_depth = 2048;
+int max_allowed_tree_depth =
+#ifdef _MSC_VER
+ /*
+ * When traversing into too-deep trees, Visual C-compiled Git seems to
+ * run into some internal stack overflow detection in the
+ * `RtlpAllocateHeap()` function that is called from within
+ * `git_inflate_init()`'s call tree. The following value seems to be
+ * low enough to avoid that by letting Git exit with an error before
+ * the stack overflow can occur.
+ */
+ 512;
+#else
+ 2048;
+#endif
#ifndef PROTECT_HFS_DEFAULT
#define PROTECT_HFS_DEFAULT 0
base-commit: 3130c155df9a65ebccf128b4af5a19af49532580
--
gitgitgadget
^ permalink raw reply related
* [PATCH v5 8/8] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-11-01 13:03 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6717 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 set 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 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 send better patch series upstream and thus reduce overhead for
the maintainer. Results of these pipeline runs will be made available
(at least) in GitLab's 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. Note that as the builds on GitLab CI run as unprivileged
user, we need to pull in both sudo and shadow packages to our Alpine
based job to set this up.
[1]: https://gitlab.com/gitlab-org/git
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
.gitlab-ci.yml | 53 +++++++++++++++++++++++++++++++
ci/install-docker-dependencies.sh | 13 +++++++-
ci/lib.sh | 44 +++++++++++++++++++++++++
ci/print-test-failures.sh | 6 ++++
4 files changed, 115 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..cd98bcb18aa
--- /dev/null
+++ b/.gitlab-ci.yml
@@ -0,0 +1,53 @@
+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 --create-home
+ - 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: pedantic
+ image: fedora:latest
+ - 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 6e845283680..48c43f0f907 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -16,11 +16,22 @@ linux32)
'
;;
linux-musl)
- apk add --update build-base curl-dev openssl-dev expat-dev gettext \
+ apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
pcre2-dev python3 musl-libintl perl-utils ncurses \
apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
+linux-*)
+ # Required so that apt doesn't wait for user input on certain packages.
+ export DEBIAN_FRONTEND=noninteractive
+
+ 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 \
+ libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
+ apache2 cvs cvsps gnupg libcgi-pm-perl subversion
+ ;;
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 f0a2f80f094..2718ce8776c 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -14,6 +14,22 @@ then
need_to_end_group=
echo '::endgroup::' >&2
}
+elif test true = "$GITLAB_CI"
+then
+ begin_group () {
+ need_to_end_group=t
+ printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K$1\n"
+ trap "end_group '$1'" EXIT
+ set -x
+ }
+
+ end_group () {
+ test -n "$need_to_end_group" || return 0
+ set +x
+ need_to_end_group=
+ printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n"
+ trap - EXIT
+ }
else
begin_group () { :; }
end_group () { :; }
@@ -229,6 +245,34 @@ then
GIT_TEST_OPTS="--github-workflow-markup"
JOBS=10
+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:*|fedora:*|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 : -)
+ JOBS=$(nproc)
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 v5 7/8] ci: install test dependencies for linux-musl
From: Patrick Steinhardt @ 2023-11-01 13:03 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3180 bytes --]
The linux-musl CI job executes tests on Alpine Linux, which is based on
musl libc instead of glibc. We're missing some test dependencies though,
which causes us to skip a subset of tests.
Install these test dependencies to increase our test coverage on this
platform. There are still some missing test dependecies, but these do
not have a corresponding package in the Alpine repositories:
- p4 and p4d, both parts of the Perforce version control system.
- cvsps, which generates patch sets for CVS.
- Subversion and the SVN::Core Perl library, the latter of which is
not available in the Alpine repositories. While the tool itself is
available, all Subversion-related tests are skipped without the
SVN::Core Perl library anyway.
The Apache2-based tests require a bit more care though. For one, the
module path is different on Alpine Linux, which requires us to add it to
the list of known module paths to detect it. But second, the WebDAV
module on Alpine Linux is broken because it does not bundle the default
database backend [1]. We thus need to skip the WebDAV-based tests on
Alpine Linux for now.
[1]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/install-docker-dependencies.sh | 4 +++-
t/lib-httpd.sh | 17 ++++++++++++++++-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
index d0bc19d3bb3..6e845283680 100755
--- a/ci/install-docker-dependencies.sh
+++ b/ci/install-docker-dependencies.sh
@@ -17,7 +17,9 @@ linux32)
;;
linux-musl)
apk add --update build-base curl-dev openssl-dev expat-dev gettext \
- pcre2-dev python3 musl-libintl perl-utils ncurses >/dev/null
+ pcre2-dev python3 musl-libintl perl-utils ncurses \
+ apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
+ bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
pedantic)
dnf -yq update >/dev/null &&
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 2fb1b2ae561..9ea74927c40 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -67,7 +67,8 @@ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
'/usr/lib/apache2/modules' \
'/usr/lib64/httpd/modules' \
'/usr/lib/httpd/modules' \
- '/usr/libexec/httpd'
+ '/usr/libexec/httpd' \
+ '/usr/lib/apache2'
do
if test -d "$DEFAULT_HTTPD_MODULE_PATH"
then
@@ -127,6 +128,20 @@ else
"Could not identify web server at '$LIB_HTTPD_PATH'"
fi
+if test -n "$LIB_HTTPD_DAV" && test -f /etc/os-release
+then
+ case "$(grep "^ID=" /etc/os-release | cut -d= -f2-)" in
+ alpine)
+ # The WebDAV module in Alpine Linux is broken at least up to
+ # Alpine v3.16 as the default DBM driver is missing.
+ #
+ # https://gitlab.alpinelinux.org/alpine/aports/-/issues/13112
+ test_skip_or_die GIT_TEST_HTTPD \
+ "Apache WebDAV module does not have default DBM backend driver"
+ ;;
+ esac
+fi
+
install_script () {
write_script "$HTTPD_ROOT_PATH/$1" <"$TEST_PATH/$1"
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v5 6/8] ci: squelch warnings when testing with unusable Git repo
From: Patrick Steinhardt @ 2023-11-01 13:03 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2232 bytes --]
Our CI jobs that run on Docker also use mostly the same architecture to
build and test Git via the "ci/run-build-and-tests.sh" script. These
scripts also provide some functionality to massage the Git repository
we're supposedly operating in.
In our Docker-based infrastructure we may not even have a Git repository
available though, which leads to warnings when those functions execute.
Make the helpers exit gracefully in case either there is no Git in our
PATH, or when not running in a Git repository.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/ci/lib.sh b/ci/lib.sh
index 0b35da3cfdb..f0a2f80f094 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -69,10 +69,32 @@ skip_branch_tip_with_tag () {
fi
}
+# Check whether we can use the path passed via the first argument as Git
+# repository.
+is_usable_git_repository () {
+ # We require Git in our PATH, otherwise we cannot access repositories
+ # at all.
+ if ! command -v git >/dev/null
+ then
+ return 1
+ fi
+
+ # And the target directory needs to be a proper Git repository.
+ if ! git -C "$1" rev-parse 2>/dev/null
+ then
+ return 1
+ fi
+}
+
# Save some info about the current commit's tree, so we can skip the build
# job if we encounter the same tree again and can provide a useful info
# message.
save_good_tree () {
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
echo "$(git rev-parse $CI_COMMIT^{tree}) $CI_COMMIT $CI_JOB_NUMBER $CI_JOB_ID" >>"$good_trees_file"
# limit the file size
tail -1000 "$good_trees_file" >"$good_trees_file".tmp
@@ -88,6 +110,11 @@ skip_good_tree () {
return
fi
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
if ! good_tree_info="$(grep "^$(git rev-parse $CI_COMMIT^{tree}) " "$good_trees_file")"
then
# Haven't seen this tree yet, or no cached good trees file yet.
@@ -119,6 +146,11 @@ skip_good_tree () {
}
check_unignored_build_artifacts () {
+ if ! is_usable_git_repository .
+ then
+ return
+ fi
+
! git ls-files --other --exclude-standard --error-unmatch \
-- ':/*' 2>/dev/null ||
{
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v5 5/8] ci: unify setup of some environment variables
From: Patrick Steinhardt @ 2023-11-01 13:03 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2475 bytes --]
Both GitHub Actions and Azure Pipelines set up the environment variables
GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
actually the same, the setup is completely duplicate. With the upcoming
support for GitLab CI this duplication would only extend even further.
Unify the setup of those environment variables so that only the uncommon
parts are separated. While at it, we also perform some additional small
improvements:
- We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
It doesn't hurt on platforms where we don't persist the state, so
this further reduces boilerplate.
- When running on Windows systems we set `--no-chain-lint` and
`--no-bin-wrappers`. Interestingly though, we did so _after_
already having exported the respective environment variables.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
ci/lib.sh | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 9ffdf743903..0b35da3cfdb 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -175,11 +175,8 @@ then
# among *all* phases)
cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
- export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
- export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
- MAKEFLAGS="$MAKEFLAGS --jobs=10"
- test windows_nt != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--write-junit-xml"
+ JOBS=10
elif test true = "$GITHUB_ACTIONS"
then
CI_TYPE=github-actions
@@ -198,17 +195,27 @@ then
cache_dir="$HOME/none"
- export GIT_PROVE_OPTS="--timer --jobs 10"
- export GIT_TEST_OPTS="--verbose-log -x --github-workflow-markup"
- MAKEFLAGS="$MAKEFLAGS --jobs=10"
- test windows != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--github-workflow-markup"
+ JOBS=10
else
echo "Could not identify CI type" >&2
env >&2
exit 1
fi
+MAKEFLAGS="$MAKEFLAGS --jobs=$JOBS"
+GIT_PROVE_OPTS="--timer --jobs $JOBS --state=failed,slow,save"
+
+GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
+case "$CI_OS_NAME" in
+windows|windows_nt)
+ GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
+ ;;
+esac
+
+export GIT_TEST_OPTS
+export GIT_PROVE_OPTS
+
good_trees_file="$cache_dir/good-trees"
mkdir -p "$cache_dir"
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v5 4/8] ci: split out logic to set up failed test artifacts
From: Patrick Steinhardt @ 2023-11-01 13:02 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2381 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 artifacts.
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 b3411afae8e..9ffdf743903 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -131,6 +131,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}
@@ -171,25 +192,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 v5 3/8] ci: group installation of Docker dependencies
From: Patrick Steinhardt @ 2023-11-01 13:02 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]
The output of CI jobs tends to be quite long-winded and hard to digest.
To help with this, many CI systems provide the ability to group output
into collapsible sections, and we're also doing this in some of our
scripts.
One notable omission is the script to install Docker dependencies.
Address it to bring more structure to the output for Docker-based jobs.
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 v5 0/8] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-11-01 13:02 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698305961.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 7811 bytes --]
Hi,
this is the fifth version of my patch series that introduces support for
GitLab CI.
There are only minor changes compared to v4:
- Patch 5: Fixed a typo in the commit message. Furthermore, I've
dropped the note about `export VAR=value` being a Bashism.
- Patch 5: Fixed the accidentally-dropped "windows_nt" case.
- Patch 5: Introduce the JOBS variable right away so that there is
less churn in the patch that introduces GitLab CI.
- Patch 8: Moved `DEBIAN_FRONTEND` into the "linux-*" case to make
clear that it shouldn't impact anything else.
- Patch 8: Dropped the paragraph about GitLab CI not being a first
class citizen based on Junio's feedback. This setup is going to be
actively used (at least) by us at GitLab, and thus we're also
going to maintain it.
After both Junio's and Victoria's feedback I've kept `.gitlab-ci.yml` in
its canonical location.
The pipeline for this version can be found at [1].
Thanks for all the discussion and feedback so far!
Patrick
[1]: https://gitlab.com/gitlab-org/git/-/pipelines/1057566736
Patrick Steinhardt (8):
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: unify setup of some environment variables
ci: squelch warnings when testing with unusable Git repo
ci: install test dependencies for linux-musl
ci: add support for GitLab CI
.gitlab-ci.yml | 53 +++++++++
ci/install-docker-dependencies.sh | 23 +++-
ci/lib.sh | 191 +++++++++++++++++++++---------
ci/print-test-failures.sh | 6 +
t/lib-httpd.sh | 17 ++-
5 files changed, 234 insertions(+), 56 deletions(-)
create mode 100644 .gitlab-ci.yml
Range-diff against v4:
1: 8595fe5016a = 1: 0ba396f2a33 ci: reorder definitions for grouping functions
2: 7358a943392 = 2: 821cfcd6125 ci: make grouping setup more generic
3: 6d842592c6f = 3: 6e5bcf143c8 ci: group installation of Docker dependencies
4: e15651b3f5d = 4: 2182acf5bfc ci: split out logic to set up failed test artifacts
5: a64799b6e25 ! 5: 6078aea246d ci: unify setup of some environment variables
@@ Metadata
## Commit message ##
ci: unify setup of some environment variables
- Both GitHub Actions and Azue Pipelines set up the environment variables
+ Both GitHub Actions and Azure Pipelines set up the environment variables
GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
actually the same, the setup is completely duplicate. With the upcoming
support for GitLab CI this duplication would only extend even further.
@@ Commit message
`--no-bin-wrappers`. Interestingly though, we did so _after_
already having exported the respective environment variables.
- - We stop using `export VAR=value` syntax, which is a Bashism. It's
- not quite worth it as we still use this syntax all over the place,
- but it doesn't hurt readability either.
-
Signed-off-by: Patrick Steinhardt <ps@pks.im>
## ci/lib.sh ##
@@ ci/lib.sh: then
- test windows_nt != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--write-junit-xml"
++ JOBS=10
elif test true = "$GITHUB_ACTIONS"
then
CI_TYPE=github-actions
@@ ci/lib.sh: then
- test windows != "$CI_OS_NAME" ||
- GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
+ GIT_TEST_OPTS="--github-workflow-markup"
++ JOBS=10
else
echo "Could not identify CI type" >&2
env >&2
exit 1
fi
-+MAKEFLAGS="$MAKEFLAGS --jobs=10"
-+GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
++MAKEFLAGS="$MAKEFLAGS --jobs=$JOBS"
++GIT_PROVE_OPTS="--timer --jobs $JOBS --state=failed,slow,save"
+
+GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
-+if test windows = "$CI_OS_NAME"
-+then
++case "$CI_OS_NAME" in
++windows|windows_nt)
+ GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
-+fi
++ ;;
++esac
+
+export GIT_TEST_OPTS
+export GIT_PROVE_OPTS
6: f7d2a8666fe = 6: d69bde92f2f ci: squelch warnings when testing with unusable Git repo
7: 9b43b0d90e3 = 7: b911c005bae ci: install test dependencies for linux-musl
8: f3f2c98a0dc ! 8: 5784d03a6f1 ci: add support for GitLab CI
@@ Commit message
that 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 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].
+ the maintainer. Results of these pipeline runs will be made available
+ (at least) in GitLab's 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
@@ .gitlab-ci.yml (new)
+ when: on_failure
## ci/install-docker-dependencies.sh ##
-@@
-
- 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 '
@@ ci/install-docker-dependencies.sh: linux32)
'
;;
@@ ci/install-docker-dependencies.sh: linux32)
bash cvs gnupg perl-cgi perl-dbd-sqlite >/dev/null
;;
+linux-*)
++ # Required so that apt doesn't wait for user input on certain packages.
++ export DEBIAN_FRONTEND=noninteractive
++
+ 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 \
@@ ci/lib.sh: then
begin_group () { :; }
end_group () { :; }
@@ ci/lib.sh: then
- cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
-
- GIT_TEST_OPTS="--write-junit-xml"
-+ JOBS=10
- elif test true = "$GITHUB_ACTIONS"
- then
- CI_TYPE=github-actions
-@@ ci/lib.sh: then
- cache_dir="$HOME/none"
GIT_TEST_OPTS="--github-workflow-markup"
-+ JOBS=10
+ JOBS=10
+elif test true = "$GITLAB_CI"
+then
+ CI_TYPE=gitlab-ci
@@ ci/lib.sh: then
else
echo "Could not identify CI type" >&2
env >&2
- exit 1
- fi
-
--MAKEFLAGS="$MAKEFLAGS --jobs=10"
--GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
-+MAKEFLAGS="$MAKEFLAGS --jobs=${JOBS}"
-+GIT_PROVE_OPTS="--timer --jobs ${JOBS} --state=failed,slow,save"
-
- GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
- if test windows = "$CI_OS_NAME"
## ci/print-test-failures.sh ##
@@ ci/print-test-failures.sh: do
base-commit: 692be87cbba55e8488f805d236f2ad50483bd7d5
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* [PATCH v5 2/8] ci: make grouping setup more generic
From: Patrick Steinhardt @ 2023-11-01 13:02 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2645 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.
Furthermore, the `group ()` function is made generic so that it is the
same for both GitHub Actions and for other platforms. There is a
semantic conflict here though: GitHub Actions used to call `set +x` in
`group ()` whereas the non-GitHub case unconditionally uses `set -x`.
The latter would get overriden if we kept the `set +x` in the generic
version of `group ()`. To resolve this conflict, we simply drop the `set
+x` in the generic variant of this function. As `begin_group ()` calls
`set -x` anyway this is not much of a change though, as the only
commands that aren't printed anymore now are the ones between the
beginning of `group ()` and the end of `begin_group ()`.
Last, 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 | 46 ++++++++++++++++++++++------------------------
1 file changed, 22 insertions(+), 24 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index eb384f4e952..b3411afae8e 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -14,36 +14,34 @@ then
need_to_end_group=
echo '::endgroup::' >&2
}
- trap end_group EXIT
-
- group () {
- set +x
- begin_group "$1"
- shift
- # 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
- return $res
- }
-
- begin_group "CI setup"
else
begin_group () { :; }
end_group () { :; }
- group () {
- shift
- "$@"
- }
set -x
fi
+group () {
+ 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
+}
+
+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 +285,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 v5 1/8] ci: reorder definitions for grouping functions
From: Patrick Steinhardt @ 2023-11-01 13:02 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye
In-Reply-To: <cover.1698843660.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1301 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 GitHub Actions, where we define the non-stub logic in the
else branch.
Reorder the conditional branches 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 0/5] ci: add GitLab CI definition
From: Patrick Steinhardt @ 2023-11-01 11:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Taylor Blau, git
In-Reply-To: <xmqqttq6xr9k.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 4890 bytes --]
On Wed, Nov 01, 2023 at 09:15:51AM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> >> So I have some hesitation about trying to mirror this rather complicated
> >> set of build rules in another CI environment. My primary concern would
> >> be that the two might fall out of sync and a series that is green on
> >> GitHub would be red on GitLab, or vice-versa. Importantly, this can
> >> happen even without changes to the build definitions, since (AFAICT)
> >> both forges distribute new images automatically, so the set of packages
> >> installed in GitHub may not exactly match what's in GitLab (and
> >> vice-versa).
> >
> > Yup, that's a valid concern.
>
> Is it?
>
> I rather naïvely think different set of build options and tools
> running the tests would mean we gain wider test coverage. Even with
> the current setup that relies on whatever GitHub offers, we already
> see "this version passes all tests except for the job on macOS" and
> "the version that was passing yesterday is not broken today---perhas
> the image of the test environment has been updated and we need to
> adjust to it" every once in a while.
>
> > As mentioned, this patch series does not have the intent to make
> > GitLab CI a second authoritative CI platform. GitHub Actions
> > should remain the source of truth of whether a pipeline passes or
> > not.
>
> I am not sure I follow. Often we take a version that happened to
> have failed Actions tests when we know the reason of the failure
> has nothing to do with the new code. From time to time people help
> to make CI tests less flakey, but flakes are expected.
>
> > Most importantly, I do not want to require the maintainer
> > to now watch both pipelines on GitHub and GitLab.
>
> I don't even make tests by GitHub Actions force me to do anything,
> so there is no worry here.
Okay.
> > This might be another indicator that the pipeline should rather be
> > in "contrib/", so that people don't start to treat it as
> > authoritative.
>
> Let me step back and as more basic questions.
>
> - What do you mean by "authoritative"? For an authoritative CI
> test, contributors whose changes do not pass it should take it as
> a sign that their changes need more work? If so, isn't it a
> natural expectation and a good thing? Unless you expect the CI
> tests to be extra flakey, that is.
I was assuming that GitHub Actions was considered to be "the" CI
platform of the Git project. But with your explanations above I think
that assumption may not necessarily hold, or at least not to the extent
I assumed.
> - Are there reasons why you do not trust the CI tests at GitLab
> more than those run at GitHub?
No. Based on the above assumption I was simply treading carefully here.
Most importantly, I didn't want to create the impression that either:
- "Now you have to watch two pipelines", doubling the effort that CI
infrastructure creates for you as a maintainer.
- "I want to eventually replace GitHub Actions".
This carefulness probably also comes from the fact that GitLab and
GitHub are direct competitors, so I was trying to preempt any kind of
implied agenda here. There is none, I just want to make sure that it
becomes easier for us at GitLab and other potential contributors that
use GitLab to contribute to Git.
Hope that makes sense.
> > Last but not least, I actually think that having multiple supported CI
> > platforms also has the benefit that people can more readily set it up
> > for themselves. In theory, this has the potential to broaden the set of
> > people willing to contribute to our `ci/` scripts, which would in the
> > end also benefit GitHub Actions.
>
> Yes, assuming that we can do so without much cutting and pasting but
> with a clear sharing of the infrastructure code, and the multiple
> supported CI environments are not too flakey, I am with this rather
> naïve worldview that the more we have the merrier we would be.
>
> > I understand your points, and especially the point about not having a
> > second authoritative CI platform. I'm very much on the same page as you
> > are here, and would be happy to move the definitions to "contrib/" if
> > you want me to.
> >
> > But I think we should also see the potential benefit of having a second
> > CI platform, as it enables a more diverse set of people to contribute.
> > which can ultimately end up benefitting our CI infra for both GitHub
> > Actions and GitLab CI.
>
> I do *not* want to add new things, if we were to use them ourselves,
> to "contrib/". We have passed that stage long time ago that keeping
> everything in my tree gives wider exposure and foster cooperation.
Fair enough.
Thanks for taking the time to make your thoughts clearer to me!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 8/8] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-11-01 11:44 UTC (permalink / raw)
To: Victoria Dye
Cc: git, Taylor Blau, Junio C Hamano, Phillip Wood,
Oswald Buddenhagen
In-Reply-To: <4c8c2f19-1a7e-4524-81e7-c74091e88edf@github.com>
[-- Attachment #1: Type: text/plain, Size: 4427 bytes --]
On Tue, Oct 31, 2023 at 10:47:44AM -0700, Victoria Dye wrote:
> Patrick Steinhardt wrote:> diff --git a/ci/install-docker-dependencies.sh b/ci/install-docker-dependencies.sh
> > index 6e845283680..48cb2e735b5 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,11 +19,19 @@ linux32)
> > '
> > ;;
> > linux-musl)
> > - apk add --update build-base curl-dev openssl-dev expat-dev gettext \
> > + apk add --update shadow sudo build-base curl-dev openssl-dev expat-dev gettext \
> > pcre2-dev python3 musl-libintl perl-utils ncurses \
> > apache2 apache2-http2 apache2-proxy apache2-ssl apache2-webdav apr-util-dbd_sqlite3 \
> > bash cvs gnupg perl-cgi perl-dbd-sqlite >/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 \
> > + libdbd-sqlite3-perl libio-socket-ssl-perl libnet-smtp-ssl-perl ${CC_PACKAGE:-${CC:-gcc}} \
> > + apache2 cvs cvsps gnupg libcgi-pm-perl subversion
> > + ;;
> > 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 e14b1029fad..6e3d64004ec 100755
> > --- a/ci/lib.sh
> > +++ b/ci/lib.sh
> > @@ -208,6 +224,7 @@ then
> > cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
> >
> > GIT_TEST_OPTS="--write-junit-xml"
> > + JOBS=10
> > elif test true = "$GITHUB_ACTIONS"
> > then
> > CI_TYPE=github-actions
>
> ...
>
> > -MAKEFLAGS="$MAKEFLAGS --jobs=10"
> > -GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
> > +MAKEFLAGS="$MAKEFLAGS --jobs=${JOBS}"
> > +GIT_PROVE_OPTS="--timer --jobs ${JOBS} --state=failed,slow,save"
> >
>
> Organizationally, this commit seems to be doing two things at once:
>
> - Adding GitLab-specific CI setup (either in the new .gitlab-ci.yml or in
> conditions gated on "gitlab-ci").
> - Updating the common CI scripts with things that are needed for GitLab CI,
> but aren't conditioned on it (i.e. the patch excerpts I've included
> above).
>
> I'd prefer these being separated into two patches, mainly to isolate "things
> that affect all CI" from "things that affect only GitLab CI". This is
> ultimately a pretty minor nit, though; if you're not planning on re-rolling
> (or just disagree with what I'm suggesting :) ), I'm okay with leaving it
> as-is.
Yeah, the JOBS refactoring can certainly be split out into a preparatory
commit where we unify the envvars (currently patch 5). But for the other
changes it makes a bit less sense to do so, in my opinion:
- The DEBIAN_FRONTEND variable isn't needed before as the there are
no Docker-based CI jobs that use apt.
- Adding the shadow and sudo packages to the linux-musl job wouldn't
be needed either as there are no cases yet where we run
unprivileged CI builds via Docker.
- Adding the apt packages as a preparatory step doesn't make much
sense either as there is no Docker job using it.
But anyway. I will:
- Move around the JOBS variable refactoring to a preparatory patch,
which feels sensible to me.
- Move the `DEBIAN_FRONTEND` varible into the "linux-*" case, which
should further clarify that this only impacts the newly added and
thus GitLab-specific infrastructure.
With these changes, the only thing left in this commit that is not
guarded by a GitLab CI specific condition is the change to the
"linux-musl" case where we install shadow and sudo now. But I don't feel
like it makes sense to move them into a standalone preparatory commit.
Thanks!
Patrick
> Otherwise, I can't comment on the correctness of the GitLab CI definition (I
> assume you've tested it anyway), but AFAICT the changes above shouldn't break
> GitHub CI.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 5/8] ci: unify setup of some environment variables
From: Patrick Steinhardt @ 2023-11-01 11:44 UTC (permalink / raw)
To: Victoria Dye
Cc: git, Taylor Blau, Junio C Hamano, Phillip Wood,
Oswald Buddenhagen
In-Reply-To: <31ebe4c9-84aa-4d42-9aeb-712e2a6cece3@github.com>
[-- Attachment #1: Type: text/plain, Size: 3584 bytes --]
On Tue, Oct 31, 2023 at 10:06:24AM -0700, Victoria Dye wrote:
> Patrick Steinhardt wrote:
> > Both GitHub Actions and Azue Pipelines set up the environment variables
>
> s/Azue/Azure
>
> > GIT_TEST_OPTS, GIT_PROVE_OPTS and MAKEFLAGS. And while most values are
> > actually the same, the setup is completely duplicate. With the upcoming
> > support for GitLab CI this duplication would only extend even further.
> >
> > Unify the setup of those environment variables so that only the uncommon
> > parts are separated. While at it, we also perform some additional small
> > improvements:
> >
> > - We now always pass `--state=failed,slow,save` via GIT_PROVE_OPTS.
> > It doesn't hurt on platforms where we don't persist the state, so
> > this further reduces boilerplate.
> >
> > - When running on Windows systems we set `--no-chain-lint` and
> > `--no-bin-wrappers`. Interestingly though, we did so _after_
> > already having exported the respective environment variables.
> >
> > - We stop using `export VAR=value` syntax, which is a Bashism. It's
> > not quite worth it as we still use this syntax all over the place,
> > but it doesn't hurt readability either.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > ci/lib.sh | 24 ++++++++++++++----------
> > 1 file changed, 14 insertions(+), 10 deletions(-)
> >
> > diff --git a/ci/lib.sh b/ci/lib.sh
> > index 9ffdf743903..9a9b92c05b3 100755
> > --- a/ci/lib.sh
> > +++ b/ci/lib.sh
> > @@ -175,11 +175,7 @@ then
> > # among *all* phases)
> > cache_dir="$HOME/test-cache/$SYSTEM_PHASENAME"
> >
> > - export GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
> > - export GIT_TEST_OPTS="--verbose-log -x --write-junit-xml"
> > - MAKEFLAGS="$MAKEFLAGS --jobs=10"
> > - test windows_nt != "$CI_OS_NAME" ||
> > - GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
> > + GIT_TEST_OPTS="--write-junit-xml"
> > elif test true = "$GITHUB_ACTIONS"
> > then
> > CI_TYPE=github-actions
> > @@ -198,17 +194,25 @@ then
> >
> > cache_dir="$HOME/none"
> >
> > - export GIT_PROVE_OPTS="--timer --jobs 10"
> > - export GIT_TEST_OPTS="--verbose-log -x --github-workflow-markup"
> > - MAKEFLAGS="$MAKEFLAGS --jobs=10"
> > - test windows != "$CI_OS_NAME" ||
> > - GIT_TEST_OPTS="--no-chain-lint --no-bin-wrappers $GIT_TEST_OPTS"
> > + GIT_TEST_OPTS="--github-workflow-markup"
> > else
> > echo "Could not identify CI type" >&2
> > env >&2
> > exit 1
> > fi
> >
> > +MAKEFLAGS="$MAKEFLAGS --jobs=10"
> > +GIT_PROVE_OPTS="--timer --jobs 10 --state=failed,slow,save"
> > +
> > +GIT_TEST_OPTS="$GIT_TEST_OPTS --verbose-log -x"
> > +if test windows = "$CI_OS_NAME"
>
> Based on the deleted lines above, I think this would need to be:
>
> if test windows = "$CI_OS_NAME" || test windows_nt = "$CI_OS_NAME"
>
> I believe these settings are required on all Windows builds, though, so you could
> instead match on the first 7 characters of $CI_OS_NAME:
>
> if test windows = "$(echo "$CI_OS_NAME" | cut -c1-7)"
>
> (full disclosure: I'm not 100% confident in the correctness of that shell syntax)
Oh, right. I didn't notice the slight difference between "windows" and
"windows_nt". Thanks!
Patrick
> > +then
> > + GIT_TEST_OPTS="$GIT_TEST_OPTS --no-chain-lint --no-bin-wrappers"
> > +fi
> > +
> > +export GIT_TEST_OPTS
> > +export GIT_PROVE_OPTS
> > +
> > good_trees_file="$cache_dir/good-trees"
> >
> > mkdir -p "$cache_dir"
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 0/8] ci: add GitLab CI definition
From: Patrick Steinhardt @ 2023-11-01 11:44 UTC (permalink / raw)
To: Junio C Hamano
Cc: Victoria Dye, git, Taylor Blau, Phillip Wood, Oswald Buddenhagen
In-Reply-To: <xmqqpm0uw41b.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]
On Wed, Nov 01, 2023 at 12:22:56PM +0900, Junio C Hamano wrote:
> Victoria Dye <vdye@github.com> writes:
>
> > As for adding the GitLab-specific stuff, I'm not opposed to having it in the
> > main tree. For one, there doesn't seem to be a clean way to "move it into
> > `contrib/`" - '.gitlab-ci.yml' must be at the root of the project [2], and
> > moving the $GITLAB_CI conditions out of the 'ci/*.sh' files into dedicated
> > scripts would likely result in a lot of duplicated code (which doesn't solve
> > the maintenance burden issue this series intends to address).
It is possible to change the location of the `.gitlab-ci.yml` in GitLab
projects, so moving it into `contrib/` would work, too. But it does of
course require additional setup by the project admin.
> > More generally, there are lots of open source projects that include CI
> > configurations across different forges, _especially_ those that are
> > officially mirrored across a bunch of them. As long as there are
> > contributors with a vested interest in keeping the GitLab CI definition
> > stable (and your cover letter indicates that there are), and the GitLab
> > stuff doesn't negatively impact any other CI configurations, I think it
> > warrants the same treatment as e.g. GitHub CI.
>
> Thanks for expressing this so clearly. I do prefer to add this as
> the first class citizen (more generally, I do not want to add new
> things to contrib/ at this point) if we are going to use it.
I certainly know that we in the Gitaly team will use this CI definition
on a daily basis, which is my main motivation to add and maintain it in
the future. I personally don't mind to add this as a first-class
citizen, because for my own workflows I'll certainly treat it as such.
And if others can benefit from it, too, then I'm even happier.
I'll keep it in the default location at `.gitlab-ci.yml` then, thanks
for your thoughts!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] merge: --ff-one-only to apply FF if commit is one
From: Kristoffer Haugsbakk @ 2023-11-01 10:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Soref, git, Ruslan Yakauleu, Taylor Blau
In-Reply-To: <xmqqa5ryxn8i.fsf@gitster.g>
On Wed, Nov 1, 2023, at 02:42, Junio C Hamano wrote:
> Strictly speaking, the log message on a merge commit serves two
> purposes, one is to summarize commit(s) on the side branch that gets
> merged with the merge, and as you said above, it is not needed when
> merging a topic with just one commit. But the other is to justify
> why the topic suits the objective of the line of history (which is
> needed even when merging a single commit topic---imagine a commit
> that is not incorrect per-se. It may or may not be suitable for the
> maintenance track, and a merge commit of such a commit into the
> track can explain if/how the commit being merged is maint-worthy).
Yes. If you have multiple release/maintenance branches which you need to
apply something to then you can’t use this .
>> The point at which you use such a merge feature is when you are already
>> happy with the pull request (or patch series or whatever else). And then
>> you (according to this strategy) prefer to avoid merge commits for
>> single-commit pull requests.
>
> But that argues against the "--ff-one-only" option, doesn't it?
>
> If you looked at the side branch before you decide to merge it, you
> know if the topic has only one commit (in which case you decide not
> to use "--no-ff"), or if the topic consists of multiple commits (in
> which case you decide to use "--no-ff"). And the only effect to
> have the "--ff-one-only" option is to allow you *not* to look at
> what is on the side branch.
No. The only effect is that you streamline the process of “decide not to
use `--no-ff`” since the strategy does it for you.
It would act like a small `git my-merge` alias. That is all.
--
Kristoffer Haugsbakk
^ 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