[Freeipa-devel] [PATCH 0386] private_ccache: Harden the removal of KRB5CCNAME env variable

[Tomas Babej]

On 11/23/2015 01:50 PM, Jan Cholasta wrote:
> On 23.11.2015 13:40, Tomas Babej wrote:
>>
>>
>> On 11/23/2015 01:31 PM, Jan Cholasta wrote:
>>> On 23.11.2015 13:28, Tomas Babej wrote:
>>>>
>>>>
>>>> On 11/23/2015 01:11 PM, Jan Cholasta wrote:
>>>>> On 23.11.2015 12:53, Tomas Babej wrote:
>>>>>> Hi,
>>>>>>
>>>>>> If the code within the private_ccache contextmanager does not
>>>>>> set/removes the KRB5CCNAME, the pop method will raise KeyError, which
>>>>>> will cause unnecessary termination of the code flow.
>>>>>>
>>>>>> Make sure the KRB5CCNAME is popped out of os.environ only if present.
>>>>>>
>>>>>> Tomas
>>>>>
>>>>> NACK, use os.environ.pop('KRB5CCNAME', None) to safely remove it.
>>>>
>>>> Yeah, well, both ways are equivalent, I found the if statement more
>>>> explicit. We can go with the suggested version, if it's more pleasing
>>>> though - patch is attached.
>>>>
>>>>>
>>>>> Also I don't think the comment is necessary, it's quite obvious
>>>>> what the
>>>>> code does.
>>>>>
>>>>
>>>> I don't think an explanatory comment can hurt, ever. Worst thing that
>>>> happens is that somebody is assured that he understands the code
>>>> correctly.
>>>
>>> Actually the worst thing is that someone changes the code without
>>> changing the comment. If the comment does not add any real value, it's
>>> only a maintenance burden.
>>>
>>
>> Yep, that's a real issue in our code base (i.e. I recently removed such
>> a stale comment in f05846e26787da5ef6c996abf823fcc4a7f65e0f). Making
>> sure the comments describe the implementation properly is on the
>> author/reviewer though.
>>
>> What's "added value" highly depends on your skill set, and familiarity
>> with the code base. Particularly the familiarity with the code base can
>> diminish over time even for the author, and those are the times where
>> comments can come to the rescue.
> 
> Let's take a look at the code:
> 
>     if original_value is not None:
>         os.environ['KRB5CCNAME'] = original_value
>     else:
>         os.environ.pop('KRB5CCNAME', None)
> 
> Can you tell me what in there couldn't be obvious to a person with even
> the most basic skill set?
> 
> IMHO a docstring for private_cccache would add some real value, but this
> comment alone does not.
> 
>>
>> For a newcomer to the project, even a trivial comment (from the point of
>> view of the experienced developer) can be valuable.
> 
> Following this logic, there should be a comment for every line of our
> code, which is ridiculous.
> 

Here's the version of the patch with the comment removed.

Tomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0386-3-private_ccache-Harden-the-removal-of-KRB5CCNAME-env-.patch
Type: text/x-patch
Size: 1100 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20151123/cf3dfdcc/attachment.bin>