Re: [PATCH v4 3/3] rev-list: add commit object support in `--missing` option

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 24, 2023, 17:45 UTC
Message-ID: <xmqqbkcn52z4.fsf@gitster.g>
In-Reply-To: <20231024122631.158415-4-karthik.188@gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 10 quoted lines

> @@ -1176,7 +1181,11 @@ static int process_parents(struct rev_info *revs,...
>  					break;
>  				continue;
>  			}
> -			return -1;
> +
> +			if (!revs->do_not_die_on_missing_objects)
> +				return -1;
> +			else
> +				oidset_insert(&revs->missing_objects, &p->object.oid);

I would suspect that swapping if/else would make it easier to follow. Everybody else in the patch guards the use of the oidset with "were we told not to die on missing objects?", i.e.,

	if (revs->do_not_die_on_missing_objects)
		oidset_insert(&revs->missing_objects, &p->object.oid);
	else
		return -1; /* corrupt repository */

Show 6 quoted lines

> @@ -3800,6 +3809,9 @@ int prepare_revision_walk(struct rev_info *revs)
>  				       FOR_EACH_OBJECT_PROMISOR_ONLY);
>  	}
>  
> +	if (revs->do_not_die_on_missing_objects)
> +		oidset_init(&revs->missing_objects, 0);

I read the patch to make sure that .missing_objects oidset is used only when .do_not_die_on_missing_objects is set and the oidset is untouched unless it is initialized. Well done.

I know I floated "perhaps oidset can replace the object bits, especially because the number of objects that need marking is expected to be small", but I am curious what the performance implication of this would be. Is this something we can create comparison easily?

I noticed that nobody releases the resource held by this new oidset. Shouldn't we do so in revision.c:release_revisions()?

Thanks.