Re: [PATCH v2 1/5] test-list-objects: List a subset of object ids

[Junio C Hamano <gitster@pobox.com>]

Derrick Stolee <dstolee@microsoft.com> writes:

> diff --git a/t/helper/test-list-objects.c b/t/helper/test-list-objects.c
> new file mode 100644
> index 000000000..83b1250fe
> --- /dev/null
> +++ b/t/helper/test-list-objects.c
> @@ -0,0 +1,85 @@
> +#include "cache.h"
> +#include "packfile.h"
> +#include <stdio.h>

If you include "cache.h", like the ordinary "git" program, you
should not have to include any system headers like stdio.h
yourself.  The whole point of "git-compat-util.h must be the first
to be included (indirectly including it via cache.h and friends is
also OK)" rule is to make sure that system headers are included at
the right place in the include sequence (e.g. defining feature
macros before including certain headers, etc.).

> +int cmd_main(int ac, const char **av)
> +{
> +	unsigned int hash_delt = 0xFDB97531;
> +	unsigned int hash_base = 0x01020304;
> +...
> +	const int u_per_oid = sizeof(struct object_id) / sizeof(unsigned int);

Because the above will not work as you expect, unless your uint is
32-bit, you would probably want to make hash_delt and hash_base
explicitly uint32_t, I think.

Alternatively, because you assume that uint is at least 8-hex-digit
wide in the output loop, you can keep "unsigned int" above, but
change the divisor from sizeof(unsigned int) to a hardcoded 4.

Either would fix the issue.

> +	c.total = 0;
> +	if (ac > 1)
> +		n = atoi(av[1]);

Otherwise n will stay 0 as initialized.  Not checking the result of
atoi() is probably permissible, as this is merely a test helper and
we can assume that the caller will not do anything silly, like
passing "-3" in av[1].  But it certainly would be a good discipline
to make sure av[1] is a reasonable number.

I am afraid that I may be misreading the code, but the following
code seems to require that the caller must know roughly how many
objects there are (so that a large-enough value can be passed to
av[1] to force c.select_mod to 1) when it wants to show everything,
as the "missing" loop will run 0 times showing nothing, and
"listing" case will divide by zero.

"Not passing anything will show everything" the proposed log message
promises is a better design than what the code actually seems to be
doing.

> +
> +	if (ac > 2 && !strcmp(av[2], "--missing")) {
> +		while (c.total++ < n) {

When n==0, this does nothing.  I am not sure what the desired
behaviour should be for "When no count is given, we show everything"
in this codepath, though.

Also, doesn't "test-list-objects 5 --missing" look very wrong?
Shouldn't it be more like "test-list-objects --missing 5"?

> +			for (i = 0; i < u_per_oid; i++) {
> +				printf("%08x", hash_base);
> +				hash_base += hash_delt;
> +			}
> +			printf("\n");
> +		}
> +	} else {
> +		setup_git_directory();
> +
> +		for_each_loose_object(count_loose, &c, 0);
> +		for_each_packed_object(count_packed, &c, 0);
> +
> +		c.select_mod = 1 + c.total / n;

When n==0, this would divide by zero.
Perhaps

		c.select_mod = n ? (1 + c.total / n) : 1;

or something to keep the promise of showing everything?

> +		for_each_loose_object(output_loose, &c, 0);
> +		for_each_packed_object(output_packed, &c, 0);
> +	}
> +
> +	exit(0);
> +}

Thanks.