Re: [PATCH 3/5] scalar: teach `diagnose` to gather packfile info

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jan 26, 2022 at 08:41:45AM +0000, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
>
> Teach the `scalar diagnose` command to gather file size information
> about pack files.
>
> Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
> ---
>  contrib/scalar/scalar.c          | 39 ++++++++++++++++++++++++++++++++
>  contrib/scalar/t/t9099-scalar.sh |  2 ++
>  2 files changed, 41 insertions(+)
>
> diff --git a/contrib/scalar/scalar.c b/contrib/scalar/scalar.c
> index e26fb2fc018..690933ffdf3 100644
> --- a/contrib/scalar/scalar.c
> +++ b/contrib/scalar/scalar.c
> @@ -653,6 +653,39 @@ cleanup:
>  	return res;
>  }
>
> +static void dir_file_stats(struct strbuf *buf, const char *path)
> +{
> +	DIR *dir = opendir(path);
> +	struct dirent *e;
> +	struct stat e_stat;
> +	struct strbuf file_path = STRBUF_INIT;
> +	size_t base_path_len;
> +
> +	if (!dir)
> +		return;
> +
> +	strbuf_addstr(buf, "Contents of ");
> +	strbuf_add_absolute_path(buf, path);
> +	strbuf_addstr(buf, ":\n");
> +
> +	strbuf_add_absolute_path(&file_path, path);
> +	strbuf_addch(&file_path, '/');
> +	base_path_len = file_path.len;
> +
> +	while ((e = readdir(dir)) != NULL)

Hmm. Is there a reason that this couldn't use
for_each_file_in_pack_dir() with a callback that just does the stat()
and buffer manipulation?

I don't think it's critical either way, but it would eliminate some of
the boilerplate that is shared between this implementation and the one
that already exists in for_each_file_in_pack_dir().

> +		if (!is_dot_or_dotdot(e->d_name) && e->d_type == DT_REG) {
> +			strbuf_setlen(&file_path, base_path_len);
> +			strbuf_addstr(&file_path, e->d_name);

For what it's worth, I think the callback would start here:

> +			if (!stat(file_path.buf, &e_stat))
> +				strbuf_addf(buf, "%-70s %16"PRIuMAX"\n",
> +					    e->d_name,
> +					    (uintmax_t)e_stat.st_size);

...and end here.

Thanks,
Taylor