[pacman-dev] [PATCH 1/4] Internal code reorganization in preparation for changes to package resolution.
Bryan Ischo
bji-keyword-pacman.3644cb at www.ischo.com
Mon Jan 26 06:27:05 EST 2009
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 at 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
More information about the pacman-dev
mailing list