git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH 0/4] clone: allow configurable default for -o/--origin
@ 2020-09-11 18:25 Sean Barag via GitGitGadget
  2020-09-11 18:25 ` [PATCH 1/4] clone: add tests for --template and some disallowed option pairs Sean Barag via GitGitGadget
                   ` (5 more replies)
  0 siblings, 6 replies; 26+ messages in thread
From: Sean Barag via GitGitGadget @ 2020-09-11 18:25 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin, Sean Barag

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/pull.710.git.1598456751674.gitgitgadget@gmail.com/
[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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] clone: add tests for --template and some disallowed option pairs
  2020-09-11 18:25 [PATCH 0/4] clone: allow configurable default for -o/--origin Sean Barag via GitGitGadget
@ 2020-09-11 18:25 ` Sean Barag via GitGitGadget
  2020-09-11 18:57   ` Derrick Stolee
  2020-09-11 18:25 ` [PATCH 2/4] clone: call git_config before parse_options Sean Barag via GitGitGadget
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 26+ messages in thread
From: Sean Barag via GitGitGadget @ 2020-09-11 18:25 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin, Sean Barag, Sean Barag

From: Sean Barag <sean@barag.org>

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.

Signed-off-by: Sean Barag <sean@barag.org>
---
 t/t5606-clone-options.sh | 44 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index e69427f881..d20a78f84b 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 2/4] clone: call git_config before parse_options
  2020-09-11 18:25 [PATCH 0/4] clone: allow configurable default for -o/--origin Sean Barag via GitGitGadget
  2020-09-11 18:25 ` [PATCH 1/4] clone: add tests for --template and some disallowed option pairs Sean Barag via GitGitGadget
@ 2020-09-11 18:25 ` Sean Barag via GitGitGadget
  2020-09-11 18:59   ` Derrick Stolee
  2020-09-11 20:26   ` Junio C Hamano
  2020-09-11 18:25 ` [PATCH 3/4] clone: validate --origin option before use Sean Barag via GitGitGadget
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 26+ messages in thread
From: Sean Barag via GitGitGadget @ 2020-09-11 18:25 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin, Sean Barag, Sean Barag

From: Sean Barag <sean@barag.org>

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/pull.710.git.1598456751674.gitgitgadget@gmail.com/
[2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195

Signed-off-by: Sean Barag <sean@barag.org>
Thanks-to: Junio C Hamano <gitster@pobox.com>
Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/clone.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index b087ee40c2..bf095815f0 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 3/4] clone: validate --origin option before use
  2020-09-11 18:25 [PATCH 0/4] clone: allow configurable default for -o/--origin Sean Barag via GitGitGadget
  2020-09-11 18:25 ` [PATCH 1/4] clone: add tests for --template and some disallowed option pairs Sean Barag via GitGitGadget
  2020-09-11 18:25 ` [PATCH 2/4] clone: call git_config before parse_options Sean Barag via GitGitGadget
@ 2020-09-11 18:25 ` Sean Barag via GitGitGadget
  2020-09-11 19:24   ` Derrick Stolee
  2020-09-11 20:39   ` Junio C Hamano
  2020-09-11 18:25 ` [PATCH 4/4] clone: allow configurable default for `-o`/`--origin` Sean Barag via GitGitGadget
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 26+ messages in thread
From: Sean Barag via GitGitGadget @ 2020-09-11 18:25 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin, Sean Barag, Sean Barag

From: Sean Barag <sean@barag.org>

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.

Signed-off-by: Sean Barag <sean@barag.org>
Thanks-to: Junio C Hamano <gitster@pobox.com>
---
 builtin/clone.c          | 7 +++++++
 t/t5606-clone-options.sh | 8 ++++++++
 2 files changed, 15 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index bf095815f0..1cd62d0001 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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);
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index d20a78f84b..c865f96def 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -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


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 4/4] clone: allow configurable default for `-o`/`--origin`
  2020-09-11 18:25 [PATCH 0/4] clone: allow configurable default for -o/--origin Sean Barag via GitGitGadget
                   ` (2 preceding siblings ...)
  2020-09-11 18:25 ` [PATCH 3/4] clone: validate --origin option before use Sean Barag via GitGitGadget
@ 2020-09-11 18:25 ` Sean Barag via GitGitGadget
  2020-09-11 19:13   ` Derrick Stolee
                     ` (2 more replies)
  2020-09-11 19:25 ` [PATCH 0/4] clone: allow configurable default for -o/--origin Derrick Stolee
  2020-09-11 19:34 ` Junio C Hamano
  5 siblings, 3 replies; 26+ messages in thread
From: Sean Barag via GitGitGadget @ 2020-09-11 18:25 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Johannes Schindelin, Sean Barag, Sean Barag

From: Sean Barag <sean@barag.org>

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`

Signed-off-by: Sean Barag <sean@barag.org>
Thanks-to: Junio C Hamano <gitster@pobox.com>
Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3042d80978..354874facf 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -334,6 +334,8 @@ include::config/checkout.txt[]
 
 include::config/clean.txt[]
 
+include::config/clone.txt[]
+
 include::config/color.txt[]
 
 include::config/column.txt[]
diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
new file mode 100644
index 0000000000..20755d413a
--- /dev/null
+++ b/Documentation/config/clone.txt
@@ -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].
+
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index c898310099..f04bf6e6ba 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -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>::
diff --git a/builtin/clone.c b/builtin/clone.c
index 1cd62d0001..aeb41f15f3 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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,
@@ -853,16 +854,18 @@ 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;
 
@@ -918,12 +921,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);
 			}
@@ -1009,13 +1012,16 @@ 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;
 
-	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];
@@ -1167,15 +1173,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);
 	}
@@ -1186,7 +1192,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);
 
 	strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix,
 		    branch_top.buf);
@@ -1299,7 +1305,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;
@@ -1307,7 +1313,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;
@@ -1319,7 +1325,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);
 		}
 	}
@@ -1328,7 +1334,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);
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index c865f96def..017c24a454 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs
  2020-09-11 18:25 ` [PATCH 1/4] clone: add tests for --template and some disallowed option pairs Sean Barag via GitGitGadget
@ 2020-09-11 18:57   ` Derrick Stolee
  2020-09-11 19:56     ` Jeff King
  2020-09-11 21:02     ` Junio C Hamano
  0 siblings, 2 replies; 26+ messages in thread
From: Derrick Stolee @ 2020-09-11 18:57 UTC (permalink / raw)
  To: Sean Barag via GitGitGadget, git
  Cc: Junio C Hamano, Johannes Schindelin, Sean Barag

On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:
> From: Sean Barag <sean@barag.org>
> 
> 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.
> 
> Signed-off-by: Sean Barag <sean@barag.org>
> ---
>  t/t5606-clone-options.sh | 44 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index e69427f881..d20a78f84b 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -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
> +
> +'

It seems that all of your tests have an extraneous newline
at the end.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/4] clone: call git_config before parse_options
  2020-09-11 18:25 ` [PATCH 2/4] clone: call git_config before parse_options Sean Barag via GitGitGadget
@ 2020-09-11 18:59   ` Derrick Stolee
  2020-09-11 20:26   ` Junio C Hamano
  1 sibling, 0 replies; 26+ messages in thread
From: Derrick Stolee @ 2020-09-11 18:59 UTC (permalink / raw)
  To: Sean Barag via GitGitGadget, git
  Cc: Junio C Hamano, Johannes Schindelin, Sean Barag

On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:
> From: Sean Barag <sean@barag.org>
> 
> 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/pull.710.git.1598456751674.gitgitgadget@gmail.com/
> [2] https://github.com/gitgitgadget/git/pull/727#issuecomment-689740195
> 
> Signed-off-by: Sean Barag <sean@barag.org>
> Thanks-to: Junio C Hamano <gitster@pobox.com>
> Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/clone.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/clone.c b/builtin/clone.c
> index b087ee40c2..bf095815f0 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -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);
> +}
> +

Introducing this no-op seems a bit premature, but as long
as it makes your future patches cleaner, then it is
appropriate.

>  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);

However, you change this to git_clone_config() in patch 4. Perhaps
use git_clone_config() here instead?

Thanks,
-Stolee


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] clone: allow configurable default for `-o`/`--origin`
  2020-09-11 18:25 ` [PATCH 4/4] clone: allow configurable default for `-o`/`--origin` Sean Barag via GitGitGadget
@ 2020-09-11 19:13   ` Derrick Stolee
  2020-09-11 21:00   ` Junio C Hamano
  2020-09-17 15:25   ` Andrei Rybak
  2 siblings, 0 replies; 26+ messages in thread
From: Derrick Stolee @ 2020-09-11 19:13 UTC (permalink / raw)
  To: Sean Barag via GitGitGadget, git
  Cc: Junio C Hamano, Johannes Schindelin, Sean Barag

On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:

>  static char *option_origin = NULL;
> +static char *remote_name = "origin";

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:
  
> -	if (!option_origin)
> -		option_origin = "origin";
> +	if (option_origin)
> +		remote_name = option_origin;
> +	else
> +		remote_name = "origin";
  
Then this patch introducing the config option would have a
very limited change in the builtin consisting of these two
hunks:

>  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);
>  }
...
>  	if (option_origin)
>  		remote_name = option_origin;
> -	else
> -		remote_name = "origin"

along with this translators comment note:

>  	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);


> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -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)
> -
> -'
> -

Interesting that you moved this test. Probably not necessary, and
just a mistake.

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

I didn't notice it earlier, but perhaps this subshell should be split
into its own multi-line section as follows:

> +	(
> +		cd clone-default-origin &&
> +		git rev-parse --verify refs/remotes/origin/master
> +	)

But even better, this is only one line so using
"git -C clone-default-origin rev-parse" is simpler:


> +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 config "clone.defaultRemoteName" over default' '
> +
> +	test_config_global clone.defaultRemoteName from_config &&
> +	git clone parent clone-config-origin &&

This could be done using "git -c clone.defaultRemoteName=from_config" instead
of setting the global config.

> +	(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 &&

And you use the -c option here.

> +	(cd clone-o-and-inline-config && git rev-parse --verify refs/remotes/from_option/master)
> +
> +'
> +

We have the extra newline in these tests, too.

Thanks,
-Stolee


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] clone: validate --origin option before use
  2020-09-11 18:25 ` [PATCH 3/4] clone: validate --origin option before use Sean Barag via GitGitGadget
@ 2020-09-11 19:24   ` Derrick Stolee
  2020-09-16 16:28     ` Sean Barag
  2020-09-11 20:39   ` Junio C Hamano
  1 sibling, 1 reply; 26+ messages in thread
From: Derrick Stolee @ 2020-09-11 19:24 UTC (permalink / raw)
  To: Sean Barag via GitGitGadget, git
  Cc: Junio C Hamano, Johannes Schindelin, Sean Barag

On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:
> +	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);

Looking at this again, I'm not sure the translators note is
necessary. Also, I would say "is not a valid remote name".
That makes the string align with the already-translated string
in builtin/remote.c.

This code is duplicated from builtin/remote.c, so I'd rather
see this be a helper method in refspec.c and have both
builtin/clone.c and builtin/remote.c call that helper.

Here is the helper:

void 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;
}

And here is the use in builtin/clone.c:

	if (!valid_remote_name(option_origin))
		die(_("'%s' is not a valid remote name"), option_origin);

and in builtin/remote.c:

	if (!valid_remote_name(name))
		die(_("'%s' is not a valid remote name"), name);

> +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
> +
> +'
> +

Double newlines here! I personally appreciate newlines to
spread out content, but it doesn't fit our guidelines.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 0/4] clone: allow configurable default for -o/--origin
  2020-09-11 18:25 [PATCH 0/4] clone: allow configurable default for -o/--origin Sean Barag via GitGitGadget
                   ` (3 preceding siblings ...)
  2020-09-11 18:25 ` [PATCH 4/4] clone: allow configurable default for `-o`/`--origin` Sean Barag via GitGitGadget
@ 2020-09-11 19:25 ` Derrick Stolee
  2020-09-11 19:34 ` Junio C Hamano
  5 siblings, 0 replies; 26+ messages in thread
From: Derrick Stolee @ 2020-09-11 19:25 UTC (permalink / raw)
  To: Sean Barag via GitGitGadget, git
  Cc: Junio C Hamano, Johannes Schindelin, Sean Barag

On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:
> 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.

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.

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

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

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 0/4] clone: allow configurable default for -o/--origin
  2020-09-11 18:25 [PATCH 0/4] clone: allow configurable default for -o/--origin Sean Barag via GitGitGadget
                   ` (4 preceding siblings ...)
  2020-09-11 19:25 ` [PATCH 0/4] clone: allow configurable default for -o/--origin Derrick Stolee
@ 2020-09-11 19:34 ` Junio C Hamano
  5 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2020-09-11 19:34 UTC (permalink / raw)
  To: Sean Barag via GitGitGadget; +Cc: git, Johannes Schindelin, Sean Barag

"Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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!

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs
  2020-09-11 18:57   ` Derrick Stolee
@ 2020-09-11 19:56     ` Jeff King
  2020-09-11 20:07       ` Eric Sunshine
                         ` (2 more replies)
  2020-09-11 21:02     ` Junio C Hamano
  1 sibling, 3 replies; 26+ messages in thread
From: Jeff King @ 2020-09-11 19:56 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: Sean Barag via GitGitGadget, git, Junio C Hamano,
	Johannes Schindelin, Sean Barag

On Fri, Sep 11, 2020 at 02:57:11PM -0400, Derrick Stolee wrote:

> > diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> > index e69427f881..d20a78f84b 100755
> > --- a/t/t5606-clone-options.sh
> > +++ b/t/t5606-clone-options.sh
> > @@ -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
> > +
> > +'
> 
> It seems that all of your tests have an extraneous newline
> at the end.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs
  2020-09-11 19:56     ` Jeff King
@ 2020-09-11 20:07       ` Eric Sunshine
  2020-09-16  3:15         ` [PATCH 0/4] clone: allow configurable default for -o/--origin Sean Barag
  2020-09-12  3:17       ` [PATCH 1/4] clone: add tests for --template and some disallowed option pairs Taylor Blau
  2020-09-15 16:09       ` Sean Barag
  2 siblings, 1 reply; 26+ messages in thread
From: Eric Sunshine @ 2020-09-11 20:07 UTC (permalink / raw)
  To: Jeff King
  Cc: Derrick Stolee, Sean Barag via GitGitGadget, Git List,
	Junio C Hamano, Johannes Schindelin, Sean Barag

On Fri, Sep 11, 2020 at 3:56 PM Jeff King <peff@peff.net> wrote:
> 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.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/4] clone: call git_config before parse_options
  2020-09-11 18:25 ` [PATCH 2/4] clone: call git_config before parse_options Sean Barag via GitGitGadget
  2020-09-11 18:59   ` Derrick Stolee
@ 2020-09-11 20:26   ` Junio C Hamano
  2020-09-16 16:12     ` Sean Barag
  1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2020-09-11 20:26 UTC (permalink / raw)
  To: Sean Barag via GitGitGadget; +Cc: git, Johannes Schindelin, Sean Barag

"Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sean Barag <sean@barag.org>
>
> 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.

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.

> Signed-off-by: Sean Barag <sean@barag.org>
> Thanks-to: Junio C Hamano <gitster@pobox.com>
> Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

> +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
> +	 */

Overlong lines...

> +	int apply_failed = git_default_config(key, value, data);

Not git_clone_config()?  Presumably you'll make git_clone_config()
recognise more variables than git_default_config() does, and the
caller of this helper wants us to recognise "clone.*" that are
ignored by git_default_config() callback, no?

> +	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);

The comment that explains the location is very much appropriate.

> -	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);

Does this call read from the freshly written file?

I thought git_config() iterates over the in-core configset that was
read by the first call to git_config(), which in turn calls
git_config_check_init() and calls repo_read_config() only once to
populate the in-core configset, and I suspect we are not clearing
it in between.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] clone: validate --origin option before use
  2020-09-11 18:25 ` [PATCH 3/4] clone: validate --origin option before use Sean Barag via GitGitGadget
  2020-09-11 19:24   ` Derrick Stolee
@ 2020-09-11 20:39   ` Junio C Hamano
  2020-09-16 17:11     ` Sean Barag
  1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2020-09-11 20:39 UTC (permalink / raw)
  To: Sean Barag via GitGitGadget; +Cc: git, Johannes Schindelin, Sean Barag

"Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Sean Barag <sean@barag.org>
>
> 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).  

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.

> This patch reintroduces

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

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

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.

> +	strbuf_addf(&resolved_refspec, "refs/heads/test:refs/remotes/%s/test", option_origin);
> +	if (!valid_fetch_refspec(resolved_refspec.buf))

If we reintroduce pre-8434c2f1 check, then we would want

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

or something like that?

> +		/* TRANSLATORS: %s will be the user-provided --origin / -o option */
> +		die(_("'%s' is not a valid origin name"), option_origin);

I am not sure if this translator comment is helping.

The message makes it sound as if "%s" here is used to switch between
'-o' or '--origin'.  If it said "'%s' will be the value given to
--origin/-o option", it would become much less confusing.

> +	strbuf_release(&resolved_refspec);
> +
>  	repo_name = argv[0];
>  
>  	path = get_repo_path(repo_name, &is_bundle);
> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index d20a78f84b..c865f96def 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -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
> +
> +'

... and we can also test for "bad/name" here in another test.

>  test_expect_success 'disallows --bare with --origin' '
>  
>  	test_expect_code 128 git clone -o foo --bare parent clone-bare-o 2>err &&

Thanks.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] clone: allow configurable default for `-o`/`--origin`
  2020-09-11 18:25 ` [PATCH 4/4] clone: allow configurable default for `-o`/`--origin` Sean Barag via GitGitGadget
  2020-09-11 19:13   ` Derrick Stolee
@ 2020-09-11 21:00   ` Junio C Hamano
  2020-09-17 15:25   ` Andrei Rybak
  2 siblings, 0 replies; 26+ messages in thread
From: Junio C Hamano @ 2020-09-11 21:00 UTC (permalink / raw)
  To: Sean Barag via GitGitGadget; +Cc: git, Johannes Schindelin, Sean Barag

"Sean Barag via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 1cd62d0001..aeb41f15f3 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -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";

This has a side effect of making all the code locations that used to
refer to option_origin much easier to read, like ...

> @@ -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);

... this place ;-)  Happy.

>  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);
>  }

clone.defaultremotename is a single valued configuration variable,
and this correctly implements the "last one wins" behaviour (but
previous remote_name will leak every time clone.defaultremotename
is seen in the config stream).

Also this code arrangement is not quite satisfactory.  It means that
we cannot re-read any configuration variable that does not have an
accompanying command line option.  I thought the whole point of
doing the write_config() was so that anything came from the command
line option can be written back to the configuration file, so I am
not sure what the harm would be to update remote_name from the
configuration whether option_origin is used or not here.  Perhaps
add "clone.defaultremotename" to the set of configuration setting
write_config() uses, when --option is given from the command line,
and remove this special case?

By the way, I now realized why 2/4's "read twice" is OK.  init_db()
calls create_default_files() and we do clare the cached configset by
calling git_config_clear() there.

Thanks.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs
  2020-09-11 18:57   ` Derrick Stolee
  2020-09-11 19:56     ` Jeff King
@ 2020-09-11 21:02     ` Junio C Hamano
  2020-09-12  0:41       ` Derrick Stolee
  1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2020-09-11 21:02 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: Sean Barag via GitGitGadget, git, Johannes Schindelin, Sean Barag

Derrick Stolee <stolee@gmail.com> writes:

>> +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
>> +
>> +'
>
> It seems that all of your tests have an extraneous newline
> at the end.

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.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs
  2020-09-11 21:02     ` Junio C Hamano
@ 2020-09-12  0:41       ` Derrick Stolee
  0 siblings, 0 replies; 26+ messages in thread
From: Derrick Stolee @ 2020-09-12  0:41 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Sean Barag via GitGitGadget, git, Johannes Schindelin,
	Sean Barag, Jeff King

On 9/11/2020 5:02 PM, Junio C Hamano wrote:
> Derrick Stolee <stolee@gmail.com> writes:
> 
>>> +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
>>> +
>>> +'
>>
>> It seems that all of your tests have an extraneous newline
>> at the end.
> 
> 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 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



^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs
  2020-09-11 19:56     ` Jeff King
  2020-09-11 20:07       ` Eric Sunshine
@ 2020-09-12  3:17       ` Taylor Blau
  2020-09-15 16:09       ` Sean Barag
  2 siblings, 0 replies; 26+ messages in thread
From: Taylor Blau @ 2020-09-12  3:17 UTC (permalink / raw)
  To: Jeff King
  Cc: Derrick Stolee, Sean Barag via GitGitGadget, git, Junio C Hamano,
	Johannes Schindelin, Sean Barag

On Fri, Sep 11, 2020 at 03:56:22PM -0400, Jeff King wrote:
>  - 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).

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.

> -Peff

Thanks,
Taylor

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs
  2020-09-11 19:56     ` Jeff King
  2020-09-11 20:07       ` Eric Sunshine
  2020-09-12  3:17       ` [PATCH 1/4] clone: add tests for --template and some disallowed option pairs Taylor Blau
@ 2020-09-15 16:09       ` Sean Barag
  2020-09-16 16:36         ` Jeff King
  2 siblings, 1 reply; 26+ messages in thread
From: Sean Barag @ 2020-09-15 16:09 UTC (permalink / raw)
  To: peff; +Cc: git, gitgitgadget, gitster, johannes.schindelin, sean, stolee

On Fri, 11 Sep 2020 15:56:22 -0400, Jeff King wrote:
 > do we really care about code 128, or just failure? test_must_fail
   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.

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

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

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

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

Sean Barag

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 0/4] clone: allow configurable default for -o/--origin
  2020-09-11 20:07       ` Eric Sunshine
@ 2020-09-16  3:15         ` Sean Barag
  0 siblings, 0 replies; 26+ messages in thread
From: Sean Barag @ 2020-09-16  3:15 UTC (permalink / raw)
  To: sunshine
  Cc: git, gitgitgadget, gitster, johannes.schindelin, peff, sean, stolee

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

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

> 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

Oooh `test_cmp` looks much better.  Thanks for the tip!

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 2/4] clone: call git_config before parse_options
  2020-09-11 20:26   ` Junio C Hamano
@ 2020-09-16 16:12     ` Sean Barag
  0 siblings, 0 replies; 26+ messages in thread
From: Sean Barag @ 2020-09-16 16:12 UTC (permalink / raw)
  To: gitster; +Cc: git, gitgitgadget, johannes.schindelin, sean

> From: Junio C Hamano <gitster@pobox.com>
> > From: Sean Barag <sean@barag.org>
> >
> > 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.
>
> 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.

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

> > +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
> > +	 */
>
> Overlong lines...

How embarrassing!  Re-wrapped for v2.

> > +	int apply_failed = git_default_config(key, value, data);
>
> Not git_clone_config()?  Presumably you'll make git_clone_config()
> recognise more variables than git_default_config() does, and the
> caller of this helper wants us to recognise "clone.*" that are
> ignored by git_default_config() callback, no?

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] clone: validate --origin option before use
  2020-09-11 19:24   ` Derrick Stolee
@ 2020-09-16 16:28     ` Sean Barag
  0 siblings, 0 replies; 26+ messages in thread
From: Sean Barag @ 2020-09-16 16:28 UTC (permalink / raw)
  To: stolee; +Cc: git, gitgitgadget, gitster, johannes.schindelin, sean

On Fri, 11 Sep 2020 at 15:24:15 -0400, Derrick Stolee writes:
> On 9/11/2020 2:25 PM, Sean Barag via GitGitGadget wrote:
> > +	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);
>
> Looking at this again, I'm not sure the translators note is
> necessary. Also, I would say "is not a valid remote name".
> That makes the string align with the already-translated string
> in builtin/remote.c.

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.

> This code is duplicated from builtin/remote.c, so I'd rather
> see this be a helper method in refspec.c and have both
> builtin/clone.c and builtin/remote.c call that helper.
>
> Here is the helper:
>
> void 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;
> }
>
> And here is the use in builtin/clone.c:
>
> 	if (!valid_remote_name(option_origin))
> 		die(_("'%s' is not a valid remote name"), option_origin);
>
> and in builtin/remote.c:
>
> 	if (!valid_remote_name(name))
> 		die(_("'%s' is not a valid remote name"), name);

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

> > +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
> > +
> > +'
> > +
>
> Double newlines here! I personally appreciate newlines to
> spread out content, but it doesn't fit our guidelines.

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

--
Sean

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 1/4] clone: add tests for --template and some disallowed option pairs
  2020-09-15 16:09       ` Sean Barag
@ 2020-09-16 16:36         ` Jeff King
  0 siblings, 0 replies; 26+ messages in thread
From: Jeff King @ 2020-09-16 16:36 UTC (permalink / raw)
  To: Sean Barag; +Cc: git, gitgitgadget, gitster, johannes.schindelin, stolee

On Tue, Sep 15, 2020 at 09:09:44AM -0700, Sean Barag wrote:

> On Fri, 11 Sep 2020 15:56:22 -0400, Jeff King wrote:
>  > do we really care about code 128, or just failure? test_must_fail
>    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.

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 3/4] clone: validate --origin option before use
  2020-09-11 20:39   ` Junio C Hamano
@ 2020-09-16 17:11     ` Sean Barag
  0 siblings, 0 replies; 26+ messages in thread
From: Sean Barag @ 2020-09-16 17:11 UTC (permalink / raw)
  To: gitster; +Cc: git, gitgitgadget, johannes.schindelin, sean

On Fri, 11 Sep 2020 at 13:39:20 -0700, Junio C Hamano writes:
> > From: Sean Barag <sean@barag.org>
> >
> > 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).
>
> 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.

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

> A micronit.  We describe the current status in present tense when
> presenting the problem to be solved, so "currently" can be dropped.
>
> > This patch reintroduces
>
> 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".

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

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


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?

> > +	strbuf_addf(&resolved_refspec, "refs/heads/test:refs/remotes/%s/test", option_origin);
> > +	if (!valid_fetch_refspec(resolved_refspec.buf))
>
> If we reintroduce pre-8434c2f1 check, then we would want
>
> 	if (!valid_fetch_refspec(...) || strchr(option_origin, '/'))
>
> or something like that?

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

> > +		/* TRANSLATORS: %s will be the user-provided --origin / -o option */
> > +		die(_("'%s' is not a valid origin name"), option_origin);
>
> I am not sure if this translator comment is helping.
>
> The message makes it sound as if "%s" here is used to switch between
> '-o' or '--origin'.  If it said "'%s' will be the value given to
> --origin/-o option", it would become much less confusing.

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/bf0107fb-2a6c-68d3-df24-72c6a9df6182@gmail.com/


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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] clone: allow configurable default for `-o`/`--origin`
  2020-09-11 18:25 ` [PATCH 4/4] clone: allow configurable default for `-o`/`--origin` Sean Barag via GitGitGadget
  2020-09-11 19:13   ` Derrick Stolee
  2020-09-11 21:00   ` Junio C Hamano
@ 2020-09-17 15:25   ` Andrei Rybak
  2 siblings, 0 replies; 26+ messages in thread
From: Andrei Rybak @ 2020-09-17 15:25 UTC (permalink / raw)
  To: Sean Barag via GitGitGadget, git
  Cc: Junio C Hamano, Johannes Schindelin, Sean Barag

On 2020-09-11 20:25, Sean Barag via GitGitGadget wrote:
> From: Sean Barag <sean@barag.org>
>
> 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

Number 3 is used twice in this list.

> 4. The default value of `origin`
>
> Signed-off-by: Sean Barag <sean@barag.org>
> Thanks-to: Junio C Hamano <gitster@pobox.com>
> Thanks-to: Johannes Schindelin <johannes.schindelin@gmx.de>


^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2020-09-17 15:27 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-11 18:25 [PATCH 0/4] clone: allow configurable default for -o/--origin Sean Barag via GitGitGadget
2020-09-11 18:25 ` [PATCH 1/4] clone: add tests for --template and some disallowed option pairs Sean Barag via GitGitGadget
2020-09-11 18:57   ` Derrick Stolee
2020-09-11 19:56     ` Jeff King
2020-09-11 20:07       ` Eric Sunshine
2020-09-16  3:15         ` [PATCH 0/4] clone: allow configurable default for -o/--origin Sean Barag
2020-09-12  3:17       ` [PATCH 1/4] clone: add tests for --template and some disallowed option pairs Taylor Blau
2020-09-15 16:09       ` Sean Barag
2020-09-16 16:36         ` Jeff King
2020-09-11 21:02     ` Junio C Hamano
2020-09-12  0:41       ` Derrick Stolee
2020-09-11 18:25 ` [PATCH 2/4] clone: call git_config before parse_options Sean Barag via GitGitGadget
2020-09-11 18:59   ` Derrick Stolee
2020-09-11 20:26   ` Junio C Hamano
2020-09-16 16:12     ` Sean Barag
2020-09-11 18:25 ` [PATCH 3/4] clone: validate --origin option before use Sean Barag via GitGitGadget
2020-09-11 19:24   ` Derrick Stolee
2020-09-16 16:28     ` Sean Barag
2020-09-11 20:39   ` Junio C Hamano
2020-09-16 17:11     ` Sean Barag
2020-09-11 18:25 ` [PATCH 4/4] clone: allow configurable default for `-o`/`--origin` Sean Barag via GitGitGadget
2020-09-11 19:13   ` Derrick Stolee
2020-09-11 21:00   ` Junio C Hamano
2020-09-17 15:25   ` Andrei Rybak
2020-09-11 19:25 ` [PATCH 0/4] clone: allow configurable default for -o/--origin Derrick Stolee
2020-09-11 19:34 ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git