On Mon, Aug 11, 2014 at 05:21:27PM +0200, Lukas Slebodnik wrote:
On (11/08/14 16:21), Michal Židek wrote:
>On 08/11/2014 02:25 PM, Pavel Březina wrote:
>>On 08/11/2014 09:54 AM, Jakub Hrozek wrote:
>>>On Mon, Aug 11, 2014 at 09:12:24AM +0200, Lukas Slebodnik wrote:
>>>>It is your decision. You can:
>>>> a) fix problem in function ifp_user_get_attr_search
>>>> b) remove changes in IFP responder.
>>>>
>>>>LS
>>>
>>>Thanks for the quick review. I would prefer to keep the ifp changes in
>>>-- I don't realistically expect anyone being bitten by spaces when using
>>>InfoPipe, but it's more about being consistent. When we say we want to
>>>replace the spaces, we should replace them everywhere..
>>
>>Patch 1:
>>Ack.
>>
>>Patch 2:
>>Man page says "override_space", but you renamed the option to
>>"override_default_space". Actually I like the first name better, can
you
>>switch it back? Or what is the reason for using "default" word?
That was a mistake that crept in when I was reshuffling the patches
about Lukas' changes. Thanks for catching it!
>>
>>Patch 3:
>>Ack.
>>
>>However, I'm not really sure it is a good idea to allow both logins.
>>Wouldn't it be better to allow only one depending whether the option is
>>set or not?
I don't see the benefit, sorry.
>>
>>Patch 4:
>>Ack.
>>
>>Patch 5:
>>Ack.
>>
>>Path 6:
>>Ack.
>>
>>The code is good, however, I still see the problem that the
>>transformation is not symmetrical. If an input contains the replacement
>>character, we cannot perform reverse transformation correctly. Either we
>>have to very loudly warn that the character must be unused all over the
>>database or fix it.
>
>We should properly document that the character can not be used
>in the database.
>There is no way to properly "fix" this issue. For example there
>is no way to fix the following:
+1
LS
I agree as well and I amended the man page hunk in the second patch.
Thank you for the review.