Bug 37038 - koha-elasticsearch creates a file named 0
Summary: koha-elasticsearch creates a file named 0
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low trivial
Assignee: Janusz Kaczmarek
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-06-05 15:54 UTC by Janusz Kaczmarek
Modified: 2024-06-28 11:50 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 37038: koha-elasticsearch creates a file named 0 (1.55 KB, patch)
2024-06-05 17:28 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 37038: koha-elasticsearch creates a file named 0 (1.60 KB, patch)
2024-06-05 19:08 UTC, Roman Dolny
Details | Diff | Splinter Review
Bug 37038: koha-elasticsearch creates a file named 0 (1.70 KB, patch)
2024-06-28 06:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37038: (QA follow-up) Replace \> by -gt (1.22 KB, patch)
2024-06-28 06:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 37038: (QA follow-up) Replace \> by -gt (1.31 KB, patch)
2024-06-28 06:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Janusz Kaczmarek 2024-06-05 15:54:01 UTC
After execution of koha-elasticsearch command a file names 0 remains in the current directory.
Comment 1 Janusz Kaczmarek 2024-06-05 17:28:50 UTC
Created attachment 167487 [details] [review]
Bug 37038: koha-elasticsearch creates a file named 0

After execution of koha-elasticsearch command a file named 0 remains in
the current directory.

Inside the single parentheses, the character '>' is treated as a file
redirection, not as a comparison operator.

Test plan:
==========
1. Have a test installation with Elasticsearch.  As root, perform:
   ./debian/scripts/koha-elasticsearch --rebuild kohadev
   When the command finishes, check the contents of the current
   directory (ls -ltr | tail).  You sould notice a fresh file named '0'
2. Apply the patch. Delete the file named 0.
3. Repeat p. 1.  There should be no file named 0 now.
Comment 2 Roman Dolny 2024-06-05 19:08:03 UTC
Created attachment 167491 [details] [review]
Bug 37038: koha-elasticsearch creates a file named 0

After execution of koha-elasticsearch command a file named 0 remains in
the current directory.

Inside the single parentheses, the character '>' is treated as a file
redirection, not as a comparison operator.

Test plan:
==========
1. Have a test installation with Elasticsearch.  As root, perform:
   ./debian/scripts/koha-elasticsearch --rebuild kohadev
   When the command finishes, check the contents of the current
   directory (ls -ltr | tail).  You sould notice a fresh file named '0'
2. Apply the patch. Delete the file named 0.
3. Repeat p. 1.  There should be no file named 0 now.

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>
Comment 3 Tomás Cohen Arazi 2024-06-05 21:49:22 UTC
Shouldn't it be

if [ "${clo_commit_size}" -gt "0" ]; then

?
Comment 4 Janusz Kaczmarek 2024-06-05 21:59:51 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> Shouldn't it be
> 
> if [ "${clo_commit_size}" -gt "0" ]; then
> 
> ?

Thanks, Tomás, for having a look.

Well, yes, and it was my first shot.  It would be OK, provided that clo_commit_size is defined and is a integer number.

Otherwise we get:

./debian/scripts/koha-elasticsearch: 93: [: Illegal number:

I have tried several options (including nested ifs) -- the proposed solution seems to be a reasonable, working trade-off.  But I'm not a bash expert ;)
Comment 5 Marcel de Rooy 2024-06-28 06:33:12 UTC
Created attachment 168220 [details] [review]
Bug 37038: koha-elasticsearch creates a file named 0

After execution of koha-elasticsearch command a file named 0 remains in
the current directory.

Inside the single parentheses, the character '>' is treated as a file
redirection, not as a comparison operator.

Test plan:
==========
1. Have a test installation with Elasticsearch.  As root, perform:
   ./debian/scripts/koha-elasticsearch --rebuild kohadev
   When the command finishes, check the contents of the current
   directory (ls -ltr | tail).  You sould notice a fresh file named '0'
2. Apply the patch. Delete the file named 0.
3. Repeat p. 1.  There should be no file named 0 now.

Signed-off-by: Roman Dolny <roman.dolny@jezuici.pl>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2024-06-28 06:33:14 UTC
Created attachment 168221 [details] [review]
Bug 37038: (QA follow-up) Replace \> by -gt

Note that -gt is the bash integer comparison and > is the
lexical one:
$ if [ '1' \> '09' ]; then echo true; fi
true
$ if [ '1' -gt '09' ]; then echo true; fi

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2024-06-28 06:34:17 UTC
(In reply to Janusz Kaczmarek from comment #4)
> (In reply to Tomás Cohen Arazi from comment #3)
> > Shouldn't it be
> > 
> > if [ "${clo_commit_size}" -gt "0" ]; then
> > 
> > ?
> 
> Thanks, Tomás, for having a look.
> 
> Well, yes, and it was my first shot.  It would be OK, provided that
> clo_commit_size is defined and is a integer number.
> 
> Otherwise we get:
> 
> ./debian/scripts/koha-elasticsearch: 93: [: Illegal number:
> 
> I have tried several options (including nested ifs) -- the proposed solution
> seems to be a reasonable, working trade-off.  But I'm not a bash expert ;)

Ah just seeing this now.. Hang on.
Comment 8 Marcel de Rooy 2024-06-28 06:42:16 UTC
A simple workaround might be:

clo_processes=aa; if [ "${clo_processes}" -gt 0 ] 2>/dev/null; then echo true; fi
for bin/sh
We just oppress the warn. The condition is false as expected.

And for bin/bash this would just work:
clo_processes=aa; if [[ "${clo_processes}" -gt 0 ]]; then echo true; fi
But this script is marked bin/sh
Which actually is confusing when you git grep bash in that folder.. But not for here.
Comment 9 Marcel de Rooy 2024-06-28 06:45:04 UTC
Created attachment 168222 [details] [review]
Bug 37038: (QA follow-up) Replace \> by -gt

Note that -gt is the bash integer comparison and > is the
lexical one:
$ if [ '1' \> '09' ]; then echo true; fi
true
$ if [ '1' -gt '09' ]; then echo true; fi

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Added the dev/null redirection to suppress warn in bin/sh.
Comment 10 David Cook 2024-06-28 07:10:39 UTC
O_O

Good catch folks
Comment 11 Katrin Fischer 2024-06-28 11:50:31 UTC
Pushed for 24.11!

Well done everyone, thank you!