Update for nmbug, round 2

Update for nmbug, round 2

[david]

This obsoletes 

     id:1360374019-20988-1-git-send-email-david@tethera.net

This less broken than the last version ;). I've used these patches for
a few days without ill effects. The first two patches use
batch-tagging, which should have some speedup. The includes fixes for
the issue about quoting that Tomi raised.

The second two patches are a style improvement and a bug fix for a bug
that probably not many people hit.

[Patch v2 1/4] nmbug: use dump --format=batch-tag

[david]

From: David Bremner <bremner@debian.org>

This should make nmbug tolerate tags with whitespace and other special
characters it.  At the moment this relies on _not_ passing calls to
notmuch tag through the shell, which is a documented feature of perl's
system function.
---
 devel/nmbug/nmbug |   27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/devel/nmbug/nmbug b/devel/nmbug/nmbug
index fe103b3..befc3d9 100755
--- a/devel/nmbug/nmbug
+++ b/devel/nmbug/nmbug
@@ -39,6 +39,11 @@ my %command = (
 	     status	=> \&do_status,
 	     );
 
+# Convert prefix into form suitable for literal matching against
+# notmuch dump --format=batch-tag output.
+my $ENCPREFIX = encode_for_fs ($TAGPREFIX);
+$ENCPREFIX =~ s/:/%3a/g;
+
 my $subcommand = shift || usage ();
 
 if (!exists $command{$subcommand}) {
@@ -203,9 +208,9 @@ sub index_tags {
 
   my $index = $NMBGIT.'/nmbug.index';
 
-  my $query = join ' ', map ("tag:$_", get_tags ($TAGPREFIX));
+  my $query = join ' ', map ("tag:\"$_\"", get_tags ($TAGPREFIX));
 
-  my $fh = spawn ('-|', qw/notmuch dump --/, $query)
+  my $fh = spawn ('-|', qw/notmuch dump --format=batch-tag --/, $query)
     or die "notmuch dump: $!";
 
   git ('read-tree', '--empty');
@@ -214,22 +219,30 @@ sub index_tags {
     or die 'git update-index';
 
   while (<$fh>) {
-    m/ ( [^ ]* ) \s+ \( ([^\)]* ) \) /x || die 'syntax error in dump';
-    my ($id,$rest) = ($1,$2);
 
-    #strip prefixes before writing
-    my @tags = grep { s/^$TAGPREFIX//; } split (' ', $rest);
+    chomp();
+    my ($rest,$id) = split(/ -- id:/);
+
+    if ($id =~ s/^"(.*)"\s*$/$1/) {
+      # xapian quoted string, dequote.
+      $id =~ s/""/"/g;
+    }
+
+    #strip prefixes from tags before writing
+    my @tags = grep { s/^[+]$ENCPREFIX//; } split (' ', $rest);
     index_tags_for_msg ($git,$id, 'A', @tags);
   }
   unless (close $git) {
     die "'git update-index --index-info' exited with nonzero value\n";
   }
   unless (close $fh) {
-    die "'notmuch dump -- $query' exited with nonzero value\n";
+    die "'notmuch dump --format=batch-tag -- $query' exited with nonzero value\n";
   }
   return $index;
 }
 
+# update the git index to either create or delete an empty file.
+# Neither argument should be encoded/escaped.
 sub index_tags_for_msg {
   my $fh = shift;
   my $msgid = shift;
-- 
1.7.10.4

[Patch v2 2/4] nmbug: use 'notmuch tag --batch'

[david]

From: David Bremner <bremner@debian.org>

This should be more robust with respect to tags with whitespace and
and other special characters. It also (hopefully) fixes a remaining
bug handling message-ids with whitespace.  It should also be
noticeably faster for large sets of changes since it does one exec per
change set as opposed to one exec per tag changed.
---
 devel/nmbug/nmbug |   27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/devel/nmbug/nmbug b/devel/nmbug/nmbug
index befc3d9..73d64fe 100755
--- a/devel/nmbug/nmbug
+++ b/devel/nmbug/nmbug
@@ -267,6 +267,20 @@ sub do_checkout {
   do_sync (action => 'checkout');
 }
 
+sub quote_for_xapian {
+  my $str = shift;
+  $str =~ s/"/""/g;
+  return '"' . $str . '"';
+}
+
+sub pair_to_batch_line {
+  my ($action, $pair) = @_;
+
+  # the tag should already be suitably encoded
+
+  return $action . $ENCPREFIX . $pair->{tag} .
+    ' -- id:' . quote_for_xapian ($pair->{id})."\n";
+}
 
 sub do_sync {
 
@@ -283,17 +297,20 @@ sub do_sync {
     $D_action = '-';
   }
 
-  foreach my $pair (@{$status->{added}}) {
+  my $notmuch = spawn ({}, '|-', qw/notmuch tag --batch/)
+    or die 'notmuch tag --batch';
 
-    notmuch ('tag', $A_action.$TAGPREFIX.$pair->{tag},
-	     'id:'.$pair->{id});
+  foreach my $pair (@{$status->{added}}) {
+    print $notmuch pair_to_batch_line ($A_action, $pair);
   }
 
   foreach my $pair (@{$status->{deleted}}) {
-    notmuch ('tag', $D_action.$TAGPREFIX.$pair->{tag},
-	     'id:'.$pair->{id});
+    print $notmuch pair_to_batch_line ($D_action, $pair);
   }
 
+  unless (close $notmuch) {
+    die "'notmuch tag --batch' exited with nonzero value\n";
+  }
 }
 
 
-- 
1.7.10.4

[Patch v2 3/4] nmbug: replace hard-coded magic hash with git-hash-object

[david]

From: David Bremner <bremner@debian.org>

This is at least easier to understand than the magic hash. It may also
be a bit more robust, although it is hard to imagine these numbers
changing without many other changes in git.
---
 devel/nmbug/nmbug |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/devel/nmbug/nmbug b/devel/nmbug/nmbug
index 73d64fe..b9c70e4 100755
--- a/devel/nmbug/nmbug
+++ b/devel/nmbug/nmbug
@@ -15,9 +15,6 @@ $NMBGIT .= '/.git' if (-d $NMBGIT.'/.git');
 
 my $TAGPREFIX = $ENV{NMBPREFIX} || 'notmuch::';
 
-# magic hash for git
-my $EMPTYBLOB = 'e69de29bb2d1d6434b8b29ae775ad8c2e48c5391';
-
 # for encoding
 
 my $ESCAPE_CHAR =	'%';
@@ -50,6 +47,9 @@ if (!exists $command{$subcommand}) {
   usage ();
 }
 
+# magic hash for git
+my $EMPTYBLOB = git (qw{hash-object -t blob /dev/null});
+
 &{$command{$subcommand}}(@ARGV);
 
 sub git_pipe {
-- 
1.7.10.4

[Patch v2 4/4] nmbug: allow empty prefix

[david]

From: David Bremner <bremner@debian.org>

Current code does not distinguish between an empty string in the
NMBPREFIX environment variable and the variable being undefined. This
makes it impossible to define an empty prefix, if, e.g. somebody wants
to dump all of their tags with nmbug.
---
 devel/nmbug/nmbug |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devel/nmbug/nmbug b/devel/nmbug/nmbug
index b9c70e4..90d98b6 100755
--- a/devel/nmbug/nmbug
+++ b/devel/nmbug/nmbug
@@ -13,7 +13,7 @@ my $NMBGIT = $ENV{NMBGIT} || $ENV{HOME}.'/.nmbug';
 
 $NMBGIT .= '/.git' if (-d $NMBGIT.'/.git');
 
-my $TAGPREFIX = $ENV{NMBPREFIX} || 'notmuch::';
+my $TAGPREFIX = defined($ENV{NMBPREFIX}) ? $ENV{NMBPREFIX} : 'notmuch::';
 
 # for encoding
 
-- 
1.7.10.4

Re: [Patch v2 1/4] nmbug: use dump --format=batch-tag

[Tomi Ollila]

On Thu, Feb 21 2013, david@tethera.net wrote:

> From: David Bremner <bremner@debian.org>
>
> This should make nmbug tolerate tags with whitespace and other special
> characters it.  At the moment this relies on _not_ passing calls to
> notmuch tag through the shell, which is a documented feature of perl's
> system function.

This patch series looks good to me (as far as I can understand, I did
not find the "silly bug" in your previous patch...)

Instead of mentioning that "calls are _not_ passed to shell" here,
that could be briefly mentioned just before system() calls in the
script -- and that definitely should not be 'At the moment' feature.

the system() function in perl(1) never pass execution through the
shell in case the args are list more than one arg:

1 $ perl -e 'system( qw/echo |cat; echo second line ?/ )'
|cat; echo second line ?

2 $ perl -le 'system( qw/echo| cat;/ ); print "system: ",$!'
system: No such file or directory

3 $ perl -le 'system( qw/echo|/ ); print "system: ",$!'
sh: -c: line 1: syntax error: unexpected end of file
system:

execve calls to the above could have been:

1: execve( "/bin/echo" [ "echo" "|cat;" "echo" "second" "line" "?" ] )

2: no execve: perl(1) did not find 'echo|' in the PATH

3: execve( "/bin/sh" [ "sh" "-c" "echo|" ] )

Tomi

> ---
>  devel/nmbug/nmbug |   27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/devel/nmbug/nmbug b/devel/nmbug/nmbug
> index fe103b3..befc3d9 100755
> --- a/devel/nmbug/nmbug
> +++ b/devel/nmbug/nmbug
> @@ -39,6 +39,11 @@ my %command = (
>  	     status	=> \&do_status,
>  	     );
>  
> +# Convert prefix into form suitable for literal matching against
> +# notmuch dump --format=batch-tag output.
> +my $ENCPREFIX = encode_for_fs ($TAGPREFIX);
> +$ENCPREFIX =~ s/:/%3a/g;
> +
>  my $subcommand = shift || usage ();
>  
>  if (!exists $command{$subcommand}) {
> @@ -203,9 +208,9 @@ sub index_tags {
>  
>    my $index = $NMBGIT.'/nmbug.index';
>  
> -  my $query = join ' ', map ("tag:$_", get_tags ($TAGPREFIX));
> +  my $query = join ' ', map ("tag:\"$_\"", get_tags ($TAGPREFIX));
>  
> -  my $fh = spawn ('-|', qw/notmuch dump --/, $query)
> +  my $fh = spawn ('-|', qw/notmuch dump --format=batch-tag --/, $query)
>      or die "notmuch dump: $!";
>  
>    git ('read-tree', '--empty');
> @@ -214,22 +219,30 @@ sub index_tags {
>      or die 'git update-index';
>  
>    while (<$fh>) {
> -    m/ ( [^ ]* ) \s+ \( ([^\)]* ) \) /x || die 'syntax error in dump';
> -    my ($id,$rest) = ($1,$2);
>  
> -    #strip prefixes before writing
> -    my @tags = grep { s/^$TAGPREFIX//; } split (' ', $rest);
> +    chomp();
> +    my ($rest,$id) = split(/ -- id:/);
> +
> +    if ($id =~ s/^"(.*)"\s*$/$1/) {
> +      # xapian quoted string, dequote.
> +      $id =~ s/""/"/g;
> +    }
> +
> +    #strip prefixes from tags before writing
> +    my @tags = grep { s/^[+]$ENCPREFIX//; } split (' ', $rest);
>      index_tags_for_msg ($git,$id, 'A', @tags);
>    }
>    unless (close $git) {
>      die "'git update-index --index-info' exited with nonzero value\n";
>    }
>    unless (close $fh) {
> -    die "'notmuch dump -- $query' exited with nonzero value\n";
> +    die "'notmuch dump --format=batch-tag -- $query' exited with nonzero value\n";
>    }
>    return $index;
>  }
>  
> +# update the git index to either create or delete an empty file.
> +# Neither argument should be encoded/escaped.
>  sub index_tags_for_msg {
>    my $fh = shift;
>    my $msgid = shift;
> -- 
> 1.7.10.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [Patch v2 1/4] nmbug: use dump --format=batch-tag

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

>
> This patch series looks good to me (as far as I can understand, I did
> not find the "silly bug" in your previous patch...)
>

Compare :

+    print $notmuch $A_action.$TAGPREFIX.$pair->{tag}, " -- ",
+      'id:'.$pair->{id};

vs.

+    print $notmuch $D_action.$TAGPREFIX.$pair->{tag},
+      'id:'.$pair->{id};

I obviously did not test that version very well.

> Instead of mentioning that "calls are _not_ passed to shell" here,
> that could be briefly mentioned just before system() calls in the
> script -- and that definitely should not be 'At the moment' feature.

Sure, the "At the moment" is meant to modify "relies". In the next
patch, we stop relying on this feature of Perl.

>
> the system() function in perl(1) never pass execution through the
> shell in case the args are list more than one arg:
>

right. that's what I meant by "is a documented feature of perl's system
function"

So I think we mean to say the same thing here; but I could I add a
comment in this patch (to delete it in the next). Or maybe reword that
commit message somehow.

d

Re: [Patch v2 1/4] nmbug: use dump --format=batch-tag

[Tomi Ollila]

On Thu, Feb 21 2013, David Bremner <david@tethera.net> wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>>
>> This patch series looks good to me (as far as I can understand, I did
>> not find the "silly bug" in your previous patch...)
>>
>
> Compare :
>
> +    print $notmuch $A_action.$TAGPREFIX.$pair->{tag}, " -- ",
> +      'id:'.$pair->{id};
>
> vs.
>
> +    print $notmuch $D_action.$TAGPREFIX.$pair->{tag},
> +      'id:'.$pair->{id};

Ah, ok :)

> I obviously did not test that version very well.

IIRC that has happened to some other people, too ;(

>> Instead of mentioning that "calls are _not_ passed to shell" here,
>> that could be briefly mentioned just before system() calls in the
>> script -- and that definitely should not be 'At the moment' feature.
>
> Sure, the "At the moment" is meant to modify "relies". In the next
> patch, we stop relying on this feature of Perl.

Well, one have to expect the execution to happen one way or another
(and arrange quoting accordingly); for example:

$ echo; perl -e 'exec q/echo "foobar"/'

foobar

vs.

$ echo; perl -e 'exec qw/echo "foobar"/'

"foobar"

first gave exec one arg, 'echo "foobar"' and due to "":s perl used sh
to execute it, second gave exec list [ 'echo', '"foobar"' ] and therefore
echo got foobar with quotes as a command line argument.

Anyone interested for more information, see 'perldoc -f exec'.


>> the system() function in perl(1) never pass execution through the
>> shell in case the args are list more than one arg:
>
> right. that's what I meant by "is a documented feature of perl's system
> function"
>
> So I think we mean to say the same thing here; but I could I add a
> comment in this patch (to delete it in the next). Or maybe reword that
> commit message somehow.

Yes, the message is the same, just that there should not be need for that..

> d

Tomi

Re: Update for nmbug, round 2

[Tomi Ollila]

On Thu, Feb 21 2013, david@tethera.net wrote:

> This obsoletes 
>
>      id:1360374019-20988-1-git-send-email-david@tethera.net
>
> This less broken than the last version ;). I've used these patches for
> a few days without ill effects. The first two patches use
> batch-tagging, which should have some speedup. The includes fixes for
> the issue about quoting that Tomi raised.
>
> The second two patches are a style improvement and a bug fix for a bug
> that probably not many people hit.

I withdraw all my request for changing content or commit message --
as the code part in question is changing to 'tag --batch' all my
issues are answered and IMHO the changes can be pushed as is.

David: are you going to send followup patches soon? I'm running nmbug
with these patches and it works fine.

Tomi

Re: Update for nmbug, round 2

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

>
> David: are you going to send followup patches soon? I'm running nmbug
> with these patches and it works fine.
>

I have two followup patches that add an "nmbug init" command to create
an empty nmbug repo, but this is only really needed/useful for using
nmbug to track other kinds of tags, and I'm not sure yet if this is the
right direction to go. So maybe I'll just push these patches as is.

d

Re: Update for nmbug, round 2

[David Bremner]

David Bremner <david@tethera.net> writes:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>>
>> David: are you going to send followup patches soon? I'm running nmbug
>> with these patches and it works fine.
>>
>
> I have two followup patches that add an "nmbug init" command to create
> an empty nmbug repo, but this is only really needed/useful for using
> nmbug to track other kinds of tags, and I'm not sure yet if this is the
> right direction to go. So maybe I'll just push these patches as is.

I pushed this series.

d