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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 26, 2017, 09:24 UTC
Message-ID: <xmqqd16dak2s.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170925095452.66833-2-dstolee@microsoft.com>

Derrick Stolee <dstolee@microsoft.com> writes:

Show 9 quoted lines

> 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.).

Show 6 quoted lines

> +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"?

Show 13 quoted lines

> +			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?

Show 6 quoted lines

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

Thanks.