[PATCH] Repeatability when copying a whole directory into a new one.

[PATCH] Repeatability when copying a whole directory into a new one.

[Thomas Schwinge]

This new test currently fails -- but it shouldn't.
---

Hi!

I found this while manually copying directories and running notmuch new.

Am I just too sleepy at this time, or is it another DB vs. directory
mtime issue?

     BROKEN Repeatability when copying a whole directory into a new one
            --- new.18.expected     2011-09-29 23:23:39.000000000 +0000
            +++ new.18.output       2011-09-29 23:23:39.000000000 +0000
            @@ -1,2 +1 @@
            -Processed 51 total files in almost no time.
             No new mail.


Grüße,
 Thomas

---
 test/new |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/test/new b/test/new
index 49f390d..0afb04c 100755
--- a/test/new
+++ b/test/new
@@ -153,4 +153,25 @@ rm -rf "${MAIL_DIR}"/two
 output=$(NOTMUCH_NEW)
 test_expect_equal "$output" "No new mail. Removed 3 messages."
 
+
+test_begin_subtest 'Repeatability when copying a whole directory into a new one'
+
+add_email_corpus
+
+mkdir "$MAIL_DIR"/2nd
+cp -a "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
+output1=$(notmuch new)
+
+rm -rf "$MAIL_DIR"/2nd
+notmuch new > /dev/null
+
+# This was quite enjoyable.  Let's do it again.
+mkdir "$MAIL_DIR"/2nd
+cp -a "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
+output2=$(notmuch new)
+
+test_subtest_known_broken
+test_expect_equal "$output2" "$output1"
+
+
 test_done
-- 
1.7.5.4

Re: [PATCH] Repeatability when copying a whole directory into a new one.

[David Bremner]

On Fri, 30 Sep 2011 01:26:46 +0200, Thomas Schwinge <thomas@schwinge.name> wrote:
> This new test currently fails -- but it shouldn't.
> ---
> 
> Hi!
> 
> I found this while manually copying directories and running notmuch new.
> 
> Am I just too sleepy at this time, or is it another DB vs. directory
> mtime issue?
> 
>      BROKEN Repeatability when copying a whole directory into a new one
>             --- new.18.expected     2011-09-29 23:23:39.000000000 +0000
>             +++ new.18.output       2011-09-29 23:23:39.000000000 +0000
>             @@ -1,2 +1 @@
>             -Processed 51 total files in almost no time.
>              No new mail.

I'm a bit confused here too. When the files are removed, the "notmuch new"
sent to /dev/null in your test detects the deletes as renames. Shouldn't
the copies be detected as duplicates or something?

d

Re: [PATCH] Repeatability when copying a whole directory into a new one.

[Pieter Praet]

[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]

On Mon, 31 Oct 2011 23:59:49 -0300, David Bremner <david@tethera.net> wrote:
> On Fri, 30 Sep 2011 01:26:46 +0200, Thomas Schwinge <thomas@schwinge.name> wrote:
> > This new test currently fails -- but it shouldn't.
> > ---
> > 
> > Hi!
> > 
> > I found this while manually copying directories and running notmuch new.
> > 
> > Am I just too sleepy at this time, or is it another DB vs. directory
> > mtime issue?
> > 
> >      BROKEN Repeatability when copying a whole directory into a new one
> >             --- new.18.expected     2011-09-29 23:23:39.000000000 +0000
> >             +++ new.18.output       2011-09-29 23:23:39.000000000 +0000
> >             @@ -1,2 +1 @@
> >             -Processed 51 total files in almost no time.
> >              No new mail.
> 
> I'm a bit confused here too. When the files are removed, the "notmuch new"
> sent to /dev/null in your test detects the deletes as renames. Shouldn't
> the copies be detected as duplicates or something?
> 

They should.

Applying any one of the attached patches makes the test pass;
I vote "directory mtime issue".



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: update-dir-mtime.patch --]
[-- Type: text/x-patch, Size: 329 bytes --]

diff --git a/test/new b/test/new
index 0afb04c..32b058f 100755
--- a/test/new
+++ b/test/new
@@ -168,6 +168,7 @@ notmuch new > /dev/null
 # This was quite enjoyable.  Let's do it again.
 mkdir "$MAIL_DIR"/2nd
 cp -a "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
+touch "$MAIL_DIR"/2nd/cur
 output2=$(notmuch new)
 
 test_subtest_known_broken

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: cp-without-archive-opt.patch --]
[-- Type: text/x-patch, Size: 612 bytes --]

diff --git a/test/new b/test/new
index 0afb04c..0627d69 100755
--- a/test/new
+++ b/test/new
@@ -159,7 +159,7 @@ test_begin_subtest 'Repeatability when copying a whole directory into a new one'
 add_email_corpus
 
 mkdir "$MAIL_DIR"/2nd
-cp -a "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
+cp "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
 output1=$(notmuch new)
 
 rm -rf "$MAIL_DIR"/2nd
@@ -167,7 +167,7 @@ notmuch new > /dev/null
 
 # This was quite enjoyable.  Let's do it again.
 mkdir "$MAIL_DIR"/2nd
-cp -a "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
+cp "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
 output2=$(notmuch new)
 
 test_subtest_known_broken

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: cp-to-other-dir.patch --]
[-- Type: text/x-patch, Size: 391 bytes --]

diff --git a/test/new b/test/new
index 0afb04c..a3c270c 100755
--- a/test/new
+++ b/test/new
@@ -166,8 +166,8 @@ rm -rf "$MAIL_DIR"/2nd
 notmuch new > /dev/null
 
 # This was quite enjoyable.  Let's do it again.
-mkdir "$MAIL_DIR"/2nd
-cp -a "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
+mkdir "$MAIL_DIR"/3rd
+cp -a "$MAIL_DIR"/cur "$MAIL_DIR"/3rd/
 output2=$(notmuch new)
 
 test_subtest_known_broken

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: cp-dir-content.patch --]
[-- Type: text/x-patch, Size: 622 bytes --]

diff --git a/test/new b/test/new
index 0afb04c..f600983 100755
--- a/test/new
+++ b/test/new
@@ -159,7 +159,7 @@ test_begin_subtest 'Repeatability when copying a whole directory into a new one'
 add_email_corpus
 
 mkdir "$MAIL_DIR"/2nd
-cp -a "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
+cp -a "$MAIL_DIR"/cur/* "$MAIL_DIR"/2nd/
 output1=$(notmuch new)
 
 rm -rf "$MAIL_DIR"/2nd
@@ -167,7 +167,7 @@ notmuch new > /dev/null
 
 # This was quite enjoyable.  Let's do it again.
 mkdir "$MAIL_DIR"/2nd
-cp -a "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
+cp -a "$MAIL_DIR"/cur/* "$MAIL_DIR"/2nd/
 output2=$(notmuch new)
 
 test_subtest_known_broken

[-- Attachment #6: Type: text/plain, Size: 175 bytes --]



> d
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch


Peace

-- 
Pieter

Re: [PATCH] Repeatability when copying a whole directory into a new one.

[Austin Clements]

On Thu, Sep 29, 2011 at 7:26 PM, Thomas Schwinge <thomas@schwinge.name> wrote:
> This new test currently fails -- but it shouldn't.
> ---
>
> Hi!
>
> I found this while manually copying directories and running notmuch new.
>
> Am I just too sleepy at this time, or is it another DB vs. directory
> mtime issue?

Nice catch.  I haven't verified this, but I believe the problem is
that notmuch never deletes directory documents.  In fact, there isn't
even an API to do so.  Even though it detects the deleted directory
and deletes all messages under it, the directory document sticks
around.  When the directory comes back, notmuch finds the old
directory document with the old directory mtime and thinks it doesn't
have to rescan the directory because the cp -a reproduced the same
mtime the directory used to have.

So, I guess part of the answer is "don't cp -a" because that mucks
with mtimes and mtimes are all notmuch has to go by.  But that's no
excuse for not removing the directory documents when the directory is
removed.

Fixing this is slightly tricky.  I feel like there *shouldn't* be an
API to simply remove a directory document because that would let you
violate database consistency.  Instead, I think the API should
recursively remove everything under the removed directory, exactly
like what notmuch-new.c:_remove_directory does right now (plus
removing the directory documents).  But _remove_directory depends on
remove_filename, which currently has notmuch-new-specific logic in it.
 I feel like there must be a nice solution to this, and I'm just not
thinking of it.

Re: [PATCH] Repeatability when copying a whole directory into a new one.

[Tomi Ollila]

On Fri, 30 Sep 2011 01:26:46 +0200, Thomas Schwinge <thomas@schwinge.name> wrote:
> This new test currently fails -- but it shouldn't.
> ---
> 
> Hi!
> 
> I found this while manually copying directories and running notmuch new.
> 
> Am I just too sleepy at this time, or is it another DB vs. directory
> mtime issue?

I played a bit with this... making directories early and touching and...

... and always got the same output as below.

>      BROKEN Repeatability when copying a whole directory into a new one
>             --- new.18.expected     2011-09-29 23:23:39.000000000 +0000
>             +++ new.18.output       2011-09-29 23:23:39.000000000 +0000
>             @@ -1,2 +1 @@
>             -Processed 51 total files in almost no time.
>              No new mail.

Then, finally, the following line changes

+output1=$(notmuch new; notmuch search --format=files '*')
...
+output2=$(notmuch new; notmuch search --format=files '*')

we see this

 BROKEN Repeatability when copying a whole directory into a new one
        --- new.18.expected     2011-12-18 13:01:47.000000000 +0000
        +++ new.18.output       2011-12-18 13:01:47.000000000 +0000
        @@ -1,104 +1,52 @@
        -Processed 51 total files in almost no time.
         No new mail.
         /home/too/vc/ext/notmuch/test/tmp.new/mail/cur/50:2,
        -/home/too/vc/ext/notmuch/test/tmp.new/mail/2nd/cur/50:2,
         /home/too/vc/ext/notmuch/test/tmp.new/mail/cur/49:2,
        -/home/too/vc/ext/notmuch/test/tmp.new/mail/2nd/cur/49:2,
         /home/too/vc/ext/notmuch/test/tmp.new/mail/cur/48:2,
        -/home/too/vc/ext/notmuch/test/tmp.new/mail/2nd/cur/48:2,
...


We see that after second time the files are not added to the message
(again). 

I cannot see that the issue is with directory mtimes, but perhaps that
the directory .../2nd/... is not forgotten...

... but whatever the reason will eventually be I suggest to make
this change to the test -- it provides more concrete information
to anyone who will be looking fix for this case.

> 
> 
> Grüße,
>  Thomas

Tomi

> 
> ---
>  test/new |   21 +++++++++++++++++++++
>  1 files changed, 21 insertions(+), 0 deletions(-)
> 
> diff --git a/test/new b/test/new
> index 49f390d..0afb04c 100755
> --- a/test/new
> +++ b/test/new
> @@ -153,4 +153,25 @@ rm -rf "${MAIL_DIR}"/two
>  output=$(NOTMUCH_NEW)
>  test_expect_equal "$output" "No new mail. Removed 3 messages."
>  
> +
> +test_begin_subtest 'Repeatability when copying a whole directory into a new one'
> +
> +add_email_corpus
> +
> +mkdir "$MAIL_DIR"/2nd
> +cp -a "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
> +output1=$(notmuch new)
> +
> +rm -rf "$MAIL_DIR"/2nd
> +notmuch new > /dev/null
> +
> +# This was quite enjoyable.  Let's do it again.
> +mkdir "$MAIL_DIR"/2nd
> +cp -a "$MAIL_DIR"/cur "$MAIL_DIR"/2nd/
> +output2=$(notmuch new)
> +
> +test_subtest_known_broken
> +test_expect_equal "$output2" "$output1"
> +
> +
>  test_done
> -- 
> 1.7.5.4
> 

Re: [PATCH] Repeatability when copying a whole directory into a new one.

[Jani Nikula]

On Sat, 05 Nov 2011, Austin Clements <amdragon@mit.edu> wrote:
> On Thu, Sep 29, 2011 at 7:26 PM, Thomas Schwinge <thomas@schwinge.name> wrote:
>> This new test currently fails -- but it shouldn't.
>> ---
>>
>> Hi!
>>
>> I found this while manually copying directories and running notmuch new.
>>
>> Am I just too sleepy at this time, or is it another DB vs. directory
>> mtime issue?
>
> Nice catch.  I haven't verified this, but I believe the problem is
> that notmuch never deletes directory documents.  In fact, there isn't
> even an API to do so.  Even though it detects the deleted directory
> and deletes all messages under it, the directory document sticks
> around.  When the directory comes back, notmuch finds the old
> directory document with the old directory mtime and thinks it doesn't
> have to rescan the directory because the cp -a reproduced the same
> mtime the directory used to have.
>
> So, I guess part of the answer is "don't cp -a" because that mucks
> with mtimes and mtimes are all notmuch has to go by.  But that's no
> excuse for not removing the directory documents when the directory is
> removed.
>
> Fixing this is slightly tricky.  I feel like there *shouldn't* be an
> API to simply remove a directory document because that would let you
> violate database consistency.  Instead, I think the API should
> recursively remove everything under the removed directory, exactly
> like what notmuch-new.c:_remove_directory does right now (plus
> removing the directory documents).  But _remove_directory depends on
> remove_filename, which currently has notmuch-new-specific logic in it.
>  I feel like there must be a nice solution to this, and I'm just not
> thinking of it.

Stumbled upon this one while checking old bug reports. Maybe the fix in

commit e26d99dc7b02f33299c281f97a13deaef802bc7a
Author: Jani Nikula <jani@nikula.org>
Date:   Fri Sep 25 23:48:46 2015 +0300

    cli: delete directory documents on directory removal

is inelegant, as it adds the API to remove directory document, but it's
there now. I was unaware of this bug report and your analysis at the
time.

BR,
Jani.