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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 22, 2016, 17:01 UTC
Message-ID: <xmqqinvxboa1.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <57911B5B.9090604@jeffhostetler.com>

Jeff Hostetler <git@jeffhostetler.com> writes:

Show 12 quoted lines

>>> +	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"?

Show 21 quoted lines

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

Show 20 quoted lines

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