threads / patch / 39589

patchclone: check if server supports shallow clones

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

## tl;dr

12 messages between Jun 10, 2015 and Jun 17, 2015. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Mike Edgar· Jun 10, 2015, 18:35 UTC · lore

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(+)

Show changes to builtin/clone.c +3 −0

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

Jeff King· Jun 10, 2015, 19:05 UTC · re: Mike Edgar · lore

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

On Wed, Jun 10, 2015 at 02:35:20PM -0400, Mike Edgar wrote:

Show 5 quoted lines

> 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...

Show 9 quoted lines

> --- 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

Michael Edgar· Jun 10, 2015, 20:25 UTC · re: Jeff King · lore

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

Show 5 quoted lines

> 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.

Jeff King· Jun 10, 2015, 20:56 UTC · re: Michael Edgar · lore

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

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

Show 14 quoted lines

> > 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

Duy Nguyen· Jun 11, 2015, 13:02 UTC · re: Jeff King · lore

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

On Thu, Jun 11, 2015 at 2:05 AM, Jeff King <peff@peff.net> wrote:

Show 33 quoted lines

> 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

Jeff King· Jun 11, 2015, 14:32 UTC · re: Duy Nguyen · lore

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

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

Show 8 quoted lines

> > 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

Michael Edgar· Jun 11, 2015, 18:18 UTC · re: Jeff King · lore

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

On Thu, Jun 11, 2015 at 10:32 AM, Jeff King <peff@peff.net> wrote:

Show 21 quoted lines

> 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

Junio C Hamano· Jun 11, 2015, 20:44 UTC · re: Michael Edgar · lore

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

Michael Edgar <adgar@google.com> writes:

Show 26 quoted lines

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

Mike Edgar· Jun 17, 2015, 11:48 UTC · re: Junio C Hamano · lore

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

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(-)

Show changes to fetch-pack.c +1 −1

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

Jeff King· Jun 17, 2015, 17:00 UTC · re: Mike Edgar · lore

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

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

Show 8 quoted lines

> 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

Junio C Hamano· Jun 17, 2015, 16:35 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:

Show 25 quoted lines

> 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?

Jeff King· Jun 17, 2015, 16:59 UTC · re: Junio C Hamano · lore

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

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

Show 9 quoted lines

> > 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