{"thread":{"id":"20210","subject":"[PATCH 0/2] Make git-gc more robust with regard to grafts","startedAt":"2009-07-23T15:33:06Z","lastAt":"2009-07-24T15:24:40Z","messageCount":10,"participants":["Johannes Schindelin","Björn Steinbrink","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"118582","messageId":"cover.1248362827u.git.johannes.schindelin@gmx.de","threadId":"20210","inReplyTo":null,"subject":"[PATCH 0/2] Make git-gc more robust with regard to grafts","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-23T15:33:06Z","receivedAt":"2009-07-23T15:33:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"More and more Git newbies are taught the wonders of Git grafts. But that \nis dangerous if the exercise is not to use filter-branch right away.\n\nSo let's make git-repack (and thereby, git-gc) aware of grafts and avoid \nculling the parent commits hidden by grafts.\n\nBjörn Steinbrink (1):\n Add a test showing that 'git repack' throws away grafted-away parents\n\nJohannes Schindelin (1):\n git repack: keep commits hidden by a graft\n\n Documentation/git-pack-objects.txt | 7 ++++++-\n builtin-pack-objects.c | 4 ++++\n cache.h | 2 ++\n commit.c | 2 +-\n environment.c | 1 +\n git-repack.sh | 2 +-\n t/t7700-repack.sh | 12 ++++++++++++\n 7 files changed, 27 insertions(+), 3 deletions(-)"},{"id":"118583","messageId":"alpine.DEB.1.00.0907231733300.8306@pacific.mpi-cbg.de","threadId":"20210","inReplyTo":"cover.1248362827u.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/2] Add a test showing that 'git repack' throws away grafted-away parents","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-23T15:33:45Z","receivedAt":"2009-07-23T15:33:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Signed-off-by: Johannes Schindelin \n---\n\n\tBjoern said that the reflogs are not thrown away if there is no \n\t\"sleep 1\" (which I turned into \"test-chmtime -1 .git/logs/HEAD\n\t.git/logs/refs/heads/master\" only to delete it as things worked \n\there without it).\n\n t/t7700-repack.sh | 12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t7700-repack.sh b/t/t7700-repack.sh\nindex 87c9b0e..a4dddb7 100755\n--- a/t/t7700-repack.sh\n+++ b/t/t7700-repack.sh\n@@ -149,5 +149,17 @@ test_expect_success 'local packed unreachable obs that exist in alternate ODB ar\n \ttest_must_fail git show $csha1\n '\n \n+test_expect_failure 'objects made unreachable by grafts only are kept' '\n+\ttest_tick &&\n+\tgit commit --allow-empty -m \"commit 4\" &&\n+\tH0=$(git rev-parse HEAD) &&\n+\tH1=$(git rev-parse HEAD^) &&\n+\tH2=$(git rev-parse HEAD^^) &&\n+\techo \"$H0 $H2\" > .git/info/grafts &&\n+\tgit reflog expire --expire=now --expire-unreachable=now --all &&\n+\tgit repack -a -d &&\n+\tgit cat-file -t $H1\n+\t'\n+\n test_done\n \n-- \n1.6.4.rc1.289.gf87df\n"},{"id":"118584","messageId":"34dfd22bb99c7c466b6131876e8b52ac46f388aa.1248362827u.git.johannes.schindelin@gmx.de","threadId":"20210","inReplyTo":"cover.1248362827u.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/2] git repack: keep commits hidden by a graft","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-23T15:33:49Z","receivedAt":"2009-07-23T15:33:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"When you have grafts that pretend that a given commit has different\nparents than the ones recorded in the commit object, it is dangerous\nto let 'git repack' remove those hidden parents, as you can easily\nremove the graft and end up with a broken repository.\n\nSo let's play it safe and keep those parent objects and everything\nthat is reachable by them.\n\nSigned-off-by: Johannes Schindelin \n---\n\n\tMaybe we should not even bother documenting this option?\n\n Documentation/git-pack-objects.txt | 7 ++++++-\n builtin-pack-objects.c | 4 ++++\n cache.h | 2 ++\n commit.c | 2 +-\n environment.c | 1 +\n git-repack.sh | 2 +-\n t/t7700-repack.sh | 2 +-\n 7 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex 7d4c1a7..06390e3 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -11,7 +11,8 @@ SYNOPSIS\n [verse]\n 'git pack-objects' [-q] [--no-reuse-delta] [--delta-base-offset] [--non-empty]\n \t[--local] [--incremental] [--window=N] [--depth=N] [--all-progress]\n-\t[--revs [--unpacked | --all]*] [--stdout | base-name] < object-list\n+\t[--revs [--unpacked | --all]*] [--stdout | base-name]\n+\t[--grafts-replace-parents=no] < object-list\n \n \n DESCRIPTION\n@@ -197,6 +198,10 @@ base-name::\n \tto force the version for the generated pack index, and to force\n \t64-bit index entries on objects located above the given offset.\n \n+--grafts-replace-parents=no::\n+\tWith this option, parents that are hidden by grafts are packed\n+\tnevertheless.\n+\n \n Author\n ------\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 1689bd1..77bff85 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -2257,6 +2257,10 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\tdie(\"bad %s\", arg);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(arg, \"--grafts-replace-parents=no\")) {\n+\t\t\tgrafts_replace_parents = 0;\n+\t\t\tcontinue;\n+\t\t}\n \t\tusage(pack_usage);\n \t}\n \ndiff --git a/cache.h b/cache.h\nindex dbe460c..1a2a3c9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -566,6 +566,8 @@ enum object_creation_mode {\n \n extern enum object_creation_mode object_creation_mode;\n \n+extern int grafts_replace_parents;\n+\n #define GIT_REPO_VERSION 0\n extern int repository_format_version;\n extern int check_repository_format(void);\ndiff --git a/commit.c b/commit.c\nindex a47fb4d..ef8e911 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n \t\t bufptr[47] != '\\n')\n \t\t\treturn error(\"bad parents in commit %s\", sha1_to_hex(item->object.sha1));\n \t\tbufptr += 48;\n-\t\tif (graft)\n+\t\tif (graft && (graft->nr_parent < 0 || grafts_replace_parents))\n \t\t\tcontinue;\n \t\tnew_parent = lookup_commit(parent);\n \t\tif (new_parent)\ndiff --git a/environment.c b/environment.c\nindex 95aa8a6..d478125 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -51,6 +51,7 @@ enum push_default_type push_default = PUSH_DEFAULT_MATCHING;\n #define OBJECT_CREATION_MODE OBJECT_CREATION_USES_HARDLINKS\n #endif\n enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;\n+int grafts_replace_parents = 1;\n \n /* Parallel index stat data preload? */\n int core_preload_index = 0;\ndiff --git a/git-repack.sh b/git-repack.sh\nindex 1bf2394..1bafba8 100755\n--- a/git-repack.sh\n+++ b/git-repack.sh\n@@ -81,7 +81,7 @@ case \",$all_into_one,\" in\n esac\n \n args=\"$args $local ${GIT_QUIET:+-q} $no_reuse$extra\"\n-names=$(git pack-objects --honor-pack-keep --non-empty --all --reflog $args Signed-off-by: Johannes Schindelin \n\nSigned-off-by: Björn Steinbrink \n\n> ---\n> \n> \tBjoern said that the reflogs are not thrown away if there is no \n> \t\"sleep 1\" (which I turned into \"test-chmtime -1 .git/logs/HEAD\n> \t.git/logs/refs/heads/master\" only to delete it as things worked \n> \there without it).\n\nMy original version of the test lacked the test_tick, with it, the test\nworks without \"sleep 1\" for me, too.\n\nBjörn\n"},{"id":"118609","messageId":"7veis6vfh5.fsf@alter.siamese.dyndns.org","threadId":"20210","inReplyTo":"34dfd22bb99c7c466b6131876e8b52ac46f388aa.1248362827u.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/2] git repack: keep commits hidden by a graft","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-24T05:04:38Z","receivedAt":"2009-07-24T05:04:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> When you have grafts that pretend that a given commit has different\n> parents than the ones recorded in the commit object, it is dangerous\n> to let 'git repack' remove those hidden parents, as you can easily\n> remove the graft and end up with a broken repository.\n>\n> So let's play it safe and keep those parent objects and everything\n> that is reachable by them.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n>\n> \tMaybe we should not even bother documenting this option?\n\nWhy isn't it \"--[no-]grafts-replace-parents\"?\n\n\"--grafts-replace-parents=no\" tempts users to say =never or =false, and\nalso suggests we might add settings with other semantics later, but I do\nnot think of any offhand.\n\nAs far as I can tell, what the updated code does when this option is in\neffect is not \"do not replace parents\", but is \"use both real parents and\ngrafted parents\", which is the right thing to do (if we said \"use only\nthe true parents\", then parents reachable only through grafts will be lost\nafter repacking, which is not what we want).\n\nIt would probably make more sense to call it \"--keep-real-parents\"\nor something.\n\nDo we want to cull duplicated parents? I do not think it matters in this\nparticular context of checking the reachability, but I am just double\nchecking if you thought about the issue.\n"},{"id":"118610","messageId":"7v8wievf20.fsf@alter.siamese.dyndns.org","threadId":"20210","inReplyTo":"34dfd22bb99c7c466b6131876e8b52ac46f388aa.1248362827u.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 2/2] git repack: keep commits hidden by a graft","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-24T05:13:43Z","receivedAt":"2009-07-24T05:13:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> diff --git a/commit.c b/commit.c\n> index a47fb4d..ef8e911 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n> \t\t bufptr[47] != '\\n')\n> \t\t\treturn error(\"bad parents in commit %s\", sha1_to_hex(item->object.sha1));\n> \t\tbufptr += 48;\n> -\t\tif (graft)\n> +\t\tif (graft && (graft->nr_parent < 0 || grafts_replace_parents))\n> \t\t\tcontinue;\n\nHmm, what is this \"if it is negative\" check for? I did not see it\nmentioned in the proposed commit log message.\n"},{"id":"118612","messageId":"20090724053506.GB12855@atjola.homenet","threadId":"20210","inReplyTo":"7v8wievf20.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git repack: keep commits hidden by a graft","fromName":"Björn Steinbrink","fromEmail":"b.steinbrink@gmx.de","sentAt":"2009-07-24T05:35:06Z","receivedAt":"2009-07-24T05:35:06Z","isPatch":true,"sender":{"key":"b.steinbrink@gmx.de","avatar":"https://avatars.githubusercontent.com/u/230962?v=4"},"body":"On 2009.07.23 22:13:43 -0700, Junio C Hamano wrote:\n> Johannes Schindelin writes:\n> \n> > diff --git a/commit.c b/commit.c\n> > index a47fb4d..ef8e911 100644\n> > --- a/commit.c\n> > +++ b/commit.c\n> > @@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n> > \t\t bufptr[47] != '\\n')\n> > \t\t\treturn error(\"bad parents in commit %s\", sha1_to_hex(item->object.sha1));\n> > \t\tbufptr += 48;\n> > -\t\tif (graft)\n> > +\t\tif (graft && (graft->nr_parent < 0 || grafts_replace_parents))\n> > \t\t\tcontinue;\n> \n> Hmm, what is this \"if it is negative\" check for? I did not see it\n> mentioned in the proposed commit log message.\n\nThat's for the special grafts for shallow clones. They override the\nother grafts, and are identified by the negative nr_parent value. Those\nhave to stay in effect as the original parents aren't present in the\nrepo.\n\nBjörn\n"},{"id":"118620","messageId":"alpine.DEB.1.00.0907241133080.19988@intel-tinevez-2-302","threadId":"20210","inReplyTo":"7veis6vfh5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] git repack: keep commits hidden by a graft","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-24T09:36:00Z","receivedAt":"2009-07-24T09:36:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 23 Jul 2009, Junio C Hamano wrote:\n\n> It would probably make more sense to call it \"--keep-real-parents\" or \n> something.\n\nMuch better.\n\n> Do we want to cull duplicated parents? I do not think it matters in \n> this particular context of checking the reachability, but I am just \n> double checking if you thought about the issue.\n\nI thought about it, and decided it is not worth the hassle: originally I \nhad the option in setup_revisions() until I realized that this is never \ncalled by pack-objects. So how about this addition to the commit message:\n\n\tAs this behavior can only be triggered by git pack-objects, and as \n\tthat command handles duplicate parents gracefully, we do not \n\tbother to cull such parents.\n\nCiao,\nDscho\n"},{"id":"118621","messageId":"alpine.DEB.1.00.0907241136081.19988@intel-tinevez-2-302","threadId":"20210","inReplyTo":"20090724053506.GB12855@atjola.homenet","subject":"Re: [PATCH 2/2] git repack: keep commits hidden by a graft","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-07-24T09:37:38Z","receivedAt":"2009-07-24T09:37:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 24 Jul 2009, Björn Steinbrink wrote:\n\n> On 2009.07.23 22:13:43 -0700, Junio C Hamano wrote:\n> > Johannes Schindelin writes:\n> > \n> > > diff --git a/commit.c b/commit.c\n> > > index a47fb4d..ef8e911 100644\n> > > --- a/commit.c\n> > > +++ b/commit.c\n> > > @@ -262,7 +262,7 @@ int parse_commit_buffer(struct commit *item, void *buffer, unsigned long size)\n> > > \t\t bufptr[47] != '\\n')\n> > > \t\t\treturn error(\"bad parents in commit %s\", sha1_to_hex(item->object.sha1));\n> > > \t\tbufptr += 48;\n> > > -\t\tif (graft)\n> > > +\t\tif (graft && (graft->nr_parent < 0 || grafts_replace_parents))\n> > > \t\t\tcontinue;\n> > \n> > Hmm, what is this \"if it is negative\" check for? I did not see it\n> > mentioned in the proposed commit log message.\n> \n> That's for the special grafts for shallow clones. They override the \n> other grafts, and are identified by the negative nr_parent value. Those \n> have to stay in effect as the original parents aren't present in the \n> repo.\n\nRight. How about a comment above the if():\n\n\t\t/*\n\t\t * If nr_parent is negative, the commit is shallow, and\n\t\t * we must not traverse its real parents.\n\t\t */\n\nHmm?\n\nCiao,\nDscho\n"},{"id":"118631","messageId":"7vmy6uqf2f.fsf@alter.siamese.dyndns.org","threadId":"20210","inReplyTo":"alpine.DEB.1.00.0907241136081.19988@intel-tinevez-2-302","subject":"Re: [PATCH 2/2] git repack: keep commits hidden by a graft","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-07-24T15:24:40Z","receivedAt":"2009-07-24T15:24:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Right. How about a comment above the if():\n>\n> \t\t/*\n> \t\t * If nr_parent is negative, the commit is shallow, and\n> \t\t * we must not traverse its real parents.\n> \t\t */\n\nThat's much better.\n"}]}