From: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
To: Tomi Ollila <tomi.ollila@iki.fi>, notmuch@notmuchmail.org
Subject: Re: [PATCH 1/3] test: add functions to count how much times notmuch
	was called
In-Reply-To: <yf6aa7gj7w5.fsf@taco2.nixu.fi>
References: <1322271878-32614-1-git-send-email-dmitry.kurochkin@gmail.com>
	<1322450895-32523-1-git-send-email-dmitry.kurochkin@gmail.com>
	<1322450895-32523-2-git-send-email-dmitry.kurochkin@gmail.com>
	<yf6aa7gj7w5.fsf@taco2.nixu.fi>
User-Agent: Notmuch/0.10+44~g067c44f (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Tue, 29 Nov 2011 01:26:39 +0400
Message-ID: <87hb1ovsz4.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

Hi Tomi.

On Mon, 28 Nov 2011 22:42:50 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Mon, 28 Nov 2011 07:28:13 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> 
> [...]
> > +
> > +    These allow to count how many times notmuch binary is called.
> > +    notmuch_counter_reset() function generates a script that counts
> > +    how many times it is called and resets the counter to zero.  The
> > +    function sets $notmuch_counter_command variable to the path to the
> > +    generated script that should be called instead of notmuch to do
> > +    the counting.  The notmuch_counter_value() function prints the
> > +    current counter value.
> > diff --git a/test/test-lib.sh b/test/test-lib.sh
> > index 076f929..880bed9 100644
> > --- a/test/test-lib.sh
> > +++ b/test/test-lib.sh
> > @@ -868,6 +868,38 @@ test_emacs () {
> >  	emacsclient --socket-name="$EMACS_SERVER" --eval "(progn $@)"
> >  }
> >  
> > +# Creates a script that counts how much time it is executed and calls
> > +# notmuch.  $notmuch_counter_command is set to the path to the
> > +# generated script.  Use notmuch_counter_value() function to get the
> > +# current counter value.
> > +notmuch_counter_reset () {
> > +	notmuch_counter_command="$TMP_DIRECTORY/notmuch_counter"
> > +	if [ ! -x "$notmuch_counter_command" ]; then
> > +		notmuch_counter_state_path="$TMP_DIRECTORY/notmuch_counter.state"
> > +		cat >"$notmuch_counter_command" <<EOF || return
> > +#!/bin/sh
> > +
> > +count=\$(cat "$notmuch_counter_state_path")
> > +echo -n \$(expr \$count + 1) > "$notmuch_counter_state_path"
> > +
> > +exec notmuch "\$@"
> > +EOF
> > +		chmod +x "$notmuch_counter_command" || return
> > +	fi
> > +
> > +	echo -n 0 > "$notmuch_counter_state_path"
> > +}
> > +
> > +# Returns the current notmuch counter value.
> > +notmuch_counter_value () {
> > +	if [ -r "$notmuch_counter_state_path" ]; then
> > +		count=$(cat "$notmuch_counter_state_path")
> > +	else
> > +		count=0
> > +	fi
> > +	echo -n $count
> > +}
> > +
> 
> Good work! It would be nice if the state file contained newline after
> count number.

I wonder why it is actually nice :)  I do not have strong preference
here.  So a newline is added in v3.  Also a newline is added to
notmuch_counter_value() output for consistency.

> Also some optimizations could be done:
> 

(Would be nice if you send a diff, or a human-friendly description of
the changes.)

> 		cat >"$notmuch_counter_command" <<EOF || return
> #!/bin/sh
> 
> read count < "$notmuch_counter_state_path"

Nice.  Fixed in the new patch version.

> echo \$((count + 1)) > "$notmuch_counter_state_path"
> 

I do not think this is really an optimization.  And I find expr more
clear than using $(()).  I always have troubles remembering "random
special char syntax" (yeah, not a Perl fan :)), prefer human friendly
words.

> exec notmuch "\$@"
> EOF
> 		chmod +x "$notmuch_counter_command" || return
> 	fi
> 
> 	echo 0 > "$notmuch_counter_state_path"
> }
> 
> # Returns the current notmuch counter value.
> notmuch_counter_value () {
> 	if [ -r "$notmuch_counter_state_path" ]; then
> 		read count < "$notmuch_counter_state_path"

Also changes in v3.

Regards,
  Dmitry

> 	else
> 		count=0
> 	fi
> 	echo -n $count
> }
> 
> 
> Tomi