Re: [gentoo-portage-dev] [PATCH] dispatch-conf: avoid symlink "File exists" error (535850)

[Zac Medico <zmedico@gentoo.org>]

On 01/11/2015 05:21 PM, Brian Dolbec wrote:
> On Sun, 11 Jan 2015 00:49:50 -0800
> Zac Medico <zmedico@gentoo.org> wrote:
> 
>> Since commit f17448317166bfac42dc279b8795cd581c189582, an existing
>> symlink in /etc/config-archive could trigger a fatal "File exists"
>> error. Handle this by removing the destination file if it exists. This
>> was not necessary when dispatch-conf only supported regular files,
>> since shutil.copy2 would simply overwrite the regular destination
>> file.
>>
>> Fixes: f17448317166 ("dispatch-conf: symlink support for bug #485598")
>> X-Gentoo-Bug: 535850
>> X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=535850
>> ---
>>  pym/portage/dispatch_conf.py | 24 ++++++++++++++++++++++++
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/pym/portage/dispatch_conf.py
>> b/pym/portage/dispatch_conf.py index b27e68b..7d55182 100644
>> --- a/pym/portage/dispatch_conf.py
>> +++ b/pym/portage/dispatch_conf.py
>> @@ -179,6 +179,12 @@ def rcs_archive(archive, curconf, newconf,
>> mrgconf): if curconf_st is not None and \
>>  		(stat.S_ISREG(curconf_st.st_mode) or
>>  		stat.S_ISLNK(curconf_st.st_mode)):
>> +		# Remove destination file in order to ensure that
>> the following
>> +		# symlink or copy2 call won't fail (see bug #535850).
>> +		try:
>> +			os.unlink(archive)
>> +		except OSError:
>> +			pass
>>  		try:
>>  			if stat.S_ISLNK(curconf_st.st_mode):
>>  				os.symlink(os.readlink(curconf),
>> archive) @@ -208,6 +214,12 @@ def rcs_archive(archive, curconf,
>> newconf, mrgconf): if has_branch:
>>  			os.rename(archive, archive + '.dist')
>>  
>> +		# Remove destination file in order to ensure that
>> the following
>> +		# symlink or copy2 call won't fail (see bug #535850).
>> +		try:
>> +			os.unlink(archive)
>> +		except OSError:
>> +			pass
>>  		try:
>>  			if stat.S_ISLNK(mystat.st_mode):
>>  				os.symlink(os.readlink(newconf),
>> archive) @@ -264,6 +276,12 @@ def file_archive(archive, curconf,
>> newconf, mrgconf): if curconf_st is not None and \
>>  		(stat.S_ISREG(curconf_st.st_mode) or
>>  		stat.S_ISLNK(curconf_st.st_mode)):
>> +		# Remove destination file in order to ensure that
>> the following
>> +		# symlink or copy2 call won't fail (see bug #535850).
>> +		try:
>> +			os.unlink(archive)
>> +		except OSError:
>> +			pass
>>  		try:
>>  			if stat.S_ISLNK(curconf_st.st_mode):
>>  				os.symlink(os.readlink(curconf),
>> archive) @@ -285,6 +303,12 @@ def file_archive(archive, curconf,
>> newconf, mrgconf): stat.S_ISLNK(mystat.st_mode)):
>>  		# Save off new config file in the archive dir
>> with .dist.new suffix newconf_archive = archive + '.dist.new'
>> +		# Remove destination file in order to ensure that
>> the following
>> +		# symlink or copy2 call won't fail (see bug #535850).
>> +		try:
>> +			os.unlink(newconf_archive)
>> +		except OSError:
>> +			pass
>>  		try:
>>  			if stat.S_ISLNK(mystat.st_mode):
>>  				os.symlink(os.readlink(newconf),
>> newconf_archive)
> 
> 
> That makes duplicated code in 4 places :/  I know it's only 4 lines of
> actual code plus 2 more for the same repeated comments.
> 
> BUT...
> 
> This diff doens't show if it's all in the same class.

The changes affect 2 different functions (rcs_archive and file_archive),
and add 2 unlink calls per function. Both function are very similar, but
only one of them is called, depending on whether or not the user has rcs
support enabled.

> If possible I'd prefer a small breakout function to do the unlink.
> Decorate it with @staticmethod even.  If it's named right, you won't
> even need the comments repeated throughout the code.   Just the
> docstring with that comment info.

Maybe should just omit the comments and leave the unlink calls inline?
It feels like overkill to split out a function for this.
-- 
Thanks,
Zac