Re: [PATCH 1/5] cache-tree: add perf test comparing update and prime

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Nov 10, 2022, 07:23 UTC
Message-ID: <20221110072342.GA1159673@szeder.dev>
In-Reply-To: <45c198c629da1627eabf0e63539f50aaa50381bf.1667947465.git.gitgitgadget@gmail.com>

On Tue, Nov 08, 2022 at 10:44:21PM +0000, Victoria Dye via GitGitGadget wrote:

Show 14 quoted lines

> diff --git a/t/helper/test-cache-tree.c b/t/helper/test-cache-tree.c
> new file mode 100644
> index 00000000000..2fad6d06d30
> --- /dev/null
> +++ b/t/helper/test-cache-tree.c
> @@ -0,0 +1,52 @@
> +#include "test-tool.h"
> +#include "cache.h"
> +#include "tree.h"
> +#include "cache-tree.h"
> +#include "parse-options.h"
> +
> +static char const * const test_cache_tree_usage[] = {
> +	N_("test-tool cache-tree <options> (prime|repair)"),

The code looking at 'argv[0]' below only handles "prime" and "update", but not "repair".

Show 22 quoted lines

> +	NULL
> +};
> +
> +int cmd__cache_tree(int argc, const char **argv)
> +{
> +	struct object_id oid;
> +	struct tree *tree;
> +	int fresh = 0;
> +	int count = 1;
> +	int i;
> +
> +	struct option options[] = {
> +		OPT_BOOL(0, "fresh", &fresh,
> +			 N_("clear the cache tree before each repetition")),
> +		OPT_INTEGER_F(0, "count", &count, N_("number of times to repeat the operation"),
> +			      PARSE_OPT_NONEG),
> +		OPT_END()
> +	};
> +
> +	setup_git_directory();
> +
> +	parse_options(argc, argv, NULL, options, test_cache_tree_usage, 0);

Here 'argc' must be updated with the return value of parse_options(), otherwise the 'if (!argc)' condition doesn't catch what it's supposed to, and the subsequent 'else if' segfaults when passes the NULL argv[0] to strcmp().

Show 11 quoted lines

> +
> +	if (read_cache() < 0)
> +		die("unable to read index file");
> +
> +	get_oid("HEAD", &oid);
> +	tree = parse_tree_indirect(&oid);
> +	for (i = 0; i < count; i++) {
> +		if (fresh)
> +			cache_tree_free(&the_index.cache_tree);
> +
> +		if (!argc)

What if argc > 1?

> +			die("Must specify subcommand");

I think it would be nice to show usage here ...

Show 6 quoted lines

> +		else if (!strcmp(argv[0], "prime"))
> +			prime_cache_tree(the_repository, &the_index, tree);
> +		else if (!strcmp(argv[0], "update"))
> +			cache_tree_update(&the_index, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);
> +		else
> +			die("Unknown command %s", argv[0]);

... and here as well.

> +	}
> +
> +	return 0;
> +}