[PATCH] notmuch restore --accumulate

[Thomas Schwinge]

Hi!

On Fri, 9 Sep 2011 12:13:06 -0400, Austin Clements  wrote:
> The idea behind sending the test first is that people can see that it fails
> and that the subsequent patch indeed fixes it.  What I find works well is to
> submit the test case with the test marked as broken and then the main patch,
> including the change to un-mark it as broken.

Ah, that's indeed a good approach for bug fixes (and it also preserves
git bisect compatibility), but still: why separate patches for new
functionality?  (I'm not trying to be a pain here, but would like to
understand your rationale behind this.)


Gr??e,
 Thomas
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 489 bytes
Desc: not available
URL: 

[PATCH] notmuch restore --accumulate

[Austin Clements]

Quoth Thomas Schwinge on Sep 09 at  7:22 pm:
> Hi!
> 
> On Fri, 9 Sep 2011 12:13:06 -0400, Austin Clements  
> wrote:
> > The idea behind sending the test first is that people can see that it fails
> > and that the subsequent patch indeed fixes it.  What I find works well is to
> > submit the test case with the test marked as broken and then the main patch,
> > including the change to un-mark it as broken.
> 
> Ah, that's indeed a good approach for bug fixes (and it also preserves
> git bisect compatibility), but still: why separate patches for new
> functionality?  (I'm not trying to be a pain here, but would like to
> understand your rationale behind this.)

*shrugs* I don't think it's too important for new functionality,
though for review it's nice to be able to focus on just the
implementation or just the tests, rather than having the patches
intermingled.

[PATCH] notmuch restore --accumulate

[Austin Clements]

The idea behind sending the test first is that people can see that it fails
and that the subsequent patch indeed fixes it.  What I find works well is to
submit the test case with the test marked as broken and then the main patch,
including the change to un-mark it as broken.
On Sep 9, 2011 5:45 AM, "Thomas Schwinge"  wrote:
> Hi!
>
> On Fri, 9 Sep 2011 11:06:34 +0200, Louis Rilling 
wrote:
>> On 05/09/11 12:31 -0700, Jameson Graef Rollins wrote:
>> > Also, we generally prefer to have modifications to the test suite in
>> > separate patches that precede the patches that add the features/fix the
>> > bugs.
>> >
>>
>> For new features, this does not look like 'git bisect'-safe.
>
> Exactly my thoughts. I can perhaps see the usefulness (for first
> separately committing the testcase) for bugfixes, but why for new
> features?
>
>> I would say that
>> the testsuite patch should follow the new feature patch, don't you?
>
> I would keep them together; why separate them?
>
>
> Gr??e,
> Thomas
-- next part --
An HTML attachment was scrubbed...
URL: 

[PATCH] notmuch restore --accumulate

[Jameson Graef Rollins]

On Fri, 09 Sep 2011 19:22:49 +0200, Thomas Schwinge  
wrote:
> Ah, that's indeed a good approach for bug fixes (and it also preserves
> git bisect compatibility), but still: why separate patches for new
> functionality?  (I'm not trying to be a pain here, but would like to
> understand your rationale behind this.)

I tend to think of the test as an actual spec of the behavior I'm trying
to implement.  By defining before hand exactly what I expect to happen I
can confirm that my patch achieves what I want it to.

As an example, you might look at my patch that adds better rfc822 part
handling [0].  I tried to fully flesh out what I wanted to happen in the
test first, and then modified the code to achieve that result.

jamie.

[0] id:"1307320169-29905-4-git-send-email-jrollins at finestructure.net"
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: 

[PATCH] notmuch restore --accumulate

[Thomas Schwinge]

Hi!

On Fri, 9 Sep 2011 11:06:34 +0200, Louis Rilling  wrote:
> On 05/09/11 12:31 -0700, Jameson Graef Rollins wrote:
> > Also, we generally prefer to have modifications to the test suite in
> > separate patches that precede the patches that add the features/fix the
> > bugs.
> > 
> 
> For new features, this does not look like 'git bisect'-safe.

Exactly my thoughts.  I can perhaps see the usefulness (for first
separately committing the testcase) for bugfixes, but why for new
features?

> I would say that
> the testsuite patch should follow the new feature patch, don't you?

I would keep them together; why separate them?


Gr??e,
 Thomas
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 489 bytes
Desc: not available
URL: 

[PATCH] notmuch restore --accumulate

[Louis Rilling]

On 05/09/11 12:31 -0700, Jameson Graef Rollins wrote:
> Also, we generally prefer to have modifications to the test suite in
> separate patches that precede the patches that add the features/fix the
> bugs.
> 

For new features, this does not look like 'git bisect'-safe. I would say that
the testsuite patch should follow the new feature patch, don't you?

Thanks,

Louis

> > Signed-off-by: Thomas Schwinge 
> 
> Finally, you don't need to sign off on your own patches.
> 
> Thanks.
> 
> jamie.



> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] notmuch restore --accumulate

[Louis Rilling]

On 05/09/11 12:31 -0700, Jameson Graef Rollins wrote:
 Also, we generally prefer to have modifications to the test suite in
 separate patches that precede the patches that add the features/fix the
 bugs.
 

For new features, this does not look like 'git bisect'-safe. I would say that
the testsuite patch should follow the new feature patch, don't you?

Thanks,

Louis

  Signed-off-by: Thomas Schwinge tho...@schwinge.name
 
 Finally, you don't need to sign off on your own patches.
 
 Thanks.
 
 jamie.



 ___
 notmuch mailing list
 notmuch@notmuchmail.org
 http://notmuchmail.org/mailman/listinfo/notmuch

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] notmuch restore --accumulate

[Thomas Schwinge]

Hi!

On Fri, 9 Sep 2011 11:06:34 +0200, Louis Rilling l.rill...@av7.net wrote:
 On 05/09/11 12:31 -0700, Jameson Graef Rollins wrote:
  Also, we generally prefer to have modifications to the test suite in
  separate patches that precede the patches that add the features/fix the
  bugs.
  
 
 For new features, this does not look like 'git bisect'-safe.

Exactly my thoughts.  I can perhaps see the usefulness (for first
separately committing the testcase) for bugfixes, but why for new
features?

 I would say that
 the testsuite patch should follow the new feature patch, don't you?

I would keep them together; why separate them?


Grüße,
 Thomas


pgp7NEw98lD7v.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH] notmuch restore --accumulate

[Jameson Graef Rollins]

On Fri, 09 Sep 2011 19:22:49 +0200, Thomas Schwinge tho...@schwinge.name 
wrote:
 Ah, that's indeed a good approach for bug fixes (and it also preserves
 git bisect compatibility), but still: why separate patches for new
 functionality?  (I'm not trying to be a pain here, but would like to
 understand your rationale behind this.)

I tend to think of the test as an actual spec of the behavior I'm trying
to implement.  By defining before hand exactly what I expect to happen I
can confirm that my patch achieves what I want it to.

As an example, you might look at my patch that adds better rfc822 part
handling [0].  I tried to fully flesh out what I wanted to happen in the
test first, and then modified the code to achieve that result.

jamie.

[0] id:1307320169-29905-4-git-send-email-jroll...@finestructure.net


pgppsztYIsv7G.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH] notmuch restore --accumulate

[Thomas Schwinge]

From: Thomas Schwinge 

Also enhance the dump-restore testsuite, and make it generally more
failure-proof.

Signed-off-by: Thomas Schwinge 

---

Hi!

Beware that I have not yet used this new functionality in the wild.  ;-)
(But I do plan to do so, soon.)  And, I think that the testsuite
enhancements cover quite a number of real-world scenarios.


Gr??e,
 Thomas

---

 NEWS  |   13 ++
 notmuch-restore.c |   42 ++---
 notmuch.1 |   14 ---
 notmuch.c |   10 +--
 test/dump-restore |   67 
 test/test-lib.sh  |1 +
 6 files changed, 115 insertions(+), 32 deletions(-)

diff --git a/NEWS b/NEWS
index f715142..d2a788f 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,16 @@
+Notmuch TODO (TODO)
+===
+
+New command-line features
+-
+
+Add "notmuch restore --accumulate" option
+
+  The --accumulate switch causes the union of the existing and new tags to be
+  applied, instead of replacing each message's tags as they are read in from
+  the dump file.
+
+
 Notmuch 0.7 (2011-08-01)
 

diff --git a/notmuch-restore.c b/notmuch-restore.c
index f095f64..5aad60c 100644
--- a/notmuch-restore.c
+++ b/notmuch-restore.c
@@ -31,7 +31,8 @@ notmuch_restore_command (unused (void *ctx), int argc, char 
*argv[])
 size_t line_size;
 ssize_t line_len;
 regex_t regex;
-int rerr;
+notmuch_bool_t accumulate;
+int i, rerr;

 config = notmuch_config_open (ctx, NULL, NULL);
 if (config == NULL)
@@ -44,14 +45,28 @@ notmuch_restore_command (unused (void *ctx), int argc, char 
*argv[])

 synchronize_flags = notmuch_config_get_maildir_synchronize_flags (config);

-if (argc) {
-   input = fopen (argv[0], "r");
-   if (input == NULL) {
-   fprintf (stderr, "Error opening %s for reading: %s\n",
-argv[0], strerror (errno));
-   return 1;
+accumulate = FALSE;
+input = NULL;
+for (i = 0; i < argc; i++) {
+   if (STRNCMP_LITERAL (argv[i], "--accumulate") == 0) {
+   accumulate = TRUE;
+   } else {
+   if (input == NULL) {
+   input = fopen (argv[i], "r");
+   if (input == NULL) {
+   fprintf (stderr, "Error opening %s for reading: %s\n",
+argv[i], strerror (errno));
+   return 1;
+   }
+   } else {
+   fprintf (stderr,
+"Cannot read dump from more than one file: %s\n",
+argv[i]);
+   return 1;
+   }
}
-} else {
+}
+if (input == NULL) {
printf ("No filename given. Reading dump from stdin.\n");
input = stdin;
 }
@@ -94,6 +109,13 @@ notmuch_restore_command (unused (void *ctx), int argc, char 
*argv[])
goto NEXT_LINE;
}

+   /* In order to detect missing messages, this check/optimization is
+* intentionally done *after* first finding the message.  */
+   if (accumulate && (file_tags == NULL || *file_tags == '\0'))
+   {
+   goto NEXT_LINE;
+   }
+
db_tags_str = NULL;
for (db_tags = notmuch_message_get_tags (message);
 notmuch_tags_valid (db_tags);
@@ -115,7 +137,9 @@ notmuch_restore_command (unused (void *ctx), int argc, char 
*argv[])
}

notmuch_message_freeze (message);
-   notmuch_message_remove_all_tags (message);
+
+   if (!accumulate)
+   notmuch_message_remove_all_tags (message);

next = file_tags;
while (next) {
diff --git a/notmuch.1 b/notmuch.1
index 5a8c83d..883371d 100644
--- a/notmuch.1
+++ b/notmuch.1
@@ -454,7 +454,7 @@ section below for details of the supported syntax for 
.
 The
 .BR dump " and " restore
 commands can be used to create a textual dump of email tags for backup
-purposes, and to restore from that dump
+purposes, and to restore from that dump.

 .RS 4
 .TP 4
@@ -462,17 +462,19 @@ purposes, and to restore from that dump

 Creates a plain-text dump of the tags of each message.

-The output is to the given filename, if any, or to stdout.
+The output is written to the given filename, if any, or to stdout.

 These tags are the only data in the notmuch database that can't be
 recreated from the messages themselves.  The output of notmuch dump is
 therefore the only critical thing to backup (and much more friendly to
 incremental backup than the native database files.)
 .TP
-.BR restore " "
+.BR restore " [--accumulate] []"

 Restores the tags from the given file (see
-.BR "notmuch dump" "."
+.BR "notmuch dump" ")."
+
+The input is read from the given filename, if any, or from stdin.

 Note: The dump file format is specifically chosen to be
 compatible with the format of files produced by sup-dump.
@@ -480,6 +482,10 @@ So if you've previously been using sup for mail, 

[PATCH] notmuch restore --accumulate

[Jameson Graef Rollins]

On Mon,  5 Sep 2011 21:07:17 +0200, Thomas Schwinge  
wrote:
> From: Thomas Schwinge 
> 
> Also enhance the dump-restore testsuite, and make it generally more
> failure-proof.

Hi, Thomas.  Your commit log doesn't describe what this patch does.  If
you're adding a new feature you should describe in detail what this new
feature does and how it works.

Also, we generally prefer to have modifications to the test suite in
separate patches that precede the patches that add the features/fix the
bugs.

> Signed-off-by: Thomas Schwinge 

Finally, you don't need to sign off on your own patches.

Thanks.

jamie.
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: 

Re: [PATCH] notmuch restore --accumulate

[Jameson Graef Rollins]

On Mon,  5 Sep 2011 21:07:17 +0200, Thomas Schwinge tho...@schwinge.name 
wrote:
 From: Thomas Schwinge tho...@schwinge.name
 
 Also enhance the dump-restore testsuite, and make it generally more
 failure-proof.

Hi, Thomas.  Your commit log doesn't describe what this patch does.  If
you're adding a new feature you should describe in detail what this new
feature does and how it works.

Also, we generally prefer to have modifications to the test suite in
separate patches that precede the patches that add the features/fix the
bugs.

 Signed-off-by: Thomas Schwinge tho...@schwinge.name

Finally, you don't need to sign off on your own patches.

Thanks.

jamie.


pgpppqBJRqVL5.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch