* Re: [BUG] fetch output is ugly in 'next'
From: Duy Nguyen @ 2016-10-21 12:11 UTC (permalink / raw)
To: Jeff King; +Cc: Git Mailing List
In-Reply-To: <20161021002654.qo6kcya4gocp73rs@sigill.intra.peff.net>
On Fri, Oct 21, 2016 at 7:26 AM, Jeff King <peff@peff.net> wrote:
> I recently started using lt/abbrev-auto via 'next'. This is the fetch
> output I saw today:
>
> $ git fetch
> remote: Counting objects: 332, done.
> remote: Compressing objects: 100% (237/237), done.
> remote: Total 332 (delta 182), reused 64 (delta 64), pack-reused 31
> Receiving objects: 100% (332/332), 351.53 KiB | 0 bytes/s, done.
> Resolving deltas: 100% (184/184), completed with 26 local objects.
> From git://github.com/gitster/git
> + fb65135d9c...16ab66ec97 jch -> origin/jch (forced update)
> fd47ae6a5b..98985c6911 jk/diff-submodule-diff-inline -> origin/jk/diff-submodule-diff-inline
> * [new branch] jk/no-looking-at-dotgit-outside-repo -> origin/jk/no-looking-at-dotgit-outside-repo
> + 3a8f853f16...1369f9b5ba jt/trailer-with-cruft -> origin/jt/trailer-with-cruft (forced update)
> * [new branch] pt/gitgui-updates -> origin/pt/gitgui-updates
> + 7594b34cbb...be8e40093b pu -> origin/pu (forced update)
> + 7b95cf9a4e...76e368c378 tg/add-chmod+x-fix -> origin/tg/add-chmod+x-fix (forced update)
> + c4cf1f93cf...221912514c va/i18n-perl-scripts -> origin/va/i18n-perl-scripts (forced update)
> * [new branch] yk/git-tag-remove-mention-of-old-layout-in-doc -> origin/yk/git-tag-remove-mention-of-old-layout-in-doc
>
> Yuck.
My eyes!
> I could maybe get over the wasted space due to the longer sha1s,
> but we clearly need to fix the alignment computation. I haven't dug on
> it at all; it's probably low-hanging fruit if somebody is interested.
Yeah.. replacing the 4 DEFAULT_ABBREV in fetch.c with something
sensible should do it.
--
Duy
^ permalink raw reply
* Re: [PATCH v4 20/25] sequencer: refactor write_message()
From: Johannes Schindelin @ 2016-10-21 11:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqqd1iug42f.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 20 Oct 2016, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > If I were doing this, I would make this into three separate steps:
> >
> > - move the strbuf_release(msgbuf) to the caller in
> > do_pick_commit();
> >
> > - add the missing rollback_lock_file(), which is a bugfix; and
> > then finally
> >
> > - allow the helper to take not a strbuf but <buf, len> pair as
> > parameters.
> >
> > The end result of this patch achieves two thirds of the above, but
> > especially given that write_message() only has two call sites in a
> > single function, I think it is OK and preferrable even to do all
> > three.
>
> Ah, make that four steps. The final one is:
>
> - add append_eol parameter that nobody uses at this step in the
> series.
Done,
Dscho
^ permalink raw reply
* Re: [PATCH v4 18/25] sequencer: do not try to commit when there were merge conflicts
From: Johannes Schindelin @ 2016-10-21 11:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jakub Narębski, Johannes Sixt, Ramsay Jones
In-Reply-To: <xmqqmvhyg4ru.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 20 Oct 2016, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > The return value of do_recursive_merge() may be positive (indicating merge
> > conflicts), or 0 (indicating success). It also may be negative, indicating
> > a fatal error that requires us to abort.
> >
> > Now, if the return value indicates that there are merge conflicts, we
> > should not try to commit those changes, of course.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > sequencer.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index cbc3742..9ffc090 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -787,7 +787,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> > res = allow;
> > goto leave;
> > }
> > - if (!opts->no_commit)
> > + if (!res && !opts->no_commit)
> > res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
> > opts, allow, opts->edit, 0, 0);
>
> This by itself looks more like a bugfix than preparation for later
> steps. The only reason why it is not a bugfix is because there is
> nothing in this function that makes res a non-zero value and reach
> this if statement at this step. We would have been caught by an
> "if (res) { ... rerere(); goto leave; }" or
> "if (allow < 0) { res = allow; goto leave; }"
> that appear before this part of the code.
>
> So while it is not wrong per-se, I think this should be part of an
> actual change that makes it possible for the control flow to reach
> here with non-zero res.
It looks like it is no longer needed (I *think* that it was made obsolete
by the change where I now "goto fast_forward_edit" only in case there were
no errors).
In any case, the patch's gone now,
Dscho
^ permalink raw reply
* Re: [BUG] [PATCH]: run-command.c
From: Duncan Roe @ 2016-10-21 11:07 UTC (permalink / raw)
To: git
In-Reply-To: <20161021090029.glr5u6gwrxluavir@sigill.intra.peff.net>
On Fri, Oct 21, 2016 at 05:00:29AM -0400, Jeff King wrote:
> On Fri, Oct 21, 2016 at 04:50:13PM +1100, Duncan Roe wrote:
>
> > For example, if .git/config has this alias (the sleep is to leave time to
> > examine output from ps, &c.):
> >
> > [alias]
> > tryme = "!echo $PWD;sleep 600"
> >
> > [...]
> > 16:42:06$ ps axf|grep -A2 trym[e]
> > 2599 pts/4 S+ 0:00 \_ git tryme
> > 2601 pts/4 S+ 0:00 \_ /bin/sh -c echo $PWD;sleep 600 echo $PWD;sleep 600
> > 2602 pts/4 S+ 0:00 \_ sleep 600
> > 16:42:45$ cat /proc/2601/cmdline | xargs -0 -n1 echo
> > /bin/sh
> > -c
> > echo $PWD;sleep 600
> > echo $PWD;sleep 600
>
> This duplicated argument is expected and normal. The arguments after "-c
> whatever" become positional parameters $0, $1, etc. The actual script
> arguments start at "$1", and "$0" is typically the "script name".
> So you have to stick some placeholder value in the "$0" slot, so that
> the sub-script can find the actual arguments. E.g., try:
>
> sh -c '
> for i in "$@"; do
> echo "got $i"
> done
> ' one two three
>
> it will print only:
>
> got two
> got three
>
> But if you stick a placeholder there, it works:
>
> sh -c '
> for i in "$@"; do
> echo "got $i"
> done
> ' placeholder one two three
>
> The value of the placeholder does not matter to the shell. But it is
> accessible to the script inside via $0:
>
> sh -c '
> echo "\$0 = $0"
> echo "\$1 = $1"
> echo "\$2 = $2"
> echo "\$3 = $3"
> ' placeholder one two three
>
> Since our script does not have a filename, we just stick the script
> contents there (which is really just a convention, and one I doubt
> anybody is really relying on, but there's no point in breaking it now).
>
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -182,8 +182,8 @@ static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)
> > else
> > argv_array_pushf(out, "%s \"$@\"", argv[0]);
> > }
> > -
> > - argv_array_pushv(out, argv);
> > + else
> > + argv_array_pushv(out, argv);
> > return out->argv;
> > }
>
> Try running "make test" with this. Lots of things break, because we are
> not sending the positional parameters to the shell script at all.
>
> If we just cared about the positional parmeters, we _could_ do something
> like:
>
> if (argv[0]) {
> argv_array_push(out, "sh");
> argv_array_pushv(out, argv + 1);
> }
>
> That would omit "$0" entirely when we have no positional parameters (and
> the shell generally fills in "sh" there itself), and provide a dummy
> "sh" value when we need to use it as a placeholder.
>
> But again, there's no real value in breaking the existing convention.
>
> -Peff
Agreed - tests 110 and 111 in t1300-repo-config.sh fail. After that, "make test"
gives up, losing about 14000 lines of output.
Sorry for the noise,
Cheers ... Duncan.
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Duy Nguyen @ 2016-10-21 11:03 UTC (permalink / raw)
To: Dennis Kaarsemaker
Cc: Jeff King, Johannes Schindelin, Junio C Hamano, Git Mailing List
In-Reply-To: <1476996017.28685.10.camel@kaarsemaker.net>
On Fri, Oct 21, 2016 at 3:40 AM, Dennis Kaarsemaker
<dennis@kaarsemaker.net> wrote:
> On Thu, 2016-10-20 at 08:31 -0400, Jeff King wrote:
>
>> I'm also not entirely convinced that the test suite being a shell script
>> is the main culprit for its slowness. We run git a lot of times, and
>> that's inherent in testing it. I ran the whole test suite under
>> "strace -f -e execve". There are ~335K execs. Here's the breakdown of
>> the top ones:
>
> You're measuring execve's, but fork (well, fork emulation. There's no
> actual fork) is also expensive on windows iirc, so subshells add a lot
> to this cost.
shells fork on piping as well, and redirection and some other
construct if I remember correctly (I attempted to port busybox ash to
windows and had to find and "fix" all the forks)
> That said, strace -eclone says that a 'make test' forks
> ~408k times, and while this is significantly more than the amount of
> execs in your example, this does include cvs and svn tests and it's
> still in the same ballpark.
--
Duy
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Duy Nguyen @ 2016-10-21 10:59 UTC (permalink / raw)
To: René Scharfe; +Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List
In-Reply-To: <e0e320ce-14a1-b4a8-aa07-6bc4f3c4a424@web.de>
On Thu, Oct 20, 2016 at 11:40 PM, René Scharfe <l.s.r@web.de> wrote:
> Am 20.10.2016 um 13:02 schrieb Duy Nguyen:
>> On Wed, Oct 19, 2016 at 4:18 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>>> Hi Junio,
>>>
>>> I know you are a fan of testing things thoroughly in the test suite, but I
>>> have to say that it is getting out of hand, in particular due to our
>>> over-use of shell script idioms (which really only run fast on Linux, not
>>> a good idea for a portable software).
>>>
>>> My builds of `pu` now time out, after running for 3h straight in the VM
>>> dedicated to perform the daily routine of building and testing the git.git
>>> branches in Git for Windows' SDK. For comparison, `next` passes build &
>>> tests in 2.6h. That is quite the jump.
>>
>> I'm just curious, will running git.exe from WSL [1] help speed things
>> up a bit (or, hopefully, a lot)? I'm assuming that shell's speed in
>> WSL is quite fast.
>>
>> I'm pretty sure the test suite would need some adaptation, but if the
>> speedup is significant, maybe it's worth spending time on.
>>
>> [1] https://news.ycombinator.com/item?id=12748395
>
> I get this on WSL with prove -j8:
>
> Files=750, Tests=13657, 906 wallclock secs ( 8.51 usr 17.17 sys + 282.62 cusr 3731.85 csys = 4040.15 CPU)
>
> And this for a run on Debian inside a Hyper-V VM on the same system:
>
> Files=759, Tests=13895, 99 wallclock secs ( 4.81 usr 1.06 sys + 39.70 cusr 25.82 csys = 71.39 CPU)
>
> All tests pass on master.
Thank you for doing this. 10 times slower is probably not worth
following up (though absolute numbers still look amazing, you have
some beefy machine there).
--
Duy
^ permalink raw reply
* [PATCH 3/3] travis: use --verbose-log test option
From: Jeff King @ 2016-10-21 10:48 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, git
In-Reply-To: <20161021104107.vh3bjx6x6pd6izat@sigill.intra.peff.net>
Because we run the tests via "prove", the output from
"--verbose" may interfere with our TAP output. Using
"--verbose-log" solves this while letting us retain our
on-disk log.
Signed-off-by: Jeff King <peff@peff.net>
---
.travis.yml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/.travis.yml b/.travis.yml
index 37a1e1fb6d..9a65514d82 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -31,7 +31,7 @@ env:
- LINUX_GIT_LFS_VERSION="1.2.0"
- DEFAULT_TEST_TARGET=prove
- GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
- - GIT_TEST_OPTS="--verbose --tee"
+ - GIT_TEST_OPTS="--verbose-log"
- GIT_TEST_HTTPD=true
- GIT_TEST_CLONE_2GB=YesPlease
# t9810 occasionally fails on Travis CI OS X
--
2.10.1.776.ge0e381e
^ permalink raw reply related
* [PATCH 2/3] test-lib: add --verbose-log option
From: Jeff King @ 2016-10-21 10:48 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, git
In-Reply-To: <20161021104107.vh3bjx6x6pd6izat@sigill.intra.peff.net>
The "--verbose" option redirects output from arbitrary
test commands to stdout. This is useful for examining the
output manually, like:
./t5547-push-quarantine.sh -v | less
But it also means that the output is intermingled with the
TAP directives, which can confuse a TAP parser like "prove".
This has always been a potential problem, but became an
issue recently when one test happened to output the word
"ok" on a line by itself, which prove interprets as a test
success:
$ prove t5547-push-quarantine.sh :: -v
t5547-push-quarantine.sh .. 1/? To dest.git
* [new branch] HEAD -> master
To dest.git
! [remote rejected] reject -> reject (pre-receive hook declined)
error: failed to push some refs to 'dest.git'
fatal: git cat-file d08c8eba97f4e683ece08654c7c8d2ba0c03b129: bad file
t5547-push-quarantine.sh .. Failed -1/4 subtests
Test Summary Report
-------------------
t5547-push-quarantine.sh (Wstat: 0 Tests: 5 Failed: 0)
Parse errors: Tests out of sequence. Found (2) but expected (3)
Tests out of sequence. Found (3) but expected (4)
Tests out of sequence. Found (4) but expected (5)
Bad plan. You planned 4 tests but ran 5.
Files=1, Tests=5, 0 wallclock secs ( 0.01 usr + 0.01 sys = 0.02 CPU)
Result: FAIL
One answer is "if it hurts, don't do it", but that's not
quite the whole story. The Travis tests use "--verbose
--tee" so that they can get the benefit of prove's parallel
options, along with a verbose log in case there is a
failure. We just need the verbose output to go to the log,
but keep stdout clean.
Getting this right turns out to be surprisingly difficult.
Here's the progression of alternatives I considered:
1. Add an option to write verbose output to stderr. This is
hard to capture, though, because we want each test to
have its own log (because they're all run in parallel
and the jumbled output would be useless).
2. Add an option to write verbose output to a file in
test-results. This works, but the log is missing all of
the non-verbose output, which gives context.
3. Like (2), but teach say_color() to additionally output
to the log. This mostly works, but misses any output
that happens outside of the say() functions (which isn't
a lot, but is a potential maintenance headache).
4. Like (2), but make the log file the same as the "--tee"
file. That almost works, but now we have two processes
opening the same file. That gives us two separate
descriptors, each with their own idea of the current
position. They'll each start writing at offset 0, and
overwrite each other's data.
5. Like (4), but in each case open the file for appending.
That atomically positions each write at the end of the
file.
It's possible we may still get sheared writes between
the two processes, but this is already the case when
writing to stdout. It's not a problem in practice
because the test harness generally waits for snippets to
finish before writing the TAP output.
We can ignore buffering issues with tee, because POSIX
mandates that it does not buffer. Likewise, POSIX
specifies "tee -a", so it should be available
everywhere.
This patch implements option (5), which seems to work well
in practice.
Signed-off-by: Jeff King <peff@peff.net>
---
Arguably we don't need a new option for this, and could just do it
automatically. I wasn't sure when, though:
- if "--tee --verbose" triggers it, then somebody running that outside
of "prove" who _wants_ the verbose output on stdout (because they're
looking at it, but also want to save a copy to the log) would be
regressed
- possibly "--tee" could just always write verbose output to the
logfile (but not stdout). That's kind of weirdly magical, and we'd
have to update the travis invocation anyway.
So I went with a new option which implies the other ones. No chance of
regression, and it's easy to explain.
t/README | 6 ++++++
t/test-lib.sh | 22 +++++++++++++++++++---
2 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/t/README b/t/README
index 0f764c0aef..4982d1c521 100644
--- a/t/README
+++ b/t/README
@@ -153,6 +153,12 @@ appropriately before running "make".
As the names depend on the tests' file names, it is safe to
run the tests with this option in parallel.
+--verbose-log::
+ Write verbose output to the same logfile as `--tee`, but do
+ _not_ write it to stdout. Unlike `--tee --verbose`, this option
+ is safe to use when stdout is being consumed by a TAP parser
+ like `prove`. Implies `--tee` and `--verbose`.
+
--with-dashes::
By default tests are run without dashed forms of
commands (like git-commit) in the PATH (it only uses
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 33cbbb7806..85946ec40d 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -54,11 +54,21 @@ case "$GIT_TEST_TEE_STARTED, $* " in
done,*)
# do not redirect again
;;
-*' --tee '*|*' --va'*)
+*' --tee '*|*' --va'*|*' --verbose-log '*)
mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
+
+ # Make this filename available to the sub-process in case it is using
+ # --verbose-log.
+ GIT_TEST_TEE_OUTPUT_FILE=$BASE.out
+ export GIT_TEST_TEE_OUTPUT_FILE
+
+ # Truncate before calling "tee -a" to get rid of the results
+ # from any previous runs.
+ >"$GIT_TEST_TEE_OUTPUT_FILE"
+
(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} "$0" "$@" 2>&1;
- echo $? >"$BASE.exit") | tee "$BASE.out"
+ echo $? >"$BASE.exit") | tee -a "$GIT_TEST_TEE_OUTPUT_FILE"
test "$(cat "$BASE.exit")" = 0
exit
;;
@@ -246,6 +256,9 @@ do
trace=t
verbose=t
shift ;;
+ --verbose-log)
+ verbose_log=t
+ shift ;;
*)
echo "error: unknown test option '$1'" >&2; exit 1 ;;
esac
@@ -319,7 +332,10 @@ fi
exec 5>&1
exec 6<&0
-if test "$verbose" = "t"
+if test "$verbose_log" = "t"
+then
+ exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
+elif test "$verbose" = "t"
then
exec 4>&2 3>&1
else
--
2.10.1.776.ge0e381e
^ permalink raw reply related
* [PATCH 1/3] test-lib: handle TEST_OUTPUT_DIRECTORY with spaces
From: Jeff King @ 2016-10-21 10:42 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, git
In-Reply-To: <20161021104107.vh3bjx6x6pd6izat@sigill.intra.peff.net>
We are careful in test_done to handle a results directory
with a space in it, but the "--tee" code path does not.
Doing:
export TEST_OUTPUT_DIRECTORY='/tmp/path with spaces'
./t000-init.sh --tee
results in errors. Let's consistently double-quote our path
variables so that this works.
Signed-off-by: Jeff King <peff@peff.net>
---
I imagine nobody cares much about this, but I just happened to notice it
while writing the rest of the patches (and it's obviously quite easy to
fix).
t/test-lib.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bde10..33cbbb7806 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -58,8 +58,8 @@ done,*)
mkdir -p "$TEST_OUTPUT_DIRECTORY/test-results"
BASE="$TEST_OUTPUT_DIRECTORY/test-results/$(basename "$0" .sh)"
(GIT_TEST_TEE_STARTED=done ${SHELL_PATH} "$0" "$@" 2>&1;
- echo $? > $BASE.exit) | tee $BASE.out
- test "$(cat $BASE.exit)" = 0
+ echo $? >"$BASE.exit") | tee "$BASE.out"
+ test "$(cat "$BASE.exit")" = 0
exit
;;
esac
--
2.10.1.776.ge0e381e
^ permalink raw reply related
* [PATCH 0/3] fix travis TAP/--verbose conflict
From: Jeff King @ 2016-10-21 10:41 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, git
In-Reply-To: <20161021084348.dp4jfpfownodl7nz@sigill.intra.peff.net>
On Fri, Oct 21, 2016 at 04:43:48AM -0400, Jeff King wrote:
> The obvious fix would be to send "--verbose" output to stderr, but I
> suspect that would end up annoying for people who do:
>
> ./t5547-push-quarantine.sh -v | less
>
> to read long output. Probably we need some option like "--log" which
> logs in the same way that "--tee" does, but _without_ sending the data
> to stdout. Naively, that just means replacing the "tee" invocation with
> "cat", but I suspect it will be a lot more complicated than that,
> because we still need to let the TAP output go to stdout.
Yeah, it was definitely a lot more complicated. This patch series fixes
it.
[1/3]: test-lib: handle TEST_OUTPUT_DIRECTORY with spaces
[2/3]: test-lib: add --verbose-log option
[3/3]: travis: use --verbose-log test option
.travis.yml | 2 +-
t/README | 6 ++++++
t/test-lib.sh | 24 ++++++++++++++++++++----
3 files changed, 27 insertions(+), 5 deletions(-)
-Peff
^ permalink raw reply
* [PATCH 2/2] Git.pm: add comment pointing to t9000
From: Matthieu Moy @ 2016-10-21 9:20 UTC (permalink / raw)
To: gitster; +Cc: git, Ramsay Jones, Matthieu Moy
In-Reply-To: <20161021092024.15861-1-Matthieu.Moy@imag.fr>
parse_mailboxes should probably eventually be completely equivalent to
Mail::Address, and if this happens we can drop the Mail::Address
dependency. Add a comment in the code reminding the current state of the
code, and point to the corresponding failing test to help future
contributors to get it right.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
perl/Git.pm | 2 ++
1 file changed, 2 insertions(+)
diff --git a/perl/Git.pm b/perl/Git.pm
index 42e0895ef7..8bb2b7c7e3 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -870,6 +870,8 @@ Return an array of mailboxes extracted from a string.
=cut
+# Very close to Mail::Address's parser, but we still have minor
+# differences in some cases (see t9000 for examples).
sub parse_mailboxes {
my $re_comment = qr/\((?:[^)]*)\)/;
my $re_quote = qr/"(?:[^\"\\]|\\.)*"/;
--
2.10.1.651.gffd0de0
^ permalink raw reply related
* [PATCH 1/2] t9000-addresses: update expected results after fix
From: Matthieu Moy @ 2016-10-21 9:20 UTC (permalink / raw)
To: gitster; +Cc: git, Ramsay Jones, Matthieu Moy
In-Reply-To: <xmqq1szaeda9.fsf@gitster.mtv.corp.google.com>
e3fdbcc8e1 (parse_mailboxes: accept extra text after <...> address,
2016-10-13) improved our in-house address parser and made it closer to
Mail::Address. As a consequence, some tests comparing it to
Mail::Address now pass, but e3fdbcc8e1 forgot to update the test.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
t/t9000/test.pl | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/t/t9000/test.pl b/t/t9000/test.pl
index 2d05d3eeab..dfeaa9c655 100755
--- a/t/t9000/test.pl
+++ b/t/t9000/test.pl
@@ -32,15 +32,15 @@ my @success_list = (q[Jane],
q["Jane\" Doe" <jdoe@example.com>],
q[Doe, jane <jdoe@example.com>],
q["Jane Doe <jdoe@example.com>],
- q['Jane 'Doe' <jdoe@example.com>]);
+ q['Jane 'Doe' <jdoe@example.com>],
+ q[Jane@:;\.,()<>Doe <jdoe@example.com>],
+ q[Jane <jdoe@example.com> Doe],
+ q[<jdoe@example.com> Jane Doe]);
my @known_failure_list = (q[Jane\ Doe <jdoe@example.com>],
q["Doe, Ja"ne <jdoe@example.com>],
q["Doe, Katarina" Jane <jdoe@example.com>],
- q[Jane@:;\.,()<>Doe <jdoe@example.com>],
q[Jane jdoe@example.com],
- q[<jdoe@example.com> Jane Doe],
- q[Jane <jdoe@example.com> Doe],
q["Jane "Kat"a" ri"na" ",Doe" <jdoe@example.com>],
q[Jane Doe],
q[Jane "Doe <jdoe@example.com>"],
--
2.10.1.651.gffd0de0
^ permalink raw reply related
* Re: [BUG] [PATCH]: run-command.c
From: Jeff King @ 2016-10-21 9:00 UTC (permalink / raw)
To: git
In-Reply-To: <20161021055013.GA31554@dimstar.local.net>
On Fri, Oct 21, 2016 at 04:50:13PM +1100, Duncan Roe wrote:
> For example, if .git/config has this alias (the sleep is to leave time to
> examine output from ps, &c.):
>
> [alias]
> tryme = "!echo $PWD;sleep 600"
>
> [...]
> 16:42:06$ ps axf|grep -A2 trym[e]
> 2599 pts/4 S+ 0:00 \_ git tryme
> 2601 pts/4 S+ 0:00 \_ /bin/sh -c echo $PWD;sleep 600 echo $PWD;sleep 600
> 2602 pts/4 S+ 0:00 \_ sleep 600
> 16:42:45$ cat /proc/2601/cmdline | xargs -0 -n1 echo
> /bin/sh
> -c
> echo $PWD;sleep 600
> echo $PWD;sleep 600
This duplicated argument is expected and normal. The arguments after "-c
whatever" become positional parameters $0, $1, etc. The actual script
arguments start at "$1", and "$0" is typically the "script name".
So you have to stick some placeholder value in the "$0" slot, so that
the sub-script can find the actual arguments. E.g., try:
sh -c '
for i in "$@"; do
echo "got $i"
done
' one two three
it will print only:
got two
got three
But if you stick a placeholder there, it works:
sh -c '
for i in "$@"; do
echo "got $i"
done
' placeholder one two three
The value of the placeholder does not matter to the shell. But it is
accessible to the script inside via $0:
sh -c '
echo "\$0 = $0"
echo "\$1 = $1"
echo "\$2 = $2"
echo "\$3 = $3"
' placeholder one two three
Since our script does not have a filename, we just stick the script
contents there (which is really just a convention, and one I doubt
anybody is really relying on, but there's no point in breaking it now).
> --- a/run-command.c
> +++ b/run-command.c
> @@ -182,8 +182,8 @@ static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)
> else
> argv_array_pushf(out, "%s \"$@\"", argv[0]);
> }
> -
> - argv_array_pushv(out, argv);
> + else
> + argv_array_pushv(out, argv);
> return out->argv;
> }
Try running "make test" with this. Lots of things break, because we are
not sending the positional parameters to the shell script at all.
If we just cared about the positional parmeters, we _could_ do something
like:
if (argv[0]) {
argv_array_push(out, "sh");
argv_array_pushv(out, argv + 1);
}
That would omit "$0" entirely when we have no positional parameters (and
the shell generally fills in "sh" there itself), and provide a dummy
"sh" value when we need to use it as a placeholder.
But again, there's no real value in breaking the existing convention.
-Peff
^ permalink raw reply
* Re: Prove "Tests out of sequence" Error
From: Jeff King @ 2016-10-21 8:43 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, git
In-Reply-To: <20161021082035.xad6wfny5i6wtshh@sigill.intra.peff.net>
On Fri, Oct 21, 2016 at 04:20:35AM -0400, Jeff King wrote:
> Yes, but I do not see how it can trigger this:
>
> > > Parse errors: Tests out of sequence. Found (2) but expected (3)
> > > Tests out of sequence. Found (3) but expected (4)
> > > Tests out of sequence. Found (4) but expected (5)
> > > Bad plan. You planned 4 tests but ran 5.
>
> The TAP output from one of our tests should look something like:
>
> ok 1 - subject one
> ok 2 - subject two
> ok 3 - subject three
> ok 4 - subject four
> # passed all 4 test(s)
> 1..4
>
> the "plan" is the bit at the end. That looks like $test_count
> accidentally got incremented by one and we generated something like:
>
> ok 1 - subject one
> ok 3 - subject two
> ok 4 - subject three
> ok 5 - subject four
> 1..4
>
> which would explain the "out of sequence" errors as well as the "planned
> 4 but ran 5".
Hmm, actually the numbering problem is the other way around. It finds 2
but expects 3, so it is more like:
ok 1 - subject one
ok 2 - something else stuck in here!
ok 2 - subject two
...
which gives us a clue. And thanks to TAP auto-numbering, you can also
trigger this like:
ok 1 - subject one
ok
ok 2 - subject two
The "ok" by itself is taken to mean "ok 2". And now I have enough to
generate this locally. t5547 does:
test_commit ok &&
...
git cat-file commit $commit
which will print a line with just "ok" on it. Normally this is not sent
to stdout at all; test output goes to /dev/null unless "--verbose" is
given. When "--verbose" is used, we get all manner of random program
output intermingled with our TAP output, which is an accident waiting to
happen. Usually nobody cares, because they only use "--verbose" when
debugging a test individually (and nothing is parsing the TAP output).
But I can trigger the problem with:
prove t5547-push-quarantine.sh :: -v
The Travis tests do exactly this (along with --tee to actually save the
output). It seems like a minor miracle that this is the first test
output that has actually triggered as TAP input. I'd suggest that the
problem is not in the test, though, but that our "--verbose" option is
unsuitable for using with a TAP harness.
The obvious fix would be to send "--verbose" output to stderr, but I
suspect that would end up annoying for people who do:
./t5547-push-quarantine.sh -v | less
to read long output. Probably we need some option like "--log" which
logs in the same way that "--tee" does, but _without_ sending the data
to stdout. Naively, that just means replacing the "tee" invocation with
"cat", but I suspect it will be a lot more complicated than that,
because we still need to let the TAP output go to stdout.
-Peff
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Jeff King @ 2016-10-21 8:24 UTC (permalink / raw)
To: Johannes Sixt
Cc: Stefan Beller, Johannes Schindelin, Junio C Hamano,
git@vger.kernel.org
In-Reply-To: <4d97fcc7-d15b-e7d8-c787-483c67d4a66b@kdbg.org>
On Fri, Oct 21, 2016 at 07:27:40AM +0200, Johannes Sixt wrote:
> Am 20.10.2016 um 23:38 schrieb Jeff King:
> > test_cmp () {
> > # optimize for common "they are the same" case
> > # without any subshells or subprograms
>
> We do this already on Windows; it's the function mingw_test_cmp in our test
> suite:
> https://github.com/git/git/blob/master/t/test-lib-functions.sh#L884-L945
Ah, thanks, I didn't even think to look for an existing solution like
this. Looks like it was done for LF/CRLF reasons, though, not
performance. I wonder if anybody has measured whether it improves things
there or not.
-Peff
^ permalink raw reply
* Re: Prove "Tests out of sequence" Error
From: Jeff King @ 2016-10-21 8:20 UTC (permalink / raw)
To: Stefan Beller; +Cc: Lars Schneider, git
In-Reply-To: <CAGZ79kZo3LdcRmrjQTAvgx=H6U2tdjASK3qv5A2K5J2HQ2NvSw@mail.gmail.com>
On Thu, Oct 20, 2016 at 11:10:39PM -0700, Stefan Beller wrote:
> > on TravisCI I see these weird "Tests out of sequence" errors with prove
> > and they seem to not go away. I assume the reason that they not go away
> > is that the ".prove" file is carried over from on build to another (but I can't
> > look into this file on TravisCI).
> >
> > Has anyone an idea where these errors might come from?
> >
> > ------------------------------------------------------------------------
> > t5547-push-quarantine.sh (Wstat: 0 Tests: 5 Failed: 0)
>
> push quarantine is a new thing made by Jeff 2 weeks ago, IIRC.
Yes, but I do not see how it can trigger this:
> > Parse errors: Tests out of sequence. Found (2) but expected (3)
> > Tests out of sequence. Found (3) but expected (4)
> > Tests out of sequence. Found (4) but expected (5)
> > Bad plan. You planned 4 tests but ran 5.
The TAP output from one of our tests should look something like:
ok 1 - subject one
ok 2 - subject two
ok 3 - subject three
ok 4 - subject four
# passed all 4 test(s)
1..4
the "plan" is the bit at the end. That looks like $test_count
accidentally got incremented by one and we generated something like:
ok 1 - subject one
ok 3 - subject two
ok 4 - subject three
ok 5 - subject four
1..4
which would explain the "out of sequence" errors as well as the "planned
4 but ran 5".
But I do not see how the test script could screw that up. The counting
is handled entirely by the harness in test-lib.sh.
Nor do I see how a stale .prove file could matter. It does not store
information about the test plan at all. E.g., here is the entry from
mine for t5547:
t5547-push-quarantine.sh:
elapsed: 0.0762169361114502
gen: 1
last_pass_time: 1477037708.741
last_result: 0
last_run_time: 1477037708.741
last_todo: 0
seq: 437
total_passes: 1
Puzzling.
-Peff
^ permalink raw reply
* [BUG] [PATCH]: run-command.c
From: Duncan Roe @ 2016-10-21 5:50 UTC (permalink / raw)
To: git
prepare_shell_cmd() executes /bin/sh with superfluous arguments on all but
single-word shell commands.
For example, if .git/config has this alias (the sleep is to leave time to
examine output from ps, &c.):
[alias]
tryme = "!echo $PWD;sleep 600"
running "git tryme" in one console and checking what it does in another
--- 1st xterm
16:42:12$ git tryme
/usr/src/git/.git
echo $PWD;sleep 600: line 1: 2602 Terminated sleep 600
16:43:15$
--- 2nd xterm
16:42:06$ ps axf|grep -A2 trym[e]
2599 pts/4 S+ 0:00 \_ git tryme
2601 pts/4 S+ 0:00 \_ /bin/sh -c echo $PWD;sleep 600 echo $PWD;sleep 600
2602 pts/4 S+ 0:00 \_ sleep 600
16:42:45$ cat /proc/2601/cmdline | xargs -0 -n1 echo
/bin/sh
-c
echo $PWD;sleep 600
echo $PWD;sleep 600
16:43:04$ kill 2602
16:43:15$
---
There is an extra "-c" argument. This is caused by a missing "else", fixed by
the appended patch,
Cheers ... Duncan.
----------8<-------------------
--- a/run-command.c
+++ b/run-command.c
@@ -182,8 +182,8 @@ static const char **prepare_shell_cmd(struct argv_array *out, const char **argv)
else
argv_array_pushf(out, "%s \"$@\"", argv[0]);
}
-
- argv_array_pushv(out, argv);
+ else
+ argv_array_pushv(out, argv);
return out->argv;
}
^ permalink raw reply
* Re: Prove "Tests out of sequence" Error
From: Stefan Beller @ 2016-10-21 6:10 UTC (permalink / raw)
To: Lars Schneider, Jeff King; +Cc: git
In-Reply-To: <D9C1E13F-88A2-483E-A549-1C2294EACFEB@gmail.com>
On Thu, Oct 20, 2016 at 9:14 PM, Lars Schneider
<larsxschneider@gmail.com> wrote:
> Hi,
>
> on TravisCI I see these weird "Tests out of sequence" errors with prove
> and they seem to not go away. I assume the reason that they not go away
> is that the ".prove" file is carried over from on build to another (but I can't
> look into this file on TravisCI).
>
> Has anyone an idea where these errors might come from?
>
> ------------------------------------------------------------------------
> t5547-push-quarantine.sh (Wstat: 0 Tests: 5 Failed: 0)
push quarantine is a new thing made by Jeff 2 weeks ago, IIRC.
> Parse errors: Tests out of sequence. Found (2) but expected (3)
> Tests out of sequence. Found (3) but expected (4)
> Tests out of sequence. Found (4) but expected (5)
> Bad plan. You planned 4 tests but ran 5.
> Files=760, Tests=15109, 679 wallclock secs (21.91 usr 1.78 sys + 320.79 cusr 529.13 csys = 873.61 CPU)
> Result: FAIL
> make[1]: *** [prove] Error 1
> make: *** [test] Error 2
> ------------------------------------------------------------------------
>
> Example:
> https://s3.amazonaws.com/archive.travis-ci.org/jobs/169385219/log.txt
>
> Thanks,
> Lars
^ permalink raw reply
* Re: Drastic jump in the time required for the test suite
From: Johannes Sixt @ 2016-10-21 5:27 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, Johannes Schindelin, Junio C Hamano,
git@vger.kernel.org
In-Reply-To: <20161020213803.7d7bymby7pouzij3@sigill.intra.peff.net>
Am 20.10.2016 um 23:38 schrieb Jeff King:
> test_cmp () {
> # optimize for common "they are the same" case
> # without any subshells or subprograms
We do this already on Windows; it's the function mingw_test_cmp in our
test suite:
https://github.com/git/git/blob/master/t/test-lib-functions.sh#L884-L945
-- Hannes
^ permalink raw reply
* Prove "Tests out of sequence" Error
From: Lars Schneider @ 2016-10-21 4:14 UTC (permalink / raw)
To: git
Hi,
on TravisCI I see these weird "Tests out of sequence" errors with prove
and they seem to not go away. I assume the reason that they not go away
is that the ".prove" file is carried over from on build to another (but I can't
look into this file on TravisCI).
Has anyone an idea where these errors might come from?
------------------------------------------------------------------------
t5547-push-quarantine.sh (Wstat: 0 Tests: 5 Failed: 0)
Parse errors: Tests out of sequence. Found (2) but expected (3)
Tests out of sequence. Found (3) but expected (4)
Tests out of sequence. Found (4) but expected (5)
Bad plan. You planned 4 tests but ran 5.
Files=760, Tests=15109, 679 wallclock secs (21.91 usr 1.78 sys + 320.79 cusr 529.13 csys = 873.61 CPU)
Result: FAIL
make[1]: *** [prove] Error 1
make: *** [test] Error 2
------------------------------------------------------------------------
Example:
https://s3.amazonaws.com/archive.travis-ci.org/jobs/169385219/log.txt
Thanks,
Lars
^ permalink raw reply
* Re: t9000-addresses.sh: unexpected pases
From: Junio C Hamano @ 2016-10-21 0:50 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Matthieu Moy, GIT Mailing-list
In-Reply-To: <3e9bfa01-d69c-05c1-e24e-960a3add2551@ramsayjones.plus.com>
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> I have started seeing unexpected passes in this test (am I the
> only one?) on the next and pu branch, which seems to be caused
> by commit e3fdbcc8 ("parse_mailboxes: accept extra text after
> <...> address", 13-10-2016). Thus:
>
> $ tail -15 ntest-out
> [15:17:44]
> All tests successful.
>
> Test Summary Report
> -------------------
> t9000-addresses.sh (Wstat: 0 Tests: 37 Failed: 0)
> TODO passed: 28, 30-31
Yeah, I noticed this in some of my integration runs but didn't pay
attention and forgot about it; thanks for bringing it up.
^ permalink raw reply
* Re: [PATCH] doc: fix merge-base ASCII art tab spacing
From: Junio C Hamano @ 2016-10-21 0:48 UTC (permalink / raw)
To: Jeff King; +Cc: Philip Oakley, GitList
In-Reply-To: <20161021002252.u5v3mwjopvsdcexg@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Oct 21, 2016 at 12:40:09AM +0100, Philip Oakley wrote:
>
>> The doc-tool stack does not always respect the 'tab = 8 spaces' rule,
>> particularly the git-scm doc pages https://git-scm.com/docs/git-merge-base
>> and the Git generated html pages.
>
> Hmm, I thought git-scm.com was fixed by:
>
> https://github.com/git/git-scm.com/commit/1e13397edccdecd0e06ff851cffaa1c44e140bf3
>
> Not that I mind using spaces consistently here on principle. I just
> didn't think it was a problem anymore (my asciidoc seems to get it
> right, too).
I do not terribly mind it, either, and I'd prefer to apply Philip's
patch eventually. But I'd rather want to do so _after_ known broken
sites, if there are any, are fixed by correcting their tools.
^ permalink raw reply
* Re: [PATCH] doc: fix merge-base ASCII art tab spacing
From: Junio C Hamano @ 2016-10-21 0:27 UTC (permalink / raw)
To: Philip Oakley; +Cc: GitList
In-Reply-To: <20161020234009.1768-1-philipoakley@iee.org>
Philip Oakley <philipoakley@iee.org> writes:
> The doc-tool stack does not always respect the 'tab = 8 spaces' rule,
> particularly the git-scm doc pages https://git-scm.com/docs/git-merge-base
> and the Git generated html pages.
Sorry, but I do not understand this change.
https://git.github.io/htmldocs/git-merge-base.html is "Git generated
HTML page" and I do not see any breakage around the drawings this
patch touches, and the fp-path series does not touch these drawings,
either.
If a broken "doc-tool stack" breaks the formatting, I'd prefer to
see that "doc-tool stack" fixed, instead of working around by
updating the source they work on. Otherwise, the broken "doc-tool
stack" will keep producing broken output next time a source that
respects "tab is to skip to the next multiple of 8" rule is fed to
it, no?
^ permalink raw reply
* [BUG] fetch output is ugly in 'next'
From: Jeff King @ 2016-10-21 0:26 UTC (permalink / raw)
To: git
I recently started using lt/abbrev-auto via 'next'. This is the fetch
output I saw today:
$ git fetch
remote: Counting objects: 332, done.
remote: Compressing objects: 100% (237/237), done.
remote: Total 332 (delta 182), reused 64 (delta 64), pack-reused 31
Receiving objects: 100% (332/332), 351.53 KiB | 0 bytes/s, done.
Resolving deltas: 100% (184/184), completed with 26 local objects.
From git://github.com/gitster/git
+ fb65135d9c...16ab66ec97 jch -> origin/jch (forced update)
fd47ae6a5b..98985c6911 jk/diff-submodule-diff-inline -> origin/jk/diff-submodule-diff-inline
* [new branch] jk/no-looking-at-dotgit-outside-repo -> origin/jk/no-looking-at-dotgit-outside-repo
+ 3a8f853f16...1369f9b5ba jt/trailer-with-cruft -> origin/jt/trailer-with-cruft (forced update)
* [new branch] pt/gitgui-updates -> origin/pt/gitgui-updates
+ 7594b34cbb...be8e40093b pu -> origin/pu (forced update)
+ 7b95cf9a4e...76e368c378 tg/add-chmod+x-fix -> origin/tg/add-chmod+x-fix (forced update)
+ c4cf1f93cf...221912514c va/i18n-perl-scripts -> origin/va/i18n-perl-scripts (forced update)
* [new branch] yk/git-tag-remove-mention-of-old-layout-in-doc -> origin/yk/git-tag-remove-mention-of-old-layout-in-doc
Yuck. I could maybe get over the wasted space due to the longer sha1s,
but we clearly need to fix the alignment computation. I haven't dug on
it at all; it's probably low-hanging fruit if somebody is interested.
-Peff
^ permalink raw reply
* t9000-addresses.sh: unexpected pases
From: Ramsay Jones @ 2016-10-21 0:24 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Junio C Hamano, GIT Mailing-list
Hi Matthieu,
I have started seeing unexpected passes in this test (am I the
only one?) on the next and pu branch, which seems to be caused
by commit e3fdbcc8 ("parse_mailboxes: accept extra text after
<...> address", 13-10-2016). Thus:
$ tail -15 ntest-out
[15:17:44]
All tests successful.
Test Summary Report
-------------------
t9000-addresses.sh (Wstat: 0 Tests: 37 Failed: 0)
TODO passed: 28, 30-31
Files=760, Tests=13940, 484 wallclock secs ( 4.04 usr 1.30 sys + 60.52 cusr 36.76 csys = 102.62 CPU)
Result: PASS
make clean-except-prove-cache
make[2]: Entering directory '/home/ramsay/git/t'
rm -f -r 'trash directory'.* 'test-results'
rm -f -r valgrind/bin
make[2]: Leaving directory '/home/ramsay/git/t'
make[1]: Leaving directory '/home/ramsay/git/t'
$
I have not even looked, but I suspect that it simply requires
a change from expect_fail to expect_success, since your commit
has 'fixed' these tests ... would you mind taking a quick look?
Thanks!
ATB,
Ramsay Jones
^ 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