Re: [PATCH] t7700-repack: repack -a now works properly, expect success from test

From: Brandon Casey <casey@nrlssc.navy.mil>
Date: Mar 18, 2009, 20:59 UTC
Message-ID: <EblkEXbcYUWby1OEDup9n0l6lbwNL-4-jyt02Cjab2pqLqekAUji0w@cipher.nrlssc.navy.mil>
In-Reply-To: <7Vazs5mFk91IKAarOd0wrBNmYj7eSJxVIcR0PEQxJl8R0aQmQDEqSJMphMrXhmVu570fijupQ34@cipher.nrlssc.navy.mil>

Brandon Casey wrote:

Show 34 quoted lines

> Since Junio's keep-pack series was merged in at aec81306, git-repack now
> properly packs objects from alternate repositories even when the local
> repository contains packs.
> 
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
> 
> 
> Well, I finally got some time last week to take a look at this issue, and
> what do I find, but you've already fixed it.  Did you even realize? :)
> 
> 
> I was too busy to notice the emails on the subject last month...
> 
> http://article.gmane.org/gmane.comp.version-control.git/111758
> 
> Junio C Hamano wrote:
>> I think we probably could get rid of --honor-pack-keep mechanism after
>> this series, but I didn't look very deeply into it.
> 
> This makes sense, but doesn't seem to be working.  Possibly the way
>  --kept-pack-only is implemented, it only deals with commits.  So if the
> commit object is _not_ in a kept pack, but the trees and/or blobs that it
> references _are_, then both the commit and the trees/blobs will be added to
> the list of objects to pack even though the trees/blobs should not be.
> 
> The test in t7700 is contrived, and creates two pack files; one marked with
> .keep which contains a single blob object that is missing from the other.
> Then the test repacks and checks whether the blob object was placed into the
> new pack.  The --honor-pack-keep mechanism handles this case since it works
> on the back end and checks whether each object that is added to the objects
> list exists in a kept pack, and skips it if so.
> 
> Disclaimer: I have not taken the time to fully understand revision.c

Hmm, I think there are a couple of new issues.

  1) The --kept-pack-only mechanism does not operate solely on "local"
     packs now.  This means that objects residing in an alternate pack
     which has a .keep file will not be repacked with repack -a.  My
     current opinion is that .keep files in an alternate object database
     should not be honored since the user may not have any control over
     them.  This seems to be in line with your statement about this feature
     affecting locally existing packs from the Release Notes:

       git-gc spent excessive amount of time to decide if an object appears
       in a locally existing pack

  2) The 'repack unpacked objects' and 'loosen unpacked objects' mechanisms
     now do not operate solely on local packs.  I think this means that
     objects residing in alternate not-kept packs will be repacked when
     '-A -d' is used, and will be loosened when '-a -d' is used.

I have a test for #1 above. I'll think about a test for #2.

I wonder if we'll ever be interested in non-local keep files? If not, then why not set pack_keep _only_ for local packs?

diff --git a/sha1_file.c b/sha1_file.c
index 4563173..a595eac 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -881,9 +881,11 @@ struct packed_git *add_packed_git(const char *path, int pat
        }
        memcpy(p->pack_name, path, path_len);
 
-       strcpy(p->pack_name + path_len, ".keep");
-       if (!access(p->pack_name, F_OK))
-               p->pack_keep = 1;
+       if (local) {
+               strcpy(p->pack_name + path_len, ".keep");
+               if (!access(p->pack_name, F_OK))
+                       p->pack_keep = 1;
+       }
 
        strcpy(p->pack_name + path_len, ".pack");
        if (stat(p->pack_name, &st) || !S_ISREG(st.st_mode)) {


I'd also change the name from pack_keep to something that identivies it as local.

The alternative would require that find_pack_ent() differentiate between
local kept packs and alternate kept packs.

-brandon