git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Li Linchao via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <stolee@gmail.com>,
	dscho <johannes.schindelin@gmx.de>,
	Li Linchao <lilinchao@oschina.cn>
Subject: Re: [PATCH] builtin/clone.c: add --no-shallow option
Date: Wed, 03 Feb 2021 21:50:24 -0800	[thread overview]
Message-ID: <xmqq35yc9yan.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <pull.865.git.1612409491842.gitgitgadget@gmail.com> (Li Linchao via GitGitGadget's message of "Thu, 04 Feb 2021 03:31:31 +0000")

"Li Linchao via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: lilinchao <lilinchao@oschina.cn>
>
> This patch add a new option that reject to clone a shallow repository.

A canonical form of our log message starts by explaining the need,
and then presents the solution at the end.

> Clients don't know it's a shallow repository until they download it
> locally, in some scenariors, clients just don't want to clone this kind

"scenarios".  "in some scenarios" would have to be clarified a bit
more to justify why it is a good idea to have such a feature.

> of repository, and want to exit the process immediately without creating
> any unnecessary files.

"clients don't know it's a shallow repository until they download"
leading to "so let's reject immediately upon finding out that they
are shallow" does make sense as a flow of thought, though.

> +--no-shallow::
> +	Don't clone a shallow source repository. In some scenariors, clients

"scenarios" (no 'r').

> diff --git a/builtin/clone.c b/builtin/clone.c
> old mode 100644
> new mode 100755

Unwarranted "chmod +x"; accidents do happen, but please be careful
before making what you did public ;-)

> @@ -90,6 +91,7 @@ static struct option builtin_clone_options[] = {
>  	OPT__VERBOSITY(&option_verbosity),
>  	OPT_BOOL(0, "progress", &option_progress,
>  		 N_("force progress reporting")),
> +	OPT_BOOL(0, "no-shallow", &option_no_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")),

It is a bad idea to give a name that begins with "no-" to an option
whose default can be tweaked by a configuration variable [*].  If
the configuration is named "rejectshallow", perhaps it is better to
call it "--reject-shallow" instead.

This is because configured default must be overridable from the
command line.  I.e. even if you have in your ~/.gitconfig this:

    [clone]
        rejectshallow = true

you should be able to say "allow it only this time", with

    $ git clone --no-reject-shallow http://github.com/git/git/ git

and you do not want to have to say "--no-no-shallow", which sounds
just silly.

	Side note. it is a bad idea in general, even if the option
	does not have corresponding configuration variable.  The
	existing "no-checkout" is a historical accident that
	happened long time ago and cannot be removed due to
	compatibility.  Let's not introduce a new option that
	follows such a bad pattern.

> @@ -963,6 +968,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 is_shallow = 0;
>  	const char *repo_name, *repo, *work_tree, *git_dir;
>  	char *path, *dir, *display_repo = NULL;
>  	int dest_exists, real_dest_exists = 0;
> @@ -1215,6 +1221,7 @@ 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)) {
> +			is_shallow = 1;
>  			if (option_local > 0)
>  				warning(_("source repository is shallow, ignoring --local"));
>  			is_local = 0;

This change is to the local clone codepath.  Cloning over the wire
would not go through this part.  And throughout the patch, this is
the only place that sets is_shallow to 1.

Also let's note that this is after we called parse_options(), so the
value of option_no_shallow is known at this point.

So, this patch does not even *need* to introduce a new "is_shallow"
variable at all.  It only needs to add

                        if (option_no_shallow)
                                die(...);

instead of adding "is_shallow = 1" to the above hunk.

I somehow think that this is only half a feature---wouldn't it be
more useful if we also rejected a non-local clone from a shallow
repository?

And for that ...


> diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh
> index 7f082fb23b6a..9d310dbb158a 100755
> --- a/t/t5606-clone-options.sh
> +++ b/t/t5606-clone-options.sh
> @@ -42,6 +42,13 @@ test_expect_success 'disallows --bare with --separate-git-dir' '
>  
>  '
>  
> +test_expect_success 'reject clone shallow repository' '
> +	git clone --depth=1 --no-local parent shallow-repo &&
> +	test_must_fail git clone --no-shallow shallow-repo out 2>err &&
> +	test_i18ngrep -e "source repository is shallow, reject to clone." err
> +
> +'
> +

... in addition to the test for a local clone above, you'd also want
to test a non-local clone, perhaps like so:

test_expect_success 'reject clone shallow repository' '
	rm -fr shallow-repo &&
	git clone --depth=1 --no-local parent shallow-repo &&
	test_must_fail git clone --no-shallow --no-local shallow-repo out 2>err &&
	test_i18ngrep -e "source repository is shallow, reject to clone." err

'

Ditto for the other test script.

Also, you would want to make sure that the command line overrides
the configured default.  I.e.

	git -c clone.rejectshallow=false clone --reject-shallow

should refuse to clone from a shallow one, while there should be a
way to countermand a configured "I always refuse to clone from a
shallow repository" with "but let's allow it only this time", i.e.

	git -c clone.rejectshallow=true clone --no-reject-shallow

or something along the line.


> diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
> index 8e0fd398236b..3aab86ad4def 100755
> --- a/t/t5611-clone-config.sh
> +++ b/t/t5611-clone-config.sh
> @@ -92,6 +92,13 @@ test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'clone -c clone.rejectshallow' '
> +	rm -rf child &&
> +	git clone --depth=1 --no-local . child &&
> +	test_must_fail git clone -c clone.rejectshallow child out 2>err &&

This is not quite right, even though it may happen to work.  The
"clone.rejectshallow" variable is a configuration about what should
happen when creating a new repository by cloning, so letting "git
clone -c var[=val]" to set the variable _in_ the resulting repository
would not make much sense.  Even if the clone succeeded, nobody would
look at that particular configuration variable that is set in the
resulting repository.

I think it would communicate to the readers better what we are
trying to do, if we write

	test_must_fail git -c clone.rejectshallow=true clone child out

instead.

Thanks.

  reply	other threads:[~2021-02-04  5:51 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-04  3:31 [PATCH] builtin/clone.c: add --no-shallow option Li Linchao via GitGitGadget
2021-02-04  5:50 ` Junio C Hamano [this message]
2021-02-04 10:32   ` lilinchao
2021-02-04 18:36     ` Junio C Hamano
2021-02-04 14:00 ` Johannes Schindelin
2021-02-04 18:24   ` Junio C Hamano
2021-02-08  8:31 ` [PATCH v2 0/2] " Li Linchao via GitGitGadget
2021-02-08  8:31   ` [PATCH v2 1/2] " lilinchao via GitGitGadget
2021-02-08  8:31   ` [PATCH v2 2/2] builtin/clone.c: add --reject-shallow option lilinchao via GitGitGadget
2021-02-08 13:33   ` [PATCH v2 0/2] builtin/clone.c: add --no-shallow option Derrick Stolee
     [not found]   ` <32bb0d006a1211ebb94254a05087d89a835@gmail.com>
2021-02-08 13:48     ` lilinchao
2021-02-08 14:12   ` [PATCH v3] builtin/clone.c: add --reject-shallow option Li Linchao via GitGitGadget
2021-02-09 20:32     ` Junio C Hamano
     [not found]     ` <026bd8966b1611eb975aa4badb2c2b1190694@pobox.com>
2021-02-10  9:07       ` lilinchao
2021-02-10 16:27         ` Junio C Hamano
     [not found]         ` <eaa219a86bbc11ebb6c7a4badb2c2b1165032@pobox.com>
2021-02-20 10:40           ` lilinchao
2021-02-21  7:05     ` [PATCH v4] " Li Linchao via GitGitGadget
2021-02-22 18:12       ` Junio C Hamano
2021-03-01 22:03         ` Jonathan Tan
2021-03-01 22:34           ` Junio C Hamano
2021-03-02  8:44           ` lilinchao
2021-03-03 23:59           ` Junio C Hamano
2021-03-04  1:53             ` Jonathan Tan
     [not found]       ` <8f3c00de753911eb93d3d4ae5278bc1270191@pobox.com>
2021-02-28 17:58         ` lilinchao
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-04  6:26             ` 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
2021-03-12 18:25           ` lilinchao
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>
2021-03-26  3:34               ` lilinchao
     [not found]             ` <7a71c96c8dbd11eb8bb0d4ae5278bc1296681@pobox.com>
2021-03-26  3:49               ` lilinchao
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
2021-03-31 13:30                   ` Johannes Schindelin
     [not found]               ` <f8b2582c913d11ebaddbd4ae5278bc1214940@gmx.de>
2021-03-31 11:03                 ` lilinchao
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-03-31 22:37                     ` Junio C Hamano
2021-04-01 10:46                 ` [PATCH v10] " Li Linchao via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq35yc9yan.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=lilinchao@oschina.cn \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).