[pacman-dev] Huge cleanup of add.c and my attempt at fixing FS#7484

[Xavier]

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