On Sat, Jul 14, 2007 at 02:54:50PM -0400, Dan McGee wrote:
Ok, I've been slaving away at this for a while and libalpm/add.c is finally getting a bit more manageable. Showing a patch is worthless because it is huge and is much harder to follow due to the refactoring stuff. Instead I've attached a drop-in add.c that compiles cleanly with no other changes needed. I'm looking for some feedback on this.
Some highlights: 1. All pactests still pass, so no regressions on that front. 2. *Much* cleaner layout. add_commit has now been split across four functions (upgrade_remove, extract_single_file, commit_single_pkg, and _alpm_add_commit). 3. No more super functions. The heaviest function (extract_single_file) now weighs in at 350 lines, and a lot of that is comments. Comments/concerns/questions appreciated. I still want to cleanup the huge if(needbackup) section a bit, that is still quite long but hard to refactor.
Very nice job here, but yes, there is still that part to clean up. Maybe the suggestion from Aaron here could give some ideas : http://www.archlinux.org/pipermail/pacman-dev/2006-December/006286.html
4. Most of what Andrew started to do in his 7484 patch has been brought into this. However, I rewrote it a bit for clarity (at least I thought so). We may need a bit more meat added to this section.
Yes I find it more clear that way, but there are still several things to take care of in my opinion. + /* Check for file existence. This is one of the more crucial parts + * to get 'right'. Here are the possibilities, with the filesystem + * on the left and the package on the top: + * (F=file, N=node, S=symlink, D=dir) + * | F/N | S | D + * non-existent | 1 | 2 | 3 + * F/N | 4 | 5 | 6 + * S | 7 | 8 | 9 + * D | 10 | 11 | 12 + * + * 1,2,3- just extract, no magic necessary. lstat will fail here. + * 4,5,6,7,8,10- conflict checks should have caught this. overwrite/backup. + * 9- follow the symlink, hopefully it is a directory + * 11,12- skip extraction, dir already exists + */ About cases 6 and 10, I agree that conflict checks should catch this, but do they ? (they don't from my understanding of the code) About cases 9 and 11, I think it depends on the nature of the symlink : * if the symlink points to a directory, then skip extraction * if not, then conflict