clone: add remote.cloneDefault config option

From: Sean Barag <[email protected]>

While the default remote name of origin can be overridden both pre-clone (with git clone --origin foo) and post-clone (with git remote rename origin foo), it'd be handy to have a configurable system-wide default for clones!

This commit implements
remote.cloneDefault as a parallel to remote.pushDefault, with prioritized name resolution:

builtin/clone.c +12 -0

index b087ee40c2..b0dbb848c6 100644
@@ -941,6 +941,29 @@ static int path_exists(const char *path)
 	return !stat(path, &sb);
 }
 
+struct clone_default_info
+{
+	enum config_scope scope;
+	struct strbuf remote_name;
+	int linenr;
+};
+
+static int config_read_clone_default(const char *key, const char *value,
+	void *cb)
+{
+	struct clone_default_info* info = cb;
+	if (strcmp(key, "remote.clonedefault") || !value) {

builtin/clone.c +0 -1

@@ -992,8 +1015,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_no_checkout = 1;
 	}
 
-	if (!option_origin)

t/t5606-clone-options.sh +3 -0

index e69427f881..8aac67b385 100755
@@ -19,6 +19,20 @@ test_expect_success 'clone -o' '
 
 '
 
+test_expect_success 'clone respects remote.cloneDefault' '
+
+	git -c remote.cloneDefault=bar clone parent clone-config &&

Quoted 3 lines · Sean Barag via GitGitGadget

This commit implements
remote.cloneDefault as a parallel to remote.pushDefault, with prioritized name resolution:

I highly doubt that .cloneDefault is a good name. After reading only the title of the patch e-mail, i.e. when the only available information on the change available to me was the name of the configuration variable and the fact that it pertains to the command "git clone", I thought it is to specify a URL, from which "git clone" without the URL would clone from that single repository.

And the name will cause the same misunderstanding to normal users, not just to reviewers of your patch, after this change hits a future Git release.

Taking a parallel from init.defaultBranchName, I would probably call it clone.defaultUpstreamName if I were writing this feature.

Quoted 11 lines

t/t5606-clone-options.sh +3 -0

index e69427f881..8aac67b385 100755
@@ -19,6 +19,20 @@ test_expect_success 'clone -o' '
 
 '
 
+test_expect_success 'clone respects remote.cloneDefault' '
+
+	git -c remote.cloneDefault=bar clone parent clone-config &&

t/t5606-clone-options.sh test_expect_success 'clone -o' ' · line 34

Quoted 10 lines

t/t5606-clone-options.sh +10 -0

+	(cd clone-config && git rev-parse --verify refs/remotes/bar/master)
+
+'
+
+test_expect_success 'clone chooses correct remote name' '
+
+	git -c remote.cloneDefault=bar clone -o foo parent clone-o-and-config &&
+	(cd clone-o-and-config && git rev-parse --verify refs/remotes/foo/master)
+
+'

These two are "showing off my shiny new toy" tests, which are needed, but we also need negative tests where the shiny new toy does not kick in when it should not. For example

git -c remote.cloneDefault="bad.../...name" clone parent

should fail, no?

Quoted 16 lines · Junio C Hamano

Quoted 3 lines · Sean Barag via GitGitGadget

This commit implements
remote.cloneDefault as a parallel to remote.pushDefault, with prioritized name resolution:

I highly doubt that .cloneDefault is a good name. After reading only the title of the patch e-mail, i.e. when the only available information on the change available to me was the name of the configuration variable and the fact that it pertains to the command "git clone", I thought it is to specify a URL, from which "git clone" without the URL would clone from that single repository.

And the name will cause the same misunderstanding to normal users, not just to reviewers of your patch, after this change hits a future Git release.

Quoted 2 lines

Taking a parallel from init.defaultBranchName, I would probably call it clone.defaultUpstreamName if I were writing this feature.

I was thinking "clone.defaultRemoteName" makes it clear we are naming the remote for the provided <url> in the command.

Quoted 5 lines

... For example

git -c remote.cloneDefault="bad.../...name" clone parent

should fail, no?

This is an important suggestion.

Quoted 17 lines · Derrick Stolee

Quoted 16 lines · Junio C Hamano

Quoted 3 lines · Sean Barag via GitGitGadget

This commit implements
remote.cloneDefault as a parallel to remote.pushDefault, with prioritized name resolution:

I highly doubt that .cloneDefault is a good name. After reading only the title of the patch e-mail, i.e. when the only available information on the change available to me was the name of the configuration variable and the fact that it pertains to the command "git clone", I thought it is to specify a URL, from which "git clone" without the URL would clone from that single repository.

And the name will cause the same misunderstanding to normal users, not just to reviewers of your patch, after this change hits a future Git release.

Quoted 5 lines

Quoted 2 lines

Taking a parallel from init.defaultBranchName, I would probably call it clone.defaultUpstreamName if I were writing this feature.

I was thinking "clone.defaultRemoteName" makes it clear we are naming the remote for the provided <url> in the command.

I 100% agree that defaultremotename is much better.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

... For example

git -c remote.cloneDefault="bad.../...name" clone parent

should fail, no?

This is an important suggestion.

To be fair, the current code does not handle the "--origin" command line option not so carefully.

It is somewhat sad that we have the git_config(git_default_config) call so late in the control flow. I wonder if we can update the start-up sequence to match the usual flow
...
One oddity "git clone" has is that it wants to delay the reading of

configuration files (they are read only once, and second and subsequent git_config() calls will reuse what was read before [*]) so that it can read what clone.c::write_config() wrote, so if we were to "fix" the start-up sequence to match the usual flow, we need to satisfy what that odd arrangement wanted to achieve in some other way (e.g. feed what is in option_config to git_default_config ourselves, without using git_config(), as part of the "main control flow uses the variable" part), but it should be doable.

Quoted 5 lines

It is somewhat sad that we have the git_config(git_default_config) call so late in the control flow. I wonder if we can update the start-up sequence to match the usual flow
...
One oddity "git clone" has is that it wants to delay the reading of

Quoted 8 lines

configuration files (they are read only once, and second and subsequent git_config() calls will reuse what was read before [*]) so that it can read what clone.c::write_config() wrote, so if we were to "fix" the start-up sequence to match the usual flow, we need to satisfy what that odd arrangement wanted to achieve in some other way (e.g. feed what is in option_config to git_default_config ourselves, without using git_config(), as part of the "main control flow uses the variable" part), but it should be doable.

Sounds like a pretty big change! I'm willing to take a crack at it, but given that this is my first patch I'm frankly a bit intimidated :) How would you feel about that being a separate patch?

In order to realize any new feature that needs to read the existing (i.e. per-machine or per-user) configuration files to affect the behaviour of "git clone", whether it is the "give default to --origin option" or any other thing, first needs to fix the start-up sequence so that the configuration is read once before we process command line options, which is the norm. Only after that is done, we can build the clone.defaultRemoteName and other features that would be affected by the settings of clone.* variables on top, and it is not wise to mix the foundation with a new feature that uses the foundation.