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

[Brian Dolbec <dolsen@gentoo.org>]

On Sun, 11 Jan 2015 17:39:23 -0800
Zac Medico <zmedico@gentoo.org> wrote:

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

> >> 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.


Ok, looking at the actual file in whole, it is even worse.

Here is the full chunk of code that is repeated 4 times with only some
variable names that are different.  Those differences are easily handled via
parameter passing.

		# 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)
			else:
				shutil.copy2(newconf, archive)
		except(IOError, os.error) as why:
			print(_('dispatch-conf: Error copying %(newconf)s to %(archive)s: %(reason)s; fatal') % \
				{"newconf": newconf, "archive": archive, "reason": str(why)}, file=sys.stderr)

That's a total of 14 lines of code for a single function to process.  And only 2 of them are comments

That is 42 lines of redundant code.

That is more than "WORTH IT" and not overkill IMHO.
But I do concede you should make a second commit optimizing this into one function so as to not confuse the optimizing with the original bugfix change.
-- 
Brian Dolbec <dolsen>