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

[Jan Cholasta]

On 23.11.2015 15:19, Rob Crittenden wrote:
> Tomas Babej wrote:
>>
>>
>> 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.
>
> IMHO the comment should have been something like "ensure no KRB5CCNAME
> otherwise it blows up in ..." If it took you 20 minutes to track down a
> one-line change then a comment might save the next guy who changes the
> behavior.

As I wrote earlier, I think it would make more sense to put this into 
the docstring of private_ccache().

ACK on the patch.

-- 
Jan Cholasta