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

Hi,

On Mon, 29 Mar 2021, Li Linchao via GitGitGadget wrote:

From: lilinchao <redacted>

I see "Li Linchao" in the email, but "lilinchao" in the author
information. Maybe you want to align them? Or maybe even use Unicode to
write your non-Latinized name?

In some scenarios, users may want more history than the repository
offered for cloning, which happens to be a shallow repository, can
give them. But because users don't know it is a shallow repository
until they download it to local, users should have the option to
refuse to clone this kind of repository, and may want to exit the
process immediately without creating any unnecessary files.

Althought there is an option '--depth=x' for users to decide how
deep history they can fetch, but as the unshallow cloning's depth
is INFINITY, we can't know exactly the minimun 'x' value that can
satisfy the minimum integrity, so we can't pass 'x' value to --depth,
and expect this can obtain a complete history of a repository.

In other scenarios, if we have an API that allow us to import external
repository, and then perform various operations on the repo.
But if the imported is a shallow one(which is actually possible), it
will affect the subsequent operations. So we can choose to refuse to
clone, and let's just import a normal repository.

This patch offers a new option '--reject-shallow' that can reject to
clone a shallow repository.

Good.

I like most of the patch, and will only point out a couple of things that
I think can be improved even further.

I am not a native speaker, either, but I believe that it would "roll off
the tongue" a bit better to say "to specify the default".

I would much prefer those variable names to include an indicator that this
is about _rejecting_ shallow clones. I.e. `option_reject_shallow`.

Also, I think that we can do with just a single `option_reject_shallow`
(we do not even need that `reject_shallow` variable in `cmd_clone()`):

- in `git_clone_config()`, only override it if it is still unspecified:

	if (!strcmp(k, "clone.rejectshallow") && option_reject_shallow < 0)
		option_reject_shallow = git_config_bool(k,v);

- in `cmd_clone()`, test for a _positive_ value:

	if (option_reject_shallow > 0)
		die(_("source repository is shallow, reject to clone."));

  and

	if (option_reject_shallow > 0)
 		transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");

One thing to note (in the commit message, would be my preference) is that
`cmd_clone()` is _particular_ in that it runs `git_config()` _twice_. Once
before the command-line options are parsed, and once after the new Git
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.

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.

Please name this attribute `reject_shallow` instead of `remote_shallow`.
That will prevent future puzzlement.

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

Should this line not come immediately after the bare clone into
<DOCUMENT_ROOT>/repo.git? Or even better, as a `test_when_finished`
command.

And maybe you want to extract this preparatory step into its own test
case:

test_expect_success 'set up shallow http repository' '
	test_when_finished "rm -rf shallow-repo" &&
	git clone --depth=1 --no-local parent shallow-repo &&
	git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
'

Please remove the extra empty line. (This goes for at least a couple test
cases added by this patch.)

+'

This test case does not require `start_httpd`, and should therefore come
before the test cases that do require it (actually, it should come before
the `start_httpd` call, even).

Hmm. Reading through three test cases that all create `shallow-repo` in
the same way, I wonder whether we should not simply set it up once, and
then not even bother removing it. I think that would simplify not only the
patch, it would also simplify debugging later on.

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.

In the following, this shallow repository is needed a couple of times.
Better set it up once, in a dedicated `set up shallow repository` test
case.

And `shallow-repo` would probably make for a much better name than
`child`.

These two can be combined (and should, if you ask me, to simplify things).

Personally, I think this is overkill. What I would do is to have a single
test case that verifies that

- `clone.rejectShallow=true` fails as expected,

- `clone.rejectShallow=false [...] --reject-shallow` fails as expected, and

- `clone.rejectShallow=false` succeeds.

If we do this, we do not even need a preparatory test case setting up the
shallow repository.

I see that this is the established pattern (I am so grateful that I have
https://github.com/gitgitgadget/git/pull/865/files to look at the context,
something with which a pure mail-only patch contribution would not bless
me!), that those Boolean options are `NULL` vs non-`NULL`. So while you
pass `"1"` as the `value` parameter to `set_git_option()`, the parameter
`"0"` would _enable that option just the same_, you would have to pass
`NULL` to turn it off. I find that highly unintuitive, but that's not the
fault of your patch. The pattern is established, and you did the right
thing by following it.

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

Thank you,
Dscho