[pacman-dev] [PATCH] Corrected return codes for Sg flag
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> This is a follow up from the 'Fix for FS#36097' thread. I tried to reply to it with git send-mail but I can't because I didn't start the thread with send-mail, so sorry for the new thread! I've taken Allan's suggestion and made the behaviour mirror -Ss. A 1 is returned if you search for a group that does not exist, or if you don't search for a specific group and no groups exist. Otherwise a 0 is returned. Ashley Whetter (1): Corrected return codes for Sg flag src/pacman/sync.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) -- 1.8.3.2
On Fri, Jul 12, 2013 at 04:56:31PM +0100, awhetter.2011@my.bristol.ac.uk wrote:
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk>
This is a follow up from the 'Fix for FS#36097' thread. I tried to reply to it with git send-mail but I can't because I didn't start the thread with send-mail, so sorry for the new thread!
You can use the --in-reply-to flag and specify the message ID.
I've taken Allan's suggestion and made the behaviour mirror -Ss. A 1 is returned if you search for a group that does not exist, or if you don't search for a specific group and no groups exist. Otherwise a 0 is returned.
Ashley Whetter (1): Corrected return codes for Sg flag
src/pacman/sync.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
-- 1.8.3.2
I saw that but I thought the message ID was something git generated. I've just found out what it actually is. Thanks for the tip :) On 12 July 2013 16:01, Dave Reisner <d@falconindy.com> wrote:
On Fri, Jul 12, 2013 at 04:56:31PM +0100, awhetter.2011@my.bristol.ac.uk wrote:
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk>
This is a follow up from the 'Fix for FS#36097' thread. I tried to reply to it with git send-mail but I can't because I didn't start the thread with send-mail, so sorry for the new thread!
You can use the --in-reply-to flag and specify the message ID.
I've taken Allan's suggestion and made the behaviour mirror -Ss. A 1 is returned if you search for a group that does not exist, or if you don't search for a specific group and no groups exist. Otherwise a 0 is returned.
Ashley Whetter (1): Corrected return codes for Sg flag
src/pacman/sync.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
-- 1.8.3.2
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> Non-zero is now returned if a group is searched for that doesn't exist Signed-off-by: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> --- src/pacman/sync.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/pacman/sync.c b/src/pacman/sync.c index fc1314b..35a5c59 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -367,6 +367,7 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) { alpm_list_t *i, *j, *k, *s = NULL; + int found_group = 0, found_all = 1; if(targets) { for(i = targets; i; i = alpm_list_next(i)) { @@ -376,6 +377,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_group_t *grp = alpm_db_get_group(db, grpname); if(grp) { + found_group++; /* get names of packages in group */ for(k = grp->packages; k; k = alpm_list_next(k)) { if(!config->quiet) { @@ -387,6 +389,11 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) } } } + + if (!found_group) { + found_all = 0; + } + found_group = 0; } } else { for(i = syncs; i; i = alpm_list_next(i)) { @@ -394,6 +401,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) for(j = alpm_db_get_groupcache(db); j; j = alpm_list_next(j)) { alpm_group_t *grp = j->data; + found_all = 1; if(level > 1) { for(k = grp->packages; k; k = alpm_list_next(k)) { @@ -412,7 +420,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_list_free(s); } - return 0; + return !found_all; } static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) -- 1.8.3.2
On Fri, Jul 12, 2013 at 04:56:32PM +0100, awhetter.2011@my.bristol.ac.uk wrote:
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk>
Non-zero is now returned if a group is searched for that doesn't exist
Signed-off-by: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> --- src/pacman/sync.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/pacman/sync.c b/src/pacman/sync.c index fc1314b..35a5c59 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -367,6 +367,7 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) { alpm_list_t *i, *j, *k, *s = NULL; + int found_group = 0, found_all = 1;
if(targets) { for(i = targets; i; i = alpm_list_next(i)) { @@ -376,6 +377,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_group_t *grp = alpm_db_get_group(db, grpname);
if(grp) { + found_group++; /* get names of packages in group */ for(k = grp->packages; k; k = alpm_list_next(k)) { if(!config->quiet) { @@ -387,6 +389,11 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) } } } + + if (!found_group) { + found_all = 0; + } + found_group = 0; }
Unless I misunderstood something, I don't think this is the logic you described. You're counting each time a group is found in a given db, and then checking that the count is non-zero. Thus you'll only trigger a return code of 1 if _none_ of your searches result in a match. It should return 1 if any one search fails.
} else { for(i = syncs; i; i = alpm_list_next(i)) { @@ -394,6 +401,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets)
for(j = alpm_db_get_groupcache(db); j; j = alpm_list_next(j)) { alpm_group_t *grp = j->data; + found_all = 1;
found_all is already set to one. This else block is completely independent.
- return 0; + return !found_all;
The negation here seems arbitrary and needless. You're explicitly setting found_all to either 1 or 0 in the first place.
I'm pretty sure the logic checks out. I've just run it on the command line to check again. found_group increments whenever the group is in any of the databases. If it's in _none_ of the databases (ie you've !found the group) then found_all is permanently set to 1 so that an error will be returned, and it moves onto searching for the next group. If a group is found then found_all isn't touched, and it moves onto searching for the next group. I think I set found_all to 1 originally because I was going to do something slightly different but I'll initialise it as 0 and rename it to not_found_all when I next update the patch. On 12 July 2013 20:54, Simon Gomizelj <simongmzlj@gmail.com> wrote:
On Fri, Jul 12, 2013 at 04:56:32PM +0100, awhetter.2011@my.bristol.ac.uk wrote:
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk>
Non-zero is now returned if a group is searched for that doesn't exist
Signed-off-by: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> --- src/pacman/sync.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/pacman/sync.c b/src/pacman/sync.c index fc1314b..35a5c59 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -367,6 +367,7 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) { alpm_list_t *i, *j, *k, *s = NULL; + int found_group = 0, found_all = 1;
if(targets) { for(i = targets; i; i = alpm_list_next(i)) { @@ -376,6 +377,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_group_t *grp = alpm_db_get_group(db, grpname);
if(grp) { + found_group++; /* get names of packages in group */ for(k = grp->packages; k; k = alpm_list_next(k)) { if(!config->quiet) { @@ -387,6 +389,11 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) } } } + + if (!found_group) { + found_all = 0; + } + found_group = 0; }
Unless I misunderstood something, I don't think this is the logic you described. You're counting each time a group is found in a given db, and then checking that the count is non-zero. Thus you'll only trigger a return code of 1 if _none_ of your searches result in a match.
It should return 1 if any one search fails.
} else { for(i = syncs; i; i = alpm_list_next(i)) { @@ -394,6 +401,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets)
for(j = alpm_db_get_groupcache(db); j; j = alpm_list_next(j)) { alpm_group_t *grp = j->data; + found_all = 1;
found_all is already set to one. This else block is completely independent.
- return 0; + return !found_all;
The negation here seems arbitrary and needless. You're explicitly setting found_all to either 1 or 0 in the first place.
I'm pretty sure the logic checks out. I've just run it on the command line to check again. found_group increments whenever the group is in any of the databases. If it's in _none_ of the databases (ie you've !found the group) then found_all is permanently set to 1 so that an error will be returned, and it moves onto searching for the next group. If a group is found then found_all isn't touched, and it moves onto searching for the next group.
You're right, my bad. I forgot there's three for loops there. The diff cuts hid the middle one On that note, maybe the variable names could be clearer: rename 'found_group' to 'found_in_db' and 'found_all' to just 'found' (we use found everywhere else in pacman and libalpm). Two more concerns: - You need to keep with the established coding style: https://www.archlinux.org/pacman/HACKING.html Don't put a space after the if. - Don't top post please.
On 13/07/13 01:56, awhetter.2011@my.bristol.ac.uk wrote:
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk>
This is a follow up from the 'Fix for FS#36097' thread. I tried to reply to it with git send-mail but I can't because I didn't start the thread with send-mail, so sorry for the new thread!
I've taken Allan's suggestion and made the behaviour mirror -Ss. A 1 is returned if you search for a group that does not exist, or if you don't search for a specific group and no groups exist. Otherwise a 0 is returned.
Ashley Whetter (1): Corrected return codes for Sg flag
src/pacman/sync.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
Two things to note: 1) Please don't top post when replying to messages on this list. 2) Don't bother with a cover letter unless you have a whole patch series. Instead put notes like this under the "---" line below the commit message in your patch. Allan
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> Non-zero is now returned if a group is searched for that doesn't exist Fixes FS#36097 Signed-off-by: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> --- In this version I've renamed the variables as Simon suggested. Also I noticed that when no targets were specified the behaviour of -Ss wasn't mirrored and a success would always be returned. So I've have to reinitialise 'found' if there are no targets, which isn't ideal but the only other decent options I could think of was to either name 'found' as 'not_found' and flip the 0s and 1s accordingly, or to check the length of alpm_db_get_groupcache(db) by taking the 'j =' initiasation out of the for loop, which seemed worse. Also the way I'm returning 'found' is inconsistent with how it's done in sync_search, so I feel like sync_search should be changed as well because '!found' is more readable than '(found == 0)'. src/pacman/sync.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/pacman/sync.c b/src/pacman/sync.c index fc1314b..313c3cd 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -367,6 +367,7 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) { alpm_list_t *i, *j, *k, *s = NULL; + int found_in_db = 0, found = 1; if(targets) { for(i = targets; i; i = alpm_list_next(i)) { @@ -376,6 +377,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_group_t *grp = alpm_db_get_group(db, grpname); if(grp) { + found_in_db++; /* get names of packages in group */ for(k = grp->packages; k; k = alpm_list_next(k)) { if(!config->quiet) { @@ -387,13 +389,19 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) } } } + if (!found_in_db) { + found = 0; + } + found_in_db = 0; } } else { + found = 0; for(i = syncs; i; i = alpm_list_next(i)) { alpm_db_t *db = i->data; for(j = alpm_db_get_groupcache(db); j; j = alpm_list_next(j)) { alpm_group_t *grp = j->data; + found = 1; if(level > 1) { for(k = grp->packages; k; k = alpm_list_next(k)) { @@ -412,7 +420,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_list_free(s); } - return 0; + return !found; } static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) -- 1.8.3.2
On 14/07/13 02:01, awhetter.2011@my.bristol.ac.uk wrote:
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk>
Non-zero is now returned if a group is searched for that doesn't exist Fixes FS#36097
Signed-off-by: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> --- In this version I've renamed the variables as Simon suggested. Also I noticed that when no targets were specified the behaviour of -Ss wasn't mirrored and a success would always be returned. So I've have to reinitialise 'found' if there are no targets, which isn't ideal but the only other decent options I could think of was to either name 'found' as 'not_found' and flip the 0s and 1s accordingly, or to check the length of alpm_db_get_groupcache(db) by taking the 'j =' initiasation out of the for loop, which seemed worse. Also the way I'm returning 'found' is inconsistent with how it's done in sync_search, so I feel like sync_search should be changed as well because '!found' is more readable than '(found == 0)'. src/pacman/sync.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/pacman/sync.c b/src/pacman/sync.c index fc1314b..313c3cd 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -367,6 +367,7 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) { alpm_list_t *i, *j, *k, *s = NULL; + int found_in_db = 0, found = 1;
I'm going to ask for a change here, which is mostly just a variable renaming... but requires other minor adjustment. The "found" variable is really just tracking our return value. So name it that. Also "found_in_db" makes me concerned that things can be found outside a db! So: found_in_db -> found found -> ret
if(targets) {
found_in_db (-> found) is only used in this scope so define it here.
for(i = targets; i; i = alpm_list_next(i)) { @@ -376,6 +377,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_group_t *grp = alpm_db_get_group(db, grpname);
if(grp) { + found_in_db++; /* get names of packages in group */ for(k = grp->packages; k; k = alpm_list_next(k)) { if(!config->quiet) { @@ -387,13 +389,19 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) } } } + if (!found_in_db) { + found = 0; + } + found_in_db = 0;
Set this at the start of the loop.
} } else { + found = 0; for(i = syncs; i; i = alpm_list_next(i)) { alpm_db_t *db = i->data;
for(j = alpm_db_get_groupcache(db); j; j = alpm_list_next(j)) { alpm_group_t *grp = j->data; + found = 1;
if(level > 1) { for(k = grp->packages; k; k = alpm_list_next(k)) { @@ -412,7 +420,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_list_free(s); }
- return 0; + return !found; }
static int sync_info(alpm_list_t *syncs, alpm_list_t *targets)
On 15 July 2013 06:19, Allan McRae <allan@archlinux.org> wrote:
On 14/07/13 02:01, awhetter.2011@my.bristol.ac.uk wrote:
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk>
Non-zero is now returned if a group is searched for that doesn't exist Fixes FS#36097
Signed-off-by: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> --- In this version I've renamed the variables as Simon suggested. Also I noticed that when no targets were specified the behaviour of -Ss wasn't mirrored and a success would always be returned. So I've have to reinitialise 'found' if there are no targets, which isn't ideal but the only other decent options I could think of was to either name 'found' as 'not_found' and flip the 0s and 1s accordingly, or to check the length of alpm_db_get_groupcache(db) by taking the 'j =' initiasation out of the for loop, which seemed worse. Also the way I'm returning 'found' is inconsistent with how it's done in sync_search, so I feel like sync_search should be changed as well because '!found' is more readable than '(found == 0)'.
What about this issue with returning '!found' or '(found == 0)'? Should I change how I'm doing it, or how sync_search and any other similar functions are doing it? Or neither?
src/pacman/sync.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/pacman/sync.c b/src/pacman/sync.c index fc1314b..313c3cd 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -367,6 +367,7 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) { alpm_list_t *i, *j, *k, *s = NULL; + int found_in_db = 0, found = 1;
I'm going to ask for a change here, which is mostly just a variable renaming... but requires other minor adjustment. The "found" variable is really just tracking our return value. So name it that. Also "found_in_db" makes me concerned that things can be found outside a db! So:
found_in_db -> found found -> ret
if(targets) {
found_in_db (-> found) is only used in this scope so define it here.
I hadn't thought about that. Thanks for the feedbck. I'll get those changed.
for(i = targets; i; i = alpm_list_next(i)) { @@ -376,6 +377,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_group_t *grp = alpm_db_get_group(db, grpname);
if(grp) { + found_in_db++; /* get names of packages in group */ for(k = grp->packages; k; k = alpm_list_next(k)) { if(!config->quiet) { @@ -387,13 +389,19 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) } } } + if (!found_in_db) { + found = 0; + } + found_in_db = 0;
Set this at the start of the loop.
} } else { + found = 0; for(i = syncs; i; i = alpm_list_next(i)) { alpm_db_t *db = i->data;
for(j = alpm_db_get_groupcache(db); j; j = alpm_list_next(j)) { alpm_group_t *grp = j->data; + found = 1;
if(level > 1) { for(k = grp->packages; k; k = alpm_list_next(k)) { @@ -412,7 +420,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_list_free(s); }
- return 0; + return !found; }
static int sync_info(alpm_list_t *syncs, alpm_list_t *targets)
On 15/07/13 17:42, Ashley Whetter wrote:
On 15 July 2013 06:19, Allan McRae <allan@archlinux.org> wrote:
On 14/07/13 02:01, awhetter.2011@my.bristol.ac.uk wrote:
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk>
Non-zero is now returned if a group is searched for that doesn't exist Fixes FS#36097
Signed-off-by: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> --- In this version I've renamed the variables as Simon suggested. Also I noticed that when no targets were specified the behaviour of -Ss wasn't mirrored and a success would always be returned. So I've have to reinitialise 'found' if there are no targets, which isn't ideal but the only other decent options I could think of was to either name 'found' as 'not_found' and flip the 0s and 1s accordingly, or to check the length of alpm_db_get_groupcache(db) by taking the 'j =' initiasation out of the for loop, which seemed worse. Also the way I'm returning 'found' is inconsistent with how it's done in sync_search, so I feel like sync_search should be changed as well because '!found' is more readable than '(found == 0)'.
What about this issue with returning '!found' or '(found == 0)'? Should I change how I'm doing it, or how sync_search and any other similar functions are doing it? Or neither?
I did not answer because following my suggestions that variable will be called ret and store the return value.
src/pacman/sync.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/pacman/sync.c b/src/pacman/sync.c index fc1314b..313c3cd 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -367,6 +367,7 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) { alpm_list_t *i, *j, *k, *s = NULL; + int found_in_db = 0, found = 1;
I'm going to ask for a change here, which is mostly just a variable renaming... but requires other minor adjustment. The "found" variable is really just tracking our return value. So name it that. Also "found_in_db" makes me concerned that things can be found outside a db! So:
found_in_db -> found found -> ret
if(targets) {
found_in_db (-> found) is only used in this scope so define it here.
I hadn't thought about that. Thanks for the feedbck. I'll get those changed.
for(i = targets; i; i = alpm_list_next(i)) { @@ -376,6 +377,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_group_t *grp = alpm_db_get_group(db, grpname);
if(grp) { + found_in_db++; /* get names of packages in group */ for(k = grp->packages; k; k = alpm_list_next(k)) { if(!config->quiet) { @@ -387,13 +389,19 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) } } } + if (!found_in_db) { + found = 0; + } + found_in_db = 0;
Set this at the start of the loop.
} } else { + found = 0; for(i = syncs; i; i = alpm_list_next(i)) { alpm_db_t *db = i->data;
for(j = alpm_db_get_groupcache(db); j; j = alpm_list_next(j)) { alpm_group_t *grp = j->data; + found = 1;
if(level > 1) { for(k = grp->packages; k; k = alpm_list_next(k)) { @@ -412,7 +420,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_list_free(s); }
- return 0; + return !found; }
static int sync_info(alpm_list_t *syncs, alpm_list_t *targets)
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> Non-zero is now returned if a group is searched for that doesn't exist Fixes FS#36097 Signed-off-by: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> --- The only changes in this version are the one's recommended by Allan. 'found' is now called 'ret' and 'found_in_db' is now called 'found'. 'found_in_db' (now 'found') is initialised in the 'if (targets)' condition. src/pacman/sync.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/pacman/sync.c b/src/pacman/sync.c index fc1314b..e211f7b 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -367,8 +367,10 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) { alpm_list_t *i, *j, *k, *s = NULL; + int ret = 0; if(targets) { + int found = 0; for(i = targets; i; i = alpm_list_next(i)) { const char *grpname = i->data; for(j = syncs; j; j = alpm_list_next(j)) { @@ -376,6 +378,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_group_t *grp = alpm_db_get_group(db, grpname); if(grp) { + found++; /* get names of packages in group */ for(k = grp->packages; k; k = alpm_list_next(k)) { if(!config->quiet) { @@ -387,13 +390,19 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) } } } + if (!found) { + ret = 1; + } + found = 0; } } else { + ret = 1; for(i = syncs; i; i = alpm_list_next(i)) { alpm_db_t *db = i->data; for(j = alpm_db_get_groupcache(db); j; j = alpm_list_next(j)) { alpm_group_t *grp = j->data; + ret = 0; if(level > 1) { for(k = grp->packages; k; k = alpm_list_next(k)) { @@ -412,7 +421,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_list_free(s); } - return 0; + return ret; } static int sync_info(alpm_list_t *syncs, alpm_list_t *targets) -- 1.8.3.3
On 16/07/13 19:18, awhetter.2011@my.bristol.ac.uk wrote:
From: Ashley Whetter <awhetter.2011@my.bristol.ac.uk>
Non-zero is now returned if a group is searched for that doesn't exist Fixes FS#36097
Please us proper punctuation in commit messages. That means fullstops at the end of sentences. I will fix when committing.
Signed-off-by: Ashley Whetter <awhetter.2011@my.bristol.ac.uk> --- The only changes in this version are the one's recommended by Allan. 'found' is now called 'ret' and 'found_in_db' is now called 'found'. 'found_in_db' (now 'found') is initialised in the 'if (targets)' condition. src/pacman/sync.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/src/pacman/sync.c b/src/pacman/sync.c index fc1314b..e211f7b 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -367,8 +367,10 @@ static int sync_search(alpm_list_t *syncs, alpm_list_t *targets) static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) { alpm_list_t *i, *j, *k, *s = NULL; + int ret = 0;
if(targets) { + int found = 0; for(i = targets; i; i = alpm_list_next(i)) { const char *grpname = i->data; for(j = syncs; j; j = alpm_list_next(j)) { @@ -376,6 +378,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_group_t *grp = alpm_db_get_group(db, grpname);
if(grp) { + found++; /* get names of packages in group */ for(k = grp->packages; k; k = alpm_list_next(k)) { if(!config->quiet) { @@ -387,13 +390,19 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) } } } + if (!found) { + ret = 1; + } + found = 0;
The only change I am making is to move this to the start of the loop. I'd much rather it is set there than relying on it being set at the end. Then if a "continue" ever gets put in the loop, it will still work.
} } else { + ret = 1; for(i = syncs; i; i = alpm_list_next(i)) { alpm_db_t *db = i->data;
for(j = alpm_db_get_groupcache(db); j; j = alpm_list_next(j)) { alpm_group_t *grp = j->data; + ret = 0;
if(level > 1) { for(k = grp->packages; k; k = alpm_list_next(k)) { @@ -412,7 +421,7 @@ static int sync_group(int level, alpm_list_t *syncs, alpm_list_t *targets) alpm_list_free(s); }
- return 0; + return ret; }
static int sync_info(alpm_list_t *syncs, alpm_list_t *targets)
participants (5)
-
Allan McRae
-
Ashley Whetter
-
awhetter.2011＠my.bristol.ac.uk
-
Dave Reisner
-
Simon Gomizelj