Re: [PATCH v4 07/10] commit-graph: implement generation data chunk

>>> We test the overflow-related code with the following repo history:
>>>
>>>            F - N - U
>>>           /         \
>>> U - N - U            N
>>>          \          /
>>>            N - F - N
>> 
>> Do we need such complex history? I guess we need to test the handling of
>> merge commits too.
>> 
>
> I wanted to test three cases - a root epoch zero commit, a commit that's
> far enough in past to overflow the offset and a commit that's far enough
> in the future to overflow the offset.

>>>
>>> Where the commits denoted by U have committer date of zero seconds
>>> since Unix epoch, the commits denoted by N have committer date of
>>> 1112354055 (default committer date for the test suite) seconds since
>>> Unix epoch and the commits denoted by F have committer date of
>>> (2 ^ 31 - 2) seconds since Unix epoch.
>>>
>>> The largest offset observed is 2 ^ 31, just large enough to overflow.

>>> @@ -1120,6 +1149,44 @@ static int write_graph_chunk_data(struct hashfile *f,
>>>  	return 0;
>>>  }
>>>  
>>> +static int write_graph_chunk_generation_data(struct hashfile *f,
>>> +					      struct write_commit_graph_context *ctx)
>>> +{
>>> +	int i, num_generation_data_overflows = 0;
>> 
>> Minor nitpick: in my opinion there should be empty line here, between
>> the variables declaration and the code... however not all
>> write_graph_chunk_*() functions have it.
>> 
>>> +	for (i = 0; i < ctx->commits.nr; i++) {
>>> +		struct commit *c = ctx->commits.list[i];
>>> +		timestamp_t offset = commit_graph_data_at(c)->generation - c->date;
>>> +		display_progress(ctx->progress, ++ctx->progress_cnt);
>> 
>> All right.
>> 
>>> +
>>> +		if (offset > GENERATION_NUMBER_V2_OFFSET_MAX) {
>>> +			offset = CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW | num_generation_data_overflows;
>>> +			num_generation_data_overflows++;
>>> +		}
>> 
>> Hmmm... shouldn't we store these commits that need overflow handling
>> (with corrected commit date offset greater than GENERATION_NUMBER_V2_OFFSET_MAX)
>> in a list or a queue, to remember them for writing GDOV chunk?
>> 
>
> We could, although write_graph_chunk_extra_edges() (just like this function)
> prefers to iterate over all commits again. Both octopus merges and
> overflowing corrected commit dates are exceedingly rare, might be
> worthwhile to trade some memory to avoid looping again.

>> We could store oids, or we could store commits themselves, for example:
>> 
>> 		if (offset > GENERATION_NUMBER_V2_OFFSET_MAX) {
>> 			offset = CORRECTED_COMMIT_DATE_OFFSET_OVERFLOW | num_generation_data_overflows;
>> 			num_generation_data_overflows++;
>> 
>> 			ALLOC_GROW(ctx->gdov_commits.list, ctx->gdov_commits.nr + 1, ctx->gdov_commits.alloc);
>> 			ctx->commits.list[ctx->gdov_commits.nr] = c;
>>             ctx->gdov_commits.nr++;
>> 		}
>> 
>> Though in the above proposal we could get rid of `num_generation_data_overflows`, 
>> as it should be the same as `ctx->gdov_commits.nr`.
>> 
>> I have called the extra commit list member of write_commit_graph_context
>> `gdov_commits`, but perhaps a better name would be `commits_gen_v2_overflow`, 
>> or similar more descriptive name.

>>> @@ -741,4 +742,47 @@ test_expect_success 'corrupt commit-graph write (missing tree)' '
>>>  	)
>>>  '
>>>  
>>> +test_commit_with_date() {
>>> +  file="$1.t" &&
>>> +  echo "$1" >"$file" &&
>>> +  git add "$file" &&
>>> +  GIT_COMMITTER_DATE="$2" GIT_AUTHOR_DATE="$2" git commit -m "$1"
>>> +  git tag "$1"
>>> +}
>> 
>> Here we add a helper function.  All right.
>> 
>> I wonder though if it wouldn't be a better idea to add `--date <date>`
>> option to the test_commit() function in test-lib-functions.sh (which
>> option would set GIT_COMMITTER_DATE and GIT_AUTHOR_DATE, and also
>> set notick=yes).
>> 
>
> Yes, that's a better idea - I didn't know how to change test_commit()
> well enough to tinker with what's working.
>
>> For example:
>> 
>> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
>> index f1ae935fee..a1f9a2b09b 100644
>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -202,6 +202,12 @@ test_commit () {
>>  		--signoff)
>>  			signoff="$1"
>>  			;;
>> +        --date)
>> +            notick=yes
>> +            GIT_COMMITTER_DATE="$2"
>> +            GIT_AUTHOR_DATE="$2"
>> +            shift
>> +            ;;
>>  		-C)
>>  			indir="$2"
>>  			shift

>>> +test_expect_success 'overflow corrected commit date offset' '
>>> +	objdir=".git/objects" &&
>>> +	UNIX_EPOCH_ZERO="1970-01-01 00:00 +0000" &&
>>> +	FUTURE_DATE="@2147483646 +0000" &&
>> 
>> It is a bit funny to see UNIX_EPOCH_ZERO spelled one way, and
>> FUTURE_DATE other way.
>> 
>> Wouldn't be more readable to use UNIX_EPOCH_ZERO="@0 +0000"?
>
> It would, for some reason - I couldn't figure out the valid format for
> this. Changed.

>>> +graph_git_behavior 'overflow corrected commit date offset' repo left right
>> 
>> All right, here we compare the Git behavior with the commit-graph to the
>> behavior without it... however I think that those two tests really
>> should have distinct (different) test names. Currently they both use
>> 'overflow corrected commit date offset'.
>> 
>
> Following the earlier tests, the first test could be "set up and verify
> repo with generation data overflow chunk" and the git behavior test can
> be "generation data overflow chunk repo"

>> Though to reduce "noise" in this patch, the rename of run_three_modes()
>> to run_all_modes() and test_three_modes() to test_all_modes() could have
>> been done in a separate preparatory patch.  It would be pure refactoring
>> patch, without introducing any new functionality.
>> 
>
> Sure, that makes sense to me - this is patch is over 200 lines long
> already.