From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lists.gentoo.org (pigeon.gentoo.org [208.92.234.80]) by finch.gentoo.org (Postfix) with ESMTP id 221EF138A1A for ; Mon, 12 Jan 2015 02:07:43 +0000 (UTC) Received: from pigeon.gentoo.org (localhost [127.0.0.1]) by pigeon.gentoo.org (Postfix) with SMTP id BE157E0807; Mon, 12 Jan 2015 02:07:41 +0000 (UTC) Received: from smtp.gentoo.org (smtp.gentoo.org [140.211.166.183]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by pigeon.gentoo.org (Postfix) with ESMTPS id 27DE0E07EB for ; Mon, 12 Jan 2015 02:07:41 +0000 (UTC) Received: from big_daddy.dol-sen.ca (S010634bdfa9ecf80.vc.shawcable.net [96.49.31.57]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: dolsen) by smtp.gentoo.org (Postfix) with ESMTPSA id 0584A340699 for ; Mon, 12 Jan 2015 02:07:39 +0000 (UTC) Date: Sun, 11 Jan 2015 18:07:25 -0800 From: Brian Dolbec To: gentoo-portage-dev@lists.gentoo.org Subject: Re: [gentoo-portage-dev] [PATCH] dispatch-conf: avoid symlink "File exists" error (535850) Message-ID: <20150111180725.68bde153.dolsen@gentoo.org> In-Reply-To: <54B325CB.10100@gentoo.org> References: <1420966190-7007-1-git-send-email-zmedico@gentoo.org> <20150111172135.13c39400.dolsen@gentoo.org> <54B325CB.10100@gentoo.org> Organization: Gentoo Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-Id: Gentoo Linux mail X-BeenThere: gentoo-portage-dev@lists.gentoo.org Reply-to: gentoo-portage-dev@lists.gentoo.org MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Archives-Salt: ddf326e0-0c7c-4fc8-be0b-a76c73e83ee1 X-Archives-Hash: e50fd947ebe9a5322888d3cadae9bcbe On Sun, 11 Jan 2015 17:39:23 -0800 Zac Medico wrote: > On 01/11/2015 05:21 PM, Brian Dolbec wrote: > > On Sun, 11 Jan 2015 00:49:50 -0800 > > Zac Medico 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