There was a TODO to make this code more legible. I still think it's
somewhat ugly, but it does read _slightly_ better as a switch statement.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
By avoiding lots of small string allocations and reallocations when
appending to the output buffer, we can get a pretty decent performance
improvement (~6x for strings that do not require escaping, and ~2x for
most other multi-byte utf8 strings).
goos: linux
goarch: amd64
pkg: github.com/vbatts/go-mtree/pkg/govis
cpu: AMD Ryzen 7 7840U w/ Radeon 780M Graphics
│ before │ after │
│ sec/op │ sec/op vs base │
Vis/NoChange-16 2372.5n ± 2% 379.1n ± 1% -84.02% (p=0.000 n=10)
Vis/Binary-16 2.104µ ± 8% 1.319µ ± 8% -37.35% (p=0.000 n=10)
Vis/ASCII-16 2070.0n ± 1% 737.3n ± 0% -64.38% (p=0.000 n=10)
Vis/German-16 3.380µ ± 1% 1.181µ ± 2% -65.04% (p=0.000 n=10)
Vis/Russian-16 10.927µ ± 2% 5.293µ ± 2% -51.56% (p=0.000 n=10)
Vis/Japanese-16 7.489µ ± 1% 3.990µ ± 0% -46.72% (p=0.000 n=10)
geomean 3.767µ 1.447µ -61.58%
In theory we could get more performance if switch away from fmt.Sprintf,
but the %.N handling would be a little annoying to implement and so we
can punt on that for now.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
By using a buffer, we can avoid a bunch of small allocations that the
previous implementation did. Based on a few small benchmarks, the
performance improvement is very stark (~3x faster for strings that don't
require any escaping, and ~20% faster for multi-byte utf8 strings):
goos: linux
goarch: amd64
pkg: github.com/vbatts/go-mtree/pkg/govis
cpu: AMD Ryzen 7 7840U w/ Radeon 780M Graphics
│ before │ after │
│ sec/op │ sec/op vs base │
Unvis/NoChange-16 1501.0n ± 0% 497.7n ± 1% -66.84% (p=0.000 n=10)
Unvis/Binary-16 1317.5n ± 3% 934.9n ± 9% -29.04% (p=0.000 n=10)
Unvis/ASCII-16 1325.5n ± 1% 616.8n ± 1% -53.47% (p=0.000 n=10)
Unvis/German-16 1884.5n ± 1% 986.9n ± 2% -47.63% (p=0.000 n=10)
Unvis/Russian-16 4.636µ ± 1% 3.796µ ± 1% -18.11% (p=0.000 n=10)
Unvis/Japanese-16 3.453µ ± 1% 2.867µ ± 1% -16.99% (p=0.000 n=10)
geomean 2.072µ 1.206µ -41.77%
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This makes it a little easier to read the common case code which
consumes the token and helps highlight which sub-parsers are explicitly
not consuming tokens until we are sure we are using it.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Passing the parsers as an argument is very C-like and is not really as
idiomadic as just using methods (in my defence, I was still pretty green
when I wrote this code and I was trying to port some logic from C).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
These error cases were already handled correctly, but we really should
have tests for them anyway. Now that we have proper error variables
declared we can also test for specific errors as well.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This code was written before %w was added to Go, and there were a fair
few mistakes in the copy-pasted error code.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
testify makes most bog-standard test checks much easier to read and
maintain, and is quite widely used. It wasn't really well known back
when govis was first written, but the migration is fairly
straight-forward for most tests.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
It seems that this test actually had a bug that was not caught until I
tried to refactor it. The new test has behaviour that makes more sense
to me.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
testify makes most bog-standard test checks much easier to read and
maintain, and is quite widely used. It wasn't really well known back
when go-mtree was first written, but the migration is fairly
straight-forward for most tests.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This was added in Go 1.15 and avoids polluting the host /tmp with test
directories if the test crashes or is forcefully killed.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Rather than parsing the value as a float and then truncating it, just
parse it as an integer in the first place (this also adds some
validation that we are parsing a reasonable-looking value).
While we're at it, add some integration tests for this code to make sure
this quite complicated special-case behaviour doesn't regress.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
In certain circumstances, if a manifest with "time" keywords was
compared to another manifest with "tar_time" keywords, differences were
not actually reported despite there being logic for it.
Unfortunately, even though commit 26ff922da6 ("compare: implement
mtree.DirectoryHierarchy comparisons") explicitly included logic to
handle the necessary "time" -> "tar_time" conversions, the same commit
also made it so that Compare() could be instructed to only consider a
subset of keywords and failed to take into account said "time" ->
"tar_time" remapping.
Most users have likely not noticed this because gomtree will re-use the
keywords from a manifest when doing a straightforward "gomtree validate"
invocation, but if you explicitly requested "time" when validating a
"tar_time" manifest (and *not* the other way around) then any time
changes would be undetected.
Fixes: 26ff922da6 ("compare: implement mtree.DirectoryHierarchy comparisons")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
frustratingly, `golang.org/x/sys/unix` started importing from `slices`
and didn't cleanly put the go headers version support, so all libraries
that used this library needed to be updated and could not take the
update without also updating to `go1.24`. So dumb.
Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
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 83c9fdb78b ("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: 5d7f6c36e0 ("walk: directory is expected to be walked. A file is not.")
Fixes: 2d841d54bf ("test: testing the double -f comparison")
Fixes: f6c295f2e9 ("test: cli: add unicode verification test")
Fixes: 071977cef6 ("test: cli: add xattr tests")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This was broken during the refactor for "gomtree validate" in commit
83c9fdb78b ("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: 83c9fdb78b ("refactor: prefactor for adding new subcommands")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Update the README to show the validate subcommand by default.
This doesn't eliminate the default behavior of _not_ using the command,
but begins the visibility of using it by default.
Also copy one of the existing tests, to ensure the same behaviour works
as we add more subcommands and/or global flags.
Signed-off-by: Vincent Batts <vbatts@hashbangbash.com>
The spec[1] doesn't mention anything about cleaning paths, but it does
explicitly refer to the path containing a "/". Cleaning the path before
checking if the entry is a FullType would result in the simplest way of
forcing directories to be FullTypes (appending a "/" to the pathname of
any directory) not working with go-mtree.
[1]: https://man.netbsd.org/mtree.5
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
