Re: [PATCH 1/2] unpack-trees: lose USE_THE_REPOSITORY

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

Patrick Steinhardt <ps@pks.im> writes:

>> In any correctly written clients of this library file, the
>> repository instance that is pointed at by these two structs MUST be
>> the same one.  For now, make sure that is the case (and otherwise
>> BUG out) by introducing a unpack_repo() helper function that takes
>> these two structures (when available) and returns the repository.
>
> Fair. It's a bit curious of an interface to have this kind of ambiguity.
> But as an intermediate step I think the proposed solution is okayish,
> and I do like that we at least verify that it's the same repository.

Yeah, while I am reasonably sure that all three (when they exist)
must always be pointing at the same repository, I may have missed a
legitimate use case where that assumption does not hold.  And that
is why the initial implementation is checking them with paranoia to
easily detect cases that I missed.


>> +	if (istate) {
>> +		if (!istate->repo)
>> +			BUG("istate->repo == NULL");
>> +		repo = istate->repo;
>> +		camefrom = "istate->repo";
>> +	}
>> +	if (o) {
>> +		if (o->src_index) {
>> +			if (!o->src_index->repo)
>> +				BUG("o->src_index->repo == NULL");
>> +			if (repo && repo != o->src_index->repo)
>> +				BUG("%s != o->src_index->repo", camefrom);
>> +			repo = o->src_index->repo;
>> +			camefrom = "o->src_index->repo";
>> +		}
>> +
>> +		if (o->dst_index) {
>> +			if (!o->dst_index->repo)
>> +				BUG("o->dst_index->repo == NULL");
>> +			if (repo && repo != o->dst_index->repo)
>> +				BUG("%s != o->dst_index->repo", camefrom);
>> +			repo = o->dst_index->repo;
>> +		}
>> +	}
>> +	if (!repo)
>> +		BUG("no repository for unpack_trees()?");
>> +	return repo;
>> +
>> +}
>
> So this overall surprisingly complex, but I think it's sensible to be
> defensive.

Yes, we could do

	if (istate && istate->repo)
		return itate->repo;
	if (o && o->src_index && o->src_index->repo)
		return o->src_index->repo;
	if (o && o->dst_index && o->dst_index->repo)
		return o->dst_index->repo;
	BUG();

but then you'd need to start a debugger to tell which assumption was
violated.

>> @@ -345,15 +394,16 @@ static void load_gitmodules_file(struct index_state *index,
>>  				 struct checkout *state)
>>  {
>>  	int pos = index_name_pos(index, GITMODULES_FILE, strlen(GITMODULES_FILE));
>> +	struct repository *repo = unpack_repo(index, NULL);
>
> This conversion here raises the question what we want to do in the case
> where the caller has both structures available, but we don't end up
> passing them to the pointer. This could in theory hide a bug where we
> have mismatching repositories when the entry points don't verify that
> property.

Puzzled.  Here we do have an istate but have no unpack_trees_options
available, so it is natural to pass NULL for the latter to tell the
validator "we do not have unpack_trees_options".  Are you worried
about a case where a programmer who writes an unpack_repo() call
passes NULL by mistake efven when istate or unpack_trees_options are
available?

Thanks.