From: Tomi Ollila <tomi.ollila@iki.fi>
To: Jani Nikula <jani@nikula.org>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/3] lib: fix handling of one character long directory
 names at top level
In-Reply-To: <1460317403-13714-2-git-send-email-jani@nikula.org>
References: <1460317403-13714-1-git-send-email-jani@nikula.org>
 <1460317403-13714-2-git-send-email-jani@nikula.org>
User-Agent: Notmuch/0.21+99~g7cbc880 (http://notmuchmail.org) Emacs/24.3.1
 (x86_64-unknown-linux-gnu)
Date: Mon, 11 Apr 2016 09:44:49 +0300
Message-ID: <m2d1pwzlsu.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: list

On Sun, Apr 10 2016, Jani Nikula <jani@nikula.org> wrote:

> The code to skip multiple slashes in _notmuch_database_split_path()
> skips back one character too much. This is compensated by a +1 in the
> length parameter to the strndup() call. Mostly this works fine, but if
> the path is to a file under a top level directory with one character
> long name, the directory part is mistaken to be part of the file name
> (slash == path in code). The returned directory name will be the empty
> string and the basename will be the full path, breaking the indexing
> logic in notmuch new.
>
> Fix the multiple slash skipping to keep the slash variable pointing at
> the last slash, and adjust strndup() accordingly.

Good work. Changes look good. Tests pass...

... I also debugged this yesterday with an extra debug print:

+++ b/notmuch-new.c
@@ -382,2 +382,3 @@ add_files (notmuch_database_t *notmuch,
     status = notmuch_database_get_directory (notmuch, path, &directory);
+    printf("(Z) %s %p %d\n", path, directory, status);
     if (status) {

Without these changes the %p printing directory value never changed from
(nil) to ... non-nil with one-character directory names (meaning directory
was found in database), after this patch the directory value worked like
with longer directory names.

Tomi

>
> The bug was introduced in
>
> commit e890b0cf4011fd9fd77ebd87343379e4a778888b
> Author: Carl Worth <cworth@cworth.org>
> Date:   Sat Dec 19 13:20:26 2009 -0800
>
>     database: Store the parent ID for each directory document.
>
> just a little over two months after the initial commit in the Notmuch
> code history, making this the longest living bug in Notmuch to date.
> ---
>  lib/database.cc  | 4 ++--
>  test/T050-new.sh | 5 -----
>  2 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 3b342f136a53..b8486f7d5271 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -1781,7 +1781,7 @@ _notmuch_database_split_path (void *ctx,
>  
>      /* Finally, skip multiple slashes. */
>      while (slash != path) {
> -	if (*slash != '/')
> +	if (*(slash - 1) != '/')
>  	    break;
>  
>  	--slash;
> @@ -1794,7 +1794,7 @@ _notmuch_database_split_path (void *ctx,
>  	    *basename = path;
>      } else {
>  	if (directory)
> -	    *directory = talloc_strndup (ctx, path, slash - path + 1);
> +	    *directory = talloc_strndup (ctx, path, slash - path);
>      }
>  
>      return NOTMUCH_STATUS_SUCCESS;
> diff --git a/test/T050-new.sh b/test/T050-new.sh
> index 174715aa2781..53e02d22c383 100755
> --- a/test/T050-new.sh
> +++ b/test/T050-new.sh
> @@ -170,7 +170,6 @@ test_expect_equal "$output" "(D) add_files, pass 3: queuing leftover directory $
>  No new mail. Removed 3 messages."
>  
>  test_begin_subtest "One character directory at top level"
> -test_subtest_known_broken
>  
>  generate_message [dir]=A
>  generate_message [dir]=A/B
> @@ -179,10 +178,6 @@ generate_message [dir]=A/B/C
>  output=$(NOTMUCH_NEW --debug)
>  test_expect_equal "$output" "Added 3 new messages to the database."
>  
> -# clean up after the broken test to not mess up other tests
> -rm -rf "${MAIL_DIR}"/A
> -NOTMUCH_NEW 2>&1 > /dev/null
> -
>  test_begin_subtest "Support single-message mbox"
>  cat > "${MAIL_DIR}"/mbox_file1 <<EOF
>  From test_suite@notmuchmail.org Fri Jan  5 15:43:57 2001
> -- 
> 2.1.4
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> https://notmuchmail.org/mailman/listinfo/notmuch