Re: fetching packs and storing them as packs

From: Shawn Pearce <spearce@spearce.org>
Date: Oct 29, 2006, 03:50 UTC
Message-ID: <20061029035025.GC3435@spearce.org>
In-Reply-To: <7vfyd88d6s.fsf@assigned-by-dhcp.cox.net>

Junio C Hamano <junkio@cox.net> wrote:

Show 21 quoted lines

> Shawn Pearce <spearce@spearce.org> writes:
> 
> > Shawn Pearce <spearce@spearce.org> wrote:
> >> Why not just use create a new flag file?
> >> 
> >> Lets say that a pack X is NOT eligible to be repacked if
> >> "$GIT_DIR/objects/pack/pack-X.keep" exists.
> >
> > Here's the `git repack -a -d` portion of that.
> > Thoughts?
> 
> > +	args=--unpacked
> > +	active=
> > +	if test -d "$PACKDIR"
> > +	then
> > +		for p in `find "$PACKDIR" -type f -name '*.pack' -print`
> 
> This change to run 'find "$PACKDIR"' is fragile when your
> $GIT_OBJECT_DIRECTORY has $IFS in it; running "find ." after
> "cd" in a subprocess was done very much on purpose to avoid that
> issue.  Please don't break it.

I only broke it because you backed me into a corner with --unpacked= :-)

The issue is --unpacked= uses the path of the pack name, which includes $GIT_OBJECT_DIRECTORY, whatever that may be. This makes it impossible for the shell script to hand through a proper --unpacked= line for the active packs without including $GIT_OBJECT_DIRECTORY as part of the option.

I agree with you about the $IFS issue. I'll redraft this patch tonight such that $IFS doesn't get broken here but that's going to take a small code patch over in revisions.c, which I'll also do tonight.

> > +	if test "X$args" = X--unpacked
> > +	then
> > +		args='--unpacked --incremental'
> > +	fi

> I do not remember offhand what --incremental meant, but
> presumably this is for the very initial "repack" (PACKDIR did
> not exist or find loop did not find anything to repack) and the
> flag would not make a difference?  Care to explain?

 
I think there is a bug in pack-objects but I couldn't find it last
night.  Using --incremental worked around it.  :-)

According to the documentation:

  --unpacked tells pack-objects to only pack loose objects.

  --incremental tells pack-objects to skip any object that is
  already contained in a pack even if it appears in the input list.

What I really wanted here was to just use '--unpacked'; if there are no active packs and the user asked for '-a' we actually just want to pack the loose objects into a new active pack as we aren't allowed to touch any of the existing packs (they are all kept or there simply aren't any packs yet).

However on the git.git repository if I ran `git repack -a -d` with every single object in a kept pack and no loose objects I kept repacking the same 102 objects into a new active pack, even though there were no loose objects to repack and no active packs. Uh, yea.

Adding --incremental in this case kept it from repacking those same 102 objects on every invocation. Yea, its a bug. I meant to mention it in my email.