Re: [PATCH v2 1/2] list-objects-filter: teach tree:# how to handle >0

From: Jonathan Tan <jonathantanmy@google.com>
To: matvore@google.com
Cc: git@vger.kernel.org, sbeller@google.com, git@jeffhostetler.com,
	jeffhost@microsoft.com, peff@peff.net, stefanbeller@gmail.com,
	jonathantanmy@google.com, pclouds@gmail.com
Subject: Re: [PATCH v2 1/2] list-objects-filter: teach tree:# how to handle >0
Date: Mon,  7 Jan 2019 17:56:31 -0800	[thread overview]
Message-ID: <20190108015631.22727-1-jonathantanmy@google.com> (raw)
In-Reply-To: <20181210234030.176178-2-matvore@google.com>

Thanks - as stated in your commit message, this adds quite a useful
piece of functionality.

> -	case LOFS_BEGIN_TREE:
> -	case LOFS_BLOB:
> -		if (filter_data->omits) {
> -			oidset_insert(filter_data->omits, &obj->oid);
> -			/* _MARK_SEEN but not _DO_SHOW (hard omit) */
> -			return LOFR_MARK_SEEN;
> -		} else {
> -			/*
> -			 * Not collecting omits so no need to to traverse tree.
> -			 */
> -			return LOFR_SKIP_TREE | LOFR_MARK_SEEN;
> -		}
> -
>  	case LOFS_END_TREE:
>  		assert(obj->type == OBJ_TREE);
> +		filter_data->current_depth--;
>  		return LOFR_ZERO;
>  
> +	case LOFS_BLOB:
> +		filter_trees_update_omits(obj, filter_data, include_it);
> +		return include_it ? LOFR_MARK_SEEN | LOFR_DO_SHOW : LOFR_ZERO;

Any reason for moving "case LOFS_BLOB" (and "case LOFS_BEGIN_TREE"
below) after LOFS_END_TREE?

This is drastically different from the previous case, but this makes
sense - previously, all blobs accessed through traversal were not shown,
but now they are sometimes shown. Here, filter_trees_update_omits() is
only ever used to remove a blob from the omits set, since once this blob
is encountered with include_it == true, it is marked as LOFR_MARK_SEEN
and will not be traversed again.

> +	case LOFS_BEGIN_TREE:
> +		seen_info = oidmap_get(
> +			&filter_data->seen_at_depth, &obj->oid);
> +		if (!seen_info) {
> +			seen_info = xcalloc(1, sizeof(struct seen_map_entry));

Use sizeof(*seen_info).

> +			seen_info->base.oid = obj->oid;

We have been using oidcpy, but come to think of it, I'm not sure why...

> +			seen_info->depth = filter_data->current_depth;
> +			oidmap_put(&filter_data->seen_at_depth, seen_info);
> +			already_seen = 0;
> +		} else
> +			already_seen =
> +				filter_data->current_depth >= seen_info->depth;

There has been recently some clarification that if one branch of an
if/else construct requires braces, braces should be put on all of them:
1797dc5176 ("CodingGuidelines: clarify multi-line brace style",
2017-01-17). Likewise below.

> +		if (already_seen)
> +			filter_res = LOFR_SKIP_TREE;
> +		else {
> +			seen_info->depth = filter_data->current_depth;
> +			filter_trees_update_omits(obj, filter_data, include_it);
> +
> +			if (include_it)
> +				filter_res = LOFR_DO_SHOW;
> +			else if (filter_data->omits)
> +				filter_res = LOFR_ZERO;
> +			else
> +				filter_res = LOFR_SKIP_TREE;

Looks straightforward. If we have already seen it at a shallower or
equal depth, we can skip it (since we have already done the appropriate
processing). Otherwise, we need to ensure that its "omit" is correctly
set, and:
 - show it if include_it
 - don't do anything special if not include_it and we need the omit set
 - skip the tree if not include_it and we don't need the omit set

> +static void filter_trees_free(void *filter_data) {
> +	struct filter_trees_depth_data* d = filter_data;
> +	oidmap_free(&d->seen_at_depth, 1);
> +	free(d);
> +}

Check for NULL-ness of filter_data too, to match the usual behavior of
free functions.

> diff --git a/t/t6112-rev-list-filters-objects.sh b/t/t6112-rev-list-filters-objects.sh
> index eb32505a6e..54e7096d40 100755
> --- a/t/t6112-rev-list-filters-objects.sh
> +++ b/t/t6112-rev-list-filters-objects.sh

[snip]

Thanks for the tests that cover quite a wide range of cases. Can you
also demonstrate the case where a blob would normally be omitted
(because it is too deep) but it is directly specified, so it is
included.

> +expect_has_with_different_name () {
> +	repo=$1 &&
> +	name=$2 &&
> +
> +	hash=$(git -C $repo rev-parse HEAD:$name) &&
> +	! grep "^$hash $name$" actual &&
> +	grep "^$hash " actual &&
> +	! grep "~$hash" actual
> +}

Should we also check that a "~" entry appears with $name?