Re: [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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [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

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Tomas Babej]

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
From 201bc398ec59920f7cd34de69de66cb3489f417d Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Mon, 23 Nov 2015 12:47:56 +0100
Subject: [PATCH] private_ccache: Harden the removal of KRB5CCNAME env variable

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.
---
 ipapython/ipautil.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4acdd1a98818bf311a8fef103e7219cc62a28ec1..ec85786e18928838b51573c35e17986ff38f72d9 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1367,7 +1367,9 @@ def private_ccache(path=None):
 if original_value is not None:
 os.environ['KRB5CCNAME'] = original_value
 else:
-os.environ.pop('KRB5CCNAME')
+# No value was set originally, make sure no value is set now
+if 'KRB5CCNAME' in os.environ:
+os.environ.pop('KRB5CCNAME')
 
 if os.path.exists(path):
 os.remove(path)
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Jan Cholasta]

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.

Also I don't think the comment is necessary, it's quite obvious what the 
code does.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Tomas Babej]

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.

Tomas
From 65d4efa341ff73ef6a3a2da9d443577305aa82dd Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Mon, 23 Nov 2015 12:47:56 +0100
Subject: [PATCH] private_ccache: Harden the removal of KRB5CCNAME env variable

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.
---
 ipapython/ipautil.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4acdd1a98818bf311a8fef103e7219cc62a28ec1..b4af0868c15178f75007b4f1e407070ae102ceb4 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1367,7 +1367,8 @@ def private_ccache(path=None):
 if original_value is not None:
 os.environ['KRB5CCNAME'] = original_value
 else:
-os.environ.pop('KRB5CCNAME')
+# No value was set originally, make sure no value is set now
+os.environ.pop('KRB5CCNAME', None)
 
 if os.path.exists(path):
 os.remove(path)
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Martin Kosek]

On 11/23/2015 01:40 PM, 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.
> 
> For a newcomer to the project, even a trivial comment (from the point of
> view of the experienced developer) can be valuable.

While I agree with Tomas in general, in this particular case I also do not see
the value of the comment. All is said in the 4 lines of code, no deep FreeIPA
knowledge required:

 if original_value is not None:
 os.environ['KRB5CCNAME'] = original_value
 else:
-os.environ.pop('KRB5CCNAME')
+# No value was set originally, make sure no value is set now
+os.environ.pop('KRB5CCNAME', None)

I hope the discussion around the comment does not expand too much, there is
enough work in 4.3 to do...

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Jan Cholasta]

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.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Tomas Babej]

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.

For a newcomer to the project, even a trivial comment (from the point of
view of the experienced developer) can be valuable.

Tomas

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [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
From f87ae19e673d4fe2ae1330fb2b1e1a1dbaa55630 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Mon, 23 Nov 2015 12:47:56 +0100
Subject: [PATCH] private_ccache: Harden the removal of KRB5CCNAME env variable

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.
---
 ipapython/ipautil.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4acdd1a98818bf311a8fef103e7219cc62a28ec1..a5545688d61354716745c7814b6bbf1ae316c13d 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1367,7 +1367,7 @@ def private_ccache(path=None):
 if original_value is not None:
 os.environ['KRB5CCNAME'] = original_value
 else:
-os.environ.pop('KRB5CCNAME')
+os.environ.pop('KRB5CCNAME', None)
 
 if os.path.exists(path):
 os.remove(path)
-- 
2.5.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

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

[Rob Crittenden]

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.

rob

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code