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

Jeff Hostetler [off-list ref] writes:

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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