* Re: git-send-email: Send with mutt(1)
From: Konstantin Ryabitsev @ 2023-11-09 16:08 UTC (permalink / raw)
To: Alejandro Colomar; +Cc: Jeff King, git
In-Reply-To: <ZUz6H3IqRc1YGPZM@debian>
On Thu, Nov 09, 2023 at 04:26:23PM +0100, Alejandro Colomar wrote:
> I used it for sending a couple of patches to linux-man@, and it seems to
> work. I don't have much experience with mutt, so maybe I'm missing some
> corner cases. Do you expect it to not work for some case? Otherwise,
> we might have a winner. :)
Since it's a Linux project, I suggest also checking out b4, which will do the
mail sending for you as part of the contributor-oriented features:
https://b4.docs.kernel.org/en/latest/contributor/overview.html
We also provide a web relay for people who can't configure or use SMTP due to
their company policies.
> > > Would you mind adding this as part of git? Or should we suggest the
> > > mutt project adding this script?
> >
> > IMHO it is a little too weird and user-specific to really make sense in
> > either project. It's really glue-ing together two systems. And as it's
> > not something I use myself, I don't plan it moving it further along. But
> > you are welcome to take what I wrote and do what you will with it,
> > including submitting it to mutt.
>
> I'll start by creating a git repository in my own server, and will write
> something about it to let the public know about it. I'll also start
> requiring contributors to linux-man@ to sign their patches, and
> recommend them using this if they use mutt(1).
B4 will also sign your patches for you. ;)
-K
^ permalink raw reply
* Re: git-send-email: Send with mutt(1)
From: Alejandro Colomar @ 2023-11-09 15:26 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20231108212702.GA1586965@coredump.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2416 bytes --]
Hi Jeff,
On Wed, Nov 08, 2023 at 04:27:02PM -0500, Jeff King wrote:
> > > # spool the message to a fake mbox; we need to add
> > > # a "From" line to make it look legit
> > > trap 'rm -f to-send' 0 &&
> > > {
> > > echo "From whatever Mon Sep 17 00:00:00 2001" &&
> > > cat
> > > } >to-send &&
> >
> > Would a named pipe work? Or maybe we could use $(mktemp)?
>
> I suspect mutt wants it to be a real file. But yeah, mktemp would
> definitely work. I actually started to write it that way but switched to
> a static name for simplicity in demonstrating the idea. :)
>
> One note, though. Later we need to pass this filename to mutt config:
>
> > > mutt -p \
> > > -e 'set postponed=to-send' \
>
> so it's a potential worry if "mktemp" might use a path with spaces or
> funny characters (e.g., from $TMPDIR). Probably not much of a problem in
> practice, though.
>
> > Huh, this is magic sauce! Works perfect for what I need. This would
> > need to be packaged to the masses. :-)
> >
> > I found a minor problem: If I ctrl+C within mutt(1), I expect it to
> > cancel the last action, but this script intercepts the signal and exits.
> > We would probably need to ignore SIGINT from mutt-as-mta.
>
> Yeah, that might make sense, and can be done with trap.
I've tried something even simpler:
---8<---
#!/bin/sh
mutt -H -;
--->8---
I used it for sending a couple of patches to linux-man@, and it seems to
work. I don't have much experience with mutt, so maybe I'm missing some
corner cases. Do you expect it to not work for some case? Otherwise,
we might have a winner. :)
>
> > Would you mind adding this as part of git? Or should we suggest the
> > mutt project adding this script?
>
> IMHO it is a little too weird and user-specific to really make sense in
> either project. It's really glue-ing together two systems. And as it's
> not something I use myself, I don't plan it moving it further along. But
> you are welcome to take what I wrote and do what you will with it,
> including submitting it to mutt.
I'll start by creating a git repository in my own server, and will write
something about it to let the public know about it. I'll also start
requiring contributors to linux-man@ to sign their patches, and
recommend them using this if they use mutt(1).
Cheers,
Alex
--
<https://www.alejandro-colomar.es/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: first-class conflicts?
From: phillip.wood123 @ 2023-11-09 14:50 UTC (permalink / raw)
To: Martin von Zweigbergk, phillip.wood
Cc: Elijah Newren, Sandra Snan, git, Randall S. Becker
In-Reply-To: <CAESOdVDmQ85-des6Au-LH0fkUB9BZBZho0r-5=8MkPLJVA5WQQ@mail.gmail.com>
Hi Martin
On 07/11/2023 17:38, Martin von Zweigbergk wrote:
> (new attempt in plain text)
Oh, the joys of the mailing list! Thanks for your comments below and in
your reply to Elijah, I found them really helpful to get a better
understanding of how 'jj' handles this.
Best Wishes
Phillip
> On Tue, Nov 7, 2023 at 3:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Elijah
>>
>> [I've cc'd Martin to see if he has anything to add about how "jj"
>> manages the issues around storing conflicts.]
>>
>> On 07/11/2023 08:16, Elijah Newren wrote:
>>> On Mon, Nov 6, 2023 at 1:26 PM Sandra Snan
>>> <sandra.snan@idiomdrottning.org> wrote:
>>>>
>>>> Is this feature from jj also a good idea for git?
>>>> https://martinvonz.github.io/jj/v0.11.0/conflicts/
>>>
>>> Martin talked about this and other features at Git Merge 2022, a
>>> little over a year ago. I talked to him in more depth about these
>>> while there. I personally think he has some really interesting
>>> features here, though at the time, I thought that the additional
>>> object type might be too much to ask for in a Git change, and it was
>>> an intrinsic part of the implementation back then.
>>>
>>> Martin also gave us an update at the 2023 Git Contributors summit, and
>>> in particular noted a significant implementation change to not have
>>> per-file storage of conflicts, but rather storing at the commit level
>>> the multiple conflicting trees involved. That model might be
>>> something we could implement in Git. And if we did, it'd solve
>>> various issues such as people wanting to be able to stash conflicts,
>>> or wanting to be able to partially resolve conflicts and fix it up
>>> later, or be able to collaboratively resolve conflicts without having
>>> everyone have access to the same checkout.
>>
>> One thing to think about if we ever want to implement this is what other
>> data we need to store along with the conflict trees to preserve the
>> context in which the conflict was created. For example the files that
>> are read by "git commit" when it commits a conflict resolution. For a
>> single cherry-pick/revert it would probably be fairly straight forward
>> to store CHERRY_PICK_HEAD/REVERT_HEAD and add it as a parent so it gets
>> transferred along with the conflicts. For a sequence of cherry-picks or
>> a rebase it is more complicated to preserve the context of the conflict.
>> Even "git merge" can create several files in addition to MERGE_HEAD
>> which are read when the conflict resolution is committed.
>
> Good point. We actually don't store any extra data in jj. The old
> per-path conflict model was prepared for having some label associated
> with each term of the conflict but we never actually used it.
>
> If we add such metadata, it would probably have to be something that
> makes sense even after pushing the conflict to another repo, so it
> probably shouldn't be commit ids, unless we made sure to also push
> those commits. Also note that if you `jj restore --from <commit with
> conflict>`, you can get a conflict into a commit that didn't have
> conflicts previously. Or if you already had conflicts in the
> destination commit, your root trees (the multiple root trees
> constituting the conflict) will now have conflicts that potentially
> were created by two completely unrelated operations, so you would kind
> of need different labels for different paths.
>
> https://github.com/martinvonz/jj/issues/1176 has some more discussion
> about this.
>
>>> But we'd also have to be careful and think through usecases, including
>>> in the surrounding community. People would probably want to ensure
>>> that e.g. "Protected" or "Integration" branches don't get accept
>>> fetches or pushes of conflicted commits,
>>
>> I think this is a really important point, while it can be useful to
>> share conflicts so they can be collaboratively resolved we don't want to
>> propagate them into "stable" or production branches. I wonder how 'jj'
>> handles this.
>
> Agreed. `jj git push` refuses to push commits with conflicts, because
> it's very unlikely that the remote will be able to make any sense of
> it. Our commit backend at Google does support conflicts, so users can
> check out each other's conflicted commits there (except that we
> haven't even started dogfooding yet).
>
>>> git status would probably
>>> need some special warnings or notices, git checkout would probably
>>> benefit from additional warnings/notices checks for those cases, git
>>> log should probably display conflicted commits differently, we'd need
>>> to add special handling for higher order conflicts (e.g. a merge with
>>> conflicts is itself involved in a merge) probably similar to what jj
>>> has done, and audit a lot of other code paths to see what would be
>>> needed.
>>
>> As you point out there is a lot more to this than just being able to
>> store the conflict data in a commit - in many ways I think that is the
>> easiest part of the solution to sharing conflicts.
>
> Yes, I think it would be a very large project. Unlike jj, Git of
> course has to worry about backwards compatibility. For example, you
> would have to decide if your goal - even in the long term - is to make
> `git rebase` etc. not get interrupted due to conflicts.
^ permalink raw reply
* Re: first-class conflicts?
From: Phillip Wood @ 2023-11-09 14:45 UTC (permalink / raw)
To: Elijah Newren, phillip.wood
Cc: Sandra Snan, git, Martin von Zweigbergk, Randall S. Becker
In-Reply-To: <CABPp-BGd-W8T7EsvKYyjdi3=mfSTJ8zM-uzVsFnh1AWyV2wEzQ@mail.gmail.com>
Hi Elijah
On 08/11/2023 06:31, Elijah Newren wrote:
> Hi Phillip,
>
> On Tue, Nov 7, 2023 at 3:49 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Elijah
>>
>> [I've cc'd Martin to see if he has anything to add about how "jj"
>> manages the issues around storing conflicts.]
>
> +1. I'll add some other questions for him too while we're at it,
> separately in this thread.
>
> [...]
>
>>> Martin also gave us an update at the 2023 Git Contributors summit, and
>>> in particular noted a significant implementation change to not have
>>> per-file storage of conflicts, but rather storing at the commit level
>>> the multiple conflicting trees involved. That model might be
>>> something we could implement in Git. And if we did, it'd solve
>>> various issues such as people wanting to be able to stash conflicts,
>>> or wanting to be able to partially resolve conflicts and fix it up
>>> later, or be able to collaboratively resolve conflicts without having
>>> everyone have access to the same checkout.
>>
>> One thing to think about if we ever want to implement this is what other
>> data we need to store along with the conflict trees to preserve the
>> context in which the conflict was created. For example the files that
>> are read by "git commit" when it commits a conflict resolution. For a
>> single cherry-pick/revert it would probably be fairly straight forward
>> to store CHERRY_PICK_HEAD/REVERT_HEAD and add it as a parent so it gets
>> transferred along with the conflicts.
>
> This is a great thing to think about and bring up. However, I'm not
> sure what part of it actually needs to be preserved; in fact, it's not
> clear to me that any of it needs preserving -- especially not the
> files read by "git commit". A commit was already created, after all.
>
> It seems that CHERRY_PICK_HEAD/REVERT_HEAD files exist primarily to
> clue in that we are in-the-middle-of-<op>, and the conflict header
> (the "tree A + tree B - tree C" thing; whatever that's called)
> similarly provides signal that the commit still has conflicts.
> Secondarily, these files contain information about the tree we came
> from and its parent tree, which allows users to investigate the diff
> between those...but that information is also available from the
> conflict header in the recorded commit. The CHERRY_PICK_HEAD and
> REVERT_HEAD files could also be used to access the commit message, but
> that would have been stored in the conflicted commit as well. Are
> there any other pieces of information I'm missing?
Mainly that I'm an idiot and forgot we were actually creating a commit
and can store the message and authorship there! More seriously I think
being able to inspect the commit being cherry-picked (including the
original commit message) is useful so we'd need to recreate something
like CHERRY_PICK_HEAD when the conflict commit is checked out.
Recreating CHERRY_PICK_HEAD is useful for "git status" as well. I think
that means storing a little more that just the "tree A + tree B - tree
C" thing.
>> For a sequence of cherry-picks or
>> a rebase it is more complicated to preserve the context of the conflict.
>
> I think the big piece here is whether we also want to adopt jj's
> behavior of automatically rebasing all descendant commits when
> checking out and amending some historical commit (or at least having
> the option of doing so). That behavior allows users to amend commits
> to resolve conflicts without figuring out complicated interactive
> rebases to fix all the descendant commits across all relevant
> branches.
That's a potentially attractive option which is fairly simple to
implement locally as I think you can use the commit DAG to find all the
descendants though that could be expensive if there are lots of
branches. However, if we're going to share conflicts I think we'd need
something like "hg evolve" - if I push a commit with conflicts and you
base some work on it and then I resolve the conflict and push again you
would want to your work to be rebased onto my conflict resolution. To
handle "rebase --exec" we could store the exec command and run it when
the conflicts are resolved.
Also I wonder how annoying it would be in cases where I just want to
rebase and resolve the conflicts now. At the moment "git rebase" stops
at the conflict, with this feature I'd have to go and checkout the
conflicted commit and fix the conflicts after the rebase had finished.
> Without that feature, I agree this might be a bit more
> difficult,
Yes, when I wrote my original message I was imagining that we'd stop at
the first conflicting pick and store all the rebase state like some kind
of stash on steroids so it could be continued when the conflict was
resolved. It would be much simpler to try and avoid that.
> but with that feature, I'm having a hard time figuring out
> what context we actually need to preserve for a sequence of
> cherry-picks or a rebase.
>
> Digging into a few briefly...
>
> Many of the state files are about the status of the in-progress
> operation (todo-list, numbers of commits done and to do, what should
> be done with not-yet-handled commits, temporary refs corresponding to
> temporary labels that need to be deleted, rescheduling failed execs,
> dropping or keeping redundant commits, etc.), but if the operation has
> completed and new commits created (potentially with multiple files
> with conflict headers), I don't see how this information is useful
> anymore.
Agreed
> There are some special state files related to half-completed
> operations (e.g. squash commits when we haven't yet reached the final
> one in the sequence, a file to note that we want to edit a commit
> message once the user has finished resolving conflicts, whether we
> need to create a new root commit), but again, the operation has
> completed and commits have been created with appropriate parentage and
> commit messages so I don't think these are useful anymore either.
Yes, though we may want to remember which commits were squashed together
so the user can inspect that when resolving conflicts.
> Other state files are related to things needing to be done at the end
> of the operation, like invoke the post-rewrite hook or pop the
> autostash (with knowledge of what was rewritten to what). But the
> operation would have been completed and those things done already, so
> I don't see how this is necessary either.
Agreed
> Some state files are for controlling how commits are created (setting
> committer date to author date, gpg signing options, whether to add
> signoff), but, again, commits have already been created, and can be
> further amended as the user wants (hopefully including resolving the
> conflicts).
Agreed
> The biggest issue is perhaps that REBASE_HEAD is used in the
> implementation of `git rebase --show-current-patch`, but all
> information stored in that is still accessible -- the commit message
> is stored in the commit, the author time is stored in the commit, and
> the trees involved are in the conflict header. The only thing missing
> is committer timestamp, which isn't relevant anyway.
The commit message may have been edited so we lose the original message
but I'm not sure how important that is.
> The only ones I'm pausing a bit on are the strategy and
> strategy-options. Those might be useful somehow...but I can't
> currently quite put my finger on explaining how they would be useful
> and I'm not sure they are.
I can't think of an immediate use for them. When we re-create conflicts
we do it per-file based on the index entries created by the original
merge so I don't think we need to know anything about the strategy or
strategy-options.
> Am I missing anything?
exec commands? If the user runs "git rebase --exec" and there are
conflicts then we'd need to defer running the exec commands until the
conflicts are resolved. For something like "git rebase --exec 'make
test'" that should be fine. I wonder if there are corner cases where the
exec command changes HEAD though.
>> Even "git merge" can create several files in addition to MERGE_HEAD
>> which are read when the conflict resolution is committed.
>
> That's a good one to bring up too, but I'm not sure I understand how
> these could be useful to preserve either. Am I missing something? My
> breakdown:
> * MERGE_HEAD: was recorded in the commit as a second parent, so we
> already have that info
> * MERGE_MSG: was recorded in the commit as the commit message, so
> again we already have that info
> * MERGE_AUTOSTASH: irrelevant since the stashed stuff isn't part of
> the commit and was in fact unstashed after the
> merge-commit-with-conflicts was created
> * MERGE_MODE: irrelevant since it's only used for reducing heads at
> time of git-commit, and git-commit has already been run
> * MERGE_RR: I think this is irrelevant; the conflict record (tree A
> + tree B - tree C) lets us redo the merge if needed to get the list of
> conflicted files and textual conflicts found therein
>
> So I don't see how any of the information in these files need to be
> recorded as additional auxiliary information. However, that last item
> might depend upon the strategy and strategy-options, which currently
> is not recorded...hmm....
Yes, as we're creating some kind of commit we don't need to preserve
those files separately.
>>> But we'd also have to be careful and think through usecases, including
>>> in the surrounding community. People would probably want to ensure
>>> that e.g. "Protected" or "Integration" branches don't get accept
>>> fetches or pushes of conflicted commits,
>>
>> I think this is a really important point, while it can be useful to
>> share conflicts so they can be collaboratively resolved we don't want to
>> propagate them into "stable" or production branches. I wonder how 'jj'
>> handles this.
>
> Yeah, figuring this out might be the biggest sticking point.
Indeed
>>> git status would probably
>>> need some special warnings or notices, git checkout would probably
>>> benefit from additional warnings/notices checks for those cases, git
>>> log should probably display conflicted commits differently, we'd need
>>> to add special handling for higher order conflicts (e.g. a merge with
>>> conflicts is itself involved in a merge) probably similar to what jj
>>> has done, and audit a lot of other code paths to see what would be
>>> needed.
>>
>> As you point out there is a lot more to this than just being able to
>> store the conflict data in a commit - in many ways I think that is the
>> easiest part of the solution to sharing conflicts.
>
> Yeah, another one I just thought of is that the trees referenced in
> the conflicts would also need to affect reachability computations as
> well, to make sure they both don't get gc'ed and that they are
> transferred when appropriate. There are lots of things that would be
> involved in implementing this idea.
Yes, it would certainly be lots of work.
Best Wishes
Phillip
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2023, #04; Thu, 9)
From: Taylor Blau @ 2023-11-09 13:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqq34xg5ek3.fsf@gitster.g>
On Thu, Nov 09, 2023 at 02:40:28AM +0900, Junio C Hamano wrote:
> * tb/merge-tree-write-pack (2023-10-23) 5 commits
> - builtin/merge-tree.c: implement support for `--write-pack`
> - bulk-checkin: introduce `index_tree_bulk_checkin_incore()`
> - bulk-checkin: introduce `index_blob_bulk_checkin_incore()`
> - bulk-checkin: generify `stream_blob_to_pack()` for arbitrary types
> - bulk-checkin: extract abstract `bulk_checkin_source`
>
> "git merge-tree" learned "--write-pack" to record its result
> without creating loose objects.
>
> Comments?
> source: <cover.1698101088.git.me@ttaylorr.com>
This series received a couple of LGTMs from you and Patrick:
- https://lore.kernel.org/git/xmqqo7go7w63.fsf@gitster.g/#t
- https://lore.kernel.org/git/ZTjKmcV5c_EFuoGo@tanuki/
Johannes had posted some comments[1] about instead using a temporary
object store where objects are written as loose that would extend to git
replay. Like Peff mentions[2] below in that thread, that approach would
still involve writing loose objects, and it is the goal of my series to
avoid doing so.
I demonstrated in a follow-up thread[3] that my approach of using the
bulk-checkin and tmp-objdir APIs does extend straightforwardly to 'git
replay'. This works by writing out one pack per replay step in a
temporary object directory, and then running 'git repack -adf' on that
temporary object directory before migrating a single pack containing all
new objects back into the main object store.
I am fairly confident that tb/merge-tree-write-pack is ready to go. I'll
spin off a separate thread based on that branch and cc/git-replay as a
non-RFC series that extends this approach to 'git replay', so we'll be
ready to go there once Christian's series progresses.
[1]: https://lore.kernel.org/git/0ac32374-7d52-8f0c-8583-110de678291e@gmx.de/
[2]: https://lore.kernel.org/git/20231107034224.GA874199@coredump.intra.peff.net/
[3]: https://lore.kernel.org/git/cover.1699381371.git.me@ttaylorr.com/
Thanks,
Taylor
^ permalink raw reply
* Re: What's cooking in git.git (Nov 2023, #04; Thu, 9)
From: Taylor Blau @ 2023-11-09 13:15 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20231109075052.GA2699557@coredump.intra.peff.net>
On Thu, Nov 09, 2023 at 02:50:52AM -0500, Jeff King wrote:
> On Thu, Nov 09, 2023 at 02:40:28AM +0900, Junio C Hamano wrote:
>
> > * tb/pair-chunk-expect-size (2023-10-14) 8 commits
> > - midx: read `OOFF` chunk with `pair_chunk_expect()`
> > - midx: read `OIDL` chunk with `pair_chunk_expect()`
> > - midx: read `OIDF` chunk with `pair_chunk_expect()`
> > - commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
> > - commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
> > - commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
> > - commit-graph: read `OIDF` chunk with `pair_chunk_expect()`
> > - chunk-format: introduce `pair_chunk_expect()` helper
> >
> > Code clean-up for jk/chunk-bounds topic.
> >
> > Comments?
> > source: <45cac29403e63483951f7766c6da3c022c68d9f0.1697225110.git.me@ttaylorr.com>
> > source: <cover.1697225110.git.me@ttaylorr.com>
>
> Sorry it took me a while to circle back to this topic. I posted a
> competing series just now in:
>
> https://lore.kernel.org/git/20231109070310.GA2697602@coredump.intra.peff.net/
>
> that I think should take precedence (and would require some reworking of
> Taylor's patches, so you'd just eject them in the meantime).
ACK: that sounds like a good plan to me. Thanks for picking this back
up, Peff!
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Junio C Hamano @ 2023-11-09 11:41 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King
In-Reply-To: <c5e627eb3fef0df162da56723093a03bfd2321fb.1699526999.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index e54492f8271..e4d31cbbd6a 100755
> --- a/GIT-VERSION-GEN
> +++ b/GIT-VERSION-GEN
> @@ -11,7 +11,7 @@ LF='
> if test -f version
> then
> VN=$(cat version) || VN="$DEF_VER"
> -elif test -d ${GIT_DIR:-.git} -o -f .git &&
> +elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&
I do not think this is strictly necessary.
Because the command line parser of "test" comes from left, notices
"-d" and takes the next one to check if it is a directory. There is
no value in ${GIT_DIR} can make "test -d ${GIT_DIR} -o ..." fail the
same way as the problem Peff pointed out during the discussion.
I do not need a subshell for grouping, either. Plain {} should do
(but you may need a LF or semicolon after the statement)..
> diff --git a/configure.ac b/configure.ac
> index 276593cd9dd..d1a96da14eb 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -94,7 +94,7 @@ AC_DEFUN([GIT_PARSE_WITH_SET_MAKE_VAR],
> [AC_ARG_WITH([$1],
> [AS_HELP_STRING([--with-$1=VALUE], $3)],
> if test -n "$withval"; then
> - if test "$withval" = "yes" -o "$withval" = "no"; then
> + if test "$withval" = "yes" || test "$withval" = "no"; then
This is correct and is in line with the way generated ./configure
protects "if $withval is yes or no, then do this" against a funny
value like "-f" in "$withval" breaking the parsing.
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index e0c5d3b0de6..43b5fec7320 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -489,13 +489,13 @@ find_existing_splits () {
> ;;
> END)
> debug "Main is: '$main'"
> - if test -z "$main" -a -n "$sub"
> + if test -z "$main" && test -n "$sub"
This should be OK as-is, for the same reason "-d" operator would not
be broken by arbitrary operand..
> then
> # squash commits refer to a subtree
> debug " Squash: $sq from $sub"
> cache_set "$sq" "$sub"
> fi
> - if test -n "$main" -a -n "$sub"
> + if test -n "$main" && test -n "$sub"
Ditto.
> then
> debug " Prior: $main -> $sub"
> cache_set $main $sub
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index e7786775a90..b952e5024b4 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -31,7 +31,7 @@ unset GIT_CONFIG_NOSYSTEM
> GIT_CONFIG_SYSTEM="$TEST_DIRECTORY/perf/config"
> export GIT_CONFIG_SYSTEM
>
> -if test -n "$GIT_TEST_INSTALLED" -a -z "$PERF_SET_GIT_TEST_INSTALLED"
> +if test -n "$GIT_TEST_INSTALLED" && test -z "$PERF_SET_GIT_TEST_INSTALLED"
Ditto.
> then
> error "Do not use GIT_TEST_INSTALLED with the perf tests.
>
> diff --git a/t/perf/run b/t/perf/run
> index 34115edec35..486ead21980 100755
> --- a/t/perf/run
> +++ b/t/perf/run
> @@ -91,10 +91,10 @@ set_git_test_installed () {
> run_dirs_helper () {
> mydir=${1%/}
> shift
> - while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
> + while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
As "$1" could be anything (including an insanity like "-n"), this
change is prudent.
> shift
> done
> - if test $# -gt 0 -a "$1" = --; then
> + if test $# -gt 0 && test "$1" = --; then
Ditto.
> shift
> fi
>
> @@ -124,7 +124,7 @@ run_dirs_helper () {
> }
>
> run_dirs () {
> - while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
> + while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
> run_dirs_helper "$@"
> shift
> done
> @@ -180,7 +180,8 @@ run_subsection () {
> GIT_PERF_AGGREGATING_LATER=t
> export GIT_PERF_AGGREGATING_LATER
>
> - if test $# = 0 -o "$1" = -- -o -f "$1"; then
> + if test $# = 0 || test "$1" = -- || test -f "$1"
> + then
Ditto.
> set -- . "$@"
> fi
>
> diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
> index 669ebaf68be..9fbf90cee7c 100755
> --- a/t/valgrind/valgrind.sh
> +++ b/t/valgrind/valgrind.sh
> @@ -23,7 +23,7 @@ memcheck)
> VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
> VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
> test 3 -gt "$VALGRIND_MAJOR" ||
> - test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
> + ( test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR" ) ||
This should be OK as-is; once "3 --eq" is parsed the parameter
reference would not be taken as anything but just a value.
> TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
> ;;
> *)
^ permalink raw reply
* [PATCH] format-patch: fix ignored encode_email_headers for cover letter
From: Simon Ser @ 2023-11-09 11:19 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
When writing the cover letter, the encode_email_headers option was
ignored. That is, UTF-8 subject lines and email addresses were
written out as-is, without any Q-encoding, even if
--encode-email-headers was passed on the command line.
This is due to encode_email_headers not being copied over from
struct rev_info to struct pretty_print_context. Fix that and add
a test.
Signed-off-by: Simon Ser <contact@emersion.fr>
---
builtin/log.c | 1 +
t/t4014-format-patch.sh | 10 ++++++++++
2 files changed, 11 insertions(+)
diff --git a/builtin/log.c b/builtin/log.c
index ba775d7b5cf8..87fd1c8560de 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1364,6 +1364,7 @@ static void make_cover_letter(struct rev_info *rev, int use_separate_file,
pp.date_mode.type = DATE_RFC2822;
pp.rev = rev;
pp.print_email_subject = 1;
+ pp.encode_email_headers = rev->encode_email_headers;
pp_user_info(&pp, NULL, &sb, committer, encoding);
prepare_cover_text(&pp, description_file, branch_name, &sb,
encoding, need_8bit_cte);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 5ced27ed4571..e37a1411ee24 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1906,6 +1906,16 @@ body" &&
grep "^body$" actual
'
+test_expect_success 'cover letter with --cover-from-description subject (UTF-8 subject line)' '
+ test_config branch.rebuild-1.description "Café?
+
+body" &&
+ git checkout rebuild-1 &&
+ git format-patch --stdout --cover-letter --cover-from-description subject --encode-email-headers main >actual &&
+ grep "^Subject: \[PATCH 0/2\] =?UTF-8?q?Caf=C3=A9=3F?=$" actual &&
+ ! grep "Café" actual
+'
+
test_expect_success 'cover letter with format.coverFromDescription = auto (short subject line)' '
test_config branch.rebuild-1.description "config subject
--
2.42.0
^ permalink raw reply related
* [PATCH 4/4] Makefile: stop using `test -o` when unlinking duplicate executables
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699526999.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]
When building executables we may end up with both `foo` and `foo.exe` in
the project's root directory. This can cause issues on Cygwin, which is
why we unlink the `foo` binary (see 6fc301bbf68 (Makefile: remove $foo
when $foo.exe is built/installed., 2007-01-10)). This step is skipped if
either:
- `foo` is a directory, which can happen when building Git on
Windows via MSVC (see ade2ca0ca9f (Do not try to remove
directories when removing old links, 2009-10-27)).
- `foo` is a hardlink to `foo.exe`, which can happen on Cygwin (see
0d768f7c8f1 (Makefile: building git in cygwin 1.7.0, 2008-08-15)).
These two conditions are currently chained together via `test -o`, which
is discouraged by our code style guide. Convert the recipe to instead
use an `if` statement with `&&`'d conditions, which both matches our
style guide and is easier to ready.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Makefile b/Makefile
index 03adcb5a480..1094a557711 100644
--- a/Makefile
+++ b/Makefile
@@ -2342,7 +2342,7 @@ profile-fast: profile-clean
all:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
ifneq (,$X)
- $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), test -d '$p' -o '$p' -ef '$p$X' || $(RM) '$p';)
+ $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_COMMANDS_TO_INSTALL) $(OTHER_PROGRAMS))), if test ! -d '$p' && test ! '$p' -ef '$p$X'; then $(RM) '$p'; fi;)
endif
all::
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 3/4] contrib/subtree: convert subtree type check to use case statement
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699526999.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 1227 bytes --]
The `subtree_for_commit ()` helper function asserts that the subtree
identified by its parameters are either a commit or tree. This is done
via the `-o` parameter of test, which is discouraged.
Refactor the code to instead use a switch statement over the type.
Despite being aligned with our coding guidelines, the resulting code is
arguably also easier to read.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/subtree/git-subtree.sh | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 8af0a81ba3f..3028029ac2d 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -641,10 +641,16 @@ subtree_for_commit () {
while read mode type tree name
do
assert test "$name" = "$dir"
- assert test "$type" = "tree" -o "$type" = "commit"
- test "$type" = "commit" && continue # ignore submodules
- echo $tree
- break
+
+ case "$type" in
+ commit)
+ continue;; # ignore submodules
+ tree)
+ echo $tree
+ break;;
+ *)
+ die "fatal: tree entry is of type ${type}, expected tree or commit";;
+ esac
done || exit $?
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 2/4] contrib/subtree: stop using `-o` to test for number of args
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699526999.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]
Functions in git-subtree.sh all assert that they are being passed the
correct number of arguments. In cases where we accept a variable number
of arguments we assert this via a single call to `test` with `-o`, which
is discouraged by our coding guidelines.
Convert these cases to stop doing so.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
contrib/subtree/git-subtree.sh | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 43b5fec7320..8af0a81ba3f 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -373,7 +373,8 @@ try_remove_previous () {
# Usage: process_subtree_split_trailer SPLIT_HASH MAIN_HASH [REPOSITORY]
process_subtree_split_trailer () {
- assert test $# = 2 -o $# = 3
+ assert test $# -ge 2
+ assert test $# -le 3
b="$1"
sq="$2"
repository=""
@@ -402,7 +403,8 @@ process_subtree_split_trailer () {
# Usage: find_latest_squash DIR [REPOSITORY]
find_latest_squash () {
- assert test $# = 1 -o $# = 2
+ assert test $# -ge 1
+ assert test $# -le 2
dir="$1"
repository=""
if test "$#" = 2
@@ -455,7 +457,8 @@ find_latest_squash () {
# Usage: find_existing_splits DIR REV [REPOSITORY]
find_existing_splits () {
- assert test $# = 2 -o $# = 3
+ assert test $# -ge 2
+ assert test $# -le 3
debug "Looking for prior splits..."
local indent=$(($indent + 1))
@@ -916,7 +919,7 @@ cmd_split () {
if test $# -eq 0
then
rev=$(git rev-parse HEAD)
- elif test $# -eq 1 -o $# -eq 2
+ elif test $# -eq 1 || test $# -eq 2
then
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
@@ -1006,8 +1009,11 @@ cmd_split () {
# Usage: cmd_merge REV [REPOSITORY]
cmd_merge () {
- test $# -eq 1 -o $# -eq 2 ||
+ if test $# -lt 1 || test $# -gt 2
+ then
die "fatal: you must provide exactly one revision, and optionally a repository. Got: '$*'"
+ fi
+
rev=$(git rev-parse -q --verify "$1^{commit}") ||
die "fatal: '$1' does not refer to a commit"
repository=""
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 1/4] global: convert trivial usages of `test <expr> -a/-o <expr>`
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
In-Reply-To: <cover.1699526999.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 4188 bytes --]
Our coding guidelines say to not use `test` with `-a` and `-o` because
it can easily lead to bugs. Convert trivial cases where we still use
these to instead instead concatenate multiple invocations of `test` via
`&&` and `||`, respectively.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
GIT-VERSION-GEN | 2 +-
configure.ac | 2 +-
contrib/subtree/git-subtree.sh | 4 ++--
t/perf/perf-lib.sh | 2 +-
t/perf/run | 9 +++++----
t/valgrind/valgrind.sh | 2 +-
6 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index e54492f8271..e4d31cbbd6a 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -11,7 +11,7 @@ LF='
if test -f version
then
VN=$(cat version) || VN="$DEF_VER"
-elif test -d ${GIT_DIR:-.git} -o -f .git &&
+elif ( test -d ${GIT_DIR:-.git} || test -f .git ) &&
VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
case "$VN" in
*$LF*) (exit 1) ;;
diff --git a/configure.ac b/configure.ac
index 276593cd9dd..d1a96da14eb 100644
--- a/configure.ac
+++ b/configure.ac
@@ -94,7 +94,7 @@ AC_DEFUN([GIT_PARSE_WITH_SET_MAKE_VAR],
[AC_ARG_WITH([$1],
[AS_HELP_STRING([--with-$1=VALUE], $3)],
if test -n "$withval"; then
- if test "$withval" = "yes" -o "$withval" = "no"; then
+ if test "$withval" = "yes" || test "$withval" = "no"; then
AC_MSG_WARN([You likely do not want either 'yes' or 'no' as]
[a value for $1 ($2). Maybe you do...?])
fi
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index e0c5d3b0de6..43b5fec7320 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -489,13 +489,13 @@ find_existing_splits () {
;;
END)
debug "Main is: '$main'"
- if test -z "$main" -a -n "$sub"
+ if test -z "$main" && test -n "$sub"
then
# squash commits refer to a subtree
debug " Squash: $sq from $sub"
cache_set "$sq" "$sub"
fi
- if test -n "$main" -a -n "$sub"
+ if test -n "$main" && test -n "$sub"
then
debug " Prior: $main -> $sub"
cache_set $main $sub
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index e7786775a90..b952e5024b4 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -31,7 +31,7 @@ unset GIT_CONFIG_NOSYSTEM
GIT_CONFIG_SYSTEM="$TEST_DIRECTORY/perf/config"
export GIT_CONFIG_SYSTEM
-if test -n "$GIT_TEST_INSTALLED" -a -z "$PERF_SET_GIT_TEST_INSTALLED"
+if test -n "$GIT_TEST_INSTALLED" && test -z "$PERF_SET_GIT_TEST_INSTALLED"
then
error "Do not use GIT_TEST_INSTALLED with the perf tests.
diff --git a/t/perf/run b/t/perf/run
index 34115edec35..486ead21980 100755
--- a/t/perf/run
+++ b/t/perf/run
@@ -91,10 +91,10 @@ set_git_test_installed () {
run_dirs_helper () {
mydir=${1%/}
shift
- while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
+ while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
shift
done
- if test $# -gt 0 -a "$1" = --; then
+ if test $# -gt 0 && test "$1" = --; then
shift
fi
@@ -124,7 +124,7 @@ run_dirs_helper () {
}
run_dirs () {
- while test $# -gt 0 -a "$1" != -- -a ! -f "$1"; do
+ while test $# -gt 0 && test "$1" != -- && test ! -f "$1"; do
run_dirs_helper "$@"
shift
done
@@ -180,7 +180,8 @@ run_subsection () {
GIT_PERF_AGGREGATING_LATER=t
export GIT_PERF_AGGREGATING_LATER
- if test $# = 0 -o "$1" = -- -o -f "$1"; then
+ if test $# = 0 || test "$1" = -- || test -f "$1"
+ then
set -- . "$@"
fi
diff --git a/t/valgrind/valgrind.sh b/t/valgrind/valgrind.sh
index 669ebaf68be..9fbf90cee7c 100755
--- a/t/valgrind/valgrind.sh
+++ b/t/valgrind/valgrind.sh
@@ -23,7 +23,7 @@ memcheck)
VALGRIND_MAJOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*\([0-9]*\)')
VALGRIND_MINOR=$(expr "$VALGRIND_VERSION" : '[^0-9]*[0-9]*\.\([0-9]*\)')
test 3 -gt "$VALGRIND_MAJOR" ||
- test 3 -eq "$VALGRIND_MAJOR" -a 4 -gt "$VALGRIND_MINOR" ||
+ ( test 3 -eq "$VALGRIND_MAJOR" && test 4 -gt "$VALGRIND_MINOR" ) ||
TOOL_OPTIONS="$TOOL_OPTIONS --track-origins=yes"
;;
*)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH 0/4] Replace use of `test <expr> -o/a <expr>`
From: Patrick Steinhardt @ 2023-11-09 10:53 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano
[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]
Hi,
this patch series replaces all uses of `test <expr> -o/a <expr>` that I
was able to find in our codebase. This is in accordance to our coding
guidelines:
- We do not write our "test" command with "-a" and "-o" and use "&&"
or "||" to concatenate multiple "test" commands instead, because
the use of "-a/-o" is often error-prone. E.g.
test -n "$x" -a "$a" = "$b"
is buggy and breaks when $x is "=", but
test -n "$x" && test "$a" = "$b"
does not have such a problem.
This patch series is a result of the discussion at [1].
Patrick
[1]: <20231109073250.GA2698227@coredump.intra.peff.net>
Patrick Steinhardt (4):
global: convert trivial usages of `test <expr> -a/-o <expr>`
contrib/subtree: stop using `-o` to test for number of args
contrib/subtree: convert subtree type check to use case statement
Makefile: stop using `test -o` when unlinking duplicate executables
GIT-VERSION-GEN | 2 +-
Makefile | 2 +-
configure.ac | 2 +-
contrib/subtree/git-subtree.sh | 34 +++++++++++++++++++++++-----------
t/perf/perf-lib.sh | 2 +-
t/perf/run | 9 +++++----
t/valgrind/valgrind.sh | 2 +-
7 files changed, 33 insertions(+), 20 deletions(-)
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v6 0/8] ci: add GitLab CI definition
From: Junio C Hamano @ 2023-11-09 10:06 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Taylor Blau, Phillip Wood, Oswald Buddenhagen, Victoria Dye,
Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> There's only a single change compared to v5 based on Chris' feedback,
> namely to move around a `return 1`. The newly extracted helper function
> `create_failed_test_artifacts()` indeed wasn't the correct place to put
> this error code.
The caller of the helper always signals failure by returning with 1
after calling this helper, and if we later add new callers, it is
very plausible that the new callers also are calling the helper
because they have noticed that the tests failed.
If the helper were not named "create_failed_test_artifacts" but were
given a more appropriate name, say, "fail_with_test_artifacts", we
wouldn't have said that the helper is not the right place to return
with a failure status. So, quite honestly, the difference since the
previous round is strictly a Meh to me.
But let's queue this version, because it is not making it worse ;-)
Thanks.
^ permalink raw reply
* [PATCH v6 8/8] ci: add support for GitLab CI
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 6731 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 | 45 ++++++++++++++++++++++++++
ci/print-test-failures.sh | 6 ++++
4 files changed, 116 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 04997102308..643e75d0577 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,35 @@ 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
+ return 1
+ }
+
+ 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 v6 7/8] ci: install test dependencies for linux-musl
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.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 5fe3c8ab69d..dbc99775934 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 v6 6/8] ci: squelch warnings when testing with unusable Git repo
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.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 8357ad77e4f..04997102308 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 v6 5/8] ci: unify setup of some environment variables
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.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 6fe3c08be83..8357ad77e4f 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -174,11 +174,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 v6 4/8] ci: split out logic to set up failed test artifacts
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2341 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 | 38 +++++++++++++++++++++-----------------
1 file changed, 21 insertions(+), 17 deletions(-)
diff --git a/ci/lib.sh b/ci/lib.sh
index 2ee5abeb02d..6fe3c08be83 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -131,6 +131,26 @@ 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
+}
+
# GitHub Action doesn't set TERM, which is required by tput
export TERM=${TERM:-dumb}
@@ -171,24 +191,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
+ create_failed_test_artifacts
return 1
}
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* [PATCH v6 3/8] ci: group installation of Docker dependencies
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.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 v6 2/8] ci: make grouping setup more generic
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.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 029819673b4..2ee5abeb02d 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.
#
@@ -285,5 +283,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 v6 1/8] ci: reorder definitions for grouping functions
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1699514143.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 bc0b23099df..029819673b4 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 v6 0/8] ci: add GitLab CI definition
From: Patrick Steinhardt @ 2023-11-09 8:05 UTC (permalink / raw)
To: git
Cc: Taylor Blau, Junio C Hamano, Phillip Wood, Oswald Buddenhagen,
Victoria Dye, Christian Couder
In-Reply-To: <cover.1698305961.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]
Hi,
this is the 6th version of my patch series to introduce support for
GitLab CI into the Git project.
There's only a single change compared to v5 based on Chris' feedback,
namely to move around a `return 1`. The newly extracted helper function
`create_failed_test_artifacts()` indeed wasn't the correct place to put
this error code.
A test run of this pipeline can be found at [1].
Thanks!
Patrick
[1]: https://gitlab.com/gitlab-org/git/-/pipelines/1066250852
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 | 190 ++++++++++++++++++++++--------
ci/print-test-failures.sh | 6 +
t/lib-httpd.sh | 17 ++-
5 files changed, 234 insertions(+), 55 deletions(-)
create mode 100644 .gitlab-ci.yml
Range-diff against v5:
1: 0ba396f2a33 = 1: a1413b76422 ci: reorder definitions for grouping functions
2: 821cfcd6125 = 2: 29039d7aa3a ci: make grouping setup more generic
3: 6e5bcf143c8 = 3: 414655ffb2d ci: group installation of Docker dependencies
4: 2182acf5bfc ! 4: 96d710faec8 ci: split out logic to set up failed test artifacts
@@ ci/lib.sh: handle_failed_tests () {
+ 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
@@ ci/lib.sh: then
- 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
+ return 1
}
- cache_dir="$HOME/none"
5: 6078aea246d = 5: 486d4bbf8b0 ci: unify setup of some environment variables
6: d69bde92f2f = 6: 534b14f0262 ci: squelch warnings when testing with unusable Git repo
7: b911c005bae = 7: a060613f039 ci: install test dependencies for linux-musl
8: 5784d03a6f1 ! 8: c05ff28cc2c ci: add support for GitLab CI
@@ ci/lib.sh: then
+ DONT_SKIP_TAGS=t
+ handle_failed_tests () {
+ create_failed_test_artifacts
++ return 1
+ }
+
+ cache_dir="$HOME/none"
base-commit: dadef801b365989099a9929e995589e455c51fed
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v3 1/3] t/lib-httpd: dynamically detect httpd and modules path
From: Patrick Steinhardt @ 2023-11-09 7:57 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqzfzn1i9u.fsf@gitster.g>
[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]
On Thu, Nov 09, 2023 at 04:46:05PM +0900, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > Yeah, I was grepping for it in our codebase and saw other occurrences,
> > so I assumed it was fair game. If we're going to convert it to the
> > below, how about I send another patch on top that also converts the
> > preexisting instances so that the next one grepping for it isn't going
> > to repeat the same mistake?
>
> Yup, an independent clean-up would be fine. Now we need to find a
> way to give better visibility to CodingGuidelines, which already
> says this:
Okay, I'll send one in. Do you want me to send a v4 of this patch series
or will you squash in below changes into patch 1/3?
> - We do not write our "test" command with "-a" and "-o" and use "&&"
> or "||" to concatenate multiple "test" commands instead, because
> the use of "-a/-o" is often error-prone. E.g.
>
> test -n "$x" -a "$a" = "$b"
>
> is buggy and breaks when $x is "=", but
>
> test -n "$x" && test "$a" = "$b"
>
> does not have such a problem.
I did indeed spot this part of our coding style now. I didn't bother to
look farther when I found other examples where we used `-a` and `-o`,
but that issue will be gone once we've dropped all of these usages.
Patrick
-- >8 --
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 6ab8f273a3..0a74922d7f 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -60,7 +60,7 @@ for DEFAULT_HTTPD_PATH in '/usr/sbin/httpd' \
"$(command -v httpd)" \
"$(command -v apache2)"
do
- if test -n "$DEFAULT_HTTPD_PATH" -a -x "$DEFAULT_HTTPD_PATH"
+ if test -n "$DEFAULT_HTTPD_PATH" && test -x "$DEFAULT_HTTPD_PATH"
then
break
fi
@@ -78,7 +78,7 @@ for DEFAULT_HTTPD_MODULE_PATH in '/usr/libexec/apache2' \
'/usr/libexec/httpd' \
"${DETECTED_HTTPD_ROOT:+${DETECTED_HTTPD_ROOT}/modules}"
do
- if test -n "$DEFAULT_HTTPD_MODULE_PATH" -a -d "$DEFAULT_HTTPD_MODULE_PATH"
+ if test -n "$DEFAULT_HTTPD_MODULE_PATH" && test -d "$DEFAULT_HTTPD_MODULE_PATH"
then
break
fi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related
* Re: What's cooking in git.git (Nov 2023, #04; Thu, 9)
From: Jeff King @ 2023-11-09 7:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Taylor Blau
In-Reply-To: <xmqq34xg5ek3.fsf@gitster.g>
On Thu, Nov 09, 2023 at 02:40:28AM +0900, Junio C Hamano wrote:
> * tb/pair-chunk-expect-size (2023-10-14) 8 commits
> - midx: read `OOFF` chunk with `pair_chunk_expect()`
> - midx: read `OIDL` chunk with `pair_chunk_expect()`
> - midx: read `OIDF` chunk with `pair_chunk_expect()`
> - commit-graph: read `BIDX` chunk with `pair_chunk_expect()`
> - commit-graph: read `GDAT` chunk with `pair_chunk_expect()`
> - commit-graph: read `CDAT` chunk with `pair_chunk_expect()`
> - commit-graph: read `OIDF` chunk with `pair_chunk_expect()`
> - chunk-format: introduce `pair_chunk_expect()` helper
>
> Code clean-up for jk/chunk-bounds topic.
>
> Comments?
> source: <45cac29403e63483951f7766c6da3c022c68d9f0.1697225110.git.me@ttaylorr.com>
> source: <cover.1697225110.git.me@ttaylorr.com>
Sorry it took me a while to circle back to this topic. I posted a
competing series just now in:
https://lore.kernel.org/git/20231109070310.GA2697602@coredump.intra.peff.net/
that I think should take precedence (and would require some reworking of
Taylor's patches, so you'd just eject them in the meantime).
-Peff
^ 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