1 |
On 01/11/2015 05:21 PM, Brian Dolbec wrote: |
2 |
> On Sun, 11 Jan 2015 00:49:50 -0800 |
3 |
> Zac Medico <zmedico@g.o> wrote: |
4 |
> |
5 |
>> Since commit f17448317166bfac42dc279b8795cd581c189582, an existing |
6 |
>> symlink in /etc/config-archive could trigger a fatal "File exists" |
7 |
>> error. Handle this by removing the destination file if it exists. This |
8 |
>> was not necessary when dispatch-conf only supported regular files, |
9 |
>> since shutil.copy2 would simply overwrite the regular destination |
10 |
>> file. |
11 |
>> |
12 |
>> Fixes: f17448317166 ("dispatch-conf: symlink support for bug #485598") |
13 |
>> X-Gentoo-Bug: 535850 |
14 |
>> X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=535850 |
15 |
>> --- |
16 |
>> pym/portage/dispatch_conf.py | 24 ++++++++++++++++++++++++ |
17 |
>> 1 file changed, 24 insertions(+) |
18 |
>> |
19 |
>> diff --git a/pym/portage/dispatch_conf.py |
20 |
>> b/pym/portage/dispatch_conf.py index b27e68b..7d55182 100644 |
21 |
>> --- a/pym/portage/dispatch_conf.py |
22 |
>> +++ b/pym/portage/dispatch_conf.py |
23 |
>> @@ -179,6 +179,12 @@ def rcs_archive(archive, curconf, newconf, |
24 |
>> mrgconf): if curconf_st is not None and \ |
25 |
>> (stat.S_ISREG(curconf_st.st_mode) or |
26 |
>> stat.S_ISLNK(curconf_st.st_mode)): |
27 |
>> + # Remove destination file in order to ensure that |
28 |
>> the following |
29 |
>> + # symlink or copy2 call won't fail (see bug #535850). |
30 |
>> + try: |
31 |
>> + os.unlink(archive) |
32 |
>> + except OSError: |
33 |
>> + pass |
34 |
>> try: |
35 |
>> if stat.S_ISLNK(curconf_st.st_mode): |
36 |
>> os.symlink(os.readlink(curconf), |
37 |
>> archive) @@ -208,6 +214,12 @@ def rcs_archive(archive, curconf, |
38 |
>> newconf, mrgconf): if has_branch: |
39 |
>> os.rename(archive, archive + '.dist') |
40 |
>> |
41 |
>> + # Remove destination file in order to ensure that |
42 |
>> the following |
43 |
>> + # symlink or copy2 call won't fail (see bug #535850). |
44 |
>> + try: |
45 |
>> + os.unlink(archive) |
46 |
>> + except OSError: |
47 |
>> + pass |
48 |
>> try: |
49 |
>> if stat.S_ISLNK(mystat.st_mode): |
50 |
>> os.symlink(os.readlink(newconf), |
51 |
>> archive) @@ -264,6 +276,12 @@ def file_archive(archive, curconf, |
52 |
>> newconf, mrgconf): if curconf_st is not None and \ |
53 |
>> (stat.S_ISREG(curconf_st.st_mode) or |
54 |
>> stat.S_ISLNK(curconf_st.st_mode)): |
55 |
>> + # Remove destination file in order to ensure that |
56 |
>> the following |
57 |
>> + # symlink or copy2 call won't fail (see bug #535850). |
58 |
>> + try: |
59 |
>> + os.unlink(archive) |
60 |
>> + except OSError: |
61 |
>> + pass |
62 |
>> try: |
63 |
>> if stat.S_ISLNK(curconf_st.st_mode): |
64 |
>> os.symlink(os.readlink(curconf), |
65 |
>> archive) @@ -285,6 +303,12 @@ def file_archive(archive, curconf, |
66 |
>> newconf, mrgconf): stat.S_ISLNK(mystat.st_mode)): |
67 |
>> # Save off new config file in the archive dir |
68 |
>> with .dist.new suffix newconf_archive = archive + '.dist.new' |
69 |
>> + # Remove destination file in order to ensure that |
70 |
>> the following |
71 |
>> + # symlink or copy2 call won't fail (see bug #535850). |
72 |
>> + try: |
73 |
>> + os.unlink(newconf_archive) |
74 |
>> + except OSError: |
75 |
>> + pass |
76 |
>> try: |
77 |
>> if stat.S_ISLNK(mystat.st_mode): |
78 |
>> os.symlink(os.readlink(newconf), |
79 |
>> newconf_archive) |
80 |
> |
81 |
> |
82 |
> That makes duplicated code in 4 places :/ I know it's only 4 lines of |
83 |
> actual code plus 2 more for the same repeated comments. |
84 |
> |
85 |
> BUT... |
86 |
> |
87 |
> This diff doens't show if it's all in the same class. |
88 |
|
89 |
The changes affect 2 different functions (rcs_archive and file_archive), |
90 |
and add 2 unlink calls per function. Both function are very similar, but |
91 |
only one of them is called, depending on whether or not the user has rcs |
92 |
support enabled. |
93 |
|
94 |
> If possible I'd prefer a small breakout function to do the unlink. |
95 |
> Decorate it with @staticmethod even. If it's named right, you won't |
96 |
> even need the comments repeated throughout the code. Just the |
97 |
> docstring with that comment info. |
98 |
|
99 |
Maybe should just omit the comments and leave the unlink calls inline? |
100 |
It feels like overkill to split out a function for this. |
101 |
-- |
102 |
Thanks, |
103 |
Zac |