Re: [PATCH v5 3/3] upload-pack: optionally allow fetching reachable sha1

2015-05-22 Thread Junio C Hamano 
Fredrik Medley  writes:

> 2015-05-22 0:15 GMT+02:00 Junio C Hamano :
>
>> It looks like this new set of tests are well thought out; good job.
>>
>> I spotted a few minor nits, though.  All I'll amend while applying
>> so there is no need to resend only to correct them.
>
> I agree on all your comments and your proposed amendment further down
> looks good.

Thanks.

> Should the test code contain the explanations you've written in this email?

No, I don't think so.

I was just showing that thinking aloud while reviewing would be a
good way to do a review.  The practice

 (1) makes sure that the reviewer actually understood what the patch
 wanted to do (and the reviewee can point out misunderstandings
 if there are any); and

 (2) shows others that the reviewer actually read the patch ;-).

The latter is primarily meant for other people who review the
patches.  I want to see people get in the habit of responding with
something more than just a single-liner "Reviewed-by:", which I
often have hard time guessing if the reviewer really read the patch,
or just skimmed without spending effort to spot issues, and this
message was my attempt to lead with an example.

Will squash in the fix-up in the message you are responding to.
Let's move the topic to 'next' after that.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 3/3] upload-pack: optionally allow fetching reachable sha1

2015-05-22 Thread Fredrik Medley 
2015-05-22 0:15 GMT+02:00 Junio C Hamano :
> Fredrik Medley  writes:
>
>> --- a/Documentation/technical/protocol-capabilities.txt
>> +++ b/Documentation/technical/protocol-capabilities.txt
>> @@ -260,6 +260,13 @@ If the upload-pack server advertises this capability, 
>> fetch-pack may
>>  send "want" lines with SHA-1s that exist at the server but are not
>>  advertised by upload-pack.
>>
>> +allow-reachable-sha1-in-want
>> +--
>
> This is an underline applied to one line prior, and their length
> must match.  I'll amend while applying (attached at end), so there
> is no need to resend with correction unless you have other reasons
> to do so.
>
>> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
>> index 8a5f236..fdcc114 100755
>> --- a/t/t5516-fetch-push.sh
>> +++ b/t/t5516-fetch-push.sh
>> @@ -1120,6 +1120,61 @@ test_expect_success 'fetch exact SHA1' '
>>   )
>>  '
>
> It looks like this new set of tests are well thought out; good job.
>
> I spotted a few minor nits, though.  All I'll amend while applying
> so there is no need to resend only to correct them.

I agree on all your comments and your proposed amendment further down
looks good.
Should the test code contain the explanations you've written in this email?

>
>> +for configallowtipsha1inwant in true false
>> +do
>> + test_expect_success "shallow fetch reachable SHA1 (but not a ref), 
>> allowtipsha1inwant=$configallowtipsha1inwant" '
>> + mk_empty testrepo &&
>> + (
>> + cd testrepo &&
>> + git config uploadpack.allowtipsha1inwant 
>> $configallowtipsha1inwant &&
>> + git commit --allow-empty -m foo &&
>> + git commit --allow-empty -m bar
>> + ) &&
>> + SHA1=$(git --git-dir=testrepo/.git rev-parse HEAD^) &&
>> + mk_empty shallow &&
>> + (
>> + cd shallow &&
>> + test_must_fail git fetch --depth=1 ../testrepo/.git 
>> $SHA1 &&
>
> This tries to fetch one before the tip with "allowTipSHA1InWant" set
> to true or false; either should fail.  Good.
>
>> + git --git-dir=../testrepo/.git config 
>> uploadpack.allowreachablesha1inwant true &&
>> + git fetch --depth=1 ../testrepo/.git $SHA1 &&
>
> And regardless of allowTip setting, with allowReachable set to true,
> fetching the reachable HEAD^ would succeed.  Good.
>
>> + git cat-file commit $SHA1 >/dev/null
>
> Minor nit; drop ">/dev/null", as test framework will squelch the
> output by default, and when the test is run with "-v" option, the
> output would help debugging the script.
>
>> + )
>> + '
>> +
>> + test_expect_success "deny fetch unreachable SHA1, 
>> allowtipsha1inwant=$configallowtipsha1inwant" '
>> + mk_empty testrepo &&
>> + (
>> + cd testrepo &&
>> + git config uploadpack.allowtipsha1inwant 
>> $configallowtipsha1inwant &&
>> + git commit --allow-empty -m foo &&
>> + git commit --allow-empty -m bar &&
>> + git commit --allow-empty -m xyz
>> + )
>
> Broken && chain
>
>> + SHA1_1=$(git --git-dir=testrepo/.git rev-parse HEAD^^) &&
>> + SHA1_2=$(git --git-dir=testrepo/.git rev-parse HEAD^) &&
>> + SHA1_3=$(git --git-dir=testrepo/.git rev-parse HEAD) &&
>> + (
>> + cd testrepo &&
>> + git reset --hard $SHA1_2 &&
>
> We have one before the tip (SHA1_1), one after the tip and no longer
> reachable (SHA1_3); SHA1_2 is sitting at the tip of a ref.
>
>> + git cat-file commit $SHA1_3 >/dev/null &&
>> + git cat-file commit $SHA1_3 >/dev/null
>
> I think one of the latter two is $SHA1_1, i.e. you make sure SHA1_{1,2,3}
> are there in testrepo.

Yes, that was intended.

>
>> + ) &&
>> + mk_empty shallow &&
>> + (
>> + cd shallow &&
>> + test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
>> + test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
>
> With allowTip only, whether it is set to true or false, fetching _1
> or _3 that are not at tip will fail.  Good.
>
>> + git --git-dir=../testrepo/.git config 
>> uploadpack.allowreachablesha1inwant true &&
>> + git fetch ../testrepo/.git $SHA1_1 &&
>> + git cat-file commit $SHA1_1 >/dev/null &&
>
> With allowReachable, _1 which is reachable from tip is possible,
> regardless of the setting of allowTip.  Good.
>
>> + test_must_fail git cat-file commit $SHA1_2 >/dev/null 
>> &&
>
> And fetching _1 will not pull in _2, which is _1's child, that we
> did not ask for.  Good (but it is probably not very relevant for the
> purp

Re: [PATCH v5 3/3] upload-pack: optionally allow fetching reachable sha1

2015-05-21 Thread Junio C Hamano 
Fredrik Medley  writes:

> --- a/Documentation/technical/protocol-capabilities.txt
> +++ b/Documentation/technical/protocol-capabilities.txt
> @@ -260,6 +260,13 @@ If the upload-pack server advertises this capability, 
> fetch-pack may
>  send "want" lines with SHA-1s that exist at the server but are not
>  advertised by upload-pack.
>  
> +allow-reachable-sha1-in-want
> +--

This is an underline applied to one line prior, and their length
must match.  I'll amend while applying (attached at end), so there
is no need to resend with correction unless you have other reasons
to do so.

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 8a5f236..fdcc114 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1120,6 +1120,61 @@ test_expect_success 'fetch exact SHA1' '
>   )
>  '

It looks like this new set of tests are well thought out; good job.

I spotted a few minor nits, though.  All I'll amend while applying
so there is no need to resend only to correct them.

> +for configallowtipsha1inwant in true false
> +do
> + test_expect_success "shallow fetch reachable SHA1 (but not a ref), 
> allowtipsha1inwant=$configallowtipsha1inwant" '
> + mk_empty testrepo &&
> + (
> + cd testrepo &&
> + git config uploadpack.allowtipsha1inwant 
> $configallowtipsha1inwant &&
> + git commit --allow-empty -m foo &&
> + git commit --allow-empty -m bar
> + ) &&
> + SHA1=$(git --git-dir=testrepo/.git rev-parse HEAD^) &&
> + mk_empty shallow &&
> + (
> + cd shallow &&
> + test_must_fail git fetch --depth=1 ../testrepo/.git 
> $SHA1 &&

This tries to fetch one before the tip with "allowTipSHA1InWant" set
to true or false; either should fail.  Good.

> + git --git-dir=../testrepo/.git config 
> uploadpack.allowreachablesha1inwant true &&
> + git fetch --depth=1 ../testrepo/.git $SHA1 &&

And regardless of allowTip setting, with allowReachable set to true,
fetching the reachable HEAD^ would succeed.  Good.

> + git cat-file commit $SHA1 >/dev/null

Minor nit; drop ">/dev/null", as test framework will squelch the
output by default, and when the test is run with "-v" option, the
output would help debugging the script.

> + )
> + '
> +
> + test_expect_success "deny fetch unreachable SHA1, 
> allowtipsha1inwant=$configallowtipsha1inwant" '
> + mk_empty testrepo &&
> + (
> + cd testrepo &&
> + git config uploadpack.allowtipsha1inwant 
> $configallowtipsha1inwant &&
> + git commit --allow-empty -m foo &&
> + git commit --allow-empty -m bar &&
> + git commit --allow-empty -m xyz
> + )

Broken && chain

> + SHA1_1=$(git --git-dir=testrepo/.git rev-parse HEAD^^) &&
> + SHA1_2=$(git --git-dir=testrepo/.git rev-parse HEAD^) &&
> + SHA1_3=$(git --git-dir=testrepo/.git rev-parse HEAD) &&
> + (
> + cd testrepo &&
> + git reset --hard $SHA1_2 &&

We have one before the tip (SHA1_1), one after the tip and no longer
reachable (SHA1_3); SHA1_2 is sitting at the tip of a ref.

> + git cat-file commit $SHA1_3 >/dev/null &&
> + git cat-file commit $SHA1_3 >/dev/null

I think one of the latter two is $SHA1_1, i.e. you make sure SHA1_{1,2,3}
are there in testrepo.

> + ) &&
> + mk_empty shallow &&
> + (
> + cd shallow &&
> + test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
> + test_must_fail git fetch ../testrepo/.git $SHA1_1 &&

With allowTip only, whether it is set to true or false, fetching _1
or _3 that are not at tip will fail.  Good.

> + git --git-dir=../testrepo/.git config 
> uploadpack.allowreachablesha1inwant true &&
> + git fetch ../testrepo/.git $SHA1_1 &&
> + git cat-file commit $SHA1_1 >/dev/null &&

With allowReachable, _1 which is reachable from tip is possible,
regardless of the setting of allowTip.  Good.

> + test_must_fail git cat-file commit $SHA1_2 >/dev/null &&

And fetching _1 will not pull in _2, which is _1's child, that we
did not ask for.  Good (but it is probably not very relevant for the
purpose of these tests).

> + git fetch ../testrepo/.git $SHA1_2 &&
> + git cat-file commit $SHA1_2 >/dev/null &&

And of course, _2 can be fetched.

> + test_must_fail git fetch ../testrepo/.git $SHA1_3

And _3 that is not reachable cannot.

> + )
> + '
> +done

Again, very carefully covering combinations

[PATCH v5 3/3] upload-pack: optionally allow fetching reachable sha1

2015-05-21 Thread Fredrik Medley 
With uploadpack.allowReachableSHA1InWant configuration option set on the
server side, "git fetch" can make a request with a "want" line that names
an object that has not been advertised (likely to have been obtained out
of band or from a submodule pointer). Only objects reachable from the
branch tips, i.e. the union of advertised branches and branches hidden by
transfer.hideRefs, will be processed. Note that there is an associated
cost of having to walk back the history to check the reachability.

This feature can be used when obtaining the content of a certain commit,
for which the sha1 is known, without the need of cloning the whole
repository, especially if a shallow fetch is used. Useful cases are e.g.
repositories containing large files in the history, fetching only the
needed data for a submodule checkout, when sharing a sha1 without telling
which exact branch it belongs to and in Gerrit, if you think in terms of
commits instead of change numbers. (The Gerrit case has already been
solved through allowTipSHA1InWant as every Gerrit change has a ref.)

Signed-off-by: Fredrik Medley 
---
 Documentation/config.txt  |  6 +++
 Documentation/technical/http-protocol.txt |  3 +-
 Documentation/technical/protocol-capabilities.txt |  7 +++
 fetch-pack.c  | 10 -
 t/t5516-fetch-push.sh | 55 +++
 upload-pack.c | 22 +++--
 6 files changed, 97 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1311383..b8215ba 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2547,6 +2547,12 @@ uploadpack.allowTipSHA1InWant::
of a hidden ref (by default, such a request is rejected).
see also `uploadpack.hideRefs`.
 
+uploadpack.allowReachableSHA1InWant::
+   Allow `upload-pack` to accept a fetch request that asks for an
+   object that is reachable from any ref tip. However, note that
+   calculating object reachability is computationally expensive.
+   Defaults to `false`.
+
 uploadpack.keepAlive::
When `upload-pack` has started `pack-objects`, there may be a
quiet period while `pack-objects` prepares the pack. Normally
diff --git a/Documentation/technical/http-protocol.txt 
b/Documentation/technical/http-protocol.txt
index 229f845..1c561bd 100644
--- a/Documentation/technical/http-protocol.txt
+++ b/Documentation/technical/http-protocol.txt
@@ -319,7 +319,8 @@ Servers SHOULD support all capabilities defined here.
 Clients MUST send at least one "want" command in the request body.
 Clients MUST NOT reference an id in a "want" command which did not
 appear in the response obtained through ref discovery unless the
-server advertises capability `allow-tip-sha1-in-want`.
+server advertises capability `allow-tip-sha1-in-want` or
+`allow-reachable-sha1-in-want`.
 
   compute_request   =  want_list
   have_list
diff --git a/Documentation/technical/protocol-capabilities.txt 
b/Documentation/technical/protocol-capabilities.txt
index 4f8a7bf..265fcab 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -260,6 +260,13 @@ If the upload-pack server advertises this capability, 
fetch-pack may
 send "want" lines with SHA-1s that exist at the server but are not
 advertised by upload-pack.
 
+allow-reachable-sha1-in-want
+--
+
+If the upload-pack server advertises this capability, fetch-pack may
+send "want" lines with SHA-1s that exist at the server but are not
+advertised by upload-pack.
+
 push-cert=
 -
 
diff --git a/fetch-pack.c b/fetch-pack.c
index 699f586..0d83d47 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -46,6 +46,8 @@ static struct prio_queue rev_list = { 
compare_commits_by_commit_date };
 static int non_common_revs, multi_ack, use_sideband;
 /* Allow specifying sha1 if it is a ref tip. */
 #define ALLOW_TIP_SHA1 01
+/* Allow request of a sha1 if it is reachable from a ref (possibly hidden 
ref). */
+#define ALLOW_REACHABLE_SHA1   02
 static int allow_unadvertised_object_request;
 
 static void rev_list_push(struct commit *commit, int mark)
@@ -545,7 +547,8 @@ static void filter_refs(struct fetch_pack_args *args,
}
 
/* Append unmatched requests to the list */
-   if ((allow_unadvertised_object_request & ALLOW_TIP_SHA1)) {
+   if ((allow_unadvertised_object_request &
+   (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
for (i = 0; i < nr_sought; i++) {
unsigned char sha1[20];
 
@@ -826,6 +829,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args 
*args,
fprintf(stderr, "Server supports 
allow-tip-sha1-in-want\n");
allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
}
+   if (server_supports("allow-reachable-s