threads / discuss / 6535

Seriously broken "git pack-refs"

Subject: Seriously broken "git pack-refs"

## tl;dr

6 messages between Jan 26, 2007 and Jan 26, 2007.

replies: 5people: 2as markdown or json

Linus Torvalds· Jan 26, 2007, 00:51 UTC · lore

Do *NOT* try this on a repository you care about:

	git pack-refs --all --prune
	git pack-refs

because while the first "pack-refs" does the right thing, the second pack-refs will totally screw you over.

Why?

The default for "git pack-refs" is to pack just tags. Which is a HORRIBLE MISTAKE. Becuase it means that if you actually had any packed non-tags, they now get removed entirely.

I'd call whoever made that decision a complete crack-addict and total idiot, but it might be me, so I'll just tread carefully and call the choice "interesting".

This should fix it.

		Linus
----

diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
index 6de7128..3de9b3e 100644
--- a/builtin-pack-refs.c
+++ b/builtin-pack-refs.c
@@ -37,7 +37,9 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
 	if ((flags & REF_ISSYMREF))
 		return 0;
 	is_tag_ref = !strncmp(path, "refs/tags/", 10);
-	if (!cb->all && !is_tag_ref)
+
+	/* ALWAYS pack refs that were already packed or are tags */
+	if (!cb->all && !is_tag_ref && !(flags & REF_ISPACKED))
 		return 0;
 
 	fprintf(cb->refs_file, "%s %s\n", sha1_to_hex(sha1), path);

Junio C Hamano· Jan 26, 2007, 03:22 UTC · re: Linus Torvalds · lore

Re: Seriously broken "git pack-refs"

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 7 quoted lines

> The default for "git pack-refs" is to pack just tags. Which is a HORRIBLE 
> MISTAKE. Becuase it means that if you actually had any packed non-tags, 
> they now get removed entirely.
>
> I'd call whoever made that decision a complete crack-addict and total 
> idiot, but it might be me, so I'll just tread carefully and call the 
> choice "interesting".

Guilty as charged, I guess (I haven't asked 'blame' about it but I am reasonably sure it was me). And thanks for catching it.

But I think "only packing tags" is a reasonable default, as local branch heads are meant to be advanced, and remote tracking branches follow whatever happens on the remote end upon every fetch. Tags on the other hand are meant to be stationary.

What was a HORRIBLE MISTAKE was not repacking what came out of the pack if it was pruned.

Linus Torvalds· Jan 26, 2007, 06:05 UTC · re: Junio C Hamano · lore

Re: Seriously broken "git pack-refs"

On Thu, 25 Jan 2007, Junio C Hamano wrote:

> 
> But I think "only packing tags" is a reasonable default

Sure, I agree. I just think that it had the unintended side effect when the code then didn't repack stuff that had already been packed earlier.

Which is why my fix doesn't actually change the fact that we only pack tags by default, but just fixes the thing that caused us to then totally drop previously packed non-tags.

So now the default isn't really "pack only tags", but "pack only tags *or* things that were already packed".

The reason, of course, is that if we don't pack stuff that used to be packed, it will just be dropped entirely. Oops.

		Linus

Junio C Hamano· Jan 26, 2007, 06:08 UTC · re: Linus Torvalds · lore

Re: Seriously broken "git pack-refs"

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 5 quoted lines

> So now the default isn't really "pack only tags", but "pack only tags *or* 
> things that were already packed".
>
> The reason, of course, is that if we don't pack stuff that used to be 
> packed, it will just be dropped entirely. Oops.

Sorry again.

On the other hand, I do not think it is worth unpacking non-tags that are packed when --all is not given, so...

Linus Torvalds· Jan 26, 2007, 06:24 UTC · re: Junio C Hamano · lore

Re: Seriously broken "git pack-refs"

On Thu, 25 Jan 2007, Junio C Hamano wrote:

> 
> On the other hand, I do not think it is worth unpacking non-tags
> that are packed when --all is not given, so...

Exactly.

I'd much rather have

	git pack-refs --all --prune

just pack everything once and for all. Afterwards, if you do just

	git pack-refs

it will keep everything that was packed packed, and any loose refs (which might be loose because they are new, but perhaps because they simply got overridden by being changed) will not be added to the pack unless they are tags.

Which should be the semantics with my patch applied.

The above actually makes tons of sense: operations like big imports (or clones) might want to start out with *everything* packed, but then as we update, commit and modify, what we actually want to do is to basically let the branches that are actively developed "become unpacked" as they are updated.

So the "repack refs that were already packed, or refs that are tags" is actually a very sane default. It's not just that we don't want to lose the refs entirely, it's also that what we actually want to do by default is to pack the stuff we have reason to believe won't be actively changing.

Tags automatically fall under that heading (which is why it makes sense to pack them by default in the first place!) but so does any ref that was already packed, and hadn't become unpacked since the last repack. By definition, such a ref isn't "actively changing" even if it isn't a tag.

No?

		Linus

Junio C Hamano· Jan 26, 2007, 06:50 UTC · re: Linus Torvalds · lore

Re: Seriously broken "git pack-refs"

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 6 quoted lines

> Tags automatically fall under that heading (which is why it makes sense to 
> pack them by default in the first place!) but so does any ref that was 
> already packed, and hadn't become unpacked since the last repack. By 
> definition, such a ref isn't "actively changing" even if it isn't a tag.
>
> No?

Yes.

diff --git a/Documentation/git-pack-refs.txt b/Documentation/git-pack-refs.txt
index 464269f..b79b79d 100644
--- a/Documentation/git-pack-refs.txt
+++ b/Documentation/git-pack-refs.txt
@@ -34,11 +34,16 @@ OPTIONS
 
 \--all::
 
-The command by default packs all tags and leaves branch tips
+The command by default packs all tags and refs that are already
+packed, and leaves other refs
 alone.  This is because branches are expected to be actively
 developed and packing their tips does not help performance.
 This option causes branch tips to be packed as well.  Useful for
 a repository with many branches of historical interests.
+Once packing with `\--all` is done, further repacking with the
+default will keep the already packed refs (including branch
+heads) packed, while still actively developed branch heads will
+not be packed (because they become unpacked once modified).
 
 \--no-prune::