Re: [PATCH v1 3/6] Per-file output for Porcelain Status V2

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

Jeff Hostetler <git@jeffhostetler.com> writes:

>>> +	case DIFF_STATUS_DELETED:
>>> +		d->porcelain_v2.mode_index = p->one->mode;
>>> +		hashcpy(d->porcelain_v2.sha1_index, p->one->sha1);
>>> +		break;
>>> +
>>> +	case DIFF_STATUS_COPIED:
>>> +	case DIFF_STATUS_MODIFIED:
>>> +	case DIFF_STATUS_RENAMED:
>>
>> Can RENAMED or COPIED happen here?
>
> If we don't report renamed or copied for unstaged changes, then no.

The question was "do we report such cases"?

>>> +	if (!d->index_status) {
>>> +		if (d->worktree_status == DIFF_STATUS_MODIFIED ||
>>> +			d->worktree_status == DIFF_STATUS_DELETED) {
>>> +			/* X=' ' Y=[MD]
>>> +			 * The item did not change in head-vs-index scan so the head
>>> +			 * column was never set. (The index column was set during the
>>> +			 * index-vs-worktree scan.)
>>> +			 * Force set the head column to make the output complete.
>>> +			 */
>>> +			d->porcelain_v2.mode_head = d->porcelain_v2.mode_index;
>>> +			hashcpy(d->porcelain_v2.sha1_head, d->porcelain_v2.sha1_index);
>>> +		}
>>
>> Can there be "else" clause for this inner "if"?  When
>> d->index_status is not set, but worktree_status is not one of these
>> two, what does it indicate?  The same for the next one.
>
> For a normal entry (not unmerged) when X is ' ', Y can only
> be 'M' or 'D'.  The only other value for Y is ' ', but then if
> both were ' ' the entry would not be in the list.  So I think
> we're OK, but I'll document that here.  And below.

Good.

>>> +	memset(stages, 0, sizeof(stages));
>>> +	pos = cache_name_pos(it->string, strlen(it->string));
>>> +	assert(pos < 0);
>>> +	pos = -pos-1;
>>> +	while (pos < active_nr) {
>>> +		ce = active_cache[pos++];
>>> +		stage = ce_stage(ce);
>>> +		if (strcmp(ce->name, it->string) || !stage)
>>> +			break;
>>> +		stages[stage - 1].mode = ce->ce_mode;
>>> +		hashcpy(stages[stage - 1].sha1, ce->sha1);
>>> +	}
>>
>> You did assert(pos < 0) to make sure that the path the caller told
>> you is unmerged indeed is unmerged, which is a very good check.  In
>> the same spirit, you would want to make sure that you got at least
>> one result from the above loop, to diagnose a bug where the path did
>> not even exist at all in the index.
>
> Would that be possible for a conflict/unmerged entry?

It is possible to exactly the same degree as it is possible you
would get !(pos < 0) answer from cache_name_pos() here, I would
think.  The assert() you have up above is protecting us from either
a broken caller to this function that gives an it that points at a
merged path (in which case the assert is violated) or a breakage in
cache_name_pos().  Making sure the loop finds at least one unmerged
entry protects us from similar kind of breakage that violates the
expectation this code has from the other parts of the code.


Thanks.