Re: [PATCH 3/7] worktree move: new command

[Duy Nguyen <pclouds@gmail.com>]

On Mon, Feb 12, 2018 at 11:15:06PM +0100, Martin Ågren wrote:
> On 12 February 2018 at 10:56, Duy Nguyen <pclouds@gmail.com> wrote:
> > On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren <martin.agren@gmail.com> wrote:
> >> On 6 February 2018 at 03:13, Jeff King <peff@peff.net> wrote:
> >>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:
> >>>> I learned SANITIZE=leak today! It not only catches this but also "dst".
> >>>>
> >>>> Jeff is there any ongoing effort to make the test suite pass with
> >>>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test
> >>>> suite and saw so many failures. I did see in your original mails that
> >>>> you focused on t0000 and t0001 only..
> >>>
> >>> Yeah, I did those two scripts to try to prove to myself that the
> >>> approach was good. But I haven't really pushed it any further.
> >>>
> >>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a
> >>> long way to go.
> >>
> >> Agreed. :-)
> >
> > Should we mark the test files that pass SANITIZE=leak and run as a sub
> > testsuite? This way we can start growing it until it covers everything
> > (unlikely) and make sure people don't break what's already passed.
> >
> > Of course I don't expect everybody to run this new make target with
> > SANITIZE=leak. Travis can do that for us and hopefully have some way
> > to tell git@vger about new breakages.
> 
> Makes sense to try to make sure that we don't regress leak-free tests. I
> don't know what our Travis-budget looks like, but I would volunteer to
> run something like this periodically using my own cycles.
> 
> My experience with the innards of our Makefiles and test-lib.sh is
> non-existant, but from a very cursory look it seems like something as
> simple as loading GIT_SKIP_TESTS from a blacklist-file might do the
> trick.

Yeah my first throught was something along that line too. But maybe
it's nicer to mark a test file leak-free like this:

-- 8< --
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 459f676683..1446f075b7 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -2,6 +2,8 @@
 
 test_description='test git worktree move, remove, lock and unlock'
 
+test_leak_free=yes
+
 . ./test-lib.sh
 
 test_expect_success 'setup' '
-- 8< --

And we can run these test files with a new option --leak-check (or
something like that, we already support a similar option --valgrind).

Most of the work will be in test-lib.sh. If we detect --leak-check and
test_leak_free is not set, we skip the whole file. In the far far far
future when all files have test_leak_free=yes, we can flip the default
and delete these lines.

It would be nice if test-lib.sh can determine if 'git' binary is built
with SANITIZE=yes, but I think we can live without it.

> I could try to look into it in the next couple of days.

Have fun :) Let me know if you give up though, I'll give it a shot.
--
Duy