From 23151ae80f843196ac7873f40612d9188013c7de Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Mon, 8 Sep 2025 21:52:22 +1000 Subject: [PATCH 1/2] gomtree: return exit status != 0 on error This was broken during the refactor for "gomtree validate" in commit 83c9fdb78bf7 ("refactor: prefactor for adding new subcommands"), resulting in any code that relied on our exit code to silently treat all errors as non-fatal. Our tests did not catch this due to a quirky POSIX-ism with regards to "! cmd" and "set -e" which is fixed in a follow-up patch. Fixes: 83c9fdb78bf7 ("refactor: prefactor for adding new subcommands") Signed-off-by: Aleksa Sarai --- cmd/gomtree/main.go | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/cmd/gomtree/main.go b/cmd/gomtree/main.go index 90fff23..7150ce5 100644 --- a/cmd/gomtree/main.go +++ b/cmd/gomtree/main.go @@ -1,7 +1,6 @@ package main import ( - "fmt" "os" "strings" @@ -51,23 +50,20 @@ to support xattrs and interacting with tar archives.` app.OnUsageError = func(ctx *cli.Context, err error, isSubcommand bool) error { if ctx.Command.Name == "gomtree" && strings.Contains(err.Error(), "flag provided but not defined") { runValidate = true - return nil } return err } - if err := app.Run(os.Args); err != nil { - fmt.Println(err.Error()) - } - - // So we run the command again with the validate command as the default. - if runValidate { + err := app.Run(os.Args) + // If it failed, run the command again with the validate command as the + // default if it failed. + if err != nil && runValidate { app.OnUsageError = nil args := []string{os.Args[0], "validate"} args = append(args, os.Args[1:]...) - if err := app.Run(args); err != nil { - fmt.Println(err.Error()) - } + err = app.Run(args) + } + if err != nil { + logrus.Fatal(err) } - } From 945eeeda2d907d8d376ebcce10b8d623cadaf3aa Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Mon, 8 Sep 2025 23:39:17 +1000 Subject: [PATCH 2/2] tests: cli: properly check for exit status codes in error cases POSIX explicitly requires "set -e" to NOT treat "! cmd" as an error even if fails[1]: > The -e setting shall be ignored when executing the compound list > following the while, until, if, or elif reserved word, *a pipeline > beginning with the ! reserved word*, or any command of an AND-OR list > other than the last. *[emphasis added]* And bash has similar documentation on this behaviour[2]. This means that our tests were completely ineffective at detecting error codes from gomtree, and as a result we did not detect the regression in commit 83c9fdb78bf7 ("refactor: prefactor for adding new subcommands"). The simplest solution (as done in this patch) is to just wrap all of the failing examples in a subshell, which causes the shell to no longer consider them exempt from "set -e". A more complete solution would be to either switch to something like bats entirely or at least use something like their "run" helper function to test for exit status codes correctly. [1]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#set [2]: https://www.gnu.org/software/bash/manual/html_node/The-Set-Builtin.html Fixes: 5d7f6c36e0c0 ("walk: directory is expected to be walked. A file is not.") Fixes: 2d841d54bf69 ("test: testing the double -f comparison") Fixes: f6c295f2e9ef ("test: cli: add unicode verification test") Fixes: 071977cef680 ("test: cli: add xattr tests") Signed-off-by: Aleksa Sarai --- test/cli/0003.sh | 6 +++--- test/cli/0009.sh | 2 +- test/cli/0010.sh | 2 +- test/cli/0011.sh | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/cli/0003.sh b/test/cli/0003.sh index 3722073..8a7e486 100644 --- a/test/cli/0003.sh +++ b/test/cli/0003.sh @@ -17,14 +17,14 @@ setfattr -n user.mtree.testing -v "apples and=bananas" "${t}/dir/file" $gomtree -c -k "sha256digest,xattrs" -p ${t}/dir > ${t}/${name}.mtree setfattr -n user.mtree.testing -v "bananas and lemons" "${t}/dir/file" -! $gomtree -p ${t}/dir -f ${t}/${name}.mtree +(! $gomtree -p ${t}/dir -f ${t}/${name}.mtree) setfattr -x user.mtree.testing "${t}/dir/file" -! $gomtree -p ${t}/dir -f ${t}/${name}.mtree +(! $gomtree -p ${t}/dir -f ${t}/${name}.mtree) setfattr -n user.mtree.testing -v "apples and=bananas" "${t}/dir/file" setfattr -n user.mtree.another -v "another a=b" "${t}/dir/file" -! $gomtree -p ${t}/dir -f ${t}/${name}.mtree +(! $gomtree -p ${t}/dir -f ${t}/${name}.mtree) setfattr -n user.mtree.testing -v "apples and=bananas" "${t}/dir/file" setfattr -x user.mtree.another "${t}/dir/file" diff --git a/test/cli/0009.sh b/test/cli/0009.sh index 89cb76e..8448be7 100644 --- a/test/cli/0009.sh +++ b/test/cli/0009.sh @@ -24,7 +24,7 @@ ${gomtree} -k uid,gid,size,type,link,nlink,sha256digest -f ${t}/root.mtree -p ${ # Modify it and make sure that it successfully figures out what changed. echo "othe data" > "${t}/root/$(printf 'this file has \u042a some unicode !!')" -! ${gomtree} -k uid,gid,size,type,link,nlink,sha256digest -f ${t}/root.mtree -p ${t}/root +(! ${gomtree} -k uid,gid,size,type,link,nlink,sha256digest -f ${t}/root.mtree -p ${t}/root) echo "some data" > "${t}/root/$(printf 'this file has \u042a some unicode !!')" ${gomtree} -k uid,gid,size,type,link,nlink,sha256digest -f ${t}/root.mtree -p ${t}/root diff --git a/test/cli/0010.sh b/test/cli/0010.sh index 265fdc0..2cf10ae 100644 --- a/test/cli/0010.sh +++ b/test/cli/0010.sh @@ -19,7 +19,7 @@ rm -rf ${t}/extract/*.go ${gomtree} -K sha256digest -c -p ${t}/extract/ > ${t}/${name}-2.mtree # this _ought_ to fail because the files are missing now -! ${gomtree} -f ${t}/${name}-1.mtree -f ${t}/${name}-2.mtree +(! ${gomtree} -f ${t}/${name}-1.mtree -f ${t}/${name}-2.mtree) popd rm -rf ${t} diff --git a/test/cli/0011.sh b/test/cli/0011.sh index 11298cf..d73eb35 100644 --- a/test/cli/0011.sh +++ b/test/cli/0011.sh @@ -16,7 +16,7 @@ touch ${t}/foo ## can not walk a file. We're expecting a directory. ## https://github.com/vbatts/go-mtree/issues/166 -! ${gomtree} -c -K uname,uid,gname,gid,type,nlink,link,mode,flags,xattr,xattrs,size,time,sha256 -p ${t}/foo +(! ${gomtree} -c -K uname,uid,gname,gid,type,nlink,link,mode,flags,xattr,xattrs,size,time,sha256 -p ${t}/foo) popd rm -rf ${t}