From: Justin Tobler <jltobler@gmail.com>
Date: Tue, 16 Dec 2025 14:48:30 GMT
Subject: Re: [PATCH v3 7/7] builtin/repo: add object disk size info to structure table
Message-ID: <y7kutectqntle5557tjmta44wwjvk2f4tvsxfuajaktj647275@6kupww6ldexe>
In-Reply-To: <aUEXeuCkMDWSfwHi@pks.im>

On 25/12/16 09:25AM, Patrick Steinhardt wrote:
> On Mon, Dec 15, 2025 at 02:56:39PM -0600, Justin Tobler wrote:
> > diff --git a/t/t1901-repo-structure.sh b/t/t1901-repo-structure.sh
> > index dd17caad05..64db191234 100755
> > --- a/t/t1901-repo-structure.sh
> > +++ b/t/t1901-repo-structure.sh
> > @@ -5,8 +5,18 @@ test_description='test git repo structure'
> >  . ./test-lib.sh
> >  
> >  object_type_disk_usage() {
> > -	git rev-list --all --objects --disk-usage --filter=object:type=$1 \
> > -		--filter-provided-objects
> > +	disk_usage_opt="--disk-usage"
> > +
> > +	if [ "$2" = "true" ]; then
> > +		disk_usage_opt="--disk-usage=human"
> > +	fi
> > +
> > +	if [ "$1" = "all" ]; then
> > +		git rev-list --all --objects $disk_usage_opt
> > +	else
> > +		git rev-list --all --objects $disk_usage_opt \
> > +			--filter=object:type=$1 --filter-provided-objects
> > +	fi
> >  }
> >  
> >  test_expect_success 'empty repository' '
> 
> We don't use `if [ ... ]` in our codebase, and we typically have the
> `then` on the next line:
> 
>     if test "$2" = "true"
>     then
>         ...
>     fi
> 
>     if test "$1" = "all"
>     then
>         ...
>     else
>         ...
>     fi

Noted, will fix.

> > @@ -79,6 +94,11 @@ test_expect_success SHA1 'repository with references and objects' '
> >  		|     * Trees          |  15.81 MiB |
> >  		|     * Blobs          |  11.68 KiB |
> >  		|     * Tags           |    132 B   |
> > +		|   * Disk size        | $(object_type_disk_usage all true) |
> > +		|     * Commits        | $(object_type_disk_usage commit true) |
> > +		|     * Trees          | $(object_type_disk_usage tree true) |
> > +		|     * Blobs          |  $(object_type_disk_usage blob true) |
> > +		|     * Tags           |    $(object_type_disk_usage tag) B   |
> >  		EOF
> 
> Curious, but why is the last one special here?

The `--disk-usage=human` rev-list option here outputs "byte/bytes"
instead of "B". In patch 5, the HUMANISE_COMPACT flag was added to
humanise_bytes() to toggle this behavior. For the git-repo(1) structure
table output, I wanted to always use the more compact unit prefix
representation.

I'll leave a comment here to explain this special case.

-Justin