Mike Kupfer wrote: > I took at look at the webrev; it looks okay. Thanks for cleaning up the > misuse of $PDIR, $PF, $DIR, and $F. And thanks for the summary writeup > in Bugzilla!
You're welcome. Thanks for the review. > > I did notice some (pre-existing) cruftiness in the build_old_new* > functions. > > First wart: build_old_new gets passed in $olddir, $newdir, $DIR, and $F, > but not $PDIR or $PF. > > Second wart: not all the build_old_new* functions use the arguments > they're given. In particular, build_old_new_teamware just goes ahead > and uses the globals. > > I'd like to see this cleaned up, but I don't think it needs to be done > immediately. So your choice: you can fix this stuff now if you want. > Otherwise, please file a bug. I think priority=P3 and milestone="ON > migration" are appropriate. I agree. I will file a separate bug, since the changes so far are already prepared and tested. Also, I usually try to resist making general clean up changes in the same delta as a bug fix, since this makes things easier for the next person trying to follow the code's history. > > I'm not sure what the right cleanup is. Using function parameters and > local variables seems cleaner, but it's real easy to accidentally use a > global instead of a parameter, at which point we haven't really gained > anything. (And in fact we might have made things even more confusing.) > > So if there's some way to make DIR, PDIR, F, and PF local to the loop, > I'd favor using the function parameters for everything. If not, I'd get > rid of the function parameters and just use the globals for DIR, PDIR, > F, and PF; and maybe olddir and newdir, too. > > mike At the very least, the build_old_new* functions can be made more consistent. Then I'll look for ways to eliminate global variables. I'm not sure if the variables can be made local to the loop unless the loop itself becomes a function as well. --Nathan
