[PATCH 0/2] Make git-gc more robust with regard to grafts

[PATCH 0/2] Make git-gc more robust with regard to grafts

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 819 bytes --]

More and more Git newbies are taught the wonders of Git grafts.  But that 
is dangerous if the exercise is not to use filter-branch right away.

So let's make git-repack (and thereby, git-gc) aware of grafts and avoid 
culling the parent commits hidden by grafts.

Björn Steinbrink (1):
  Add a test showing that 'git repack' throws away grafted-away parents

Johannes Schindelin (1):
  git repack: keep commits hidden by a graft

 Documentation/git-pack-objects.txt |    7 ++++++-
 builtin-pack-objects.c             |    4 ++++
 cache.h                            |    2 ++
 commit.c                           |    2 +-
 environment.c                      |    1 +
 git-repack.sh                      |    2 +-
 t/t7700-repack.sh                  |   12 ++++++++++++
 7 files changed, 27 insertions(+), 3 deletions(-)

[PATCH 1/2] Add a test showing that 'git repack' throws away grafted-away parents

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Bjoern said that the reflogs are not thrown away if there is no 
	"sleep 1" (which I turned into "test-chmtime -1 .git/logs/HEAD
	.git/logs/refs/heads/master" only to delete it as things worked 
	here without it).

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

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 87c9b0e..a4dddb7 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -149,5 +149,17 @@ test_expect_success 'local packed unreachable obs that exist in alternate ODB ar
 	test_must_fail git show $csha1
 '
 
+test_expect_failure 'objects made unreachable by grafts only are kept' '
+	test_tick &&
+	git commit --allow-empty -m "commit 4" &&
+	H0=$(git rev-parse HEAD) &&
+	H1=$(git rev-parse HEAD^) &&
+	H2=$(git rev-parse HEAD^^) &&
+	echo "$H0 $H2" > .git/info/grafts &&
+	git reflog expire --expire=now --expire-unreachable=now --all &&
+	git repack -a -d &&
+	git cat-file -t $H1
+	'
+
 test_done
 
-- 
1.6.4.rc1.289.gf87df

[PATCH 2/2] git repack: keep commits hidden by a graft

[Johannes Schindelin]

When you have grafts that pretend that a given commit has different
parents than the ones recorded in the commit object, it is dangerous
to let 'git repack' remove those hidden parents, as you can easily
remove the graft and end up with a broken repository.

So let's play it safe and keep those parent objects and everything
that is reachable by them.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Maybe we should not even bother documenting this option?

 Documentation/git-pack-objects.txt |    7 ++++++-
 builtin-pack-objects.c             |    4 ++++
 cache.h                            |    2 ++
 commit.c                           |    2 +-
 environment.c                      |    1 +
 git-repack.sh                      |    2 +-
 t/t7700-repack.sh                  |    2 +-
 7 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt
index 7d4c1a7..06390e3 100644
--- a/Documentation/git-pack-objects.txt
+++ b/Documentation/git-pack-objects.txt
@@ -11,7 +11,8 @@ SYNOPSIS
 [verse]
 'git pack-objects' [-q] [--no-reuse-delta] [--delta-base-offset] [--non-empty]
 	[--local] [--incremental] [--window=N] [--depth=N] [--all-progress]
-	[--revs [--unpacked | --all]*] [--stdout | base-name] < object-list
+	[--revs [--unpacked | --all]*] [--stdout | base-name]
+	[--grafts-replace-parents=no] < object-list
 
 
 DESCRIPTION
@@ -197,6 +198,10 @@ base-name::
 	to force the version for the generated pack index, and to force
 	64-bit index entries on objects located above the given offset.
 
+--grafts-replace-parents=no::
+	With this option, parents that are hidden by grafts are packed
+	nevertheless.
+
 
 Author
 ------
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 1689bd1..77bff85 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -2257,6 +2257,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 				die("bad %s", arg);
 			continue;
 		}
+		if (!strcmp(arg, "--grafts-replace-parents=no")) {
+			grafts_replace_parents = 0;
+			continue;
+		}
 		usage(pack_usage);
 	}
 
diff --git a/cache.h b/cache.h
index dbe460c..1a2a3c9 100644
--- a/cache.h
+++ b/cache.h
@@ -566,6 +566,8 @@ enum object_creation_mode {
 
 extern enum object_creation_mode object_creation_mode;
 
+extern int grafts_replace_parents;
+
 #define GIT_REPO_VERSION 0
 extern int repository_format_version;
 extern int check_repository_format(void);
diff --git a/commit.c b/commit.c
index a47fb4d..ef8e911 100644
--- a/commit.c
+++ b/commit.c
@@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)
 		    bufptr[47] != '\n')
 			return error("bad parents in commit %s", sha1_to_hex(item->object.sha1));
 		bufptr += 48;
-		if (graft)
+		if (graft && (graft->nr_parent < 0 || grafts_replace_parents))
 			continue;
 		new_parent = lookup_commit(parent);
 		if (new_parent)
diff --git a/environment.c b/environment.c
index 95aa8a6..d478125 100644
--- a/environment.c
+++ b/environment.c
@@ -51,6 +51,7 @@ enum push_default_type push_default = PUSH_DEFAULT_MATCHING;
 #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS
 #endif
 enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
+int grafts_replace_parents = 1;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
diff --git a/git-repack.sh b/git-repack.sh
index 1bf2394..1bafba8 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -81,7 +81,7 @@ case ",$all_into_one," in
 esac
 
 args="$args $local ${GIT_QUIET:+-q} $no_reuse$extra"
-names=$(git pack-objects --honor-pack-keep --non-empty --all --reflog $args </dev/null "$PACKTMP") ||
+names=$(git pack-objects --grafts-replace-parents=no --honor-pack-keep --non-empty --all --reflog $args </dev/null "$PACKTMP") ||
 	exit 1
 if [ -z "$names" ]; then
 	say Nothing new to pack.
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index a4dddb7..f4aa054 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -149,7 +149,7 @@ test_expect_success 'local packed unreachable obs that exist in alternate ODB ar
 	test_must_fail git show $csha1
 '
 
-test_expect_failure 'objects made unreachable by grafts only are kept' '
+test_expect_success 'objects made unreachable by grafts only are kept' '
 	test_tick &&
 	git commit --allow-empty -m "commit 4" &&
 	H0=$(git rev-parse HEAD) &&
-- 
1.6.4.rc1.289.gf87df

Re: [PATCH 1/2] Add a test showing that 'git repack' throws away grafted-away parents

[Björn Steinbrink]

On 2009.07.23 17:33:45 +0200, Johannes Schindelin wrote:
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>

> ---
> 
> 	Bjoern said that the reflogs are not thrown away if there is no 
> 	"sleep 1" (which I turned into "test-chmtime -1 .git/logs/HEAD
> 	.git/logs/refs/heads/master" only to delete it as things worked 
> 	here without it).

My original version of the test lacked the test_tick, with it, the test
works without "sleep 1" for me, too.

Björn

Re: [PATCH 2/2] git repack: keep commits hidden by a graft

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> When you have grafts that pretend that a given commit has different
> parents than the ones recorded in the commit object, it is dangerous
> to let 'git repack' remove those hidden parents, as you can easily
> remove the graft and end up with a broken repository.
>
> So let's play it safe and keep those parent objects and everything
> that is reachable by them.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>
> 	Maybe we should not even bother documenting this option?

Why isn't it "--[no-]grafts-replace-parents"?

"--grafts-replace-parents=no" tempts users to say =never or =false, and
also suggests we might add settings with other semantics later, but I do
not think of any offhand.

As far as I can tell, what the updated code does when this option is in
effect is not "do not replace parents", but is "use both real parents and
grafted parents", which is the right thing to do (if we said "use only
the true parents", then parents reachable only through grafts will be lost
after repacking, which is not what we want).

It would probably make more sense to call it "--keep-real-parents"
or something.

Do we want to cull duplicated parents?  I do not think it matters in this
particular context of checking the reachability, but I am just double
checking if you thought about the issue.

Re: [PATCH 2/2] git repack: keep commits hidden by a graft

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> diff --git a/commit.c b/commit.c
> index a47fb4d..ef8e911 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)
>  		    bufptr[47] != '\n')
>  			return error("bad parents in commit %s", sha1_to_hex(item->object.sha1));
>  		bufptr += 48;
> -		if (graft)
> +		if (graft && (graft->nr_parent < 0 || grafts_replace_parents))
>  			continue;

Hmm, what is this "if it is negative" check for?  I did not see it
mentioned in the proposed commit log message.

Re: [PATCH 2/2] git repack: keep commits hidden by a graft

[Björn Steinbrink]

On 2009.07.23 22:13:43 -0700, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > diff --git a/commit.c b/commit.c
> > index a47fb4d..ef8e911 100644
> > --- a/commit.c
> > +++ b/commit.c
> > @@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)
> >  		    bufptr[47] != '\n')
> >  			return error("bad parents in commit %s", sha1_to_hex(item->object.sha1));
> >  		bufptr += 48;
> > -		if (graft)
> > +		if (graft && (graft->nr_parent < 0 || grafts_replace_parents))
> >  			continue;
> 
> Hmm, what is this "if it is negative" check for?  I did not see it
> mentioned in the proposed commit log message.

That's for the special grafts for shallow clones. They override the
other grafts, and are identified by the negative nr_parent value. Those
have to stay in effect as the original parents aren't present in the
repo.

Björn

Re: [PATCH 2/2] git repack: keep commits hidden by a graft

[Johannes Schindelin]

Hi,

On Thu, 23 Jul 2009, Junio C Hamano wrote:

> It would probably make more sense to call it "--keep-real-parents" or 
> something.

Much better.

> Do we want to cull duplicated parents?  I do not think it matters in 
> this particular context of checking the reachability, but I am just 
> double checking if you thought about the issue.

I thought about it, and decided it is not worth the hassle: originally I 
had the option in setup_revisions() until I realized that this is never 
called by pack-objects.  So how about this addition to the commit message:

	As this behavior can only be triggered by git pack-objects, and as 
	that command handles duplicate parents gracefully, we do not 
	bother to cull such parents.

Ciao,
Dscho

Re: [PATCH 2/2] git repack: keep commits hidden by a graft

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1178 bytes --]

Hi,

On Fri, 24 Jul 2009, Björn Steinbrink wrote:

> On 2009.07.23 22:13:43 -0700, Junio C Hamano wrote:
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> > 
> > > diff --git a/commit.c b/commit.c
> > > index a47fb4d..ef8e911 100644
> > > --- a/commit.c
> > > +++ b/commit.c
> > > @@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)
> > >  		    bufptr[47] != '\n')
> > >  			return error("bad parents in commit %s", sha1_to_hex(item->object.sha1));
> > >  		bufptr += 48;
> > > -		if (graft)
> > > +		if (graft && (graft->nr_parent < 0 || grafts_replace_parents))
> > >  			continue;
> > 
> > Hmm, what is this "if it is negative" check for?  I did not see it
> > mentioned in the proposed commit log message.
> 
> That's for the special grafts for shallow clones. They override the 
> other grafts, and are identified by the negative nr_parent value. Those 
> have to stay in effect as the original parents aren't present in the 
> repo.

Right.  How about a comment above the if():

		/*
		 * If nr_parent is negative, the commit is shallow, and
		 * we must not traverse its real parents.
		 */

Hmm?

Ciao,
Dscho

Re: [PATCH 2/2] git repack: keep commits hidden by a graft

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Right.  How about a comment above the if():
>
> 		/*
> 		 * If nr_parent is negative, the commit is shallow, and
> 		 * we must not traverse its real parents.
> 		 */

That's much better.