Re: [PATCH v2 4/8] status: per-file data collection for --porcelain=v2

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

Jeff Hostetler <jeffhost@microsoft.com> writes:

> +static void aux_updated_entry_porcelain_v2(
> +	struct wt_status *s,
> +	struct wt_status_change_data *d,
> +	struct diff_filepair *p)
> +{
> +	switch (p->status) {
> +	case DIFF_STATUS_ADDED:
> +		/* {mode,sha1}_head are zero for an add. */
> +		d->aux.porcelain_v2.mode_index = p->two->mode;

I doubt that it makes sense in the longer term to have a new "aux"
field.  Why isn't it part of the wt_status_change_data structure?
For that matter, why should these new functions have both "aux" and
"v2" in their names?

Imagine what should happen when somebody wants to add --porcelain=v3
format in 6 months.  Why must "v3" be treated differently from "v1"
and in a way close to "v2"?  Why shouldn't all the three be treated
in a similar way that "v1" has already?

> +		oidcpy(&d->aux.porcelain_v2.oid_index, &p->two->oid);
> +		break;
> +
> +	case DIFF_STATUS_DELETED:
> +		d->aux.porcelain_v2.mode_head = p->one->mode;
> +		oidcpy(&d->aux.porcelain_v2.oid_head, &p->one->oid);
> +		/* {mode,oid}_index are zero for a delete. */
> +		break;
> +
> +	case DIFF_STATUS_RENAMED:
> +		d->aux.porcelain_v2.rename_score = p->score * 100 / MAX_SCORE;

I have a slight aversion against losing the precision in a helper
function like this that does not do the actual output, but it
probably is OK.

Don't we have copy detection score that is computed exactly the same
way for DIFF_STATUS_COPIED case, too?

For readability, unless a case arm is completely empty, we should
have
		/* fallthru */

comment where "break;" would go for a normal case arm.

> +	case DIFF_STATUS_COPIED:
> +	case DIFF_STATUS_MODIFIED:
> +	case DIFF_STATUS_TYPE_CHANGED:
> +	case DIFF_STATUS_UNMERGED:
> +		d->aux.porcelain_v2.mode_head = p->one->mode;
> +		d->aux.porcelain_v2.mode_index = p->two->mode;
> +		oidcpy(&d->aux.porcelain_v2.oid_head, &p->one->oid);
> +		oidcpy(&d->aux.porcelain_v2.oid_index, &p->two->oid);
> +		break;