On 08/13/2014 01:34 PM, Jakub Hrozek wrote:
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.
Ok, I'm fine with that. Ack.
Just fix the option name also in commit message prior pushing please.