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