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

[Derrick Stolee <derrickstolee@github.com>]

On 2/28/2022 10:30 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> 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"

Sure, that makes sense.

>> +	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...

I prefer to use a variable so the code is self-documenting.

>>  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...

These cases are different enough in the needs of the test files
that they cannot be shared without significant complication.

Thanks,
-Stolee