Re: [PATCH v5 3/3] upload-pack: optionally allow fetching reachable sha1
2015-05-22 0:15 GMT+02:00 Junio C Hamano gits...@pobox.com:
Fredrik Medley fredrik.med...@gmail.com 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
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
Re: [PATCH v5 3/3] upload-pack: optionally allow fetching reachable sha1
Fredrik Medley fredrik.med...@gmail.com writes: 2015-05-22 0:15 GMT+02:00 Junio C Hamano gits...@pobox.com: 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
Fredrik Medley fredrik.med...@gmail.com 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. Good.
Documentation/technical/protocol-capabilities.txt | 2 +-
t/t5516-fetch-push.sh | 14
