clone: allow configurable default for -o/--origin

Took another pass at supporting a configurable default for-o/--origin, this time following Junio's suggestions from a previous approach as much as possible [1]. Unfortunately, Johannes mentioned that --template can write new config values that aren't automatically merged without re-calling git_config. There doesn't appear to be a way around this without rewriting significant amounts of init and config logic across the codebase.

While this could have been v2 of the original patchset, it's diverged so drastically from the original that it likely warrants its own root thread. If that's not appropriate though, I'd be happy to restructure!

Thanks for all the advice Junio and Johannes! This feels significantly better than my first attempt.

[1]
https://lore.kernel.org/git/[email protected]/
[2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195

Sean Barag (4):
clone: add tests for --template and some disallowed option pairs
clone: call git_config before parse_options
clone: validate --origin option before use
clone: allow configurable default for -o/--origin

 Documentation/config.txt       |  2 +
 Documentation/config/clone.txt |  5 +++
 Documentation/git-clone.txt    |  5 ++-
 builtin/clone.c                | 65 ++++++++++++++++++++++++++-------
 t/t5606-clone-options.sh       | 67 ++++++++++++++++++++++++++++++++++
 5 files changed, 128 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/config/clone.txt

base-commit: 54e85e7af1ac9e9a92888060d6811ae767fea1bc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-727%2Fsjbarag%2Fclone_add-configurable-default-for--o-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-727/sjbarag/clone_add-configurable-default-for--o-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/727

gitgitgadget

From: Sean Barag <[email protected]>

Some combinations of command-line options to git clone are invalid, but there were previously no tests ensuring those combinations reported errors. Similarly, git clone --template didn't appear to have any tests.

---
 t/t5606-clone-options.sh | 44 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

index e69427f881..d20a78f84b 100755
@@ -19,6 +19,50 @@ test_expect_success 'clone -o' '
 
 '
 
+test_expect_success 'disallows --bare with --origin' '
+
+	test_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&
+	test_debug "cat err" &&
+	test_i18ngrep "\-\-bare and --origin foo options are incompatible" err
+
+'
+
+test_expect_success 'disallows --bare with --separate-git-dir' '
+
+	test_expect_code 128 git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err &&
+	test_debug "cat err" &&
+	test_i18ngrep "\-\-bare and --separate-git-dir are incompatible" err
+
+'
+
+test_expect_success 'uses "origin" for default remote name' '
+
+	git clone parent clone-default-origin &&
+	(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)
+
+'
+
+test_expect_success 'prefers --template config over normal config' '
+
+	template="$TRASH_DIRECTORY/template-with-config" &&
+	mkdir "$template" &&
+	git config --file "$template/config" foo.bar from_template &&
+	test_config_global foo.bar from_global &&
+	git clone "--template=$template" parent clone-template-config &&
+	(cd clone-template-config && test "$(git config --local foo.bar)" = "from_template")
+
+'
+
+test_expect_success 'prefers -c config over --template config' '
+
+	template="$TRASH_DIRECTORY/template-with-ignored-config" &&
+	mkdir "$template" &&
+	git config --file "$template/config" foo.bar from_template &&
+	git clone "--template=$template" -c foo.bar=inline parent clone-template-inline-config &&
+	(cd clone-template-inline-config && test "$(git config --local foo.bar)" = "inline")
+
+'
+
 test_expect_success 'redirected clone does not show progress' '
 
 	git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&

gitgitgadget

And the beginning. :)

It's matching the surrounding test, which are written in an older inconsistent style. I think it's OK to match them, but cleaning them would be welcome, too.

While I'm looking at this hunk, two other things:

- do we really care about code 128, or just failure? test_must_fail might be a better choice

- I didn't even know we had test_debug. ;) The last time somebody added a call to it was in 2012. I think it's being used as intended here, but I'm not sure that the clutter to the test is worth it (we have other tools like "-i" to stop at the right spot and let you inspect the broken state).

- the backslash escapes confused me for a moment. I guess they are trying to hide the dashes from grep's option parser. That's OK, though I'd have probably just started with "bare" since we're matching a substring anyway. I think you could also use "-e" with test_i18ngrep.

-Peff

Thanks, you said everything I was going to say about this, thus saving me some typing.

A couple more observations related to a few of the subsequent tests. This:

    template="$TRASH_DIRECTORY/template-with-config" &&

probably doesn't need $TRASH_DIRECTORY since that happens to be the current working directory anyhow, so:

    template=template-with-config &&

should suffice (unless you had a problem doing it that way). Or you could drop the variable altogether and use the literal name where you need it.

Also, instead of:

    (cd clone-template-config && test "$(git config --local foo.bar)"

= "from_template")

would probably be written these days as:

    echo from_template >expect &&
    git -C clone-template-config config --local foo.bar >actual &&
    test_cmp expect actual

I hadn't realized $TRASH_DIRECTORY was the pwd, but it makes perfect sense. Will update for v2.

Oooh test_cmp looks much better. Thanks for the tip!

Me either :). I think what you said about using -i to inspect whatever is broken applies not only to this test, but also to all other tests in the suite.

I'd be pretty happy to see 'test_debug' go away. There are only 104 uses of it in the tree, and I think most of them could simply be removed without needing to touch anything else.

I've never found it useful in debugging a problem on my end, but perhaps others have. Food for thought.

Thanks,
Taylor

might be a better choice

Good point - test_must_fail is probably fine here. I went with an explicit error code so this test wouldn't pass in the event of an outright crash, but I'm happy to adjust for v2.

a call to it was in 2012. I think it's being used as intended here, but I'm not sure that the clutter to the test is worth it (we have other tools like "-i" to stop at the right spot and let you inspect the broken state).

Frankly I'd forgotten I'd included it! It's definitely not necessary. Will remove for v2 as well.

trying to hide the dashes from grep's option parser. That's OK, though I'd have probably just started with "bare" since we're matching a substring anyway. I think you could also use "-e" with test_i18ngrep.

Adding -e would solve this handily. Thanks for the suggestion!

Sean Barag

That's good thinking, but test_must_fail already has you covered; it will complain about any death-by-signal. It wouldn't distinguish between, say exit codes 128 and 1, but 128 is the code used by our die() function, so expecting it isn't very specific anyway. :)

-Peff

That matches the older style to make the test body stand out by having blank lines around it. All existing tests from the era (not limited to this script) used to do so.

It is OK to update them to modern style, but let's not do so in the middle of the series.

Thanks.

Thanks for correcting me. (and Peff for doing the same).

I should have looked at the context better, because that is frequently the reason for "inconsistent" style. My apologies.

-Stolee

From: Sean Barag <[email protected]>

While Junio's request [1] was to avoids the unusual "write config then immediately read it" pattern that exists in cmd_clone, Johannes mentioned that --template can write new config values that aren't automatically included in the environment [2]. This requires a config re-read after init_db is called.

Moving the initial config up does allow settings from config to be overwritten by ones provided via CLI options in a more natural way though, so that part of Junio's suggestion remains.

[1] https://lore.kernel.org/git/[email protected]/
[2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195

Thanks-to: Junio C Hamano <[email protected]>
Thanks-to: Johannes Schindelin <[email protected]>

---
 builtin/clone.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

index b087ee40c2..bf095815f0 100644
@@ -851,8 +851,21 @@ static int checkout(int submodule_progress)
 	return err;
 }
 
+static int git_clone_config(const char *k, const char *v, void *cb)
+{
+	return git_default_config(k, v, cb);
+}
+
 static int write_one_config(const char *key, const char *value, void *data)
 {
+	/*
+	 * give git_config_default a chance to write config values back to the environment, since
+	 * git_config_set_multivar_gently only deals with config-file writes
+	 */
+	int apply_failed = git_default_config(key, value, data);
+	if (apply_failed)
+		return apply_failed;
+
 	return git_config_set_multivar_gently(key,
 					      value ? value : "true",
 					      CONFIG_REGEX_NONE, 0);

@@ -964,6 +977,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	struct strvec ref_prefixes = STRVEC_INIT;
 
 	packet_trace_identity("clone");
+
+	git_config(git_clone_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
 			     builtin_clone_usage, 0);
 

@@ -1125,9 +1141,17 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (real_git_dir)
 		git_dir = real_git_dir;
 
+	/*
+	 * additional config can be injected with -c, make sure it's included
+	 * after init_db, which clears the entire config environment.
+	 */
 	write_config(&option_config);
 
-	git_config(git_default_config, NULL);
+	/*
+	 * re-read config after init_db and write_config to pick up any config
+	 * injected by --template and --config, respectively
+	 */
+	git_config(git_clone_config, NULL);
 
 	if (option_bare) {
 		if (option_mirror)

gitgitgadget

The title says what the code does after this change. The code calls git_config() before calling parse_options(), but not much in the proposed log message explains what the patch tries to achieve by doing so.

The above refers to suggestions but does not describe what problem the patch is trying to address and what approach is taken to address it.

Usually these two are spelled Helped-by: in this project, and are given in chronological order. People gave input to you and then finally you send out a signed copy, so your sign-off is placed at the end of the sequence.

Thanks for the pointer - I completely agree. Rewriting for v2.

How embarrassing! Re-wrapped for v2.

Great catch! This gets changed to git_clone_config in patch 4/4 anyway, so there's no need for the confusing intermediate state in 2/4. Will be fixed in v2.

--
Thanks for being so patient with a newbie :)
Sean

From: Sean Barag <[email protected]>

Providing a bad origin name to git clone currently reports an 'invalid refspec' error instead of a more explicit message explaining that the --origin option was malformed. Per Junio, it's been this way since 8434c2f1 (Build in clone, 2008-04-27). This patch reintroduces validation for the provided --origin option, but notably _doesn't_ include a multi-level check (e.g. "foo/bar") that was present in the original git-clone.sh. It seems git remote allows multi-level remote names, so applying that same validation in git clone seems reasonable.

Thanks-to: Junio C Hamano <[email protected]>

---
 builtin/clone.c          | 7 +++++++
 t/t5606-clone-options.sh | 8 ++++++++
 2 files changed, 15 insertions(+)

index bf095815f0..1cd62d0001 100644
@@ -967,6 +967,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	const struct ref *ref;
 	struct strbuf key = STRBUF_INIT;
 	struct strbuf default_refspec = STRBUF_INIT;
+	struct strbuf resolved_refspec = STRBUF_INIT;
 	struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;
 	struct transport *transport = NULL;
 	const char *src_ref_prefix = "refs/heads/";

@@ -1011,6 +1012,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (!option_origin)
 		option_origin = "origin";
 
+	strbuf_addf(&resolved_refspec, "refs/heads/test:refs/remotes/%s/test", option_origin);
+	if (!valid_fetch_refspec(resolved_refspec.buf))
+		/* TRANSLATORS: %s will be the user-provided --origin / -o option */
+		die(_("'%s' is not a valid origin name"), option_origin);
+	strbuf_release(&resolved_refspec);
+
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);

index d20a78f84b..c865f96def 100755
@@ -19,6 +19,14 @@ test_expect_success 'clone -o' '
 
 '
 
+test_expect_success 'rejects invalid -o/--origin' '
+
+	test_expect_code 128 git clone -o "bad...name" parent clone-bad-name 2>err &&
+	test_debug "cat err" &&
+	test_i18ngrep "'\''bad...name'\'' is not a valid origin name" err
+
+'
+
 test_expect_success 'disallows --bare with --origin' '
 
 	test_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&

gitgitgadget

Makes perfect sense! My original intention was to provide a separate use-case specific message, but you're right: "is not a valid remote name" (as in builtin/remote.c) is still very clear.

That's a fantastic idea -- thanks for the suggestion! I'll do that for v2.

Darn, I missed this one. Thanks for the heads-up :)

--
Sean

If you know it as a fact that it has been this way since the first version of rewritten "git clone", don't blame others.

A micronit. We describe the current status in present tense when presenting the problem to be solved, so "currently" can be dropped.

When presenting the solution, we write as if we are giving an order to a patch monkey to "make the code like so".

"This patch reintroduces" -> "Reintroduce".

Even though I suspect "git remote" is being overly loose and careless here, I am not sure if it is a good idea to tighten it retroactively. But if this is meant as a bugfix for misconversion made in 8434c2f1, we should be as strict as the original. I dunno.

	if (!valid_fetch_refspec(...) || strchr(option_origin, '/'))

Oh gosh, I'm so sorry! I didn't mean for that to read as blaming, was just trying to cite you as my source. On a second read, it comes across more "blame-y" than I originally intended. I'll fix this in v2 (and have learned a valuable lesson :) ).

Great to know! Thanks again for helping a newbie fit in. Will fix in v2.

To be honest, I'm struggling to decide which route to go. It seems like multilevel fetch and push refspecs are allowed as far back as 46220ca100 (remote.c: Fix overtight refspec validation, 2008-03-20) or ef00d150e4 (Tighten refspec processing, 2008-03-17), so perhaps removing the multilevel check in 8434c2f1 (Build in clone, 2008-04-27) was intentional? If removing that check in 8434c2f1 was a mistake and we reintroduce it, that's probably a breaking change for some users. What sort of accommodations would I need to include in this patchset to ease that pain for users?

Absolutely! Happy to include the multilevel check if you think it's the right path forward (see above).

Agreed. I plan to take Derrick's suggestion [1] and use the same " is not a valid remote name" message from builtin/remote.c, which should make that translator comment a non-issue.

[1] https://lore.kernel.org/git/[email protected]/

I can't stress enough how sorry I am for the improper blame, and how much I appreciate your help!
--
Sean

Thinking about this more, I'd be very surprised as a git user if introducing a new config option broke backwards compatibility. Other git UIs have mixed support for slashes in remote names:

* GitHub Desktop has an open issue regarding remote names that contain
slashes: https://github.com/desktop/desktop/issues/3618
* Sublime Merge (as-of build 2032) renders remote names with slashes as a tree of remotes, e.g.:

      $ git remote -v
      foo/bar     /tmp/example_repo
      foo/baz     /tmp/example_repo2

is rendered with as a collapsible tree, roughly:

      REMOTES (2)
      v foo
        bar
        baz

* tig (2.4.1) renders remote names with slashes identically to those without slashes

Retroactively tightening those rules would cause more harm than good (both for end-users and for downstream projects), especially with no safe way to fix existing /-containing remote names. I'm going to keep the multilevel check out of this patchset (thus continuing to allowing multilevel remote names), but if anyone feels strongly either way please let me know! :)

Sean

From: Sean Barag <[email protected]>

While the default remote name of "origin" can be changed at clone-time with git clone's --origin option, it was previously not possible to specify a default value for the name of that remote. This commit adds support for a new clone.defaultRemoteName config.

It's resolved in the expected priority order:

1. (Highest priority) A remote name passed directly to git clone -o
2. A clone.defaultRemoteName=new_name in config git clone -c
3. A clone.defaultRemoteName value set in /path/to/template/config, where --template=/path/to/template is provided
3. A clone.defaultRemoteName value set in a non-template config file
4. The default value of origin

Thanks-to: Junio C Hamano <[email protected]>
Thanks-to: Johannes Schindelin <[email protected]>

---
 Documentation/config.txt       |  2 ++
 Documentation/config/clone.txt |  5 ++++
 Documentation/git-clone.txt    |  5 ++--
 builtin/clone.c                | 42 +++++++++++++++++++---------------
 t/t5606-clone-options.sh       | 29 +++++++++++++++++------
 5 files changed, 56 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/config/clone.txt

index 3042d80978..354874facf 100644
@@ -334,6 +334,8 @@ include::config/checkout.txt[]
 
 include::config/clean.txt[]
 
+include::config/clone.txt[]
+
 include::config/color.txt[]
 
 include::config/column.txt[]

new file mode 100644
index 0000000000..20755d413a
@@ -0,0 +1,5 @@
+clone.defaultRemoteName::
+	The name of the remote to create when cloning a repository.  Defaults to
+	`origin`, and can be overridden by passing the `--origin` command-line
+	option to linkgit:git-clone[1].
+

index c898310099..f04bf6e6ba 100644
@@ -183,8 +183,9 @@ objects from the source repository into a pack in the cloned repository.
 
 -o <name>::
 --origin <name>::
-	Instead of using the remote name `origin` to keep track
-	of the upstream repository, use `<name>`.
+	Instead of using the remote name `origin` to keep track of the upstream
+	repository, use `<name>`.  Overrides `clone.defaultRemoteName` from the
+	config.
 
 -b <name>::
 --branch <name>::

index c865f96def..017c24a454 100755
@@ -43,13 +43,6 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
 
 '
 
-test_expect_success 'uses "origin" for default remote name' '
-
-	git clone parent clone-default-origin &&
-	(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)
-
-'
-
 test_expect_success 'prefers --template config over normal config' '
 
 	template="$TRASH_DIRECTORY/template-with-config" &&

@@ -71,6 +64,28 @@ test_expect_success 'prefers -c config over --template config' '
 
 '
 
+test_expect_success 'uses "origin" for default remote name' '
+
+	git clone parent clone-default-origin &&
+	(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)
+
+'
+
+test_expect_success 'prefers config "clone.defaultRemoteName" over default' '
+
+	test_config_global clone.defaultRemoteName from_config &&
+	git clone parent clone-config-origin &&
+	(cd clone-config-origin && git rev-parse --verify refs/remotes/from_config/master)
+
+'
+
+test_expect_success 'prefers --origin over -c config' '
+
+	git clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config &&
+	(cd clone-o-and-inline-config && git rev-parse --verify refs/remotes/from_option/master)
+
+'
+
 test_expect_success 'redirected clone does not show progress' '
 
 	git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&

gitgitgadget

This patch could have used a prep patch that had all consumers of option_origin use remote_name instead, with this adjustment to the way to use option_origin:

...

That's a great idea! Implemented for v2 :)

Sean

Great catch - fixed for v2.

That's true! The special case was there mostly to keep --origin as the highest priority, but that can be solved much more naturally by moving the assignment from option_origin to remote_name down below the second git_config call. Included in v2.

Sean

Number 3 is used twice in this list.

Thanks for this update. I like the feature quite a bit, and all of my comments are about style and organization instead of functional issues.

I think it is fine to restart the thread if the range-diff is not helpful.

Thanks,
-Stolee

I did wonder if this series came from the same motivation while scanning messages in the mailbox and saw no "v2" in the subject, but I am OK either way in a case like this.

I DO appreciate this note to explain why it is not marked with "v2".

v2 (changes since v1):

* Convert Thanks-to trailer to Helped-by
* Rewrite several commit titles and messages
* Unify error reporting between clone.c and remote.c
* Add tests for git remote add and git remote rename with invalid remote names
* Prevent leak of old remote_name

v1: Took another pass at supporting a configurable default for-o/--origin, this time following Junio's suggestions from a previous approach as much as possible [1]. Unfortunately, Johannes mentioned that --template can write new config values that aren't automatically merged without re-calling git_config. There doesn't appear to be a way around this without rewriting significant amounts of init and config logic across the codebase.

While this could have been v2 of the original patchset, it's diverged so drastically from the original that it likely warrants its own root thread. If that's not appropriate though, I'd be happy to restructure!

Thanks for all the advice Junio and Johannes! This feels significantly better than my first attempt.

[1]
https://lore.kernel.org/git/[email protected]/
[2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195

Sean Barag (7):
clone: add tests for --template and some disallowed option pairs
clone: use more conventional config/option layering
remote: add tests for add and rename with invalid names
refs: consolidate remote name validation
clone: validate --origin option before use
clone: read new remote name from remote_name instead of option_origin
clone: allow configurable default for -o/--origin

 Documentation/config.txt       |  2 +
 Documentation/config/clone.txt |  5 +++
 Documentation/git-clone.txt    |  5 ++-
 builtin/clone.c                | 70 ++++++++++++++++++++++++++--------
 builtin/remote.c               |  7 +---
 refspec.c                      | 10 +++++
 refspec.h                      |  1 +
 t/t5505-remote.sh              | 13 +++++++
 t/t5606-clone-options.sh       | 68 ++++++++++++++++++++++++++++++++-
 9 files changed, 158 insertions(+), 23 deletions(-)
 create mode 100644 Documentation/config/clone.txt

base-commit: 9bc233ae1cf19a49e51842c7959d80a675dbd1c0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-727%2Fsjbarag%2Fclone_add-configurable-default-for--o-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-727/sjbarag/clone_add-configurable-default-for--o-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/727

Range-diff vs v1:

1: 4cdcedff31 ! 1: 29ab418b1b clone: add tests for --template and some disallowed option pairs

     @@ Commit message
          errors.  Similarly, `git clone --template` didn't appear to have any
          tests.
      
     +    Helped-by: Jeff King <[email protected]>
     +    Helped-by: Derrick Stolee <[email protected]>
          Signed-off-by: Sean Barag <[email protected]>
      
       ## t/t5606-clone-options.sh ##
     -@@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '
     - 
     - '
     +@@ t/t5606-clone-options.sh: test_expect_success 'setup' '
     + test_expect_success 'clone -o' '
       
     + 	git clone -o foo parent clone-o &&
     +-	(cd clone-o && git rev-parse --verify refs/remotes/foo/master)
     ++	git -C clone-o rev-parse --verify refs/remotes/foo/master
     ++
     ++'
     ++
      +test_expect_success 'disallows --bare with --origin' '
      +
     -+	test_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&
     ++	test_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&
      +	test_debug "cat err" &&
     -+	test_i18ngrep "\-\-bare and --origin foo options are incompatible" err
     ++	test_i18ngrep -e "--bare and --origin foo options are incompatible" err
      +
      +'
      +
      +test_expect_success 'disallows --bare with --separate-git-dir' '
      +
     -+	test_expect_code 128 git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err &&
     ++	test_must_fail git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err &&
      +	test_debug "cat err" &&
     -+	test_i18ngrep "\-\-bare and --separate-git-dir are incompatible" err
     ++	test_i18ngrep -e "--bare and --separate-git-dir are incompatible" err
      +
      +'
      +
      +test_expect_success 'uses "origin" for default remote name' '
      +
      +	git clone parent clone-default-origin &&
     -+	(cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)
     ++	git -C clone-default-origin rev-parse --verify refs/remotes/origin/master
      +
      +'
      +
     @@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '
      +	git config --file "$template/config" foo.bar from_template &&
      +	test_config_global foo.bar from_global &&
      +	git clone "--template=$template" parent clone-template-config &&
     -+	(cd clone-template-config && test "$(git config --local foo.bar)" = "from_template")
     ++	test "$(git -C clone-template-config config --local foo.bar)" = "from_template"
      +
      +'
      +
     @@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '
      +	mkdir "$template" &&
      +	git config --file "$template/config" foo.bar from_template &&
      +	git clone "--template=$template" -c foo.bar=inline parent clone-template-inline-config &&
     -+	(cd clone-template-inline-config && test "$(git config --local foo.bar)" = "inline")
     -+
     -+'
     -+
     - test_expect_success 'redirected clone does not show progress' '
     ++	test "$(git -C clone-template-inline-config config --local foo.bar)" = "inline"
     + 
     + '
       
     - 	git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&

2: 51ef776f85 ! 2: e3479e7b35 clone: call git_config before parse_options
@@ Metadata
Author: Sean Barag <[email protected]>

## Commit message ##
- clone: call git_config before parse_options
+ clone: use more conventional config/option layering

- While Junio's request [1] was to avoids the unusual "write config then
- immediately read it" pattern that exists in cmd_clone, Johannes
- mentioned that --template can write new config values that aren't
- automatically included in the environment [2]. This requires a config
- re-read after init_db is called.
-
- Moving the initial config up does allow settings from config to be
- overwritten by ones provided via CLI options in a more natural way
- though, so that part of Junio's suggestion remains.
-
- [1] https://lore.kernel.org/git/[email protected]/
- [2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195
+ Parsing command-line options before reading from config required careful
+ handling to ensure CLI options were treated with higher priority. Read
+ config first to let parsed CLI naively overwrite matching config values.

+ Helped-by: Junio C Hamano <[email protected]>
+ Helped-by: Johannes Schindelin <[email protected]>

- Thanks-to: Junio C Hamano <[email protected]>
- Thanks-to: Johannes Schindelin <[email protected]>

## builtin/clone.c ##
@@ builtin/clone.c: static int checkout(int submodule_progress)
@@ builtin/clone.c: static int checkout(int submodule_progress)
static int write_one_config(const char *key, const char *value, void *data) {
+ /*
-+ * give git_config_default a chance to write config values back to the environment, since
-+ * git_config_set_multivar_gently only deals with config-file writes
++ * give git_clone_config a chance to write config values back to the
++ * environment, since git_config_set_multivar_gently only deals with
++ * config-file writes
+ */
-+ int apply_failed = git_default_config(key, value, data);
++ int apply_failed = git_clone_config(key, value, data);
+ if (apply_failed)
+ return apply_failed;
+

 -:  ---------- > 3:  85be780b8e remote: add tests for add and rename with invalid names
 -:  ---------- > 4:  42dc18601a refs: consolidate remote name validation
 -:  ---------- > 5:  fdc9d3b369 clone: validate --origin option before use
 4:  5c519376c2 ! 6:  99f4d765b4 clone: allow configurable default for `-o`/`--origin`

@@ Metadata
Author: Sean Barag <[email protected]>

## Commit message ##
- clone: allow configurable default for -o/--origin
+ clone: read new remote name from remote_name instead of option_origin

- While the default remote name of "origin" can be changed at clone-time
- with git clone's --origin option, it was previously not possible
- to specify a default value for the name of that remote. This commit
- adds support for a new clone.defaultRemoteName config.
-
- It's resolved in the expected priority order:
-
- 1. (Highest priority) A remote name passed directly to git clone -o
- 2. A clone.defaultRemoteName=new_name in config git clone -c
- 3. A clone.defaultRemoteName value set in /path/to/template/config,
- where --template=/path/to/template is provided
- 3. A clone.defaultRemoteName value set in a non-template config file
- 4. The default value of origin
+ In a future patch, the name of the remote created by git clone may
+ come from multiple sources. To avoid confusion, convert most uses of
+ option_origin to remote_name, leaving option_origin to exclusively
+ represent the -o/--origin option.

+ Helped-by: Derrick Stolee <[email protected]>

- Thanks-to: Junio C Hamano <[email protected]>
- Thanks-to: Johannes Schindelin <[email protected]>
-
- ## Documentation/config.txt ##
-@@ Documentation/config.txt: include::config/checkout.txt[]
-
- include::config/clean.txt[]
-
-+include::config/clone.txt[]
-+
- include::config/color.txt[]
-
- include::config/column.txt[]
-
- ## Documentation/config/clone.txt (new) ##
-@@
-+clone.defaultRemoteName::
-+ The name of the remote to create when cloning a repository. Defaults to
-+ origin, and can be overridden by passing the --origin command-line
-+ option to linkgit:git-clone[1].
-+
-
- ## Documentation/git-clone.txt ##
-@@ Documentation/git-clone.txt: objects from the source repository into a pack in the cloned repository. -
- -o <name>::
- --origin <name>::
-- Instead of using the remote name origin to keep track
-- of the upstream repository, use <name>.
-+ Instead of using the remote name origin to keep track of the upstream
-+ repository, use <name>. Overrides clone.defaultRemoteName from the
-+ config.
-
- -b <name>::
- --branch <name>::

## builtin/clone.c ##
@@ builtin/clone.c: static int option_shallow_submodules;
@@ builtin/clone.c: static void update_head(const struct ref *our, const struct ref
}
} else if (our) {
struct commit *c = lookup_commit_reference(the_repository,
-@@ builtin/clone.c: static int checkout(int submodule_progress)
-
- static int git_clone_config(const char *k, const char *v, void *cb)
- {
-+ if (!strcmp(k, "clone.defaultremotename") && !option_origin)
-+ remote_name = xstrdup(v);
- return git_default_config(k, v, cb);
- }
-
- static int write_one_config(const char *key, const char *value, void *data)
- {
- /*
-- * give git_config_default a chance to write config values back to the environment, since
-+ * give git_clone_config a chance to write config values back to the environment, since
- * git_config_set_multivar_gently only deals with config-file writes
- */
-- int apply_failed = git_default_config(key, value, data);
-+ int apply_failed = git_clone_config(key, value, data);
- if (apply_failed)
- return apply_failed;
-
@@ builtin/clone.c: static void write_refspec_config(const char *src_ref_prefix,
}
/* Configure the remote */
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
+ if (option_origin)
+ remote_name = option_origin;

-- strbuf_addf(&resolved_refspec, "refs/heads/test:refs/remotes/%s/test", option_origin);
-+ strbuf_addf(&resolved_refspec, "refs/heads/test:refs/remotes/%s/test", remote_name);
- if (!valid_fetch_refspec(resolved_refspec.buf))
-- /* TRANSLATORS: %s will be the user-provided --origin / -o option */
-- die(_("'%s' is not a valid origin name"), option_origin);
-+ /*
-+ * TRANSLATORS: %s will be the user-provided --origin / -o option, or the value
-+ * of clone.defaultremotename from the config.
-+ */
-+ die(_("'%s' is not a valid origin name"), remote_name);
- strbuf_release(&resolved_refspec);
-
- repo_name = argv[0];
+ if (!valid_remote_name(remote_name))
+ die(_("'%s' is not a valid remote name"), remote_name);
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)

git_config_set("core.bare", "true");
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
- remote = remote_get(option_origin);
+ remote = remote_get(remote_name);

- strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix,
- branch_top.buf);
+ refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix,
+ branch_top.buf);
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)

if (!our_head_points_at)
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)

if (is_local)
clone_local(path, git_dir);
-
- ## t/t5606-clone-options.sh ##
-@@ t/t5606-clone-options.sh: test_expect_success 'disallows --bare with --separate-git-dir' ' -
- '
-
--test_expect_success 'uses "origin" for default remote name' '
--
-- git clone parent clone-default-origin &&
-- (cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)
--
--'
--
- test_expect_success 'prefers --template config over normal config' '
-
- template="$TRASH_DIRECTORY/template-with-config" &&
-@@ t/t5606-clone-options.sh: test_expect_success 'prefers -c config over --template config' ' -
- '
-
-+test_expect_success 'uses "origin" for default remote name' '
-+
-+ git clone parent clone-default-origin &&
-+ (cd clone-default-origin && git rev-parse --verify refs/remotes/origin/master)
-+
-+'
-+
-+test_expect_success 'prefers config "clone.defaultRemoteName" over default' ' -+
-+ test_config_global clone.defaultRemoteName from_config &&
-+ git clone parent clone-config-origin &&
-+ (cd clone-config-origin && git rev-parse --verify refs/remotes/from_config/master)
-+
-+'
-+
-+test_expect_success 'prefers --origin over -c config' '
-+
-+ git clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config &&
-+ (cd clone-o-and-inline-config && git rev-parse --verify refs/remotes/from_option/master)
-+
-+'
-+
- test_expect_success 'redirected clone does not show progress' '
-
- git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&
3: 0dff8cd669 ! 7: 737f91c624 clone: validate --origin option before use
@@ Metadata
Author: Sean Barag <[email protected]>

## Commit message ##
- clone: validate --origin option before use
-
- Providing a bad origin name to git clone currently reports an
- 'invalid refspec' error instead of a more explicit message explaining
- that the --origin option was malformed. Per Junio, it's been this way
- since 8434c2f1 (Build in clone, 2008-04-27). This patch reintroduces
- validation for the provided --origin option, but notably _doesn't_
- include a multi-level check (e.g. "foo/bar") that was present in the
- original git-clone.sh. It seems git remote allows multi-level
- remote names, so applying that same validation in git clone seems
- reasonable.
+ clone: allow configurable default for -o/--origin

+ While the default remote name of "origin" can be changed at clone-time
+ with git clone's --origin option, it was previously not possible
+ to specify a default value for the name of that remote. Add support for
+ a new clone.defaultRemoteName config, with the newly-created remote
+ name resolved in priority order:
+
+ 1. (Highest priority) A remote name passed directly to git clone -o
+ 2. A clone.defaultRemoteName=new_name in config git clone -c
+ 3. A clone.defaultRemoteName value set in /path/to/template/config,
+ where --template=/path/to/template is provided
+ 4. A clone.defaultRemoteName value set in a non-template config file
+ 5. The default value of origin
+
+ Helped-by: Junio C Hamano <[email protected]>
+ Helped-by: Johannes Schindelin <[email protected]>
+ Helped-by: Derrick Stolee <[email protected]>
+ Helped-by: Andrei Rybak <[email protected]>

- Thanks-to: Junio C Hamano <[email protected]>
+
+ ## Documentation/config.txt ##
+@@ Documentation/config.txt: include::config/checkout.txt[]
+
+ include::config/clean.txt[]
+
++include::config/clone.txt[]
++
+ include::config/color.txt[]
+
+ include::config/column.txt[]
+
+ ## Documentation/config/clone.txt (new) ##
+@@
++clone.defaultRemoteName::
++ The name of the remote to create when cloning a repository. Defaults to
++ origin, and can be overridden by passing the --origin command-line
++ option to linkgit:git-clone[1].
++
+
+ ## Documentation/git-clone.txt ##
+@@ Documentation/git-clone.txt: objects from the source repository into a pack in the cloned repository. +
+ -o <name>::
+ --origin <name>::
+- Instead of using the remote name origin to keep track
+- of the upstream repository, use <name>.
++ Instead of using the remote name origin to keep track of the upstream
++ repository, use <name>. Overrides clone.defaultRemoteName from the
++ config.
+
+ -b <name>::
+ --branch <name>::

## builtin/clone.c ##
+@@ builtin/clone.c: static int option_shallow_submodules;
+ static int deepen;
+ static char *option_template, *option_depth, *option_since;
+ static char *option_origin = NULL;
+-static char *remote_name = "origin";
++static char *default_remote_name = "origin";
++static char *remote_name = NULL;
+ static char *option_branch = NULL;
+ static struct string_list option_not = STRING_LIST_INIT_NODUP;
+ static const char *real_git_dir;
+@@ builtin/clone.c: static int checkout(int submodule_progress)
+
+ static int git_clone_config(const char *k, const char *v, void *cb)
+ {
++ if (!strcmp(k, "clone.defaultremotename")) {
++ if (remote_name != default_remote_name)
++ free(remote_name);
++ remote_name = xstrdup(v);
++ }
+ return git_default_config(k, v, cb);
+ }
+
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
- const struct ref *ref;
- struct strbuf key = STRBUF_INIT;
- struct strbuf default_refspec = STRBUF_INIT;
-+ struct strbuf resolved_refspec = STRBUF_INIT;
- struct strbuf branch_top = STRBUF_INIT, reflog_msg = STRBUF_INIT;
- struct transport *transport = NULL;
- const char *src_ref_prefix = "refs/heads/";
+ int submodule_progress;
+
+ struct strvec ref_prefixes = STRVEC_INIT;
++ remote_name = default_remote_name;
+
+ packet_trace_identity("clone");
+
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
- if (!option_origin)
- option_origin = "origin";
+ option_no_checkout = 1;
+ }

-+ strbuf_addf(&resolved_refspec, "refs/heads/test:refs/remotes/%s/test", option_origin);
-+ if (!valid_fetch_refspec(resolved_refspec.buf))
-+ /* TRANSLATORS: %s will be the user-provided --origin / -o option */
-+ die(_("'%s' is not a valid origin name"), option_origin);
-+ strbuf_release(&resolved_refspec);
-+
+- if (option_origin)
+- remote_name = option_origin;
+-
+- if (!valid_remote_name(remote_name))
+- die(_("'%s' is not a valid remote name"), remote_name);
+-
repo_name = argv[0];

path = get_repo_path(repo_name, &is_bundle);
+@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix) +
+ /*
+ * re-read config after init_db and write_config to pick up any config
+- * injected by --template and --config, respectively
++ * injected by --template and --config, respectively.
+ */
+ git_config(git_clone_config, NULL);
+
++ /*
++ * apply the remote name provided by --origin only after this second
++ * call to git_config, to ensure it overrides all config-based values.
++ */
++ if (option_origin)
++ remote_name = option_origin;
++
++ if (!valid_remote_name(remote_name))
++ die(_("'%s' is not a valid remote name"), remote_name);
++
+ if (option_bare) {
+ if (option_mirror)
+ src_ref_prefix = "refs/";

## t/t5606-clone-options.sh ##
@@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '
@@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '

+test_expect_success 'rejects invalid -o/--origin' ' +
-+ test_expect_code 128 git clone -o "bad...name" parent clone-bad-name 2>err &&
-+ test_debug "cat err" &&
-+ test_i18ngrep "'\''bad...name'\'' is not a valid origin name" err
++ test_must_fail git clone -o "bad...name" parent clone-bad-name 2>err &&
++ test_i18ngrep "'\''bad...name'\'' is not a valid remote name" err
+
+'
+
test_expect_success 'disallows --bare with --origin' '

- test_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&
+ test_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&
+@@ t/t5606-clone-options.sh: test_expect_success 'prefers -c config over --template config' ' +
+ '
+
++test_expect_success 'prefers config "clone.defaultRemoteName" over default' ' ++
++ test_config_global clone.defaultRemoteName from_config &&
++ git clone parent clone-config-origin &&
++ git -C clone-config-origin rev-parse --verify refs/remotes/from_config/master
++
++'
++
++test_expect_success 'prefers --origin over -c config' '
++
++ git clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config &&
++ git -C clone-o-and-inline-config rev-parse --verify refs/remotes/from_option/master
++
++'
++
+ test_expect_success 'redirected clone does not show progress' '
+
+ git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&

gitgitgadget

From: Sean Barag <[email protected]>

Some combinations of command-line options to git clone are invalid, but there were previously no tests ensuring those combinations reported errors. Similarly, git clone --template didn't appear to have any tests.

---
 t/t5606-clone-options.sh | 46 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

index e69427f881..0422b24258 100755
@@ -15,7 +15,51 @@ test_expect_success 'setup' '
 test_expect_success 'clone -o' '
 
 	git clone -o foo parent clone-o &&
-	(cd clone-o && git rev-parse --verify refs/remotes/foo/master)
+	git -C clone-o rev-parse --verify refs/remotes/foo/master
+
+'
+
+test_expect_success 'disallows --bare with --origin' '
+
+	test_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&
+	test_debug "cat err" &&
+	test_i18ngrep -e "--bare and --origin foo options are incompatible" err
+
+'
+
+test_expect_success 'disallows --bare with --separate-git-dir' '
+
+	test_must_fail git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err &&
+	test_debug "cat err" &&
+	test_i18ngrep -e "--bare and --separate-git-dir are incompatible" err
+
+'
+
+test_expect_success 'uses "origin" for default remote name' '
+
+	git clone parent clone-default-origin &&
+	git -C clone-default-origin rev-parse --verify refs/remotes/origin/master
+
+'
+
+test_expect_success 'prefers --template config over normal config' '
+
+	template="$TRASH_DIRECTORY/template-with-config" &&
+	mkdir "$template" &&
+	git config --file "$template/config" foo.bar from_template &&
+	test_config_global foo.bar from_global &&
+	git clone "--template=$template" parent clone-template-config &&
+	test "$(git -C clone-template-config config --local foo.bar)" = "from_template"
+
+'
+
+test_expect_success 'prefers -c config over --template config' '
+
+	template="$TRASH_DIRECTORY/template-with-ignored-config" &&
+	mkdir "$template" &&
+	git config --file "$template/config" foo.bar from_template &&
+	git clone "--template=$template" -c foo.bar=inline parent clone-template-inline-config &&
+	test "$(git -C clone-template-inline-config config --local foo.bar)" = "inline"
 
 '
 

gitgitgadget

From: Sean Barag <[email protected]>

Parsing command-line options before reading from config required careful handling to ensure CLI options were treated with higher priority. Read config first to let parsed CLI naively overwrite matching config values.

---
 builtin/clone.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

index fbfd6568cd..93ccd05b5d 100644
@@ -851,8 +851,22 @@ static int checkout(int submodule_progress)
 	return err;
 }
 
+static int git_clone_config(const char *k, const char *v, void *cb)
+{
+	return git_default_config(k, v, cb);
+}
+
 static int write_one_config(const char *key, const char *value, void *data)
 {
+	/*
+	 * give git_clone_config a chance to write config values back to the
+	 * environment, since git_config_set_multivar_gently only deals with
+	 * config-file writes
+	 */
+	int apply_failed = git_clone_config(key, value, data);
+	if (apply_failed)
+		return apply_failed;
+
 	return git_config_set_multivar_gently(key,
 					      value ? value : "true",
 					      CONFIG_REGEX_NONE, 0);

@@ -963,6 +977,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	struct strvec ref_prefixes = STRVEC_INIT;
 
 	packet_trace_identity("clone");
+
+	git_config(git_clone_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
 			     builtin_clone_usage, 0);
 

@@ -1124,9 +1141,17 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (real_git_dir)
 		git_dir = real_git_dir;
 
+	/*
+	 * additional config can be injected with -c, make sure it's included
+	 * after init_db, which clears the entire config environment.
+	 */
 	write_config(&option_config);
 
-	git_config(git_default_config, NULL);
+	/*
+	 * re-read config after init_db and write_config to pick up any config
+	 * injected by --template and --config, respectively
+	 */
+	git_config(git_clone_config, NULL);
 
 	if (option_bare) {
 		if (option_mirror)

gitgitgadget

From: Sean Barag <[email protected]>

In preparation for a future patch that moves builtin/remote.c's remote-name validation, ensure git remote add and git remote rename report errors when the new name isn't valid.

---
 t/t5505-remote.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

index 8d62edd98b..1156f52069 100755
@@ -179,6 +179,13 @@ test_expect_success 'rename errors out early when deleting non-existent branch'
 	)
 '
 
+test_expect_success 'rename errors out early when when new name is invalid' '
+	test_config remote.foo.vcs bar &&
+	echo "fatal: '\''invalid...name'\'' is not a valid remote name" >expect &&
+	test_must_fail git remote rename foo invalid...name 2>actual &&
+	test_i18ncmp expect actual
+'
+
 test_expect_success 'add existing foreign_vcs remote' '
 	test_config remote.foo.vcs bar &&
 	echo "fatal: remote foo already exists." >expect &&

@@ -194,6 +201,12 @@ test_expect_success 'add existing foreign_vcs remote' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'add invalid foreign_vcs remote' '
+	echo "fatal: '\''invalid...name'\'' is not a valid remote name" >expect &&
+	test_must_fail git remote add invalid...name bar 2>actual &&
+	test_i18ncmp expect actual
+'
+
 cat >test/expect <<EOF
 * remote origin
   Fetch URL: $(pwd)/one

gitgitgadget

From: Sean Barag <[email protected]>

In preparation for a future patch, extract from remote.c a function that validates possible remote names so that its rules can be used consistently in other places.

---
 builtin/remote.c |  7 ++-----
 refspec.c        | 10 ++++++++++
 refspec.h        |  1 +
 3 files changed, 13 insertions(+), 5 deletions(-)

index 64b4b551eb..63f2b46c3d 100644
@@ -194,8 +194,7 @@ static int add(int argc, const char **argv)
 	if (remote_is_configured(remote, 1))
 		die(_("remote %s already exists."), name);
 
-	strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
-	if (!valid_fetch_refspec(buf2.buf))
+	if (!valid_remote_name(name))
 		die(_("'%s' is not a valid remote name"), name);
 
 	strbuf_addf(&buf, "remote.%s.url", name);

@@ -696,11 +695,9 @@ static int mv(int argc, const char **argv)
 	if (remote_is_configured(newremote, 1))
 		die(_("remote %s already exists."), rename.new_name);
 
-	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new_name);
-	if (!valid_fetch_refspec(buf.buf))
+	if (!valid_remote_name(rename.new_name))
 		die(_("'%s' is not a valid remote name"), rename.new_name);
 
-	strbuf_reset(&buf);
 	strbuf_addf(&buf, "remote.%s", rename.old_name);
 	strbuf_addf(&buf2, "remote.%s", rename.new_name);
 	if (git_config_rename_section(buf.buf, buf2.buf) < 1)

index 8d0affc34a..3056ffdfb8 100644
@@ -215,6 +215,16 @@ int valid_fetch_refspec(const char *fetch_refspec_str)
 	return ret;
 }
 
+int valid_remote_name(const char *name)
+{
+	int result;
+	struct strbuf refspec = STRBUF_INIT;
+	strbuf_addf(&refspec, "refs/heads/test:refs/remotes/%s/test", name);
+	result = valid_fetch_refspec(refspec.buf);
+	strbuf_release(&refspec);
+	return result;
+}
+
 void refspec_ref_prefixes(const struct refspec *rs,
 			  struct strvec *ref_prefixes)
 {

index 7569248d11..bbb25968a8 100644
@@ -62,6 +62,7 @@ void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
 void refspec_clear(struct refspec *rs);
 
 int valid_fetch_refspec(const char *refspec);
+int valid_remote_name(const char *name);
 
 struct strvec;
 /*

gitgitgadget

From: Sean Barag <[email protected]>

Providing a bad origin name to git clone currently reports an 'invalid refspec' error instead of a more explicit message explaining that the --origin option was malformed. This behavior dates back to since 8434c2f1 (Build in clone, 2008-04-27). Reintroduce validation for the provided --origin option, but notably _don't_ include a multi-level check (e.g. "foo/bar") that was present in the original git-clone.sh. git remote allows multi-level remote names since at least 46220ca100 (remote.c: Fix overtight refspec validation, 2008-03-20), so that appears to be the desired behavior.

---
 builtin/clone.c | 3 +++
 1 file changed, 3 insertions(+)

index 93ccd05b5d..673f7b68c3 100644
@@ -1011,6 +1011,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (!option_origin)
 		option_origin = "origin";
 
+	if (!valid_remote_name(remote_name))
+		die(_("'%s' is not a valid remote name"), remote_name);
+
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);

gitgitgadget

From: Sean Barag <[email protected]>

In a future patch, the name of the remote created by git clone may come from multiple sources. To avoid confusion, convert most uses of option_origin to remote_name, leaving option_origin to exclusively represent the -o/--origin option.

---
 builtin/clone.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

index 673f7b68c3..7fdd00cd95 100644
@@ -53,6 +53,7 @@ static int option_shallow_submodules;
 static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
+static char *remote_name = "origin";
 static char *option_branch = NULL;
 static struct string_list option_not = STRING_LIST_INIT_NODUP;
 static const char *real_git_dir;

@@ -721,7 +722,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		if (!option_bare) {
 			update_ref(msg, "HEAD", &our->old_oid, NULL, 0,
 				   UPDATE_REFS_DIE_ON_ERR);
-			install_branch_config(0, head, option_origin, our->name);
+			install_branch_config(0, head, remote_name, our->name);
 		}
 	} else if (our) {
 		struct commit *c = lookup_commit_reference(the_repository,

@@ -919,12 +920,12 @@ static void write_refspec_config(const char *src_ref_prefix,
 		}
 		/* Configure the remote */
 		if (value.len) {
-			strbuf_addf(&key, "remote.%s.fetch", option_origin);
+			strbuf_addf(&key, "remote.%s.fetch", remote_name);
 			git_config_set_multivar(key.buf, value.buf, "^$", 0);
 			strbuf_reset(&key);
 
 			if (option_mirror) {
-				strbuf_addf(&key, "remote.%s.mirror", option_origin);
+				strbuf_addf(&key, "remote.%s.mirror", remote_name);
 				git_config_set(key.buf, "true");
 				strbuf_reset(&key);
 			}

@@ -1008,8 +1009,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_no_checkout = 1;
 	}
 
-	if (!option_origin)
-		option_origin = "origin";
+	if (option_origin)
+		remote_name = option_origin;
 
 	if (!valid_remote_name(remote_name))
 		die(_("'%s' is not a valid remote name"), remote_name);

@@ -1163,15 +1164,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 		git_config_set("core.bare", "true");
 	} else {
-		strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin);
+		strbuf_addf(&branch_top, "refs/remotes/%s/", remote_name);
 	}
 
-	strbuf_addf(&key, "remote.%s.url", option_origin);
+	strbuf_addf(&key, "remote.%s.url", remote_name);
 	git_config_set(key.buf, repo);
 	strbuf_reset(&key);
 
 	if (option_no_tags) {
-		strbuf_addf(&key, "remote.%s.tagOpt", option_origin);
+		strbuf_addf(&key, "remote.%s.tagOpt", remote_name);
 		git_config_set(key.buf, "--no-tags");
 		strbuf_reset(&key);
 	}

@@ -1182,7 +1183,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_sparse_checkout && git_sparse_checkout_init(dir))
 		return 1;
 
-	remote = remote_get(option_origin);
+	remote = remote_get(remote_name);
 
 	refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix,
 			branch_top.buf);

@@ -1294,7 +1295,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 			if (!our_head_points_at)
 				die(_("Remote branch %s not found in upstream %s"),
-				    option_branch, option_origin);
+				    option_branch, remote_name);
 		}
 		else
 			our_head_points_at = remote_head_points_at;

@@ -1302,7 +1303,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	else {
 		if (option_branch)
 			die(_("Remote branch %s not found in upstream %s"),
-					option_branch, option_origin);
+					option_branch, remote_name);
 
 		warning(_("You appear to have cloned an empty repository."));
 		mapped_refs = NULL;

@@ -1314,7 +1315,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			const char *branch = git_default_branch_name();
 			char *ref = xstrfmt("refs/heads/%s", branch);
 
-			install_branch_config(0, branch, option_origin, ref);
+			install_branch_config(0, branch, remote_name, ref);
 			free(ref);
 		}
 	}

@@ -1323,7 +1324,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			remote_head_points_at, &branch_top);
 
 	if (filter_options.choice)
-		partial_clone_register(option_origin, &filter_options);
+		partial_clone_register(remote_name, &filter_options);
 
 	if (is_local)
 		clone_local(path, git_dir);

gitgitgadget

From: Sean Barag <[email protected]>

While the default remote name of "origin" can be changed at clone-time with git clone's --origin option, it was previously not possible to specify a default value for the name of that remote. Add support for a new clone.defaultRemoteName config, with the newly-created remote name resolved in priority order:

1. (Highest priority) A remote name passed directly to git clone -o
2. A clone.defaultRemoteName=new_name in config git clone -c
3. A clone.defaultRemoteName value set in /path/to/template/config, where --template=/path/to/template is provided
4. A clone.defaultRemoteName value set in a non-template config file
5. The default value of origin

---
 Documentation/config.txt       |  2 ++
 Documentation/config/clone.txt |  5 +++++
 Documentation/git-clone.txt    |  5 +++--
 builtin/clone.c                | 27 +++++++++++++++++++--------
 t/t5606-clone-options.sh       | 22 ++++++++++++++++++++++
 5 files changed, 51 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/config/clone.txt

index f93b6837e4..8c58c53ae7 100644
@@ -334,6 +334,8 @@ include::config/checkout.txt[]
 
 include::config/clean.txt[]
 
+include::config/clone.txt[]
+
 include::config/color.txt[]
 
 include::config/column.txt[]

new file mode 100644
index 0000000000..20755d413a
@@ -0,0 +1,5 @@
+clone.defaultRemoteName::
+	The name of the remote to create when cloning a repository.  Defaults to
+	`origin`, and can be overridden by passing the `--origin` command-line
+	option to linkgit:git-clone[1].
+

index 097e6a86c5..876aedcd47 100644
@@ -183,8 +183,9 @@ objects from the source repository into a pack in the cloned repository.
 
 -o <name>::
 --origin <name>::
-	Instead of using the remote name `origin` to keep track
-	of the upstream repository, use `<name>`.
+	Instead of using the remote name `origin` to keep track of the upstream
+	repository, use `<name>`.  Overrides `clone.defaultRemoteName` from the
+	config.
 
 -b <name>::
 --branch <name>::

index 7fdd00cd95..4c821de242 100644
@@ -53,7 +53,8 @@ static int option_shallow_submodules;
 static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
-static char *remote_name = "origin";
+static char *default_remote_name = "origin";
+static char *remote_name = NULL;
 static char *option_branch = NULL;
 static struct string_list option_not = STRING_LIST_INIT_NODUP;
 static const char *real_git_dir;

@@ -854,6 +855,11 @@ static int checkout(int submodule_progress)
 
 static int git_clone_config(const char *k, const char *v, void *cb)
 {
+	if (!strcmp(k, "clone.defaultremotename")) {
+		if (remote_name != default_remote_name)
+			free(remote_name);
+		remote_name = xstrdup(v);
+	}
 	return git_default_config(k, v, cb);
 }
 

@@ -976,6 +982,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	int submodule_progress;
 
 	struct strvec ref_prefixes = STRVEC_INIT;
+	remote_name = default_remote_name;
 
 	packet_trace_identity("clone");
 

@@ -1009,12 +1016,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_no_checkout = 1;
 	}
 
-	if (option_origin)
-		remote_name = option_origin;
-
-	if (!valid_remote_name(remote_name))
-		die(_("'%s' is not a valid remote name"), remote_name);
-
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);

@@ -1153,10 +1154,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 	/*
 	 * re-read config after init_db and write_config to pick up any config
-	 * injected by --template and --config, respectively
+	 * injected by --template and --config, respectively.
 	 */
 	git_config(git_clone_config, NULL);
 
+	/*
+	 * apply the remote name provided by --origin only after this second
+	 * call to git_config, to ensure it overrides all config-based values.
+	 */
+	if (option_origin)
+		remote_name = option_origin;
+
+	if (!valid_remote_name(remote_name))
+		die(_("'%s' is not a valid remote name"), remote_name);
+
 	if (option_bare) {
 		if (option_mirror)
 			src_ref_prefix = "refs/";

index 0422b24258..5e201e7d85 100755
@@ -19,6 +19,13 @@ test_expect_success 'clone -o' '
 
 '
 
+test_expect_success 'rejects invalid -o/--origin' '
+
+	test_must_fail git clone -o "bad...name" parent clone-bad-name 2>err &&
+	test_i18ngrep "'\''bad...name'\'' is not a valid remote name" err
+
+'
+
 test_expect_success 'disallows --bare with --origin' '
 
 	test_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&

@@ -63,6 +70,21 @@ test_expect_success 'prefers -c config over --template config' '
 
 '
 
+test_expect_success 'prefers config "clone.defaultRemoteName" over default' '
+
+	test_config_global clone.defaultRemoteName from_config &&
+	git clone parent clone-config-origin &&
+	git -C clone-config-origin rev-parse --verify refs/remotes/from_config/master
+
+'
+
+test_expect_success 'prefers --origin over -c config' '
+
+	git clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config &&
+	git -C clone-o-and-inline-config rev-parse --verify refs/remotes/from_option/master
+
+'
+
 test_expect_success 'redirected clone does not show progress' '
 
 	git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&

gitgitgadget

I agree, it felt pretty awkward while writing it! I was hoping to match the start-up sequence you'd mentioned in my original attempt [1] but I'm happy to move the default value assignment down.

How embarrassing, I thought I'd gotten all of those. Will do.

Sean

--
[1] Original attempt at this feature, in separate thread because of the
divergence: https://lore.kernel.org/git/[email protected]/

Self reply, but I wanted to thank you *all* for being so supportive and helpful through my first contribution. This is a great experience :)

Sean

v3 (changes since v2):

* [5/7] fix compilation error: validate option_origin since remote_name doesn't exist yet
* [7/7] remove default_remote_name; apply default value inline if no other value applied

v2 (changes since v1):

* Convert Thanks-to trailer to Helped-by
* Rewrite several commit titles and messages
* Unify error reporting between clone.c and remote.c
* Add tests for git remote add and git remote rename with invalid remote names
* Prevent leak of old remote_name

v1: Took another pass at supporting a configurable default for-o/--origin, this time following Junio's suggestions from a previous approach as much as possible [1]. Unfortunately, Johannes mentioned that --template can write new config values that aren't automatically merged without re-calling git_config. There doesn't appear to be a way around this without rewriting significant amounts of init and config logic across the codebase.

While this could have been v2 of the original patchset, it's diverged so drastically from the original that it likely warrants its own root thread. If that's not appropriate though, I'd be happy to restructure!

Thanks for all the advice Junio and Johannes! This feels significantly better than my first attempt.

[1]
https://lore.kernel.org/git/[email protected]/
[2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195

Sean Barag (7):
clone: add tests for --template and some disallowed option pairs
clone: use more conventional config/option layering
remote: add tests for add and rename with invalid names
refs: consolidate remote name validation
clone: validate --origin option before use
clone: read new remote name from remote_name instead of option_origin
clone: allow configurable default for -o/--origin

 Documentation/config.txt       |  2 +
 Documentation/config/clone.txt |  5 +++
 Documentation/git-clone.txt    |  5 ++-
 builtin/clone.c                | 71 +++++++++++++++++++++++++++-------
 builtin/remote.c               |  7 +---
 refspec.c                      | 10 +++++
 refspec.h                      |  1 +
 t/t5505-remote.sh              | 13 +++++++
 t/t5606-clone-options.sh       | 68 +++++++++++++++++++++++++++++++-
 9 files changed, 159 insertions(+), 23 deletions(-)
 create mode 100644 Documentation/config/clone.txt

base-commit: 306ee63a703ad67c54ba1209dc11dd9ea500dc1f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-727%2Fsjbarag%2Fclone_add-configurable-default-for--o-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-727/sjbarag/clone_add-configurable-default-for--o-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/727

Range-diff vs v2:

 1:  29ab418b1b = 1:  4195dec00c clone: add tests for --template and some disallowed option pairs
 2:  e3479e7b35 ! 2:  1abcf417d9 clone: use more conventional config/option layering
     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
      -	git_config(git_default_config, NULL);
      +	/*
      +	 * re-read config after init_db and write_config to pick up any config
     -+	 * injected by --template and --config, respectively
     ++	 * injected by --template and --config, respectively.
      +	 */
      +	git_config(git_clone_config, NULL);

 3:  85be780b8e = 3:  ec371306ec remote: add tests for add and rename with invalid names
 4:  42dc18601a = 4:  cb686b4129 refs: consolidate remote name validation
 5:  fdc9d3b369 ! 5:  e1d3b54fdc clone: validate --origin option before use
     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       	if (!option_origin)
       		option_origin = "origin";
       
     -+	if (!valid_remote_name(remote_name))
     -+		die(_("'%s' is not a valid remote name"), remote_name);
     ++	if (!valid_remote_name(option_origin))
     ++		die(_("'%s' is not a valid remote name"), option_origin);
      +
       	repo_name = argv[0];

6: 99f4d765b4 ! 6: c3fba50092 clone: read new remote name from remote_name instead of option_origin

     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
      +	if (option_origin)
      +		remote_name = option_origin;
       
     - 	if (!valid_remote_name(remote_name))
     - 		die(_("'%s' is not a valid remote name"), remote_name);
     +-	if (!valid_remote_name(option_origin))
     +-		die(_("'%s' is not a valid remote name"), option_origin);
     ++	if (!valid_remote_name(remote_name))
     ++		die(_("'%s' is not a valid remote name"), remote_name);
     + 
     + 	repo_name = argv[0];
     + 
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       
       		git_config_set("core.bare", "true");
 7:  737f91c624 ! 7:  da8212e500 clone: allow configurable default for `-o`/`--origin`
     @@ builtin/clone.c: static int option_shallow_submodules;
       static char *option_template, *option_depth, *option_since;
       static char *option_origin = NULL;
      -static char *remote_name = "origin";
     -+static char *default_remote_name = "origin";
      +static char *remote_name = NULL;
       static char *option_branch = NULL;
       static struct string_list option_not = STRING_LIST_INIT_NODUP;
     @@ builtin/clone.c: static int checkout(int submodule_progress)
       static int git_clone_config(const char *k, const char *v, void *cb)
       {
      +	if (!strcmp(k, "clone.defaultremotename")) {
     -+		if (remote_name != default_remote_name)
     -+			free(remote_name);
     ++		free(remote_name);
      +		remote_name = xstrdup(v);
      +	}
       	return git_default_config(k, v, cb);
       }
       
     -@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     - 	int submodule_progress;
     - 
     - 	struct strvec ref_prefixes = STRVEC_INIT;
     -+	remote_name = default_remote_name;
     - 
     - 	packet_trace_identity("clone");
     - 
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       		option_no_checkout = 1;
       	}
     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       
       	path = get_repo_path(repo_name, &is_bundle);
      @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     - 
     - 	/*
     - 	 * re-read config after init_db and write_config to pick up any config
     --	 * injected by --template and --config, respectively
     -+	 * injected by --template and --config, respectively.
       	 */
       	git_config(git_clone_config, NULL);
       
     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
      +	 * apply the remote name provided by --origin only after this second
      +	 * call to git_config, to ensure it overrides all config-based values.
      +	 */
     -+	if (option_origin)
     -+		remote_name = option_origin;
     ++	if (option_origin != NULL)
     ++		remote_name = xstrdup(option_origin);
     ++
     ++	if (remote_name == NULL)
     ++		remote_name = xstrdup("origin");
      +
      +	if (!valid_remote_name(remote_name))
      +		die(_("'%s' is not a valid remote name"), remote_name);
     @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
       	if (option_bare) {
       		if (option_mirror)
       			src_ref_prefix = "refs/";
     +@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     + 	junk_mode = JUNK_LEAVE_REPO;
     + 	err = checkout(submodule_progress);
     + 
     ++	free(remote_name);
     + 	strbuf_release(&reflog_msg);
     + 	strbuf_release(&branch_top);
     + 	strbuf_release(&key);
      
       ## t/t5606-clone-options.sh ##
      @@ t/t5606-clone-options.sh: test_expect_success 'clone -o' '

gitgitgadget

From: Sean Barag <[email protected]>

Some combinations of command-line options to git clone are invalid, but there were previously no tests ensuring those combinations reported errors. Similarly, git clone --template didn't appear to have any tests.

---
 t/t5606-clone-options.sh | 46 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

index e69427f881..0422b24258 100755
@@ -15,7 +15,51 @@ test_expect_success 'setup' '
 test_expect_success 'clone -o' '
 
 	git clone -o foo parent clone-o &&
-	(cd clone-o && git rev-parse --verify refs/remotes/foo/master)
+	git -C clone-o rev-parse --verify refs/remotes/foo/master
+
+'
+
+test_expect_success 'disallows --bare with --origin' '
+
+	test_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&
+	test_debug "cat err" &&
+	test_i18ngrep -e "--bare and --origin foo options are incompatible" err
+
+'
+
+test_expect_success 'disallows --bare with --separate-git-dir' '
+
+	test_must_fail git clone --bare --separate-git-dir dot-git-destiation parent clone-bare-sgd 2>err &&
+	test_debug "cat err" &&
+	test_i18ngrep -e "--bare and --separate-git-dir are incompatible" err
+
+'
+
+test_expect_success 'uses "origin" for default remote name' '
+
+	git clone parent clone-default-origin &&
+	git -C clone-default-origin rev-parse --verify refs/remotes/origin/master
+
+'
+
+test_expect_success 'prefers --template config over normal config' '
+
+	template="$TRASH_DIRECTORY/template-with-config" &&
+	mkdir "$template" &&
+	git config --file "$template/config" foo.bar from_template &&
+	test_config_global foo.bar from_global &&
+	git clone "--template=$template" parent clone-template-config &&
+	test "$(git -C clone-template-config config --local foo.bar)" = "from_template"
+
+'
+
+test_expect_success 'prefers -c config over --template config' '
+
+	template="$TRASH_DIRECTORY/template-with-ignored-config" &&
+	mkdir "$template" &&
+	git config --file "$template/config" foo.bar from_template &&
+	git clone "--template=$template" -c foo.bar=inline parent clone-template-inline-config &&
+	test "$(git -C clone-template-inline-config config --local foo.bar)" = "inline"
 
 '
 

gitgitgadget

From: Sean Barag <[email protected]>

Parsing command-line options before reading from config required careful handling to ensure CLI options were treated with higher priority. Read config first to let parsed CLI naively overwrite matching config values.

---
 builtin/clone.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

index 391aa41075..a76dacd988 100644
@@ -851,8 +851,22 @@ static int checkout(int submodule_progress)
 	return err;
 }
 
+static int git_clone_config(const char *k, const char *v, void *cb)
+{
+	return git_default_config(k, v, cb);
+}
+
 static int write_one_config(const char *key, const char *value, void *data)
 {
+	/*
+	 * give git_clone_config a chance to write config values back to the
+	 * environment, since git_config_set_multivar_gently only deals with
+	 * config-file writes
+	 */
+	int apply_failed = git_clone_config(key, value, data);
+	if (apply_failed)
+		return apply_failed;
+
 	return git_config_set_multivar_gently(key,
 					      value ? value : "true",
 					      CONFIG_REGEX_NONE, 0);

@@ -963,6 +977,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	struct strvec ref_prefixes = STRVEC_INIT;
 
 	packet_trace_identity("clone");
+
+	git_config(git_clone_config, NULL);
+
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
 			     builtin_clone_usage, 0);
 

@@ -1124,9 +1141,17 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (real_git_dir)
 		git_dir = real_git_dir;
 
+	/*
+	 * additional config can be injected with -c, make sure it's included
+	 * after init_db, which clears the entire config environment.
+	 */
 	write_config(&option_config);
 
-	git_config(git_default_config, NULL);
+	/*
+	 * re-read config after init_db and write_config to pick up any config
+	 * injected by --template and --config, respectively.
+	 */
+	git_config(git_clone_config, NULL);
 
 	if (option_bare) {
 		if (option_mirror)

gitgitgadget

From: Sean Barag <[email protected]>

In preparation for a future patch that moves builtin/remote.c's remote-name validation, ensure git remote add and git remote rename report errors when the new name isn't valid.

---
 t/t5505-remote.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

index 8d62edd98b..1156f52069 100755
@@ -179,6 +179,13 @@ test_expect_success 'rename errors out early when deleting non-existent branch'
 	)
 '
 
+test_expect_success 'rename errors out early when when new name is invalid' '
+	test_config remote.foo.vcs bar &&
+	echo "fatal: '\''invalid...name'\'' is not a valid remote name" >expect &&
+	test_must_fail git remote rename foo invalid...name 2>actual &&
+	test_i18ncmp expect actual
+'
+
 test_expect_success 'add existing foreign_vcs remote' '
 	test_config remote.foo.vcs bar &&
 	echo "fatal: remote foo already exists." >expect &&

@@ -194,6 +201,12 @@ test_expect_success 'add existing foreign_vcs remote' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'add invalid foreign_vcs remote' '
+	echo "fatal: '\''invalid...name'\'' is not a valid remote name" >expect &&
+	test_must_fail git remote add invalid...name bar 2>actual &&
+	test_i18ncmp expect actual
+'
+
 cat >test/expect <<EOF
 * remote origin
   Fetch URL: $(pwd)/one

gitgitgadget

From: Sean Barag <[email protected]>

In preparation for a future patch, extract from remote.c a function that validates possible remote names so that its rules can be used consistently in other places.

---
 builtin/remote.c |  7 ++-----
 refspec.c        | 10 ++++++++++
 refspec.h        |  1 +
 3 files changed, 13 insertions(+), 5 deletions(-)

index 64b4b551eb..63f2b46c3d 100644
@@ -194,8 +194,7 @@ static int add(int argc, const char **argv)
 	if (remote_is_configured(remote, 1))
 		die(_("remote %s already exists."), name);
 
-	strbuf_addf(&buf2, "refs/heads/test:refs/remotes/%s/test", name);
-	if (!valid_fetch_refspec(buf2.buf))
+	if (!valid_remote_name(name))
 		die(_("'%s' is not a valid remote name"), name);
 
 	strbuf_addf(&buf, "remote.%s.url", name);

@@ -696,11 +695,9 @@ static int mv(int argc, const char **argv)
 	if (remote_is_configured(newremote, 1))
 		die(_("remote %s already exists."), rename.new_name);
 
-	strbuf_addf(&buf, "refs/heads/test:refs/remotes/%s/test", rename.new_name);
-	if (!valid_fetch_refspec(buf.buf))
+	if (!valid_remote_name(rename.new_name))
 		die(_("'%s' is not a valid remote name"), rename.new_name);
 
-	strbuf_reset(&buf);
 	strbuf_addf(&buf, "remote.%s", rename.old_name);
 	strbuf_addf(&buf2, "remote.%s", rename.new_name);
 	if (git_config_rename_section(buf.buf, buf2.buf) < 1)

index 8d0affc34a..3056ffdfb8 100644
@@ -215,6 +215,16 @@ int valid_fetch_refspec(const char *fetch_refspec_str)
 	return ret;
 }
 
+int valid_remote_name(const char *name)
+{
+	int result;
+	struct strbuf refspec = STRBUF_INIT;
+	strbuf_addf(&refspec, "refs/heads/test:refs/remotes/%s/test", name);
+	result = valid_fetch_refspec(refspec.buf);
+	strbuf_release(&refspec);
+	return result;
+}
+
 void refspec_ref_prefixes(const struct refspec *rs,
 			  struct strvec *ref_prefixes)
 {

index 7569248d11..bbb25968a8 100644
@@ -62,6 +62,7 @@ void refspec_appendn(struct refspec *rs, const char **refspecs, int nr);
 void refspec_clear(struct refspec *rs);
 
 int valid_fetch_refspec(const char *refspec);
+int valid_remote_name(const char *name);
 
 struct strvec;
 /*

gitgitgadget

From: Sean Barag <[email protected]>

Providing a bad origin name to git clone currently reports an 'invalid refspec' error instead of a more explicit message explaining that the --origin option was malformed. This behavior dates back to since 8434c2f1 (Build in clone, 2008-04-27). Reintroduce validation for the provided --origin option, but notably _don't_ include a multi-level check (e.g. "foo/bar") that was present in the original git-clone.sh. git remote allows multi-level remote names since at least 46220ca100 (remote.c: Fix overtight refspec validation, 2008-03-20), so that appears to be the desired behavior.

---
 builtin/clone.c | 3 +++
 1 file changed, 3 insertions(+)

index a76dacd988..bd67ff7b01 100644
@@ -1011,6 +1011,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (!option_origin)
 		option_origin = "origin";
 
+	if (!valid_remote_name(option_origin))
+		die(_("'%s' is not a valid remote name"), option_origin);
+
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);

gitgitgadget

From: Sean Barag <[email protected]>

In a future patch, the name of the remote created by git clone may come from multiple sources. To avoid confusion, convert most uses of option_origin to remote_name, leaving option_origin to exclusively represent the -o/--origin option.

---
 builtin/clone.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

index bd67ff7b01..6bcdfa4cd6 100644
@@ -53,6 +53,7 @@ static int option_shallow_submodules;
 static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
+static char *remote_name = "origin";
 static char *option_branch = NULL;
 static struct string_list option_not = STRING_LIST_INIT_NODUP;
 static const char *real_git_dir;

@@ -721,7 +722,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
 		if (!option_bare) {
 			update_ref(msg, "HEAD", &our->old_oid, NULL, 0,
 				   UPDATE_REFS_DIE_ON_ERR);
-			install_branch_config(0, head, option_origin, our->name);
+			install_branch_config(0, head, remote_name, our->name);
 		}
 	} else if (our) {
 		struct commit *c = lookup_commit_reference(the_repository,

@@ -919,12 +920,12 @@ static void write_refspec_config(const char *src_ref_prefix,
 		}
 		/* Configure the remote */
 		if (value.len) {
-			strbuf_addf(&key, "remote.%s.fetch", option_origin);
+			strbuf_addf(&key, "remote.%s.fetch", remote_name);
 			git_config_set_multivar(key.buf, value.buf, "^$", 0);
 			strbuf_reset(&key);
 
 			if (option_mirror) {
-				strbuf_addf(&key, "remote.%s.mirror", option_origin);
+				strbuf_addf(&key, "remote.%s.mirror", remote_name);
 				git_config_set(key.buf, "true");
 				strbuf_reset(&key);
 			}

@@ -1008,11 +1009,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_no_checkout = 1;
 	}
 
-	if (!option_origin)
-		option_origin = "origin";
+	if (option_origin)
+		remote_name = option_origin;
 
-	if (!valid_remote_name(option_origin))
-		die(_("'%s' is not a valid remote name"), option_origin);
+	if (!valid_remote_name(remote_name))
+		die(_("'%s' is not a valid remote name"), remote_name);
 
 	repo_name = argv[0];
 

@@ -1163,15 +1164,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 		git_config_set("core.bare", "true");
 	} else {
-		strbuf_addf(&branch_top, "refs/remotes/%s/", option_origin);
+		strbuf_addf(&branch_top, "refs/remotes/%s/", remote_name);
 	}
 
-	strbuf_addf(&key, "remote.%s.url", option_origin);
+	strbuf_addf(&key, "remote.%s.url", remote_name);
 	git_config_set(key.buf, repo);
 	strbuf_reset(&key);
 
 	if (option_no_tags) {
-		strbuf_addf(&key, "remote.%s.tagOpt", option_origin);
+		strbuf_addf(&key, "remote.%s.tagOpt", remote_name);
 		git_config_set(key.buf, "--no-tags");
 		strbuf_reset(&key);
 	}

@@ -1182,7 +1183,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	if (option_sparse_checkout && git_sparse_checkout_init(dir))
 		return 1;
 
-	remote = remote_get(option_origin);
+	remote = remote_get(remote_name);
 
 	refspec_appendf(&remote->fetch, "+%s*:%s*", src_ref_prefix,
 			branch_top.buf);

@@ -1294,7 +1295,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 
 			if (!our_head_points_at)
 				die(_("Remote branch %s not found in upstream %s"),
-				    option_branch, option_origin);
+				    option_branch, remote_name);
 		}
 		else
 			our_head_points_at = remote_head_points_at;

@@ -1302,7 +1303,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	else {
 		if (option_branch)
 			die(_("Remote branch %s not found in upstream %s"),
-					option_branch, option_origin);
+					option_branch, remote_name);
 
 		warning(_("You appear to have cloned an empty repository."));
 		mapped_refs = NULL;

@@ -1314,7 +1315,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			const char *branch = git_default_branch_name();
 			char *ref = xstrfmt("refs/heads/%s", branch);
 
-			install_branch_config(0, branch, option_origin, ref);
+			install_branch_config(0, branch, remote_name, ref);
 			free(ref);
 		}
 	}

@@ -1323,7 +1324,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 			remote_head_points_at, &branch_top);
 
 	if (filter_options.choice)
-		partial_clone_register(option_origin, &filter_options);
+		partial_clone_register(remote_name, &filter_options);
 
 	if (is_local)
 		clone_local(path, git_dir);

gitgitgadget

From: Sean Barag <[email protected]>

While the default remote name of "origin" can be changed at clone-time with git clone's --origin option, it was previously not possible to specify a default value for the name of that remote. Add support for a new clone.defaultRemoteName config, with the newly-created remote name resolved in priority order:

1. (Highest priority) A remote name passed directly to git clone -o
2. A clone.defaultRemoteName=new_name in config git clone -c
3. A clone.defaultRemoteName value set in /path/to/template/config, where --template=/path/to/template is provided
4. A clone.defaultRemoteName value set in a non-template config file
5. The default value of origin

---
 Documentation/config.txt       |  2 ++
 Documentation/config/clone.txt |  5 +++++
 Documentation/git-clone.txt    |  5 +++--
 builtin/clone.c                | 26 +++++++++++++++++++-------
 t/t5606-clone-options.sh       | 22 ++++++++++++++++++++++
 5 files changed, 51 insertions(+), 9 deletions(-)
 create mode 100644 Documentation/config/clone.txt

index bf706b950e..025ca4df11 100644
@@ -334,6 +334,8 @@ include::config/checkout.txt[]
 
 include::config/clean.txt[]
 
+include::config/clone.txt[]
+
 include::config/color.txt[]
 
 include::config/column.txt[]

new file mode 100644
index 0000000000..20755d413a
@@ -0,0 +1,5 @@
+clone.defaultRemoteName::
+	The name of the remote to create when cloning a repository.  Defaults to
+	`origin`, and can be overridden by passing the `--origin` command-line
+	option to linkgit:git-clone[1].
+

index 097e6a86c5..876aedcd47 100644
@@ -183,8 +183,9 @@ objects from the source repository into a pack in the cloned repository.
 
 -o <name>::
 --origin <name>::
-	Instead of using the remote name `origin` to keep track
-	of the upstream repository, use `<name>`.
+	Instead of using the remote name `origin` to keep track of the upstream
+	repository, use `<name>`.  Overrides `clone.defaultRemoteName` from the
+	config.
 
 -b <name>::
 --branch <name>::

index 6bcdfa4cd6..a0841923cf 100644
@@ -53,7 +53,7 @@ static int option_shallow_submodules;
 static int deepen;
 static char *option_template, *option_depth, *option_since;
 static char *option_origin = NULL;
-static char *remote_name = "origin";
+static char *remote_name = NULL;
 static char *option_branch = NULL;
 static struct string_list option_not = STRING_LIST_INIT_NODUP;
 static const char *real_git_dir;

@@ -854,6 +854,10 @@ static int checkout(int submodule_progress)
 
 static int git_clone_config(const char *k, const char *v, void *cb)
 {
+	if (!strcmp(k, "clone.defaultremotename")) {
+		free(remote_name);
+		remote_name = xstrdup(v);
+	}
 	return git_default_config(k, v, cb);
 }
 

@@ -1009,12 +1013,6 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		option_no_checkout = 1;
 	}
 
-	if (option_origin)
-		remote_name = option_origin;
-
-	if (!valid_remote_name(remote_name))
-		die(_("'%s' is not a valid remote name"), remote_name);
-
 	repo_name = argv[0];
 
 	path = get_repo_path(repo_name, &is_bundle);

@@ -1157,6 +1155,19 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	 */
 	git_config(git_clone_config, NULL);
 
+	/*
+	 * apply the remote name provided by --origin only after this second
+	 * call to git_config, to ensure it overrides all config-based values.
+	 */
+	if (option_origin != NULL)
+		remote_name = xstrdup(option_origin);
+
+	if (remote_name == NULL)
+		remote_name = xstrdup("origin");
+
+	if (!valid_remote_name(remote_name))
+		die(_("'%s' is not a valid remote name"), remote_name);
+
 	if (option_bare) {
 		if (option_mirror)
 			src_ref_prefix = "refs/";

@@ -1356,6 +1367,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	junk_mode = JUNK_LEAVE_REPO;
 	err = checkout(submodule_progress);
 
+	free(remote_name);
 	strbuf_release(&reflog_msg);
 	strbuf_release(&branch_top);
 	strbuf_release(&key);

index 0422b24258..5e201e7d85 100755
@@ -19,6 +19,13 @@ test_expect_success 'clone -o' '
 
 '
 
+test_expect_success 'rejects invalid -o/--origin' '
+
+	test_must_fail git clone -o "bad...name" parent clone-bad-name 2>err &&
+	test_i18ngrep "'\''bad...name'\'' is not a valid remote name" err
+
+'
+
 test_expect_success 'disallows --bare with --origin' '
 
 	test_must_fail git clone -o foo --bare parent clone-bare-o 2>err &&

@@ -63,6 +70,21 @@ test_expect_success 'prefers -c config over --template config' '
 
 '
 
+test_expect_success 'prefers config "clone.defaultRemoteName" over default' '
+
+	test_config_global clone.defaultRemoteName from_config &&
+	git clone parent clone-config-origin &&
+	git -C clone-config-origin rev-parse --verify refs/remotes/from_config/master
+
+'
+
+test_expect_success 'prefers --origin over -c config' '
+
+	git clone -c clone.defaultRemoteName=inline --origin from_option parent clone-o-and-inline-config &&
+	git -C clone-o-and-inline-config rev-parse --verify refs/remotes/from_option/master
+
+'
+
 test_expect_success 'redirected clone does not show progress' '
 
 	git clone "file://$(pwd)/parent" clone-redirected >out 2>err &&

gitgitgadget

Sorry for being late to these versions, but I think the changes across these two versions are excellent improvements. I'm happy with this patch series!

Thanks,
-Stolee