| Message ID | pull.1172.git.git.1641528221530.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hook: test -a|o is not POSIX | expand |
"Issam Maghni via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: "Issam E. Maghni" <issam.e.maghni@mailbox.org> > > I faced `test: too many arguments` when building using sbase [1] Building "git" with sbase? It is curious how a loosely written sample hook script can cause build failures on a platform with a more strict userspace. > This is due to a non-POSIX syntax `test ... -a ...` and `test … -o …`. That's all correct. The formatting of the above line feels a bit off, though. >> The XSI extensions specifying the -a and -o binary primaries and the >> '(' and ')' operators have been marked obsolescent. > [2] > > [1] https://core.suckless.org/sbase/ > [2] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html > Perhaps... The sample update script in the templates directory uses the `-o` and `-a` binary primaries of the "test" command, which are marked obsolescent in the recent versions of POSIX. ...would be sufficient, as 'sbase' would not be the only source of the userspace whose 'test' lack the -o/-a primaries.
> On 01/07/2022 4:39 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Issam Maghni via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: "Issam E. Maghni" <issam.e.maghni@mailbox.org> > > > > I faced `test: too many arguments` when building using sbase [1] > > Building "git" with sbase? It is curious how a loosely written > sample hook script can cause build failures on a platform with a > more strict userspace. Sorry, I rather meant using git with sbase. I’ve submitted at least a half dozen patches similar to this one. My brain is in auto-mode. > Perhaps... > > The sample update script in the templates directory uses the > `-o` and `-a` binary primaries of the "test" command, which > are marked obsolescent in the recent versions of POSIX. > > ...would be sufficient, as 'sbase' would not be the only source of > the userspace whose 'test' lack the -o/-a primaries. You’re right, this is much cleaner. I force-pushed the changes in https://github.com/git/git/pull/1172. I must admit, I’m not familiar (yet) to submitting patches through mailing lists.
On 07/01/22 11.03, Issam Maghni via GitGitGadget wrote: > From: "Issam E. Maghni" <issam.e.maghni@mailbox.org> > > I faced `test: too many arguments` when building using sbase [1] > > This is due to a non-POSIX syntax `test ... -a ...` and `test … -o …`. > >> The XSI extensions specifying the -a and -o binary primaries and the >> '(' and ')' operators have been marked obsolescent. > [2] > > [1] https://core.suckless.org/sbase/ > [2] https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html > > Signed-off-by: Issam E. Maghni <issam.e.maghni@mailbox.org> > --- > shell: test -a|o is not POSIX > > I faced test: too many arguments when building using sbase > [https://core.suckless.org/sbase/]. This is due to a non-POSIX syntax > test ... -a ... and test … -o …. > > > The XSI extensions specifying the -a and -o binary primaries and the > > '(' and ')' operators have been marked obsolescent. > > https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1172%2Fconcatime%2Fpatch-1-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1172/concatime/patch-1-v1 > Pull-Request: https://github.com/git/git/pull/1172 > > templates/hooks--update.sample | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/templates/hooks--update.sample b/templates/hooks--update.sample > index c4d426bc6ee..6cc46ebcf3a 100755 > --- a/templates/hooks--update.sample > +++ b/templates/hooks--update.sample > @@ -37,7 +37,7 @@ if [ -z "$GIT_DIR" ]; then > exit 1 > fi > > -if [ -z "$refname" -o -z "$oldrev" -o -z "$newrev" ]; then > +if [ -z "$refname" ] || [ -z "$oldrev" ] || [ -z "$newrev" ]; then > echo "usage: $0 <ref> <oldrev> <newrev>" >&2 > exit 1 > fi > @@ -95,7 +95,7 @@ case "$refname","$newrev_type" in > ;; > refs/heads/*,commit) > # branch > - if [ "$oldrev" = "$zero" -a "$denycreatebranch" = "true" ]; then > + if [ "$oldrev" = "$zero" ] && [ "$denycreatebranch" = "true" ]; then > echo "*** Creating a branch is not allowed in this repository" >&2 > exit 1 > fi > > base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907 From the patch above, the title should be "replace non-POSIX test -a & test -o with logical operator".
Bagas Sanjaya <bagasdotme@gmail.com> writes: >> - if [ "$oldrev" = "$zero" -a "$denycreatebranch" = "true" ]; then >> + if [ "$oldrev" = "$zero" ] && [ "$denycreatebranch" = "true" ]; then >> echo "*** Creating a branch is not allowed in this repository" >&2 >> exit 1 >> fi >> base-commit: 2ae0a9cb8298185a94e5998086f380a355dd8907 > > From the patch above, the title should be "replace non-POSIX test -a & test -o with logical operator". I think "should" is a bit too strong a word. While yours is a bit more explicit on _what_ the solution is, "hook: test -a|o is not POSIX" already implies that we are fixing that non-posix-ness by rewriting, and it is obvious (cf. Documentation/CodingGuidelines) what the right rewrite should be. One thing the original does a bit better tha yours is that it tell us _where_ the problem is. So, perhaps sample hook: use "test ... &&/|| test ..." instead of "test -a/-o" But I find the original just fine.
diff --git a/templates/hooks--update.sample b/templates/hooks--update.sample index c4d426bc6ee..6cc46ebcf3a 100755 --- a/templates/hooks--update.sample +++ b/templates/hooks--update.sample @@ -37,7 +37,7 @@ if [ -z "$GIT_DIR" ]; then exit 1 fi -if [ -z "$refname" -o -z "$oldrev" -o -z "$newrev" ]; then +if [ -z "$refname" ] || [ -z "$oldrev" ] || [ -z "$newrev" ]; then echo "usage: $0 <ref> <oldrev> <newrev>" >&2 exit 1 fi @@ -95,7 +95,7 @@ case "$refname","$newrev_type" in ;; refs/heads/*,commit) # branch - if [ "$oldrev" = "$zero" -a "$denycreatebranch" = "true" ]; then + if [ "$oldrev" = "$zero" ] && [ "$denycreatebranch" = "true" ]; then echo "*** Creating a branch is not allowed in this repository" >&2 exit 1 fi