[PATCH] Updating documentation to match Brandon Casey's proposed git-repack patch.

[PATCH] Updating documentation to match Brandon Casey's proposed git-repack patch.

[Chris Frey]

I haven't heard any negative commentary on this patch, and I see
the repack patch is in next, so resending in case it was missed.

This patch clarifies the git-prune man page, documenting that it only
prunes unpacked objects.  git-repack is documented according to
the new git-repack -A behaviour, which does not depend on
git-pack-objects --keep-unreachable anymore.

Signed-off-by: Chris Frey <cdfrey@foursquare.net>
---
 Documentation/git-prune.txt  |    5 ++++-
 Documentation/git-repack.txt |   14 +++++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-prune.txt b/Documentation/git-prune.txt
index f92bb8c..3178bc4 100644
--- a/Documentation/git-prune.txt
+++ b/Documentation/git-prune.txt
@@ -18,12 +18,15 @@ git-prune. See the section "NOTES", below.
 
 This runs `git-fsck --unreachable` using all the refs
 available in `$GIT_DIR/refs`, optionally with additional set of
-objects specified on the command line, and prunes all
+objects specified on the command line, and prunes all unpacked
 objects unreachable from any of these head objects from the object database.
 In addition, it
 prunes the unpacked objects that are also found in packs by
 running `git prune-packed`.
 
+Note that unreachable, packed objects will remain.  If this is
+not desired, see linkgit:git-repack[1].
+
 OPTIONS
 -------
 
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 3d95749..906d3c7 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -8,7 +8,7 @@ git-repack - Pack unpacked objects in a repository
 
 SYNOPSIS
 --------
-'git-repack' [-a] [-d] [-f] [-l] [-n] [-q] [--window=N] [--depth=N]
+'git-repack' [-a] [-A] [-d] [-f] [-l] [-n] [-q] [--window=N] [--depth=N]
 
 DESCRIPTION
 -----------
@@ -37,6 +37,18 @@ OPTIONS
 	leaves behind, but `git fsck --full` shows as
 	dangling.
 
+-A::
+	Same as `-a`, but any unreachable objects in a previous
+	pack become loose, unpacked objects, instead of being
+	left in the old pack.  Unreachable objects are never
+	intentionally added to a pack, even when repacking.
+	When used with '-d', this option
+	prevents unreachable objects from being immediately
+	deleted by way of being left in the old pack and then
+	removed.  Instead, the loose unreachable objects
+	will be pruned according to normal expiry rules
+	with the next linkgit:git-gc[1].
+
 -d::
 	After packing, if the newly created packs make some
 	existing packs redundant, remove the redundant packs.
-- 
1.5.4.4

Re: [PATCH] Updating documentation to match Brandon Casey's proposed git-repack patch.

[Johannes Schindelin]

Hi,

On Wed, 14 May 2008, Chris Frey wrote:

> I haven't heard any negative commentary on this patch, and I see
> the repack patch is in next, so resending in case it was missed.

This part, together with the subject, is not really meant for application 
as-is, is it?  Please follow the guidelines for SubmittingPatches, and 
write supporting arguments that are not meant to be part of the commit 
messages _after_ the "---".

Ciao,
Dscho

Re: git gc & deleted branches

[Brandon Casey]

Jeff King wrote:
> On Thu, May 08, 2008 at 04:23:53PM -0500, Brandon Casey wrote:
> 
>>> I thought that -A would eventually put them all into a single pack,
>>> killing off the old packs.
>> '-a' puts everything in a single pack and kills off old packs. Anything that
>> was unreachable is not repacked in the new pack.
>>
>> '-A' does the same thing but it also repacks the unreachable objects that were
>> previously packed.
> 
> Ah, indeed. I hadn't looked closely at the -A behavior before. So yes,
> we are never killing off prunable packed objects. Probably we could use
> the same solution as "git prune --expire"; perhaps a
> "--keep-unreachable=2.weeks.ago"?

The 'prune --expire' behavior is based on object mtime (i.e. file modification time).
That is lost once something is packed right?

I was thinking that either repack or pack-objects could be modified to unpack those
unreachable objects and leave them loose, and also give them the timestamp of the
pack file they came from. Then the --expire behavior of git-prune could work normally
and remove them. This seems like it would work nicely since prune follows repack in
git-gc.

-brandon

Re: git gc & deleted branches

[Jeff King]

On Thu, May 08, 2008 at 04:40:20PM -0500, Brandon Casey wrote:

> The 'prune --expire' behavior is based on object mtime (i.e. file
> modification time).  That is lost once something is packed right?

Yes. You would have to use the pack mtime. But of course you would have
to actually _leave_ them in a pack, or they would just keep getting
added to the new pack.

> I was thinking that either repack or pack-objects could be modified to
> unpack those unreachable objects and leave them loose, and also give
> them the timestamp of the pack file they came from. Then the --expire
> behavior of git-prune could work normally and remove them. This seems
> like it would work nicely since prune follows repack in git-gc.

That is sensible, I think.

-Peff

Re: git gc & deleted branches

[Brandon Casey]

Jeff King wrote:
> On Thu, May 08, 2008 at 04:40:20PM -0500, Brandon Casey wrote:
> 
>> The 'prune --expire' behavior is based on object mtime (i.e. file
>> modification time).  That is lost once something is packed right?
> 
> Yes. You would have to use the pack mtime. But of course you would have
> to actually _leave_ them in a pack, or they would just keep getting
> added to the new pack.

I had the impression that unreachable objects would not be packed. Maybe it
was more of an assumption.

-brandon

Re: git gc & deleted branches

[Jeff King]

On Thu, May 08, 2008 at 04:53:20PM -0500, Brandon Casey wrote:

> > Yes. You would have to use the pack mtime. But of course you would have
> > to actually _leave_ them in a pack, or they would just keep getting
> > added to the new pack.
> 
> I had the impression that unreachable objects would not be packed. Maybe it
> was more of an assumption.

Look in builtin-pack-objects.c:1981-1982. We basically just say "if it's
in a pack now, then it should go into the new pack."

-Peff

Re: git gc & deleted branches

[Brandon Casey]

Jeff King <peff <at> peff.net> writes:

> 
> On Thu, May 08, 2008 at 04:53:20PM -0500, Brandon Casey wrote:
> 
> > > Yes. You would have to use the pack mtime. But of course you would have
> > > to actually _leave_ them in a pack, or they would just keep getting
> > > added to the new pack.
> > 
> > I had the impression that unreachable objects would not be packed. Maybe it
> > was more of an assumption.
> 
> Look in builtin-pack-objects.c:1981-1982. We basically just say "if it's
> in a pack now, then it should go into the new pack."
> 
> -Peff
> 


Here's what I was thinking (posted using gmane):

diff --git a/git-repack.sh b/git-repack.sh
index e18eb3f..064c331 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -30,7 +30,7 @@ do
        -n)     no_update_info=t ;;
        -a)     all_into_one=t ;;
        -A)     all_into_one=t
-               keep_unreachable=--keep-unreachable ;;
+               keep_unreachable=t ;;
        -d)     remove_redundant=t ;;
        -q)     quiet=-q ;;
        -f)     no_reuse=--no-reuse-object ;;
@@ -78,9 +78,6 @@ case ",$all_into_one," in
        if test -z "$args"
        then
                args='--unpacked --incremental'
-       elif test -n "$keep_unreachable"
-       then
-               args="$args $keep_unreachable"
        fi
        ;;
 esac
@@ -116,7 +113,15 @@ for name in $names ; do
                echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
                exit 1
        }
-       rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"
+       rm -f "$PACKDIR/old-pack-$name.idx"
+       test -z "$keep_unreachable" ||
+         ! test -f "$PACKDIR/old-pack-$name.pack" ||
+         git unpack-objects < "$PACKDIR/old-pack-$name.pack" || {
+               echo >&2 "Failed unpacking unreachable objects from old pack"
+               echo >&2 "saved as old-pack-$name.pack in $PACKDIR."
+               exit 1
+       }
+       rm -f "$PACKDIR/old-pack-$name.pack"
 done
 
 if test "$remove_redundant" = t
@@ -130,7 +135,18 @@ then
                  do
                        case " $fullbases " in
                        *" $e "*) ;;
-                       *)      rm -f "$e.pack" "$e.idx" "$e.keep" ;;
+                       *)
+                               rm -f "$e.idx" "$e.keep"
+                               if test -n "$keep_unreachable" &&
+                                  test -f "$e.pack"
+                               then
+                                       git unpack-objects < "$e.pack" || {
+                                               echo >&2 "Fail AVOID GMANE WRAP"
+                                               exit 1
+                                       }
+                               fi
+                               rm -f "$e.pack"
+                       ;;
                        esac
                  done
                )


Is the first invocation of unpack-objects necessary? pack-objects has created
a pack which hashes to the same name of a pack we already have, and we replace
the original with the new one. Is that what is happening? They will be identical
right?

Of course this won't set the timestamp on the created objects based on the
timestamp of the pack file, but this was easy. Setting the timestamp would be
proper, but what's another two weeks. Besides, for those users not manually
running git-gc, this code path won't even be executed until there are enough
pack files for git-gc to add -A to the repack options.

Then, for git-gc it should be enough to just always use -A with repack when
manually running it. Then --prune can be deprecated.

-brandon

Re: git gc & deleted branches

[Jeff King]

On Fri, May 09, 2008 at 01:41:30AM +0000, Brandon Casey wrote:

> Here's what I was thinking (posted using gmane):
> 
> diff --git a/git-repack.sh b/git-repack.sh
> index e18eb3f..064c331 100755
> --- a/git-repack.sh
> +++ b/git-repack.sh

I like it. It makes an easy rule to say "packed objects _never_ get
pruned, they only get demoted to loose objects." And then of course
we have sane rules for pruning loose objects.

> -       rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"
> +       rm -f "$PACKDIR/old-pack-$name.idx"
> +       test -z "$keep_unreachable" ||
> +         ! test -f "$PACKDIR/old-pack-$name.pack" ||
> +         git unpack-objects < "$PACKDIR/old-pack-$name.pack" || {
> +               echo >&2 "Failed unpacking unreachable objects from old pack"
> +               echo >&2 "saved as old-pack-$name.pack in $PACKDIR."
> +               exit 1
> +       }
> +       rm -f "$PACKDIR/old-pack-$name.pack"
> [...]
> 
> Is the first invocation of unpack-objects necessary? pack-objects has created
> a pack which hashes to the same name of a pack we already have, and we replace
> the original with the new one. Is that what is happening? They will be
> identical right?

Yeah, that's what it looks like to me (that the first unpack is
unnecessary, because we will just be putting the new pack into place
that has all the same objects). AIUI, two packs with identical hashes
must contain the exact same objects.

> Of course this won't set the timestamp on the created objects based on the
> timestamp of the pack file, but this was easy. Setting the timestamp would be
> proper, but what's another two weeks. Besides, for those users not manually
> running git-gc, this code path won't even be executed until there are enough
> pack files for git-gc to add -A to the repack options.

I think the extra two weeks is fine.

-Peff

Re: git gc & deleted branches

[Geert Bosch]

On May 9, 2008, at 00:19, Jeff King wrote:

> I like it. It makes an easy rule to say "packed objects _never_ get
> pruned, they only get demoted to loose objects." And then of course
> we have sane rules for pruning loose objects.

Isn't there an issue with the "git gc" triggering because there
may be too many loose unreferenced objects?
Still, I do like the approach.

Maybe unreferenced objects and old refs should go to a .git/lost+found
directory and be expired from there. This has a couple of benefits:

   -  Easy to manually inspect or blow away any crud
   -  One git-gc run can make one pack in lost+found,
      avoiding huge numbers of loose objects (and massive disk use)
      when trying to do a large cleanup (to possibly reclaim disk space)
   -  Objects will not be accessible by ordinary git commands for a  
while,
      before they are really removed, avoiding surprises

Only some tools would look in the lost+found to restore stuff.

   -Geert

Re: git gc & deleted branches

[Brandon Casey]

Geert Bosch wrote:
> 
> On May 9, 2008, at 00:19, Jeff King wrote:
> 
>> I like it. It makes an easy rule to say "packed objects _never_ get
>> pruned, they only get demoted to loose objects." And then of course
>> we have sane rules for pruning loose objects.
> 
> Isn't there an issue with the "git gc" triggering because there
> may be too many loose unreferenced objects?
> Still, I do like the approach.

This would be an argument for going the extra mile and having the loose
objects adopt the timestamp of their pack file. In the normal case they
would probably be pruned immediately during the same git-gc run.

> Maybe unreferenced objects and old refs should go to a .git/lost+found
> directory and be expired from there. This has a couple of benefits:

>   -  Objects will not be accessible by ordinary git commands for a while,
>      before they are really removed, avoiding surprises

Unreferenced objects are sometimes used by other repositories which have
this repository listed as an alternate. So it may not be a good idea to
make the unreferenced objects inaccessible.

-brandon

Re: git gc & deleted branches

[Nicolas Pitre]

On Fri, 9 May 2008, Brandon Casey wrote:

> Geert Bosch wrote:
> > 
> > On May 9, 2008, at 00:19, Jeff King wrote:
> > 
> >> I like it. It makes an easy rule to say "packed objects _never_ get
> >> pruned, they only get demoted to loose objects." And then of course
> >> we have sane rules for pruning loose objects.
> > 
> > Isn't there an issue with the "git gc" triggering because there
> > may be too many loose unreferenced objects?
> > Still, I do like the approach.
> 
> This would be an argument for going the extra mile and having the loose
> objects adopt the timestamp of their pack file. In the normal case they
> would probably be pruned immediately during the same git-gc run.

Well, not necessarily.  If you created a large branch yesterday and you 
are deleting it today, then if you repacked in between means that those 
loose objects won't be more than one day old.  Yet there could be enough 
of them to trigger auto gc.  But that auto gc won't pack those objects 
since they are unreferenced.  Hence auto gc will trigger all the time 
without making any progress.

> > Maybe unreferenced objects and old refs should go to a .git/lost+found
> > directory and be expired from there. This has a couple of benefits:
> 
> >   -  Objects will not be accessible by ordinary git commands for a while,
> >      before they are really removed, avoiding surprises
> 
> Unreferenced objects are sometimes used by other repositories which have
> this repository listed as an alternate. So it may not be a good idea to
> make the unreferenced objects inaccessible.

Nah.  If this is really the case then you shouldn't be running gc at all 
in the first place.


Nicolas

Re: git gc & deleted branches

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> On Fri, 9 May 2008, Brandon Casey wrote:
>
>> Unreferenced objects are sometimes used by other repositories which have
>> this repository listed as an alternate. So it may not be a good idea to
>> make the unreferenced objects inaccessible.
>
> Nah.  If this is really the case then you shouldn't be running gc at all 
> in the first place.

True.

I think the true motivation behind --keep-unreachable is not about the
shared object store (aka "alternates") but about races between gc and
push (or fetch).  Before push (or fetch) finishes and updates refs, the
new objects they create would be dangling _and_ the objects these dangling
objects refer to may be packed but unreferenced.  Repacking unreferenced
packed objects was a way to avoid losing them.

[PATCH] Updating documentation to match Brandon Casey's proposed git-repack patch.

[Chris Frey]

On Fri, May 09, 2008 at 03:33:41PM -0700, Junio C Hamano wrote:
> I think the true motivation behind --keep-unreachable is not about the
> shared object store (aka "alternates") but about races between gc and
> push (or fetch).  Before push (or fetch) finishes and updates refs, the
> new objects they create would be dangling _and_ the objects these dangling
> objects refer to may be packed but unreferenced.  Repacking unreferenced
> packed objects was a way to avoid losing them.

This is what the log history seems to indicate:

	git log -p --grep=keep-unreach

So pack-objects --keep-unreachable was implemented in order to add repack -A,
which now doesn't need --keep-unreachable, and can become obsolete.

Which is just as well, since --keep-unreachable never made it to the
man pages. :-)

If I understand things correctly, there is no user-friendly way to add
loose, unreachable objects to a pack.  This whole architecture was just
to prevent a repack from silently deleting things.

If this is right, the patch below updates the docs.

- Chris


>From 443b1201d54f0b7197d18779ce934823e9897b36 Mon Sep 17 00:00:00 2001
From: Chris Frey <cdfrey@foursquare.net>
Date: Fri, 9 May 2008 19:08:26 -0400
Subject: [PATCH] Updating documentation to match Brandon Casey's proposed git-repack patch.

This patch clarifies the git-prune man page, documenting that it only
prunes unpacked objects.  git-repack is documented according to
the new git-repack -A behaviour, which does not depend on
git-pack-objects --keep-unreachable anymore.

Signed-off-by: Chris Frey <cdfrey@foursquare.net>
---
 Documentation/git-prune.txt  |    5 ++++-
 Documentation/git-repack.txt |   14 +++++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-prune.txt b/Documentation/git-prune.txt
index f92bb8c..3178bc4 100644
--- a/Documentation/git-prune.txt
+++ b/Documentation/git-prune.txt
@@ -18,12 +18,15 @@ git-prune. See the section "NOTES", below.
 
 This runs `git-fsck --unreachable` using all the refs
 available in `$GIT_DIR/refs`, optionally with additional set of
-objects specified on the command line, and prunes all
+objects specified on the command line, and prunes all unpacked
 objects unreachable from any of these head objects from the object database.
 In addition, it
 prunes the unpacked objects that are also found in packs by
 running `git prune-packed`.
 
+Note that unreachable, packed objects will remain.  If this is
+not desired, see linkgit:git-repack[1].
+
 OPTIONS
 -------
 
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 3d95749..906d3c7 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -8,7 +8,7 @@ git-repack - Pack unpacked objects in a repository
 
 SYNOPSIS
 --------
-'git-repack' [-a] [-d] [-f] [-l] [-n] [-q] [--window=N] [--depth=N]
+'git-repack' [-a] [-A] [-d] [-f] [-l] [-n] [-q] [--window=N] [--depth=N]
 
 DESCRIPTION
 -----------
@@ -37,6 +37,18 @@ OPTIONS
 	leaves behind, but `git fsck --full` shows as
 	dangling.
 
+-A::
+	Same as `-a`, but any unreachable objects in a previous
+	pack become loose, unpacked objects, instead of being
+	left in the old pack.  Unreachable objects are never
+	intentionally added to a pack, even when repacking.
+	When used with '-d', this option
+	prevents unreachable objects from being immediately
+	deleted by way of being left in the old pack and then
+	removed.  Instead, the loose unreachable objects
+	will be pruned according to normal expiry rules
+	with the next linkgit:git-gc[1].
+
 -d::
 	After packing, if the newly created packs make some
 	existing packs redundant, remove the redundant packs.
-- 
1.5.4.4