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

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

[Fredrik Medley]

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

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

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

[Junio C Hamano]

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.

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

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

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

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

[Fredrik Medley]

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

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.

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

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

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

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

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

[Eric Sunshine]

On Sun, May 3, 2015 at 4:13 PM, Fredrik Medley <fredrik.medley@gmail.com> wrote:
> 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.

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

[Fredrik Medley]

With uploadpack.allowreachablesha1inwant configuration option set on the
server side, "git fetch" can make a request with a "want" line that names
an object that has not been advertised (likely to have been obtained out
of band or from a submodule pointer). Only objects reachable from the
branch tips, i.e. the union of advertised branches and branches hidden by
transfer.hiderefs, will be processed. Note that there is an associated
cost of having to walk back the 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(-)

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

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

[Junio C Hamano]

Fredrik Medley <fredrik.medley@gmail.com> writes:

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

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

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

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

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

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

[Fredrik Medley]

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

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

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

[Junio C Hamano]

Fredrik Medley <fredrik.medley@gmail.com> writes:

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

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

[Fredrik Medley]

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

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

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

[Fredrik Medley]

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

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

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

[Eric Sunshine]

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.

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

>                 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

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

[Junio C Hamano]

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?

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

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

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

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

[Fredrik Medley]

2015-05-12 23:39 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> 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.

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

[Fredrik Medley]

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

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

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

[Eric Sunshine]

On Tue, May 12, 2015 at 5:14 PM, Fredrik Medley
<fredrik.medley@gmail.com> wrote:
> 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/

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

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

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

[Fredrik Medley]

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

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

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

[Fredrik Medley]

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

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

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

[Junio C Hamano]

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?

> +	} else if (!strcmp("uploadpack.keepalive", var)) {
>  		keepalive = git_config_int(var, value);
>  		if (!keepalive)
>  			keepalive = -1;

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

[Fredrik Medley]

2015-05-20 0:00 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> 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?

>
>> +     } else if (!strcmp("uploadpack.keepalive", var)) {
>>               keepalive = git_config_int(var, value);
>>               if (!keepalive)
>>                       keepalive = -1;
>

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

[Fredrik Medley]

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

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

Signed-off-by: Fredrik Medley <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(-)

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

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

[Junio C Hamano]

Fredrik Medley <fredrik.medley@gmail.com> writes:

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

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

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

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

[Fredrik Medley]

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

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

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

[Fredrik Medley]

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

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

[Junio C Hamano]

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.

Thanks.

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

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

[Fredrik Medley]

2015-05-22 0:07 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> 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.

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

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

[Junio C Hamano]

Fredrik Medley <fredrik.medley@gmail.com> writes:

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

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

[Fredrik Medley]

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

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

Signed-off-by: Fredrik Medley <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(-)

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

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

[Junio C Hamano]

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.

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

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

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

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

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

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

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

Broken && chain

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

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

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

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

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

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

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

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

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

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

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

And of course, _2 can be fetched.

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

And _3 that is not reachable cannot.

> +		)
> +	'
> +done

Again, very carefully covering combinations.  Good.




 Documentation/technical/protocol-capabilities.txt |  2 +-
 t/t5516-fetch-push.sh                             | 14 +++++++-------
 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

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

[Fredrik Medley]

2015-05-22 0:15 GMT+02:00 Junio C Hamano <gitster@pobox.com>:
> 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?

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

Yes, that was intended.

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

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

[Junio C Hamano]

Fredrik Medley <fredrik.medley@gmail.com> writes:

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

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

[Eric Sunshine]

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.

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

>         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

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

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

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

[Junio C Hamano]

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?

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

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

[Eric Sunshine]

On Tue, May 5, 2015 at 5:21 PM, Fredrik Medley <fredrik.medley@gmail.com> wrote:
> 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^) &&

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