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

[Tomas Babej]

On 11/25/2015 09:04 AM, Jan Cholasta wrote:
> 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.
> 

Pushed to master: 1904d7cc3ab33046428dbdcb7c6f521f9e083287