[pacman-dev] [PATCH v2 1/6] pacsearch: removed useless comment
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 1 - 1 file changed, 1 deletion(-) diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 21043d4..71e0107 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -138,7 +138,6 @@ if ($#querypkgs >= 0) { foreach $_ (@querypkgs) { # we grab the following fields: repo, name, ver, group, installed, and desc my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))? ?(\[.*\])?\n(.*)$/s; - # my @pkgfields = /^(.*?)\/(.*?) ?(\[.*\])?\n(.*)$/s; # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out -- 1.8.5.3
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 71e0107..d3d461f 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -124,7 +124,7 @@ foreach $_ (@syncpkgs) { # add a last field that indicates original order push (@pkgfields, $cnt++); # add each sync pkg by name/ver to a hash table for quick lookup - $allpkgs{$pkgfields[1]} = [ @pkgfields ]; + $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; } my $queryout = `pacman -Qs '@ARGV'`; @@ -141,14 +141,14 @@ foreach $_ (@querypkgs) { # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out - if (not exists $allpkgs{$pkgfields[1]}) { + if (not exists $allpkgs{$pkgfields[1] . $pkgfields[2]}) { # since 'group' is optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = "[$LC_INSTALLED]"; # add a last field that indicates original order (after sync) push (@pkgfields, $cnt++); # add our local-only package to the hash - $allpkgs{$pkgfields[1]} = [ @pkgfields ]; + $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; } } -- 1.8.5.3
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
What is the gain here? The pacman output already includes the local version in the sync results if it's different from the sync version.
diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 71e0107..d3d461f 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -124,7 +124,7 @@ foreach $_ (@syncpkgs) { # add a last field that indicates original order push (@pkgfields, $cnt++); # add each sync pkg by name/ver to a hash table for quick lookup - $allpkgs{$pkgfields[1]} = [ @pkgfields ]; + $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; }
my $queryout = `pacman -Qs '@ARGV'`; @@ -141,14 +141,14 @@ foreach $_ (@querypkgs) { # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out - if (not exists $allpkgs{$pkgfields[1]}) { + if (not exists $allpkgs{$pkgfields[1] . $pkgfields[2]}) { # since 'group' is optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = "[$LC_INSTALLED]"; # add a last field that indicates original order (after sync) push (@pkgfields, $cnt++); # add our local-only package to the hash - $allpkgs{$pkgfields[1]} = [ @pkgfields ]; + $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; } }
-- 1.8.5.3
On 14-02-05 09:15:28, Andrew Gregory wrote:
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
What is the gain here? The pacman output already includes the local version in the sync results if it's different from the sync version.
Not sure I understood your comment correctly. In the old pacsearch pkgname and pkgver were stored in the same cell. When I changed that later on, I inadvertently introduced a regression... This fixes it.
diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 71e0107..d3d461f 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -124,7 +124,7 @@ foreach $_ (@syncpkgs) { # add a last field that indicates original order push (@pkgfields, $cnt++); # add each sync pkg by name/ver to a hash table for quick lookup - $allpkgs{$pkgfields[1]} = [ @pkgfields ]; + $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; }
my $queryout = `pacman -Qs '@ARGV'`; @@ -141,14 +141,14 @@ foreach $_ (@querypkgs) { # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out - if (not exists $allpkgs{$pkgfields[1]}) { + if (not exists $allpkgs{$pkgfields[1] . $pkgfields[2]}) { # since 'group' is optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = "[$LC_INSTALLED]"; # add a last field that indicates original order (after sync) push (@pkgfields, $cnt++); # add our local-only package to the hash - $allpkgs{$pkgfields[1]} = [ @pkgfields ]; + $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; } }
-- 1.8.5.3
-- Pierre Neidhardt Q: Why did the astrophysicist order three hamburgers? A: Because he was hungry.
On 02/05/14 at 04:15pm, Pierre Neidhardt wrote:
On 14-02-05 09:15:28, Andrew Gregory wrote:
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
What is the gain here? The pacman output already includes the local version in the sync results if it's different from the sync version.
Not sure I understood your comment correctly. In the old pacsearch pkgname and pkgver were stored in the same cell. When I changed that later on, I inadvertently introduced a regression... This fixes it.
Ah, I see. That would have been a good detail to include in the commit message.
diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 71e0107..d3d461f 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -124,7 +124,7 @@ foreach $_ (@syncpkgs) { # add a last field that indicates original order push (@pkgfields, $cnt++); # add each sync pkg by name/ver to a hash table for quick lookup - $allpkgs{$pkgfields[1]} = [ @pkgfields ]; + $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; }
my $queryout = `pacman -Qs '@ARGV'`; @@ -141,14 +141,14 @@ foreach $_ (@querypkgs) { # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out - if (not exists $allpkgs{$pkgfields[1]}) { + if (not exists $allpkgs{$pkgfields[1] . $pkgfields[2]}) { # since 'group' is optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = "[$LC_INSTALLED]"; # add a last field that indicates original order (after sync) push (@pkgfields, $cnt++); # add our local-only package to the hash - $allpkgs{$pkgfields[1]} = [ @pkgfields ]; + $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; } }
-- 1.8.5.3
On 07/02/14 12:07, Andrew Gregory wrote:
On 02/05/14 at 04:15pm, Pierre Neidhardt wrote:
On 14-02-05 09:15:28, Andrew Gregory wrote:
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
What is the gain here? The pacman output already includes the local version in the sync results if it's different from the sync version.
Not sure I understood your comment correctly. In the old pacsearch pkgname and pkgver were stored in the same cell. When I changed that later on, I inadvertently introduced a regression... This fixes it.
Ah, I see. That would have been a good detail to include in the commit message.
Yes it would be good to have a detailed commit message, as I am still not sure what exactly the regression is here? Allan
On 14-02-09 11:22:03, Allan McRae wrote:
On 07/02/14 12:07, Andrew Gregory wrote:
On 02/05/14 at 04:15pm, Pierre Neidhardt wrote:
On 14-02-05 09:15:28, Andrew Gregory wrote:
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
What is the gain here? The pacman output already includes the local version in the sync results if it's different from the sync version.
Not sure I understood your comment correctly. In the old pacsearch pkgname and pkgver were stored in the same cell. When I changed that later on, I inadvertently introduced a regression... This fixes it.
Ah, I see. That would have been a good detail to include in the commit message.
Yes it would be good to have a detailed commit message, as I am still not sure what exactly the regression is here?
Allan
In the old pacsearch, packages were identified uniquely by pkgfields[1], which contained pkgname+pkgver. My first patch wave stored pkgver in pkgfields[2], and continued to identify packages with pkgfields[1] only. Because of that packages with a different version would appear once only. What I'm doing now is identifying packages with both pkgfields[1] and pkgfields[2]. -- Pierre Neidhardt When smashing monuments, save the pedstals -- they always come in handy. -- Stanislaw J. Lem, "Unkempt Thoughts"
Package are processed in the same order as pacman output, so there is no real need to sort, we can print directly. This makes the code simpler and faster. The only difference is that local packages will always be printed at the end. Previously, they were printed before multilib for instance. Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 46 ++++++++++++++++------------------------------ 1 file changed, 16 insertions(+), 30 deletions(-) diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index d3d461f..66b1683 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -81,21 +81,21 @@ if ($ARGV[0] eq "--nocolor" || $ARGV[0] eq "-n") { # localization my $LC_INSTALLED = `gettext pacman installed`; -# Color a "repo/pkgname pkgver (groups) [installed]" line. -# We try to stick to pacman colors. -sub to_color { +# Print a "repo/pkgname pkgver (groups) [installed]" line. +# We stick to pacman colors. +sub print_pkg { my @v = @_; - my $line = "$RESET$BOLD"; + print "$RESET$BOLD"; if ( "$v[0]" eq "local" ) { - $line .= "$RED"; + print "$RED"; } else { - $line .= "$MAGENTA"; + print "$MAGENTA"; } - $line .= "$v[0]/$RESET$BOLD$v[1] $GREEN$v[2]"; - $line .= " $BLUE$v[3]" if $v[3] ne ""; - $line .= " $CYAN$v[4]" if $v[4] ne ""; - $line .= " $RESET"; - return $line; + print "$v[0]/$RESET$BOLD$v[1] $GREEN$v[2]"; + print " $BLUE$v[3]" if $v[3] ne ""; + print " $CYAN$v[4]" if $v[4] ne ""; + print " $RESET\n"; + print " $v[5]\n"; } my %allpkgs = (); @@ -108,8 +108,6 @@ if ($#syncpkgs >= 0) { chomp($syncpkgs[$#syncpkgs]); } -# counter var for packages, used here and in the query loop too -my $cnt = 0; foreach $_ (@syncpkgs) { # we grab the following fields: repo, name, ver, group, installed, and desc my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))? ?(\[.*\])?\n(.*)$/s; @@ -121,10 +119,10 @@ foreach $_ (@syncpkgs) { # since 'group' and 'installed' are optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = "" if not defined $pkgfields[4]; - # add a last field that indicates original order - push (@pkgfields, $cnt++); - # add each sync pkg by name/ver to a hash table for quick lookup - $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; + # Add each sync pkg by name/ver to a hash table. + # Any value is good since we only check for existence. + $allpkgs{$pkgfields[1] . $pkgfields[2]} = 1; + print_pkg(@pkgfields); } my $queryout = `pacman -Qs '@ARGV'`; @@ -145,20 +143,8 @@ foreach $_ (@querypkgs) { # since 'group' is optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = "[$LC_INSTALLED]"; - # add a last field that indicates original order (after sync) - push (@pkgfields, $cnt++); - # add our local-only package to the hash - $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; + print_pkg(@pkgfields); } } -# sort by original order (the last field) and print -foreach $_ ( sort{ @{$allpkgs{$a}}[6] <=> @{$allpkgs{$b}}[6] } keys %allpkgs) { - my @v = @{$allpkgs{$_}}; - my $line = to_color(@v); - # print colorized "repo/pkgname pkgver ..." string with possible installed text - print "$line\n"; - print "$v[5]\n"; -} - #vim: set noet: -- 1.8.5.3
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Package are processed in the same order as pacman output, so there is no real need to sort, we can print directly. This makes the code simpler and faster.
The only difference is that local packages will always be printed at the end. Previously, they were printed before multilib for instance.
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 46 ++++++++++++++++------------------------------ 1 file changed, 16 insertions(+), 30 deletions(-)
This adds an 8 second delay between sync and local results for me on a cold run. It's not a deal-breaker for me, but I'd prefer to remove the sort by just storing the results in a list.
diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index d3d461f..66b1683 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -81,21 +81,21 @@ if ($ARGV[0] eq "--nocolor" || $ARGV[0] eq "-n") { # localization my $LC_INSTALLED = `gettext pacman installed`;
-# Color a "repo/pkgname pkgver (groups) [installed]" line. -# We try to stick to pacman colors. -sub to_color { +# Print a "repo/pkgname pkgver (groups) [installed]" line. +# We stick to pacman colors. +sub print_pkg { my @v = @_; - my $line = "$RESET$BOLD"; + print "$RESET$BOLD"; if ( "$v[0]" eq "local" ) { - $line .= "$RED"; + print "$RED"; } else { - $line .= "$MAGENTA"; + print "$MAGENTA"; } - $line .= "$v[0]/$RESET$BOLD$v[1] $GREEN$v[2]"; - $line .= " $BLUE$v[3]" if $v[3] ne ""; - $line .= " $CYAN$v[4]" if $v[4] ne ""; - $line .= " $RESET"; - return $line; + print "$v[0]/$RESET$BOLD$v[1] $GREEN$v[2]"; + print " $BLUE$v[3]" if $v[3] ne ""; + print " $CYAN$v[4]" if $v[4] ne ""; + print " $RESET\n"; + print " $v[5]\n"; }
my %allpkgs = (); @@ -108,8 +108,6 @@ if ($#syncpkgs >= 0) { chomp($syncpkgs[$#syncpkgs]); }
-# counter var for packages, used here and in the query loop too -my $cnt = 0; foreach $_ (@syncpkgs) { # we grab the following fields: repo, name, ver, group, installed, and desc my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))? ?(\[.*\])?\n(.*)$/s; @@ -121,10 +119,10 @@ foreach $_ (@syncpkgs) { # since 'group' and 'installed' are optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = "" if not defined $pkgfields[4]; - # add a last field that indicates original order - push (@pkgfields, $cnt++); - # add each sync pkg by name/ver to a hash table for quick lookup - $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; + # Add each sync pkg by name/ver to a hash table. + # Any value is good since we only check for existence. + $allpkgs{$pkgfields[1] . $pkgfields[2]} = 1; + print_pkg(@pkgfields); }
my $queryout = `pacman -Qs '@ARGV'`; @@ -145,20 +143,8 @@ foreach $_ (@querypkgs) { # since 'group' is optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = "[$LC_INSTALLED]"; - # add a last field that indicates original order (after sync) - push (@pkgfields, $cnt++); - # add our local-only package to the hash - $allpkgs{$pkgfields[1] . $pkgfields[2]} = [ @pkgfields ]; + print_pkg(@pkgfields); } }
-# sort by original order (the last field) and print -foreach $_ ( sort{ @{$allpkgs{$a}}[6] <=> @{$allpkgs{$b}}[6] } keys %allpkgs) { - my @v = @{$allpkgs{$_}}; - my $line = to_color(@v); - # print colorized "repo/pkgname pkgver ..." string with possible installed text - print "$line\n"; - print "$v[5]\n"; -} - #vim: set noet: -- 1.8.5.3
On 14-02-05 09:18:48, Andrew Gregory wrote:
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Package are processed in the same order as pacman output, so there is no real need to sort, we can print directly. This makes the code simpler and faster.
The only difference is that local packages will always be printed at the end. Previously, they were printed before multilib for instance.
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 46 ++++++++++++++++------------------------------ 1 file changed, 16 insertions(+), 30 deletions(-)
This adds an 8 second delay between sync and local results for me on a cold run. It's not a deal-breaker for me, but I'd prefer to remove the sort by just storing the results in a list.
How is the overall runtime? Your alternative sounds OK to me, but why is it so important to have everything printed in one run? -- Pierre Neidhardt You're ugly and your mother dresses you funny.
On 06/02/14 01:16, Pierre Neidhardt wrote:
On 14-02-05 09:18:48, Andrew Gregory wrote:
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Package are processed in the same order as pacman output, so there is no real need to sort, we can print directly. This makes the code simpler and faster.
The only difference is that local packages will always be printed at the end. Previously, they were printed before multilib for instance.
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 46 ++++++++++++++++------------------------------ 1 file changed, 16 insertions(+), 30 deletions(-)
This adds an 8 second delay between sync and local results for me on a cold run. It's not a deal-breaker for me, but I'd prefer to remove the sort by just storing the results in a list.
How is the overall runtime?
Your alternative sounds OK to me, but why is it so important to have everything printed in one run?
It is bad to have software freeze in the middle of output. The user has no indication what happened. Allan
On 14-02-09 11:17:06, Allan McRae wrote:
On 06/02/14 01:16, Pierre Neidhardt wrote:
On 14-02-05 09:18:48, Andrew Gregory wrote:
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Package are processed in the same order as pacman output, so there is no real need to sort, we can print directly. This makes the code simpler and faster.
The only difference is that local packages will always be printed at the end. Previously, they were printed before multilib for instance.
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 46 ++++++++++++++++------------------------------ 1 file changed, 16 insertions(+), 30 deletions(-)
This adds an 8 second delay between sync and local results for me on a cold run. It's not a deal-breaker for me, but I'd prefer to remove the sort by just storing the results in a list.
How is the overall runtime?
Your alternative sounds OK to me, but why is it so important to have everything printed in one run?
It is bad to have software freeze in the middle of output. The user has no indication what happened.
Allan
Right. I did not realize since the "freeze" is below 100ms on my machine. I'll use Andrew's solution. -- Pierre Neidhardt I'm receiving a coded message from EUBIE BLAKE!!
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 66b1683..5656b07 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -134,8 +134,9 @@ if ($#querypkgs >= 0) { } foreach $_ (@querypkgs) { - # we grab the following fields: repo, name, ver, group, installed, and desc - my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))? ?(\[.*\])?\n(.*)$/s; + # We grab the same field as before, even the "installed" which is always + # empty for local searches. This allows us to reserve a cell in @pkgfields. + my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))?()?\n(.*)$/s; # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out -- 1.8.5.3
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
-1 from me. I think this is a step in the wrong direction. The sync and local search code are virtually identical and should be consolidated into a function, not made more different.
diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 66b1683..5656b07 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -134,8 +134,9 @@ if ($#querypkgs >= 0) { }
foreach $_ (@querypkgs) { - # we grab the following fields: repo, name, ver, group, installed, and desc - my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))? ?(\[.*\])?\n(.*)$/s; + # We grab the same field as before, even the "installed" which is always + # empty for local searches. This allows us to reserve a cell in @pkgfields. + my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))?()?\n(.*)$/s; # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out -- 1.8.5.3
On 14-02-05 09:22:49, Andrew Gregory wrote:
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
-1 from me. I think this is a step in the wrong direction. The sync and local search code are virtually identical and should be consolidated into a function, not made more different.
These part were always ~ 50% different. It's less thant 10 lines of code anyway, no big deal. Trying to make a common function out of this would result in longer and more complex code I believe.
diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 66b1683..5656b07 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -134,8 +134,9 @@ if ($#querypkgs >= 0) { }
foreach $_ (@querypkgs) { - # we grab the following fields: repo, name, ver, group, installed, and desc - my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))? ?(\[.*\])?\n(.*)$/s; + # We grab the same field as before, even the "installed" which is always + # empty for local searches. This allows us to reserve a cell in @pkgfields. + my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))?()?\n(.*)$/s; # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out -- 1.8.5.3
-- Pierre Neidhardt Your lover will never wish to leave you.
On 02/05/14 at 04:19pm, Pierre Neidhardt wrote:
On 14-02-05 09:22:49, Andrew Gregory wrote:
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
-1 from me. I think this is a step in the wrong direction. The sync and local search code are virtually identical and should be consolidated into a function, not made more different.
These part were always ~ 50% different. It's less thant 10 lines of code anyway, no big deal. Trying to make a common function out of this would result in longer and more complex code I believe.
The sync search code is 21 lines. Only 1 line of that is actually unique to it and can very easily be moved outside of a consolidated function. The only significant difference between the two is that the sync search prints lines it can't parse and the local search doesn't. Neither the script nor the git log explains the disparity and I don't see any reason to continue it.
diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 66b1683..5656b07 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -134,8 +134,9 @@ if ($#querypkgs >= 0) { }
foreach $_ (@querypkgs) { - # we grab the following fields: repo, name, ver, group, installed, and desc - my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))? ?(\[.*\])?\n(.*)$/s; + # We grab the same field as before, even the "installed" which is always + # empty for local searches. This allows us to reserve a cell in @pkgfields. + my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))?()?\n(.*)$/s; # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out -- 1.8.5.3
On 14-02-06 21:43:58, Andrew Gregory wrote:
These part were always ~ 50% different. It's less thant 10 lines of code anyway, no big deal. Trying to make a common function out of this would result in longer and more complex code I believe.
The sync search code is 21 lines. Only 1 line of that is actually unique to it and can very easily be moved outside of a consolidated function. The only significant difference between the two is that the sync search prints lines it can't parse and the local search doesn't. Neither the script nor the git log explains the disparity and I don't see any reason to continue it.
I've tried to factor the code into one function and see how it looks. The naive funcion (without much modification from previous version) had 8 lines common to both -Ss and -Qs, 3 different lines, and 6 lines added during the factoring process. No big win if you ask me... It adds one branching in the middle to execute code specific to either -Ss or -Qs. Ugly. Saved 5 lines in total. With a little trick I could remove this condition, thus having in total 0 different line (everything is factored) and 6 additional lines. 1 line is run for nothing in the -Qs case. No big loss, but not very pretty. The function calls is not very elegant either. Saved 8 lines in total. Now we need to decide about what we want: "clarity and efficiency" versus "compactness and no code duplicates". I'll post a patch as soon as possible for you to see how it looks. -- Pierre Neidhardt "Those who believe in astrology are living in houses with foundations of Silly Putty." - Dennis Rawlins, astronomer
We include the leading space in the match for 'group' and 'installed'. This allows us to remove the conditions when printing. Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 5656b07..2fde52f 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -91,11 +91,8 @@ sub print_pkg { } else { print "$MAGENTA"; } - print "$v[0]/$RESET$BOLD$v[1] $GREEN$v[2]"; - print " $BLUE$v[3]" if $v[3] ne ""; - print " $CYAN$v[4]" if $v[4] ne ""; - print " $RESET\n"; - print " $v[5]\n"; + print "$v[0]/$RESET$BOLD$v[1] $GREEN$v[2]$BLUE$v[3]$CYAN$v[4]$RESET\n"; + print "$v[5]\n"; } my %allpkgs = (); @@ -109,8 +106,10 @@ if ($#syncpkgs >= 0) { } foreach $_ (@syncpkgs) { - # we grab the following fields: repo, name, ver, group, installed, and desc - my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))? ?(\[.*\])?\n(.*)$/s; + # We grab the following fields: repo, name, ver, group, installed, and + # desc. We grab leading space for 'group' and 'installed' so that we do not + # need to test if non-empty when printing. + my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?( \[.*\])?\n(.*)$/s; if(not @pkgfields) { # skip any non-matching line and just print it for the user print $_, "\n"; @@ -136,14 +135,14 @@ if ($#querypkgs >= 0) { foreach $_ (@querypkgs) { # We grab the same field as before, even the "installed" which is always # empty for local searches. This allows us to reserve a cell in @pkgfields. - my @pkgfields = /^(.*?)\/(.*?) (.*?) ?(\(.*?\))?()?\n(.*)$/s; + my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?()?\n(.*)$/s; # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out if (not exists $allpkgs{$pkgfields[1] . $pkgfields[2]}) { # since 'group' is optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; - $pkgfields[4] = "[$LC_INSTALLED]"; + $pkgfields[4] = " [$LC_INSTALLED]"; print_pkg(@pkgfields); } } -- 1.8.5.3
Previously only one pattern was allowed. $ pacsearch foo bar Search for packages containing 'foo bar'. $ pacman -Ss foo bar Search for packages containing both 'foo' and 'bar'. Note that removing the quotes from the call was not enough since $ pacsearch 'foo|bar' would then fail. Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 2fde52f..f8ebfbd 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -92,24 +92,18 @@ sub print_pkg { print "$MAGENTA"; } print "$v[0]/$RESET$BOLD$v[1] $GREEN$v[2]$BLUE$v[3]$CYAN$v[4]$RESET\n"; - print "$v[5]\n"; + print "$v[5]"; } my %allpkgs = (); -my $syncout = `pacman -Ss '@ARGV'`; -# split each sync search entry into its own array entry -my @syncpkgs = split(/\n^(?=\w)/m, $syncout); -# remove the extra \n from the last desc entry -if ($#syncpkgs >= 0) { - chomp($syncpkgs[$#syncpkgs]); -} - -foreach $_ (@syncpkgs) { +open (my $syncout, '-|', 'pacman', '-Ss', @ARGV); +while (<$syncout>) { # We grab the following fields: repo, name, ver, group, installed, and # desc. We grab leading space for 'group' and 'installed' so that we do not # need to test if non-empty when printing. - my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?( \[.*\])?\n(.*)$/s; + my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?( \[.*\])?$/s; + my $desc = <$syncout>; if(not @pkgfields) { # skip any non-matching line and just print it for the user print $_, "\n"; @@ -118,24 +112,20 @@ foreach $_ (@syncpkgs) { # since 'group' and 'installed' are optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = "" if not defined $pkgfields[4]; + $pkgfields[5] = $desc; # Add each sync pkg by name/ver to a hash table. # Any value is good since we only check for existence. $allpkgs{$pkgfields[1] . $pkgfields[2]} = 1; print_pkg(@pkgfields); } +close ($syncout); -my $queryout = `pacman -Qs '@ARGV'`; -# split each querysearch entry into its own array entry -my @querypkgs = split(/\n^(?=\w)/m, $queryout); -# remove the extra \n from the last desc entry -if ($#querypkgs >= 0) { - chomp ($querypkgs[$#querypkgs]); -} - -foreach $_ (@querypkgs) { +open (my $queryout, '-|', 'pacman', '-Qs', @ARGV); +while (<$queryout>) { # We grab the same field as before, even the "installed" which is always # empty for local searches. This allows us to reserve a cell in @pkgfields. - my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?()?\n(.*)$/s; + my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?()?$/s; + my $desc = <$queryout>; # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out @@ -143,8 +133,10 @@ foreach $_ (@querypkgs) { # since 'group' is optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = " [$LC_INSTALLED]"; + $pkgfields[5] = $desc; print_pkg(@pkgfields); } } +close ($queryout); #vim: set noet: -- 1.8.5.3
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
Previously only one pattern was allowed.
$ pacsearch foo bar Search for packages containing 'foo bar'.
$ pacman -Ss foo bar Search for packages containing both 'foo' and 'bar'.
Note that removing the quotes from the call was not enough since $ pacsearch 'foo|bar' would then fail.
Signed-off-by: Pierre Neidhardt <ambrevar@gmail.com> --- contrib/pacsearch.in | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/contrib/pacsearch.in b/contrib/pacsearch.in index 2fde52f..f8ebfbd 100644 --- a/contrib/pacsearch.in +++ b/contrib/pacsearch.in @@ -92,24 +92,18 @@ sub print_pkg { print "$MAGENTA"; } print "$v[0]/$RESET$BOLD$v[1] $GREEN$v[2]$BLUE$v[3]$CYAN$v[4]$RESET\n"; - print "$v[5]\n"; + print "$v[5]"; }
my %allpkgs = ();
-my $syncout = `pacman -Ss '@ARGV'`; -# split each sync search entry into its own array entry -my @syncpkgs = split(/\n^(?=\w)/m, $syncout); -# remove the extra \n from the last desc entry -if ($#syncpkgs >= 0) { - chomp($syncpkgs[$#syncpkgs]); -} - -foreach $_ (@syncpkgs) { +open (my $syncout, '-|', 'pacman', '-Ss', @ARGV);
You should check the return value from open.
+while (<$syncout>) {
Style nitpick: I'd prefer readline() over <> for readability.
# We grab the following fields: repo, name, ver, group, installed, and # desc. We grab leading space for 'group' and 'installed' so that we do not # need to test if non-empty when printing. - my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?( \[.*\])?\n(.*)$/s; + my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?( \[.*\])?$/s; + my $desc = <$syncout>;
The description needs to be read after the check for a regex match, otherwise that line will just be lost on failures.
if(not @pkgfields) { # skip any non-matching line and just print it for the user print $_, "\n";
This newline needs to be removed.
@@ -118,24 +112,20 @@ foreach $_ (@syncpkgs) { # since 'group' and 'installed' are optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = "" if not defined $pkgfields[4]; + $pkgfields[5] = $desc; # Add each sync pkg by name/ver to a hash table. # Any value is good since we only check for existence. $allpkgs{$pkgfields[1] . $pkgfields[2]} = 1; print_pkg(@pkgfields); } +close ($syncout);
-my $queryout = `pacman -Qs '@ARGV'`; -# split each querysearch entry into its own array entry -my @querypkgs = split(/\n^(?=\w)/m, $queryout); -# remove the extra \n from the last desc entry -if ($#querypkgs >= 0) { - chomp ($querypkgs[$#querypkgs]); -} - -foreach $_ (@querypkgs) {
Comments for sync search apply here too.
+open (my $queryout, '-|', 'pacman', '-Qs', @ARGV); +while (<$queryout>) { # We grab the same field as before, even the "installed" which is always # empty for local searches. This allows us to reserve a cell in @pkgfields. - my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?()?\n(.*)$/s; + my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?()?$/s; + my $desc = <$queryout>; # skip any non-matching line next if not defined $pkgfields[1]; # check if the package was listed in the sync out @@ -143,8 +133,10 @@ foreach $_ (@querypkgs) { # since 'group' is optional, we should fill it in if necessary $pkgfields[3] = "" if not defined $pkgfields[3]; $pkgfields[4] = " [$LC_INSTALLED]"; + $pkgfields[5] = $desc; print_pkg(@pkgfields); } } +close ($queryout);
#vim: set noet: -- 1.8.5.3
On 14-02-05 09:51:17, Andrew Gregory wrote:
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
# We grab the following fields: repo, name, ver, group, installed, and # desc. We grab leading space for 'group' and 'installed' so that we do not # need to test if non-empty when printing. - my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?( \[.*\])?\n(.*)$/s; + my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?( \[.*\])?$/s; + my $desc = <$syncout>;
The description needs to be read after the check for a regex match, otherwise that line will just be lost on failures.
Actually this is done on purpose. If we do not read 'desc' right now, on failure the next line will be that description which will be parsed as if it were a 'repo/pkgname pkgver ...' line. I am getting you wrong on this?
if(not @pkgfields) { # skip any non-matching line and just print it for the user print $_, "\n";
This newline needs to be removed.
Right, forgot that. Thought I cannot find a case when that happens. -- Pierre Neidhardt Generosity and perfection are your everlasting goals.
On 02/05/14 at 04:25pm, Pierre Neidhardt wrote:
On 14-02-05 09:51:17, Andrew Gregory wrote:
On 02/04/14 at 12:16am, Pierre Neidhardt wrote:
# We grab the following fields: repo, name, ver, group, installed, and # desc. We grab leading space for 'group' and 'installed' so that we do not # need to test if non-empty when printing. - my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?( \[.*\])?\n(.*)$/s; + my @pkgfields = /^(.*?)\/(.*?) (.*?)( \(.*?\))?( \[.*\])?$/s; + my $desc = <$syncout>;
The description needs to be read after the check for a regex match, otherwise that line will just be lost on failures.
Actually this is done on purpose. If we do not read 'desc' right now, on failure the next line will be that description which will be parsed as if it were a 'repo/pkgname pkgver ...' line.
I am getting you wrong on this?
Right now pacsearch prints any line it can't parse. If you read an extra line for the description before you check for a regex match, that line will simply be lost.
if(not @pkgfields) { # skip any non-matching line and just print it for the user print $_, "\n";
This newline needs to be removed.
Right, forgot that. Thought I cannot find a case when that happens.
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Pierre Neidhardt