[PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs

[PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs

[Brandon Casey]

If pack-objects is called with the --unpack-unreachable option then it will
unpack (i.e. loosen) all unreferenced objects from local not-kept packs,
including those that also exist in packs residing in an alternate object
database or a local kept pack.  The primary(sole?) user of this option is
git-repack.  In this case, repack will follow the call to pack-objects with
a call to prune-packed which will delete these newly loosened objects,
making the act of loosening a waste of time.  The unnecessary loosening can
be avoided by checking whether an object exists in a non-local pack or a
local kept pack before loosening it.

This fixes the 'local packed unreachable obs that exist in alternate ODB
are not loosened' test in t7700.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---


Signed version of the previous version.


 builtin-pack-objects.c |   26 +++++++++++++++++++++++++-
 t/t7700-repack.sh      |    2 +-
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 6222f19..3f477c5 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -1944,6 +1944,29 @@ static void
add_objects_in_unpacked_packs(struct rev_info *revs)
 	free(in_pack.array);
 }

+static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)
+{
+	static struct packed_git *last_found = (void *)1;
+	struct packed_git *p;
+
+	p = (last_found == (void *)1) ? packed_git : last_found;
+
+	while (p) {
+		if ((!p->pack_local || p->pack_keep) &&
+			find_pack_entry_one(sha1, p)) {
+			last_found = p;
+			return 1;
+		}
+		if (p == last_found)
+			p = packed_git;
+		else
+			p = p->next;
+		if (p == last_found)
+			p = p->next;
+	}
+	return 0;
+}
+
 static void loosen_unused_packed_objects(struct rev_info *revs)
 {
 	struct packed_git *p;
@@ -1959,7 +1982,8 @@ static void loosen_unused_packed_objects(struct
rev_info *revs)

 		for (i = 0; i < p->num_objects; i++) {
 			sha1 = nth_packed_object_sha1(p, i);
-			if (!locate_object_entry(sha1))
+			if (!locate_object_entry(sha1) &&
+				!has_sha1_pack_kept_or_nonlocal(sha1))
 				if (force_object_loose(sha1, p->mtime))
 					die("unable to force loose object");
 		}
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 013e488..9ce546e 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -113,7 +113,7 @@ test_expect_success 'packed unreachable obs in
alternate ODB are not loosened' '
 	test_must_fail git show $csha1
 '

-test_expect_failure 'local packed unreachable obs that exist in
alternate ODB are not loosened' '
+test_expect_success 'local packed unreachable obs that exist in
alternate ODB are not loosened' '
 	echo `pwd`/alt_objects > .git/objects/info/alternates &&
 	echo "$csha1" | git pack-objects --non-empty --all --reflog pack &&
 	rm -f .git/objects/pack/* &&
-- 
1.6.2.12.g83676

Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> If pack-objects is called with the --unpack-unreachable option then it will
> unpack (i.e. loosen) all unreferenced objects from local not-kept packs,
> including those that also exist in packs residing in an alternate object
> database or a local kept pack.  The primary(sole?) user of this option is
> git-repack.  In this case, repack will follow the call to pack-objects with
> a call to prune-packed which will delete these newly loosened objects,
> making the act of loosening a waste of time.  The unnecessary loosening can
> be avoided by checking whether an object exists in a non-local pack or a
> local kept pack before loosening it.
>
> This fixes the 'local packed unreachable obs that exist in alternate ODB
> are not loosened' test in t7700.
>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>

Thanks.

Both patches are whitespace damaged, but I can cope.  But I am not sure
about one thing...

>  builtin-pack-objects.c |   26 +++++++++++++++++++++++++-
>  t/t7700-repack.sh      |    2 +-
>  2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index 6222f19..3f477c5 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -1944,6 +1944,29 @@ static void
> add_objects_in_unpacked_packs(struct rev_info *revs)
>  	free(in_pack.array);
>  }
>
> +static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)
> +{
> +	static struct packed_git *last_found = (void *)1;
> +	struct packed_git *p;
> +
> +	p = (last_found == (void *)1) ? packed_git : last_found;

Why (void *)1, not like:

	static struct packed_git *last_found;
	struct packed_git *p = last_found ? last_found : packed_git;

Am I missing something?

> +	while (p) {
> +		if ((!p->pack_local || p->pack_keep) &&
> +			find_pack_entry_one(sha1, p)) {
> +			last_found = p;
> +			return 1;
> +		}
> +		if (p == last_found)
> +			p = packed_git;
> +		else
> +			p = p->next;
> +		if (p == last_found)
> +			p = p->next;
> +	}
> +	return 0;
> +}

Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs

[Brandon Casey]

On Sat, Mar 21, 2009 at 11:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:

> Both patches are whitespace damaged, but I can cope.

I just retrieved one of the patches from gmane and some long lines
were wrapped.  I tried out the gmail imap instructions from
SubmittingPatches for sending these patches.  Those instructions say
that it is possible to send properly formatted patches through gmail,
and seem to instruct to use the web interface for actually sending the
patches.  I wonder if there is some way to instruct gmail to not wrap
long lines? Or whether I did something else wrong?

Previously, I have used gmail's pop interface indirectly through my
phone provider which strips out the "From" field and replaces it with
one that only has my email address and not my name.

>  But I am not sure about one thing...
>
>> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
>> index 6222f19..3f477c5 100644
>> --- a/builtin-pack-objects.c
>> +++ b/builtin-pack-objects.c
>> @@ -1944,6 +1944,29 @@ static void
>> add_objects_in_unpacked_packs(struct rev_info *revs)
>>       free(in_pack.array);
>>  }
>>
>> +static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)
>> +{
>> +     static struct packed_git *last_found = (void *)1;
>> +     struct packed_git *p;
>> +
>> +     p = (last_found == (void *)1) ? packed_git : last_found;
>
> Why (void *)1, not like:
>
>        static struct packed_git *last_found;
>        struct packed_git *p = last_found ? last_found : packed_git;
>
> Am I missing something?

Heh, I am missing something too.  Maybe I should have _thought_ more
about this code that I copied from sha1_file.c: find_pack_entry() and
I would have asked the same question about _that_ code.

Maybe Nico has some idea?

I'll send a new patch, unless Nico has some thoughts.

-brandon

Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs

[Nicolas Pitre]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 967 bytes --]

On Sun, 22 Mar 2009, Brandon Casey wrote:

> >> +static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)
> >> +{
> >> +     static struct packed_git *last_found = (void *)1;
> >> +     struct packed_git *p;
> >> +
> >> +     p = (last_found == (void *)1) ? packed_git : last_found;
> >
> > Why (void *)1, not like:
> >
> >        static struct packed_git *last_found;
> >        struct packed_git *p = last_found ? last_found : packed_git;
> >
> > Am I missing something?
> 
> Heh, I am missing something too.  Maybe I should have _thought_ more
> about this code that I copied from sha1_file.c: find_pack_entry() and
> I would have asked the same question about _that_ code.
> 
> Maybe Nico has some idea?

Well...  I know this is my code, and I must have had a reason to keep 
the NULL pointer distinct from the initial value.  But at the moment I 
just can't remember about it, and the code doesn't show a need for it 
either.


Nicolas

Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs

[Junio C Hamano]

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

>> +static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)
>> +{
>> +	static struct packed_git *last_found = (void *)1;
>> +	struct packed_git *p;
>> +
>> +	p = (last_found == (void *)1) ? packed_git : last_found;
>
> Why (void *)1, not like:
>
> 	static struct packed_git *last_found;
> 	struct packed_git *p = last_found ? last_found : packed_git;
>
> Am I missing something?
>
>> +	while (p) {
>> +		if ((!p->pack_local || p->pack_keep) &&
>> +			find_pack_entry_one(sha1, p)) {
>> +			last_found = p;
>> +			return 1;
>> +		}
>> +		if (p == last_found)
>> +			p = packed_git;
>> +		else
>> +			p = p->next;
>> +		if (p == last_found)
>> +			p = p->next;
>> +	}
>> +	return 0;
>> +}

Yes I was.

If you allow (last_found == NULL), the loop iterates over packed_git list,
and when it has its last element in p after discovering nothing matches
the criteria, this happens:

	if (p == last_found) /* false, p is the last element on the list */
        	p = packed_git; /* not taken */
	else
        	p = p->next; /* taken, p == NULL now */
	if (p == last_found) /* true! */
		p = p->next; /* OOPS */

Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs

[Nicolas Pitre]

On Sun, 22 Mar 2009, Junio C Hamano wrote:

> If you allow (last_found == NULL), the loop iterates over packed_git list,
> and when it has its last element in p after discovering nothing matches
> the criteria, this happens:
> 
> 	if (p == last_found) /* false, p is the last element on the list */
>         	p = packed_git; /* not taken */
> 	else
>         	p = p->next; /* taken, p == NULL now */
> 	if (p == last_found) /* true! */
> 		p = p->next; /* OOPS */

Yeah, you got it.  Now I remember.


Nicolas

Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs

[Brandon Casey]

Junio,

I notice this patch is in next now (I noticed earlier than today,
but haven't said anything).

I just want to point one minor thing.  After figuring out why last_found
was initialized to (void *)1 rather then NULL, you seem to have committed
a version that was slightly different from what I submitted.  The logic
is the same, but now the following line is different from the line in
sha1_file.c where I copied it from:

builtin-pack-objects.c (line 1952):

   p = (last_found != (void *)1) ? last_found : packed_git;

sha1_file.c (line 1931): 

   p = (last_found == (void *)1) ? packed_git : last_found;


People should be able to easily recognize the two procedures as "the same".
This small difference may give them pause.  If it is possible to amend
this commit before applying to master, perhaps you would like to do so.
Otherwise, no big deal.

-brandon