Twas brillig at 02:05:47 17.10.2009 UTC+02 when guillem@debian.org did gyre and gimble:
GJ> Hi!
GJ> For recurring issues I've commented on the first occurence only. And
GJ> I've stopped reviewing in detail at some point as major parts of the
GJ> code need to be rewritten anyway.
Wow. Thanks for really detailed review. I'll comment on questions below
and going to prepare a new patch.
>> +static int
>> +rename_mv(const char *src, const char *dest)
>> +{
>> + pid_t pid;
>> +
>> + if (rename(src, dest) == 0)
>> + return 0;
>> +
>> + pid = fork();
>> + if(pid == -1)
>> + return -1;
>> +
>> + if (!pid) {
>> + /* child */
>> + execlp("mv", "mv", src, dest, NULL);
>> + exit(EXIT_FAILURE);
>> + }
>> +
>> + /* parent */
>> + int status;
>> + if(waitpid(pid, &status, 0) == -1)
>> + return -1;
>> +
>> + return !(WIFEXITED(status) && WEXITSTATUS(status) == 0);
>> +}
GJ> Use the stuff from subproc.h. Or actually just copy it yourself, use
GJ> fd_fd_copy and file_copy_perms.
Ah, did not know about it. One codepath less to check.
>> +static diversion *
>> +invert_diversions_list(diversion *diversions)
>> +{
>> + diversion *n = NULL;
>> + while (diversions)
>> + {
>> + diversion *newhead = diversions;
>> + diversions = newhead->next;
>> + newhead->next = n;
>> + n = newhead;
>> + }
>> + return n;
>> +}
GJ> Instead of inverting the list after the fact, just add to its tail
GJ> instead. OTOH I don't think this should be needed at all.
I have tried to replicate dpkg-divert.pl behaviour exactly, including
sorting of diversions file. If it is not necessary, I could make a
testsuite a bit smarter instead.
>> +static int
>> +match_diversion(diversion *diversion, char *const *patterns)
>> +{
>> + for (; *patterns; patterns++) {
>> + if(!fnmatch(*patterns, diversion->contest, FNM_NOESCAPE))
>> + return 0;
>> + if(!fnmatch(*patterns, diversion->altname, FNM_NOESCAPE))
>> + return 0;
>> + if(!fnmatch(*patterns, diversion->package, FNM_NOESCAPE))
>> + return 0;
GJ> Why the FNM_NOESCAPE?
To match Perl code - '\' is escaped there. Not that it really matters.
--
http://fossarchy.blogspot.com/
Attachment:
pgp70idRsZlv8.pgp
Description: PGP signature