布景
第一次向 Git 社区提交代码起源于 Gitee 的一个需求。Gitee 创建新库房有多种方法，其间一种是根据用户供给的库房 URL 导入新库房，Gitee 首要会对用户供给的 URL 进行校验，只要合格的库房地址才会生成新库房。可是有种状况比较特别，若用户的确供给了一个合法的 Git 库房地址，可是这个库房不是完整的，常见的状况便是浅库房(shallow repo)，这类库房缺少足够的前史提交信息，因而会带来一些问题。为了防止这种状况，在服务端就需求对用户导入的库房进行特别处理。在服务端，导入库房的动作本质上便是履行 git clone --mirror 指令，那么有没有或许在履行这个克隆指令的进程中就对库房进行判别呢？如果能，这将防止克隆完整个库房再进行判别库房是否为浅库房，这样无疑更有效。
所以我开端研究 Git 源码，揣摩怎么给 git-clone 指令赋予一项新的才能。
按照前面的方法向 Git 社区提交了第一版代码：v1。
很快收到 Junio C Hamano 反应，首要问题有：
- commit 信息中说明不清，英文语法过错：
> This patch add a new option that reject to clone a shallow repository.
一条标准的 commit 信息格式应该是在开端就解说更改需求，然后在结尾供给处理方案。
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.
这儿表述不行清晰，Junio 顺便供给了他的主张
"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').
- 新增选项
--no-shallow不对，应该考虑到布尔类选项存在反选状况，即--no-no-shallow，因而不该该这么写，主张改为--reject-shallow， 它的反选即为--no-reject-shallow，这样语义上也更符合直觉：
> diff --git a/builtin/clone.c b/builtin/clone.c
> @@ -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.
Junio 进一步解说：现存的选项 `no-checkout` 是一个很久之前的前史过错，
因为兼容性，已经无法移除这个过错了，现在新的选项不该该按照这种形式命名。
P.S. 刚好我便是按照`no-checkout`选项的形式创造出`no-shllow`选项的-:)
更重要的是，指令行优先于装备，即指令行选项要能够覆盖装备文件选项。当装备文件从全局上不答应克隆 shllow 库房时，而用户想要答应单次克隆 shallow 库房时，自然地，他会运用 --no-shallow 选项的反选项 --no-no-shallow 来覆盖掉装备中的选项 。但这样 --no-no-shallow 听来去就很傻。
- 因为 Git 存在多种传输协议，现在的修正只是处理了本地克隆问题，因而仍需改善：
> @@ -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.
经过测验代码 Junio 指出： clone.rejectshallow 装备和指令行选项 --reject-shallow 存在逻辑上的穿插重叠问题，因而测验时应该体现出这一点。
以上便是我第一次提交的代码，没想到能很快收到那么多名贵的检视主张。
可是，这只是开端，后面还有更多的检视回合，收到了愈加详尽的检视定见。
比如 Johannes Schindelin 给了一些定见：
我觉得这个补丁大部分都很好，但我还有一点点改善主张
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.
运用 option_reject_shallow 比 config_shallow 更好一些，它能更直接地标明这个选项是要回绝 shallow clone 的。一起他提示，在 。cmd_clone()函数中 git_config 会被调用两次，运用 option_reject_shallow 能防止在 cmd_clone 中运用 reject_shallow
P.S. 划线这段话是过错的，见 Johannes 自己回复：Johannes’reply，以及 Junio 的回复：Junio’s reply。最后用了两个变量: option_reject_shallow，config_reject_shallow，在 cmd_clone() 中它们共同决定另一个变量：reject_shallow。
后面同样变量命名：
> 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_pac=
k_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.
用 `reject_shallow` 替代 `remote_shllow`
Please name this attribute `reject_shallow` instead of `remote_shallow`.
That will prevent future puzzlement.
Johannes 还对测验用例提出了一些检视定见，篇幅有限，这儿省略。
在最后几个回合，Junio 又给出了许多有意义的检视定见：
> 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.
> @@ -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.
尽管比上一版的更新好了许多，但Junio并没有放弃任何或许的问题，仍对config装备
和option装备有疑问，他的顾忌是正确的，这为下一版的更新供给了正确的思路。
这便是我第一次向 Git 社区提交代码的状况：问题多多，反应多多。各种谨慎详尽又有启发性的检视定见让我感触到了 Git 社区的技术氛围。这个进程中让我不断思考怎么让代码写得更好：更有逻辑，更简练，更谨慎，而不仅仅是完成功用。
这个阅历让我想到，英语中两个单词：polish，cooking 能够很贴切的形容他们对待代码的情绪。
polish 意为润饰，修正，抛光，打磨。用在代码上， commit 信息上，甚至写文档上，意味着这些进程是不断改善，不断打磨，才能变得更好。
cooking 意为烹饪，比喻写代码就像烹饪，所谓心急吃不了热豆腐，即使做简单的菜都需求耐心，仔细。
回到主题，前面的每次修正，直接强推到原来的 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'，那就标明马上会合入主线啦。
下一篇文章，将会继续写我在后续的提交代码进程中更多的阅历。
