getting list of objects for packing

I'm trying to write a script that will repack large binary or compressed objects into their own non-compressed, non-delta'ed pack file.

To make the decision about whether an object should go into this special pack file or not, I want the output from 'git cat-file --batch-check'. I get it with something similar to:

   git rev-list --objects --all |
      sed -e 's/^\([0-9a-f]\{40\}\).*/\1/' |
      git cat-file --batch-check

First question: Is the rev-list call correct?
  -If I am understanding things right, then the list of objects produced
   by rev-list will be in the right order for piping to pack-objects. 
  -The sed statement is stripping off anything after the sha1. Any way to
   get rev-list to print out just the sha1 so that sed is not necessary?

Then I want to parse the output from cat-file and use an external program to detect the file format. Here is a simplified version:

  | while read sha1 type size; do

       if [ $type = "blob" ]; then
           if ! ( git cat-file blob "$sha1" | file -b - | grep text ) &&
              [ $size -ge $threshhold ]; then
               # pack into special pack
           else
               # pack normally into normal pack
           fi
       fi
  done

All of this has actually been rewritten into a perl script, so ignore any syntax mistakes.

I have successfully created two of the pack files that I have been trying to make. Where the definition of successful means that after removing the existing packs and objects, and putting in place the two pack files that I generated, 'git fsck --full' prints no errors and exits successfully.

These two packs will be placed into a central repository.

ISSUE TWO:

I have placed these two packs into my own personal repo, and I have unpacked all of the other objects so that they are loose.

I thought I could use a similar sequence of commands to pack those loose objects into a normal and special pack. I added the --unpacked option to my rev-list command, but it still lists many more objects than exist loosely in the repository.

   git rev-list --objects --unpacked --all

The man page says:

   --objects
          Print  the  object  IDs  of any object referenced by the listed
          commits. --objects foo ^bar thus means "send me all object  IDs
          which  I  need to download if I have the commit object bar, but
          not foo".

   --unpacked
          Only useful with --objects; print the object IDs that  are  not
          in packs.

Is this the correct behavior for rev-list --unpacked? Am I mis-reading the --unpacked text, or should it be changed?

-brandon

On Fri, 31 Oct 2008, Brandon Casey wrote:

yes.

If you strip the data after the SHA1 when pipping into pack-objects then you'll have horrible delta compression results. The path names after each SHA1 is used to sort objects when trying to find best matches for delta compression. So you should preserve those and feed it back especially with those packs that you still want delta compression for.

Well, don't think so. Although I have zero unpacked objects in my git repository, the listing still gives me a hundred objects or so, and they all appear to be tag objects. There is probably a bug here.

Nicolas

Nicolas Pitre wrote:

Ah, I'll have to rethink my script then. Thanks!

-brandon

Brandon Casey <casey@nrlssc.navy.mil> writes:

Yeah, but wasn't the purpose of your whole exercise to list objects that do not delta nor compress well with each other, in which case the delta compression order (aka name hash) would not matter, no?

Junio C Hamano wrote:

The script I wrote actually starts up two pack-objects instances and I was writing the objects I wanted to pack _normally_ to one, and the ones that I did not want compressed/deltafied to the other (which was started with --no-reuse-object --window=0 --depth=0 --compression=0).

I didn't mentioned that fact in my first email, but I'm very glad Nico made his point.

-brandon

Brandon Casey <casey@nrlssc.navy.mil> writes:

Wasn't there some gitattribute which prohibited deltification of some files (`delta` or something)? Or wasn't this patch accepted, as I cannot find such attribute in documentation (gitattributes(5))... ... err, it was added in commit a74db82e15cd8a2c53a4a83e9a36dc7bf7a4c750 (Teach "delta" attribute to pack-objects.) by Junio C Hamano in May 19, _without_ documentation.

Brandon Casey wrote:

So, my script created two pack files: one packed normally, and one packed without compression or delta. I removed my original packs, and put these two new ones in my pack directory and ran 'git fsck --full' and it completed successfully. There are no loose objects in the repo.

I added a .keep file for each pack.

Since my script removed the extra info from rev-parse's output, I removed the .keep file from the appropriate pack and ran 'git gc --aggressive'.

The 1.7GB pack that had the .keep file removed has been replaced with a +3GB pack file. The other pack file which still has the .keep file is 2.3GB.

In another repo with 3 packs marked .keep, and one 388KB pack with ~300 objects in it, and 3 loose dangling objects, the 388KB pack was replaced with a 3.5GB pack.

It appears that the entire repository is being packed into the new pack file even though there are existing pack files with .keep files.

If I compare the output from 'git verify-pack -v' I can see that many of the objects in the packs marked with a .keep file are indeed in the new pack file. But not all of them.

-brandon

From: Brandon Casey <drafnel@gmail.com>

Objects residing in pack files that have an associated .keep file are not supposed to be repacked into new pack files, but they are.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 t/t7700-repack.sh |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100755 t/t7700-repack.sh

From: Brandon Casey <drafnel@gmail.com>

This converts the pack_local flag of the packed_git structure into a generic bit mask and introduces a PACK_LOCAL mask and an ispacklocal() access macro.

So instead of this:

   if (p->pack_local)
      do_something

you would do this:

   if (ispacklocal(p))
      do_something

This is in preparation for adding a flag indicating whether a .keep file is present.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 builtin-count-objects.c |    2 +-
 builtin-gc.c            |    2 +-
 builtin-pack-objects.c  |    2 +-
 cache.h                 |    5 ++++-
 pack-redundant.c        |    4 ++--
 server-info.c           |    4 ++--
 sha1_file.c             |    5 +++--
 7 files changed, 14 insertions(+), 10 deletions(-)

drafnel@gmail.com wrote:

Good idea.
 

Hmm, isn't this a smaller change to make?

- int pack_local; + unsigned pack_local:1;

Then later you can do:

- unsigned pack_local:1; + unsigned pack_local:1, + pack_keep:1;

and the compiler handles all the bitmask stuff for you?

In general in git.git we like to use the struct bitmask stuff when possible as the code is easier to follow. We only use explicit mask constants and mask operations when the data is being stored on disk or written over the network and we need to ensure it is consistent across compilers. But for in-core only stuff, struct bitmasks are easier.

Shawn O. Pearce wrote:

heh, well if you want to do it the "easy" way. :)

For some reason I've never used this bit field mechanism, but I agree it is more readable and simpler, and you can't argue with that.

-brandon

From: Brandon Casey <drafnel@gmail.com>

Objects residing in pack files that have an associated .keep file are not supposed to be repacked into new pack files, but they are.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

This version replaces the use of 'head -n -1' with a grep, and should work on all platforms.

-brandon

 t/t7700-repack.sh |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100755 t/t7700-repack.sh

From: Brandon Casey <drafnel@gmail.com>

pack_keep will be set when a pack file has an associated .keep file.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

This patch and the following one redo the previous 3-patch series using a bitfield as prudently suggested by Shawn.

It seemed silly to keep the conversion of pack_local into a bitfield, and the introduction of pack_keep separate, so all 7 lines are in this one patch.

-brandon

 cache.h     |    3 ++-
 sha1_file.c |    5 +++++
 2 files changed, 7 insertions(+), 1 deletions(-)

From: Brandon Casey <drafnel@gmail.com>

By default, pack-objects creates a pack file with every object specified by the user. There are two options which can be used to exclude objects which are accessible by the repository.

   1) --incremental
     This excludes any object which already exists in an accessible pack.

   2) --local
     This excludes any object which exists in a non-local pack.

With this patch, both arguments also cause objects which exist in packs marked with a .keep file to be excluded. Only the --local option requires an explicit check for the .keep file. If the user doesn't want the objects in a pack marked with .keep to be exclude, then the .keep file should be removed.

Additionally, this fixes the repack bug which allowed porcelain repack to create packs which contained objects already contained in existing packs marked with a .keep file.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 builtin-pack-objects.c |    2 +-
 t/t7700-repack.sh      |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Brandon Casey <casey@nrlssc.navy.mil> wrote:

This one and the one before it (2/3):

Acked-by: Shawn O. Pearce <spearce@spearce.org>

Junio,

Please leave this in pu for now, I have some concerns that I haven't had time to write down yet.

-brandon

Brandon Casey wrote:

Brandon Casey wrote:

I've been thinking about pack-objects and repack.

Here's how I think the semantics of repack should be defined:

repack:

  <no-options>
    -incremental repack which does not repack any object currently packed
     in any accessible pack.

    Works (it currently works this way).

  <-a>
    -create a new pack containing all objects required by the repository
     including those accessible through alternates, but excluding objects
     in _local_ packs with .keep

    Flawed, even with my recent patches.

    If there are no local packs (or they all have .keep files), then the
    pack-objects call will use --incremental which will exclude objects
    packed in alt object store, even though -l was not used.
 
    My patches do not differentiate local .keep files from remote .keep files,
    which a user may have no control over.

  <-A>
     -Like -a, but local unreferenced objects which were previously packed
      are made to be loose.

    Ditto.

  <-a -l>
     -Restrict operation to only local objects. Only has any effect with -a|-A.
     -Like -a, but additionally exclude objects in packs accessible through
      alternates.

     Works with my recent patches.

  <-A -l>
     -Like '-a -l', but loosen unreferenced local packed objects.

     Ditto.

That set of repack operations needs to map to a combination of pack-objects options:

  <no-options>
     -Create a pack with _all_ specified objects

  <--unpacked>
     -Exclude from packing any object already in an accessible pack.

     (Ahh, this came from rev-list interface, and rejects objects at an
      earlier stage than --incremental)

  <--unpacked=sha1>
     -Like '--unpacked', exclude already packed objects, but treat the objects
      in the pack with specified sha1 as unpacked.

  <--incremental>
     -Exclude from packing any object already in an accessible pack,
      regardless of whether it is in a pack specified by --unpacked=

      (How is this different from --unpacked, even though the exclusion
       operation is performed at a different stage? See my epiphany above
       about the source of the --unpacked option)

  <--unpacked --incremental>
     -seems redundant, is there any functional difference?

  <--local>
     -Exclude objects from being packed that are not in the local object store.

The issue is how to provide my described 'repack -a' functionality. There does not seem to be a mapping between the above options and the required functionality.

I see two solutions, both require introducing a new option to pack-objects.
  1) allow specifying a set of packs such that if an object resides
     in any of the set, the object will not be included in the produced
     pack.

     Benefits:
     -allows keeping pack-objects ignorant of .keep mechanism
     -repack can easily be modified to produce the set of packs to ignore

     Drawback:
     -very round-about way just to have functionality to skip packs with
      .keep file

  2) New option telling pack-objects to skip objects in local .keep'd packs

     Benefits:
     -easy to implement in pack-objects
     -easy to modify repack

     Drawbacks:
     -introduces new concept to pack-objects

Questions aside:
  1) Are both --incremental and --unpacked still necessary pack-objects options?
  2) Can --incremental become an alias for --unpacked, and go away?

patch(es) will follow.

-brandon

Brandon Casey <casey@nrlssc.navy.mil> writes:

I have a feeling that it is debatable if this "fattening to dissociate from alternates" is what people want.

Presumably you meant "exclude objects accessible through alternates, either in packs or in loose form"? If so then I think it is a good thing to have.

I am not sure if listing the behaviour by combination of flags is a good way to start thinking about this. Wouldn't it be more productive to list what kinds of repacking are needed, and then label them with combination of flags? Otherwise you would miss a potentially useful operation that cannot be expressed with the current set of flags you have.

I think the useful kinds are only these five:

 - scoop loose objects that exist in local repository into a new pack,
   without touching existing packs at all; exclude anything available in
   any existing pack or in alternate repository (either loose or packed);

 - pack everything that is needed by the local ref, except the ones that
   are borrowed from alternate repositories (either loose or packed), into
   a single new pack.  There are two variants of this: eject what is
   currently packed but unnecessary into loose format when existing local
   packs are replaced with the new pack, or lose them (i.e. -A).

 - fatten local repository by packing everything that is needed by the
   local ref into a single new pack, including things that are currently
   borrowed from alternates.  There are two variants of this: eject what
   is currently packed but unnecessary into loose format when existing
   local packs are replaced with the new pack, or lose them (i.e. -A).

Junio C Hamano wrote:

I'm not sure I understand you here.

Andreas has suggested previously that 'repack -a' should pack everything, including objects in packs with .keep. Is that what you mean?

With my current understanding it seems that that would muddy the semantics of repack. If -a does not honor packs with .keep, then would it be intuitive to expect that adding -l (i.e. exclude alternate packed objects) _would_ honor .keep?

Would that be an enhancement to the current behavior? I don't think I saw any mechanism to exclude packing remote loose objects.

The documentation for pack-objects --local says:

  --local
         This flag is similar to --incremental; instead of ignoring  all
         packed objects, it only ignores objects that are packed and not
         in the local object store (i.e. borrowed from an alternate).

It only mentions packed alternate objects.

I agree. I made a list of the options because I was trying to understand what effect each option had, then I turned it into an email.

You didn't say when local .keep should be honored, i.e. objects in local packs with .keep should be excluded from repacking. always, never, only with -l, new repack option?

-brandon

Brandon Casey wrote:

Only -d should honor .keep, imo. .keep-files is nothing about "don't copy objects from this file" and all about "never delete this file".

The only muddying comes from you, who decided that .keep-files should have impact on anything else than deleting the protected pack. Before that, .keep files had a clear semantic, and repack's documentation was correct.

How do you explain .keep-files now? "protects pack-files that will forever be used"? Then why the hell is it called ".keep" instead of "eternal"?

Andreas Ericsson <ae@op5.se> wrote:

The _intent_ behind .keep files has always been to avoid copying the objects into a new pack during git-repack, because the objects are held in a pack that cannot be deleted.

Sadly the implementation has never quite worked right, which is what led to this patch series I think.

This was developed on top of the previous repack/pack-objects series.

Junio wrote:

This patch set looks like what is necessary to exclude loose objects accessible through alternates from packing.

  repack -l

  repack -a -l
  repack -A -l

  repack -a
  repack -A

-brandon

drafnel@gmail.com writes:

Thanks.  Looked alright from a cursory reading.

By the way, I've been meaning to suggest that we should straighten out the semantics of "unpacked" vs "incremental".

What the latter means is quite clear. We are creating a new packfile to bundle loose ones into one, and after the new packfile is installed we can remove the loose objects.

But what --unpacked means often confuses people, primarily because it is a performance heuristics that makes certain assumptions on how the objects are packed.

Namely, "unpacked" is about discovery process of objects to be packed. Without the option, we enumerate all objects that are reachable from the commits in the given range, excluding the trees and blobs that should exist in commits that are excluded (e.g, if you say "--objects A..B", we exclude trees and blobs referenced by commit A).

With the option, we also omit commits that are packed. What's funny is that their trees and blobs are omitted, even if they are loose ;-)

This is typically not an issue, because you do not say "pack only this commit object, without its trees or blobs" when repacking, and because you must have all the trees and blobs necessary for a commit available when you pack a commit; for these reasons, the trees and blobs are typically packed together with the commit. It is not an issue that the rev-list with --unpacked option does not list loose trees and blobs that belong to a packed commit for this reason.

You could however arrange so that a commit itself is packed but some of the tree and blob objects it refers to are loose, in which case these loose objects may not ever get repacked incrementally.

In an empty directory, try this:

        git init &&
        echo 0 >file && git add file && git commit -m initial &&

        P=$(git rev-list HEAD | git pack-objects pack) &&
        mv pack-$P.* .git/objects/pack/ &&
        git prune && git count-objects -v

        git repack && git count-objects -v

It packs only the commit object (and prunes it), leaving a tree and a blob loose. The repack won't find anything to pack.

This is not so bad in the sense that it will never corrupt your repository, but it is confusing. Admittedly, not peeking into a commit that is packed is a reasonable good heuristics for performance reasons.

Interestingly enough, the object listing machinery do traverse into parents of packed commits when --unpacked is given. So if you pack a commit and arrange to keep its parents unpacked, they are subject to the incremental repacking. In other words, the performance heuristics may not be buying us very much --- we are traversing the history down to the root commits regardless.

This adds a new option to pack-objects which will cause it to ignore an object which appears in a local pack which has a .keep file, even if it was specified for packing.

This option will be used by the porcelain repack.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

This series replaces the previous series starting at 6ee726bc "pack-objects: honor '.keep' files"

It should be applied on top of f34cf12d "packed_git: convert pack_local flag into a bitfield and add pack_keep"

I created the series on top of f34cf12d rebased on top of master.

Suggestions for a more appropriate name for --honor-pack-keep are very welcome.

-brandon

 Documentation/git-pack-objects.txt |    5 +++++
 builtin-pack-objects.c             |    7 +++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

If the user created a .keep file for a local pack, then it can be inferred that the user does not want those objects repacked.

This fixes the repack bug tested by t7700.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 git-repack.sh     |    2 +-
 t/t7700-repack.sh |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

When repack is called with either the -a or -A option, the user has requested to repack all objects including those referenced by the alternates mechanism. Currently, if there are no local packs without .keep files, then repack will call pack-objects with the '--unpacked --incremental' options which causes it to exclude alternate packed objects. So, remove this fallback.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 git-repack.sh |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

I think the existing code was broken. Looks like it would only skip counting a pack if the pack disappeared between the prepare_packed_git() and the access() call. It never used the path it create with the .keep extension.

-brandon

 builtin-gc.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

Brandon Casey wrote:

I disagree. It can be inferred that the user doesn't want those packfiles *removed*.

Brandon Casey wrote:

Keep-files are *always* honored. Make this option "--ignore-kept" or something instead, otherwise people will see the synopsis and think they need to always pass it to not remove .keep-protected packs, which is stupid.

From: Brandon Casey <drafnel@gmail.com>

Objects residing in pack files that have an associated .keep file are not supposed to be repacked into new pack files, but they are.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

Whoops, white space contaminated in two spots, this one fixes it.

-brandon

 t/t7700-repack.sh |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100755 t/t7700-repack.sh

Brandon Casey wrote:

sed 1q is faster, as it stops parsing after the first line (the same as 'head -n 1' does, but in a more portable fashion).

Andreas Ericsson wrote:

Except that I wanted all but the _last_ line though.

I didn't think about using sed. Perhaps I could have used something like

   sed -n -e '$q' -e 'p'

The grep works though.

-brandon

Brandon Casey <casey@nrlssc.navy.mil> writes:

If your sed is GNU, and you are doing this for a small file, the startup cost of it may dwarf such gain ;-)

You surely meant "sed -e '$d'", right?

Indeed.

Junio C Hamano wrote:

Your sed-fu is quite impressive.

-b

Brandon Casey wrote:

Ach pooie. That's what I get for trying to review stuff while watching old 70's samurai movies. I misread your 'head' command.

Thanks; queued.

On Mon, Nov 03, 2008 at 02:37:05PM -0600, Brandon Casey wrote:

Hmm. I'm not sure what happened, but the version in 'next' has "head -n -1" in it.

-Peff

Jeff King <peff@peff.net> writes:

You mean this one?

commit 31d92611e45d1286b805e362dbc451936af24121
Author:     Brandon Casey <drafnel@gmail.com>
AuthorDate: Sat Nov 1 22:35:19 2008 -0500
Commit:     Junio C Hamano <gitster@pobox.com>
CommitDate: Sun Nov 2 15:58:09 2008 -0800

    t7700: demonstrate mishandling of objects in packs with a .keep file

On Wed, Nov 12, 2008 at 09:10:45AM -0800, Junio C Hamano wrote:

Yes, exactly.

-Peff

Jeff King wrote:

Well, there were so many revisions, I probably should have re-rolled the whole series. I wasn't sure this was going to go in as is, based on the [Dropped] message in the "What's Cooking" email and Junio's last email about reconciling --unpacked and --incremental. I have been working through the --unpacked code path, but I'm not to the point where I can suggest a change there.

But, I think it's worse than just the wrong t7700.

These two:

   packed_git: convert pack_local flag into generic bit mask
   packed_git: add new PACK_KEEP flag and haspackkeep() access macro

should have been replaced by:

   packed_git: convert pack_local flag into a bitfield and add pack_keep

which uses a struct bitfield rather than a bitmask.

And then this

   pack-objects: honor '.keep' files

was replaced by this

   pack-objects: new option --honor-pack-keep
   repack: don't repack local objects in packs with .keep file

if that's the way we want to go. I'm not partial to the phrase honor-pack-keep, but I don't think ignore-pack-keep is appropriate, and it's the best I've come up with.

So,

   31d92611e45d1286b805e362dbc451936af24121
   7c335327be664751fa4c04e81b2fe3bfedceaada
   77b5a5478a77cc04b674891b542db1ba1a1bf4f7
   13e7f5d2f1da42619bd545590d0044b30d00ce4b

should be reverted, and replaced by the series to follow.

-brandon

Here is an updated series built on top of master.

What follows is the 6 patches that were sent, but are not in next.

-brandon

From: Brandon Casey <drafnel@gmail.com>

Objects residing in pack files that have an associated .keep file are not supposed to be repacked into new pack files, but they are.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 t/t7700-repack.sh |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)
 create mode 100755 t/t7700-repack.sh

From: Brandon Casey <drafnel@gmail.com>

pack_keep will be set when a pack file has an associated .keep file.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 cache.h     |    3 ++-
 sha1_file.c |    5 +++++
 2 files changed, 7 insertions(+), 1 deletions(-)

This adds a new option to pack-objects which will cause it to ignore an object which appears in a local pack which has a .keep file, even if it was specified for packing.

This option will be used by the porcelain repack.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 Documentation/git-pack-objects.txt |    5 +++++
 builtin-pack-objects.c             |    7 +++++++
 2 files changed, 12 insertions(+), 0 deletions(-)

If the user created a .keep file for a local pack, then it can be inferred that the user does not want those objects repacked.

This fixes the repack bug tested by t7700.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 git-repack.sh     |    2 +-
 t/t7700-repack.sh |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

When repack is called with either the -a or -A option, the user has requested to repack all objects including those referenced by the alternates mechanism. Currently, if there are no local packs without .keep files, then repack will call pack-objects with the '--unpacked --incremental' options which causes it to exclude alternate packed objects. So, remove this fallback.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 git-repack.sh |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 builtin-gc.c |   12 +-----------
 1 files changed, 1 insertions(+), 11 deletions(-)

Previously, when 'repack -a' was called and there were no packs in the local repository without a .keep file, the repack would fall back to calling pack-objects with '--unpacked --incremental'. This resulted in the created pack file, if any, to be missing the packed objects in the alternate object store. Test that this specific case has been fixed.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

Here is a test to demonstrate what
  [PATCH 5/6] repack: do not fall back to incremental repacking with [-a|-A]
fixes.

This should apply cleanly to next since it includes
  a836cfa3 t7700: demonstrate mishandling of loose objects in an alternate ODB
which has a few context lines showing through in the diff below.

-brandon

 t/t7700-repack.sh |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

Brandon Casey <casey@nrlssc.navy.mil> writes:

Ok, care to send in a series in incremental updates format, please?

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

Nah, nevermind. I see you already sent a replacement series overriding what is in 'next'.

drafnel@gmail.com wrote:

I think that's a misconception. Packfiles that are marked with .keep files should never be deleted. There are, afaik, no rules against packing the same objects into other packfiles as well. This is nifty for dumb ref walkers, as they can use a small pack for incremental fetching while using a mega-pack for initial cloning.

Andreas Ericsson wrote:

Having no rules against an object residing in more than one pack is different from intending for git to produce pack files with redundant objects.

I think one intention for the .keep mechanism was to allow for a size optimized pack to be produced and distributed. Currently, if I am handed such a pack file, I can not merely place it into my pack directory (along with the .idx and .keep files) and then run git-gc to remove any redundancy. Instead, I would get a _new_ pack file which would contain all of the objects in the repository and effectively double the size of my objects store. That doesn't seem like something a user would expect or should expect.

-brandon

Brandon Casey wrote:

So long as "git repack -a" still creates a mega-pack, I'm fine with whatever.

Andreas Ericsson wrote:

I don't think it will after pack-objects is taught about .keep files, and I don't think it will _now_ if all of your packs have .keep files.

'repack -a' will call pack-objects with either '--unpack=<packfile>' for each pack file without a .keep file, or with '--unpacked --incremental' if there are no pack files without .keep files.

In the first case, the modifications to pack-objects that I propose will prevent objects that exist in local packs with .keep files from being packed into the new pack.

In the second case, the --incremental option would have done the same thing.

So this inconsistency already existed, but will now be removed in favor of honoring .keep files.

Mega-pack creation will become an "advanced operation" along the lines of:

    git rev-list --objects --all | git pack-objects

-brandon

Brandon Casey wrote:

In that case you're almost certainly breaking something.

It should, by copying the objects from the .keep-marked packfiles. Otherwise either repack or the repack docs are in error.

That means the nifty hack of incrementally and sometimes fully repack the odb to speed up cloning over dumb protocols no longer works properly, then.

From: Brandon Casey <drafnel@gmail.com>

If you want to tell whether a pack has an associated ".keep" file you would do:

   if (haspackkeep(p))
      do_something

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 cache.h     |    2 ++
 sha1_file.c |    5 +++++
 2 files changed, 7 insertions(+), 0 deletions(-)

From: Brandon Casey <drafnel@gmail.com>

By default, pack-objects creates a pack file with every object specified by the user. There are two options which can be used to exclude objects which are accessible by the repository.

   1) --incremental
     This excludes any object which already exists in an accessible pack.

   2) --local
     This excludes any object which exists in a non-local pack.

With this patch, both arguments also cause objects which exist in packs marked with a .keep file to be excluded. Only the --local option requires an explicit check for the .keep file. If the user doesn't want the objects in a pack marked with .keep to be exclude, then the .keep file should be removed.

Additionally, this fixes the repack bug which allowed porcelain repack to create packs which contained objects already contained in existing packs marked with a .keep file.

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

This seems to be the correct fix.

I thought about two alternatives:

  1) New option to pack-objects which causes it to honor .keep files

     I didn't think this was necessary since that is why we have .keep
     files in the first place.

     So, now there are three variants:

     Neither --incremental or --local is used)
        Ignore repo packs, pack is created using all objects specified by
        user.
     --incremental is used)
        Look at repo packs, exclude already packed objects (including those
        marked with .keep file)
     --local is used)
        Look at repo packs, exclude objects in non-local packs (including local
        objects marked with .keep file)

  2) Always honor .keep files, even when --incremental or --local is
     not specified. 

     I think this may be counter-intuitive, since if you specify all of
     the objects explicitly, without using any other options, you expect
     all of those objects to be placed into the created pack file. If
     .keep is always honored, then a new option would be required to
     indicate ignoring .keep files, or we would adopt the platform that
     "the user must just remove the .keep file".

So, honoring .keep files only with --incremental (which is a side effect) or
 --local seems to be the most appropriate.

Comments welcome.

-brandon

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

drafnel@gmail.com wrote:

This looks sane to me, but I'm holding off on the ack because I don't like the way the flags are managed in struct packed_git. (See my reply to 1/3 in the series.)

From: Brandon Casey <drafnel@gmail.com>

Loose objects residing in an alternate object database should not be packed when the -l option to repack is used.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 t/t7700-repack.sh |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

From: Brandon Casey <drafnel@gmail.com>

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 cache.h     |    1 +
 sha1_file.c |   19 +++++++++++++------
 2 files changed, 14 insertions(+), 6 deletions(-)

From: Brandon Casey <drafnel@gmail.com>

With this patch, --local means pack only local objects that are not already packed.

Additionally, this fixes t7700 testing whether loose objects in an alternate object database are repacked.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 Documentation/git-pack-objects.txt |    2 +-
 builtin-pack-objects.c             |    3 +++
 t/t7700-repack.sh                  |    2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)