* Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1
From: Junio C Hamano @ 2018-12-10 6:42 UTC (permalink / raw)
To: brian m. carlson
Cc: Ævar Arnfjörð Bjarmason,
Carlo Marcelo Arenas Belón, git, pcre-dev
In-Reply-To: <20181210004252.GK890086@genre.crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> Considering that some Linux users use PaX kernels with standard
> distributions and that most BSD kernels can be custom-compiled with a
> variety of options enabled or disabled, I think this is something we
> should detect dynamically.
> ...
> My view is that JIT is a nice performance optimization, but it's
> optional. I honestly don't think it should even be exposed through the
> API: if it works, then things are faster, and if it doesn't, then
> they're not. I don't see the value in an option for causing things to be
> broken if someone improves the security of the system.
I agree to both of these two points. Thanks for a thoughtful
discussion, all.
^ permalink raw reply
* Re: [PATCH] parse-options: fix SunCC compiler warning
From: Junio C Hamano @ 2018-12-10 6:36 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: avarab, git, sunshine, szeder.dev
In-Reply-To: <20181209102724.8707-1-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> The compiler reports this because show_gitcomp() never actually
> returns a value:
>
> "parse-options.c", line 520: warning: Function has no return
> statement : show_gitcomp
>
> The function calls exit() and will never return. Update and mark it
> NORETURN.
Yuck. This should do for now, but I am not impressed by the choice
to hook show_gitcomp() call into parse_options_step(), which forces
such an abnormal exit deeper in the callchain [*1*]. For readers
(not compilers), it would help to have a comment at the caller that
says that show_gitcomp() never returns and exits.
side note #1. Perhaps parse_options_start() would have been
a better place, instead of parse_options_step() that is
repeatedly called in a loop and itself has a loop. ANd
worse yet, the check is done inside the loop even though the
call is to be made only when the --git-completion-helper
option is given as the sole request.
Thanks.
> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> parse-options.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 3b874a83a0..6577e52f63 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -474,8 +474,8 @@ static void show_negated_gitcomp(const struct option *opts, int nr_noopts)
> }
> }
>
> -static int show_gitcomp(struct parse_opt_ctx_t *ctx,
> - const struct option *opts)
> +static void NORETURN show_gitcomp(struct parse_opt_ctx_t *ctx,
> + const struct option *opts)
> {
> const struct option *original_opts = opts;
> int nr_noopts = 0;
> @@ -550,7 +550,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>
> /* lone --git-completion-helper is asked by git-completion.bash */
> if (ctx->total == 1 && !strcmp(arg + 1, "-git-completion-helper"))
> - return show_gitcomp(ctx, options);
> + show_gitcomp(ctx, options);
>
> if (arg[1] != '-') {
> ctx->opt = arg + 1;
^ permalink raw reply
* Fwd:Payment Swift $128.905.05
From: Stephanie Nicholl @ 2018-12-10 6:06 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 594 bytes --]
Good day!
Kindly see attached T/T payment copy for your reference and kind
confirmation.Details of paid Invoice are as follows;
1)inv no.AMB -129st =$21,742.41
2)inv no.AMB -130st =$26,607.14
3)inv no.AMB-131st =$27,614.74
4)inv no. AMB-132st =$33,782.26
5)inv no.AMB-133st =$19,158.50
=============
Total amt us$128,905.05
=============
Please confirm receipt of payment by a return mail.
Regards,
Chuck Coote
Accounting Services
Crate Designs Ltd.
chuck@crate.ca
P.O Box 460
87 7th St. S. W.Chesley, Ontario N0G 1L0
Tel: 519 363 3221 Fax 519 363 2509
[-- Attachment #2: Scan Copy.gz --]
[-- Type: application/octet-stream, Size: 415967 bytes --]
^ permalink raw reply
* Re: [PATCH v3 2/3] commit-graph: fix buffer read-overflow
From: SZEDER Gábor @ 2018-12-10 4:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Josh Steadmon, git, stolee, avarab, peff
In-Reply-To: <xmqqsgz74acm.fsf@gitster-ct.c.googlers.com>
On Sun, Dec 09, 2018 at 01:01:29PM +0900, Junio C Hamano wrote:
> Josh Steadmon <steadmon@google.com> writes:
>
> > diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> > index 5fe21db99f..5b6b44b78e 100755
> > --- a/t/t5318-commit-graph.sh
> > +++ b/t/t5318-commit-graph.sh
> > @@ -366,24 +366,30 @@ GRAPH_OCTOPUS_DATA_OFFSET=$(($GRAPH_COMMIT_DATA_OFFSET + \
> > GRAPH_BYTE_OCTOPUS=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4))
> > GRAPH_BYTE_FOOTER=$(($GRAPH_OCTOPUS_DATA_OFFSET + 4 * $NUM_OCTOPUS_EDGES))
> >
> > -# usage: corrupt_graph_and_verify <position> <data> <string>
> > +# usage: corrupt_graph_and_verify <position> <data> <string> [<zero_pos>]
> > # Manipulates the commit-graph file at the position
> > -# by inserting the data, then runs 'git commit-graph verify'
> > +# by inserting the data, optionally zeroing the file
> > +# starting at <zero_pos>, then runs 'git commit-graph verify'
> > # and places the output in the file 'err'. Test 'err' for
> > # the given string.
> > corrupt_graph_and_verify() {
> > pos=$1
> > data="${2:-\0}"
> > grepstr=$3
> > + orig_size=$(stat --format=%s $objdir/info/commit-graph)
>
> "stat(1)" is not so portable, so you'll get complaints from minority
> platform users later. So is "truncate(1)".
I complain: this patch breaks on macOS (on Travis CI), but in a
curious way. First, 'stat' in the above line errors out with:
+++stat --format=%s .git/objects/info/commit-graph
stat: illegal option -- -
usage: stat [-FlLnqrsx] [-f format] [-t timefmt] [file ...]
Alas, this doesn't immediately fail the test, because it's not part of
the &&-chain.
> > + zero_pos=${4:-${orig_size}}
No && here, either.
> > cd "$TRASH_DIRECTORY/full" &&
> > test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> > cp $objdir/info/commit-graph commit-graph-backup &&
> > printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" conv=notrunc &&
> > + truncate --size=$zero_pos $objdir/info/commit-graph &&
++truncate --size= .git/objects/info/commit-graph
t5318-commit-graph.sh: line 385: truncate: command not found
Note that even if 'truncate' were available, it would most likely
complain about the empty '--size=' argument resulting from the 'stat'
error above.
Alas, this doesn't fail the test, either, because ...
> > + truncate --size=$orig_size $objdir/info/commit-graph &&
> > test_must_fail git commit-graph verify 2>test_err &&
> > grep -v "^+" test_err >err
... here the &&-chain was broken already before this patch. However,
since this above command was not executed due to the missing
'truncate', it didn't have a chance to create the 'err' file, ...
> > test_i18ngrep "$grepstr" err
... so 'test_i18ngrep' can't find the file, which triggers its linting
error, finally aborting the whole test script.
> > }
> >
> > +
Stray newline.
> > test_expect_success 'detect bad signature' '
> > corrupt_graph_and_verify 0 "\0" \
> > "graph signature"
> > @@ -484,6 +490,11 @@ test_expect_success 'detect invalid checksum hash' '
> > "incorrect checksum"
> > '
> >
> > +test_expect_success 'detect incorrect chunk count' '
> > + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\xff" \
>
> Implementations of printf(1) may not grok "\xff" as a valid
> representation of "\377". The shell built-in of dash(1) for example
> would not work with this.
>
> > + "chunk lookup table entry missing" $GRAPH_CHUNK_LOOKUP_OFFSET
> > +'
> > +
> > test_expect_success 'git fsck (checks commit-graph)' '
> > cd "$TRASH_DIRECTORY/full" &&
> > git fsck &&
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2018, #01; Sun, 9)
From: Denton Liu @ 2018-12-10 3:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqo99u2hjy.fsf@gitster-ct.c.googlers.com>
On Mon, Dec 10, 2018 at 12:21:05PM +0900, Junio C Hamano wrote:
> I think v3 (which we see above) describes what it wants to do
> clearly enough and implements what it wants to do cleanly. I do not
> think the patch itself has much room for further improvement.
>
> When I re-read the final patch and all the earlier comments I made
> in the thread that began at [*1*], I still do not see in what
> practical workflow and usecase the users would find the feature this
> change adds useful.
>
> For each new feature, I want a story we can sell it to the users:
> "if your workflow is like this or that, you would find yourself
> wanting to do this, which was (impossible|cumbersome) to do before;
> with this new thing, you can".
>
> And the "story" is not "If you have remote.$name.url and want to
> move its value to remote.$name.pushurl while setting the former to a
> new value, then..." I want to know why the user gets in such a
> situation in the first place.
>
> To be helped by the feature, the user
>
> (1) must first have a remote.$name.url (but not remote.$name.pushurl)
>
> (2) that URL must also be usable for pushing
>
> (3) and then has another URL that can be used for fetching
>
> (4) and somehow that new URL is more suitable for fetching than the
> original one.
>
> When all of the above holds, then "set-url --save-to-push" can be
> used to move the original URL that can be used for both fetching and
> pushing to remote.$name.pushurl and set remote.$name.url to the new
> value with a single command. But is that a sensible situation to be
> in the first place?
The following is the story that led to me writing the feature in the
first place:
I have a project that's been developed on my own machine. I want to
deploy it onto a server to test, so I use SSH key forwarding and clone
the project with the SSH URL onto the server. After fixing some small
bugs, I'll push the changes directly from the server up.
Now that the server is running, I only touch the repo on the server
occasionally. I want to pull updates from the main repository without
necessarily having to use SSH key forwarding because I might be
forgetful and forget to start ssh-agent or I might have someone else
administer the server who doesn't have key-access to the repository.
However, I also don't want to get rid of my ability to push over SSH so,
in this case, I'd run
`git remote set-url --save-to-push origin <HTTPS_URL>`.
Although it's definitely not an every day activity, I've run into the
use case a few times where having this ability would definitely be
useful.
>
> I guess it would help the readers if the documentation (or proposed
> log message) were more explicit that this is to help the project
> originator more than the project followers, perhaps. My working
> assumption during the review discussion on this patch has been that
> there are orders-of-magnitude many project followers who start from
> fetching and locally tweaking without ever publishing than those who
> start to pushing to a project from day one of joining, or the day
> they themselves initiated the project. And for these majority
> "followers", the first URL is often the one to fetch, which may not
> necessarily be usable for pushing, and that URL is advertised for
> the wider general public as the most suitable URL for fetching the
> project's source. So to these people, neither (2) or (4) would
> hold.
>
> But for project initiators and those joining a project with write
> access from day one, the story may be a bit different. They may
> start with a single URL that can be used for both fetching and
> pushing, which means (2) would hold for them, unlike for the
> majority of users.
>
> I am still not sure what a good example situation is that makes (4)
> hold, though. Perhaps you originally had a R/W URL that always
> require authentication, but now you want to use an anonymous R/O URL
> for your fetch traffic without having to authenticate? If there is
> a model situation to make all of these four hold, perhaps it can be
> added somewhere to help users who would find the new feature useful
> discover it.
If the above makes sense to you, I can try to distill the use-case down
to its essence and include it documentation for the patch.
>
> Without that, I remain unconvinced.
>
> Thanks.
>
>
> [Reference]
>
> *1* https://public-inbox.org/git/1d1b0fe85ddd89cf8172e730e8886d5b4a9ea7eb.1540627720.git.liu.denton@gmail.com/
^ permalink raw reply
* Re: [PATCH 1/8] move worktree tests to t24*
From: Junio C Hamano @ 2018-12-10 3:48 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git, Nguyễn Thái Ngọc Duy, Elijah Newren
In-Reply-To: <20181209200449.16342-2-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> The 'git worktree' command used to be just another mode in 'git
> checkout', namely 'git checkout --to'. When the tests for the latter
> were retrofitted for the former, the test name was adjusted, but the
> test number was kept, even though the test is testing a different
> command now. t/README states: "Second digit tells the particular
> command we are testing.", so 'git worktree' should have a separate
> number just for itself.
That probably was written in the old world where there were only 10
commands in each category ;-) Nevertheless I have no problem with
this move (and I do not think there are in-flight topics in these
areas).
Thanks.
>
> Move the worktree tests to t24* to adhere to that guideline. We're
> going to make use of the free'd up numbers in a subsequent commit.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> ---
> t/{t2025-worktree-add.sh => t2400-worktree-add.sh} | 0
> t/{t2026-worktree-prune.sh => t2401-worktree-prune.sh} | 0
> t/{t2027-worktree-list.sh => t2402-worktree-list.sh} | 0
> 3 files changed, 0 insertions(+), 0 deletions(-)
> rename t/{t2025-worktree-add.sh => t2400-worktree-add.sh} (100%)
> rename t/{t2026-worktree-prune.sh => t2401-worktree-prune.sh} (100%)
> rename t/{t2027-worktree-list.sh => t2402-worktree-list.sh} (100%)
>
> diff --git a/t/t2025-worktree-add.sh b/t/t2400-worktree-add.sh
> similarity index 100%
> rename from t/t2025-worktree-add.sh
> rename to t/t2400-worktree-add.sh
> diff --git a/t/t2026-worktree-prune.sh b/t/t2401-worktree-prune.sh
> similarity index 100%
> rename from t/t2026-worktree-prune.sh
> rename to t/t2401-worktree-prune.sh
> diff --git a/t/t2027-worktree-list.sh b/t/t2402-worktree-list.sh
> similarity index 100%
> rename from t/t2027-worktree-list.sh
> rename to t/t2402-worktree-list.sh
^ permalink raw reply
* Re: [PATCH 0/8] introduce no-overlay and cached mode in git checkout
From: Junio C Hamano @ 2018-12-10 3:47 UTC (permalink / raw)
To: Thomas Gummerer; +Cc: git, Nguyễn Thái Ngọc Duy, Elijah Newren
In-Reply-To: <20181209200449.16342-1-t.gummerer@gmail.com>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> Basically the idea is to also delete files when the match <pathspec>
> in 'git checkout <tree-ish> -- <pathspec>' in the current tree, but
> don't match <pathspec> in <tree-ish>.
I cannot quite parse it, but perhaps.
"git checkout --no-overlay <tree-ish> -- <pathspec>" can
remove paths in the index and in the working tree that match
<pathspec>, if they do not appear in <tree-ish>.
If a new file D/F is in the index and in the working tree but not in
HEAD, "git checkout HEAD D/" or "git checkout HEAD D/F" would not
remove D/F from the index or the working tree.
With the --no-overlay option, it would, and that is often closer to
the wish of the user who wanted to say "restore the working tree
state of D/ (or D/F) from the state recorded in HEAD".
> The final step in the series is to actually make use of this in 'git
> stash', which simplifies the code there a bit. I am however happy to
> hold off on this step until the stash-in-C series is merged, so we
> don't delay that further.
I think that is probably a good idea, for now.
> In addition to the no-overlay mode, we also add a --cached mode, which
> works only on the index, thus similar to 'git reset <tree-ish> -- <pathspec>'.
>
> Actually deprecating 'git reset <tree-ish> -- <pathspec>' should come
> later,...
Or we may not even need to deprecate it. IIRC, what "stash" wished
to exist was "git reset --hard <tree-ish> -- <pathspec>", which, if
the command followed "--cached/--index" convention, would have been
called "git reset --index ...". Did we actually have the need for
"--cached" mode?
> probably not before Duy's restore-files command lands, as 'git
> checkout --no-overlay <tree-ish> -- <pathspec>' is a bit cumbersome to
> type compared to 'git reset <tree-ish> -- <pathspec>'.
Yes, between "checkout --cached" and "checkout --no-overlay", the
latter is much more important, as the latter is what a missing "git
reset --hard <tree-ish> -- <pathspec>" would have been, but the
former can be written with an existing command.
> My hope is also that the no-overlay mode could become the new default
> in the restore-files command Duy is currently working on.
Yup, that is my hope, too ;-).
^ permalink raw reply
* Re: [PATCH] rebase docs: drop stray word in merge command description
From: Junio C Hamano @ 2018-12-10 3:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Kyle Meyer, git
In-Reply-To: <nycvar.QRO.7.76.6.1812092040320.43@tvgsbejvaqbjf.bet>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Kyle,
>
> On Sat, 8 Dec 2018, Kyle Meyer wrote:
>
>> Delete a misplaced word introduced by caafecfcf1 (rebase
>> --rebase-merges: adjust man page for octopus support, 2018-03-09).
>>
>> Signed-off-by: Kyle Meyer <kyle@kyleam.com>
>
> ACK.
Thanks.
> Too bad this did not make it into v2.20.0, but at least it can make it
> into a future version.
The right way to fix it is to prepare a topic that can be merged
down to the 2.19.x track, and proceed normally to percolate it down
via 'next', 'master' and 'maint' as any other fixes. That is
already happening.
The original documentation bug is older than where the 2.20 track
forked; the bug is in 2.19. Any such old bugs, users have survived
without it being fixed for a cycle already, and the fix is not that
urgent to interrupt the release engineering that is already underway
and redo it.
A regression that appears only in -rc and a known bug in a new
feature that appears only in -rc are different matters. It is
prudent to always first access how serious they are and we must be
prepared to even delay the final as necessary. But I do not think
this one is.
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2018, #01; Sun, 9)
From: Junio C Hamano @ 2018-12-10 3:21 UTC (permalink / raw)
To: Denton Liu; +Cc: git
In-Reply-To: <20181209090300.GA32640@archbookpro.localdomain>
Denton Liu <liu.denton@gmail.com> writes:
> On Sun, Dec 09, 2018 at 05:42:27PM +0900, Junio C Hamano wrote:
>> * dl/merge-cleanup-scissors-fix (2018-11-21) 2 commits
>> (merged to 'next' on 2018-11-21 at 217be06acb)
>> + merge: add scissors line on merge conflict
>> + t7600: clean up 'merge --squash c3 with c7' test
>>
>> The list of conflicted paths shown in the editor while concluding a
>> conflicted merge was shown above the scissors line when the
>> clean-up mode is set to "scissors", even though it was commented
>> out just like the list of updated paths and other information to
>> help the user explain the merge better.
>>
>> Will cook in 'next'.
>
> From our discussion[1], expect a reroll with the ability to do scissors
> cleanup in messages generated by git-merge. Unfortunately, it'll be a
> couple of weeks because of finals season.
OK, I'd imagine that it would be cleaner to do so as a fresh series,
instead of incremental updates on top of these two, so let's plan to
eject what we have in 'next' when it happens. Thanks for a heads-up.
>> * dl/remote-save-to-push (2018-11-13) 1 commit
>> - remote: add --save-to-push option to git remote set-url
>>
>> "git remote set-url" learned a new option that moves existing value
>> of the URL field to pushURL field of the remote before replacing
>> the URL field with a new value.
>>
>> I am personally not yet quite convinced if this is worth pursuing.
>
> Any way I could convince you otherwise?
I think v3 (which we see above) describes what it wants to do
clearly enough and implements what it wants to do cleanly. I do not
think the patch itself has much room for further improvement.
When I re-read the final patch and all the earlier comments I made
in the thread that began at [*1*], I still do not see in what
practical workflow and usecase the users would find the feature this
change adds useful.
For each new feature, I want a story we can sell it to the users:
"if your workflow is like this or that, you would find yourself
wanting to do this, which was (impossible|cumbersome) to do before;
with this new thing, you can".
And the "story" is not "If you have remote.$name.url and want to
move its value to remote.$name.pushurl while setting the former to a
new value, then..." I want to know why the user gets in such a
situation in the first place.
To be helped by the feature, the user
(1) must first have a remote.$name.url (but not remote.$name.pushurl)
(2) that URL must also be usable for pushing
(3) and then has another URL that can be used for fetching
(4) and somehow that new URL is more suitable for fetching than the
original one.
When all of the above holds, then "set-url --save-to-push" can be
used to move the original URL that can be used for both fetching and
pushing to remote.$name.pushurl and set remote.$name.url to the new
value with a single command. But is that a sensible situation to be
in the first place?
I guess it would help the readers if the documentation (or proposed
log message) were more explicit that this is to help the project
originator more than the project followers, perhaps. My working
assumption during the review discussion on this patch has been that
there are orders-of-magnitude many project followers who start from
fetching and locally tweaking without ever publishing than those who
start to pushing to a project from day one of joining, or the day
they themselves initiated the project. And for these majority
"followers", the first URL is often the one to fetch, which may not
necessarily be usable for pushing, and that URL is advertised for
the wider general public as the most suitable URL for fetching the
project's source. So to these people, neither (2) or (4) would
hold.
But for project initiators and those joining a project with write
access from day one, the story may be a bit different. They may
start with a single URL that can be used for both fetching and
pushing, which means (2) would hold for them, unlike for the
majority of users.
I am still not sure what a good example situation is that makes (4)
hold, though. Perhaps you originally had a R/W URL that always
require authentication, but now you want to use an anonymous R/O URL
for your fetch traffic without having to authenticate? If there is
a model situation to make all of these four hold, perhaps it can be
added somewhere to help users who would find the new feature useful
discover it.
Without that, I remain unconvinced.
Thanks.
[Reference]
*1* https://public-inbox.org/git/1d1b0fe85ddd89cf8172e730e8886d5b4a9ea7eb.1540627720.git.liu.denton@gmail.com/
^ permalink raw reply
* [PATCH] fixup! test-lib: add the '--stress' option to run a test repeatedly under load
From: SZEDER Gábor @ 2018-12-10 1:34 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20181209225628.22216-8-szeder.dev@gmail.com>
---
Erm. 'trace=t' must be set before checking whether the shell
supports BASH_XTRACEFD.
t/test-lib.sh | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index e405191164..3e9916b39b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -446,6 +446,13 @@ then
test -z "$verbose_log" && verbose=t
fi
+if test -n "$stress"
+then
+ verbose=t
+ trace=t
+ immediate=t
+fi
+
if test -n "$trace" && test -n "$test_untraceable"
then
# '-x' tracing requested, but this test script can't be reliably
@@ -469,13 +476,6 @@ then
verbose=t
fi
-if test -n "$stress"
-then
- verbose=t
- trace=t
- immediate=t
-fi
-
if test -n "$color"
then
# Save the color control sequences now rather than run tput
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related
* Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1
From: Carlo Arenas @ 2018-12-10 1:25 UTC (permalink / raw)
To: sandals, avarab, git, pcre-dev
In-Reply-To: <20181210004252.GK890086@genre.crustytoothpaste.net>
On Sun, Dec 9, 2018 at 4:42 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On Mon, Dec 10, 2018 at 12:51:01AM +0100, Ævar Arnfjörð Bjarmason wrote:
> > Obviously this & what you have in 2/2 needs to be fixed in some way.
> >
> > Is the issue on SELinux, OpenBSD, NetBSD etc. *how* PCRE is creating the
> > the JIT'd code? I.e. presumably Google Chrome's JIT engine, Java JIT and
> > the like work on those setup, or not? I.e. is this something upstream
> > can/is likely to fix eventually?
>
> From the cover letter (but without testing), it seems like it would
> probably be fine to first map the pages read-write to write the code and
> then, once that's done, to map them read-executable. I know JIT
> compilation does work on the BSDs, so presumably that's the technique to
> make it do so.
and that has been implemented (sljitProtExecAllocator.c) as part of the work
triggered by the bug I linked about [1], deep inside sljit (which is
what pcre uses for JIT)
the code AS-IS wouldn't compile for the BSD[2] but that is easy to fix
and sure works as expected but I am under the impression that is not
something that can be considered as a solution as explained by the open issues
described with crashes after a fork()
note that changing the map from read-write to executable will be
prevented by the
same policy so you have to create 2 maps (and therefore a backing file) and I
don't think there is a way to solve that in a foolproof way in a
library which is why
I mentioned more work might be needed to define the right interfaces
so it can be
solved by the application, and that is unlikely to happen with the old library.
Carlo
[1] https://bugs.exim.org/show_bug.cgi?id=1749
[2] https://bugs.exim.org/show_bug.cgi?id=2155
^ permalink raw reply
* Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1
From: brian m. carlson @ 2018-12-10 0:42 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Carlo Marcelo Arenas Belón, git, pcre-dev
In-Reply-To: <87r2eqxnru.fsf@evledraar.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2142 bytes --]
On Mon, Dec 10, 2018 at 12:51:01AM +0100, Ævar Arnfjörð Bjarmason wrote:
> Obviously this & what you have in 2/2 needs to be fixed in some way.
>
> Is the issue on SELinux, OpenBSD, NetBSD etc. *how* PCRE is creating the
> the JIT'd code? I.e. presumably Google Chrome's JIT engine, Java JIT and
> the like work on those setup, or not? I.e. is this something upstream
> can/is likely to fix eventually?
From the cover letter (but without testing), it seems like it would
probably be fine to first map the pages read-write to write the code and
then, once that's done, to map them read-executable. I know JIT
compilation does work on the BSDs, so presumably that's the technique to
make it do so.
Both versions of PCRE map pages both write and executable at the same
time, which is presumably where things go wrong. I assume it can be
fixed, but whether that's easy in the context of PCRE, I wouldn't know.
> Are we mixing a condition where one some OS's or OS versions this just
> won't work at all, and thus maybe should be something turned on in
> config.mak.uname, v.s. e.g. SELinux where presumably it'll dynamically
> change.
Considering that some Linux users use PaX kernels with standard
distributions and that most BSD kernels can be custom-compiled with a
variety of options enabled or disabled, I think this is something we
should detect dynamically.
> I'm inclined to suggest that we should have another ifdef here for "if
> JIT fails I'd like it to die", so that e.g. packages I build (for
> internal use) don't silently slow down in the future, only for me to
> find some months later that someone enabled an overzealous SELinux
> policy and we swept this under the rug.
My view is that JIT is a nice performance optimization, but it's
optional. I honestly don't think it should even be exposed through the
API: if it works, then things are faster, and if it doesn't, then
they're not. I don't see the value in an option for causing things to be
broken if someone improves the security of the system.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1
From: Ævar Arnfjörð Bjarmason @ 2018-12-09 23:51 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón; +Cc: git, pcre-dev
In-Reply-To: <20181209230024.43444-2-carenas@gmail.com>
On Sun, Dec 09 2018, Carlo Marcelo Arenas Belón wrote:
[+CC pcre-dev]
> JIT support was added to 8.20 but the interface we rely on is only
> enabled after 8.32 so try to make the message clearer.
>
> in systems where there are restrictions against creating executable
> pages programatically (like OpenBSD, NetBSD, macOS or seLinux) JIT
> will fail, resulting in a error message to the user.
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
> Makefile | 12 ++++++------
> grep.c | 6 ++++++
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 1a44c811aa..62b0cb6ee6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -32,14 +32,14 @@ all::
> # USE_LIBPCRE is a synonym for USE_LIBPCRE2, define USE_LIBPCRE1
> # instead if you'd like to use the legacy version 1 of the PCRE
> # library. Support for version 1 will likely be removed in some future
> -# release of Git, as upstream has all but abandoned it.
> +# release of Git, as upstream is focusing all development for new
> +# features in the newer version instead.
I think whatever we do here it makes sense to split this into its own
patch, since it doesn't have to do with this fallback mechanism.
FWIW I was trying to word this in some way that very briefly described
the v1 v.s. v2 situation. Just saying "new features" doesn't quite
capture it, e.g. some bugs in v1 are closed with some resolution like
"this isn't trivial to fix, use v2 instead".
> # When using USE_LIBPCRE1, define NO_LIBPCRE1_JIT if the PCRE v1
> -# library is compiled without --enable-jit. We will auto-detect
> -# whether the version of the PCRE v1 library in use has JIT support at
> -# all, but we unfortunately can't auto-detect whether JIT support
> -# hasn't been compiled in in an otherwise JIT-supporting version. If
> -# you have link-time errors about a missing `pcre_jit_exec` define
> +# library is newer than 8.32 but compiled without --enable-jit or
> +# you want to disable JIT
> +#
> +# If you have link-time errors about a missing `pcre_jit_exec` define
> # this, or recompile PCRE v1 with --enable-jit.
> #
> # Define LIBPCREDIR=/foo/bar if your PCRE header and library files are
> diff --git a/grep.c b/grep.c
> index 4db1510d16..5ccc0421a1 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -405,6 +405,12 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
> die("%s", error);
>
> #ifdef GIT_PCRE1_USE_JIT
> + if (p->pcre1_extra_info &&
> + !(p->pcre1_extra_info->flags & PCRE_EXTRA_EXECUTABLE_JIT)) {
> + /* JIT failed so fallback to the interpreter */
> + p->pcre1_jit_on = 0;
> + return;
> + }
Obviously this & what you have in 2/2 needs to be fixed in some way.
Is the issue on SELinux, OpenBSD, NetBSD etc. *how* PCRE is creating the
the JIT'd code? I.e. presumably Google Chrome's JIT engine, Java JIT and
the like work on those setup, or not? I.e. is this something upstream
can/is likely to fix eventually?
Are there cases where we can JIT, but fail for some entirely unrelated
reason, and are now hiding the error?
Are we mixing a condition where one some OS's or OS versions this just
won't work at all, and thus maybe should be something turned on in
config.mak.uname, v.s. e.g. SELinux where presumably it'll dynamically
change.
I'm inclined to suggest that we should have another ifdef here for "if
JIT fails I'd like it to die", so that e.g. packages I build (for
internal use) don't silently slow down in the future, only for me to
find some months later that someone enabled an overzealous SELinux
policy and we swept this under the rug.
But maybe that's just dumb for some reason and we always need to do this
dynamically...
^ permalink raw reply
* [RFC PATCH 2/2] grep: fallback to interpreter if JIT fails with pcre2
From: Carlo Marcelo Arenas Belón @ 2018-12-09 23:00 UTC (permalink / raw)
To: git; +Cc: avarab
In-Reply-To: <20181209230024.43444-1-carenas@gmail.com>
starting with 10.23, and as a side effect of the work for bug1749[1] (grep
-P crash with seLinux), pcre2grep was modified to ignore any errors from
pcre2_jit_compile so the interpreter could be used as a fallback
[1] https://bugs.exim.org/show_bug.cgi?id=1749
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
grep.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/grep.c b/grep.c
index 5ccc0421a1..c751c8cc74 100644
--- a/grep.c
+++ b/grep.c
@@ -530,8 +530,11 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);
if (p->pcre2_jit_on == 1) {
jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
- if (jitret)
- die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n", p->pattern, jitret);
+ if (jitret) {
+ /* JIT failed so fallback to the interpreter */
+ p->pcre2_jit_on = 0;
+ return;
+ }
/*
* The pcre2_config(PCRE2_CONFIG_JIT, ...) call just
--
2.20.0
^ permalink raw reply related
* [RFC PATCH 1/2] grep: fallback to interpreter if JIT fails with pcre1
From: Carlo Marcelo Arenas Belón @ 2018-12-09 23:00 UTC (permalink / raw)
To: git; +Cc: avarab
In-Reply-To: <20181209230024.43444-1-carenas@gmail.com>
JIT support was added to 8.20 but the interface we rely on is only
enabled after 8.32 so try to make the message clearer.
in systems where there are restrictions against creating executable
pages programatically (like OpenBSD, NetBSD, macOS or seLinux) JIT
will fail, resulting in a error message to the user.
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Makefile | 12 ++++++------
grep.c | 6 ++++++
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/Makefile b/Makefile
index 1a44c811aa..62b0cb6ee6 100644
--- a/Makefile
+++ b/Makefile
@@ -32,14 +32,14 @@ all::
# USE_LIBPCRE is a synonym for USE_LIBPCRE2, define USE_LIBPCRE1
# instead if you'd like to use the legacy version 1 of the PCRE
# library. Support for version 1 will likely be removed in some future
-# release of Git, as upstream has all but abandoned it.
+# release of Git, as upstream is focusing all development for new
+# features in the newer version instead.
#
# When using USE_LIBPCRE1, define NO_LIBPCRE1_JIT if the PCRE v1
-# library is compiled without --enable-jit. We will auto-detect
-# whether the version of the PCRE v1 library in use has JIT support at
-# all, but we unfortunately can't auto-detect whether JIT support
-# hasn't been compiled in in an otherwise JIT-supporting version. If
-# you have link-time errors about a missing `pcre_jit_exec` define
+# library is newer than 8.32 but compiled without --enable-jit or
+# you want to disable JIT
+#
+# If you have link-time errors about a missing `pcre_jit_exec` define
# this, or recompile PCRE v1 with --enable-jit.
#
# Define LIBPCREDIR=/foo/bar if your PCRE header and library files are
diff --git a/grep.c b/grep.c
index 4db1510d16..5ccc0421a1 100644
--- a/grep.c
+++ b/grep.c
@@ -405,6 +405,12 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)
die("%s", error);
#ifdef GIT_PCRE1_USE_JIT
+ if (p->pcre1_extra_info &&
+ !(p->pcre1_extra_info->flags & PCRE_EXTRA_EXECUTABLE_JIT)) {
+ /* JIT failed so fallback to the interpreter */
+ p->pcre1_jit_on = 0;
+ return;
+ }
pcre_config(PCRE_CONFIG_JIT, &p->pcre1_jit_on);
if (p->pcre1_jit_on == 1) {
p->pcre1_jit_stack = pcre_jit_stack_alloc(1, 1024 * 1024);
--
2.20.0
^ permalink raw reply related
* [RFC PATCH 0/2] fallback to interpreter if JIT fails with pcre
From: Carlo Marcelo Arenas Belón @ 2018-12-09 23:00 UTC (permalink / raw)
To: git; +Cc: avarab
while testing in NetBSD 8, was surprised to find that most test cases
using PCRE2 where failing with some cryptic error from git :
fatal: Couldn't JIT the PCRE2 pattern '$PATTERN', got '-48'
interestingly enough, using a JIT enabled PCRE1 library (not the default)
will show a similar error but a different error code.
the underlying problem is the same though; NetBSD includes PAX support
which restricts the use of memory that is both writeable and executable
and that prevents the JIT to create a compiled expression to jump into,
and while the "fix" for NetBSD is simple it would seem the user experience
could be improved if instead of aborting, git will instead return the matches
using the slower interpreter (which seem to be also the recomendation from
the library developers)
it is important to note that the problem is not unique to NetBSD and had
reproduced it in OpenBSD where working around the issue is more complicated
as WˆX exceptions require a filesystem mount option, and I can see it being
problematic with linux (as shown by the open bug[1] and the development of
an alternative allocator to workaround the issue with seLinux) and with
macOS (specially versions older than 10.14) where there are restrictions to
the number of maps allowed with those flags.
I am also curious if expanding NO_LIBPCRE1_JIT as an option to disable JIT
with PCRE2 (with a different name) might be worth pursuing? as well as some
ways to narrow the failures that will trigger the fallback, but the later
is likely to need library changes which might not be possible with the old
version anyway.
[1] https://bugs.exim.org/show_bug.cgi?id=1749
Carlo Marcelo Arenas Belón (2):
grep: fallback to interpreter if JIT fails with pcre1
grep: fallback to interpreter if JIT fails with pcre2
Makefile | 12 ++++++------
grep.c | 13 +++++++++++--
2 files changed, 17 insertions(+), 8 deletions(-)
--
2.20.0
^ permalink raw reply
* [PATCH v2 7/7] test-lib: add the '--stress' option to run a test repeatedly under load
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20181209225628.22216-1-szeder.dev@gmail.com>
Unfortunately, we have a few flaky tests, whose failures tend to be
hard to reproduce. We've found that the best we can do to reproduce
such a failure is to run the test repeatedly while the machine is
under load, and wait in the hope that the load creates enough variance
in the timing of the test's commands that a failure is evenually
triggered. I have a command to do that, and I noticed that two other
contributors have rolled their own scripts to do the same, all
choosing slightly different approaches.
To help reproduce failures in flaky tests, introduce the '--stress'
option to run a test script repeatedly in multiple parallel jobs until
one of them fails, thereby using the test script itself to increase
the load on the machine.
The number of parallel jobs is determined by, in order of precedence:
the number specified as '--stress=<N>', or the value of the
GIT_TEST_STRESS_LOAD environment variable, or twice the number of
available processors (as reported by the 'getconf' utility), or 8.
Make '--stress' imply '--verbose -x --immediate' to get the most
information about rare failures; there is really no point in spending
all the extra effort to reproduce such a failure, and then not know
which command failed and why.
To prevent the several parallel invocations of the same test from
interfering with each other:
- Include the parallel job's number in the name of the trash
directory and the various output files under 't/test-results/' as
a '.stress-<Nr>' suffix.
- Add the parallel job's number to the port number specified by the
user or to the test number, so even tests involving daemons
listening on a TCP socket can be stressed.
- Redirect each parallel test run's output to
't/test-results/$TEST_NAME.stress-<nr>.out', because dumping the
output of several parallel running tests to the terminal would
create a big ugly mess.
For convenience, print the output of the failed test job at the end,
and rename its trash directory to end with the '.stress-failed'
suffix, so it's easy to find in a predictable path. If, in an
unlikely case, more than one jobs were to fail nearly at the same
time, then print the output of all failed jobs, and rename the trash
directory of only the last one (i.e. with the highest job number), as
it is the trash directory of the test whose output will be at the
bottom of the user's terminal.
Based on Jeff King's 'stress' script.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/README | 19 +++++++-
t/test-lib-functions.sh | 7 ++-
t/test-lib.sh | 103 ++++++++++++++++++++++++++++++++++++++--
3 files changed, 123 insertions(+), 6 deletions(-)
diff --git a/t/README b/t/README
index 28711cc508..11ce7675e3 100644
--- a/t/README
+++ b/t/README
@@ -186,6 +186,22 @@ appropriately before running "make".
this feature by setting the GIT_TEST_CHAIN_LINT environment
variable to "1" or "0", respectively.
+--stress::
+--stress=<N>::
+ Run the test script repeatedly in multiple parallel jobs until
+ one of them fails. Useful for reproducing rare failures in
+ flaky tests. The number of parallel jobs is, in order of
+ precedence: <N>, or the value of the GIT_TEST_STRESS_LOAD
+ environment variable, or twice the number of available
+ processors (as shown by the 'getconf' utility), or 8.
+ Implies `--verbose -x --immediate` to get the most information
+ about the failure. Note that the verbose output of each test
+ job is saved to 't/test-results/$TEST_NAME.stress-<nr>.out',
+ and only the output of the failed test job is shown on the
+ terminal. The names of the trash directories get a
+ '.stress-<nr>' suffix, and the trash directory of the failed
+ test job is renamed to end with a '.stress-failed' suffix.
+
You can also set the GIT_TEST_INSTALLED environment variable to
the bindir of an existing git installation to test that installation.
You still need to have built this git sandbox, from which various
@@ -425,7 +441,8 @@ This test harness library does the following things:
- Creates an empty test directory with an empty .git/objects database
and chdir(2) into it. This directory is 't/trash
directory.$test_name_without_dotsh', with t/ subject to change by
- the --root option documented above.
+ the --root option documented above, and a '.stress-<N>' suffix
+ appended by the --stress option.
- Defines standard test helper functions for your scripts to
use. These functions are designed to make all scripts behave
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3746327bde..ee3435c6df 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1288,8 +1288,6 @@ test_set_port () {
# root-only port, use a larger one instead.
port=$(($port + 10000))
fi
-
- eval $var=$port
;;
*[^0-9]*)
error >&7 "invalid port number: $port"
@@ -1298,4 +1296,9 @@ test_set_port () {
# The user has specified the port.
;;
esac
+
+ # Make sure that parallel '--stress' test jobs get different
+ # ports.
+ port=$(($port + ${GIT_TEST_STRESS_JOB_NR:-0}))
+ eval $var=$port
}
diff --git a/t/test-lib.sh b/t/test-lib.sh
index d55d158580..e405191164 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -87,21 +87,110 @@ do
valgrind_only=${opt#--*=} ;;
--root=*)
root=${opt#--*=} ;;
+ --stress)
+ stress=t ;;
+ --stress=*)
+ stress=${opt#--*=} ;;
*)
# Other options will be handled later.
esac
done
+TEST_STRESS_JOB_SFX="${GIT_TEST_STRESS_JOB_NR:+.stress-$GIT_TEST_STRESS_JOB_NR}"
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
-TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
-TRASH_DIRECTORY="trash directory.$TEST_NAME"
+TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME$TEST_STRESS_JOB_SFX"
+TRASH_DIRECTORY="trash directory.$TEST_NAME$TEST_STRESS_JOB_SFX"
test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
case "$TRASH_DIRECTORY" in
/*) ;; # absolute path is good
*) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
esac
+# If --stress was passed, run this test repeatedly in several parallel loops.
+if test "$GIT_TEST_STRESS_STARTED" = "done"
+then
+ : # Don't stress test again.
+elif test -n "$stress"
+then
+ if test "$stress" != t
+ then
+ job_count=$stress
+ elif test -n "$GIT_TEST_STRESS_LOAD"
+ then
+ job_count="$GIT_TEST_STRESS_LOAD"
+ elif job_count=$(getconf _NPROCESSORS_ONLN 2>/dev/null) &&
+ test -n "$job_count"
+ then
+ job_count=$((2 * $job_count))
+ else
+ job_count=8
+ fi
+
+ mkdir -p "$TEST_RESULTS_DIR"
+ stressfail="$TEST_RESULTS_BASE.stress-failed"
+ rm -f "$stressfail"
+
+ stress_exit=0
+ trap '
+ kill $job_pids 2>/dev/null
+ wait
+ stress_exit=1
+ ' TERM INT HUP
+
+ job_pids=
+ job_nr=0
+ while test $job_nr -lt "$job_count"
+ do
+ (
+ GIT_TEST_STRESS_STARTED=done
+ GIT_TEST_STRESS_JOB_NR=$job_nr
+ export GIT_TEST_STRESS_STARTED GIT_TEST_STRESS_JOB_NR
+
+ trap '
+ kill $test_pid 2>/dev/null
+ wait
+ exit 1
+ ' TERM INT
+
+ cnt=0
+ while ! test -e "$stressfail"
+ do
+ $TEST_SHELL_PATH "$0" "$@" >"$TEST_RESULTS_BASE.stress-$job_nr.out" 2>&1 &
+ test_pid=$!
+
+ if wait $test_pid
+ then
+ printf "OK %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
+ else
+ echo $GIT_TEST_STRESS_JOB_NR >>"$stressfail"
+ printf "FAIL %2d.%d\n" $GIT_TEST_STRESS_JOB_NR $cnt
+ fi
+ cnt=$(($cnt + 1))
+ done
+ ) &
+ job_pids="$job_pids $!"
+ job_nr=$(($job_nr + 1))
+ done
+
+ wait
+
+ if test -f "$stressfail"
+ then
+ echo "Log(s) of failed test run(s):"
+ for failed_job_nr in $(sort -n "$stressfail")
+ do
+ echo "Contents of '$TEST_RESULTS_BASE.stress-$failed_job_nr.out':"
+ cat "$TEST_RESULTS_BASE.stress-$failed_job_nr.out"
+ done
+ rm -rf "$TRASH_DIRECTORY.stress-failed"
+ # Move the last one.
+ mv "$TRASH_DIRECTORY.stress-$failed_job_nr" "$TRASH_DIRECTORY.stress-failed"
+ fi
+
+ exit $stress_exit
+fi
+
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
if test "$GIT_TEST_TEE_STARTED" = "done"
@@ -340,7 +429,8 @@ do
--va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind|\
--valgrind=*|\
--valgrind-only=*|\
- --root=*)
+ --root=*|\
+ --stress|--stress=*)
shift ;; # These options were handled already.
*)
echo "error: unknown test option '$1'" >&2; exit 1 ;;
@@ -379,6 +469,13 @@ then
verbose=t
fi
+if test -n "$stress"
+then
+ verbose=t
+ trace=t
+ immediate=t
+fi
+
if test -n "$color"
then
# Save the color control sequences now rather than run tput
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related
* [PATCH v2 6/7] test-lib-functions: introduce the 'test_set_port' helper function
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20181209225628.22216-1-szeder.dev@gmail.com>
Several test scripts run daemons like 'git-daemon' or Apache, and
communicate with them through TCP sockets. To have unique ports where
these daemons are accessible, the ports are usually the number of the
corresponding test scripts, unless the user overrides them via
environment variables, and thus all those tests and test libs contain
more or less the same bit of one-liner boilerplate code to find out
the port. The last patch in this series will make this a bit more
complicated.
Factor out finding the port for a daemon into the common helper
function 'test_set_port' to avoid repeating ourselves.
Take special care of test scripts with "low" numbers:
- Test numbers below 1024 would result in a port that's only usable
as root, so set their port to '10000 + test-nr' to make sure it
doesn't interfere with other tests in the test suite. This makes
the hardcoded port number in 't0410-partial-clone.sh' unnecessary,
remove it.
- The shell's arithmetic evaluation interprets numbers with leading
zeros as octal values, which means that test number below 1000 and
containing the digits 8 or 9 will trigger an error. Remove all
leading zeros from the test numbers to prevent this.
Note that the 'git p4' tests are unlike the other tests involving
daemons in that:
- 'lib-git-p4.sh' doesn't use the test's number for unique port as
is, but does a bit of additional arithmetic on top [1].
- The port is not overridable via an environment variable.
With this patch even 'git p4' tests will use the test's number as
default port, and it will be overridable via the P4DPORT environment
variable.
[1] Commit fc00233071 (git-p4 tests: refactor and cleanup, 2011-08-22)
introduced that "unusual" unique port computation without
explaining why it was necessary (as opposed to simply using the
test number as is). It seems to be just unnecessary complication,
and in any case that commit came way before the "test nr as unique
port" got "standardized" for other daemons in commits c44132fcf3
(tests: auto-set git-daemon port, 2014-02-10), 3bb486e439 (tests:
auto-set LIB_HTTPD_PORT from test name, 2014-02-10), and
bf9d7df950 (t/lib-git-svn.sh: improve svnserve tests with parallel
make test, 2017-12-01).
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/lib-git-daemon.sh | 2 +-
t/lib-git-p4.sh | 9 +--------
t/lib-git-svn.sh | 2 +-
t/lib-httpd.sh | 2 +-
t/t0410-partial-clone.sh | 1 -
t/t5512-ls-remote.sh | 2 +-
t/test-lib-functions.sh | 36 ++++++++++++++++++++++++++++++++++++
7 files changed, 41 insertions(+), 13 deletions(-)
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index f98de95c15..41eb1e3ae8 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -28,7 +28,7 @@ then
test_skip_or_die $GIT_TEST_GIT_DAEMON "file system does not support FIFOs"
fi
-LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-${this_test#t}}
+test_set_port LIB_GIT_DAEMON_PORT
GIT_DAEMON_PID=
GIT_DAEMON_DOCUMENT_ROOT_PATH="$PWD"/repo
diff --git a/t/lib-git-p4.sh b/t/lib-git-p4.sh
index c27599474c..b3be3ba011 100644
--- a/t/lib-git-p4.sh
+++ b/t/lib-git-p4.sh
@@ -53,14 +53,7 @@ time_in_seconds () {
(cd / && "$PYTHON_PATH" -c 'import time; print(int(time.time()))')
}
-# Try to pick a unique port: guess a large number, then hope
-# no more than one of each test is running.
-#
-# This does not handle the case where somebody else is running the
-# same tests and has chosen the same ports.
-testid=${this_test#t}
-git_p4_test_start=9800
-P4DPORT=$((10669 + ($testid - $git_p4_test_start)))
+test_set_port P4DPORT
P4PORT=localhost:$P4DPORT
P4CLIENT=client
diff --git a/t/lib-git-svn.sh b/t/lib-git-svn.sh
index a8130f9119..f3b478c307 100644
--- a/t/lib-git-svn.sh
+++ b/t/lib-git-svn.sh
@@ -13,6 +13,7 @@ fi
GIT_DIR=$PWD/.git
GIT_SVN_DIR=$GIT_DIR/svn/refs/remotes/git-svn
SVN_TREE=$GIT_SVN_DIR/svn-tree
+test_set_port SVNSERVE_PORT
svn >/dev/null 2>&1
if test $? -ne 1
@@ -119,7 +120,6 @@ require_svnserve () {
}
start_svnserve () {
- SVNSERVE_PORT=${SVNSERVE_PORT-${this_test#t}}
svnserve --listen-port $SVNSERVE_PORT \
--root "$rawsvnrepo" \
--listen-once \
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index a8729f8232..e465116ef9 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -82,7 +82,7 @@ case $(uname) in
esac
LIB_HTTPD_PATH=${LIB_HTTPD_PATH-"$DEFAULT_HTTPD_PATH"}
-LIB_HTTPD_PORT=${LIB_HTTPD_PORT-${this_test#t}}
+test_set_port LIB_HTTPD_PORT
TEST_PATH="$TEST_DIRECTORY"/lib-httpd
HTTPD_ROOT_PATH="$PWD"/httpd
diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
index ba3887f178..0aca8d7588 100755
--- a/t/t0410-partial-clone.sh
+++ b/t/t0410-partial-clone.sh
@@ -480,7 +480,6 @@ test_expect_success 'gc stops traversal when a missing but promised object is re
! grep "$TREE_HASH" out
'
-LIB_HTTPD_PORT=12345 # default port, 410, cannot be used as non-root
. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 32e722db2e..cd9e60632d 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -260,7 +260,7 @@ test_lazy_prereq GIT_DAEMON '
# This test spawns a daemon, so run it only if the user would be OK with
# testing with git-daemon.
test_expect_success PIPE,JGIT,GIT_DAEMON 'indicate no refs in standards-compliant empty remote' '
- JGIT_DAEMON_PORT=${JGIT_DAEMON_PORT-${this_test#t}} &&
+ test_set_port JGIT_DAEMON_PORT &&
JGIT_DAEMON_PID= &&
git init --bare empty.git &&
>empty.git/git-daemon-export-ok &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6b3bbf99e4..3746327bde 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -1263,3 +1263,39 @@ test_oid () {
fi &&
eval "printf '%s' \"\${$var}\""
}
+
+# Choose a port number based on the test script's number and store it in
+# the given variable name, unless that variable already contains a number.
+test_set_port () {
+ local var=$1 port
+
+ if test $# -ne 1 || test -z "$var"
+ then
+ BUG "test_set_port requires a variable name"
+ fi
+
+ eval port=\$$var
+ case "$port" in
+ "")
+ # No port is set in the given env var, use the test
+ # number as port number instead.
+ # Remove not only the leading 't', but all leading zeros
+ # as well, so the arithmetic below won't (mis)interpret
+ # a test number like '0123' as an octal value.
+ port=${this_test#${this_test%%[1-9]*}}
+ if test "${port:-0}" -lt 1024
+ then
+ # root-only port, use a larger one instead.
+ port=$(($port + 10000))
+ fi
+
+ eval $var=$port
+ ;;
+ *[^0-9]*)
+ error >&7 "invalid port number: $port"
+ ;;
+ *)
+ # The user has specified the port.
+ ;;
+ esac
+}
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related
* [PATCH v2 3/7] test-lib: consolidate naming of test-results paths
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20181209225628.22216-1-szeder.dev@gmail.com>
There are two places where we strip off any leading path components
and the '.sh' suffix from the test script's pathname, and there are
four places where we construct the name of the 't/test-results'
directory or the name of various test-specific files in there. The
last patch in this series will add even more.
Factor these out into helper variables to avoid repeating ourselves.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 5577d9dc5a..09c77cbd1b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -90,6 +90,10 @@ do
esac
done
+TEST_NAME="$(basename "$0" .sh)"
+TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
+TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
+
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
if test "$GIT_TEST_TEE_STARTED" = "done"
@@ -98,12 +102,11 @@ then
elif test -n "$tee" || test -n "$verbose_log" ||
test -n "$valgrind" || test -n "$valgrind_only"
then
- mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
- BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
+ mkdir -p "$TEST_RESULTS_DIR"
# Make this filename available to the sub-process in case it is using
# --verbose-log.
- GIT_TEST_TEE_OUTPUT_FILE=$BASE.out
+ GIT_TEST_TEE_OUTPUT_FILE=$TEST_RESULTS_BASE.out
export GIT_TEST_TEE_OUTPUT_FILE
# Truncate before calling "tee -a" to get rid of the results
@@ -111,8 +114,8 @@ then
>"$GIT_TEST_TEE_OUTPUT_FILE"
(GIT_TEST_TEE_STARTED=done ${TEST_SHELL_PATH} "$0" "$@" 2>&1;
- echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
- test "$(cat "$BASE.exit")" = 0
+ echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
+ test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
exit
fi
@@ -829,12 +832,9 @@ test_done () {
if test -z "$HARNESS_ACTIVE"
then
- test_results_dir="$TEST_OUTPUT_DIRECTORY/test-results"
- mkdir -p "$test_results_dir"
- base=${0##*/}
- test_results_path="$test_results_dir/${base%.sh}.counts"
+ mkdir -p "$TEST_RESULTS_DIR"
- cat >"$test_results_path" <<-EOF
+ cat >"$TEST_RESULTS_BASE.counts" <<-EOF
total $test_count
success $test_success
fixed $test_fixed
@@ -1040,7 +1040,7 @@ then
fi
# Test repository
-TRASH_DIRECTORY="trash directory.$(basename "$0" .sh)"
+TRASH_DIRECTORY="trash directory.$TEST_NAME"
test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
case "$TRASH_DIRECTORY" in
/*) ;; # absolute path is good
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related
* [PATCH v2 5/7] test-lib: extract Bash version check for '-x' tracing
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20181209225628.22216-1-szeder.dev@gmail.com>
Some of our test scripts can't be reliably run with '-x' tracing
enabled unless executed with a Bash version supporting BASH_XTRACEFD
(since v4.1), and we have a lengthy condition to disable tracing if
such a test script is not executed with a suitable Bash version.
Move this check out from the option parsing loop, so other options can
imply '-x' by setting 'trace=t', without missing this Bash version
check.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index ea1cd34013..d55d158580 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -333,24 +333,7 @@ do
GIT_TEST_CHAIN_LINT=0
shift ;;
-x)
- # Some test scripts can't be reliably traced with '-x',
- # unless the test is run with a Bash version supporting
- # BASH_XTRACEFD (introduced in Bash v4.1). Check whether
- # this test is marked as such, and ignore '-x' if it
- # isn't executed with a suitable Bash version.
- if test -z "$test_untraceable" || {
- test -n "$BASH_VERSION" && {
- test ${BASH_VERSINFO[0]} -gt 4 || {
- test ${BASH_VERSINFO[0]} -eq 4 &&
- test ${BASH_VERSINFO[1]} -ge 1
- }
- }
- }
- then
- trace=t
- else
- echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
- fi
+ trace=t
shift ;;
--tee|\
-V|--verbose-log|\
@@ -373,6 +356,24 @@ then
test -z "$verbose_log" && verbose=t
fi
+if test -n "$trace" && test -n "$test_untraceable"
+then
+ # '-x' tracing requested, but this test script can't be reliably
+ # traced, unless it is run with a Bash version supporting
+ # BASH_XTRACEFD (introduced in Bash v4.1).
+ if test -n "$BASH_VERSION" && {
+ test ${BASH_VERSINFO[0]} -gt 4 || {
+ test ${BASH_VERSINFO[0]} -eq 4 &&
+ test ${BASH_VERSINFO[1]} -ge 1
+ }
+ }
+ then
+ : Executed by a Bash version supporting BASH_XTRACEFD. Good.
+ else
+ echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
+ trace=
+ fi
+fi
if test -n "$trace" && test -z "$verbose_log"
then
verbose=t
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related
* [PATCH v2 4/7] test-lib: set $TRASH_DIRECTORY earlier
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20181209225628.22216-1-szeder.dev@gmail.com>
A later patch in this series will need to know the path to the trash
directory early in 'test-lib.sh', but $TRASH_DIRECTORY is set much
later. Furthermore, the path to the trash directory depends on the
'--root=<path>' option, which, too, is parsed too late.
Move parsing '--root=...' to the early option parsing loop, and set
$TRASH_DIRECTORY where the other test-specific path variables are set.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 09c77cbd1b..ea1cd34013 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -85,6 +85,8 @@ do
valgrind=${opt#--*=} ;;
--valgrind-only=*)
valgrind_only=${opt#--*=} ;;
+ --root=*)
+ root=${opt#--*=} ;;
*)
# Other options will be handled later.
esac
@@ -93,6 +95,12 @@ done
TEST_NAME="$(basename "$0" .sh)"
TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
+TRASH_DIRECTORY="trash directory.$TEST_NAME"
+test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
+case "$TRASH_DIRECTORY" in
+/*) ;; # absolute path is good
+ *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
+esac
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
@@ -318,9 +326,6 @@ do
with_dashes=t; shift ;;
--no-color)
color=; shift ;;
- --root=*)
- root=${1#--*=}
- shift ;;
--chain-lint)
GIT_TEST_CHAIN_LINT=1
shift ;;
@@ -351,7 +356,8 @@ do
-V|--verbose-log|\
--va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind|\
--valgrind=*|\
- --valgrind-only=*)
+ --valgrind-only=*|\
+ --root=*)
shift ;; # These options were handled already.
*)
echo "error: unknown test option '$1'" >&2; exit 1 ;;
@@ -1040,12 +1046,6 @@ then
fi
# Test repository
-TRASH_DIRECTORY="trash directory.$TEST_NAME"
-test -n "$root" && TRASH_DIRECTORY="$root/$TRASH_DIRECTORY"
-case "$TRASH_DIRECTORY" in
-/*) ;; # absolute path is good
- *) TRASH_DIRECTORY="$TEST_OUTPUT_DIRECTORY/$TRASH_DIRECTORY" ;;
-esac
rm -fr "$TRASH_DIRECTORY" || {
GIT_EXIT_OK=t
echo >&5 "FATAL: Cannot prepare test area"
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related
* [PATCH v2 1/7] test-lib: translate SIGTERM and SIGHUP to an exit
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20181209225628.22216-1-szeder.dev@gmail.com>
Right now if a test script receives SIGTERM or SIGHUP (e.g., because a
test was hanging and the user 'kill'-ed it or simply closed the
terminal window the test was running in), the shell exits immediately.
This can be annoying if the test script did any global setup, like
starting apache or git-daemon, as it will not have an opportunity to
clean up after itself. A subsequent run of the test won't be able to
start its own daemon, and will either fail or skip the tests.
Instead, let's trap SIGTERM and SIGHUP as well to make sure we do a
clean shutdown, and just chain it to a normal exit (which will trigger
any cleanup).
This patch follows suit of da706545f7 (t: translate SIGINT to an exit,
2015-03-13), and even stole its commit message as well.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0f1faa24b2..9a3f7930a3 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -476,7 +476,7 @@ die () {
GIT_EXIT_OK=
trap 'die' EXIT
-trap 'exit $?' INT
+trap 'exit $?' INT TERM HUP
# The user-facing functions are loaded from a separate file so that
# test_perf subshells can have them too
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related
* [PATCH v2 2/7] test-lib: parse some --options earlier
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20181209225628.22216-1-szeder.dev@gmail.com>
'test-lib.sh' looks for the presence of certain options like '--tee'
and '--verbose-log', so it can execute the test script again to save
its standard output and error. This happens way before the actual
option parsing loop, and the condition looking for these options looks
a bit odd, too. This patch series will add two more options to look
out for, and, in addition, will have to extract these options' stuck
arguments (i.e. '--opt=arg') as well.
Add a proper option parsing loop to check these select options early
in 'test-lib.sh', making this early option checking more readable and
keeping those later changes in this series simpler. Use a 'for opt in
"$@"' loop to iterate over the options to preserve "$@" intact, so
options like '--verbose-log' can execute the test script again with
all the original options.
As an alternative, we could parse all options early, but there are
options that do require an _unstuck_ argument, which is tricky to
handle properly in such a for loop, and the resulting complexity is,
in my opinion, worse than having this extra, partial option parsing
loop.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
t/test-lib.sh | 53 +++++++++++++++++++++++++++++++--------------------
1 file changed, 32 insertions(+), 21 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 9a3f7930a3..5577d9dc5a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -71,13 +71,33 @@ then
exit 1
fi
+# Parse some options early, taking care to leave $@ intact.
+for opt
+do
+ case "$opt" in
+ --tee)
+ tee=t ;;
+ -V|--verbose-log)
+ verbose_log=t ;;
+ --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
+ valgrind=memcheck ;;
+ --valgrind=*)
+ valgrind=${opt#--*=} ;;
+ --valgrind-only=*)
+ valgrind_only=${opt#--*=} ;;
+ *)
+ # Other options will be handled later.
+ esac
+done
+
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
-case "$GIT_TEST_TEE_STARTED, $* " in
-done,*)
- # do not redirect again
- ;;
-*' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*)
+if test "$GIT_TEST_TEE_STARTED" = "done"
+then
+ : # do not redirect again
+elif test -n "$tee" || test -n "$verbose_log" ||
+ test -n "$valgrind" || test -n "$valgrind_only"
+then
mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
@@ -94,8 +114,7 @@ done,*)
echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
test "$(cat "$BASE.exit")" = 0
exit
- ;;
-esac
+fi
# For repeatability, reset the environment to known value.
# TERM is sanitized below, after saving color control sequences.
@@ -296,17 +315,6 @@ do
with_dashes=t; shift ;;
--no-color)
color=; shift ;;
- --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind)
- valgrind=memcheck
- shift ;;
- --valgrind=*)
- valgrind=${1#--*=}
- shift ;;
- --valgrind-only=*)
- valgrind_only=${1#--*=}
- shift ;;
- --tee)
- shift ;; # was handled already
--root=*)
root=${1#--*=}
shift ;;
@@ -336,9 +344,12 @@ do
echo >&2 "warning: ignoring -x; '$0' is untraceable without BASH_XTRACEFD"
fi
shift ;;
- -V|--verbose-log)
- verbose_log=t
- shift ;;
+ --tee|\
+ -V|--verbose-log|\
+ --va|--val|--valg|--valgr|--valgri|--valgrin|--valgrind|\
+ --valgrind=*|\
+ --valgrind-only=*)
+ shift ;; # These options were handled already.
*)
echo "error: unknown test option '$1'" >&2; exit 1 ;;
esac
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply related
* [PATCH v2 0/7] test-lib: add the '--stress' option to help reproduce occasional failures in flaky tests
From: SZEDER Gábor @ 2018-12-09 22:56 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, SZEDER Gábor
In-Reply-To: <20181204163457.15717-1-szeder.dev@gmail.com>
This patch series tries to make reproducing rare failures in flaky
tests easier: it adds the '--stress' option to our test library to run
the test script repeatedly in multiple parallel jobs, in the hope that
the increased load creates enough variance in the timing of the test's
commands that such a failure is eventually triggered.
Notable changes since v1:
- Made it more Peff-friendly :), namely it will now show the log of
the failed test and will rename its trash directory.
Furthermore, '--stress' will now imply '--verbose -x --immediate'.
- Improved abort handling based on the discussion of the previous
version. (As a result, the patch is so heavily modified, that
'range-diff' with default parameters consideres it a different
patch; increasing the creation factor then results in one big ugly
diff of a diff, so I left it as-is.)
- Added a few new patches, mostly preparatory refactorings, though
the first one might be considered a bugfix.
- Other minor cleanups suggested in the previous discussion.
v1: https://public-inbox.org/git/20181204163457.15717-1-szeder.dev@gmail.com/T/
SZEDER Gábor (7):
test-lib: translate SIGTERM and SIGHUP to an exit
test-lib: parse some --options earlier
test-lib: consolidate naming of test-results paths
test-lib: set $TRASH_DIRECTORY earlier
test-lib: extract Bash version check for '-x' tracing
test-lib-functions: introduce the 'test_set_port' helper function
test-lib: add the '--stress' option to run a test repeatedly under
load
t/README | 19 +++-
t/lib-git-daemon.sh | 2 +-
t/lib-git-p4.sh | 9 +-
t/lib-git-svn.sh | 2 +-
t/lib-httpd.sh | 2 +-
t/t0410-partial-clone.sh | 1 -
t/t5512-ls-remote.sh | 2 +-
t/test-lib-functions.sh | 39 +++++++
t/test-lib.sh | 227 +++++++++++++++++++++++++++++----------
9 files changed, 230 insertions(+), 73 deletions(-)
Range-diff:
-: ---------- > 1: 3a5c926167 test-lib: translate SIGTERM and SIGHUP to an exit
-: ---------- > 2: 8eee8d7fba test-lib: parse some --options earlier
1: f4bb53e676 ! 3: dd20ae5e9a test-lib: consolidate naming of test-results paths
@@ -4,8 +4,9 @@
There are two places where we strip off any leading path components
and the '.sh' suffix from the test script's pathname, and there are
- two places where we construct the filename of test output files in
- 't/test-results/'. The last patch in this series will add even more.
+ four places where we construct the name of the 't/test-results'
+ directory or the name of various test-specific files in there. The
+ last patch in this series will add even more.
Factor these out into helper variables to avoid repeating ourselves.
@@ -15,22 +16,23 @@
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@
- exit 1
- fi
+ esac
+ done
+TEST_NAME="$(basename "$0" .sh)"
-+TEST_RESULTS_BASE="$TEST_OUTPUT_DIRECTORY/test-results/$TEST_NAME"
++TEST_RESULTS_DIR="$TEST_OUTPUT_DIRECTORY/test-results"
++TEST_RESULTS_BASE="$TEST_RESULTS_DIR/$TEST_NAME"
+
# if --tee was passed, write the output not only to the terminal, but
# additionally to the file test-results/$BASENAME.out, too.
- case "$GIT_TEST_TEE_STARTED, $* " in
+ if test "$GIT_TEST_TEE_STARTED" = "done"
@@
- # do not redirect again
- ;;
- *' --tee '*|*' --va'*|*' -V '*|*' --verbose-log '*)
+ elif test -n "$tee" || test -n "$verbose_log" ||
+ test -n "$valgrind" || test -n "$valgrind_only"
+ then
- mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
- BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
-+ mkdir -p "$(dirname "$TEST_RESULTS_BASE")"
++ mkdir -p "$TEST_RESULTS_DIR"
# Make this filename available to the sub-process in case it is using
# --verbose-log.
@@ -48,8 +50,8 @@
+ echo $? >"$TEST_RESULTS_BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
+ test "$(cat "$TEST_RESULTS_BASE.exit")" = 0
exit
- ;;
- esac
+ fi
+
@@
if test -z "$HARNESS_ACTIVE"
@@ -58,7 +60,7 @@
- mkdir -p "$test_results_dir"
- base=${0##*/}
- test_results_path="$test_results_dir/${base%.sh}.counts"
-+ mkdir -p "$(dirname "$TEST_RESULTS_BASE")"
++ mkdir -p "$TEST_RESULTS_DIR"
- cat >"$test_results_path" <<-EOF
+ cat >"$TEST_RESULTS_BASE.counts" <<-EOF
-: ---------- > 4: f3941077e6 test-lib: set $TRASH_DIRECTORY earlier
-: ---------- > 5: fefbca96ee test-lib: extract Bash version check for '-x' tracing
2: 9aec8662f9 ! 6: b4b6844a3e test-lib-functions: introduce the 'test_set_port' helper function
@@ -27,7 +27,7 @@
containing the digits 8 or 9 will trigger an error. Remove all
leading zeros from the test numbers to prevent this.
- Note that the Perforce tests are unlike the other tests involving
+ Note that the 'git p4' tests are unlike the other tests involving
daemons in that:
- 'lib-git-p4.sh' doesn't use the test's number for unique port as
@@ -35,7 +35,7 @@
- The port is not overridable via an environment variable.
- With this patch even Perforce tests will use the test's number as
+ With this patch even 'git p4' tests will use the test's number as
default port, and it will be overridable via the P4DPORT environment
variable.
@@ -161,7 +161,7 @@
+ BUG "test_set_port requires a variable name"
+ fi
+
-+ eval port=\"\${$var}\"
++ eval port=\$$var
+ case "$port" in
+ "")
+ # No port is set in the given env var, use the test
3: a5aa71f20c < -: ---------- test-lib: add the '--stress' option to run a test repeatedly under load
-: ---------- > 7: 8470b55f65 test-lib: add the '--stress' option to run a test repeatedly under load
--
2.20.0.rc2.156.g5a9fd2ce9c
^ permalink raw reply
* pw/add-p-select, was Re: What's cooking in git.git (Dec 2018, #01; Sun, 9)
From: Johannes Schindelin @ 2018-12-09 20:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqq8t0z3xcc.fsf@gitster-ct.c.googlers.com>
Hi Junio,
On Sun, 9 Dec 2018, Junio C Hamano wrote:
> * pw/add-p-select (2018-07-26) 4 commits
> - add -p: optimize line selection for short hunks
> - add -p: allow line selection to be inverted
> - add -p: select modified lines correctly
> - add -p: select individual hunk lines
>
> "git add -p" interactive interface learned to let users choose
> individual added/removed lines to be used in the operation, instead
> of accepting or rejecting a whole hunk.
>
> Will discard.
> No further feedbacks on the topic for quite some time.
That is not quite true. I did comment that this feature (which I take as
being inspired by Git GUI's "Stage Selected Line"), and thought that it
would be useful.
I could imagine, however, that it would make sense for `git add -p` to
imitate that feature more closely: by allowing to stage a single line and
then presenting the current hunk (re-computed) again.
Ciao,
Dscho
^ 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