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

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

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

> @@ -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 */

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