* [PATCH 1/3] test: test one character long directory names at top level @ 2016-04-10 19:43 Jani Nikula 2016-04-10 19:43 ` [PATCH 2/3] lib: fix handling of " Jani Nikula 2016-04-10 19:43 ` [PATCH 3/3] lib: clean up _notmuch_database_split_path Jani Nikula 0 siblings, 2 replies; 5+ messages in thread From: Jani Nikula @ 2016-04-10 19:43 UTC (permalink / raw) To: notmuch Yes, it's broken. Reported by h01ger on IRC. --- test/T050-new.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/T050-new.sh b/test/T050-new.sh index 93a6fa94ae7c..174715aa2781 100755 --- a/test/T050-new.sh +++ b/test/T050-new.sh @@ -169,6 +169,20 @@ output=$(NOTMUCH_NEW --debug) test_expect_equal "$output" "(D) add_files, pass 3: queuing leftover directory ${MAIL_DIR}/two for deletion from database 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 +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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] lib: fix handling of one character long directory names at top level 2016-04-10 19:43 [PATCH 1/3] test: test one character long directory names at top level Jani Nikula @ 2016-04-10 19:43 ` Jani Nikula 2016-04-11 6:44 ` Tomi Ollila 2016-04-10 19:43 ` [PATCH 3/3] lib: clean up _notmuch_database_split_path Jani Nikula 1 sibling, 1 reply; 5+ messages in thread From: Jani Nikula @ 2016-04-10 19:43 UTC (permalink / raw) To: notmuch 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. 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/3] lib: fix handling of one character long directory names at top level 2016-04-10 19:43 ` [PATCH 2/3] lib: fix handling of " Jani Nikula @ 2016-04-11 6:44 ` Tomi Ollila 0 siblings, 0 replies; 5+ messages in thread From: Tomi Ollila @ 2016-04-11 6:44 UTC (permalink / raw) To: Jani Nikula, notmuch 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 3/3] lib: clean up _notmuch_database_split_path 2016-04-10 19:43 [PATCH 1/3] test: test one character long directory names at top level Jani Nikula 2016-04-10 19:43 ` [PATCH 2/3] lib: fix handling of " Jani Nikula @ 2016-04-10 19:43 ` Jani Nikula 2016-04-12 23:51 ` David Bremner 1 sibling, 1 reply; 5+ messages in thread From: Jani Nikula @ 2016-04-10 19:43 UTC (permalink / raw) To: notmuch Make the logic it a bit easier to read. No functional changes. --- lib/database.cc | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/lib/database.cc b/lib/database.cc index b8486f7d5271..c8c5e26106ad 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -1761,18 +1761,11 @@ _notmuch_database_split_path (void *ctx, slash = path + strlen (path) - 1; /* First, skip trailing slashes. */ - while (slash != path) { - if (*slash != '/') - break; - + while (slash != path && *slash == '/') --slash; - } /* Then, find a slash. */ - while (slash != path) { - if (*slash == '/') - break; - + while (slash != path && *slash != '/') { if (basename) *basename = slash; @@ -1780,12 +1773,8 @@ _notmuch_database_split_path (void *ctx, } /* Finally, skip multiple slashes. */ - while (slash != path) { - if (*(slash - 1) != '/') - break; - + while (slash != path && *(slash - 1) == '/') --slash; - } if (slash == path) { if (directory) -- 2.1.4 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] lib: clean up _notmuch_database_split_path 2016-04-10 19:43 ` [PATCH 3/3] lib: clean up _notmuch_database_split_path Jani Nikula @ 2016-04-12 23:51 ` David Bremner 0 siblings, 0 replies; 5+ messages in thread From: David Bremner @ 2016-04-12 23:51 UTC (permalink / raw) To: Jani Nikula, notmuch Jani Nikula <jani@nikula.org> writes: > Make the logic it a bit easier to read. No functional changes. > --- I decided to push all three patches; it wasn't clear Tomi reviewed all 3, but I had a look at the third. So, uh, if it's broken OH LOOK OVER THERE -> d ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-04-12 23:51 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-04-10 19:43 [PATCH 1/3] test: test one character long directory names at top level Jani Nikula 2016-04-10 19:43 ` [PATCH 2/3] lib: fix handling of " Jani Nikula 2016-04-11 6:44 ` Tomi Ollila 2016-04-10 19:43 ` [PATCH 3/3] lib: clean up _notmuch_database_split_path Jani Nikula 2016-04-12 23:51 ` David Bremner
Code repositories for project(s) associated with this public inbox https://yhetil.org/notmuch.git/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).