* Re: [PATCH v5] builtin/clone.c: add --reject-shallow option
2021-02-28 18:06 ` [PATCH v5] " Li Linchao via GitGitGadget
@ 2021-03-01 7:11 ` lilinchao
2021-03-01 22:40 ` Johannes Schindelin
2021-03-03 23:21 ` Junio C Hamano
2021-03-04 17:19 ` [PATCH v6] " Li Linchao via GitGitGadget
2 siblings, 1 reply; 48+ messages in thread
From: lilinchao @ 2021-03-01 7:11 UTC (permalink / raw)
To: Li Linchao via GitGitGadget, git; +Cc: Junio C Hamano, Derrick Stolee, dscho
--------------
lilinchao@oschina.cn
>From: lilinchao <lilinchao@oschina.cn>
>
>In some scenarios, users may want more history than the repository
>offered for cloning, which happens to be a shallow repository, can
>give them. But because users don't know it is a shallow repository
>until they download it to local, users should have the option to
>refuse to clone this kind of repository, and may want to exit the
>process immediately without creating any unnecessary files.
>
>Althought there is an option '--depth=x' for users to decide how
>deep history they can fetch, but as the unshallow cloning's depth
>is INFINITY, we can't know exactly the minimun 'x' value that can
>satisfy the minimum integrity, so we can't pass 'x' value to --depth,
>and expect this can obtain a complete history of a repository.
>
>In other scenarios, if we have an API that allow us to import external
>repository, and then perform various operations on the repo.
>But if the imported is a shallow one(which is actually possible), it
>will affect the subsequent operations. So we can choose to refuse to
>clone, and let's just import a normal repository.
>
>This patch offers a new option '--reject-shallow' that can reject to
>clone a shallow repository.
>
>Signed-off-by: lilinchao <lilinchao@oschina.cn>
>---
> builtin/clone.c: add --reject-shallow option
>
> Changes since v1:
>
> * Rename --no-shallow to --reject-shallow
> * Enable to reject a non-local clone
> * Enable --[no-]reject-shallow from CLI override configuration.
> * Add more testcases.
> * Reword commit messages and relative documentation.
>
> Changes since v3:
>
> * Add support to reject clone shallow repo over https protocol
> * Add testcase to reject clone shallow repo over https:// transport
> * Reword commit messages and relative documentation according
> suggestions from Junio.
>
> Signed-off-by: lilinchao lilinchao@oschina.cn
>
>Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v5
>Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v5
>Pull-Request: https://github.com/gitgitgadget/git/pull/865
>
>Range-diff vs v4:
>
> 1: ee4fb840a32f ! 1: 3f765e49e4a7 builtin/clone.c: add --reject-shallow option
> @@ Documentation/git-clone.txt: objects from the source repository into a pack in t
> creating `<directory>` and placing the administrative
>
> ## builtin/clone.c ##
> -@@
> - #include "connected.h"
> - #include "packfile.h"
> - #include "list-objects-filter-options.h"
> -+#include "shallow.h"
> -
> - /*
> - * Overall FIXMEs:
> @@ builtin/clone.c: static int option_no_checkout, option_bare, option_mirror, option_single_branch
> static int option_local = -1, option_no_hardlinks, option_shared;
> static int option_no_tags;
> @@ builtin/clone.c: static int path_exists(const char *path)
> int cmd_clone(int argc, const char **argv, const char *prefix)
> {
> int is_bundle = 0, is_local;
> -+ int local_shallow = 0;
> + int reject_shallow = 0;
> const char *repo_name, *repo, *work_tree, *git_dir;
> char *path, *dir, *display_repo = NULL;
> @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
> if (filter_options.choice)
> warning(_("--filter is ignored in local clones; use file:// instead."));
> if (!access(mkpath("%s/shallow", path), F_OK)) {
> -+ local_shallow = 1;
> ++ if (reject_shallow)
> ++ die("source repository is shallow, reject to clone.");
> if (option_local > 0)
> warning(_("source repository is shallow, ignoring --local"));
> is_local = 0;
> @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
> - goto cleanup;
> - }
>
> -+ if (reject_shallow) {
> -+ if (local_shallow || is_repository_shallow(the_repository)) {
> -+ die(_("source repository is shallow, reject to clone."));
> -+ }
> -+ }
> + transport_set_option(transport, TRANS_OPT_KEEP, "yes");
> +
> ++ if (reject_shallow)
> ++ transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
> + if (option_depth)
> + transport_set_option(transport, TRANS_OPT_DEPTH,
> + option_depth);
> +
> + ## fetch-pack.c ##
> +@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
> +
> + if (args->stateless_rpc)
> + packet_flush(fd[1]);
> ++
> ++ if (!args->deepen && args->remote_shallow)
> ++ die("source repository is shallow, reject to clone.");
> +
> - update_remote_refs(refs, mapped_refs, remote_head_points_at,
> - branch_top.buf, reflog_msg.buf, transport,
> - !is_local);
> + if (args->deepen)
> + setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
> + NULL);
> +@@ fetch-pack.c: static void receive_shallow_info(struct fetch_pack_args *args,
> + * shallow. In v0, remote refs that reach these objects are
> + * rejected (unless --update-shallow is set); do the same.
> + */
> ++ if (args->remote_shallow)
> ++ die("source repository is shallow, reject to clone.");
> + prepare_shallow_info(si, shallows);
> + if (si->nr_ours || si->nr_theirs)
> + alternate_shallow_file =
> +
> + ## fetch-pack.h ##
> +@@ fetch-pack.h: struct fetch_pack_args {
> + unsigned self_contained_and_connected:1;
> + unsigned cloning:1;
> + unsigned update_shallow:1;
> ++ unsigned remote_shallow:1;
> + unsigned deepen:1;
> +
> + /*
>
> ## t/t5606-clone-options.sh ##
> @@ t/t5606-clone-options.sh: GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
> +'
> +
> +test_expect_success 'clone.rejectshallow=false should succeed' '
> -+ rm -rf child &&
> ++ rm -rf child out &&
> + git clone --depth=1 --no-local . child &&
> + git -c clone.rejectshallow=false clone --no-local child out
> +'
> @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
> +'
> +
> +test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
> -+ rm -rf child &&
> ++ rm -rf child out &&
> + git clone --depth=1 --no-local . child &&
> + git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
> +'
> @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
> test_expect_success MINGW 'clone -c core.hideDotFiles' '
> test_commit attributes .gitattributes "" &&
> rm -rf child &&
> +
> + ## transport-helper.c ##
> +@@ transport-helper.c: static const char *boolean_options[] = {
> + TRANS_OPT_THIN,
> + TRANS_OPT_KEEP,
> + TRANS_OPT_FOLLOWTAGS,
> +- TRANS_OPT_DEEPEN_RELATIVE
> ++ TRANS_OPT_DEEPEN_RELATIVE,
> ++ TRANS_OPT_REJECT_SHALLOW
> + };
> +
> + static int strbuf_set_helper_option(struct helper_data *data,
> +
> + ## transport.c ##
> +@@ transport.c: static int set_git_option(struct git_transport_options *opts,
> + list_objects_filter_die_if_populated(&opts->filter_options);
> + parse_list_objects_filter(&opts->filter_options, value);
> + return 0;
> ++ } else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
> ++ opts->reject_shallow = !!value;
> ++ return 0;
> + }
> + return 1;
> + }
> +@@ transport.c: static int fetch_refs_via_pack(struct transport *transport,
> + args.stateless_rpc = transport->stateless_rpc;
> + args.server_options = transport->server_options;
> + args.negotiation_tips = data->options.negotiation_tips;
> ++ args.remote_shallow = transport->smart_options->reject_shallow;
> +
> + if (!data->got_remote_heads) {
> + int i;
> +
> + ## transport.h ##
> +@@ transport.h: struct git_transport_options {
> + unsigned check_self_contained_and_connected : 1;
> + unsigned self_contained_and_connected : 1;
> + unsigned update_shallow : 1;
> ++ unsigned reject_shallow : 1;
> + unsigned deepen_relative : 1;
> +
> + /* see documentation of corresponding flag in fetch-pack.h */
> +@@ transport.h: void transport_check_allowed(const char *type);
> + /* Aggressively fetch annotated tags if possible */
> + #define TRANS_OPT_FOLLOWTAGS "followtags"
> +
> ++/* reject shallow repo transport */
> ++#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
> ++
> + /* Accept refs that may update .git/shallow without --depth */
> + #define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
> +
>
>
> Documentation/config/clone.txt | 4 +++
> Documentation/git-clone.txt | 7 ++++-
> builtin/clone.c | 22 ++++++++++++++++
> fetch-pack.c | 6 +++++
> fetch-pack.h | 1 +
> t/t5606-clone-options.sh | 47 ++++++++++++++++++++++++++++++++++
> t/t5611-clone-config.sh | 32 +++++++++++++++++++++++
> transport-helper.c | 3 ++-
> transport.c | 4 +++
> transport.h | 4 +++
> 10 files changed, 128 insertions(+), 2 deletions(-)
>
>diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
>index 47de36a5fedf..50ebc170bb81 100644
>--- a/Documentation/config/clone.txt
>+++ b/Documentation/config/clone.txt
>@@ -2,3 +2,7 @@ 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].
>+
>+clone.rejectshallow::
>+ Reject to clone a repository if it is a shallow one, can be overridden by
>+ passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
>diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
>index 02d9c19cec75..cb458123eef6 100644
>--- a/Documentation/git-clone.txt
>+++ b/Documentation/git-clone.txt
>@@ -15,7 +15,7 @@ SYNOPSIS
> [--dissociate] [--separate-git-dir <git dir>]
> [--depth <depth>] [--[no-]single-branch] [--no-tags]
> [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
>- [--[no-]remote-submodules] [--jobs <n>] [--sparse]
>+ [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
> [--filter=<filter>] [--] <repository>
> [<directory>]
>
>@@ -149,6 +149,11 @@ objects from the source repository into a pack in the cloned repository.
> --no-checkout::
> No checkout of HEAD is performed after the clone is complete.
>
>+--[no-]reject-shallow::
>+ Fail if the source repository is a shallow repository. The
>+ 'clone.rejectshallow' configuration variable can be used to
>+ give the default.
>+
> --bare::
> Make a 'bare' Git repository. That is, instead of
> creating `<directory>` and placing the administrative
>diff --git a/builtin/clone.c b/builtin/clone.c
>index 51e844a2de0a..70d8ca77988f 100644
>--- a/builtin/clone.c
>+++ b/builtin/clone.c
>@@ -50,6 +50,8 @@ static int option_no_checkout, option_bare, option_mirror, option_single_branch
> static int option_local = -1, option_no_hardlinks, option_shared;
> static int option_no_tags;
> static int option_shallow_submodules;
>+static int option_shallow = -1; /* unspecified */
>+static int config_shallow = -1; /* unspecified */
> static int deepen;
> static char *option_template, *option_depth, *option_since;
> static char *option_origin = NULL;
>@@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
> OPT__VERBOSITY(&option_verbosity),
> OPT_BOOL(0, "progress", &option_progress,
> N_("force progress reporting")),
>+ OPT_BOOL(0, "reject-shallow", &option_shallow,
>+ N_("don't clone shallow repository")),
> OPT_BOOL('n', "no-checkout", &option_no_checkout,
> N_("don't create a checkout")),
> OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),
>@@ -858,6 +862,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
> free(remote_name);
> remote_name = xstrdup(v);
> }
>+ if (!strcmp(k, "clone.rejectshallow")) {
>+ config_shallow = git_config_bool(k, v);
>+ }
> return git_default_config(k, v, cb);
> }
>
>@@ -963,6 +970,7 @@ static int path_exists(const char *path)
> int cmd_clone(int argc, const char **argv, const char *prefix)
> {
> int is_bundle = 0, is_local;
>+ int reject_shallow = 0;
> const char *repo_name, *repo, *work_tree, *git_dir;
> char *path, *dir, *display_repo = NULL;
> int dest_exists, real_dest_exists = 0;
>@@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> */
> git_config(git_clone_config, NULL);
>
>+ /*
>+ * If option_shallow is specified from CLI option,
>+ * ignore config_shallow from git_clone_config.
>+ */
>+ if (config_shallow != -1) {
>+ reject_shallow = config_shallow;
>+ }
>+ if (option_shallow != -1) {
>+ reject_shallow = option_shallow;
>+ }
> /*
> * apply the remote name provided by --origin only after this second
> * call to git_config, to ensure it overrides all config-based values.
>@@ -1216,6 +1234,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> if (filter_options.choice)
> warning(_("--filter is ignored in local clones; use file:// instead."));
> if (!access(mkpath("%s/shallow", path), F_OK)) {
>+ if (reject_shallow)
>+ die("source repository is shallow, reject to clone.");
> if (option_local > 0)
> warning(_("source repository is shallow, ignoring --local"));
> is_local = 0;
>@@ -1227,6 +1247,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>
> transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>
>+ if (reject_shallow)
>+ transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
> if (option_depth)
> transport_set_option(transport, TRANS_OPT_DEPTH,
> option_depth);
>diff --git a/fetch-pack.c b/fetch-pack.c
>index 1eaedcb5dc2e..41ef700bde38 100644
>--- a/fetch-pack.c
>+++ b/fetch-pack.c
>@@ -1071,6 +1071,10 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>
> if (args->stateless_rpc)
> packet_flush(fd[1]);
>+
>+ if (!args->deepen && args->remote_shallow)
>+ die("source repository is shallow, reject to clone.");
>+
I am not sure if we should apply this to non-protocol-v2.
> if (args->deepen)
> setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
> NULL);
>@@ -1440,6 +1444,8 @@ static void receive_shallow_info(struct fetch_pack_args *args,
> * shallow. In v0, remote refs that reach these objects are
> * rejected (unless --update-shallow is set); do the same.
> */
>+ if (args->remote_shallow)
>+ die("source repository is shallow, reject to clone.");
I just found that Johannes Schindelin wrote a document 14 year ago
in Documentation/technical/shallow.txt:
"There are some unfinished ends of the whole shallow business:
A special handling of a shallow upstream is needed. At some stage,
upload-pack has to check if it sends a shallow commit, and it should
send that information early (or fail, if the client does not support
shallow repositories). There is no support at all for this in this patch
series."
It seems that my patch can sovle his worry in some degree,
and maybe we could warn client in fetch-pack stage, if we don't
choose to reject shallow cloning.
if (args->remote_shallow)
die("source repository is shallow, reject to clone.");
else
warning("remote source repository is shallow.");
> prepare_shallow_info(si, shallows);
> if (si->nr_ours || si->nr_theirs)
> alternate_shallow_file =
>diff --git a/fetch-pack.h b/fetch-pack.h
>index 736a3dae467a..6e4f8f0d738c 100644
>--- a/fetch-pack.h
>+++ b/fetch-pack.h
>@@ -39,6 +39,7 @@ struct fetch_pack_args {
> unsigned self_contained_and_connected:1;
> unsigned cloning:1;
> unsigned update_shallow:1;
>+ unsigned remote_shallow:1;
> unsigned deepen:1;
>
> /*
>diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
>index 52e5789fb050..6170d0513227 100755
>--- a/t/t5606-clone-options.sh
>+++ b/t/t5606-clone-options.sh
>@@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> . ./test-lib.sh
>+. "$TEST_DIRECTORY"/lib-httpd.sh
>+start_httpd
>
> test_expect_success 'setup' '
>
>@@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>
> '
>
>+test_expect_success 'fail to clone http shallow repository' '
>+ git clone --depth=1 --no-local parent shallow-repo &&
>+ git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
>+ test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git out 2>err &&
>+ test_i18ngrep -e "source repository is shallow, reject to clone." err
>+
>+'
>+
>+test_expect_success 'fail to clone shallow repository' '
>+ rm -rf shallow-repo &&
>+ git clone --depth=1 --no-local parent shallow-repo &&
>+ test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
>+ test_i18ngrep -e "source repository is shallow, reject to clone." err
>+
>+'
>+
>+test_expect_success 'fail to clone non-local shallow repository' '
>+ rm -rf shallow-repo &&
>+ git clone --depth=1 --no-local parent shallow-repo &&
>+ test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
>+ test_i18ngrep -e "source repository is shallow, reject to clone." err
>+
>+'
>+
>+test_expect_success 'clone shallow repository with --no-reject-shallow' '
>+ rm -rf shallow-repo &&
>+ git clone --depth=1 --no-local parent shallow-repo &&
>+ git clone --no-reject-shallow --no-local shallow-repo clone-repo
>+
>+'
>+
>+test_expect_success 'clone normal repository with --reject-shallow' '
>+ rm -rf clone-repo &&
>+ git clone --no-local parent normal-repo &&
>+ git clone --reject-shallow --no-local normal-repo clone-repo
>+
>+'
>+
>+test_expect_success 'unspecified any configs or options' '
>+ rm -rf shallow-repo clone-repo &&
>+ git clone --depth=1 --no-local parent shallow-repo &&
>+ git clone shallow-repo clone-repo
>+
>+'
>+
> test_expect_success 'uses "origin" for default remote name' '
>
> git clone parent clone-default-origin &&
>diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
>index 9f555b87ecdf..da10d3f10352 100755
>--- a/t/t5611-clone-config.sh
>+++ b/t/t5611-clone-config.sh
>@@ -95,6 +95,38 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
> test_cmp expect actual
> '
>
>+test_expect_success 'clone.rejectshallow=true should fail to clone' '
>+ rm -rf child &&
>+ git clone --depth=1 --no-local . child &&
>+ test_must_fail git -c clone.rejectshallow=true clone --no-local child out 2>err &&
>+ test_i18ngrep -e "source repository is shallow, reject to clone." err
>+'
>+
>+test_expect_success 'clone.rejectshallow=false should succeed' '
>+ rm -rf child out &&
>+ git clone --depth=1 --no-local . child &&
>+ git -c clone.rejectshallow=false clone --no-local child out
>+'
>+
>+test_expect_success 'clone.rejectshallow=true should succeed with normal repo' '
>+ rm -rf child out &&
>+ git clone --no-local . child &&
>+ git -c clone.rejectshallow=true clone --no-local child out
>+'
>+
>+test_expect_success 'option --reject-shallow override clone.rejectshallow' '
>+ rm -rf child out &&
>+ git clone --depth=1 --no-local . child &&
>+ test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local child out 2>err &&
>+ test_i18ngrep -e "source repository is shallow, reject to clone." err
>+'
>+
>+test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
>+ rm -rf child out &&
>+ git clone --depth=1 --no-local . child &&
>+ git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
>+'
>+
> test_expect_success MINGW 'clone -c core.hideDotFiles' '
> test_commit attributes .gitattributes "" &&
> rm -rf child &&
>diff --git a/transport-helper.c b/transport-helper.c
>index 49b7fb4dcb9a..b57b55c8180c 100644
>--- a/transport-helper.c
>+++ b/transport-helper.c
>@@ -265,7 +265,8 @@ static const char *boolean_options[] = {
> TRANS_OPT_THIN,
> TRANS_OPT_KEEP,
> TRANS_OPT_FOLLOWTAGS,
>- TRANS_OPT_DEEPEN_RELATIVE
>+ TRANS_OPT_DEEPEN_RELATIVE,
>+ TRANS_OPT_REJECT_SHALLOW
> };
>
> static int strbuf_set_helper_option(struct helper_data *data,
>diff --git a/transport.c b/transport.c
>index b13fab5dc3b1..34fe01221ee0 100644
>--- a/transport.c
>+++ b/transport.c
>@@ -236,6 +236,9 @@ static int set_git_option(struct git_transport_options *opts,
> list_objects_filter_die_if_populated(&opts->filter_options);
> parse_list_objects_filter(&opts->filter_options, value);
> return 0;
>+ } else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
>+ opts->reject_shallow = !!value;
>+ return 0;
> }
> return 1;
> }
>@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> args.stateless_rpc = transport->stateless_rpc;
> args.server_options = transport->server_options;
> args.negotiation_tips = data->options.negotiation_tips;
>+ args.remote_shallow = transport->smart_options->reject_shallow;
>
> if (!data->got_remote_heads) {
> int i;
>diff --git a/transport.h b/transport.h
>index 24e15799e714..f6273d268b09 100644
>--- a/transport.h
>+++ b/transport.h
>@@ -14,6 +14,7 @@ struct git_transport_options {
> unsigned check_self_contained_and_connected : 1;
> unsigned self_contained_and_connected : 1;
> unsigned update_shallow : 1;
>+ unsigned reject_shallow : 1;
> unsigned deepen_relative : 1;
>
> /* see documentation of corresponding flag in fetch-pack.h */
>@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
> /* Aggressively fetch annotated tags if possible */
> #define TRANS_OPT_FOLLOWTAGS "followtags"
>
>+/* reject shallow repo transport */
>+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
>+
> /* Accept refs that may update .git/shallow without --depth */
> #define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
>
>
>base-commit: 225365fb5195e804274ab569ac3cc4919451dc7f
>--
>gitgitgadget
>
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v5] builtin/clone.c: add --reject-shallow option
2021-03-01 7:11 ` lilinchao
@ 2021-03-01 22:40 ` Johannes Schindelin
2021-03-04 6:26 ` lilinchao
0 siblings, 1 reply; 48+ messages in thread
From: Johannes Schindelin @ 2021-03-01 22:40 UTC (permalink / raw)
To: lilinchao@oschina.cn
Cc: Li Linchao via GitGitGadget, git, Junio C Hamano, Derrick Stolee
[-- Attachment #1: Type: text/plain, Size: 8366 bytes --]
Hi,
On Mon, 1 Mar 2021, lilinchao@oschina.cn wrote:
> >@@ -1440,6 +1444,8 @@ static void receive_shallow_info(struct fetch_pack_args *args,
> > * shallow. In v0, remote refs that reach these objects are
> > * rejected (unless --update-shallow is set); do the same.
> > */
> >+ if (args->remote_shallow)
> >+ die("source repository is shallow, reject to clone.");
>
> I just found that Johannes Schindelin wrote a document 14 year ago
> in Documentation/technical/shallow.txt:
>
> "There are some unfinished ends of the whole shallow business:
>
> A special handling of a shallow upstream is needed. At some stage,
> upload-pack has to check if it sends a shallow commit, and it should
> send that information early (or fail, if the client does not support
> shallow repositories). There is no support at all for this in this patch
> series."
Oh wow, what a blast from the past.
I do agree that your patch is an improvement over the current situation.
Thanks,
Johannes
> It seems that my patch can sovle his worry in some degree,
> and maybe we could warn client in fetch-pack stage, if we don't
> choose to reject shallow cloning.
>
> if (args->remote_shallow)
> die("source repository is shallow, reject to clone.");
> else
> warning("remote source repository is shallow.");
>
> > prepare_shallow_info(si, shallows);
> > if (si->nr_ours || si->nr_theirs)
> > alternate_shallow_file =
> >diff --git a/fetch-pack.h b/fetch-pack.h
> >index 736a3dae467a..6e4f8f0d738c 100644
> >--- a/fetch-pack.h
> >+++ b/fetch-pack.h
> >@@ -39,6 +39,7 @@ struct fetch_pack_args {
> > unsigned self_contained_and_connected:1;
> > unsigned cloning:1;
> > unsigned update_shallow:1;
> >+ unsigned remote_shallow:1;
> > unsigned deepen:1;
> >
> > /*
> >diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> >index 52e5789fb050..6170d0513227 100755
> >--- a/t/t5606-clone-options.sh
> >+++ b/t/t5606-clone-options.sh
> >@@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> > export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> >
> > . ./test-lib.sh
> >+. "$TEST_DIRECTORY"/lib-httpd.sh
> >+start_httpd
> >
> > test_expect_success 'setup' '
> >
> >@@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
> >
> > '
> >
> >+test_expect_success 'fail to clone http shallow repository' '
> >+ git clone --depth=1 --no-local parent shallow-repo &&
> >+ git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> >+ test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git out 2>err &&
> >+ test_i18ngrep -e "source repository is shallow, reject to clone." err
> >+
> >+'
> >+
> >+test_expect_success 'fail to clone shallow repository' '
> >+ rm -rf shallow-repo &&
> >+ git clone --depth=1 --no-local parent shallow-repo &&
> >+ test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
> >+ test_i18ngrep -e "source repository is shallow, reject to clone." err
> >+
> >+'
> >+
> >+test_expect_success 'fail to clone non-local shallow repository' '
> >+ rm -rf shallow-repo &&
> >+ git clone --depth=1 --no-local parent shallow-repo &&
> >+ test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
> >+ test_i18ngrep -e "source repository is shallow, reject to clone." err
> >+
> >+'
> >+
> >+test_expect_success 'clone shallow repository with --no-reject-shallow' '
> >+ rm -rf shallow-repo &&
> >+ git clone --depth=1 --no-local parent shallow-repo &&
> >+ git clone --no-reject-shallow --no-local shallow-repo clone-repo
> >+
> >+'
> >+
> >+test_expect_success 'clone normal repository with --reject-shallow' '
> >+ rm -rf clone-repo &&
> >+ git clone --no-local parent normal-repo &&
> >+ git clone --reject-shallow --no-local normal-repo clone-repo
> >+
> >+'
> >+
> >+test_expect_success 'unspecified any configs or options' '
> >+ rm -rf shallow-repo clone-repo &&
> >+ git clone --depth=1 --no-local parent shallow-repo &&
> >+ git clone shallow-repo clone-repo
> >+
> >+'
> >+
> > test_expect_success 'uses "origin" for default remote name' '
> >
> > git clone parent clone-default-origin &&
> >diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
> >index 9f555b87ecdf..da10d3f10352 100755
> >--- a/t/t5611-clone-config.sh
> >+++ b/t/t5611-clone-config.sh
> >@@ -95,6 +95,38 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
> > test_cmp expect actual
> > '
> >
> >+test_expect_success 'clone.rejectshallow=true should fail to clone' '
> >+ rm -rf child &&
> >+ git clone --depth=1 --no-local . child &&
> >+ test_must_fail git -c clone.rejectshallow=true clone --no-local child out 2>err &&
> >+ test_i18ngrep -e "source repository is shallow, reject to clone." err
> >+'
> >+
> >+test_expect_success 'clone.rejectshallow=false should succeed' '
> >+ rm -rf child out &&
> >+ git clone --depth=1 --no-local . child &&
> >+ git -c clone.rejectshallow=false clone --no-local child out
> >+'
> >+
> >+test_expect_success 'clone.rejectshallow=true should succeed with normal repo' '
> >+ rm -rf child out &&
> >+ git clone --no-local . child &&
> >+ git -c clone.rejectshallow=true clone --no-local child out
> >+'
> >+
> >+test_expect_success 'option --reject-shallow override clone.rejectshallow' '
> >+ rm -rf child out &&
> >+ git clone --depth=1 --no-local . child &&
> >+ test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local child out 2>err &&
> >+ test_i18ngrep -e "source repository is shallow, reject to clone." err
> >+'
> >+
> >+test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
> >+ rm -rf child out &&
> >+ git clone --depth=1 --no-local . child &&
> >+ git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
> >+'
> >+
> > test_expect_success MINGW 'clone -c core.hideDotFiles' '
> > test_commit attributes .gitattributes "" &&
> > rm -rf child &&
> >diff --git a/transport-helper.c b/transport-helper.c
> >index 49b7fb4dcb9a..b57b55c8180c 100644
> >--- a/transport-helper.c
> >+++ b/transport-helper.c
> >@@ -265,7 +265,8 @@ static const char *boolean_options[] = {
> > TRANS_OPT_THIN,
> > TRANS_OPT_KEEP,
> > TRANS_OPT_FOLLOWTAGS,
> >- TRANS_OPT_DEEPEN_RELATIVE
> >+ TRANS_OPT_DEEPEN_RELATIVE,
> >+ TRANS_OPT_REJECT_SHALLOW
> > };
> >
> > static int strbuf_set_helper_option(struct helper_data *data,
> >diff --git a/transport.c b/transport.c
> >index b13fab5dc3b1..34fe01221ee0 100644
> >--- a/transport.c
> >+++ b/transport.c
> >@@ -236,6 +236,9 @@ static int set_git_option(struct git_transport_options *opts,
> > list_objects_filter_die_if_populated(&opts->filter_options);
> > parse_list_objects_filter(&opts->filter_options, value);
> > return 0;
> >+ } else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
> >+ opts->reject_shallow = !!value;
> >+ return 0;
> > }
> > return 1;
> > }
> >@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> > args.stateless_rpc = transport->stateless_rpc;
> > args.server_options = transport->server_options;
> > args.negotiation_tips = data->options.negotiation_tips;
> >+ args.remote_shallow = transport->smart_options->reject_shallow;
> >
> > if (!data->got_remote_heads) {
> > int i;
> >diff --git a/transport.h b/transport.h
> >index 24e15799e714..f6273d268b09 100644
> >--- a/transport.h
> >+++ b/transport.h
> >@@ -14,6 +14,7 @@ struct git_transport_options {
> > unsigned check_self_contained_and_connected : 1;
> > unsigned self_contained_and_connected : 1;
> > unsigned update_shallow : 1;
> >+ unsigned reject_shallow : 1;
> > unsigned deepen_relative : 1;
> >
> > /* see documentation of corresponding flag in fetch-pack.h */
> >@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
> > /* Aggressively fetch annotated tags if possible */
> > #define TRANS_OPT_FOLLOWTAGS "followtags"
> >
> >+/* reject shallow repo transport */
> >+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
> >+
> > /* Accept refs that may update .git/shallow without --depth */
> > #define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
> >
> >
> >base-commit: 225365fb5195e804274ab569ac3cc4919451dc7f
> >--
> >gitgitgadget
> >
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: Re: [PATCH v5] builtin/clone.c: add --reject-shallow option
2021-03-01 22:40 ` Johannes Schindelin
@ 2021-03-04 6:26 ` lilinchao
0 siblings, 0 replies; 48+ messages in thread
From: lilinchao @ 2021-03-04 6:26 UTC (permalink / raw)
To: dscho; +Cc: Li Linchao via GitGitGadget, git, Junio C Hamano, Derrick Stolee
>Hi,
>
>On Mon, 1 Mar 2021, lilinchao@oschina.cn wrote:
>
>> >@@ -1440,6 +1444,8 @@ static void receive_shallow_info(struct fetch_pack_args *args,
>> > * shallow. In v0, remote refs that reach these objects are
>> > * rejected (unless --update-shallow is set); do the same.
>> > */
>> >+ if (args->remote_shallow)
>> >+ die("source repository is shallow, reject to clone.");
>>
>> I just found that Johannes Schindelin wrote a document 14 year ago
>> in Documentation/technical/shallow.txt:
>>
>> "There are some unfinished ends of the whole shallow business:
>>
>> A special handling of a shallow upstream is needed. At some stage,
>> upload-pack has to check if it sends a shallow commit, and it should
>> send that information early (or fail, if the client does not support
>> shallow repositories). There is no support at all for this in this patch
>> series."
>
>Oh wow, what a blast from the past.
>
>I do agree that your patch is an improvement over the current situation.
>
Thanks. Glad to hear the voice from you, hope I can get some suggestions
from you too, especially on the transport part.
>Thanks,
>Johannes
>
>> It seems that my patch can sovle his worry in some degree,
>> and maybe we could warn client in fetch-pack stage, if we don't
>> choose to reject shallow cloning.
>>
>> if (args->remote_shallow)
>> die("source repository is shallow, reject to clone.");
>> else
>> warning("remote source repository is shallow.");
>>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v5] builtin/clone.c: add --reject-shallow option
2021-02-28 18:06 ` [PATCH v5] " Li Linchao via GitGitGadget
2021-03-01 7:11 ` lilinchao
@ 2021-03-03 23:21 ` Junio C Hamano
2021-03-04 5:50 ` lilinchao
2021-03-04 17:19 ` [PATCH v6] " Li Linchao via GitGitGadget
2 siblings, 1 reply; 48+ messages in thread
From: Junio C Hamano @ 2021-03-03 23:21 UTC (permalink / raw)
To: Li Linchao via GitGitGadget; +Cc: git, Derrick Stolee, dscho, Li Linchao
"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
> index 47de36a5fedf..50ebc170bb81 100644
> --- a/Documentation/config/clone.txt
> +++ b/Documentation/config/clone.txt
> @@ -2,3 +2,7 @@ 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].
> +
> +clone.rejectshallow::
> + Reject to clone a repository if it is a shallow one, can be overridden by
> + passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
Let's camelCase this, i.e. "clone.rejectShallow", as this file would
be a good candidate to be the authoritative record of canonical
spelling of these variables.
cf. https://lore.kernel.org/git/xmqq7dmy389l.fsf@gitster.g/
> +--[no-]reject-shallow::
> + Fail if the source repository is a shallow repository. The
> + 'clone.rejectshallow' configuration variable can be used to
> + give the default.
Let's camelCase the reference to the variable, too. Also, typeset
in monospace, i.e.
The `clone.rejectShallow` configuration variable ...
> @@ -858,6 +862,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
> free(remote_name);
> remote_name = xstrdup(v);
> }
> + if (!strcmp(k, "clone.rejectshallow")) {
> + config_shallow = git_config_bool(k, v);
> + }
No need to use {} around a single-statement block, especially when
the "if" statement does not have an "else" block.
The use of strcmp() against the variable name in all lowercase is
correct here.
> @@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> */
> git_config(git_clone_config, NULL);
>
> + /*
> + * If option_shallow is specified from CLI option,
> + * ignore config_shallow from git_clone_config.
> + */
> + if (config_shallow != -1) {
> + reject_shallow = config_shallow;
> + }
> + if (option_shallow != -1) {
> + reject_shallow = option_shallow;
> + }
Needless use of {} around single-statement blocks.
As reject_shallow is initialized to 0, this lets the option to be of
the most priority, then the config (presumably coming from the per-user
or per-system configuration), by without them, defaults to false. Good.
We'll have an extra git_config() call later, but that one will only
read into config_shallow, never to be looked at because we will use
reject_shallow variable anyway. OK.
> @@ -1216,6 +1234,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> if (filter_options.choice)
> warning(_("--filter is ignored in local clones; use file:// instead."));
> if (!access(mkpath("%s/shallow", path), F_OK)) {
> + if (reject_shallow)
> + die("source repository is shallow, reject to clone.");
With the local transport, it (hopefully) is trivial to see if the
source is shallow. OK.
> if (option_local > 0)
> warning(_("source repository is shallow, ignoring --local"));
> is_local = 0;
> @@ -1227,6 +1247,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>
> transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>
> + if (reject_shallow)
> + transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
> if (option_depth)
> transport_set_option(transport, TRANS_OPT_DEPTH,
> option_depth);
OK. What is really interesting will all happen inside the transport
layer; the caller only has to ask for it.
The asymmetry with other options like "--depth" stands out and
puzzles readers, though.
Do we really want to add the clone.rejectShallow configuration?
After all, we do not give "clone.depth = 1" etc., and that is the
reason why we only need "if (option_depth)" here in the near-by
code.
I'd stop here for today, hoping that somebody much more familiar
with the transport layer than I am will review and comment on the
changes there.
Thanks.
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: Re: [PATCH v5] builtin/clone.c: add --reject-shallow option
2021-03-03 23:21 ` Junio C Hamano
@ 2021-03-04 5:50 ` lilinchao
0 siblings, 0 replies; 48+ messages in thread
From: lilinchao @ 2021-03-04 5:50 UTC (permalink / raw)
To: Junio C Hamano, Li Linchao via GitGitGadget; +Cc: git, Derrick Stolee, dscho
>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
>> index 47de36a5fedf..50ebc170bb81 100644
>> --- a/Documentation/config/clone.txt
>> +++ b/Documentation/config/clone.txt
>> @@ -2,3 +2,7 @@ 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].
>> +
>> +clone.rejectshallow::
>> + Reject to clone a repository if it is a shallow one, can be overridden by
>> + passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
>
>Let's camelCase this, i.e. "clone.rejectShallow", as this file would
>be a good candidate to be the authoritative record of canonical
>spelling of these variables.
>
>cf. https://lore.kernel.org/git/xmqq7dmy389l.fsf@gitster.g/
OK，thanks for reminding this.
>
>> +--[no-]reject-shallow::
>> + Fail if the source repository is a shallow repository. The
>> + 'clone.rejectshallow' configuration variable can be used to
>> + give the default.
>
>Let's camelCase the reference to the variable, too. Also, typeset
>in monospace, i.e.
>
> The `clone.rejectShallow` configuration variable ...
>
>> @@ -858,6 +862,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
>> free(remote_name);
>> remote_name = xstrdup(v);
>> }
>> + if (!strcmp(k, "clone.rejectshallow")) {
>> + config_shallow = git_config_bool(k, v);
>> + }
>
>No need to use {} around a single-statement block, especially when
>the "if" statement does not have an "else" block.
>
>The use of strcmp() against the variable name in all lowercase is
>correct here.
>
>> @@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>> */
>> git_config(git_clone_config, NULL);
>>
>> + /*
>> + * If option_shallow is specified from CLI option,
>> + * ignore config_shallow from git_clone_config.
>> + */
>> + if (config_shallow != -1) {
>> + reject_shallow = config_shallow;
>> + }
>> + if (option_shallow != -1) {
>> + reject_shallow = option_shallow;
>> + }
>
>Needless use of {} around single-statement blocks.
>
>As reject_shallow is initialized to 0, this lets the option to be of
>the most priority, then the config (presumably coming from the per-user
>or per-system configuration), by without them, defaults to false. Good.
>
>We'll have an extra git_config() call later, but that one will only
>read into config_shallow, never to be looked at because we will use
>reject_shallow variable anyway. OK.
>
>> @@ -1216,6 +1234,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>> if (filter_options.choice)
>> warning(_("--filter is ignored in local clones; use file:// instead."));
>> if (!access(mkpath("%s/shallow", path), F_OK)) {
>> + if (reject_shallow)
>> + die("source repository is shallow, reject to clone.");
>
>With the local transport, it (hopefully) is trivial to see if the
>source is shallow. OK.
>
>> if (option_local > 0)
>> warning(_("source repository is shallow, ignoring --local"));
>> is_local = 0;
>> @@ -1227,6 +1247,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>
>> transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>>
>> + if (reject_shallow)
>> + transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
>> if (option_depth)
>> transport_set_option(transport, TRANS_OPT_DEPTH,
>> option_depth);
>
>OK. What is really interesting will all happen inside the transport
>layer; the caller only has to ask for it.
>
>The asymmetry with other options like "--depth" stands out and
>puzzles readers, though.
>
True, but since `reject_shallow` is not just an option from CLI, so I think
we can't just call it "option_shallow" here to order to keep symmetry
with other options below.
>Do we really want to add the clone.rejectShallow configuration?
Only when I want all my git clone in my machine can avoid cloning
shallow repo, and no need to provide the option "--reject-shallow"
every time, then this configuration would make sense.
>After all, we do not give "clone.depth = 1" etc., and that is the
>reason why we only need "if (option_depth)" here in the near-by
>code.
>
>I'd stop here for today, hoping that somebody much more familiar
>with the transport layer than I am will review and comment on the
>changes there.
>
>Thanks.
>
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v6] builtin/clone.c: add --reject-shallow option
2021-02-28 18:06 ` [PATCH v5] " Li Linchao via GitGitGadget
2021-03-01 7:11 ` lilinchao
2021-03-03 23:21 ` Junio C Hamano
@ 2021-03-04 17:19 ` Li Linchao via GitGitGadget
2021-03-12 18:25 ` lilinchao
2021-03-25 11:09 ` [PATCH v7] " Li Linchao via GitGitGadget
2 siblings, 2 replies; 48+ messages in thread
From: Li Linchao via GitGitGadget @ 2021-03-04 17:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, dscho, Jonathan Tan, Li Linchao,
lilinchao
From: lilinchao <lilinchao@oschina.cn>
In some scenarios, users may want more history than the repository
offered for cloning, which happens to be a shallow repository, can
give them. But because users don't know it is a shallow repository
until they download it to local, users should have the option to
refuse to clone this kind of repository, and may want to exit the
process immediately without creating any unnecessary files.
Althought there is an option '--depth=x' for users to decide how
deep history they can fetch, but as the unshallow cloning's depth
is INFINITY, we can't know exactly the minimun 'x' value that can
satisfy the minimum integrity, so we can't pass 'x' value to --depth,
and expect this can obtain a complete history of a repository.
In other scenarios, if we have an API that allow us to import external
repository, and then perform various operations on the repo.
But if the imported is a shallow one(which is actually possible), it
will affect the subsequent operations. So we can choose to refuse to
clone, and let's just import a normal repository.
This patch offers a new option '--reject-shallow' that can reject to
clone a shallow repository.
Signed-off-by: lilinchao <lilinchao@oschina.cn>
---
builtin/clone.c: add --reject-shallow option
Changes since v1:
* Rename --no-shallow to --reject-shallow
* Enable to reject a non-local clone
* Enable --[no-]reject-shallow from CLI override configuration.
* Add more testcases.
* Reword commit messages and relative documentation.
Changes since v3:
* Add support to reject clone shallow repo over https protocol
* Add testcase to reject clone shallow repo over https:// transport
* Reword commit messages and relative documentation according
suggestions from Junio.
Changes since v5:
* camelcase config variable
* warning client that source repo is shallow
* better support ssh:// and git:// protocol v1, v2
Signed-off-by: lilinchao lilinchao@oschina.cn
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/865
Range-diff vs v5:
1: 3f765e49e4a7 ! 1: 953122588ca8 builtin/clone.c: add --reject-shallow option
@@ Documentation/config/clone.txt: clone.defaultRemoteName::
`origin`, and can be overridden by passing the `--origin` command-line
option to linkgit:git-clone[1].
+
-+clone.rejectshallow::
++clone.rejectShallow::
+ Reject to clone a repository if it is a shallow one, can be overridden by
+ passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
@@ Documentation/git-clone.txt: objects from the source repository into a pack in t
No checkout of HEAD is performed after the clone is complete.
+--[no-]reject-shallow::
-+ Fail if the source repository is a shallow repository. The
-+ 'clone.rejectshallow' configuration variable can be used to
++ Fail if the source repository is a shallow repository.
++ The 'clone.rejectShallow' configuration variable can be used to
+ give the default.
+
--bare::
@@ builtin/clone.c: static int git_clone_config(const char *k, const char *v, void
free(remote_name);
remote_name = xstrdup(v);
}
-+ if (!strcmp(k, "clone.rejectshallow")) {
++ if (!strcmp(k, "clone.rejectshallow"))
+ config_shallow = git_config_bool(k, v);
-+ }
++
return git_default_config(k, v, cb);
}
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
+ * If option_shallow is specified from CLI option,
+ * ignore config_shallow from git_clone_config.
+ */
-+ if (config_shallow != -1) {
++ if (config_shallow != -1)
+ reject_shallow = config_shallow;
-+ }
-+ if (option_shallow != -1) {
++
++ if (option_shallow != -1)
+ reject_shallow = option_shallow;
-+ }
++
/*
* apply the remote name provided by --origin only after this second
* call to git_config, to ensure it overrides all config-based values.
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
if (!access(mkpath("%s/shallow", path), F_OK)) {
+ if (reject_shallow)
+ die("source repository is shallow, reject to clone.");
++ else
++ warning("source repository is shallow.");
if (option_local > 0)
warning(_("source repository is shallow, ignoring --local"));
is_local = 0;
@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
if (args->stateless_rpc)
packet_flush(fd[1]);
-+
-+ if (!args->deepen && args->remote_shallow)
-+ die("source repository is shallow, reject to clone.");
+
if (args->deepen)
setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
NULL);
+- else if (si->nr_ours || si->nr_theirs)
++ else if (si->nr_ours || si->nr_theirs) {
++ if (args->remote_shallow)
++ die("source repository is shallow, reject to clone.");
++ else
++ warning("source repository is shallow.");
+ alternate_shallow_file = setup_temporary_shallow(si->shallow);
+- else
++ } else
+ alternate_shallow_file = NULL;
+ if (get_pack(args, fd, pack_lockfiles, NULL, sought, nr_sought,
+ &gitmodules_oids))
@@ fetch-pack.c: static void receive_shallow_info(struct fetch_pack_args *args,
- * shallow. In v0, remote refs that reach these objects are
* rejected (unless --update-shallow is set); do the same.
*/
-+ if (args->remote_shallow)
-+ die("source repository is shallow, reject to clone.");
prepare_shallow_info(si, shallows);
- if (si->nr_ours || si->nr_theirs)
+- if (si->nr_ours || si->nr_theirs)
++ if (si->nr_ours || si->nr_theirs) {
++ if (args->remote_shallow)
++ die("source repository is shallow, reject to clone.");
++ else
++ warning("source repository is shallow.");
alternate_shallow_file =
+ setup_temporary_shallow(si->shallow);
+- else
++ } else
+ alternate_shallow_file = NULL;
+ } else {
+ alternate_shallow_file = NULL;
## fetch-pack.h ##
@@ fetch-pack.h: struct fetch_pack_args {
@@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
test_commit attributes .gitattributes "" &&
rm -rf child &&
- ## transport-helper.c ##
-@@ transport-helper.c: static const char *boolean_options[] = {
- TRANS_OPT_THIN,
- TRANS_OPT_KEEP,
- TRANS_OPT_FOLLOWTAGS,
-- TRANS_OPT_DEEPEN_RELATIVE
-+ TRANS_OPT_DEEPEN_RELATIVE,
-+ TRANS_OPT_REJECT_SHALLOW
- };
-
- static int strbuf_set_helper_option(struct helper_data *data,
-
## transport.c ##
@@ transport.c: static int set_git_option(struct git_transport_options *opts,
list_objects_filter_die_if_populated(&opts->filter_options);
@@ transport.h: void transport_check_allowed(const char *type);
/* Aggressively fetch annotated tags if possible */
#define TRANS_OPT_FOLLOWTAGS "followtags"
-+/* reject shallow repo transport */
++/* Reject shallow repo transport */
+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
+
/* Accept refs that may update .git/shallow without --depth */
Documentation/config/clone.txt | 4 +++
Documentation/git-clone.txt | 7 ++++-
builtin/clone.c | 24 +++++++++++++++++
fetch-pack.c | 17 +++++++++---
fetch-pack.h | 1 +
t/t5606-clone-options.sh | 47 ++++++++++++++++++++++++++++++++++
t/t5611-clone-config.sh | 32 +++++++++++++++++++++++
transport.c | 4 +++
transport.h | 4 +++
9 files changed, 135 insertions(+), 5 deletions(-)
diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 47de36a5fedf..7bcfbd18a52a 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -2,3 +2,7 @@ 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].
+
+clone.rejectShallow::
+ Reject to clone a repository if it is a shallow one, can be overridden by
+ passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 02d9c19cec75..0adc98fa7eee 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,7 @@ SYNOPSIS
[--dissociate] [--separate-git-dir <git dir>]
[--depth <depth>] [--[no-]single-branch] [--no-tags]
[--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
- [--[no-]remote-submodules] [--jobs <n>] [--sparse]
+ [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
[--filter=<filter>] [--] <repository>
[<directory>]
@@ -149,6 +149,11 @@ objects from the source repository into a pack in the cloned repository.
--no-checkout::
No checkout of HEAD is performed after the clone is complete.
+--[no-]reject-shallow::
+ Fail if the source repository is a shallow repository.
+ The 'clone.rejectShallow' configuration variable can be used to
+ give the default.
+
--bare::
Make a 'bare' Git repository. That is, instead of
creating `<directory>` and placing the administrative
diff --git a/builtin/clone.c b/builtin/clone.c
index 51e844a2de0a..5c64837e8f7b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,8 @@ static int option_no_checkout, option_bare, option_mirror, option_single_branch
static int option_local = -1, option_no_hardlinks, option_shared;
static int option_no_tags;
static int option_shallow_submodules;
+static int option_shallow = -1; /* unspecified */
+static int config_shallow = -1; /* unspecified */
static int deepen;
static char *option_template, *option_depth, *option_since;
static char *option_origin = NULL;
@@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
OPT_BOOL(0, "progress", &option_progress,
N_("force progress reporting")),
+ OPT_BOOL(0, "reject-shallow", &option_shallow,
+ N_("don't clone shallow repository")),
OPT_BOOL('n', "no-checkout", &option_no_checkout,
N_("don't create a checkout")),
OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),
@@ -858,6 +862,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
free(remote_name);
remote_name = xstrdup(v);
}
+ if (!strcmp(k, "clone.rejectshallow"))
+ config_shallow = git_config_bool(k, v);
+
return git_default_config(k, v, cb);
}
@@ -963,6 +970,7 @@ static int path_exists(const char *path)
int cmd_clone(int argc, const char **argv, const char *prefix)
{
int is_bundle = 0, is_local;
+ int reject_shallow = 0;
const char *repo_name, *repo, *work_tree, *git_dir;
char *path, *dir, *display_repo = NULL;
int dest_exists, real_dest_exists = 0;
@@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
*/
git_config(git_clone_config, NULL);
+ /*
+ * If option_shallow is specified from CLI option,
+ * ignore config_shallow from git_clone_config.
+ */
+ if (config_shallow != -1)
+ reject_shallow = config_shallow;
+
+ if (option_shallow != -1)
+ reject_shallow = option_shallow;
+
/*
* apply the remote name provided by --origin only after this second
* call to git_config, to ensure it overrides all config-based values.
@@ -1216,6 +1234,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (filter_options.choice)
warning(_("--filter is ignored in local clones; use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
+ if (reject_shallow)
+ die("source repository is shallow, reject to clone.");
+ else
+ warning("source repository is shallow.");
if (option_local > 0)
warning(_("source repository is shallow, ignoring --local"));
is_local = 0;
@@ -1227,6 +1249,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
transport_set_option(transport, TRANS_OPT_KEEP, "yes");
+ if (reject_shallow)
+ transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
if (option_depth)
transport_set_option(transport, TRANS_OPT_DEPTH,
option_depth);
diff --git a/fetch-pack.c b/fetch-pack.c
index 0cb59acc4866..860ff45d46e7 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1126,12 +1126,17 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
if (args->stateless_rpc)
packet_flush(fd[1]);
+
if (args->deepen)
setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
NULL);
- else if (si->nr_ours || si->nr_theirs)
+ else if (si->nr_ours || si->nr_theirs) {
+ if (args->remote_shallow)
+ die("source repository is shallow, reject to clone.");
+ else
+ warning("source repository is shallow.");
alternate_shallow_file = setup_temporary_shallow(si->shallow);
- else
+ } else
alternate_shallow_file = NULL;
if (get_pack(args, fd, pack_lockfiles, NULL, sought, nr_sought,
&gitmodules_oids))
@@ -1498,10 +1503,14 @@ static void receive_shallow_info(struct fetch_pack_args *args,
* rejected (unless --update-shallow is set); do the same.
*/
prepare_shallow_info(si, shallows);
- if (si->nr_ours || si->nr_theirs)
+ if (si->nr_ours || si->nr_theirs) {
+ if (args->remote_shallow)
+ die("source repository is shallow, reject to clone.");
+ else
+ warning("source repository is shallow.");
alternate_shallow_file =
setup_temporary_shallow(si->shallow);
- else
+ } else
alternate_shallow_file = NULL;
} else {
alternate_shallow_file = NULL;
diff --git a/fetch-pack.h b/fetch-pack.h
index 736a3dae467a..6e4f8f0d738c 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -39,6 +39,7 @@ struct fetch_pack_args {
unsigned self_contained_and_connected:1;
unsigned cloning:1;
unsigned update_shallow:1;
+ unsigned remote_shallow:1;
unsigned deepen:1;
/*
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 52e5789fb050..6170d0513227 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
test_expect_success 'setup' '
@@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
'
+test_expect_success 'fail to clone http shallow repository' '
+ git clone --depth=1 --no-local parent shallow-repo &&
+ git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+
+'
+
+test_expect_success 'fail to clone shallow repository' '
+ rm -rf shallow-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+
+'
+
+test_expect_success 'fail to clone non-local shallow repository' '
+ rm -rf shallow-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+
+'
+
+test_expect_success 'clone shallow repository with --no-reject-shallow' '
+ rm -rf shallow-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ git clone --no-reject-shallow --no-local shallow-repo clone-repo
+
+'
+
+test_expect_success 'clone normal repository with --reject-shallow' '
+ rm -rf clone-repo &&
+ git clone --no-local parent normal-repo &&
+ git clone --reject-shallow --no-local normal-repo clone-repo
+
+'
+
+test_expect_success 'unspecified any configs or options' '
+ rm -rf shallow-repo clone-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ git clone shallow-repo clone-repo
+
+'
+
test_expect_success 'uses "origin" for default remote name' '
git clone parent clone-default-origin &&
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index 9f555b87ecdf..da10d3f10352 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -95,6 +95,38 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
test_cmp expect actual
'
+test_expect_success 'clone.rejectshallow=true should fail to clone' '
+ rm -rf child &&
+ git clone --depth=1 --no-local . child &&
+ test_must_fail git -c clone.rejectshallow=true clone --no-local child out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+'
+
+test_expect_success 'clone.rejectshallow=false should succeed' '
+ rm -rf child out &&
+ git clone --depth=1 --no-local . child &&
+ git -c clone.rejectshallow=false clone --no-local child out
+'
+
+test_expect_success 'clone.rejectshallow=true should succeed with normal repo' '
+ rm -rf child out &&
+ git clone --no-local . child &&
+ git -c clone.rejectshallow=true clone --no-local child out
+'
+
+test_expect_success 'option --reject-shallow override clone.rejectshallow' '
+ rm -rf child out &&
+ git clone --depth=1 --no-local . child &&
+ test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local child out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+'
+
+test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
+ rm -rf child out &&
+ git clone --depth=1 --no-local . child &&
+ git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
+'
+
test_expect_success MINGW 'clone -c core.hideDotFiles' '
test_commit attributes .gitattributes "" &&
rm -rf child &&
diff --git a/transport.c b/transport.c
index b13fab5dc3b1..34fe01221ee0 100644
--- a/transport.c
+++ b/transport.c
@@ -236,6 +236,9 @@ static int set_git_option(struct git_transport_options *opts,
list_objects_filter_die_if_populated(&opts->filter_options);
parse_list_objects_filter(&opts->filter_options, value);
return 0;
+ } else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
+ opts->reject_shallow = !!value;
+ return 0;
}
return 1;
}
@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
args.stateless_rpc = transport->stateless_rpc;
args.server_options = transport->server_options;
args.negotiation_tips = data->options.negotiation_tips;
+ args.remote_shallow = transport->smart_options->reject_shallow;
if (!data->got_remote_heads) {
int i;
diff --git a/transport.h b/transport.h
index 24e15799e714..4d5db0a7f22b 100644
--- a/transport.h
+++ b/transport.h
@@ -14,6 +14,7 @@ struct git_transport_options {
unsigned check_self_contained_and_connected : 1;
unsigned self_contained_and_connected : 1;
unsigned update_shallow : 1;
+ unsigned reject_shallow : 1;
unsigned deepen_relative : 1;
/* see documentation of corresponding flag in fetch-pack.h */
@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
/* Aggressively fetch annotated tags if possible */
#define TRANS_OPT_FOLLOWTAGS "followtags"
+/* Reject shallow repo transport */
+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
+
/* Accept refs that may update .git/shallow without --depth */
#define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
base-commit: f01623b2c9d14207e497b21ebc6b3ec4afaf4b46
--
gitgitgadget
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v6] builtin/clone.c: add --reject-shallow option
2021-03-04 17:19 ` [PATCH v6] " Li Linchao via GitGitGadget
@ 2021-03-12 18:25 ` lilinchao
2021-03-25 11:09 ` [PATCH v7] " Li Linchao via GitGitGadget
1 sibling, 0 replies; 48+ messages in thread
From: lilinchao @ 2021-03-12 18:25 UTC (permalink / raw)
To: Li Linchao via GitGitGadget, git
Cc: Junio C Hamano, Derrick Stolee, dscho, Jonathan Tan
PING.
Seems that this patch have been forgotten by everyone.
--------------
>From: lilinchao <lilinchao@oschina.cn>
>
>In some scenarios, users may want more history than the repository
>offered for cloning, which happens to be a shallow repository, can
>give them. But because users don't know it is a shallow repository
>until they download it to local, users should have the option to
>refuse to clone this kind of repository, and may want to exit the
>process immediately without creating any unnecessary files.
>
>Althought there is an option '--depth=x' for users to decide how
>deep history they can fetch, but as the unshallow cloning's depth
>is INFINITY, we can't know exactly the minimun 'x' value that can
>satisfy the minimum integrity, so we can't pass 'x' value to --depth,
>and expect this can obtain a complete history of a repository.
>
>In other scenarios, if we have an API that allow us to import external
>repository, and then perform various operations on the repo.
>But if the imported is a shallow one(which is actually possible), it
>will affect the subsequent operations. So we can choose to refuse to
>clone, and let's just import a normal repository.
>
>This patch offers a new option '--reject-shallow' that can reject to
>clone a shallow repository.
>
>Signed-off-by: lilinchao <lilinchao@oschina.cn>
>---
> builtin/clone.c: add --reject-shallow option
>
> Changes since v1:
>
> * Rename --no-shallow to --reject-shallow
> * Enable to reject a non-local clone
> * Enable --[no-]reject-shallow from CLI override configuration.
> * Add more testcases.
> * Reword commit messages and relative documentation.
>
> Changes since v3:
>
> * Add support to reject clone shallow repo over https protocol
> * Add testcase to reject clone shallow repo over https:// transport
> * Reword commit messages and relative documentation according
> suggestions from Junio.
>
> Changes since v5:
>
> * camelcase config variable
> * warning client that source repo is shallow
> * better support ssh:// and git:// protocol v1, v2
>
> Signed-off-by: lilinchao lilinchao@oschina.cn
>
>Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v6
>Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v6
>Pull-Request: https://github.com/gitgitgadget/git/pull/865
>
>Range-diff vs v5:
>
> 1: 3f765e49e4a7 ! 1: 953122588ca8 builtin/clone.c: add --reject-shallow option
> @@ Documentation/config/clone.txt: clone.defaultRemoteName::
> `origin`, and can be overridden by passing the `--origin` command-line
> option to linkgit:git-clone[1].
> +
> -+clone.rejectshallow::
> ++clone.rejectShallow::
> + Reject to clone a repository if it is a shallow one, can be overridden by
> + passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
>
> @@ Documentation/git-clone.txt: objects from the source repository into a pack in t
> No checkout of HEAD is performed after the clone is complete.
>
> +--[no-]reject-shallow::
> -+ Fail if the source repository is a shallow repository. The
> -+ 'clone.rejectshallow' configuration variable can be used to
> ++ Fail if the source repository is a shallow repository.
> ++ The 'clone.rejectShallow' configuration variable can be used to
> + give the default.
> +
> --bare::
> @@ builtin/clone.c: static int git_clone_config(const char *k, const char *v, void
> free(remote_name);
> remote_name = xstrdup(v);
> }
> -+ if (!strcmp(k, "clone.rejectshallow")) {
> ++ if (!strcmp(k, "clone.rejectshallow"))
> + config_shallow = git_config_bool(k, v);
> -+ }
> ++
> return git_default_config(k, v, cb);
> }
>
> @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
> + * If option_shallow is specified from CLI option,
> + * ignore config_shallow from git_clone_config.
> + */
> -+ if (config_shallow != -1) {
> ++ if (config_shallow != -1)
> + reject_shallow = config_shallow;
> -+ }
> -+ if (option_shallow != -1) {
> ++
> ++ if (option_shallow != -1)
> + reject_shallow = option_shallow;
> -+ }
> ++
> /*
> * apply the remote name provided by --origin only after this second
> * call to git_config, to ensure it overrides all config-based values.
> @@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
> if (!access(mkpath("%s/shallow", path), F_OK)) {
> + if (reject_shallow)
> + die("source repository is shallow, reject to clone.");
> ++ else
> ++ warning("source repository is shallow.");
> if (option_local > 0)
> warning(_("source repository is shallow, ignoring --local"));
> is_local = 0;
> @@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>
> if (args->stateless_rpc)
> packet_flush(fd[1]);
> -+
> -+ if (!args->deepen && args->remote_shallow)
> -+ die("source repository is shallow, reject to clone.");
> +
> if (args->deepen)
> setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
> NULL);
> +- else if (si->nr_ours || si->nr_theirs)
> ++ else if (si->nr_ours || si->nr_theirs) {
> ++ if (args->remote_shallow)
> ++ die("source repository is shallow, reject to clone.");
> ++ else
> ++ warning("source repository is shallow.");
> + alternate_shallow_file = setup_temporary_shallow(si->shallow);
> +- else
> ++ } else
> + alternate_shallow_file = NULL;
> + if (get_pack(args, fd, pack_lockfiles, NULL, sought, nr_sought,
> + &gitmodules_oids))
> @@ fetch-pack.c: static void receive_shallow_info(struct fetch_pack_args *args,
> - * shallow. In v0, remote refs that reach these objects are
> * rejected (unless --update-shallow is set); do the same.
> */
> -+ if (args->remote_shallow)
> -+ die("source repository is shallow, reject to clone.");
> prepare_shallow_info(si, shallows);
> - if (si->nr_ours || si->nr_theirs)
> +- if (si->nr_ours || si->nr_theirs)
> ++ if (si->nr_ours || si->nr_theirs) {
> ++ if (args->remote_shallow)
> ++ die("source repository is shallow, reject to clone.");
> ++ else
> ++ warning("source repository is shallow.");
> alternate_shallow_file =
> + setup_temporary_shallow(si->shallow);
> +- else
> ++ } else
> + alternate_shallow_file = NULL;
> + } else {
> + alternate_shallow_file = NULL;
>
> ## fetch-pack.h ##
> @@ fetch-pack.h: struct fetch_pack_args {
> @@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
> test_commit attributes .gitattributes "" &&
> rm -rf child &&
>
> - ## transport-helper.c ##
> -@@ transport-helper.c: static const char *boolean_options[] = {
> - TRANS_OPT_THIN,
> - TRANS_OPT_KEEP,
> - TRANS_OPT_FOLLOWTAGS,
> -- TRANS_OPT_DEEPEN_RELATIVE
> -+ TRANS_OPT_DEEPEN_RELATIVE,
> -+ TRANS_OPT_REJECT_SHALLOW
> - };
> -
> - static int strbuf_set_helper_option(struct helper_data *data,
> -
> ## transport.c ##
> @@ transport.c: static int set_git_option(struct git_transport_options *opts,
> list_objects_filter_die_if_populated(&opts->filter_options);
> @@ transport.h: void transport_check_allowed(const char *type);
> /* Aggressively fetch annotated tags if possible */
> #define TRANS_OPT_FOLLOWTAGS "followtags"
>
> -+/* reject shallow repo transport */
> ++/* Reject shallow repo transport */
> +#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
> +
> /* Accept refs that may update .git/shallow without --depth */
>
>
> Documentation/config/clone.txt | 4 +++
> Documentation/git-clone.txt | 7 ++++-
> builtin/clone.c | 24 +++++++++++++++++
> fetch-pack.c | 17 +++++++++---
> fetch-pack.h | 1 +
> t/t5606-clone-options.sh | 47 ++++++++++++++++++++++++++++++++++
> t/t5611-clone-config.sh | 32 +++++++++++++++++++++++
> transport.c | 4 +++
> transport.h | 4 +++
> 9 files changed, 135 insertions(+), 5 deletions(-)
>
>diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
>index 47de36a5fedf..7bcfbd18a52a 100644
>--- a/Documentation/config/clone.txt
>+++ b/Documentation/config/clone.txt
>@@ -2,3 +2,7 @@ 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].
>+
>+clone.rejectShallow::
>+ Reject to clone a repository if it is a shallow one, can be overridden by
>+ passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
>diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
>index 02d9c19cec75..0adc98fa7eee 100644
>--- a/Documentation/git-clone.txt
>+++ b/Documentation/git-clone.txt
>@@ -15,7 +15,7 @@ SYNOPSIS
> [--dissociate] [--separate-git-dir <git dir>]
> [--depth <depth>] [--[no-]single-branch] [--no-tags]
> [--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
>- [--[no-]remote-submodules] [--jobs <n>] [--sparse]
>+ [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
> [--filter=<filter>] [--] <repository>
> [<directory>]
>
>@@ -149,6 +149,11 @@ objects from the source repository into a pack in the cloned repository.
> --no-checkout::
> No checkout of HEAD is performed after the clone is complete.
>
>+--[no-]reject-shallow::
>+ Fail if the source repository is a shallow repository.
>+ The 'clone.rejectShallow' configuration variable can be used to
>+ give the default.
>+
> --bare::
> Make a 'bare' Git repository. That is, instead of
> creating `<directory>` and placing the administrative
>diff --git a/builtin/clone.c b/builtin/clone.c
>index 51e844a2de0a..5c64837e8f7b 100644
>--- a/builtin/clone.c
>+++ b/builtin/clone.c
>@@ -50,6 +50,8 @@ static int option_no_checkout, option_bare, option_mirror, option_single_branch
> static int option_local = -1, option_no_hardlinks, option_shared;
> static int option_no_tags;
> static int option_shallow_submodules;
>+static int option_shallow = -1; /* unspecified */
>+static int config_shallow = -1; /* unspecified */
> static int deepen;
> static char *option_template, *option_depth, *option_since;
> static char *option_origin = NULL;
>@@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
> OPT__VERBOSITY(&option_verbosity),
> OPT_BOOL(0, "progress", &option_progress,
> N_("force progress reporting")),
>+ OPT_BOOL(0, "reject-shallow", &option_shallow,
>+ N_("don't clone shallow repository")),
> OPT_BOOL('n', "no-checkout", &option_no_checkout,
> N_("don't create a checkout")),
> OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),
>@@ -858,6 +862,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
> free(remote_name);
> remote_name = xstrdup(v);
> }
>+ if (!strcmp(k, "clone.rejectshallow"))
>+ config_shallow = git_config_bool(k, v);
>+
> return git_default_config(k, v, cb);
> }
>
>@@ -963,6 +970,7 @@ static int path_exists(const char *path)
> int cmd_clone(int argc, const char **argv, const char *prefix)
> {
> int is_bundle = 0, is_local;
>+ int reject_shallow = 0;
> const char *repo_name, *repo, *work_tree, *git_dir;
> char *path, *dir, *display_repo = NULL;
> int dest_exists, real_dest_exists = 0;
>@@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> */
> git_config(git_clone_config, NULL);
>
>+ /*
>+ * If option_shallow is specified from CLI option,
>+ * ignore config_shallow from git_clone_config.
>+ */
>+ if (config_shallow != -1)
>+ reject_shallow = config_shallow;
>+
>+ if (option_shallow != -1)
>+ reject_shallow = option_shallow;
>+
> /*
> * apply the remote name provided by --origin only after this second
> * call to git_config, to ensure it overrides all config-based values.
>@@ -1216,6 +1234,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> if (filter_options.choice)
> warning(_("--filter is ignored in local clones; use file:// instead."));
> if (!access(mkpath("%s/shallow", path), F_OK)) {
>+ if (reject_shallow)
>+ die("source repository is shallow, reject to clone.");
>+ else
>+ warning("source repository is shallow.");
> if (option_local > 0)
> warning(_("source repository is shallow, ignoring --local"));
> is_local = 0;
>@@ -1227,6 +1249,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>
> transport_set_option(transport, TRANS_OPT_KEEP, "yes");
>
>+ if (reject_shallow)
>+ transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
> if (option_depth)
> transport_set_option(transport, TRANS_OPT_DEPTH,
> option_depth);
>diff --git a/fetch-pack.c b/fetch-pack.c
>index 0cb59acc4866..860ff45d46e7 100644
>--- a/fetch-pack.c
>+++ b/fetch-pack.c
>@@ -1126,12 +1126,17 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>
> if (args->stateless_rpc)
> packet_flush(fd[1]);
>+
> if (args->deepen)
> setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
> NULL);
>- else if (si->nr_ours || si->nr_theirs)
>+ else if (si->nr_ours || si->nr_theirs) {
>+ if (args->remote_shallow)
>+ die("source repository is shallow, reject to clone.");
>+ else
>+ warning("source repository is shallow.");
> alternate_shallow_file = setup_temporary_shallow(si->shallow);
>- else
>+ } else
> alternate_shallow_file = NULL;
> if (get_pack(args, fd, pack_lockfiles, NULL, sought, nr_sought,
> &gitmodules_oids))
>@@ -1498,10 +1503,14 @@ static void receive_shallow_info(struct fetch_pack_args *args,
> * rejected (unless --update-shallow is set); do the same.
> */
> prepare_shallow_info(si, shallows);
>- if (si->nr_ours || si->nr_theirs)
>+ if (si->nr_ours || si->nr_theirs) {
>+ if (args->remote_shallow)
>+ die("source repository is shallow, reject to clone.");
>+ else
>+ warning("source repository is shallow.");
> alternate_shallow_file =
> setup_temporary_shallow(si->shallow);
>- else
>+ } else
> alternate_shallow_file = NULL;
> } else {
> alternate_shallow_file = NULL;
>diff --git a/fetch-pack.h b/fetch-pack.h
>index 736a3dae467a..6e4f8f0d738c 100644
>--- a/fetch-pack.h
>+++ b/fetch-pack.h
>@@ -39,6 +39,7 @@ struct fetch_pack_args {
> unsigned self_contained_and_connected:1;
> unsigned cloning:1;
> unsigned update_shallow:1;
>+ unsigned remote_shallow:1;
> unsigned deepen:1;
>
> /*
>diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
>index 52e5789fb050..6170d0513227 100755
>--- a/t/t5606-clone-options.sh
>+++ b/t/t5606-clone-options.sh
>@@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> . ./test-lib.sh
>+. "$TEST_DIRECTORY"/lib-httpd.sh
>+start_httpd
>
> test_expect_success 'setup' '
>
>@@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>
> '
>
>+test_expect_success 'fail to clone http shallow repository' '
>+ git clone --depth=1 --no-local parent shallow-repo &&
>+ git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
>+ test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git out 2>err &&
>+ test_i18ngrep -e "source repository is shallow, reject to clone." err
>+
>+'
>+
>+test_expect_success 'fail to clone shallow repository' '
>+ rm -rf shallow-repo &&
>+ git clone --depth=1 --no-local parent shallow-repo &&
>+ test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
>+ test_i18ngrep -e "source repository is shallow, reject to clone." err
>+
>+'
>+
>+test_expect_success 'fail to clone non-local shallow repository' '
>+ rm -rf shallow-repo &&
>+ git clone --depth=1 --no-local parent shallow-repo &&
>+ test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
>+ test_i18ngrep -e "source repository is shallow, reject to clone." err
>+
>+'
>+
>+test_expect_success 'clone shallow repository with --no-reject-shallow' '
>+ rm -rf shallow-repo &&
>+ git clone --depth=1 --no-local parent shallow-repo &&
>+ git clone --no-reject-shallow --no-local shallow-repo clone-repo
>+
>+'
>+
>+test_expect_success 'clone normal repository with --reject-shallow' '
>+ rm -rf clone-repo &&
>+ git clone --no-local parent normal-repo &&
>+ git clone --reject-shallow --no-local normal-repo clone-repo
>+
>+'
>+
>+test_expect_success 'unspecified any configs or options' '
>+ rm -rf shallow-repo clone-repo &&
>+ git clone --depth=1 --no-local parent shallow-repo &&
>+ git clone shallow-repo clone-repo
>+
>+'
>+
> test_expect_success 'uses "origin" for default remote name' '
>
> git clone parent clone-default-origin &&
>diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
>index 9f555b87ecdf..da10d3f10352 100755
>--- a/t/t5611-clone-config.sh
>+++ b/t/t5611-clone-config.sh
>@@ -95,6 +95,38 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
> test_cmp expect actual
> '
>
>+test_expect_success 'clone.rejectshallow=true should fail to clone' '
>+ rm -rf child &&
>+ git clone --depth=1 --no-local . child &&
>+ test_must_fail git -c clone.rejectshallow=true clone --no-local child out 2>err &&
>+ test_i18ngrep -e "source repository is shallow, reject to clone." err
>+'
>+
>+test_expect_success 'clone.rejectshallow=false should succeed' '
>+ rm -rf child out &&
>+ git clone --depth=1 --no-local . child &&
>+ git -c clone.rejectshallow=false clone --no-local child out
>+'
>+
>+test_expect_success 'clone.rejectshallow=true should succeed with normal repo' '
>+ rm -rf child out &&
>+ git clone --no-local . child &&
>+ git -c clone.rejectshallow=true clone --no-local child out
>+'
>+
>+test_expect_success 'option --reject-shallow override clone.rejectshallow' '
>+ rm -rf child out &&
>+ git clone --depth=1 --no-local . child &&
>+ test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local child out 2>err &&
>+ test_i18ngrep -e "source repository is shallow, reject to clone." err
>+'
>+
>+test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
>+ rm -rf child out &&
>+ git clone --depth=1 --no-local . child &&
>+ git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
>+'
>+
> test_expect_success MINGW 'clone -c core.hideDotFiles' '
> test_commit attributes .gitattributes "" &&
> rm -rf child &&
>diff --git a/transport.c b/transport.c
>index b13fab5dc3b1..34fe01221ee0 100644
>--- a/transport.c
>+++ b/transport.c
>@@ -236,6 +236,9 @@ static int set_git_option(struct git_transport_options *opts,
> list_objects_filter_die_if_populated(&opts->filter_options);
> parse_list_objects_filter(&opts->filter_options, value);
> return 0;
>+ } else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
>+ opts->reject_shallow = !!value;
>+ return 0;
> }
> return 1;
> }
>@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> args.stateless_rpc = transport->stateless_rpc;
> args.server_options = transport->server_options;
> args.negotiation_tips = data->options.negotiation_tips;
>+ args.remote_shallow = transport->smart_options->reject_shallow;
>
> if (!data->got_remote_heads) {
> int i;
>diff --git a/transport.h b/transport.h
>index 24e15799e714..4d5db0a7f22b 100644
>--- a/transport.h
>+++ b/transport.h
>@@ -14,6 +14,7 @@ struct git_transport_options {
> unsigned check_self_contained_and_connected : 1;
> unsigned self_contained_and_connected : 1;
> unsigned update_shallow : 1;
>+ unsigned reject_shallow : 1;
> unsigned deepen_relative : 1;
>
> /* see documentation of corresponding flag in fetch-pack.h */
>@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
> /* Aggressively fetch annotated tags if possible */
> #define TRANS_OPT_FOLLOWTAGS "followtags"
>
>+/* Reject shallow repo transport */
>+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
>+
> /* Accept refs that may update .git/shallow without --depth */
> #define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
>
>
>base-commit: f01623b2c9d14207e497b21ebc6b3ec4afaf4b46
>--
>gitgitgadget
>
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH v7] builtin/clone.c: add --reject-shallow option
2021-03-04 17:19 ` [PATCH v6] " Li Linchao via GitGitGadget
2021-03-12 18:25 ` lilinchao
@ 2021-03-25 11:09 ` Li Linchao via GitGitGadget
2021-03-25 20:31 ` Junio C Hamano
` (4 more replies)
1 sibling, 5 replies; 48+ messages in thread
From: Li Linchao via GitGitGadget @ 2021-03-25 11:09 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, dscho, Jonathan Tan, Li Linchao,
lilinchao
From: lilinchao <lilinchao@oschina.cn>
In some scenarios, users may want more history than the repository
offered for cloning, which happens to be a shallow repository, can
give them. But because users don't know it is a shallow repository
until they download it to local, users should have the option to
refuse to clone this kind of repository, and may want to exit the
process immediately without creating any unnecessary files.
Althought there is an option '--depth=x' for users to decide how
deep history they can fetch, but as the unshallow cloning's depth
is INFINITY, we can't know exactly the minimun 'x' value that can
satisfy the minimum integrity, so we can't pass 'x' value to --depth,
and expect this can obtain a complete history of a repository.
In other scenarios, if we have an API that allow us to import external
repository, and then perform various operations on the repo.
But if the imported is a shallow one(which is actually possible), it
will affect the subsequent operations. So we can choose to refuse to
clone, and let's just import a normal repository.
This patch offers a new option '--reject-shallow' that can reject to
clone a shallow repository.
Signed-off-by: lilinchao <lilinchao@oschina.cn>
Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
Reviewed-by: Junio C Hamano<gitster@pobox.com>
Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Reviewed-by: Jonathan Tan<jonathantanmy@google.com>
---
builtin/clone.c: add --reject-shallow option
Changes since v1:
* Rename --no-shallow to --reject-shallow
* Enable to reject a non-local clone
* Enable --[no-]reject-shallow from CLI override configuration.
* Add more testcases.
* Reword commit messages and relative documentation.
Changes since v3:
* Add support to reject clone shallow repo over https protocol
* Add testcase to reject clone shallow repo over https:// transport
* Reword commit messages and relative documentation according
suggestions from Junio.
Changes since v5:
* camelcase config variable
* warning client that source repo is shallow
* better support ssh:// and git:// protocol v1, v2
Changes since v6:
* use _() for warning/die statement
Signed-off-by: lilinchao lilinchao@oschina.cn
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v7
Pull-Request: https://github.com/gitgitgadget/git/pull/865
Range-diff vs v6:
1: 953122588ca8 ! 1: b957b77bfd79 builtin/clone.c: add --reject-shallow option
@@ Commit message
Signed-off-by: lilinchao <lilinchao@oschina.cn>
+ Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
+ Reviewed-by: Junio C Hamano<gitster@pobox.com>
+ Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
+ Reviewed-by: Jonathan Tan<jonathantanmy@google.com>
+
## Documentation/config/clone.txt ##
@@ Documentation/config/clone.txt: clone.defaultRemoteName::
The name of the remote to create when cloning a repository. Defaults to
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
warning(_("--filter is ignored in local clones; use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
+ if (reject_shallow)
-+ die("source repository is shallow, reject to clone.");
++ die(_("source repository is shallow, reject to clone."));
+ else
-+ warning("source repository is shallow.");
++ warning(_("source repository is shallow."));
if (option_local > 0)
warning(_("source repository is shallow, ignoring --local"));
is_local = 0;
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
## fetch-pack.c ##
@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
-
- if (args->stateless_rpc)
- packet_flush(fd[1]);
-+
if (args->deepen)
setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
NULL);
- else if (si->nr_ours || si->nr_theirs)
+ else if (si->nr_ours || si->nr_theirs) {
+ if (args->remote_shallow)
-+ die("source repository is shallow, reject to clone.");
++ die(_("source repository is shallow, reject to clone."));
+ else
-+ warning("source repository is shallow.");
++ warning(_("source repository is shallow."));
alternate_shallow_file = setup_temporary_shallow(si->shallow);
- else
+ } else
@@ fetch-pack.c: static void receive_shallow_info(struct fetch_pack_args *args,
- if (si->nr_ours || si->nr_theirs)
+ if (si->nr_ours || si->nr_theirs) {
+ if (args->remote_shallow)
-+ die("source repository is shallow, reject to clone.");
++ die(_("source repository is shallow, reject to clone."));
+ else
-+ warning("source repository is shallow.");
++ warning(_("source repository is shallow."));
alternate_shallow_file =
setup_temporary_shallow(si->shallow);
- else
Documentation/config/clone.txt | 4 +++
Documentation/git-clone.txt | 7 ++++-
builtin/clone.c | 24 +++++++++++++++++
fetch-pack.c | 16 +++++++++---
fetch-pack.h | 1 +
t/t5606-clone-options.sh | 47 ++++++++++++++++++++++++++++++++++
t/t5611-clone-config.sh | 32 +++++++++++++++++++++++
transport.c | 4 +++
transport.h | 4 +++
9 files changed, 134 insertions(+), 5 deletions(-)
diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 47de36a5fedf..7bcfbd18a52a 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -2,3 +2,7 @@ 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].
+
+clone.rejectShallow::
+ Reject to clone a repository if it is a shallow one, can be overridden by
+ passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 02d9c19cec75..0adc98fa7eee 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,7 @@ SYNOPSIS
[--dissociate] [--separate-git-dir <git dir>]
[--depth <depth>] [--[no-]single-branch] [--no-tags]
[--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
- [--[no-]remote-submodules] [--jobs <n>] [--sparse]
+ [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
[--filter=<filter>] [--] <repository>
[<directory>]
@@ -149,6 +149,11 @@ objects from the source repository into a pack in the cloned repository.
--no-checkout::
No checkout of HEAD is performed after the clone is complete.
+--[no-]reject-shallow::
+ Fail if the source repository is a shallow repository.
+ The 'clone.rejectShallow' configuration variable can be used to
+ give the default.
+
--bare::
Make a 'bare' Git repository. That is, instead of
creating `<directory>` and placing the administrative
diff --git a/builtin/clone.c b/builtin/clone.c
index 51e844a2de0a..a1956b0c9c6f 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,8 @@ static int option_no_checkout, option_bare, option_mirror, option_single_branch
static int option_local = -1, option_no_hardlinks, option_shared;
static int option_no_tags;
static int option_shallow_submodules;
+static int option_shallow = -1; /* unspecified */
+static int config_shallow = -1; /* unspecified */
static int deepen;
static char *option_template, *option_depth, *option_since;
static char *option_origin = NULL;
@@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
OPT_BOOL(0, "progress", &option_progress,
N_("force progress reporting")),
+ OPT_BOOL(0, "reject-shallow", &option_shallow,
+ N_("don't clone shallow repository")),
OPT_BOOL('n', "no-checkout", &option_no_checkout,
N_("don't create a checkout")),
OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),
@@ -858,6 +862,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
free(remote_name);
remote_name = xstrdup(v);
}
+ if (!strcmp(k, "clone.rejectshallow"))
+ config_shallow = git_config_bool(k, v);
+
return git_default_config(k, v, cb);
}
@@ -963,6 +970,7 @@ static int path_exists(const char *path)
int cmd_clone(int argc, const char **argv, const char *prefix)
{
int is_bundle = 0, is_local;
+ int reject_shallow = 0;
const char *repo_name, *repo, *work_tree, *git_dir;
char *path, *dir, *display_repo = NULL;
int dest_exists, real_dest_exists = 0;
@@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
*/
git_config(git_clone_config, NULL);
+ /*
+ * If option_shallow is specified from CLI option,
+ * ignore config_shallow from git_clone_config.
+ */
+ if (config_shallow != -1)
+ reject_shallow = config_shallow;
+
+ if (option_shallow != -1)
+ reject_shallow = option_shallow;
+
/*
* apply the remote name provided by --origin only after this second
* call to git_config, to ensure it overrides all config-based values.
@@ -1216,6 +1234,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (filter_options.choice)
warning(_("--filter is ignored in local clones; use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
+ if (reject_shallow)
+ die(_("source repository is shallow, reject to clone."));
+ else
+ warning(_("source repository is shallow."));
if (option_local > 0)
warning(_("source repository is shallow, ignoring --local"));
is_local = 0;
@@ -1227,6 +1249,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
transport_set_option(transport, TRANS_OPT_KEEP, "yes");
+ if (reject_shallow)
+ transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
if (option_depth)
transport_set_option(transport, TRANS_OPT_DEPTH,
option_depth);
diff --git a/fetch-pack.c b/fetch-pack.c
index fb04a76ca263..72b378449a07 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1129,9 +1129,13 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
if (args->deepen)
setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
NULL);
- else if (si->nr_ours || si->nr_theirs)
+ else if (si->nr_ours || si->nr_theirs) {
+ if (args->remote_shallow)
+ die(_("source repository is shallow, reject to clone."));
+ else
+ warning(_("source repository is shallow."));
alternate_shallow_file = setup_temporary_shallow(si->shallow);
- else
+ } else
alternate_shallow_file = NULL;
if (get_pack(args, fd, pack_lockfiles, NULL, sought, nr_sought,
&gitmodules_oids))
@@ -1498,10 +1502,14 @@ static void receive_shallow_info(struct fetch_pack_args *args,
* rejected (unless --update-shallow is set); do the same.
*/
prepare_shallow_info(si, shallows);
- if (si->nr_ours || si->nr_theirs)
+ if (si->nr_ours || si->nr_theirs) {
+ if (args->remote_shallow)
+ die(_("source repository is shallow, reject to clone."));
+ else
+ warning(_("source repository is shallow."));
alternate_shallow_file =
setup_temporary_shallow(si->shallow);
- else
+ } else
alternate_shallow_file = NULL;
} else {
alternate_shallow_file = NULL;
diff --git a/fetch-pack.h b/fetch-pack.h
index 736a3dae467a..6e4f8f0d738c 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -39,6 +39,7 @@ struct fetch_pack_args {
unsigned self_contained_and_connected:1;
unsigned cloning:1;
unsigned update_shallow:1;
+ unsigned remote_shallow:1;
unsigned deepen:1;
/*
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 428b0aac93fa..2863b8b28d44 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
test_expect_success 'setup' '
@@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
'
+test_expect_success 'fail to clone http shallow repository' '
+ git clone --depth=1 --no-local parent shallow-repo &&
+ git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+
+'
+
+test_expect_success 'fail to clone shallow repository' '
+ rm -rf shallow-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+
+'
+
+test_expect_success 'fail to clone non-local shallow repository' '
+ rm -rf shallow-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+
+'
+
+test_expect_success 'clone shallow repository with --no-reject-shallow' '
+ rm -rf shallow-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ git clone --no-reject-shallow --no-local shallow-repo clone-repo
+
+'
+
+test_expect_success 'clone normal repository with --reject-shallow' '
+ rm -rf clone-repo &&
+ git clone --no-local parent normal-repo &&
+ git clone --reject-shallow --no-local normal-repo clone-repo
+
+'
+
+test_expect_success 'unspecified any configs or options' '
+ rm -rf shallow-repo clone-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ git clone shallow-repo clone-repo
+
+'
+
test_expect_success 'uses "origin" for default remote name' '
git clone parent clone-default-origin &&
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index 9f555b87ecdf..da10d3f10352 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -95,6 +95,38 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
test_cmp expect actual
'
+test_expect_success 'clone.rejectshallow=true should fail to clone' '
+ rm -rf child &&
+ git clone --depth=1 --no-local . child &&
+ test_must_fail git -c clone.rejectshallow=true clone --no-local child out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+'
+
+test_expect_success 'clone.rejectshallow=false should succeed' '
+ rm -rf child out &&
+ git clone --depth=1 --no-local . child &&
+ git -c clone.rejectshallow=false clone --no-local child out
+'
+
+test_expect_success 'clone.rejectshallow=true should succeed with normal repo' '
+ rm -rf child out &&
+ git clone --no-local . child &&
+ git -c clone.rejectshallow=true clone --no-local child out
+'
+
+test_expect_success 'option --reject-shallow override clone.rejectshallow' '
+ rm -rf child out &&
+ git clone --depth=1 --no-local . child &&
+ test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local child out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+'
+
+test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
+ rm -rf child out &&
+ git clone --depth=1 --no-local . child &&
+ git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
+'
+
test_expect_success MINGW 'clone -c core.hideDotFiles' '
test_commit attributes .gitattributes "" &&
rm -rf child &&
diff --git a/transport.c b/transport.c
index 1c4ab676d1b1..a6b9f404d86a 100644
--- a/transport.c
+++ b/transport.c
@@ -236,6 +236,9 @@ static int set_git_option(struct git_transport_options *opts,
list_objects_filter_die_if_populated(&opts->filter_options);
parse_list_objects_filter(&opts->filter_options, value);
return 0;
+ } else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
+ opts->reject_shallow = !!value;
+ return 0;
}
return 1;
}
@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
args.stateless_rpc = transport->stateless_rpc;
args.server_options = transport->server_options;
args.negotiation_tips = data->options.negotiation_tips;
+ args.remote_shallow = transport->smart_options->reject_shallow;
if (!data->got_remote_heads) {
int i;
diff --git a/transport.h b/transport.h
index 24e15799e714..4d5db0a7f22b 100644
--- a/transport.h
+++ b/transport.h
@@ -14,6 +14,7 @@ struct git_transport_options {
unsigned check_self_contained_and_connected : 1;
unsigned self_contained_and_connected : 1;
unsigned update_shallow : 1;
+ unsigned reject_shallow : 1;
unsigned deepen_relative : 1;
/* see documentation of corresponding flag in fetch-pack.h */
@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
/* Aggressively fetch annotated tags if possible */
#define TRANS_OPT_FOLLOWTAGS "followtags"
+/* Reject shallow repo transport */
+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
+
/* Accept refs that may update .git/shallow without --depth */
#define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
base-commit: 9198c13e34f6d51c983b31a9397d4d62bc2147ac
--
gitgitgadget
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v7] builtin/clone.c: add --reject-shallow option
2021-03-25 11:09 ` [PATCH v7] " Li Linchao via GitGitGadget
@ 2021-03-25 20:31 ` Junio C Hamano
2021-03-25 22:57 ` Junio C Hamano
` (3 subsequent siblings)
4 siblings, 0 replies; 48+ messages in thread
From: Junio C Hamano @ 2021-03-25 20:31 UTC (permalink / raw)
To: Li Linchao via GitGitGadget
Cc: git, Derrick Stolee, dscho, Jonathan Tan, Li Linchao
"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
> ...
> This patch offers a new option '--reject-shallow' that can reject to
> clone a shallow repository.
>
> Signed-off-by: lilinchao <lilinchao@oschina.cn>
>
> Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
> Reviewed-by: Junio C Hamano<gitster@pobox.com>
> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> Reviewed-by: Jonathan Tan<jonathantanmy@google.com>
The Reviewed-by trailer means something quite different from what
you seem to think here. It is only given by the reviewers to the
patch when they carefully reviewed and agrees what is in the patch.
The patch authors are in no position to add it, unless they are
explicitly told by reviewers that "this patch now can have my
Reviewed-by:" or some equivalent.
The (ideal) flow of events is
0. The author comes up with an idea and writes a patch.
1. The patch is sent to the list and Cc'ed to people who may be
familiar with the area the patch touches. For second and
subsequent iterations, those who gave review comments to the
previous iterations are also good people to Cc to.
2. People give comments as reponses to the patch.
(a) some may be happy with the iteration of the patch they
reviewed, and may say "Thanks for contributing, this is now
Reviewed-by: me". For second and subsequent iterations,
they may say "This was improved relative to the previous
iteration, and it still looks good and you have my
Reviewed-by:".
(b) some may give constructive criticism, alternatives,
enhancements, or outright "not a good idea, don't do this
because ...".
(c) some may just act as cheerleaders.
3. The author thinks about the review comments and also may find
improvement him/herself.
(a) There may need an update to the patch. If the patch has
changed since the previous version in any way, ignore
Reviewed-by: received in 2-(a). When a significant help was
given to update the patch, you may add "Helped-by:" trailer
to credit the person's contribution.
Your own "Signed-off-by:" appears the last in the trailers
(i.e. "this iteration of the patch was written with help
from these people, and then I am signing it off just before
sending it out").
Go back to 1. and repeat as many times as it takes.
(b) There may not be a need for any update to the patch. Only
add the Reviewed-by: received in 2-(a) and otherwise do not
change anything. Your own "Signed-off-by:" appears the last
in the trailers. Send it to the list and to the maintainer
(me).
4. The maintainer applies the patch, unless there is no other
comments received on that supposedly-the-final version sent in
3-(b), but a late review comment may make us realize that it was
premature, in which case we may go back to 3-(a).
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v7] builtin/clone.c: add --reject-shallow option
2021-03-25 11:09 ` [PATCH v7] " Li Linchao via GitGitGadget
2021-03-25 20:31 ` Junio C Hamano
@ 2021-03-25 22:57 ` Junio C Hamano
[not found] ` <19c9dc128da911ebacc7d4ae5278bc1233465@pobox.com>
` (2 subsequent siblings)
4 siblings, 0 replies; 48+ messages in thread
From: Junio C Hamano @ 2021-03-25 22:57 UTC (permalink / raw)
To: Li Linchao via GitGitGadget
Cc: git, Derrick Stolee, dscho, Jonathan Tan, Li Linchao
"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -1216,6 +1234,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> if (filter_options.choice)
> warning(_("--filter is ignored in local clones; use file:// instead."));
> if (!access(mkpath("%s/shallow", path), F_OK)) {
> + if (reject_shallow)
> + die(_("source repository is shallow, reject to clone."));
> + else
> + warning(_("source repository is shallow."));
Hmph, is it an improvement to warn() when the user does not mind
cloning a shallow repository?
$ git clone --depth=3 $URL clone-1
$ git clone file://$(pwd)/clone-1 clone-2
would give us clone-2 that is just as functional as clone-1 is, no?
clone-1 may be missing objects that is needed far into the past, and
clone-2 would lack the same set of objects as clone-1 does, but a
user who is happily using clone-1 would be happy with clone-2 the
same way, no?
> diff --git a/fetch-pack.c b/fetch-pack.c
> index fb04a76ca263..72b378449a07 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1129,9 +1129,13 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
> if (args->deepen)
> setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
> NULL);
> - else if (si->nr_ours || si->nr_theirs)
> + else if (si->nr_ours || si->nr_theirs) {
> + if (args->remote_shallow)
> + die(_("source repository is shallow, reject to clone."));
Stopping early before calling get_pack() would significantly reduce
the overhead, which is good.
> + else
> + warning(_("source repository is shallow."));
The same question on the wisdom of warning here.
> @@ -1498,10 +1502,14 @@ static void receive_shallow_info(struct fetch_pack_args *args,
> * rejected (unless --update-shallow is set); do the same.
> */
> prepare_shallow_info(si, shallows);
> - if (si->nr_ours || si->nr_theirs)
> + if (si->nr_ours || si->nr_theirs) {
> + if (args->remote_shallow)
> + die(_("source repository is shallow, reject to clone."));
> + else
> + warning(_("source repository is shallow."));
OK, so, this is the equivalent of the above for protocol-v2? The
same comments apply, then.
> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index 428b0aac93fa..2863b8b28d44 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-httpd.sh
> +start_httpd
>
> test_expect_success 'setup' '
>
> @@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>
> '
>
> +test_expect_success 'fail to clone http shallow repository' '
s/fail to clone/reject cloning/, perhaps.
> +test_expect_success 'clone shallow repository with --no-reject-shallow' '
> + rm -rf shallow-repo &&
> + git clone --depth=1 --no-local parent shallow-repo &&
> + git clone --no-reject-shallow --no-local shallow-repo clone-repo
OK. Also without "--no-reject-shallow" option, the command would
successfully clone from the shallow-repo, I presume?
The changes look more-or-less good to me, except for the "warning()"
bit, which I do not think is a good idea.
^ permalink raw reply [flat|nested] 48+ messages in thread[parent not found: <19c9dc128da911ebacc7d4ae5278bc1233465@pobox.com>]
* Re: Re: [PATCH v7] builtin/clone.c: add --reject-shallow option
[not found] ` <19c9dc128da911ebacc7d4ae5278bc1233465@pobox.com>
@ 2021-03-26 3:34 ` lilinchao
0 siblings, 0 replies; 48+ messages in thread
From: lilinchao @ 2021-03-26 3:34 UTC (permalink / raw)
To: Junio C Hamano, Li Linchao via GitGitGadget
Cc: git, Derrick Stolee, dscho, Jonathan Tan
--------------
lilinchao@oschina.cn
>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> ...
>> This patch offers a new option '--reject-shallow' that can reject to
>> clone a shallow repository.
>>
>> Signed-off-by: lilinchao <lilinchao@oschina.cn>
>>
>> Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
>> Reviewed-by: Junio C Hamano<gitster@pobox.com>
>> Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> Reviewed-by: Jonathan Tan<jonathantanmy@google.com>
>
>The Reviewed-by trailer means something quite different from what
>you seem to think here. It is only given by the reviewers to the
>patch when they carefully reviewed and agrees what is in the patch.
>The patch authors are in no position to add it, unless they are
>explicitly told by reviewers that "this patch now can have my
>Reviewed-by:" or some equivalent.
>
>The (ideal) flow of events is
>
> 0. The author comes up with an idea and writes a patch.
>
> 1. The patch is sent to the list and Cc'ed to people who may be
> familiar with the area the patch touches. For second and
> subsequent iterations, those who gave review comments to the
> previous iterations are also good people to Cc to.
>
> 2. People give comments as reponses to the patch.
>
> (a) some may be happy with the iteration of the patch they
> reviewed, and may say "Thanks for contributing, this is now
> Reviewed-by: me". For second and subsequent iterations,
> they may say "This was improved relative to the previous
> iteration, and it still looks good and you have my
> Reviewed-by:".
>
> (b) some may give constructive criticism, alternatives,
> enhancements, or outright "not a good idea, don't do this
> because ...".
>
> (c) some may just act as cheerleaders.
>
> 3. The author thinks about the review comments and also may find
> improvement him/herself.
>
> (a) There may need an update to the patch. If the patch has
> changed since the previous version in any way, ignore
> Reviewed-by: received in 2-(a). When a significant help was
> given to update the patch, you may add "Helped-by:" trailer
> to credit the person's contribution.
>
> Your own "Signed-off-by:" appears the last in the trailers
> (i.e. "this iteration of the patch was written with help
> from these people, and then I am signing it off just before
> sending it out").
>
> Go back to 1. and repeat as many times as it takes.
>
> (b) There may not be a need for any update to the patch. Only
> add the Reviewed-by: received in 2-(a) and otherwise do not
> change anything. Your own "Signed-off-by:" appears the last
> in the trailers. Send it to the list and to the maintainer
> (me).
>
> 4. The maintainer applies the patch, unless there is no other
> comments received on that supposedly-the-final version sent in
> 3-(b), but a late review comment may make us realize that it was
> premature, in which case we may go back to 3-(a).
>
Many thanks for so detailed instructions about work flow in git community.
I will follow this flow tightly.
^ permalink raw reply [flat|nested] 48+ messages in thread
[parent not found: <7a71c96c8dbd11eb8bb0d4ae5278bc1296681@pobox.com>]
* Re: Re: [PATCH v7] builtin/clone.c: add --reject-shallow option
[not found] ` <7a71c96c8dbd11eb8bb0d4ae5278bc1296681@pobox.com>
@ 2021-03-26 3:49 ` lilinchao
0 siblings, 0 replies; 48+ messages in thread
From: lilinchao @ 2021-03-26 3:49 UTC (permalink / raw)
To: Junio C Hamano, Li Linchao via GitGitGadget
Cc: git, Derrick Stolee, dscho, Jonathan Tan
Thanks for your review.
--------------
lilinchao@oschina.cn
>"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -1216,6 +1234,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>> if (filter_options.choice)
>> warning(_("--filter is ignored in local clones; use file:// instead."));
>> if (!access(mkpath("%s/shallow", path), F_OK)) {
>> + if (reject_shallow)
>> + die(_("source repository is shallow, reject to clone."));
>> + else
>> + warning(_("source repository is shallow."));
>
>Hmph, is it an improvement to warn() when the user does not mind
>cloning a shallow repository?
>
Uh, the idea to warn comes from previous comments I wrote 25 days ago:
"and maybe we could warn client in fetch-pack stage, if we don't choose
to reject shallow cloning."
then no one response to that point, so I think is ok to apply it.
Now, it seems like a bad idea. I will remove it.
> $ git clone --depth=3 $URL clone-1
> $ git clone file://$(pwd)/clone-1 clone-2
>
>would give us clone-2 that is just as functional as clone-1 is, no?
>clone-1 may be missing objects that is needed far into the past, and
>clone-2 would lack the same set of objects as clone-1 does, but a
>user who is happily using clone-1 would be happy with clone-2 the
>same way, no?
>
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index fb04a76ca263..72b378449a07 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -1129,9 +1129,13 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>> if (args->deepen)
>> setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
>> NULL);
>> - else if (si->nr_ours || si->nr_theirs)
>> + else if (si->nr_ours || si->nr_theirs) {
>> + if (args->remote_shallow)
>> + die(_("source repository is shallow, reject to clone."));
>
>Stopping early before calling get_pack() would significantly reduce
>the overhead, which is good.
>
>> + else
>> + warning(_("source repository is shallow."));
>
>The same question on the wisdom of warning here.
>
>> @@ -1498,10 +1502,14 @@ static void receive_shallow_info(struct fetch_pack_args *args,
>> * rejected (unless --update-shallow is set); do the same.
>> */
>> prepare_shallow_info(si, shallows);
>> - if (si->nr_ours || si->nr_theirs)
>> + if (si->nr_ours || si->nr_theirs) {
>> + if (args->remote_shallow)
>> + die(_("source repository is shallow, reject to clone."));
>> + else
>> + warning(_("source repository is shallow."));
>
>OK, so, this is the equivalent of the above for protocol-v2? The
>same comments apply, then.
Yes, this is for protocol v2.
>
>> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
>> index 428b0aac93fa..2863b8b28d44 100755
>> --- a/t/t5606-clone-options.sh
>> +++ b/t/t5606-clone-options.sh
>> @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>
>> . ./test-lib.sh
>> +. "$TEST_DIRECTORY"/lib-httpd.sh
>> +start_httpd
>>
>> test_expect_success 'setup' '
>>
>> @@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>>
>> '
>>
>> +test_expect_success 'fail to clone http shallow repository' '
>
>s/fail to clone/reject cloning/, perhaps.
Ok, will do.
>
>> +test_expect_success 'clone shallow repository with --no-reject-shallow' '
>> + rm -rf shallow-repo &&
>> + git clone --depth=1 --no-local parent shallow-repo &&
>> + git clone --no-reject-shallow --no-local shallow-repo clone-repo
>
>OK. Also without "--no-reject-shallow" option, the command would
>successfully clone from the shallow-repo, I presume?
Yes, exactly.
>
>The changes look more-or-less good to me, except for the "warning()"
>bit, which I do not think is a good idea.
I will remove the unnecessary warning() part.
Thanks!
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v8] builtin/clone.c: add --reject-shallow option
2021-03-25 11:09 ` [PATCH v7] " Li Linchao via GitGitGadget
` (3 preceding siblings ...)
[not found] ` <7a71c96c8dbd11eb8bb0d4ae5278bc1296681@pobox.com>
@ 2021-03-29 10:19 ` Li Linchao via GitGitGadget
2021-03-29 21:36 ` Junio C Hamano
` (3 more replies)
4 siblings, 4 replies; 48+ messages in thread
From: Li Linchao via GitGitGadget @ 2021-03-29 10:19 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, dscho, Jonathan Tan, Li Linchao,
lilinchao
From: lilinchao <lilinchao@oschina.cn>
In some scenarios, users may want more history than the repository
offered for cloning, which happens to be a shallow repository, can
give them. But because users don't know it is a shallow repository
until they download it to local, users should have the option to
refuse to clone this kind of repository, and may want to exit the
process immediately without creating any unnecessary files.
Althought there is an option '--depth=x' for users to decide how
deep history they can fetch, but as the unshallow cloning's depth
is INFINITY, we can't know exactly the minimun 'x' value that can
satisfy the minimum integrity, so we can't pass 'x' value to --depth,
and expect this can obtain a complete history of a repository.
In other scenarios, if we have an API that allow us to import external
repository, and then perform various operations on the repo.
But if the imported is a shallow one(which is actually possible), it
will affect the subsequent operations. So we can choose to refuse to
clone, and let's just import a normal repository.
This patch offers a new option '--reject-shallow' that can reject to
clone a shallow repository.
Signed-off-by: lilinchao <lilinchao@oschina.cn>
---
builtin/clone.c: add --reject-shallow option
Changes since v1:
* Rename --no-shallow to --reject-shallow
* Enable to reject a non-local clone
* Enable --[no-]reject-shallow from CLI override configuration.
* Add more testcases.
* Reword commit messages and relative documentation.
Changes since v3:
* Add support to reject clone shallow repo over https protocol
* Add testcase to reject clone shallow repo over https:// transport
* Reword commit messages and relative documentation according
suggestions from Junio.
Changes since v5:
* camelcase config variable
* warning client that source repo is shallow
* better support ssh:// and git:// protocol v1, v2
Changes since v6:
* use _() for warning/die statement
Signed-off-by: lilinchao lilinchao@oschina.cn
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v8
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v8
Pull-Request: https://github.com/gitgitgadget/git/pull/865
Range-diff vs v7:
1: b957b77bfd79 ! 1: 83a724f6d771 builtin/clone.c: add --reject-shallow option
@@ Commit message
Signed-off-by: lilinchao <lilinchao@oschina.cn>
- Reviewed-by: Derrick Stolee <dstolee@microsoft.com>
- Reviewed-by: Junio C Hamano<gitster@pobox.com>
- Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de>
- Reviewed-by: Jonathan Tan<jonathantanmy@google.com>
-
## Documentation/config/clone.txt ##
@@ Documentation/config/clone.txt: clone.defaultRemoteName::
The name of the remote to create when cloning a repository. Defaults to
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
if (!access(mkpath("%s/shallow", path), F_OK)) {
+ if (reject_shallow)
+ die(_("source repository is shallow, reject to clone."));
-+ else
-+ warning(_("source repository is shallow."));
if (option_local > 0)
warning(_("source repository is shallow, ignoring --local"));
is_local = 0;
@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
+ else if (si->nr_ours || si->nr_theirs) {
+ if (args->remote_shallow)
+ die(_("source repository is shallow, reject to clone."));
-+ else
-+ warning(_("source repository is shallow."));
alternate_shallow_file = setup_temporary_shallow(si->shallow);
- else
+ } else
@@ fetch-pack.c: static void receive_shallow_info(struct fetch_pack_args *args,
+ if (si->nr_ours || si->nr_theirs) {
+ if (args->remote_shallow)
+ die(_("source repository is shallow, reject to clone."));
-+ else
-+ warning(_("source repository is shallow."));
alternate_shallow_file =
setup_temporary_shallow(si->shallow);
- else
@@ t/t5606-clone-options.sh: test_expect_success 'disallows --bare with --separate-
'
-+test_expect_success 'fail to clone http shallow repository' '
++test_expect_success 'reject cloning http shallow repository' '
+ git clone --depth=1 --no-local parent shallow-repo &&
+ git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git out 2>err &&
@@ t/t5606-clone-options.sh: test_expect_success 'disallows --bare with --separate-
+
+'
+
-+test_expect_success 'fail to clone shallow repository' '
++test_expect_success 'reject cloning shallow repository' '
+ rm -rf shallow-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
@@ t/t5606-clone-options.sh: test_expect_success 'disallows --bare with --separate-
+
+'
+
-+test_expect_success 'fail to clone non-local shallow repository' '
++test_expect_success 'reject cloning non-local shallow repository' '
+ rm -rf shallow-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
@@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
test_cmp expect actual
'
-+test_expect_success 'clone.rejectshallow=true should fail to clone' '
++test_expect_success 'clone.rejectshallow=true should reject cloning' '
+ rm -rf child &&
+ git clone --depth=1 --no-local . child &&
+ test_must_fail git -c clone.rejectshallow=true clone --no-local child out 2>err &&
Documentation/config/clone.txt | 4 +++
Documentation/git-clone.txt | 7 ++++-
builtin/clone.c | 22 ++++++++++++++++
fetch-pack.c | 12 ++++++---
fetch-pack.h | 1 +
t/t5606-clone-options.sh | 47 ++++++++++++++++++++++++++++++++++
t/t5611-clone-config.sh | 32 +++++++++++++++++++++++
transport.c | 4 +++
transport.h | 4 +++
9 files changed, 128 insertions(+), 5 deletions(-)
diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 47de36a5fedf..7bcfbd18a52a 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -2,3 +2,7 @@ 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].
+
+clone.rejectShallow::
+ Reject to clone a repository if it is a shallow one, can be overridden by
+ passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 02d9c19cec75..0adc98fa7eee 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,7 @@ SYNOPSIS
[--dissociate] [--separate-git-dir <git dir>]
[--depth <depth>] [--[no-]single-branch] [--no-tags]
[--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
- [--[no-]remote-submodules] [--jobs <n>] [--sparse]
+ [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
[--filter=<filter>] [--] <repository>
[<directory>]
@@ -149,6 +149,11 @@ objects from the source repository into a pack in the cloned repository.
--no-checkout::
No checkout of HEAD is performed after the clone is complete.
+--[no-]reject-shallow::
+ Fail if the source repository is a shallow repository.
+ The 'clone.rejectShallow' configuration variable can be used to
+ give the default.
+
--bare::
Make a 'bare' Git repository. That is, instead of
creating `<directory>` and placing the administrative
diff --git a/builtin/clone.c b/builtin/clone.c
index 51e844a2de0a..eeddd68a51f4 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,8 @@ static int option_no_checkout, option_bare, option_mirror, option_single_branch
static int option_local = -1, option_no_hardlinks, option_shared;
static int option_no_tags;
static int option_shallow_submodules;
+static int option_shallow = -1; /* unspecified */
+static int config_shallow = -1; /* unspecified */
static int deepen;
static char *option_template, *option_depth, *option_since;
static char *option_origin = NULL;
@@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
OPT_BOOL(0, "progress", &option_progress,
N_("force progress reporting")),
+ OPT_BOOL(0, "reject-shallow", &option_shallow,
+ N_("don't clone shallow repository")),
OPT_BOOL('n', "no-checkout", &option_no_checkout,
N_("don't create a checkout")),
OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),
@@ -858,6 +862,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
free(remote_name);
remote_name = xstrdup(v);
}
+ if (!strcmp(k, "clone.rejectshallow"))
+ config_shallow = git_config_bool(k, v);
+
return git_default_config(k, v, cb);
}
@@ -963,6 +970,7 @@ static int path_exists(const char *path)
int cmd_clone(int argc, const char **argv, const char *prefix)
{
int is_bundle = 0, is_local;
+ int reject_shallow = 0;
const char *repo_name, *repo, *work_tree, *git_dir;
char *path, *dir, *display_repo = NULL;
int dest_exists, real_dest_exists = 0;
@@ -1156,6 +1164,16 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
*/
git_config(git_clone_config, NULL);
+ /*
+ * If option_shallow is specified from CLI option,
+ * ignore config_shallow from git_clone_config.
+ */
+ if (config_shallow != -1)
+ reject_shallow = config_shallow;
+
+ if (option_shallow != -1)
+ reject_shallow = option_shallow;
+
/*
* apply the remote name provided by --origin only after this second
* call to git_config, to ensure it overrides all config-based values.
@@ -1216,6 +1234,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (filter_options.choice)
warning(_("--filter is ignored in local clones; use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
+ if (reject_shallow)
+ die(_("source repository is shallow, reject to clone."));
if (option_local > 0)
warning(_("source repository is shallow, ignoring --local"));
is_local = 0;
@@ -1227,6 +1247,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
transport_set_option(transport, TRANS_OPT_KEEP, "yes");
+ if (reject_shallow)
+ transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
if (option_depth)
transport_set_option(transport, TRANS_OPT_DEPTH,
option_depth);
diff --git a/fetch-pack.c b/fetch-pack.c
index fb04a76ca263..34d0c2896e2e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1129,9 +1129,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
if (args->deepen)
setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
NULL);
- else if (si->nr_ours || si->nr_theirs)
+ else if (si->nr_ours || si->nr_theirs) {
+ if (args->remote_shallow)
+ die(_("source repository is shallow, reject to clone."));
alternate_shallow_file = setup_temporary_shallow(si->shallow);
- else
+ } else
alternate_shallow_file = NULL;
if (get_pack(args, fd, pack_lockfiles, NULL, sought, nr_sought,
&gitmodules_oids))
@@ -1498,10 +1500,12 @@ static void receive_shallow_info(struct fetch_pack_args *args,
* rejected (unless --update-shallow is set); do the same.
*/
prepare_shallow_info(si, shallows);
- if (si->nr_ours || si->nr_theirs)
+ if (si->nr_ours || si->nr_theirs) {
+ if (args->remote_shallow)
+ die(_("source repository is shallow, reject to clone."));
alternate_shallow_file =
setup_temporary_shallow(si->shallow);
- else
+ } else
alternate_shallow_file = NULL;
} else {
alternate_shallow_file = NULL;
diff --git a/fetch-pack.h b/fetch-pack.h
index 736a3dae467a..6e4f8f0d738c 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -39,6 +39,7 @@ struct fetch_pack_args {
unsigned self_contained_and_connected:1;
unsigned cloning:1;
unsigned update_shallow:1;
+ unsigned remote_shallow:1;
unsigned deepen:1;
/*
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 428b0aac93fa..de1cd85983ed 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_httpd
test_expect_success 'setup' '
@@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
'
+test_expect_success 'reject cloning http shallow repository' '
+ git clone --depth=1 --no-local parent shallow-repo &&
+ git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+
+'
+
+test_expect_success 'reject cloning shallow repository' '
+ rm -rf shallow-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+
+'
+
+test_expect_success 'reject cloning non-local shallow repository' '
+ rm -rf shallow-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+
+'
+
+test_expect_success 'clone shallow repository with --no-reject-shallow' '
+ rm -rf shallow-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ git clone --no-reject-shallow --no-local shallow-repo clone-repo
+
+'
+
+test_expect_success 'clone normal repository with --reject-shallow' '
+ rm -rf clone-repo &&
+ git clone --no-local parent normal-repo &&
+ git clone --reject-shallow --no-local normal-repo clone-repo
+
+'
+
+test_expect_success 'unspecified any configs or options' '
+ rm -rf shallow-repo clone-repo &&
+ git clone --depth=1 --no-local parent shallow-repo &&
+ git clone shallow-repo clone-repo
+
+'
+
test_expect_success 'uses "origin" for default remote name' '
git clone parent clone-default-origin &&
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index 9f555b87ecdf..adf873f60300 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -95,6 +95,38 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
test_cmp expect actual
'
+test_expect_success 'clone.rejectshallow=true should reject cloning' '
+ rm -rf child &&
+ git clone --depth=1 --no-local . child &&
+ test_must_fail git -c clone.rejectshallow=true clone --no-local child out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+'
+
+test_expect_success 'clone.rejectshallow=false should succeed' '
+ rm -rf child out &&
+ git clone --depth=1 --no-local . child &&
+ git -c clone.rejectshallow=false clone --no-local child out
+'
+
+test_expect_success 'clone.rejectshallow=true should succeed with normal repo' '
+ rm -rf child out &&
+ git clone --no-local . child &&
+ git -c clone.rejectshallow=true clone --no-local child out
+'
+
+test_expect_success 'option --reject-shallow override clone.rejectshallow' '
+ rm -rf child out &&
+ git clone --depth=1 --no-local . child &&
+ test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local child out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err
+'
+
+test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
+ rm -rf child out &&
+ git clone --depth=1 --no-local . child &&
+ git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
+'
+
test_expect_success MINGW 'clone -c core.hideDotFiles' '
test_commit attributes .gitattributes "" &&
rm -rf child &&
diff --git a/transport.c b/transport.c
index 1c4ab676d1b1..a6b9f404d86a 100644
--- a/transport.c
+++ b/transport.c
@@ -236,6 +236,9 @@ static int set_git_option(struct git_transport_options *opts,
list_objects_filter_die_if_populated(&opts->filter_options);
parse_list_objects_filter(&opts->filter_options, value);
return 0;
+ } else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
+ opts->reject_shallow = !!value;
+ return 0;
}
return 1;
}
@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
args.stateless_rpc = transport->stateless_rpc;
args.server_options = transport->server_options;
args.negotiation_tips = data->options.negotiation_tips;
+ args.remote_shallow = transport->smart_options->reject_shallow;
if (!data->got_remote_heads) {
int i;
diff --git a/transport.h b/transport.h
index 24e15799e714..4d5db0a7f22b 100644
--- a/transport.h
+++ b/transport.h
@@ -14,6 +14,7 @@ struct git_transport_options {
unsigned check_self_contained_and_connected : 1;
unsigned self_contained_and_connected : 1;
unsigned update_shallow : 1;
+ unsigned reject_shallow : 1;
unsigned deepen_relative : 1;
/* see documentation of corresponding flag in fetch-pack.h */
@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
/* Aggressively fetch annotated tags if possible */
#define TRANS_OPT_FOLLOWTAGS "followtags"
+/* Reject shallow repo transport */
+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
+
/* Accept refs that may update .git/shallow without --depth */
#define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
base-commit: 84d06cdc06389ae7c462434cb7b1db0980f63860
--
gitgitgadget
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v8] builtin/clone.c: add --reject-shallow option
2021-03-29 10:19 ` [PATCH v8] " Li Linchao via GitGitGadget
@ 2021-03-29 21:36 ` Junio C Hamano
2021-03-30 9:54 ` Johannes Schindelin
` (2 subsequent siblings)
3 siblings, 0 replies; 48+ messages in thread
From: Junio C Hamano @ 2021-03-29 21:36 UTC (permalink / raw)
To: Li Linchao via GitGitGadget
Cc: git, Derrick Stolee, dscho, Jonathan Tan, Li Linchao
"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Changes since v6:
>
> * use _() for warning/die statement
This round tweaks a few test titles and drops warning() that was
given in v7 when the feature is not in use.
Comments from other reviewers who are more familiar with the
transport layer than I am? As far as I see, this version is
done cleanly enough to call it ready for 'next'.
Thanks.
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v8] builtin/clone.c: add --reject-shallow option
2021-03-29 10:19 ` [PATCH v8] " Li Linchao via GitGitGadget
2021-03-29 21:36 ` Junio C Hamano
@ 2021-03-30 9:54 ` Johannes Schindelin
2021-03-30 17:46 ` Junio C Hamano
[not found] ` <f8b2582c913d11ebaddbd4ae5278bc1214940@gmx.de>
2021-03-31 15:51 ` [PATCH v9] " lilinchao via GitGitGadget
3 siblings, 1 reply; 48+ messages in thread
From: Johannes Schindelin @ 2021-03-30 9:54 UTC (permalink / raw)
To: Li Linchao via GitGitGadget
Cc: git, Junio C Hamano, Derrick Stolee, Jonathan Tan, Li Linchao,
lilinchao
Hi,
On Mon, 29 Mar 2021, Li Linchao via GitGitGadget wrote:
> From: lilinchao <lilinchao@oschina.cn>
I see "Li Linchao" in the email, but "lilinchao" in the author
information. Maybe you want to align them? Or maybe even use Unicode to
write your non-Latinized name?
> In some scenarios, users may want more history than the repository
> offered for cloning, which happens to be a shallow repository, can
> give them. But because users don't know it is a shallow repository
> until they download it to local, users should have the option to
> refuse to clone this kind of repository, and may want to exit the
> process immediately without creating any unnecessary files.
>
> Althought there is an option '--depth=x' for users to decide how
> deep history they can fetch, but as the unshallow cloning's depth
> is INFINITY, we can't know exactly the minimun 'x' value that can
> satisfy the minimum integrity, so we can't pass 'x' value to --depth,
> and expect this can obtain a complete history of a repository.
>
> In other scenarios, if we have an API that allow us to import external
> repository, and then perform various operations on the repo.
> But if the imported is a shallow one(which is actually possible), it
> will affect the subsequent operations. So we can choose to refuse to
> clone, and let's just import a normal repository.
>
> This patch offers a new option '--reject-shallow' that can reject to
> clone a shallow repository.
Good.
I like most of the patch, and will only point out a couple of things that
I think can be improved even further.
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index 02d9c19cec75..0adc98fa7eee 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -149,6 +149,11 @@ objects from the source repository into a pack in the cloned repository.
> --no-checkout::
> No checkout of HEAD is performed after the clone is complete.
>
> +--[no-]reject-shallow::
> + Fail if the source repository is a shallow repository.
> + The 'clone.rejectShallow' configuration variable can be used to
> + give the default.
I am not a native speaker, either, but I believe that it would "roll off
the tongue" a bit better to say "to specify the default".
> +
> --bare::
> Make a 'bare' Git repository. That is, instead of
> creating `<directory>` and placing the administrative
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 51e844a2de0a..eeddd68a51f4 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -50,6 +50,8 @@ static int option_no_checkout, option_bare, option_mirror, option_single_branch
> static int option_local = -1, option_no_hardlinks, option_shared;
> static int option_no_tags;
> static int option_shallow_submodules;
> +static int option_shallow = -1; /* unspecified */
> +static int config_shallow = -1; /* unspecified */
I would much prefer those variable names to include an indicator that this
is about _rejecting_ shallow clones. I.e. `option_reject_shallow`.
Also, I think that we can do with just a single `option_reject_shallow`
(we do not even need that `reject_shallow` variable in `cmd_clone()`):
- in `git_clone_config()`, only override it if it is still unspecified:
if (!strcmp(k, "clone.rejectshallow") && option_reject_shallow < 0)
option_reject_shallow = git_config_bool(k,v);
- in `cmd_clone()`, test for a _positive_ value:
if (option_reject_shallow > 0)
die(_("source repository is shallow, reject to clone."));
and
if (option_reject_shallow > 0)
transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
One thing to note (in the commit message, would be my preference) is that
`cmd_clone()` is _particular_ in that it runs `git_config()` _twice_. Once
before the command-line options are parsed, and once after the new Git
repository has been initialized. Note that my suggestion still works with
that: if either the original config, or the new config set
`clone.rejectShallow`, it is picked up correctly, with the latter
overriding the former if both configs want to set it.
> diff --git a/fetch-pack.c b/fetch-pack.c
> index fb04a76ca263..34d0c2896e2e 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -1129,9 +1129,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
> if (args->deepen)
> setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
> NULL);
> - else if (si->nr_ours || si->nr_theirs)
> + else if (si->nr_ours || si->nr_theirs) {
> + if (args->remote_shallow)
Even as a non-casual reader, this name `remote_shallow` leads me to assume
incorrect things. This option is not about wanting a remote shallow
repository, it is about rejecting a remote shallow repository.
Please name this attribute `reject_shallow` instead of `remote_shallow`.
That will prevent future puzzlement.
> + die(_("source repository is shallow, reject to clone."));
> alternate_shallow_file = setup_temporary_shallow(si->shallow);
> - else
> + } else
> alternate_shallow_file = NULL;
> if (get_pack(args, fd, pack_lockfiles, NULL, sought, nr_sought,
> &gitmodules_oids))
> [...]
> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index 428b0aac93fa..de1cd85983ed 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>
> . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-httpd.sh
> +start_httpd
That's not good. What happens if there is no `httpd`? Then the rest of the
tests are either skipped, or if `GIT_TEST_HTTPD` is set to `true`, we
fail. The failure is intentional, but the skipping is not. There are many
tests in t5606 that do not require a running HTTP daemon, and we should
not skip them (for example, in our CI runs, there are quite a few jobs
that run without any working `httpd`).
A much better alternative, I think, would be to move those new test cases
that require `httpd` to be running to t5601 (which _already_ calls
`start_httpd`, near the end, so as to not skip any tests that do not
require `httpd`).
>
> test_expect_success 'setup' '
>
> @@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>
> '
>
> +test_expect_success 'reject cloning http shallow repository' '
> + git clone --depth=1 --no-local parent shallow-repo &&
> + git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> + test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git out 2>err &&
> + test_i18ngrep -e "source repository is shallow, reject to clone." err
> +
> +'
> +
> +test_expect_success 'reject cloning shallow repository' '
> + rm -rf shallow-repo &&
Should this line not come immediately after the bare clone into
<DOCUMENT_ROOT>/repo.git? Or even better, as a `test_when_finished`
command.
And maybe you want to extract this preparatory step into its own test
case:
test_expect_success 'set up shallow http repository' '
test_when_finished "rm -rf shallow-repo" &&
git clone --depth=1 --no-local parent shallow-repo &&
git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
'
> + git clone --depth=1 --no-local parent shallow-repo &&
> + test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
> + test_i18ngrep -e "source repository is shallow, reject to clone." err
> +
Please remove the extra empty line. (This goes for at least a couple test
cases added by this patch.)
> +'
This test case does not require `start_httpd`, and should therefore come
before the test cases that do require it (actually, it should come before
the `start_httpd` call, even).
> +
> +test_expect_success 'reject cloning non-local shallow repository' '
> + rm -rf shallow-repo &&
> + git clone --depth=1 --no-local parent shallow-repo &&
> + test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
> + test_i18ngrep -e "source repository is shallow, reject to clone." err
> +
> +'
Hmm. Reading through three test cases that all create `shallow-repo` in
the same way, I wonder whether we should not simply set it up once, and
then not even bother removing it. I think that would simplify not only the
patch, it would also simplify debugging later on.
> +
> +test_expect_success 'clone shallow repository with --no-reject-shallow' '
> + rm -rf shallow-repo &&
> + git clone --depth=1 --no-local parent shallow-repo &&
> + git clone --no-reject-shallow --no-local shallow-repo clone-repo
> +
> +'
> +
> +test_expect_success 'clone normal repository with --reject-shallow' '
> + rm -rf clone-repo &&
> + git clone --no-local parent normal-repo &&
> + git clone --reject-shallow --no-local normal-repo clone-repo
> +
> +'
> +
> +test_expect_success 'unspecified any configs or options' '
> + rm -rf shallow-repo clone-repo &&
> + git clone --depth=1 --no-local parent shallow-repo &&
> + git clone shallow-repo clone-repo
> +
> +'
> +
Having read through these test cases, I think they can be simplified by
- first setting up `shallow-repo`
- then testing in the same test case whether `--reject-shallow` fails and
`--no-reject-shallow` succeeds, without `--no-local`
- then testing the same _with_ `--no-local`
These can go to t5606, no problem.
Then, in t5601, after the `start_httpd` call, add a single test case that
- sets up the shallow clone _directly_, i.e.
git clone --bare --no-local --depth=1 parent \
"$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
- verifies that `--reject-shallow` fails as expected, and
- verifies that `--no-reject-shallow` works as expected.
> test_expect_success 'uses "origin" for default remote name' '
>
> git clone parent clone-default-origin &&
> diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
> index 9f555b87ecdf..adf873f60300 100755
> --- a/t/t5611-clone-config.sh
> +++ b/t/t5611-clone-config.sh
> @@ -95,6 +95,38 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
> test_cmp expect actual
> '
>
> +test_expect_success 'clone.rejectshallow=true should reject cloning' '
> + rm -rf child &&
> + git clone --depth=1 --no-local . child &&
In the following, this shallow repository is needed a couple of times.
Better set it up once, in a dedicated `set up shallow repository` test
case.
And `shallow-repo` would probably make for a much better name than
`child`.
> + test_must_fail git -c clone.rejectshallow=true clone --no-local child out 2>err &&
> + test_i18ngrep -e "source repository is shallow, reject to clone." err
> +'
> +
> +test_expect_success 'clone.rejectshallow=false should succeed' '
> + rm -rf child out &&
> + git clone --depth=1 --no-local . child &&
> + git -c clone.rejectshallow=false clone --no-local child out
> +'
These two can be combined (and should, if you ask me, to simplify things).
> +
> +test_expect_success 'clone.rejectshallow=true should succeed with normal repo' '
> + rm -rf child out &&
> + git clone --no-local . child &&
> + git -c clone.rejectshallow=true clone --no-local child out
> +'
> +
> +test_expect_success 'option --reject-shallow override clone.rejectshallow' '
> + rm -rf child out &&
> + git clone --depth=1 --no-local . child &&
> + test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local child out 2>err &&
> + test_i18ngrep -e "source repository is shallow, reject to clone." err
> +'
> +
> +test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
> + rm -rf child out &&
> + git clone --depth=1 --no-local . child &&
> + git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
> +'
> +
Personally, I think this is overkill. What I would do is to have a single
test case that verifies that
- `clone.rejectShallow=true` fails as expected,
- `clone.rejectShallow=false [...] --reject-shallow` fails as expected, and
- `clone.rejectShallow=false` succeeds.
If we do this, we do not even need a preparatory test case setting up the
shallow repository.
> test_expect_success MINGW 'clone -c core.hideDotFiles' '
> test_commit attributes .gitattributes "" &&
> rm -rf child &&
> diff --git a/transport.c b/transport.c
> index 1c4ab676d1b1..a6b9f404d86a 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -236,6 +236,9 @@ static int set_git_option(struct git_transport_options *opts,
> list_objects_filter_die_if_populated(&opts->filter_options);
> parse_list_objects_filter(&opts->filter_options, value);
> return 0;
> + } else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
> + opts->reject_shallow = !!value;
I see that this is the established pattern (I am so grateful that I have
https://github.com/gitgitgadget/git/pull/865/files to look at the context,
something with which a pure mail-only patch contribution would not bless
me!), that those Boolean options are `NULL` vs non-`NULL`. So while you
pass `"1"` as the `value` parameter to `set_git_option()`, the parameter
`"0"` would _enable that option just the same_, you would have to pass
`NULL` to turn it off. I find that highly unintuitive, but that's not the
fault of your patch. The pattern is established, and you did the right
thing by following it.
> + return 0;
> }
> return 1;
> }
As I said, the rest of the patch looks good to me. With the few
suggestions I offered, I would be totally fine with this patch entering
`next`.
Thank you,
Dscho
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v8] builtin/clone.c: add --reject-shallow option
2021-03-30 9:54 ` Johannes Schindelin
@ 2021-03-30 17:46 ` Junio C Hamano
2021-03-31 13:30 ` Johannes Schindelin
0 siblings, 1 reply; 48+ messages in thread
From: Junio C Hamano @ 2021-03-30 17:46 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Li Linchao via GitGitGadget, git, Derrick Stolee, Jonathan Tan,
Li Linchao
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> +static int option_shallow = -1; /* unspecified */
>> +static int config_shallow = -1; /* unspecified */
>
> I would much prefer those variable names to include an indicator that this
> is about _rejecting_ shallow clones. I.e. `option_reject_shallow`.
Good.
> ... repository has been initialized. Note that my suggestion still works with
> that: if either the original config, or the new config set
> `clone.rejectShallow`, it is picked up correctly, with the latter
> overriding the former if both configs want to set it.
All four combinations ("set it to true" vs "set it to false" makes
two, and before and after doubles that to four)?
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index fb04a76ca263..34d0c2896e2e 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -1129,9 +1129,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>> if (args->deepen)
>> setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
>> NULL);
>> - else if (si->nr_ours || si->nr_theirs)
>> + else if (si->nr_ours || si->nr_theirs) {
>> + if (args->remote_shallow)
>
> Even as a non-casual reader, this name `remote_shallow` leads me to assume
> incorrect things. This option is not about wanting a remote shallow
> repository, it is about rejecting a remote shallow repository.
Yeah, I've seen this code too long that my eyes were contaminated
;-) Thanks for an extra set of eyeballs to spot this. What this
option means is to "reject-shallow-remote", and I agree 'reject'
is the most important part (args->reject_shallow_remote is not
overly long, either).
>> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
>> index 428b0aac93fa..de1cd85983ed 100755
>> --- a/t/t5606-clone-options.sh
>> +++ b/t/t5606-clone-options.sh
>> @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>
>> . ./test-lib.sh
>> +. "$TEST_DIRECTORY"/lib-httpd.sh
>> +start_httpd
>
> That's not good. What happens if there is no `httpd`? Then the rest of the
> tests are either skipped, or if `GIT_TEST_HTTPD` is set to `true`, we
> fail. The failure is intentional, but the skipping is not. There are many
> tests in t5606 that do not require a running HTTP daemon, and we should
> not skip them (for example, in our CI runs, there are quite a few jobs
> that run without any working `httpd`).
>
> A much better alternative, I think, would be to move those new test cases
> that require `httpd` to be running to t5601 (which _already_ calls
> `start_httpd`, near the end, so as to not skip any tests that do not
> require `httpd`).
Or move the tests that require httpd, and use of httpd library, to
the end of this script.
> Having read through these test cases, I think they can be simplified by
>
> - first setting up `shallow-repo`
>
> - then testing in the same test case whether `--reject-shallow` fails and
> `--no-reject-shallow` succeeds, without `--no-local`
>
> - then testing the same _with_ `--no-local`
>
> These can go to t5606, no problem.
>
> Then, in t5601, after the `start_httpd` call, add a single test case that
>
> - sets up the shallow clone _directly_, i.e.
>
> git clone --bare --no-local --depth=1 parent \
> "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
>
> - verifies that `--reject-shallow` fails as expected, and
>
> - verifies that `--no-reject-shallow` works as expected.
>
>> test_expect_success 'uses "origin" for default remote name' '
That would work.
> ...
> As I said, the rest of the patch looks good to me. With the few
> suggestions I offered, I would be totally fine with this patch entering
> `next`.
Thanks for a review. Looking good.
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH v8] builtin/clone.c: add --reject-shallow option
2021-03-30 17:46 ` Junio C Hamano
@ 2021-03-31 13:30 ` Johannes Schindelin
0 siblings, 0 replies; 48+ messages in thread
From: Johannes Schindelin @ 2021-03-31 13:30 UTC (permalink / raw)
To: Junio C Hamano
Cc: Li Linchao via GitGitGadget, git, Derrick Stolee, Jonathan Tan,
Li Linchao
Hi Junio,
On Tue, 30 Mar 2021, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > ... repository has been initialized. Note that my suggestion still works with
> > that: if either the original config, or the new config set
> > `clone.rejectShallow`, it is picked up correctly, with the latter
> > overriding the former if both configs want to set it.
>
> All four combinations ("set it to true" vs "set it to false" makes
> two, and before and after doubles that to four)?
I don't think we need to test all four combinations, that's part of my
point. We only need to verify that we can reject a shallow clone, and that
we can allow it, and that the command-line option overrides the config
option. That's three ;-)
> >> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> >> index 428b0aac93fa..de1cd85983ed 100755
> >> --- a/t/t5606-clone-options.sh
> >> +++ b/t/t5606-clone-options.sh
> >> @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> >> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> >>
> >> . ./test-lib.sh
> >> +. "$TEST_DIRECTORY"/lib-httpd.sh
> >> +start_httpd
> >
> > That's not good. What happens if there is no `httpd`? Then the rest of the
> > tests are either skipped, or if `GIT_TEST_HTTPD` is set to `true`, we
> > fail. The failure is intentional, but the skipping is not. There are many
> > tests in t5606 that do not require a running HTTP daemon, and we should
> > not skip them (for example, in our CI runs, there are quite a few jobs
> > that run without any working `httpd`).
> >
> > A much better alternative, I think, would be to move those new test cases
> > that require `httpd` to be running to t5601 (which _already_ calls
> > `start_httpd`, near the end, so as to not skip any tests that do not
> > require `httpd`).
>
> Or move the tests that require httpd, and use of httpd library, to
> the end of this script.
In the interest of saving some time during the already-quite-long test
runs, I would recommend piggy-backing onto a script that _already_ spins
up its own Apache server.
Thanks,
Dscho
^ permalink raw reply [flat|nested] 48+ messages in thread
[parent not found: <f8b2582c913d11ebaddbd4ae5278bc1214940@gmx.de>]
* Re: Re: [PATCH v8] builtin/clone.c: add --reject-shallow option
[not found] ` <f8b2582c913d11ebaddbd4ae5278bc1214940@gmx.de>
@ 2021-03-31 11:03 ` lilinchao
0 siblings, 0 replies; 48+ messages in thread
From: lilinchao @ 2021-03-31 11:03 UTC (permalink / raw)
To: dscho; +Cc: git, Junio C Hamano, Derrick Stolee, Jonathan Tan
Thanks for your suggestions!
I've combined your suggestions in this comment with Junio's which based on yours.
>Hi,
>
>On Mon, 29 Mar 2021, Li Linchao via GitGitGadget wrote:
>
>> From: lilinchao <lilinchao@oschina.cn>
>
>I see "Li Linchao" in the email, but "lilinchao" in the author
>information. Maybe you want to align them? Or maybe even use Unicode to
>write your non-Latinized name?
>
The "Li Lilinchao" comes from my Github profile name. Actually, it contains my
first name and last name respectively.
I will changed it to "Li Linchao" in the author info to keep these info consistent.
>> In some scenarios, users may want more history than the repository
>> offered for cloning, which happens to be a shallow repository, can
>> give them. But because users don't know it is a shallow repository
>> until they download it to local, users should have the option to
>> refuse to clone this kind of repository, and may want to exit the
>> process immediately without creating any unnecessary files.
>>
>> Althought there is an option '--depth=x' for users to decide how
>> deep history they can fetch, but as the unshallow cloning's depth
>> is INFINITY, we can't know exactly the minimun 'x' value that can
>> satisfy the minimum integrity, so we can't pass 'x' value to --depth,
>> and expect this can obtain a complete history of a repository.
>>
>> In other scenarios, if we have an API that allow us to import external
>> repository, and then perform various operations on the repo.
>> But if the imported is a shallow one(which is actually possible), it
>> will affect the subsequent operations. So we can choose to refuse to
>> clone, and let's just import a normal repository.
>>
>> This patch offers a new option '--reject-shallow' that can reject to
>> clone a shallow repository.
>
>Good.
>
>I like most of the patch, and will only point out a couple of things that
>I think can be improved even further.
>
>> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
>> index 02d9c19cec75..0adc98fa7eee 100644
>> --- a/Documentation/git-clone.txt
>> +++ b/Documentation/git-clone.txt
>> @@ -149,6 +149,11 @@ objects from the source repository into a pack in the cloned repository.
>> --no-checkout::
>> No checkout of HEAD is performed after the clone is complete.
>>
>> +--[no-]reject-shallow::
>> + Fail if the source repository is a shallow repository.
>> + The 'clone.rejectShallow' configuration variable can be used to
>> + give the default.
>
>I am not a native speaker, either, but I believe that it would "roll off
>the tongue" a bit better to say "to specify the default".
>
>> +
>> --bare::
>> Make a 'bare' Git repository. That is, instead of
>> creating `<directory>` and placing the administrative
>> diff --git a/builtin/clone.c b/builtin/clone.c
>> index 51e844a2de0a..eeddd68a51f4 100644
>> --- a/builtin/clone.c
>> +++ b/builtin/clone.c
>> @@ -50,6 +50,8 @@ static int option_no_checkout, option_bare, option_mirror, option_single_branch
>> static int option_local = -1, option_no_hardlinks, option_shared;
>> static int option_no_tags;
>> static int option_shallow_submodules;
>> +static int option_shallow = -1; /* unspecified */
>> +static int config_shallow = -1; /* unspecified */
>
>I would much prefer those variable names to include an indicator that this
>is about _rejecting_ shallow clones. I.e. `option_reject_shallow`.
>
>Also, I think that we can do with just a single `option_reject_shallow`
>(we do not even need that `reject_shallow` variable in `cmd_clone()`):
>
>- in `git_clone_config()`, only override it if it is still unspecified:
>
> if (!strcmp(k, "clone.rejectshallow") && option_reject_shallow < 0)
> option_reject_shallow = git_config_bool(k,v);
>
>- in `cmd_clone()`, test for a _positive_ value:
>
> if (option_reject_shallow > 0)
> die(_("source repository is shallow, reject to clone."));
>
> and
>
> if (option_reject_shallow > 0)
> transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
>
>One thing to note (in the commit message, would be my preference) is that
>`cmd_clone()` is _particular_ in that it runs `git_config()` _twice_. Once
>before the command-line options are parsed, and once after the new Git
>repository has been initialized. Note that my suggestion still works with
>that: if either the original config, or the new config set
>`clone.rejectShallow`, it is picked up correctly, with the latter
>overriding the former if both configs want to set it.
>
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index fb04a76ca263..34d0c2896e2e 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -1129,9 +1129,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>> if (args->deepen)
>> setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
>> NULL);
>> - else if (si->nr_ours || si->nr_theirs)
>> + else if (si->nr_ours || si->nr_theirs) {
>> + if (args->remote_shallow)
>
>Even as a non-casual reader, this name `remote_shallow` leads me to assume
>incorrect things. This option is not about wanting a remote shallow
>repository, it is about rejecting a remote shallow repository.
>
>Please name this attribute `reject_shallow` instead of `remote_shallow`.
>That will prevent future puzzlement.
>
>> + die(_("source repository is shallow, reject to clone."));
>> alternate_shallow_file = setup_temporary_shallow(si->shallow);
>> - else
>> + } else
>> alternate_shallow_file = NULL;
>> if (get_pack(args, fd, pack_lockfiles, NULL, sought, nr_sought,
>> &gitmodules_oids))
>> [...]
>> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
>> index 428b0aac93fa..de1cd85983ed 100755
>> --- a/t/t5606-clone-options.sh
>> +++ b/t/t5606-clone-options.sh
>> @@ -5,6 +5,8 @@ GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>> export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>>
>> . ./test-lib.sh
>> +. "$TEST_DIRECTORY"/lib-httpd.sh
>> +start_httpd
>
>That's not good. What happens if there is no `httpd`? Then the rest of the
>tests are either skipped, or if `GIT_TEST_HTTPD` is set to `true`, we
>fail. The failure is intentional, but the skipping is not. There are many
>tests in t5606 that do not require a running HTTP daemon, and we should
>not skip them (for example, in our CI runs, there are quite a few jobs
>that run without any working `httpd`).
>
>A much better alternative, I think, would be to move those new test cases
>that require `httpd` to be running to t5601 (which _already_ calls
>`start_httpd`, near the end, so as to not skip any tests that do not
>require `httpd`).
>
>>
>> test_expect_success 'setup' '
>>
>> @@ -45,6 +47,51 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>>
>> '
>>
>> +test_expect_success 'reject cloning http shallow repository' '
>> + git clone --depth=1 --no-local parent shallow-repo &&
>> + git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
>> + test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git out 2>err &&
>> + test_i18ngrep -e "source repository is shallow, reject to clone." err
>> +
>> +'
>> +
>> +test_expect_success 'reject cloning shallow repository' '
>> + rm -rf shallow-repo &&
>
>Should this line not come immediately after the bare clone into
><DOCUMENT_ROOT>/repo.git? Or even better, as a `test_when_finished`
>command.
>
>And maybe you want to extract this preparatory step into its own test
>case:
>
>test_expect_success 'set up shallow http repository' '
> test_when_finished "rm -rf shallow-repo" &&
> git clone --depth=1 --no-local parent shallow-repo &&
> git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
>'
>
>> + git clone --depth=1 --no-local parent shallow-repo &&
>> + test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
>> + test_i18ngrep -e "source repository is shallow, reject to clone." err
>> +
>
>Please remove the extra empty line. (This goes for at least a couple test
>cases added by this patch.)
>
>> +'
>
>This test case does not require `start_httpd`, and should therefore come
>before the test cases that do require it (actually, it should come before
>the `start_httpd` call, even).
>
>> +
>> +test_expect_success 'reject cloning non-local shallow repository' '
>> + rm -rf shallow-repo &&
>> + git clone --depth=1 --no-local parent shallow-repo &&
>> + test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
>> + test_i18ngrep -e "source repository is shallow, reject to clone." err
>> +
>> +'
>
>Hmm. Reading through three test cases that all create `shallow-repo` in
>the same way, I wonder whether we should not simply set it up once, and
>then not even bother removing it. I think that would simplify not only the
>patch, it would also simplify debugging later on.
>
>> +
>> +test_expect_success 'clone shallow repository with --no-reject-shallow' '
>> + rm -rf shallow-repo &&
>> + git clone --depth=1 --no-local parent shallow-repo &&
>> + git clone --no-reject-shallow --no-local shallow-repo clone-repo
>> +
>> +'
>> +
>> +test_expect_success 'clone normal repository with --reject-shallow' '
>> + rm -rf clone-repo &&
>> + git clone --no-local parent normal-repo &&
>> + git clone --reject-shallow --no-local normal-repo clone-repo
>> +
>> +'
>> +
>> +test_expect_success 'unspecified any configs or options' '
>> + rm -rf shallow-repo clone-repo &&
>> + git clone --depth=1 --no-local parent shallow-repo &&
>> + git clone shallow-repo clone-repo
>> +
>> +'
>> +
>
>Having read through these test cases, I think they can be simplified by
>
>- first setting up `shallow-repo`
>
>- then testing in the same test case whether `--reject-shallow` fails and
> `--no-reject-shallow` succeeds, without `--no-local`
>
>- then testing the same _with_ `--no-local`
>
>These can go to t5606, no problem.
>
>Then, in t5601, after the `start_httpd` call, add a single test case that
>
>- sets up the shallow clone _directly_, i.e.
>
> git clone --bare --no-local --depth=1 parent \
> "$HTTPD_DOCUMENT_ROOT_PATH/repo.git"
>
>- verifies that `--reject-shallow` fails as expected, and
>
>- verifies that `--no-reject-shallow` works as expected.
>
>> test_expect_success 'uses "origin" for default remote name' '
>>
>> git clone parent clone-default-origin &&
>> diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
>> index 9f555b87ecdf..adf873f60300 100755
>> --- a/t/t5611-clone-config.sh
>> +++ b/t/t5611-clone-config.sh
>> @@ -95,6 +95,38 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
>> test_cmp expect actual
>> '
>>
>> +test_expect_success 'clone.rejectshallow=true should reject cloning' '
>> + rm -rf child &&
>> + git clone --depth=1 --no-local . child &&
>
>In the following, this shallow repository is needed a couple of times.
>Better set it up once, in a dedicated `set up shallow repository` test
>case.
>
>And `shallow-repo` would probably make for a much better name than
>`child`.
>
>> + test_must_fail git -c clone.rejectshallow=true clone --no-local child out 2>err &&
>> + test_i18ngrep -e "source repository is shallow, reject to clone." err
>> +'
>> +
>> +test_expect_success 'clone.rejectshallow=false should succeed' '
>> + rm -rf child out &&
>> + git clone --depth=1 --no-local . child &&
>> + git -c clone.rejectshallow=false clone --no-local child out
>> +'
>
>These two can be combined (and should, if you ask me, to simplify things).
>
>> +
>> +test_expect_success 'clone.rejectshallow=true should succeed with normal repo' '
>> + rm -rf child out &&
>> + git clone --no-local . child &&
>> + git -c clone.rejectshallow=true clone --no-local child out
>> +'
>> +
>> +test_expect_success 'option --reject-shallow override clone.rejectshallow' '
>> + rm -rf child out &&
>> + git clone --depth=1 --no-local . child &&
>> + test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local child out 2>err &&
>> + test_i18ngrep -e "source repository is shallow, reject to clone." err
>> +'
>> +
>> +test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
>> + rm -rf child out &&
>> + git clone --depth=1 --no-local . child &&
>> + git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
>> +'
>> +
>
>Personally, I think this is overkill. What I would do is to have a single
>test case that verifies that
>
>- `clone.rejectShallow=true` fails as expected,
>
>- `clone.rejectShallow=false [...] --reject-shallow` fails as expected, and
>
>- `clone.rejectShallow=false` succeeds.
>
>If we do this, we do not even need a preparatory test case setting up the
>shallow repository.
>
>> test_expect_success MINGW 'clone -c core.hideDotFiles' '
>> test_commit attributes .gitattributes "" &&
>> rm -rf child &&
>> diff --git a/transport.c b/transport.c
>> index 1c4ab676d1b1..a6b9f404d86a 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -236,6 +236,9 @@ static int set_git_option(struct git_transport_options *opts,
>> list_objects_filter_die_if_populated(&opts->filter_options);
>> parse_list_objects_filter(&opts->filter_options, value);
>> return 0;
>> + } else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
>> + opts->reject_shallow = !!value;
>
>I see that this is the established pattern (I am so grateful that I have
>https://github.com/gitgitgadget/git/pull/865/files to look at the context,
>something with which a pure mail-only patch contribution would not bless
>me!), that those Boolean options are `NULL` vs non-`NULL`. So while you
>pass `"1"` as the `value` parameter to `set_git_option()`, the parameter
>`"0"` would _enable that option just the same_, you would have to pass
>`NULL` to turn it off. I find that highly unintuitive, but that's not the
>fault of your patch. The pattern is established, and you did the right
>thing by following it.
>
>> + return 0;
>> }
>> return 1;
>> }
>
>As I said, the rest of the patch looks good to me. With the few
>suggestions I offered, I would be totally fine with this patch entering
>`next`.
>
>Thank you,
>Dscho
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v9] builtin/clone.c: add --reject-shallow option
2021-03-29 10:19 ` [PATCH v8] " Li Linchao via GitGitGadget
` (2 preceding siblings ...)
[not found] ` <f8b2582c913d11ebaddbd4ae5278bc1214940@gmx.de>
@ 2021-03-31 15:51 ` lilinchao via GitGitGadget
2021-03-31 19:14 ` Junio C Hamano
2021-04-01 10:46 ` [PATCH v10] " Li Linchao via GitGitGadget
3 siblings, 2 replies; 48+ messages in thread
From: lilinchao via GitGitGadget @ 2021-03-31 15:51 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, dscho, Jonathan Tan, lilinchao,
lilinchao
From: lilinchao <lilinchao@oschina.cn>
In some scenarios, users may want more history than the repository
offered for cloning, which happens to be a shallow repository, can
give them. But because users don't know it is a shallow repository
until they download it to local, users should have the option to
refuse to clone this kind of repository, and may want to exit the
process immediately without creating any unnecessary files.
Althought there is an option '--depth=x' for users to decide how
deep history they can fetch, but as the unshallow cloning's depth
is INFINITY, we can't know exactly the minimun 'x' value that can
satisfy the minimum integrity, so we can't pass 'x' value to --depth,
and expect this can obtain a complete history of a repository.
In other scenarios, if we have an API that allow us to import external
repository, and then perform various operations on the repo.
But if the imported is a shallow one(which is actually possible), it
will affect the subsequent operations. So we can choose to refuse to
clone, and let's just import a normal repository.
This patch offers a new option '--reject-shallow' that can reject to
clone a shallow repository.
Signed-off-by: Li Linchao <lilinchao@oschina.cn>
---
builtin/clone.c: add --reject-shallow option
Changes since v1:
* Rename --no-shallow to --reject-shallow
* Enable to reject a non-local clone
* Enable --[no-]reject-shallow from CLI override configuration.
* Add more testcases.
* Reword commit messages and relative documentation.
Changes since v3:
* Add support to reject clone shallow repo over https protocol
* Add testcase to reject clone shallow repo over https:// transport
* Reword commit messages and relative documentation according
suggestions from Junio.
Changes since v5:
* camelcase config variable
* warning client that source repo is shallow
* better support ssh:// and git:// protocol v1, v2
Changes since v6:
* use _() for warning/die statement
Signed-off-by: lilinchao lilinchao@oschina.cn
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v9
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v9
Pull-Request: https://github.com/gitgitgadget/git/pull/865
Range-diff vs v8:
1: 83a724f6d771 ! 1: 11043a2344dd builtin/clone.c: add --reject-shallow option
@@ Commit message
This patch offers a new option '--reject-shallow' that can reject to
clone a shallow repository.
- Signed-off-by: lilinchao <lilinchao@oschina.cn>
+ Signed-off-by: Li Linchao <lilinchao@oschina.cn>
## Documentation/config/clone.txt ##
@@ Documentation/config/clone.txt: clone.defaultRemoteName::
@@ Documentation/git-clone.txt: objects from the source repository into a pack in t
+--[no-]reject-shallow::
+ Fail if the source repository is a shallow repository.
+ The 'clone.rejectShallow' configuration variable can be used to
-+ give the default.
++ specify the default.
+
--bare::
Make a 'bare' Git repository. That is, instead of
@@ builtin/clone.c: static int option_no_checkout, option_bare, option_mirror, opti
static int option_local = -1, option_no_hardlinks, option_shared;
static int option_no_tags;
static int option_shallow_submodules;
-+static int option_shallow = -1; /* unspecified */
-+static int config_shallow = -1; /* unspecified */
++static int option_reject_shallow = -1; /* unspecified */
static int deepen;
static char *option_template, *option_depth, *option_since;
static char *option_origin = NULL;
@@ builtin/clone.c: static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
OPT_BOOL(0, "progress", &option_progress,
N_("force progress reporting")),
-+ OPT_BOOL(0, "reject-shallow", &option_shallow,
++ OPT_BOOL(0, "reject-shallow", &option_reject_shallow,
+ N_("don't clone shallow repository")),
OPT_BOOL('n', "no-checkout", &option_no_checkout,
N_("don't create a checkout")),
@@ builtin/clone.c: static int git_clone_config(const char *k, const char *v, void
free(remote_name);
remote_name = xstrdup(v);
}
-+ if (!strcmp(k, "clone.rejectshallow"))
-+ config_shallow = git_config_bool(k, v);
++ if (!strcmp(k, "clone.rejectshallow") && option_reject_shallow < 0)
++ option_reject_shallow = git_config_bool(k, v);
+
return git_default_config(k, v, cb);
}
-@@ builtin/clone.c: static int path_exists(const char *path)
- int cmd_clone(int argc, const char **argv, const char *prefix)
- {
- int is_bundle = 0, is_local;
-+ int reject_shallow = 0;
- const char *repo_name, *repo, *work_tree, *git_dir;
- char *path, *dir, *display_repo = NULL;
- int dest_exists, real_dest_exists = 0;
-@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
- */
- git_config(git_clone_config, NULL);
-
-+ /*
-+ * If option_shallow is specified from CLI option,
-+ * ignore config_shallow from git_clone_config.
-+ */
-+ if (config_shallow != -1)
-+ reject_shallow = config_shallow;
-+
-+ if (option_shallow != -1)
-+ reject_shallow = option_shallow;
-+
- /*
- * apply the remote name provided by --origin only after this second
- * call to git_config, to ensure it overrides all config-based values.
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
if (filter_options.choice)
warning(_("--filter is ignored in local clones; use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
-+ if (reject_shallow)
++ if (option_reject_shallow > 0)
+ die(_("source repository is shallow, reject to clone."));
if (option_local > 0)
warning(_("source repository is shallow, ignoring --local"));
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
transport_set_option(transport, TRANS_OPT_KEEP, "yes");
-+ if (reject_shallow)
++ if (option_reject_shallow > 0)
+ transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
if (option_depth)
transport_set_option(transport, TRANS_OPT_DEPTH,
@@ fetch-pack.c: static struct ref *do_fetch_pack(struct fetch_pack_args *args,
NULL);
- else if (si->nr_ours || si->nr_theirs)
+ else if (si->nr_ours || si->nr_theirs) {
-+ if (args->remote_shallow)
++ if (args->reject_shallow_remote)
+ die(_("source repository is shallow, reject to clone."));
alternate_shallow_file = setup_temporary_shallow(si->shallow);
- else
@@ fetch-pack.c: static void receive_shallow_info(struct fetch_pack_args *args,
prepare_shallow_info(si, shallows);
- if (si->nr_ours || si->nr_theirs)
+ if (si->nr_ours || si->nr_theirs) {
-+ if (args->remote_shallow)
++ if (args->reject_shallow_remote)
+ die(_("source repository is shallow, reject to clone."));
alternate_shallow_file =
setup_temporary_shallow(si->shallow);
@@ fetch-pack.h: struct fetch_pack_args {
unsigned self_contained_and_connected:1;
unsigned cloning:1;
unsigned update_shallow:1;
-+ unsigned remote_shallow:1;
++ unsigned reject_shallow_remote:1;
unsigned deepen:1;
/*
- ## t/t5606-clone-options.sh ##
-@@ t/t5606-clone-options.sh: GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
- export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+ ## t/t5601-clone.sh ##
+@@ t/t5601-clone.sh: test_expect_success 'partial clone using HTTP' '
+ partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
+ '
- . ./test-lib.sh
-+. "$TEST_DIRECTORY"/lib-httpd.sh
-+start_httpd
++test_expect_success 'reject cloning shallow repository using HTTP' '
++ test_when_finished "rm -rf repo" &&
++ git clone --bare --no-local --depth=1 src "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
++ test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git repo 2>err &&
++ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
++
++ git clone --no-reject-shallow $HTTPD_URL/smart/repo.git repo
++'
++
+ # DO NOT add non-httpd-specific tests here, because the last part of this
+ # test script is only executed when httpd is available and enabled.
+
+
+ ## t/t5606-clone-options.sh ##
+@@ t/t5606-clone-options.sh: test_expect_success 'setup' '
+ mkdir parent &&
+ (cd parent && git init &&
+ echo one >file && git add file &&
+- git commit -m one)
++ git commit -m one) &&
++ git clone --depth=1 --no-local parent shallow-repo
- test_expect_success 'setup' '
+ '
@@ t/t5606-clone-options.sh: test_expect_success 'disallows --bare with --separate-git-dir' '
'
-+test_expect_success 'reject cloning http shallow repository' '
-+ git clone --depth=1 --no-local parent shallow-repo &&
-+ git clone --bare --no-local shallow-repo "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
-+ test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git out 2>err &&
-+ test_i18ngrep -e "source repository is shallow, reject to clone." err
-+
-+'
-+
+test_expect_success 'reject cloning shallow repository' '
-+ rm -rf shallow-repo &&
-+ git clone --depth=1 --no-local parent shallow-repo &&
++ test_when_finished "rm -rf repo" &&
+ test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
-+ test_i18ngrep -e "source repository is shallow, reject to clone." err
++ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
++ git clone --no-reject-shallow shallow-repo repo
+'
+
+test_expect_success 'reject cloning non-local shallow repository' '
-+ rm -rf shallow-repo &&
-+ git clone --depth=1 --no-local parent shallow-repo &&
++ test_when_finished "rm -rf repo" &&
+ test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
-+ test_i18ngrep -e "source repository is shallow, reject to clone." err
++ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
++ git clone --no-reject-shallow --no-local shallow-repo repo
+'
+
-+test_expect_success 'clone shallow repository with --no-reject-shallow' '
-+ rm -rf shallow-repo &&
-+ git clone --depth=1 --no-local parent shallow-repo &&
-+ git clone --no-reject-shallow --no-local shallow-repo clone-repo
-+
-+'
-+
-+test_expect_success 'clone normal repository with --reject-shallow' '
-+ rm -rf clone-repo &&
-+ git clone --no-local parent normal-repo &&
-+ git clone --reject-shallow --no-local normal-repo clone-repo
-+
-+'
-+
-+test_expect_success 'unspecified any configs or options' '
-+ rm -rf shallow-repo clone-repo &&
-+ git clone --depth=1 --no-local parent shallow-repo &&
-+ git clone shallow-repo clone-repo
-+
++test_expect_success 'succeed cloning normal repository' '
++ test_when_finished "rm -rf chilad1 child2 child3 child4 " &&
++ git clone --reject-shallow parent child1 &&
++ git clone --reject-shallow --no-local parent child2 &&
++ git clone --no-reject-shallow parent child3 &&
++ git clone --no-reject-shallow --no-local parent child4
+'
+
test_expect_success 'uses "origin" for default remote name' '
@@ t/t5611-clone-config.sh: test_expect_success 'clone -c remote.<remote>.fetch=<re
test_cmp expect actual
'
-+test_expect_success 'clone.rejectshallow=true should reject cloning' '
-+ rm -rf child &&
-+ git clone --depth=1 --no-local . child &&
-+ test_must_fail git -c clone.rejectshallow=true clone --no-local child out 2>err &&
-+ test_i18ngrep -e "source repository is shallow, reject to clone." err
++test_expect_success 'set up shallow repository' '
++ git clone --depth=1 --no-local . shallow-repo
+'
+
-+test_expect_success 'clone.rejectshallow=false should succeed' '
-+ rm -rf child out &&
-+ git clone --depth=1 --no-local . child &&
-+ git -c clone.rejectshallow=false clone --no-local child out
-+'
++test_expect_success 'clone.rejectshallow=true should reject cloning shallow repo' '
++ test_when_finished "rm -rf out" &&
++ test_must_fail git -c clone.rejectshallow=true clone --no-local shallow-repo out 2>err &&
++ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
-+test_expect_success 'clone.rejectshallow=true should succeed with normal repo' '
-+ rm -rf child out &&
-+ git clone --no-local . child &&
-+ git -c clone.rejectshallow=true clone --no-local child out
++ git -c clone.rejectshallow=false clone --no-local shallow-repo out
+'
+
-+test_expect_success 'option --reject-shallow override clone.rejectshallow' '
-+ rm -rf child out &&
-+ git clone --depth=1 --no-local . child &&
-+ test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local child out 2>err &&
-+ test_i18ngrep -e "source repository is shallow, reject to clone." err
++test_expect_success 'option --[no-]reject-shallow override clone.rejectshallow config' '
++ test_when_finished "rm -rf out" &&
++ test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local shallow-repo out 2>err &&
++ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
++
++ git -c clone.rejectshallow=true clone --no-reject-shallow --no-local shallow-repo out
+'
+
-+test_expect_success 'option --no-reject-shallow override clone.rejectshallow' '
-+ rm -rf child out &&
-+ git clone --depth=1 --no-local . child &&
-+ git -c clone.rejectshallow=true clone --no-reject-shallow --no-local child out
++test_expect_success 'clone.rejectshallow=true should succeed cloning normal repo' '
++ test_when_finished "rm -rf out" &&
++ git -c clone.rejectshallow=true clone --no-local . out
+'
+
test_expect_success MINGW 'clone -c core.hideDotFiles' '
@@ transport.c: static int fetch_refs_via_pack(struct transport *transport,
args.stateless_rpc = transport->stateless_rpc;
args.server_options = transport->server_options;
args.negotiation_tips = data->options.negotiation_tips;
-+ args.remote_shallow = transport->smart_options->reject_shallow;
++ args.reject_shallow_remote = transport->smart_options->reject_shallow;
if (!data->got_remote_heads) {
int i;
Documentation/config/clone.txt | 4 ++++
Documentation/git-clone.txt | 7 ++++++-
builtin/clone.c | 10 ++++++++++
fetch-pack.c | 12 ++++++++----
fetch-pack.h | 1 +
t/t5601-clone.sh | 9 +++++++++
t/t5606-clone-options.sh | 27 ++++++++++++++++++++++++++-
t/t5611-clone-config.sh | 25 +++++++++++++++++++++++++
transport.c | 4 ++++
transport.h | 4 ++++
10 files changed, 97 insertions(+), 6 deletions(-)
diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 47de36a5fedf..7bcfbd18a52a 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -2,3 +2,7 @@ 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].
+
+clone.rejectShallow::
+ Reject to clone a repository if it is a shallow one, can be overridden by
+ passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 02d9c19cec75..3fe3810f1ce1 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,7 @@ SYNOPSIS
[--dissociate] [--separate-git-dir <git dir>]
[--depth <depth>] [--[no-]single-branch] [--no-tags]
[--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
- [--[no-]remote-submodules] [--jobs <n>] [--sparse]
+ [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
[--filter=<filter>] [--] <repository>
[<directory>]
@@ -149,6 +149,11 @@ objects from the source repository into a pack in the cloned repository.
--no-checkout::
No checkout of HEAD is performed after the clone is complete.
+--[no-]reject-shallow::
+ Fail if the source repository is a shallow repository.
+ The 'clone.rejectShallow' configuration variable can be used to
+ specify the default.
+
--bare::
Make a 'bare' Git repository. That is, instead of
creating `<directory>` and placing the administrative
diff --git a/builtin/clone.c b/builtin/clone.c
index 51e844a2de0a..d67a9f254c80 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,7 @@ static int option_no_checkout, option_bare, option_mirror, option_single_branch
static int option_local = -1, option_no_hardlinks, option_shared;
static int option_no_tags;
static int option_shallow_submodules;
+static int option_reject_shallow = -1; /* unspecified */
static int deepen;
static char *option_template, *option_depth, *option_since;
static char *option_origin = NULL;
@@ -90,6 +91,8 @@ static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
OPT_BOOL(0, "progress", &option_progress,
N_("force progress reporting")),
+ OPT_BOOL(0, "reject-shallow", &option_reject_shallow,
+ N_("don't clone shallow repository")),
OPT_BOOL('n', "no-checkout", &option_no_checkout,
N_("don't create a checkout")),
OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),
@@ -858,6 +861,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
free(remote_name);
remote_name = xstrdup(v);
}
+ if (!strcmp(k, "clone.rejectshallow") && option_reject_shallow < 0)
+ option_reject_shallow = git_config_bool(k, v);
+
return git_default_config(k, v, cb);
}
@@ -1216,6 +1222,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (filter_options.choice)
warning(_("--filter is ignored in local clones; use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
+ if (option_reject_shallow > 0)
+ die(_("source repository is shallow, reject to clone."));
if (option_local > 0)
warning(_("source repository is shallow, ignoring --local"));
is_local = 0;
@@ -1227,6 +1235,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
transport_set_option(transport, TRANS_OPT_KEEP, "yes");
+ if (option_reject_shallow > 0)
+ transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
if (option_depth)
transport_set_option(transport, TRANS_OPT_DEPTH,
option_depth);
diff --git a/fetch-pack.c b/fetch-pack.c
index fb04a76ca263..40392692ad07 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1129,9 +1129,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
if (args->deepen)
setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
NULL);
- else if (si->nr_ours || si->nr_theirs)
+ else if (si->nr_ours || si->nr_theirs) {
+ if (args->reject_shallow_remote)
+ die(_("source repository is shallow, reject to clone."));
alternate_shallow_file = setup_temporary_shallow(si->shallow);
- else
+ } else
alternate_shallow_file = NULL;
if (get_pack(args, fd, pack_lockfiles, NULL, sought, nr_sought,
&gitmodules_oids))
@@ -1498,10 +1500,12 @@ static void receive_shallow_info(struct fetch_pack_args *args,
* rejected (unless --update-shallow is set); do the same.
*/
prepare_shallow_info(si, shallows);
- if (si->nr_ours || si->nr_theirs)
+ if (si->nr_ours || si->nr_theirs) {
+ if (args->reject_shallow_remote)
+ die(_("source repository is shallow, reject to clone."));
alternate_shallow_file =
setup_temporary_shallow(si->shallow);
- else
+ } else
alternate_shallow_file = NULL;
} else {
alternate_shallow_file = NULL;
diff --git a/fetch-pack.h b/fetch-pack.h
index 736a3dae467a..f114d7277567 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -39,6 +39,7 @@ struct fetch_pack_args {
unsigned self_contained_and_connected:1;
unsigned cloning:1;
unsigned update_shallow:1;
+ unsigned reject_shallow_remote:1;
unsigned deepen:1;
/*
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index e7e6c089554c..329ae599fd3c 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -759,6 +759,15 @@ test_expect_success 'partial clone using HTTP' '
partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
'
+test_expect_success 'reject cloning shallow repository using HTTP' '
+ test_when_finished "rm -rf repo" &&
+ git clone --bare --no-local --depth=1 src "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git repo 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
+ git clone --no-reject-shallow $HTTPD_URL/smart/repo.git repo
+'
+
# DO NOT add non-httpd-specific tests here, because the last part of this
# test script is only executed when httpd is available and enabled.
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 428b0aac93fa..3a595c0f82c7 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -11,7 +11,8 @@ test_expect_success 'setup' '
mkdir parent &&
(cd parent && git init &&
echo one >file && git add file &&
- git commit -m one)
+ git commit -m one) &&
+ git clone --depth=1 --no-local parent shallow-repo
'
@@ -45,6 +46,30 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
'
+test_expect_success 'reject cloning shallow repository' '
+ test_when_finished "rm -rf repo" &&
+ test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
+ git clone --no-reject-shallow shallow-repo repo
+'
+
+test_expect_success 'reject cloning non-local shallow repository' '
+ test_when_finished "rm -rf repo" &&
+ test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
+ git clone --no-reject-shallow --no-local shallow-repo repo
+'
+
+test_expect_success 'succeed cloning normal repository' '
+ test_when_finished "rm -rf chilad1 child2 child3 child4 " &&
+ git clone --reject-shallow parent child1 &&
+ git clone --reject-shallow --no-local parent child2 &&
+ git clone --no-reject-shallow parent child3 &&
+ git clone --no-reject-shallow --no-local parent child4
+'
+
test_expect_success 'uses "origin" for default remote name' '
git clone parent clone-default-origin &&
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index 9f555b87ecdf..f8625f915821 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -95,6 +95,31 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
test_cmp expect actual
'
+test_expect_success 'set up shallow repository' '
+ git clone --depth=1 --no-local . shallow-repo
+'
+
+test_expect_success 'clone.rejectshallow=true should reject cloning shallow repo' '
+ test_when_finished "rm -rf out" &&
+ test_must_fail git -c clone.rejectshallow=true clone --no-local shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
+ git -c clone.rejectshallow=false clone --no-local shallow-repo out
+'
+
+test_expect_success 'option --[no-]reject-shallow override clone.rejectshallow config' '
+ test_when_finished "rm -rf out" &&
+ test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
+ git -c clone.rejectshallow=true clone --no-reject-shallow --no-local shallow-repo out
+'
+
+test_expect_success 'clone.rejectshallow=true should succeed cloning normal repo' '
+ test_when_finished "rm -rf out" &&
+ git -c clone.rejectshallow=true clone --no-local . out
+'
+
test_expect_success MINGW 'clone -c core.hideDotFiles' '
test_commit attributes .gitattributes "" &&
rm -rf child &&
diff --git a/transport.c b/transport.c
index 1c4ab676d1b1..b231894f9039 100644
--- a/transport.c
+++ b/transport.c
@@ -236,6 +236,9 @@ static int set_git_option(struct git_transport_options *opts,
list_objects_filter_die_if_populated(&opts->filter_options);
parse_list_objects_filter(&opts->filter_options, value);
return 0;
+ } else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
+ opts->reject_shallow = !!value;
+ return 0;
}
return 1;
}
@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
args.stateless_rpc = transport->stateless_rpc;
args.server_options = transport->server_options;
args.negotiation_tips = data->options.negotiation_tips;
+ args.reject_shallow_remote = transport->smart_options->reject_shallow;
if (!data->got_remote_heads) {
int i;
diff --git a/transport.h b/transport.h
index 24e15799e714..4d5db0a7f22b 100644
--- a/transport.h
+++ b/transport.h
@@ -14,6 +14,7 @@ struct git_transport_options {
unsigned check_self_contained_and_connected : 1;
unsigned self_contained_and_connected : 1;
unsigned update_shallow : 1;
+ unsigned reject_shallow : 1;
unsigned deepen_relative : 1;
/* see documentation of corresponding flag in fetch-pack.h */
@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
/* Aggressively fetch annotated tags if possible */
#define TRANS_OPT_FOLLOWTAGS "followtags"
+/* Reject shallow repo transport */
+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
+
/* Accept refs that may update .git/shallow without --depth */
#define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
base-commit: a65ce7f831aa5fcc596c6d23fcde543d98b39bd7
--
gitgitgadget
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH v9] builtin/clone.c: add --reject-shallow option
2021-03-31 15:51 ` [PATCH v9] " lilinchao via GitGitGadget
@ 2021-03-31 19:14 ` Junio C Hamano
2021-03-31 22:24 ` Johannes Schindelin
2021-04-01 10:46 ` [PATCH v10] " Li Linchao via GitGitGadget
1 sibling, 1 reply; 48+ messages in thread
From: Junio C Hamano @ 2021-03-31 19:14 UTC (permalink / raw)
To: lilinchao via GitGitGadget
Cc: git, Derrick Stolee, dscho, Jonathan Tan, lilinchao
"lilinchao via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: lilinchao <lilinchao@oschina.cn>
This is not corrected, and no longer matches your sign-off.
I can edit your 'From:' to match this time, but please make sure
they match for the next time.
> In some scenarios, users may want more history than the repository
> offered for cloning, which happens to be a shallow repository, can
> give them. But because users don't know it is a shallow repository
> until they download it to local, users should have the option to
I find the "should" too strong, but let's keep reading.
> refuse to clone this kind of repository, and may want to exit the
> process immediately without creating any unnecessary files.
Yes, that is too strong and also redundant.
> Althought there is an option '--depth=x' for users to decide how
> deep history they can fetch, but as the unshallow cloning's depth
"Although"; if you begin with "although", you shouldn't write "but".
> is INFINITY, we can't know exactly the minimun 'x' value that can
> satisfy the minimum integrity,
> so we can't pass 'x' value to --depth,
> and expect this can obtain a complete history of a repository.
If the argument were "we might start with depth x, and the source
may be deep enough to give us x right now, but we may want to deepen
more than they can offer, and such a user would want to be able to
say 'I want depth=x now, but make sure they are not shallow'", I
would understand it, but I do not see where the "minimum integrity"
comes from---there doesn't appear to be anything related to
integrity here.
> In other scenarios, if we have an API that allow us to import external
"allows"
> repository, and then perform various operations on the repo.
> But if the imported is a shallow one(which is actually possible), it
> will affect the subsequent operations. So we can choose to refuse to
> clone, and let's just import a normal repository.
I'd suggest dropping this entire paragraph. That is not any new
scenario at all. API or not, you essentially just said "you can do
various things on your clone once you have it, but some things you
may want to do you would want a full history". That is what you
started the whole discussion above and does not add any new
information.
> This patch offers a new option '--reject-shallow' that can reject to
> clone a shallow repository.
Teach '--reject-shallow' option to "git clone" to abort as
soon as we find out that we are cloning from a shallow
repository.
perhaps? cf. Documentation/SubmittingPatches[[describe-changes]]
especially [[imperative-mood]].
> Signed-off-by: Li Linchao <lilinchao@oschina.cn>
> ---
> builtin/clone.c: add --reject-shallow option
>
> Changes since v1:
>...
> Changes since v6:
>
> * use _() for warning/die statement
What are the more recent changes?
> Signed-off-by: lilinchao lilinchao@oschina.cn
> @@ -858,6 +861,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
> free(remote_name);
> remote_name = xstrdup(v);
> }
> + if (!strcmp(k, "clone.rejectshallow") && option_reject_shallow < 0)
> + option_reject_shallow = git_config_bool(k, v);
Does this "single variable is enough" really work?
Imagine that the first time around we'd read from $HOME/.gitconfig
that says true (flips the variable from "unspecified"). Further
imagine that we are running "git clone -c config.rejectShallow=no"
to countermand the global config. We call write_config() to write
the extra configuration value out, and then call git_config() to
read from the repository configuration again.
Because of the value taken from $HOME/.gitconfig, however, the
attempt to override is silently ignored, isn't it?
Other than that, the changes to the code from the previous round
looked sensible.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v9] builtin/clone.c: add --reject-shallow option
2021-03-31 19:14 ` Junio C Hamano
@ 2021-03-31 22:24 ` Johannes Schindelin
2021-03-31 22:37 ` Junio C Hamano
0 siblings, 1 reply; 48+ messages in thread
From: Johannes Schindelin @ 2021-03-31 22:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: lilinchao via GitGitGadget, git, Derrick Stolee, Jonathan Tan,
lilinchao
Hi Junio,
On Wed, 31 Mar 2021, Junio C Hamano wrote:
> > @@ -858,6 +861,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
> > free(remote_name);
> > remote_name = xstrdup(v);
> > }
> > + if (!strcmp(k, "clone.rejectshallow") && option_reject_shallow < 0)
> > + option_reject_shallow = git_config_bool(k, v);
>
> Does this "single variable is enough" really work?
>
> Imagine that the first time around we'd read from $HOME/.gitconfig
> that says true (flips the variable from "unspecified"). Further
> imagine that we are running "git clone -c config.rejectShallow=no"
> to countermand the global config. We call write_config() to write
> the extra configuration value out, and then call git_config() to
> read from the repository configuration again.
>
> Because of the value taken from $HOME/.gitconfig, however, the
> attempt to override is silently ignored, isn't it?
That's my mistake, and you don't even need a second config to see the
problem:
[clone]
rejectShallow = false
rejectShallow = true
Clearly, the second one should override the first one, but the code I
suggested (and which was integrated into this iteration) simply does not
work correctly.
So yes, I think you're right, and we need to have that second variable,
and then do something like this:
if (option_reject_shallow < 0)
option_reject_shallow = config_reject_shallow;
Sorry for the trouble!
Dscho
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH v9] builtin/clone.c: add --reject-shallow option
2021-03-31 22:24 ` Johannes Schindelin
@ 2021-03-31 22:37 ` Junio C Hamano
0 siblings, 0 replies; 48+ messages in thread
From: Junio C Hamano @ 2021-03-31 22:37 UTC (permalink / raw)
To: Johannes Schindelin
Cc: lilinchao via GitGitGadget, git, Derrick Stolee, Jonathan Tan,
lilinchao
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> Does this "single variable is enough" really work?
>> ...
> That's my mistake, and you don't even need a second config to see the
> problem:
>
> [clone]
> rejectShallow = false
> rejectShallow = true
Yup, that is a much simpler example.
> Sorry for the trouble!
Don't feel bad---in an earlier round of review I made a similar
mistake to wonder if these two variables can be reduced into one
myself ;-). As you said, this one is special in making two calls to
git_config() and need to be handled carefully.
Thanks.
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH v10] builtin/clone.c: add --reject-shallow option
2021-03-31 15:51 ` [PATCH v9] " lilinchao via GitGitGadget
2021-03-31 19:14 ` Junio C Hamano
@ 2021-04-01 10:46 ` Li Linchao via GitGitGadget
1 sibling, 0 replies; 48+ messages in thread
From: Li Linchao via GitGitGadget @ 2021-04-01 10:46 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Derrick Stolee, dscho, Jonathan Tan, Li Linchao,
Li Linchao
From: Li Linchao <lilinchao@oschina.cn>
In some scenarios, users may want more history than the repository
offered for cloning, which happens to be a shallow repository, can
give them. But because users don't know it is a shallow repository
until they download it to local, we may want to refuse to clone
this kind of repository, without creating any unnecessary files.
Althought there is an option '--depth=x' for users to decide how
deep history they can fetch, as the unshallow cloning's depth is
INFINITY, we might start with depth x, and the source may be deep
enough to give us x right now, but we may want to deepen more than
they can offer, and such a user would want to be able to say
"I want depth=x now, but make sure they are not shallow".
it's hard to find such a x value to make sure they are not shallow.
Teach '--reject-shallow' option to "git clone" to abort as
soon as we find out that we are cloning from a shallow
repository.
Signed-off-by: Li Linchao <lilinchao@oschina.cn>
---
builtin/clone.c: add --reject-shallow option
Changes since v1:
* Rename --no-shallow to --reject-shallow
* Enable to reject a non-local clone
* Enable --[no-]reject-shallow from CLI override configuration.
* Add more testcases.
* Reword commit messages and relative documentation.
Changes since v3:
* Add support to reject clone shallow repo over https protocol
* Add testcase to reject clone shallow repo over https:// transport
* Reword commit messages and relative documentation according
suggestions from Junio.
Changes since v5:
* camelcase config variable
* warning client that source repo is shallow
* better support ssh:// and git:// protocol v1, v2
Changes since v6:
* use _() for warning/die statement
Changes since v7:
* Clear reviewed-by trailer
* Drop warning() statement
* Reword a few test titles
Changes since v8:
* Use single variable for option and config
* Rename remote_shallow to reject_shallow_remote
* Simplify test cases
Changes since v9:
* Reword commit message
* Use option_reject_shallow and config_reject_shallow
Signed-off-by: Li Linchao lilinchao@oschina.cn
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-865%2FCactusinhand%2Fgit-clone-options-v10
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-865/Cactusinhand/git-clone-options-v10
Pull-Request: https://github.com/gitgitgadget/git/pull/865
Range-diff vs v9:
1: 11043a2344dd ! 1: cacda5a6af07 builtin/clone.c: add --reject-shallow option
@@
## Metadata ##
-Author: lilinchao <lilinchao@oschina.cn>
+Author: Li Linchao <lilinchao@oschina.cn>
## Commit message ##
builtin/clone.c: add --reject-shallow option
@@ Commit message
In some scenarios, users may want more history than the repository
offered for cloning, which happens to be a shallow repository, can
give them. But because users don't know it is a shallow repository
- until they download it to local, users should have the option to
- refuse to clone this kind of repository, and may want to exit the
- process immediately without creating any unnecessary files.
+ until they download it to local, we may want to refuse to clone
+ this kind of repository, without creating any unnecessary files.
Althought there is an option '--depth=x' for users to decide how
- deep history they can fetch, but as the unshallow cloning's depth
- is INFINITY, we can't know exactly the minimun 'x' value that can
- satisfy the minimum integrity, so we can't pass 'x' value to --depth,
- and expect this can obtain a complete history of a repository.
+ deep history they can fetch, as the unshallow cloning's depth is
+ INFINITY, we might start with depth x, and the source may be deep
+ enough to give us x right now, but we may want to deepen more than
+ they can offer, and such a user would want to be able to say
+ "I want depth=x now, but make sure they are not shallow".
+ it's hard to find such a x value to make sure they are not shallow.
- In other scenarios, if we have an API that allow us to import external
- repository, and then perform various operations on the repo.
- But if the imported is a shallow one(which is actually possible), it
- will affect the subsequent operations. So we can choose to refuse to
- clone, and let's just import a normal repository.
-
- This patch offers a new option '--reject-shallow' that can reject to
- clone a shallow repository.
+ Teach '--reject-shallow' option to "git clone" to abort as
+ soon as we find out that we are cloning from a shallow
+ repository.
Signed-off-by: Li Linchao <lilinchao@oschina.cn>
@@ builtin/clone.c: static int option_no_checkout, option_bare, option_mirror, opti
static int option_no_tags;
static int option_shallow_submodules;
+static int option_reject_shallow = -1; /* unspecified */
++static int config_reject_shallow = -1; /* unspecified */
static int deepen;
static char *option_template, *option_depth, *option_since;
static char *option_origin = NULL;
@@ builtin/clone.c: static int git_clone_config(const char *k, const char *v, void
free(remote_name);
remote_name = xstrdup(v);
}
-+ if (!strcmp(k, "clone.rejectshallow") && option_reject_shallow < 0)
-+ option_reject_shallow = git_config_bool(k, v);
++ if (!strcmp(k, "clone.rejectshallow"))
++ config_reject_shallow = git_config_bool(k, v);
+
return git_default_config(k, v, cb);
}
+@@ builtin/clone.c: static int path_exists(const char *path)
+ int cmd_clone(int argc, const char **argv, const char *prefix)
+ {
+ int is_bundle = 0, is_local;
++ int reject_shallow = 0;
+ const char *repo_name, *repo, *work_tree, *git_dir;
+ char *path, *dir, *display_repo = NULL;
+ int dest_exists, real_dest_exists = 0;
+@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
+ */
+ git_config(git_clone_config, NULL);
+
++ /*
++ * If option_reject_shallow is specified from CLI option,
++ * ignore config_reject_shallow from git_clone_config.
++ */
++ if (config_reject_shallow != -1)
++ reject_shallow = config_reject_shallow;
++ if (option_reject_shallow != -1)
++ reject_shallow = option_reject_shallow;
++
+ /*
+ * apply the remote name provided by --origin only after this second
+ * call to git_config, to ensure it overrides all config-based values.
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
if (filter_options.choice)
warning(_("--filter is ignored in local clones; use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
-+ if (option_reject_shallow > 0)
++ if (reject_shallow)
+ die(_("source repository is shallow, reject to clone."));
if (option_local > 0)
warning(_("source repository is shallow, ignoring --local"));
@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
transport_set_option(transport, TRANS_OPT_KEEP, "yes");
-+ if (option_reject_shallow > 0)
++ if (reject_shallow)
+ transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
if (option_depth)
transport_set_option(transport, TRANS_OPT_DEPTH,
Documentation/config/clone.txt | 4 ++++
Documentation/git-clone.txt | 7 ++++++-
builtin/clone.c | 21 +++++++++++++++++++++
fetch-pack.c | 12 ++++++++----
fetch-pack.h | 1 +
t/t5601-clone.sh | 9 +++++++++
t/t5606-clone-options.sh | 27 ++++++++++++++++++++++++++-
t/t5611-clone-config.sh | 25 +++++++++++++++++++++++++
transport.c | 4 ++++
transport.h | 4 ++++
10 files changed, 108 insertions(+), 6 deletions(-)
diff --git a/Documentation/config/clone.txt b/Documentation/config/clone.txt
index 47de36a5fedf..7bcfbd18a52a 100644
--- a/Documentation/config/clone.txt
+++ b/Documentation/config/clone.txt
@@ -2,3 +2,7 @@ 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].
+
+clone.rejectShallow::
+ Reject to clone a repository if it is a shallow one, can be overridden by
+ passing option `--reject-shallow` in command line. See linkgit:git-clone[1]
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 02d9c19cec75..3fe3810f1ce1 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,7 @@ SYNOPSIS
[--dissociate] [--separate-git-dir <git dir>]
[--depth <depth>] [--[no-]single-branch] [--no-tags]
[--recurse-submodules[=<pathspec>]] [--[no-]shallow-submodules]
- [--[no-]remote-submodules] [--jobs <n>] [--sparse]
+ [--[no-]remote-submodules] [--jobs <n>] [--sparse] [--[no-]reject-shallow]
[--filter=<filter>] [--] <repository>
[<directory>]
@@ -149,6 +149,11 @@ objects from the source repository into a pack in the cloned repository.
--no-checkout::
No checkout of HEAD is performed after the clone is complete.
+--[no-]reject-shallow::
+ Fail if the source repository is a shallow repository.
+ The 'clone.rejectShallow' configuration variable can be used to
+ specify the default.
+
--bare::
Make a 'bare' Git repository. That is, instead of
creating `<directory>` and placing the administrative
diff --git a/builtin/clone.c b/builtin/clone.c
index 51e844a2de0a..2a5485b72499 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,8 @@ static int option_no_checkout, option_bare, option_mirror, option_single_branch
static int option_local = -1, option_no_hardlinks, option_shared;
static int option_no_tags;
static int option_shallow_submodules;
+static int option_reject_shallow = -1; /* unspecified */
+static int config_reject_shallow = -1; /* unspecified */
static int deepen;
static char *option_template, *option_depth, *option_since;
static char *option_origin = NULL;
@@ -90,6 +92,8 @@ static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
OPT_BOOL(0, "progress", &option_progress,
N_("force progress reporting")),
+ OPT_BOOL(0, "reject-shallow", &option_reject_shallow,
+ N_("don't clone shallow repository")),
OPT_BOOL('n', "no-checkout", &option_no_checkout,
N_("don't create a checkout")),
OPT_BOOL(0, "bare", &option_bare, N_("create a bare repository")),
@@ -858,6 +862,9 @@ static int git_clone_config(const char *k, const char *v, void *cb)
free(remote_name);
remote_name = xstrdup(v);
}
+ if (!strcmp(k, "clone.rejectshallow"))
+ config_reject_shallow = git_config_bool(k, v);
+
return git_default_config(k, v, cb);
}
@@ -963,6 +970,7 @@ static int path_exists(const char *path)
int cmd_clone(int argc, const char **argv, const char *prefix)
{
int is_bundle = 0, is_local;
+ int reject_shallow = 0;
const char *repo_name, *repo, *work_tree, *git_dir;
char *path, *dir, *display_repo = NULL;
int dest_exists, real_dest_exists = 0;
@@ -1156,6 +1164,15 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
*/
git_config(git_clone_config, NULL);
+ /*
+ * If option_reject_shallow is specified from CLI option,
+ * ignore config_reject_shallow from git_clone_config.
+ */
+ if (config_reject_shallow != -1)
+ reject_shallow = config_reject_shallow;
+ if (option_reject_shallow != -1)
+ reject_shallow = option_reject_shallow;
+
/*
* apply the remote name provided by --origin only after this second
* call to git_config, to ensure it overrides all config-based values.
@@ -1216,6 +1233,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (filter_options.choice)
warning(_("--filter is ignored in local clones; use file:// instead."));
if (!access(mkpath("%s/shallow", path), F_OK)) {
+ if (reject_shallow)
+ die(_("source repository is shallow, reject to clone."));
if (option_local > 0)
warning(_("source repository is shallow, ignoring --local"));
is_local = 0;
@@ -1227,6 +1246,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
transport_set_option(transport, TRANS_OPT_KEEP, "yes");
+ if (reject_shallow)
+ transport_set_option(transport, TRANS_OPT_REJECT_SHALLOW, "1");
if (option_depth)
transport_set_option(transport, TRANS_OPT_DEPTH,
option_depth);
diff --git a/fetch-pack.c b/fetch-pack.c
index fb04a76ca263..40392692ad07 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1129,9 +1129,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
if (args->deepen)
setup_alternate_shallow(&shallow_lock, &alternate_shallow_file,
NULL);
- else if (si->nr_ours || si->nr_theirs)
+ else if (si->nr_ours || si->nr_theirs) {
+ if (args->reject_shallow_remote)
+ die(_("source repository is shallow, reject to clone."));
alternate_shallow_file = setup_temporary_shallow(si->shallow);
- else
+ } else
alternate_shallow_file = NULL;
if (get_pack(args, fd, pack_lockfiles, NULL, sought, nr_sought,
&gitmodules_oids))
@@ -1498,10 +1500,12 @@ static void receive_shallow_info(struct fetch_pack_args *args,
* rejected (unless --update-shallow is set); do the same.
*/
prepare_shallow_info(si, shallows);
- if (si->nr_ours || si->nr_theirs)
+ if (si->nr_ours || si->nr_theirs) {
+ if (args->reject_shallow_remote)
+ die(_("source repository is shallow, reject to clone."));
alternate_shallow_file =
setup_temporary_shallow(si->shallow);
- else
+ } else
alternate_shallow_file = NULL;
} else {
alternate_shallow_file = NULL;
diff --git a/fetch-pack.h b/fetch-pack.h
index 736a3dae467a..f114d7277567 100644
--- a/fetch-pack.h
+++ b/fetch-pack.h
@@ -39,6 +39,7 @@ struct fetch_pack_args {
unsigned self_contained_and_connected:1;
unsigned cloning:1;
unsigned update_shallow:1;
+ unsigned reject_shallow_remote:1;
unsigned deepen:1;
/*
diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index e7e6c089554c..329ae599fd3c 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -759,6 +759,15 @@ test_expect_success 'partial clone using HTTP' '
partial_clone "$HTTPD_DOCUMENT_ROOT_PATH/server" "$HTTPD_URL/smart/server"
'
+test_expect_success 'reject cloning shallow repository using HTTP' '
+ test_when_finished "rm -rf repo" &&
+ git clone --bare --no-local --depth=1 src "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ test_must_fail git clone --reject-shallow $HTTPD_URL/smart/repo.git repo 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
+ git clone --no-reject-shallow $HTTPD_URL/smart/repo.git repo
+'
+
# DO NOT add non-httpd-specific tests here, because the last part of this
# test script is only executed when httpd is available and enabled.
diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
index 428b0aac93fa..3a595c0f82c7 100755
--- a/t/t5606-clone-options.sh
+++ b/t/t5606-clone-options.sh
@@ -11,7 +11,8 @@ test_expect_success 'setup' '
mkdir parent &&
(cd parent && git init &&
echo one >file && git add file &&
- git commit -m one)
+ git commit -m one) &&
+ git clone --depth=1 --no-local parent shallow-repo
'
@@ -45,6 +46,30 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
'
+test_expect_success 'reject cloning shallow repository' '
+ test_when_finished "rm -rf repo" &&
+ test_must_fail git clone --reject-shallow shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
+ git clone --no-reject-shallow shallow-repo repo
+'
+
+test_expect_success 'reject cloning non-local shallow repository' '
+ test_when_finished "rm -rf repo" &&
+ test_must_fail git clone --reject-shallow --no-local shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
+ git clone --no-reject-shallow --no-local shallow-repo repo
+'
+
+test_expect_success 'succeed cloning normal repository' '
+ test_when_finished "rm -rf chilad1 child2 child3 child4 " &&
+ git clone --reject-shallow parent child1 &&
+ git clone --reject-shallow --no-local parent child2 &&
+ git clone --no-reject-shallow parent child3 &&
+ git clone --no-reject-shallow --no-local parent child4
+'
+
test_expect_success 'uses "origin" for default remote name' '
git clone parent clone-default-origin &&
diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
index 9f555b87ecdf..f8625f915821 100755
--- a/t/t5611-clone-config.sh
+++ b/t/t5611-clone-config.sh
@@ -95,6 +95,31 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
test_cmp expect actual
'
+test_expect_success 'set up shallow repository' '
+ git clone --depth=1 --no-local . shallow-repo
+'
+
+test_expect_success 'clone.rejectshallow=true should reject cloning shallow repo' '
+ test_when_finished "rm -rf out" &&
+ test_must_fail git -c clone.rejectshallow=true clone --no-local shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
+ git -c clone.rejectshallow=false clone --no-local shallow-repo out
+'
+
+test_expect_success 'option --[no-]reject-shallow override clone.rejectshallow config' '
+ test_when_finished "rm -rf out" &&
+ test_must_fail git -c clone.rejectshallow=false clone --reject-shallow --no-local shallow-repo out 2>err &&
+ test_i18ngrep -e "source repository is shallow, reject to clone." err &&
+
+ git -c clone.rejectshallow=true clone --no-reject-shallow --no-local shallow-repo out
+'
+
+test_expect_success 'clone.rejectshallow=true should succeed cloning normal repo' '
+ test_when_finished "rm -rf out" &&
+ git -c clone.rejectshallow=true clone --no-local . out
+'
+
test_expect_success MINGW 'clone -c core.hideDotFiles' '
test_commit attributes .gitattributes "" &&
rm -rf child &&
diff --git a/transport.c b/transport.c
index 1c4ab676d1b1..b231894f9039 100644
--- a/transport.c
+++ b/transport.c
@@ -236,6 +236,9 @@ static int set_git_option(struct git_transport_options *opts,
list_objects_filter_die_if_populated(&opts->filter_options);
parse_list_objects_filter(&opts->filter_options, value);
return 0;
+ } else if (!strcmp(name, TRANS_OPT_REJECT_SHALLOW)) {
+ opts->reject_shallow = !!value;
+ return 0;
}
return 1;
}
@@ -370,6 +373,7 @@ static int fetch_refs_via_pack(struct transport *transport,
args.stateless_rpc = transport->stateless_rpc;
args.server_options = transport->server_options;
args.negotiation_tips = data->options.negotiation_tips;
+ args.reject_shallow_remote = transport->smart_options->reject_shallow;
if (!data->got_remote_heads) {
int i;
diff --git a/transport.h b/transport.h
index 24e15799e714..4d5db0a7f22b 100644
--- a/transport.h
+++ b/transport.h
@@ -14,6 +14,7 @@ struct git_transport_options {
unsigned check_self_contained_and_connected : 1;
unsigned self_contained_and_connected : 1;
unsigned update_shallow : 1;
+ unsigned reject_shallow : 1;
unsigned deepen_relative : 1;
/* see documentation of corresponding flag in fetch-pack.h */
@@ -194,6 +195,9 @@ void transport_check_allowed(const char *type);
/* Aggressively fetch annotated tags if possible */
#define TRANS_OPT_FOLLOWTAGS "followtags"
+/* Reject shallow repo transport */
+#define TRANS_OPT_REJECT_SHALLOW "rejectshallow"
+
/* Accept refs that may update .git/shallow without --depth */
#define TRANS_OPT_UPDATE_SHALLOW "updateshallow"
base-commit: a65ce7f831aa5fcc596c6d23fcde543d98b39bd7
--
gitgitgadget
^ permalink raw reply related [flat|nested] 48+ messages in thread