Re: [PATCH 2/4] insert: strip trailing / in folder path

[Yuri Volchkov <yuri.volchkov@gmail.com>]

David Bremner <david@tethera.net> writes:

> Yuri Volchkov <yuri.volchkov@gmail.com> writes:
>
>> I have faced a problem, that messages sent by emacs could not be shown
>> or found later. The "notmuch show id:<msg_id>" says "no such file or
>> directory".
>>
>> The reason of this behavior is the following chain of events:
>> 1) While sending a message, emacs calls
>>       notmuch insert --folder=maildir/Sent/ < test.msg
>>
>
> I think it will be less confusing if you give a more reduced description
> of the bug fix here. In particular the test in your previous patch shows
> that the neither offlineimap nor multiple copies of a message are needed
> to trigger this bug; rather it has to do with insufficient path
> canonicalization.

You are probably right, I was too obsessed with the problem blocking my
switch-to-notmuch project . I'll try to reduce the commit message.

>>
>> The solution is simple, we have to strip trailing '/' from the insert
>> path.
>>
>> diff --git a/lib/database.cc b/lib/database.cc
>> index 8f0e22a..79eb3d6 100644
>> --- a/lib/database.cc
>> +++ b/lib/database.cc
>> @@ -858,8 +858,7 @@ notmuch_database_open_verbose (const char *path,
>>      notmuch->status_string = NULL;
>>      notmuch->path = talloc_strdup (notmuch, path);
>>  
>> -    if (notmuch->path[strlen (notmuch->path) - 1] == '/')
>> -	notmuch->path[strlen (notmuch->path) - 1] = '\0';
>> +    strip_trailing(notmuch->path, '/');
>
> These seems like a reasonable change, but I don't see the connection
> with the notmuch-insert problem. Please either explain the connection in
> the commit message or split the change out into a different commit with
> an explanation of what it is fixing (and perhaps a seperate test, if
> there's a real problem there, rather than just tidying up).

This is just for consistency reasons. Fixing my problem required the
same piece of code, which was used here. Duplicating is not nice, so I
made a function around this code. That's why it feels atomic change
to me.

I think, explanation like this is superfluously detailed.

I can move the introduction of the strip_trailing and cleaning
notmuch_database_open_verbose into separate patch, but the fix then will
be just a one line. And I'll get unnecessarily tiny patches.

What do you think?