From: John Keeping <john@keeping.me.uk>
To: Jeff King <peff@peff.net>
Cc: Junio C Hamano <gitster@pobox.com>,
Thomas Rast <trast@inf.ethz.ch>,
Ramkumar Ramachandra <artagnon@gmail.com>,
Linus Torvalds <torvalds@linux-foundation.org>,
Git Mailing List <git@vger.kernel.org>
Subject: Re: "git grep" parallelism question
Date: Tue, 30 Apr 2013 17:12:08 +0100 [thread overview]
Message-ID: <20130430161207.GQ472@serenity.lan> (raw)
In-Reply-To: <20130430155939.GA31881@sigill.intra.peff.net>
On Tue, Apr 30, 2013 at 11:59:39AM -0400, Jeff King wrote:
> On Tue, Apr 30, 2013 at 09:08:49AM +0100, John Keeping wrote:
>
> > > With your patch, doesn't "tXXXX-*.sh --root $there" automatically
> > > use the fast $there temporary location as the result depot, too?
> >
> > No, the current code uses:
> >
> > $TEST_OUTPUT_DIRECTORY/$root/trash\ directory.tXXXX
> >
> > where we don't prepend $TEST_OUTPUT_DIRECTORY/ if $root is absolute.
> >
> > > If it doesn't with the current code, shouldn't it?
> >
> > I think the current behaviour is fine and the two options complement
> > each other.
> >
> > TEST_OUTPUT_DIRECTORY is something you set once and forget about which
> > says "all of the test output should go over here", whereas --root is
> > passed to a specific test and says "put your output here" but does not
> > affect the result aggregation which is not specific to that test.
>
> The original intent of "--root" (and how I use it) is to set and forget
> it, too, via GIT_TEST_OPTS. I intentionally didn't move test results
> with it, because to me the point was a pure optimization: put the trash
> directories on a faster disk, and leave everything else identical. With
> "--root", any scripts which later want to look at test-results will find
> them in the usual place.
>
> Your patch updates all of the in-tree spots which look at the results,
> but any third-party scripts would need to take it into account, too
> (though I have no idea if any such scripts even exist).
>
> I'm curious if there is a good reason to want to move the results. Some
> possibilities I can think of are:
>
> 1. More optimization, as results are written to the faster filesystem.
> I doubt this is noticeable, though, as the amount of data written
> is relatively small compared to the tests themselves (which are
> constantly creating and deleting repos).
>
> 2. You can run tests in a read-only git checkout. I'm not sure how
> useful that is, though, since you would already need to compile
> git.
>
> 3. You could have multiple sets of test results to keep or compare.
> I'd think you'd want to keep the built versions of git around, too,
> though. Which would mean that a full checkout like git-new-workdir
> would be a much simpler way to accomplish the same thing.
>
> So I'm not against TEST_OUTPUT_DIRECTORY as a concept, but I'm having
> trouble seeing how it is more useful than "--root".
I think the original intent of TEST_OUTPUT_DIRECTORY was to allow other
users of the test framework (in contrib/ or the performance tests) to
put their output in a sensible place for those tests, like you describe
below.
The patch being discussed here [1] just makes sure that it applies
to everything - previously it was applied to test-results/
inconsistently; test-lib.sh used TEST_OUTPUT_DIRECTORY but the makefile
didn't. So we haven't actually changed where test-results/ live as a
result of this change, just where the makefile looks in order to display
the aggregate results and clean them up.
> > Note that setting TEST_OUTPUT_DIRECTORY in config.mak affects all tests
> > no matter how you run them (via make or as ./tXXXX-yyyy.sh) whereas
> > setting --root=... in GIT_TEST_OPTS only affect tests run via make.
>
> I actually consider that a feature of "--root". When I run "make test"
> everything happens fast. When I run the script manually (which is
> usually because I'm debugging), the trash directory appears in the
> current directory, so I can easily investigate it. And if you are
> running a single test, the performance impact is usually negligible
> (where you really notice it is when running "make -j32 test").
This confirms to me that the patch as it currently stands is correct: we
have made TEST_OUTPUT_DIRECTORY consistent and --root still works as
before.
[1] http://article.gmane.org/gmane.comp.version-control.git/222555
next prev parent reply other threads:[~2013-04-30 16:12 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-26 17:31 "git grep" parallelism question Linus Torvalds
2013-04-26 18:47 ` Junio C Hamano
2013-04-26 18:54 ` Linus Torvalds
2013-04-26 19:19 ` Junio C Hamano
2013-04-26 20:31 ` Linus Torvalds
2013-04-27 13:46 ` Thomas Rast
2013-04-29 14:05 ` Ramkumar Ramachandra
2013-04-29 16:18 ` John Keeping
2013-04-29 18:04 ` Thomas Rast
2013-04-29 18:08 ` John Keeping
2013-04-29 22:22 ` Junio C Hamano
2013-04-30 8:08 ` John Keeping
2013-04-30 15:59 ` Jeff King
2013-04-30 16:12 ` John Keeping [this message]
2013-04-30 16:14 ` Jeff King
2013-05-05 15:40 ` Pete Wyckoff
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130430161207.GQ472@serenity.lan \
--to=john@keeping.me.uk \
--cc=artagnon@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=torvalds@linux-foundation.org \
--cc=trast@inf.ethz.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).