"lilinchao via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > From: lilinchao <lilinchao@xxxxxxxxxx> 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@xxxxxxxxxx> > --- > 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@xxxxxxxxxx > @@ -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.
