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

[Junio C Hamano]

Duy Nguyen  writes:

> On Wed, Feb 14, 2018 at 10:16 AM, Jeff King  wrote:
>> Hmm. That is not too bad, but somehow it feels funny to me to be
>> polluting each test script with these annotations. And to be driving it
>> from inside the test scripts.
>>
>> It seems like:
>>
>>   make SANITIZE=leak test GIT_SKIP_TESTS="$(cat known-leaky)"
>>
>> would be sufficient.
>
> And all new test files are considered leak-free by default? I like that!

Sounds good ;-)

>
>> And updating the list would just be:
>>
>>   # assume we're using prove, which will keep running after failure,
>>   # and will record the results for us to parse (using "--state=").
>>   # Otherwise use "make -k" and grep in t/test-results.
>>   make SANITIZE=leak test
>>   (cd t && prove --dry --state=failed) |
>>   perl -lne '/^(t[0-9]{4})-.*.sh$/ and print $1' |
>>   sort >known-leaky
>>
>> That would update both now-passing and now-failing tests. Presumably
>> we'd keep it checked in, so "git diff" would show you the changes.

Sounds good, too.

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

[Duy Nguyen]

On Wed, Feb 14, 2018 at 10:16 AM, Jeff King  wrote:
> Hmm. That is not too bad, but somehow it feels funny to me to be
> polluting each test script with these annotations. And to be driving it
> from inside the test scripts.
>
> It seems like:
>
>   make SANITIZE=leak test GIT_SKIP_TESTS="$(cat known-leaky)"
>
> would be sufficient.

And all new test files are considered leak-free by default? I like that!

> And updating the list would just be:
>
>   # assume we're using prove, which will keep running after failure,
>   # and will record the results for us to parse (using "--state=").
>   # Otherwise use "make -k" and grep in t/test-results.
>   make SANITIZE=leak test
>   (cd t && prove --dry --state=failed) |
>   perl -lne '/^(t[0-9]{4})-.*.sh$/ and print $1' |
>   sort >known-leaky
>
> That would update both now-passing and now-failing tests. Presumably
> we'd keep it checked in, so "git diff" would show you the changes.
>
> -Peff



-- 
Duy

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

[Jeff King]

On Tue, Feb 13, 2018 at 07:27:58AM +0700, Duy Nguyen wrote:

> > 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< --

Hmm. That is not too bad, but somehow it feels funny to me to be
polluting each test script with these annotations. And to be driving it
from inside the test scripts.

It seems like:

  make SANITIZE=leak test GIT_SKIP_TESTS="$(cat known-leaky)"

would be sufficient. And updating the list would just be:

  # assume we're using prove, which will keep running after failure,
  # and will record the results for us to parse (using "--state=").
  # Otherwise use "make -k" and grep in t/test-results.
  make SANITIZE=leak test
  (cd t && prove --dry --state=failed) |
  perl -lne '/^(t[0-9]{4})-.*.sh$/ and print $1' |
  sort >known-leaky

That would update both now-passing and now-failing tests. Presumably
we'd keep it checked in, so "git diff" would show you the changes.

-Peff

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

[Duy Nguyen]

On Mon, Feb 12, 2018 at 11:15:06PM +0100, Martin Ågren wrote:
> On 12 February 2018 at 10:56, Duy Nguyen  wrote:
> > On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren  wrote:
> >> On 6 February 2018 at 03:13, Jeff King  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 t 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

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

[Martin Ågren]

On 12 February 2018 at 10:56, Duy Nguyen  wrote:
> On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren  wrote:
>> On 6 February 2018 at 03:13, Jeff King  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 t 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. I could try to look into it in the next couple of days.

Martin

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

[Duy Nguyen]

On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren  wrote:
> On 6 February 2018 at 03:13, Jeff King  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 t 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.
-- 
Duy

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

[Martin Ågren]

On 6 February 2018 at 03:13, Jeff King  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 t 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. :-)

> My hope is that people who are interested in
> leak-checking their new code can run some specific script they're
> interested in, and maybe fix up one or two nearby bits while they're
> there (either by fixing a leak, or just annotating via UNLEAK). Then we
> can slowly converge on correctness. :)

Yeah. My experience is that it's easy -- or was for me, anyway -- to
get carried away trying to fix all "related" leaks to the one you
started with, since there are so many dimensions to search in. Two
obvious aspects are "leaks nearby" and "leaks using the same API". This
is not to suggest that the situation is horrible. It's just that if you
pick a leak at random and there is a fair number of "clusters" of
leaks, chances are good you'll find yourself in such a cluster.

Addressing a leak without worrying too much about how it generalizes
would still help. ;-)

Martin

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

[Jeff King]

On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote:

> >> This is still leaking 'worktrees'[1]. You probably want
> >> free_worktrees() immediately after the find_worktree() invocation.
> >
> > Sorry, free_worktrees() after the last use of 'wt' since you still
> > need to access its fields, which would be the end of the function.
> 
> 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 t 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. My hope is that people who are interested in
leak-checking their new code can run some specific script they're
interested in, and maybe fix up one or two nearby bits while they're
there (either by fixing a leak, or just annotating via UNLEAK). Then we
can slowly converge on correctness. :)

-Peff

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

[Duy Nguyen]

On Fri, Feb 2, 2018 at 6:23 PM, Eric Sunshine  wrote:
> On Fri, Feb 2, 2018 at 4:15 AM, Eric Sunshine  wrote:
>> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy  
>> wrote:
>>> +static int move_worktree(int ac, const char **av, const char *prefix)
>>> +{
>>> +   [...]
>>> +   worktrees = get_worktrees(0);
>>> +   wt = find_worktree(worktrees, prefix, av[0]);
>>> +   if (!wt)
>>> +   die(_("'%s' is not a working tree"), av[0]);
>>
>> This is still leaking 'worktrees'[1]. You probably want
>> free_worktrees() immediately after the find_worktree() invocation.
>
> Sorry, free_worktrees() after the last use of 'wt' since you still
> need to access its fields, which would be the end of the function.

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 t and t0001 only..
-- 
Duy

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

[Eric Sunshine]

On Fri, Feb 2, 2018 at 4:15 AM, Eric Sunshine  wrote:
> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy  
> wrote:
>> +static int move_worktree(int ac, const char **av, const char *prefix)
>> +{
>> +   [...]
>> +   worktrees = get_worktrees(0);
>> +   wt = find_worktree(worktrees, prefix, av[0]);
>> +   if (!wt)
>> +   die(_("'%s' is not a working tree"), av[0]);
>
> This is still leaking 'worktrees'[1]. You probably want
> free_worktrees() immediately after the find_worktree() invocation.

Sorry, free_worktrees() after the last use of 'wt' since you still
need to access its fields, which would be the end of the function.

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

[Eric Sunshine]

On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy  wrote:
> This command allows to relocate linked worktrees. Main worktree cannot
> (yet) be moved.
> [...]
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -79,6 +80,11 @@ files from being pruned automatically. This also prevents 
> it from
> +move::
> +
> +Move a working tree to a new location. Note that the main working tree
> +cannot be moved.
> +
> @@ -281,7 +287,6 @@ performed manually, such as:
>  - `remove` to remove a linked working tree and its administrative files (and
>warn if the working tree is dirty)
> -- `mv` to move or rename a working tree and update its administrative files

A couple other places in this document ought to be updated.
Specifically, in DESCRIPTION:

If you move a linked working tree, you need to manually update the
administrative files so that they do not get pruned automatically.
See section "DETAILS" for more information.

which can probably be dropped in its entirety. And, in DETAILS:

If you move a linked working tree, you need to update the 'gitdir'
file...

should probably be reworded to

If you _manually_ move a linked working tree, you need to update
the 'gitdir' file...

> diff --git a/builtin/worktree.c b/builtin/worktree.c
> @@ -605,6 +606,53 @@ static int unlock_worktree(int ac, const char **av, 
> const char *prefix)
> +static int move_worktree(int ac, const char **av, const char *prefix)
> +{
> +   [...]
> +   worktrees = get_worktrees(0);
> +   wt = find_worktree(worktrees, prefix, av[0]);
> +   if (!wt)
> +   die(_("'%s' is not a working tree"), av[0]);

This is still leaking 'worktrees'[1]. You probably want
free_worktrees() immediately after the find_worktree() invocation.

> +   if (is_main_worktree(wt))
> +   die(_("'%s' is a main working tree"), av[0]);
> +   reason = is_worktree_locked(wt);
> +   if (reason) {
> +   if (*reason)
> +   die(_("cannot move a locked working tree, lock 
> reason: %s"),
> +   reason);
> +   die(_("cannot move a locked working tree"));
> +   }
> +   if (validate_worktree(wt, ))
> +   die(_("validation failed, cannot move working tree:\n%s"),
> +   errmsg.buf);

Minor: All the other error messages are presented on a single line.
Despite this error message potentially being quite long, it worries me
a bit that presenting it on two lines could complicate scripted
clients if they need to collect the error message for special
handling.

> diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
> @@ -59,4 +59,35 @@ test_expect_success 'unlock worktree twice' '
> +test_expect_success 'move locked worktree' '
> +   git worktree lock source &&
> +   test_must_fail git worktree move source destination &&
> +   git worktree unlock source
> +'

Also from [1], wrapping 'unlock' inside a test_when_finished()
invocation seems potentially desirable.

> +test_expect_success 'move worktree' '
> +   toplevel="$(pwd)" &&
> +   git worktree move source destination &&
> +   test_path_is_missing source &&
> +   git worktree list --porcelain | grep "^worktree" >actual &&
> +   cat <<-EOF >expected &&
> +   worktree $toplevel
> +   worktree $toplevel/destination
> +   worktree $toplevel/elsewhere
> +   EOF
> +   test_cmp expected actual &&

This seems somewhat fragile. If someone inserts a test before this one
which adds another worktree, then this test would break. Perhaps the
filtering of the --porcelain output could be more strict? (Not
necessarily worth a re-roll, though.)

> +   git -C destination log --format=%s >actual2 &&
> +   echo init >expected2 &&
> +   test_cmp expected2 actual2
> +'

[1]: 
https://public-inbox.org/git/CAPig+cRqgzB4tiTb=fhufbtqo3qegm3m378pvose06kdrek...@mail.gmail.com/