From: Ben Gamari <bgamari.foss@gmail.com>
To: Tomi Ollila <tomi.ollila@iki.fi>, notmuch <notmuch@notmuchmail.org>
Subject: Re: [PATCH 1/3] Add notmuch_database_close_compact
In-Reply-To: <m27gssitot.fsf@guru.guru-group.fi>
References: <1345476704-17091-1-git-send-email-bgamari.foss@gmail.com>
	<1345476704-17091-2-git-send-email-bgamari.foss@gmail.com>
	<m27gssitot.fsf@guru.guru-group.fi>
User-Agent: Notmuch/0.13.2+137~g3dc1094 (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Tue, 21 Aug 2012 10:49:47 -0400
Message-ID: <87y5l8z4dw.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Mon, Aug 20 2012, Ben Gamari <bgamari.foss@gmail.com> wrote:
>
>> ---
>>  configure       |   25 ++++++++++++++++++++++++-
>>  lib/database.cc |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  lib/notmuch.h   |   14 ++++++++++++++
>>  3 files changed, 92 insertions(+), 1 deletion(-)
>>
>> diff --git a/configure b/configure
>> index dc0dba4..370fedd 100755
>> --- a/configure
>> +++ b/configure
>> @@ -270,7 +270,8 @@ printf "Checking for Xapian development files... "
>>  have_xapian=0
>>  for xapian_config in ${XAPIAN_CONFIG}; do
>>      if ${xapian_config} --version > /dev/null 2>&1; then
>> -	printf "Yes (%s).\n" $(${xapian_config} --version | sed -e 's/.* //')
>> +	xapian_version=$(${xapian_config} --version | sed -e 's/.* //')
>> +	printf "Yes (%s).\n" ${xapian_version}
>>  	have_xapian=1
>>  	xapian_cxxflags=$(${xapian_config} --cxxflags)
>>  	xapian_ldflags=$(${xapian_config} --libs)
>> @@ -282,6 +283,24 @@ if [ ${have_xapian} = "0" ]; then
>>      errors=$((errors + 1))
>>  fi
>>  
>> +have_xapian_compact=0
>> +if [ ${have_xapian} = "1" ]; then
>> +    printf "Checking for Xapian compact support... "
>> +    IFS='.'
>> +    old_args=$@
>> +    set -- ${xapian_version}
>> +    xapian_major_version=$1
>> +    xapian_minor_version=$2
>> +    xapian_subminor_version=$3
>> +    set -- ${old_args}
>
> The part above breaks the argument vector in case there are spaces in 
> args (I though $IFS chars, but try the script), execute:
>
Hmmm, I suppose so.

> $ echo '#!/bin/bash
> IFS=.
> for x in "$@"; do echo $x; done; echo
> foo=$@
> for x in $foo; do echo $x; done; echo
> set -- $foo
> for x in "$@"; do echo $x; done; echo
> ' > foo.bash
>
> $ bash foo.bash a "b c" d
>
> Also, after processing, IFS is not restored (to $DEFAULT_IFS)
>
I assumed this would be alright since IFS is set in the next 

> an alternative is to put the code in function like the following
> way:
>
Sounds good to me.

> Hmm, I guess the check above is to determine whether xapian version is
> 1.2.6 or newer, but is there xapian version 1.1.6 or 1.0.6 or 0.3.0 or so ?
>
Eh? 1.2.6 is the first Xapian release to have Compactor exposed in the
public API.

> I am not qualified to comment about compaction itself :)
>
Nor am I really. I just noticed that this functionality was blocking on
library support which is now in place. It seemed that a pretty
straightforward thing to implement and it hasn't broken my index yet.

> In the last patch you give copyright to Carl (which is OK). However I'd
> debate whether it is good practise to declare Carl as author; I'd say that
> is OK if he agrees to claim authorship to your potentially shi^H^H^Hperfect
> code ;)
>
Yikes. That certainly wasn't intentional. I'll fix this in the next
iteration.

> There are at least a few style issues below: e.g. no space between function
> name and opening parenthesis.
>
Duly noted.

Thanks!

Cheers,

- Ben