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

[Jan Cholasta]

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.

-- 
Jan Cholasta