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  writes:
> 
> >>>   struct object_id oid;
> >>>
> >>> - if (!get_oid_hex(name, ) && name[GIT_SHA1_HEXSZ] == ' ')
> >>> - name += GIT_SHA1_HEXSZ + 1;
> >>> - else
> >>> + if (get_oid_hex(name, ))
> >>>   oidclr();
> >>> + 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() 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Gabriel Souza Franco  writes:

>>>   struct object_id oid;
>>>
>>> - if (!get_oid_hex(name, ) && name[GIT_SHA1_HEXSZ] == ' ')
>>> - name += GIT_SHA1_HEXSZ + 1;
>>> - else
>>> + if (get_oid_hex(name, ))
>>>   oidclr();
>>> + 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() 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Gabriel Souza Franco]

On Sat, Feb 27, 2016 at 7:12 PM, Jeff King  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 
>> ---
>>  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, ) && name[GIT_SHA1_HEXSZ] == ' ')
>> - name += GIT_SHA1_HEXSZ + 1;
>> - else
>> + if (get_oid_hex(name, ))
>>   oidclr();
>> + 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() and letting it
fail elsewhere enough?
Also, it already fails precisely because of that check I wanted to
remove earlier.

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

Re: [PATCH] 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 
> ---
>  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, ) && name[GIT_SHA1_HEXSZ] == ' ')
> - name += GIT_SHA1_HEXSZ + 1;
> - else
> + if (get_oid_hex(name, ))
>   oidclr();
> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html