Oh ... I was just looking through some old email and I realized that there were more comments in your response than I had initially seen. I had never responded to those and didn't realize that you were waiting for my response. Sorry about that, my responses are below inline. Dan McGee wrote:
On Fri, Jan 16, 2009 at 8:13 PM, Bryan Ischo <bji-keyword-pacman.3644cb@www.ischo.com> wrote:
This change reorganizes the internal code so that packages are resolved one at a time instead of all at once from a list. This will allow a future checkin to prompt the user to see if they'd rather remove unresolvable packages from the tranasaction and continue, or fail the transaction. This change does not affect the actual behavior of libalpm and all tests pass without changes.
Please wrap your commit message at 76 characters so it doesn't make the git-log console output go crazy.
Will do.
-int _alpm_resolvedeps(pmdb_t *local, alpm_list_t *dbs_sync, alpm_list_t *list, - alpm_list_t *remove, alpm_list_t **data) +int _alpm_resolvedeps(pmdb_t *local, alpm_list_t *dbs_sync, pmpkg_t *pkg, + alpm_list_t **packages, alpm_list_t *remove, + alpm_list_t **data)
No need to use two lines for what will fit on one line.
Using tabs of width 4, the above is required to prevent breaking the 80 column rule. If you are using smaller tabs, you might be able to fit more on a line. Using tabs instead of spaces for indentation is the root cause of problems like this, but since the pacman coding conventions require tabs for indentation, there isn't a good way around this problem. That being said, since I just want this patch to get accepted, I'm happy to make the indentation look however you want, even if it breaks the 80 column rule. But I don't know exactly what you want - can you please send me a patch on my patch or something that re-jiggers these lines how you want them to be? In the short term, I'll try to guess what you wanted and hopefully I'll get it right.
+ if(_alpm_pkg_find(*packages, pkg->name) != NULL) { + /* It's already on the list, meaning it's already been resolved and + it and all of its dependencies have already been added */
Drop this comment- its pretty obvious what the conditional is checking.
Obvious to you, maybe. I think the comment is helpful, but I think we have a philosophical difference about comments as becomes apparent from your later comments. I can't see how comments could possibly be harmful; just because something is obvious to you on your read through the code doesn't mean it's obvious to someone else. But like I said, I just want my patch to be accepted, so I'll remove the comment.
+ /* Remove all packages that were added to [*packages], so that + * we return from error cleanly without having affected the + * [*packages] list. Detach the tail of the list beginning at + * [working] from the packages list and free it. */ + if (working == *packages) { + *packages = NULL; + } + else { + (*packages)->prev = working->prev; + (*packages)->prev->next = NULL; + working->prev = NULL; + } + alpm_list_free(working);
Hmm, I'm not a fan of this manual manipulation- the data structures are there for a reason, so we aren't playing with pointers. The problem here is edge cases are likely to break- I'm not sure if you know, but we ensured alpm_list_last() is an O(1) operation for example by linking first->prev to the last item (the inverse is not true).
Yes, I figured out how the linked list code worked via inspection. Having a back pointer from the head to the tail but not a forward pointer from the tail to the head is pretty weird, but I guess it works here. I tried adding a "alpm_list_cut" function to cut the ending segment of a linked list which would have worked here, but it seemed like a pretty arbitrary function to be adding to the linked list code, so I backed it out and just did some manual manipulation here.
You might be better off keeping two independent lists and then finding a way to join them when we are past the point of no return.
Yes, I tried that too, but it was alot more code to manage two lists than it was to simply 'undo' modifications to the list by removing them. Probably the simplest thing to do would be to copy the input list, and then either restore that copy before returning an error, or delete the copy before returning success. It is not the most runtime efficient, but it is simple and it re-uses existing linked list functions. I hope that is sufficient, because I really don't want to complicate the code needlessly by keeping the list segmented in two pieces until a join at the end.
diff --git a/lib/libalpm/deps.h b/lib/libalpm/deps.h index 2f3c450..9dca91d 100644 --- a/lib/libalpm/deps.h +++ b/lib/libalpm/deps.h @@ -48,8 +48,9 @@ void _alpm_depmiss_free(pmdepmissing_t *miss); alpm_list_t *_alpm_sortbydeps(alpm_list_t *targets, int reverse); void _alpm_recursedeps(pmdb_t *db, alpm_list_t *targs, int include_explicit); pmpkg_t *_alpm_resolvedep(pmdepend_t *dep, alpm_list_t *dbs, alpm_list_t *excluding, pmpkg_t *tpkg); -int _alpm_resolvedeps(pmdb_t *local, alpm_list_t *dbs_sync, alpm_list_t *list, - alpm_list_t *remove, alpm_list_t **data); +int _alpm_resolvedeps(pmdb_t *local, alpm_list_t *dbs_sync, pmpkg_t *pkg, + alpm_list_t **packages, alpm_list_t *remove, + alpm_list_t **data);
Coalesce the previous two lines. If you do that I'll stop pulling stupid words like 'coalesce' out of my vocab too. :)
Once again, I cannot without breaking the 80 column rule (with width-4 tab stops). Of course, there's no such thing as "80 columns" when you're using tabs for indentation so I guess it's kind of arbitrary where lines are split. If I set my own tab stops to 2 spaces and then indent for 80 columns will this break lines up the way you'd like to see them?
+ if(trans->flags & PM_TRANS_FLAG_NODEPS) { + /* Simply build up [list] from all of the transaction packages */
Unnecessary comment.
Once again, philosophical differences. But I will do as you ask.
+ if(_alpm_resolvedeps(db_local, dbs_sync, pkg, &list, remove, data) == -1) { + /* Failed to resolve a dependency of [pkg]. It goes on the + unresolvable list. [list] was not touched, so no + unnecessary dependency packages were added to it. */
I know it sounds weird to hate on comments, but it takes me a lot longer to get through the comment than just reading the code and seeing what it says. Shorten or kill this one.
As before, OK.
+ + /* If there were unresolvable top-level packages, fail the + transaction. In a future checkin, the user will be asked if they'd + like to drop the unresolvable packages intead. */
Don't tie in future checkins, we all know its coming. Just drop the comment.
Well I really disagree here. And I don't understand why it's important to change this because this comment is undone in the next patch in the patch set. But as with everything else, I'll change it just to get my patch accepted.
I know I commented a lot, but overall this patch looks fine to me. If you make the adjustments I've pointed out I'll merge this.
I will make the changes you requested. I don't know exactly what I need to do to 'fix' the two cases where I broke up a function prototype/signature because it seems dependent on how you've set your tab stops. But I will do my best. Thanks, Bryan