Re: [PATCH v8] builtin/clone.c: add --reject-shallow option

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2021, 17:46 UTC
Message-ID: <xmqqk0poedr4.fsf@gitster.g>
In-Reply-To: <nycvar.QRO.7.76.6.2103301110040.52@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 5 quoted lines

>> +static int option_shallow = -1;    /* unspecified */
>> +static int config_shallow = -1;    /* unspecified */
>
> I would much prefer those variable names to include an indicator that this
> is about _rejecting_ shallow clones. I.e. `option_reject_shallow`.

Good.

> ... repository has been initialized. Note that my suggestion still works with
> that: if either the original config, or the new config set
> `clone.rejectShallow`, it is picked up correctly, with the latter
> overriding the former if both configs want to set it.

All four combinations ("set it to true" vs "set it to false" makes two, and before and after doubles that to four)?

Show 15 quoted lines

>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index fb04a76ca263..34d0c2896e2e 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -1129,9 +1129,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>>  	if (args->deepen)
>>  		setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
>>  					NULL);
>> -	else if (si->nr_ours || si->nr_theirs)
>> +	else if (si->nr_ours || si->nr_theirs) {
>> +		if (args->remote_shallow)
>
> Even as a non-casual reader, this name `remote_shallow` leads me to assume
> incorrect things. This option is not about wanting a remote shallow
> repository, it is about rejecting a remote shallow repository.

Yeah, I've seen this code too long that my eyes were contaminated ;-) Thanks for an extra set of eyeballs to spot this. What this option means is to "reject-shallow-remote", and I agree 'reject' is the most important part (args->reject_shallow_remote is not overly long, either).

Show 22 quoted lines

>> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
>> index 428b0aac93fa..de1cd85983ed 100755
>> --- a/t/t5606-clone-options.sh
>> +++ b/t/t5606-clone-options.sh
>> @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>>  export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>
>>  . ./test-lib.sh
>> +. "$TEST_DIRECTORY"/lib-httpd.sh
>> +start_httpd
>
> That's not good. What happens if there is no `httpd`? Then the rest of the
> tests are either skipped, or if `GIT_TEST_HTTPD` is set to `true`, we
> fail. The failure is intentional, but the skipping is not. There are many
> tests in t5606 that do not require a running HTTP daemon, and we should
> not skip them (for example, in our CI runs, there are quite a few jobs
> that run without any working `httpd`).
>
> A much better alternative, I think, would be to move those new test cases
> that require `httpd` to be running to t5601 (which _already_ calls
> `start_httpd`, near the end, so as to not skip any tests that do not
> require `httpd`).

Or move the tests that require httpd, and use of httpd library, to the end of this script.

Show 23 quoted lines

> Having read through these test cases, I think they can be simplified by
>
> - first setting up `shallow-repo`
>
> - then testing in the same test case whether `--reject-shallow` fails and
>   `--no-reject-shallow` succeeds, without `--no-local`
>
> - then testing the same _with_ `--no-local`
>
> These can go to t5606, no problem.
>
> Then, in t5601, after the `start_httpd` call, add a single test case that
>
> - sets up the shallow clone _directly_, i.e.
>
> 	git clone --bare --no-local --depth=1 parent \
> 		"$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
>
> - verifies that `--reject-shallow` fails as expected, and
>
> - verifies that `--no-reject-shallow` works as expected.
>
>>  test_expect_success 'uses "origin" for default remote name' '

That would work.

> ...
> As I said, the rest of the patch looks good to me. With the few
> suggestions I offered, I would be totally fine with this patch entering
> `next`.

Thanks for a review.  Looking good.