| 1 |
On 10/27/2014 01:08 AM, Alexander Berntsen wrote: |
| 2 |
> I'm not sure how anyone is seriously supposed to do an in-depth review |
| 3 |
> of 500+ changes. For what it's worth, what I could stomach reading of |
| 4 |
> the vartree changes, looked relatively sane. The commit messages also |
| 5 |
> seem to indicate you're doing something sane. |
| 6 |
> |
| 7 |
> Is there no way of splitting these things into smaller commits that |
| 8 |
> make sense? |
| 9 |
|
| 10 |
I think the current split into 3 patches is as small as we should go |
| 11 |
here. We've got 3 logical parts, and it makes sense to look at all the |
| 12 |
changes together for each of those parts. |
| 13 |
|
| 14 |
> As they are right now, it's almost pointless to even look |
| 15 |
> at the diff. One would have to look at the old and new source in their |
| 16 |
> entireties next to each other to get the bigger picture. (I might find |
| 17 |
> time for this later this week.) |
| 18 |
|
| 19 |
The dispatch-conf and etc-update changes should be easier to review |
| 20 |
without needing more context than what's in the patches. All of the |
| 21 |
changes there simply to take code that only supported regular files and |
| 22 |
fix it to handle symlinks differently from regular files. |
| 23 |
|
| 24 |
The bigger picture is harder to see in the vartree patch, since |
| 25 |
dblink.mergeme is such a large method. So, it may help if I give some |
| 26 |
more overview in the commit message. The changes to dblink.mergeme do 3 |
| 27 |
things: |
| 28 |
|
| 29 |
1) Move the bulk of config protection logic from dblink.mergeme to a new |
| 30 |
dblink._protect method. The new method only returns 3 variables, which |
| 31 |
makes it easier to understand how config protection interacts with the |
| 32 |
dblink.mergeme code that uses those variables. This is important, since |
| 33 |
dblink.mergeme has so many variables. |
| 34 |
|
| 35 |
2) Initialize more variables at the beginning of dblink.mergeme, since |
| 36 |
those variables are used by the dblink._protect method. |
| 37 |
|
| 38 |
3) Use the variables returned from dblink._protect to trigger |
| 39 |
appropriate behavior later in dblink.mergeme. |
| 40 |
|
| 41 |
The new_protect_filename changes are required since this function |
| 42 |
compares the new file to old ._cfg* files that may already exist, in |
| 43 |
order to avoid creating duplicate .cfg* files. In these comparisons, it |
| 44 |
needs to handle symlinks differently from regular files. |
| 45 |
-- |
| 46 |
Thanks, |
| 47 |
Zac |
Archives