Re: [PATCH v2 3/4] commit-graph: start parsing generation v2 (again)

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 28, 2022, 15:30 UTC
Message-ID: <220228.86czj73sre.gmgdl@evledraar.gmail.com>
In-Reply-To: <5bc6a7660d897ca6c52eabba8fb9ecfb6304dabb.1646056423.git.gitgitgadget@gmail.com>

On Mon, Feb 28 2022, Derrick Stolee via GitGitGadget wrote:

> From: Derrick Stolee <derrickstolee@github.com>
> [...]
> +	GENERATION_VERSION=2
> +	if test ! -z "$3"

TIL this works somewhere :)

I thought it *might* be unportable behavior (but didn't check at first), but it's not! We have a few such cases already.

But IMO much less puzzling would be at least:

    if ! test -z "$3"

Or in this case, more plainly:

    if test -n "$3"

Show 8 quoted lines

> +	then
> +		GENERATION_VERSION=$3
> +	fi
> +	OPTIONS=
> +	if test $GENERATION_VERSION -gt 1
> +	then
> +		OPTIONS=" read_generation_data"
> +	fi

Or actually, since we don't use $GENERATION_VERSION further down setting it to a default etc. here seems a bit odd. Perhaps something closer to:

    if test $# -eq 3 && test $3 -gt 1

It's also possible to be more clever as e.g.:

    test "${3:-2}" -gt 1

But that hardly seems worth it...

Show 23 quoted lines

>  NUM_COMMITS=9
> diff --git a/t/t5324-split-commit-graph.sh b/t/t5324-split-commit-graph.sh
> index 778fa418de2..669ddc645fa 100755
> --- a/t/t5324-split-commit-graph.sh
> +++ b/t/t5324-split-commit-graph.sh
> @@ -30,11 +30,16 @@ graph_read_expect() {
>  	then
>  		NUM_BASE=$2
>  	fi
> +	OPTIONS=
> +	if test -z "$3"
> +	then
> +		OPTIONS=" read_generation_data"
> +	fi
>  	cat >expect <<- EOF
>  	header: 43475048 1 $(test_oid oid_version) 4 $NUM_BASE
>  	num_commits: $1
>  	chunks: oid_fanout oid_lookup commit_metadata generation_data
> -	options:
> +	options:$OPTIONS
>  	EOF
>  	test-tool read-graph >output &&
>  	test_cmp expect output

Not a new issue, but it would be nice to have the mostly copy/pasted code in a lib-commit-graph.sh or something, but probably too distracting for this small series...