unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [PATCH] Repeatability when copying a whole directory into a new one.
@ 2011-09-29 23:26 Thomas Schwinge
  2011-11-01  2:59 ` David Bremner
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Thomas Schwinge @ 2011-09-29 23:26 UTC (permalink / raw)
  To: notmuch; +Cc: 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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] Repeatability when copying a whole directory into a new one.
  2011-09-29 23:26 [PATCH] Repeatability when copying a whole directory into a new one Thomas Schwinge
@ 2011-11-01  2:59 ` David Bremner
  2011-11-03 16:49   ` Pieter Praet
  2011-11-05 17:26 ` Austin Clements
  2011-12-18 13:08 ` Tomi Ollila
  2 siblings, 1 reply; 6+ messages in thread
From: David Bremner @ 2011-11-01  2:59 UTC (permalink / raw)
  To: Thomas Schwinge; +Cc: notmuch

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Repeatability when copying a whole directory into a new one.
  2011-11-01  2:59 ` David Bremner
@ 2011-11-03 16:49   ` Pieter Praet
  0 siblings, 0 replies; 6+ messages in thread
From: Pieter Praet @ 2011-11-03 16:49 UTC (permalink / raw)
  To: David Bremner, Thomas Schwinge; +Cc: notmuch

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] Repeatability when copying a whole directory into a new one.
  2011-09-29 23:26 [PATCH] Repeatability when copying a whole directory into a new one Thomas Schwinge
  2011-11-01  2:59 ` David Bremner
@ 2011-11-05 17:26 ` Austin Clements
  2016-04-13 19:50   ` Jani Nikula
  2011-12-18 13:08 ` Tomi Ollila
  2 siblings, 1 reply; 6+ messages in thread
From: Austin Clements @ 2011-11-05 17:26 UTC (permalink / raw)
  To: Thomas Schwinge; +Cc: notmuch

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Repeatability when copying a whole directory into a new one.
  2011-09-29 23:26 [PATCH] Repeatability when copying a whole directory into a new one Thomas Schwinge
  2011-11-01  2:59 ` David Bremner
  2011-11-05 17:26 ` Austin Clements
@ 2011-12-18 13:08 ` Tomi Ollila
  2 siblings, 0 replies; 6+ messages in thread
From: Tomi Ollila @ 2011-12-18 13:08 UTC (permalink / raw)
  To: Thomas Schwinge, notmuch

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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Repeatability when copying a whole directory into a new one.
  2011-11-05 17:26 ` Austin Clements
@ 2016-04-13 19:50   ` Jani Nikula
  0 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2016-04-13 19:50 UTC (permalink / raw)
  To: Austin Clements, Thomas Schwinge; +Cc: notmuch

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-04-13 19:51 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-29 23:26 [PATCH] Repeatability when copying a whole directory into a new one Thomas Schwinge
2011-11-01  2:59 ` David Bremner
2011-11-03 16:49   ` Pieter Praet
2011-11-05 17:26 ` Austin Clements
2016-04-13 19:50   ` Jani Nikula
2011-12-18 13:08 ` Tomi Ollila

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

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 read-only IMAP folder(s) and NNTP newsgroup(s).