> 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. 一条规范的 commit 信息格式应该是在开始就解释更改需求，然后在结尾提供解决方案。
> 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. 这里表述不够清晰，Junio 顺便提供了他的建议 > +--no-shallow:: > + Don't clone a shallow source repository. In some scenariors, clients
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
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.
Junio 进一步解释：现存的选项 `no-checkout` 是一个很久之前的历史错误， 由于兼容性，已经无法移除这个错误了，现在新的选项不应该按照这种模式命名。 P.S. 刚好我就是按照`no-checkout`选项的模式创造出`no-shllow`选项的-:)
> @@ -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?
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.
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.
通过测试代码 Junio 指出： clone.rejectshallow 配置和命令行选项 --reject-shallow 存在逻辑上的交叉重叠问题，因此测试时应该体现出这一点。
我觉得这个补丁大部分都很好，但我还有一点点改进建议 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 t= he 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.
使用 `to specify 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".
> 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_mir= ror, option_single_branch > static int option_local =3D -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 =3D 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.
P.S. 划线这段话是错误的，见 Johannes 本人回复：Johannes’reply，以及 Junio 的回复：Junio’s reply。最后用了两个变量: option_reject_shallow，config_reject_shallow，在 cmd_clone() 中它们共同决定另一个变量：reject_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. 用 `reject_shallow` 代替 `remote_shllow` Please name this attribute `reject_shallow` instead of `remote_shallow`. That will prevent future puzzlement.
> 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
'should' 在这里感觉语气太重了 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' 开头，则后面不应该用 `but`做转折。 "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.
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.
回到主题，前面的每次修改，直接强推到原来的 PR 中即可， GitGitGadget 会自动将每次的更新转换为不同的版本再提交到上游社区。那如何确定提交的版本是最终版本呢？前面提到过 Git 的几个主要分支，当我提交到第十版后，很快就被合入到 seen 分支，意味着被初步接受，然后又马上进入 next 分支，意味着各项测试也没问题，然后又马上进入 master 分支。特别地，这个过程会有 状态更新 提醒， Git 社区有个 What’s cooking in git.git 栏目，是维护者 Junio 用来管理各种提交的状态更新的，它会表明目前社区正在 cooking 哪些人的哪些代码(patch)，以及各个代码的目前状态。当你的代码在 What’s cooking in git.git 中进入 Graduated to 'master' 或者 Will merge to 'master'，那就表明马上会合入主线啦。