threads / patch / 39224

patchupload-pack: Optionally allow fetching reachable sha1

Subject: [PATCH] upload-pack: Optionally allow fetching reachable sha1

## tl;dr

34 messages between May 2, 2015 and May 23, 2015. Diffs are folded; open one to read it.

replies: 33people: 3as markdown or json

Fredrik Medley· May 2, 2015, 22:01 UTC · lore
For you who don't remember the email discussion, look through the references.

Since before, it is allowed to ask for a sha1, but most git servers refuse the request[1]. This patch is based on a suggested location for implementation[2] which has been updated according to ideas in the reply[3].

[1] http://thread.gmane.org/gmane.comp.version-control.git/257809 [2] http://thread.gmane.org/gmane.comp.version-control.git/258002 [3] http://thread.gmane.org/gmane.comp.version-control.git/258080

With uploadpack.allowreachablesha1inwant configuration option set, "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, hidden by transfer.hiderefs or not, will be processed.

Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
---
 Documentation/config.txt |  6 ++++++
 fetch-pack.c             |  9 ++++++--
 t/t5516-fetch-push.sh    | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
 upload-pack.c            | 13 +++++++++---
 4 files changed, 78 insertions(+), 5 deletions(-)
Show changes to 4 files +78 −5

Documentation/config.txt, fetch-pack.c, t/t5516-fetch-push.sh, upload-pack.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e5ceaf..76cd713 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2538,6 +2538,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/fetch-pack.c b/fetch-pack.c
index 48526aa..fb01b6c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -43,7 +43,7 @@ static int marked;
 #define MAX_IN_VAIN 256
 
 static struct prio_queue rev_list = { compare_commits_by_commit_date };
-static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
+static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want, allow_reachable_sha1_in_want;
 
 static void rev_list_push(struct commit *commit, int mark)
 {
@@ -542,7 +542,7 @@ static void filter_refs(struct fetch_pack_args *args,
 	}
 
 	/* Append unmatched requests to the list */
-	if (allow_tip_sha1_in_want) {
+	if (allow_tip_sha1_in_want || allow_reachable_sha1_in_want) {
 		for (i = 0; i < nr_sought; i++) {
 			unsigned char sha1[20];
 
@@ -823,6 +823,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 			fprintf(stderr, "Server supports allow-tip-sha1-in-want\n");
 		allow_tip_sha1_in_want = 1;
 	}
+	if (server_supports("allow-reachable-sha1-in-want")) {
+		if (args->verbose)
+			fprintf(stderr, "Server supports allow-reachable-sha1-in-want\n");
+		allow_reachable_sha1_in_want = 1;
+	}
 	if (!server_supports("thin-pack"))
 		args->use_thin_pack = 0;
 	if (!server_supports("no-progress"))
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8a5f236..01fabfb 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1120,6 +1120,61 @@ test_expect_success 'fetch exact SHA1' '
 	)
 '
 
+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 &&
+			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+			git fetch --depth=1 ../testrepo/.git $SHA1 &&
+			git cat-file commit $SHA1 >/dev/null
+		)
+	'
+
+	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
+		)
+		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 &&
+			git cat-file commit $SHA1_3 >/dev/null &&
+			git cat-file commit $SHA1_3 >/dev/null
+		) &&
+		mk_empty shallow &&
+		(
+			cd shallow &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
+			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+			git fetch ../testrepo/.git $SHA1_1 &&
+			git cat-file commit $SHA1_1 >/dev/null &&
+			test_must_fail git cat-file commit $SHA1_2 >/dev/null &&
+			git fetch ../testrepo/.git $SHA1_2 &&
+			git cat-file commit $SHA1_2 >/dev/null &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_3
+		)
+	'
+done
+
 test_expect_success 'fetch follows tags by default' '
 	mk_test testrepo heads/master &&
 	rm -fr src dst &&
diff --git a/upload-pack.c b/upload-pack.c
index aa84576..2e704d6 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -36,6 +36,7 @@ static int no_done;
 static int use_thin_pack, use_ofs_delta, use_include_tag;
 static int no_progress, daemon_mode;
 static int allow_tip_sha1_in_want;
+static int allow_reachable_sha1_in_want;
 static int shallow_nr;
 static struct object_array have_obj;
 static struct object_array want_obj;
@@ -443,7 +444,7 @@ static int get_common_commits(void)
 static int is_our_ref(struct object *o)
 {
 	return o->flags &
-		((allow_tip_sha1_in_want ? HIDDEN_REF : 0) | OUR_REF);
+		(((allow_tip_sha1_in_want || allow_reachable_sha1_in_want) ? HIDDEN_REF : 0) | OUR_REF);
 }
 
 static void check_non_tip(void)
@@ -456,8 +457,11 @@ static void check_non_tip(void)
 	char namebuf[42]; /* ^ + SHA-1 + LF */
 	int i;
 
-	/* In the normal in-process case non-tip request can never happen */
-	if (!stateless_rpc)
+	/*
+	 * In the normal in-process case without
+	 * uploadpack.allowreachablesha1inwant, non-tip requests can never happen.
+	 */
+	if (!stateless_rpc && !allow_reachable_sha1_in_want)
 		goto error;
 
 	cmd.argv = argv;
@@ -728,6 +732,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
 			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
+			     allow_reachable_sha1_in_want ? " allow-reachable-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
 			     git_user_agent_sanitized());
@@ -789,6 +794,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 {
 	if (!strcmp("uploadpack.allowtipsha1inwant", var))
 		allow_tip_sha1_in_want = git_config_bool(var, value);
+	else if (!strcmp("uploadpack.allowreachablesha1inwant", var))
+		allow_reachable_sha1_in_want = git_config_bool(var, value);
 	else if (!strcmp("uploadpack.keepalive", var)) {
 		keepalive = git_config_int(var, value);
 		if (!keepalive)
-- 
1.9.1
Junio C Hamano· May 3, 2015, 17:40 UTC · re: Fredrik Medley · lore

Re: [PATCH] upload-pack: Optionally allow fetching reachable sha1

Fredrik Medley <fredrik.medley@gmail.com> writes:
> For you who don't remember the email discussion, look through the references.

Please don't do this. Always describe the background yourself in the log message so that "git log" can be read offline. "describe yourself" can be done by summarizing earlier discussion, borrowing others' words, of course. And it is a very good idea to give references like you did after your summary to optionally allow people to verify your summary is correct.

I do not quite understand the early part of the following:
> Since before, it is allowed to ask for a sha1, but most git servers refuse
> the request[1].

That makes it sound like "then talk to the server operator to reconfigure and your problem is solved", no? I do not think the exchange [2] and [3] are about that---your wish will not be granted, even if we extended the system to allow non-tip to be fetched, if the server is configured not use that feature.

> This patch is based on a suggested location for
> implementation[2] which has been updated according to ideas in the reply[3].

I quite am not sure what the suggested "location" for implemenation is.

Show 5 quoted lines
> [1] http://thread.gmane.org/gmane.comp.version-control.git/257809
> [2] http://thread.gmane.org/gmane.comp.version-control.git/258002
> [3] http://thread.gmane.org/gmane.comp.version-control.git/258080
>
> With uploadpack.allowreachablesha1inwant configuration option set,
"option set" on the server side, I presume?
> "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,
> hidden by transfer.hiderefs or not, will be processed.

I am not sure if I am reading the last sentence correctly. If there are two branches, 'master' (exposed) and 'occult' (hidden),

    ---o---o---2---1---x master
        \
         o---2---1---y occult

you can ask for the 40-hex object name of occult~2, but you cannot ask for the 40-hex object name of master~2? Why such a restriction?

 ... wait for an answer that justfies the restriction ...
Does that justification applies for allowing occult~4 but not master~4?
Show 10 quoted lines
> @@ -456,8 +457,11 @@ static void check_non_tip(void)
>  	char namebuf[42]; /* ^ + SHA-1 + LF */
>  	int i;
>  
> -	/* In the normal in-process case non-tip request can never happen */
> -	if (!stateless_rpc)
> +	/*
> +	 * In the normal in-process case without
> +	 * uploadpack.allowreachablesha1inwant, non-tip requests can never happen.
> +	 */

That's quite an overlong line; if you switched it to multi-line, then fold the line once more, perhaps before "non-tip"?

Show 5 quoted lines
> @@ -728,6 +732,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
>  			     sha1_to_hex(sha1), refname_nons,
>  			     0, capabilities,
>  			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
> +			     allow_reachable_sha1_in_want ? " allow-reachable-sha1-in-want" : "",
Which printf format specifier does this new element correspond to?
Fredrik Medley· May 3, 2015, 20:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] upload-pack: Optionally allow fetching reachable sha1

2015-05-03 19:40 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
Show 10 quoted lines
> Fredrik Medley <fredrik.medley@gmail.com> writes:
>
>> For you who don't remember the email discussion, look through the references.
>
> Please don't do this.  Always describe the background yourself in
> the log message so that "git log" can be read offline.  "describe
> yourself" can be done by summarizing earlier discussion, borrowing
> others' words, of course.  And it is a very good idea to give
> references like you did after your summary to optionally allow
> people to verify your summary is correct.

Okay, I understand. My intention was to recapture the old thread, but to keep the part under the references for the commit message. When I get this answer, I do see that this is impossible to understand and I should probably have added a cover-letter instead. (This is my first time every I've supplied patches to such an open-source project.)

Is the text under the reference enough describing or should there be added some more background text?

Unless someone asks for more answers about my attempt to recap the old mail thread, I skip commenting on this part.

Show 6 quoted lines
>
> ...
>
>> With uploadpack.allowreachablesha1inwant configuration option set,
>
> "option set" on the server side, I presume?

Yes, can fix that. "...configuration option set on the server side, "git fetch" can..."

Show 19 quoted lines
>
>> "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,
>> hidden by transfer.hiderefs or not, will be processed.
>
> I am not sure if I am reading the last sentence correctly.  If there
> are two branches, 'master' (exposed) and 'occult' (hidden),
>
>     ---o---o---2---1---x master
>         \
>          o---2---1---y occult
>
> you can ask for the 40-hex object name of occult~2, but you cannot
> ask for the 40-hex object name of master~2?  Why such a restriction?
>
>  ... wait for an answer that justfies the restriction ...
>
> Does that justification applies for allowing occult~4 but not master~4?

I thought I wrote that both visible and hidden branches will be counted as branch tips. Obviously need to reformulate. Maybe: "Only objects reachable from the branch tips, i.e. the union of advertised branches and branches hidden by transfer.hiderefs, will be processed."

Show 14 quoted lines
>
>> @@ -456,8 +457,11 @@ static void check_non_tip(void)
>>       char namebuf[42]; /* ^ + SHA-1 + LF */
>>       int i;
>>
>> -     /* In the normal in-process case non-tip request can never happen */
>> -     if (!stateless_rpc)
>> +     /*
>> +      * In the normal in-process case without
>> +      * uploadpack.allowreachablesha1inwant, non-tip requests can never happen.
>> +      */
>
> That's quite an overlong line; if you switched it to multi-line,
> then fold the line once more, perhaps before "non-tip"?

Yes, can change that. This was to avoid more than 80 characters per line. Should I split to three lines then?

Show 8 quoted lines
>
>> @@ -728,6 +732,7 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
>>                            sha1_to_hex(sha1), refname_nons,
>>                            0, capabilities,
>>                            allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
>> +                          allow_reachable_sha1_in_want ? " allow-reachable-sha1-in-want" : "",
>
> Which printf format specifier does this new element correspond to?
Ouch! How could I miss that?! Fixing.
Eric Sunshine· May 3, 2015, 21:35 UTC · re: Fredrik Medley · lore

Re: [PATCH] upload-pack: Optionally allow fetching reachable sha1

On Sun, May 3, 2015 at 4:13 PM, Fredrik Medley <fredrik.medley@gmail.com> wrote:
Show 16 quoted lines
> 2015-05-03 19:40 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
>> Fredrik Medley <fredrik.medley@gmail.com> writes:
>>> For you who don't remember the email discussion, look through the references.
>>
>> Please don't do this.  Always describe the background yourself in
>> the log message so that "git log" can be read offline.  "describe
>> yourself" can be done by summarizing earlier discussion, borrowing
>> others' words, of course.  And it is a very good idea to give
>> references like you did after your summary to optionally allow
>> people to verify your summary is correct.
>
> Okay, I understand. My intention was to recapture the old thread, but
> to keep the part under the references for the commit message. When
> I get this answer, I do see that this is impossible to understand and I
> should probably have added a cover-letter instead. (This is my first
> time every I've supplied patches to such an open-source project.)
Welcome to open-source and to git.
> Is the text under the reference enough describing or should there
> be added some more background text?
> Unless someone asks for more answers about my attempt to recap
> the old mail thread, I skip commenting on this part.

What Junio meant was that the commit message should be sufficiently self-contained such that someone reading it should be able to understand the problem you're solving without being familiar with past discussions (and without having to chase down links), and why the solution you've chosen is desirable.

In this case, the paragraph following the references in your patch, which you had intended as the full commit message, may not be sufficient. You might come closer to a good commit message by fleshing out the paragraph preceding the references, and following that by the paragraph after the references. By "fleshing out", I mean providing salient information from the cited discussions at the points where you referenced them via [n] notation.

Your commit message should explain the problem in the current implementation (possibly justifying why you consider it a problem), and an explanation of the solution. You will want to keep the references (but move them to the bottom of the commit message), and cite them when appropriate in your explanation.

For a single patch like this, you don't really need a cover letter. Just make sure the commit message provides sufficient explanation and is self-contained. If you want to provide extra commentary not intended for the commit message, place it below the "---" line after your sign-off and before the diffstat.

Fredrik Medley· May 5, 2015, 21:21 UTC · re: Fredrik Medley · lore

[PATCH v2] upload-pack: Optionally allow fetching reachable sha1

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 hstory 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 allow-tip-sha1-in-want as every change has a ref.)

Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
---
 Documentation/config.txt |  6 ++++++
 fetch-pack.c             |  9 ++++++--
 t/t5516-fetch-push.sh    | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
 upload-pack.c            | 19 ++++++++++++-----
 4 files changed, 82 insertions(+), 7 deletions(-)
Show changes to 4 files +82 −7

Documentation/config.txt, fetch-pack.c, t/t5516-fetch-push.sh, upload-pack.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e5ceaf..76cd713 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2538,6 +2538,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/fetch-pack.c b/fetch-pack.c
index 48526aa..fb01b6c 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -43,7 +43,7 @@ static int marked;
 #define MAX_IN_VAIN 256
 
 static struct prio_queue rev_list = { compare_commits_by_commit_date };
-static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
+static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want, allow_reachable_sha1_in_want;
 
 static void rev_list_push(struct commit *commit, int mark)
 {
@@ -542,7 +542,7 @@ static void filter_refs(struct fetch_pack_args *args,
 	}
 
 	/* Append unmatched requests to the list */
-	if (allow_tip_sha1_in_want) {
+	if (allow_tip_sha1_in_want || allow_reachable_sha1_in_want) {
 		for (i = 0; i < nr_sought; i++) {
 			unsigned char sha1[20];
 
@@ -823,6 +823,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 			fprintf(stderr, "Server supports allow-tip-sha1-in-want\n");
 		allow_tip_sha1_in_want = 1;
 	}
+	if (server_supports("allow-reachable-sha1-in-want")) {
+		if (args->verbose)
+			fprintf(stderr, "Server supports allow-reachable-sha1-in-want\n");
+		allow_reachable_sha1_in_want = 1;
+	}
 	if (!server_supports("thin-pack"))
 		args->use_thin_pack = 0;
 	if (!server_supports("no-progress"))
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 8a5f236..01fabfb 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1120,6 +1120,61 @@ test_expect_success 'fetch exact SHA1' '
 	)
 '
 
+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 &&
+			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+			git fetch --depth=1 ../testrepo/.git $SHA1 &&
+			git cat-file commit $SHA1 >/dev/null
+		)
+	'
+
+	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
+		)
+		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 &&
+			git cat-file commit $SHA1_3 >/dev/null &&
+			git cat-file commit $SHA1_3 >/dev/null
+		) &&
+		mk_empty shallow &&
+		(
+			cd shallow &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
+			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+			git fetch ../testrepo/.git $SHA1_1 &&
+			git cat-file commit $SHA1_1 >/dev/null &&
+			test_must_fail git cat-file commit $SHA1_2 >/dev/null &&
+			git fetch ../testrepo/.git $SHA1_2 &&
+			git cat-file commit $SHA1_2 >/dev/null &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_3
+		)
+	'
+done
+
 test_expect_success 'fetch follows tags by default' '
 	mk_test testrepo heads/master &&
 	rm -fr src dst &&
diff --git a/upload-pack.c b/upload-pack.c
index aa84576..cea4c57 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -36,6 +36,7 @@ static int no_done;
 static int use_thin_pack, use_ofs_delta, use_include_tag;
 static int no_progress, daemon_mode;
 static int allow_tip_sha1_in_want;
+static int allow_reachable_sha1_in_want;
 static int shallow_nr;
 static struct object_array have_obj;
 static struct object_array want_obj;
@@ -442,8 +443,9 @@ static int get_common_commits(void)
 
 static int is_our_ref(struct object *o)
 {
-	return o->flags &
-		((allow_tip_sha1_in_want ? HIDDEN_REF : 0) | OUR_REF);
+	int allow_hidden_ref =
+		allow_tip_sha1_in_want || allow_reachable_sha1_in_want;
+	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
 
 static void check_non_tip(void)
@@ -456,8 +458,12 @@ static void check_non_tip(void)
 	char namebuf[42]; /* ^ + SHA-1 + LF */
 	int i;
 
-	/* In the normal in-process case non-tip request can never happen */
-	if (!stateless_rpc)
+	/*
+	 * In the normal in-process case without
+	 * uploadpack.allowreachablesha1inwant,
+	 * non-tip requests can never happen.
+	 */
+	if (!stateless_rpc && !allow_reachable_sha1_in_want)
 		goto error;
 
 	cmd.argv = argv;
@@ -724,10 +730,11 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 		struct strbuf symref_info = STRBUF_INIT;
 
 		format_symref_info(&symref_info, cb_data);
-		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
+		packet_write(1, "%s %s%c%s%s%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
 			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
+			     allow_reachable_sha1_in_want ? " allow-reachable-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
 			     git_user_agent_sanitized());
@@ -789,6 +796,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 {
 	if (!strcmp("uploadpack.allowtipsha1inwant", var))
 		allow_tip_sha1_in_want = git_config_bool(var, value);
+	else if (!strcmp("uploadpack.allowreachablesha1inwant", var))
+		allow_reachable_sha1_in_want = git_config_bool(var, value);
 	else if (!strcmp("uploadpack.keepalive", var)) {
 		keepalive = git_config_int(var, value);
 		if (!keepalive)
-- 
1.9.1
Junio C Hamano· May 5, 2015, 22:16 UTC · re: Fredrik Medley · lore

Re: [PATCH v2] upload-pack: Optionally allow fetching reachable sha1

Fredrik Medley <fredrik.medley@gmail.com> writes:
Show 7 quoted lines
> 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 hstory to check the reachability.
s/hstory/history/
Show 12 quoted lines
>
> 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 allow-tip-sha1-in-want as every change has a ref.)
>
> Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
> ---
Much easier to read and understand.
Show 15 quoted lines
>  Documentation/config.txt |  6 ++++++
>  fetch-pack.c             |  9 ++++++--
>  t/t5516-fetch-push.sh    | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
>  upload-pack.c            | 19 ++++++++++++-----
>  4 files changed, 82 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e5ceaf..76cd713 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2538,6 +2538,12 @@ uploadpack.allowtipsha1inwant::
>  	of a hidden ref (by default, such a request is rejected).
>  	see also `uploadpack.hideRefs`.
>  
> +uploadpack.allowreachablesha1inwant::

I know that the existing allowtipsha1inwant is spelled that way, and it may be better done as a separate clean-up patch (either before or after this step), but the documentation and the first line of the log message would be easier to read with

	uploadpack.allowReachableSHA1InWant
I'd think.
> +	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`.
s/<TAB>object/<SPACE>object/
Show 10 quoted lines
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 48526aa..fb01b6c 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -43,7 +43,7 @@ static int marked;
>  #define MAX_IN_VAIN 256
>  
>  static struct prio_queue rev_list = { compare_commits_by_commit_date };
> -static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
> +static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want, allow_reachable_sha1_in_want;

Do we anticipate need for other variations of "allowing bare SHA-1 that they did not advertise" in the future?

That is a trick question. We didn't anticipate it, and that is why the existing feature squats on a whole integer variable. And we are paying the price of that lack of foresight by having to enhance with this patch. So the only sensible answer to that question is "we might need to keep this extensible".

How about renaming the existing allow_tip_sha1_in_want to something more generic to cover all such needs, e.g.

    #define ALLOW_TIP          01
    #define ALLOW_REACHABLE    02
    static int allow_request_with_bare_object_name;

Then you do not have to write (tip || reachable), and more importantly, you do not have to force the next person to update that to (tip || reachable || his_new_kind), I would think.

Show 6 quoted lines
> @@ -789,6 +796,8 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
>  {
>  	if (!strcmp("uploadpack.allowtipsha1inwant", var))
>  		allow_tip_sha1_in_want = git_config_bool(var, value);
> +	else if (!strcmp("uploadpack.allowreachablesha1inwant", var))
> +		allow_reachable_sha1_in_want = git_config_bool(var, value);

Using all lowercase is correct here, even though the "camelCase in documentation and log message for humans" suggestion above still stands.

Thanks.
Fredrik Medley· May 6, 2015, 20:10 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] upload-pack: Optionally allow fetching reachable sha1

2015-05-06 0:16 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
Show 21 quoted lines
> Fredrik Medley <fredrik.medley@gmail.com> writes:
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 2e5ceaf..76cd713 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -2538,6 +2538,12 @@ uploadpack.allowtipsha1inwant::
>>       of a hidden ref (by default, such a request is rejected).
>>       see also `uploadpack.hideRefs`.
>>
>> +uploadpack.allowreachablesha1inwant::
>
> I know that the existing allowtipsha1inwant is spelled that way, and
> it may be better done as a separate clean-up patch (either before or
> after this step), but the documentation and the first line of the
> log message would be easier to read with
>
>         uploadpack.allowReachableSHA1InWant
>
> I'd think.
>

I would prefer using allowReachableSha1InWant. Please tell me if I should use SHA1InWant instead of Sha1InWant. (I cannot find anything similar in the repository.)

Show 32 quoted lines
>
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index 48526aa..fb01b6c 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -43,7 +43,7 @@ static int marked;
>>  #define MAX_IN_VAIN 256
>>
>>  static struct prio_queue rev_list = { compare_commits_by_commit_date };
>> -static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
>> +static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want, allow_reachable_sha1_in_want;
>
> Do we anticipate need for other variations of "allowing bare SHA-1
> that they did not advertise" in the future?
>
> That is a trick question.  We didn't anticipate it, and that is why
> the existing feature squats on a whole integer variable.  And we are
> paying the price of that lack of foresight by having to enhance with
> this patch.  So the only sensible answer to that question is "we
> might need to keep this extensible".
>
> How about renaming the existing allow_tip_sha1_in_want to something
> more generic to cover all such needs, e.g.
>
>     #define ALLOW_TIP          01
>     #define ALLOW_REACHABLE    02
>     static int allow_request_with_bare_object_name;
>
> Then you do not have to write (tip || reachable), and more
> importantly, you do not have to force the next person to update that
> to (tip || reachable || his_new_kind), I would think.
>

I think the reasoning is sensible and I can see a possibility to allow non-reachable SHA1 in the future, even I would very much recommend against the use of such option for security reasons.

What I can understand, the capability protocol will still need both options as separate variables. (I forgot to update the technical documentation before will do that as well.)

Thank you all for quick replies.
Junio C Hamano· May 6, 2015, 20:19 UTC · re: Fredrik Medley · lore

Re: [PATCH v2] upload-pack: Optionally allow fetching reachable sha1

Fredrik Medley <fredrik.medley@gmail.com> writes:
Show 26 quoted lines
> 2015-05-06 0:16 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
>> Fredrik Medley <fredrik.medley@gmail.com> writes:
>>>
>>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>>> index 2e5ceaf..76cd713 100644
>>> --- a/Documentation/config.txt
>>> +++ b/Documentation/config.txt
>>> @@ -2538,6 +2538,12 @@ uploadpack.allowtipsha1inwant::
>>>       of a hidden ref (by default, such a request is rejected).
>>>       see also `uploadpack.hideRefs`.
>>>
>>> +uploadpack.allowreachablesha1inwant::
>>
>> I know that the existing allowtipsha1inwant is spelled that way, and
>> it may be better done as a separate clean-up patch (either before or
>> after this step), but the documentation and the first line of the
>> log message would be easier to read with
>>
>>         uploadpack.allowReachableSHA1InWant
>>
>> I'd think.
>>
>
> I would prefer using allowReachableSha1InWant. Please tell
> me if I should use SHA1InWant instead of Sha1InWant.
> (I cannot find anything similar in the repository.)
Keep in mind what was discussed recently:
  http://thread.gmane.org/gmane.comp.version-control.git/265225/focus=265322

I would think SHA1 should be upcased (so should SSL, SMTP, etc.) even in the existing ones when we do the "clean-up" pass. Even though this patch is not about cleaning up existing mess, there is no point adding more cruft that we need to clean up later ;-)

Fredrik Medley· May 12, 2015, 21:14 UTC · re: Junio C Hamano · lore

[PATCH 1/3] config.txt: Clarify allowTipSHA1InWant with camelCase

Most of the options in config.txt are camelCase. Improve the readability
for allowtipsha1inwant by changing to allowTipSHA1InWant.
---
 Documentation/config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to Documentation/config.txt +2 −2
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e5ceaf..2b86fe6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2530,9 +2530,9 @@ uploadpack.hideRefs::
 	are under the hierarchies listed on the value of this
 	variable is excluded, and is hidden from `git ls-remote`,
 	`git fetch`, etc.  An attempt to fetch a hidden ref by `git
-	fetch` will fail.  See also `uploadpack.allowtipsha1inwant`.
+	fetch` will fail.  See also `uploadpack.allowTipSHA1InWant`.
 
-uploadpack.allowtipsha1inwant::
+uploadpack.allowTipSHA1InWant::
 	When `uploadpack.hideRefs` is in effect, allow `upload-pack`
 	to accept a fetch request that asks for an object at the tip
 	of a hidden ref (by default, such a request is rejected).
-- 
1.9.1
Fredrik Medley· May 12, 2015, 21:14 UTC · re: Fredrik Medley · lore

[PATCH 2/3] upload-pack: Prepare to extend allow-tip-sha1-in-want

Rename the allow_tip_sha1_in_want variable to
allow_request_with_bare_object_name to allow for future extensions, e.g.
allowing non-tip sha1.
---
 fetch-pack.c  |  9 ++++++---
 upload-pack.c | 18 +++++++++++-------
 2 files changed, 17 insertions(+), 10 deletions(-)
Show changes to 2 files +17 −10

fetch-pack.c, upload-pack.c

diff --git a/fetch-pack.c b/fetch-pack.c
index 48526aa..77174f9 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -43,7 +43,10 @@ static int marked;
 #define MAX_IN_VAIN 256
 
 static struct prio_queue rev_list = { compare_commits_by_commit_date };
-static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
+static int non_common_revs, multi_ack, use_sideband;
+/* Allow specifying sha1 if it is a ref tip. */
+#define ALLOW_TIP	01
+static int allow_request_with_bare_object_name;
 
 static void rev_list_push(struct commit *commit, int mark)
 {
@@ -542,7 +545,7 @@ static void filter_refs(struct fetch_pack_args *args,
 	}
 
 	/* Append unmatched requests to the list */
-	if (allow_tip_sha1_in_want) {
+	if (allow_request_with_bare_object_name & ALLOW_TIP) {
 		for (i = 0; i < nr_sought; i++) {
 			unsigned char sha1[20];
 
@@ -821,7 +824,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 	if (server_supports("allow-tip-sha1-in-want")) {
 		if (args->verbose)
 			fprintf(stderr, "Server supports allow-tip-sha1-in-want\n");
-		allow_tip_sha1_in_want = 1;
+		allow_request_with_bare_object_name |= ALLOW_TIP;
 	}
 	if (!server_supports("thin-pack"))
 		args->use_thin_pack = 0;
diff --git a/upload-pack.c b/upload-pack.c
index aa84576..708a502 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -35,7 +35,9 @@ static int multi_ack;
 static int no_done;
 static int use_thin_pack, use_ofs_delta, use_include_tag;
 static int no_progress, daemon_mode;
-static int allow_tip_sha1_in_want;
+/* Allow specifying sha1 if it is a ref tip. */
+#define ALLOW_TIP	01
+static int allow_request_with_bare_object_name;
 static int shallow_nr;
 static struct object_array have_obj;
 static struct object_array want_obj;
@@ -442,8 +444,8 @@ static int get_common_commits(void)
 
 static int is_our_ref(struct object *o)
 {
-	return o->flags &
-		((allow_tip_sha1_in_want ? HIDDEN_REF : 0) | OUR_REF);
+	int allow_hidden_ref = (allow_request_with_bare_object_name & ALLOW_TIP);
+	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
 
 static void check_non_tip(void)
@@ -727,7 +729,8 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
-			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
+			     (allow_request_with_bare_object_name & ALLOW_TIP) ?
+				     " allow-tip-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
 			     git_user_agent_sanitized());
@@ -787,9 +790,10 @@ static void upload_pack(void)
 
 static int upload_pack_config(const char *var, const char *value, void *unused)
 {
-	if (!strcmp("uploadpack.allowtipsha1inwant", var))
-		allow_tip_sha1_in_want = git_config_bool(var, value);
-	else if (!strcmp("uploadpack.keepalive", var)) {
+	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
+		if (git_config_bool(var, value))
+			allow_request_with_bare_object_name |= ALLOW_TIP;
+	} else if (!strcmp("uploadpack.keepalive", var)) {
 		keepalive = git_config_int(var, value);
 		if (!keepalive)
 			keepalive = -1;
-- 
1.9.1
Eric Sunshine· May 12, 2015, 21:37 UTC · re: Fredrik Medley · lore

Re: [PATCH 2/3] upload-pack: Prepare to extend allow-tip-sha1-in-want

On Tue, May 12, 2015 at 5:14 PM, Fredrik Medley <fredrik.medley@gmail.com> wrote:

> Rename the allow_tip_sha1_in_want variable to
> allow_request_with_bare_object_name to allow for future extensions, e.g.
> allowing non-tip sha1.

'allow_request_with_bare_object_name' is quite a mouthful. Does it need to be this long?

Regarding the commit message: Isn't this preparatory step really about changing the variable from a simple boolean to a "flag"-style so that it can hold bits for multiple options? Perhaps the commit message should mention something about that?

Missing sign-off.
More below.
Show 23 quoted lines
> ---
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 48526aa..77174f9 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -43,7 +43,10 @@ static int marked;
>  #define MAX_IN_VAIN 256
>
>  static struct prio_queue rev_list = { compare_commits_by_commit_date };
> -static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
> +static int non_common_revs, multi_ack, use_sideband;
> +/* Allow specifying sha1 if it is a ref tip. */
> +#define ALLOW_TIP      01
> +static int allow_request_with_bare_object_name;
>
>  static void rev_list_push(struct commit *commit, int mark)
>  {
> @@ -542,7 +545,7 @@ static void filter_refs(struct fetch_pack_args *args,
>         }
>
>         /* Append unmatched requests to the list */
> -       if (allow_tip_sha1_in_want) {
> +       if (allow_request_with_bare_object_name & ALLOW_TIP) {

Some compilers are going to warn about this (warning: did you mean "&&" rather than "&"?). Wrap it in an extra set of parentheses to avoid the warning.

Show 64 quoted lines
>                 for (i = 0; i < nr_sought; i++) {
>                         unsigned char sha1[20];
>
> @@ -821,7 +824,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>         if (server_supports("allow-tip-sha1-in-want")) {
>                 if (args->verbose)
>                         fprintf(stderr, "Server supports allow-tip-sha1-in-want\n");
> -               allow_tip_sha1_in_want = 1;
> +               allow_request_with_bare_object_name |= ALLOW_TIP;
>         }
>         if (!server_supports("thin-pack"))
>                 args->use_thin_pack = 0;
> diff --git a/upload-pack.c b/upload-pack.c
> index aa84576..708a502 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -35,7 +35,9 @@ static int multi_ack;
>  static int no_done;
>  static int use_thin_pack, use_ofs_delta, use_include_tag;
>  static int no_progress, daemon_mode;
> -static int allow_tip_sha1_in_want;
> +/* Allow specifying sha1 if it is a ref tip. */
> +#define ALLOW_TIP      01
> +static int allow_request_with_bare_object_name;
>  static int shallow_nr;
>  static struct object_array have_obj;
>  static struct object_array want_obj;
> @@ -442,8 +444,8 @@ static int get_common_commits(void)
>
>  static int is_our_ref(struct object *o)
>  {
> -       return o->flags &
> -               ((allow_tip_sha1_in_want ? HIDDEN_REF : 0) | OUR_REF);
> +       int allow_hidden_ref = (allow_request_with_bare_object_name & ALLOW_TIP);
> +       return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
>  }
>
>  static void check_non_tip(void)
> @@ -727,7 +729,8 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
>                 packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
>                              sha1_to_hex(sha1), refname_nons,
>                              0, capabilities,
> -                            allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
> +                            (allow_request_with_bare_object_name & ALLOW_TIP) ?
> +                                    " allow-tip-sha1-in-want" : "",
>                              stateless_rpc ? " no-done" : "",
>                              symref_info.buf,
>                              git_user_agent_sanitized());
> @@ -787,9 +790,10 @@ static void upload_pack(void)
>
>  static int upload_pack_config(const char *var, const char *value, void *unused)
>  {
> -       if (!strcmp("uploadpack.allowtipsha1inwant", var))
> -               allow_tip_sha1_in_want = git_config_bool(var, value);
> -       else if (!strcmp("uploadpack.keepalive", var)) {
> +       if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
> +               if (git_config_bool(var, value))
> +                       allow_request_with_bare_object_name |= ALLOW_TIP;
> +       } else if (!strcmp("uploadpack.keepalive", var)) {
>                 keepalive = git_config_int(var, value);
>                 if (!keepalive)
>                         keepalive = -1;
> --
> 1.9.1
Junio C Hamano· May 12, 2015, 21:39 UTC · re: Fredrik Medley · lore

Re: [PATCH 2/3] upload-pack: Prepare to extend allow-tip-sha1-in-want

Fredrik Medley <fredrik.medley@gmail.com> writes:
> Subject: Re: [PATCH 2/3] upload-pack: Prepare to extend allow-tip-sha1-in-want
s/: Prepare/: prepare/;
> Rename the allow_tip_sha1_in_want variable to
> allow_request_with_bare_object_name to allow for future extensions, e.g.
> allowing non-tip sha1.
> ---
Sign-off?
Show 17 quoted lines
>  fetch-pack.c  |  9 ++++++---
>  upload-pack.c | 18 +++++++++++-------
>  2 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 48526aa..77174f9 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -43,7 +43,10 @@ static int marked;
>  #define MAX_IN_VAIN 256
>  
>  static struct prio_queue rev_list = { compare_commits_by_commit_date };
> -static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
> +static int non_common_revs, multi_ack, use_sideband;
> +/* Allow specifying sha1 if it is a ref tip. */
> +#define ALLOW_TIP	01
> +static int allow_request_with_bare_object_name;

This side is OK, as "request" is by the end user giving the object name from its command line.

Show 12 quoted lines
> diff --git a/upload-pack.c b/upload-pack.c
> index aa84576..708a502 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -35,7 +35,9 @@ static int multi_ack;
>  static int no_done;
>  static int use_thin_pack, use_ofs_delta, use_include_tag;
>  static int no_progress, daemon_mode;
> -static int allow_tip_sha1_in_want;
> +/* Allow specifying sha1 if it is a ref tip. */
> +#define ALLOW_TIP	01
> +static int allow_request_with_bare_object_name;

This side is not quite good, as the request coming over wire is always 40-hex bare object name. We are allowing requests against what we did not advertise (either the tip of hidden refs, or somewhere deep in the history from some tip that may or may not have been advertised).

allow-unadvertised-object-request or something, perhaps?
Show 38 quoted lines
>  static int shallow_nr;
>  static struct object_array have_obj;
>  static struct object_array want_obj;
> @@ -442,8 +444,8 @@ static int get_common_commits(void)
>  
>  static int is_our_ref(struct object *o)
>  {
> -	return o->flags &
> -		((allow_tip_sha1_in_want ? HIDDEN_REF : 0) | OUR_REF);
> +	int allow_hidden_ref = (allow_request_with_bare_object_name & ALLOW_TIP);
> +	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
>  }
>  
>  static void check_non_tip(void)
> @@ -727,7 +729,8 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
>  		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
>  			     sha1_to_hex(sha1), refname_nons,
>  			     0, capabilities,
> -			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
> +			     (allow_request_with_bare_object_name & ALLOW_TIP) ?
> +				     " allow-tip-sha1-in-want" : "",
>  			     stateless_rpc ? " no-done" : "",
>  			     symref_info.buf,
>  			     git_user_agent_sanitized());
> @@ -787,9 +790,10 @@ static void upload_pack(void)
>  
>  static int upload_pack_config(const char *var, const char *value, void *unused)
>  {
> -	if (!strcmp("uploadpack.allowtipsha1inwant", var))
> -		allow_tip_sha1_in_want = git_config_bool(var, value);
> -	else if (!strcmp("uploadpack.keepalive", var)) {
> +	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
> +		if (git_config_bool(var, value))
> +			allow_request_with_bare_object_name |= ALLOW_TIP;
> +	} else if (!strcmp("uploadpack.keepalive", var)) {
>  		keepalive = git_config_int(var, value);
>  		if (!keepalive)
>  			keepalive = -1;
Fredrik Medley· May 19, 2015, 20:19 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] upload-pack: Prepare to extend allow-tip-sha1-in-want

2015-05-12 23:39 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
Show 42 quoted lines
> Fredrik Medley <fredrik.medley@gmail.com> writes:
>
>> Subject: Re: [PATCH 2/3] upload-pack: Prepare to extend allow-tip-sha1-in-want
>>
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index 48526aa..77174f9 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -43,7 +43,10 @@ static int marked;
>>  #define MAX_IN_VAIN 256
>>
>>  static struct prio_queue rev_list = { compare_commits_by_commit_date };
>> -static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
>> +static int non_common_revs, multi_ack, use_sideband;
>> +/* Allow specifying sha1 if it is a ref tip. */
>> +#define ALLOW_TIP    01
>> +static int allow_request_with_bare_object_name;
>
> This side is OK, as "request" is by the end user giving the object
> name from its command line.
>
>> diff --git a/upload-pack.c b/upload-pack.c
>> index aa84576..708a502 100644
>> --- a/upload-pack.c
>> +++ b/upload-pack.c
>> @@ -35,7 +35,9 @@ static int multi_ack;
>>  static int no_done;
>>  static int use_thin_pack, use_ofs_delta, use_include_tag;
>>  static int no_progress, daemon_mode;
>> -static int allow_tip_sha1_in_want;
>> +/* Allow specifying sha1 if it is a ref tip. */
>> +#define ALLOW_TIP    01
>> +static int allow_request_with_bare_object_name;
>
> This side is not quite good, as the request coming over wire is
> always 40-hex bare object name.  We are allowing requests against
> what we did not advertise (either the tip of hidden refs, or
> somewhere deep in the history from some tip that may or may not have
> been advertised).
>
> allow-unadvertised-object-request or something, perhaps?
>

My imagination is not any better, so I will stick with allow_unadvertised_object_request. I do think that that name fits okay in fetch-pack.c as well. The name will also allow for a possibly future option of requesting (unadvertised) hidden refs by name, but it would of course require larger changes of the protocol.

Fredrik Medley· May 12, 2015, 21:14 UTC · re: Fredrik Medley · lore

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

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 hstory 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 <fredrik.medley@gmail.com>
---
 Documentation/config.txt                          |  6 +++
 Documentation/technical/http-protocol.txt         |  3 +-
 Documentation/technical/protocol-capabilities.txt |  7 +++
 fetch-pack.c                                      |  9 +++-
 t/t5516-fetch-push.sh                             | 55 +++++++++++++++++++++++
 upload-pack.c                                     | 20 +++++++--
 6 files changed, 94 insertions(+), 6 deletions(-)
Show changes to 6 files +94 −6

Documentation/config.txt, Documentation/technical/http-protocol.txt, Documentation/technical/protocol-capabilities.txt, fetch-pack.c, t/t5516-fetch-push.sh, upload-pack.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2b86fe6..ffc4cf4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2538,6 +2538,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=<nonce>
 -----------------
 
diff --git a/fetch-pack.c b/fetch-pack.c
index 77174f9..6c63d8e 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	01
+/* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */
+#define ALLOW_REACHABLE	02
 static int allow_request_with_bare_object_name;
 
 static void rev_list_push(struct commit *commit, int mark)
@@ -545,7 +547,7 @@ static void filter_refs(struct fetch_pack_args *args,
 	}
 
 	/* Append unmatched requests to the list */
-	if (allow_request_with_bare_object_name & ALLOW_TIP) {
+	if (allow_request_with_bare_object_name & (ALLOW_TIP | ALLOW_REACHABLE)) {
 		for (i = 0; i < nr_sought; i++) {
 			unsigned char sha1[20];
 
@@ -826,6 +828,11 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 			fprintf(stderr, "Server supports allow-tip-sha1-in-want\n");
 		allow_request_with_bare_object_name |= ALLOW_TIP;
 	}
+	if (server_supports("allow-reachable-sha1-in-want")) {
+		if (args->verbose)
+			fprintf(stderr, "Server supports allow-reachable-sha1-in-want\n");
+		allow_request_with_bare_object_name |= ALLOW_REACHABLE;
+	}
 	if (!server_supports("thin-pack"))
 		args->use_thin_pack = 0;
 	if (!server_supports("no-progress"))
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' '
 	)
 '
 
+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 &&
+			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+			git fetch --depth=1 ../testrepo/.git $SHA1 &&
+			git cat-file commit $SHA1 >/dev/null
+		)
+	'
+
+	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
+		)
+		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 &&
+			git cat-file commit $SHA1_3 >/dev/null &&
+			git cat-file commit $SHA1_3 >/dev/null
+		) &&
+		mk_empty shallow &&
+		(
+			cd shallow &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
+			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+			git fetch ../testrepo/.git $SHA1_1 &&
+			git cat-file commit $SHA1_1 >/dev/null &&
+			test_must_fail git cat-file commit $SHA1_2 >/dev/null &&
+			git fetch ../testrepo/.git $SHA1_2 &&
+			git cat-file commit $SHA1_2 >/dev/null &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_3
+		)
+	'
+done
+
 test_expect_success 'fetch follows tags by default' '
 	mk_test testrepo heads/master &&
 	rm -fr src dst &&
diff --git a/upload-pack.c b/upload-pack.c
index 708a502..bd14a81 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -37,6 +37,8 @@ static int use_thin_pack, use_ofs_delta, use_include_tag;
 static int no_progress, daemon_mode;
 /* Allow specifying sha1 if it is a ref tip. */
 #define ALLOW_TIP	01
+/* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */
+#define ALLOW_REACHABLE	02
 static int allow_request_with_bare_object_name;
 static int shallow_nr;
 static struct object_array have_obj;
@@ -444,7 +446,8 @@ static int get_common_commits(void)
 
 static int is_our_ref(struct object *o)
 {
-	int allow_hidden_ref = (allow_request_with_bare_object_name & ALLOW_TIP);
+	int allow_hidden_ref = (allow_request_with_bare_object_name &
+		(ALLOW_TIP | ALLOW_REACHABLE));
 	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
 
@@ -458,8 +461,12 @@ static void check_non_tip(void)
 	char namebuf[42]; /* ^ + SHA-1 + LF */
 	int i;
 
-	/* In the normal in-process case non-tip request can never happen */
-	if (!stateless_rpc)
+	/*
+	 * In the normal in-process case without
+	 * uploadpack.allowReachableSHA1InWant,
+	 * non-tip requests can never happen.
+	 */
+	if (!stateless_rpc && !(allow_request_with_bare_object_name & ALLOW_REACHABLE))
 		goto error;
 
 	cmd.argv = argv;
@@ -726,11 +733,13 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 		struct strbuf symref_info = STRBUF_INIT;
 
 		format_symref_info(&symref_info, cb_data);
-		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
+		packet_write(1, "%s %s%c%s%s%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
 			     (allow_request_with_bare_object_name & ALLOW_TIP) ?
 				     " allow-tip-sha1-in-want" : "",
+			     (allow_request_with_bare_object_name & ALLOW_REACHABLE) ?
+				     " allow-reachable-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
 			     git_user_agent_sanitized());
@@ -793,6 +802,9 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
 		if (git_config_bool(var, value))
 			allow_request_with_bare_object_name |= ALLOW_TIP;
+	} else if (!strcmp("uploadpack.allowreachablesha1inwant", var)) {
+		if (git_config_bool(var, value))
+			allow_request_with_bare_object_name |= ALLOW_REACHABLE;
 	} else if (!strcmp("uploadpack.keepalive", var)) {
 		keepalive = git_config_int(var, value);
 		if (!keepalive)
-- 
1.9.1
Eric Sunshine· May 12, 2015, 22:01 UTC · re: Fredrik Medley · lore

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

On Tue, May 12, 2015 at 5:14 PM, Fredrik Medley <fredrik.medley@gmail.com> wrote:

Show 7 quoted lines
> 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 hstory to check the reachability.
Mentioned previously[1]: s/hstory/history/
Show 23 quoted lines
> 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 <fredrik.medley@gmail.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2b86fe6..ffc4cf4 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2538,6 +2538,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.
Mentioned previously[1]: s/\s+/ /
Show 12 quoted lines
> +       Defaults to `false`.
> +
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 77174f9..6c63d8e 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -545,7 +547,7 @@ static void filter_refs(struct fetch_pack_args *args,
>         }
>
>         /* Append unmatched requests to the list */
> -       if (allow_request_with_bare_object_name & ALLOW_TIP) {
> +       if (allow_request_with_bare_object_name & (ALLOW_TIP | ALLOW_REACHABLE)) {
Wrap this in extra parentheses to avoid compilation warnings:
    if ((foo & (bar | baz))) {
>                 for (i = 0; i < nr_sought; i++) {
>                         unsigned char sha1[20];
>
[1]: http://article.gmane.org/gmane.comp.version-control.git/268428
Fredrik Medley· May 19, 2015, 20:44 UTC · re: Eric Sunshine · lore

[PATCH 1/3] config.txt: clarify allowTipSHA1InWant with camelCase

Most of the options in config.txt are camelCase. Improve the readability for allowtipsha1inwant by changing to allowTipSHA1InWant.

Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
---
This patch is optional. There has been work on fixing the whole
Documentation/config.txt which has not been merged yet. When adding
allowReachableSHA1InWant later, it would be good to already have changed
to allowTipSHA1InWant.
 Documentation/config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to Documentation/config.txt +2 −2
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2e5ceaf..2b86fe6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2530,9 +2530,9 @@ uploadpack.hideRefs::
 	are under the hierarchies listed on the value of this
 	variable is excluded, and is hidden from `git ls-remote`,
 	`git fetch`, etc.  An attempt to fetch a hidden ref by `git
-	fetch` will fail.  See also `uploadpack.allowtipsha1inwant`.
+	fetch` will fail.  See also `uploadpack.allowTipSHA1InWant`.
 
-uploadpack.allowtipsha1inwant::
+uploadpack.allowTipSHA1InWant::
 	When `uploadpack.hideRefs` is in effect, allow `upload-pack`
 	to accept a fetch request that asks for an object at the tip
 	of a hidden ref (by default, such a request is rejected).
-- 
1.9.1
Fredrik Medley· May 19, 2015, 20:44 UTC · re: Fredrik Medley · lore

[PATCH 2/3] upload-pack: prepare to extend allow-tip-sha1-in-want

To allow future extensions, e.g. allowing non-tip sha1, replace the boolean allow_tip_sha1_in_want variable with the flag-style allow_request_with_bare_object_name variable.

Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
---
 fetch-pack.c  |  9 ++++++---
 upload-pack.c | 18 +++++++++++-------
 2 files changed, 17 insertions(+), 10 deletions(-)
Show changes to 2 files +17 −10

fetch-pack.c, upload-pack.c

diff --git a/fetch-pack.c b/fetch-pack.c
index 48526aa..699f586 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -43,7 +43,10 @@ static int marked;
 #define MAX_IN_VAIN 256
 
 static struct prio_queue rev_list = { compare_commits_by_commit_date };
-static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
+static int non_common_revs, multi_ack, use_sideband;
+/* Allow specifying sha1 if it is a ref tip. */
+#define ALLOW_TIP_SHA1	01
+static int allow_unadvertised_object_request;
 
 static void rev_list_push(struct commit *commit, int mark)
 {
@@ -542,7 +545,7 @@ static void filter_refs(struct fetch_pack_args *args,
 	}
 
 	/* Append unmatched requests to the list */
-	if (allow_tip_sha1_in_want) {
+	if ((allow_unadvertised_object_request & ALLOW_TIP_SHA1)) {
 		for (i = 0; i < nr_sought; i++) {
 			unsigned char sha1[20];
 
@@ -821,7 +824,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 	if (server_supports("allow-tip-sha1-in-want")) {
 		if (args->verbose)
 			fprintf(stderr, "Server supports allow-tip-sha1-in-want\n");
-		allow_tip_sha1_in_want = 1;
+		allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
 	}
 	if (!server_supports("thin-pack"))
 		args->use_thin_pack = 0;
diff --git a/upload-pack.c b/upload-pack.c
index aa84576..d443e58 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -35,7 +35,9 @@ static int multi_ack;
 static int no_done;
 static int use_thin_pack, use_ofs_delta, use_include_tag;
 static int no_progress, daemon_mode;
-static int allow_tip_sha1_in_want;
+/* Allow specifying sha1 if it is a ref tip. */
+#define ALLOW_TIP_SHA1	01
+static int allow_unadvertised_object_request;
 static int shallow_nr;
 static struct object_array have_obj;
 static struct object_array want_obj;
@@ -442,8 +444,8 @@ static int get_common_commits(void)
 
 static int is_our_ref(struct object *o)
 {
-	return o->flags &
-		((allow_tip_sha1_in_want ? HIDDEN_REF : 0) | OUR_REF);
+	int allow_hidden_ref = (allow_unadvertised_object_request & ALLOW_TIP_SHA1);
+	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
 
 static void check_non_tip(void)
@@ -727,7 +729,8 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
-			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
+			     (allow_unadvertised_object_request & ALLOW_TIP_SHA1) ?
+				     " allow-tip-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
 			     git_user_agent_sanitized());
@@ -787,9 +790,10 @@ static void upload_pack(void)
 
 static int upload_pack_config(const char *var, const char *value, void *unused)
 {
-	if (!strcmp("uploadpack.allowtipsha1inwant", var))
-		allow_tip_sha1_in_want = git_config_bool(var, value);
-	else if (!strcmp("uploadpack.keepalive", var)) {
+	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
+		if (git_config_bool(var, value))
+			allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
+	} else if (!strcmp("uploadpack.keepalive", var)) {
 		keepalive = git_config_int(var, value);
 		if (!keepalive)
 			keepalive = -1;
-- 
1.9.1
Junio C Hamano· May 19, 2015, 22:00 UTC · re: Fredrik Medley · lore

Re: [PATCH 2/3] upload-pack: prepare to extend allow-tip-sha1-in-want

Fredrik Medley <fredrik.medley@gmail.com> writes:
Show 8 quoted lines
>  static int upload_pack_config(const char *var, const char *value, void *unused)
>  {
> -	if (!strcmp("uploadpack.allowtipsha1inwant", var))
> -		allow_tip_sha1_in_want = git_config_bool(var, value);
> -	else if (!strcmp("uploadpack.keepalive", var)) {
> +	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
> +		if (git_config_bool(var, value))
> +			allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
Doesn't this change break the behaviour?
Shouldn't you be able to say
	[uploadpack]
        	allowTipSHA1InWant = false

in a higher-precedence configuration file to override the same variable in other files in the configuration chain that may set it to true?

> +	} else if (!strcmp("uploadpack.keepalive", var)) {
>  		keepalive = git_config_int(var, value);
>  		if (!keepalive)
>  			keepalive = -1;
Fredrik Medley· May 20, 2015, 19:31 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] upload-pack: prepare to extend allow-tip-sha1-in-want

2015-05-20 0:00 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
Show 21 quoted lines
> Fredrik Medley <fredrik.medley@gmail.com> writes:
>
>>  static int upload_pack_config(const char *var, const char *value, void *unused)
>>  {
>> -     if (!strcmp("uploadpack.allowtipsha1inwant", var))
>> -             allow_tip_sha1_in_want = git_config_bool(var, value);
>> -     else if (!strcmp("uploadpack.keepalive", var)) {
>> +     if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
>> +             if (git_config_bool(var, value))
>> +                     allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
>
> Doesn't this change break the behaviour?
>
> Shouldn't you be able to say
>
>         [uploadpack]
>                 allowTipSHA1InWant = false
>
> in a higher-precedence configuration file to override the same
> variable in other files in the configuration chain that may set it
> to true?

Of course, thought it work differently. Should I add tests with test_config_global to check that loading the config works as well?

Show 6 quoted lines
>
>> +     } else if (!strcmp("uploadpack.keepalive", var)) {
>>               keepalive = git_config_int(var, value);
>>               if (!keepalive)
>>                       keepalive = -1;
>
Fredrik Medley· May 19, 2015, 20:44 UTC · re: Fredrik Medley · lore

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

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 <fredrik.medley@gmail.com>
---
 Documentation/config.txt                          |  6 +++
 Documentation/technical/http-protocol.txt         |  3 +-
 Documentation/technical/protocol-capabilities.txt |  7 +++
 fetch-pack.c                                      | 11 ++++-
 t/t5516-fetch-push.sh                             | 55 +++++++++++++++++++++++
 upload-pack.c                                     | 22 ++++++---
 6 files changed, 96 insertions(+), 8 deletions(-)
Show changes to 6 files +96 −8

Documentation/config.txt, Documentation/technical/http-protocol.txt, Documentation/technical/protocol-capabilities.txt, fetch-pack.c, t/t5516-fetch-push.sh, upload-pack.c

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2b86fe6..980520b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2538,6 +2538,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=<nonce>
 -----------------
 
diff --git a/fetch-pack.c b/fetch-pack.c
index 699f586..875b688 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -45,7 +45,9 @@ static int marked;
 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
+#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,7 @@ 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 +828,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-sha1-in-want")) {
+		if (args->verbose)
+			fprintf(stderr, "Server supports allow-reachable-sha1-in-want\n");
+		allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
+	}
 	if (!server_supports("thin-pack"))
 		args->use_thin_pack = 0;
 	if (!server_supports("no-progress"))
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' '
 	)
 '
 
+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 &&
+			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+			git fetch --depth=1 ../testrepo/.git $SHA1 &&
+			git cat-file commit $SHA1 >/dev/null
+		)
+	'
+
+	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
+		)
+		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 &&
+			git cat-file commit $SHA1_3 >/dev/null &&
+			git cat-file commit $SHA1_3 >/dev/null
+		) &&
+		mk_empty shallow &&
+		(
+			cd shallow &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
+			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+			git fetch ../testrepo/.git $SHA1_1 &&
+			git cat-file commit $SHA1_1 >/dev/null &&
+			test_must_fail git cat-file commit $SHA1_2 >/dev/null &&
+			git fetch ../testrepo/.git $SHA1_2 &&
+			git cat-file commit $SHA1_2 >/dev/null &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_3
+		)
+	'
+done
+
 test_expect_success 'fetch follows tags by default' '
 	mk_test testrepo heads/master &&
 	rm -fr src dst &&
diff --git a/upload-pack.c b/upload-pack.c
index d443e58..f99eb99 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -36,7 +36,9 @@ static int no_done;
 static int use_thin_pack, use_ofs_delta, use_include_tag;
 static int no_progress, daemon_mode;
 /* Allow specifying sha1 if it is a ref tip. */
-#define ALLOW_TIP_SHA1	01
+#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 int shallow_nr;
 static struct object_array have_obj;
@@ -444,7 +446,8 @@ static int get_common_commits(void)
 
 static int is_our_ref(struct object *o)
 {
-	int allow_hidden_ref = (allow_unadvertised_object_request & ALLOW_TIP_SHA1);
+	int allow_hidden_ref = (allow_unadvertised_object_request &
+		(ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));
 	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
 
@@ -458,8 +461,12 @@ static void check_non_tip(void)
 	char namebuf[42]; /* ^ + SHA-1 + LF */
 	int i;
 
-	/* In the normal in-process case non-tip request can never happen */
-	if (!stateless_rpc)
+	/*
+	 * In the normal in-process case without
+	 * uploadpack.allowReachableSHA1InWant,
+	 * non-tip requests can never happen.
+	 */
+	if (!stateless_rpc && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))
 		goto error;
 
 	cmd.argv = argv;
@@ -726,11 +733,13 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 		struct strbuf symref_info = STRBUF_INIT;
 
 		format_symref_info(&symref_info, cb_data);
-		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
+		packet_write(1, "%s %s%c%s%s%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
 			     (allow_unadvertised_object_request & ALLOW_TIP_SHA1) ?
 				     " allow-tip-sha1-in-want" : "",
+			     (allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1) ?
+				     " allow-reachable-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
 			     git_user_agent_sanitized());
@@ -793,6 +802,9 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
 		if (git_config_bool(var, value))
 			allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
+	} else if (!strcmp("uploadpack.allowreachablesha1inwant", var)) {
+		if (git_config_bool(var, value))
+			allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
 	} else if (!strcmp("uploadpack.keepalive", var)) {
 		keepalive = git_config_int(var, value);
 		if (!keepalive)
-- 
1.9.1
Junio C Hamano· May 19, 2015, 22:06 UTC · re: Fredrik Medley · lore

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

Fredrik Medley <fredrik.medley@gmail.com> writes:
Show 10 quoted lines
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 699f586..875b688 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -45,7 +45,9 @@ static int marked;
>  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
> +#define ALLOW_TIP_SHA1		01
What is this change about?  You do not have to align value columns.
> -	if ((allow_unadvertised_object_request & ALLOW_TIP_SHA1)) {
> +	if ((allow_unadvertised_object_request & (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
Perhaps wrap it like this?
	if ((allow_unadvertised_object_request &
            (ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1))) {
Show 7 quoted lines
> @@ -444,7 +446,8 @@ static int get_common_commits(void)
>  
>  static int is_our_ref(struct object *o)
>  {
> -	int allow_hidden_ref = (allow_unadvertised_object_request & ALLOW_TIP_SHA1);
> +	int allow_hidden_ref = (allow_unadvertised_object_request &
> +		(ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));
Good, but push that to even more right?
Show 10 quoted lines
> @@ -793,6 +802,9 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
>  	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
>  		if (git_config_bool(var, value))
>  			allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
> +	} else if (!strcmp("uploadpack.allowreachablesha1inwant", var)) {
> +		if (git_config_bool(var, value))
> +			allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
>  	} else if (!strcmp("uploadpack.keepalive", var)) {
>  		keepalive = git_config_int(var, value);
>  		if (!keepalive)
Same issue as [2/3]
		...
	} else if (!strcmp("configVariable", var)) {
        	if (git_config_bool(var, value))
			value |= ALLOW_THIS;
		else
                	value &= ~ALLOW_THIS;
	} else if ...
should be OK.
Thanks.
Fredrik Medley· May 21, 2015, 20:23 UTC · re: Fredrik Medley · lore

[PATCH v5 1/3] config.txt: clarify allowTipSHA1InWant with camelCase

Most of the options in config.txt are camelCase. Improve the readability for allowtipsha1inwant by changing to allowTipSHA1InWant.

Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
---
This patch is optional. There has been work on fixing the whole
Documentation/config.txt which has not been merged yet. When adding
allowReachableSHA1InWant later, it would be good to already have changed
to allowTipSHA1InWant.
 Documentation/config.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to Documentation/config.txt +2 −2
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 948b8b0..1311383 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2539,9 +2539,9 @@ uploadpack.hideRefs::
 	are under the hierarchies listed on the value of this
 	variable is excluded, and is hidden from `git ls-remote`,
 	`git fetch`, etc.  An attempt to fetch a hidden ref by `git
-	fetch` will fail.  See also `uploadpack.allowtipsha1inwant`.
+	fetch` will fail.  See also `uploadpack.allowTipSHA1InWant`.
 
-uploadpack.allowtipsha1inwant::
+uploadpack.allowTipSHA1InWant::
 	When `uploadpack.hideRefs` is in effect, allow `upload-pack`
 	to accept a fetch request that asks for an object at the tip
 	of a hidden ref (by default, such a request is rejected).
-- 
1.9.1
Fredrik Medley· May 21, 2015, 20:23 UTC · re: Fredrik Medley · lore

[PATCH v5 2/3] upload-pack: prepare to extend allow-tip-sha1-in-want

To allow future extensions, e.g. allowing non-tip sha1, replace the boolean allow_tip_sha1_in_want variable with the flag-style allow_request_with_bare_object_name variable.

Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
---
 fetch-pack.c  |  9 ++++++---
 upload-pack.c | 20 +++++++++++++-------
 2 files changed, 19 insertions(+), 10 deletions(-)
Show changes to 2 files +19 −10

fetch-pack.c, upload-pack.c

diff --git a/fetch-pack.c b/fetch-pack.c
index 48526aa..699f586 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -43,7 +43,10 @@ static int marked;
 #define MAX_IN_VAIN 256
 
 static struct prio_queue rev_list = { compare_commits_by_commit_date };
-static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
+static int non_common_revs, multi_ack, use_sideband;
+/* Allow specifying sha1 if it is a ref tip. */
+#define ALLOW_TIP_SHA1	01
+static int allow_unadvertised_object_request;
 
 static void rev_list_push(struct commit *commit, int mark)
 {
@@ -542,7 +545,7 @@ static void filter_refs(struct fetch_pack_args *args,
 	}
 
 	/* Append unmatched requests to the list */
-	if (allow_tip_sha1_in_want) {
+	if ((allow_unadvertised_object_request & ALLOW_TIP_SHA1)) {
 		for (i = 0; i < nr_sought; i++) {
 			unsigned char sha1[20];
 
@@ -821,7 +824,7 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
 	if (server_supports("allow-tip-sha1-in-want")) {
 		if (args->verbose)
 			fprintf(stderr, "Server supports allow-tip-sha1-in-want\n");
-		allow_tip_sha1_in_want = 1;
+		allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
 	}
 	if (!server_supports("thin-pack"))
 		args->use_thin_pack = 0;
diff --git a/upload-pack.c b/upload-pack.c
index 745fda8..726486b 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -35,7 +35,9 @@ static int multi_ack;
 static int no_done;
 static int use_thin_pack, use_ofs_delta, use_include_tag;
 static int no_progress, daemon_mode;
-static int allow_tip_sha1_in_want;
+/* Allow specifying sha1 if it is a ref tip. */
+#define ALLOW_TIP_SHA1	01
+static int allow_unadvertised_object_request;
 static int shallow_nr;
 static struct object_array have_obj;
 static struct object_array want_obj;
@@ -442,8 +444,8 @@ static int get_common_commits(void)
 
 static int is_our_ref(struct object *o)
 {
-	return o->flags &
-		((allow_tip_sha1_in_want ? HIDDEN_REF : 0) | OUR_REF);
+	int allow_hidden_ref = (allow_unadvertised_object_request & ALLOW_TIP_SHA1);
+	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
 
 static void check_non_tip(void)
@@ -727,7 +729,8 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
-			     allow_tip_sha1_in_want ? " allow-tip-sha1-in-want" : "",
+			     (allow_unadvertised_object_request & ALLOW_TIP_SHA1) ?
+				     " allow-tip-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
 			     git_user_agent_sanitized());
@@ -787,9 +790,12 @@ static void upload_pack(void)
 
 static int upload_pack_config(const char *var, const char *value, void *unused)
 {
-	if (!strcmp("uploadpack.allowtipsha1inwant", var))
-		allow_tip_sha1_in_want = git_config_bool(var, value);
-	else if (!strcmp("uploadpack.keepalive", var)) {
+	if (!strcmp("uploadpack.allowtipsha1inwant", var)) {
+		if (git_config_bool(var, value))
+			allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
+		else
+			allow_unadvertised_object_request &= ~ALLOW_TIP_SHA1;
+	} else if (!strcmp("uploadpack.keepalive", var)) {
 		keepalive = git_config_int(var, value);
 		if (!keepalive)
 			keepalive = -1;
-- 
1.9.1
Junio C Hamano· May 21, 2015, 22:07 UTC · re: Fredrik Medley · lore

Re: [PATCH v5 2/3] upload-pack: prepare to extend allow-tip-sha1-in-want

Fredrik Medley <fredrik.medley@gmail.com> writes:
Show 23 quoted lines
> To allow future extensions, e.g. allowing non-tip sha1, replace the
> boolean allow_tip_sha1_in_want variable with the flag-style
> allow_request_with_bare_object_name variable.
>
> Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
> ---
>  fetch-pack.c  |  9 ++++++---
>  upload-pack.c | 20 +++++++++++++-------
>  2 files changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 48526aa..699f586 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -43,7 +43,10 @@ static int marked;
>  #define MAX_IN_VAIN 256
>  
>  static struct prio_queue rev_list = { compare_commits_by_commit_date };
> -static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
> +static int non_common_revs, multi_ack, use_sideband;
> +/* Allow specifying sha1 if it is a ref tip. */
> +#define ALLOW_TIP_SHA1	01
> +static int allow_unadvertised_object_request;

It is better to use "unsigned int" for these bit masks, as we are not interested in the top-most bit getting special-cased by using a signed type. I'll amend this (and the one in upload-pack.c) while applying, so no need to resend only to correct these two, unless you have other reasons to reroll.

Thanks.
Show 12 quoted lines
> diff --git a/upload-pack.c b/upload-pack.c
> index 745fda8..726486b 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -35,7 +35,9 @@ static int multi_ack;
>  static int no_done;
>  static int use_thin_pack, use_ofs_delta, use_include_tag;
>  static int no_progress, daemon_mode;
> -static int allow_tip_sha1_in_want;
> +/* Allow specifying sha1 if it is a ref tip. */
> +#define ALLOW_TIP_SHA1	01
> +static int allow_unadvertised_object_request;
Fredrik Medley· May 22, 2015, 23:47 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 2/3] upload-pack: prepare to extend allow-tip-sha1-in-want

2015-05-22 0:07 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
Show 31 quoted lines
> Fredrik Medley <fredrik.medley@gmail.com> writes:
>
>> To allow future extensions, e.g. allowing non-tip sha1, replace the
>> boolean allow_tip_sha1_in_want variable with the flag-style
>> allow_request_with_bare_object_name variable.
>>
>> Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
>> ---
>>  fetch-pack.c  |  9 ++++++---
>>  upload-pack.c | 20 +++++++++++++-------
>>  2 files changed, 19 insertions(+), 10 deletions(-)
>>
>> diff --git a/fetch-pack.c b/fetch-pack.c
>> index 48526aa..699f586 100644
>> --- a/fetch-pack.c
>> +++ b/fetch-pack.c
>> @@ -43,7 +43,10 @@ static int marked;
>>  #define MAX_IN_VAIN 256
>>
>>  static struct prio_queue rev_list = { compare_commits_by_commit_date };
>> -static int non_common_revs, multi_ack, use_sideband, allow_tip_sha1_in_want;
>> +static int non_common_revs, multi_ack, use_sideband;
>> +/* Allow specifying sha1 if it is a ref tip. */
>> +#define ALLOW_TIP_SHA1       01
>> +static int allow_unadvertised_object_request;
>
> It is better to use "unsigned int" for these bit masks, as we are
> not interested in the top-most bit getting special-cased by using a
> signed type.  I'll amend this (and the one in upload-pack.c) while
> applying, so no need to resend only to correct these two, unless you
> have other reasons to reroll.
Sounds like a good idea to change. Please amend it while applying.
>
> Thanks.
Thank you too for the review.
Show 14 quoted lines
>
>> diff --git a/upload-pack.c b/upload-pack.c
>> index 745fda8..726486b 100644
>> --- a/upload-pack.c
>> +++ b/upload-pack.c
>> @@ -35,7 +35,9 @@ static int multi_ack;
>>  static int no_done;
>>  static int use_thin_pack, use_ofs_delta, use_include_tag;
>>  static int no_progress, daemon_mode;
>> -static int allow_tip_sha1_in_want;
>> +/* Allow specifying sha1 if it is a ref tip. */
>> +#define ALLOW_TIP_SHA1       01
>> +static int allow_unadvertised_object_request;
>
Junio C Hamano· May 23, 2015, 00:53 UTC · re: Fredrik Medley · lore

Re: [PATCH v5 2/3] upload-pack: prepare to extend allow-tip-sha1-in-want

Fredrik Medley <fredrik.medley@gmail.com> writes:
Show 10 quoted lines
>>> +#define ALLOW_TIP_SHA1       01
>>> +static int allow_unadvertised_object_request;
>>
>> It is better to use "unsigned int" for these bit masks, as we are
>> not interested in the top-most bit getting special-cased by using a
>> signed type.  I'll amend this (and the one in upload-pack.c) while
>> applying, so no need to resend only to correct these two, unless you
>> have other reasons to reroll.
>
> Sounds like a good idea to change. Please amend it while applying.
Alright, will do.
Thanks.
Fredrik Medley· May 21, 2015, 20:23 UTC · re: Fredrik Medley · lore

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

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 <fredrik.medley@gmail.com>
---
 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(-)
Show changes to 6 files +97 −6

Documentation/config.txt, Documentation/technical/http-protocol.txt, Documentation/technical/protocol-capabilities.txt, fetch-pack.c, t/t5516-fetch-push.sh, upload-pack.c

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=<nonce>
 -----------------
 
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-sha1-in-want")) {
+		if (args->verbose)
+			fprintf(stderr, "Server supports allow-reachable-sha1-in-want\n");
+		allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
+	}
 	if (!server_supports("thin-pack"))
 		args->use_thin_pack = 0;
 	if (!server_supports("no-progress"))
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' '
 	)
 '
 
+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 &&
+			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+			git fetch --depth=1 ../testrepo/.git $SHA1 &&
+			git cat-file commit $SHA1 >/dev/null
+		)
+	'
+
+	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
+		)
+		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 &&
+			git cat-file commit $SHA1_3 >/dev/null &&
+			git cat-file commit $SHA1_3 >/dev/null
+		) &&
+		mk_empty shallow &&
+		(
+			cd shallow &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
+			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
+			git fetch ../testrepo/.git $SHA1_1 &&
+			git cat-file commit $SHA1_1 >/dev/null &&
+			test_must_fail git cat-file commit $SHA1_2 >/dev/null &&
+			git fetch ../testrepo/.git $SHA1_2 &&
+			git cat-file commit $SHA1_2 >/dev/null &&
+			test_must_fail git fetch ../testrepo/.git $SHA1_3
+		)
+	'
+done
+
 test_expect_success 'fetch follows tags by default' '
 	mk_test testrepo heads/master &&
 	rm -fr src dst &&
diff --git a/upload-pack.c b/upload-pack.c
index 726486b..15571d7 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -37,6 +37,8 @@ static int use_thin_pack, use_ofs_delta, use_include_tag;
 static int no_progress, daemon_mode;
 /* 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 int shallow_nr;
 static struct object_array have_obj;
@@ -444,7 +446,8 @@ static int get_common_commits(void)
 
 static int is_our_ref(struct object *o)
 {
-	int allow_hidden_ref = (allow_unadvertised_object_request & ALLOW_TIP_SHA1);
+	int allow_hidden_ref = (allow_unadvertised_object_request &
+			(ALLOW_TIP_SHA1 | ALLOW_REACHABLE_SHA1));
 	return o->flags & ((allow_hidden_ref ? HIDDEN_REF : 0) | OUR_REF);
 }
 
@@ -458,8 +461,12 @@ static void check_non_tip(void)
 	char namebuf[42]; /* ^ + SHA-1 + LF */
 	int i;
 
-	/* In the normal in-process case non-tip request can never happen */
-	if (!stateless_rpc)
+	/*
+	 * In the normal in-process case without
+	 * uploadpack.allowReachableSHA1InWant,
+	 * non-tip requests can never happen.
+	 */
+	if (!stateless_rpc && !(allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1))
 		goto error;
 
 	cmd.argv = argv;
@@ -726,11 +733,13 @@ static int send_ref(const char *refname, const unsigned char *sha1, int flag, vo
 		struct strbuf symref_info = STRBUF_INIT;
 
 		format_symref_info(&symref_info, cb_data);
-		packet_write(1, "%s %s%c%s%s%s%s agent=%s\n",
+		packet_write(1, "%s %s%c%s%s%s%s%s agent=%s\n",
 			     sha1_to_hex(sha1), refname_nons,
 			     0, capabilities,
 			     (allow_unadvertised_object_request & ALLOW_TIP_SHA1) ?
 				     " allow-tip-sha1-in-want" : "",
+			     (allow_unadvertised_object_request & ALLOW_REACHABLE_SHA1) ?
+				     " allow-reachable-sha1-in-want" : "",
 			     stateless_rpc ? " no-done" : "",
 			     symref_info.buf,
 			     git_user_agent_sanitized());
@@ -795,6 +804,11 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
 			allow_unadvertised_object_request |= ALLOW_TIP_SHA1;
 		else
 			allow_unadvertised_object_request &= ~ALLOW_TIP_SHA1;
+	} else if (!strcmp("uploadpack.allowreachablesha1inwant", var)) {
+		if (git_config_bool(var, value))
+			allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
+		else
+			allow_unadvertised_object_request &= ~ALLOW_REACHABLE_SHA1;
 	} else if (!strcmp("uploadpack.keepalive", var)) {
 		keepalive = git_config_int(var, value);
 		if (!keepalive)
-- 
1.9.1
Junio C Hamano· May 21, 2015, 22:15 UTC · re: Fredrik Medley · lore

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

Fredrik Medley <fredrik.medley@gmail.com> writes:
Show 8 quoted lines
> --- 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.

Show 7 quoted lines
> 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.

Show 15 quoted lines
> +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.

Show 12 quoted lines
> +		)
> +	'
> +
> +	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
Show 6 quoted lines
> +		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.

Show 6 quoted lines
> +		) &&
> +		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 +++++++-------
 2 files changed, 8 insertions(+), 8 deletions(-)
Show changes to 2 files +8 −7

Documentation/technical/protocol-capabilities.txt, t/t5516-fetch-push.sh

diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
index 265fcab..eaab6b4 100644
--- a/Documentation/technical/protocol-capabilities.txt
+++ b/Documentation/technical/protocol-capabilities.txt
@@ -261,7 +261,7 @@ 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
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index fdcc114..ec22c98 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1137,7 +1137,7 @@ do
 			test_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&
 			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
 			git fetch --depth=1 ../testrepo/.git $SHA1 &&
-			git cat-file commit $SHA1 >/dev/null
+			git cat-file commit $SHA1
 		)
 	'
 
@@ -1149,15 +1149,15 @@ do
 			git commit --allow-empty -m foo &&
 			git commit --allow-empty -m bar &&
 			git commit --allow-empty -m xyz
-		)
+		) &&
 		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 &&
-			git cat-file commit $SHA1_3 >/dev/null &&
-			git cat-file commit $SHA1_3 >/dev/null
+			git cat-file commit $SHA1_1 &&
+			git cat-file commit $SHA1_3
 		) &&
 		mk_empty shallow &&
 		(
@@ -1166,10 +1166,10 @@ do
 			test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
 			git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
 			git fetch ../testrepo/.git $SHA1_1 &&
-			git cat-file commit $SHA1_1 >/dev/null &&
-			test_must_fail git cat-file commit $SHA1_2 >/dev/null &&
+			git cat-file commit $SHA1_1 &&
+			test_must_fail git cat-file commit $SHA1_2 &&
 			git fetch ../testrepo/.git $SHA1_2 &&
-			git cat-file commit $SHA1_2 >/dev/null &&
+			git cat-file commit $SHA1_2 &&
 			test_must_fail git fetch ../testrepo/.git $SHA1_3
 		)
 	'
-- 
2.4.1-439-gcfa393f
Fredrik Medley· May 22, 2015, 23:55 UTC · re: Junio C Hamano · lore

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

2015-05-22 0:15 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
Show 28 quoted lines
> Fredrik Medley <fredrik.medley@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?

Show 62 quoted lines
>
>> +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.
Show 108 quoted lines
>
>> +             ) &&
>> +             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 +++++++-------
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/technical/protocol-capabilities.txt b/Documentation/technical/protocol-capabilities.txt
> index 265fcab..eaab6b4 100644
> --- a/Documentation/technical/protocol-capabilities.txt
> +++ b/Documentation/technical/protocol-capabilities.txt
> @@ -261,7 +261,7 @@ 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
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index fdcc114..ec22c98 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1137,7 +1137,7 @@ do
>                         test_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&
>                         git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
>                         git fetch --depth=1 ../testrepo/.git $SHA1 &&
> -                       git cat-file commit $SHA1 >/dev/null
> +                       git cat-file commit $SHA1
>                 )
>         '
>
> @@ -1149,15 +1149,15 @@ do
>                         git commit --allow-empty -m foo &&
>                         git commit --allow-empty -m bar &&
>                         git commit --allow-empty -m xyz
> -               )
> +               ) &&
>                 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 &&
> -                       git cat-file commit $SHA1_3 >/dev/null &&
> -                       git cat-file commit $SHA1_3 >/dev/null
> +                       git cat-file commit $SHA1_1 &&
> +                       git cat-file commit $SHA1_3
>                 ) &&
>                 mk_empty shallow &&
>                 (
> @@ -1166,10 +1166,10 @@ do
>                         test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
>                         git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
>                         git fetch ../testrepo/.git $SHA1_1 &&
> -                       git cat-file commit $SHA1_1 >/dev/null &&
> -                       test_must_fail git cat-file commit $SHA1_2 >/dev/null &&
> +                       git cat-file commit $SHA1_1 &&
> +                       test_must_fail git cat-file commit $SHA1_2 &&
>                         git fetch ../testrepo/.git $SHA1_2 &&
> -                       git cat-file commit $SHA1_2 >/dev/null &&
> +                       git cat-file commit $SHA1_2 &&
>                         test_must_fail git fetch ../testrepo/.git $SHA1_3
>                 )
>         '
> --
> 2.4.1-439-gcfa393f
>
Junio C Hamano· May 23, 2015, 01:01 UTC · re: Fredrik Medley · lore

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

Fredrik Medley <fredrik.medley@gmail.com> writes:
Show 9 quoted lines
> 2015-05-22 0:15 GMT+02:00 Junio C Hamano <gitster@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.
Eric Sunshine· May 12, 2015, 21:24 UTC · re: Fredrik Medley · lore

Re: [PATCH 1/3] config.txt: Clarify allowTipSHA1InWant with camelCase

On Tue, May 12, 2015 at 5:14 PM, Fredrik Medley <fredrik.medley@gmail.com> wrote:

> Most of the options in config.txt are camelCase. Improve the readability
> for allowtipsha1inwant by changing to allowTipSHA1InWant.
Missing sign-off. More below.
Show 14 quoted lines
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e5ceaf..2b86fe6 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2530,9 +2530,9 @@ uploadpack.hideRefs::
>         are under the hierarchies listed on the value of this
>         variable is excluded, and is hidden from `git ls-remote`,
>         `git fetch`, etc.  An attempt to fetch a hidden ref by `git
> -       fetch` will fail.  See also `uploadpack.allowtipsha1inwant`.
> +       fetch` will fail.  See also `uploadpack.allowTipSHA1InWant`.
>
> -uploadpack.allowtipsha1inwant::
> +uploadpack.allowTipSHA1InWant::

This was already attempted here[1]; some inconsistencies with acronyms and abbreviations pointed out here[2]; and delayed here[3]. Perhaps it's now time to revisit the acronyms/abbreviations issue?

[1]: http://thread.gmane.org/gmane.comp.version-control.git/265225/focus=265225 [2]: http://thread.gmane.org/gmane.comp.version-control.git/265225/focus=265262 [3]: http://thread.gmane.org/gmane.comp.version-control.git/265225/focus=265282

Show 5 quoted lines
>         When `uploadpack.hideRefs` is in effect, allow `upload-pack`
>         to accept a fetch request that asks for an object at the tip
>         of a hidden ref (by default, such a request is rejected).
> --
> 1.9.1
Junio C Hamano· May 12, 2015, 21:33 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/3] config.txt: Clarify allowTipSHA1InWant with camelCase

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 5 quoted lines
>> -uploadpack.allowtipsha1inwant::
>> +uploadpack.allowTipSHA1InWant::
>
> This was already attempted here[1]; some inconsistencies with acronyms
> and abbreviations pointed out here[2]; and delayed here[3].
This was entirely my "fault"; see $gmane/268493.
Junio C Hamano· May 12, 2015, 21:35 UTC · re: Fredrik Medley · lore

Re: [PATCH 1/3] config.txt: Clarify allowTipSHA1InWant with camelCase

Fredrik Medley <fredrik.medley@gmail.com> writes:
> Subject: Re: [PATCH 1/3] config.txt: Clarify allowTipSHA1InWant with camelCase
s/: Clarify/: clarify/;
> Most of the options in config.txt are camelCase. Improve the readability
> for allowtipsha1inwant by changing to allowTipSHA1InWant.
Sign-off?
Show 20 quoted lines
> ---
>  Documentation/config.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 2e5ceaf..2b86fe6 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2530,9 +2530,9 @@ uploadpack.hideRefs::
>  	are under the hierarchies listed on the value of this
>  	variable is excluded, and is hidden from `git ls-remote`,
>  	`git fetch`, etc.  An attempt to fetch a hidden ref by `git
> -	fetch` will fail.  See also `uploadpack.allowtipsha1inwant`.
> +	fetch` will fail.  See also `uploadpack.allowTipSHA1InWant`.
>  
> -uploadpack.allowtipsha1inwant::
> +uploadpack.allowTipSHA1InWant::
>  	When `uploadpack.hideRefs` is in effect, allow `upload-pack`
>  	to accept a fetch request that asks for an object at the tip
>  	of a hidden ref (by default, such a request is rejected).
Eric Sunshine· May 5, 2015, 22:29 UTC · re: Fredrik Medley · lore

Re: [PATCH v2] upload-pack: Optionally allow fetching reachable sha1

On Tue, May 5, 2015 at 5:21 PM, Fredrik Medley <fredrik.medley@gmail.com> wrote:
Show 29 quoted lines
> 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 hstory to check the reachability.
> [...]
> Signed-off-by: Fredrik Medley <fredrik.medley@gmail.com>
> ---
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 8a5f236..01fabfb 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1120,6 +1120,61 @@ test_expect_success 'fetch exact SHA1' '
>         )
>  '
>
> +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^` &&
Use $(..) here and elsewhere rather than `...`.
    SHA1=$(git --git-dir=testrepo/.git rev-parse HEAD^) &&
Show 47 quoted lines
> +               mk_empty shallow &&
> +               (
> +                       cd shallow &&
> +                       test_must_fail git fetch --depth=1 ../testrepo/.git $SHA1 &&
> +                       git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
> +                       git fetch --depth=1 ../testrepo/.git $SHA1 &&
> +                       git cat-file commit $SHA1 >/dev/null
> +               )
> +       '
> +
> +       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
> +               )
> +               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 &&
> +                       git cat-file commit $SHA1_3 >/dev/null &&
> +                       git cat-file commit $SHA1_3 >/dev/null
> +               ) &&
> +               mk_empty shallow &&
> +               (
> +                       cd shallow &&
> +                       test_must_fail git fetch ../testrepo/.git $SHA1_3 &&
> +                       test_must_fail git fetch ../testrepo/.git $SHA1_1 &&
> +                       git --git-dir=../testrepo/.git config uploadpack.allowreachablesha1inwant true &&
> +                       git fetch ../testrepo/.git $SHA1_1 &&
> +                       git cat-file commit $SHA1_1 >/dev/null &&
> +                       test_must_fail git cat-file commit $SHA1_2 >/dev/null &&
> +                       git fetch ../testrepo/.git $SHA1_2 &&
> +                       git cat-file commit $SHA1_2 >/dev/null &&
> +                       test_must_fail git fetch ../testrepo/.git $SHA1_3
> +               )
> +       '
> +done
> +
>  test_expect_success 'fetch follows tags by default' '
>         mk_test testrepo heads/master &&
>         rm -fr src dst &&

← back to recent threads