Re: [PATCH v2 3/6] t6601: add helper for testing path-walk API

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 21, 2024, 22:39 UTC
Message-ID: <Zz+2hWlYCo6jlvj2@nand.local>
In-Reply-To: <a3c754d93cc3ecfcb4d2764930873af0b611eda9.1731181273.git.gitgitgadget@gmail.com>

On Sat, Nov 09, 2024 at 07:41:09PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 19 quoted lines

> From: Derrick Stolee <stolee@gmail.com>
>
> Add some tests based on the current behavior, doing interesting checks
> for different sets of branches, ranges, and the --boundary option. This
> sets a baseline for the behavior and we can extend it as new options are
> introduced.
>
> Store and output a 'batch_nr' value so we can demonstrate that the paths are
> grouped together in a batch and not following some other ordering. This
> allows us to test the depth-first behavior of the path-walk API. However, we
> purposefully do not test the order of the objects in the batch, so the
> output is compared to the expected output through a sort.
>
> It is important to mention that the behavior of the API will change soon as
> we start to handle UNINTERESTING objects differently, but these tests will
> demonstrate the change in behavior.
>
> Signed-off-by: Derrick Stolee <stolee@gmail.com>
> ---

Nice. I like the approach of implementing the API in a single commit, and then demonstrating a trivial "caller" by way of a custom test helper. I think that the artifact of having a test helper here is useful on its own, but it also serves as a good example of how to use the API, and provides something to actually test the implementation with.

I'm going to steal this pattern the next time I need to work on something that necessitates a complex new API ;-).

Show 19 quoted lines

> +static int emit_block(const char *path, struct oid_array *oids,
> +		      enum object_type type, void *data)
> +{
> +	struct path_walk_test_data *tdata = data;
> +	const char *typestr;
> +
> +	switch (type) {
> +	case OBJ_TREE:
> +		typestr = "TREE";
> +		tdata->tree_nr += oids->nr;
> +		break;
> +
> +	case OBJ_BLOB:
> +		typestr = "BLOB";
> +		tdata->blob_nr += oids->nr;
> +		break;
> +	default:
> +		BUG("we do not understand this type");
> +	}

I think you could write this as:

    if (type == OBJ_TREE)
        tdata->tree_nr += oids->nr;
    else if (type == OBJ_BLOB)
        tdata->blob_nr += oids->nr;
    else
        BUG("we do not understand this type");

    typestr = type_name(type);

Which DRYs things up a bit and uses the type_name() helper. That will give you strings like "tree" and "blob" instead of "TREE" and "BLOB", but I'm not sure if the casing is important.

Thanks, Taylor