Re: [PATCH v3 1/3] index-pack --promisor: dedup before checking links

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 4, 2024, 04:36 UTC
Message-ID: <xmqqy10w3w0a.fsf@gitster.g>
In-Reply-To: <7ae21c921fe367d4b15cd4a299196009c15205d9.1733262662.git.jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

> @@ -1781,26 +1775,41 @@ static void repack_local_links(void)
>  	struct object_id *oid;
>  	char *base_name;

We may want to give a meaningless NULL initialization to this variable, due to false positive from a compliler.

Show 5 quoted lines

> -	if (!oidset_size(&local_links))
> +	if (!oidset_size(&outgoing_links))
>  		return;
>  
> -	base_name = mkpathdup("%s/pack/pack", repo_get_object_directory(the_repository));

It used to be that it was really obvious that base_name is always initialized. But now due to micro-optimization ...

Show 13 quoted lines

> +	oidset_iter_init(&outgoing_links, &iter);
> +	while ((oid = oidset_iter_next(&iter))) {
> +		struct object_info info = OBJECT_INFO_INIT;
> +		if (oid_object_info_extended(the_repository, oid, &info, 0))
> +			/* Missing; assume it is a promisor object */
> +			continue;
> +		if (info.whence == OI_PACKED && info.u.packed.pack->pack_promisor)
> +			continue;
> ...
> +		if (!cmd.args.nr) {
> +			base_name = mkpathdup(
> +				"%s/pack/pack",
> +				repo_get_object_directory(the_repository));

... we lazily allocate only after we know we will run a command.

Show 10 quoted lines

> +			strvec_push(&cmd.args, "pack-objects");
> +			strvec_push(&cmd.args,
> +				    "--exclude-promisor-objects-best-effort");
> +			strvec_push(&cmd.args, base_name);
> +			cmd.git_cmd = 1;
> +			cmd.in = -1;
> +			cmd.out = -1;
> +			if (start_command(&cmd))
> +				die(_("could not start pack-objects to repack local links"));
> +		}

We know outgoing_links is not empty, so we know we will enter the while() loop at least once, but it may be possible that all the objects in the outgoing_links oidset end up to be missing or packed in a promisor pack, hitting continue and never running the command.

Show 9 quoted lines

> -	oidset_iter_init(&local_links, &iter);
> -	while ((oid = oidset_iter_next(&iter))) {
>  		if (write_in_full(cmd.in, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||
>  		    write_in_full(cmd.in, "\n", 1) < 0)
>  			die(_("failed to feed local object to pack-objects"));
>  	}
> +
> +	if (!cmd.args.nr)
> +		return;

But then we have this early return, so from human-reader's point of view, we will never hit free(base_name) at the end of this function.

But GCC used in the macOS build does not seem to realize it.

https://github.com/git/git/actions/runs/12152173257/job/33888089229#step:4:380

It may be safer to give a meaningless NULL as the initial value of the variable.

Thanks.