On Thu, Jun 11, 2015 at 2:05 AM, Jeff King <peff@xxxxxxxx> wrote: > On Wed, Jun 10, 2015 at 02:35:20PM -0400, Mike Edgar wrote: > >> When the user passes --depth to git-clone the server's capabilities are >> not currently consulted. The client will send shallow requests even if >> the server does not understand them, and the resulting error may be >> unhelpful to the user. This change pre-emptively checks so git-clone can >> exit with a helpful error if necessary. > > This sounds like a good thing to do, but... > >> --- a/builtin/clone.c >> +++ b/builtin/clone.c >> @@ -944,6 +944,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix) >> >> refs = transport_get_remote_refs(transport); >> >> + if (option_depth && !is_local && !server_supports("shallow")) >> + die(_("Server does not support shallow clients")); >> + > > It feels a little weird to be checking the option here in cmd_clone. > The transport layer knows we have specified a depth, so it seems like a > more natural place for it (or possibly even lower, in the actual > git-protocol code). > > That being said, I think the current capabilities handling is a bit > messy and crosses module boundaries freely. So I would not be surprised > if this is the most reasonable place to put it. But it does make me > wonder whether "git fetch --depth=..." needs the same treatment. > > I see that do_fetch_pack checks server_supports("shallow"). Is that > enough to cover all fetch cases? And if it is, why does it not cover the > matching clone cases? I think this replacement check would do if ((args->depth > 0 || is_repository_shallow()) && !server_supports("shallow")) die("Server does not support shallow clients"); -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html