[PATCH] clone: check if server supports shallow clones

[PATCH] clone: check if server supports shallow clones

[Mike Edgar]

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.

Signed-off-by: Mike Edgar <adgar@google.com>
---
 builtin/clone.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/clone.c b/builtin/clone.c
index b878252..b4e9846 100644
--- 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"));
+
 	if (refs) {
 		mapped_refs = wanted_peer_refs(refs, refspec);
 		/*
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH] clone: check if server supports shallow clones

[Jeff King]

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?

-Peff

Re: [PATCH] clone: check if server supports shallow clones

[Michael Edgar]

> On Wed, Jun 10, 2015 at 3:05 PM, Jeff King <peff@peff.net> wrote:
> 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?
> -Peff

Great question. I determined that the do_fetch_pack logic doesn't work for
clones because it also checks is_repository_shallow(), which looks for the
.git/shallow file (or the alternate file).

I considered changing clone to create an empty .git/shallow file or alternate,
but it turns out that an empty shallow file is treated as no shallow file at
all. Since at this stage in cloning we have nothing to put in the shallow file,
it seemed like any other fix would require more substantial changes.

Re: [PATCH] clone: check if server supports shallow clones

[Jeff King]

On Wed, Jun 10, 2015 at 04:25:45PM -0400, Michael Edgar wrote:

> > On Wed, Jun 10, 2015 at 3:05 PM, Jeff King <peff@peff.net> wrote:
> > 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?
> > -Peff
> 
> Great question. I determined that the do_fetch_pack logic doesn't work for
> clones because it also checks is_repository_shallow(), which looks for the
> .git/shallow file (or the alternate file).
> 
> I considered changing clone to create an empty .git/shallow file or alternate,
> but it turns out that an empty shallow file is treated as no shallow file at
> all. Since at this stage in cloning we have nothing to put in the shallow file,
> it seemed like any other fix would require more substantial changes.

Hmm. Can we improve the check in do_fetch_pack? That is, do we have a
way of checking whether the _current_ fetch is going to make us shallow?
I don't see anything obvious (like a "depth" flag) passed into
do_fetch_pack, so I guess it is relying solely on global shallow state
that we've already set up? Or is it the case that "git fetch
--depth=" in a non-shallow repository does not properly check this flag?

If it turns out that "fetch --depth" is fine, and only "clone --depth"
is broken, I can certainly live with the patch you provided. But I think
we need to clarify that a bit in the commit message.

-Peff

Re: [PATCH] clone: check if server supports shallow clones

[Duy Nguyen]

On Thu, Jun 11, 2015 at 2:05 AM, Jeff King <peff@peff.net> 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

Re: [PATCH] clone: check if server supports shallow clones

[Jeff King]

On Thu, Jun 11, 2015 at 08:02:33PM +0700, Duy Nguyen wrote:

> > 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");

Oh, indeed, there is the depth flag I was looking for. :)

And from some rudimentary testing, I believe that:

  git init
  git fetch --depth=1 ...

is currently broken in the same way as clone (we are not shallow yet, so
it does not complain when the server does not support it). I think the
patch above fixes both that and the clone case.

Of course it's hard to add to the test suite, since we do not have a way
of hitting a server that does not understand shallow (I simply fudged
server_supports() to return false on the client).

-Peff

Re: [PATCH] clone: check if server supports shallow clones

[Michael Edgar]

On Thu, Jun 11, 2015 at 10:32 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Jun 11, 2015 at 08:02:33PM +0700, Duy Nguyen wrote:
>
>> > 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");
>
> Oh, indeed, there is the depth flag I was looking for. :)
>
> And from some rudimentary testing, I believe that:
>
>   git init
>   git fetch --depth=1 ...
>
> is currently broken in the same way as clone (we are not shallow yet, so
> it does not complain when the server does not support it). I think the
> patch above fixes both that and the clone case.

Great point! Duy's proposed fix worked for me testing against the Git
server that
led me to the original bug (code.google.com), for both clones and init/fetch.

Shall I send that out as a revised patch for review? (This is my first
stab at a Git
patch)

> Of course it's hard to add to the test suite, since we do not have a way
> of hitting a server that does not understand shallow (I simply fudged
> server_supports() to return false on the client).

I noticed this, and figured it would be more dangerous for users to include a
testing backdoor (eg a flag overriding the advertised capabilities). Is that an
accurate assessment, or is there a safer way to test these sorts of interop
cases?

Cheers!
-- 
Michael Edgar | Software Engineer | adgar@google.com

Re: [PATCH] clone: check if server supports shallow clones

[Junio C Hamano]

Michael Edgar <adgar@google.com> writes:

> On Thu, Jun 11, 2015 at 10:32 AM, Jeff King <peff@peff.net> wrote:
>> On Thu, Jun 11, 2015 at 08:02:33PM +0700, Duy Nguyen wrote:
>>
>>> > 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");
>>
>> Oh, indeed, there is the depth flag I was looking for. :)
>>
>> And from some rudimentary testing, I believe that:
>>
>>   git init
>>   git fetch --depth=1 ...
>>
>> is currently broken in the same way as clone (we are not shallow yet, so
>> it does not complain when the server does not support it). I think the
>> patch above fixes both that and the clone case.
>
> Shall I send that out as a revised patch for review? (This is my first
> stab at a Git
> patch)

Surely, and thanks.  Hopefully one or both of them will Ack it and
all there is left for me to do will be to pick it up ;-)

[PATCH v2] fetch-pack: check for shallow if depth given

[Mike Edgar]

When a repository is first fetched as a shallow clone, either by
git-clone or by fetching into an empty repo, 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 we can exit
with a helpful error if necessary.

Signed-off-by: Mike Edgar <adgar@google.com>
---
 fetch-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index a912935..a136772 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -809,7 +809,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 	sort_ref_list(&ref, ref_compare_name);
 	qsort(sought, nr_sought, sizeof(*sought), cmp_ref_by_name);
 
-	if (is_repository_shallow() && !server_supports("shallow"))
+	if ((args->depth > 0 || is_repository_shallow()) && !server_supports("shallow"))
 		die("Server does not support shallow clients");
 	if (server_supports("multi_ack_detailed")) {
 		if (args->verbose)
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCH] clone: check if server supports shallow clones

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Jun 11, 2015 at 08:02:33PM +0700, Duy Nguyen wrote:
>
>> > 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");
>
> Oh, indeed, there is the depth flag I was looking for. :)
>
> And from some rudimentary testing, I believe that:
>
>   git init
>   git fetch --depth=1 ...
>
> is currently broken in the same way as clone (we are not shallow yet, so
> it does not complain when the server does not support it). I think the
> patch above fixes both that and the clone case.
>
> Of course it's hard to add to the test suite, since we do not have a way
> of hitting a server that does not understand shallow (I simply fudged
> server_supports() to return false on the client).

We've had the "shallow" capability advertised since ed09aef0
(support fetching into a shallow repository, 2006-10-30), and this
patch itself may not be that super-important in practice.  Let's not
worry too much about a test for situations that may not likely
matter to us [*1*].

Thanks, all.


[Footnote]

*1* How behind are re-implementations of upload-pack by other
people, I have to wonder, though?

Re: [PATCH] clone: check if server supports shallow clones

[Jeff King]

On Wed, Jun 17, 2015 at 09:35:23AM -0700, Junio C Hamano wrote:

> > Of course it's hard to add to the test suite, since we do not have a way
> > of hitting a server that does not understand shallow (I simply fudged
> > server_supports() to return false on the client).
> 
> We've had the "shallow" capability advertised since ed09aef0
> (support fetching into a shallow repository, 2006-10-30), and this
> patch itself may not be that super-important in practice.  Let's not
> worry too much about a test for situations that may not likely
> matter to us [*1*].

I had actually started looking at doing a generic interop testing
suite. It would be nice to be able to do something like:

  cd t/interop
  ./run v1.0.0 v2.0.0 ./t0001-clone.sh

and then the test script looks something like:

  test_expect_success 'clone with A from B' '
	git.a clone -u "git.b upload-pack"
  '

The "run" script is similar to the version in t/perf that builds
arbitrary revisions for testing, but with the twist that it points
the PATH to "git.a" and "git.b", which symlink into the bin-wrappers/ of
the built directories (and probably disallows bare "git" to prevent
mistakes).

But I agree that this particular bug is not all that exciting to test.

> *1* How behind are re-implementations of upload-pack by other
> people, I have to wonder, though?

JGit advertises "shallow". Libgit2 does not, but it also does not
implement upload-pack. :)

I do wonder which server Mike was hitting to come across this in the
first place. Maybe the Google Code dulwich-based one?

-Peff

Re: [PATCH v2] fetch-pack: check for shallow if depth given

[Jeff King]

On Wed, Jun 17, 2015 at 07:48:14AM -0400, Mike Edgar wrote:

> When a repository is first fetched as a shallow clone, either by
> git-clone or by fetching into an empty repo, 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 we can exit
> with a helpful error if necessary.
> 
> Signed-off-by: Mike Edgar <adgar@google.com>

Looks good to me.

-Peff