[PATCH] fetch-pack: fix unadvertised requests validation

[PATCH] fetch-pack: fix unadvertised requests validation

[Gabriel Souza Franco]

Check was introduced in b791642 (filter_ref: avoid overwriting
ref->old_sha1 with garbage, 2015-03-19), but was always false because
ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.

Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
---
 fetch-pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 01e34b6..83b937b 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -569,11 +569,11 @@ static void filter_refs(struct fetch_pack_args *args,
 			if (ref->matched)
 				continue;
 			if (get_sha1_hex(ref->name, sha1) ||
-			    ref->name[40] != '\0' ||
-			    hashcmp(sha1, ref->old_oid.hash))
+			    ref->name[40] != '\0')
 				continue;
 
 			ref->matched = 1;
+			hashcpy(ref->old_oid.hash, sha1);
 			*newtail = copy_ref(ref);
 			newtail = &(*newtail)->next;
 		}
-- 
2.7.1

Re: [PATCH] fetch-pack: fix unadvertised requests validation

[Junio C Hamano]

Gabriel Souza Franco <gabrielfrancosouza@gmail.com> writes:

> Check was introduced in b791642 (filter_ref: avoid overwriting
> ref->old_sha1 with garbage, 2015-03-19), but was always false because
> ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.
>
> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
> ---

Peff, that commit points me at your direction.  And I can see the
original patch avoids overwriting old_sha1 by saving the result from
get_sha1_hex() in a temporary, it is true that old_sha1 is not
updated from the temporary.

The original code before b791642 wanted to say "if ref->name is not
40-hex, continue, and otherwise, do the ref->matched thing" and an
implementation of b791642 that is more faithful to the original
would indeed have been the result of applying this patch from
Gabriel, but I am scratching my head why we have hashcmp() there.

Was it to avoid adding the same thing twice to the resulting list,
or something?


>  fetch-pack.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 01e34b6..83b937b 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -569,11 +569,11 @@ static void filter_refs(struct fetch_pack_args *args,
>  			if (ref->matched)
>  				continue;
>  			if (get_sha1_hex(ref->name, sha1) ||
> -			    ref->name[40] != '\0' ||
> -			    hashcmp(sha1, ref->old_oid.hash))
> +			    ref->name[40] != '\0')
>  				continue;
>  
>  			ref->matched = 1;
> +			hashcpy(ref->old_oid.hash, sha1);
>  			*newtail = copy_ref(ref);
>  			newtail = &(*newtail)->next;
>  		}

Re: [PATCH] fetch-pack: fix unadvertised requests validation

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Gabriel Souza Franco <gabrielfrancosouza@gmail.com> writes:
>
>> Check was introduced in b791642 (filter_ref: avoid overwriting
>> ref->old_sha1 with garbage, 2015-03-19), but was always false because
>> ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.
>>
>> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
>> ---
>
> Peff, that commit points me at your direction.  And I can see the
> original patch avoids overwriting old_sha1 by saving the result from
> get_sha1_hex() in a temporary, it is true that old_sha1 is not
> updated from the temporary.
>
> The original code before b791642 wanted to say "if ref->name is not
> 40-hex, continue, and otherwise, do the ref->matched thing" and an
> implementation of b791642 that is more faithful to the original
> would indeed have been the result of applying this patch from
> Gabriel, but I am scratching my head why we have hashcmp() there.
>
> Was it to avoid adding the same thing twice to the resulting list,
> or something?

Nah, I think you just misspelt hashcpy() as hashcmp().  The original
wanted to get the binary representation of the hex in old_sha1 when
it continued, and you wanted to delay the touching of old_sha1 until
we make sure that the input is valid 40-hex, so something like this
is what Gabriel wants to do (which I agree with), isn't it?

 fetch-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 058c258..bb5237f 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -551,7 +551,7 @@ static void filter_refs(struct fetch_pack_args *args,
 				continue;
 			if (get_sha1_hex(ref->name, sha1) ||
 			    ref->name[40] != '\0' ||
-			    hashcmp(sha1, ref->old_sha1))
+			    hashcpy(ref->old_sha1, sha1))
 				continue;
 
 			ref->matched = 1;

Re: [PATCH] fetch-pack: fix unadvertised requests validation

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nah, I think you just misspelt hashcpy() as hashcmp().  The original
> wanted to get the binary representation of the hex in old_sha1 when
> it continued, and you wanted to delay the touching of old_sha1 until
> we make sure that the input is valid 40-hex.

That much was correct, but...

> , so something like this
> is what Gabriel wants to do (which I agree with), isn't it?

... definitely not that X-<.  The "do this or fail || do that or
fail || ..." chain in the if condition part confused me.

I think Gabriel's patch is exactly what we want.

Sorry for starting a review before caffeine ;-)

Re: [PATCH] fetch-pack: fix unadvertised requests validation

[Jeff King]

On Sat, Feb 27, 2016 at 10:25:40AM -0800, Junio C Hamano wrote:

> Gabriel Souza Franco <gabrielfrancosouza@gmail.com> writes:
> 
> > Check was introduced in b791642 (filter_ref: avoid overwriting
> > ref->old_sha1 with garbage, 2015-03-19), but was always false because
> > ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.
> >
> > Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
> > ---
> 
> Peff, that commit points me at your direction.  And I can see the
> original patch avoids overwriting old_sha1 by saving the result from
> get_sha1_hex() in a temporary, it is true that old_sha1 is not
> updated from the temporary.
> 
> The original code before b791642 wanted to say "if ref->name is not
> 40-hex, continue, and otherwise, do the ref->matched thing" and an
> implementation of b791642 that is more faithful to the original
> would indeed have been the result of applying this patch from
> Gabriel, but I am scratching my head why we have hashcmp() there.
> 
> Was it to avoid adding the same thing twice to the resulting list,
> or something?

It is a sanity check. The code is looking in our list of things to fetch
for items that are pure objects, not refs (we already matched the refs
by name, but obviously would not have matched any pure-sha1 requests to
refnames).  So the conditional really is:

   if (!is_a_pure_sha1(ref))
	continue;

We can implement that as:

  if (get_sha1_hex(ref->name, sha1) || ref->name[40] != '\0')

but as noted in the commit message for b791642:

  We could just check that we have exactly 40 characters of
  sha1. But let's be even more careful and make sure that we
  have a 40-char hex refname that matches what is in old_sha1.
  This is perhaps overly defensive, but spells out our
  assumptions clearly.

E.g., if you did this:

  git fetch-pack --stdin $remote <<\EOF
  1234567890123456789012345678901234567890 abcdef1234abcdef1234abcdef1234abcdef1234
  EOF

you'd have a "struct ref" with a 40-hex sha1, but which does _not_ want
the object of the same name. This is not a pure-object request, and we
should only request 1234... if the ref abcd... is present on the remote.

I doubt it would ever come up in real life; refs tend to start with
"refs/", and I suspect short of manual prodding as above, you could not
get anything without "refs/" to this point of the code.

So the patch:

> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index 01e34b6..83b937b 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -569,11 +569,11 @@ static void filter_refs(struct fetch_pack_args *args,
> >  			if (ref->matched)
> >  				continue;
> >  			if (get_sha1_hex(ref->name, sha1) ||
> > -			    ref->name[40] != '\0' ||
> > -			    hashcmp(sha1, ref->old_oid.hash))
> > +			    ref->name[40] != '\0')
> >  				continue;
> >  
> >  			ref->matched = 1;
> > +			hashcpy(ref->old_oid.hash, sha1);
> >  			*newtail = copy_ref(ref);
> >  			newtail = &(*newtail)->next;
> >  		}

is a wrong direction, I think. It removes the extra safety check that
skips the ref above. But worse, in the example above, it overwrites the
real object "1234..." with the name of the ref "abcd..." in the sha1
field. We'll ask for an object that may not even exist.

The commit message for Gabriel's patch says:

> > Check was introduced in b791642 (filter_ref: avoid overwriting
> > ref->old_sha1 with garbage, 2015-03-19), but was always false because
> > ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.

but I don't think ref->old_oid.hash _is_ empty. At least, that was not
the conclusion from our discussion in:

   http://thread.gmane.org/gmane.comp.version-control.git/265480

We expect whoever creates the "sought" list to fill in the name and sha1
as appropriate. If that is not happening in some code path, then yeah,
filter_refs() will not work as intended. But I think the solution there
would be to fix the caller to set up the "struct ref" more completely.

Gabriel, did this come from a bug you noticed in practice, or was it
just an intended cleanup?

-Peff

Re: [PATCH] fetch-pack: fix unadvertised requests validation

[Jeff King]

On Sat, Feb 27, 2016 at 02:07:12PM -0500, Jeff King wrote:

> We expect whoever creates the "sought" list to fill in the name and sha1
> as appropriate. If that is not happening in some code path, then yeah,
> filter_refs() will not work as intended. But I think the solution there
> would be to fix the caller to set up the "struct ref" more completely.
> 
> Gabriel, did this come from a bug you noticed in practice, or was it
> just an intended cleanup?

I double-checked that the code for git-fetch does so. It's in
get_fetch_map()

    if (refspec->exact_sha1) {
	    ref_map = alloc_ref(name);
	    get_oid_hex(name, &ref_map->old_oid);
    } else ...

So we should always have old_oid filled in already, and there is no need
to do so in filter_refs() (and in fact it is wrong, for the degenerate
example I gave earlier).

-Peff

Re: [PATCH] fetch-pack: fix unadvertised requests validation

[Gabriel Souza Franco]

On Sat, Feb 27, 2016 at 4:19 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 27, 2016 at 02:07:12PM -0500, Jeff King wrote:
>
>> We expect whoever creates the "sought" list to fill in the name and sha1
>> as appropriate. If that is not happening in some code path, then yeah,
>> filter_refs() will not work as intended. But I think the solution there
>> would be to fix the caller to set up the "struct ref" more completely.
>>
>> Gabriel, did this come from a bug you noticed in practice, or was it
>> just an intended cleanup?

I was experimenting with uploadpack.hiderefs and uploadpack.allowTipSHA1InWant
and couldn't get

        git fetch-pack $remote <sha1>

to work, and I traced the failure until that check. Reading more, I now see
that currently it requires

       git fetch-pack $remote "<sha1> <sha1>"

to do what I want.

>
> I double-checked that the code for git-fetch does so. It's in
> get_fetch_map()
>
>     if (refspec->exact_sha1) {
>             ref_map = alloc_ref(name);
>             get_oid_hex(name, &ref_map->old_oid);
>     } else ...
>
> So we should always have old_oid filled in already, and there is no need
> to do so in filter_refs() (and in fact it is wrong, for the degenerate
> example I gave earlier).

git fetch-pack doesn't use these code paths. I'll send a new patch
shortly to allow
bare sha1's in fetch-pack.

>
> -Peff

[PATCH] fetch-pack: fix object_id of exact sha1

[Gabriel Souza Franco]

Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
2013-12-05) added support for specifying a SHA-1 as well as a ref name.
Add support for specifying just a SHA-1 and set the ref name to the same
value in this case.

Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
---
 builtin/fetch-pack.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 79a611f..d7e439a 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -16,10 +16,10 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
 	struct ref *ref;
 	struct object_id oid;
 
-	if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')
-		name += GIT_SHA1_HEXSZ + 1;
-	else
+	if (get_oid_hex(name, &oid))
 		oidclr(&oid);
+	else if (name[GIT_SHA1_HEXSZ] == ' ')
+		name += GIT_SHA1_HEXSZ + 1;
 
 	ref = alloc_ref(name);
 	oidcpy(&ref->old_oid, &oid);
-- 
2.7.1

Re: [PATCH] fetch-pack: fix object_id of exact sha1

[Jeff King]

On Sat, Feb 27, 2016 at 05:32:54PM -0300, Gabriel Souza Franco wrote:

> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.
> Add support for specifying just a SHA-1 and set the ref name to the same
> value in this case.
> 
> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
> ---
>  builtin/fetch-pack.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 79a611f..d7e439a 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -16,10 +16,10 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
>  	struct ref *ref;
>  	struct object_id oid;
>  
> -	if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')
> -		name += GIT_SHA1_HEXSZ + 1;
> -	else
> +	if (get_oid_hex(name, &oid))
>  		oidclr(&oid);
> +	else if (name[GIT_SHA1_HEXSZ] == ' ')
> +		name += GIT_SHA1_HEXSZ + 1;

This makes sense to me. I wonder if we should be more particular about
the pure-sha1 case consuming the whole string, though. E.g., if we get:

  1234567890123456789012345678901234567890-bananas

that should probably not have sha1 1234...

I don't think it should ever really happen in practice, but it might be
worth noticing and complaining when name[GIT_SHA1_HEXSZ] is neither
space nor '\0'.

-Peff

Re: [PATCH] fetch-pack: fix object_id of exact sha1

[Gabriel Souza Franco]

On Sat, Feb 27, 2016 at 7:12 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 27, 2016 at 05:32:54PM -0300, Gabriel Souza Franco wrote:
>
>> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
>> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.
>> Add support for specifying just a SHA-1 and set the ref name to the same
>> value in this case.
>>
>> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
>> ---
>>  builtin/fetch-pack.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
>> index 79a611f..d7e439a 100644
>> --- a/builtin/fetch-pack.c
>> +++ b/builtin/fetch-pack.c
>> @@ -16,10 +16,10 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
>>       struct ref *ref;
>>       struct object_id oid;
>>
>> -     if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')
>> -             name += GIT_SHA1_HEXSZ + 1;
>> -     else
>> +     if (get_oid_hex(name, &oid))
>>               oidclr(&oid);
>> +     else if (name[GIT_SHA1_HEXSZ] == ' ')
>> +             name += GIT_SHA1_HEXSZ + 1;
>
> This makes sense to me. I wonder if we should be more particular about
> the pure-sha1 case consuming the whole string, though. E.g., if we get:
>
>   1234567890123456789012345678901234567890-bananas
>
> that should probably not have sha1 1234...
>
> I don't think it should ever really happen in practice, but it might be
> worth noticing and complaining when name[GIT_SHA1_HEXSZ] is neither
> space nor '\0'.

Right. What kind of complaining? Is doing oidclr(&oid) and letting it
fail elsewhere enough?
Also, it already fails precisely because of that check I wanted to
remove earlier.

>
> -Peff

Re: [PATCH] fetch-pack: fix object_id of exact sha1

[Junio C Hamano]

Gabriel Souza Franco <gabrielfrancosouza@gmail.com> writes:

>>>       struct object_id oid;
>>>
>>> -     if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')
>>> -             name += GIT_SHA1_HEXSZ + 1;
>>> -     else
>>> +     if (get_oid_hex(name, &oid))
>>>               oidclr(&oid);
>>> +     else if (name[GIT_SHA1_HEXSZ] == ' ')
>>> +             name += GIT_SHA1_HEXSZ + 1;
>>
>> This makes sense to me. I wonder if we should be more particular about
>> the pure-sha1 case consuming the whole string, though. E.g., if we get:
>>
>>   1234567890123456789012345678901234567890-bananas
>>
>> that should probably not have sha1 1234...
>>
>> I don't think it should ever really happen in practice, but it might be
>> worth noticing and complaining when name[GIT_SHA1_HEXSZ] is neither
>> space nor '\0'.
>
> Right. What kind of complaining? Is doing oidclr(&oid) and letting it
> fail elsewhere enough?

I think that is the most sensible.  After all, the first get-oid-hex
expects to find a valid 40-hex object name at the beginning of line,
and oidclr() is the way for it to signal a broken input, which is
how the callers of this codepath expects errors to be caught.

Thanks.

[PATCH v2] fetch-pack: fix object_id of exact sha1

[Gabriel Souza Franco]

Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
2013-12-05) added support for specifying a SHA-1 as well as a ref name.
Add support for specifying just a SHA-1 and set the ref name to the same
value in this case.

Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
---

Not the cleanest conditional I've ever written, but it should handle
all cases correctly.

 builtin/fetch-pack.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 79a611f..763f510 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -16,10 +16,12 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
 	struct ref *ref;
 	struct object_id oid;
 
-	if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')
-		name += GIT_SHA1_HEXSZ + 1;
-	else
+	if (get_oid_hex(name, &oid) ||
+			(name[GIT_SHA1_HEXSZ] != ' ' &&
+			 name[GIT_SHA1_HEXSZ] != '\0'))
 		oidclr(&oid);
+	else if (name[GIT_SHA1_HEXSZ] == ' ')
+		name += GIT_SHA1_HEXSZ + 1;
 
 	ref = alloc_ref(name);
 	oidcpy(&ref->old_oid, &oid);
-- 
2.7.2

Re: [PATCH v2] fetch-pack: fix object_id of exact sha1

[Johannes Schindelin]

Hi Gabriel,

On Sun, 28 Feb 2016, Gabriel Souza Franco wrote:

> Not the cleanest conditional I've ever written, but it should handle
> all cases correctly.

It could be much worse:

> +	if (get_oid_hex(name, &oid) ||
> +			(name[GIT_SHA1_HEXSZ] != ' ' &&
> +			 name[GIT_SHA1_HEXSZ] != '\0'))

I know developers who would write this as

	if (get_oid_hex(name, &oid) || (name[GIT_SHA1_HEXSZ] & ' '))

and not even begin to realize that this is a problem.

So I'd say your conditional is good.

Having said that, this *might* be a good opportunity to imitate the
skip_prefix() function. If there are enough similar code constructs, we
could simplify all of them by introducing the function

	skip_oid_hex(const char *str, struct object_id *oid, const char **out)

that returns 1 if and only if an oid was parsed, and stores the pointer
after the oid in "out" (skipping an additional space if there is one)?

Ciao,
Dscho

Re: [PATCH v2] fetch-pack: fix object_id of exact sha1

[Jeff King]

On Sun, Feb 28, 2016 at 07:22:24PM -0300, Gabriel Souza Franco wrote:

> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.
> Add support for specifying just a SHA-1 and set the ref name to the same
> value in this case.
> 
> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
> ---
> 
> Not the cleanest conditional I've ever written, but it should handle
> all cases correctly.

I think it does. But I wonder if it wouldn't be more readable to cover
the three formats independently, like:

  if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == ' ') {
	/* <sha1> <ref>, find refname */
	name += GIT_SHA1_HEXSZ + 1;
  } else if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == '\0') {
	/* <sha1>, leave sha1 as name */
  } else {
	/* <ref>, clear any cruft from get_oid_hex */
	oidclr(&ref->old_oid);
  }

And as a bonus you get rid of the separate "oid".  That does call into
get_oid_hex twice, but I doubt the performance impact is measurable.

We could also do:

  if (!get_oid_hex(name, &ref->old_oid)) {
	if (name[GIT_SHA1_HEXSZ] == ' ') {
		/* <sha1> <ref>, find refname */
		name += GIT_SHA1_HEXSZ + 1;
	} else if (name[GIT_SHA1_HEXSZ] == '\0') {
		/* <sha1>, leave sha1 as name */
	} else {
		/* <ref>, clear cruft from oid */
		oidclr(&ref->old_oid);
	}
  } else {
	/* <ref>, clear cruft from get_oid_hex */
	oidclr(&ref->old_oid);
  }

if you want to minimize the calls at the expense of having to repeat the
oidclr().

-Peff

Re: [PATCH v2] fetch-pack: fix object_id of exact sha1

[Gabriel Souza Franco]

On Mon, Feb 29, 2016 at 5:30 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Having said that, this *might* be a good opportunity to imitate the
> skip_prefix() function. If there are enough similar code constructs, we
> could simplify all of them by introducing the function
>
>         skip_oid_hex(const char *str, struct object_id *oid, const char **out)
>
> that returns 1 if and only if an oid was parsed, and stores the pointer
> after the oid in "out" (skipping an additional space if there is one)?

I don't think there's any other place that accepts all of "<sha1>",
"<sha1> <ref>" and "<ref>"
based on a quick grep for get_oid_hex.

On Mon, Feb 29, 2016 at 7:00 AM, Jeff King <peff@peff.net> wrote:
> On Sun, Feb 28, 2016 at 07:22:24PM -0300, Gabriel Souza Franco wrote:
>
>> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
>> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.
>> Add support for specifying just a SHA-1 and set the ref name to the same
>> value in this case.
>>
>> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
>> ---
>>
>> Not the cleanest conditional I've ever written, but it should handle
>> all cases correctly.
>
> I think it does. But I wonder if it wouldn't be more readable to cover
> the three formats independently, like:
>
>   if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == ' ') {
>         /* <sha1> <ref>, find refname */
>         name += GIT_SHA1_HEXSZ + 1;
>   } else if (!get_oid_hex(name, &ref->old_oid) && name[GIT_SHA1_HEXSZ] == '\0') {
>         /* <sha1>, leave sha1 as name */
>   } else {
>         /* <ref>, clear any cruft from get_oid_hex */
>         oidclr(&ref->old_oid);
>   }
>
> And as a bonus you get rid of the separate "oid".  That does call into
> get_oid_hex twice, but I doubt the performance impact is measurable.
>
> We could also do:
>
>   if (!get_oid_hex(name, &ref->old_oid)) {
>         if (name[GIT_SHA1_HEXSZ] == ' ') {
>                 /* <sha1> <ref>, find refname */
>                 name += GIT_SHA1_HEXSZ + 1;
>         } else if (name[GIT_SHA1_HEXSZ] == '\0') {
>                 /* <sha1>, leave sha1 as name */
>         } else {
>                 /* <ref>, clear cruft from oid */
>                 oidclr(&ref->old_oid);
>         }
>   } else {
>         /* <ref>, clear cruft from get_oid_hex */
>         oidclr(&ref->old_oid);
>   }
>
> if you want to minimize the calls at the expense of having to repeat the
> oidclr().

I think I like this version more, and is close to what I had initially
before I tried to be clever about it.
Besides, this isn't a performance critical function, so it shouldn't
matter much.
Will send a new (and hopefully final) version shortly.

>
> -Peff

[PATCH v3] fetch-pack: fix object_id of exact sha1

[Gabriel Souza Franco]

Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
2013-12-05) added support for specifying a SHA-1 as well as a ref name.
Add support for specifying just a SHA-1 and set the ref name to the same
value in this case.

Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
---

I did keep the oid variable because ref is uninitialized at that point,
and this means having to copy either name or old_oid afterwards anyway.

 builtin/fetch-pack.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 79a611f..50c9901 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -16,10 +16,20 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
 	struct ref *ref;
 	struct object_id oid;
 
-	if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')
-		name += GIT_SHA1_HEXSZ + 1;
-	else
+	if (!get_oid_hex(name, &oid)) {
+		if (name[GIT_SHA1_HEXSZ] == ' ') {
+			/* <sha1> <ref>, find refname */
+			name += GIT_SHA1_HEXSZ + 1;
+		} else if (name[GIT_SHA1_HEXSZ] == '\0') {
+			/* <sha1>, leave sha1 as name */
+		} else {
+			/* <ref>, clear cruft from oid */
+			oidclr(&oid);
+		}
+	} else {
+		/* <ref>, clear cruft from get_oid_hex */
 		oidclr(&oid);
+	}
 
 	ref = alloc_ref(name);
 	oidcpy(&ref->old_oid, &oid);
-- 
2.7.2

Re: [PATCH v3] fetch-pack: fix object_id of exact sha1

[Jeff King]

On Mon, Feb 29, 2016 at 11:12:56PM -0300, Gabriel Souza Franco wrote:

> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.
> Add support for specifying just a SHA-1 and set the ref name to the same
> value in this case.
> 
> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
> ---
> 
> I did keep the oid variable because ref is uninitialized at that point,
> and this means having to copy either name or old_oid afterwards anyway.

Oh, right. That's why we had the variable in the first place (even in
the original, we could otherwise have gone without the extra variable).

>  builtin/fetch-pack.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)

The code looks good to me. Do we need documentation or test updates?

Here's a test that can be squashed in. For documentation, it looks like
we don't cover the "<sha1> <ref>" form at all. That's maybe OK, as it's
mostly for internal use by remote-http (though fetch-pack _is_ plumbing,
so perhaps some other remote-* could make use of it). But perhaps we
should document that "<sha1>" should work.

diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index e5f83bf..9b9bec4 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -531,6 +531,20 @@ test_expect_success 'shallow fetch with tags does not break the repository' '
 		git fsck
 	)
 '
+
+test_expect_success 'fetch-pack can fetch a raw sha1' '
+	git init hidden &&
+	(
+		cd hidden &&
+		test_commit 1 &&
+		test_commit 2 &&
+		git update-ref refs/hidden/one HEAD^ &&
+		git config transfer.hiderefs refs/hidden &&
+		git config uploadpack.allowtipsha1inwant true
+	) &&
+	git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)
+'
+
 check_prot_path () {
 	cat >expected <<-EOF &&
 	Diag: url=$1

Re: [PATCH v3] fetch-pack: fix object_id of exact sha1

[Gabriel Souza Franco]

On Tue, Mar 1, 2016 at 1:54 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Feb 29, 2016 at 11:12:56PM -0300, Gabriel Souza Franco wrote:
>
>> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
>> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.
>> Add support for specifying just a SHA-1 and set the ref name to the same
>> value in this case.
>>
>> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
>> ---
>>
>> I did keep the oid variable because ref is uninitialized at that point,
>> and this means having to copy either name or old_oid afterwards anyway.
>
> Oh, right. That's why we had the variable in the first place (even in
> the original, we could otherwise have gone without the extra variable).
>
>>  builtin/fetch-pack.c | 16 +++++++++++++---
>>  1 file changed, 13 insertions(+), 3 deletions(-)
>
> The code looks good to me. Do we need documentation or test updates?
>
> Here's a test that can be squashed in. For documentation, it looks like
> we don't cover the "<sha1> <ref>" form at all. That's maybe OK, as it's
> mostly for internal use by remote-http (though fetch-pack _is_ plumbing,
> so perhaps some other remote-* could make use of it). But perhaps we
> should document that "<sha1>" should work.

Thanks for providing a test, I hadn't looked up those yet. For
documentation, should
it be on the same patch or a new one? Also, I'm not exactly sure how
to word that <refs>...
can also contain a hash instead of a ref.

>
> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
> index e5f83bf..9b9bec4 100755
> --- a/t/t5500-fetch-pack.sh
> +++ b/t/t5500-fetch-pack.sh
> @@ -531,6 +531,20 @@ test_expect_success 'shallow fetch with tags does not break the repository' '
>                 git fsck
>         )
>  '
> +
> +test_expect_success 'fetch-pack can fetch a raw sha1' '
> +       git init hidden &&
> +       (
> +               cd hidden &&
> +               test_commit 1 &&
> +               test_commit 2 &&
> +               git update-ref refs/hidden/one HEAD^ &&
> +               git config transfer.hiderefs refs/hidden &&
> +               git config uploadpack.allowtipsha1inwant true
> +       ) &&
> +       git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)
> +'
> +
>  check_prot_path () {
>         cat >expected <<-EOF &&
>         Diag: url=$1

Re: [PATCH v3] fetch-pack: fix object_id of exact sha1

[Jeff King]

On Thu, Mar 03, 2016 at 08:35:54PM -0300, Gabriel Souza Franco wrote:

> > The code looks good to me. Do we need documentation or test updates?
> >
> > Here's a test that can be squashed in. For documentation, it looks like
> > we don't cover the "<sha1> <ref>" form at all. That's maybe OK, as it's
> > mostly for internal use by remote-http (though fetch-pack _is_ plumbing,
> > so perhaps some other remote-* could make use of it). But perhaps we
> > should document that "<sha1>" should work.
> 
> Thanks for providing a test, I hadn't looked up those yet. For
> documentation, should
> it be on the same patch or a new one? Also, I'm not exactly sure how
> to word that <refs>...
> can also contain a hash instead of a ref.

I think it make sense as part of the same patch. I guess you could still
call the argument "<refs>" even though it takes more now, and just
explain the new feature in the appropriate section. I can't think of a
better word to use (somehow "<objects>" feels too broad, and the primary
use would still be a list of refs).

-Peff

[PATCH v4] fetch-pack: fix object_id of exact sha1

[Gabriel Souza Franco]

Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
2013-12-05) added support for specifying a SHA-1 as well as a ref name.
Add support for specifying just a SHA-1 and set the ref name to the same
value in this case.

Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
---
 Documentation/git-fetch-pack.txt |  4 ++++
 builtin/fetch-pack.c             | 16 +++++++++++++---
 t/t5500-fetch-pack.sh            | 14 ++++++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 8680f45..239623c 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -104,6 +104,10 @@ be in a separate packet, and the list must end with a flush packet.
 	The remote heads to update from. This is relative to
 	$GIT_DIR (e.g. "HEAD", "refs/heads/master").  When
 	unspecified, update from all heads the remote side has.
++
+If the remote has enabled the options `uploadpack.allowTipSHA1InWant` or
+`uploadpack.allowReachableSHA1InWant`, they may alternatively be 40-hex
+sha1s present on the remote.
 
 SEE ALSO
 --------
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 79a611f..50c9901 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -16,10 +16,20 @@ static void add_sought_entry(struct ref ***sought, int *nr, int *alloc,
 	struct ref *ref;
 	struct object_id oid;
 
-	if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')
-		name += GIT_SHA1_HEXSZ + 1;
-	else
+	if (!get_oid_hex(name, &oid)) {
+		if (name[GIT_SHA1_HEXSZ] == ' ') {
+			/* <sha1> <ref>, find refname */
+			name += GIT_SHA1_HEXSZ + 1;
+		} else if (name[GIT_SHA1_HEXSZ] == '\0') {
+			/* <sha1>, leave sha1 as name */
+		} else {
+			/* <ref>, clear cruft from oid */
+			oidclr(&oid);
+		}
+	} else {
+		/* <ref>, clear cruft from get_oid_hex */
 		oidclr(&oid);
+	}
 
 	ref = alloc_ref(name);
 	oidcpy(&ref->old_oid, &oid);
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index e5f83bf..9b9bec4 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -531,6 +531,20 @@ test_expect_success 'shallow fetch with tags does not break the repository' '
 		git fsck
 	)
 '
+
+test_expect_success 'fetch-pack can fetch a raw sha1' '
+	git init hidden &&
+	(
+		cd hidden &&
+		test_commit 1 &&
+		test_commit 2 &&
+		git update-ref refs/hidden/one HEAD^ &&
+		git config transfer.hiderefs refs/hidden &&
+		git config uploadpack.allowtipsha1inwant true
+	) &&
+	git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)
+'
+
 check_prot_path () {
 	cat >expected <<-EOF &&
 	Diag: url=$1
-- 
2.7.2

Re: [PATCH v4] fetch-pack: fix object_id of exact sha1

[Jeff King]

On Fri, Mar 04, 2016 at 10:11:38PM -0300, Gabriel Souza Franco wrote:

> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.
> Add support for specifying just a SHA-1 and set the ref name to the same
> value in this case.
> 
> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
> ---
>  Documentation/git-fetch-pack.txt |  4 ++++
>  builtin/fetch-pack.c             | 16 +++++++++++++---
>  t/t5500-fetch-pack.sh            | 14 ++++++++++++++
>  3 files changed, 31 insertions(+), 3 deletions(-)

Thanks, this version looks good to me.

-Peff

Re: [PATCH v4] fetch-pack: fix object_id of exact sha1

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Mar 04, 2016 at 10:11:38PM -0300, Gabriel Souza Franco wrote:
>
>> Commit 58f2ed0 (remote-curl: pass ref SHA-1 to fetch-pack as well,
>> 2013-12-05) added support for specifying a SHA-1 as well as a ref name.
>> Add support for specifying just a SHA-1 and set the ref name to the same
>> value in this case.
>> 
>> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
>> ---
>>  Documentation/git-fetch-pack.txt |  4 ++++
>>  builtin/fetch-pack.c             | 16 +++++++++++++---
>>  t/t5500-fetch-pack.sh            | 14 ++++++++++++++
>>  3 files changed, 31 insertions(+), 3 deletions(-)
>
> Thanks, this version looks good to me.

It does to me too, but unfortunately the previous one is already in
'next'.  Something like this as an incremental update would suffice.

-- >8 --
From: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
Date: Fri,  4 Mar 2016 22:11:38 -0300
Subject: fetch-pack: update the documentation for "<refs>..." arguments

When we started allowing an exact object name to be fetched from the
command line, we forgot to update the documentation. 

Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
--
 Documentation/git-fetch-pack.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 8680f45..239623c 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -104,6 +104,10 @@ be in a separate packet, and the list must end with a flush packet.
 	The remote heads to update from. This is relative to
 	$GIT_DIR (e.g. "HEAD", "refs/heads/master").  When
 	unspecified, update from all heads the remote side has.
++
+If the remote has enabled the options `uploadpack.allowTipSHA1InWant` or
+`uploadpack.allowReachableSHA1InWant`, they may alternatively be 40-hex
+sha1s present on the remote.
 
 SEE ALSO
 --------

[PATCH] fetch-pack: update the documentation for "<refs>..." arguments

[Gabriel Souza Franco]

When we started allowing an exact object name to be fetched from the
command line, we forgot to update the documentation.

Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
---
 Documentation/git-fetch-pack.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index 8680f45..239623c 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -104,6 +104,10 @@ be in a separate packet, and the list must end with a flush packet.
 	The remote heads to update from. This is relative to
 	$GIT_DIR (e.g. "HEAD", "refs/heads/master").  When
 	unspecified, update from all heads the remote side has.
++
+If the remote has enabled the options `uploadpack.allowTipSHA1InWant` or
+`uploadpack.allowReachableSHA1InWant`, they may alternatively be 40-hex
+sha1s present on the remote.
 
 SEE ALSO
 --------
-- 
2.7.2

Re: [PATCH] fetch-pack: update the documentation for "<refs>..." arguments

[Junio C Hamano]

Gabriel Souza Franco <gabrielfrancosouza@gmail.com> writes:

> When we started allowing an exact object name to be fetched from the
> command line, we forgot to update the documentation.
>
> Signed-off-by: Gabriel Souza Franco <gabrielfrancosouza@gmail.com>
> ---

Thanks for resending ;-)

Re: [PATCH v2] fetch-pack: fix object_id of exact sha1

[Jeff King]

On Mon, Feb 29, 2016 at 11:08:07PM -0300, Gabriel Souza Franco wrote:

> On Mon, Feb 29, 2016 at 5:30 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Having said that, this *might* be a good opportunity to imitate the
> > skip_prefix() function. If there are enough similar code constructs, we
> > could simplify all of them by introducing the function
> >
> >         skip_oid_hex(const char *str, struct object_id *oid, const char **out)
> >
> > that returns 1 if and only if an oid was parsed, and stores the pointer
> > after the oid in "out" (skipping an additional space if there is one)?
> 
> I don't think there's any other place that accepts all of "<sha1>",
> "<sha1> <ref>" and "<ref>"
> based on a quick grep for get_oid_hex.

Yes, but there are places where we get_oid_hex and then skip past that,
which could use the skip_oid_hex function, like:

diff --git a/connect.c b/connect.c
index 0478631..ba22ee6 100644
--- a/connect.c
+++ b/connect.c
@@ -149,10 +149,9 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
 			continue;
 		}
 
-		if (len < GIT_SHA1_HEXSZ + 2 || get_oid_hex(buffer, &old_oid) ||
-			buffer[GIT_SHA1_HEXSZ] != ' ')
+		if (!skip_oid_hex(buffer, &old_oid, &name) ||
+		    !skip_prefix(name, " ", &name))
 			die("protocol error: expected sha/ref, got '%s'", buffer);
-		name = buffer + GIT_SHA1_HEXSZ + 1;
 
 		name_len = strlen(name);
 		if (len != name_len + GIT_SHA1_HEXSZ + 1) {

_But_, if you look at the context just below, we make another implicit
assumption about GIT_SHA1_HEXSZ. So it's really not buying us that much
(unless we switch around the whole function to keep reading to the final
pointer, and compare "end - start" to the original "len" here).

So I'm not sure it's worth the trouble. I am really happy with the
skip_prefix() function for parsing like this, but I think it's just not
nearly as big a deal with oid-parsing, because we already have a nice
constant of GIT_SHA1_HEXSZ to match it (whereas skipping "foo" requires
us writing the magical "3" somewhere).

Anyway. Whether we want to pursue that or not, I don't think it needs to
be part of your series. Let's focus on the original goal.

-Peff

Re: [PATCH] fetch-pack: fix object_id of exact sha1

[Jeff King]

On Sun, Feb 28, 2016 at 11:29:39AM -0800, Junio C Hamano wrote:

> Gabriel Souza Franco <gabrielfrancosouza@gmail.com> writes:
> 
> >>>       struct object_id oid;
> >>>
> >>> -     if (!get_oid_hex(name, &oid) && name[GIT_SHA1_HEXSZ] == ' ')
> >>> -             name += GIT_SHA1_HEXSZ + 1;
> >>> -     else
> >>> +     if (get_oid_hex(name, &oid))
> >>>               oidclr(&oid);
> >>> +     else if (name[GIT_SHA1_HEXSZ] == ' ')
> >>> +             name += GIT_SHA1_HEXSZ + 1;
> >>
> >> This makes sense to me. I wonder if we should be more particular about
> >> the pure-sha1 case consuming the whole string, though. E.g., if we get:
> >>
> >>   1234567890123456789012345678901234567890-bananas
> >>
> >> that should probably not have sha1 1234...
> >>
> >> I don't think it should ever really happen in practice, but it might be
> >> worth noticing and complaining when name[GIT_SHA1_HEXSZ] is neither
> >> space nor '\0'.
> >
> > Right. What kind of complaining? Is doing oidclr(&oid) and letting it
> > fail elsewhere enough?
> 
> I think that is the most sensible.  After all, the first get-oid-hex
> expects to find a valid 40-hex object name at the beginning of line,
> and oidclr() is the way for it to signal a broken input, which is
> how the callers of this codepath expects errors to be caught.

Actually, I think we _don't_ want to signal an error here, but checking
for '\0' is still the right thing to do.

Once upon a time, fetch-pack took just the names of refs, like:

  git fetch-pack $remote refs/heads/foo

and the same format was used for --stdin. Then in 58f2ed0 (remote-curl:
pass ref SHA-1 to fetch-pack as well, 2013-12-05), it learned to take
"$sha1 $ref". But if we didn't see a sha1, then we continued to treat it
as a refname.

This patch adds a new format, just "$sha1". So if get_oid_hex() succeeds
_and_ we see '\0', we know we have that case. But if we don't see '\0',
then we should assume it's a refname (e.g., "1234abcd...-bananas").

I think in practice it shouldn't matter much, as callers should be
feeding fully qualified refs (and we document this). However, we still
want to distinguish so that we give the correct error ("no such remote
ref 1234abcd...-bananas", not "whoops, the other side doesn't have
1234abcd").

-Peff

Re: [PATCH] fetch-pack: fix unadvertised requests validation

[Jeff King]

On Sat, Feb 27, 2016 at 05:28:55PM -0300, Gabriel Souza Franco wrote:

> On Sat, Feb 27, 2016 at 4:19 PM, Jeff King <peff@peff.net> wrote:
> > On Sat, Feb 27, 2016 at 02:07:12PM -0500, Jeff King wrote:
> >
> >> We expect whoever creates the "sought" list to fill in the name and sha1
> >> as appropriate. If that is not happening in some code path, then yeah,
> >> filter_refs() will not work as intended. But I think the solution there
> >> would be to fix the caller to set up the "struct ref" more completely.
> >>
> >> Gabriel, did this come from a bug you noticed in practice, or was it
> >> just an intended cleanup?
> 
> I was experimenting with uploadpack.hiderefs and uploadpack.allowTipSHA1InWant
> and couldn't get
> 
>         git fetch-pack $remote <sha1>
> 
> to work, and I traced the failure until that check. Reading more, I now see
> that currently it requires
> 
>        git fetch-pack $remote "<sha1> <sha1>"
> 
> to do what I want.

Ah, that makes sense. I do think the "<sha1> <sha1>" syntax is a bit
weird, and I think mostly because the pure-object fetch came much later
in git's life; at this point hardly anybody uses a manual fetch-pack.

It would probably make sense to "<sha1>" to set up the ref correctly.

> > I double-checked that the code for git-fetch does so. It's in
> > get_fetch_map()
> [...]
> 
> git fetch-pack doesn't use these code paths. I'll send a new patch
> shortly to allow
> bare sha1's in fetch-pack.

Right. Sounds like a good plan.

-Peff

Re: [PATCH] fetch-pack: fix unadvertised requests validation

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So the patch:
>
>> > diff --git a/fetch-pack.c b/fetch-pack.c
>>...
> is a wrong direction, I think. It removes the extra safety check that
> skips the ref above. But worse, in the example above, it overwrites the
> real object "1234..." with the name of the ref "abcd..." in the sha1
> field. We'll ask for an object that may not even exist.
>
> The commit message for Gabriel's patch says:
>
>> > Check was introduced in b791642 (filter_ref: avoid overwriting
>> > ref->old_sha1 with garbage, 2015-03-19), but was always false because
>> > ref->old_oid.hash is empty in this case. Instead copy sha1 from ref->name.
>
> but I don't think ref->old_oid.hash _is_ empty. At least, that was not
> the conclusion from our discussion in:
>
>    http://thread.gmane.org/gmane.comp.version-control.git/265480
>
> We expect whoever creates the "sought" list to fill in the name and sha1
> as appropriate. If that is not happening in some code path, then yeah,
> filter_refs() will not work as intended. But I think the solution there
> would be to fix the caller to set up the "struct ref" more completely.

Ah, I forgot that thread completely.