Re: [PATCH] rev-parse: Identify short sha1 sums correctly.

Re: [PATCH] rev-parse: Identify short sha1 sums correctly.

[Junio C Hamano]

James Bowes <jbowes@dangerouslyinc.com> writes:

> find_short_packed_object was not loading the pack index files.
> Teach it to do so.
>
> Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>

I think this is the proper fix of the problem I was unhappy
about with 'next', rather than reverting the lazy index
loading.  But I wonder how many _other_ places like this there
are that we might be missing...

Shawn, an Ack, and any ideas for futureproofing?

>  sha1_name.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/sha1_name.c b/sha1_name.c
> index 8dfceb2..7df01af 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -76,8 +76,11 @@ static int find_short_packed_object(int len, const unsigned char *match, unsigne
>  
>  	prepare_packed_git();
>  	for (p = packed_git; p && found < 2; p = p->next) {
> -		uint32_t num = p->num_objects;
> -		uint32_t first = 0, last = num;
> +		uint32_t num, last;
> +		uint32_t first = 0;
> +		open_pack_index(p);
> +		num = p->num_objects;
> +		last = num;
>  		while (first < last) {
>  			uint32_t mid = (first + last) / 2;
>  			const unsigned char *now;
> -- 
> 1.5.2.869.g6b3ba

Re: [PATCH] rev-parse: Identify short sha1 sums correctly.

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> James Bowes <jbowes@dangerouslyinc.com> writes:
> 
> > find_short_packed_object was not loading the pack index files.
> > Teach it to do so.
> >
> > Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
> 
> I think this is the proper fix of the problem I was unhappy
> about with 'next', rather than reverting the lazy index
> loading.  But I wonder how many _other_ places like this there
> are that we might be missing...
> 
> Shawn, an Ack, and any ideas for futureproofing?

Ack, though late.  ;-)

I actually found this exact same bug today.  At first I thought it
was related to alternates, but when I dug into the code I came up
with basically the same patch as James did.  His was sent in first
and is logically identical, so I'm not going to send my version.

With regards to future proofing this, I have no idea.  I'm going to
write up a test case that catches this and submit that, to avoid a
future regression here, but otherwise I'm not sure what we can do.
I had thought I had visited every callsite and checked them, but
apparently that wasn't true.

What would probably help is to change the name of the structure
member in the .h file when its initialization time changes (or its
meaning changes in a subtle way) and then go through and manually
update every mention of the old name to the new name, then flip it
back with a global search and replace when done.

E.g. I should have done:

	* in cache.h rename num_objects to num_objects_SHAWNCHANGEHERE;
	* manually update every num_objects usage to my private name;
	* finally globally search-replace back to the standard name.

It would have forced me to more carefully visit every damn callsite.

I'll go back through the code tonight and double check my work,
because this bug was completely my fault.  I'm hoping this was the
only bug however.

-- 
Shawn.

Re: [PATCH] rev-parse: Identify short sha1 sums correctly.

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> James Bowes <jbowes@dangerouslyinc.com> writes:
> 
> > find_short_packed_object was not loading the pack index files.
> > Teach it to do so.
> >
> > Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
> 
> I think this is the proper fix of the problem I was unhappy
> about with 'next'

Actually this bug is exactly the reason why I *always* run 'next'
in production use on all of my systems, and why I also get a number
of coworkers to run it.  Many people beating on the same code in
weird ways notice things.

Today I found this because I have a shell script that tried to
do something of the form "git checkout v1.8.0-18-g18as1f' and Git
couldn't believe that was a revision...  It held up day-job work
for an hour while I tracked it down, but I did catch it.  ;-)

-- 
Shawn.

[PATCH] rev-parse: Identify short sha1 sums correctly.

[James Bowes]

find_short_packed_object was not loading the pack index files.
Teach it to do so.

Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
---
 sha1_name.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 8dfceb2..7df01af 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -76,8 +76,11 @@ static int find_short_packed_object(int len, const unsigned char *match, unsigne
 
 	prepare_packed_git();
 	for (p = packed_git; p && found < 2; p = p->next) {
-		uint32_t num = p->num_objects;
-		uint32_t first = 0, last = num;
+		uint32_t num, last;
+		uint32_t first = 0;
+		open_pack_index(p);
+		num = p->num_objects;
+		last = num;
 		while (first < last) {
 			uint32_t mid = (first + last) / 2;
 			const unsigned char *now;
-- 
1.5.2.869.g6b3ba

Re: [PATCH] rev-parse: Identify short sha1 sums correctly.

[Junio C Hamano]

Thanks, James.

This seems to fix the bug I mentioned about 'next' in the last
"What's cooking" message.  Also I have been seeing a segfault
from rev-parse in t5500 (rev-parse --short hits the same issue,
because the bug caused object name not to be abreviated) but
that is also fixed with this patch.

Will apply, instead of reverting the "lazy index loading".

Re: [PATCH] rev-parse: Identify short sha1 sums correctly.

[James Bowes]

On 5/29/07, Junio C Hamano <junkio@cox.net> wrote:
> Thanks, James.

Glad to help when I can.

> This seems to fix the bug I mentioned about 'next' in the last
> "What's cooking" message.  Also I have been seeing a segfault
> from rev-parse in t5500 (rev-parse --short hits the same issue,
> because the bug caused object name not to be abreviated) but
> that is also fixed with this patch.
>
> Will apply, instead of reverting the "lazy index loading".
>
>
>

-James